-
Notifications
You must be signed in to change notification settings - Fork 404
Restore support for the wasm32-wasip2 target #4663
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
base: master
Are you sure you want to change the base?
Conversation
|
Thank you for contributing to Miri! |
This adjusts find_mir_or_eval_fn to respect #[wasm_import_module] on wasm. To avoid having to thread through the import module everywhere, I opted to merge it with the item name with a $$$ as separator. This is unlikely to occur in a real import module name or item name. This reverts commit c38128b and makes a few minor adjustments.
ce62291 to
092ffea
Compare
| - `solaris` / `illumos`: maintained by @devnexen. Supports the entire test suite. | ||
| - `freebsd`: maintained by @YohDeadfall and @LorrensP-2158466. Supports the entire test suite. | ||
| - `android`: **maintainer wanted**. Support very incomplete, but a basic "hello world" works. | ||
| - `wasi`: **maintainer wanted**. Support very incomplete, but a basic "hello world" works. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you willing to be listed as target maintainer for this?
I don't think I want to re-land this without a target maintainer.
e29e4d9 to
1bb382f
Compare
|
Marking as "blocked" since we should have a target maintainer. |
|
|
||
| let (interface, name) = if let Some(instance) = instance | ||
| && let Some(module) = | ||
| this.tcx.wasm_import_module_map(instance.def_id().krate).get(&instance.def_id()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW I think it is a bug in rustc that link_name returns the wrong name here... link_name should return whatever is necessary to find the symbol to link to. Apparently that's a tuple of two names for some targets -- fine. But instead we return a single name that's sometimes one component of that tuple, and sometimes completely irrelevant to linking, and then one needs to use out-of-band mechanisms to get the actual tuple -- that's not a good way to deal with this IMO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With core wasm the import name is a tuple of two names. However for the linking tool convention the symbol name used for resolving symbols within the linker session is entirely separate from the name used for importing items from outside the final linked wasm module. The symbol name is ignored after linking, while the import name tuple is used at runtime. Rustc's link_name returns the symbol name, while this code computes the import name tuple.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rustc's link_name returns the symbol name, while this code computes the import name tuple.
That's at best very confusing. I would expect that query to return the information about what symbol this links to (what I would call the "link name", and what wasm apparently calls "import name"). Apparently we don't have any query that returns this information, instead every backend duplicates this logic?
As you said elsewhere, this isn't even wasm-specific. Windows also uses a tuple for names, IIUC.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For Windows we also use the link name and there is no option to use the import name. The import library is what defines the mapping from link name to dll name + import name. So the linker only sees the symbol name (as returned by tcx.symbol_name() in rustc) when resolving symbols. On wasm the symbol table directly contains the import name tuple rather than putting it in a separate file, but I believe the linker still only matches on the symbol name and handles the imports later.
(I don't think wasm formally uses symbol name/import name. I made this naming scheme up to help describe what I meant)
|
☔ The latest upstream changes (possibly 37c660f) made this pull request unmergeable. Please resolve the merge conflicts. |
This adjusts find_mir_or_eval_fn to respect #[wasm_import_module] on wasm. To avoid having to thread through the import module everywhere, I opted to merge it with the item name with a $$$ as separator. This is unlikely to occur in a real import module name or item name.
This reverts commit c38128b and makes a few minor adjustments.