Skip to content
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.

Sign up for GitHub

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

Closed
wants to merge 2 commits into from
Closed

add subslice support in drop ladder for array #46686

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2decde6
add subslice support in drop ladder for array
mikhail-m1 Dec 12, 2017
a4c7d75
bugfix and add tests for elaborate array drop ladder
mikhail-m1 Dec 14, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 8 additions & 3 deletions src/librustc_mir/shim.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -268,7 +268,11 @@ impl<'a, 'tcx> DropElaborator<'a, 'tcx> for DropShimElaborator<'a, 'tcx> {
None
}

fn clear_drop_flag(&mut self, _location: Location, _path: Self::Path, _mode: DropFlagMode) {
fn clear_drop_flag(&mut self,
_location: Location,
_path: Self::Path,
_mode: DropFlagMode,
_opt_flag: Option<Local>) {
}

fn field_subpath(&self, _path: Self::Path, _field: Field) -> Option<Self::Path> {
Expand All @@ -280,8 +284,9 @@ impl<'a, 'tcx> DropElaborator<'a, 'tcx> for DropShimElaborator<'a, 'tcx> {
fn downcast_subpath(&self, _path: Self::Path, _variant: usize) -> Option<Self::Path> {
Some(())
}
fn array_subpath(&self, _path: Self::Path, _index: u32, _size: u32) -> Option<Self::Path> {
None
fn array_subpaths(&self, _path: Self::Path, _size: u64)
-> Vec<(Place<'tcx>, Option<Self::Path>, Option<Local>)> {
vec![]
}
}

Expand Down
168 changes: 144 additions & 24 deletions src/librustc_mir/transform/elaborate_drops.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ use util::elaborate_drops::{DropElaborator, DropStyle, DropFlagMode};
use syntax::ast;
use syntax_pos::Span;

use std::fmt;
use std::{fmt, u32};

pub struct ElaborateDrops;

Expand Down Expand Up @@ -74,6 +74,7 @@ impl MirPass for ElaborateDrops {
flow_inits,
flow_uninits,
drop_flags: FxHashMap(),
array_items_drop_flags: FxHashMap(),
patch: MirPatch::new(mir),
}.elaborate()
};
Expand Down Expand Up @@ -224,6 +225,7 @@ impl<'a, 'b, 'tcx> DropElaborator<'a, 'tcx> for Elaborator<'a, 'b, 'tcx> {
((some_live, some_dead), children_count != 1)
}
};

match (maybe_live, maybe_dead, multipart) {
(false, _, _) => DropStyle::Dead,
(true, false, _) => DropStyle::Static,
Expand All @@ -232,7 +234,16 @@ impl<'a, 'b, 'tcx> DropElaborator<'a, 'tcx> for Elaborator<'a, 'b, 'tcx> {
}
}

fn clear_drop_flag(&mut self, loc: Location, path: Self::Path, mode: DropFlagMode) {
fn clear_drop_flag(
&mut self,
loc: Location,
path: Self::Path,
mode: DropFlagMode,
opt_flag: Option<Local>) {
if let Some(flag) = opt_flag {
self.ctxt.set_drop_flag_impl(loc, flag, DropFlagState::Absent);
return;
}
match mode {
DropFlagMode::Shallow => {
self.ctxt.set_drop_flag(loc, path, DropFlagState::Absent);
Expand All @@ -257,18 +268,38 @@ impl<'a, 'b, 'tcx> DropElaborator<'a, 'tcx> for Elaborator<'a, 'b, 'tcx> {
})
}

fn array_subpath(&self, path: Self::Path, index: u32, size: u32) -> Option<Self::Path> {
dataflow::move_path_children_matching(self.ctxt.move_data(), path, |p| {
match p {
&Projection {
elem: ProjectionElem::ConstantIndex{offset, min_length: _, from_end: false}, ..
} => offset == index,
&Projection {
elem: ProjectionElem::ConstantIndex{offset, min_length: _, from_end: true}, ..
} => size - offset == index,
_ => false
}
})
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,
Copy link
Contributor

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?

|_| true).is_none() {
return vec![];
}

assert!(size <= (u32::MAX as u64),
"move out check doesn't implemented for array bigger then u32");

let size = size as u32;
let flags = self.ctxt.array_items_drop_flags.get(&path);
(0..size).map(|i| {
let move_path = dataflow::move_path_children_matching(self.ctxt.move_data(), path, |p|
match p.elem {
ProjectionElem::ConstantIndex{offset, min_length: _, from_end: false} =>
offset == i,
ProjectionElem::ConstantIndex{offset, min_length: _, from_end: true} =>
size - offset == i,
ProjectionElem::Subslice{from, to} => from <= i && i < size - to,
_ => false
}
);
let place = &self.ctxt.move_data().move_paths[path].place;
(place.clone().elem(ProjectionElem::ConstantIndex{
offset: i,
min_length: size,
from_end: false
}),
move_path,
Copy link
Contributor

@arielb1 arielb1 Dec 14, 2017 •
edited
Loading

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

Copy link
Contributor

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])

Copy link
Contributor

@arielb1 arielb1 Dec 14, 2017 •
edited
Loading

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.

Copy link
Contributor

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.

move_path.and(flags.map(|f| f[i as usize])))
}).collect()
}

