Auto merge of #148190 - RalfJung:box_new, r=RalfJung
replace box_new with lower-level intrinsics The `box_new` intrinsic is super special: during THIR construction it turns into an `ExprKind::Box` (formerly known as the `box` keyword), which then during MIR building turns into a special instruction sequence that invokes the exchange_malloc lang item (which has a name from a different time) and a special MIR statement to represent a shallowly-initialized `Box` (which raises [interesting opsem questions](https://github.com/rust-lang/rust/issues/97270)). This PR is the n-th attempt to get rid of `box_new`. That's non-trivial because it usually causes a perf regression: replacing `box_new` by naive unsafe code will incur extra copies in debug builds, making the resulting binaries a lot slower, and will generate a lot more MIR, making compilation measurably slower. Furthermore, `vec!` is a macro, so the exact code it expands to is highly relevant for borrow checking, type inference, and temporary scopes. To avoid those problems, this PR does its best to make the MIR almost exactly the same as what it was before. `box_new` is used in two places, `Box::new` and `vec!`: - For `Box::new` that is fairly easy: the `move_by_value` intrinsic is basically all we need. However, to avoid the extra copy that would usually be generated for the argument of a function call, we need to special-case this intrinsic during MIR building. That's what the first commit does. - `vec!` is a lot more tricky. As a macro, its details leak to stable code, so almost every variant I tried broke either type inference or the lifetimes of temporaries in some ui test or ended up accepting unsound code due to the borrow checker not enforcing all the constraints I hoped it would enforce. I ended up with a variant that involves a new intrinsic, `fn write_box_via_move<T>(b: Box<MaybeUninit<T>>, x: T) -> Box<MaybeUninit<T>>`, that writes a value into a `Box<MaybeUninit<T>>` and returns that box again. In exchange we can get rid of somewhat similar code in the lowering for `ExprKind::Box`, and the `exchange_malloc` lang item. (We can also get rid of `Rvalue::ShallowInitBox`; I didn't include that in this PR -- I think @cjgillot has a commit for this somewhere [around here](https://github.com/rust-lang/rust/pull/147862/commits).) See [here](https://github.com/rust-lang/rust/pull/148190#issuecomment-3457454814) for the latest perf numbers. Most of the regressions are in deep-vector which consists entirely of an invocation of `vec!`, so any change to that macro affects this benchmark disproportionally. This is my first time even looking at MIR building code, so I am very low confidence in that part of the patch, in particular when it comes to scopes and drops and things like that. I also had do nerf some clippy tests because clippy gets confused by the new expansion of `vec!` so it makes fewer suggestions when `vec!` is involved. ### `vec!` FAQ - Why does `write_box_via_move` return the `Box` again? Because we need to expand `vec!` to a bunch of method invocations without any blocks or let-statements, or else the temporary scopes (and type inference) don't work out. - Why is `box_assume_init_into_vec_unsafe` (unsoundly!) a safe function? Because we can't use an unsafe block in `vec!` as that would necessarily also include the `$x` (due to it all being one big method invocation) and therefore interpret the user's code as being inside `unsafe`, which would be bad (and 10 years later, we still don't have safe blocks for macros like this). - Why does `write_box_via_move` use `Box` as input/output type, and not, say, raw pointers? Because that is the only way to get the correct behavior when `$x` panics or has control effects: we need the `Box` to be dropped in that case. (As a nice side-effect this also makes the intrinsic safe, which is imported as explained in the previous bullet.) - Can't we make it safe by having `write_box_via_move` return `Box<T>`? Yes we could, but there's no easy way for the intrinsic to convert its `Box<MaybeUninit<T>>` to a `Box<T>`. Transmuting would be unsound as the borrow checker would no longer properly enforce that lifetimes involved in a `vec!` invocation behave correctly. - Is this macro truly cursed? Yes, yes it is.
This commit is contained in:
bors
commit
3c9faa0d03
100 changed files with 1256 additions and 1508 deletions
|
|
@ -297,12 +297,12 @@ impl<'a> VecArgs<'a> {
|
|||
// `vec![elem; size]` case
|
||||
Some(VecArgs::Repeat(elem, size))
|
||||
},
|
||||
(sym::slice_into_vec, [slice])
|
||||
if let ExprKind::Call(_, [arg]) = slice.kind
|
||||
&& let ExprKind::Array(args) = arg.kind =>
|
||||
(sym::box_assume_init_into_vec_unsafe, [write_box_via_move])
|
||||
if let ExprKind::Call(_, [_box, elems]) = write_box_via_move.kind
|
||||
&& let ExprKind::Array(elems) = elems.kind =>
|
||||
{
|
||||
// `vec![a, b, c]` case
|
||||
Some(VecArgs::Vec(args))
|
||||
Some(VecArgs::Vec(elems))
|
||||
},
|
||||
(sym::vec_new, []) => Some(VecArgs::Vec(&[])),
|
||||
_ => None,
|
||||
|
|
|
|||
|
|
@ -91,6 +91,7 @@ generate! {
|
|||
author,
|
||||
borrow,
|
||||
borrow_mut,
|
||||
box_assume_init_into_vec_unsafe,
|
||||
build_hasher,
|
||||
by_ref,
|
||||
bytes,
|
||||
|
|
|
|||
|
|
@ -113,10 +113,7 @@ fn main() {
|
|||
}
|
||||
|
||||
// #6811
|
||||
match Some(0) {
|
||||
Some(x) => Some(vec![x]),
|
||||
None => None,
|
||||
};
|
||||
Some(0).map(|x| vec![x]);
|
||||
|
||||
// Don't lint, coercion
|
||||
let x: Option<Vec<&[u8]>> = match Some(()) {
|
||||
|
|
|
|||
|
|
@ -183,6 +183,7 @@ fn main() {
|
|||
|
||||
// #6811
|
||||
match Some(0) {
|
||||
//~^ manual_map
|
||||
Some(x) => Some(vec![x]),
|
||||
None => None,
|
||||
};
|
||||
|
|
|
|||
|
|
@ -173,7 +173,17 @@ LL | | };
|
|||
| |_____^ help: try: `Some((String::new(), "test")).as_ref().map(|(x, y)| (y, x))`
|
||||
|
||||
error: manual implementation of `Option::map`
|
||||
--> tests/ui/manual_map_option.rs:196:5
|
||||
--> tests/ui/manual_map_option.rs:185:5
|
||||
|
|
||||
LL | / match Some(0) {
|
||||
LL | |
|
||||
LL | | Some(x) => Some(vec![x]),
|
||||
LL | | None => None,
|
||||
LL | | };
|
||||
| |_____^ help: try: `Some(0).map(|x| vec![x])`
|
||||
|
||||
error: manual implementation of `Option::map`
|
||||
--> tests/ui/manual_map_option.rs:197:5
|
||||
|
|
||||
LL | / match option_env!("") {
|
||||
LL | |
|
||||
|
|
@ -183,7 +193,7 @@ LL | | };
|
|||
| |_____^ help: try: `option_env!("").map(String::from)`
|
||||
|
||||
error: manual implementation of `Option::map`
|
||||
--> tests/ui/manual_map_option.rs:217:12
|
||||
--> tests/ui/manual_map_option.rs:218:12
|
||||
|
|
||||
LL | } else if let Some(x) = Some(0) {
|
||||
| ____________^
|
||||
|
|
@ -195,7 +205,7 @@ LL | | };
|
|||
| |_____^ help: try: `{ Some(0).map(|x| x + 1) }`
|
||||
|
||||
error: manual implementation of `Option::map`
|
||||
--> tests/ui/manual_map_option.rs:226:12
|
||||
--> tests/ui/manual_map_option.rs:227:12
|
||||
|
|
||||
LL | } else if let Some(x) = Some(0) {
|
||||
| ____________^
|
||||
|
|
@ -206,5 +216,5 @@ LL | | None
|
|||
LL | | };
|
||||
| |_____^ help: try: `{ Some(0).map(|x| x + 1) }`
|
||||
|
||||
error: aborting due to 20 previous errors
|
||||
error: aborting due to 21 previous errors
|
||||
|
||||
|
|
|
|||
|
|
@ -53,7 +53,7 @@ fn or_fun_call() {
|
|||
with_constructor.unwrap_or_else(make);
|
||||
//~^ or_fun_call
|
||||
|
||||
let with_new = Some(vec![1]);
|
||||
let with_new: Option<Vec<i32>> = Some(vec![1]);
|
||||
with_new.unwrap_or_default();
|
||||
//~^ unwrap_or_default
|
||||
|
||||
|
|
@ -101,7 +101,7 @@ fn or_fun_call() {
|
|||
real_default.unwrap_or_default();
|
||||
//~^ unwrap_or_default
|
||||
|
||||
let with_vec = Some(vec![1]);
|
||||
let with_vec: Option<Vec<i32>> = Some(vec![1]);
|
||||
with_vec.unwrap_or_default();
|
||||
//~^ unwrap_or_default
|
||||
|
||||
|
|
@ -329,7 +329,7 @@ mod lazy {
|
|||
}
|
||||
}
|
||||
|
||||
let with_new = Some(vec![1]);
|
||||
let with_new: Option<Vec<i32>> = Some(vec![1]);
|
||||
with_new.unwrap_or_default();
|
||||
//~^ unwrap_or_default
|
||||
|
||||
|
|
|
|||
|
|
@ -53,7 +53,7 @@ fn or_fun_call() {
|
|||
with_constructor.unwrap_or(make());
|
||||
//~^ or_fun_call
|
||||
|
||||
let with_new = Some(vec![1]);
|
||||
let with_new: Option<Vec<i32>> = Some(vec![1]);
|
||||
with_new.unwrap_or(Vec::new());
|
||||
//~^ unwrap_or_default
|
||||
|
||||
|
|
@ -101,7 +101,7 @@ fn or_fun_call() {
|
|||
real_default.unwrap_or(<FakeDefault as Default>::default());
|
||||
//~^ unwrap_or_default
|
||||
|
||||
let with_vec = Some(vec![1]);
|
||||
let with_vec: Option<Vec<i32>> = Some(vec![1]);
|
||||
with_vec.unwrap_or(Vec::new());
|
||||
//~^ unwrap_or_default
|
||||
|
||||
|
|
@ -329,7 +329,7 @@ mod lazy {
|
|||
}
|
||||
}
|
||||
|
||||
let with_new = Some(vec![1]);
|
||||
let with_new: Option<Vec<i32>> = Some(vec![1]);
|
||||
with_new.unwrap_or_else(Vec::new);
|
||||
//~^ unwrap_or_default
|
||||
|
||||
|
|
|
|||
|
|
@ -43,8 +43,7 @@ fn unwrap_or_else_default() {
|
|||
with_enum.unwrap_or_else(Enum::A);
|
||||
|
||||
let with_new = Some(vec![1]);
|
||||
with_new.unwrap_or_default();
|
||||
//~^ unwrap_or_default
|
||||
with_new.unwrap_or_else(Vec::new);
|
||||
|
||||
let with_err: Result<_, ()> = Ok(vec![1]);
|
||||
with_err.unwrap_or_else(make);
|
||||
|
|
|
|||
|
|
@ -44,7 +44,6 @@ fn unwrap_or_else_default() {
|
|||
|
||||
let with_new = Some(vec![1]);
|
||||
with_new.unwrap_or_else(Vec::new);
|
||||
//~^ unwrap_or_default
|
||||
|
||||
let with_err: Result<_, ()> = Ok(vec![1]);
|
||||
with_err.unwrap_or_else(make);
|
||||
|
|
|
|||
|
|
@ -1,101 +1,95 @@
|
|||
error: use of `unwrap_or_else` to construct default value
|
||||
--> tests/ui/unwrap_or_else_default.rs:46:14
|
||||
--> tests/ui/unwrap_or_else_default.rs:60:23
|
||||
|
|
||||
LL | with_new.unwrap_or_else(Vec::new);
|
||||
| ^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `unwrap_or_default()`
|
||||
LL | with_real_default.unwrap_or_else(<HasDefaultAndDuplicate as Default>::default);
|
||||
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `unwrap_or_default()`
|
||||
|
|
||||
= note: `-D clippy::unwrap-or-default` implied by `-D warnings`
|
||||
= help: to override `-D warnings` add `#[allow(clippy::unwrap_or_default)]`
|
||||
|
||||
error: use of `unwrap_or_else` to construct default value
|
||||
--> tests/ui/unwrap_or_else_default.rs:61:23
|
||||
|
|
||||
LL | with_real_default.unwrap_or_else(<HasDefaultAndDuplicate as Default>::default);
|
||||
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `unwrap_or_default()`
|
||||
|
||||
error: use of `unwrap_or_else` to construct default value
|
||||
--> tests/ui/unwrap_or_else_default.rs:65:24
|
||||
--> tests/ui/unwrap_or_else_default.rs:64:24
|
||||
|
|
||||
LL | with_default_trait.unwrap_or_else(Default::default);
|
||||
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `unwrap_or_default()`
|
||||
|
||||
error: use of `unwrap_or_else` to construct default value
|
||||
--> tests/ui/unwrap_or_else_default.rs:69:23
|
||||
--> tests/ui/unwrap_or_else_default.rs:68:23
|
||||
|
|
||||
LL | with_default_type.unwrap_or_else(u64::default);
|
||||
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `unwrap_or_default()`
|
||||
|
||||
error: use of `unwrap_or_else` to construct default value
|
||||
--> tests/ui/unwrap_or_else_default.rs:73:23
|
||||
--> tests/ui/unwrap_or_else_default.rs:72:23
|
||||
|
|
||||
LL | with_default_type.unwrap_or_else(Vec::new);
|
||||
| ^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `unwrap_or_default()`
|
||||
|
||||
error: use of `unwrap_or_else` to construct default value
|
||||
--> tests/ui/unwrap_or_else_default.rs:77:18
|
||||
--> tests/ui/unwrap_or_else_default.rs:76:18
|
||||
|
|
||||
LL | empty_string.unwrap_or_else(|| "".to_string());
|
||||
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `unwrap_or_default()`
|
||||
|
||||
error: use of `unwrap_or_else` to construct default value
|
||||
--> tests/ui/unwrap_or_else_default.rs:82:12
|
||||
--> tests/ui/unwrap_or_else_default.rs:81:12
|
||||
|
|
||||
LL | option.unwrap_or_else(Vec::new).push(1);
|
||||
| ^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `unwrap_or_default()`
|
||||
|
||||
error: use of `unwrap_or_else` to construct default value
|
||||
--> tests/ui/unwrap_or_else_default.rs:86:12
|
||||
--> tests/ui/unwrap_or_else_default.rs:85:12
|
||||
|
|
||||
LL | option.unwrap_or_else(Vec::new).push(1);
|
||||
| ^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `unwrap_or_default()`
|
||||
|
||||
error: use of `unwrap_or_else` to construct default value
|
||||
--> tests/ui/unwrap_or_else_default.rs:90:12
|
||||
--> tests/ui/unwrap_or_else_default.rs:89:12
|
||||
|
|
||||
LL | option.unwrap_or_else(Vec::new).push(1);
|
||||
| ^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `unwrap_or_default()`
|
||||
|
||||
error: use of `unwrap_or_else` to construct default value
|
||||
--> tests/ui/unwrap_or_else_default.rs:94:12
|
||||
--> tests/ui/unwrap_or_else_default.rs:93:12
|
||||
|
|
||||
LL | option.unwrap_or_else(Vec::new).push(1);
|
||||
| ^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `unwrap_or_default()`
|
||||
|
||||
error: use of `unwrap_or_else` to construct default value
|
||||
--> tests/ui/unwrap_or_else_default.rs:98:12
|
||||
--> tests/ui/unwrap_or_else_default.rs:97:12
|
||||
|
|
||||
LL | option.unwrap_or_else(Vec::new).push(1);
|
||||
| ^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `unwrap_or_default()`
|
||||
|
||||
error: use of `unwrap_or_else` to construct default value
|
||||
--> tests/ui/unwrap_or_else_default.rs:102:12
|
||||
--> tests/ui/unwrap_or_else_default.rs:101:12
|
||||
|
|
||||
LL | option.unwrap_or_else(Vec::new).push(1);
|
||||
| ^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `unwrap_or_default()`
|
||||
|
||||
error: use of `unwrap_or_else` to construct default value
|
||||
--> tests/ui/unwrap_or_else_default.rs:106:12
|
||||
--> tests/ui/unwrap_or_else_default.rs:105:12
|
||||
|
|
||||
LL | option.unwrap_or_else(Vec::new).push(1);
|
||||
| ^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `unwrap_or_default()`
|
||||
|
||||
error: use of `unwrap_or_else` to construct default value
|
||||
--> tests/ui/unwrap_or_else_default.rs:110:12
|
||||
--> tests/ui/unwrap_or_else_default.rs:109:12
|
||||
|
|
||||
LL | option.unwrap_or_else(Vec::new).push(1);
|
||||
| ^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `unwrap_or_default()`
|
||||
|
||||
error: use of `unwrap_or_else` to construct default value
|
||||
--> tests/ui/unwrap_or_else_default.rs:127:12
|
||||
--> tests/ui/unwrap_or_else_default.rs:126:12
|
||||
|
|
||||
LL | option.unwrap_or_else(Vec::new).push(1);
|
||||
| ^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `unwrap_or_default()`
|
||||
|
||||
error: use of `or_insert_with` to construct default value
|
||||
--> tests/ui/unwrap_or_else_default.rs:145:32
|
||||
--> tests/ui/unwrap_or_else_default.rs:144:32
|
||||
|
|
||||
LL | let _ = inner_map.entry(0).or_insert_with(Default::default);
|
||||
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `or_default()`
|
||||
|
||||
error: aborting due to 16 previous errors
|
||||
error: aborting due to 15 previous errors
|
||||
|
||||
|
|
|
|||
|
|
@ -2,7 +2,7 @@ error: Undefined Behavior: write access through <TAG> at ALLOC[0x0] is forbidden
|
|||
--> RUSTLIB/core/src/mem/mod.rs:LL:CC
|
||||
|
|
||||
LL | crate::intrinsics::write_via_move(dest, src);
|
||||
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Undefined Behavior occurred here
|
||||
| ^^^ Undefined Behavior occurred here
|
||||
|
|
||||
= help: this indicates a potential bug in the program: it performed an invalid operation, but the Tree Borrows rules it violated are still experimental
|
||||
= help: see https://github.com/rust-lang/unsafe-code-guidelines/blob/master/wip/tree-borrows.md for further information
|
||||
|
|
|
|||
|
|
@ -178,6 +178,24 @@ fn extract_if() {
|
|||
}
|
||||
}
|
||||
|
||||
fn vec_macro_cleanup() {
|
||||
// Ensure memory gets deallocated when control flow leaves the `vec!` macro.
|
||||
#[allow(unreachable_code)]
|
||||
loop {
|
||||
let _v = vec![Box::new(0), break];
|
||||
}
|
||||
|
||||
fn panic<T>() -> T {
|
||||
panic!()
|
||||
}
|
||||
// Ensure all memory gets deallocated on a panic: the `Box` we construct, and the `Box`
|
||||
// constructed inside `vec!` to eventually turn into a `Vec`.
|
||||
std::panic::catch_unwind(|| {
|
||||
let _v = vec![Box::new(0), panic()];
|
||||
})
|
||||
.unwrap_err();
|
||||
}
|
||||
|
||||
fn main() {
|
||||
assert_eq!(vec_reallocate().len(), 5);
|
||||
|
||||
|
|
@ -209,4 +227,5 @@ fn main() {
|
|||
reverse();
|
||||
miri_issue_2759();
|
||||
extract_if();
|
||||
vec_macro_cleanup();
|
||||
}
|
||||
|
|
|
|||
5
src/tools/miri/tests/pass/vec.stack.stderr
Normal file
5
src/tools/miri/tests/pass/vec.stack.stderr
Normal file
|
|
@ -0,0 +1,5 @@
|
|||
|
||||
thread 'main' ($TID) panicked at tests/pass/vec.rs:LL:CC:
|
||||
explicit panic
|
||||
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
|
||||
note: in Miri, you may have to set `MIRIFLAGS=-Zmiri-env-forward=RUST_BACKTRACE` for the environment variable to have an effect
|
||||
5
src/tools/miri/tests/pass/vec.tree.stderr
Normal file
5
src/tools/miri/tests/pass/vec.tree.stderr
Normal file
|
|
@ -0,0 +1,5 @@
|
|||
|
||||
thread 'main' ($TID) panicked at tests/pass/vec.rs:LL:CC:
|
||||
explicit panic
|
||||
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
|
||||
note: in Miri, you may have to set `MIRIFLAGS=-Zmiri-env-forward=RUST_BACKTRACE` for the environment variable to have an effect
|
||||
|
|
@ -2450,7 +2450,6 @@ ui/single-use-lifetime/issue-107998.rs
|
|||
ui/single-use-lifetime/issue-117965.rs
|
||||
ui/span/issue-107353.rs
|
||||
ui/span/issue-11925.rs
|
||||
ui/span/issue-15480.rs
|
||||
ui/span/issue-23338-locals-die-before-temps-of-body.rs
|
||||
ui/span/issue-23729.rs
|
||||
ui/span/issue-23827.rs
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue