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

Use panic::set_hook to print the ICE message #60584

Merged
merged 8 commits into from
Sep 15, 2019
Merged

Use panic::set_hook to print the ICE message #60584

merged 8 commits into from
Sep 15, 2019

Conversation

jonas-schievink
Copy link
Contributor

This allows custom frontends and backends to override the hook with their own, for example to point people to a different issue tracker.

ICE messages are printed in a slightly different order now. Nightly prints:

thread 'rustc' panicked at 'called `Option::unwrap()` on a `None` value', src/libcore/option.rs:347:21
note: Run with `RUST_BACKTRACE=1` environment variable to display a backtrace.
error: aborting due to 2 previous errors

Some errors have detailed explanations: E0277, E0658.
For more information about an error, try `rustc --explain E0277`.

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/rust-lang/rust/blob/master/CONTRIBUTING.md#bug-reports

note: rustc 1.36.0-nightly (08bfe1612 2019-05-02) running on x86_64-unknown-linux-gnu

After this PR, rustc prints:

thread 'rustc' panicked at 'called `Option::unwrap()` on a `None` value', src/libcore/option.rs:347:21
note: Run with `RUST_BACKTRACE=1` environment variable to display a backtrace.

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/rust-lang/rust/blob/master/CONTRIBUTING.md#bug-reports

note: rustc 1.36.0-dev running on x86_64-unknown-linux-gnu

error: aborting due to 2 previous errors

Some errors have detailed explanations: E0277, E0658.
For more information about an error, try `rustc --explain E0277`.

@jonas-schievink jonas-schievink 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. labels May 6, 2019
@davidtwco
Copy link
Member

r? @Zoxc (suggested by GitHub)

@rust-highfive rust-highfive assigned Zoxc and unassigned davidtwco May 6, 2019
@Zoxc

This comment has been minimized.

@jonas-schievink

This comment has been minimized.

@jonas-schievink jonas-schievink requested a review from Zoxc May 9, 2019 23:13
src/librustc_driver/lib.rs Outdated Show resolved Hide resolved
@@ -444,7 +444,8 @@ where R: 'static + Send,

let (tx, rx) = channel();

