Skip to content

Conversation

saethlin
Copy link
Member

This panic already never unwinds, but that's only because it always hits the unwind guard that's created by our UnwindAction::Terminate. Hitting the unwind guard generates a huge double-panic backtrace. Now we generate a normal-looking panic message when this check is hit.

r? @thomcc

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jun 13, 2023
@@ -171,9 +171,9 @@ fn panic_misaligned_pointer_dereference(required: usize, found: usize) -> ! {
super::intrinsics::abort()
}

panic!(
panic_nounwind_fmt(format_args!(
Copy link
Member

Choose a reason for hiding this comment

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

We should also make this function #[rustc_nounwind] and document that this function MUST NOT unwind.

I wouldn't worry about non-local dependency that you mentioned at #112403 (comment), since this is a lang item and it's expected that lang item may need special guarantees. A #[rustc_nounwind] just near the lang item attribute with a comment should be clear enough to prevent mistakes.

@rustbot
Copy link
Collaborator

rustbot commented Jun 14, 2023

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

@bors
Copy link
Collaborator

bors commented Jun 16, 2023

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

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@thomcc
Copy link
Member

thomcc commented Jun 17, 2023

This looks fine to me.

@bors r+

@bors
Copy link
Collaborator

bors commented Jun 17, 2023

📌 Commit 7a2490e has been approved by thomcc

It is now in the queue for this repository.

@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-review Status: Awaiting review from the assignee but also interested parties. labels Jun 17, 2023
compiler-errors added a commit to compiler-errors/rust that referenced this pull request Jun 18, 2023
Launch a non-unwinding panic for misaligned pointer deref

This panic already never unwinds, but that's only because it always hits the unwind guard that's created by our `UnwindAction::Terminate`. Hitting the unwind guard generates a huge double-panic backtrace. Now we generate a normal-looking panic message when this check is hit.

r? `@thomcc`
@bors
Copy link
Collaborator

bors commented Jun 18, 2023

⌛ Testing commit 7a2490e with merge 0c2c243...

@bors
Copy link
Collaborator

bors commented Jun 18, 2023

☀️ Test successful - checks-actions
Approved by: thomcc
Pushing 0c2c243 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jun 18, 2023
@bors bors merged commit 0c2c243 into rust-lang:master Jun 18, 2023
@rustbot rustbot added this to the 1.72.0 milestone Jun 18, 2023
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (0c2c243): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

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

mean range count
Regressions ❌
(primary)
2.2% [1.2%, 3.2%] 3
Regressions ❌
(secondary)
2.7% [2.7%, 2.7%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-3.8% [-3.8%, -3.8%] 1
All ❌✅ (primary) 2.2% [1.2%, 3.2%] 3

Cycles

Results

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

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-1.1% [-1.1%, -1.1%] 1
Improvements ✅
(secondary)
-1.3% [-1.3%, -1.3%] 1
All ❌✅ (primary) -1.1% [-1.1%, -1.1%] 1

Binary size

Results

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

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-0.1% [-0.2%, -0.0%] 30
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -0.1% [-0.2%, -0.0%] 30

Bootstrap: 655.45s -> 656.801s (0.21%)

} else {
UnwindAction::Unreachable
},
// The panic symbol that this calls is #[rustc_nounwind]. We never want to insert an
Copy link
Member

Choose a reason for hiding this comment

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

Would be good to mention panic_misaligned_pointer_dereference here so one can grep for what that symbol is. (I was not aware/forgot that different AssertKind dispatch to completely different code on assertion failure.)

@saethlin saethlin deleted the cleaner-panics branch June 27, 2023 02:22
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 27, 2023
…fJung

Mention the panic function in CheckAlignment

Per rust-lang#112599 (comment)
r? `@RalfJung`
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-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants