-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
add subslice support in drop ladder for array #46686
add subslice support in drop ladder for array #46686
Conversation
(rust_highfive has picked a reviewer for you, use r? to override) |
r? @arielb1 |
src/librustc_mir/borrow_check/mod.rs
Outdated
@@ -1279,6 +1278,7 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { | |||
); | |||
} | |||
} | |||
Write(WriteKind::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.
Why this change?
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.
Why did you make this change.
411c05d
to
2decde6
Compare
fn clear_drop_flag(&mut self, location: Location, path: Self::Path, mode: DropFlagMode); | ||
fn get_drop_flags(&mut self, path: Self::Path) -> Option<Operand<'tcx>>; | ||
fn clear_drop_flag(&mut self, | ||
location: Location, path: |
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.
nit: path should go in the next line
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.
location: Location,
path: Self::Path
fn array_subpaths(&self, path: Self::Path, size: u64) | ||
-> Vec<(Place<'tcx>, Option<Self::Path>, Option<Local>)> { | ||
if dataflow::move_path_children_matching(self.ctxt.move_data(), path, |_| { | ||
assert!(size <= (u32::MAX as u64), |
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.
can you move the assertion out of the loop?
} | ||
|
||
let size = size as u32; | ||
let flags = self.ctxt.array_items_drop_flags.get(&path); |
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.
nit: stray double space
flags = self.ctxt
^^
Do you handle the case where either individual indexes or subslices are moved out, aka: fn subslice_pattern_test(a: &Allocator, arg: bool) {
let a = [a.alloc(), a.alloc(), a.alloc(), a.alloc(), a.alloc()];
if arg {
let[.., _x, _] = a;
} else {
let[_, _y..] = a;
}
} |
@@ -88,14 +88,19 @@ pub trait DropElaborator<'a, 'tcx: 'a> : fmt::Debug { | |||
fn param_env(&self) -> ty::ParamEnv<'tcx>; | |||
|
|||
fn drop_style(&self, path: Self::Path, mode: DropFlagMode) -> DropStyle; | |||
fn get_drop_flag(&mut self, path: Self::Path) -> Option<Operand<'tcx>>; | |||
fn clear_drop_flag(&mut self, location: Location, path: Self::Path, mode: DropFlagMode); | |||
fn get_drop_flags(&mut self, path: Self::Path) -> Option<Operand<'tcx>>; |
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 still can't return multiple drop flags, so why did you rename it?
Another test: fn subslice_pattern_test(a: &Allocator, arg: bool, arg2: bool) {
let a = [a.alloc(), a.alloc(), a.alloc(), a.alloc(), a.alloc()];
if arg2 {
drop(a);
return;
}
if arg {
let[.., _x, _] = a;
} else {
let[_, _y..] = a;
}
} |
@@ -389,6 +492,7 @@ impl<'b, 'tcx> ElaborateDropsCtxt<'b, 'tcx> { | |||
} | |||
}; | |||
|
|||
|
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.
nit: stray newline
min_length: size, | ||
from_end: false | ||
}), | ||
move_path, |
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.
So reading this, I'm afraid that this can cause a drop to be run with a "wrong" MovePath
- a Subslice
when the drop can also be accessed through a ConstantIndex
(or vice versa), which might cause the "unconditional" drop code to trigger when it shouldn't, as in
let mut x = [a.alloc(), a.alloc(), a.alloc()];
// The MP for x[1..3] is now initialized, x[2] is now uninitialized
let [_, _, _x] = x;
// this shouldn't rely on any drop flags, and just drop x[0] & x[1]
x = [a.alloc(), a.alloc(), a.alloc()];
let [_, .._x] = x;
// The MP for x[1..3] is now uninitialized, x[2] is now initialized
// end of scope drop for x - should drop x[0] without any drop flags
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.
And in some cases you might need the drop flags:
let alloc = || (a.alloc(). a.alloc());
let mut x = [alloc(), alloc(), alloc()];
// The MP for x[1..3] is now initialized, x[2].0 is now uninitialized
let [_, _, (_x,_)] = x;
// this shouldn't rely on any drop flags, and just drop x[0], x[1], and x[2].1
x = [a.alloc(), a.alloc(), a.alloc()];
let [_, .._x] = x;
// The MP for x[1..3] is now uninitialized, x[1].1 is now initialized
// end of scope drop for x - should drop x[0] without any drop flags
So I think you do need to generate move paths for all the array elements when you see a subslice, to have a place to focus (similarly, you want to normalize the move paths so that an access to array[-2 of 3]
is the same as an access to array[1 of 3]
)
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.
At least, this is how it appears to me. There might be a better solution but it eludes me.
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 go back to european time and no meetings in Monday (I'm currently in the US for the Mozilla All Hands), so if you need any help I'll go on IRC then.
}) | ||
fn array_subpaths(&self, path: Self::Path, size: u64) | ||
-> Vec<(Place<'tcx>, Option<Self::Path>, Option<Local>)> { | ||
if dataflow::move_path_children_matching(self.ctxt.move_data(), path, |
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.
Nit: speaking as someone who doesn't know this code that well, I'd appreciate some comments here. =)
I guess that this is just a "trivial case" check, however? i.e., if we never touched any of the indices in this array, then we there is nothing to do here?
I don't think I can finish handling this PR. |
@nikomatsakis could you try to get this working with @mikhail-m1 ? |
@arielb1 @mikhail-m1 yep! |
Hi @mikhail-m1, just to check if you're still working on this. Looks like @arielb1 has some questions for you too! |
Cool! I'll be sure to review your PR when you get it done. |
Ping from triage @mikhail-m1! It's been over a week since we last heard from you — will you still be able to make the needed revisions? |
@shepmaster sorry, I am still working on it but don't have enough time to finish, hope I will do it in next few days |
☔ The latest upstream changes (presumably #47837) made this pull request unmergeable. Please resolve the merge conflicts. |
…omatsakis add transform for uniform array move out reworked second step for fix #34708 previous try #46686 r? @nikomatsakis
second step for fix #34708