let result = rustc_driver::report_ices_to_stderr_if_any(move || {
rustc_driver::install_ice_hook();
Copy link
Contributor

Choose a reason for hiding this comment

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

This may cause problem for tests. We don't want to use the custom panic hook for them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've played around with this for a bit and couldn't notice any rustdoc changes, except that ICEs while building a test are missing the query stack for some reason, which seems odd.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correction: The hook in this PR never seems to be called when doing rustdoc --test bla.rs. On nightly, the query stack hook still runs, but the normal ICE message isn't printed.

run_compiler is apparently only called for building documentation, not for running tests.

I'll change it to also register the hook when running doctests, since that matches the old behavior more closely, and having proper ICE messages there is a useful thing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The ICE message printed by the hook ends up escaping libtest's output capturing, so it looks really jarring. I've removed this line instead. The only thing lost is the query stack. A user will still see that an ICE is an ICE and can extract it into a non-doctest instead.

Let me know if this isn't good enough. Ideally we'd rework how rustdoc runs tests entirely so it behaves closer to the normal rustc frontend, but that's clearly out of scope for this PR.

@rust-highfive

This comment has been minimized.

@Zoxc
Copy link
Contributor

Zoxc commented May 19, 2019

@QuietMisdreavus Do you know enough about how panics are handled in rustdoc to review this?

@jonas-schievink
Copy link
Contributor Author

@QuietMisdreavus is no longer reviewing PRs, so maybe @GuillaumeGomez can help with the rustdoc-specific changes in here?

@GuillaumeGomez
Copy link
Member

I confirm that rustdoc changes look good. But we still need the @rust-lang/compiler team to take a look.

@bors
Copy link
Contributor

bors commented Jun 6, 2019

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

@oli-obk
Copy link
Contributor

oli-obk commented Jun 6, 2019

I'm not 100% sure how this would be used by custom drivers to overwrite the panic behavior. Maybe add some docs and/or show it in a custom driver test.

The impl lgtm

@QuietMisdreavus
Copy link
Member

AFAIK, rustdoc doesn't do anything special to handle panics when building docs, that rustc doesn't already do - there's a thread spawned in main, and the documentation process is surrounded by that call to rustc_driver that you changed.

When running doctests, though, it called inteface::run_compiler twice:

Once when collecting doctests:

let tests = interface::run_compiler(config, |compiler| -> Result<_, ErrorReported> {
let lower_to_hir = compiler.lower_to_hir()?;
let mut opts = scrape_test_config(lower_to_hir.peek().0.borrow().krate());
opts.display_warnings |= options.display_warnings;
let mut collector = Collector::new(
compiler.crate_name()?.peek().to_string(),
options.cfgs,
options.libs,
options.codegen_options,
options.externs,
false,
opts,
options.maybe_sysroot,
Some(compiler.source_map().clone()),
None,
options.linker,
options.edition,
options.persist_doctests,
);
let mut global_ctxt = compiler.global_ctxt()?.take();
global_ctxt.enter(|tcx| {
let krate = tcx.hir().krate();
let mut hir_collector = HirCollector {
sess: compiler.session(),
collector: &mut collector,
map: tcx.hir(),
codes: ErrorCodes::from(compiler.session().opts
.unstable_features.is_nightly_build()),
};
hir_collector.visit_testable("".to_string(), &krate.attrs, |this| {
intravisit::walk_crate(this, krate);
});
});
Ok(collector.tests)
}).expect("compiler aborted in rustdoc!");

And once per-doctest to compile them:

rust/src/librustdoc/test.rs

Lines 327 to 338 in 51dc52b

let compile_result = panic::catch_unwind(AssertUnwindSafe(|| {
interface::run_compiler(config, |compiler| {
if no_run {
compiler.global_ctxt().and_then(|global_ctxt| global_ctxt.take().enter(|tcx| {
tcx.analysis(LOCAL_CRATE)
})).ok();
} else {
compiler.compile().ok();
};
compiler.session().compile_status()
})
})).map_err(|_| ()).and_then(|s| s.map_err(|_| ()));

In addition, there's some manual work with parsers during make_test which is itself wrapped in a catch_unwind to deliberately mask ICEs (the assumption being that the ICE would be re-emitted later on):

rust/src/librustdoc/test.rs

Lines 202 to 212 in 51dc52b

let (test, line_offset) = match panic::catch_unwind(|| {
make_test(test, Some(cratename), as_test_harness, opts, edition)
}) {
Ok((test, line_offset)) => (test, line_offset),
Err(cause) if cause.is::<errors::FatalErrorMarker>() => {
// If the parser used by `make_test` panicked due to a fatal error, pass the test code
// through unchanged. The error will be reported during compilation.
(test.to_owned(), 0)
},
Err(cause) => panic::resume_unwind(cause),
};

I'm not sure what the desired approach is in this situation, but i or @euclio could provide more information if needed.

@Mark-Simulacrum
Copy link
Member

r? @oli-obk

@rust-highfive rust-highfive assigned oli-obk and unassigned Zoxc Jul 9, 2019
@Mark-Simulacrum Mark-Simulacrum 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 Jul 9, 2019
@joelpalmer
Copy link

Ping from Triage, @oli-obk @jonas-schievink - any update on this PR? Thanks!

@jonas-schievink
Copy link
Contributor Author

Yeah, sorry for dropping the ball on this. I'll get to this soon, I hope.

@jonas-schievink
Copy link
Contributor Author

Okay, rebased this and added docs.

@jonas-schievink jonas-schievink added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 26, 2019
@GuillaumeGomez
Copy link
Member

Just one question and then it's good to go for me.

@jonas-schievink
Copy link
Contributor Author

@bors r=oli-obk

@bors
Copy link
Contributor

bors commented Sep 14, 2019

📌 Commit dab6813 has been approved by oli-obk

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 14, 2019
@bors
Copy link
Contributor

bors commented Sep 15, 2019

⌛ Testing commit dab6813 with merge 572d3d9...

bors added a commit that referenced this pull request Sep 15, 2019
Use `panic::set_hook` to print the ICE message

This allows custom frontends and backends to override the hook with their own, for example to point people to a different issue tracker.

ICE messages are printed in a slightly different order now. Nightly prints:

```
thread 'rustc' panicked at 'called `Option::unwrap()` on a `None` value', src/libcore/option.rs:347:21
note: Run with `RUST_BACKTRACE=1` environment variable to display a backtrace.
error: aborting due to 2 previous errors

Some errors have detailed explanations: E0277, E0658.
For more information about an error, try `rustc --explain E0277`.

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/rust-lang/rust/blob/master/CONTRIBUTING.md#bug-reports

note: rustc 1.36.0-nightly (08bfe16 2019-05-02) running on x86_64-unknown-linux-gnu
```

After this PR, rustc prints:

```
thread 'rustc' panicked at 'called `Option::unwrap()` on a `None` value', src/libcore/option.rs:347:21
note: Run with `RUST_BACKTRACE=1` environment variable to display a backtrace.

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/rust-lang/rust/blob/master/CONTRIBUTING.md#bug-reports

note: rustc 1.36.0-dev running on x86_64-unknown-linux-gnu

error: aborting due to 2 previous errors

Some errors have detailed explanations: E0277, E0658.
For more information about an error, try `rustc --explain E0277`.
```
@bors
Copy link
Contributor

bors commented Sep 15, 2019

☀️ Test successful - checks-azure
Approved by: oli-obk
Pushing 572d3d9 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 15, 2019
@bors bors merged commit dab6813 into rust-lang:master Sep 15, 2019
@rust-highfive
Copy link
Collaborator

📣 Toolstate changed by #60584!

Tested on commit 572d3d9.
Direct link to PR: #60584

💔 rls on windows: test-fail → build-fail (cc @Xanewok, @rust-lang/infra).
💔 rls on linux: test-fail → build-fail (cc @Xanewok, @rust-lang/infra).

rust-highfive added a commit to rust-lang-nursery/rust-toolstate that referenced this pull request Sep 15, 2019
Tested on commit rust-lang/rust@572d3d9.
Direct link to PR: <rust-lang/rust#60584>

💔 rls on windows: test-fail → build-fail (cc @Xanewok, @rust-lang/infra).
💔 rls on linux: test-fail → build-fail (cc @Xanewok, @rust-lang/infra).
Xanewok added a commit to Xanewok/rls that referenced this pull request Sep 15, 2019
Xanewok added a commit to Xanewok/rls that referenced this pull request Sep 15, 2019
Xanewok added a commit to Xanewok/rls that referenced this pull request Sep 15, 2019
@jonas-schievink jonas-schievink deleted the ice-panic-hook branch September 15, 2019 12:11
bors added a commit to rust-lang/miri that referenced this pull request Sep 16, 2019
update for rustc changes

rust-lang/rust#60584 changed some stuff around ICEs. What I am not sure about is whether we should call `install_ice_hook` or not. @jonas-schievink @oli-obk any advice?
phansch added a commit to phansch/rust-clippy that referenced this pull request Sep 27, 2019
This utilizes rust-lang/rust#60584 by setting
our own `panic_hook` and pointing to our own issue tracker instead of
the rustc issue tracker.

This also adds a new internal lint to test the ICE message.

**Potential downsides**

* This essentially copies rustc's `report_ice` function as
  `report_clippy_ice`. I think that's how it's meant to be implemented, but
  maybe @jonas-schievink could have a look as well =)

  The downside of more-or-less copying this function is that we have to
  maintain it as well now.
  The original function can be found [here][original].
* `driver` now depends directly on `rustc` and `rustc_errors`

Closes rust-lang#2734

[original]: https://github.com/rust-lang/rust/blob/59367b074f1523353dddefa678ffe3cac9fd4e50/src/librustc_driver/lib.rs#L1185
phansch added a commit to phansch/rust-clippy that referenced this pull request Oct 8, 2019
This utilizes rust-lang/rust#60584 by setting
our own `panic_hook` and pointing to our own issue tracker instead of
the rustc issue tracker.

This also adds a new internal lint to test the ICE message.

**Potential downsides**

* This essentially copies rustc's `report_ice` function as
  `report_clippy_ice`. I think that's how it's meant to be implemented, but
  maybe @jonas-schievink could have a look as well =)

  The downside of more-or-less copying this function is that we have to
  maintain it as well now.
  The original function can be found [here][original].
