nicer errors from assert_unsafe_precondition #102732

RalfJung · 2022-10-06T09:57:14Z

This makes the errors shown by cargo-careful nicer, and since panic_no_unwind is nounwind noreturn it hopefully doesn't have bad codegen impact. Thanks to @bjorn3 for the hint!

Would be nice if we could somehow supply our own (static) message to print, currently it always prints panic in a function that cannot unwind. But still, this is better than before.

rustbot · 2022-10-06T09:57:17Z

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

Stabilizing library features
Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
Changing public documentation in ways that create new stability guarantees
Changing observable runtime behavior of library APIs

rust-highfive · 2022-10-06T09:57:18Z

r? @joshtriplett

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

rustbot · 2022-10-06T10:41:47Z

The Miri subtree was changed

cc @rust-lang/miri

RalfJung · 2022-10-06T10:42:37Z

I extended this to a slight refactor of the no-unwind part of the core::panicking module, so that we can get a nicer error message.
r? @bjorn3 since this is now mostly a compiler change.

RalfJung · 2022-10-06T10:48:24Z

library/core/src/panicking.rs

+#[cfg_attr(feature = "panic_immediate_abort", inline)]
+#[cfg_attr(not(bootstrap), rustc_nounwind)]
+#[cfg_attr(bootstrap, rustc_allocator_nounwind)]
+pub fn panic_fmt_nounwind(fmt: fmt::Arguments<'_>) -> ! {


I hope I got all these attributes right. :D They are basically copied from panic_fmt.

RalfJung · 2022-10-06T12:58:41Z

Ah, looks like that codegen test wants things to be inlined when panic_fmt_nounwind is called from panic_no_unwind...

RalfJung · 2022-10-06T13:01:20Z

... but of course when we call it elsewhere we don't want it to be inlined. There's no way to force inlining for one but not all call sites, is there?

saethlin · 2022-10-06T13:44:16Z

library/core/src/intrinsics.rs

+                    // don't unwind to reduce impact on code size
+                    ::core::panicking::panic_fmt_nounwind(format_args!("unsafe precondition violated"));


If you give a moose a muffin...

Can we stringify the predicate and pass that instead?

I think I'd be in favor of letting the macro user give a string. But that's for a future PR, first this one needs to work.

RalfJung · 2022-10-06T14:27:06Z

Yeah I am out of ideas here for how to make that test pass...

RalfJung · 2022-10-07T08:35:42Z

@bjorn3 any idea how to make progress here?

bjorn3 · 2022-10-07T08:37:35Z

Maybe just accept code duplication between the two functions?

RalfJung · 2022-10-07T10:28:24Z

Is that really the beet we can do? Sounds terrible.^^

RalfJung · 2022-10-07T10:30:50Z

Cc @nikic @bbjornse who worked on this test in the past -- we're having trouble with the assembly/stack-protector/stack-protector-heuristics-effect.rs test failing and I have no idea what this is even testing or why it fails now.

nikic · 2022-10-07T12:30:01Z

I think the actual problem here is that this implementation fails the goal of reducing code size impact in the caller: You are passing in a fmt::Arguments, which is a complex structure that requires a stack allocation. What you should be doing is only pass in a &str and then translate that to the formatting machinery internally.

This is probably also how it ends up failing the stack protector test.

RalfJung · 2022-10-07T12:41:49Z

I shouldn't be passing anything if that all gets inlined. And both the code before and after this PR construct a fmt::Arguments in the end...

... so, I do not understand how this can make a difference. But I'll try your suggestion.

… panic_no_unwind

RalfJung · 2022-10-11T21:26:13Z

@bors r=bjorn3

bors · 2022-10-11T21:26:15Z

📌 Commit 38c78a9 has been approved by bjorn3

It is now in the queue for this repository.

bors · 2022-10-12T14:39:46Z

⌛ Testing commit 38c78a9 with merge 538f118...

bors · 2022-10-12T17:48:25Z

☀️ Test successful - checks-actions
Approved by: bjorn3
Pushing 538f118 to master...

rust-timer · 2022-10-12T19:07:29Z

Finished benchmarking commit (538f118): comparison URL.

Overall result: ❌ regressions - ACTION NEEDED

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean¹	range	count²
Regressions ❌ (primary)	0.8%	[0.6%, 0.9%]	6
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.8%	[0.6%, 0.9%]	6

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.9%	[2.2%, 3.3%]	4
Regressions ❌ (secondary)	2.7%	[1.3%, 4.0%]	59
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	2.9%	[2.2%, 3.3%]	4

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)	2.8%	[2.8%, 2.8%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-1.9%	[-1.9%, -1.9%]	1
Improvements ✅ (secondary)	-2.1%	[-2.1%, -2.1%]	1
All ❌✅ (primary)	0.5%	[-1.9%, 2.8%]	2

the arithmetic mean of the percent change ↩ ↩² ↩³
number of relevant changes ↩ ↩² ↩³

nnethercote · 2022-10-12T22:07:12Z

diesel has been noisy lately.

@rustbot label: +perf-regression-triaged

…n3, r=thomcc Even nicer errors from assert_unsafe_precondition For example, now running `cargo test` with this patch I get things like: ``` $ cargo +stage1 test Finished test [unoptimized + debuginfo] target(s) in 0.01s Running unittests src/lib.rs (target/debug/deps/malloc_buf-9d105ddf86862995) running 5 tests thread 'tests::test_null_buf' panicked at 'unsafe precondition violated: is_aligned_and_not_null(data) && crate::mem::size_of::<T>().saturating_mul(len) <= isize::MAX as usize', /home/ben/rust/library/core/src/slice/raw.rs:93:9 note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace thread panicked while panicking. aborting. error: test failed, to rerun pass `--lib` Caused by: process didn't exit successfully: `/tmp/malloc_buf-1.0.0/target/debug/deps/malloc_buf-9d105ddf86862995` (signal: 6, SIGABRT: process abort signal) ``` This is still not perfect, but these are better for another PR: * `stringify!` is trying to do clever pretty-printing on the `expr` inside `assert_unsafe_precondition` and can even add a newline. * It would be nice to print a bit more information about where the problem is. Perhaps this is `cfg_attr(debug_assertions, track_caller)`, or perhaps it the function name added to `Location`. cc `@RalfJung` this is what I was thinking of for rust-lang#102732 (comment)

…n3, r=thomcc Even nicer errors from assert_unsafe_precondition For example, now running `cargo test` with this patch I get things like: ``` $ cargo +stage1 test Finished test [unoptimized + debuginfo] target(s) in 0.01s Running unittests src/lib.rs (target/debug/deps/malloc_buf-9d105ddf86862995) running 5 tests thread 'tests::test_null_buf' panicked at 'unsafe precondition violated: is_aligned_and_not_null(data) && crate::mem::size_of::<T>().saturating_mul(len) <= isize::MAX as usize', /home/ben/rust/library/core/src/slice/raw.rs:93:9 note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace thread panicked while panicking. aborting. error: test failed, to rerun pass `--lib` Caused by: process didn't exit successfully: `/tmp/malloc_buf-1.0.0/target/debug/deps/malloc_buf-9d105ddf86862995` (signal: 6, SIGABRT: process abort signal) ``` This is still not perfect, but these are better for another PR: * `stringify!` is trying to do clever pretty-printing on the `expr` inside `assert_unsafe_precondition` and can even add a newline. * It would be nice to print a bit more information about where the problem is. Perhaps this is `cfg_attr(debug_assertions, track_caller)`, or perhaps it the function name added to `Location`. cc ``@RalfJung`` this is what I was thinking of for rust-lang#102732 (comment)

…, r=bjorn3 nicer errors from assert_unsafe_precondition This makes the errors shown by cargo-careful nicer, and since `panic_no_unwind` is `nounwind noreturn` it hopefully doesn't have bad codegen impact. Thanks to `@bjorn3` for the hint! Would be nice if we could somehow supply our own (static) message to print, currently it always prints `panic in a function that cannot unwind`. But still, this is better than before.

Even nicer errors from assert_unsafe_precondition For example, now running `cargo test` with this patch I get things like: ``` $ cargo +stage1 test Finished test [unoptimized + debuginfo] target(s) in 0.01s Running unittests src/lib.rs (target/debug/deps/malloc_buf-9d105ddf86862995) running 5 tests thread 'tests::test_null_buf' panicked at 'unsafe precondition violated: is_aligned_and_not_null(data) && crate::mem::size_of::<T>().saturating_mul(len) <= isize::MAX as usize', /home/ben/rust/library/core/src/slice/raw.rs:93:9 note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace thread panicked while panicking. aborting. error: test failed, to rerun pass `--lib` Caused by: process didn't exit successfully: `/tmp/malloc_buf-1.0.0/target/debug/deps/malloc_buf-9d105ddf86862995` (signal: 6, SIGABRT: process abort signal) ``` This is still not perfect, but these are better for another PR: * `stringify!` is trying to do clever pretty-printing on the `expr` inside `assert_unsafe_precondition` and can even add a newline. * It would be nice to print a bit more information about where the problem is. Perhaps this is `cfg_attr(debug_assertions, track_caller)`, or perhaps it the function name added to `Location`. cc ``@RalfJung`` this is what I was thinking of for rust-lang/rust#102732 (comment)

rustbot added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Oct 6, 2022

rust-highfive assigned joshtriplett Oct 6, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 6, 2022

RalfJung closed this Oct 6, 2022

RalfJung reopened this Oct 6, 2022

RalfJung force-pushed the assert_unsafe_precondition2 branch from 04ac775 to e4a8e33 Compare October 6, 2022 10:41

rust-highfive assigned bjorn3 and unassigned joshtriplett Oct 6, 2022

RalfJung commented Oct 6, 2022

View reviewed changes

RalfJung closed this Oct 6, 2022

RalfJung reopened this Oct 6, 2022

This comment has been minimized.

# to view

RalfJung force-pushed the assert_unsafe_precondition2 branch from e4a8e33 to d772a6d Compare October 6, 2022 13:11

This comment has been minimized.

# to view

RalfJung force-pushed the assert_unsafe_precondition2 branch from d772a6d to 663b6ba Compare October 6, 2022 13:38

saethlin reviewed Oct 6, 2022

View reviewed changes

This comment has been minimized.

# to view

bors added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Oct 10, 2022

RalfJung added 4 commits October 11, 2022 22:47

rename rustc_allocator_nounwind to rustc_nounwind

2b50cd1

add panic_fmt_nounwind for panicing without unwinding, and use it for…

66282cb

… panic_no_unwind

use panic_fmt_nounwind for assert_unsafe_precondition

b61e742

reorder panicking.rs to put main entry points at the top

38c78a9

RalfJung force-pushed the assert_unsafe_precondition2 branch from 604ddf3 to 38c78a9 Compare October 11, 2022 20:47

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 Oct 11, 2022

bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 12, 2022

bors merged commit 538f118 into rust-lang:master Oct 12, 2022

rustbot added this to the 1.66.0 milestone Oct 12, 2022

This was referenced Oct 12, 2022

Disable unwinding for catch_unwind error payloads. #99032

Closed

Refactor unwind in MIR #102906

Merged

rustbot added the perf-regression Performance regression. label Oct 12, 2022

rustbot added the perf-regression-triaged The performance regression has been triaged. label Oct 12, 2022

saethlin mentioned this pull request Oct 14, 2022

Even nicer errors from assert_unsafe_precondition #103035

Merged

RalfJung deleted the assert_unsafe_precondition2 branch October 14, 2022 06:23

saethlin mentioned this pull request Feb 9, 2024

Totally-not-a-tracking-issue for UB-detecting debug assertions in the standard library #120848

Open

9 tasks

saethlin mentioned this pull request Aug 27, 2024

Add some track_caller info to precondition panics #129658

Open

		// don't unwind to reduce impact on code size
		::core::panicking::panic_fmt_nounwind(format_args!("unsafe precondition violated"));

nicer errors from assert_unsafe_precondition #102732

nicer errors from assert_unsafe_precondition #102732

Uh oh!

Conversation

RalfJung commented Oct 6, 2022

Uh oh!

rustbot commented Oct 6, 2022

Uh oh!

rust-highfive commented Oct 6, 2022

Uh oh!

rustbot commented Oct 6, 2022

Uh oh!

RalfJung commented Oct 6, 2022

Uh oh!

RalfJung Oct 6, 2022

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

RalfJung commented Oct 6, 2022

Uh oh!

RalfJung commented Oct 6, 2022

Uh oh!

This comment has been minimized.

saethlin Oct 6, 2022

Choose a reason for hiding this comment

Uh oh!

RalfJung Oct 6, 2022

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

RalfJung commented Oct 6, 2022

Uh oh!

RalfJung commented Oct 7, 2022

Uh oh!

bjorn3 commented Oct 7, 2022

Uh oh!

RalfJung commented Oct 7, 2022

Uh oh!

RalfJung commented Oct 7, 2022

Uh oh!

nikic commented Oct 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RalfJung commented Oct 7, 2022

Uh oh!

RalfJung commented Oct 11, 2022

Uh oh!

bors commented Oct 11, 2022

Uh oh!

bors commented Oct 12, 2022

Uh oh!

bors commented Oct 12, 2022

Uh oh!

rust-timer commented Oct 12, 2022

Overall result: ❌ regressions - ACTION NEEDED

Instruction count

Max RSS (memory usage)

Cycles

Footnotes

Uh oh!

nnethercote commented Oct 12, 2022

Uh oh!

Uh oh!

nikic commented Oct 7, 2022 •

edited

Loading