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

bjorn3 · 2021-05-24T16:57:08Z

cc https://github.com/bjorn3/rustc_codegen_cranelift/issues/1174

rust-highfive · 2021-05-24T16:57:10Z

Some changes occured to rustc_codegen_cranelift

cc @bjorn3

rust-highfive · 2021-05-24T16:57:12Z

r? @jackh726

(rust-highfive has picked a reviewer for you, use r? to override)

jackh726 · 2021-05-24T17:05:49Z

r? @bjorn3

bjorn3 · 2021-05-24T17:08:12Z

I opened this PR, so I probably shouldn't review it myself. :)

r? @jackh726

jackh726 · 2021-05-24T17:25:03Z

🤦‍♀️ whoops

jackh726

Honestly, I'm not sure how I feel about this. I feel like most ICEs happen in the frontend, not backend. So usually reporting to rust-lang/rust is a better idea. So maybe instead it's better to just include some extra information in the ICE report?

I'm probably not the best person to review this, since I'm not really in the loop here.

jackh726 · 2021-05-24T17:32:37Z

compiler/rustc_driver/src/lib.rs

-    // 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!();
-


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

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.

bjorn3 · 2021-05-24T17:44:15Z

I feel like most ICEs happen in the frontend, not backend. So usually reporting to rust-lang/rust is a better idea.

While this is true for the mature cg_llvm backend, this is not true for cg_clif. I do still sometimes get an ice caused by cg_clif. If the ice is in the frontend, I can redirect the issue to rust-lang/rust. In addition when using cg_clif, it is reasonable for someone to assume that cg_clif caused the issue if it doesn't reproduce with cg_llvm.

jackh726 · 2021-06-01T02:38:34Z

I don't feel comfortable approving this myself (not necessarily because these changes are technically hard, but because this is really a decision that potentially impacts e.g. future codegen backends). Maybe if someone from @rust-lang/compiler wants to weigh in or an MCP, that might be better.

nikomatsakis · 2021-06-03T14:58:15Z

I think an MCP would be a good idea here, but I approve the general concept.

bjorn3 · 2021-06-03T16:44:21Z

I think an MCP would be a good idea here

Opened rust-lang/compiler-team#436

oli-obk · 2021-06-03T16:48:23Z

cc @rust-lang/clippy we may want this, too

flip1995 · 2021-06-03T20:57:45Z

Clippy already has this since 2019 👍

rust/src/tools/clippy/src/driver.rs

Lines 155 to 159 in 2577825

    
           static ICE_HOOK: SyncLazy<Box<dyn Fn(&panic::PanicInfo<'_>) + Sync + Send + 'static>> = SyncLazy::new(|| { 
        
               let hook = panic::take_hook(); 
        
               panic::set_hook(Box::new(|info| report_clippy_ice(info, BUG_REPORT_URL))); 
        
               hook 
        
           });

jackh726 · 2021-06-24T14:57:57Z

MCP is finished

@bors r+ rollup

bors · 2021-06-24T14:57:58Z

📌 Commit 0bc60c0 has been approved by jackh726

Allow changing the bug report url for the ice hook cc https://github.com/bjorn3/rustc_codegen_cranelift/issues/1174

bors · 2021-06-25T04:06:36Z

⌛ Testing commit 0bc60c0 with merge 50a9081...

bors · 2021-06-25T06:47:24Z

☀️ Test successful - checks-actions
Approved by: jackh726
Pushing 50a9081 to master...

Allow changing the bug report url for the ice hook cc https://github.com/bjorn3/rustc_codegen_cranelift/issues/1174

eddyb · 2021-08-02T09:45:08Z

compiler/rustc_driver/src/lib.rs

-        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);
+        }));


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).

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.

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.

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.

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.

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!

bjorn3 added 2 commits May 24, 2021 18:47

Don't invoke the default panic hook from report_ice

048fe53

