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 #109507

Merged
merged 4 commits into from
Apr 22, 2023
Merged

Conversation

Amanieu
Copy link
Member

@Amanieu Amanieu commented Mar 23, 2023

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 review 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 Mar 23, 2023

r? @davidtwco

(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 Mar 23, 2023
@rustbot
Copy link
Collaborator

rustbot commented Mar 23, 2023

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

  • Stabilizing library features
  • Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
  • Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
  • Changing public documentation in ways that create new stability guarantees
  • Changing observable runtime behavior of library APIs

Some changes occurred in src/tools/rust-analyzer

cc @rust-lang/rust-analyzer

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 added the S-waiting-on-fcp Status: PR is in FCP and is awaiting for FCP to complete. label Mar 23, 2023
@Amanieu Amanieu force-pushed the panic-oom-payload branch from d50da58 to ccad895 Compare March 23, 2023 00:50
@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Mar 23, 2023

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

@Amanieu Amanieu force-pushed the panic-oom-payload branch from 04025a1 to 7cdaf6e Compare March 23, 2023 23:14
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

Copy link
Member

@davidtwco davidtwco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, r=me if you're just waiting on reviewer

@Amanieu
Copy link
Member Author

Amanieu commented Mar 28, 2023

Currently blocked on this FCP.

@bors

This comment was marked as resolved.

@Amanieu Amanieu force-pushed the panic-oom-payload branch from 8511a3f to 2355a4a Compare April 16, 2023 15:36
@Amanieu Amanieu removed the S-waiting-on-fcp Status: PR is in FCP and is awaiting for FCP to complete. label Apr 16, 2023
@Amanieu
Copy link
Member Author

Amanieu commented Apr 16, 2023

FCP is complete, this is now ready to merge.

@Amanieu Amanieu force-pushed the panic-oom-payload branch from 2355a4a to 4b981c2 Compare April 16, 2023 18:50
@davidtwco
Copy link
Member

@bors r+

Amanieu added a commit to Amanieu/rust that referenced this pull request Apr 23, 2023
@Manishearth
Copy link
Member

I've opened #110733

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Apr 24, 2023
format panic message only once

For `panic!` and friends, the `std` panic runtime will always set the `.payload()` of `PanicInfo` to the formatted string. The linked issues show that formatting the message twice can cause problems, so we simply print the already formatted message instead of formatting it again. We can't remove the preformatted payload, because it can be observed by custom panic hooks.

fixes rust-lang#110717
fixes rust-itertools/itertools#694

cc `@Amanieu` who broke this in rust-lang#109507
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Apr 24, 2023
format panic message only once

For `panic!` and friends, the `std` panic runtime will always set the `.payload()` of `PanicInfo` to the formatted string. The linked issues show that formatting the message twice can cause problems, so we simply print the already formatted message instead of formatting it again. We can't remove the preformatted payload, because it can be observed by custom panic hooks.

fixes rust-lang#110717
fixes rust-itertools/itertools#694

cc ``@Amanieu`` who broke this in rust-lang#109507
equation314 added a commit to arceos-org/arceos that referenced this pull request Apr 24, 2023
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Apr 24, 2023
format panic message only once

For `panic!` and friends, the `std` panic runtime will always set the `.payload()` of `PanicInfo` to the formatted string. The linked issues show that formatting the message twice can cause problems, so we simply print the already formatted message instead of formatting it again. We can't remove the preformatted payload, because it can be observed by custom panic hooks.

fixes rust-lang#110717
fixes rust-itertools/itertools#694

cc ```@Amanieu``` who broke this in rust-lang#109507
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Apr 25, 2023
…manieu

Revert panic oom

This temporarily reverts rust-lang#109507 until rust-lang#110771 is addressed

r? `@Amanieu`
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Apr 26, 2023
jamienicol pushed a commit to jamienicol/gecko that referenced this pull request Apr 27, 2023
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Apr 28, 2023
format panic message only once

Formatting the panic message multiple times can cause problems for some real-world crates, so here's a test to ensure that we don't do that.

This was regressed in rust-lang#109507 and reverted in rust-lang#110782.

fixes rust-lang#110717
fixes rust-itertools/itertools#694
bjorn3 pushed a commit to bjorn3/rust that referenced this pull request Apr 29, 2023
Report allocation errors as panics

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 review 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 rust-lang#51540
Closes rust-lang#51245
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this pull request May 1, 2023
…milio

The new API landed in rust-lang/rust#109507

Differential Revision: https://phabricator.services.mozilla.com/D176383

UltraBlame original commit: 04bb12c2c8fd7036f9f2d0b85a73c6cb31b0dcf0
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request May 1, 2023
…milio

The new API landed in rust-lang/rust#109507

Differential Revision: https://phabricator.services.mozilla.com/D176383

UltraBlame original commit: 04bb12c2c8fd7036f9f2d0b85a73c6cb31b0dcf0
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this pull request May 1, 2023
…milio

The new API landed in rust-lang/rust#109507

Differential Revision: https://phabricator.services.mozilla.com/D176383

UltraBlame original commit: 04bb12c2c8fd7036f9f2d0b85a73c6cb31b0dcf0
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 5, 2023
Report allocation errors as panics

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 review 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 rust-lang#51540
Closes rust-lang#51245
antoyo pushed a commit to antoyo/rust that referenced this pull request Jun 19, 2023
Report allocation errors as panics

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 review 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 rust-lang#51540
Closes rust-lang#51245
thomcc pushed a commit to tcdi/postgrestd that referenced this pull request Jul 18, 2023
Revert panic oom

This temporarily reverts rust-lang/rust#109507 until rust-lang/rust#110771 is addressed

r? `@Amanieu`
RalfJung pushed a commit to RalfJung/rust-analyzer that referenced this pull request Apr 20, 2024
Revert panic oom

This temporarily reverts rust-lang/rust#109507 until rust-lang/rust#110771 is addressed

r? `@Amanieu`
RalfJung pushed a commit to RalfJung/rust-analyzer that referenced this pull request Apr 27, 2024
Revert panic oom

This temporarily reverts rust-lang/rust#109507 until rust-lang/rust#110771 is addressed

r? `@Amanieu`
Azure-stars pushed a commit to Starry-OS/axalloc that referenced this pull request May 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. 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