From 580467d3065c5520dbe6c8318a1a0e3519b01a3a Mon Sep 17 00:00:00 2001 From: bobtwinkles Date: Sun, 4 Mar 2018 21:47:39 -0500 Subject: [PATCH 1/9] Rename BorrowData::location to BorrowData::reserve_location in preparation for rewritting two phase borrow support --- src/librustc_mir/borrow_check/error_reporting.rs | 6 +++--- src/librustc_mir/dataflow/impls/borrows.rs | 11 +++++++---- 2 files changed, 10 insertions(+), 7 deletions(-) diff --git a/src/librustc_mir/borrow_check/error_reporting.rs b/src/librustc_mir/borrow_check/error_reporting.rs index 77a3f138fe590..a47ced010e604 100644 --- a/src/librustc_mir/borrow_check/error_reporting.rs +++ b/src/librustc_mir/borrow_check/error_reporting.rs @@ -250,7 +250,7 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { let new_closure_span = self.find_closure_span(span, context.loc); let span = new_closure_span.map(|(args, _)| args).unwrap_or(span); - let old_closure_span = self.find_closure_span(issued_span, issued_borrow.location); + let old_closure_span = self.find_closure_span(issued_span, issued_borrow.reserve_location); let issued_span = old_closure_span .map(|(args, _)| args) .unwrap_or(issued_span); @@ -380,7 +380,7 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { .last() .unwrap(); - let borrow_span = self.mir.source_info(borrow.location).span; + let borrow_span = self.mir.source_info(borrow.reserve_location).span; let proper_span = match *root_place { Place::Local(local) => self.mir.local_decls[local].source_info.span, _ => drop_span, @@ -817,7 +817,7 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { // Retrieve span of given borrow from the current MIR representation pub fn retrieve_borrow_span(&self, borrow: &BorrowData) -> Span { - self.mir.source_info(borrow.location).span + self.mir.source_info(borrow.reserve_location).span } // Retrieve type of a place for the current MIR representation diff --git a/src/librustc_mir/dataflow/impls/borrows.rs b/src/librustc_mir/dataflow/impls/borrows.rs index a150335a1ae4d..859618d27b33b 100644 --- a/src/librustc_mir/dataflow/impls/borrows.rs +++ b/src/librustc_mir/dataflow/impls/borrows.rs @@ -111,7 +111,9 @@ impl<'a, 'gcx, 'tcx> ActiveBorrows<'a, 'gcx, 'tcx> { #[allow(dead_code)] #[derive(Debug)] pub struct BorrowData<'tcx> { - pub(crate) location: Location, + /// Location where the borrow reservation starts. + /// In many cases, this will be equal to the activation location but not always. + pub(crate) reserve_location: Location, pub(crate) kind: mir::BorrowKind, pub(crate) region: Region<'tcx>, pub(crate) borrowed_place: mir::Place<'tcx>, @@ -209,7 +211,8 @@ impl<'a, 'gcx, 'tcx> Borrows<'a, 'gcx, 'tcx> { if is_unsafe_place(self.tcx, self.mir, borrowed_place) { return; } let borrow = BorrowData { - location, kind, region, + reserve_location: location, + kind, region, borrowed_place: borrowed_place.clone(), assigned_place: assigned_place.clone(), }; @@ -245,7 +248,7 @@ impl<'a, 'gcx, 'tcx> Borrows<'a, 'gcx, 'tcx> { let mut found_it = false; for idx in &self.region_map[region] { let bd = &self.idx_vec[*idx]; - if bd.location == location && + if bd.reserve_location == location && bd.kind == kind && bd.region == region && bd.borrowed_place == *place @@ -277,7 +280,7 @@ impl<'a, 'gcx, 'tcx> Borrows<'a, 'gcx, 'tcx> { pub fn scope_tree(&self) -> &Lrc { &self.scope_tree } pub fn location(&self, idx: BorrowIndex) -> &Location { - &self.borrows[idx].location + &self.borrows[idx].reserve_location } /// Add all borrows to the kill set, if those borrows are out of scope at `location`. From 138365368a71a04ff9539704f478b014379b7ee5 Mon Sep 17 00:00:00 2001 From: bobtwinkles Date: Mon, 5 Mar 2018 02:44:10 -0500 Subject: [PATCH 2/9] Finally start down the right path --- .../borrow_check/error_reporting.rs | 6 +- src/librustc_mir/borrow_check/flows.rs | 7 +- src/librustc_mir/borrow_check/mod.rs | 53 +- src/librustc_mir/dataflow/impls/borrows.rs | 716 +++++++++--------- src/librustc_mir/dataflow/mod.rs | 41 +- ...o-phase-activation-sharing-interference.rs | 4 +- ...o-phase-allow-access-during-reservation.rs | 6 +- .../borrowck/two-phase-nonrecv-autoref.rs | 16 +- ...-phase-reservation-sharing-interference.rs | 9 +- 9 files changed, 427 insertions(+), 431 deletions(-) diff --git a/src/librustc_mir/borrow_check/error_reporting.rs b/src/librustc_mir/borrow_check/error_reporting.rs index a47ced010e604..7ab52e98a0ee1 100644 --- a/src/librustc_mir/borrow_check/error_reporting.rs +++ b/src/librustc_mir/borrow_check/error_reporting.rs @@ -18,7 +18,7 @@ use rustc_data_structures::sync::Lrc; use super::{Context, MirBorrowckCtxt}; use super::{InitializationRequiringAction, PrefixSet}; -use dataflow::{ActiveBorrows, BorrowData, FlowAtLocation, MovingOutStatements}; +use dataflow::{Borrows, BorrowData, FlowAtLocation, MovingOutStatements}; use dataflow::move_paths::MovePathIndex; use util::borrowck_errors::{BorrowckErrors, Origin}; @@ -372,10 +372,10 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { context: Context, borrow: &BorrowData<'tcx>, drop_span: Span, - borrows: &ActiveBorrows<'cx, 'gcx, 'tcx>, + borrows: &Borrows<'cx, 'gcx, 'tcx> ) { let end_span = borrows.opt_region_end_span(&borrow.region); - let scope_tree = borrows.0.scope_tree(); + let scope_tree = borrows.scope_tree(); let root_place = self.prefixes(&borrow.borrowed_place, PrefixSet::All) .last() .unwrap(); diff --git a/src/librustc_mir/borrow_check/flows.rs b/src/librustc_mir/borrow_check/flows.rs index ba966c9d4e316..ceff380c594ed 100644 --- a/src/librustc_mir/borrow_check/flows.rs +++ b/src/librustc_mir/borrow_check/flows.rs @@ -17,13 +17,14 @@ use rustc::mir::{BasicBlock, Location}; use dataflow::{MaybeInitializedPlaces, MaybeUninitializedPlaces}; use dataflow::{EverInitializedPlaces, MovingOutStatements}; -use dataflow::{ActiveBorrows, FlowAtLocation, FlowsAtLocation}; +use dataflow::{Borrows}; +use dataflow::{FlowAtLocation, FlowsAtLocation}; use dataflow::move_paths::HasMoveData; use std::fmt; // (forced to be `pub` due to its use as an associated type below.) pub(crate) struct Flows<'b, 'gcx: 'tcx, 'tcx: 'b> { - pub borrows: FlowAtLocation>, + pub borrows: FlowAtLocation>, pub inits: FlowAtLocation>, pub uninits: FlowAtLocation>, pub move_outs: FlowAtLocation>, @@ -32,7 +33,7 @@ pub(crate) struct Flows<'b, 'gcx: 'tcx, 'tcx: 'b> { impl<'b, 'gcx, 'tcx> Flows<'b, 'gcx, 'tcx> { pub fn new( - borrows: FlowAtLocation>, + borrows: FlowAtLocation>, inits: FlowAtLocation>, uninits: FlowAtLocation>, move_outs: FlowAtLocation>, diff --git a/src/librustc_mir/borrow_check/mod.rs b/src/librustc_mir/borrow_check/mod.rs index e51b16a373618..3f111ebcb7802 100644 --- a/src/librustc_mir/borrow_check/mod.rs +++ b/src/librustc_mir/borrow_check/mod.rs @@ -34,11 +34,10 @@ use syntax_pos::Span; use dataflow::{do_dataflow, DebugFormatted}; use dataflow::FlowAtLocation; use dataflow::MoveDataParamEnv; -use dataflow::{DataflowAnalysis, DataflowResultsConsumer}; +use dataflow::{DataflowResultsConsumer}; use dataflow::{MaybeInitializedPlaces, MaybeUninitializedPlaces}; use dataflow::{EverInitializedPlaces, MovingOutStatements}; use dataflow::{BorrowData, Borrows, ReserveOrActivateIndex}; -use dataflow::{ActiveBorrows, Reservations}; use dataflow::indexes::BorrowIndex; use dataflow::move_paths::{IllegalMoveOriginKind, MoveError}; use dataflow::move_paths::{HasMoveData, LookupResult, MoveData, MovePathIndex}; @@ -54,8 +53,6 @@ mod error_reporting; mod flows; mod prefixes; -use std::borrow::Cow; - pub(crate) mod nll; pub fn provide(providers: &mut Providers) { @@ -209,6 +206,18 @@ fn do_mir_borrowck<'a, 'gcx, 'tcx>( }; let flow_inits = flow_inits; // remove mut + let flow_borrows = FlowAtLocation::new(do_dataflow( + tcx, + mir, + id, + &attributes, + &dead_unwinds, + Borrows::new(tcx, mir, opt_regioncx.clone(), def_id, body_id), + |rs, i| { + DebugFormatted::new(&(i.kind(), rs.location(i.borrow_index()))) + } + )); + let movable_generator = !match tcx.hir.get(id) { hir::map::Node::NodeExpr(&hir::Expr { node: hir::ExprClosure(.., Some(hir::GeneratorMovability::Static)), @@ -230,44 +239,12 @@ fn do_mir_borrowck<'a, 'gcx, 'tcx>( }, access_place_error_reported: FxHashSet(), reservation_error_reported: FxHashSet(), - nonlexical_regioncx: opt_regioncx.clone(), + nonlexical_regioncx: opt_regioncx, nonlexical_cause_info: None, }; - let borrows = Borrows::new(tcx, mir, opt_regioncx, def_id, body_id); - let flow_reservations = do_dataflow( - tcx, - mir, - id, - &attributes, - &dead_unwinds, - Reservations::new(borrows), - |rs, i| { - // In principle we could make the dataflow ensure that - // only reservation bits show up, and assert so here. - // - // In practice it is easier to be looser; in particular, - // it is okay for the kill-sets to hold activation bits. - DebugFormatted::new(&(i.kind(), rs.location(i))) - }, - ); - let flow_active_borrows = { - let reservations_on_entry = flow_reservations.0.sets.entry_set_state(); - let reservations = flow_reservations.0.operator; - let a = DataflowAnalysis::new_with_entry_sets( - mir, - &dead_unwinds, - Cow::Borrowed(reservations_on_entry), - ActiveBorrows::new(reservations), - ); - let results = a.run(tcx, id, &attributes, |ab, i| { - DebugFormatted::new(&(i.kind(), ab.location(i))) - }); - FlowAtLocation::new(results) - }; - let mut state = Flows::new( - flow_active_borrows, + flow_borrows, flow_inits, flow_uninits, flow_move_outs, diff --git a/src/librustc_mir/dataflow/impls/borrows.rs b/src/librustc_mir/dataflow/impls/borrows.rs index 859618d27b33b..4b63d8e2ff78a 100644 --- a/src/librustc_mir/dataflow/impls/borrows.rs +++ b/src/librustc_mir/dataflow/impls/borrows.rs @@ -63,46 +63,25 @@ pub struct Borrows<'a, 'gcx: 'tcx, 'tcx: 'a> { /// to its borrow-indexes. assigned_map: FxHashMap, FxHashSet>, + /// Locations which activate borrows. + /// NOTE: A given location may activate more than one borrow in the future + /// when more general two-phase borrow support is introduced, but for now we + /// only need to store one borrow index + activation_map: FxHashMap, + /// Every borrow has a region; this maps each such regions back to /// its borrow-indexes. region_map: FxHashMap, FxHashSet>, + + /// Map from local to all the borrows on that local local_map: FxHashMap>, - region_span_map: FxHashMap, - nonlexical_regioncx: Option>>, -} -// Two-phase borrows actually requires two flow analyses; they need -// to be separate because the final results of the first are used to -// construct the gen+kill sets for the second. (The dataflow system -// is not designed to allow the gen/kill sets to change during the -// fixed-point iteration.) - -/// The `Reservations` analysis is the first of the two flow analyses -/// tracking (phased) borrows. It computes where a borrow is reserved; -/// i.e. where it can reach in the control flow starting from its -/// initial `assigned = &'rgn borrowed` statement, and ending -/// wherever `'rgn` itself ends. -pub(crate) struct Reservations<'a, 'gcx: 'tcx, 'tcx: 'a>(pub(crate) Borrows<'a, 'gcx, 'tcx>); - -/// The `ActiveBorrows` analysis is the second of the two flow -/// analyses tracking (phased) borrows. It computes where any given -/// borrow `&assigned = &'rgn borrowed` is *active*, which starts at -/// the first use of `assigned` after the reservation has started, and -/// ends wherever `'rgn` itself ends. -pub(crate) struct ActiveBorrows<'a, 'gcx: 'tcx, 'tcx: 'a>(pub(crate) Borrows<'a, 'gcx, 'tcx>); - -impl<'a, 'gcx, 'tcx> Reservations<'a, 'gcx, 'tcx> { - pub(crate) fn new(b: Borrows<'a, 'gcx, 'tcx>) -> Self { Reservations(b) } - pub(crate) fn location(&self, idx: ReserveOrActivateIndex) -> &Location { - self.0.location(idx.borrow_index()) - } -} + /// Maps regions to their corresponding source spans + /// Only contains ReScope()s as keys + region_span_map: FxHashMap, -impl<'a, 'gcx, 'tcx> ActiveBorrows<'a, 'gcx, 'tcx> { - pub(crate) fn new(r: Reservations<'a, 'gcx, 'tcx>) -> Self { ActiveBorrows(r.0) } - pub(crate) fn location(&self, idx: ReserveOrActivateIndex) -> &Location { - self.0.location(idx.borrow_index()) - } + /// NLL region inference context with which NLL queries should be resolved + nonlexical_regioncx: Option>>, } // temporarily allow some dead fields: `kind` and `region` will be @@ -114,9 +93,15 @@ pub struct BorrowData<'tcx> { /// Location where the borrow reservation starts. /// In many cases, this will be equal to the activation location but not always. pub(crate) reserve_location: Location, + /// Point where the borrow is activated. + pub(crate) activate_location: Location, + /// What kind of borrow this is pub(crate) kind: mir::BorrowKind, + /// The region for which this borrow is live pub(crate) region: Region<'tcx>, + /// Place from which we are borrowing pub(crate) borrowed_place: mir::Place<'tcx>, + /// Place to which the borrow was stored pub(crate) assigned_place: mir::Place<'tcx>, } @@ -165,9 +150,11 @@ impl<'a, 'gcx, 'tcx> Borrows<'a, 'gcx, 'tcx> { idx_vec: IndexVec::new(), location_map: FxHashMap(), assigned_map: FxHashMap(), + activation_map: FxHashMap(), region_map: FxHashMap(), local_map: FxHashMap(), - region_span_map: FxHashMap() + region_span_map: FxHashMap(), + nonlexical_regioncx: nonlexical_regioncx.clone() }; visitor.visit_mir(mir); return Borrows { tcx: tcx, @@ -177,6 +164,7 @@ impl<'a, 'gcx, 'tcx> Borrows<'a, 'gcx, 'tcx> { root_scope, location_map: visitor.location_map, assigned_map: visitor.assigned_map, + activation_map: visitor.activation_map, region_map: visitor.region_map, local_map: visitor.local_map, region_span_map: visitor.region_span_map, @@ -188,9 +176,11 @@ impl<'a, 'gcx, 'tcx> Borrows<'a, 'gcx, 'tcx> { idx_vec: IndexVec>, location_map: FxHashMap, assigned_map: FxHashMap, FxHashSet>, + activation_map: FxHashMap, region_map: FxHashMap, FxHashSet>, local_map: FxHashMap>, region_span_map: FxHashMap, + nonlexical_regioncx: Option>>, } impl<'a, 'gcx, 'tcx> Visitor<'tcx> for GatherBorrows<'a, 'gcx, 'tcx> { @@ -210,15 +200,25 @@ impl<'a, 'gcx, 'tcx> Borrows<'a, 'gcx, 'tcx> { if let mir::Rvalue::Ref(region, kind, ref borrowed_place) = *rvalue { if is_unsafe_place(self.tcx, self.mir, borrowed_place) { return; } + let activate_location = self.compute_activation_location(location, + &assigned_place, + region, + kind); let borrow = BorrowData { + activate_location, kind, region, reserve_location: location, - kind, region, borrowed_place: borrowed_place.clone(), assigned_place: assigned_place.clone(), }; let idx = self.idx_vec.push(borrow); self.location_map.insert(location, idx); + // This assert is a good sanity check until more general 2-phase borrow + // support is introduced. See NOTE on the activation_map field for more + assert!(!self.activation_map.contains_key(&activate_location), + "More than one activation introduced at the same location."); + self.activation_map.insert(activate_location, idx); + insert(&mut self.assigned_map, assigned_place, idx); insert(&mut self.region_map, ®ion, idx); if let Some(local) = root_local(borrowed_place) { @@ -273,6 +273,246 @@ impl<'a, 'gcx, 'tcx> Borrows<'a, 'gcx, 'tcx> { return self.super_statement(block, statement, location); } } + + /// Represents what kind of usage we've seen. + enum PlaceUsageType { + /// No usage seen + None, + /// Has been seen as the argument to a StorageDead statement. This is required to + /// gracefully handle cases where user code has an unneeded + StorageKilled, + /// Has been used in borrow-activating context + BorrowActivateUsage + } + + /// A MIR visitor that determines if a specific place is used in a two-phase activating + /// manner in a given chunk of MIR. + struct ContainsUseOfPlace<'b, 'tcx: 'b> { + target: &'b Place<'tcx>, + use_found: bool, + } + + impl<'b, 'tcx: 'b> ContainsUseOfPlace<'b, 'tcx> { + fn new(place: &'b Place<'tcx>) -> Self { + Self { target: place, use_found: false } + } + + /// return whether `context` should be considered a "use" of a + /// place found in that context. "Uses" activate associated + /// borrows (at least when such uses occur while the borrow also + /// has a reservation at the time). + fn is_potential_use(context: PlaceContext) -> bool { + match context { + // storage effects on a place do not activate it + PlaceContext::StorageLive | PlaceContext::StorageDead => false, + + // validation effects do not activate a place + // + // FIXME: Should they? Is it just another read? Or can we + // guarantee it won't dereference the stored address? How + // "deep" does validation go? + PlaceContext::Validate => false, + + // FIXME: This is here to not change behaviour from before + // AsmOutput existed, but it's not necessarily a pure overwrite. + // so it's possible this should activate the place. + PlaceContext::AsmOutput | + // pure overwrites of a place do not activate it. (note + // PlaceContext::Call is solely about dest place) + PlaceContext::Store | PlaceContext::Call => false, + + // reads of a place *do* activate it + PlaceContext::Move | + PlaceContext::Copy | + PlaceContext::Drop | + PlaceContext::Inspect | + PlaceContext::Borrow { .. } | + PlaceContext::Projection(..) => true, + } + } + } + + impl<'b, 'tcx: 'b> Visitor<'tcx> for ContainsUseOfPlace<'b, 'tcx> { + fn visit_place(&mut self, + place: &mir::Place<'tcx>, + context: PlaceContext<'tcx>, + location: Location) { + if Self::is_potential_use(context) && place == self.target { + self.use_found = true; + return; + // There is no need to keep checking the statement, we already found a use + } + + self.super_place(place, context, location); + } + + /* + fn visit_statement(&mut self, + block: BasicBlock, + statement: &mir::Statement<'tcx>, + location: Location) { + if let mir::StatementKind::StorageDead(loc) = *statement { + } + + self.super_statement(block, statement, location); + } + */ + } + + impl<'a, 'gcx, 'tcx> GatherBorrows<'a, 'gcx, 'tcx> { + /// Returns true if the borrow represented by `kind` is + /// allowed to be split into separate Reservation and + /// Activation phases. + fn allow_two_phase_borrow(&self, kind: mir::BorrowKind) -> bool { + self.tcx.sess.two_phase_borrows() && + (kind.allows_two_phase_borrow() || + self.tcx.sess.opts.debugging_opts.two_phase_beyond_autoref) + } + + /// Returns true if the given location contains an NLL-activating use of the given place + fn location_contains_use(&self, location: Location, place: &Place) -> bool { + let mut use_checker = ContainsUseOfPlace::new(place); + let block = &self.mir.basic_blocks().get(location.block).unwrap_or_else(|| { + panic!("could not find block at location {:?}", location); + }); + if location.statement_index != block.statements.len() { + // This is a statement + let stmt = block.statements.get(location.statement_index).unwrap_or_else(|| { + panic!("could not find statement at location {:?}"); + }); + use_checker.visit_statement(location.block, stmt, location); + } else { + // This is a terminator + match block.terminator { + Some(ref term) => { + use_checker.visit_terminator(location.block, term, location); + } + None => { + // There is no way for Place to be used by the terminator if there is no + // terminator + } + } + } + + use_checker.use_found + } + + /// Determines if the provided region is terminated after the provided location. + /// EndRegion statements terminate their enclosed region::Scope. + /// We also consult with the NLL region inference engine, should one be available + fn region_terminated_after(&self, region: Region<'tcx>, location: Location) -> bool { + let block_data = &self.mir[location.block]; + if location.statement_index != block_data.statements.len() { + let stmt = &block_data.statements[location.statement_index]; + if let mir::StatementKind::EndRegion(region_scope) = stmt.kind { + if &ReScope(region_scope) == region { + // We encountered an EndRegion statement that terminates the provided region + return true; + } + } + } + if let Some(ref regioncx) = self.nonlexical_regioncx { + if !regioncx.region_contains_point(region, location) { + // NLL says the region has ended already + return true; + } + } + + false + } + + /// Computes the activation location of a borrow. + /// The general idea is to start at the beginning of the region and perform a DFS + /// until we exit the region, either via an explicit EndRegion or because NLL tells + /// us so. If we find more than one valid activation point, we currently panic the + /// compiler since two-phase borrows are only currently supported for compiler- + /// generated code. More precisely, we only allow two-phase borrows for: + /// - Function calls (fn some_func(&mut self, ....)) + /// - *Assign operators (a += b -> fn add_assign(&mut self, other: Self)) + /// See + /// - https://github.com/rust-lang/rust/issues/48431 + /// for detailed design notes. + /// See the TODO in the body of the function for notes on extending support to more + /// general two-phased borrows. + fn compute_activation_location(&self, + start_location: Location, + assigned_place: &mir::Place<'tcx>, + region: Region<'tcx>, + kind: mir::BorrowKind) -> Location { + debug!("Borrows::compute_activation_location({:?}, {:?}, {:?})", + start_location, + assigned_place, + region); + if !self.allow_two_phase_borrow(kind) { + debug!(" -> {:?}", start_location); + return start_location; + } + + // Perform the DFS. + // `stack` is the stack of locations still under consideration + // `found_use` is an Option that becomes Some when we find a use + let mut stack = vec![start_location]; + let mut found_use = None; + while let Some(curr_loc) = stack.pop() { + let block_data = &self.mir.basic_blocks() + .get(curr_loc.block) + .unwrap_or_else(|| { + panic!("could not find block at location {:?}", curr_loc); + }); + + if self.region_terminated_after(region, curr_loc) { + // No need to process this statement. + // It's either an EndRegion (and thus couldn't use assigned_place) or not + // contained in the NLL region and thus a use would be invalid + continue; + } + + if self.location_contains_use(curr_loc, assigned_place) { + // TODO: Handle this case a little more gracefully. Perhaps collect + // all uses in a vector, and find the point in the CFG that dominates + // all of them? + // Right now this is sufficient though since there should only be exactly + // one borrow-activating use of the borrow. + assert!(found_use.is_none(), "Found secondary use of place"); + found_use = Some(curr_loc); + } + + // Push the points we should consider next. + if curr_loc.statement_index < block_data.statements.len() { + stack.push(curr_loc.successor_within_block()); + } else { + stack.extend(block_data.terminator().successors().iter().map( + |&basic_block| { + Location { + statement_index: 0, + block: basic_block + } + } + )) + } + } + + let found_use = found_use.expect("Did not find use of two-phase place"); + debug!(" -> {:?}", found_use); + found_use + } + } + } + + /// Returns the span for the "end point" given region. This will + /// return `None` if NLL is enabled, since that concept has no + /// meaning there. Otherwise, return region span if it exists and + /// span for end of the function if it doesn't exist. + pub(crate) fn opt_region_end_span(&self, region: &Region) -> Option { + match self.nonlexical_regioncx { + Some(_) => None, + None => { + match self.region_span_map.get(region) { + Some(span) => Some(self.tcx.sess.codemap().end_point(*span)), + None => Some(self.tcx.sess.codemap().end_point(self.mir.span)) + } + } + } } pub fn borrows(&self) -> &IndexVec> { &self.borrows } @@ -284,18 +524,24 @@ impl<'a, 'gcx, 'tcx> Borrows<'a, 'gcx, 'tcx> { } /// Add all borrows to the kill set, if those borrows are out of scope at `location`. - /// - /// `is_activations` tracks whether we are in the Reservations or - /// the ActiveBorrows flow analysis, and does not set the - /// activation kill bits in the former case. (Technically, we - /// could set those kill bits without such a guard, since they are - /// never gen'ed by Reservations in the first place. But it makes - /// the instrumentation and graph renderings nicer to leave - /// activations out when of the Reservations kill sets.) + /// That means either they went out of either a nonlexical scope, if we care about those + /// at the moment, or the location represents a lexical EndRegion fn kill_loans_out_of_scope_at_location(&self, sets: &mut BlockSets, - location: Location, - is_activations: bool) { + location: Location) { + /* + XXX: bob_twinkles reintroduce this + let block_data = &self.mir[location.block]; + if location.statement_index != block_data.statements.len() { + let statement = &block_data.statements[location.statement_index]; + if let mir::StatementKind::EndRegion(region_scope) = statement.kind { + for &borrow_index in &self.region_map[&ReScope(region_scope)] { + sets.kill(&ReserveOrActivateIndex::reserved(borrow_index)); + sets.kill(&ReserveOrActivateIndex::active(borrow_index)); + } + } + } + */ if let Some(ref regioncx) = self.nonlexical_regioncx { // NOTE: The state associated with a given `location` // reflects the dataflow on entry to the statement. If it @@ -312,21 +558,46 @@ impl<'a, 'gcx, 'tcx> Borrows<'a, 'gcx, 'tcx> { let borrow_region = borrow_data.region.to_region_vid(); if !regioncx.region_contains_point(borrow_region, location) { sets.kill(&ReserveOrActivateIndex::reserved(borrow_index)); - if is_activations { - sets.kill(&ReserveOrActivateIndex::active(borrow_index)); - } + sets.kill(&ReserveOrActivateIndex::active(borrow_index)); } } } } - /// Models statement effect in Reservations and ActiveBorrows flow - /// analyses; `is activations` tells us if we are in the latter - /// case. - fn statement_effect_on_borrows(&self, - sets: &mut BlockSets, - location: Location, - is_activations: bool) { + fn kill_borrows_on_local(&self, + sets: &mut BlockSets, + local: &rustc::mir::Local) + { + if let Some(borrow_indexes) = self.local_map.get(local) { + sets.kill_all(borrow_indexes.iter() + .map(|b| ReserveOrActivateIndex::reserved(*b))); + sets.kill_all(borrow_indexes.iter() + .map(|b| ReserveOrActivateIndex::active(*b))); + } + } +} + +impl<'a, 'gcx, 'tcx> BitDenotation for Borrows<'a, 'gcx, 'tcx> { + type Idx = ReserveOrActivateIndex; + fn name() -> &'static str { "borrows" } + fn bits_per_block(&self) -> usize { + self.borrows.len() * 2 + } + + fn start_block_effect(&self, _entry_set: &mut IdxSet) { + // no borrows of code region_scopes have been taken prior to + // function execution, so this method has no effect on + // `_sets`. + } + + fn before_statement_effect(&self, sets: &mut BlockSets, location: Location) { + debug!("Borrows::before_statement_effect sets: {:?} location: {:?}", sets, location); + self.kill_loans_out_of_scope_at_location(sets, location); + } + + fn statement_effect(&self, sets: &mut BlockSets, location: Location) { + debug!("Borrows::statement_effect sets: {:?} location: {:?}", sets, location); + let block = &self.mir.basic_blocks().get(location.block).unwrap_or_else(|| { panic!("could not find block at location {:?}", location); }); @@ -334,20 +605,12 @@ impl<'a, 'gcx, 'tcx> Borrows<'a, 'gcx, 'tcx> { panic!("could not find statement at location {:?}"); }); - // Do kills introduced by NLL before setting up any potential - // gens. (See NOTE in kill_loans_out_of_scope_at_location.) - self.kill_loans_out_of_scope_at_location(sets, location, is_activations); - - if is_activations { - // INVARIANT: `sets.on_entry` accurately captures - // reservations on entry to statement (b/c - // accumulates_intrablock_state is overridden for - // ActiveBorrows). - // - // Now compute the activations generated by uses within - // the statement based on that reservation state. - let mut find = FindPlaceUses { sets, assigned_map: &self.assigned_map }; - find.visit_statement(location.block, stmt, location); + // Handle activations + match self.activation_map.get(&location) { + Some(&activated) => { + sets.gen(&ReserveOrActivateIndex::active(activated)) + } + None => {} } match stmt.kind { @@ -357,9 +620,7 @@ impl<'a, 'gcx, 'tcx> Borrows<'a, 'gcx, 'tcx> { assert!(self.nonlexical_regioncx.is_none()); for idx in borrow_indexes { sets.kill(&ReserveOrActivateIndex::reserved(*idx)); - if is_activations { - sets.kill(&ReserveOrActivateIndex::active(*idx)); - } + sets.kill(&ReserveOrActivateIndex::active(*idx)); } } else { // (if there is no entry, then there are no borrows to be tracked) @@ -372,7 +633,7 @@ impl<'a, 'gcx, 'tcx> Borrows<'a, 'gcx, 'tcx> { if let Place::Local(ref local) = *lhs { // FIXME: Handle the case in which we're assigning over // a projection (`foo.bar`). - self.kill_borrows_on_local(sets, local, is_activations); + self.kill_borrows_on_local(sets, local); } // NOTE: if/when the Assign case is revised to inspect @@ -396,18 +657,17 @@ impl<'a, 'gcx, 'tcx> Borrows<'a, 'gcx, 'tcx> { }).contains(&index)); sets.gen(&ReserveOrActivateIndex::reserved(*index)); - if is_activations { - // Issue #46746: Two-phase borrows handles - // stmts of form `Tmp = &mut Borrow` ... - match lhs { - Place::Local(..) | Place::Static(..) => {} // okay - Place::Projection(..) => { - // ... can assign into projections, - // e.g. `box (&mut _)`. Current - // conservative solution: force - // immediate activation here. - sets.gen(&ReserveOrActivateIndex::active(*index)); - } + // Issue #46746: Two-phase borrows handles + // stmts of form `Tmp = &mut Borrow` ... + // XXX bob_twinkles experiment with removing this + match lhs { + Place::Local(..) | Place::Static(..) => {} // okay + Place::Projection(..) => { + // ... can assign into projections, + // e.g. `box (&mut _)`. Current + // conservative solution: force + // immediate activation here. + sets.gen(&ReserveOrActivateIndex::active(*index)); } } } @@ -416,7 +676,7 @@ impl<'a, 'gcx, 'tcx> Borrows<'a, 'gcx, 'tcx> { mir::StatementKind::StorageDead(local) => { // Make sure there are no remaining borrows for locals that // are gone out of scope. - self.kill_borrows_on_local(sets, &local, is_activations) + self.kill_borrows_on_local(sets, &local) } mir::StatementKind::InlineAsm { ref outputs, ref asm, .. } => { @@ -427,7 +687,7 @@ impl<'a, 'gcx, 'tcx> Borrows<'a, 'gcx, 'tcx> { if let Place::Local(ref local) = *output { // FIXME: Handle the case in which we're assigning over // a projection (`foo.bar`). - self.kill_borrows_on_local(sets, local, is_activations); + self.kill_borrows_on_local(sets, local); } } } @@ -441,47 +701,26 @@ impl<'a, 'gcx, 'tcx> Borrows<'a, 'gcx, 'tcx> { } } - fn kill_borrows_on_local(&self, - sets: &mut BlockSets, - local: &rustc::mir::Local, - is_activations: bool) - { - if let Some(borrow_indexes) = self.local_map.get(local) { - sets.kill_all(borrow_indexes.iter() - .map(|b| ReserveOrActivateIndex::reserved(*b))); - if is_activations { - sets.kill_all(borrow_indexes.iter() - .map(|b| ReserveOrActivateIndex::active(*b))); - } - } + fn before_terminator_effect(&self, sets: &mut BlockSets, location: Location) { + debug!("Borrows::before_terminator_effect sets: {:?} location: {:?}", sets, location); + self.kill_loans_out_of_scope_at_location(sets, location); } - /// Models terminator effect in Reservations and ActiveBorrows - /// flow analyses; `is activations` tells us if we are in the - /// latter case. - fn terminator_effect_on_borrows(&self, - sets: &mut BlockSets, - location: Location, - is_activations: bool) { + fn terminator_effect(&self, sets: &mut BlockSets, location: Location) { + debug!("Borrows::terminator_effect sets: {:?} location: {:?}", sets, location); + let block = &self.mir.basic_blocks().get(location.block).unwrap_or_else(|| { panic!("could not find block at location {:?}", location); }); - // Do kills introduced by NLL before setting up any potential - // gens. (See NOTE in kill_loans_out_of_scope_at_location.) - self.kill_loans_out_of_scope_at_location(sets, location, is_activations); - let term = block.terminator(); - if is_activations { - // INVARIANT: `sets.on_entry` accurately captures - // reservations on entry to terminator (b/c - // accumulates_intrablock_state is overridden for - // ActiveBorrows). - // - // Now compute effect of the terminator on the activations - // themselves in the ActiveBorrows state. - let mut find = FindPlaceUses { sets, assigned_map: &self.assigned_map }; - find.visit_terminator(location.block, term, location); + + // Handle activations + match self.activation_map.get(&location) { + Some(&activated) => { + sets.gen(&ReserveOrActivateIndex::active(activated)) + } + None => {} } match term.kind { @@ -504,9 +743,7 @@ impl<'a, 'gcx, 'tcx> Borrows<'a, 'gcx, 'tcx> { self.scope_tree.is_subscope_of(*scope, root_scope) { sets.kill(&ReserveOrActivateIndex::reserved(borrow_index)); - if is_activations { - sets.kill(&ReserveOrActivateIndex::active(borrow_index)); - } + sets.kill(&ReserveOrActivateIndex::active(borrow_index)); } } } @@ -525,161 +762,6 @@ impl<'a, 'gcx, 'tcx> Borrows<'a, 'gcx, 'tcx> { mir::TerminatorKind::Unreachable => {} } } -} - -impl<'a, 'gcx, 'tcx> ActiveBorrows<'a, 'gcx, 'tcx> { - pub(crate) fn borrows(&self) -> &IndexVec> { - self.0.borrows() - } - - /// Returns the span for the "end point" given region. This will - /// return `None` if NLL is enabled, since that concept has no - /// meaning there. Otherwise, return region span if it exists and - /// span for end of the function if it doesn't exist. - pub(crate) fn opt_region_end_span(&self, region: &Region) -> Option { - match self.0.nonlexical_regioncx { - Some(_) => None, - None => { - match self.0.region_span_map.get(region) { - Some(span) => Some(self.0.tcx.sess.codemap().end_point(*span)), - None => Some(self.0.tcx.sess.codemap().end_point(self.0.mir.span)) - } - } - } - } -} - -/// `FindPlaceUses` is a MIR visitor that updates `self.sets` for all -/// of the borrows activated by a given statement or terminator. -/// -/// ---- -/// -/// The `ActiveBorrows` flow analysis, when inspecting any given -/// statement or terminator, needs to "generate" (i.e. set to 1) all -/// of the bits for the borrows that are activated by that -/// statement/terminator. -/// -/// This struct will seek out all places that are assignment-targets -/// for borrows (gathered in `self.assigned_map`; see also the -/// `assigned_map` in `struct Borrows`), and set the corresponding -/// gen-bits for activations of those borrows in `self.sets` -struct FindPlaceUses<'a, 'b: 'a, 'tcx: 'a> { - assigned_map: &'a FxHashMap, FxHashSet>, - sets: &'a mut BlockSets<'b, ReserveOrActivateIndex>, -} - -impl<'a, 'b, 'tcx> FindPlaceUses<'a, 'b, 'tcx> { - fn has_been_reserved(&self, b: &BorrowIndex) -> bool { - self.sets.on_entry.contains(&ReserveOrActivateIndex::reserved(*b)) - } - - /// return whether `context` should be considered a "use" of a - /// place found in that context. "Uses" activate associated - /// borrows (at least when such uses occur while the borrow also - /// has a reservation at the time). - fn is_potential_use(context: PlaceContext) -> bool { - match context { - // storage effects on a place do not activate it - PlaceContext::StorageLive | PlaceContext::StorageDead => false, - - // validation effects do not activate a place - // - // FIXME: Should they? Is it just another read? Or can we - // guarantee it won't dereference the stored address? How - // "deep" does validation go? - PlaceContext::Validate => false, - - // FIXME: This is here to not change behaviour from before - // AsmOutput existed, but it's not necessarily a pure overwrite. - // so it's possible this should activate the place. - PlaceContext::AsmOutput | - // pure overwrites of a place do not activate it. (note - // PlaceContext::Call is solely about dest place) - PlaceContext::Store | PlaceContext::Call => false, - - // reads of a place *do* activate it - PlaceContext::Move | - PlaceContext::Copy | - PlaceContext::Drop | - PlaceContext::Inspect | - PlaceContext::Borrow { .. } | - PlaceContext::Projection(..) => true, - } - } -} - -impl<'a, 'b, 'tcx> Visitor<'tcx> for FindPlaceUses<'a, 'b, 'tcx> { - fn visit_place(&mut self, - place: &mir::Place<'tcx>, - context: PlaceContext<'tcx>, - location: Location) { - debug!("FindPlaceUses place: {:?} assigned from borrows: {:?} \ - used in context: {:?} at location: {:?}", - place, self.assigned_map.get(place), context, location); - if Self::is_potential_use(context) { - if let Some(borrows) = self.assigned_map.get(place) { - for borrow_idx in borrows { - debug!("checking if index {:?} for {:?} is reserved ({}) \ - and thus needs active gen-bit set in sets {:?}", - borrow_idx, place, self.has_been_reserved(&borrow_idx), self.sets); - if self.has_been_reserved(&borrow_idx) { - self.sets.gen(&ReserveOrActivateIndex::active(*borrow_idx)); - } else { - // (This can certainly happen in valid code. I - // just want to know about it in the short - // term.) - debug!("encountered use of Place {:?} of borrow_idx {:?} \ - at location {:?} outside of reservation", - place, borrow_idx, location); - } - } - } - } - - self.super_place(place, context, location); - } -} - - -impl<'a, 'gcx, 'tcx> BitDenotation for Reservations<'a, 'gcx, 'tcx> { - type Idx = ReserveOrActivateIndex; - fn name() -> &'static str { "reservations" } - fn bits_per_block(&self) -> usize { - self.0.borrows.len() * 2 - } - fn start_block_effect(&self, _entry_set: &mut IdxSet) { - // no borrows of code region_scopes have been taken prior to - // function execution, so this method has no effect on - // `_sets`. - } - - fn before_statement_effect(&self, - sets: &mut BlockSets, - location: Location) { - debug!("Reservations::before_statement_effect sets: {:?} location: {:?}", sets, location); - self.0.kill_loans_out_of_scope_at_location(sets, location, false); - } - - fn statement_effect(&self, - sets: &mut BlockSets, - location: Location) { - debug!("Reservations::statement_effect sets: {:?} location: {:?}", sets, location); - self.0.statement_effect_on_borrows(sets, location, false); - } - - fn before_terminator_effect(&self, - sets: &mut BlockSets, - location: Location) { - debug!("Reservations::before_terminator_effect sets: {:?} location: {:?}", sets, location); - self.0.kill_loans_out_of_scope_at_location(sets, location, false); - } - - fn terminator_effect(&self, - sets: &mut BlockSets, - location: Location) { - debug!("Reservations::terminator_effect sets: {:?} location: {:?}", sets, location); - self.0.terminator_effect_on_borrows(sets, location, false); - } fn propagate_call_return(&self, _in_out: &mut IdxSet, @@ -694,85 +776,17 @@ impl<'a, 'gcx, 'tcx> BitDenotation for Reservations<'a, 'gcx, 'tcx> { } } -impl<'a, 'gcx, 'tcx> BitDenotation for ActiveBorrows<'a, 'gcx, 'tcx> { - type Idx = ReserveOrActivateIndex; - fn name() -> &'static str { "active_borrows" } - - /// Overriding this method; `ActiveBorrows` uses the intrablock - /// state in `on_entry` to track the current reservations (which - /// then affect the construction of the gen/kill sets for - /// activations). - fn accumulates_intrablock_state() -> bool { true } - - fn bits_per_block(&self) -> usize { - self.0.borrows.len() * 2 - } - - fn start_block_effect(&self, _entry_sets: &mut IdxSet) { - // no borrows of code region_scopes have been taken prior to - // function execution, so this method has no effect on - // `_sets`. - } - - fn before_statement_effect(&self, - sets: &mut BlockSets, - location: Location) { - debug!("ActiveBorrows::before_statement_effect sets: {:?} location: {:?}", sets, location); - self.0.kill_loans_out_of_scope_at_location(sets, location, true); - } - - fn statement_effect(&self, - sets: &mut BlockSets, - location: Location) { - debug!("ActiveBorrows::statement_effect sets: {:?} location: {:?}", sets, location); - self.0.statement_effect_on_borrows(sets, location, true); - } - - fn before_terminator_effect(&self, - sets: &mut BlockSets, - location: Location) { - debug!("ActiveBorrows::before_terminator_effect sets: {:?} location: {:?}", sets, location); - self.0.kill_loans_out_of_scope_at_location(sets, location, true); - } - - fn terminator_effect(&self, - sets: &mut BlockSets, - location: Location) { - debug!("ActiveBorrows::terminator_effect sets: {:?} location: {:?}", sets, location); - self.0.terminator_effect_on_borrows(sets, location, true); - } - - fn propagate_call_return(&self, - _in_out: &mut IdxSet, - _call_bb: mir::BasicBlock, - _dest_bb: mir::BasicBlock, - _dest_place: &mir::Place) { - // there are no effects on borrows from method call return... - // - // ... but If overwriting a place can affect flow state, then - // latter is not true; see NOTE on Assign case in - // statement_effect_on_borrows. - } -} - -impl<'a, 'gcx, 'tcx> BitwiseOperator for Reservations<'a, 'gcx, 'tcx> { +impl<'a, 'gcx, 'tcx> BitwiseOperator for Borrows<'a, 'gcx, 'tcx> { #[inline] fn join(&self, pred1: usize, pred2: usize) -> usize { pred1 | pred2 // union effects of preds when computing reservations } } -impl<'a, 'gcx, 'tcx> BitwiseOperator for ActiveBorrows<'a, 'gcx, 'tcx> { - #[inline] - fn join(&self, pred1: usize, pred2: usize) -> usize { - pred1 | pred2 // union effects of preds when computing activations - } -} - -impl<'a, 'gcx, 'tcx> InitialFlow for Reservations<'a, 'gcx, 'tcx> { +impl<'a, 'gcx, 'tcx> InitialFlow for Borrows<'a, 'gcx, 'tcx> { #[inline] fn bottom_value() -> bool { - false // bottom = no Rvalue::Refs are reserved by default + false // bottom = nothing is reserved or activated yet } } diff --git a/src/librustc_mir/dataflow/mod.rs b/src/librustc_mir/dataflow/mod.rs index aa7bb6f97786c..f7675d611cc5c 100644 --- a/src/librustc_mir/dataflow/mod.rs +++ b/src/librustc_mir/dataflow/mod.rs @@ -18,7 +18,7 @@ use rustc::ty::{self, TyCtxt}; use rustc::mir::{self, Mir, BasicBlock, BasicBlockData, Location, Statement, Terminator}; use rustc::session::Session; -use std::borrow::{Borrow, Cow}; +use std::borrow::Borrow; use std::fmt; use std::io; use std::mem; @@ -31,7 +31,7 @@ pub use self::impls::{DefinitelyInitializedPlaces, MovingOutStatements}; pub use self::impls::EverInitializedPlaces; pub use self::impls::borrows::{Borrows, BorrowData}; pub use self::impls::HaveBeenBorrowedLocals; -pub(crate) use self::impls::borrows::{ActiveBorrows, Reservations, ReserveOrActivateIndex}; +pub(crate) use self::impls::borrows::{ReserveOrActivateIndex}; pub use self::at_location::{FlowAtLocation, FlowsAtLocation}; pub(crate) use self::drop_flag_effects::*; @@ -584,9 +584,6 @@ impl AllSets { pub fn on_entry_set_for(&self, block_idx: usize) -> &IdxSet { self.lookup_set_for(&self.on_entry_sets, block_idx) } - pub(crate) fn entry_set_state(&self) -> &Bits { - &self.on_entry_sets - } } /// Parameterization for the precise form of data flow that is used. @@ -739,27 +736,17 @@ impl<'a, 'tcx, D> DataflowAnalysis<'a, 'tcx, D> where D: BitDenotation dead_unwinds: &'a IdxSet, denotation: D) -> Self where D: InitialFlow { let bits_per_block = denotation.bits_per_block(); + let usize_bits = mem::size_of::() * 8; + let words_per_block = (bits_per_block + usize_bits - 1) / usize_bits; let num_overall = Self::num_bits_overall(mir, bits_per_block); + + let zeroes = Bits::new(IdxSetBuf::new_empty(num_overall)); let on_entry = Bits::new(if D::bottom_value() { IdxSetBuf::new_filled(num_overall) } else { IdxSetBuf::new_empty(num_overall) }); - Self::new_with_entry_sets(mir, dead_unwinds, Cow::Owned(on_entry), denotation) - } - - pub(crate) fn new_with_entry_sets(mir: &'a Mir<'tcx>, - dead_unwinds: &'a IdxSet, - on_entry: Cow>, - denotation: D) - -> Self { - let bits_per_block = denotation.bits_per_block(); - let usize_bits = mem::size_of::() * 8; - let words_per_block = (bits_per_block + usize_bits - 1) / usize_bits; - let num_overall = Self::num_bits_overall(mir, bits_per_block); - assert_eq!(num_overall, on_entry.bits.words().len() * usize_bits); - let zeroes = Bits::new(IdxSetBuf::new_empty(num_overall)); DataflowAnalysis { mir, dead_unwinds, @@ -769,13 +756,27 @@ impl<'a, 'tcx, D> DataflowAnalysis<'a, 'tcx, D> where D: BitDenotation words_per_block, gen_sets: zeroes.clone(), kill_sets: zeroes, - on_entry_sets: on_entry.into_owned(), + on_entry_sets: on_entry, }, operator: denotation, } } } + pub fn new_from_sets(mir: &'a Mir<'tcx>, + dead_unwinds: &'a IdxSet, + sets: AllSets, + denotation: D) -> Self { + DataflowAnalysis { + mir, + dead_unwinds, + flow_state: DataflowState { + sets: sets, + operator: denotation, + } + } + } + fn num_bits_overall(mir: &Mir, bits_per_block: usize) -> usize { let usize_bits = mem::size_of::() * 8; let words_per_block = (bits_per_block + usize_bits - 1) / usize_bits; diff --git a/src/test/compile-fail/borrowck/two-phase-activation-sharing-interference.rs b/src/test/compile-fail/borrowck/two-phase-activation-sharing-interference.rs index 709c00ba8464a..1d3d61fb3fbf8 100644 --- a/src/test/compile-fail/borrowck/two-phase-activation-sharing-interference.rs +++ b/src/test/compile-fail/borrowck/two-phase-activation-sharing-interference.rs @@ -10,10 +10,12 @@ // ignore-tidy-linelength -// revisions: lxl_beyond nll_beyond nll_target +// revisions: nll_target +// The following revisions are disabled due to missing support from two-phase beyond autorefs //[lxl_beyond] compile-flags: -Z borrowck=mir -Z two-phase-borrows -Z two-phase-beyond-autoref //[nll_beyond] compile-flags: -Z borrowck=mir -Z two-phase-borrows -Z two-phase-beyond-autoref -Z nll + //[nll_target] compile-flags: -Z borrowck=mir -Z two-phase-borrows -Z nll // This is an important corner case pointed out by Niko: one is diff --git a/src/test/compile-fail/borrowck/two-phase-allow-access-during-reservation.rs b/src/test/compile-fail/borrowck/two-phase-allow-access-during-reservation.rs index dd174981fb1e2..d2f4154433ab1 100644 --- a/src/test/compile-fail/borrowck/two-phase-allow-access-during-reservation.rs +++ b/src/test/compile-fail/borrowck/two-phase-allow-access-during-reservation.rs @@ -10,9 +10,13 @@ // ignore-tidy-linelength -// revisions: lxl_beyond nll_beyond nll_target +// revisions: nll_target + +// The following revisions are disabled due to missing support for two_phase_beyond_autoref //[lxl_beyond] compile-flags: -Z borrowck=mir -Z two-phase-borrows -Z two_phase_beyond_autoref //[nll_beyond] compile-flags: -Z borrowck=mir -Z two-phase-borrows -Z two_phase_beyond_autoref -Z nll + + //[nll_target] compile-flags: -Z borrowck=mir -Z two-phase-borrows -Z nll // This is the second counter-example from Niko's blog post diff --git a/src/test/compile-fail/borrowck/two-phase-nonrecv-autoref.rs b/src/test/compile-fail/borrowck/two-phase-nonrecv-autoref.rs index f4c36157fe98a..bf8e02adb1ae6 100644 --- a/src/test/compile-fail/borrowck/two-phase-nonrecv-autoref.rs +++ b/src/test/compile-fail/borrowck/two-phase-nonrecv-autoref.rs @@ -8,10 +8,12 @@ // option. This file may not be copied, modified, or distributed // except according to those terms. -// revisions: lxl nll g2p +// revisions: lxl nll //[lxl]compile-flags: -Z borrowck=mir -Z two-phase-borrows //[nll]compile-flags: -Z borrowck=mir -Z two-phase-borrows -Z nll + //[g2p]compile-flags: -Z borrowck=mir -Z two-phase-borrows -Z nll -Z two-phase-beyond-autoref +// the above revision is disabled until two-phase-beyond-autoref support is better // This is a test checking that when we limit two-phase borrows to // method receivers, we do not let other kinds of auto-ref to leak @@ -70,10 +72,8 @@ fn overloaded_call_traits() { fn twice_ten_sm i32>(f: &mut F) { f(f(10)); //[lxl]~^ ERROR cannot borrow `*f` as mutable more than once at a time - //[lxl]~| ERROR cannot borrow `*f` as mutable more than once at a time - //[nll]~^^^ ERROR cannot borrow `*f` as mutable more than once at a time - //[nll]~| ERROR cannot borrow `*f` as mutable more than once at a time - //[g2p]~^^^^^ ERROR cannot borrow `*f` as mutable more than once at a time + //[nll]~^^ ERROR cannot borrow `*f` as mutable more than once at a time + //[g2p]~^^^ ERROR cannot borrow `*f` as mutable more than once at a time } fn twice_ten_si i32>(f: &mut F) { f(f(10)); @@ -88,10 +88,8 @@ fn overloaded_call_traits() { fn twice_ten_om(f: &mut FnMut(i32) -> i32) { f(f(10)); //[lxl]~^ ERROR cannot borrow `*f` as mutable more than once at a time - //[lxl]~| ERROR cannot borrow `*f` as mutable more than once at a time - //[nll]~^^^ ERROR cannot borrow `*f` as mutable more than once at a time - //[nll]~| ERROR cannot borrow `*f` as mutable more than once at a time - //[g2p]~^^^^^ ERROR cannot borrow `*f` as mutable more than once at a time + //[nll]~^^ ERROR cannot borrow `*f` as mutable more than once at a time + //[g2p]~^^^ ERROR cannot borrow `*f` as mutable more than once at a time } fn twice_ten_oi(f: &mut Fn(i32) -> i32) { f(f(10)); diff --git a/src/test/compile-fail/borrowck/two-phase-reservation-sharing-interference.rs b/src/test/compile-fail/borrowck/two-phase-reservation-sharing-interference.rs index b5fda4985f23f..058022ad588e8 100644 --- a/src/test/compile-fail/borrowck/two-phase-reservation-sharing-interference.rs +++ b/src/test/compile-fail/borrowck/two-phase-reservation-sharing-interference.rs @@ -12,8 +12,12 @@ // revisions: lxl_beyond nll_beyond nll_target +// The following revisions are disabled due to missing support from two-phase beyond autorefs //[lxl_beyond]compile-flags: -Z borrowck=mir -Z two-phase-borrows -Z two-phase-beyond-autoref +//[lxl_beyond] should-fail //[nll_beyond]compile-flags: -Z borrowck=mir -Z two-phase-borrows -Z two-phase-beyond-autoref -Z nll +//[nll_beyond] should-fail + //[nll_target]compile-flags: -Z borrowck=mir -Z two-phase-borrows -Z nll // This is a corner case that the current implementation is (probably) @@ -31,10 +35,6 @@ // "nll_beyond" means the generalization of two-phase borrows to all // `&mut`-borrows (doing so makes it easier to write code for specific // corner cases). -// -// FIXME: in "nll_target", we currently see the same error reported -// twice. This is injected by `-Z two-phase-borrows`; not sure why as -// of yet. fn main() { let mut vec = vec![0, 1]; @@ -49,7 +49,6 @@ fn main() { //[lxl_beyond]~^ ERROR cannot borrow `vec` as mutable because it is also borrowed as immutable //[nll_beyond]~^^ ERROR cannot borrow `vec` as mutable because it is also borrowed as immutable //[nll_target]~^^^ ERROR cannot borrow `vec` as mutable because it is also borrowed as immutable - //[nll_target]~| ERROR cannot borrow `vec` as mutable because it is also borrowed as immutable shared[0]; } From 047bec69b9ec54d400b1e255c8757bff8a5a854d Mon Sep 17 00:00:00 2001 From: bobtwinkles Date: Sat, 3 Mar 2018 17:44:06 -0500 Subject: [PATCH 3/9] mir dataflow: change graphviz output The new output format is perhaps a little more readable. As a bonus, we get labels on the outgoing edges to more easily corroborate the dataflow with the plain MIR graphviz output. --- src/libgraphviz/lib.rs | 6 + src/librustc_mir/dataflow/graphviz.rs | 285 +++++++++++++------------- 2 files changed, 149 insertions(+), 142 deletions(-) diff --git a/src/libgraphviz/lib.rs b/src/libgraphviz/lib.rs index cd893b9784ab6..d8c366d2413d9 100644 --- a/src/libgraphviz/lib.rs +++ b/src/libgraphviz/lib.rs @@ -711,6 +711,12 @@ impl<'a> IntoCow<'a, str> for &'a str { } } +impl<'a> IntoCow<'a, str> for Cow<'a, str> { + fn into_cow(self) -> Cow<'a, str> { + self + } +} + impl<'a, T: Clone> IntoCow<'a, [T]> for Vec { fn into_cow(self) -> Cow<'a, [T]> { Cow::Owned(self) diff --git a/src/librustc_mir/dataflow/graphviz.rs b/src/librustc_mir/dataflow/graphviz.rs index fb3cb1518cbb8..0305e4c93b6a0 100644 --- a/src/librustc_mir/dataflow/graphviz.rs +++ b/src/librustc_mir/dataflow/graphviz.rs @@ -20,12 +20,9 @@ use dot::IntoCow; use std::fs; use std::io; -use std::io::prelude::*; use std::marker::PhantomData; use std::path::Path; -use util; - use super::{BitDenotation, DataflowState}; use super::DataflowBuilder; use super::DebugFormatted; @@ -98,157 +95,161 @@ impl<'a, 'tcx, MWF, P> dot::Labeller<'a> for Graph<'a, 'tcx, MWF, P> } fn node_label(&self, n: &Node) -> dot::LabelText { - // A standard MIR label, as generated by write_node_label, is - // presented in a single column in a table. - // - // The code below does a bunch of formatting work to format a - // node (i.e. MIR basic-block) label with extra - // dataflow-enriched information. In particular, the goal is - // to add extra columns that present the three dataflow - // bitvectors, and the data those bitvectors represent. - // - // It presents it in the following format (where I am - // presenting the table rendering via ASCII art, one line per - // row of the table, and a chunk size of 3 rather than 5): - // - // ------ ----------------------- ------------ -------------------- - // [e1, e3, e4] - // [e8, e9] "= ENTRY:" - // ------ ----------------------- ------------ -------------------- - // Left - // Most - // Column - // Is - // Just - // Normal - // Series - // Of - // MIR - // Stmts - // ------ ----------------------- ------------ -------------------- - // [g1, g4, g5] "= GEN:" - // ------ ----------------------- ------------ -------------------- - // "KILL:" "=" [k1, k3, k8] - // [k9] - // ------ ----------------------- ------------ -------------------- - // - // (In addition, the added dataflow is rendered with a colored - // background just so it will stand out compared to the - // statements.) + // Node label is something like this: + // +---------+----------------------------------+------------------+------------------+ + // | ENTRY | MIR | GEN | KILL | + // +---------+----------------------------------+------------------+------------------+ + // | | 0: StorageLive(_7) | bb3[2]: reserved | bb2[0]: reserved | + // | | 1: StorageLive(_8) | bb3[2]: active | bb2[0]: active | + // | | 2: _8 = &mut _1 | | bb4[2]: reserved | + // | | | | bb4[2]: active | + // | | | | bb9[0]: reserved | + // | | | | bb9[0]: active | + // | | | | bb10[0]: reserved| + // | | | | bb10[0]: active | + // | | | | bb11[0]: reserved| + // | | | | bb11[0]: active | + // +---------+----------------------------------+------------------+------------------+ + // | [00-00] | _7 = const Foo::twiddle(move _8) | [0c-00] | [f3-0f] | + // +---------+----------------------------------+------------------+------------------+ let mut v = Vec::new(); + self.node_label_internal(n, &mut v, *n, self.mbcx.mir()).unwrap(); + dot::LabelText::html(String::from_utf8(v).unwrap()) + } + + + fn node_shape(&self, _n: &Node) -> Option { + Some(dot::LabelText::label("none")) + } + + fn edge_label(&'a self, e: &Edge) -> dot::LabelText<'a> { + let term = self.mbcx.mir()[e.source].terminator(); + let label = &term.kind.fmt_successor_labels()[e.index]; + dot::LabelText::label(label.clone()) + } +} + +impl<'a, 'tcx, MWF, P> Graph<'a, 'tcx, MWF, P> +where MWF: MirWithFlowState<'tcx>, + P: Fn(&MWF::BD, ::Idx) -> DebugFormatted, +{ + /// Generate the node label + fn node_label_internal(&self, + n: &Node, + w: &mut W, + block: BasicBlock, + mir: &Mir) -> io::Result<()> { + // Header rows + const HDRS: [&'static str; 4] = ["ENTRY", "MIR", "GEN", "KILL"]; + const HDR_FMT: &'static str = "bgcolor=\"grey\""; + write!(w, "")?; + for hdr in &HDRS { + write!(w, "", HDR_FMT, hdr)?; + } + write!(w, "")?; + + // Data row + self.node_label_verbose_row(n, w, block, mir)?; + self.node_label_final_row(n, w, block, mir)?; + write!(w, "
", HDRS.len())?; + write!(w, "{:?}", block.index())?; + write!(w, "
{}
")?; + + Ok(()) + } + + /// Build the verbose row: full MIR data, and detailed gen/kill/entry sets + fn node_label_verbose_row(&self, + n: &Node, + w: &mut W, + block: BasicBlock, + mir: &Mir) + -> io::Result<()> { let i = n.index(); - let chunk_size = 5; - const BG_FLOWCONTENT: &'static str = r#"bgcolor="pink""#; - const ALIGN_RIGHT: &'static str = r#"align="right""#; - const FACE_MONOSPACE: &'static str = r#"FACE="Courier""#; - fn chunked_present_left(w: &mut W, - interpreted: &[DebugFormatted], - chunk_size: usize) - -> io::Result<()> - { - // This function may emit a sequence of 's, but it - // always finishes with an (unfinished) - // - // - // Thus, after being called, one should finish both the - // pending as well as the itself. - let mut seen_one = false; - for c in interpreted.chunks(chunk_size) { - if seen_one { - // if not the first row, finish off the previous row - write!(w, "")?; + + macro_rules! dump_set_for { + ($set:ident) => { + write!(w, "")?; + + let flow = self.mbcx.flow_state(); + let entry_interp = flow.interpret_set(&flow.operator, + flow.sets.$set(i), + &self.render_idx); + for e in &entry_interp { + write!(w, "{:?}
", e)?; } - write!(w, "{objs:?}", - bg = BG_FLOWCONTENT, - align = ALIGN_RIGHT, - objs = c)?; - seen_one = true; + write!(w, "")?; } - if !seen_one { - write!(w, "[]", - bg = BG_FLOWCONTENT, - align = ALIGN_RIGHT)?; + } + + write!(w, "")?; + // Entry + dump_set_for!(on_entry_set_for); + + // MIR statements + write!(w, "")?; + { + let data = &mir[block]; + for (i, statement) in data.statements.iter().enumerate() { + write!(w, "{}
", + dot::escape_html(&format!("{:3}: {:?}", i, statement)))?; } - Ok(()) } - util::write_graphviz_node_label( - *n, self.mbcx.mir(), &mut v, 4, - |w| { - let flow = self.mbcx.flow_state(); - let entry_interp = flow.interpret_set(&flow.operator, - flow.sets.on_entry_set_for(i), - &self.render_idx); - chunked_present_left(w, &entry_interp[..], chunk_size)?; - let bits_per_block = flow.sets.bits_per_block(); - let entry = flow.sets.on_entry_set_for(i); - debug!("entry set for i={i} bits_per_block: {bpb} entry: {e:?} interp: {ei:?}", - i=i, e=entry, bpb=bits_per_block, ei=entry_interp); - write!(w, "= ENTRY:{entrybits:?}\ - ", - bg = BG_FLOWCONTENT, - face = FACE_MONOSPACE, - entrybits=bits_to_string(entry.words(), bits_per_block)) - }, - |w| { + write!(w, "")?; + + // Gen + dump_set_for!(gen_set_for); + + // Kill + dump_set_for!(kill_set_for); + + write!(w, "")?; + + Ok(()) + } + + /// Build the summary row: terminator, gen/kill/entry bit sets + fn node_label_final_row(&self, + n: &Node, + w: &mut W, + block: BasicBlock, + mir: &Mir) + -> io::Result<()> { + let i = n.index(); + + macro_rules! dump_set_for { + ($set:ident) => { let flow = self.mbcx.flow_state(); - let gen_interp = - flow.interpret_set(&flow.operator, flow.sets.gen_set_for(i), &self.render_idx); - let kill_interp = - flow.interpret_set(&flow.operator, flow.sets.kill_set_for(i), &self.render_idx); - chunked_present_left(w, &gen_interp[..], chunk_size)?; let bits_per_block = flow.sets.bits_per_block(); - { - let gen = flow.sets.gen_set_for(i); - debug!("gen set for i={i} bits_per_block: {bpb} gen: {g:?} interp: {gi:?}", - i=i, g=gen, bpb=bits_per_block, gi=gen_interp); - write!(w, " = GEN:{genbits:?}\ - ", - bg = BG_FLOWCONTENT, - face = FACE_MONOSPACE, - genbits=bits_to_string(gen.words(), bits_per_block))?; - } + let set = flow.sets.$set(i); + write!(w, "{:?}", + dot::escape_html(&bits_to_string(set.words(), bits_per_block)))?; + } + } - { - let kill = flow.sets.kill_set_for(i); - debug!("kill set for i={i} bits_per_block: {bpb} kill: {k:?} interp: {ki:?}", - i=i, k=kill, bpb=bits_per_block, ki=kill_interp); - write!(w, "KILL:\ - {killbits:?}", - bg = BG_FLOWCONTENT, - align = ALIGN_RIGHT, - face = FACE_MONOSPACE, - killbits=bits_to_string(kill.words(), bits_per_block))?; - } + write!(w, "")?; + // Entry + dump_set_for!(on_entry_set_for); - // (chunked_present_right) - let mut seen_one = false; - for k in kill_interp.chunks(chunk_size) { - if !seen_one { - // continuation of row; this is fourth - write!(w, "= {kill:?}", - bg = BG_FLOWCONTENT, - kill=k)?; - } else { - // new row, with indent of three 's - write!(w, "{kill:?}", - bg = BG_FLOWCONTENT, - kill=k)?; - } - seen_one = true; - } - if !seen_one { - write!(w, "= []", - bg = BG_FLOWCONTENT)?; - } + // Terminator + write!(w, "")?; + { + let data = &mir[block]; + let mut terminator_head = String::new(); + data.terminator().kind.fmt_head(&mut terminator_head).unwrap(); + write!(w, "{}", dot::escape_html(&terminator_head))?; + } + write!(w, "")?; - Ok(()) - }) - .unwrap(); - dot::LabelText::html(String::from_utf8(v).unwrap()) - } + // Gen + dump_set_for!(gen_set_for); - fn node_shape(&self, _n: &Node) -> Option { - Some(dot::LabelText::label("none")) + // Kill + dump_set_for!(kill_set_for); + + write!(w, "")?; + + Ok(()) } } From 47d75afd1156fca4f3b3f414b3ca467b0e3f113f Mon Sep 17 00:00:00 2001 From: bobtwinkles Date: Mon, 5 Mar 2018 22:43:43 -0500 Subject: [PATCH 4/9] Complete re-implementation of 2-phase borrows See #48431 for discussion as to why this was necessary and what we hoped to accomplish. A brief summary: - the first implementation of 2-phase borrows was hard to limit in the way we wanted. That is, it was too good at accepting all 2-phase borrows rather than just autorefs =) - Numerous diagnostic regressions were introduced by 2-phase borrow support which were difficult to fix --- src/librustc_mir/dataflow/impls/borrows.rs | 64 +++++++++---------- ...o-phase-activation-sharing-interference.rs | 1 - 2 files changed, 32 insertions(+), 33 deletions(-) diff --git a/src/librustc_mir/dataflow/impls/borrows.rs b/src/librustc_mir/dataflow/impls/borrows.rs index 4b63d8e2ff78a..3d50b9467c132 100644 --- a/src/librustc_mir/dataflow/impls/borrows.rs +++ b/src/librustc_mir/dataflow/impls/borrows.rs @@ -450,8 +450,10 @@ impl<'a, 'gcx, 'tcx> Borrows<'a, 'gcx, 'tcx> { // Perform the DFS. // `stack` is the stack of locations still under consideration + // `visited` is the set of points we have already visited // `found_use` is an Option that becomes Some when we find a use let mut stack = vec![start_location]; + let mut visited = FxHashSet(); let mut found_use = None; while let Some(curr_loc) = stack.pop() { let block_data = &self.mir.basic_blocks() @@ -467,6 +469,11 @@ impl<'a, 'gcx, 'tcx> Borrows<'a, 'gcx, 'tcx> { continue; } + if !visited.insert(curr_loc) { + debug!(" Already visited {:?}", curr_loc); + continue; + } + if self.location_contains_use(curr_loc, assigned_place) { // TODO: Handle this case a little more gracefully. Perhaps collect // all uses in a vector, and find the point in the CFG that dominates @@ -529,19 +536,6 @@ impl<'a, 'gcx, 'tcx> Borrows<'a, 'gcx, 'tcx> { fn kill_loans_out_of_scope_at_location(&self, sets: &mut BlockSets, location: Location) { - /* - XXX: bob_twinkles reintroduce this - let block_data = &self.mir[location.block]; - if location.statement_index != block_data.statements.len() { - let statement = &block_data.statements[location.statement_index]; - if let mir::StatementKind::EndRegion(region_scope) = statement.kind { - for &borrow_index in &self.region_map[&ReScope(region_scope)] { - sets.kill(&ReserveOrActivateIndex::reserved(borrow_index)); - sets.kill(&ReserveOrActivateIndex::active(borrow_index)); - } - } - } - */ if let Some(ref regioncx) = self.nonlexical_regioncx { // NOTE: The state associated with a given `location` // reflects the dataflow on entry to the statement. If it @@ -575,6 +569,20 @@ impl<'a, 'gcx, 'tcx> Borrows<'a, 'gcx, 'tcx> { .map(|b| ReserveOrActivateIndex::active(*b))); } } + + /// Performs the activations for a given location + fn perform_activations_at_location(&self, + sets: &mut BlockSets, + location: Location) { + // Handle activations + match self.activation_map.get(&location) { + Some(&activated) => { + debug!("activating borrow {:?}", activated); + sets.gen(&ReserveOrActivateIndex::active(activated)) + } + None => {} + } + } } impl<'a, 'gcx, 'tcx> BitDenotation for Borrows<'a, 'gcx, 'tcx> { @@ -605,13 +613,8 @@ impl<'a, 'gcx, 'tcx> BitDenotation for Borrows<'a, 'gcx, 'tcx> { panic!("could not find statement at location {:?}"); }); - // Handle activations - match self.activation_map.get(&location) { - Some(&activated) => { - sets.gen(&ReserveOrActivateIndex::active(activated)) - } - None => {} - } + self.perform_activations_at_location(sets, location); + self.kill_loans_out_of_scope_at_location(sets, location); match stmt.kind { // EndRegion kills any borrows (reservations and active borrows both) @@ -643,15 +646,17 @@ impl<'a, 'gcx, 'tcx> BitDenotation for Borrows<'a, 'gcx, 'tcx> { if let mir::Rvalue::Ref(region, _, ref place) = *rhs { if is_unsafe_place(self.tcx, self.mir, place) { return; } + let index = self.location_map.get(&location).unwrap_or_else(|| { + panic!("could not find BorrowIndex for location {:?}", location); + }); + if let RegionKind::ReEmpty = region { - // If the borrowed value is dead, the region for it - // can be empty. Don't track the borrow in that case. + // If the borrowed value dies before the borrow is used, the region for + // the borrow can be empty. Don't track the borrow in that case. + sets.kill(&ReserveOrActivateIndex::active(*index)); return } - let index = self.location_map.get(&location).unwrap_or_else(|| { - panic!("could not find BorrowIndex for location {:?}", location); - }); assert!(self.region_map.get(region).unwrap_or_else(|| { panic!("could not find BorrowIndexs for region {:?}", region); }).contains(&index)); @@ -714,14 +719,9 @@ impl<'a, 'gcx, 'tcx> BitDenotation for Borrows<'a, 'gcx, 'tcx> { }); let term = block.terminator(); + self.perform_activations_at_location(sets, location); + self.kill_loans_out_of_scope_at_location(sets, location); - // Handle activations - match self.activation_map.get(&location) { - Some(&activated) => { - sets.gen(&ReserveOrActivateIndex::active(activated)) - } - None => {} - } match term.kind { mir::TerminatorKind::Resume | diff --git a/src/test/compile-fail/borrowck/two-phase-activation-sharing-interference.rs b/src/test/compile-fail/borrowck/two-phase-activation-sharing-interference.rs index 1d3d61fb3fbf8..90933c6b31fa8 100644 --- a/src/test/compile-fail/borrowck/two-phase-activation-sharing-interference.rs +++ b/src/test/compile-fail/borrowck/two-phase-activation-sharing-interference.rs @@ -53,7 +53,6 @@ fn not_ok() { *y += 1; //[lxl_beyond]~^ ERROR cannot borrow `x` as mutable because it is also borrowed as immutable //[nll_beyond]~^^ ERROR cannot borrow `x` as mutable because it is also borrowed as immutable - //[nll_target]~^^^ ERROR cannot borrow `x` as mutable because it is also borrowed as immutable read(z); } From e4e377f6e8dfb59ecfba014543508fedb61a9f4e Mon Sep 17 00:00:00 2001 From: bobtwinkles Date: Tue, 6 Mar 2018 00:13:29 -0500 Subject: [PATCH 5/9] Remove unused field on BorrowData --- src/librustc_mir/dataflow/impls/borrows.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/librustc_mir/dataflow/impls/borrows.rs b/src/librustc_mir/dataflow/impls/borrows.rs index 3d50b9467c132..8f1c7945339a9 100644 --- a/src/librustc_mir/dataflow/impls/borrows.rs +++ b/src/librustc_mir/dataflow/impls/borrows.rs @@ -93,8 +93,6 @@ pub struct BorrowData<'tcx> { /// Location where the borrow reservation starts. /// In many cases, this will be equal to the activation location but not always. pub(crate) reserve_location: Location, - /// Point where the borrow is activated. - pub(crate) activate_location: Location, /// What kind of borrow this is pub(crate) kind: mir::BorrowKind, /// The region for which this borrow is live @@ -205,7 +203,7 @@ impl<'a, 'gcx, 'tcx> Borrows<'a, 'gcx, 'tcx> { region, kind); let borrow = BorrowData { - activate_location, kind, region, + kind, region, reserve_location: location, borrowed_place: borrowed_place.clone(), assigned_place: assigned_place.clone(), From 03f198fcee46b49a69845784c313433496f7cf0e Mon Sep 17 00:00:00 2001 From: bobtwinkles Date: Tue, 6 Mar 2018 03:37:21 -0500 Subject: [PATCH 6/9] Fix tests after two-phase borrow rewrite --- src/librustc_mir/dataflow/graphviz.rs | 4 +-- src/librustc_mir/dataflow/impls/borrows.rs | 27 +++++++------------ .../nll/region-ends-after-if-condition.rs | 2 +- src/test/ui/issue-45157.rs | 1 - src/test/ui/issue-45157.stderr | 13 +-------- 5 files changed, 14 insertions(+), 33 deletions(-) diff --git a/src/librustc_mir/dataflow/graphviz.rs b/src/librustc_mir/dataflow/graphviz.rs index 0305e4c93b6a0..07585c08f6a29 100644 --- a/src/librustc_mir/dataflow/graphviz.rs +++ b/src/librustc_mir/dataflow/graphviz.rs @@ -111,7 +111,7 @@ impl<'a, 'tcx, MWF, P> dot::Labeller<'a> for Graph<'a, 'tcx, MWF, P> // | | | | bb11[0]: active | // +---------+----------------------------------+------------------+------------------+ // | [00-00] | _7 = const Foo::twiddle(move _8) | [0c-00] | [f3-0f] | - // +---------+----------------------------------+------------------+------------------+ + // +---------+----------------------------------+------------------+------------------+ let mut v = Vec::new(); self.node_label_internal(n, &mut v, *n, self.mbcx.mir()).unwrap(); dot::LabelText::html(String::from_utf8(v).unwrap()) @@ -140,7 +140,7 @@ where MWF: MirWithFlowState<'tcx>, block: BasicBlock, mir: &Mir) -> io::Result<()> { // Header rows - const HDRS: [&'static str; 4] = ["ENTRY", "MIR", "GEN", "KILL"]; + const HDRS: [&'static str; 4] = ["ENTRY", "MIR", "BLOCK GENS", "BLOCK KILLS"]; const HDR_FMT: &'static str = "bgcolor=\"grey\""; write!(w, "
", HDRS.len())?; write!(w, "{:?}", block.index())?; diff --git a/src/librustc_mir/dataflow/impls/borrows.rs b/src/librustc_mir/dataflow/impls/borrows.rs index 8f1c7945339a9..b7c95da09be97 100644 --- a/src/librustc_mir/dataflow/impls/borrows.rs +++ b/src/librustc_mir/dataflow/impls/borrows.rs @@ -272,17 +272,6 @@ impl<'a, 'gcx, 'tcx> Borrows<'a, 'gcx, 'tcx> { } } - /// Represents what kind of usage we've seen. - enum PlaceUsageType { - /// No usage seen - None, - /// Has been seen as the argument to a StorageDead statement. This is required to - /// gracefully handle cases where user code has an unneeded - StorageKilled, - /// Has been used in borrow-activating context - BorrowActivateUsage - } - /// A MIR visitor that determines if a specific place is used in a two-phase activating /// manner in a given chunk of MIR. struct ContainsUseOfPlace<'b, 'tcx: 'b> { @@ -404,7 +393,8 @@ impl<'a, 'gcx, 'tcx> Borrows<'a, 'gcx, 'tcx> { let stmt = &block_data.statements[location.statement_index]; if let mir::StatementKind::EndRegion(region_scope) = stmt.kind { if &ReScope(region_scope) == region { - // We encountered an EndRegion statement that terminates the provided region + // We encountered an EndRegion statement that terminates the provided + // region return true; } } @@ -430,7 +420,7 @@ impl<'a, 'gcx, 'tcx> Borrows<'a, 'gcx, 'tcx> { /// See /// - https://github.com/rust-lang/rust/issues/48431 /// for detailed design notes. - /// See the TODO in the body of the function for notes on extending support to more + /// See the FIXME in the body of the function for notes on extending support to more /// general two-phased borrows. fn compute_activation_location(&self, start_location: Location, @@ -473,7 +463,7 @@ impl<'a, 'gcx, 'tcx> Borrows<'a, 'gcx, 'tcx> { } if self.location_contains_use(curr_loc, assigned_place) { - // TODO: Handle this case a little more gracefully. Perhaps collect + // FIXME: Handle this case a little more gracefully. Perhaps collect // all uses in a vector, and find the point in the CFG that dominates // all of them? // Right now this is sufficient though since there should only be exactly @@ -596,7 +586,9 @@ impl<'a, 'gcx, 'tcx> BitDenotation for Borrows<'a, 'gcx, 'tcx> { // `_sets`. } - fn before_statement_effect(&self, sets: &mut BlockSets, location: Location) { + fn before_statement_effect(&self, + sets: &mut BlockSets, + location: Location) { debug!("Borrows::before_statement_effect sets: {:?} location: {:?}", sets, location); self.kill_loans_out_of_scope_at_location(sets, location); } @@ -662,7 +654,6 @@ impl<'a, 'gcx, 'tcx> BitDenotation for Borrows<'a, 'gcx, 'tcx> { // Issue #46746: Two-phase borrows handles // stmts of form `Tmp = &mut Borrow` ... - // XXX bob_twinkles experiment with removing this match lhs { Place::Local(..) | Place::Static(..) => {} // okay Place::Projection(..) => { @@ -704,7 +695,9 @@ impl<'a, 'gcx, 'tcx> BitDenotation for Borrows<'a, 'gcx, 'tcx> { } } - fn before_terminator_effect(&self, sets: &mut BlockSets, location: Location) { + fn before_terminator_effect(&self, + sets: &mut BlockSets, + location: Location) { debug!("Borrows::before_terminator_effect sets: {:?} location: {:?}", sets, location); self.kill_loans_out_of_scope_at_location(sets, location); } diff --git a/src/test/compile-fail/nll/region-ends-after-if-condition.rs b/src/test/compile-fail/nll/region-ends-after-if-condition.rs index 1128d65af95c5..da9187f43ae9c 100644 --- a/src/test/compile-fail/nll/region-ends-after-if-condition.rs +++ b/src/test/compile-fail/nll/region-ends-after-if-condition.rs @@ -12,7 +12,7 @@ // in the type of `p` includes the points after `&v[0]` up to (but not // including) the call to `use_x`. The `else` branch is not included. -// compile-flags:-Zborrowck=compare -Znll +// compile-flags:-Zborrowck=compare -Znll -Ztwo-phase-borrows #![allow(warnings)] #![feature(rustc_attrs)] diff --git a/src/test/ui/issue-45157.rs b/src/test/ui/issue-45157.rs index cff338f76c507..a906d193d995e 100644 --- a/src/test/ui/issue-45157.rs +++ b/src/test/ui/issue-45157.rs @@ -37,7 +37,6 @@ fn main() { let nref = &u.z.c; //~^ ERROR cannot borrow `u.z.c` as immutable because it is also borrowed as mutable [E0502] println!("{} {}", mref, nref) - //~^ ERROR cannot borrow `u.s.a` as mutable because it is also borrowed as immutable [E0502] } } diff --git a/src/test/ui/issue-45157.stderr b/src/test/ui/issue-45157.stderr index bec91f7f70dee..07102f68633cf 100644 --- a/src/test/ui/issue-45157.stderr +++ b/src/test/ui/issue-45157.stderr @@ -10,17 +10,6 @@ LL | //~^ ERROR cannot borrow `u.z.c` as immutable because it is also bo LL | println!("{} {}", mref, nref) | ---- borrow later used here -error[E0502]: cannot borrow `u.s.a` as mutable because it is also borrowed as immutable - --> $DIR/issue-45157.rs:39:27 - | -LL | let nref = &u.z.c; - | ------ immutable borrow occurs here -LL | //~^ ERROR cannot borrow `u.z.c` as immutable because it is also borrowed as mutable [E0502] -LL | println!("{} {}", mref, nref) - | ^^^^ ---- borrow later used here - | | - | mutable borrow occurs here - -error: aborting due to 2 previous errors +error: aborting due to previous error If you want more information on this error, try using "rustc --explain E0502" From 2ed0f516dd16c8111de0e0015d0ff42e312e2847 Mon Sep 17 00:00:00 2001 From: bobtwinkles Date: Tue, 6 Mar 2018 04:19:41 -0500 Subject: [PATCH 7/9] Check for two_phase_borrows in the right place Fix a small compilation issue after I missed a critical change after rebasing yesterday (ref c933440) --- src/librustc_mir/dataflow/impls/borrows.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/librustc_mir/dataflow/impls/borrows.rs b/src/librustc_mir/dataflow/impls/borrows.rs index b7c95da09be97..9a5221a325745 100644 --- a/src/librustc_mir/dataflow/impls/borrows.rs +++ b/src/librustc_mir/dataflow/impls/borrows.rs @@ -351,7 +351,7 @@ impl<'a, 'gcx, 'tcx> Borrows<'a, 'gcx, 'tcx> { /// allowed to be split into separate Reservation and /// Activation phases. fn allow_two_phase_borrow(&self, kind: mir::BorrowKind) -> bool { - self.tcx.sess.two_phase_borrows() && + self.tcx.two_phase_borrows() && (kind.allows_two_phase_borrow() || self.tcx.sess.opts.debugging_opts.two_phase_beyond_autoref) } From 8b4c623702225931811893ae21145b951358f552 Mon Sep 17 00:00:00 2001 From: bobtwinkles Date: Fri, 9 Mar 2018 19:00:18 -0500 Subject: [PATCH 8/9] Remove added two-phase-borrows flag It seems whatever was causing problems has been fixed. --- src/test/compile-fail/nll/region-ends-after-if-condition.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/compile-fail/nll/region-ends-after-if-condition.rs b/src/test/compile-fail/nll/region-ends-after-if-condition.rs index da9187f43ae9c..1128d65af95c5 100644 --- a/src/test/compile-fail/nll/region-ends-after-if-condition.rs +++ b/src/test/compile-fail/nll/region-ends-after-if-condition.rs @@ -12,7 +12,7 @@ // in the type of `p` includes the points after `&v[0]` up to (but not // including) the call to `use_x`. The `else` branch is not included. -// compile-flags:-Zborrowck=compare -Znll -Ztwo-phase-borrows +// compile-flags:-Zborrowck=compare -Znll #![allow(warnings)] #![feature(rustc_attrs)] From 9a5d61a840b2dd3dc6d069750945759035c85286 Mon Sep 17 00:00:00 2001 From: bobtwinkles Date: Fri, 9 Mar 2018 19:02:22 -0500 Subject: [PATCH 9/9] Remove some commented out code Left over from prior experimentation, no longer required. --- src/librustc_mir/dataflow/impls/borrows.rs | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/src/librustc_mir/dataflow/impls/borrows.rs b/src/librustc_mir/dataflow/impls/borrows.rs index 9a5221a325745..499ed55fad41f 100644 --- a/src/librustc_mir/dataflow/impls/borrows.rs +++ b/src/librustc_mir/dataflow/impls/borrows.rs @@ -332,18 +332,6 @@ impl<'a, 'gcx, 'tcx> Borrows<'a, 'gcx, 'tcx> { self.super_place(place, context, location); } - - /* - fn visit_statement(&mut self, - block: BasicBlock, - statement: &mir::Statement<'tcx>, - location: Location) { - if let mir::StatementKind::StorageDead(loc) = *statement { - } - - self.super_statement(block, statement, location); - } - */ } impl<'a, 'gcx, 'tcx> GatherBorrows<'a, 'gcx, 'tcx> {