-
Notifications
You must be signed in to change notification settings - Fork 13.3k
abort immediately on bad mem::zeroed/uninit #105997
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,9 +1,9 @@ | ||
// run-pass | ||
// needs-unwind | ||
// revisions: mir thir strict | ||
// [thir]compile-flags: -Zthir-unsafeck | ||
// revisions: default strict | ||
// [strict]compile-flags: -Zstrict-init-checks | ||
// ignore-tidy-linelength | ||
// ignore-emscripten spawning processes is not supported | ||
// ignore-sgx no processes | ||
|
||
// This test checks panic emitted from `mem::{uninitialized,zeroed}`. | ||
|
||
|
@@ -12,7 +12,6 @@ | |
|
||
use std::{ | ||
mem::{self, MaybeUninit, ManuallyDrop}, | ||
panic, | ||
ptr::NonNull, | ||
num, | ||
}; | ||
|
@@ -70,21 +69,42 @@ enum ZeroIsValid { | |
} | ||
|
||
#[track_caller] | ||
fn test_panic_msg<T>(op: impl (FnOnce() -> T) + panic::UnwindSafe, msg: &str) { | ||
let err = panic::catch_unwind(op).err(); | ||
assert_eq!( | ||
err.as_ref().and_then(|a| a.downcast_ref::<&str>()), | ||
Some(&msg) | ||
); | ||
fn test_panic_msg<T, F: (FnOnce() -> T) + 'static>(op: F, msg: &str) { | ||
use std::{panic, env, process}; | ||
|
||
// The tricky part is that we can't just run `op`, as that would *abort* the process. | ||
// So instead, we reinvoke this process with the caller location as argument. | ||
// For the purpose of this test, the line number is unique enough. | ||
// If we are running in such a re-invocation, we skip all the tests *except* for the one with that type name. | ||
let our_loc = panic::Location::caller().line().to_string(); | ||
let mut args = env::args(); | ||
let this = args.next().unwrap(); | ||
if let Some(loc) = args.next() { | ||
if loc == our_loc { | ||
op(); | ||
panic!("we did not abort"); | ||
} else { | ||
// Nothing, we are running another test. | ||
} | ||
} else { | ||
// Invoke new process for actual test, and check result. | ||
let mut cmd = process::Command::new(this); | ||
cmd.arg(our_loc); | ||
let res = cmd.output().unwrap(); | ||
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); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This does make the test very slow. But that seems fine? I can't think of a better way... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You could spawn all the processes and only once that's done collect all the results. But that requires a bit more plumbing. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Or these tests could be rewritten as There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You have to use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
} | ||
|
||
#[track_caller] | ||
fn test_panic_msg_only_if_strict<T>(op: impl (FnOnce() -> T) + panic::UnwindSafe, msg: &str) { | ||
let err = panic::catch_unwind(op).err(); | ||
assert_eq!( | ||
err.as_ref().and_then(|a| a.downcast_ref::<&str>()), | ||
if cfg!(strict) { Some(&msg) } else { None }, | ||
); | ||
fn test_panic_msg_only_if_strict<T>(op: impl (FnOnce() -> T) + 'static, msg: &str) { | ||
if !cfg!(strict) { | ||
// Just run it. | ||
op(); | ||
} else { | ||
test_panic_msg(op, msg); | ||
} | ||
} | ||
|
||
fn main() { | ||
|
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.
My brain wants to parse
PanicNounwind
asPanic-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
orpanic_nounwind
.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.
The issue is that the old
PanicNoUnwind
is actually subtly different from the newPanicNounwind
, which is why I did the rename -- the old lang item is equal to what is now calledPannicCannotUnwind
.If you are okay with giving a lang item a different meaning without renaming it, I can change this to
PanicNoUnwind
.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.
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
.