From 673d97e3ca764e4fd9a3c6389f5f0f2e19e0584d Mon Sep 17 00:00:00 2001 From: David Wood Date: Sat, 27 Jan 2018 20:15:18 +0000 Subject: [PATCH 1/6] Added tests for #47703 --- src/test/run-pass/issue-47703-1.rs | 33 ++++++++++++++++++++++++++++++ src/test/run-pass/issue-47703.rs | 28 +++++++++++++++++++++++++ 2 files changed, 61 insertions(+) create mode 100644 src/test/run-pass/issue-47703-1.rs create mode 100644 src/test/run-pass/issue-47703.rs diff --git a/src/test/run-pass/issue-47703-1.rs b/src/test/run-pass/issue-47703-1.rs new file mode 100644 index 0000000000000..facdee5cc176f --- /dev/null +++ b/src/test/run-pass/issue-47703-1.rs @@ -0,0 +1,33 @@ +// Copyright 2012 The Rust Project Developers. See the COPYRIGHT +// file at the top-level directory of this distribution and at +// http://rust-lang.org/COPYRIGHT. +// +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +#![feature(nll)] + +struct AtomicRefMut<'a> { + value: &'a mut i32, + borrow: AtomicBorrowRefMut, +} + +struct AtomicBorrowRefMut { +} + +impl Drop for AtomicBorrowRefMut { + fn drop(&mut self) { + } +} + +fn map(orig: AtomicRefMut) -> AtomicRefMut { + AtomicRefMut { + value: orig.value, + borrow: orig.borrow, + } +} + +fn main() {} diff --git a/src/test/run-pass/issue-47703.rs b/src/test/run-pass/issue-47703.rs new file mode 100644 index 0000000000000..2146986377a05 --- /dev/null +++ b/src/test/run-pass/issue-47703.rs @@ -0,0 +1,28 @@ +// Copyright 2012 The Rust Project Developers. See the COPYRIGHT +// file at the top-level directory of this distribution and at +// http://rust-lang.org/COPYRIGHT. +// +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +#![feature(nll)] + +struct MyStruct<'a> { + field: &'a mut (), + field2: WithDrop +} + +struct WithDrop; + +impl Drop for WithDrop { + fn drop(&mut self) {} +} + +impl<'a> MyStruct<'a> { + fn consume(self) -> &'a mut () { self.field } +} + +fn main() {} From 6493ab6f2aa709c019baf640e31699535679553d Mon Sep 17 00:00:00 2001 From: David Wood Date: Wed, 31 Jan 2018 23:46:05 +0000 Subject: [PATCH 2/6] Fixed incorrect reporting of errors when checking borrows in drops. --- src/librustc_mir/borrow_check/mod.rs | 52 ++++++++++++++++++++++------ 1 file changed, 41 insertions(+), 11 deletions(-) diff --git a/src/librustc_mir/borrow_check/mod.rs b/src/librustc_mir/borrow_check/mod.rs index c4df7349391e2..e321661e40ef0 100644 --- a/src/librustc_mir/borrow_check/mod.rs +++ b/src/librustc_mir/borrow_check/mod.rs @@ -2073,7 +2073,7 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { /// currently in, when such distinction matters. fn each_borrow_involving_path( &mut self, - _context: Context, + context: Context, access_place: (ShallowOrDeep, &Place<'tcx>), flow_state: &Flows<'cx, 'gcx, 'tcx>, mut op: F, @@ -2085,20 +2085,50 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { // FIXME: analogous code in check_loans first maps `place` to // its base_path. + // When this function is called as a result of an `access_terminator` call attempting + // to drop a struct, if that struct does not have a destructor, then we need to check + // each of the fields in the struct. See #47703. + let (access, places) = if let ContextKind::Drop = context.kind { + let ty = place.ty(self.mir, self.tcx).to_ty(self.tcx); + + match ty.sty { + ty::TyAdt(def, substs) if def.is_struct() && !def.has_dtor(self.tcx) => { + let mut places = Vec::new(); + + for (index, field) in def.all_fields().enumerate() { + let proj = Projection { + base: place.clone(), + elem: ProjectionElem::Field(Field::new(index), + field.ty(self.tcx, substs)), + }; + + places.push(Place::Projection(Box::new(proj))); + } + + (ShallowOrDeep::Shallow(None), places) + }, + _ => (access, vec![ place.clone() ]), + } + } else { + (access, vec![ place.clone() ]) + }; + let data = flow_state.borrows.operator().borrows(); // check for loan restricting path P being used. Accounts for // borrows of P, P.a.b, etc. - let mut elems_incoming = flow_state.borrows.elems_incoming(); - while let Some(i) = elems_incoming.next() { - let borrowed = &data[i.borrow_index()]; - - if self.places_conflict(&borrowed.borrowed_place, place, access) { - debug!("each_borrow_involving_path: {:?} @ {:?} vs. {:?}/{:?}", - i, borrowed, place, access); - let ctrl = op(self, i, borrowed); - if ctrl == Control::Break { - return; + for place in places { + let mut elems_incoming = flow_state.borrows.elems_incoming(); + while let Some(i) = elems_incoming.next() { + let borrowed = &data[i.borrow_index()]; + + if self.places_conflict(&borrowed.borrowed_place, &place, access) { + debug!("each_borrow_involving_path: {:?} @ {:?} vs. {:?}/{:?}", + i, borrowed, place, access); + let ctrl = op(self, i, borrowed); + if ctrl == Control::Break { + return; + } } } } From e7f328ea574bbfc3e927be01eb5f7b66bae5a578 Mon Sep 17 00:00:00 2001 From: David Wood Date: Thu, 1 Feb 2018 12:27:56 +0000 Subject: [PATCH 3/6] Handle recursive case of dropping structs with field accesses when struct has no dtor. --- src/librustc_mir/borrow_check/mod.rs | 119 ++++++++++++++++----------- 1 file changed, 71 insertions(+), 48 deletions(-) diff --git a/src/librustc_mir/borrow_check/mod.rs b/src/librustc_mir/borrow_check/mod.rs index e321661e40ef0..af834ede1ee33 100644 --- a/src/librustc_mir/borrow_check/mod.rs +++ b/src/librustc_mir/borrow_check/mod.rs @@ -463,13 +463,7 @@ impl<'cx, 'gcx, 'tcx> DataflowResultsConsumer<'cx, 'tcx> for MirBorrowckCtxt<'cx target: _, unwind: _, } => { - self.access_place( - ContextKind::Drop.new(loc), - (drop_place, span), - (Deep, Write(WriteKind::StorageDeadOrDrop)), - LocalMutationIsAllowed::Yes, - flow_state, - ); + self.visit_terminator_drop(loc, term, flow_state, drop_place, span); } TerminatorKind::DropAndReplace { location: ref drop_place, @@ -717,6 +711,65 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { self.tcx.sess.opts.debugging_opts.two_phase_beyond_autoref) } + fn visit_terminator_drop( + &mut self, + loc: Location, + term: &Terminator<'tcx>, + flow_state: &Flows<'cx, 'gcx, 'tcx>, + drop_place: &Place<'tcx>, + span: Span, + ) { + let ty = drop_place.ty(self.mir, self.tcx).to_ty(self.tcx); + match ty.sty { + // When a struct is being dropped, we need to check whether it has a + // destructor, if it does, then we can call it, if it does not then we + // need to check the individual fields instead. + // See #47703. + ty::TyAdt(def, substs) if def.is_struct() && !def.has_dtor(self.tcx) => { + for (index, field) in def.all_fields().enumerate() { + let field_ty = field.ty(self.tcx, substs); + let proj = Projection { + base: drop_place.clone(), + elem: ProjectionElem::Field(Field::new(index), field_ty), + }; + let place = Place::Projection(Box::new(proj)); + + match field_ty.sty { + // It may be the case that this issue occurs with a struct within a + // struct, so we recurse to handle that. + ty::TyAdt(def, _) if def.is_struct() && !def.has_dtor(self.tcx) => { + self.visit_terminator_drop( + loc, + term, + flow_state, + &place, + span, + ); + }, + _ => { + self.access_place( + ContextKind::Drop.new(loc), + (&place, span), + (Shallow(None), Write(WriteKind::StorageDeadOrDrop)), + LocalMutationIsAllowed::Yes, + flow_state, + ); + }, + } + } + }, + _ => { + self.access_place( + ContextKind::Drop.new(loc), + (drop_place, span), + (Deep, Write(WriteKind::StorageDeadOrDrop)), + LocalMutationIsAllowed::Yes, + flow_state, + ); + }, + } + } + /// Checks an access to the given place to see if it is allowed. Examines the set of borrows /// that are in scope, as well as which paths have been initialized, to ensure that (a) the /// place is initialized and (b) it is not borrowed in some way that would prevent this @@ -2073,7 +2126,7 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { /// currently in, when such distinction matters. fn each_borrow_involving_path( &mut self, - context: Context, + _context: Context, access_place: (ShallowOrDeep, &Place<'tcx>), flow_state: &Flows<'cx, 'gcx, 'tcx>, mut op: F, @@ -2085,50 +2138,20 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { // FIXME: analogous code in check_loans first maps `place` to // its base_path. - // When this function is called as a result of an `access_terminator` call attempting - // to drop a struct, if that struct does not have a destructor, then we need to check - // each of the fields in the struct. See #47703. - let (access, places) = if let ContextKind::Drop = context.kind { - let ty = place.ty(self.mir, self.tcx).to_ty(self.tcx); - - match ty.sty { - ty::TyAdt(def, substs) if def.is_struct() && !def.has_dtor(self.tcx) => { - let mut places = Vec::new(); - - for (index, field) in def.all_fields().enumerate() { - let proj = Projection { - base: place.clone(), - elem: ProjectionElem::Field(Field::new(index), - field.ty(self.tcx, substs)), - }; - - places.push(Place::Projection(Box::new(proj))); - } - - (ShallowOrDeep::Shallow(None), places) - }, - _ => (access, vec![ place.clone() ]), - } - } else { - (access, vec![ place.clone() ]) - }; - let data = flow_state.borrows.operator().borrows(); // check for loan restricting path P being used. Accounts for // borrows of P, P.a.b, etc. - for place in places { - let mut elems_incoming = flow_state.borrows.elems_incoming(); - while let Some(i) = elems_incoming.next() { - let borrowed = &data[i.borrow_index()]; - - if self.places_conflict(&borrowed.borrowed_place, &place, access) { - debug!("each_borrow_involving_path: {:?} @ {:?} vs. {:?}/{:?}", - i, borrowed, place, access); - let ctrl = op(self, i, borrowed); - if ctrl == Control::Break { - return; - } + let mut elems_incoming = flow_state.borrows.elems_incoming(); + while let Some(i) = elems_incoming.next() { + let borrowed = &data[i.borrow_index()]; + + if self.places_conflict(&borrowed.borrowed_place, &place, access) { + debug!("each_borrow_involving_path: {:?} @ {:?} vs. {:?}/{:?}", + i, borrowed, place, access); + let ctrl = op(self, i, borrowed); + if ctrl == Control::Break { + return; } } } From 5fd64dde2198971df0869cbf4944d3996395b826 Mon Sep 17 00:00:00 2001 From: David Wood Date: Mon, 5 Feb 2018 17:04:25 +0000 Subject: [PATCH 4/6] Simplified logic and corrected shallow to deep. --- src/librustc_mir/borrow_check/mod.rs | 40 ++++++++-------------------- 1 file changed, 11 insertions(+), 29 deletions(-) diff --git a/src/librustc_mir/borrow_check/mod.rs b/src/librustc_mir/borrow_check/mod.rs index af834ede1ee33..46f94f2388ffe 100644 --- a/src/librustc_mir/borrow_check/mod.rs +++ b/src/librustc_mir/borrow_check/mod.rs @@ -727,35 +727,17 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { // See #47703. ty::TyAdt(def, substs) if def.is_struct() && !def.has_dtor(self.tcx) => { for (index, field) in def.all_fields().enumerate() { - let field_ty = field.ty(self.tcx, substs); - let proj = Projection { - base: drop_place.clone(), - elem: ProjectionElem::Field(Field::new(index), field_ty), - }; - let place = Place::Projection(Box::new(proj)); - - match field_ty.sty { - // It may be the case that this issue occurs with a struct within a - // struct, so we recurse to handle that. - ty::TyAdt(def, _) if def.is_struct() && !def.has_dtor(self.tcx) => { - self.visit_terminator_drop( - loc, - term, - flow_state, - &place, - span, - ); - }, - _ => { - self.access_place( - ContextKind::Drop.new(loc), - (&place, span), - (Shallow(None), Write(WriteKind::StorageDeadOrDrop)), - LocalMutationIsAllowed::Yes, - flow_state, - ); - }, - } + let place = drop_place.clone(); + let place = place.field(Field::new(index), field.ty(self.tcx, substs)); + let place = place.deref(); + + self.visit_terminator_drop( + loc, + term, + flow_state, + &place, + span, + ); } }, _ => { From 552024a52eb17b2542e87f9fc97b7b643707d759 Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Mon, 5 Feb 2018 14:49:27 -0500 Subject: [PATCH 5/6] check that types "need drop" before we access them Also, add some comments and remove extra deref. --- src/librustc_mir/borrow_check/mod.rs | 40 +++++++++++++++++++--------- 1 file changed, 28 insertions(+), 12 deletions(-) diff --git a/src/librustc_mir/borrow_check/mod.rs b/src/librustc_mir/borrow_check/mod.rs index 46f94f2388ffe..3cdfe64000624 100644 --- a/src/librustc_mir/borrow_check/mod.rs +++ b/src/librustc_mir/borrow_check/mod.rs @@ -711,6 +711,12 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { self.tcx.sess.opts.debugging_opts.two_phase_beyond_autoref) } + /// Invokes `access_place` as appropriate for dropping the value + /// at `drop_place`. Note that the *actual* `Drop` in the MIR is + /// always for a variable (e.g., `Drop(x)`) -- but we recursively + /// break this variable down into subpaths (e.g., `Drop(x.foo)`) + /// to indicate more precisely which fields might actually be + /// accessed by a destructor. fn visit_terminator_drop( &mut self, loc: Location, @@ -721,15 +727,16 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { ) { let ty = drop_place.ty(self.mir, self.tcx).to_ty(self.tcx); match ty.sty { - // When a struct is being dropped, we need to check whether it has a - // destructor, if it does, then we can call it, if it does not then we - // need to check the individual fields instead. - // See #47703. + // When a struct is being dropped, we need to check + // whether it has a destructor, if it does, then we can + // call it, if it does not then we need to check the + // individual fields instead. This way if `foo` has a + // destructor but `bar` does not, we will only check for + // borrows of `x.foo` and not `x.bar`. See #47703. ty::TyAdt(def, substs) if def.is_struct() && !def.has_dtor(self.tcx) => { for (index, field) in def.all_fields().enumerate() { let place = drop_place.clone(); let place = place.field(Field::new(index), field.ty(self.tcx, substs)); - let place = place.deref(); self.visit_terminator_drop( loc, @@ -741,13 +748,22 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { } }, _ => { - self.access_place( - ContextKind::Drop.new(loc), - (drop_place, span), - (Deep, Write(WriteKind::StorageDeadOrDrop)), - LocalMutationIsAllowed::Yes, - flow_state, - ); + // We have now refined the type of the value being + // dropped (potentially) to just the type of a + // subfield; so check whether that field's type still + // "needs drop". If so, we assume that the destructor + // may access any data it likes (i.e., a Deep Write). + let gcx = self.tcx.global_tcx(); + let erased_ty = gcx.lift(&self.tcx.erase_regions(&ty)).unwrap(); + if erased_ty.needs_drop(gcx, self.param_env) { + self.access_place( + ContextKind::Drop.new(loc), + (drop_place, span), + (Deep, Write(WriteKind::StorageDeadOrDrop)), + LocalMutationIsAllowed::Yes, + flow_state, + ); + } }, } } From 98904c2ecc580b27f87e43dbe3e7f6995a2ec3a2 Mon Sep 17 00:00:00 2001 From: David Wood Date: Wed, 14 Feb 2018 18:14:31 +0000 Subject: [PATCH 6/6] Normalizing associated types when checking borrows in drops. --- src/librustc_mir/borrow_check/mod.rs | 31 +++++++++++++++++++++------- 1 file changed, 23 insertions(+), 8 deletions(-) diff --git a/src/librustc_mir/borrow_check/mod.rs b/src/librustc_mir/borrow_check/mod.rs index 3cdfe64000624..650f99828ae48 100644 --- a/src/librustc_mir/borrow_check/mod.rs +++ b/src/librustc_mir/borrow_check/mod.rs @@ -463,7 +463,20 @@ impl<'cx, 'gcx, 'tcx> DataflowResultsConsumer<'cx, 'tcx> for MirBorrowckCtxt<'cx target: _, unwind: _, } => { - self.visit_terminator_drop(loc, term, flow_state, drop_place, span); + let gcx = self.tcx.global_tcx(); + + // Compute the type with accurate region information. + let drop_place_ty = drop_place.ty(self.mir, self.tcx); + + // Erase the regions. + let drop_place_ty = self.tcx.erase_regions(&drop_place_ty).to_ty(self.tcx); + + // "Lift" into the gcx -- once regions are erased, this type should be in the + // global arenas; this "lift" operation basically just asserts that is true, but + // that is useful later. + let drop_place_ty = gcx.lift(&drop_place_ty).unwrap(); + + self.visit_terminator_drop(loc, term, flow_state, drop_place, drop_place_ty, span); } TerminatorKind::DropAndReplace { location: ref drop_place, @@ -723,10 +736,10 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { term: &Terminator<'tcx>, flow_state: &Flows<'cx, 'gcx, 'tcx>, drop_place: &Place<'tcx>, + erased_drop_place_ty: ty::Ty<'gcx>, span: Span, ) { - let ty = drop_place.ty(self.mir, self.tcx).to_ty(self.tcx); - match ty.sty { + match erased_drop_place_ty.sty { // When a struct is being dropped, we need to check // whether it has a destructor, if it does, then we can // call it, if it does not then we need to check the @@ -735,14 +748,17 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { // borrows of `x.foo` and not `x.bar`. See #47703. ty::TyAdt(def, substs) if def.is_struct() && !def.has_dtor(self.tcx) => { for (index, field) in def.all_fields().enumerate() { - let place = drop_place.clone(); - let place = place.field(Field::new(index), field.ty(self.tcx, substs)); + let gcx = self.tcx.global_tcx(); + let field_ty = field.ty(gcx, substs); + let field_ty = gcx.normalize_associated_type_in_env(&field_ty, self.param_env); + let place = drop_place.clone().field(Field::new(index), field_ty); self.visit_terminator_drop( loc, term, flow_state, &place, + field_ty, span, ); } @@ -754,8 +770,7 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { // "needs drop". If so, we assume that the destructor // may access any data it likes (i.e., a Deep Write). let gcx = self.tcx.global_tcx(); - let erased_ty = gcx.lift(&self.tcx.erase_regions(&ty)).unwrap(); - if erased_ty.needs_drop(gcx, self.param_env) { + if erased_drop_place_ty.needs_drop(gcx, self.param_env) { self.access_place( ContextKind::Drop.new(loc), (drop_place, span), @@ -2144,7 +2159,7 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { while let Some(i) = elems_incoming.next() { let borrowed = &data[i.borrow_index()]; - if self.places_conflict(&borrowed.borrowed_place, &place, access) { + if self.places_conflict(&borrowed.borrowed_place, place, access) { debug!("each_borrow_involving_path: {:?} @ {:?} vs. {:?}/{:?}", i, borrowed, place, access); let ctrl = op(self, i, borrowed);