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

Fix double panic when printing query stack during an ICE #64799

Merged
merged 2 commits into from Sep 29, 2019

Conversation

@Aaron1011
Copy link
Contributor

commented Sep 26, 2019

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.

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

commented Sep 26, 2019

r? @cramertj

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

@Aaron1011

This comment has been minimized.

Copy link
Contributor Author

commented Sep 26, 2019

@rust-highfive rust-highfive assigned Centril and unassigned cramertj Sep 26, 2019
Copy link
Member

left a comment

First some nits you can apply.

src/librustc_errors/lib.rs Outdated Show resolved Hide resolved
src/librustc_errors/lib.rs Outdated Show resolved Hide resolved
src/librustc_errors/lib.rs Outdated Show resolved Hide resolved
src/librustc_errors/lib.rs Outdated Show resolved Hide resolved
src/librustc_errors/lib.rs Outdated Show resolved Hide resolved
self.emit_diagnostic(&Diagnostic::new(Bug, msg));
panic!(ExplicitBug);

This comment has been minimized.

Copy link
@Centril

Centril Sep 26, 2019

Member

The change here doesn't seem to account for .emit_error above which is called by fn err and fn fatal.

@Mark-Simulacrum

This comment has been minimized.

Copy link
Member

commented Sep 26, 2019

r? @Mark-Simulacrum

I have some thoughts on this approach, will write a more detailed comment later today, please don't merge yet though.

@Mark-Simulacrum

This comment has been minimized.

Copy link
Member

commented Sep 26, 2019

I think this patch takes the wrong approach. We likely can't hope to eliminate all sources of panic! with this lock held, so we probably want to take something of a two pronged approach.

I would add a field, something like Box<dyn FnMut(&mut HandlerInner) -> !> (and maybe ideally more constrained than that) which is registered whenever (i.e., we have a set_bug_callback or so). The intent would be that code interested in using diagnostic machinery from panic handlers wouldn't attempt to directly do something like sess.diagnostic().print_things(..) but rather the diagnostic machinery itself would invoke this callback instead of the current panic!(ExplicitBug) and pass in the inner handler.

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.

@Mark-Simulacrum

This comment has been minimized.

Copy link
Member

commented Sep 26, 2019

Ah, and forgot to mention -- we could expose something like invoke_panic_callback on the Handler itself that'll attempt to acquire the lock (maybe via try_lock, not sure, to avoid deadlocks in the parallel case) and then call the callback, too. That would be called in the default panic handler unless some flag is set or so I guess.

@Aaron1011 Aaron1011 force-pushed the Aaron1011:fix/double-panic branch from dc51a90 to c52e165 Sep 26, 2019
@Aaron1011

This comment has been minimized.

Copy link
Contributor Author

commented Sep 26, 2019

@Mark-Simulacrum: I just saw your comment. I also came to the conclusion that we shouldn't attempt to move all panics into Handler.

I came up with a different approach. I created a new helper trait, which I implemented for LockGuard<HandlerInner>. I then moved all of the methods from impl HandlerInner to the trait. Whenever a method wants to panic, it takes self by-value, calls drop(self), and then panics. This ensures that the lock is released before panicking, without requiring Handler to be aware of which methods of HanlderInner might panic.

src/librustc_errors/lib.rs Outdated Show resolved Hide resolved
src/librustc_errors/lib.rs Outdated Show resolved Hide resolved
src/librustc_errors/lib.rs Outdated Show resolved Hide resolved
@Aaron1011

This comment has been minimized.

Copy link
Contributor Author

commented Sep 26, 2019

The intent would be that code interested in using diagnostic machinery from panic handlers wouldn't attempt to directly do something like sess.diagnostic().print_things(..) but rather the diagnostic machinery itself would invoke this callback instead of the current panic!(ExplicitBug) and pass in the inner handler.

