-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
when terminating during unwinding, show the reason why #115045
Conversation
r? @davidtwco (rustbot has picked a reviewer for you, use r? to override) |
Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt This PR changes Stable MIR cc @oli-obk, @celinval, @spastorino Some changes occurred in compiler/rustc_codegen_cranelift cc @bjorn3 This PR changes MIR cc @oli-obk, @RalfJung, @JakobDegen, @davidtwco, @celinval, @vakaras Some changes occurred to the CTFE / Miri engine cc @rust-lang/miri The Miri subtree was changed cc @rust-lang/miri |
This comment has been minimized.
This comment has been minimized.
bea4414
to
444d1d7
Compare
This comment has been minimized.
This comment has been minimized.
444d1d7
to
9b33256
Compare
Some changes occurred in src/tools/clippy cc @rust-lang/clippy |
@@ -781,7 +781,7 @@ impl<'tcx> Stable<'tcx> for mir::Terminator<'tcx> { | |||
otherwise: targets.otherwise().as_usize(), | |||
}, | |||
UnwindResume => Terminator::Resume, | |||
UnwindTerminate => Terminator::Abort, | |||
UnwindTerminate(_) => Terminator::Abort, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This means the reason
information is just lost in smir. I wasn't sure what the policy was wrt updating smir when MIR changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @rust-lang/project-stable-mir
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replied to the review comments you've left, this otherwise looks good to me.
Today in things I didn't know I needed... |
b6862c6
to
22784b2
Compare
Let's see if this change in the cache has any measurable perf impact... |
This comment has been minimized.
This comment has been minimized.
⌛ Trying commit 22784b2746c6e18b10bd5e6c2b99a11b4fba5143 with merge 58249f74573a1aa9babf16161f0fa46b3a6fc563... |
☀️ Try build successful - checks-actions |
This comment has been minimized.
This comment has been minimized.
Finished benchmarking commit (58249f74573a1aa9babf16161f0fa46b3a6fc563): comparison URL. Overall result: ❌ regressions - no action neededBenchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf. @bors rollup=never Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)ResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesThis benchmark run did not return any relevant results for this metric. Binary sizeResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Bootstrap: 633.956s -> 635.882s (0.30%) |
Why is it always the CTFE stress test? 🙈 Seriously though, this time I really don't get why CTFE would be impacted... I think we can conclude though that this does not have negative impacts of codegen perf in general. |
d5a4fa9
to
0bbd1fa
Compare
So |
8c16d0e
to
5804c1f
Compare
@bors r=davidtwco |
📌 Commit 5804c1f674d4141538497062db5be13d21f256ed has been approved by It is now in the queue for this repository. |
⌛ Testing commit 5804c1f674d4141538497062db5be13d21f256ed with merge 2e4ec0bb3dd8f60638fd2e89c4477e3acc516717... |
💔 Test failed - checks-actions |
5804c1f
to
df5a248
Compare
@bors r=davidtwco |
☀️ Test successful - checks-actions |
1 similar comment
☀️ Test successful - checks-actions |
Finished benchmarking commit (b60f7b5): comparison URL. Overall result: ❌ regressions - no action needed@rustbot label: -perf-regression Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)ResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Binary sizeResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Bootstrap: 630.829s -> 631.944s (0.18%) |
With this, the output on double-panic becomes something like that:
If we also land #115020, the 2nd backtrace disappears, hopefully making the "panic in a destructor during cleanup" easier to see.
Fixes #114954.