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

Stage2: wasm-linker - Only export symbols notated as such #10517

Merged
merged 2 commits into from
Jan 11, 2022

Conversation

Luukdegram
Copy link
Contributor

This exposes a function from stage2 to stage1 to append symbols to automatically export them.
Stage1 will only call this function when the target is wasm.

The linker will then only append --export=<symbol> for when the user has not done so manually and did not provide the -rdynamic flag also. This allows the user to still override the default behavior do they wish to.

I also went ahead and implemented the same behavior for the LLVM backend when using stage2, meaning we can remove all stage1 related code for this without losing the behavior when stage2 becomes the default compiler.

Closes #2910
Closes #7133

@Luukdegram
Copy link
Contributor Author

This is interesting. The error in the CI actually only happens when using wasmtime, where as wasmer executes the tests perfectly. For some reason, wasmtime assume an exported u32 global is a function. (wasm-objdump correctly shows it as a global, that points to the symbol's value in the data section).
I'll investigate more tomorrow and create an issue downstream if it's indeed a bug on their end.

@kubkon
Copy link
Member

kubkon commented Jan 5, 2022

This is interesting. The error in the CI actually only happens when using wasmtime, where as wasmer executes the tests perfectly. For some reason, wasmtime assume an exported u32 global is a function. (wasm-objdump correctly shows it as a global, that points to the symbol's value in the data section). I'll investigate more tomorrow and create an issue downstream if it's indeed a bug on their end.

Note that this very well may be a bug with Wasmtime. Great job btw!

@andrewrk
Copy link
Member

andrewrk commented Jan 6, 2022

Should we switch to wasmer in the CI, or perhaps upgrade to a newer wasmtime version?

@Luukdegram
Copy link
Contributor Author

It appears that under the current WASI application ABI, instances may assume exports are not accessed when using the command execution model. This is exactly the model we use when running tests. Wasmer seems to simply ignore unexpected exports, whereas wasmtime is a bit more strict about it and errors. (There's a flag --allow-unknown-exports available to allow those).

Here's how I think we should solve it for now:
Append the --allow-unknown-exports flag to wasmtime, and revisit this flag when stage2 becomes the default compiler. (As we will have much more information about the export decls, as well as the execution model we're using).

@kubkon
Copy link
Member

kubkon commented Jan 6, 2022

Sounds reasonable to me!

This exposes a function from stage2 to stage1 to append symbols to automatically export them.

This happends under the following conditions:
- Target is wasm
- User has not provided --export/--rdynamic flags themselves.
@Luukdegram Luukdegram force-pushed the linker-eport-symbols branch 3 times, most recently from c9e6474 to 21c4720 Compare January 6, 2022 19:28
Copy link
Member

@andrewrk andrewrk left a comment

Choose a reason for hiding this comment

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

Nice work! Just need an additional cache input and then this is good to go.

@Luukdegram Luukdegram force-pushed the linker-eport-symbols branch from 21c4720 to 1b054c3 Compare January 6, 2022 19:42
Copy link
Member

@andrewrk andrewrk left a comment

Choose a reason for hiding this comment

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

After making the previous review, I realized my suggestion about the cache hash is incorrect, since export_symbol_names depends only on the source files and cannot be overridden by CLI args.

This is good to merge.

@Luukdegram Luukdegram force-pushed the linker-eport-symbols branch from 1b054c3 to da6da66 Compare January 6, 2022 19:46
@andrewrk
Copy link
Member

andrewrk commented Jan 6, 2022

The Windows failure does look potentially relevant. The failing test is using the wasm32-wasi target.

@Luukdegram Luukdegram force-pushed the linker-eport-symbols branch 2 times, most recently from 7f38005 to e4fd1e8 Compare January 7, 2022 08:23
@Luukdegram
Copy link
Contributor Author

The Windows failure does look potentially relevant. The failing test is using the wasm32-wasi target.

Failure definitely seems to be related to this change, as it is reproducible on the CI. However, I fail to see why this is happening on Windows only. I'll see if I can rent out some cheap Windows VM as I don't own any Windows machine nor have the RAM to run a VM that hosts Windows and is able to build the compiler. Will need some more time to get this PR ready, but we'll get there!

@Luukdegram Luukdegram force-pushed the linker-eport-symbols branch 5 times, most recently from 75a8f43 to 26c526c Compare January 7, 2022 16:50
@Luukdegram
Copy link
Contributor Author

Interestingly, commenting out the call to stage2_append_symbol does make the test cases for Windows succeed. This makes me think it may be some ABI issue, tho the function definition does seem alright.

@Luukdegram Luukdegram force-pushed the linker-eport-symbols branch from 26c526c to 231ccde Compare January 7, 2022 17:27
Also skip exporting non-function symbols when we're building a WASI command using the stage2 llvm backend.
@Luukdegram Luukdegram force-pushed the linker-eport-symbols branch from 231ccde to 247b638 Compare January 7, 2022 17:35
@kubkon
Copy link
Member

kubkon commented Jan 10, 2022

@Luukdegram I got my local Win10 installation setup for development today so can have a look at this failure tomorrow. Will keep you posted!

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 automatically links and re-exports all of libc wasm output contains compiler_rt exports
3 participants