From 93c4ffe72f8e406a166e258c80723606cd7d8304 Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Fri, 24 Nov 2017 13:00:09 +0100 Subject: [PATCH 01/23] Revised graphviz rendering API to avoid requiring borrowed state. Made `do_dataflow` and related API `pub(crate)`. --- src/librustc_mir/borrow_check/mod.rs | 12 ++--- src/librustc_mir/dataflow/graphviz.rs | 10 ++--- src/librustc_mir/dataflow/mod.rs | 45 +++++++++++++------ src/librustc_mir/transform/elaborate_drops.rs | 20 ++++----- src/librustc_mir/transform/generator.rs | 6 +-- src/librustc_mir/transform/rustc_peek.rs | 8 ++-- 6 files changed, 60 insertions(+), 41 deletions(-) diff --git a/src/librustc_mir/borrow_check/mod.rs b/src/librustc_mir/borrow_check/mod.rs index 128052e58949d..8bfb5fee9f646 100644 --- a/src/librustc_mir/borrow_check/mod.rs +++ b/src/librustc_mir/borrow_check/mod.rs @@ -28,7 +28,7 @@ use rustc_data_structures::indexed_vec::Idx; use syntax::ast; use syntax_pos::Span; -use dataflow::do_dataflow; +use dataflow::{do_dataflow, DebugFormatted}; use dataflow::MoveDataParamEnv; use dataflow::DataflowResultsConsumer; use dataflow::{FlowAtLocation, FlowsAtLocation}; @@ -157,7 +157,7 @@ fn do_mir_borrowck<'a, 'gcx, 'tcx>( &attributes, &dead_unwinds, MaybeInitializedLvals::new(tcx, mir, &mdpe), - |bd, i| &bd.move_data().move_paths[i], + |bd, i| DebugFormatted::new(&bd.move_data().move_paths[i]), )); let flow_uninits = FlowAtLocation::new(do_dataflow( tcx, @@ -166,7 +166,7 @@ fn do_mir_borrowck<'a, 'gcx, 'tcx>( &attributes, &dead_unwinds, MaybeUninitializedLvals::new(tcx, mir, &mdpe), - |bd, i| &bd.move_data().move_paths[i], + |bd, i| DebugFormatted::new(&bd.move_data().move_paths[i]), )); let flow_move_outs = FlowAtLocation::new(do_dataflow( tcx, @@ -175,7 +175,7 @@ fn do_mir_borrowck<'a, 'gcx, 'tcx>( &attributes, &dead_unwinds, MovingOutStatements::new(tcx, mir, &mdpe), - |bd, i| &bd.move_data().moves[i], + |bd, i| DebugFormatted::new(&bd.move_data().moves[i]), )); let flow_ever_inits = FlowAtLocation::new(do_dataflow( tcx, @@ -184,7 +184,7 @@ fn do_mir_borrowck<'a, 'gcx, 'tcx>( &attributes, &dead_unwinds, EverInitializedLvals::new(tcx, mir, &mdpe), - |bd, i| &bd.move_data().inits[i], + |bd, i| DebugFormatted::new(&bd.move_data().inits[i]), )); // If we are in non-lexical mode, compute the non-lexical lifetimes. @@ -212,7 +212,7 @@ fn do_mir_borrowck<'a, 'gcx, 'tcx>( &attributes, &dead_unwinds, Borrows::new(tcx, mir, opt_regioncx, def_id, body_id), - |bd, i| bd.location(i), + |bd, i| DebugFormatted::new(bd.location(i)), )); let mut state = Flows::new( diff --git a/src/librustc_mir/dataflow/graphviz.rs b/src/librustc_mir/dataflow/graphviz.rs index 7ff4fbcf199e0..b79e044b24f20 100644 --- a/src/librustc_mir/dataflow/graphviz.rs +++ b/src/librustc_mir/dataflow/graphviz.rs @@ -18,7 +18,6 @@ use rustc_data_structures::indexed_vec::Idx; use dot; use dot::IntoCow; -use std::fmt::Debug; use std::fs::File; use std::io; use std::io::prelude::*; @@ -29,6 +28,7 @@ use util; use super::{BitDenotation, DataflowState}; use super::DataflowBuilder; +use super::DebugFormatted; pub trait MirWithFlowState<'tcx> { type BD: BitDenotation; @@ -60,9 +60,9 @@ pub(crate) fn print_borrowck_graph_to<'a, 'tcx, BD, P>( render_idx: P) -> io::Result<()> where BD: BitDenotation, - P: Fn(&BD, BD::Idx) -> &Debug + P: Fn(&BD, BD::Idx) -> DebugFormatted { - let g = Graph { mbcx: mbcx, phantom: PhantomData, render_idx: render_idx }; + let g = Graph { mbcx, phantom: PhantomData, render_idx }; let mut v = Vec::new(); dot::render(&g, &mut v)?; debug!("print_borrowck_graph_to path: {} node_id: {}", @@ -82,7 +82,7 @@ fn outgoing(mir: &Mir, bb: BasicBlock) -> Vec { impl<'a, 'tcx, MWF, P> dot::Labeller<'a> for Graph<'a, 'tcx, MWF, P> where MWF: MirWithFlowState<'tcx>, - P: for <'b> Fn(&'b MWF::BD, ::Idx) -> &'b Debug, + P: Fn(&MWF::BD, ::Idx) -> DebugFormatted, { type Node = Node; type Edge = Edge; @@ -142,7 +142,7 @@ impl<'a, 'tcx, MWF, P> dot::Labeller<'a> for Graph<'a, 'tcx, MWF, P> const ALIGN_RIGHT: &'static str = r#"align="right""#; const FACE_MONOSPACE: &'static str = r#"FACE="Courier""#; fn chunked_present_left(w: &mut W, - interpreted: &[&Debug], + interpreted: &[DebugFormatted], chunk_size: usize) -> io::Result<()> { diff --git a/src/librustc_mir/dataflow/mod.rs b/src/librustc_mir/dataflow/mod.rs index 2136b41e46222..8624cbbf50ab3 100644 --- a/src/librustc_mir/dataflow/mod.rs +++ b/src/librustc_mir/dataflow/mod.rs @@ -19,7 +19,7 @@ use rustc::mir::{self, Mir, BasicBlock, BasicBlockData, Location, Statement, Ter use rustc::session::Session; use std::borrow::Borrow; -use std::fmt::{self, Debug}; +use std::fmt; use std::io; use std::mem; use std::path::PathBuf; @@ -51,10 +51,29 @@ pub(crate) struct DataflowBuilder<'a, 'tcx: 'a, BD> where BD: BitDenotation print_postflow_to: Option, } -pub trait Dataflow { +/// `DebugFormatted` encapsulates the "{:?}" rendering of some +/// arbitrary value. This way: you pay cost of allocating an extra +/// string (as well as that of rendering up-front); in exchange, you +/// don't have to hand over ownership of your value or deal with +/// borrowing it. +pub(crate) struct DebugFormatted(String); + +impl DebugFormatted { + pub fn new(input: &fmt::Debug) -> DebugFormatted { + DebugFormatted(format!("{:?}", input)) + } +} + +impl fmt::Debug for DebugFormatted { + fn fmt(&self, w: &mut fmt::Formatter) -> fmt::Result { + write!(w, "{}", self.0) + } +} + +pub(crate) trait Dataflow { /// Sets up and runs the dataflow problem, using `p` to render results if /// implementation so chooses. - fn dataflow

(&mut self, p: P) where P: Fn(&BD, BD::Idx) -> &Debug { + fn dataflow

(&mut self, p: P) where P: Fn(&BD, BD::Idx) -> DebugFormatted { let _ = p; // default implementation does not instrument process. self.build_sets(); self.propagate(); @@ -69,7 +88,7 @@ pub trait Dataflow { impl<'a, 'tcx: 'a, BD> Dataflow for DataflowBuilder<'a, 'tcx, BD> where BD: BitDenotation { - fn dataflow

(&mut self, p: P) where P: Fn(&BD, BD::Idx) -> &Debug { + fn dataflow

(&mut self, p: P) where P: Fn(&BD, BD::Idx) -> DebugFormatted { self.flow_state.build_sets(); self.pre_dataflow_instrumentation(|c,i| p(c,i)).unwrap(); self.flow_state.propagate(); @@ -109,7 +128,7 @@ pub(crate) fn do_dataflow<'a, 'gcx, 'tcx, BD, P>(tcx: TyCtxt<'a, 'gcx, 'tcx>, p: P) -> DataflowResults where BD: BitDenotation, - P: Fn(&BD, BD::Idx) -> &fmt::Debug + P: Fn(&BD, BD::Idx) -> DebugFormatted { let name_found = |sess: &Session, attrs: &[ast::Attribute], name| -> Option { if let Some(item) = has_rustc_mir_with(attrs, name) { @@ -231,7 +250,7 @@ fn dataflow_path(context: &str, prepost: &str, path: &str) -> PathBuf { impl<'a, 'tcx: 'a, BD> DataflowBuilder<'a, 'tcx, BD> where BD: BitDenotation { fn pre_dataflow_instrumentation

(&self, p: P) -> io::Result<()> - where P: Fn(&BD, BD::Idx) -> &Debug + where P: Fn(&BD, BD::Idx) -> DebugFormatted { if let Some(ref path_str) = self.print_preflow_to { let path = dataflow_path(BD::name(), "preflow", path_str); @@ -242,7 +261,7 @@ impl<'a, 'tcx: 'a, BD> DataflowBuilder<'a, 'tcx, BD> where BD: BitDenotation } fn post_dataflow_instrumentation

(&self, p: P) -> io::Result<()> - where P: Fn(&BD, BD::Idx) -> &Debug + where P: Fn(&BD, BD::Idx) -> DebugFormatted { if let Some(ref path_str) = self.print_postflow_to { let path = dataflow_path(BD::name(), "postflow", path_str); @@ -403,12 +422,12 @@ impl DataflowState { words.each_bit(bits_per_block, f) } - pub fn interpret_set<'c, P>(&self, - o: &'c O, - words: &IdxSet, - render_idx: &P) - -> Vec<&'c Debug> - where P: Fn(&O, O::Idx) -> &Debug + pub(crate) fn interpret_set<'c, P>(&self, + o: &'c O, + words: &IdxSet, + render_idx: &P) + -> Vec + where P: Fn(&O, O::Idx) -> DebugFormatted { let mut v = Vec::new(); self.each_bit(words, |i| { diff --git a/src/librustc_mir/transform/elaborate_drops.rs b/src/librustc_mir/transform/elaborate_drops.rs index b075d2637da9b..106bc39d0fc5b 100644 --- a/src/librustc_mir/transform/elaborate_drops.rs +++ b/src/librustc_mir/transform/elaborate_drops.rs @@ -14,7 +14,7 @@ use dataflow::{DataflowResults}; use dataflow::{on_all_children_bits, on_all_drop_children_bits}; use dataflow::{drop_flag_effects_for_location, on_lookup_result_bits}; use dataflow::MoveDataParamEnv; -use dataflow; +use dataflow::{self, do_dataflow, DebugFormatted}; use rustc::hir; use rustc::ty::{self, TyCtxt}; use rustc::mir::*; @@ -59,13 +59,13 @@ impl MirPass for ElaborateDrops { }; let dead_unwinds = find_dead_unwinds(tcx, mir, id, &env); let flow_inits = - dataflow::do_dataflow(tcx, mir, id, &[], &dead_unwinds, - MaybeInitializedLvals::new(tcx, mir, &env), - |bd, p| &bd.move_data().move_paths[p]); + do_dataflow(tcx, mir, id, &[], &dead_unwinds, + MaybeInitializedLvals::new(tcx, mir, &env), + |bd, p| DebugFormatted::new(&bd.move_data().move_paths[p])); let flow_uninits = - dataflow::do_dataflow(tcx, mir, id, &[], &dead_unwinds, - MaybeUninitializedLvals::new(tcx, mir, &env), - |bd, p| &bd.move_data().move_paths[p]); + do_dataflow(tcx, mir, id, &[], &dead_unwinds, + MaybeUninitializedLvals::new(tcx, mir, &env), + |bd, p| DebugFormatted::new(&bd.move_data().move_paths[p])); ElaborateDropsCtxt { tcx, @@ -96,9 +96,9 @@ fn find_dead_unwinds<'a, 'tcx>( // reach cleanup blocks, which can't have unwind edges themselves. let mut dead_unwinds = IdxSetBuf::new_empty(mir.basic_blocks().len()); let flow_inits = - dataflow::do_dataflow(tcx, mir, id, &[], &dead_unwinds, - MaybeInitializedLvals::new(tcx, mir, &env), - |bd, p| &bd.move_data().move_paths[p]); + do_dataflow(tcx, mir, id, &[], &dead_unwinds, + MaybeInitializedLvals::new(tcx, mir, &env), + |bd, p| DebugFormatted::new(&bd.move_data().move_paths[p])); for (bb, bb_data) in mir.basic_blocks().iter_enumerated() { let location = match bb_data.terminator().kind { TerminatorKind::Drop { ref location, unwind: Some(_), .. } | diff --git a/src/librustc_mir/transform/generator.rs b/src/librustc_mir/transform/generator.rs index aaa28634eb82c..455a07c04cfc0 100644 --- a/src/librustc_mir/transform/generator.rs +++ b/src/librustc_mir/transform/generator.rs @@ -78,7 +78,7 @@ use std::mem; use transform::{MirPass, MirSource}; use transform::simplify; use transform::no_landing_pads::no_landing_pads; -use dataflow::{self, MaybeStorageLive, state_for_location}; +use dataflow::{do_dataflow, DebugFormatted, MaybeStorageLive, state_for_location}; pub struct StateTransform; @@ -341,8 +341,8 @@ fn locals_live_across_suspend_points<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, let node_id = tcx.hir.as_local_node_id(source.def_id).unwrap(); let analysis = MaybeStorageLive::new(mir); let storage_live = - dataflow::do_dataflow(tcx, mir, node_id, &[], &dead_unwinds, analysis, - |bd, p| &bd.mir().local_decls[p]); + do_dataflow(tcx, mir, node_id, &[], &dead_unwinds, analysis, + |bd, p| DebugFormatted::new(&bd.mir().local_decls[p])); let mut ignored = StorageIgnored(IdxSetBuf::new_filled(mir.local_decls.len())); ignored.visit_mir(mir); diff --git a/src/librustc_mir/transform/rustc_peek.rs b/src/librustc_mir/transform/rustc_peek.rs index 08508143976e6..6b8e2b073ccd3 100644 --- a/src/librustc_mir/transform/rustc_peek.rs +++ b/src/librustc_mir/transform/rustc_peek.rs @@ -18,7 +18,7 @@ use rustc_data_structures::indexed_set::IdxSetBuf; use rustc_data_structures::indexed_vec::Idx; use transform::{MirPass, MirSource}; -use dataflow::do_dataflow; +use dataflow::{do_dataflow, DebugFormatted}; use dataflow::MoveDataParamEnv; use dataflow::BitDenotation; use dataflow::DataflowResults; @@ -51,15 +51,15 @@ impl MirPass for SanityCheck { let flow_inits = do_dataflow(tcx, mir, id, &attributes, &dead_unwinds, MaybeInitializedLvals::new(tcx, mir, &mdpe), - |bd, i| &bd.move_data().move_paths[i]); + |bd, i| DebugFormatted::new(&bd.move_data().move_paths[i])); let flow_uninits = do_dataflow(tcx, mir, id, &attributes, &dead_unwinds, MaybeUninitializedLvals::new(tcx, mir, &mdpe), - |bd, i| &bd.move_data().move_paths[i]); + |bd, i| DebugFormatted::new(&bd.move_data().move_paths[i])); let flow_def_inits = do_dataflow(tcx, mir, id, &attributes, &dead_unwinds, DefinitelyInitializedLvals::new(tcx, mir, &mdpe), - |bd, i| &bd.move_data().move_paths[i]); + |bd, i| DebugFormatted::new(&bd.move_data().move_paths[i])); if has_rustc_mir_with(&attributes, "rustc_peek_maybe_init").is_some() { sanity_check_via_rustc_peek(tcx, mir, id, &attributes, &flow_inits); From 171c2aeb25539c5bb1a1a3b231c8a17c6e6b05cc Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Fri, 24 Nov 2017 13:44:36 +0100 Subject: [PATCH 02/23] Expanded HIR `--unpretty hir,identified` to include HIR local id. Having the HIR local id is useful for cases like understanding the ReScope identifiers, which are now derived from the HIR local id, and thus one can map an ReScope back to the HIR node, once one knows what those local ids are. --- src/librustc_driver/pretty.rs | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/src/librustc_driver/pretty.rs b/src/librustc_driver/pretty.rs index d930739c9f014..7fdcbfc4d2982 100644 --- a/src/librustc_driver/pretty.rs +++ b/src/librustc_driver/pretty.rs @@ -423,7 +423,8 @@ impl<'hir> pprust_hir::PpAnn for IdentifiedAnnotation<'hir> { pprust_hir::NodeName(_) => Ok(()), pprust_hir::NodeItem(item) => { s.s.space()?; - s.synth_comment(item.id.to_string()) + s.synth_comment(format!("node_id: {} hir local_id: {}", + item.id, item.hir_id.local_id.0)) } pprust_hir::NodeSubItem(id) => { s.s.space()?; @@ -431,16 +432,19 @@ impl<'hir> pprust_hir::PpAnn for IdentifiedAnnotation<'hir> { } pprust_hir::NodeBlock(blk) => { s.s.space()?; - s.synth_comment(format!("block {}", blk.id)) + s.synth_comment(format!("block node_id: {} hir local_id: {}", + blk.id, blk.hir_id.local_id.0)) } pprust_hir::NodeExpr(expr) => { s.s.space()?; - s.synth_comment(expr.id.to_string())?; + s.synth_comment(format!("node_id: {} hir local_id: {}", + expr.id, expr.hir_id.local_id.0))?; s.pclose() } pprust_hir::NodePat(pat) => { s.s.space()?; - s.synth_comment(format!("pat {}", pat.id)) + s.synth_comment(format!("pat node_id: {} hir local_id: {}", + pat.id, pat.hir_id.local_id.0)) } } } From d4add5d52a87e0a1d45505a302d28a577bbe05c2 Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Mon, 27 Nov 2017 15:08:11 +0100 Subject: [PATCH 03/23] Refactoring: pull bitvector initialization out from other parts of dataflow. This is meant to ease development of multi-stage dataflow analyses where the output from one analysis is used to initialize the state for the next; in such a context, you cannot start with `bottom_value` for all the bits. --- src/librustc_mir/dataflow/impls/borrows.rs | 4 ++-- src/librustc_mir/dataflow/impls/mod.rs | 12 +++++------ .../dataflow/impls/storage_liveness.rs | 2 +- src/librustc_mir/dataflow/mod.rs | 21 ++++++++----------- 4 files changed, 18 insertions(+), 21 deletions(-) diff --git a/src/librustc_mir/dataflow/impls/borrows.rs b/src/librustc_mir/dataflow/impls/borrows.rs index 25bc702764a34..eeb7f32c25c29 100644 --- a/src/librustc_mir/dataflow/impls/borrows.rs +++ b/src/librustc_mir/dataflow/impls/borrows.rs @@ -22,7 +22,7 @@ use rustc_data_structures::bitslice::{BitwiseOperator}; use rustc_data_structures::indexed_set::{IdxSet}; use rustc_data_structures::indexed_vec::{IndexVec}; -use dataflow::{BitDenotation, BlockSets, DataflowOperator}; +use dataflow::{BitDenotation, BlockSets, InitialFlow}; pub use dataflow::indexes::BorrowIndex; use borrow_check::nll::region_infer::RegionInferenceContext; use borrow_check::nll::ToRegionVid; @@ -339,7 +339,7 @@ impl<'a, 'gcx, 'tcx> BitwiseOperator for Borrows<'a, 'gcx, 'tcx> { } } -impl<'a, 'gcx, 'tcx> DataflowOperator for Borrows<'a, 'gcx, 'tcx> { +impl<'a, 'gcx, 'tcx> InitialFlow for Borrows<'a, 'gcx, 'tcx> { #[inline] fn bottom_value() -> bool { false // bottom = no Rvalue::Refs are active by default diff --git a/src/librustc_mir/dataflow/impls/mod.rs b/src/librustc_mir/dataflow/impls/mod.rs index 033d2a3212f8c..106a88e703c79 100644 --- a/src/librustc_mir/dataflow/impls/mod.rs +++ b/src/librustc_mir/dataflow/impls/mod.rs @@ -23,7 +23,7 @@ use util::elaborate_drops::DropFlagState; use super::move_paths::{HasMoveData, MoveData, MoveOutIndex, MovePathIndex, InitIndex}; use super::move_paths::{LookupResult, InitKind}; -use super::{BitDenotation, BlockSets, DataflowOperator}; +use super::{BitDenotation, BlockSets, InitialFlow}; use super::drop_flag_effects_for_function_entry; use super::drop_flag_effects_for_location; @@ -702,35 +702,35 @@ impl<'a, 'gcx, 'tcx> BitwiseOperator for EverInitializedLvals<'a, 'gcx, 'tcx> { // propagating, or you start at all-ones and then use Intersect as // your merge when propagating. -impl<'a, 'gcx, 'tcx> DataflowOperator for MaybeInitializedLvals<'a, 'gcx, 'tcx> { +impl<'a, 'gcx, 'tcx> InitialFlow for MaybeInitializedLvals<'a, 'gcx, 'tcx> { #[inline] fn bottom_value() -> bool { false // bottom = uninitialized } } -impl<'a, 'gcx, 'tcx> DataflowOperator for MaybeUninitializedLvals<'a, 'gcx, 'tcx> { +impl<'a, 'gcx, 'tcx> InitialFlow for MaybeUninitializedLvals<'a, 'gcx, 'tcx> { #[inline] fn bottom_value() -> bool { false // bottom = initialized (start_block_effect counters this at outset) } } -impl<'a, 'gcx, 'tcx> DataflowOperator for DefinitelyInitializedLvals<'a, 'gcx, 'tcx> { +impl<'a, 'gcx, 'tcx> InitialFlow for DefinitelyInitializedLvals<'a, 'gcx, 'tcx> { #[inline] fn bottom_value() -> bool { true // bottom = initialized (start_block_effect counters this at outset) } } -impl<'a, 'gcx, 'tcx> DataflowOperator for MovingOutStatements<'a, 'gcx, 'tcx> { +impl<'a, 'gcx, 'tcx> InitialFlow for MovingOutStatements<'a, 'gcx, 'tcx> { #[inline] fn bottom_value() -> bool { false // bottom = no loans in scope by default } } -impl<'a, 'gcx, 'tcx> DataflowOperator for EverInitializedLvals<'a, 'gcx, 'tcx> { +impl<'a, 'gcx, 'tcx> InitialFlow for EverInitializedLvals<'a, 'gcx, 'tcx> { #[inline] fn bottom_value() -> bool { false // bottom = no initialized variables by default diff --git a/src/librustc_mir/dataflow/impls/storage_liveness.rs b/src/librustc_mir/dataflow/impls/storage_liveness.rs index 9e5a71683781b..dea61542ac4e2 100644 --- a/src/librustc_mir/dataflow/impls/storage_liveness.rs +++ b/src/librustc_mir/dataflow/impls/storage_liveness.rs @@ -74,7 +74,7 @@ impl<'a, 'tcx> BitwiseOperator for MaybeStorageLive<'a, 'tcx> { } } -impl<'a, 'tcx> DataflowOperator for MaybeStorageLive<'a, 'tcx> { +impl<'a, 'tcx> InitialFlow for MaybeStorageLive<'a, 'tcx> { #[inline] fn bottom_value() -> bool { false // bottom = dead diff --git a/src/librustc_mir/dataflow/mod.rs b/src/librustc_mir/dataflow/mod.rs index 8624cbbf50ab3..fe152b46c1d51 100644 --- a/src/librustc_mir/dataflow/mod.rs +++ b/src/librustc_mir/dataflow/mod.rs @@ -127,7 +127,7 @@ pub(crate) fn do_dataflow<'a, 'gcx, 'tcx, BD, P>(tcx: TyCtxt<'a, 'gcx, 'tcx>, bd: BD, p: P) -> DataflowResults - where BD: BitDenotation, + where BD: BitDenotation + InitialFlow, P: Fn(&BD, BD::Idx) -> DebugFormatted { let name_found = |sess: &Session, attrs: &[ast::Attribute], name| -> Option { @@ -176,7 +176,6 @@ impl<'a, 'tcx: 'a, BD> DataflowAnalysis<'a, 'tcx, BD> where BD: BitDenotation }; while propcx.changed { propcx.changed = false; - propcx.reset(&mut temp); propcx.walk_cfg(&mut temp); } } @@ -212,13 +211,6 @@ impl<'a, 'tcx: 'a, BD> DataflowAnalysis<'a, 'tcx, BD> where BD: BitDenotation impl<'b, 'a: 'b, 'tcx: 'a, BD> PropagationContext<'b, 'a, 'tcx, BD> where BD: BitDenotation { - fn reset(&mut self, bits: &mut IdxSet) { - let e = if BD::bottom_value() {!0} else {0}; - for b in bits.words_mut() { - *b = e; - } - } - fn walk_cfg(&mut self, in_out: &mut IdxSet) { let mir = self.builder.mir; for (bb_idx, bb_data) in mir.basic_blocks().iter().enumerate() { @@ -554,12 +546,17 @@ impl AllSets { } /// Parameterization for the precise form of data flow that is used. -pub trait DataflowOperator: BitwiseOperator { +/// `InitialFlow` handles initializing the bitvectors before any +/// code is inspected by the analysis. Analyses that need more nuanced +/// initialization (e.g. they need to consult the results of some other +/// dataflow analysis to set up the initial bitvectors) should not +/// implement this. +pub trait InitialFlow { /// Specifies the initial value for each bit in the `on_entry` set fn bottom_value() -> bool; } -pub trait BitDenotation: DataflowOperator { +pub trait BitDenotation: BitwiseOperator { /// Specifies what index type is used to access the bitvector. type Idx: Idx; @@ -642,7 +639,7 @@ impl<'a, 'gcx, 'tcx: 'a, D> DataflowAnalysis<'a, 'tcx, D> where D: BitDenotation pub fn new(_tcx: TyCtxt<'a, 'gcx, 'tcx>, mir: &'a Mir<'tcx>, dead_unwinds: &'a IdxSet, - denotation: D) -> Self { + 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; From e123117cb7c1b7f8854858721ccbdbca4e918061 Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Mon, 27 Nov 2017 17:37:18 +0100 Subject: [PATCH 04/23] Refactoring: Allow `BlockSets.on_entry` to denote locally accumulated intrablock state. (Still musing about whether it could make sense to revise the design here to make these constraints on usage explicit.) --- src/librustc_mir/dataflow/mod.rs | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/src/librustc_mir/dataflow/mod.rs b/src/librustc_mir/dataflow/mod.rs index fe152b46c1d51..f7efd7fe7da03 100644 --- a/src/librustc_mir/dataflow/mod.rs +++ b/src/librustc_mir/dataflow/mod.rs @@ -195,7 +195,12 @@ impl<'a, 'tcx: 'a, BD> DataflowAnalysis<'a, 'tcx, BD> where BD: BitDenotation for (bb, data) in self.mir.basic_blocks().iter_enumerated() { let &mir::BasicBlockData { ref statements, ref terminator, is_cleanup: _ } = data; + let mut interim_state; let sets = &mut self.flow_state.sets.for_block(bb.index()); + if BD::accumulates_intrablock_state() { + interim_state = sets.on_entry.to_owned(); + sets.on_entry = &mut interim_state; + } for j_stmt in 0..statements.len() { let location = Location { block: bb, statement_index: j_stmt }; self.flow_state.operator.statement_effect(sets, location); @@ -560,6 +565,31 @@ pub trait BitDenotation: BitwiseOperator { /// Specifies what index type is used to access the bitvector. type Idx: Idx; + /// Some analyses want to accumulate knowledge within a block when + /// analyzing its statements for building the gen/kill sets. Override + /// this method to return true in such cases. + /// + /// When this returns true, the statement-effect (re)construction + /// will clone the `on_entry` state and pass along a reference via + /// `sets.on_entry` to that local clone into `statement_effect` and + /// `terminator_effect`). + /// + /// When its false, no local clone is constucted; instead a + /// reference directly into `on_entry` is passed along via + /// `sets.on_entry` instead, which represents the flow state at + /// the block's start, not necessarily the state immediately prior + /// to the statement/terminator under analysis. + /// + /// In either case, the passed reference is mutable; but this is a + /// wart from using the `BlockSets` type in the API; the intention + /// is that the `statement_effect` and `terminator_effect` methods + /// mutate only the gen/kill sets. + /// + /// FIXME: We should consider enforcing the intention described in + /// the previous paragraph by passing the three sets in separate + /// parameters to encode their distinct mutabilities. + fn accumulates_intrablock_state() -> bool { false } + /// A name describing the dataflow analysis that this /// BitDenotation is supporting. The name should be something /// suitable for plugging in as part of a filename e.g. avoid From 39e126c0013ca03a6b0abfcad7fdbe361e63b6e7 Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Tue, 28 Nov 2017 12:49:06 +0100 Subject: [PATCH 05/23] Refactoring alpha-rename `place` (`BorrowData` field) to `borrowed_place`. --- src/librustc_mir/borrow_check/error_reporting.rs | 8 ++++---- src/librustc_mir/borrow_check/mod.rs | 4 ++-- src/librustc_mir/dataflow/impls/borrows.rs | 8 ++++---- 3 files changed, 10 insertions(+), 10 deletions(-) diff --git a/src/librustc_mir/borrow_check/error_reporting.rs b/src/librustc_mir/borrow_check/error_reporting.rs index 186598001da6a..1d5e09b164a1c 100644 --- a/src/librustc_mir/borrow_check/error_reporting.rs +++ b/src/librustc_mir/borrow_check/error_reporting.rs @@ -96,7 +96,7 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { Some(name) => format!("`{}`", name), None => "value".to_owned(), }; - let borrow_msg = match self.describe_place(&borrow.place) { + let borrow_msg = match self.describe_place(&borrow.borrowed_place) { Some(name) => format!("`{}`", name), None => "value".to_owned(), }; @@ -124,7 +124,7 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { span, &self.describe_place(place).unwrap_or("_".to_owned()), self.retrieve_borrow_span(borrow), - &self.describe_place(&borrow.place).unwrap_or("_".to_owned()), + &self.describe_place(&borrow.borrowed_place).unwrap_or("_".to_owned()), Origin::Mir, ); @@ -328,7 +328,7 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { ) { let end_span = borrows.opt_region_end_span(&borrow.region); let scope_tree = borrows.scope_tree(); - let root_place = self.prefixes(&borrow.place, PrefixSet::All).last().unwrap(); + let root_place = self.prefixes(&borrow.borrowed_place, PrefixSet::All).last().unwrap(); match root_place { &Place::Local(local) => { @@ -357,7 +357,7 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { _ => drop_span, }; - match (borrow.region, &self.describe_place(&borrow.place)) { + match (borrow.region, &self.describe_place(&borrow.borrowed_place)) { (RegionKind::ReScope(_), Some(name)) => { self.report_scoped_local_value_does_not_live_long_enough( name, &scope_tree, &borrow, drop_span, borrow_span, proper_span, end_span); diff --git a/src/librustc_mir/borrow_check/mod.rs b/src/librustc_mir/borrow_check/mod.rs index 8bfb5fee9f646..98f974aba5068 100644 --- a/src/librustc_mir/borrow_check/mod.rs +++ b/src/librustc_mir/borrow_check/mod.rs @@ -917,7 +917,7 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { flow_state: &Flows<'cx, 'gcx, 'tcx>) { debug!("check_for_invalidation_at_exit({:?})", borrow); - let place = &borrow.place; + let place = &borrow.borrowed_place; let root_place = self.prefixes(place, PrefixSet::All).last().unwrap(); // FIXME(nll-rfc#40): do more precise destructor tracking here. For now @@ -1792,7 +1792,7 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { for i in flow_state.borrows.elems_incoming() { let borrowed = &data[i]; - if self.places_conflict(&borrowed.place, place, access) { + if self.places_conflict(&borrowed.borrowed_place, place, access) { let ctrl = op(self, i, borrowed); if ctrl == Control::Break { return; } } diff --git a/src/librustc_mir/dataflow/impls/borrows.rs b/src/librustc_mir/dataflow/impls/borrows.rs index eeb7f32c25c29..e6a682e1be919 100644 --- a/src/librustc_mir/dataflow/impls/borrows.rs +++ b/src/librustc_mir/dataflow/impls/borrows.rs @@ -49,7 +49,7 @@ pub struct Borrows<'a, 'gcx: 'tcx, 'tcx: 'a> { } // temporarily allow some dead fields: `kind` and `region` will be -// needed by borrowck; `place` will probably be a MovePathIndex when +// needed by borrowck; `borrowed_place` will probably be a MovePathIndex when // that is extended to include borrowed data paths. #[allow(dead_code)] #[derive(Debug)] @@ -57,7 +57,7 @@ pub struct BorrowData<'tcx> { pub(crate) location: Location, pub(crate) kind: mir::BorrowKind, pub(crate) region: Region<'tcx>, - pub(crate) place: mir::Place<'tcx>, + pub(crate) borrowed_place: mir::Place<'tcx>, } impl<'tcx> fmt::Display for BorrowData<'tcx> { @@ -69,7 +69,7 @@ impl<'tcx> fmt::Display for BorrowData<'tcx> { }; let region = format!("{}", self.region); let region = if region.len() > 0 { format!("{} ", region) } else { region }; - write!(w, "&{}{}{:?}", region, kind, self.place) + write!(w, "&{}{}{:?}", region, kind, self.borrowed_place) } } @@ -131,7 +131,7 @@ impl<'a, 'gcx, 'tcx> Borrows<'a, 'gcx, 'tcx> { if is_unsafe_place(self.tcx, self.mir, place) { return; } let borrow = BorrowData { - location: location, kind: kind, region: region, place: place.clone(), + location, kind, region, borrowed_place: place.clone(), }; let idx = self.idx_vec.push(borrow); self.location_map.insert(location, idx); From e437e499d1ca9fa2dae69077358b3d1ffd06d27b Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Fri, 1 Dec 2017 12:12:41 +0100 Subject: [PATCH 06/23] Implement Borrow/BorrowMut/ToOwned relationships betweed IdxSetBuf and IdxSet. --- src/librustc_data_structures/indexed_set.rs | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/src/librustc_data_structures/indexed_set.rs b/src/librustc_data_structures/indexed_set.rs index 5d7139507b33f..223e08de826ce 100644 --- a/src/librustc_data_structures/indexed_set.rs +++ b/src/librustc_data_structures/indexed_set.rs @@ -8,6 +8,7 @@ // option. This file may not be copied, modified, or distributed // except according to those terms. +use std::borrow::{Borrow, BorrowMut, ToOwned}; use std::fmt; use std::iter; use std::marker::PhantomData; @@ -73,6 +74,25 @@ pub struct IdxSet { bits: [Word], } +impl Borrow> for IdxSetBuf { + fn borrow(&self) -> &IdxSet { + &*self + } +} + +impl BorrowMut> for IdxSetBuf { + fn borrow_mut(&mut self) -> &mut IdxSet { + &mut *self + } +} + +impl ToOwned for IdxSet { + type Owned = IdxSetBuf; + fn to_owned(&self) -> Self::Owned { + IdxSet::to_owned(self) + } +} + impl fmt::Debug for IdxSetBuf { fn fmt(&self, w: &mut fmt::Formatter) -> fmt::Result { w.debug_list() From ef64ace8aa16f05968fd46e8ee3eaa3308240d54 Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Fri, 1 Dec 2017 12:13:01 +0100 Subject: [PATCH 07/23] Allow `mir::Place` to be used as a key in hashtables. --- src/librustc/mir/mod.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/librustc/mir/mod.rs b/src/librustc/mir/mod.rs index 64e601ab1e734..36bee69d8f067 100644 --- a/src/librustc/mir/mod.rs +++ b/src/librustc/mir/mod.rs @@ -1136,7 +1136,7 @@ impl<'tcx> Debug for Statement<'tcx> { /// A path to a value; something that can be evaluated without /// changing or disturbing program state. -#[derive(Clone, PartialEq, RustcEncodable, RustcDecodable)] +#[derive(Clone, PartialEq, Eq, Hash, RustcEncodable, RustcDecodable)] pub enum Place<'tcx> { /// local variable Local(Local), @@ -1150,7 +1150,7 @@ pub enum Place<'tcx> { /// The def-id of a static, along with its normalized type (which is /// stored to avoid requiring normalization when reading MIR). -#[derive(Clone, PartialEq, RustcEncodable, RustcDecodable)] +#[derive(Clone, PartialEq, Eq, Hash, RustcEncodable, RustcDecodable)] pub struct Static<'tcx> { pub def_id: DefId, pub ty: Ty<'tcx>, From ced5a701ffd1ba3ff153c811ff81577f28d67785 Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Fri, 1 Dec 2017 12:32:51 +0100 Subject: [PATCH 08/23] New `ActiveBorrows` dataflow for two-phase `&mut`; not yet borrowed-checked. High-level picture: The old `Borrows` analysis is now called `Reservations` (implemented as a newtype wrapper around `Borrows`); this continues to compute whether a `Rvalue::Ref` can reach a statement without an intervening `EndRegion`. In addition, we also track what `Place` each such `Rvalue::Ref` was immediately assigned to in a given borrow (yay for MIR-structural properties!). The new `ActiveBorrows` analysis then tracks the initial use of any of those assigned `Places` for a given borrow. I.e. a borrow becomes "active" immediately after it starts being "used" in some way. (This is conservative in the sense that we will treat a copy `x = y;` as a use of `y`; in principle one might further delay activation in such cases.) The new `ActiveBorrows` analysis needs to take the `Reservations` results as an initial input, because the reservation state influences the gen/kill sets for `ActiveBorrows`. In particular, a use of `a` activates a borrow `a = &b` if and only if there exists a path (in the control flow graph) from the borrow to that use. So we need to know if the borrow reaches a given use to know if it really gets a gen-bit or not. * Incorporating the output from one dataflow analysis into the input of another required more changes to the infrastructure than I had expected, and even after those changes, the resulting code is still a bit subtle. * In particular, Since we need to know the intrablock reservation state, we need to dynamically update a bitvector for the reservations as we are also trying to compute the gen/kills bitvector for the active borrows. * The way I ended up deciding to do this (after also toying with at least two other designs) is to put both the reservation state and the active borrow state into a single bitvector. That is why we now have separate (but related) `BorrowIndex` and `ReserveOrActivateIndex`: each borrow index maps to a pair of neighboring reservation and activation indexes. As noted above, these changes are solely adding the active borrows dataflow analysis (and updating the existing code to cope with the switch from `Borrows` to `Reservations`). The code to process the bitvector in the borrow checker currently just skips over all of the active borrow bits. But atop this commit, one *can* observe the analysis results by looking at the graphviz output, e.g. via ```rust #[rustc_mir(borrowck_graphviz_preflow="pre_two_phase.dot", borrowck_graphviz_postflow="post_two_phase.dot")] ``` Includes doc for `FindPlaceUses`, as well as `Reservations` and `ActiveBorrows` structs, which are wrappers are the `Borrows` struct that dictate which flow analysis should be performed. --- .../borrow_check/error_reporting.rs | 6 +- src/librustc_mir/borrow_check/flows.rs | 12 +- src/librustc_mir/borrow_check/mod.rs | 76 ++- src/librustc_mir/dataflow/at_location.rs | 6 +- src/librustc_mir/dataflow/impls/borrows.rs | 458 +++++++++++++++--- src/librustc_mir/dataflow/mod.rs | 142 ++++-- src/librustc_mir/dataflow/move_paths/mod.rs | 3 + 7 files changed, 552 insertions(+), 151 deletions(-) diff --git a/src/librustc_mir/borrow_check/error_reporting.rs b/src/librustc_mir/borrow_check/error_reporting.rs index 1d5e09b164a1c..8f07c73b5eecf 100644 --- a/src/librustc_mir/borrow_check/error_reporting.rs +++ b/src/librustc_mir/borrow_check/error_reporting.rs @@ -19,7 +19,7 @@ use std::rc::Rc; use super::{MirBorrowckCtxt, Context}; use super::{InitializationRequiringAction, PrefixSet}; -use dataflow::{BorrowData, Borrows, FlowAtLocation, MovingOutStatements}; +use dataflow::{ActiveBorrows, BorrowData, FlowAtLocation, MovingOutStatements}; use dataflow::move_paths::MovePathIndex; use util::borrowck_errors::{BorrowckErrors, Origin}; @@ -324,10 +324,10 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { _: Context, borrow: &BorrowData<'tcx>, drop_span: Span, - borrows: &Borrows<'cx, 'gcx, 'tcx> + borrows: &ActiveBorrows<'cx, 'gcx, 'tcx> ) { let end_span = borrows.opt_region_end_span(&borrow.region); - let scope_tree = borrows.scope_tree(); + let scope_tree = borrows.0.scope_tree(); let root_place = self.prefixes(&borrow.borrowed_place, PrefixSet::All).last().unwrap(); match root_place { diff --git a/src/librustc_mir/borrow_check/flows.rs b/src/librustc_mir/borrow_check/flows.rs index 449062989e07e..69a08c7a30df3 100644 --- a/src/librustc_mir/borrow_check/flows.rs +++ b/src/librustc_mir/borrow_check/flows.rs @@ -17,13 +17,13 @@ use rustc::mir::{BasicBlock, Location}; use dataflow::{MaybeInitializedLvals, MaybeUninitializedLvals}; use dataflow::{EverInitializedLvals, MovingOutStatements}; -use dataflow::{Borrows, FlowAtLocation, FlowsAtLocation}; +use dataflow::{ActiveBorrows, FlowAtLocation, FlowsAtLocation}; use dataflow::move_paths::HasMoveData; use std::fmt; // (forced to be `pub` due to its use as an associated type below.) -pub struct Flows<'b, 'gcx: 'tcx, 'tcx: 'b> { - pub borrows: FlowAtLocation>, +pub(crate) struct Flows<'b, 'gcx: 'tcx, 'tcx: 'b> { + pub borrows: FlowAtLocation>, pub inits: FlowAtLocation>, pub uninits: FlowAtLocation>, pub move_outs: FlowAtLocation>, @@ -32,7 +32,7 @@ pub 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>, @@ -87,7 +87,7 @@ impl<'b, 'gcx, 'tcx> fmt::Display for Flows<'b, 'gcx, 'tcx> { s.push_str(", "); }; saw_one = true; - let borrow_data = &self.borrows.operator().borrows()[borrow]; + let borrow_data = &self.borrows.operator().borrows()[borrow.borrow_index()]; s.push_str(&format!("{}", borrow_data)); }); s.push_str("] "); @@ -99,7 +99,7 @@ impl<'b, 'gcx, 'tcx> fmt::Display for Flows<'b, 'gcx, 'tcx> { s.push_str(", "); }; saw_one = true; - let borrow_data = &self.borrows.operator().borrows()[borrow]; + let borrow_data = &self.borrows.operator().borrows()[borrow.borrow_index()]; s.push_str(&format!("{}", borrow_data)); }); s.push_str("] "); diff --git a/src/librustc_mir/borrow_check/mod.rs b/src/librustc_mir/borrow_check/mod.rs index 98f974aba5068..0be250eb96ce8 100644 --- a/src/librustc_mir/borrow_check/mod.rs +++ b/src/librustc_mir/borrow_check/mod.rs @@ -30,11 +30,12 @@ use syntax_pos::Span; use dataflow::{do_dataflow, DebugFormatted}; use dataflow::MoveDataParamEnv; -use dataflow::DataflowResultsConsumer; +use dataflow::{DataflowAnalysis, DataflowResultsConsumer}; use dataflow::{FlowAtLocation, FlowsAtLocation}; use dataflow::{MaybeInitializedLvals, MaybeUninitializedLvals}; use dataflow::{EverInitializedLvals, MovingOutStatements}; -use dataflow::{BorrowData, BorrowIndex, Borrows}; +use dataflow::{Borrows, BorrowData, ReserveOrActivateIndex}; +use dataflow::{ActiveBorrows, Reservations}; use dataflow::move_paths::{IllegalMoveOriginKind, MoveError}; use dataflow::move_paths::{HasMoveData, LookupResult, MoveData, MovePathIndex}; use util::borrowck_errors::{BorrowckErrors, Origin}; @@ -48,6 +49,9 @@ use self::MutateMode::{JustWrite, WriteAndRead}; mod error_reporting; mod flows; mod prefixes; + +use std::borrow::Cow; + pub(crate) mod nll; pub fn provide(providers: &mut Providers) { @@ -205,23 +209,6 @@ 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, def_id, body_id), - |bd, i| DebugFormatted::new(bd.location(i)), - )); - - let mut state = Flows::new( - flow_borrows, - flow_inits, - flow_uninits, - flow_move_outs, - flow_ever_inits, - ); let mut mbcx = MirBorrowckCtxt { tcx: tcx, mir: mir, @@ -237,6 +224,44 @@ fn do_mir_borrowck<'a, 'gcx, 'tcx>( storage_dead_or_drop_error_reported_s: FxHashSet(), }; + 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_inits, + flow_uninits, + flow_move_outs, + flow_ever_inits, + ); + mbcx.analyze_results(&mut state); // entry point for DataflowResultsConsumer opt_closure_req @@ -504,9 +529,8 @@ impl<'cx, 'gcx, 'tcx> DataflowResultsConsumer<'cx, 'tcx> for MirBorrowckCtxt<'cx let data = domain.borrows(); flow_state.borrows.with_elems_outgoing(|borrows| { for i in borrows { - let borrow = &data[i]; + let borrow = &data[i.borrow_index()]; let context = ContextKind::StorageDead.new(loc); - self.check_for_invalidation_at_exit(context, borrow, span, flow_state); } }); @@ -721,7 +745,8 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { WriteKind::StorageDeadOrDrop => { error_reported = true; this.report_borrowed_value_does_not_live_long_enough( - context, borrow, place_span.1, flow_state.borrows.operator()); + context, borrow, place_span.1, + flow_state.borrows.operator()); } WriteKind::Mutate => { error_reported = true; @@ -1778,7 +1803,7 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { flow_state: &Flows<'cx, 'gcx, 'tcx>, mut op: F, ) where - F: FnMut(&mut Self, BorrowIndex, &BorrowData<'tcx>) -> Control, + F: FnMut(&mut Self, ReserveOrActivateIndex, &BorrowData<'tcx>) -> Control, { let (access, place) = access_place; @@ -1790,7 +1815,10 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { // check for loan restricting path P being used. Accounts for // borrows of P, P.a.b, etc. for i in flow_state.borrows.elems_incoming() { - let borrowed = &data[i]; + // FIXME for now, just skip the activation state. + if i.is_activation() { continue } + + let borrowed = &data[i.borrow_index()]; if self.places_conflict(&borrowed.borrowed_place, place, access) { let ctrl = op(self, i, borrowed); diff --git a/src/librustc_mir/dataflow/at_location.rs b/src/librustc_mir/dataflow/at_location.rs index 90ebb45123ed8..fefb378fc753e 100644 --- a/src/librustc_mir/dataflow/at_location.rs +++ b/src/librustc_mir/dataflow/at_location.rs @@ -121,9 +121,8 @@ impl FlowsAtLocation for FlowAtLocation fn reconstruct_statement_effect(&mut self, loc: Location) { self.stmt_gen.reset_to_empty(); self.stmt_kill.reset_to_empty(); - let mut ignored = IdxSetBuf::new_empty(0); let mut sets = BlockSets { - on_entry: &mut ignored, + on_entry: &mut self.curr_state, gen_set: &mut self.stmt_gen, kill_set: &mut self.stmt_kill, }; @@ -135,9 +134,8 @@ impl FlowsAtLocation for FlowAtLocation fn reconstruct_terminator_effect(&mut self, loc: Location) { self.stmt_gen.reset_to_empty(); self.stmt_kill.reset_to_empty(); - let mut ignored = IdxSetBuf::new_empty(0); let mut sets = BlockSets { - on_entry: &mut ignored, + on_entry: &mut self.curr_state, gen_set: &mut self.stmt_gen, kill_set: &mut self.stmt_kill, }; diff --git a/src/librustc_mir/dataflow/impls/borrows.rs b/src/librustc_mir/dataflow/impls/borrows.rs index e6a682e1be919..abfc3159cff84 100644 --- a/src/librustc_mir/dataflow/impls/borrows.rs +++ b/src/librustc_mir/dataflow/impls/borrows.rs @@ -11,8 +11,8 @@ use rustc::hir; use rustc::hir::def_id::DefId; use rustc::middle::region; -use rustc::mir::{self, Location, Mir}; -use rustc::mir::visit::Visitor; +use rustc::mir::{self, Location, Place, Mir}; +use rustc::mir::visit::{PlaceContext, Visitor}; use rustc::ty::{self, Region, TyCtxt}; use rustc::ty::RegionKind; use rustc::ty::RegionKind::ReScope; @@ -20,16 +20,17 @@ use rustc::util::nodemap::{FxHashMap, FxHashSet}; use rustc_data_structures::bitslice::{BitwiseOperator}; use rustc_data_structures::indexed_set::{IdxSet}; -use rustc_data_structures::indexed_vec::{IndexVec}; +use rustc_data_structures::indexed_vec::{Idx, IndexVec}; use dataflow::{BitDenotation, BlockSets, InitialFlow}; -pub use dataflow::indexes::BorrowIndex; +pub use dataflow::indexes::{BorrowIndex, ReserveOrActivateIndex}; use borrow_check::nll::region_infer::RegionInferenceContext; use borrow_check::nll::ToRegionVid; use syntax_pos::Span; use std::fmt; +use std::hash::Hash; use std::rc::Rc; // `Borrows` maps each dataflow bit to an `Rvalue::Ref`, which can be @@ -42,12 +43,47 @@ pub struct Borrows<'a, 'gcx: 'tcx, 'tcx: 'a> { root_scope: Option, borrows: IndexVec>, location_map: FxHashMap, + assigned_map: FxHashMap, FxHashSet>, region_map: FxHashMap, FxHashSet>, 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 +/// whereever `'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 whereever `'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()) + } +} + +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()) + } +} + // temporarily allow some dead fields: `kind` and `region` will be // needed by borrowck; `borrowed_place` will probably be a MovePathIndex when // that is extended to include borrowed data paths. @@ -58,6 +94,7 @@ pub struct BorrowData<'tcx> { pub(crate) kind: mir::BorrowKind, pub(crate) region: Region<'tcx>, pub(crate) borrowed_place: mir::Place<'tcx>, + pub(crate) assigned_place: mir::Place<'tcx>, } impl<'tcx> fmt::Display for BorrowData<'tcx> { @@ -73,6 +110,21 @@ impl<'tcx> fmt::Display for BorrowData<'tcx> { } } +impl ReserveOrActivateIndex { + fn reserved(i: BorrowIndex) -> Self { ReserveOrActivateIndex::new((i.index() * 2)) } + fn active(i: BorrowIndex) -> Self { ReserveOrActivateIndex::new((i.index() * 2) + 1) } + + pub(crate) fn is_reservation(self) -> bool { self.index() % 2 == 0 } + pub(crate) fn is_activation(self) -> bool { self.index() % 2 == 1} + + pub(crate) fn kind(self) -> &'static str { + if self.is_reservation() { "reserved" } else { "active" } + } + pub(crate) fn borrow_index(self) -> BorrowIndex { + BorrowIndex::new(self.index() / 2) + } +} + impl<'a, 'gcx, 'tcx> Borrows<'a, 'gcx, 'tcx> { pub fn new(tcx: TyCtxt<'a, 'gcx, 'tcx>, mir: &'a Mir<'tcx>, @@ -89,6 +141,7 @@ impl<'a, 'gcx, 'tcx> Borrows<'a, 'gcx, 'tcx> { mir, idx_vec: IndexVec::new(), location_map: FxHashMap(), + assigned_map: FxHashMap(), region_map: FxHashMap(), local_map: FxHashMap(), region_span_map: FxHashMap() @@ -100,6 +153,7 @@ impl<'a, 'gcx, 'tcx> Borrows<'a, 'gcx, 'tcx> { scope_tree, root_scope, location_map: visitor.location_map, + assigned_map: visitor.assigned_map, region_map: visitor.region_map, local_map: visitor.local_map, region_span_map: visitor.region_span_map, @@ -110,13 +164,16 @@ impl<'a, 'gcx, 'tcx> Borrows<'a, 'gcx, 'tcx> { mir: &'a Mir<'tcx>, idx_vec: IndexVec>, location_map: FxHashMap, + assigned_map: FxHashMap, FxHashSet>, region_map: FxHashMap, FxHashSet>, local_map: FxHashMap>, region_span_map: FxHashMap, } impl<'a, 'gcx, 'tcx> Visitor<'tcx> for GatherBorrows<'a, 'gcx, 'tcx> { - fn visit_rvalue(&mut self, + fn visit_assign(&mut self, + block: mir::BasicBlock, + assigned_place: &mir::Place<'tcx>, rvalue: &mir::Rvalue<'tcx>, location: mir::Location) { fn root_local(mut p: &mir::Place<'_>) -> Option { @@ -127,23 +184,59 @@ impl<'a, 'gcx, 'tcx> Borrows<'a, 'gcx, 'tcx> { }} } - if let mir::Rvalue::Ref(region, kind, ref place) = *rvalue { - if is_unsafe_place(self.tcx, self.mir, place) { return; } + if let mir::Rvalue::Ref(region, kind, ref borrowed_place) = *rvalue { + if is_unsafe_place(self.tcx, self.mir, borrowed_place) { return; } let borrow = BorrowData { - location, kind, region, borrowed_place: place.clone(), + 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); - let borrows = self.region_map.entry(region).or_insert(FxHashSet()); - borrows.insert(idx); + insert(&mut self.assigned_map, assigned_place, idx); + insert(&mut self.region_map, ®ion, idx); + if let Some(local) = root_local(borrowed_place) { + insert(&mut self.local_map, &local, idx); + } + } + + return self.super_assign(block, assigned_place, rvalue, location); - if let Some(local) = root_local(place) { - let borrows = self.local_map.entry(local).or_insert(FxHashSet()); - borrows.insert(idx); + fn insert<'a, K, V>(map: &'a mut FxHashMap>, + k: &K, + v: V) + where K: Clone+Eq+Hash, V: Eq+Hash + { + map.entry(k.clone()) + .or_insert(FxHashSet()) + .insert(v); + } + } + + fn visit_rvalue(&mut self, + rvalue: &mir::Rvalue<'tcx>, + location: mir::Location) { + if let mir::Rvalue::Ref(region, kind, ref place) = *rvalue { + // double-check that we already registered a BorrowData for this + + let mut found_it = false; + for idx in &self.region_map[region] { + let bd = &self.idx_vec[*idx]; + if bd.location == location && + bd.kind == kind && + bd.region == region && + bd.borrowed_place == *place + { + found_it = true; + break; + } } + assert!(found_it, "Ref {:?} at {:?} missing BorrowData", rvalue, location); } + + return self.super_rvalue(rvalue, location); } fn visit_statement(&mut self, @@ -153,7 +246,7 @@ impl<'a, 'gcx, 'tcx> Borrows<'a, 'gcx, 'tcx> { if let mir::StatementKind::EndRegion(region_scope) = statement.kind { self.region_span_map.insert(ReScope(region_scope), statement.source_info.span); } - self.super_statement(block, statement, location); + return self.super_statement(block, statement, location); } } } @@ -166,26 +259,19 @@ impl<'a, 'gcx, 'tcx> Borrows<'a, 'gcx, 'tcx> { &self.borrows[idx].location } - /// 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 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(span.end_point()), - None => Some(self.mir.span.end_point()) - } - } - } - } - /// 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.) fn kill_loans_out_of_scope_at_location(&self, - sets: &mut BlockSets, - location: Location) { + sets: &mut BlockSets, + location: Location, + is_activations: bool) { if let Some(ref regioncx) = self.nonlexical_regioncx { for (borrow_index, borrow_data) in self.borrows.iter_enumerated() { let borrow_region = borrow_data.region.to_region_vid(); @@ -201,45 +287,74 @@ impl<'a, 'gcx, 'tcx> Borrows<'a, 'gcx, 'tcx> { // mismatch here by not generating a kill for the // location on the borrow itself. if location != borrow_data.location { - sets.kill(&borrow_index); + sets.kill(&ReserveOrActivateIndex::reserved(borrow_index)); + } + + // FIXME: the logic used to justify the above + // "accounting for mismatch" does not generalize + // to activations, so we set the kill-bits without + // that same location check here. + // + // But... can we get into a situation where the + // gen/kill bits are both sets in this case, in + // which case we *do* need an analogous guard of + // some kind? + if is_activations { + sets.kill(&ReserveOrActivateIndex::active(borrow_index)); } } } } } -} -impl<'a, 'gcx, 'tcx> BitDenotation for Borrows<'a, 'gcx, 'tcx> { - type Idx = BorrowIndex; - fn name() -> &'static str { "borrows" } - fn bits_per_block(&self) -> usize { - self.borrows.len() - } - fn start_block_effect(&self, _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 statement_effect(&self, - sets: &mut BlockSets, - location: Location) { + /// 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) { let block = &self.mir.basic_blocks().get(location.block).unwrap_or_else(|| { panic!("could not find block at location {:?}", location); }); let stmt = block.statements.get(location.statement_index).unwrap_or_else(|| { panic!("could not find statement at location {:?}"); }); + + if is_activations { + // INVARIANT: At this point, `sets.on_entry` should + // correctly reflect the reservations as we enter the + // statement (because accumulates_intrablock_state is + // overridden) + // + // Now compute effect of the statement on the activations + // themselves in the ActiveBorrows state. + let mut find = FindPlaceUses { sets, assigned_map: &self.assigned_map }; + find.visit_statement(location.block, stmt, location); + } + match stmt.kind { + // EndRegion kills any borrows (reservations and active borrows both) mir::StatementKind::EndRegion(region_scope) => { if let Some(borrow_indexes) = self.region_map.get(&ReScope(region_scope)) { assert!(self.nonlexical_regioncx.is_none()); - sets.kill_all(borrow_indexes); + for idx in borrow_indexes { + sets.kill(&ReserveOrActivateIndex::reserved(*idx)); + if is_activations { + sets.kill(&ReserveOrActivateIndex::active(*idx)); + } + } } else { // (if there is no entry, then there are no borrows to be tracked) } } mir::StatementKind::Assign(_, ref rhs) => { + // NOTE: if/when the Assign case is revised to inspect + // the assigned_place here, make sure to also + // re-consider the current implementations of the + // propagate_call_return method. + if let mir::Rvalue::Ref(region, _, ref place) = *rhs { if is_unsafe_place(self.tcx, self.mir, place) { return; } if let RegionKind::ReEmpty = region { @@ -254,7 +369,7 @@ impl<'a, 'gcx, 'tcx> BitDenotation for Borrows<'a, 'gcx, 'tcx> { assert!(self.region_map.get(region).unwrap_or_else(|| { panic!("could not find BorrowIndexs for region {:?}", region); }).contains(&index)); - sets.gen(&index); + sets.gen(&ReserveOrActivateIndex::reserved(*index)); } } @@ -264,7 +379,12 @@ impl<'a, 'gcx, 'tcx> BitDenotation for Borrows<'a, 'gcx, 'tcx> { // // FIXME: expand this to variables that are assigned over. if let Some(borrow_indexes) = self.local_map.get(&local) { - sets.kill_all(borrow_indexes); + 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))); + } } } @@ -276,16 +396,28 @@ impl<'a, 'gcx, 'tcx> BitDenotation for Borrows<'a, 'gcx, 'tcx> { } - self.kill_loans_out_of_scope_at_location(sets, location); + self.kill_loans_out_of_scope_at_location(sets, location, is_activations); } - fn terminator_effect(&self, - sets: &mut BlockSets, - location: 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) { let block = &self.mir.basic_blocks().get(location.block).unwrap_or_else(|| { panic!("could not find block at location {:?}", location); }); - match block.terminator().kind { + + let term = block.terminator(); + if is_activations { + // Any uses of reserved Places in the statement are now activated. + let mut find = FindPlaceUses { sets, assigned_map: &self.assigned_map }; + find.visit_terminator(location.block, term, location); + } + + match term.kind { mir::TerminatorKind::Resume | mir::TerminatorKind::Return | mir::TerminatorKind::GeneratorDrop => { @@ -304,7 +436,10 @@ impl<'a, 'gcx, 'tcx> BitDenotation for Borrows<'a, 'gcx, 'tcx> { if *scope != root_scope && self.scope_tree.is_subscope_of(*scope, root_scope) { - sets.kill(&borrow_index); + sets.kill(&ReserveOrActivateIndex::reserved(borrow_index)); + if is_activations { + sets.kill(&ReserveOrActivateIndex::active(borrow_index)); + } } } } @@ -320,29 +455,224 @@ impl<'a, 'gcx, 'tcx> BitDenotation for Borrows<'a, 'gcx, 'tcx> { mir::TerminatorKind::FalseEdges {..} | mir::TerminatorKind::Unreachable => {} } - self.kill_loans_out_of_scope_at_location(sets, location); + self.kill_loans_out_of_scope_at_location(sets, location, is_activations); + } +} + +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(span.end_point()), + None => Some(self.0.mir.span.end_point()) + } + } + } + } +} + +/// `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 an place do not activate it + PlaceContext::StorageLive | PlaceContext::StorageDead => false, + + // validation effects do not activate an 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, + + // pure overwrites of an place do not activate it. (note + // PlaceContext::Call is solely about dest place) + PlaceContext::Store | PlaceContext::Call => false, + + // reads of an 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 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 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, + _in_out: &mut IdxSet, _call_bb: mir::BasicBlock, _dest_bb: mir::BasicBlock, _dest_place: &mir::Place) { - // there are no effects on the region scopes from method calls. + // 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> 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 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 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> { + #[inline] + fn join(&self, pred1: usize, pred2: usize) -> usize { + pred1 | pred2 // union effects of preds when computing reservations } } -impl<'a, 'gcx, 'tcx> BitwiseOperator for Borrows<'a, 'gcx, 'tcx> { +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 borrows + pred1 | pred2 // union effects of preds when computing activations } } -impl<'a, 'gcx, 'tcx> InitialFlow for Borrows<'a, 'gcx, 'tcx> { +impl<'a, 'gcx, 'tcx> InitialFlow for Reservations<'a, 'gcx, 'tcx> { #[inline] fn bottom_value() -> bool { - false // bottom = no Rvalue::Refs are active by default + false // bottom = no Rvalue::Refs are reserved by default } } diff --git a/src/librustc_mir/dataflow/mod.rs b/src/librustc_mir/dataflow/mod.rs index f7efd7fe7da03..83c46e0199eeb 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; +use std::borrow::{Borrow, Cow}; use std::fmt; use std::io; use std::mem; @@ -29,7 +29,8 @@ pub use self::impls::{MaybeStorageLive}; pub use self::impls::{MaybeInitializedLvals, MaybeUninitializedLvals}; pub use self::impls::{DefinitelyInitializedLvals, MovingOutStatements}; pub use self::impls::EverInitializedLvals; -pub use self::impls::borrows::{Borrows, BorrowData, BorrowIndex}; +pub use self::impls::borrows::{Borrows, BorrowData}; +pub(crate) use self::impls::borrows::{ActiveBorrows, Reservations, ReserveOrActivateIndex}; pub use self::at_location::{FlowAtLocation, FlowsAtLocation}; pub(crate) use self::drop_flag_effects::*; @@ -120,7 +121,7 @@ pub struct MoveDataParamEnv<'gcx, 'tcx> { } pub(crate) fn do_dataflow<'a, 'gcx, 'tcx, BD, P>(tcx: TyCtxt<'a, 'gcx, 'tcx>, - mir: &Mir<'tcx>, + mir: &'a Mir<'tcx>, node_id: ast::NodeId, attributes: &[ast::Attribute], dead_unwinds: &IdxSet, @@ -130,34 +131,46 @@ pub(crate) fn do_dataflow<'a, 'gcx, 'tcx, BD, P>(tcx: TyCtxt<'a, 'gcx, 'tcx>, where BD: BitDenotation + InitialFlow, P: Fn(&BD, BD::Idx) -> DebugFormatted { - let name_found = |sess: &Session, attrs: &[ast::Attribute], name| -> Option { - if let Some(item) = has_rustc_mir_with(attrs, name) { - if let Some(s) = item.value_str() { - return Some(s.to_string()) - } else { - sess.span_err( - item.span, - &format!("{} attribute requires a path", item.name())); - return None; + let flow_state = DataflowAnalysis::new(mir, dead_unwinds, bd); + flow_state.run(tcx, node_id, attributes, p) +} + +impl<'a, 'gcx: 'tcx, 'tcx: 'a, BD> DataflowAnalysis<'a, 'tcx, BD> where BD: BitDenotation +{ + pub(crate) fn run

(self, + tcx: TyCtxt<'a, 'gcx, 'tcx>, + node_id: ast::NodeId, + attributes: &[ast::Attribute], + p: P) -> DataflowResults + where P: Fn(&BD, BD::Idx) -> DebugFormatted + { + let name_found = |sess: &Session, attrs: &[ast::Attribute], name| -> Option { + if let Some(item) = has_rustc_mir_with(attrs, name) { + if let Some(s) = item.value_str() { + return Some(s.to_string()) + } else { + sess.span_err( + item.span, + &format!("{} attribute requires a path", item.name())); + return None; + } } - } - return None; - }; + return None; + }; - let print_preflow_to = - name_found(tcx.sess, attributes, "borrowck_graphviz_preflow"); - let print_postflow_to = - name_found(tcx.sess, attributes, "borrowck_graphviz_postflow"); + let print_preflow_to = + name_found(tcx.sess, attributes, "borrowck_graphviz_preflow"); + let print_postflow_to = + name_found(tcx.sess, attributes, "borrowck_graphviz_postflow"); - let mut mbcx = DataflowBuilder { - node_id, - print_preflow_to, - print_postflow_to, - flow_state: DataflowAnalysis::new(tcx, mir, dead_unwinds, bd), - }; + let mut mbcx = DataflowBuilder { + node_id, + print_preflow_to, print_postflow_to, flow_state: self, + }; - mbcx.dataflow(p); - mbcx.flow_state.results() + mbcx.dataflow(p); + mbcx.flow_state.results() + } } struct PropagationContext<'b, 'a: 'b, 'tcx: 'a, O> where O: 'b + BitDenotation @@ -181,11 +194,7 @@ impl<'a, 'tcx: 'a, BD> DataflowAnalysis<'a, 'tcx, BD> where BD: BitDenotation } fn build_sets(&mut self) { - // First we need to build the entry-, gen- and kill-sets. The - // gather_moves information provides a high-level mapping from - // mir-locations to the MoveOuts (and those correspond - // directly to gen-sets here). But we still need to figure out - // the kill-sets. + // First we need to build the entry-, gen- and kill-sets. { let sets = &mut self.flow_state.sets.for_block(mir::START_BLOCK.index()); @@ -197,18 +206,26 @@ impl<'a, 'tcx: 'a, BD> DataflowAnalysis<'a, 'tcx, BD> where BD: BitDenotation let mut interim_state; let sets = &mut self.flow_state.sets.for_block(bb.index()); - if BD::accumulates_intrablock_state() { + let track_intrablock = BD::accumulates_intrablock_state(); + if track_intrablock { + debug!("swapping in mutable on_entry, initially {:?}", sets.on_entry); interim_state = sets.on_entry.to_owned(); sets.on_entry = &mut interim_state; } for j_stmt in 0..statements.len() { let location = Location { block: bb, statement_index: j_stmt }; self.flow_state.operator.statement_effect(sets, location); + if track_intrablock { + sets.apply_local_effect(); + } } if terminator.is_some() { let location = Location { block: bb, statement_index: statements.len() }; self.flow_state.operator.terminator_effect(sets, location); + if track_intrablock { + sets.apply_local_effect(); + } } } } @@ -271,7 +288,7 @@ impl<'a, 'tcx: 'a, BD> DataflowBuilder<'a, 'tcx, BD> where BD: BitDenotation /// Maps each block to a set of bits #[derive(Debug)] -struct Bits { +pub(crate) struct Bits { bits: IdxSetBuf, } @@ -292,7 +309,7 @@ impl Bits { /// underlying flow analysis results, because it needs to handle cases /// where we are combining the results of *multiple* flow analyses /// (e.g. borrows + inits + uninits). -pub trait DataflowResultsConsumer<'a, 'tcx: 'a> { +pub(crate) trait DataflowResultsConsumer<'a, 'tcx: 'a> { type FlowState: FlowsAtLocation; // Observation Hooks: override (at least one of) these to get analysis feedback. @@ -471,6 +488,7 @@ pub struct AllSets { /// killed during the iteration. (This is such a good idea that the /// `fn gen` and `fn kill` methods that set their state enforce this /// for you.) +#[derive(Debug)] pub struct BlockSets<'a, E: Idx> { /// Dataflow state immediately before control flow enters the given block. pub(crate) on_entry: &'a mut IdxSet, @@ -512,6 +530,7 @@ impl<'a, E:Idx> BlockSets<'a, E> { self.gen_set.remove(e); self.kill_set.add(e); } + fn kill_all(&mut self, i: I) where I: IntoIterator, I::Item: Borrow @@ -520,6 +539,11 @@ impl<'a, E:Idx> BlockSets<'a, E> { self.kill(j.borrow()); } } + + fn apply_local_effect(&mut self) { + self.on_entry.union(&self.gen_set); + self.on_entry.subtract(&self.kill_set); + } } impl AllSets { @@ -548,6 +572,9 @@ 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. @@ -664,29 +691,33 @@ pub trait BitDenotation: BitwiseOperator { dest_place: &mir::Place); } -impl<'a, 'gcx, 'tcx: 'a, D> DataflowAnalysis<'a, 'tcx, D> where D: BitDenotation +impl<'a, 'tcx, D> DataflowAnalysis<'a, 'tcx, D> where D: BitDenotation { - pub fn new(_tcx: TyCtxt<'a, 'gcx, 'tcx>, - mir: &'a Mir<'tcx>, + pub fn new(mir: &'a Mir<'tcx>, 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; - - // (now rounded up to multiple of word size) - let bits_per_block = words_per_block * usize_bits; - - let num_blocks = mir.basic_blocks().len(); - let num_overall = num_blocks * bits_per_block; - - let zeroes = Bits::new(IdxSetBuf::new_empty(num_overall)); + let num_overall = Self::num_bits_overall(mir, bits_per_block); 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, @@ -696,12 +727,23 @@ impl<'a, 'gcx, 'tcx: 'a, D> DataflowAnalysis<'a, 'tcx, D> where D: BitDenotation words_per_block, gen_sets: zeroes.clone(), kill_sets: zeroes, - on_entry_sets: on_entry, + on_entry_sets: on_entry.into_owned(), }, 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; + // (now rounded up to multiple of word size) + let bits_per_block = words_per_block * usize_bits; + + let num_blocks = mir.basic_blocks().len(); + let num_overall = num_blocks * bits_per_block; + num_overall } } diff --git a/src/librustc_mir/dataflow/move_paths/mod.rs b/src/librustc_mir/dataflow/move_paths/mod.rs index 9d91e1344dc37..bcf4662211e8b 100644 --- a/src/librustc_mir/dataflow/move_paths/mod.rs +++ b/src/librustc_mir/dataflow/move_paths/mod.rs @@ -65,6 +65,9 @@ pub(crate) mod indexes { /// Index into Borrows.locations new_index!(BorrowIndex, "bw"); + + /// Index into Reservations/Activations bitvector + new_index!(ReserveOrActivateIndex, "ra"); } pub use self::indexes::MovePathIndex; From 658ed797003ff392e42e9f29e99905d8558b36f0 Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Wed, 13 Dec 2017 00:39:42 -0600 Subject: [PATCH 09/23] Add some doc to `each_borrow_involving_path` iteration function. --- src/librustc_mir/borrow_check/mod.rs | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/src/librustc_mir/borrow_check/mod.rs b/src/librustc_mir/borrow_check/mod.rs index 0be250eb96ce8..57e49c820118e 100644 --- a/src/librustc_mir/borrow_check/mod.rs +++ b/src/librustc_mir/borrow_check/mod.rs @@ -1796,6 +1796,18 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { unreachable!("iter::repeat returned None") } + /// This function iterates over all of the current borrows + /// (represented by 1-bits in `flow_state.borrows`) that conflict + /// with an access to a place, invoking the `op` callback for each + /// one. + /// + /// "Current borrow" here means a borrow that reaches the point in + /// the control-flow where the access occurs. + /// + /// The borrow's phase is represented by the ReserveOrActivateIndex + /// passed to the callback: one can call `is_reservation()` and + /// `is_activation()` to determine what phase the borrow is + /// currently in, when such distinction matters. fn each_borrow_involving_path( &mut self, _context: Context, From 1334638144d660822e02f087b6168356375e92ac Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Wed, 13 Dec 2017 01:06:39 -0600 Subject: [PATCH 10/23] Add some doc to `struct Borrows`. --- src/librustc_mir/dataflow/impls/borrows.rs | 25 +++++++++++++++++++--- 1 file changed, 22 insertions(+), 3 deletions(-) diff --git a/src/librustc_mir/dataflow/impls/borrows.rs b/src/librustc_mir/dataflow/impls/borrows.rs index abfc3159cff84..4c36deae10113 100644 --- a/src/librustc_mir/dataflow/impls/borrows.rs +++ b/src/librustc_mir/dataflow/impls/borrows.rs @@ -33,17 +33,36 @@ use std::fmt; use std::hash::Hash; use std::rc::Rc; -// `Borrows` maps each dataflow bit to an `Rvalue::Ref`, which can be -// uniquely identified in the MIR by the `Location` of the assigment -// statement in which it appears on the right hand side. +/// `Borrows` stores the data used in the analyses that track the flow +/// of borrows. +/// +/// It uniquely identifies every borrow (`Rvalue::Ref`) by a +/// `BorrowIndex`, and maps each such index to a `BorrowData` +/// describing the borrow. These indexes are used for representing the +/// borrows in compact bitvectors. pub struct Borrows<'a, 'gcx: 'tcx, 'tcx: 'a> { tcx: TyCtxt<'a, 'gcx, 'tcx>, mir: &'a Mir<'tcx>, scope_tree: Rc, root_scope: Option, + + /// The fundamental map relating bitvector indexes to the borrows + /// in the MIR. borrows: IndexVec>, + + /// Each borrow is also uniquely identified in the MIR by the + /// `Location` of the assignment statement in which it appears on + /// the right hand side; we map each such location to the + /// corresponding `BorrowIndex`. location_map: FxHashMap, + + /// Every borrow in MIR is immediately stored into a place via an + /// assignment statement. This maps each such assigned place back + /// to its borrow-indexes. assigned_map: FxHashMap, FxHashSet>, + + /// Every borrow has a region; this maps each such regions back to + /// its borrow-indexes. region_map: FxHashMap, FxHashSet>, local_map: FxHashMap>, region_span_map: FxHashMap, From 36216456a61f9d99bbbe187fd120fb39f891683d Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Fri, 1 Dec 2017 16:02:15 +0100 Subject: [PATCH 11/23] Incorporate active-borrows dataflow into MIR borrow check, yielding two-phase `&mut`-borrow support. This (new) support sits under `-Z two-phase-borrows` debugflag. (Still needs tests. That's coming next.) --- src/librustc/session/config.rs | 2 ++ src/librustc_mir/borrow_check/mod.rs | 24 ++++++++++++++++++++---- src/librustc_mir/dataflow/at_location.rs | 6 ++++-- 3 files changed, 26 insertions(+), 6 deletions(-) diff --git a/src/librustc/session/config.rs b/src/librustc/session/config.rs index 0dcd3e8081080..bff31afb5e5f1 100644 --- a/src/librustc/session/config.rs +++ b/src/librustc/session/config.rs @@ -1011,6 +1011,8 @@ options! {DebuggingOptions, DebuggingSetter, basic_debugging_options, "emit EndRegion as part of MIR; enable transforms that solely process EndRegion"), borrowck: Option = (None, parse_opt_string, [UNTRACKED], "select which borrowck is used (`ast`, `mir`, or `compare`)"), + two_phase_borrows: bool = (false, parse_bool, [UNTRACKED], + "use two-phase reserved/active distinction for `&mut` borrows in MIR borrowck"), time_passes: bool = (false, parse_bool, [UNTRACKED], "measure time of each rustc pass"), count_llvm_insns: bool = (false, parse_bool, diff --git a/src/librustc_mir/borrow_check/mod.rs b/src/librustc_mir/borrow_check/mod.rs index 57e49c820118e..1b02bbbc152a5 100644 --- a/src/librustc_mir/borrow_check/mod.rs +++ b/src/librustc_mir/borrow_check/mod.rs @@ -701,9 +701,17 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { context, (sd, place_span.0), flow_state, - |this, _index, borrow| match (rw, borrow.kind) { + |this, index, borrow| match (rw, borrow.kind) { (Read(_), BorrowKind::Shared) => Control::Continue, + (Read(kind), BorrowKind::Unique) | (Read(kind), BorrowKind::Mut) => { + // Reading from mere reservations of mutable-borrows is OK. + if this.tcx.sess.opts.debugging_opts.two_phase_borrows && + index.is_reservation() + { + return Control::Continue; + } + match kind { ReadKind::Copy => { error_reported = true; @@ -1826,9 +1834,17 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { // check for loan restricting path P being used. Accounts for // borrows of P, P.a.b, etc. - for i in flow_state.borrows.elems_incoming() { - // FIXME for now, just skip the activation state. - if i.is_activation() { continue } + let mut elems_incoming = flow_state.borrows.elems_incoming(); + while let Some(i) = elems_incoming.next() { + // Skip any reservation that has a corresponding current + // activation. This way, the traversal will visit each + // borrow_index at most once. + if let Some(j) = elems_incoming.peek() { + if i.is_reservation() && j.is_activation() { + assert_eq!(i.borrow_index(), j.borrow_index()); + continue; + } + } let borrowed = &data[i.borrow_index()]; diff --git a/src/librustc_mir/dataflow/at_location.rs b/src/librustc_mir/dataflow/at_location.rs index fefb378fc753e..a6c3398489ad7 100644 --- a/src/librustc_mir/dataflow/at_location.rs +++ b/src/librustc_mir/dataflow/at_location.rs @@ -18,6 +18,8 @@ use rustc_data_structures::indexed_vec::Idx; use dataflow::{BitDenotation, BlockSets, DataflowResults}; use dataflow::move_paths::{HasMoveData, MovePathIndex}; +use std::iter; + /// A trait for "cartesian products" of multiple FlowAtLocation. /// /// There's probably a way to auto-impl this, but I think @@ -94,9 +96,9 @@ where self.curr_state.contains(x) } - pub fn elems_incoming(&self) -> indexed_set::Elems { + pub fn elems_incoming(&self) -> iter::Peekable> { let univ = self.base_results.sets().bits_per_block(); - self.curr_state.elems(univ) + self.curr_state.elems(univ).peekable() } pub fn with_elems_outgoing(&self, f: F) From 5f759a90e39b9f5951205621bed06235b37ad36c Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Tue, 5 Dec 2017 15:17:06 +0100 Subject: [PATCH 12/23] the minimal test for two-phase borrows: the core example from niko's blog post on it. --- .../run-pass/borrowck/two-phase-baseline.rs | 21 +++++++++++++++++++ 1 file changed, 21 insertions(+) create mode 100644 src/test/run-pass/borrowck/two-phase-baseline.rs diff --git a/src/test/run-pass/borrowck/two-phase-baseline.rs b/src/test/run-pass/borrowck/two-phase-baseline.rs new file mode 100644 index 0000000000000..6623444926398 --- /dev/null +++ b/src/test/run-pass/borrowck/two-phase-baseline.rs @@ -0,0 +1,21 @@ +// Copyright 2017 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. + +// 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 + +// This is the "goto example" for why we want two phase borrows. + +fn main() { + let mut v = vec![0, 1, 2]; + v.push(v.len()); + assert_eq!(v, [0, 1, 2, 3]); +} From dbbec4d62d579ea786d53ca7eceeb4b243fb8bf1 Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Wed, 6 Dec 2017 00:37:06 +0100 Subject: [PATCH 13/23] tests transcribed from nikos blog post. --- ...o-phase-allow-access-during-reservation.rs | 37 +++++++++++++++++++ .../two-phase-cannot-nest-mut-self-calls.rs | 34 +++++++++++++++++ .../compile-fail/borrowck/two-phase-sneaky.rs | 30 +++++++++++++++ 3 files changed, 101 insertions(+) create mode 100644 src/test/compile-fail/borrowck/two-phase-allow-access-during-reservation.rs create mode 100644 src/test/compile-fail/borrowck/two-phase-cannot-nest-mut-self-calls.rs create mode 100644 src/test/compile-fail/borrowck/two-phase-sneaky.rs 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 new file mode 100644 index 0000000000000..7695bd3e4652c --- /dev/null +++ b/src/test/compile-fail/borrowck/two-phase-allow-access-during-reservation.rs @@ -0,0 +1,37 @@ +// Copyright 2017 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. + +// 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 + +// This is the second counter-example from Niko's blog post +// smallcultfollowing.com/babysteps/blog/2017/03/01/nested-method-calls-via-two-phase-borrowing/ +// +// It is "artificial". It is meant to illustrate directly that we +// should allow an aliasing access during reservation, but *not* while +// the mutable borrow is active. + +fn main() { + /*0*/ let mut i = 0; + + /*1*/ let p = &mut i; // (reservation of `i` starts here) + + /*2*/ let j = i; // OK: `i` is only reserved here + + /*3*/ *p += 1; // (mutable borrow of `i` starts here, since `p` is used) + + /*4*/ let k = i; //[lxl]~ ERROR cannot use `i` because it was mutably borrowed [E0503] + //[nll]~^ ERROR cannot use `i` because it was mutably borrowed [E0503] + + /*5*/ *p += 1; + + let _ = (j, k, p); +} diff --git a/src/test/compile-fail/borrowck/two-phase-cannot-nest-mut-self-calls.rs b/src/test/compile-fail/borrowck/two-phase-cannot-nest-mut-self-calls.rs new file mode 100644 index 0000000000000..01b04708599c0 --- /dev/null +++ b/src/test/compile-fail/borrowck/two-phase-cannot-nest-mut-self-calls.rs @@ -0,0 +1,34 @@ +// Copyright 2017 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. + +// 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 + +// This is the third counter-example from Niko's blog post +// smallcultfollowing.com/babysteps/blog/2017/03/01/nested-method-calls-via-two-phase-borrowing/ +// +// It shows that not all nested method calls on `self` are magically +// allowed by this change. In particular, a nested `&mut` borrow is +// still disallowed. + +fn main() { + + + let mut vec = vec![0, 1]; + vec.get({ + + vec.push(2); + //[lxl]~^ ERROR cannot borrow `vec` as mutable because it is also borrowed as immutable + //[nll]~^^ ERROR cannot borrow `vec` as mutable because it is also borrowed as immutable + + 0 + }); +} diff --git a/src/test/compile-fail/borrowck/two-phase-sneaky.rs b/src/test/compile-fail/borrowck/two-phase-sneaky.rs new file mode 100644 index 0000000000000..32747407c67f0 --- /dev/null +++ b/src/test/compile-fail/borrowck/two-phase-sneaky.rs @@ -0,0 +1,30 @@ +// Copyright 2017 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. + +// 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 + +// This is the first counter-example from Niko's blog post +// smallcultfollowing.com/babysteps/blog/2017/03/01/nested-method-calls-via-two-phase-borrowing/ +// of a danger for code to crash if we just turned off the check for whether +// a mutable-borrow aliases another borrow. + +fn main() { + let mut v: Vec = vec![format!("Hello, ")]; + v[0].push_str({ + + v.push(format!("foo")); + //[lxl]~^ ERROR cannot borrow `v` as mutable more than once at a time [E0499] + //[nll]~^^ ERROR cannot borrow `v` as mutable more than once at a time [E0499] + + "World!" + }); +} From db5420b6f244df4a18c45293de4648c1962b6bb6 Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Wed, 6 Dec 2017 11:56:13 +0100 Subject: [PATCH 14/23] test describing a currently unsupported corner case. --- ...-phase-reservation-sharing-interference.rs | 46 +++++++++++++++++++ 1 file changed, 46 insertions(+) create mode 100644 src/test/compile-fail/borrowck/two-phase-reservation-sharing-interference.rs 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 new file mode 100644 index 0000000000000..cc85315263a4f --- /dev/null +++ b/src/test/compile-fail/borrowck/two-phase-reservation-sharing-interference.rs @@ -0,0 +1,46 @@ +// Copyright 2017 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. + +// 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 + +// This is a corner case that the current implementation is (probably) +// treating more conservatively than is necessary. But it also does +// not seem like a terribly important use case to cover. +// +// So this test is just making a note of the current behavior, with +// the caveat that in the future, the rules may be loosened, at which +// point this test might be thrown out. + +fn main() { + let mut vec = vec![0, 1]; + let delay: &mut Vec<_>; + { + let shared = &vec; + + // we reserve here, which could (on its own) be compatible + // with the shared borrow. But in the current implementation, + // its an error. + delay = &mut vec; + //[lxl]~^ ERROR cannot borrow `vec` as mutable because it is also borrowed as immutable + //[nll]~^^ ERROR cannot borrow `vec` as mutable because it is also borrowed as immutable + + shared[0]; + } + + // the &mut-borrow only becomes active way down here. + // + // (At least in theory; part of the reason this test fails is that + // the constructed MIR throws in extra &mut reborrows which + // flummoxes our attmpt to delay the activation point here.) + delay.push(2); +} + From 9cb92ac27d12c7ded8a1ad736b23e2fe1c99577e Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Wed, 6 Dec 2017 12:25:36 +0100 Subject: [PATCH 15/23] two-phase-reservation-sharing-interference.rs variant that is perhaps more surprising. --- ...hase-reservation-sharing-interference-2.rs | 38 +++++++++++++++++++ 1 file changed, 38 insertions(+) create mode 100644 src/test/compile-fail/borrowck/two-phase-reservation-sharing-interference-2.rs diff --git a/src/test/compile-fail/borrowck/two-phase-reservation-sharing-interference-2.rs b/src/test/compile-fail/borrowck/two-phase-reservation-sharing-interference-2.rs new file mode 100644 index 0000000000000..397b3dafa7d47 --- /dev/null +++ b/src/test/compile-fail/borrowck/two-phase-reservation-sharing-interference-2.rs @@ -0,0 +1,38 @@ +// Copyright 2017 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. + +// 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 + +// This is similar to two-phase-reservation-sharing-interference.rs +// in that it shows a reservation that overlaps with a shared borrow. +// +// However, it is also more immediately concerning because one would +// intutively think that if run-pass/borrowck/two-phase-baseline.rs +// works, then this *should* work too. +// +// As before, the current implementation is (probably) more +// conservative than is necessary. +// +// So this test is just making a note of the current behavior, with +// the caveat that in the future, the rules may be loosened, at which +// point this test might be thrown out. + +fn main() { + let mut v = vec![0, 1, 2]; + let shared = &v; + + v.push(shared.len()); + //[lxl]~^ ERROR cannot borrow `v` as mutable because it is also borrowed as immutable [E0502] + //[nll]~^^ ERROR cannot borrow `v` as mutable because it is also borrowed as immutable [E0502] + + assert_eq!(v, [0, 1, 2, 3]); +} From 18aedf6b23db6f409f8d3073ab0c8ca923e9b141 Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Wed, 13 Dec 2017 00:14:32 -0600 Subject: [PATCH 16/23] Sidestep ICE from `MirBorrowckCtxt::find_closure_span`. --- src/librustc_mir/borrow_check/error_reporting.rs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/librustc_mir/borrow_check/error_reporting.rs b/src/librustc_mir/borrow_check/error_reporting.rs index 8f07c73b5eecf..dafd030a966a9 100644 --- a/src/librustc_mir/borrow_check/error_reporting.rs +++ b/src/librustc_mir/borrow_check/error_reporting.rs @@ -143,6 +143,12 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { use rustc::hir::ExprClosure; use rustc::mir::AggregateKind; + if location.statement_index == self.mir[location.block].statements.len() { + // Code below hasn't been written in a manner to deal with + // a terminator location. + return None; + } + let local = if let StatementKind::Assign(Place::Local(local), _) = self.mir[location.block].statements[location.statement_index].kind { From 5cae7a046922375352ca1ee33ae00df2123972ad Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Thu, 7 Dec 2017 17:45:13 +0100 Subject: [PATCH 17/23] Check activation points as the place where mutable borrows become relevant. Since we are now checking activation points, I removed one of the checks at the reservation point. (You can see the effect this had on two-phase-reservation-sharing-interference-2.rs) Also, since we now have checks at both the reservation point and the activation point, we sometimes would observe duplicate errors (since either one independently interferes with another mutable borrow). To deal with this, I used a similar strategy to one used as discussed on issue #45360: keep a set of errors reported (in this case for reservations), and then avoid doing the checks for the corresponding activations. (This does mean that some errors could get masked, namely for conflicting borrows that start after the reservation but still conflict with the activation, which is unchecked when there was an error for the reservation. But this seems like a reasonable price to pay.) --- src/librustc_mir/borrow_check/mod.rs | 124 +++++++++++++++++- ...o-phase-activation-sharing-interference.rs | 62 +++++++++ ...hase-reservation-sharing-interference-2.rs | 21 ++- 3 files changed, 192 insertions(+), 15 deletions(-) create mode 100644 src/test/compile-fail/borrowck/two-phase-activation-sharing-interference.rs diff --git a/src/librustc_mir/borrow_check/mod.rs b/src/librustc_mir/borrow_check/mod.rs index 1b02bbbc152a5..e9d5bd365e2b3 100644 --- a/src/librustc_mir/borrow_check/mod.rs +++ b/src/librustc_mir/borrow_check/mod.rs @@ -36,6 +36,7 @@ use dataflow::{MaybeInitializedLvals, MaybeUninitializedLvals}; use dataflow::{EverInitializedLvals, MovingOutStatements}; use dataflow::{Borrows, BorrowData, ReserveOrActivateIndex}; use dataflow::{ActiveBorrows, Reservations}; +use dataflow::indexes::{BorrowIndex}; use dataflow::move_paths::{IllegalMoveOriginKind, MoveError}; use dataflow::move_paths::{HasMoveData, LookupResult, MoveData, MovePathIndex}; use util::borrowck_errors::{BorrowckErrors, Origin}; @@ -222,6 +223,7 @@ fn do_mir_borrowck<'a, 'gcx, 'tcx>( }, storage_dead_or_drop_error_reported_l: FxHashSet(), storage_dead_or_drop_error_reported_s: FxHashSet(), + reservation_error_reported: FxHashSet(), }; let borrows = Borrows::new(tcx, mir, opt_regioncx, def_id, body_id); @@ -287,6 +289,14 @@ pub struct MirBorrowckCtxt<'cx, 'gcx: 'tcx, 'tcx: 'cx> { storage_dead_or_drop_error_reported_l: FxHashSet, /// Same as the above, but for statics (thread-locals) storage_dead_or_drop_error_reported_s: FxHashSet, + /// This field keeps track of when borrow conflict errors are reported + /// for reservations, so that we don't report seemingly duplicate + /// errors for corresponding activations + /// + /// FIXME: Ideally this would be a set of BorrowIndex, not Places, + /// but it is currently inconvenient to track down the BorrowIndex + /// at the time we detect and report a reservation error. + reservation_error_reported: FxHashSet>, } // Check that: @@ -318,6 +328,9 @@ impl<'cx, 'gcx, 'tcx> DataflowResultsConsumer<'cx, 'tcx> for MirBorrowckCtxt<'cx flow_state ); let span = stmt.source_info.span; + + self.check_activations(location, span, flow_state); + match stmt.kind { StatementKind::Assign(ref lhs, ref rhs) => { // NOTE: NLL RFC calls for *shallow* write; using Deep @@ -424,6 +437,9 @@ impl<'cx, 'gcx, 'tcx> DataflowResultsConsumer<'cx, 'tcx> for MirBorrowckCtxt<'cx flow_state ); let span = term.source_info.span; + + self.check_activations(location, span, flow_state); + match term.kind { TerminatorKind::SwitchInt { ref discr, @@ -557,7 +573,7 @@ enum Control { } use self::ShallowOrDeep::{Deep, Shallow}; -use self::ReadOrWrite::{Read, Write}; +use self::ReadOrWrite::{Activation, Read, Reservation, Write}; #[derive(Copy, Clone, PartialEq, Eq, Debug)] enum ArtificialField { @@ -592,6 +608,12 @@ enum ReadOrWrite { /// new values or otherwise invalidated (for example, it could be /// de-initialized, as in a move operation). Write(WriteKind), + + /// For two-phase borrows, we distinguish a reservation (which is treated + /// like a Read) from an activation (which is treated like a write), and + /// each of those is furthermore distinguished from Reads/Writes above. + Reservation(WriteKind), + Activation(WriteKind, BorrowIndex), } /// Kind of read access to a value @@ -680,6 +702,14 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { ) -> AccessErrorsReported { let (sd, rw) = kind; + if let Activation(_, borrow_index) = rw { + if self.reservation_error_reported.contains(&place_span.0) { + debug!("skipping access_place for activation of invalid reservation \ + place: {:?} borrow_index: {:?}", place_span.0, borrow_index); + return AccessErrorsReported { mutability_error: false, conflict_error: false }; + } + } + let mutability_error = self.check_access_permissions(place_span, rw, is_local_mutation_allowed); let conflict_error = @@ -702,9 +732,16 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { (sd, place_span.0), flow_state, |this, index, borrow| match (rw, borrow.kind) { - (Read(_), BorrowKind::Shared) => Control::Continue, + // Obviously an activation is compatible with its own reservation; + // so don't check if they interfere. + (Activation(_, activating), _) if index.is_reservation() && + activating == index.borrow_index() => Control::Continue, - (Read(kind), BorrowKind::Unique) | (Read(kind), BorrowKind::Mut) => { + (Read(_), BorrowKind::Shared) | + (Reservation(..), BorrowKind::Shared) => Control::Continue, + + (Read(kind), BorrowKind::Unique) | + (Read(kind), BorrowKind::Mut) => { // Reading from mere reservations of mutable-borrows is OK. if this.tcx.sess.opts.debugging_opts.two_phase_borrows && index.is_reservation() @@ -734,13 +771,32 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { } Control::Break } + + (Reservation(kind), BorrowKind::Unique) | + (Reservation(kind), BorrowKind::Mut) | + (Activation(kind, _), _) | (Write(kind), _) => { + + match rw { + Reservation(_) => { + debug!("recording invalid reservation of \ + place: {:?}", place_span.0); + this.reservation_error_reported.insert(place_span.0.clone()); + } + Activation(_, activating) => { + debug!("observing check_place for activation of \ + borrow_index: {:?}", activating); + } + Read(..) | Write(..) => {} + } + match kind { WriteKind::MutableBorrow(bk) => { let end_issued_loan_span = flow_state .borrows .operator() .opt_region_end_span(&borrow.region); + error_reported = true; this.report_conflicting_borrow( context, @@ -827,9 +883,15 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { let access_kind = match bk { BorrowKind::Shared => (Deep, Read(ReadKind::Borrow(bk))), BorrowKind::Unique | BorrowKind::Mut => { - (Deep, Write(WriteKind::MutableBorrow(bk))) + let wk = WriteKind::MutableBorrow(bk); + if self.tcx.sess.opts.debugging_opts.two_phase_borrows { + (Deep, Reservation(wk)) + } else { + (Deep, Write(wk)) + } } }; + self.access_place( context, (place, span), @@ -837,6 +899,7 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { LocalMutationIsAllowed::No, flow_state, ); + self.check_if_path_is_moved( context, InitializationRequiringAction::Borrow, @@ -1007,6 +1070,49 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { ) } } + + fn check_activations(&mut self, + location: Location, + span: Span, + flow_state: &Flows<'cx, 'gcx, 'tcx>) + { + if !self.tcx.sess.opts.debugging_opts.two_phase_borrows { + return; + } + + // Two-phase borrow support: For each activation that is newly + // generated at this statement, check if it interferes with + // another borrow. + let domain = flow_state.borrows.operator(); + let data = domain.borrows(); + flow_state.borrows.each_gen_bit(|gen| { + if gen.is_activation() && // must be activation, + !flow_state.borrows.contains(&gen) // and newly generated. + { + let borrow_index = gen.borrow_index(); + let borrow = &data[borrow_index]; + // currently the flow analysis registers + // activations for both mutable and immutable + // borrows. So make sure we are talking about a + // mutable borrow before we check it. + match borrow.kind { + BorrowKind::Shared => return, + BorrowKind::Unique | + BorrowKind::Mut => {} + } + + self.access_place(ContextKind::Activation.new(location), + (&borrow.borrowed_place, span), + (Deep, Activation(WriteKind::MutableBorrow(borrow.kind), + borrow_index)), + LocalMutationIsAllowed::No, + flow_state); + // We do not need to call `check_if_path_is_moved` + // again, as we already called it when we made the + // initial reservation. + } + }); + } } impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { @@ -1250,11 +1356,13 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { ); let mut error_reported = false; match kind { + Reservation(WriteKind::MutableBorrow(BorrowKind::Unique)) | Write(WriteKind::MutableBorrow(BorrowKind::Unique)) => { if let Err(_place_err) = self.is_mutable(place, LocalMutationIsAllowed::Yes) { span_bug!(span, "&unique borrow for {:?} should not fail", place); } } + Reservation(WriteKind::MutableBorrow(BorrowKind::Mut)) | Write(WriteKind::MutableBorrow(BorrowKind::Mut)) => if let Err(place_err) = self.is_mutable(place, is_local_mutation_allowed) { @@ -1277,6 +1385,7 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { err.emit(); }, + Reservation(WriteKind::Mutate) | Write(WriteKind::Mutate) => { if let Err(place_err) = self.is_mutable(place, is_local_mutation_allowed) { error_reported = true; @@ -1298,6 +1407,9 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { err.emit(); } } + Reservation(WriteKind::Move) | + Reservation(WriteKind::StorageDeadOrDrop) | + Reservation(WriteKind::MutableBorrow(BorrowKind::Shared)) | Write(WriteKind::Move) | Write(WriteKind::StorageDeadOrDrop) | Write(WriteKind::MutableBorrow(BorrowKind::Shared)) => { @@ -1312,6 +1424,9 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { ); } } + + Activation(..) => {} // permission checks are done at Reservation point. + Read(ReadKind::Borrow(BorrowKind::Unique)) | Read(ReadKind::Borrow(BorrowKind::Mut)) | Read(ReadKind::Borrow(BorrowKind::Shared)) | @@ -1892,6 +2007,7 @@ struct Context { #[derive(Copy, Clone, PartialEq, Eq, Debug)] enum ContextKind { + Activation, AssignLhs, AssignRhs, SetDiscrim, 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 new file mode 100644 index 0000000000000..b6f5e17f1f609 --- /dev/null +++ b/src/test/compile-fail/borrowck/two-phase-activation-sharing-interference.rs @@ -0,0 +1,62 @@ +// Copyright 2017 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. + +// 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 + +// This is an important corner case pointed out by Niko: one is +// allowed to initiate a shared borrow during a reservation, but it +// *must end* before the activation occurs. +// +// FIXME: for clarity, diagnostics for these cases might be better off +// if they specifically said "cannot activate mutable borrow of `x`" + +#![allow(dead_code)] + +fn read(_: &i32) { } + +fn ok() { + let mut x = 3; + let y = &mut x; + { let z = &x; read(z); } + *y += 1; +} + +fn not_ok() { + let mut x = 3; + let y = &mut x; + let z = &x; + *y += 1; + //[lxl]~^ ERROR cannot borrow `x` as mutable because it is also borrowed as immutable + //[nll]~^^ ERROR cannot borrow `x` as mutable because it is also borrowed as immutable + read(z); +} + +fn should_be_ok_with_nll() { + let mut x = 3; + let y = &mut x; + let z = &x; + read(z); + *y += 1; + //[lxl]~^ ERROR cannot borrow `x` as mutable because it is also borrowed as immutable + // (okay with nll today) +} + +fn should_also_eventually_be_ok_with_nll() { + let mut x = 3; + let y = &mut x; + let _z = &x; + *y += 1; + //[lxl]~^ ERROR cannot borrow `x` as mutable because it is also borrowed as immutable + //[nll]~^^ ERROR cannot borrow `x` as mutable because it is also borrowed as immutable +} + +fn main() { } diff --git a/src/test/compile-fail/borrowck/two-phase-reservation-sharing-interference-2.rs b/src/test/compile-fail/borrowck/two-phase-reservation-sharing-interference-2.rs index 397b3dafa7d47..fc9100c8a9a86 100644 --- a/src/test/compile-fail/borrowck/two-phase-reservation-sharing-interference-2.rs +++ b/src/test/compile-fail/borrowck/two-phase-reservation-sharing-interference-2.rs @@ -15,24 +15,23 @@ // This is similar to two-phase-reservation-sharing-interference.rs // in that it shows a reservation that overlaps with a shared borrow. // -// However, it is also more immediately concerning because one would -// intutively think that if run-pass/borrowck/two-phase-baseline.rs -// works, then this *should* work too. +// Currently, this test fails with lexical lifetimes, but succeeds +// with non-lexical lifetimes. (The reason is because the activation +// of the mutable borrow ends up overlapping with a lexically-scoped +// shared borrow; but a non-lexical shared borrow can end before the +// activation occurs.) // -// As before, the current implementation is (probably) more -// conservative than is necessary. -// -// So this test is just making a note of the current behavior, with -// the caveat that in the future, the rules may be loosened, at which -// point this test might be thrown out. +// So this test is just making a note of the current behavior. + +#![feature(rustc_attrs)] -fn main() { +#[rustc_error] +fn main() { //[nll]~ ERROR compilation successful let mut v = vec![0, 1, 2]; let shared = &v; v.push(shared.len()); //[lxl]~^ ERROR cannot borrow `v` as mutable because it is also borrowed as immutable [E0502] - //[nll]~^^ ERROR cannot borrow `v` as mutable because it is also borrowed as immutable [E0502] assert_eq!(v, [0, 1, 2, 3]); } From 3c7d9ff90a06c44d31dad018ebb5e613b1c74033 Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Wed, 13 Dec 2017 18:07:02 -0600 Subject: [PATCH 18/23] Address review comment: use `.get` instead of indexing to cope w/ terminators. (Same net effect as code from before; just cleaner way to get there.) --- .../borrow_check/error_reporting.rs | 17 ++++------------- 1 file changed, 4 insertions(+), 13 deletions(-) diff --git a/src/librustc_mir/borrow_check/error_reporting.rs b/src/librustc_mir/borrow_check/error_reporting.rs index dafd030a966a9..31a94499fd0cc 100644 --- a/src/librustc_mir/borrow_check/error_reporting.rs +++ b/src/librustc_mir/borrow_check/error_reporting.rs @@ -11,7 +11,7 @@ use syntax_pos::Span; use rustc::middle::region::ScopeTree; use rustc::mir::{BorrowKind, Field, Local, Location, Operand}; -use rustc::mir::{Place, ProjectionElem, Rvalue, StatementKind}; +use rustc::mir::{Place, ProjectionElem, Rvalue, Statement, StatementKind}; use rustc::ty::{self, RegionKind}; use rustc_data_structures::indexed_vec::Idx; @@ -143,18 +143,9 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { use rustc::hir::ExprClosure; use rustc::mir::AggregateKind; - if location.statement_index == self.mir[location.block].statements.len() { - // Code below hasn't been written in a manner to deal with - // a terminator location. - return None; - } - - let local = if let StatementKind::Assign(Place::Local(local), _) = - self.mir[location.block].statements[location.statement_index].kind - { - local - } else { - return None; + let local = match self.mir[location.block].statements.get(location.statement_index) { + Some(&Statement { kind: StatementKind::Assign(Place::Local(local), _), .. }) => local, + _ => return None, }; for stmt in &self.mir[location.block].statements[location.statement_index + 1..] { From f96777c9ffa1d564f26fee7ed64e4ccc4f8dd1fa Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Wed, 13 Dec 2017 18:10:37 -0600 Subject: [PATCH 19/23] After discussion with ariel, replacing a guard within kill_loans_out_of_scope_at_location. Instead we are "just" careful to invoke it (which sets up a bunch of kill bits) before we go into the code that sets up the gen bits. That way, when the gen bits are set up, they will override any previously set kill-bits for those reservations or activations. --- src/librustc_mir/dataflow/impls/borrows.rs | 66 +++++++++++----------- 1 file changed, 33 insertions(+), 33 deletions(-) diff --git a/src/librustc_mir/dataflow/impls/borrows.rs b/src/librustc_mir/dataflow/impls/borrows.rs index 4c36deae10113..c61a57cdda0e9 100644 --- a/src/librustc_mir/dataflow/impls/borrows.rs +++ b/src/librustc_mir/dataflow/impls/borrows.rs @@ -292,32 +292,21 @@ impl<'a, 'gcx, 'tcx> Borrows<'a, 'gcx, 'tcx> { location: Location, is_activations: bool) { 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 + // does not contain `borrow_region`, then then that means + // that the statement at `location` kills the borrow. + // + // We are careful always to call this function *before* we + // set up the gen-bits for the statement or + // termanator. That way, if the effect of the statement or + // terminator *does* introduce a new loan of the same + // region, then setting that gen-bit will override any + // potential kill introduced here. for (borrow_index, borrow_data) in self.borrows.iter_enumerated() { let borrow_region = borrow_data.region.to_region_vid(); if !regioncx.region_contains_point(borrow_region, location) { - // The region checker really considers the borrow - // to start at the point **after** the location of - // the borrow, but the borrow checker puts the gen - // directly **on** the location of the - // borrow. This results in a gen/kill both being - // generated for same point if we are not - // careful. Probably we should change the point of - // the gen, but for now we hackily account for the - // mismatch here by not generating a kill for the - // location on the borrow itself. - if location != borrow_data.location { - sets.kill(&ReserveOrActivateIndex::reserved(borrow_index)); - } - - // FIXME: the logic used to justify the above - // "accounting for mismatch" does not generalize - // to activations, so we set the kill-bits without - // that same location check here. - // - // But... can we get into a situation where the - // gen/kill bits are both sets in this case, in - // which case we *do* need an analogous guard of - // some kind? + sets.kill(&ReserveOrActivateIndex::reserved(borrow_index)); if is_activations { sets.kill(&ReserveOrActivateIndex::active(borrow_index)); } @@ -340,14 +329,18 @@ 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: At this point, `sets.on_entry` should - // correctly reflect the reservations as we enter the - // statement (because accumulates_intrablock_state is - // overridden) + // INVARIANT: `sets.on_entry` accurately captures + // reservations on entry to statement (b/c + // accumulates_intrablock_state is overridden for + // ActiveBorrows). // - // Now compute effect of the statement on the activations - // themselves in the ActiveBorrows state. + // 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); } @@ -414,8 +407,6 @@ impl<'a, 'gcx, 'tcx> Borrows<'a, 'gcx, 'tcx> { mir::StatementKind::Nop => {} } - - self.kill_loans_out_of_scope_at_location(sets, location, is_activations); } /// Models terminator effect in Reservations and ActiveBorrows @@ -429,9 +420,19 @@ impl<'a, 'gcx, 'tcx> Borrows<'a, 'gcx, 'tcx> { 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 { - // Any uses of reserved Places in the statement are now activated. + // 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); } @@ -474,7 +475,6 @@ impl<'a, 'gcx, 'tcx> Borrows<'a, 'gcx, 'tcx> { mir::TerminatorKind::FalseEdges {..} | mir::TerminatorKind::Unreachable => {} } - self.kill_loans_out_of_scope_at_location(sets, location, is_activations); } } From b75248ef4eeac4cf06e8cfd2fa6e36ab488eb4d1 Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Thu, 14 Dec 2017 09:03:04 -0600 Subject: [PATCH 20/23] Address review note: `AccessErrorsReported` meant to track whether error reported at *any* point in past. --- src/librustc_mir/borrow_check/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/librustc_mir/borrow_check/mod.rs b/src/librustc_mir/borrow_check/mod.rs index e9d5bd365e2b3..074b4a09a2d49 100644 --- a/src/librustc_mir/borrow_check/mod.rs +++ b/src/librustc_mir/borrow_check/mod.rs @@ -706,7 +706,7 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { if self.reservation_error_reported.contains(&place_span.0) { debug!("skipping access_place for activation of invalid reservation \ place: {:?} borrow_index: {:?}", place_span.0, borrow_index); - return AccessErrorsReported { mutability_error: false, conflict_error: false }; + return AccessErrorsReported { mutability_error: false, conflict_error: true }; } } From b0421fa7de7b309cf884ecd9b23f93bbbf237048 Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Thu, 14 Dec 2017 16:28:26 -0600 Subject: [PATCH 21/23] Address review feedback: don't bother skipping reservations paired with activations. --- src/librustc_mir/borrow_check/mod.rs | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/src/librustc_mir/borrow_check/mod.rs b/src/librustc_mir/borrow_check/mod.rs index 074b4a09a2d49..adb4a7a925549 100644 --- a/src/librustc_mir/borrow_check/mod.rs +++ b/src/librustc_mir/borrow_check/mod.rs @@ -1951,16 +1951,6 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { // borrows of P, P.a.b, etc. let mut elems_incoming = flow_state.borrows.elems_incoming(); while let Some(i) = elems_incoming.next() { - // Skip any reservation that has a corresponding current - // activation. This way, the traversal will visit each - // borrow_index at most once. - if let Some(j) = elems_incoming.peek() { - if i.is_reservation() && j.is_activation() { - assert_eq!(i.borrow_index(), j.borrow_index()); - continue; - } - } - let borrowed = &data[i.borrow_index()]; if self.places_conflict(&borrowed.borrowed_place, place, access) { From d654cd3b8bdbeee7a0bf9dac89de7e3e4f535e99 Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Thu, 14 Dec 2017 16:30:05 -0600 Subject: [PATCH 22/23] Review feedback: Added test with control flow merge of two borrows "before activation" In reality the currently generated MIR has at least one of the activations in a copy that occurs before the merge. But still, good to have a test, in anticipation of that potentially changing... --- ...se-control-flow-split-before-activation.rs | 27 +++++++++++++++++++ 1 file changed, 27 insertions(+) create mode 100644 src/test/run-pass/borrowck/two-phase-control-flow-split-before-activation.rs diff --git a/src/test/run-pass/borrowck/two-phase-control-flow-split-before-activation.rs b/src/test/run-pass/borrowck/two-phase-control-flow-split-before-activation.rs new file mode 100644 index 0000000000000..a891e6072a7a6 --- /dev/null +++ b/src/test/run-pass/borrowck/two-phase-control-flow-split-before-activation.rs @@ -0,0 +1,27 @@ +// Copyright 2017 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. + +// 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 + +fn main() { + let mut a = 0; + let mut b = 0; + let p = if maybe() { + &mut a + } else { + &mut b + }; + use_(p); +} + +fn maybe() -> bool { false } +fn use_(_: T) { } From 159037e05383f2349a709aa1c1681f11f89c552a Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Thu, 14 Dec 2017 17:34:16 -0600 Subject: [PATCH 23/23] Address review feedback: don't treat "first" activation special. Instead, filter out (non-)conflicts of activiations with themselves in the same manner that we filter out non-conflict between an activation and its reservation. --- src/librustc_mir/borrow_check/mod.rs | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/src/librustc_mir/borrow_check/mod.rs b/src/librustc_mir/borrow_check/mod.rs index adb4a7a925549..39bcd2b6ae063 100644 --- a/src/librustc_mir/borrow_check/mod.rs +++ b/src/librustc_mir/borrow_check/mod.rs @@ -732,10 +732,19 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { (sd, place_span.0), flow_state, |this, index, borrow| match (rw, borrow.kind) { - // Obviously an activation is compatible with its own reservation; - // so don't check if they interfere. - (Activation(_, activating), _) if index.is_reservation() && - activating == index.borrow_index() => Control::Continue, + // Obviously an activation is compatible with its own + // reservation (or even prior activating uses of same + // borrow); so don't check if they interfere. + // + // NOTE: *reservations* do conflict with themselves; + // thus aren't injecting unsoundenss w/ this check.) + (Activation(_, activating), _) if activating == index.borrow_index() => + { + debug!("check_access_for_conflict place_span: {:?} sd: {:?} rw: {:?} \ + skipping {:?} b/c activation of same borrow_index: {:?}", + place_span, sd, rw, (index, borrow), index.borrow_index()); + Control::Continue + } (Read(_), BorrowKind::Shared) | (Reservation(..), BorrowKind::Shared) => Control::Continue, @@ -1086,8 +1095,7 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { let domain = flow_state.borrows.operator(); let data = domain.borrows(); flow_state.borrows.each_gen_bit(|gen| { - if gen.is_activation() && // must be activation, - !flow_state.borrows.contains(&gen) // and newly generated. + if gen.is_activation() { let borrow_index = gen.borrow_index(); let borrow = &data[borrow_index];