Skip to content

Hash full signature for imported functions #4269

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

Merged
merged 3 commits into from
Nov 14, 2024

Conversation

RunDevelopment
Copy link
Contributor

Fixes #4268

The problem in #4268 was the short hash added to disambiguate imported functions only hashed the Rust identifier of the function. This is enough to ensure uniqueness within a single Rust module, but not across modules as in #4268. This resulted in 2 different wasm functions being exported under the same name.

I fixed this by hashing not just the function name, but the whole signature (name + inputs + outputs + some extra). This means that 2 function will only have the same hash if they have the same name, input, and outputs. If this happens, the functions also have the same ABI and exporting them both under the same name actually works.

(Note: I don't like that 2 functions with the same signature have the same hash, so if you want to, I can hash the source code position of the function as well to make collisions very unlikely.)

Consequently, the hashes of all imported functions are now different. A lot of tests were updated, but the main new test is called modules.rs.

I also had to enable the 'extra-traits' feature on syn. This feature enables implementing Hash for syn::Signature and other types.

@daxpedda
Copy link
Collaborator

(Note: I don't like that 2 functions with the same signature have the same hash, so if you want to, I can hash the source code position of the function as well to make collisions very unlikely.)

I'm a bit surprised why this doesn't cause issues already. I will take a deeper look.

I also had to enable the 'extra-traits' feature on syn. This feature enables implementing Hash for syn::Signature and other types.

That's probably very undesirable, I will take a look if we can avoid that.

Copy link
Collaborator

@daxpedda daxpedda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for looking into it!

Unfortunately I couldn't find a way to get the line number or column from the Span or anywhere else, so I don't think this is possible outside of Rust nightly. I'm a bit perplexed how Rust is just ignoring the fact that we are exporting a function with the same exact name twice. I would have guessed a linker error or something.

I should mention that this is most likely buggy with other things as well, but I guess we will get there when we do.

@daxpedda daxpedda added the waiting for author Waiting for author to respond label Nov 14, 2024
@RunDevelopment
Copy link
Contributor Author

I'm a bit perplexed how Rust is just ignoring the fact that we are exporting a function with the same exact name twice. I would have guessed a linker error or something.

Only if the two functions have different ABIs. They explained this when they talked about why no_mangle is unsafe in the 2024 edition. rust-lang/rust#28179

I think this is also the cause of #2338, but I'm not 100% sure.

@daxpedda daxpedda merged commit 9eda4ae into rustwasm:main Nov 14, 2024
41 checks passed
@RunDevelopment RunDevelopment deleted the hash-full-signature branch November 14, 2024 15:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
waiting for author Waiting for author to respond
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Failed to import the same js functions with same rust fn name under different modules
2 participants