fn deref_subpath(&self, path: Self::Path) -> Option<Self::Path> {
Expand All @@ -292,17 +323,27 @@ impl<'a, 'b, 'tcx> DropElaborator<'a, 'tcx> for Elaborator<'a, 'b, 'tcx> {
}

fn get_drop_flag(&mut self, path: Self::Path) -> Option<Operand<'tcx>> {
self.ctxt.drop_flag(path).map(Operand::Copy)
self.ctxt.drop_flag(path).map(|f| match f{
DropFlag::Single(l) => Operand::Copy(Place::Local(*l)),
DropFlag::Subslice(_) =>
panic!("get_drop_flag shouldn't be calles for sublice move path")
})
}
}

enum DropFlag {
Single(Local),
Subslice(Vec<Local>),
}

struct ElaborateDropsCtxt<'a, 'tcx: 'a> {
tcx: TyCtxt<'a, 'tcx, 'tcx>,
mir: &'a Mir<'tcx>,
env: &'a MoveDataParamEnv<'tcx, 'tcx>,
flow_inits: DataflowResults<MaybeInitializedLvals<'a, 'tcx, 'tcx>>,
flow_uninits: DataflowResults<MaybeUninitializedLvals<'a, 'tcx, 'tcx>>,
drop_flags: FxHashMap<MovePathIndex, Local>,
drop_flags: FxHashMap<MovePathIndex, DropFlag>,
array_items_drop_flags: FxHashMap<MovePathIndex, Vec<Local>>,
patch: MirPatch<'tcx>,
}

