Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prevent generating duplicate exports for closure conversions #4380

Merged
merged 1 commit into from
Dec 25, 2024

Conversation

daxpedda
Copy link
Collaborator

@daxpedda daxpedda commented Dec 25, 2024

We have a report in #4371 where wasm-bindgen would generate multiple exported functions with the same name, which is invalid. Specifically these functions were Closure converters.

I was debugging this just with the generated Wasm module, because I couldn't build the project locally and there was no minimal re-production, the following is just speculation:

The culprit seems to be:

wasm-bindgen/src/closure.rs

Lines 340 to 344 in 88452fa

#[inline(never)]
#[cfg_attr(wasm_bindgen_unstable_test_coverage, coverage(off))]
unsafe fn breaks_if_inlined<T: WasmClosure + ?Sized>(a: usize, b: usize) -> u32 {
super::__wbindgen_describe_closure(a as u32, b as u32, describe::<T> as usize as u32)
}

For some reason, while we are interpreting this descriptor, we find multiple ones with exactly the same name, e.g.: <dyn core[bd1a764364e91139]::ops::function::FnMut<(), Output = _> as wasm_bindgen[79ff92f6c8c42bdb]::closure::WasmClosure>::describe::invoke::<()>. While finding multiple ones should not be surprising, as we are dealing with a generic, the name shouldn't be the same because as you can see Rust adds some naming scheme to prevent that. So either this is a bug in Rust, which I very much doubt, or the problem is #[inline(never)], which would not be bug in Rust because the reference clearly states that it is just a suggestion and not a guarantee.

To fix this we simply apply a number at the end to mangle the name further.

Fixes #4371.
Cc @dpc.

@dpc
Copy link

dpc commented Dec 25, 2024

I have verified that the version from PR passes wasm-opt problem I originally reported. Will report if find any issues later down the line, but so far LGTM. Thank you so much for help and fix!

@daxpedda daxpedda merged commit 24f20ae into rustwasm:main Dec 25, 2024
67 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

wasm-opt fails due to duplicated symbol
2 participants