abort immediately on bad mem::zeroed/uninit by RalfJung · Pull Request #105997 ·...
source link: https://github.com/rust-lang/rust/pull/105997
Go to the source link to view the article. You can view the picture content, updated content and better typesetting reading experience. If the link is broken, please click the button below to view the snapshot at that time.
abort immediately on bad mem::zeroed/uninit #105997
Conversation
Member
RalfJung
commented
Dec 21, 2022
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.
Collaborator
rustbot commented Dec 21, 2022
|
r? @eholk (rustbot has picked a reviewer for you, use r? to override) |
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
Collaborator
rustbot commented Dec 21, 2022
|
Hey! It looks like you've submitted a new PR for the library teams! If this PR contains changes to any Examples of
|
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); |
||
} |
Member
Author
RalfJung
Dec 21, 2022
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.
Contributor
bjorn3
Dec 21, 2022
You have to use -Cpanic=abort and -Zpanic-abort-tests together when compiling the crate to be tested to have libtest spawn new processes.
Member
Author
RalfJung
Dec 21, 2022
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.
Contributor
eholk
Dec 22, 2022
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.
This comment has been minimized.
Contributor
bjorn3 commented Dec 21, 2022
No problem. I think I will update it on the next sync after this PR lands. |
Contributor
eholk
left a comment
Looks good to me. I had a minor naming nitpick, but feel free to r=me however you decide to address it!
| @@ -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); |
Contributor
eholk
Dec 22, 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); |
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.
Member
Author
RalfJung
Dec 22, 2022
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.
Contributor
eholk
Dec 22, 2022
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.
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); |
||
} |
Contributor
eholk
Dec 22, 2022
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.
Contributor
bors commented Dec 22, 2022
|
|
The latest upstream changes (presumably Contributor
eholk commented Dec 22, 2022
|
@bors r+ |
Contributor
bors commented Dec 22, 2022
Commit 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
Member
Nilstrieb commented Dec 23, 2022
|
@bors r- |
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
Member
Author
RalfJung commented Dec 24, 2022
|
@bors r=eholk rollup=iffy |
Contributor
bors commented Dec 24, 2022
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
Contributor
bors commented Dec 24, 2022
Testing commit This comment has been minimized.
Contributor
bors commented Dec 24, 2022
|
|
Test failed - 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
Member
Author
RalfJung commented Dec 25, 2022
|
@bors r=eholk |
Contributor
bors commented Dec 25, 2022
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
Contributor
bors commented Dec 25, 2022
Contributor
bors commented Dec 25, 2022
|
|
Test successful - Collaborator
rust-timer commented Dec 26, 2022
|
Finished benchmarking commit (8dfb339): comparison URL. Overall result: no relevant changes - no action needed@rustbot label: -perf-regression Instruction countThis benchmark run did not return any relevant results for this metric. Max RSS (memory usage)Results CyclesThis benchmark run did not return any relevant results for this metric. |
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
None yet
Successfully merging this pull request may close these issues.
None yet
Recommend
About Joyk
Aggregate valuable and interesting links.
Joyk means Joy of geeK