abort immediately on bad mem::zeroed/uninit #105997

RalfJung · 2022-12-21T12:54:09Z

Now that we have non-unwinding panics, let's use them for these assertions. This re-establishes the property that mem::uninitialized and mem::zeroed will never unwind -- the earlier approach of causing panics here sometimes led to hard-to-debug segfaults when the surrounding code was not able to cope with the unexpected unwinding.

Cc @bjorn3 I did not touch cranelift but I assume it needs a similar patch. However it has a codegen_panic abstraction that I did not want to touch since I didn't know how else it is used.

rustbot · 2022-12-21T12:54:14Z

r? @eholk

(rustbot has picked a reviewer for you, use r? to override)

rustbot · 2022-12-21T12:54: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

RalfJung · 2022-12-21T13:01:44Z

src/test/ui/intrinsics/panic-uninitialized-zeroed.rs

+        assert!(!res.status.success(), "test did not fail");
+        let stderr = String::from_utf8_lossy(&res.stderr);
+        assert!(stderr.contains(msg), "test did not contain expected output: looking for {:?}, output:\n{}", msg, stderr);
+    }


This does make the test very slow. But that seems fine? I can't think of a better way...

You could spawn all the processes and only once that's done collect all the results. But that requires a bit more plumbing.

Or these tests could be rewritten as #[test] functions. This is also used in some other UI tests.
And lib-test spawns new processes for tests when the it's compiled with panic=abort or some unstable flag is passed. Not sure if compiletest can pass arguments to run-pass tests.

You have to use -Cpanic=abort and -Zpanic-abort-tests together when compiling the crate to be tested to have libtest spawn new processes.

Hm, yeah I guess that could work, though I would have to restructure the test quite a bit.

Let's see what the reviewer says, for now I'd prefer this approach.

I think this approach looks okay. It launches 75 processes or so, which I don't think will be too much time in the grand scheme of things. It might be worse on Windows since process creation is more expensive there though. Given that we run the whole test suite in parallel, I doubt one slower test will affect the overall time too much.

bjorn3 · 2022-12-21T13:45:08Z

Cc @bjorn3 I did not touch cranelift but I assume it needs a similar patch. However it has a codegen_panic abstraction that I did not want to touch since I didn't know how else it is used.

No problem. I think I will update it on the next sync after this PR lands.

eholk

Looks good to me. I had a minor naming nitpick, but feel free to r=me however you decide to address it!

eholk · 2022-12-22T00:54:53Z

compiler/rustc_hir/src/lang_items.rs

@@ -232,14 +232,15 @@ language_item_table! {
    // is required to define it somewhere. Additionally, there are restrictions on crates that use
    // a weak lang item, but do not have it defined.
    Panic,                   sym::panic,               panic_fn,                   Target::Fn,             GenericRequirement::Exact(0);
+    PanicNounwind,           sym::panic_nounwind,      panic_nounwind,             Target::Fn,             GenericRequirement::Exact(0);


Suggested change

PanicNounwind, sym::panic_nounwind, panic_nounwind, Target::Fn, GenericRequirement::Exact(0);

PanicNoUnwind, sym::panic_no_unwind, panic_no_unwind, Target::Fn, GenericRequirement::Exact(0);

My brain wants to parse PanicNounwind as Panic-Noun-Wind, so I'd prefer to keep the original spelling here.

In the snake case variant, I'm fine with either panic_no_unwind or panic_nounwind.

The issue is that the old PanicNoUnwind is actually subtly different from the new PanicNounwind, which is why I did the rename -- the old lang item is equal to what is now called PannicCannotUnwind.

If you are okay with giving a lang item a different meaning without renaming it, I can change this to PanicNoUnwind.

That makes sense. Changing the name makes it so everyone has to actively do something now that it's changed, which seems like a good thing. If it annoys people in the future we can always change it then, but this reasoning makes sense to leave it as PanicNounwind.

eholk · 2022-12-22T01:15:51Z

src/test/ui/intrinsics/panic-uninitialized-zeroed.rs

+        assert!(!res.status.success(), "test did not fail");
+        let stderr = String::from_utf8_lossy(&res.stderr);
+        assert!(stderr.contains(msg), "test did not contain expected output: looking for {:?}, output:\n{}", msg, stderr);
+    }


I think this approach looks okay. It launches 75 processes or so, which I don't think will be too much time in the grand scheme of things. It might be worse on Windows since process creation is more expensive there though. Given that we run the whole test suite in parallel, I doubt one slower test will affect the overall time too much.

bors · 2022-12-22T15:00:55Z

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

eholk · 2022-12-22T19:46:14Z

@bors r+

bors · 2022-12-22T19:46:16Z

📌 Commit 9f241b3 has been approved by eholk

It is now in the queue for this repository.

abort immediately on bad mem::zeroed/uninit Now that we have non-unwinding panics, let's use them for these assertions. This re-establishes the property that `mem::uninitialized` and `mem::zeroed` will never unwind -- the earlier approach of causing panics here sometimes led to hard-to-debug segfaults when the surrounding code was not able to cope with the unexpected unwinding. Cc `@bjorn3` I did not touch cranelift but I assume it needs a similar patch. However it has a `codegen_panic` abstraction that I did not want to touch since I didn't know how else it is used.

Noratrieb · 2022-12-23T17:00:02Z

@bors r-
Failed in rollup on wasm32 #106083 (comment)

RalfJung · 2022-12-24T07:45:06Z

@bors r=eholk rollup=iffy

bors · 2022-12-24T07:45:08Z

📌 Commit 26e0139 has been approved by eholk

It is now in the queue for this repository.

bors · 2022-12-24T13:36:25Z

⌛ Testing commit 26e0139 with merge 1e1d38625e227173c4d0a51a75892c8c9256ef49...

