Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Report allocation errors as panics, second attempt #112331

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Report allocation errors as panics, second attempt #112331

wants to merge 4 commits into from
192 −880

Conversation

Amanieu
Copy link
Member

@Amanieu Amanieu commented Jun 5, 2023

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 AllocErrorPanicPayload and changes allocation errors to always be reported as panics.
  • The second commit removes #[alloc_error_handler] and the alloc_error_hook API.

ACP: rust-lang/libs-team#192

Closes #51540
Closes #51245

@rustbot
Copy link
Collaborator

rustbot commented Jun 5, 2023

r? @jackh726

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added 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 Jun 5, 2023
@rustbot
Copy link
Collaborator

rustbot commented Jun 5, 2023

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

Some changes occurred in compiler/rustc_codegen_gcc

cc @antoyo

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bors
Copy link
Contributor

bors commented Jun 15, 2023

☔ The latest upstream changes (presumably #112671) made this pull request unmergeable. Please resolve the merge conflicts.

@jackh726
Copy link
Member

Should this be assigned to someone on libs team? I guess it's mostly compiler work?

@Amanieu
Copy link
Member Author

Amanieu commented Jun 16, 2023

Yes, most of the compiler changes are just removing #[alloc_error_handler].

r? libs

@rustbot rustbot assigned joshtriplett and unassigned jackh726 Jun 16, 2023
@breathx
Copy link

breathx commented Jun 20, 2023

@Amanieu could you say if some updates on it planned? Why not merging into master?

@Amanieu
Copy link
Member Author

Amanieu commented Jun 20, 2023

It's waiting on review.

@bors
Copy link
Contributor

bors commented Jun 25, 2023

☔ The latest upstream changes (presumably #113014) made this pull request unmergeable. Please resolve the merge conflicts.

@Amanieu
Copy link
Member Author

Amanieu commented Jun 30, 2023

r? libs-api

@rustbot rustbot assigned m-ou-se and unassigned joshtriplett Jun 30, 2023
@bors
Copy link
Contributor

bors commented Jun 30, 2023

☔ The latest upstream changes (presumably #113162) made this pull request unmergeable. Please resolve the merge conflicts.

bjorn3
bjorn3 reviewed Jun 30, 2023
View reviewed changes
compiler/rustc_codegen_cranelift/example/alloc_example.rs Outdated
#[lang = "eh_personality"]
fn eh_personality() -> ! {
loop {}
}

#[start]
Copy link
Member

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.

Copy link
Member Author

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.

@Amanieu Amanieu force-pushed the panic-oom-payload2 branch 2 times, most recently from 238d017 to 35d75e3 Compare June 30, 2023 16:49
@bors
Copy link
Contributor

bors commented Jul 6, 2023

☔ The latest upstream changes (presumably #113391) made this pull request unmergeable. Please resolve the merge conflicts.

@bjorn3 bjorn3 mentioned this pull request Jul 13, 2023
Open
@Amanieu Amanieu mentioned this pull request Oct 2, 2023
Merged
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bors
Copy link
Contributor

bors commented Oct 11, 2023

☔ The latest upstream changes (presumably #116578) made this pull request unmergeable. Please resolve the merge conflicts.

This was referenced Dec 9, 2023
Open
Open
@workingjubilee workingjubilee mentioned this pull request Dec 9, 2023
Closed
@RalfJung
Copy link
Member

RalfJung commented Dec 10, 2023 •
edited
Loading

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.

@cutecutecat cutecutecat mentioned this pull request Jan 5, 2024
Merged
@Dylan-DPC Dylan-DPC added S-waiting-on-author 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 Feb 10, 2024
@Dylan-DPC
Copy link
Member

This has conflicts to be resolved before getting reviewed

@bjorn3 bjorn3 mentioned this pull request Mar 25, 2024
Open
3 tasks
@RalfJung RalfJung mentioned this pull request Apr 17, 2024
Merged
bors added a commit to rust-lang/miri that referenced this pull request Apr 17, 2024
@bors
Auto merge of #3480 - RalfJung:alloc_error_handler, r=RalfJung
dcd17c9 
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 Apr 20, 2024
@bors
Auto merge of rust-lang#3480 - RalfJung:alloc_error_handler, r=RalfJung
9776f64 
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 May 30, 2024
@crawfxrd
uefi_std: Remove unstable features except prelude_import
a784d3b 
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 with `panic=abort`
- `_Unwind_Resume`: Symbol not present with `panic=abort`
- `alloc_error_handler`: OOM triggers a panic; feature will be removed [1]

[1]: rust-lang/rust#112331

Signed-off-by: Tim Crawford <[email protected]>
@bjorn3 bjorn3 mentioned this pull request Jun 19, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bjorn3 bjorn3 bjorn3 left review comments

Assignees

@m-ou-se m-ou-se

Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. 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
Milestone
No milestone