Change the ice hook for cg_clif to refer to cg_clif's issue tracker

0bc60c0

bjorn3 added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label May 24, 2021

rust-highfive assigned jackh726 May 24, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 24, 2021

bjorn3 mentioned this pull request May 24, 2021

Unhelful error message when proc macro fails rust-lang/rustc_codegen_cranelift#1174

Closed

rust-highfive assigned bjorn3 and unassigned jackh726 May 24, 2021

rust-highfive assigned jackh726 and unassigned bjorn3 May 24, 2021

jackh726 reviewed May 24, 2021

View reviewed changes

bjorn3 mentioned this pull request Jun 3, 2021

Allow changing the bug report url for the ice hook rust-lang/compiler-team#436

Closed

3 tasks

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-review Status: Awaiting review from the assignee but also interested parties. labels Jun 24, 2021

m-ou-se added a commit to m-ou-se/rust that referenced this pull request Jun 24, 2021

Rollup merge of rust-lang#85640 - bjorn3:custom_ice_hook, r=jackh726

5c18ed3

Allow changing the bug report url for the ice hook cc https://github.com/bjorn3/rustc_codegen_cranelift/issues/1174

m-ou-se mentioned this pull request Jun 24, 2021

Rollup of 6 pull requests #86601

Closed

bors added the merged-by-bors This PR was explicitly merged by bors. label Jun 25, 2021

bors merged commit 50a9081 into rust-lang:master Jun 25, 2021

rustbot added this to the 1.55.0 milestone Jun 25, 2021

bjorn3 pushed a commit to bjorn3/rust that referenced this pull request Jul 7, 2021

Auto merge of rust-lang#85640 - bjorn3:custom_ice_hook, r=jackh726

e475efb

Allow changing the bug report url for the ice hook cc https://github.com/bjorn3/rustc_codegen_cranelift/issues/1174

eddyb reviewed Aug 2, 2021

View reviewed changes

bjorn3 deleted the custom_ice_hook branch August 2, 2021 09:53

eddyb mentioned this pull request Aug 19, 2021

Get the default panic hook for ICEs, from std::panic::set_hook. EmbarkStudios/rust-gpu#732

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

bjorn3 commented May 24, 2021

rust-highfive commented May 24, 2021

rust-highfive commented May 24, 2021

jackh726 commented May 24, 2021

bjorn3 commented May 24, 2021

jackh726 commented May 24, 2021

jackh726 left a comment

jackh726 May 24, 2021

bjorn3 May 24, 2021

bjorn3 commented May 24, 2021

jackh726 commented Jun 1, 2021 •

edited

Loading

nikomatsakis commented Jun 3, 2021

bjorn3 commented Jun 3, 2021

oli-obk commented Jun 3, 2021

flip1995 commented Jun 3, 2021

jackh726 commented Jun 24, 2021

bors commented Jun 24, 2021

bors commented Jun 25, 2021

bors commented Jun 25, 2021

eddyb Aug 2, 2021

bjorn3 Aug 2, 2021

bjorn3 Aug 2, 2021

eddyb Aug 13, 2021

bjorn3 Aug 13, 2021

eddyb Aug 19, 2021

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

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

Conversation

bjorn3 commented May 24, 2021

rust-highfive commented May 24, 2021

rust-highfive commented May 24, 2021

jackh726 commented May 24, 2021

bjorn3 commented May 24, 2021

jackh726 commented May 24, 2021

jackh726 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bjorn3 commented May 24, 2021

jackh726 commented Jun 1, 2021 • edited Loading

nikomatsakis commented Jun 3, 2021

bjorn3 commented Jun 3, 2021

oli-obk commented Jun 3, 2021

flip1995 commented Jun 3, 2021

jackh726 commented Jun 24, 2021

bors commented Jun 24, 2021

bors commented Jun 25, 2021

bors commented Jun 25, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jackh726 commented Jun 1, 2021 •

edited

Loading