replace box_new with lower-level intrinsics #148190
Open
+1,285
−1,390
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt The Miri subtree was changed cc @rust-lang/miri Some changes occurred in cc @BoxyUwU Some changes occurred in match checking cc @Nadrieril |
rustbot
added
S-waiting-on-review
|
r? @SparrowLii rustbot has assigned @SparrowLii. Use |
RalfJung
commented
Oct 27, 2025
| // rvalue, | ||
| // "Unexpected CastKind::Transmute {ty_from:?} -> {ty:?}, which is not permitted in Analysis MIR", | ||
| // ), | ||
| // } |
There was a problem hiding this comment.
This obviously needs to be resolved before landing... what should we do here? A transmute cast is always well-typed (it is UB if the sizes mismatch), so... can we just do nothing? I don't know what the type checker inside borrow checker is about.^^
There was a problem hiding this comment.
MIR typeck exists to collect all the lifetime constraints for borrowck to check. It also acts as a little bit of a double-check that typechecking on HIR actually checked everything it was supposed to, in some sense it's kind of the "soundness critical typeck". Having this do nothing seems fine to me, there's nothing to really typeck here
There was a problem hiding this comment.
Do we need to still visit the cast type to find any lifetimes in there, or so?
There was a problem hiding this comment.
FWIW this "is not permitted in Analysis MIR" part in the error I am removing here is odd as this is not documented in the MIR syntax where we usually list such restrictions, and also not checked by the MIR validator.
There was a problem hiding this comment.
Do we need to still visit the cast type to find any lifetimes in there, or so?
I don't think so, that should be handled by the super_rvalue call at the top of this visit_rvalue fn
There was a problem hiding this comment.
I guess we do need to check something here, right now nothing enforces that the lifetimes match for the various uses of T in init_box_via_move<T>(b: Box<MaybeUninit<T>>, x: T) -> Box<T>.
RalfJung
commented
Oct 27, 2025
Comment on lines
445
to
449
| // Make sure `StorageDead` gets emitted. | ||
| this.schedule_drop_storage_and_value(expr_span, this.local_scope(), ptr); |
There was a problem hiding this comment.
Here I am completely guessing... well really for all the changes in this file I am guessing, but the drop/storage scope stuff I know even less about than the rest of this.
RalfJung
commented
Oct 27, 2025
| block, | ||
| source_info, | ||
| Place::from(ptr), | ||
| // Needs to be a `Copy` so that `b` still gets dropped if `val` panics. |
There was a problem hiding this comment.
I added a Miri test to ensure the drop does indeed happen. That's the easiest way to check for memory leaks...
RalfJung
commented
Oct 27, 2025
Comment on lines
+292
to
+304
| // Nothing below can panic so we do not have to worry about deallocating `ptr`. | ||
| // SAFETY: we just allocated the box to store `x`. | ||
| unsafe { core::intrinsics::write_via_move(ptr, x) }; | ||
| // SAFETY: we just initialized `b`. | ||
| unsafe { mem::transmute(ptr) } |
There was a problem hiding this comment.
I tried using init_box_via_move here instead and it makes things a bit slower in some secondary benchmarks. I have no idea why.
RalfJung
commented
Oct 27, 2025
There was a problem hiding this comment.
This MIR is apparently so different from the previous one that it doesn't even show a diff (and the filename changed since I had to use CleanupPostBorrowck as built contains user types which contain super fragile DefIds). I have no idea what this test is testing and there are no filecheck annotations so... 😨
RalfJung
commented
Oct 27, 2025
There was a problem hiding this comment.
Are these changes fine? Who knows! At least the filecheck annotations in the test still pass.
RalfJung
commented
Oct 27, 2025
| StorageDead(_10); | ||
| StorageDead(_8); | ||
| StorageDead(_4); | ||
| drop(_3) -> [return: bb10, unwind: bb15]; |
There was a problem hiding this comment.
I think this is the relevant drop... but the before drop-elaboration MIR makes this quite hard to say. No idea why that's what the test checks. I think after drop elaboration this is a lot more obvious as the drops of moved-out variables are gone.
This comment has been minimized.
This comment has been minimized.
|
This PR modifies |
RalfJung
commented
Oct 28, 2025
There was a problem hiding this comment.
This is fully a duplicate of something already tested in nll/user-annotations/patterns.rs.
RalfJung
commented
Oct 28, 2025
Comment on lines
63
to
69
| // FIXME: What is happening?!?? | ||
| let _: Vec<&'static String> = vec![&String::new()]; | ||
| //~^ ERROR temporary value dropped while borrowed [E0716] | ||
|
|
||
| let (_, a): (Vec<&'static String>, _) = (vec![&String::new()], 44); | ||
| //~^ ERROR temporary value dropped while borrowed [E0716] | ||
|
|
||
| let (_a, b): (Vec<&'static String>, _) = (vec![&String::new()], 44); | ||
| //~^ ERROR temporary value dropped while borrowed [E0716] | ||
| } |
There was a problem hiding this comment.
I have no idea what is happening here -- somehow code like let _: Vec<&'static String> = vec![&String::new()]; now compiles. I guess something is wrong with how I am lowering init_box_via_move?
There was a problem hiding this comment.
It's probably related to the transmutes there. Is there some way to insert those in a way that the lifetime constraints still get enforced?
This comment has been minimized.
This comment has been minimized.
RalfJung
force-pushed
the
box_new
branch
2 times, most recently
from
4e526d4 to
cb7642b
Compare
This comment has been minimized.
This comment has been minimized.
RalfJung
force-pushed
the
box_new
branch
2 times, most recently
from
86e5a72 to
020bc3a
Compare
This comment has been minimized.
This comment has been minimized.
|
I can't think of any way to actually preserve these lifetimes while using transmutes... so we'll have to add more method calls to So here's another redesign of the @bors try |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
rust-bors bot
added a commit
that referenced
this pull request
Oct 28, 2025
replace box_new with lower-level intrinsics
|
Consider this implementation: #![allow(internal_features)]
#![feature(core_intrinsics)]
#![feature(rustc_attrs)]
#![feature(super_let)]
use std::mem::MaybeUninit;
// This function is actually safe
#[inline(always)] // Or #[rustc_force_inline]
#[rustc_nounwind]
fn write_to_box<T>(mut dst: Box<MaybeUninit<T>>, src: T) -> Box<T> {
let x: &mut MaybeUninit<T> = &mut *dst;
let x = x as *mut MaybeUninit<T> as *mut T;
unsafe { core::intrinsics::write_via_move(x, src) };
unsafe { dst.assume_init() }
}
macro_rules! my_vec {
[$($x:expr),+ $(,)?] => {
{
let x = Box::new_uninit();
super let x = (write_to_box(x, [$($x),+]) as Box<[_]>).into_vec();
x
}
}
}
Unfortunately, codegen is suboptimal in debug builds. As you can see in Godbolt, there are additional copies involved for some particular type (see Godbolt), as opposed to current nightly It seems this is not because of indirection, i. e. not because of function Feel free to use my code. Or combine it with yours |
|
I tried that code (or something equivalent), but I can repeat the explanation of why it doesn't work:
Yes, because you are moving
That's because you don't have my branch where |
|
Howdy, I've looked into the test failure some. I'm not an expert on any of this by any means, but here's what I'm seeing: I'm compiling a pared-down version of the test with fn main() {
zzz(); // #break
let x = vec![42]; // #loc3
zzz(); // #loc6
}main function LLVM-IR via stable; main::main
; Function Attrs: uwtable
define internal void @_ZN4main4main17h105998d931d8a56dE() unnamed_addr #0 personality ptr @__CxxFrameHandler3 !dbg !1388 {
start:
%x = alloca [24 x i8], align 8
#dbg_declare(ptr %x, !1392, !DIExpression(), !1394)
; call main::zzz
call void @_ZN4main3zzz17h525fbd41aefd4fddE(), !dbg !1395
; call alloc::alloc::exchange_malloc
%_6 = call ptr @_ZN5alloc5alloc15exchange_malloc17h0c9c47b68377fb04E(i64 4, i64 4), !dbg !1396
%_11 = ptrtoint ptr %_6 to i64, !dbg !1396
%_14 = and i64 %_11, 3, !dbg !1396
%_15 = icmp eq i64 %_14, 0, !dbg !1396
br i1 %_15, label %bb8, label %panic, !dbg !1396
bb8: ; preds = %start
%_17 = ptrtoint ptr %_6 to i64, !dbg !1396
%_20 = icmp eq i64 %_17, 0, !dbg !1396
%_21 = and i1 %_20, true, !dbg !1396
%_22 = xor i1 %_21, true, !dbg !1396
br i1 %_22, label %bb9, label %panic1, !dbg !1396
panic: ; preds = %start
; call core::panicking::panic_misaligned_pointer_dereference
call void @_ZN4core9panicking36panic_misaligned_pointer_dereference17h6e7340435152efe7E(i64 4, i64 %_11, ptr align 8 @alloc_cf8e02def08fd6c50b9df30b8c2413e4) #14, !dbg !1396
unreachable, !dbg !1396
bb9: ; preds = %bb8
%0 = getelementptr inbounds nuw i32, ptr %_6, i64 0, !dbg !1396
store i32 42, ptr %0, align 4, !dbg !1396
; call alloc::slice::<impl [T]>::into_vec
call void @"_ZN5alloc5slice29_$LT$impl$u20$$u5b$T$u5d$$GT$8into_vec17heb69c3bf639d72bfE"(ptr sret([24 x i8]) align 8 %x, ptr align 4 %_6, i64 1), !dbg !1396
; invoke main::zzz
invoke void @_ZN4main3zzz17h525fbd41aefd4fddE()
to label %bb4 unwind label %funclet_bb6, !dbg !1397
panic1: ; preds = %bb8
; call core::panicking::panic_null_pointer_dereference
call void @_ZN4core9panicking30panic_null_pointer_dereference17hfc82d2d4db623c43E(ptr align 8 @alloc_cf8e02def08fd6c50b9df30b8c2413e4) #14, !dbg !1396
unreachable, !dbg !1396
bb6: ; preds = %funclet_bb6
; call core::ptr::drop_in_place<alloc::vec::Vec<i32>>
call void @"_ZN4core3ptr47drop_in_place$LT$alloc..vec..Vec$LT$i32$GT$$GT$17h99bece23b8270328E"(ptr align 8 %x) #15 [ "funclet"(token %cleanuppad) ], !dbg !1398
cleanupret from %cleanuppad unwind to caller, !dbg !1399
funclet_bb6: ; preds = %bb9
%cleanuppad = cleanuppad within none []
br label %bb6
bb4: ; preds = %bb9
; call core::ptr::drop_in_place<alloc::vec::Vec<i32>>
call void @"_ZN4core3ptr47drop_in_place$LT$alloc..vec..Vec$LT$i32$GT$$GT$17h99bece23b8270328E"(ptr align 8 %x), !dbg !1398
ret void, !dbg !1398
}main function LLVM-IR via this PR; main::main
; Function Attrs: uwtable
define hidden void @_ZN4main4main17h9686e224a639fa37E() unnamed_addr #0 personality ptr @rust_eh_personality !dbg !655 {
start:
%b.dbg.spill.i3 = alloca [8 x i8], align 8
%b.dbg.spill.i = alloca [8 x i8], align 8
%0 = alloca [16 x i8], align 8
%_4 = alloca [8 x i8], align 8
%x = alloca [24 x i8], align 8
#dbg_declare(ptr %x, !657, !DIExpression(), !659)
; call main::zzz
call void @_ZN4main3zzz17ha5eb45a4a150cc22E(), !dbg !660
; call alloc::boxed::box_new_uninit
%_1.i = call align 4 ptr @_ZN5alloc5boxed14box_new_uninit17he50a4dbe03251113E(i64 4, i64 4), !dbg !661
store ptr %_1.i, ptr %_4, align 8, !dbg !669
store ptr %_4, ptr %b.dbg.spill.i, align 8
#dbg_declare(ptr %b.dbg.spill.i, !670, !DIExpression(), !676)
%_4.i = load ptr, ptr %_4, align 8, !dbg !678
br label %bb3, !dbg !679
bb8: ; preds = %cleanup
; invoke core::ptr::drop_in_place<alloc::boxed::Box<core::mem::maybe_uninit::MaybeUninit<[i32; 1]>>>
invoke void @"_ZN4core3ptr114drop_in_place$LT$alloc..boxed..Box$LT$core..mem..maybe_uninit..MaybeUninit$LT$$u5b$i32$u3b$$u20$1$u5d$$GT$$GT$$GT$17h5d515d80d8e99280E"(ptr align 8 %_4) #14
to label %bb9 unwind label %terminate, !dbg !669
cleanup: ; No predecessors!
%1 = landingpad { ptr, i32 }
cleanup
%2 = extractvalue { ptr, i32 } %1, 0
%3 = extractvalue { ptr, i32 } %1, 1
store ptr %2, ptr %0, align 8
%4 = getelementptr inbounds i8, ptr %0, i64 8
store i32 %3, ptr %4, align 8
br label %bb8
bb3: ; preds = %start
%_9 = ptrtoint ptr %_4.i to i64, !dbg !669
%_11 = and i64 %_9, 3, !dbg !669
%_12 = icmp eq i64 %_11, 0, !dbg !669
br i1 %_12, label %bb10, label %panic, !dbg !669
bb10: ; preds = %bb3
%_14 = ptrtoint ptr %_4.i to i64, !dbg !669
%_16 = icmp eq i64 %_14, 0, !dbg !669
%_17 = and i1 %_16, true, !dbg !669
%_18 = xor i1 %_17, true, !dbg !669
br i1 %_18, label %bb11, label %panic1, !dbg !669
panic: ; preds = %bb3
; call core::panicking::panic_misaligned_pointer_dereference
call void @_ZN4core9panicking36panic_misaligned_pointer_dereference17h405de148aeb5f90bE(i64 4, i64 %_9, ptr align 8 @alloc_9199891e002284d755d36cef38ca1ec4) #17, !dbg !669
unreachable, !dbg !669
bb11: ; preds = %bb10
%5 = getelementptr inbounds nuw i32, ptr %_4.i, i64 0, !dbg !669
store i32 42, ptr %5, align 4, !dbg !669
%_3 = load ptr, ptr %_4, align 8, !dbg !669
store ptr %_3, ptr %b.dbg.spill.i3, align 8
#dbg_declare(ptr %b.dbg.spill.i3, !680, !DIExpression(), !685)
; call alloc::boxed::Box<core::mem::maybe_uninit::MaybeUninit<T>,A>::assume_init
%_3.i = call align 4 ptr @"_ZN5alloc5boxed60Box$LT$core..mem..maybe_uninit..MaybeUninit$LT$T$GT$$C$A$GT$11assume_init17he34683ba92371b9aE"(ptr align 4 %_3), !dbg !687
; call alloc::slice::<impl [T]>::into_vec
call void @"_ZN5alloc5slice29_$LT$impl$u20$$u5b$T$u5d$$GT$8into_vec17h281036d717d9f9adE"(ptr sret([24 x i8]) align 8 %x, ptr align 4 %_3.i, i64 1), !dbg !688
; invoke main::zzz
invoke void @_ZN4main3zzz17ha5eb45a4a150cc22E()
to label %bb5 unwind label %cleanup2, !dbg !689
panic1: ; preds = %bb10
; call core::panicking::panic_null_pointer_dereference
call void @_ZN4core9panicking30panic_null_pointer_dereference17hd15a6708779e4dd5E(ptr align 8 @alloc_9199891e002284d755d36cef38ca1ec4) #17, !dbg !669
unreachable, !dbg !669
bb7: ; preds = %cleanup2
; invoke core::ptr::drop_in_place<alloc::vec::Vec<i32>>
invoke void @"_ZN4core3ptr47drop_in_place$LT$alloc..vec..Vec$LT$i32$GT$$GT$17h513b29ff9eff6e60E"(ptr align 8 %x) #14
to label %bb9 unwind label %terminate, !dbg !690
cleanup2: ; preds = %bb11
%6 = landingpad { ptr, i32 }
cleanup
%7 = extractvalue { ptr, i32 } %6, 0
%8 = extractvalue { ptr, i32 } %6, 1
store ptr %7, ptr %0, align 8
%9 = getelementptr inbounds i8, ptr %0, i64 8
store i32 %8, ptr %9, align 8
br label %bb7
bb5: ; preds = %bb11
; call core::ptr::drop_in_place<alloc::vec::Vec<i32>>
call void @"_ZN4core3ptr47drop_in_place$LT$alloc..vec..Vec$LT$i32$GT$$GT$17h513b29ff9eff6e60E"(ptr align 8 %x), !dbg !690
ret void, !dbg !691
terminate: ; preds = %bb8, %bb7
%10 = landingpad { ptr, i32 }
filter [0 x ptr] zeroinitializer
; call core::panicking::panic_in_cleanup
call void @_ZN4core9panicking16panic_in_cleanup17h33c8f66a4744c200E() #15, !dbg !692
unreachable, !dbg !692
bb9: ; preds = %bb8, %bb7
%11 = load ptr, ptr %0, align 8, !dbg !692
%12 = getelementptr inbounds i8, ptr %0, i64 8, !dbg !692
%13 = load i32, ptr %12, align 8, !dbg !692
%14 = insertvalue { ptr, i32 } poison, ptr %11, 0, !dbg !692
%15 = insertvalue { ptr, i32 } %14, i32 %13, 1, !dbg !692
resume { ptr, i32 } %15, !dbg !692
}My first guess is that it's the actual function calls themselves, since there are 2: ; call alloc::boxed::Box<core::mem::maybe_uninit::MaybeUninit<T>,A>::assume_init
%_3.i = call align 4 ptr @"_ZN5alloc5boxed60Box$LT$core..mem..maybe_uninit..MaybeUninit$LT$T$GT$$C$A$GT$11assume_init17he34683ba92371b9aE"(ptr align 4 %_3), !dbg !687
; call alloc::slice::<impl [T]>::into_vec
call void @"_ZN5alloc5slice29_$LT$impl$u20$$u5b$T$u5d$$GT$8into_vec17h281036d717d9f9adE"(ptr sret([24 x i8]) align 8 %x, ptr align 4 %_3.i, i64 1), !dbg !688Whereas with stable there is only a single call to ; call alloc::alloc::exchange_malloc
%_6 = call ptr @_ZN5alloc5alloc15exchange_malloc17h0c9c47b68377fb04E(i64 4, i64 4), !dbg !1396My second guess would be it's the Notably, %x = alloca [24 x i8], align 8
#dbg_declare(ptr %x, !1392, !DIExpression(), !1394)
...
!1391 = !{!1392}
!1392 = !DILocalVariable(name: "x", scope: !1393, file: !1389, line: 111, type: !640, align: 64)
!1393 = distinct !DILexicalBlock(scope: !1388, file: !1389, line: 111)
!1394 = !DILocation(line: 111, scope: !1393)That's source-line info + type The pr's IR contains several %x = alloca [24 x i8], align 8
#dbg_declare(ptr %x, !657, !DIExpression(), !659)
...
!656 = !{!657}
!657 = !DILocalVariable(name: "x", scope: !658, file: !652, line: 111, type: !242, align: 64)
!658 = distinct !DILexicalBlock(scope: !655, file: !652, line: 111, column: 5)
!659 = !DILocation(line: 111, column: 9, scope: !658)
...
This looks like an intermediate variable `b` from `box_uninit_as_mut_ptr<[i32; 1]>`, which is inlined (also keep the `%_4` identifier in mind):
```llvm
store ptr %_4, ptr %b.dbg.spill.i, align 8
#dbg_declare(ptr %b.dbg.spill.i, !670, !DIExpression(), !676)
...
!670 = !DILocalVariable(name: "b", arg: 1, scope: !671, file: !663, line: 255, type: !216)
!671 = distinct !DISubprogram(name: "box_uninit_as_mut_ptr<[i32; 1]>", linkageName: "_ZN5alloc5boxed21box_uninit_as_mut_ptr17h7a86c3dbd98d7bf5E", scope: !665, file: !663, line: 255, type: !672, scopeLine: 255, flags: DIFlagPrototyped, spFlags: DISPFlagLocalToUnit | DISPFlagDefinition, unit: !29, templateParams: !231, retainedNodes: !675)
!672 = !DISubroutineType(types: !673)
!673 = !{!674, !216}
!674 = !DIDerivedType(tag: DW_TAG_pointer_type, name: "*mut [i32; 1]", baseType: !228, size: 64, align: 64, dwarfAddressSpace: 0)
!675 = !{!670}
!676 = !DILocation(line: 255, column: 36, scope: !671, inlinedAt: !677)
!677 = distinct !DILocation(line: 111, column: 13, scope: !655)This is another intermediate store ptr %_3, ptr %b.dbg.spill.i3, align 8
#dbg_declare(ptr %b.dbg.spill.i3, !680, !DIExpression(), !685)
...
!680 = !DILocalVariable(name: "b", arg: 1, scope: !681, file: !663, line: 266, type: !217)
!681 = distinct !DISubprogram(name: "box_uninit_array_into_vec_unsafe<i32, 1>", linkageName: "_ZN5alloc5boxed32box_uninit_array_into_vec_unsafe17h1eaff6ce378da9adE", scope: !665, file: !663, line: 265, type: !682, scopeLine: 265, flags: DIFlagPrototyped, spFlags: DISPFlagLocalToUnit | DISPFlagDefinition, unit: !29, templateParams: !280, retainedNodes: !684)
!682 = !DISubroutineType(types: !683)
!683 = !{!242, !217}
!684 = !{!680}
!685 = !DILocation(line: 266, column: 5, scope: !681, inlinedAt: !686)
!686 = distinct !DILocation(line: 111, column: 13, scope: !655)this |
|
(i will also say though, if there's not an easy solution, having to step twice to step over the |
RalfJung
force-pushed
the
box_new
branch
2 times, most recently
from
82fdb20 to
3ca2b21
Compare
|
Thanks for taking a look!
There are more calls, there's also I also do see a call to
That sounds more likely. However, in MIR I am giving all these things the same span, so I have no clue why the LLVM backend decides to give them different debug info. |
RalfJung
commented
Nov 3, 2025
| // gdb-check:[...]#loc3[...] | ||
| // gdb-command:next | ||
| // FIXME: for some reason we need two `next` to skip over the `vec!`. | ||
| // gdb-command:next |
There was a problem hiding this comment.
I have now made the test pass by adding a second next invocation here... but clearly that's not the actually intended behavior. I just have no idea how to make the debugger jump over the entire thing in a single step again.
I'm afraid I only really know much about our split debuginfo implementation. |
|
This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
This comment has been minimized.
This comment has been minimized.
|
@khuey you have contributed some great fixes to our debuginfo in the past, do you have any insight on what's happening with tests/debuginfo/macro-stepping.rs here? |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
fyi I'm out of the office today and can't look until later this week. |
|
Some changes occurred in diagnostic error codes |
This comment has been minimized.
This comment has been minimized.
requires lowering write_via_move during MIR building to make it just like an assignment
This comment has been minimized.
This comment has been minimized.
This allows us to get rid of box_new entirely
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
I'm going to wait with fixing Clippy until we agree we even want this change. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
13 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The
box_newintrinsic is super special: during THIR construction it turns into anExprKind::Box(formerly known as theboxkeyword), 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-initializedBox(which raises interesting opsem questions).This PR is the n-th attempt to get rid of
box_new. That's non-trivial because it usually causes a perf regression: replacingbox_newby 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_newis used in two places,Box::newandvec!:Box::newthat is fairly easy: themove_by_valueintrinsic 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 aBox<MaybeUninit<T>>and returns that box again. The MIR building code for this is non-trivial, but in exchange we can get rid of somewhat similar code in the lowering forExprKind::Box. Sadly, we need a new lang item as I found no other way to get a pointer of the right type in MIR, but we do get rid of the exchange_malloc lang item in exchange. (We can also get rid ofRvalue::ShallowInitBox; I didn't include that in this PR -- I think @cjgillot has a commit for this somewhere.)See here 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.
vec!FAQwrite_box_via_movereturn theBoxagain? Because we need to expandvec!to a bunch of method invocations without any blocks or let-statements, or else the temporary scopes (and type inference) don't work out.box_uninit_array_into_vec_unsafe(unsoundly!) a safe function? Because we can't use an unsafe block invec!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 insideunsafe, which would be bad (and 10 years later, we still don't have safe blocks for macros like this).write_box_via_moveuseBoxas input/output type, and not, say, raw pointers? Because that is the only way to get the correct behavior when$xpanics or has control effects: we need theBoxto 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.)*mut Box<MaybeUninit<T>>to*mut T? Because we need to do that conversion when compilingwrite_box_via_moveto MIR, and we can't do that conversion in pure MIR. We can't transmute because then the borrow checker loses track of how the lifetimes flow through these calls, which would be unsound. We can't get the raw pointer out from inside theBoxvia field projections because that is a*constpointer which we can't write to. We can't cast that to*mutbecause then we again lose track of the lifetimes. We can't change that pointer to be*mutbecause thenNonNullwould have the wrong variance (and 10 years later, we still don't have variance annotations).write_box_via_movereturnBox<T>? Yes we could, but there's no easy way for the intrinsic to convert itsBox<MaybeUninit<T>>to aBox<T>. We would need another lang item.