-
Notifications
You must be signed in to change notification settings - Fork 13.3k
Fix linking statics on Arm64EC #140176
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
Fix linking statics on Arm64EC #140176
Conversation
r? @Nadrieril rustbot has assigned @Nadrieril. Use |
Some changes occurred in compiler/rustc_codegen_ssa |
#
#
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.
Changes look good to me. Is it possible to write a regression test? (Either a full-scale integration test that links an exported static from one crate to another or a test that imports a static and dumps the symbol name or something?)
r? @wesleywiser |
@rustbot author |
Reminder, once the PR becomes ready for a review, use |
This PR modifies cc @jieyouxu |
Good call, found another bug: we need to correctly export variables as @rustbot ready |
#
This comment has been minimized.
This comment has been minimized.
Thanks @dpaoliello! @bors r+ rollup Also nominating for potential beta backport as this fixes a serious issue for the Tier 2 |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
d265b1f
to
6dabf7e
Compare
Verified with a modified PR job: https://github.com/rust-lang/rust/actions/runs/14870437710/job/41757432246?pr=140176 @rustbot ready |
@bors r+ |
☀️ Test successful - checks-actions |
What is this?This is an experimental post-merge analysis report that shows differences in test outcomes between the merged PR and its parent PR.Comparing 667247d (parent) -> c8b7f32 (this PR) Test differencesShow 3 test diffsStage 1
Stage 2
Job group index
Test dashboardRun cargo run --manifest-path src/ci/citool/Cargo.toml -- \
test-dashboard c8b7f32434c0306db5c1b974ee43443746098a92 --output-dir test-dashboard And then open Job duration changes
How to interpret the job duration changes?Job durations can vary a lot, based on the actual runner instance |
Finished benchmarking commit (c8b7f32): comparison URL. Overall result: no relevant changes - no action needed@rustbot label: -perf-regression Instruction countThis benchmark run did not return any relevant results for this metric. Max RSS (memory usage)Results (primary 0.1%, secondary 0.2%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesResults (primary 0.1%, secondary -2.8%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 771.655s -> 771.811s (0.02%) |
Arm64EC builds recently started to fail due to the linker not finding a symbol:
It turns out that
EMPTY_PANIC
is a new static variable that was being exported then imported from the standard library, but when exporting LLVM didn't prepend the name with#
(as only functions are prefixed with this character), whereas Rust was prefixing with#
when attempting to import it.The fix is to have Rust not prefix statics with
#
when importing.Adding tests discovered another issue: we need to correctly mark static exported from dylibs with
DATA
, otherwise MSVC's linker assumes they are functions and complains that there is no exit thunk for them.CI found another bug: we only apply
DllImport
to non-local statics that aren't foreign items (i.e., in anextern
block), that is we want to useDllImport
for statics coming from other Rust crates. However,__rust_no_alloc_shim_is_unstable
is a static generated by the Rust compiler if required, but downstream crates consider it a foreign item since it is declared in anextern "Rust"
block, thus they do not applyDllImport
to it and so fails to link if it is exported by the previous crate asDATA
. The fix is to applyDllImport
to foreign items that are marked with therustc_std_internal_symbol
attribute (i.e., we assume they aren't actually foreign and will be in some Rust crate).Fixes #138541
try-job: dist-aarch64-msvc
try-job: dist-x86_64-msvc
try-job: x86_64-msvc-1
try-job: x86_64-msvc-2