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

Get the default panic hook for ICEs, from std::panic::set_hook. #732

Merged
merged 1 commit into from
Aug 20, 2021

Conversation

eddyb
Copy link
Contributor

@eddyb eddyb commented Aug 19, 2021

Tested by adding -Ztreat-err-as-bug to test_rustc_flags in tests/src/main.rs and observing that the output does include the panic information printed by the default panic hook (i.e. std's).

Background: since the rustup in #716, we've had broken ICEs - they would show this message, and the lines after that, but none of the panic information that is supposed to precede it (and RUST_BACKTRACE=1 did nothing):

error: internal compiler error: unexpected panic

note: the compiler unexpectedly panicked. this is a bug.

note: we would appreciate a bug report: https://github.com/EmbarkStudios/rust-gpu/issues/new

note: rustc 1.56.0-nightly (ae90dcf02 2021-08-09) running on x86_64-unknown-linux-gnu

That's because of an interaction between rust-lang/rust#85640 and our own use of report_ice - thankfully, @bjorn3 provided a solution that doesn't require upstream changes, in the form of an aspect of std::panic::take_hook() I missed: you can keep calling it and it will return std's default panic hook, as opposed to crashing and/or returning some kind of noop closure.

This is documented on std::panic::take_hook as such:

If no custom hook is registered, the default hook will be returned.

Knowing that, it's easy to see why you can just call it more than once to both clear any existing panic hooks and get std's default panic hook - but I went a bit farther to try and guard against strange mishaps.

@eddyb eddyb requested a review from khyperia August 19, 2021 15:50
@eddyb eddyb enabled auto-merge (rebase) August 19, 2021 16:13
@eddyb eddyb mentioned this pull request Aug 20, 2021
khyperia added a commit that referenced this pull request Aug 20, 2021
@eddyb eddyb merged commit 2d5b8e6 into EmbarkStudios:main Aug 20, 2021
@eddyb eddyb deleted the refrost branch August 20, 2021 08:46
khyperia added a commit that referenced this pull request Aug 21, 2021
* Various fixes and cleanup

While working on other patches that ended up not being applicable, I've
gathered these changes unrelated to the irrelevant patch. So, submitting
them as a seperate change, since the bigger change isn't going in.

* Revert change superseded by #732
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants