Skip to content

Make __rust_alloc_error_handler_should_panic a function #143387

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 1 commit into from
Jul 4, 2025

Conversation

dpaoliello
Copy link
Contributor

@dpaoliello dpaoliello commented Jul 3, 2025

Fixes #143253

__rust_alloc_error_handler_should_panic is a static but was being exported as a function.

For most targets this doesn't matter, but Arm64EC Windows uses different decorations for exported variables vs functions, hence it fails to link when -Z oom=abort is enabled.

We've had issues in the past with statics like this (see #141061) but the tldr; is that Arm64EC needs symbols correctly exported as either a function or data, and data MUST and MUST ONLY be marked dllimport when the symbol is being imported from another binary, which is non-trivial to calculate for these compiler-generated statics.

So, instead, the easiest thing to do is to make __rust_alloc_error_handler_should_panic a function instead.

Since __rust_alloc_error_handler_should_panic isn't involved in any linking shenanigans, I've marked it as AlwaysInline with the hopes that the various backends will see that it is just returning a constant and perform the same optimizations as the previous implementation.

r? @bjorn3

@rustbot rustbot added A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jul 3, 2025
@rustbot
Copy link
Collaborator

rustbot commented Jul 3, 2025

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

Some changes occurred in compiler/rustc_codegen_gcc

cc @antoyo, @GuillaumeGomez

@rust-log-analyzer

This comment has been minimized.

@bjorn3
Copy link
Member

bjorn3 commented Jul 3, 2025

Since __rust_alloc_error_handler_should_panic isn't involved in any linking shenanigans, I've marked it as AlwaysInline with the hopes that the various backends will see that it is just returning a constant and perform the same optimizations as the previous implementation.

That won't have any effect. It is part of the allocator shim object file, which doesn't participate in LTO unless you use -Clinker-plugin-lto. With -Clinker-plugin-lto it should in principle always be inlined even without that attribute. Still can't hurt to have it I guess.

@rustbot
Copy link
Collaborator

rustbot commented Jul 3, 2025

The Miri subtree was changed

cc @rust-lang/miri

@rust-log-analyzer

This comment has been minimized.

@bjorn3
Copy link
Member

bjorn3 commented Jul 3, 2025

r=me with CI passing

@bjorn3
Copy link
Member

bjorn3 commented Jul 4, 2025

@bors r+

@bors
Copy link
Collaborator

bors commented Jul 4, 2025

📌 Commit 2b22d0f has been approved by bjorn3

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 4, 2025
@tgross35
Copy link
Contributor

tgross35 commented Jul 4, 2025

Since this is changing to a new _v2 name now, why not make it a static rather than a function call?

@RalfJung
Copy link
Member

RalfJung commented Jul 4, 2025

IIUC that's very hard to do correctly on arm64ec, and because one fairly niche platform makes this (needlessly?) complicated, we have to use a less optimal solution on all platforms...

(I guess we could switch between function and data variable depending on the target, but that'd mean even more hacks in rustc.)

@tgross35
Copy link
Contributor

tgross35 commented Jul 4, 2025

Ah, that's the dllimport comment in the top post? Makes sense. If the situation improves, v3 can be a static 😆

bors added a commit that referenced this pull request Jul 4, 2025
Rollup of 7 pull requests

Successful merges:

 - #140643 (Refactor StableMIR)
 - #143286 (Make -Ztrack-diagnostics emit like a note)
 - #143308 (Remove `PointerLike` trait)
 - #143387 (Make __rust_alloc_error_handler_should_panic a function)
 - #143400 (Port `#[rustc_pass_by_value]` to the new attribute system)
 - #143417 (bump termize dep)
 - #143420 (rustc-dev-guide subtree update)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 2faf66d into rust-lang:master Jul 4, 2025
11 checks passed
@rustbot rustbot added this to the 1.90.0 milestone Jul 4, 2025
rust-timer added a commit that referenced this pull request Jul 4, 2025
Rollup merge of #143387 - dpaoliello:shouldpanicfn, r=bjorn3

Make __rust_alloc_error_handler_should_panic a function

Fixes #143253

`__rust_alloc_error_handler_should_panic` is a static but was being exported as a function.

For most targets this doesn't matter, but Arm64EC Windows uses different decorations for exported variables vs functions, hence it fails to link when `-Z oom=abort` is enabled.

We've had issues in the past with statics like this (see #141061) but the tldr; is that Arm64EC needs symbols correctly exported as either a function or data, and data MUST and MUST ONLY be marked `dllimport` when the symbol is being imported from another binary, which is non-trivial to calculate for these compiler-generated statics.

So, instead, the easiest thing to do is to make `__rust_alloc_error_handler_should_panic` a function instead.

Since `__rust_alloc_error_handler_should_panic` isn't involved in any linking shenanigans, I've marked it as `AlwaysInline` with the hopes that the various backends will see that it is just returning a constant and perform the same optimizations as the previous implementation.

r? `@bjorn3`
github-actions bot pushed a commit to kosai-ksdfounder/miri that referenced this pull request Jul 5, 2025
Rollup of 7 pull requests

Successful merges:

 - rust-lang/rust#140643 (Refactor StableMIR)
 - rust-lang/rust#143286 (Make -Ztrack-diagnostics emit like a note)
 - rust-lang/rust#143308 (Remove `PointerLike` trait)
 - rust-lang/rust#143387 (Make __rust_alloc_error_handler_should_panic a function)
 - rust-lang/rust#143400 (Port `#[rustc_pass_by_value]` to the new attribute system)
 - rust-lang/rust#143417 (bump termize dep)
 - rust-lang/rust#143420 (rustc-dev-guide subtree update)

r? `@ghost`
`@rustbot` modify labels: rollup
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Linking broken on arm64ec due to __rust_alloc_error_handler_should_panic symbol
7 participants