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

Local is copy #68512

Merged
merged 9 commits into from
Jan 29, 2020
Merged

Local is copy #68512

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
a71cc4c
Place conflict functions take Local by value
spastorino Jan 14, 2020
4a86c55
record_killed_borrows_for_local takes Local by value
spastorino Jan 14, 2020
10b19f6
make_integrate_local takes Local by value
spastorino Jan 14, 2020
39d93b1
codegen_place and related functions can take PlaceRef by value
spastorino Jan 14, 2020
b5b6be0
Local field on PlaceRef and RootPlace is not a reference anymore
spastorino Jan 14, 2020
22a4827
Place::ty_from takes local by value
spastorino Jan 14, 2020
5f1caa4
./x.py fmt
spastorino Jan 24, 2020
a13a7d7
Remove unneeded & on match pattern
spastorino Jan 24, 2020
3021856
Fix some wrong dereferences after rebase
spastorino Jan 28, 2020
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
16 changes: 8 additions & 8 deletions src/librustc/mir/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1773,7 +1773,7 @@ rustc_index::newtype_index! {

#[derive(Clone, Copy, Debug, PartialEq, Eq, PartialOrd, Ord, Hash)]
pub struct PlaceRef<'a, 'tcx> {
pub local: &'a Local,
pub local: Local,
pub projection: &'a [PlaceElem<'tcx>],
Copy link
Contributor

Choose a reason for hiding this comment

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

Orthogonal, but the 'a here could be 'tcx, right?

Copy link
Member Author

@spastorino spastorino Jan 24, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Unsure, I just did it to try out and I'm getting ...

error[E0597]: `deref` does not live long enough
    --> src/librustc_mir/borrow_check/mod.rs:1400:41
     |
855  |   impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
     |             ---- lifetime `'tcx` defined here
...
1400 |                   root_place.projection = &deref;
     |                                           ^^^^^^ borrowed value does not live long enough
...
1413 |           if places_conflict::borrow_conflicts_with_place(
     |  ____________-
1414 | |             self.infcx.tcx,
1415 | |             &self.body,
1416 | |             place,
...    |
1420 | |             places_conflict::PlaceConflictBias::Overlap,
1421 | |         ) {
     | |_________- argument requires that `deref` is borrowed for `'tcx`
...
1433 |       }
     |       - `deref` dropped here while still borrowed

error[E0621]: explicit lifetime required in the type of `issued_borrow`
   --> src/librustc_mir/borrow_check/diagnostics/conflict_errors.rs:547:9
    |
344 |         issued_borrow: &BorrowData<'tcx>,
    |                        ----------------- help: add explicit lifetime `'cx` to the type of `issued_borrow`: `&'cx borrow_check::borrow_set::BorrowData<'tcx>`
...
547 |         err
    |         ^^^ lifetime `'cx` required

error: aborting due to 2 previous errors

Some errors have detailed explanations: E0597, E0621.
For more information about an error, try `rustc --explain E0597`.
error: could not compile `rustc_mir`.

To learn more, run the command again with --verbose.

Didn't pay a lot of attention to be honest, should I fix this in this PR or investigate this on a different one?.

Copy link
Contributor

Choose a reason for hiding this comment

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

no let's do it in another PR.

Copy link
Member

Choose a reason for hiding this comment

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

I think you should start by replacing PlaceRef<'_, 'tcx> with PlaceRef<'tcx, 'tcx> and see where that breaks down (most things should be able to handle it, although... unsure how useful it is?).

Copy link
Member Author

Choose a reason for hiding this comment

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

This was addressed here #69714

}

Expand All @@ -1797,8 +1797,8 @@ impl<'tcx> Place<'tcx> {
// FIXME: can we safely swap the semantics of `fn base_local` below in here instead?
pub fn local_or_deref_local(&self) -> Option<Local> {
match self.as_ref() {
PlaceRef { local, projection: &[] }
| PlaceRef { local, projection: &[ProjectionElem::Deref] } => Some(*local),
PlaceRef { local, projection: [] }
| PlaceRef { local, projection: [ProjectionElem::Deref] } => Some(local),
_ => None,
}
}
Expand All @@ -1810,7 +1810,7 @@ impl<'tcx> Place<'tcx> {
}

pub fn as_ref(&self) -> PlaceRef<'_, 'tcx> {
PlaceRef { local: &self.local, projection: &self.projection }
PlaceRef { local: self.local, projection: &self.projection }
}
}