bors · 2022-12-24T13:56:45Z

💔 Test failed - checks-actions

RalfJung · 2022-12-25T14:53:29Z

@bors r=eholk

bors · 2022-12-25T14:53:31Z

📌 Commit c1b443d has been approved by eholk

It is now in the queue for this repository.

bors · 2022-12-25T20:51:41Z

⌛ Testing commit c1b443d with merge 8dfb339...

bors · 2022-12-25T23:47:05Z

☀️ Test successful - checks-actions
Approved by: eholk
Pushing 8dfb339 to master...

rust-timer · 2022-12-26T01:07:14Z

Finished benchmarking commit (8dfb339): 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)	-	-	0
Regressions ❌ (secondary)	1.6%	[1.6%, 1.6%]	1
Improvements ✅ (primary)	-0.8%	[-0.8%, -0.8%]	1
Improvements ✅ (secondary)	-4.0%	[-4.0%, -4.0%]	1
All ❌✅ (primary)	-0.8%	[-0.8%, -0.8%]	1

Cycles

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

abort immediately on bad mem::zeroed/uninit Now that we have non-unwinding panics, let's use them for these assertions. This re-establishes the property that `mem::uninitialized` and `mem::zeroed` will never unwind -- the earlier approach of causing panics here sometimes led to hard-to-debug segfaults when the surrounding code was not able to cope with the unexpected unwinding. Cc `@bjorn3` I did not touch cranelift but I assume it needs a similar patch. However it has a `codegen_panic` abstraction that I did not want to touch since I didn't know how else it is used.

rustbot assigned eholk Dec 21, 2022

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Dec 21, 2022

RalfJung commented Dec 21, 2022

View reviewed changes

This comment has been minimized.

Sign in to view

RalfJung force-pushed the immediate-abort branch from fea46a4 to d5c48ef Compare December 21, 2022 13:35

RalfJung force-pushed the immediate-abort branch 2 times, most recently from ece139a to 086be76 Compare December 21, 2022 19:14

eholk approved these changes Dec 22, 2022

View reviewed changes

RalfJung mentioned this pull request Dec 22, 2022

default OOM handler: use non-unwinding panic, to match std handler #106045

Merged

abort immediately on bad mem::zeroed/uninit

9f241b3

RalfJung force-pushed the immediate-abort branch from 086be76 to 9f241b3 Compare December 22, 2022 15:37

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 Dec 22, 2022

Noratrieb mentioned this pull request Dec 23, 2022

Rollup of 6 pull requests #106083

Closed

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Dec 23, 2022

ignore some targets

26e0139

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 Dec 24, 2022

This comment has been minimized.

Sign in to view

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Dec 24, 2022

fix codegen test

c1b443d

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 Dec 25, 2022

bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 25, 2022

bors merged commit 8dfb339 into rust-lang:master Dec 25, 2022

rustbot added this to the 1.68.0 milestone Dec 25, 2022

This was referenced Dec 25, 2022

Insert alignment checks for pointer dereferences when debug assertions are enabled #98112

Merged

Remove box_free lang item #100036

Merged

Rebased: Mark drop calls in landing pads cold instead of noinline #102099

Merged

Error in panic #103169

Closed

RalfJung deleted the immediate-abort branch December 27, 2022 11:02

RalfJung mentioned this pull request Apr 12, 2023

Unwinding intrinsics use non-unwinding ABI #104451

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

abort immediately on bad mem::zeroed/uninit #105997

abort immediately on bad mem::zeroed/uninit #105997

RalfJung commented Dec 21, 2022

rustbot commented Dec 21, 2022

rustbot commented Dec 21, 2022

RalfJung Dec 21, 2022

the8472 Dec 21, 2022

the8472 Dec 21, 2022

bjorn3 Dec 21, 2022

RalfJung Dec 21, 2022

eholk Dec 22, 2022

This comment has been minimized.

bjorn3 commented Dec 21, 2022

eholk left a comment

eholk Dec 22, 2022

RalfJung Dec 22, 2022

eholk Dec 22, 2022

eholk Dec 22, 2022

bors commented Dec 22, 2022

eholk commented Dec 22, 2022

bors commented Dec 22, 2022

Noratrieb commented Dec 23, 2022

RalfJung commented Dec 24, 2022

bors commented Dec 24, 2022

bors commented Dec 24, 2022

This comment has been minimized.

bors commented Dec 24, 2022

RalfJung commented Dec 25, 2022

bors commented Dec 25, 2022

bors commented Dec 25, 2022

bors commented Dec 25, 2022

rust-timer commented Dec 26, 2022

	PanicNounwind, sym::panic_nounwind, panic_nounwind, Target::Fn, GenericRequirement::Exact(0);
	PanicNoUnwind, sym::panic_no_unwind, panic_no_unwind, Target::Fn, GenericRequirement::Exact(0);

abort immediately on bad mem::zeroed/uninit #105997

abort immediately on bad mem::zeroed/uninit #105997

Conversation

RalfJung commented Dec 21, 2022

rustbot commented Dec 21, 2022

rustbot commented Dec 21, 2022

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

This comment has been minimized.

bjorn3 commented Dec 21, 2022

eholk left a 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

bors commented Dec 22, 2022

eholk commented Dec 22, 2022

bors commented Dec 22, 2022

Noratrieb commented Dec 23, 2022

RalfJung commented Dec 24, 2022

bors commented Dec 24, 2022

bors commented Dec 24, 2022

This comment has been minimized.

bors commented Dec 24, 2022

RalfJung commented Dec 25, 2022

bors commented Dec 25, 2022

bors commented Dec 25, 2022

bors commented Dec 25, 2022

rust-timer commented Dec 26, 2022

Overall result: no relevant changes - no action needed