-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Stop generating code in mem::forget #79989
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 |
---|---|---|
|
@@ -141,7 +141,16 @@ pub use crate::intrinsics::transmute; | |
#[rustc_const_stable(feature = "const_forget", since = "1.46.0")] | ||
#[stable(feature = "rust1", since = "1.0.0")] | ||
pub const fn forget<T>(t: T) { | ||
let _ = ManuallyDrop::new(t); | ||
// Ideally this would just be | ||
// ``` | ||
// let _ = ManuallyDrop::new(t); | ||
// ``` | ||
// but as of 2020-12-12 that actually codegens the construction of the type, | ||
// which there's no reason to do. Please switch it back if things have changed and | ||
// the forget-is-nop codegen test confirms the intrinsic is no longer needed here. | ||
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. Should this be a FIXME pointing to the ManuallyDrop issue? |
||
|
||
// SAFETY: Forgetting is safe; it's just the intrinsic that isn't. | ||
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. FWIW, intrinsics can also be marked as safe. (But no need to do that in this PR.) |
||
unsafe { intrinsics::forget(t) } | ||
} | ||
|
||
/// Like [`forget`], but also accepts unsized values. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -137,3 +137,16 @@ fn assume_init_good() { | |
|
||
assert!(TRUE); | ||
} | ||
|
||
#[test] | ||
#[cfg(not(bootstrap))] | ||
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. The test should pass with and without bootstrap, shouldn't it? 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 would have thought so too, but https://github.com/rust-lang/rust/runs/1548179565 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. Ah, that's probably because of the missing intrinsic implementation in the bootstrap compiler. Good point. |
||
fn forget_works_in_const_fn() { | ||
const fn forget_arg_and_return_4(x: Vec<i32>) -> i32 { | ||
std::mem::forget(x); | ||
4 | ||
} | ||
|
||
const FOUR_THE_HARD_WAY: i32 = forget_arg_and_return_4(Vec::new()); | ||
|
||
assert_eq!(FOUR_THE_HARD_WAY, 4); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
// compile-flags: -C opt-level=0 | ||
|
||
#![crate_type = "lib"] | ||
|
||
// CHECK-LABEL: mem6forget{{.+}}[100 x %"std::string::String"]* | ||
// CHECK-NOT: alloca | ||
// CHECK-NOT: memcpy | ||
// CHECK: ret | ||
|
||
// CHECK-LABEL: mem6forget{{.+}}[100 x i64]* | ||
// CHECK-NOT: alloca | ||
// CHECK-NOT: memcpy | ||
// CHECK: ret | ||
|
||
pub fn forget_large_copy_type(whatever: [i64; 100]) { | ||
std::mem::forget(whatever) | ||
} | ||
|
||
pub fn forget_large_drop_type(whatever: [String; 100]) { | ||
std::mem::forget(whatever) | ||
} |
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.
Cc @rust-lang/wg-const-eval for the const stabilization here
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.
@scottmcm could you add a test making sure that this actually works at const-time? From what I can see it actually should not work since this intrinsic is not implemented... you'll probably have to copy the Miri implementation.
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.
I added a test, but it seemed to run fine? Please let me know if that's not the right way to test such a thing.
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.
@oli-obk any idea how this test can possibly pass without
forget
being implemented in this file? I am very confused right now.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 intrinsic does not even seem to be called:
The body of
std::mem::forget
seems to consist only of_0 = const ()
.Also very strange: the trace indicates two calls to
forget_arg_and_return_4
?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.
Hm, the intrinsic didn't have a body before either, so I guess there is no UB change. The intrinsic simply does not need an implementation any more.
@scottmcm you are good then, aside from the
cfg(bootstrap)
at the test which I think is unnecessary. :)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.
Turns out you actually do need to add the intrinsic implementation, as @tmiasko pointed out. It is still needed when libstd is built with
-Zmir-opt-level=0
.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.
It should be possible to test this by calling
core::intrinsics::forget
directly, instead ofcore::mem::forget
wrapper, with-Zmir-opt-level=0
,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.
I'll add the test. Looks like with #80040 it won't need the intrinsic implementation any more, though, right?
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.
Yes.