Report allocation errors as panics, second attempt by Amanieu · Pull Request #112331 · rust-lang/rust (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
Attempt to re-land #109507 now that #110771 is fixed.
OOM is now reported as a panic but with a custom payload type (AllocErrorPanicPayload) which holds the layout that was passed to handle_alloc_error.
This should be reviewed one commit at a time:
- The first commit adds
AllocErrorPanicPayloadand changes allocation errors to always be reported as panics. - The second commit removes
#[alloc_error_handler]and thealloc_error_hookAPI.
r? @jackh726
(rustbot has picked a reviewer for you, use r? to override)
Status: Awaiting review from the assignee but also interested parties.
Relevant to the compiler team, which will review and decide on the PR/issue.
Relevant to the library team, which will review and decide on the PR/issue.
labels
Some changes occurred in compiler/rustc_codegen_cranelift
cc @bjorn3
Some changes occurred in compiler/rustc_codegen_gcc
cc @antoyo
This comment has been minimized.
Should this be assigned to someone on libs team? I guess it's mostly compiler work?
Yes, most of the compiler changes are just removing #[alloc_error_handler].
r? libs
@Amanieu could you say if some updates on it planned? Why not merging into master?
| #[lang = "eh_personality"] |
|---|
| fn eh_personality() -> ! { |
| loop {} |
| } |
| #[start] |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one is still necessary.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, that was a mistake.
This comment has been minimized.
This was referenced
Dec 9, 2023
The issue at swc-project/swc#8362 and following discussion on Zulip uncovered some subtle concerns around allocation-fail-handlers -- and this here is the first time user-defined code gets run on allocation failure. (Specifically, that user-defined code is the panic hook.)
The problem in that issue is that the library assumes there is no reentrancy, but it does cause an allocation, and so if that allocation fails and then the panic handler calls back into the library, we have an unsoundness due to aliasing references (or due to any of the other problems that reentrancy can cause). We have to globally decide that either libraries must generally assume that calling the allocator will run arbitrary safe code (and add safeguards against reentrancy where needed), or the code that runs on allocation failure needs to be "careful" to not touch state that might be invalid since the library governing that state is in the middle of something.
Status: This is awaiting some action (such as code changes or more information) from the author.
and removed S-waiting-on-review
Status: Awaiting review from the assignee but also interested parties.
labels
This has conflicts to be resolved before getting reviewed
bors added a commit to rust-lang/miri that referenced this pull request
directly call handle_alloc_error
Also test more codepaths. There's like 5 different things that can happen on allocation failure! Between -Zoom, #[alloc_error_handler], and set_alloc_error_hook, we have 3 layers of behavior overrides. It's all a bit messy.
rust-lang/rust#112331 seems intended to clean this up, but has not yet reached consensus.
RalfJung pushed a commit to RalfJung/rust that referenced this pull request
directly call handle_alloc_error
Also test more codepaths. There's like 5 different things that can happen on allocation failure! Between -Zoom, #[alloc_error_handler], and set_alloc_error_hook, we have 3 layers of behavior overrides. It's all a bit messy.
rust-lang#112331 seems intended to clean this up, but has not yet reached consensus.
jackpot51 pushed a commit to redox-os/uefi that referenced this pull request
Most of the unstable features are not required for building.
For panics, the only remaining detail is having an implementation of the
panic handler (panic_handler) itself.
eh_personality: Not required withpanic=abort_Unwind_Resume: Symbol not present withpanic=abortalloc_error_handler: OOM triggers a panic; feature will be removed 1
Signed-off-by: Tim Crawford tcrawford@system76.com
Status: PR has an ACP and is waiting for the ACP to complete.
and removed S-waiting-on-author
Status: This is awaiting some action (such as code changes or more information) from the author.
labels
Labels
Status: PR has an ACP and is waiting for the ACP to complete.
Relevant to the compiler team, which will review and decide on the PR/issue.
Relevant to the library team, which will review and decide on the PR/issue.