Expand All @@ -329,15 +370,77 @@ impl<'b, 'tcx> ElaborateDropsCtxt<'b, 'tcx> {

fn create_drop_flag(&mut self, index: MovePathIndex, span: Span) {
let tcx = self.tcx;

match self.move_data().move_paths[index].place {
Place::Projection(box Projection{ref base,
elem: ProjectionElem::Subslice{from, to}}) => {
let base_ty = base.ty(self.mir, tcx).to_ty(tcx);
if let ty::TyArray(_, n) = base_ty.sty {
let n = n.val.to_const_int().and_then(|v| v.to_u64())
.expect("expected u64 size") as usize;
let from = from as usize;
let to = to as usize;
let flags = self.drop_flags_for_array(n, index)[from .. n-to]
.iter()
.map(|x| *x).collect();
let span = self.mir.span;
self.drop_flags.entry(index).or_insert_with(|| {
debug!("create_drop_flags for subslice({:?}, {:?}, {:?})",
index, span, flags);
DropFlag::Subslice(flags)
});
return;
}
}
Place::Projection(box Projection{ref base,
elem: ProjectionElem::ConstantIndex{offset,
min_length: _,
from_end}}) => {
let base_ty = base.ty(self.mir, tcx).to_ty(tcx);
if let ty::TyArray(_, n) = base_ty.sty {
let n = n.val.to_const_int().and_then(|v| v.to_u64())
.expect("expected u64 size") as usize;
let offset = offset as usize;
let offset = if from_end { n-offset } else { offset };
let flag = self.drop_flags_for_array(n, index)[offset];

let span = self.mir.span;
self.drop_flags.entry(index).or_insert_with(|| {
debug!("create_drop_flags for const index({:?}, {:?}, {:?})",
(offset, from_end), span, flag);
DropFlag::Single(flag)
});
return;
}
}
_ => {}
}

let patch = &mut self.patch;
debug!("create_drop_flag({:?})", self.mir.span);
self.drop_flags.entry(index).or_insert_with(|| {
patch.new_internal(tcx.types.bool, span)
DropFlag::Single(patch.new_internal(tcx.types.bool, span))
});
}

fn drop_flag(&mut self, index: MovePathIndex) -> Option<Place<'tcx>> {
self.drop_flags.get(&index).map(|t| Place::Local(*t))
fn drop_flags_for_array(&mut self, n: usize, index: MovePathIndex) -> &Vec<Local> {
let tcx = self.tcx;
let span = self.mir.span;
let parent_index = self.move_data().move_paths[index].parent
.expect("subslice has parent");
let patch = &mut self.patch;
self.array_items_drop_flags.entry(parent_index)
.or_insert_with(|| {
let flags = (0..n).map(|_| patch.new_internal(tcx.types.bool, span))
.collect();
debug!("create_drop_flags for array with subslice({:?}, {:?}, {:?})",
parent_index, span, flags);
flags
})
}

fn drop_flag(&mut self, index: MovePathIndex) -> Option<&DropFlag> {
self.drop_flags.get(&index)
}

/// create a patch that elaborates all drops in the input
Expand Down Expand Up @@ -389,6 +492,7 @@ impl<'b, 'tcx> ElaborateDropsCtxt<'b, 'tcx> {
}
};


Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: stray newline

on_all_drop_children_bits(self.tcx, self.mir, self.env, path, |child| {
let (maybe_live, maybe_dead) = init_data.state(child);
debug!("collect_drop_flags: collecting {:?} from {:?}@{:?} - {:?}",
Expand Down Expand Up @@ -548,11 +652,20 @@ impl<'b, 'tcx> ElaborateDropsCtxt<'b, 'tcx> {
})))
}

fn set_drop_flag_impl(&mut self, loc: Location, flag: Local, val: DropFlagState) {
let span = self.patch.source_info_for_location(self.mir, loc).span;
let val = self.constant_bool(span, val.value());
self.patch.add_assign(loc, Place::Local(flag), val);
}

fn set_drop_flag(&mut self, loc: Location, path: MovePathIndex, val: DropFlagState) {
if let Some(&flag) = self.drop_flags.get(&path) {
let span = self.patch.source_info_for_location(self.mir, loc).span;
let val = self.constant_bool(span, val.value());
self.patch.add_assign(loc, Place::Local(flag), val);
match self.drop_flags.get(&path) {
Some(DropFlag::Single(flag)) => self.set_drop_flag_impl(loc, *flag, val),
Some(DropFlag::Subslice(flags)) =>
for flag in flags {
self.set_drop_flag_impl(loc, *flag, val);
}
_ => {}
}
}

Expand All @@ -561,7 +674,14 @@ impl<'b, 'tcx> ElaborateDropsCtxt<'b, 'tcx> {
let span = self.patch.source_info_for_location(self.mir, loc).span;
let false_ = self.constant_bool(span, false);
for flag in self.drop_flags.values() {
self.patch.add_assign(loc, Place::Local(*flag), false_.clone());
match flag {
DropFlag::Single(flag) =>
self.patch.add_assign(loc, Place::Local(*flag), false_.clone()),
DropFlag::Subslice(flags) =>
for flag in flags {
self.patch.add_assign(loc, Place::Local(*flag), false_.clone());
},
}
}
}

Expand Down
Loading