Adjust for Arm64EC name mangling when checking for exported symbols - #163188
Conversation
|
r? @oli-obk rustbot has assigned @oli-obk. Use Why was this reviewer chosen?The reviewer was selected based on:
|
Whose? 😅 Got some links or other discussions that we can reference? |
|
Like, did this break because of a change on our end? |
| if strip_underscore { | ||
| name = name.strip_prefix('_').unwrap_or(&name).to_string(); | ||
| } | ||
| if header.machine() == pe::IMAGE_FILE_MACHINE_ARM64EC { | ||
| name = name.strip_prefix('#').unwrap_or(&name).to_string(); | ||
| } |
There was a problem hiding this comment.
Maybe turn this into a match on header.machine()?
|
Sorry, "Microsoft's" or specifically "Window's" - we have nightly validation for Arm64EC for both Rust and LLVM (running toolchains and tests all compiled as Arm64EC on real hardware). Can't provide links because it's all internal. But as bjorn3 confirmed, the breaks we see are typically because upstream changed or added something but didn't account for the wackiness of EC, hence we (Windows) take the burden on running tests and fixing issues. |
|
Just so we have all the information here instead of looking for PRs and stuff:
That's #160679.
So you saw this issue because the |
Updated the description to add this, thanks!
Correct. Arm64EC is a Tier 2 target: the Rust project isn't expected to run tests against it, so it is not surprising nor unexpected that gaps like this appear. While I would love for EC to be made a Tier 1 (no host tools) target, I believe that would be rejected for its lack of usage, and rightly so. The current arrangement is a reasonable balance: as a Tier 2 the Rust Project builds and distributes the Standard Library for Arm64EC, Microsoft (as a maintainer and interested vendor) spends the resources to test and validate and submits fixes upstream. Individual contributors shouldn't be expected to know how Arm64EC works (or even have an environment setup where they could test it), but the Rust Project should be receptive when Microsoft submits PRs like this to say "hey, it broke and here's the fix" (and y'all have been fantastic about this so far!). Edit: I will, in the future, try to be a bit more clear about how these bugs were discovered and where the break was introduced. |
b9e95ba to
2705561
Compare
|
@rustbot ready |
|
Hi @dpaoliello. Thanks for catching and fixing this. I'm the author of #160679 and I should have accounted for Arm64EC's # decoration here, sorry about that. After digging into this a bit more while reviewing, I noticed the stripping logic here is still more limited than the full decoration scheme that That's a latent gap rather than something that's currently breaking (those ABI decorations are opt-in via an explicit calling convention, unlike the unconditional |
Great! To be clear, it's for due diligence rather than this PR itself (which is good and appreciated), in general:
All this additional context helps understand the previous work, evaluate the current work and alternatives, and organize possible future work related to a given issue (again, in general). I think we have enough context now to both land this and know what to do next. This is short and sweet and we can improve Thanks! |
…uwer Rollup of 14 pull requests Successful merges: - #162976 (fix quadratic naming of duplicate sidebar links) - #161275 (Refactor `core::cmp::{smallest, largest}` & add `mir-opt` test) - #163143 (cg_llvm: Use fewer FFI calls to check the target CPU's features) - #163188 (Adjust for Arm64EC name mangling when checking for exported symbols) - #163211 (`rustc_builtin_macros` cleanup, part 6) - #161386 (Don't merge distinct impl candidates) - #162942 (Remove `StashKey::AssociatedTypeSuggestion`) - #163096 (Don't suggest `std::` rustfix paths in `#![no_std]` crates) - #163110 (Mark `std::os::wasip2` with correct doc-cfgs, mark as unstable) - #163185 (properly decrement available_depth on cycles and provisional cache hits) - #163214 (revert r14 register names for arm) - #163226 (miri subtree update) - #163228 (Add regression test for trait predicate with escaping bounds) - #163234 (`rustc_dump_symbol_name`: add demangling information as a note instead)
Rollup merge of #163188 - dpaoliello:arm64ecsymboledit, r=lqd,bjorn3 Adjust for Arm64EC name mangling when checking for exported symbols After #160679 the Arm64EC nightly validation run internally by Microsoft started failing with: ``` === STDOUT === main.c main.obj : error LNK2019: unresolved external symbol my_add referenced in function #my_add$exit_thunk (EC Symbol) main.obj : error LNK2019: unresolved external symbol my_hash_lookup referenced in function #my_hash_lookup$exit_thunk (EC Symbol) main.obj : error LNK2019: unresolved external symbol call_internal referenced in function #call_internal$exit_thunk (EC Symbol) main.obj : error LNK2019: unresolved external symbol my_safe_div referenced in function #my_safe_div$exit_thunk (EC Symbol) main.exe : fatal error LNK1120: 4 unresolved externals ``` Root cause is that `symbol_edit` wasn't compensating for Arm64EC's name mangling, and so these symbols were missed.
…uwer Rollup of 14 pull requests Successful merges: - rust-lang/rust#162976 (fix quadratic naming of duplicate sidebar links) - rust-lang/rust#161275 (Refactor `core::cmp::{smallest, largest}` & add `mir-opt` test) - rust-lang/rust#163143 (cg_llvm: Use fewer FFI calls to check the target CPU's features) - rust-lang/rust#163188 (Adjust for Arm64EC name mangling when checking for exported symbols) - rust-lang/rust#163211 (`rustc_builtin_macros` cleanup, part 6) - rust-lang/rust#161386 (Don't merge distinct impl candidates) - rust-lang/rust#162942 (Remove `StashKey::AssociatedTypeSuggestion`) - rust-lang/rust#163096 (Don't suggest `std::` rustfix paths in `#![no_std]` crates) - rust-lang/rust#163110 (Mark `std::os::wasip2` with correct doc-cfgs, mark as unstable) - rust-lang/rust#163185 (properly decrement available_depth on cycles and provisional cache hits) - rust-lang/rust#163214 (revert r14 register names for arm) - rust-lang/rust#163226 (miri subtree update) - rust-lang/rust#163228 (Add regression test for trait predicate with escaping bounds) - rust-lang/rust#163234 (`rustc_dump_symbol_name`: add demangling information as a note instead)
After #160679 the Arm64EC nightly validation run internally by Microsoft started failing with:
Root cause is that
symbol_editwasn't compensating for Arm64EC's name mangling, and so these symbols were missed.