Expand All @@ -1826,18 +1826,18 @@ impl<'a, 'tcx> PlaceRef<'a, 'tcx> {
//
// FIXME: can we safely swap the semantics of `fn base_local` below in here instead?
pub fn local_or_deref_local(&self) -> Option<Local> {
match self {
match *self {
PlaceRef { local, projection: [] }
| PlaceRef { local, projection: [ProjectionElem::Deref] } => Some(**local),
| PlaceRef { local, projection: [ProjectionElem::Deref] } => Some(local),
_ => None,
}
}

/// If this place represents a local variable like `_X` with no
/// projections, return `Some(_X)`.
pub fn as_local(&self) -> Option<Local> {
match self {
PlaceRef { local, projection: [] } => Some(**local),
match *self {
PlaceRef { local, projection: [] } => Some(local),
_ => None,
}
}
Expand Down
6 changes: 3 additions & 3 deletions src/librustc/mir/tcx.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,7 @@ impl<'tcx> PlaceTy<'tcx> {

impl<'tcx> Place<'tcx> {
pub fn ty_from<D>(
local: &Local,
local: Local,
projection: &[PlaceElem<'tcx>],
local_decls: &D,
tcx: TyCtxt<'tcx>,
Expand All @@ -124,7 +124,7 @@ impl<'tcx> Place<'tcx> {
{
projection
.iter()
.fold(PlaceTy::from_ty(local_decls.local_decls()[*local].ty), |place_ty, elem| {
.fold(PlaceTy::from_ty(local_decls.local_decls()[local].ty), |place_ty, elem| {
place_ty.projection_ty(tcx, elem)
})
}
Expand All @@ -133,7 +133,7 @@ impl<'tcx> Place<'tcx> {
where
D: HasLocalDecls<'tcx>,
{
Place::ty_from(&self.local, &self.projection, local_decls, tcx)
Place::ty_from(self.local, &self.projection, local_decls, tcx)
}
}

Expand Down
8 changes: 4 additions & 4 deletions src/librustc_codegen_ssa/mir/analyze.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,7 @@ impl<Bx: BuilderMethods<'a, 'tcx>> LocalAnalyzer<'mir, 'a, 'tcx, Bx> {
// ZSTs don't require any actual memory access.
let elem_ty = base_ty.projection_ty(cx.tcx(), elem).ty;
let elem_ty = self.fx.monomorphize(&elem_ty);
let span = self.fx.mir.local_decls[*place_ref.local].source_info.span;
let span = self.fx.mir.local_decls[place_ref.local].source_info.span;
if cx.spanned_layout_of(elem_ty, span).is_zst() {
return;
}
Expand Down Expand Up @@ -174,7 +174,7 @@ impl<Bx: BuilderMethods<'a, 'tcx>> LocalAnalyzer<'mir, 'a, 'tcx, Bx> {
// We use `NonUseContext::VarDebugInfo` for the base,
// which might not force the base local to memory,
// so we have to do it manually.
self.visit_local(place_ref.local, context, location);
self.visit_local(&place_ref.local, context, location);
Copy link
Member Author

Choose a reason for hiding this comment

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

This should also take a Local instead of &Local but I'd leave that for a deeper change related to visitors.

}
}

Expand Down Expand Up @@ -212,8 +212,8 @@ impl<Bx: BuilderMethods<'a, 'tcx>> LocalAnalyzer<'mir, 'a, 'tcx, Bx> {
};
}

self.visit_place_base(place_ref.local, context, location);
self.visit_projection(place_ref.local, place_ref.projection, context, location);
self.visit_place_base(&place_ref.local, context, location);
self.visit_projection(&place_ref.local, place_ref.projection, context, location);
}
}
}
Expand Down
10 changes: 5 additions & 5 deletions src/librustc_codegen_ssa/mir/block.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -264,7 +264,7 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
}

PassMode::Direct(_) | PassMode::Pair(..) => {
let op = self.codegen_consume(&mut bx, &mir::Place::return_place().as_ref());
let op = self.codegen_consume(&mut bx, mir::Place::return_place().as_ref());
if let Ref(llval, _, align) = op.val {
bx.load(llval, align)
} else {
Expand Down Expand Up @@ -319,7 +319,7 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
return;
}

let place = self.codegen_place(&mut bx, &location.as_ref());
let place = self.codegen_place(&mut bx, location.as_ref());
let (args1, args2);
let mut args = if let Some(llextra) = place.llextra {
args2 = [place.llval, llextra];
Expand Down Expand Up @@ -1111,7 +1111,7 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
} else {
self.codegen_place(
bx,
&mir::PlaceRef { local: &dest.local, projection: &dest.projection },
mir::PlaceRef { local: dest.local, projection: &dest.projection },
)
};
if fn_ret.is_indirect() {
Expand All @@ -1137,7 +1137,7 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
LocalRef::Place(place) => self.codegen_transmute_into(bx, src, place),
LocalRef::UnsizedPlace(_) => bug!("transmute must not involve unsized locals"),
LocalRef::Operand(None) => {
let dst_layout = bx.layout_of(self.monomorphized_place_ty(&dst.as_ref()));
let dst_layout = bx.layout_of(self.monomorphized_place_ty(dst.as_ref()));
assert!(!dst_layout.ty.has_erasable_regions());
let place = PlaceRef::alloca(bx, dst_layout);
place.storage_live(bx);
Expand All @@ -1151,7 +1151,7 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
}
}
} else {
let dst = self.codegen_place(bx, &dst.as_ref());
let dst = self.codegen_place(bx, dst.as_ref());
self.codegen_transmute_into(bx, src, dst);
}
}
Expand Down
8 changes: 4 additions & 4 deletions src/librustc_codegen_ssa/mir/operand.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -369,11 +369,11 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
fn maybe_codegen_consume_direct(
&mut self,
bx: &mut Bx,
place_ref: &mir::PlaceRef<'_, 'tcx>,
place_ref: mir::PlaceRef<'_, 'tcx>,
) -> Option<OperandRef<'tcx, Bx::Value>> {
debug!("maybe_codegen_consume_direct(place_ref={:?})", place_ref);

match self.locals[*place_ref.local] {
match self.locals[place_ref.local] {
LocalRef::Operand(Some(mut o)) => {
// Moves out of scalar and scalar pair fields are trivial.
for elem in place_ref.projection.iter() {
Expand Down Expand Up @@ -413,7 +413,7 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
pub fn codegen_consume(
&mut self,
bx: &mut Bx,
place_ref: &mir::PlaceRef<'_, 'tcx>,
place_ref: mir::PlaceRef<'_, 'tcx>,
) -> OperandRef<'tcx, Bx::Value> {
debug!("codegen_consume(place_ref={:?})", place_ref);

Expand Down Expand Up @@ -444,7 +444,7 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {

match *operand {
mir::Operand::Copy(ref place) | mir::Operand::Move(ref place) => {
self.codegen_consume(bx, &place.as_ref())
self.codegen_consume(bx, place.as_ref())
}

mir::Operand::Constant(ref constant) => {
Expand Down
10 changes: 5 additions & 5 deletions src/librustc_codegen_ssa/mir/place.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -408,14 +408,14 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
pub fn codegen_place(
&mut self,
bx: &mut Bx,
place_ref: &mir::PlaceRef<'_, 'tcx>,
place_ref: mir::PlaceRef<'_, 'tcx>,
) -> PlaceRef<'tcx, Bx::Value> {
debug!("codegen_place(place_ref={:?})", place_ref);
let cx = self.cx;
let tcx = self.cx.tcx();

let result = match place_ref {
mir::PlaceRef { local, projection: [] } => match self.locals[**local] {
mir::PlaceRef { local, projection: [] } => match self.locals[local] {
LocalRef::Place(place) => {
return place;
}
Expand All @@ -428,13 +428,13 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
},
mir::PlaceRef { local, projection: [proj_base @ .., mir::ProjectionElem::Deref] } => {
// Load the pointer from its location.
self.codegen_consume(bx, &mir::PlaceRef { local, projection: proj_base })
self.codegen_consume(bx, mir::PlaceRef { local, projection: proj_base })
.deref(bx.cx())
}
mir::PlaceRef { local, projection: [proj_base @ .., elem] } => {
// FIXME turn this recursion into iteration
let cg_base =
self.codegen_place(bx, &mir::PlaceRef { local, projection: proj_base });
self.codegen_place(bx, mir::PlaceRef { local, projection: proj_base });

match elem {
mir::ProjectionElem::Deref => bug!(),
Expand Down Expand Up @@ -497,7 +497,7 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
result
}

pub fn monomorphized_place_ty(&self, place_ref: &mir::PlaceRef<'_, 'tcx>) -> Ty<'tcx> {
pub fn monomorphized_place_ty(&self, place_ref: mir::PlaceRef<'_, 'tcx>) -> Ty<'tcx> {
let tcx = self.cx.tcx();
let place_ty = mir::Place::ty_from(place_ref.local, place_ref.projection, *self.mir, tcx);
self.monomorphize(&place_ty.ty)
Expand Down
6 changes: 3 additions & 3 deletions src/librustc_codegen_ssa/mir/rvalue.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -467,7 +467,7 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
mir::Rvalue::Discriminant(ref place) => {
let discr_ty = rvalue.ty(*self.mir, bx.tcx());
let discr = self
.codegen_place(&mut bx, &place.as_ref())
.codegen_place(&mut bx, place.as_ref())
.codegen_get_discr(&mut bx, discr_ty);
(
bx,
Expand Down Expand Up @@ -541,7 +541,7 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
}
}
// use common size calculation for non zero-sized types
let cg_value = self.codegen_place(bx, &place.as_ref());
let cg_value = self.codegen_place(bx, place.as_ref());
cg_value.len(bx.cx())
}

Expand All @@ -552,7 +552,7 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
place: &mir::Place<'tcx>,
mk_ptr_ty: impl FnOnce(TyCtxt<'tcx>, Ty<'tcx>) -> Ty<'tcx>,
) -> (Bx, OperandRef<'tcx, Bx::Value>) {
let cg_place = self.codegen_place(&mut bx, &place.as_ref());
let cg_place = self.codegen_place(&mut bx, place.as_ref());

let ty = cg_place.layout.ty;

Expand Down
6 changes: 3 additions & 3 deletions src/librustc_codegen_ssa/mir/statement.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,12 +41,12 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
}
}
} else {
let cg_dest = self.codegen_place(&mut bx, &place.as_ref());
let cg_dest = self.codegen_place(&mut bx, place.as_ref());
self.codegen_rvalue(bx, cg_dest, rvalue)
}
}
mir::StatementKind::SetDiscriminant { box ref place, variant_index } => {
self.codegen_place(&mut bx, &place.as_ref())
self.codegen_place(&mut bx, place.as_ref())
.codegen_set_discr(&mut bx, variant_index);
bx
}
Expand All @@ -70,7 +70,7 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
let outputs = asm
.outputs
.iter()
.map(|output| self.codegen_place(&mut bx, &output.as_ref()))
.map(|output| self.codegen_place(&mut bx, output.as_ref()))
.collect();

let input_vals = asm.inputs.iter().fold(
Expand Down
8 changes: 4 additions & 4 deletions src/librustc_mir/borrow_check/constraint_generation.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ impl<'cg, 'cx, 'tcx> Visitor<'tcx> for ConstraintGeneration<'cg, 'cx, 'tcx> {
));

// If there are borrows on this now dead local, we need to record them as `killed`.
if let StatementKind::StorageDead(ref local) = statement.kind {
if let StatementKind::StorageDead(local) = statement.kind {
record_killed_borrows_for_local(
all_facts,
self.borrow_set,
Expand Down Expand Up @@ -212,7 +212,7 @@ impl<'cx, 'cg, 'tcx> ConstraintGeneration<'cx, 'cg, 'tcx> {
local, location
);

if let Some(borrow_indices) = self.borrow_set.local_map.get(local) {
if let Some(borrow_indices) = self.borrow_set.local_map.get(&local) {
for &borrow_index in borrow_indices {
let places_conflict = places_conflict::places_conflict(
self.infcx.tcx,
Expand All @@ -239,10 +239,10 @@ fn record_killed_borrows_for_local(
all_facts: &mut AllFacts,
borrow_set: &BorrowSet<'_>,
location_table: &LocationTable,
local: &Local,
local: Local,
location: Location,
) {
if let Some(borrow_indices) = borrow_set.local_map.get(local) {
if let Some(borrow_indices) = borrow_set.local_map.get(&local) {
all_facts.killed.reserve(borrow_indices.len());
for &borrow_index in borrow_indices {
let location_index = location_table.mid_index(location);
Expand Down
32 changes: 19 additions & 13 deletions src/librustc_mir/borrow_check/diagnostics/conflict_errors.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -604,8 +604,13 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
cursor = proj_base;

match elem {
ProjectionElem::Field(field, _) if union_ty(local, proj_base).is_some() => {
return Some((PlaceRef { local, projection: proj_base }, field));
ProjectionElem::Field(field, _)
if union_ty(*local, proj_base).is_some() =>
{
return Some((
PlaceRef { local: *local, projection: proj_base },
field,
));
}
_ => {}
}
Expand All @@ -622,14 +627,17 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
cursor = proj_base;

if let ProjectionElem::Field(field, _) = elem {
if let Some(union_ty) = union_ty(local, proj_base) {
if let Some(union_ty) = union_ty(*local, proj_base) {
if field != target_field
&& local == target_base.local
&& *local == target_base.local
&& proj_base == target_base.projection
{
// FIXME when we avoid clone reuse describe_place closure
let describe_base_place = self
.describe_place(PlaceRef { local, projection: proj_base })
.describe_place(PlaceRef {
local: *local,
projection: proj_base,
})
.unwrap_or_else(|| "_".to_owned());

return Some((
Expand Down Expand Up @@ -686,12 +694,12 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
let borrow_span = borrow_spans.var_or_use();

assert!(root_place.projection.is_empty());
let proper_span = self.body.local_decls[*root_place.local].source_info.span;
let proper_span = self.body.local_decls[root_place.local].source_info.span;

let root_place_projection = self.infcx.tcx.intern_place_elems(root_place.projection);

if self.access_place_error_reported.contains(&(
Place { local: *root_place.local, projection: root_place_projection },
Place { local: root_place.local, projection: root_place_projection },
borrow_span,
)) {
debug!(
Expand All @@ -702,7 +710,7 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
}

self.access_place_error_reported.insert((
Place { local: *root_place.local, projection: root_place_projection },
Place { local: root_place.local, projection: root_place_projection },
borrow_span,
));

Expand Down Expand Up @@ -1139,7 +1147,7 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
let root_place =
self.prefixes(borrow.borrowed_place.as_ref(), PrefixSet::All).last().unwrap();
let local = root_place.local;
match self.body.local_kind(*local) {
match self.body.local_kind(local) {
LocalKind::ReturnPointer | LocalKind::Temp => {
("temporary value".to_string(), "temporary value created here".to_string())
}
Expand Down Expand Up @@ -1513,17 +1521,15 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
StorageDeadOrDrop::LocalStorageDead
| StorageDeadOrDrop::BoxedStorageDead => {
assert!(
Place::ty_from(&place.local, proj_base, *self.body, tcx)
.ty
.is_box(),
Place::ty_from(place.local, proj_base, *self.body, tcx).ty.is_box(),
"Drop of value behind a reference or raw pointer"
);
StorageDeadOrDrop::BoxedStorageDead
}
StorageDeadOrDrop::Destructor(_) => base_access,
},
ProjectionElem::Field(..) | ProjectionElem::Downcast(..) => {
let base_ty = Place::ty_from(&place.local, proj_base, *self.body, tcx).ty;
let base_ty = Place::ty_from(place.local, proj_base, *self.body, tcx).ty;
match base_ty.kind {
ty::Adt(def, _) if def.has_dtor(tcx) => {
// Report the outermost adt with a destructor
Expand Down
Loading