* `driver` now depends directly on `rustc` and `rustc_errors`

Closes rust-lang#2734

[original]: https://github.com/rust-lang/rust/blob/59367b074f1523353dddefa678ffe3cac9fd4e50/src/librustc_driver/lib.rs#L1185
bors added a commit to rust-lang/rust-clippy that referenced this pull request Oct 9, 2019
Add custom ICE message that points to Clippy repo

changelog: Link to Clippy issue tracker in ICE messages

This utilizes rust-lang/rust#60584 by setting
our own `panic_hook` and pointing to our own issue tracker instead of
the rustc issue tracker.

This also adds a new internal lint to test the ICE message.

**Potential downsides**

* This essentially copies rustc's `report_ice` function as
  `report_clippy_ice`. I think that's how it's meant to be implemented, but
  maybe @jonas-schievink could have a look as well =)

  The downside of more-or-less copying this function is that we have to
  maintain it as well now.
  The original function can be found [here][original].
* `driver` now depends directly on `rustc` and `rustc_errors`

Closes #2734

[original]: https://github.com/rust-lang/rust/blob/59367b074f1523353dddefa678ffe3cac9fd4e50/src/librustc_driver/lib.rs#L1185
bors added a commit to rust-lang/rust-clippy that referenced this pull request Oct 9, 2019
Add custom ICE message that points to Clippy repo