Not all panics come as a result of deliberate panics in HandlerInner, though. We still want to have a query stack printed for unexpected panics (e.g. assertion failures), which means that we would still need to access Handler from the panic hook. I think it's simpler to fix Handler so that it's always useable from a panic hook, instead of special-casing certain kinds of panics.

@Mark-Simulacrum

This comment has been minimized.

Copy link
Member

commented Sep 26, 2019

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 (

icx.tcx.sess.diagnostic().force_print_diagnostic(diag);
) -- it really shouldn't be, it should take an error handler as a parameter.

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.
@Aaron1011 Aaron1011 force-pushed the Aaron1011:fix/double-panic branch from e062447 to e9aa0e7 Sep 26, 2019
@Aaron1011

This comment has been minimized.

Copy link
Contributor Author

commented Sep 26, 2019

@Mark-Simulacrum: Wow, that's so much simpler 😄 . Updated

@Mark-Simulacrum

This comment has been minimized.

Copy link
Member

commented Sep 26, 2019

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+

@bors

This comment has been minimized.

Copy link
Contributor

commented Sep 26, 2019

✌️ @Aaron1011 can now approve this pull request

@Mark-Simulacrum

This comment has been minimized.

Copy link
Member

commented Sep 26, 2019

(r=me with that)

@Aaron1011

This comment has been minimized.

Copy link
Contributor Author

commented Sep 26, 2019

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

commented Sep 26, 2019

📌 Commit 97906bc has been approved by Aaron1011

@Mark-Simulacrum

This comment has been minimized.

Copy link
Member

commented Sep 26, 2019

@bors r- r+

You want r=Mark-Simulacrum or so :)

@bors

This comment has been minimized.

Copy link
Contributor

commented Sep 26, 2019

📌 Commit 97906bc has been approved by Mark-Simulacrum

@Aaron1011

This comment has been minimized.

Copy link
Contributor Author

commented Sep 26, 2019

@Mark-Simulacrum oops, sorry about that.

Centril added a commit to Centril/rust that referenced this pull request Sep 27, 2019
…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.
bors added a commit that referenced this pull request Sep 27, 2019
Rollup of 10 pull requests

Successful merges:

 - #63937 ( Fix ICE in rustdoc when merging generic and where bounds of an Fn with an output)
 - #64419 (Deduplicate some code between miri and const prop)
 - #64546 (Bugfix/rfc 2451 rerebalance tests)
 - #64589 (Differentiate AArch64 bare-metal targets between hf and non-hf.)
 - #64745 (Include message on tests that should panic but do not)
 - #64793 (Fix format macro expansions spans to be macro-generated)
 - #64799 (Fix double panic when printing query stack during an ICE)
 - #64802 (Account for tail expressions when pointing at return type)
 - #64815 (Fix div_duration() marked as stable by mistake)
 - #64818 (update rtpSpawn's parameters type(It's prototype has been updated in libc))

Failed merges:

r? @ghost
Centril added a commit to Centril/rust that referenced this pull request Sep 27, 2019
…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

This comment has been minimized.

Copy link
@aclonegeek

aclonegeek Sep 27, 2019

Suggested change
// Be careful reyling on global state here: this code is called from
// Be careful relying on global state here: this code is called from
Centril added a commit to Centril/rust that referenced this pull request Sep 28, 2019
…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.
bors added a commit that referenced this pull request Sep 28, 2019
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
Centril added a commit to Centril/rust that referenced this pull request Sep 29, 2019
…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.
bors added a commit that referenced this pull request Sep 29, 2019
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
@bors bors merged commit 97906bc into rust-lang:master Sep 29, 2019
4 checks passed
4 checks passed
pr Build #20190926.73 succeeded
Details
pr (Linux mingw-check) Linux mingw-check succeeded
Details
pr (Linux x86_64-gnu-llvm-6.0) Linux x86_64-gnu-llvm-6.0 succeeded
Details
pr (LinuxTools) LinuxTools succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.