Fix double panic when printing query stack during an ICE #64799
Conversation
|
r? @cramertj (rust_highfive has picked a reviewer for you, use r? to override) |
|
r? @Centril |
src/librustc_errors/lib.rs
Outdated
| self.emit_diagnostic(&Diagnostic::new(Bug, msg)); | ||
| panic!(ExplicitBug); |
There was a problem hiding this comment.
The change here doesn't seem to account for .emit_error above which is called by fn err and fn fatal.
|
I have some thoughts on this approach, will write a more detailed comment later today, please don't merge yet though. |
|
I think this patch takes the wrong approach. We likely can't hope to eliminate all sources of I would add a field, something like Does that sound plausible? Specifically I think that the bug today is in the panic handler which blatantly assumes that the diagnostic machinery is in a good state, which if the panic happened in diagnostic machinery, may not be the case -- this callback sort of ensures that the diagnostic machinery decides whether it's usable on panic. |
|
Ah, and forgot to mention -- we could expose something like |
dc51a90 to
c52e165
Compare
|
@Mark-Simulacrum: I just saw your comment. I also came to the conclusion that we shouldn't attempt to move all panics into I came up with a different approach. I created a new helper trait, which I implemented for |
Not all panics come as a result of deliberate panics in |
|
So I agree that the trait is a better approach then what we had here before, but I still think it's not enough to really resolve this. We would need to be really careful about what we call (including transitively) to make sure that none of that code can panic, which is ~impossible. Looking into the underlying code, I think the real problem here is that the TyCtxt query stack printing is using the global error handler ( rust/src/librustc/ty/query/plumbing.rs Line 339 in ddf4386 Basically, we shouldn't need to make Handler re-entrancy safe on panics, we should not presume in the panic handler that global state is okay still. You'll note that the ICE printing does not make assumptions itself about the global state of the compiler. (Realistically, the query stack printing should probably be much more careful about reaching into the TyCtxt, but just using the passed in diagnostic handler would be a good start, and easy to do, as we already create one in librustc_driver before calling that function on TyCtxt.) |
When the panic handler is run, the existing Handler may be in a weird state if it was responsible for triggering the panic. By using a freshly created Handler, we avoid trying to re-entrantly lock a HandlerInner, which was causing a double panic on ICEs.
e062447 to
e9aa0e7
Compare
|
@Mark-Simulacrum: Wow, that's so much simpler 😄 . Updated |
|
Indeed much simpler :) Could you add a comment to the stack printing along the lines of "be careful with global state here, as this is called in the unwind path, e.g. diagnostic printing should not go through session"? @bors delegate+ |
|
✌️ @Aaron1011 can now approve this pull request |
|
(r=me with that) |
|
@bors r+ |
|
📌 Commit 97906bc has been approved by |
|
@bors r- r+ You want |
|
📌 Commit 97906bc has been approved by |
|
@Mark-Simulacrum oops, sorry about that. |
…Simulacrum Fix double panic when printing query stack during an ICE On the latest nightly, any call to `bug` or `span_bug` will result in two panics - the first one as a normal result of calling `bug` / `span_bug`, and the second as a result of trying to print the query stack from the panic handler. This is caused by the query-printing code attempting to acquire a lock on `HandlerInnder`, which is still being held by `bug`. This PR moves the actual panic out of `HandlerInner`, into `Handler`. This allows us to release the lock on `HandlerInner` before triggering the panic, ensuring that the panic handler will be able to acquire the lock if necessary.
…Simulacrum Fix double panic when printing query stack during an ICE On the latest nightly, any call to `bug` or `span_bug` will result in two panics - the first one as a normal result of calling `bug` / `span_bug`, and the second as a result of trying to print the query stack from the panic handler. This is caused by the query-printing code attempting to acquire a lock on `HandlerInnder`, which is still being held by `bug`. This PR moves the actual panic out of `HandlerInner`, into `Handler`. This allows us to release the lock on `HandlerInner` before triggering the panic, ensuring that the panic handler will be able to acquire the lock if necessary.
| eprintln!("query stack during panic:"); | ||
|
|
||
| // Be careful reyling on global state here: this code is called from |
There was a problem hiding this comment.
| // Be careful reyling on global state here: this code is called from | |
| // Be careful relying on global state here: this code is called from |
…Simulacrum Fix double panic when printing query stack during an ICE On the latest nightly, any call to `bug` or `span_bug` will result in two panics - the first one as a normal result of calling `bug` / `span_bug`, and the second as a result of trying to print the query stack from the panic handler. This is caused by the query-printing code attempting to acquire a lock on `HandlerInnder`, which is still being held by `bug`. This F214 PR moves the actual panic out of `HandlerInner`, into `Handler`. This allows us to release the lock on `HandlerInner` before triggering the panic, ensuring that the panic handler will be able to acquire the lock if necessary.
Rollup of 6 pull requests Successful merges: - #64455 (Add Long error explanation for E0531) - #64546 (Bugfix/rfc 2451 rerebalance tests) - #64589 (Differentiate AArch64 bare-metal targets between hf and non-hf.) - #64763 (Add E0734 and its long explanation) - #64793 (Fix format macro expansions spans to be macro-generated) - #64799 (Fix double panic when printing query stack during an ICE) Failed merges: r? @ghost
…Simulacrum Fix double panic when printing query stack during an ICE On the latest nightly, any call to `bug` or `span_bug` will result in two panics - the first one as a normal result of calling `bug` / `span_bug`, and the second as a result of trying to print the query stack from the panic handler. This is caused by the query-printing code attempting to acquire a lock on `HandlerInnder`, which is still being held by `bug`. This PR moves the actual panic out of `HandlerInner`, into `Handler`. This allows us to release the lock on `HandlerInner` before triggering the panic, ensuring that the panic handler will be able to acquire the lock if necessary.
Rollup of 5 pull requests Successful merges: - #63492 (Remove redundancy from the implementation of C variadics.) - #64589 (Differentiate AArch64 bare-metal targets between hf and non-hf.) - #64799 (Fix double panic when printing query stack during an ICE) - #64824 (No StableHasherResult everywhere) - #64884 (Add pkg-config to dependency list if building for Linux on Linux) Failed merges: r? @ghost
On the latest nightly, any call to
bugorspan_bugwill result in two panics - the first one as a normal result of callingbug/span_bug, and the second as a result of trying to print the query stack from the panic handler. This is caused by the query-printing code attempting to acquire a lock onHandlerInnder, which is still being held bybug.This PR moves the actual panic out of
HandlerInner, intoHandler. This allows us to release the lock onHandlerInnerbefore triggering the panic, ensuring that the panic handler will be able to acquire the lock if necessary.