Skip to content

Allow changing the bug report url for the ice hook #85640

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Jun 25, 2021
Merged

Allow changing the bug report url for the ice hook #85640

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
048fe53
Don't invoke the default panic hook from report_ice
bjorn3 May 24, 2021
0bc60c0
Change the ice hook for cg_clif to refer to cg_clif's issue tracker
bjorn3 May 24, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 23 additions & 2 deletions compiler/rustc_codegen_cranelift/src/bin/cg_clif.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,17 +1,38 @@
#![feature(rustc_private)]
#![feature(rustc_private, once_cell)]

extern crate rustc_data_structures;
extern crate rustc_driver;
extern crate rustc_interface;
extern crate rustc_session;
extern crate rustc_target;

use std::panic;
use std::lazy::SyncLazy;

use rustc_data_structures::profiling::{get_resident_set_size, print_time_passes_entry};
use rustc_interface::interface;
use rustc_session::config::ErrorOutputType;
use rustc_session::early_error;
use rustc_target::spec::PanicStrategy;

const BUG_REPORT_URL: &str = "https://github.com/bjorn3/rustc_codegen_cranelift/issues/new";

static DEFAULT_HOOK: SyncLazy<Box<dyn Fn(&panic::PanicInfo<'_>) + Sync + Send + 'static>> =
SyncLazy::new(|| {
let hook = panic::take_hook();
panic::set_hook(Box::new(|info| {
// Invoke the default handler, which prints the actual panic message and optionally a backtrace
(*DEFAULT_HOOK)(info);

// Separate the output with an empty line
eprintln!();

// Print the ICE message
rustc_driver::report_ice(info, BUG_REPORT_URL);
}));
hook
});

#[derive(Default)]
pub struct CraneliftPassesCallbacks {
time_passes: bool,
Expand All @@ -37,7 +58,7 @@ fn main() {
let start_rss = get_resident_set_size();
rustc_driver::init_rustc_env_logger();
let mut callbacks = CraneliftPassesCallbacks::default();
rustc_driver::install_ice_hook();
SyncLazy::force(&DEFAULT_HOOK); // Install ice hook
let exit_code = rustc_driver::catch_with_exit_code(|| {
let args = std::env::args_os()
.enumerate()
Expand Down
19 changes: 11 additions & 8 deletions compiler/rustc_driver/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1151,23 +1151,26 @@ pub fn catch_with_exit_code(f: impl FnOnce() -> interface::Result<()>) -> i32 {
static DEFAULT_HOOK: SyncLazy<Box<dyn Fn(&panic::PanicInfo<'_>) + Sync + Send + 'static>> =
SyncLazy::new(|| {
let hook = panic::take_hook();
panic::set_hook(Box::new(|info| report_ice(info, BUG_REPORT_URL)));
panic::set_hook(Box::new(|info| {
// Invoke the default handler, which prints the actual panic message and optionally a backtrace
(*DEFAULT_HOOK)(info);

// Separate the output with an empty line
eprintln!();

// Print the ICE message
report_ice(info, BUG_REPORT_URL);
}));
Comment on lines -1154 to +1163
Copy link
Member

Choose a reason for hiding this comment

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

This makes report_ice useless for the situations without enough control over the driver to bypass the SyncLazy::force(&DEFAULT_HOOK);. Maybe there should be a report_ice_including_default_panic_hook function that provides the old behavior?

Though ideally the new behavior should probably use a new function name (or better, both should change names, so use sites have to pick).

Copy link
Member Author

Choose a reason for hiding this comment

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

You can set a panic hook more than once, so you can first get the default hook, then let whatever code forces DEFAULT_HOOK run and then set the new panic hook.

Copy link
Member Author

Choose a reason for hiding this comment

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

A report_ice_including_default_panic_hook function requires the DEFAULT_HOOK to be forced first to be able to call it without ending in an infinite loop.

Copy link
Member

Choose a reason for hiding this comment

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

so you can first get the default hook

The codegen backend is loaded after DEFAULT_HOOK is initially forced, so it's impossible to make it work without changes to rustc_driver. I'll hopefully take a look at it soon and try to come up with a better overall solution. It should be possible to e.g. capture more data into the boxed closure, and the use of a static is somewhat incidental.

Though the most direct solution is to offer a static that contains the report URL (and maybe additional e.g. footer), and which can be changed independently of the panic hook.

Copy link
Member Author

Choose a reason for hiding this comment

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

You could have something like https://github.com/bjorn3/rustc_codegen_cranelift/blob/d498e6d6971575714353f11aa4d3e63a9d2030b2/src/bin/cg_clif.rs#L20-L34 except that you call panic::take_hook() twice and use the result of the second time as default hook. The first time you take the hook set by rustc_driver and ignore it. The second time you take the default libstd hook.

Copy link
Member

Choose a reason for hiding this comment

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

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

Ah, this behavior documented for std::panic::take_hook was what I missed, thanks!

hook
});

/// Prints the ICE message, including backtrace and query stack.
/// Prints the ICE message, including query stack, but without backtrace.
///
/// The message will point the user at `bug_report_url` to report the ICE.
///
/// When `install_ice_hook` is called, this function will be called as the panic
/// hook.
pub fn report_ice(info: &panic::PanicInfo<'_>, bug_report_url: &str) {
// Invoke the default handler, which prints the actual panic message and optionally a backtrace
(*DEFAULT_HOOK)(info);

// Separate the output with an empty line
eprintln!();

Comment on lines -1165 to -1170
Copy link
Member

Choose a reason for hiding this comment

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

Why did this need to be taken out? I guess maybe so the right DEFAULT_HOOK gets registered?

Copy link
Member Author

Choose a reason for hiding this comment

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

DEFAULT_HOOK both gets the default hook and registers the new ice hook with the default bug report url. This means that if you register a custom ice hook that calls report_ice, the ice hook with the default bug report url will be registered again. It is also easy to get into a cycle where the default ice hook and the custom ice hook recursively call each other on panics.

let emitter = Box::new(rustc_errors::emitter::EmitterWriter::stderr(
rustc_errors::ColorConfig::Auto,
None,
Expand Down