changelog: Link to Clippy issue tracker in ICE messages

This utilizes rust-lang/rust#60584 by setting
our own `panic_hook` and pointing to our own issue tracker instead of
the rustc issue tracker.

This also adds a new internal lint to test the ICE message.

**Potential downsides**

* This essentially copies rustc's `report_ice` function as
  `report_clippy_ice`. I think that's how it's meant to be implemented, but
  maybe @jonas-schievink could have a look as well =)

  The downside of more-or-less copying this function is that we have to
  maintain it as well now.
  The original function can be found [here][original].
* `driver` now depends directly on `rustc` and `rustc_errors`

Closes #2734

[original]: https://github.com/rust-lang/rust/blob/59367b074f1523353dddefa678ffe3cac9fd4e50/src/librustc_driver/lib.rs#L1185
phansch added a commit to phansch/rust-clippy that referenced this pull request Nov 15, 2019
This utilizes rust-lang/rust#60584 by setting
our own `panic_hook` and pointing to our own issue tracker instead of
the rustc issue tracker.

This also adds a new internal lint to test the ICE message.

**Potential downsides**

* This essentially copies rustc's `report_ice` function as
  `report_clippy_ice`. I think that's how it's meant to be implemented, but
  maybe @jonas-schievink could have a look as well =)

  The downside of more-or-less copying this function is that we have to
  maintain it as well now.
  The original function can be found [here][original].
* `driver` now depends directly on `rustc` and `rustc_errors`

Closes rust-lang#2734

[original]: https://github.com/rust-lang/rust/blob/59367b074f1523353dddefa678ffe3cac9fd4e50/src/librustc_driver/lib.rs#L1185
phansch added a commit to phansch/rust-clippy that referenced this pull request Nov 29, 2019
This utilizes rust-lang/rust#60584 by setting
our own `panic_hook` and pointing to our own issue tracker instead of
the rustc issue tracker.

This also adds a new internal lint to test the ICE message.

**Potential downsides**

* This essentially copies rustc's `report_ice` function as
  `report_clippy_ice`. I think that's how it's meant to be implemented, but
  maybe @jonas-schievink could have a look as well =)

  The downside of more-or-less copying this function is that we have to
  maintain it as well now.
  The original function can be found [here][original].
* `driver` now depends directly on `rustc` and `rustc_errors`

Closes rust-lang#2734

[original]: https://github.com/rust-lang/rust/blob/59367b074f1523353dddefa678ffe3cac9fd4e50/src/librustc_driver/lib.rs#L1185
phansch added a commit to phansch/rust-clippy that referenced this pull request Nov 29, 2019
This utilizes rust-lang/rust#60584 by setting
our own `panic_hook` and pointing to our own issue tracker instead of
the rustc issue tracker.

This also adds a new internal lint to test the ICE message.

**Potential downsides**

* This essentially copies rustc's `report_ice` function as
  `report_clippy_ice`. I think that's how it's meant to be implemented, but
  maybe @jonas-schievink could have a look as well =)

  The downside of more-or-less copying this function is that we have to
  maintain it as well now.
  The original function can be found [here][original].
* `driver` now depends directly on `rustc` and `rustc_errors`

Closes rust-lang#2734

[original]: https://github.com/rust-lang/rust/blob/59367b074f1523353dddefa678ffe3cac9fd4e50/src/librustc_driver/lib.rs#L1185
bors added a commit to rust-lang/rust-clippy that referenced this pull request Nov 29, 2019
Add custom ICE message that points to Clippy repo

changelog: Link to Clippy issue tracker in ICE messages

This utilizes rust-lang/rust#60584 by setting
our own `panic_hook` and pointing to our own issue tracker instead of
the rustc issue tracker.

This also adds a new internal lint to test the ICE message.

**Potential downsides**

* This essentially copies rustc's `report_ice` function as
  `report_clippy_ice`. I think that's how it's meant to be implemented, but
  maybe @jonas-schievink could have a look as well =)

  The downside of more-or-less copying this function is that we have to
  maintain it as well now.
  The original function can be found [here][original].
* `driver` now depends directly on `rustc` and `rustc_errors`

Closes #2734

[original]: https://github.com/rust-lang/rust/blob/59367b074f1523353dddefa678ffe3cac9fd4e50/src/librustc_driver/lib.rs#L1185
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.