-
Notifications
You must be signed in to change notification settings - Fork 13.9k
replace box_new with lower-level intrinsics #148190
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
base: master
Are you sure you want to change the base?
Conversation
|
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 |
|
r? @SparrowLii rustbot has assigned @SparrowLii. Use |
| // rvalue, | ||
| // "Unexpected CastKind::Transmute {ty_from:?} -> {ty:?}, which is not permitted in Analysis MIR", | ||
| // ), | ||
| // } |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to still visit the cast type to find any lifetimes in there, or so?
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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>.
| // Make sure `StorageDead` gets emitted. | ||
| this.schedule_drop_storage_and_value(expr_span, this.local_scope(), ptr); |
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.
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.
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a Miri test to ensure the drop does indeed happen. That's the easiest way to check for memory leaks...
| // 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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.
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... 😨
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.
Are these changes fine? Who knows! At least the filecheck annotations in the test still pass.
| StorageDead(_10); | ||
| StorageDead(_8); | ||
| StorageDead(_4); | ||
| drop(_3) -> [return: bb10, unwind: bb15]; |
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 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 |
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.
This is fully a duplicate of something already tested in nll/user-annotations/patterns.rs.
| // 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
4e526d4 to
cb7642b
Compare
This comment has been minimized.
This comment has been minimized.
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.
replace box_new with lower-level intrinsics
|
It seems like for some reason, one now has to run |
| .into(), | ||
| destination: inner_ptr, | ||
| target: Some(success), | ||
| unwind: UnwindAction::Continue, |
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 UnwindAction::Continue here seems sus to me, don't we have to potentially insert some cleanup blocks to drop stuff that's locally in scope?
I copied this from the old box_new lowering.
|
☔ The latest upstream changes (presumably #144420) made this pull request unmergeable. Please resolve the merge conflicts. |
I don't know how. Even just adding curly braces without EDIT: Looks like your system clock is going wrong and github doesn't validate the time, or so...? Your post shows up as 20min in the future. ;) |
|
This seems to be very related to project goal "MIR move elimination".
Can this be solved using "super let"? ( https://blog.m-ou.se/super-let/ , #139076 ) |
As well as I understand, "super let" has curly braces in its syntax, but they somehow don't have effects of normal curly braces. (I'm not sure about this.) See this quote from here #139076 (comment) :
Also, this recent PR appears to be related, too: #146098 .
Yes, I see this, too. I reported this to Github.
This is discussed in that "super let" tracking issue, too. "We need a way to call the unsafe Pin::new_unchecked without the unsafe {} applying to the arguments." ( #139076 (comment) ) |
|
{
let mut b = Box::new_uninit();
let ptr = MaybeUninit::as_mut_ptr(&mut *b);
write_via_move(ptr, [$($x),+]);
b
}We'd want to lifetime-expand the Also note that even if lifetime expansion works, type inference also often fails for code that currently works. Feel free to experiment with this but I tried so many variants of this, I am done and just awaiting review now. |
|
rust-lang/rust-project-goals#395 seems to be related, too |
|
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 |
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. |
| // 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
requires lowering write_via_move during MIR building to make it just like an assignment
|
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 allows us to get rid of box_new entirely
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
fyi I'm out of the office today and can't look until later this week. |
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.