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.

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

Report allocation errors as panics, second attempt #112331

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

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.

@Amanieu Amanieu force-pushed the panic-oom-payload2 branch from 77ba90f to d4f1f6a Compare June 6, 2023 16:12
@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
@Amanieu Amanieu force-pushed the panic-oom-payload2 branch from d4f1f6a to bacf25d Compare June 17, 2023 22:51
@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 Amanieu force-pushed the panic-oom-payload2 branch from bacf25d to 5196f92 Compare June 30, 2023 05:57
@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.

#[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.

@Amanieu Amanieu force-pushed the panic-oom-payload2 branch from 35d75e3 to 3a6ff98 Compare October 2, 2023 16:09
@rust-log-analyzer

This comment has been minimized.

@Amanieu Amanieu force-pushed the panic-oom-payload2 branch from 3a6ff98 to 8a24112 Compare October 2, 2023 16:18
@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.

@RalfJung
Copy link
Member

RalfJung commented Dec 10, 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.

@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

bors added a commit to rust-lang/miri that referenced this pull request Apr 17, 2024
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
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
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 <tcrawford@system76.com>
@Dylan-DPC Dylan-DPC added S-waiting-on-ACP 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 Sep 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-ACP Status: PR has an ACP and is waiting for the ACP to complete. 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