From a4c1ca3ecebcdc94e708378098365d6965abe861 Mon Sep 17 00:00:00 2001 From: b-naber Date: Wed, 24 May 2023 11:23:50 +0000 Subject: [PATCH 01/20] introduce borrowed locals analysis based on liveness analysis --- Cargo.lock | 1 + compiler/rustc_mir_dataflow/Cargo.toml | 1 + .../src/framework/cursor.rs | 1 - .../src/impls/live_borrows.rs | 642 ++++++++++++++++++ compiler/rustc_mir_dataflow/src/impls/mod.rs | 4 + compiler/rustc_mir_transform/src/generator.rs | 39 +- 6 files changed, 685 insertions(+), 3 deletions(-) create mode 100644 compiler/rustc_mir_dataflow/src/impls/live_borrows.rs diff --git a/Cargo.lock b/Cargo.lock index 5dbe91557e12d..a1a31025bb405 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3760,6 +3760,7 @@ dependencies = [ name = "rustc_mir_dataflow" version = "0.0.0" dependencies = [ + "either", "polonius-engine", "regex", "rustc_ast", diff --git a/compiler/rustc_mir_dataflow/Cargo.toml b/compiler/rustc_mir_dataflow/Cargo.toml index 4a296bb33670f..2a9ab9c4269e6 100644 --- a/compiler/rustc_mir_dataflow/Cargo.toml +++ b/compiler/rustc_mir_dataflow/Cargo.toml @@ -22,3 +22,4 @@ rustc_middle = { path = "../rustc_middle" } rustc_serialize = { path = "../rustc_serialize" } rustc_target = { path = "../rustc_target" } rustc_span = { path = "../rustc_span" } +either = "1" diff --git a/compiler/rustc_mir_dataflow/src/framework/cursor.rs b/compiler/rustc_mir_dataflow/src/framework/cursor.rs index c978bddfef540..7e55d820418b7 100644 --- a/compiler/rustc_mir_dataflow/src/framework/cursor.rs +++ b/compiler/rustc_mir_dataflow/src/framework/cursor.rs @@ -140,7 +140,6 @@ where } /// Allows inspection of unreachable basic blocks even with `debug_assertions` enabled. - #[cfg(test)] pub(crate) fn allow_unreachable(&mut self) { #[cfg(debug_assertions)] self.reachable_blocks.insert_all() diff --git a/compiler/rustc_mir_dataflow/src/impls/live_borrows.rs b/compiler/rustc_mir_dataflow/src/impls/live_borrows.rs new file mode 100644 index 0000000000000..452848aea6cca --- /dev/null +++ b/compiler/rustc_mir_dataflow/src/impls/live_borrows.rs @@ -0,0 +1,642 @@ +use super::*; + +use crate::framework::{Analysis, Results, ResultsCursor}; +use crate::{ + AnalysisDomain, Backward, CallReturnPlaces, GenKill, GenKillAnalysis, ResultsRefCursor, +}; +use rustc_data_structures::fx::FxHashMap; +use rustc_data_structures::graph::implementation::{Graph, NodeIndex}; +use rustc_middle::mir::visit::PlaceContext; +use rustc_middle::mir::visit::Visitor; +use rustc_middle::mir::*; +use rustc_middle::ty::{self, Ty, TypeSuperVisitable, TypeVisitable}; + +use core::ops::ControlFlow; +use either::Either; + +/// This module defines a more fine-grained analysis for `Local`s that are live due +/// to outstanding references or raw pointers. +/// The idea behind the analysis is that we first build a dependency graph between +/// `Local`s corresponding to references or pointers and the `Local`s that are borrowed. +/// This is done by the `BorrowDependencies` struct. +/// As a second step we perform a liveness analysis for references and pointers, which is +/// done by `LiveBorrows`. +/// Finally we combine the results of the liveness analysis and the dependency graph to +/// infer which borrowed locals need to be live at a given `Location`. + +#[derive(Copy, Clone, Debug)] +enum NodeKind { + // An node corresponding to the place on the lhs of a statement like + // `_3 = Ref(_, _, _4)` + Borrow(Local), + + // Nodes corresponding to the place on the lhs of a statement like + // `_2 = Aggregate(Adt(..), _, _, _, _), [move _3, move _6])`, + // where _3 and _6 are places corresponding to references or raw pointers + // or locals that are borrowed (`_2 = Ref(..)`). + LocalOrLocalWithRefs(Local), +} + +impl NodeKind { + fn get_local(&self) -> Local { + match self { + Self::Borrow(local) => *local, + Self::LocalOrLocalWithRefs(local) => *local, + } + } +} + +/// TypeVisitor that looks for `ty::Ref`, `ty::RawPtr`. Additionally looks for `ty::Param`, +/// which could themselves refer to references or raw pointers. +struct MaybeHasRefsOrPointersVisitor { + has_refs_or_pointers: bool, + has_params: bool, +} + +impl MaybeHasRefsOrPointersVisitor { + fn new() -> Self { + MaybeHasRefsOrPointersVisitor { has_refs_or_pointers: false, has_params: false } + } +} + +impl<'tcx> ty::TypeVisitor> for MaybeHasRefsOrPointersVisitor { + type BreakTy = (); + + #[instrument(skip(self), level = "debug")] + fn visit_ty(&mut self, ty: Ty<'tcx>) -> ControlFlow { + match ty.kind() { + ty::Ref(..) => { + self.has_refs_or_pointers = true; + return ControlFlow::Break(()); + } + ty::RawPtr(..) => { + self.has_refs_or_pointers = true; + return ControlFlow::Break(()); + } + ty::Param(_) => { + self.has_params = true; + } + _ => {} + } + + ty.super_visit_with(self) + } +} + +/// Used to build a dependency graph between borrows/pointers and the `Local`s that +/// they reference. +/// We add edges to the graph in two kinds of situations: +/// * direct assignment of reference or raw pointer (e.g. `_4 = Ref(..)` or `_4 = AddressOf`) +/// * assignments to non-reference or non-pointer `Local`s, which themselves might contain +/// references or pointers (e.g. `_2 = Aggregate(Adt(..), _, _, _, _), [move _3, move _6])`, +/// where `_3` and `_6` are places corresponding to references or raw pointers). +struct BorrowDependencies<'a, 'tcx> { + tcx: TyCtxt<'tcx>, + local_decls: &'a LocalDecls<'tcx>, + + // Maps `Local`s, for which we have nodes in the graph, to the `NodeIndex`es of those nodes. + locals_to_node_indexes: FxHashMap, + + // Tracks the dependencies of places and the references/pointers they may contain, + // e.g. if we have `_3 = Ref(_, _, _2)` we add an edge from _3 to _2. We later use + // this graph to allow us to infer which locals need to be kept live in the + // liveness analysis. + dep_graph: Graph, + + // Contains the `NodeIndex` corresponding to the local to which we're currently + // assigning. + current_local: Option, +} + +impl<'a, 'tcx> BorrowDependencies<'a, 'tcx> { + #[instrument(skip(local_decls, tcx), level = "debug")] + fn new(local_decls: &'a LocalDecls<'tcx>, tcx: TyCtxt<'tcx>) -> Self { + let num_nodes = local_decls.len(); + let approx_num_edges = 3 * num_nodes; + + BorrowDependencies { + tcx, + local_decls, + dep_graph: Graph::with_capacity(num_nodes, approx_num_edges), + current_local: None, + locals_to_node_indexes: Default::default(), + } + } +} + +impl<'a, 'tcx> Visitor<'tcx> for BorrowDependencies<'a, 'tcx> { + #[instrument(skip(self, body), level = "debug")] + fn visit_body(&mut self, body: &Body<'tcx>) { + // Add nodes for the arguments + for i in 1..=body.arg_count { + let local = Local::from_usize(i); + let node_kind = NodeKind::LocalOrLocalWithRefs(local); + let node_idx = self.dep_graph.add_node(node_kind); + self.locals_to_node_indexes.insert(local, node_idx); + } + + self.super_body(body) + } + + #[instrument(skip(self), level = "debug")] + fn visit_statement(&mut self, statement: &Statement<'tcx>, location: Location) { + match &statement.kind { + StatementKind::Assign(assign) => { + let assign_place = assign.0; + let assign_local = assign_place.local; + + let node_kind = match assign_place.ty(self.local_decls, self.tcx).ty.kind() { + ty::Ref(..) | ty::RawPtr(..) => NodeKind::Borrow(assign_local), + _ => NodeKind::LocalOrLocalWithRefs(assign_local), + }; + + let node_idx = self.dep_graph.add_node(node_kind); + self.locals_to_node_indexes.insert(assign_local, node_idx); + self.current_local = Some(node_idx); + + debug!( + "set current_local to {:?} for local {:?}", + self.current_local, assign_local + ); + + // Do not call `visit_place` here as this might introduce a self-edge, which our liveness analysis + // assumes not to exist. + self.visit_rvalue(&assign.1, location) + } + StatementKind::FakeRead(..) + | StatementKind::StorageDead(_) + | StatementKind::StorageLive(_) => {} + _ => { + self.super_statement(statement, location); + } + } + + self.current_local = None; + } + + #[instrument(skip(self), level = "debug")] + fn visit_rvalue(&mut self, rvalue: &Rvalue<'tcx>, location: Location) { + debug!("self.current_local: {:?}", self.current_local); + match rvalue { + Rvalue::Ref(_, _, borrowed_place) | Rvalue::AddressOf(_, borrowed_place) => { + let Some(src_node_idx) = self.current_local else { + bug!("Expected self.current_local to be set with Rvalue::Ref|Rvalue::AddressOf"); + }; + + let borrowed_local = borrowed_place.local; + let node_idx = + if let Some(node_idx) = self.locals_to_node_indexes.get(&borrowed_local) { + *node_idx + } else { + self.dep_graph.add_node(NodeKind::Borrow(borrowed_place.local)) + }; + + debug!( + "adding edge from {:?}({:?}) -> {:?}({:?})", + src_node_idx, + self.dep_graph.node(src_node_idx).data, + node_idx, + self.dep_graph.node(node_idx).data, + ); + + self.dep_graph.add_edge(src_node_idx, node_idx, ()); + } + _ => self.super_rvalue(rvalue, location), + } + } + + #[instrument(skip(self), level = "debug")] + fn visit_place(&mut self, place: &Place<'tcx>, context: PlaceContext, location: Location) { + // Add edges for places that correspond to references or raw pointers + let place_ty = place.ty(self.local_decls, self.tcx).ty; + debug!(?place_ty); + debug!("current local: {:?}", self.current_local); + match place_ty.kind() { + ty::Ref(..) | ty::RawPtr(..) => match self.current_local { + Some(src_node_idx) => { + let borrowed_local = place.local; + let node_idx = + if let Some(node_idx) = self.locals_to_node_indexes.get(&borrowed_local) { + *node_idx + } else { + self.dep_graph.add_node(NodeKind::Borrow(borrowed_local)) + }; + + debug!( + "adding edge from {:?}({:?}) -> {:?}({:?})", + src_node_idx, + self.dep_graph.node(src_node_idx).data, + node_idx, + borrowed_local + ); + + self.dep_graph.add_edge(src_node_idx, node_idx, ()); + } + None => {} + }, + _ => {} + } + + self.super_place(place, context, location) + } + + #[instrument(skip(self), level = "debug")] + fn visit_operand(&mut self, operand: &Operand<'tcx>, location: Location) { + self.super_operand(operand, location) + } + + #[instrument(skip(self), level = "debug")] + fn visit_terminator(&mut self, terminator: &Terminator<'tcx>, location: Location) { + match &terminator.kind { + TerminatorKind::Call { destination, func, args, .. } => { + let dest_ty = destination.ty(self.local_decls, self.tcx).ty; + debug!(?dest_ty); + + let mut has_refs_or_pointers_visitor = MaybeHasRefsOrPointersVisitor::new(); + dest_ty.visit_with(&mut has_refs_or_pointers_visitor); + + let dest_might_include_refs_or_pointers = + if has_refs_or_pointers_visitor.has_refs_or_pointers { + true + } else if has_refs_or_pointers_visitor.has_params { + // if we pass in any references or raw pointers as arguments and the + // return type includes type parameters, these type parameters could + // refer to those refs/ptrs, so we have to be conservative here and possibly + // include an edge. + true + } else { + false + }; + + let node_idx = + self.dep_graph.add_node(NodeKind::LocalOrLocalWithRefs(destination.local)); + debug!("added local {:?} to graph with idx {:?}", destination.local, node_idx); + self.locals_to_node_indexes.insert(destination.local, node_idx); + + if dest_might_include_refs_or_pointers { + self.current_local = Some(node_idx); + debug!("self.current_local: {:?}", self.current_local); + } + + self.visit_operand(func, location); + for arg in args { + self.visit_operand(arg, location); + } + + self.current_local = None; + } + TerminatorKind::Yield { resume_arg, value, .. } => { + let node_idx = + self.dep_graph.add_node(NodeKind::LocalOrLocalWithRefs(resume_arg.local)); + debug!("added local {:?} to graph with idx {:?}", resume_arg.local, node_idx); + self.locals_to_node_indexes.insert(resume_arg.local, node_idx); + + self.super_operand(value, location); + } + _ => self.super_terminator(terminator, location), + } + } +} + +pub struct BorrowedLocalsResults<'mir, 'tcx> { + borrows_analysis_results: Results<'tcx, LiveBorrows<'mir, 'tcx>>, + borrowed_local_to_locals_to_keep_alive: FxHashMap>, +} + +impl<'mir, 'tcx> BorrowedLocalsResults<'mir, 'tcx> +where + 'tcx: 'mir, +{ + fn new( + tcx: TyCtxt<'tcx>, + body: &'mir Body<'tcx>, + borrows_analysis_results: Results<'tcx, LiveBorrows<'mir, 'tcx>>, + ) -> Self { + let mut borrow_deps = BorrowDependencies::new(body.local_decls(), tcx); + borrow_deps.visit_body(body); + + if cfg!(debug_assertions) { + let dep_graph = &borrow_deps.dep_graph; + + debug!( + "nodes: {:#?}", + dep_graph + .all_nodes() + .clone() + .into_iter() + .enumerate() + .map(|(i, node)| (i, node.data)) + .collect::>() + ); + + debug!("edges:"); + for edge in dep_graph.all_edges() { + let src_node_idx = edge.source(); + let src_node = dep_graph.node(src_node_idx); + let target_node_idx = edge.target(); + let target_node = dep_graph.node(target_node_idx); + debug!( + "{:?}({:?}) -> {:?}({:?}) ({:?})", + src_node_idx, src_node.data, target_node_idx, target_node.data, edge.data + ) + } + } + + let dep_graph = &borrow_deps.dep_graph; + let borrowed_local_to_locals_to_keep_alive = Self::get_locals_to_keep_alive_map(dep_graph); + Self { borrows_analysis_results, borrowed_local_to_locals_to_keep_alive } + } + + /// Uses the dependency graph to find all locals that we need to keep live for a given + /// `Node` (or more specically the `Local` corresponding to that `Node`). + fn get_locals_to_keep_alive_map<'a>( + dep_graph: &'a Graph, + ) -> FxHashMap> { + let mut borrows_to_locals: FxHashMap> = Default::default(); + for (node_idx, node) in dep_graph.enumerated_nodes() { + let current_local = node.data.get_local(); + if borrows_to_locals.get(¤t_local).is_none() { + Self::dfs_for_node(node_idx, &mut borrows_to_locals, dep_graph); + } + } + + debug!("borrows_to_locals: {:#?}", borrows_to_locals); + borrows_to_locals + } + + fn dfs_for_node( + node_idx: NodeIndex, + borrows_to_locals: &mut FxHashMap>, + dep_graph: &Graph, + ) -> Vec { + let src_node = dep_graph.node(node_idx); + let current_local = src_node.data.get_local(); + if let Some(locals_to_keep_alive) = borrows_to_locals.get(¤t_local) { + // already traversed this node + return (*locals_to_keep_alive).clone(); + } + + let mut num_succs = 0; + let mut locals_for_node = vec![]; + for (_, edge) in dep_graph.outgoing_edges(node_idx) { + num_succs += 1; + let target_node_idx = edge.target(); + let target_node = dep_graph.node(target_node_idx); + + debug!( + "edge {:?} ({:?}) -> {:?} ({:?})", + node_idx, src_node.data, target_node_idx, target_node.data, + ); + + let mut locals_to_keep_alive_for_succ = + Self::dfs_for_node(target_node_idx, borrows_to_locals, dep_graph); + locals_for_node.append(&mut locals_to_keep_alive_for_succ); + } + + if num_succs == 0 { + // base node to keep alive + vec![src_node.data.get_local()] + } else { + if matches!(src_node.data, NodeKind::LocalOrLocalWithRefs(_)) { + // These are locals that we need to keep alive, but that also contain + // successors in the graph since they contain other references/pointers. + locals_for_node.push(current_local); + } + + borrows_to_locals.insert(current_local, locals_for_node.clone()); + locals_for_node + } + } +} + +#[instrument(skip(tcx), level = "debug")] +pub fn get_borrowed_locals_results<'mir, 'tcx>( + body: &'mir Body<'tcx>, + tcx: TyCtxt<'tcx>, +) -> BorrowedLocalsResults<'mir, 'tcx> { + debug!("body: {:#?}", body); + let live_borrows = LiveBorrows::new(body, tcx); + let results = + live_borrows.into_engine(tcx, body).pass_name("borrowed_locals").iterate_to_fixpoint(); + + BorrowedLocalsResults::new(tcx, body, results) +} + +pub struct BorrowedLocalsResultsCursor<'a, 'mir, 'tcx> { + body: &'mir Body<'tcx>, + borrows_analysis_cursor: ResultsRefCursor<'a, 'mir, 'tcx, LiveBorrows<'mir, 'tcx>>, + borrowed_local_to_locals_to_keep_alive: &'a FxHashMap>, +} + +impl<'a, 'mir, 'tcx> BorrowedLocalsResultsCursor<'a, 'mir, 'tcx> { + pub fn new(body: &'mir Body<'tcx>, results: &'a BorrowedLocalsResults<'mir, 'tcx>) -> Self { + let mut cursor = ResultsCursor::new(body, &results.borrows_analysis_results); + + // We don't care about the order of the blocks, only about the result at a given location. + // This statement is necessary since we're performing a backward analysis in `LiveBorrows`, + // but want `Self::get` to be usable in forward analyses as well. + cursor.allow_unreachable(); + + Self { + body, + borrows_analysis_cursor: cursor, + borrowed_local_to_locals_to_keep_alive: &results.borrowed_local_to_locals_to_keep_alive, + } + } + + /// Returns all `Local`s that need to be live at the given `Location` because of + /// outstanding references or raw pointers. + pub fn get(&mut self, loc: Location) -> BitSet { + self.borrows_analysis_cursor.seek_before_primary_effect(loc); + let live_borrows_at_loc = self.borrows_analysis_cursor.get(); + debug!(?live_borrows_at_loc); + + let mut borrowed_locals = BitSet::new_empty(live_borrows_at_loc.domain_size()); + for borrowed_local in live_borrows_at_loc.iter() { + debug!(?borrowed_local); + if let Some(locals_to_keep_alive) = + self.borrowed_local_to_locals_to_keep_alive.get(&borrowed_local) + { + debug!(?locals_to_keep_alive); + for local in locals_to_keep_alive.iter() { + borrowed_locals.insert(*local); + } + } + } + + match self.body.stmt_at(loc) { + Either::Right(terminator) => { + match terminator.kind { + TerminatorKind::Drop { place: dropped_place, .. } => { + // Drop terminators may call custom drop glue (`Drop::drop`), which takes `&mut + // self` as a parameter. In the general case, a drop impl could launder that + // reference into the surrounding environment through a raw pointer, thus creating + // a valid `*mut` pointing to the dropped local. We are not yet willing to declare + // this particular case UB, so we must treat all dropped locals as mutably borrowed + // for now. See discussion on [#61069]. + // + // [#61069]: https://github.com/rust-lang/rust/pull/61069 + if !dropped_place.is_indirect() { + borrowed_locals.insert(dropped_place.local); + } + } + _ => {} + } + } + Either::Left(_) => {} + } + + debug!(?borrowed_locals); + borrowed_locals + } +} + +/// Performs a liveness analysis for borrows and raw pointers. +pub struct LiveBorrows<'a, 'tcx> { + body: &'a Body<'tcx>, + tcx: TyCtxt<'tcx>, +} + +impl<'a, 'tcx> LiveBorrows<'a, 'tcx> { + fn new(body: &'a Body<'tcx>, tcx: TyCtxt<'tcx>) -> Self { + LiveBorrows { body, tcx } + } + + fn transfer_function<'b, T>(&self, trans: &'b mut T) -> TransferFunction<'a, 'b, 'tcx, T> { + TransferFunction { body: self.body, tcx: self.tcx, _trans: trans } + } +} + +impl<'a, 'tcx> AnalysisDomain<'tcx> for LiveBorrows<'a, 'tcx> { + type Domain = BitSet; + type Direction = Backward; + const NAME: &'static str = "live_borrows"; + + fn bottom_value(&self, body: &mir::Body<'tcx>) -> Self::Domain { + // bottom = unborrowed + BitSet::new_empty(body.local_decls().len()) + } + + fn initialize_start_block(&self, _: &mir::Body<'tcx>, _: &mut Self::Domain) { + // Not supported for backward analyses + } +} + +impl<'a, 'tcx> GenKillAnalysis<'tcx> for LiveBorrows<'a, 'tcx> { + type Idx = Local; + + #[instrument(skip(self, trans), level = "debug")] + fn statement_effect( + &self, + trans: &mut impl GenKill, + statement: &mir::Statement<'tcx>, + location: Location, + ) { + self.transfer_function(trans).visit_statement(statement, location); + } + + #[instrument(skip(self, trans), level = "debug")] + fn terminator_effect( + &self, + trans: &mut impl GenKill, + terminator: &mir::Terminator<'tcx>, + location: Location, + ) { + self.transfer_function(trans).visit_terminator(terminator, location); + } + + fn call_return_effect( + &self, + _trans: &mut impl GenKill, + _block: mir::BasicBlock, + _return_places: CallReturnPlaces<'_, 'tcx>, + ) { + } +} + +/// A `Visitor` that defines the transfer function for `MaybeBorrowedLocals`. +struct TransferFunction<'a, 'b, 'tcx, T> { + body: &'a Body<'tcx>, + tcx: TyCtxt<'tcx>, + _trans: &'b mut T, +} + +impl<'a, 'tcx, T> Visitor<'tcx> for TransferFunction<'a, '_, 'tcx, T> +where + T: GenKill, +{ + #[instrument(skip(self), level = "debug")] + fn visit_statement(&mut self, stmt: &Statement<'tcx>, location: Location) { + match &stmt.kind { + StatementKind::Assign(assign) => { + let lhs_place = assign.0; + let lhs_place_ty = lhs_place.ty(self.body.local_decls(), self.tcx).ty; + debug!(?lhs_place, ?lhs_place_ty); + + match lhs_place_ty.kind() { + ty::Ref(..) | ty::RawPtr(..) => { + debug!("killing {:?}", lhs_place.local); + self._trans.kill(lhs_place.local); + + self.visit_rvalue(&assign.1, location); + } + _ => { + self.super_assign(&assign.0, &assign.1, location); + } + } + } + StatementKind::FakeRead(..) + | StatementKind::StorageDead(_) + | StatementKind::StorageLive(_) => {} + _ => self.super_statement(stmt, location), + } + } + + #[instrument(skip(self), level = "debug")] + fn visit_place( + &mut self, + place: &mir::Place<'tcx>, + context: PlaceContext, + location: mir::Location, + ) { + let place_ty = place.ty(self.body.local_decls(), self.tcx); + debug!(?place_ty); + + match place_ty.ty.kind() { + ty::Ref(..) | ty::RawPtr(..) => { + debug!("gen {:?}", place.local); + self._trans.gen(place.local); + } + _ => {} + } + + self.super_place(place, context, location); + } + + #[instrument(skip(self), level = "debug")] + fn visit_operand(&mut self, operand: &mir::Operand<'tcx>, location: mir::Location) { + self.super_operand(operand, location); + } + + #[instrument(skip(self), level = "debug")] + fn visit_rvalue(&mut self, rvalue: &mir::Rvalue<'tcx>, location: Location) { + self.super_rvalue(rvalue, location); + } + + #[instrument(skip(self), level = "debug")] + fn visit_terminator(&mut self, terminator: &mir::Terminator<'tcx>, location: Location) { + match terminator.kind { + TerminatorKind::Call { destination, .. } => { + debug!("killing {:?}", destination.local); + self._trans.kill(destination.local); + } + TerminatorKind::Yield { resume_arg, .. } => { + debug!("killing {:?}", resume_arg.local); + self._trans.kill(resume_arg.local); + } + _ => {} + } + + self.super_terminator(terminator, location); + } +} diff --git a/compiler/rustc_mir_dataflow/src/impls/mod.rs b/compiler/rustc_mir_dataflow/src/impls/mod.rs index 98cec1c676022..7a88eca5a2564 100644 --- a/compiler/rustc_mir_dataflow/src/impls/mod.rs +++ b/compiler/rustc_mir_dataflow/src/impls/mod.rs @@ -19,11 +19,15 @@ use crate::{drop_flag_effects, on_all_children_bits}; use crate::{lattice, AnalysisDomain, GenKill, GenKillAnalysis}; mod borrowed_locals; +mod live_borrows; mod liveness; mod storage_liveness; pub use self::borrowed_locals::borrowed_locals; pub use self::borrowed_locals::MaybeBorrowedLocals; +pub use self::live_borrows::{ + get_borrowed_locals_results, BorrowedLocalsResults, BorrowedLocalsResultsCursor, +}; pub use self::liveness::MaybeLiveLocals; pub use self::liveness::MaybeTransitiveLiveLocals; pub use self::storage_liveness::{MaybeRequiresStorage, MaybeStorageDead, MaybeStorageLive}; diff --git a/compiler/rustc_mir_transform/src/generator.rs b/compiler/rustc_mir_transform/src/generator.rs index 029fb2e9ba04c..3f86693357621 100644 --- a/compiler/rustc_mir_transform/src/generator.rs +++ b/compiler/rustc_mir_transform/src/generator.rs @@ -67,7 +67,8 @@ use rustc_middle::mir::*; use rustc_middle::ty::{self, AdtDef, Ty, TyCtxt}; use rustc_middle::ty::{GeneratorSubsts, SubstsRef}; use rustc_mir_dataflow::impls::{ - MaybeBorrowedLocals, MaybeLiveLocals, MaybeRequiresStorage, MaybeStorageLive, + get_borrowed_locals_results, MaybeBorrowedLocals, MaybeLiveLocals, MaybeRequiresStorage, + MaybeStorageLive, }; use rustc_mir_dataflow::storage::always_storage_live_locals; use rustc_mir_dataflow::{self, Analysis}; @@ -599,6 +600,8 @@ fn locals_live_across_suspend_points<'tcx>( let mut borrowed_locals_cursor = borrowed_locals_results.cloned_results_cursor(body_ref); + let mut live_borrows_cursor = get_borrowed_locals_results(body, tcx); + // Calculate the MIR locals that we actually need to keep storage around // for. let mut requires_storage_results = @@ -620,8 +623,20 @@ fn locals_live_across_suspend_points<'tcx>( let mut live_locals_at_any_suspension_point = BitSet::new_empty(body.local_decls.len()); for (block, data) in body.basic_blocks.iter_enumerated() { + for (i, stmt) in data.statements.iter().enumerate() { + debug!(?stmt); + let loc = Location { block, statement_index: i }; + debug!("live_borrows_cursor seek before"); + live_borrows_cursor.seek_before_primary_effect(loc); + debug!("finished seek before"); + let live_borrowed_locals = live_borrows_cursor.get(); + debug!(?live_borrowed_locals); + } + + debug!(?block, ?data.terminator); if let TerminatorKind::Yield { .. } = data.terminator().kind { let loc = Location { block, statement_index: data.statements.len() }; + debug!("encountered Yield at loc {:?}", loc); liveness.seek_to_block_end(block); let mut live_locals: BitSet<_> = BitSet::new_empty(body.local_decls.len()); @@ -639,7 +654,27 @@ fn locals_live_across_suspend_points<'tcx>( // forever. Note that the final liveness is still bounded by the storage liveness // of the local, which happens using the `intersect` operation below. borrowed_locals_cursor.seek_before_primary_effect(loc); - live_locals.union(borrowed_locals_cursor.get()); + let current_borrowed_locals = borrowed_locals_cursor.get(); + + debug!("live_borrows_cursor seek before"); + live_borrows_cursor.seek_before_primary_effect(loc); + debug!("finished seek before"); + let live_borrowed_locals = live_borrows_cursor.get(); + + let mut live_locals_stmt: BitSet<_> = BitSet::new_empty(body.local_decls.len()); + liveness.seek_before_primary_effect(loc); + live_locals_stmt.union(liveness.get()); + + let mut storage_req: BitSet<_> = BitSet::new_empty(body.local_decls.len()); + requires_storage_cursor.seek_before_primary_effect(loc); + storage_req.union(requires_storage_cursor.get()); + + debug!(?current_borrowed_locals); + debug!(?live_borrowed_locals); + debug!(?live_locals_stmt); + debug!(?storage_req); + + live_locals.union(current_borrowed_locals); } // Store the storage liveness for later use so we can restore the state From de5ce09287e22b7632920be58bffc7c629a4b128 Mon Sep 17 00:00:00 2001 From: b-naber Date: Sun, 28 May 2023 20:54:13 +0000 Subject: [PATCH 02/20] use new borrowed locals analysis in MaybeRequiresStorage --- .../src/impls/storage_liveness.rs | 65 +++++++++++-------- 1 file changed, 39 insertions(+), 26 deletions(-) diff --git a/compiler/rustc_mir_dataflow/src/impls/storage_liveness.rs b/compiler/rustc_mir_dataflow/src/impls/storage_liveness.rs index 666c8d50a8a8c..112d289a13cbd 100644 --- a/compiler/rustc_mir_dataflow/src/impls/storage_liveness.rs +++ b/compiler/rustc_mir_dataflow/src/impls/storage_liveness.rs @@ -1,6 +1,7 @@ pub use super::*; -use crate::{CallReturnPlaces, GenKill, ResultsClonedCursor}; +use crate::impls::{BorrowedLocalsResults, BorrowedLocalsResultsCursor}; +use crate::{CallReturnPlaces, GenKill}; use rustc_middle::mir::visit::{NonMutatingUseContext, PlaceContext, Visitor}; use rustc_middle::mir::*; use std::borrow::Cow; @@ -146,28 +147,26 @@ impl<'tcx> crate::GenKillAnalysis<'tcx> for MaybeStorageDead { } } -type BorrowedLocalsResults<'res, 'mir, 'tcx> = - ResultsClonedCursor<'res, 'mir, 'tcx, MaybeBorrowedLocals>; - /// Dataflow analysis that determines whether each local requires storage at a /// given location; i.e. whether its storage can go away without being observed. -pub struct MaybeRequiresStorage<'res, 'mir, 'tcx> { - borrowed_locals: BorrowedLocalsResults<'res, 'mir, 'tcx>, -} - -impl<'res, 'mir, 'tcx> MaybeRequiresStorage<'res, 'mir, 'tcx> { - pub fn new(borrowed_locals: BorrowedLocalsResults<'res, 'mir, 'tcx>) -> Self { - MaybeRequiresStorage { borrowed_locals } - } +pub struct MaybeRequiresStorage<'a, 'mir, 'tcx> { + body: &'mir Body<'tcx>, + borrowed_locals: RefCell>, } -impl crate::CloneAnalysis for MaybeRequiresStorage<'_, '_, '_> { - fn clone_analysis(&self) -> Self { - Self { borrowed_locals: self.borrowed_locals.new_cursor() } +impl<'a, 'mir, 'tcx> MaybeRequiresStorage<'a, 'mir, 'tcx> { + pub fn new( + body: &'mir Body<'tcx>, + borrowed_locals: &'a BorrowedLocalsResults<'mir, 'tcx>, + ) -> Self { + MaybeRequiresStorage { + body, + borrowed_locals: RefCell::new(BorrowedLocalsResultsCursor::new(body, borrowed_locals)), + } } } -impl<'tcx> crate::AnalysisDomain<'tcx> for MaybeRequiresStorage<'_, '_, 'tcx> { +impl<'a, 'mir, 'tcx> crate::AnalysisDomain<'tcx> for MaybeRequiresStorage<'a, 'mir, 'tcx> { type Domain = BitSet; const NAME: &'static str = "requires_storage"; @@ -186,7 +185,7 @@ impl<'tcx> crate::AnalysisDomain<'tcx> for MaybeRequiresStorage<'_, '_, 'tcx> { } } -impl<'tcx> crate::GenKillAnalysis<'tcx> for MaybeRequiresStorage<'_, '_, 'tcx> { +impl<'a, 'mir, 'tcx> crate::GenKillAnalysis<'tcx> for MaybeRequiresStorage<'a, 'mir, 'tcx> { type Idx = Local; fn before_statement_effect( @@ -196,7 +195,13 @@ impl<'tcx> crate::GenKillAnalysis<'tcx> for MaybeRequiresStorage<'_, '_, 'tcx> { loc: Location, ) { // If a place is borrowed in a statement, it needs storage for that statement. - self.borrowed_locals.mut_analysis().statement_effect(trans, stmt, loc); + let borrowed_locals_at_loc = self.borrowed_locals.borrow_mut().get(loc); + for i in 0..self.body.local_decls().len() { + let local = Local::from_usize(i); + if borrowed_locals_at_loc.contains(local) { + trans.gen(local); + } + } match &stmt.kind { StatementKind::StorageDead(l) => trans.kill(*l), @@ -239,8 +244,14 @@ impl<'tcx> crate::GenKillAnalysis<'tcx> for MaybeRequiresStorage<'_, '_, 'tcx> { terminator: &mir::Terminator<'tcx>, loc: Location, ) { - // If a place is borrowed in a terminator, it needs storage for that terminator. - self.borrowed_locals.mut_analysis().terminator_effect(trans, terminator, loc); + // If a place is borrowed in a statement, it needs storage for that statement. + let borrowed_locals_at_loc = self.borrowed_locals.borrow_mut().get(loc); + for i in 0..self.body.local_decls().len() { + let local = Local::from_usize(i); + if borrowed_locals_at_loc.contains(local) { + trans.gen(local); + } + } match &terminator.kind { TerminatorKind::Call { destination, .. } => { @@ -344,7 +355,7 @@ impl<'tcx> crate::GenKillAnalysis<'tcx> for MaybeRequiresStorage<'_, '_, 'tcx> { } } -impl<'tcx> MaybeRequiresStorage<'_, '_, 'tcx> { +impl<'a, 'mir, 'tcx> MaybeRequiresStorage<'a, 'mir, 'tcx> { /// Kill locals that are fully moved and have not been borrowed. fn check_for_move(&mut self, trans: &mut impl GenKill, loc: Location) { let body = self.borrowed_locals.body(); @@ -353,19 +364,21 @@ impl<'tcx> MaybeRequiresStorage<'_, '_, 'tcx> { } } -struct MoveVisitor<'a, 'res, 'mir, 'tcx, T> { - borrowed_locals: &'a mut BorrowedLocalsResults<'res, 'mir, 'tcx>, +struct MoveVisitor<'a, 'b, 'mir, 'tcx, T> { + borrowed_locals: &'a RefCell>, trans: &'a mut T, } -impl<'tcx, T> Visitor<'tcx> for MoveVisitor<'_, '_, '_, 'tcx, T> +impl<'a, 'b, 'mir, 'tcx, T> Visitor<'tcx> for MoveVisitor<'a, 'b, 'mir, 'tcx, T> where T: GenKill, { + #[instrument(skip(self), level = "debug")] fn visit_local(&mut self, local: Local, context: PlaceContext, loc: Location) { if PlaceContext::NonMutatingUse(NonMutatingUseContext::Move) == context { - self.borrowed_locals.seek_before_primary_effect(loc); - if !self.borrowed_locals.contains(local) { + let borrowed_locals = self.borrowed_locals.borrow_mut().get(loc); + debug!(?borrowed_locals); + if !borrowed_locals.contains(local) { self.trans.kill(local); } } From 0fb77941b3010a10875cde7a57c1cdd07e439c4b Mon Sep 17 00:00:00 2001 From: b-naber Date: Sun, 28 May 2023 21:02:56 +0000 Subject: [PATCH 03/20] use new analysis in generator transform --- compiler/rustc_mir_transform/src/generator.rs | 56 ++++++------------- 1 file changed, 17 insertions(+), 39 deletions(-) diff --git a/compiler/rustc_mir_transform/src/generator.rs b/compiler/rustc_mir_transform/src/generator.rs index 3f86693357621..7e39acca52ce0 100644 --- a/compiler/rustc_mir_transform/src/generator.rs +++ b/compiler/rustc_mir_transform/src/generator.rs @@ -67,8 +67,8 @@ use rustc_middle::mir::*; use rustc_middle::ty::{self, AdtDef, Ty, TyCtxt}; use rustc_middle::ty::{GeneratorSubsts, SubstsRef}; use rustc_mir_dataflow::impls::{ - get_borrowed_locals_results, MaybeBorrowedLocals, MaybeLiveLocals, MaybeRequiresStorage, - MaybeStorageLive, + get_borrowed_locals_results, BorrowedLocalsResultsCursor, MaybeLiveLocals, + MaybeRequiresStorage, MaybeStorageLive, }; use rustc_mir_dataflow::storage::always_storage_live_locals; use rustc_mir_dataflow::{self, Analysis}; @@ -593,22 +593,18 @@ fn locals_live_across_suspend_points<'tcx>( .iterate_to_fixpoint() .into_results_cursor(body_ref); - // Calculate the MIR locals which have been previously - // borrowed (even if they are still active). - let borrowed_locals_results = - MaybeBorrowedLocals.into_engine(tcx, body_ref).pass_name("generator").iterate_to_fixpoint(); - - let mut borrowed_locals_cursor = borrowed_locals_results.cloned_results_cursor(body_ref); - - let mut live_borrows_cursor = get_borrowed_locals_results(body, tcx); + // Calculate the locals that are live due to outstanding references or pointers. + let live_borrows_results = get_borrowed_locals_results(body_ref, tcx); + let mut live_borrows_cursor = BorrowedLocalsResultsCursor::new(body_ref, &live_borrows_results); // Calculate the MIR locals that we actually need to keep storage around // for. - let mut requires_storage_results = - MaybeRequiresStorage::new(borrowed_locals_results.cloned_results_cursor(body)) - .into_engine(tcx, body_ref) - .iterate_to_fixpoint(); - let mut requires_storage_cursor = requires_storage_results.as_results_cursor(body_ref); + let requires_storage_results = MaybeRequiresStorage::new(body, &live_borrows_results) + .into_engine(tcx, body_ref) + .iterate_to_fixpoint(); + + let mut requires_storage_cursor = + rustc_mir_dataflow::ResultsCursor::new(body_ref, &requires_storage_results); // Calculate the liveness of MIR locals ignoring borrows. let mut liveness = MaybeLiveLocals @@ -623,21 +619,9 @@ fn locals_live_across_suspend_points<'tcx>( let mut live_locals_at_any_suspension_point = BitSet::new_empty(body.local_decls.len()); for (block, data) in body.basic_blocks.iter_enumerated() { - for (i, stmt) in data.statements.iter().enumerate() { - debug!(?stmt); - let loc = Location { block, statement_index: i }; - debug!("live_borrows_cursor seek before"); - live_borrows_cursor.seek_before_primary_effect(loc); - debug!("finished seek before"); - let live_borrowed_locals = live_borrows_cursor.get(); - debug!(?live_borrowed_locals); - } - debug!(?block, ?data.terminator); if let TerminatorKind::Yield { .. } = data.terminator().kind { let loc = Location { block, statement_index: data.statements.len() }; - debug!("encountered Yield at loc {:?}", loc); - liveness.seek_to_block_end(block); let mut live_locals: BitSet<_> = BitSet::new_empty(body.local_decls.len()); live_locals.union(liveness.get()); @@ -653,13 +637,7 @@ fn locals_live_across_suspend_points<'tcx>( // If a borrow is converted to a raw reference, we must also assume that it lives // forever. Note that the final liveness is still bounded by the storage liveness // of the local, which happens using the `intersect` operation below. - borrowed_locals_cursor.seek_before_primary_effect(loc); - let current_borrowed_locals = borrowed_locals_cursor.get(); - - debug!("live_borrows_cursor seek before"); - live_borrows_cursor.seek_before_primary_effect(loc); - debug!("finished seek before"); - let live_borrowed_locals = live_borrows_cursor.get(); + let live_borrowed_locals = live_borrows_cursor.get(loc); let mut live_locals_stmt: BitSet<_> = BitSet::new_empty(body.local_decls.len()); liveness.seek_before_primary_effect(loc); @@ -669,12 +647,11 @@ fn locals_live_across_suspend_points<'tcx>( requires_storage_cursor.seek_before_primary_effect(loc); storage_req.union(requires_storage_cursor.get()); - debug!(?current_borrowed_locals); debug!(?live_borrowed_locals); debug!(?live_locals_stmt); debug!(?storage_req); - live_locals.union(current_borrowed_locals); + live_locals.union(&live_borrowed_locals); } // Store the storage liveness for later use so we can restore the state @@ -686,7 +663,8 @@ fn locals_live_across_suspend_points<'tcx>( // suspension points (the `liveness` variable) // and their storage is required (the `storage_required` variable) requires_storage_cursor.seek_before_primary_effect(loc); - live_locals.intersect(requires_storage_cursor.get()); + let storage_required = requires_storage_cursor.get(); + live_locals.intersect(storage_required); // The generator argument is ignored. live_locals.remove(SELF_ARG); @@ -777,11 +755,11 @@ impl ops::Deref for GeneratorSavedLocals { /// time. Generates a bitset for every local of all the other locals that may be /// StorageLive simultaneously with that local. This is used in the layout /// computation; see `GeneratorLayout` for more. -fn compute_storage_conflicts<'mir, 'tcx>( +fn compute_storage_conflicts<'a, 'mir, 'tcx>( body: &'mir Body<'tcx>, saved_locals: &GeneratorSavedLocals, always_live_locals: BitSet, - mut requires_storage: rustc_mir_dataflow::Results<'tcx, MaybeRequiresStorage<'_, 'mir, 'tcx>>, + requires_storage: rustc_mir_dataflow::Results<'tcx, MaybeRequiresStorage<'a, 'mir, 'tcx>>, ) -> BitMatrix { assert_eq!(body.local_decls.len(), saved_locals.domain_size()); From 2bc0875ce4435b6124a5a039051ac529ce05ee41 Mon Sep 17 00:00:00 2001 From: b-naber Date: Sun, 28 May 2023 20:02:37 +0000 Subject: [PATCH 04/20] bless tests --- .../async-fn-nonsend.drop_tracking_mir.stderr | 23 +------------------ .../async-fn-nonsend.no_drop_tracking.stderr | 4 ++-- tests/ui/async-await/async-fn-nonsend.rs | 3 ++- ...ld-assign-nonsend.drop_tracking_mir.stderr | 11 ++++----- ...ld-assign-nonsend.drop_tracking_mir.stderr | 11 ++++----- 5 files changed, 13 insertions(+), 39 deletions(-) diff --git a/tests/ui/async-await/async-fn-nonsend.drop_tracking_mir.stderr b/tests/ui/async-await/async-fn-nonsend.drop_tracking_mir.stderr index 219945e0971b9..05d02304d44f8 100644 --- a/tests/ui/async-await/async-fn-nonsend.drop_tracking_mir.stderr +++ b/tests/ui/async-await/async-fn-nonsend.drop_tracking_mir.stderr @@ -18,26 +18,5 @@ note: required by a bound in `assert_send` LL | fn assert_send(_: impl Send) {} | ^^^^ required by this bound in `assert_send` -error: future cannot be sent between threads safely - --> $DIR/async-fn-nonsend.rs:74:17 - | -LL | assert_send(non_sync_with_method_call()); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ future returned by `non_sync_with_method_call` is not `Send` - | - = help: within `impl Future`, the trait `Send` is not implemented for `dyn std::fmt::Write` -note: future is not `Send` as this value is used across an await - --> $DIR/async-fn-nonsend.rs:49:15 - | -LL | let f: &mut std::fmt::Formatter = &mut get_formatter(); - | --------------- has type `Formatter<'_>` which is not `Send` -... -LL | fut().await; - | ^^^^^ await occurs here, with `get_formatter()` maybe used later -note: required by a bound in `assert_send` - --> $DIR/async-fn-nonsend.rs:67:24 - | -LL | fn assert_send(_: impl Send) {} - | ^^^^ required by this bound in `assert_send` - -error: aborting due to 2 previous errors +error: aborting due to previous error diff --git a/tests/ui/async-await/async-fn-nonsend.no_drop_tracking.stderr b/tests/ui/async-await/async-fn-nonsend.no_drop_tracking.stderr index b29d2e192f4f5..afb1b432a4e2a 100644 --- a/tests/ui/async-await/async-fn-nonsend.no_drop_tracking.stderr +++ b/tests/ui/async-await/async-fn-nonsend.no_drop_tracking.stderr @@ -69,7 +69,7 @@ LL | fn assert_send(_: impl Send) {} | ^^^^ required by this bound in `assert_send` error: future cannot be sent between threads safely - --> $DIR/async-fn-nonsend.rs:76:17 + --> $DIR/async-fn-nonsend.rs:77:17 | LL | assert_send(non_sync_with_method_call_panic()); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ future returned by `non_sync_with_method_call_panic` is not `Send` @@ -93,7 +93,7 @@ LL | fn assert_send(_: impl Send) {} | ^^^^ required by this bound in `assert_send` error: future cannot be sent between threads safely - --> $DIR/async-fn-nonsend.rs:78:17 + --> $DIR/async-fn-nonsend.rs:79:17 | LL | assert_send(non_sync_with_method_call_infinite_loop()); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ future returned by `non_sync_with_method_call_infinite_loop` is not `Send` diff --git a/tests/ui/async-await/async-fn-nonsend.rs b/tests/ui/async-await/async-fn-nonsend.rs index ed440bd0182a6..8783a80f7df1d 100644 --- a/tests/ui/async-await/async-fn-nonsend.rs +++ b/tests/ui/async-await/async-fn-nonsend.rs @@ -72,7 +72,8 @@ pub fn pass_assert() { assert_send(non_send_temporary_in_match()); //~^ ERROR future cannot be sent between threads safely assert_send(non_sync_with_method_call()); - //~^ ERROR future cannot be sent between threads safely + //[no_drop_tracking]~^ ERROR future cannot be sent between threads safely + //[drop_tracking]~^^ ERROR future cannot be sent between threads safely assert_send(non_sync_with_method_call_panic()); //[no_drop_tracking]~^ ERROR future cannot be sent between threads safely assert_send(non_sync_with_method_call_infinite_loop()); diff --git a/tests/ui/async-await/drop-track-field-assign-nonsend.drop_tracking_mir.stderr b/tests/ui/async-await/drop-track-field-assign-nonsend.drop_tracking_mir.stderr index d9141cf4e3642..c4665ee26e450 100644 --- a/tests/ui/async-await/drop-track-field-assign-nonsend.drop_tracking_mir.stderr +++ b/tests/ui/async-await/drop-track-field-assign-nonsend.drop_tracking_mir.stderr @@ -5,14 +5,11 @@ LL | assert_send(agent.handle()); | ^^^^^^^^^^^^^^ future returned by `handle` is not `Send` | = help: within `impl Future`, the trait `Send` is not implemented for `Rc` -note: future is not `Send` as this value is used across an await - --> $DIR/drop-track-field-assign-nonsend.rs:23:39 +note: captured value is not `Send` because `&mut` references cannot be sent unless their referent is `Send` + --> $DIR/drop-track-field-assign-nonsend.rs:19:21 | -LL | let mut info = self.info_result.clone(); - | -------- has type `InfoResult` which is not `Send` -... -LL | let _ = send_element(element).await; - | ^^^^^ await occurs here, with `mut info` maybe used later +LL | async fn handle(&mut self) { + | ^^^^^^^^^ has type `&mut Agent` which is not `Send`, because `Agent` is not `Send` note: required by a bound in `assert_send` --> $DIR/drop-track-field-assign-nonsend.rs:40:19 | diff --git a/tests/ui/async-await/field-assign-nonsend.drop_tracking_mir.stderr b/tests/ui/async-await/field-assign-nonsend.drop_tracking_mir.stderr index d1df8e91afa2c..bbf9a3fae7ad2 100644 --- a/tests/ui/async-await/field-assign-nonsend.drop_tracking_mir.stderr +++ b/tests/ui/async-await/field-assign-nonsend.drop_tracking_mir.stderr @@ -5,14 +5,11 @@ LL | assert_send(agent.handle()); | ^^^^^^^^^^^^^^ future returned by `handle` is not `Send` | = help: within `impl Future`, the trait `Send` is not implemented for `Rc` -note: future is not `Send` as this value is used across an await - --> $DIR/field-assign-nonsend.rs:23:39 +note: captured value is not `Send` because `&mut` references cannot be sent unless their referent is `Send` + --> $DIR/field-assign-nonsend.rs:19:21 | -LL | let mut info = self.info_result.clone(); - | -------- has type `InfoResult` which is not `Send` -... -LL | let _ = send_element(element).await; - | ^^^^^ await occurs here, with `mut info` maybe used later +LL | async fn handle(&mut self) { + | ^^^^^^^^^ has type `&mut Agent` which is not `Send`, because `Agent` is not `Send` note: required by a bound in `assert_send` --> $DIR/field-assign-nonsend.rs:40:19 | From ac015de5137becebf7358b1642fa08462e3df4d7 Mon Sep 17 00:00:00 2001 From: b-naber Date: Sun, 28 May 2023 21:36:58 +0000 Subject: [PATCH 05/20] add test for #96084 --- tests/ui/async-await/issue-96084.rs | 33 +++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+) create mode 100644 tests/ui/async-await/issue-96084.rs diff --git a/tests/ui/async-await/issue-96084.rs b/tests/ui/async-await/issue-96084.rs new file mode 100644 index 0000000000000..7e84c155330a3 --- /dev/null +++ b/tests/ui/async-await/issue-96084.rs @@ -0,0 +1,33 @@ +// run-pass +// edition:2018 + +use std::mem; + +async fn foo() { + let x = [0u8; 100]; + async {}.await; + println!("{}", x.len()); +} + +async fn a() { + let fut = foo(); + let fut = fut; + fut.await; +} + +async fn b() { + let fut = foo(); + println!("{}", mem::size_of_val(&fut)); + let fut = fut; + fut.await; +} + +fn main() { + assert_eq!(mem::size_of_val(&foo()), 102); + + // 1 + sizeof(foo) + assert_eq!(mem::size_of_val(&a()), 103); + + // 1 + (sizeof(foo) * 2) + assert_eq!(mem::size_of_val(&b()), 103); +} From 21a9b517bc46280f96d66f9e510cc6dc03f05329 Mon Sep 17 00:00:00 2001 From: b-naber Date: Mon, 29 May 2023 14:31:23 +0000 Subject: [PATCH 06/20] handle projections correctly --- .../src/impls/live_borrows.rs | 33 +++++++++++++------ 1 file changed, 23 insertions(+), 10 deletions(-) diff --git a/compiler/rustc_mir_dataflow/src/impls/live_borrows.rs b/compiler/rustc_mir_dataflow/src/impls/live_borrows.rs index 452848aea6cca..ed3497d07ad5b 100644 --- a/compiler/rustc_mir_dataflow/src/impls/live_borrows.rs +++ b/compiler/rustc_mir_dataflow/src/impls/live_borrows.rs @@ -570,17 +570,29 @@ where match &stmt.kind { StatementKind::Assign(assign) => { let lhs_place = assign.0; + let projection = lhs_place.projection; let lhs_place_ty = lhs_place.ty(self.body.local_decls(), self.tcx).ty; debug!(?lhs_place, ?lhs_place_ty); - match lhs_place_ty.kind() { - ty::Ref(..) | ty::RawPtr(..) => { - debug!("killing {:?}", lhs_place.local); - self._trans.kill(lhs_place.local); - - self.visit_rvalue(&assign.1, location); + match projection.as_slice() { + &[] | &[ProjectionElem::OpaqueCast(_)] => { + // If there aren't any projections or just an OpaqueCast we need to + // kill the local if it's a ref or a pointer. + match lhs_place_ty.kind() { + ty::Ref(..) | ty::RawPtr(..) => { + debug!("killing {:?}", lhs_place.local); + self._trans.kill(lhs_place.local); + + self.visit_rvalue(&assign.1, location); + } + _ => { + self.super_assign(&assign.0, &assign.1, location); + } + } } _ => { + // With any other projection elements a projection of a local (of type ref/ptr) + // is actually a use-site, but we handle this in the call to `visit_place`. self.super_assign(&assign.0, &assign.1, location); } } @@ -599,12 +611,13 @@ where context: PlaceContext, location: mir::Location, ) { - let place_ty = place.ty(self.body.local_decls(), self.tcx); - debug!(?place_ty); + let local = place.local; + let local_ty = self.body.local_decls()[local].ty; + debug!(?local_ty); - match place_ty.ty.kind() { + match local_ty.kind() { ty::Ref(..) | ty::RawPtr(..) => { - debug!("gen {:?}", place.local); + debug!("gen {:?}", local); self._trans.gen(place.local); } _ => {} From 5168a8aad3d0dcfa5a270ed29d652f9b76038ee6 Mon Sep 17 00:00:00 2001 From: b-naber Date: Mon, 29 May 2023 19:54:37 +0000 Subject: [PATCH 07/20] add NodeKind::Local --- .../src/impls/live_borrows.rs | 158 ++++++++++++------ 1 file changed, 105 insertions(+), 53 deletions(-) diff --git a/compiler/rustc_mir_dataflow/src/impls/live_borrows.rs b/compiler/rustc_mir_dataflow/src/impls/live_borrows.rs index ed3497d07ad5b..3100c28533333 100644 --- a/compiler/rustc_mir_dataflow/src/impls/live_borrows.rs +++ b/compiler/rustc_mir_dataflow/src/impls/live_borrows.rs @@ -26,22 +26,25 @@ use either::Either; #[derive(Copy, Clone, Debug)] enum NodeKind { - // An node corresponding to the place on the lhs of a statement like - // `_3 = Ref(_, _, _4)` + // An node corresponding to the place of the borrowed place (`_4` in this case) in + // an assignment like `_3 = Ref(_, _, _4)`. Borrow(Local), // Nodes corresponding to the place on the lhs of a statement like // `_2 = Aggregate(Adt(..), _, _, _, _), [move _3, move _6])`, - // where _3 and _6 are places corresponding to references or raw pointers - // or locals that are borrowed (`_2 = Ref(..)`). - LocalOrLocalWithRefs(Local), + // where _3 and _6 are places corresponding to references or raw pointers. + LocalWithRefs(Local), + + // Nodes corresponding to the place on the lhs of an assignment like `_2 = Ref(..)`. + Local(Local), } impl NodeKind { fn get_local(&self) -> Local { match self { Self::Borrow(local) => *local, - Self::LocalOrLocalWithRefs(local) => *local, + Self::LocalWithRefs(local) => *local, + Self::Local(local) => *local, } } } @@ -103,9 +106,8 @@ struct BorrowDependencies<'a, 'tcx> { // liveness analysis. dep_graph: Graph, - // Contains the `NodeIndex` corresponding to the local to which we're currently - // assigning. - current_local: Option, + // Contains the `Local` to which we're currently assigning. + current_local: Option, } impl<'a, 'tcx> BorrowDependencies<'a, 'tcx> { @@ -122,6 +124,17 @@ impl<'a, 'tcx> BorrowDependencies<'a, 'tcx> { locals_to_node_indexes: Default::default(), } } + + fn maybe_create_node(&mut self, node_kind: NodeKind) -> NodeIndex { + let local = node_kind.get_local(); + if let Some(node_idx) = self.locals_to_node_indexes.get(&local) { + *node_idx + } else { + let node_idx = self.dep_graph.add_node(node_kind); + self.locals_to_node_indexes.insert(local, node_idx); + node_idx + } + } } impl<'a, 'tcx> Visitor<'tcx> for BorrowDependencies<'a, 'tcx> { @@ -130,7 +143,7 @@ impl<'a, 'tcx> Visitor<'tcx> for BorrowDependencies<'a, 'tcx> { // Add nodes for the arguments for i in 1..=body.arg_count { let local = Local::from_usize(i); - let node_kind = NodeKind::LocalOrLocalWithRefs(local); + let node_kind = NodeKind::Local(local); let node_idx = self.dep_graph.add_node(node_kind); self.locals_to_node_indexes.insert(local, node_idx); } @@ -144,20 +157,8 @@ impl<'a, 'tcx> Visitor<'tcx> for BorrowDependencies<'a, 'tcx> { StatementKind::Assign(assign) => { let assign_place = assign.0; let assign_local = assign_place.local; - - let node_kind = match assign_place.ty(self.local_decls, self.tcx).ty.kind() { - ty::Ref(..) | ty::RawPtr(..) => NodeKind::Borrow(assign_local), - _ => NodeKind::LocalOrLocalWithRefs(assign_local), - }; - - let node_idx = self.dep_graph.add_node(node_kind); - self.locals_to_node_indexes.insert(assign_local, node_idx); - self.current_local = Some(node_idx); - - debug!( - "set current_local to {:?} for local {:?}", - self.current_local, assign_local - ); + self.current_local = Some(assign_local); + debug!("set current_local to {:?}", self.current_local); // Do not call `visit_place` here as this might introduce a self-edge, which our liveness analysis // assumes not to exist. @@ -178,18 +179,53 @@ impl<'a, 'tcx> Visitor<'tcx> for BorrowDependencies<'a, 'tcx> { fn visit_rvalue(&mut self, rvalue: &Rvalue<'tcx>, location: Location) { debug!("self.current_local: {:?}", self.current_local); match rvalue { + Rvalue::Use(Operand::Move(place) | Operand::Copy(place)) + if matches!( + place.ty(self.local_decls, self.tcx).ty.kind(), + ty::Ref(..) | ty::RawPtr(..) + ) => + { + // these are just re-assignments of already outstanding refs or pointers, + // hence we want to treat them as `NodeKind::Borrow` + // FIXME Are these always Operand::Copy or is Operand::Move also possible for refs/ptrs? + let Some(src_local) = self.current_local else { + bug!("Expected self.current_local to be set with Rvalue::Ref|Rvalue::AddressOf"); + }; + + // These are just moves of refs/ptrs, hence `NodeKind::Borrow`. + let src_node_kind = NodeKind::Borrow(src_local); + let src_node_idx = self.maybe_create_node(src_node_kind); + + let node_idx = if let Some(node_idx) = self.locals_to_node_indexes.get(&place.local) + { + *node_idx + } else { + bug!("should have added a node for the moved borrowed place before"); + }; + + debug!( + "adding edge from {:?}({:?}) -> {:?}({:?})", + src_node_idx, + self.dep_graph.node(src_node_idx).data, + node_idx, + self.dep_graph.node(node_idx).data, + ); + + self.dep_graph.add_edge(src_node_idx, node_idx, ()); + } Rvalue::Ref(_, _, borrowed_place) | Rvalue::AddressOf(_, borrowed_place) => { - let Some(src_node_idx) = self.current_local else { + let Some(src_local) = self.current_local else { bug!("Expected self.current_local to be set with Rvalue::Ref|Rvalue::AddressOf"); }; - let borrowed_local = borrowed_place.local; - let node_idx = - if let Some(node_idx) = self.locals_to_node_indexes.get(&borrowed_local) { - *node_idx - } else { - self.dep_graph.add_node(NodeKind::Borrow(borrowed_place.local)) - }; + // we're in a statement like `_4 = Ref(..)`, hence NodeKind::Borrow for `_4` + let src_node_kind = NodeKind::Borrow(src_local); + let src_node_idx = self.maybe_create_node(src_node_kind); + + // If we haven't previously added a node for `borrowed_place.local` then it can be neither + // `NodeKind::Borrow` nor `NodeKind::LocalsWithRefs`. + let borrowed_node_kind = NodeKind::Local(borrowed_place.local); + let node_idx = self.maybe_create_node(borrowed_node_kind); debug!( "adding edge from {:?}({:?}) -> {:?}({:?})", @@ -213,14 +249,16 @@ impl<'a, 'tcx> Visitor<'tcx> for BorrowDependencies<'a, 'tcx> { debug!("current local: {:?}", self.current_local); match place_ty.kind() { ty::Ref(..) | ty::RawPtr(..) => match self.current_local { - Some(src_node_idx) => { - let borrowed_local = place.local; - let node_idx = - if let Some(node_idx) = self.locals_to_node_indexes.get(&borrowed_local) { - *node_idx - } else { - self.dep_graph.add_node(NodeKind::Borrow(borrowed_local)) - }; + Some(src_local) => { + // If we haven't created a node for this before, then this must be a + // `NodeKind::LocalWithRefs` as we would have handled the + // other possible assignment case (`NodeKind::Local`) previously in + // `visit_rvalue`. + let src_node_kind = NodeKind::LocalWithRefs(src_local); + let src_node_idx = self.maybe_create_node(src_node_kind); + + let borrowed_node_kind = NodeKind::Borrow(place.local); + let node_idx = self.maybe_create_node(borrowed_node_kind); debug!( "adding edge from {:?}({:?}) -> {:?}({:?})", @@ -268,13 +306,8 @@ impl<'a, 'tcx> Visitor<'tcx> for BorrowDependencies<'a, 'tcx> { false }; - let node_idx = - self.dep_graph.add_node(NodeKind::LocalOrLocalWithRefs(destination.local)); - debug!("added local {:?} to graph with idx {:?}", destination.local, node_idx); - self.locals_to_node_indexes.insert(destination.local, node_idx); - if dest_might_include_refs_or_pointers { - self.current_local = Some(node_idx); + self.current_local = Some(destination.local); debug!("self.current_local: {:?}", self.current_local); } @@ -286,10 +319,29 @@ impl<'a, 'tcx> Visitor<'tcx> for BorrowDependencies<'a, 'tcx> { self.current_local = None; } TerminatorKind::Yield { resume_arg, value, .. } => { - let node_idx = - self.dep_graph.add_node(NodeKind::LocalOrLocalWithRefs(resume_arg.local)); - debug!("added local {:?} to graph with idx {:?}", resume_arg.local, node_idx); - self.locals_to_node_indexes.insert(resume_arg.local, node_idx); + let resume_arg_ty = resume_arg.ty(self.local_decls, self.tcx).ty; + debug!(?resume_arg_ty); + + let mut has_refs_or_pointers_visitor = MaybeHasRefsOrPointersVisitor::new(); + resume_arg_ty.visit_with(&mut has_refs_or_pointers_visitor); + + let dest_might_include_refs_or_pointers = + if has_refs_or_pointers_visitor.has_refs_or_pointers { + true + } else if has_refs_or_pointers_visitor.has_params { + // if we pass in any references or raw pointers as arguments and the + // return type includes type parameters, these type parameters could + // refer to those refs/ptrs, so we have to be conservative here and possibly + // include an edge. + true + } else { + false + }; + + if dest_might_include_refs_or_pointers { + self.current_local = Some(resume_arg.local); + debug!("self.current_local: {:?}", self.current_local); + } self.super_operand(value, location); } @@ -397,7 +449,7 @@ where // base node to keep alive vec![src_node.data.get_local()] } else { - if matches!(src_node.data, NodeKind::LocalOrLocalWithRefs(_)) { + if matches!(src_node.data, NodeKind::LocalWithRefs(_)) { // These are locals that we need to keep alive, but that also contain // successors in the graph since they contain other references/pointers. locals_for_node.push(current_local); @@ -577,7 +629,7 @@ where match projection.as_slice() { &[] | &[ProjectionElem::OpaqueCast(_)] => { // If there aren't any projections or just an OpaqueCast we need to - // kill the local if it's a ref or a pointer. + // kill the local. match lhs_place_ty.kind() { ty::Ref(..) | ty::RawPtr(..) => { debug!("killing {:?}", lhs_place.local); @@ -591,7 +643,7 @@ where } } _ => { - // With any other projection elements a projection of a local (of type ref/ptr) + // With any other projection elements, a projection of a local (of type ref/ptr) // is actually a use-site, but we handle this in the call to `visit_place`. self.super_assign(&assign.0, &assign.1, location); } From fc87640a1b951f5f2fb132d11fe1a146daf30519 Mon Sep 17 00:00:00 2001 From: b-naber Date: Mon, 29 May 2023 20:57:02 +0000 Subject: [PATCH 08/20] gen Locals with NodeKind::LocalWithRefs in liveness analysis --- .../src/impls/live_borrows.rs | 123 +++++++++++------- 1 file changed, 73 insertions(+), 50 deletions(-) diff --git a/compiler/rustc_mir_dataflow/src/impls/live_borrows.rs b/compiler/rustc_mir_dataflow/src/impls/live_borrows.rs index 3100c28533333..6996dfc5d88dd 100644 --- a/compiler/rustc_mir_dataflow/src/impls/live_borrows.rs +++ b/compiler/rustc_mir_dataflow/src/impls/live_borrows.rs @@ -265,7 +265,7 @@ impl<'a, 'tcx> Visitor<'tcx> for BorrowDependencies<'a, 'tcx> { src_node_idx, self.dep_graph.node(src_node_idx).data, node_idx, - borrowed_local + place.local ); self.dep_graph.add_edge(src_node_idx, node_idx, ()); @@ -359,42 +359,8 @@ impl<'mir, 'tcx> BorrowedLocalsResults<'mir, 'tcx> where 'tcx: 'mir, { - fn new( - tcx: TyCtxt<'tcx>, - body: &'mir Body<'tcx>, - borrows_analysis_results: Results<'tcx, LiveBorrows<'mir, 'tcx>>, - ) -> Self { - let mut borrow_deps = BorrowDependencies::new(body.local_decls(), tcx); - borrow_deps.visit_body(body); - - if cfg!(debug_assertions) { - let dep_graph = &borrow_deps.dep_graph; - - debug!( - "nodes: {:#?}", - dep_graph - .all_nodes() - .clone() - .into_iter() - .enumerate() - .map(|(i, node)| (i, node.data)) - .collect::>() - ); - - debug!("edges:"); - for edge in dep_graph.all_edges() { - let src_node_idx = edge.source(); - let src_node = dep_graph.node(src_node_idx); - let target_node_idx = edge.target(); - let target_node = dep_graph.node(target_node_idx); - debug!( - "{:?}({:?}) -> {:?}({:?}) ({:?})", - src_node_idx, src_node.data, target_node_idx, target_node.data, edge.data - ) - } - } - - let dep_graph = &borrow_deps.dep_graph; + fn new(borrows_analysis_results: Results<'tcx, LiveBorrows<'mir, 'tcx>>) -> Self { + let dep_graph = &borrows_analysis_results.analysis.borrow_deps.dep_graph; let borrowed_local_to_locals_to_keep_alive = Self::get_locals_to_keep_alive_map(dep_graph); Self { borrows_analysis_results, borrowed_local_to_locals_to_keep_alive } } @@ -467,11 +433,41 @@ pub fn get_borrowed_locals_results<'mir, 'tcx>( tcx: TyCtxt<'tcx>, ) -> BorrowedLocalsResults<'mir, 'tcx> { debug!("body: {:#?}", body); - let live_borrows = LiveBorrows::new(body, tcx); + + let mut borrow_deps = BorrowDependencies::new(body.local_decls(), tcx); + borrow_deps.visit_body(body); + + if cfg!(debug_assertions) { + let dep_graph = &borrow_deps.dep_graph; + + debug!( + "nodes: {:#?}", + dep_graph + .all_nodes() + .clone() + .into_iter() + .enumerate() + .map(|(i, node)| (i, node.data)) + .collect::>() + ); + + debug!("edges:"); + for edge in dep_graph.all_edges() { + let src_node_idx = edge.source(); + let src_node = dep_graph.node(src_node_idx); + let target_node_idx = edge.target(); + let target_node = dep_graph.node(target_node_idx); + debug!( + "{:?}({:?}) -> {:?}({:?}) ({:?})", + src_node_idx, src_node.data, target_node_idx, target_node.data, edge.data + ) + } + } + let live_borrows = LiveBorrows::new(body, tcx, borrow_deps); let results = live_borrows.into_engine(tcx, body).pass_name("borrowed_locals").iterate_to_fixpoint(); - BorrowedLocalsResults::new(tcx, body, results) + BorrowedLocalsResults::new(results) } pub struct BorrowedLocalsResultsCursor<'a, 'mir, 'tcx> { @@ -544,18 +540,31 @@ impl<'a, 'mir, 'tcx> BorrowedLocalsResultsCursor<'a, 'mir, 'tcx> { } /// Performs a liveness analysis for borrows and raw pointers. -pub struct LiveBorrows<'a, 'tcx> { - body: &'a Body<'tcx>, +pub struct LiveBorrows<'mir, 'tcx> { + body: &'mir Body<'tcx>, tcx: TyCtxt<'tcx>, + borrow_deps: BorrowDependencies<'mir, 'tcx>, } -impl<'a, 'tcx> LiveBorrows<'a, 'tcx> { - fn new(body: &'a Body<'tcx>, tcx: TyCtxt<'tcx>) -> Self { - LiveBorrows { body, tcx } +impl<'mir, 'tcx> LiveBorrows<'mir, 'tcx> { + fn new( + body: &'mir Body<'tcx>, + tcx: TyCtxt<'tcx>, + borrow_deps: BorrowDependencies<'mir, 'tcx>, + ) -> Self { + LiveBorrows { body, tcx, borrow_deps } } - fn transfer_function<'b, T>(&self, trans: &'b mut T) -> TransferFunction<'a, 'b, 'tcx, T> { - TransferFunction { body: self.body, tcx: self.tcx, _trans: trans } + fn transfer_function<'b, T>( + &self, + trans: &'b mut T, + ) -> TransferFunction<'mir, 'b, '_, 'tcx, T> { + TransferFunction { + body: self.body, + tcx: self.tcx, + _trans: trans, + borrow_deps: &self.borrow_deps, + } } } @@ -607,13 +616,14 @@ impl<'a, 'tcx> GenKillAnalysis<'tcx> for LiveBorrows<'a, 'tcx> { } /// A `Visitor` that defines the transfer function for `MaybeBorrowedLocals`. -struct TransferFunction<'a, 'b, 'tcx, T> { +struct TransferFunction<'a, 'b, 'c, 'tcx, T> { body: &'a Body<'tcx>, tcx: TyCtxt<'tcx>, _trans: &'b mut T, + borrow_deps: &'c BorrowDependencies<'a, 'tcx>, } -impl<'a, 'tcx, T> Visitor<'tcx> for TransferFunction<'a, '_, 'tcx, T> +impl<'a, 'tcx, T> Visitor<'tcx> for TransferFunction<'a, '_, '_, 'tcx, T> where T: GenKill, { @@ -670,9 +680,22 @@ where match local_ty.kind() { ty::Ref(..) | ty::RawPtr(..) => { debug!("gen {:?}", local); - self._trans.gen(place.local); + self._trans.gen(local); + } + _ => { + if let Some(node_idx) = self.borrow_deps.locals_to_node_indexes.get(&local) { + let node = self.borrow_deps.dep_graph.node(*node_idx); + + // these are `Local`s that contain references/pointers or are raw pointers + // that were assigned to raw pointers, which were cast to usize. Hence we + // need to treat them as uses of the references/pointers that they + // refer/correspond to. + if let NodeKind::LocalWithRefs(_) = node.data { + debug!("gen {:?}", local); + self._trans.gen(local); + } + } } - _ => {} } self.super_place(place, context, location); From 1fedadd95999468a535d98ca50ea34882b42e77d Mon Sep 17 00:00:00 2001 From: b-naber Date: Mon, 29 May 2023 22:26:34 +0000 Subject: [PATCH 09/20] add doc-comment for module --- .../src/impls/live_borrows.rs | 139 ++++++++++++++++-- 1 file changed, 127 insertions(+), 12 deletions(-) diff --git a/compiler/rustc_mir_dataflow/src/impls/live_borrows.rs b/compiler/rustc_mir_dataflow/src/impls/live_borrows.rs index 6996dfc5d88dd..229e9305f87eb 100644 --- a/compiler/rustc_mir_dataflow/src/impls/live_borrows.rs +++ b/compiler/rustc_mir_dataflow/src/impls/live_borrows.rs @@ -1,3 +1,122 @@ +//! This module defines a more fine-grained analysis for `Local`s that are live due +//! to outstanding references or raw pointers than `MaybeBorrowedLocals`. +//! +//! The analysis consists of three steps: +//! 1. build a dependency graph that relates `Local`s based on their borrowing relationship. +//! As an example if we have something like this (in a simplified MIR representation): +//! +//! ```ignore(rust) +//! _4 = Bar {} +//! _5 = Ref(_4) +//! ``` +//! +//! Then we add an edge from `_5` to `_4`. +//! 2. perform a liveness analysis for borrowed `Local`s. +//! Continuing our example from step 1, if we later have a use of `_5`, `_5` is +//! live at least from its definition to that use of it. +//! 3. Combine the two analyses from step 1 and 2. For any `Local` that corresponds +//! to a borrow (`_5` in our example), we want to keep the `Local` (`_4`), which is actually +//! borrowed through it, live over the range at which the borrow is live. Hence for any point +//! in that range we traverse our dependency graph and look for leaf nodes. In our example +//! we would find an edge from `_5` to `_4`, which is a leaf node and hence we keep `_4` live +//! over that range. +//! +//! There are some corner cases we need to look out for to make this analysis sound. Let's look +//! at each of the three steps in more detail and elaborate how these steps deal with these corner +//! cases. +//! +//! 1. Dependency Graph +//! +//! The `Node`s in the dependency graph include data values of type `NodeKind`. `NodeKind` has +//! three variants: `Local`, `Borrow` and `LocalWithRefs`. +//! * `NodeKind::Local` is used for `Local`s that are borrowed somewhere (`_4` in our example) +//! * `NodeKind::Borrow` is used for `Local`s that correspond to borrows (`_5` in our example) and +//! also `Local`s that result from re-borrows. +//! * `NodeKind::LocalWithRefs` is used for `Local`s that aren't themselves borrows, but contain +//! borrowed `Local`s. We want to keep these `Local`s live and also any of the references/pointers +//! they might contain. Let's look at an example: +//! +//! ```ignore(rust) +//! _4 = Bar {} +//! _5 = Ref(_4) +//! _6 = Aggregate(..)(move _5) +//! ... +//! _7 = (_6.0) +//! ``` +//! +//! In this example `_6` would be given `NodeKind::LocalWithRefs` and our graph would look +//! as follows: +//! +//! `_7 (NodeKind::Borrow) -> `_6` (NodeKind::LocalWithRefs) -> `_5` (NodeKind::Borrow) -> `_4` (NodeKind::Local) +//! +//! In addition to keeping `_6` alive over the range of `_7` we also keep `_4` alive (leaf node). +//! +//! Additionally `NodeKind::LocalWithRefs` is also used for raw pointers that are cast to +//! `usize`: +//! +//! ```ignore(rust) +//! _4 = Bar {} +//! _5 = AddressOf(_4) +//! _6 = _5 as usize +//! _7 = Aggregate(..) (move _6) +//! _8 = (_7.0) +//! ``` +//! +//! In this example our graph would have the following edges: +//! * `_5` (Borrow) -> `_4` (Local) +//! * `_6` (LocalWithRefs) -> `_5` (Borrow) +//! * `_7` (LocalWithRefs) -> `_6` (LocalWithRefs) +//! * `_8` (LocalWithRefs) -> `_7` (LocalWithRefs) (FIXME this one is currently not being done) +//! +//! We also have to be careful when dealing with `Terminator`s. Whenever we pass references, +//! pointers or `Local`s with `NodeKind::LocalWithRefs` (FIXME currently not done) to +//! a `TerminatorKind::Call` or `TerminatorKind::Yield` and the destination `Place` or resume place, resp., +//! contains references/pointers or generic parameters we have to be careful and treat the +//! `Local`s corresponding to the `Place`s as `NodeKind::LocalWithRef`s. +//! +//! 2. Liveness analysis for borrows +//! +//! We perform a standard liveness analysis on any outstanding references, pointers or `Local`s +//! with `NodeKind::LocalWithRefs`. So we `gen` at any use site, which are either direct uses +//! of these `Local`s or projections that contain these `Local`s. So e.g.: +//! +//! ```ignore(rust) +//! 1. _3 = Foo {} +//! 2. _4 = Bar {} +//! 3. _5 = Ref(_3) +//! 4. _6 = Ref(_4) +//! 5. _7 = Aggregate(..)(move _5) +//! 6. _8 = Call(..)(move _6) (assume _8 contains no refs/ptrs or generic params) +//! 7. _9 = (_8.0) +//! 8. (_7.0) = _9 +//! ``` +//! +//! * `_5` is live from stmt 3 to stmt 5 +//! * `_6` is live from stmt 4 to stmt 6 +//! * `_7` is a `Local` of kind `LocalWithRef` so needs to be taken into account in the +//! analyis. It's live from stmt 5 to stmt 8 +//! +//! 3. Determining which `Local`s are borrowed +//! +//! Let's use our last example again. The dependency graph for that example looks as follows: +//! +//! `_5` (Borrow) -> `_3` (Local) +//! `_6` (Borrow) -> `_4` (Local) +//! `_7` (LocalWithRef) -> `_5` (Borrow) +//! `_7` (LocalWithRef) -> `_9` (Local) +//! +//! So at each of those statements we have the following `Local`s that are live due to borrows: +//! +//! 1. {} +//! 2. {} +//! 3. {_3} +//! 4. {_3, _4} +//! 5. {_3, _4, _7} +//! 6. {_3, _4, _7} +//! 7. {_3, _7} +//! 8. {_3, _7} +//! + use super::*; use crate::framework::{Analysis, Results, ResultsCursor}; @@ -14,16 +133,6 @@ use rustc_middle::ty::{self, Ty, TypeSuperVisitable, TypeVisitable}; use core::ops::ControlFlow; use either::Either; -/// This module defines a more fine-grained analysis for `Local`s that are live due -/// to outstanding references or raw pointers. -/// The idea behind the analysis is that we first build a dependency graph between -/// `Local`s corresponding to references or pointers and the `Local`s that are borrowed. -/// This is done by the `BorrowDependencies` struct. -/// As a second step we perform a liveness analysis for references and pointers, which is -/// done by `LiveBorrows`. -/// Finally we combine the results of the liveness analysis and the dependency graph to -/// infer which borrowed locals need to be live at a given `Location`. - #[derive(Copy, Clone, Debug)] enum NodeKind { // An node corresponding to the place of the borrowed place (`_4` in this case) in @@ -237,7 +346,10 @@ impl<'a, 'tcx> Visitor<'tcx> for BorrowDependencies<'a, 'tcx> { self.dep_graph.add_edge(src_node_idx, node_idx, ()); } - _ => self.super_rvalue(rvalue, location), + _ => { + // FIXME Need to also create edges for `Local`s that correspond to `NodeKind::LocalWithRefs` here + self.super_rvalue(rvalue, location) + } } } @@ -272,7 +384,9 @@ impl<'a, 'tcx> Visitor<'tcx> for BorrowDependencies<'a, 'tcx> { } None => {} }, - _ => {} + _ => { + // FIXME Need to also create edges for `Local`s that correspond to `NodeKind::LocalWithRefs` here + } } self.super_place(place, context, location) @@ -382,6 +496,7 @@ where borrows_to_locals } + // FIXME Account for cycles in the graph! fn dfs_for_node( node_idx: NodeIndex, borrows_to_locals: &mut FxHashMap>, From a9a2b89b74fa4732c233543edf8ae55155e68125 Mon Sep 17 00:00:00 2001 From: b-naber Date: Tue, 30 May 2023 12:41:38 +0000 Subject: [PATCH 10/20] always create nodes for call and yield destination locals --- .../src/impls/live_borrows.rs | 151 ++++++------------ .../src/impls/storage_liveness.rs | 1 + ...ld-assign-nonsend.drop_tracking_mir.stderr | 11 +- ...ld-assign-nonsend.drop_tracking_mir.stderr | 11 +- 4 files changed, 68 insertions(+), 106 deletions(-) diff --git a/compiler/rustc_mir_dataflow/src/impls/live_borrows.rs b/compiler/rustc_mir_dataflow/src/impls/live_borrows.rs index 229e9305f87eb..6a27739f7f8e0 100644 --- a/compiler/rustc_mir_dataflow/src/impls/live_borrows.rs +++ b/compiler/rustc_mir_dataflow/src/impls/live_borrows.rs @@ -128,9 +128,8 @@ use rustc_data_structures::graph::implementation::{Graph, NodeIndex}; use rustc_middle::mir::visit::PlaceContext; use rustc_middle::mir::visit::Visitor; use rustc_middle::mir::*; -use rustc_middle::ty::{self, Ty, TypeSuperVisitable, TypeVisitable}; +use rustc_middle::ty; -use core::ops::ControlFlow; use either::Either; #[derive(Copy, Clone, Debug)] @@ -158,43 +157,6 @@ impl NodeKind { } } -/// TypeVisitor that looks for `ty::Ref`, `ty::RawPtr`. Additionally looks for `ty::Param`, -/// which could themselves refer to references or raw pointers. -struct MaybeHasRefsOrPointersVisitor { - has_refs_or_pointers: bool, - has_params: bool, -} - -impl MaybeHasRefsOrPointersVisitor { - fn new() -> Self { - MaybeHasRefsOrPointersVisitor { has_refs_or_pointers: false, has_params: false } - } -} - -impl<'tcx> ty::TypeVisitor> for MaybeHasRefsOrPointersVisitor { - type BreakTy = (); - - #[instrument(skip(self), level = "debug")] - fn visit_ty(&mut self, ty: Ty<'tcx>) -> ControlFlow { - match ty.kind() { - ty::Ref(..) => { - self.has_refs_or_pointers = true; - return ControlFlow::Break(()); - } - ty::RawPtr(..) => { - self.has_refs_or_pointers = true; - return ControlFlow::Break(()); - } - ty::Param(_) => { - self.has_params = true; - } - _ => {} - } - - ty.super_visit_with(self) - } -} - /// Used to build a dependency graph between borrows/pointers and the `Local`s that /// they reference. /// We add edges to the graph in two kinds of situations: @@ -265,6 +227,7 @@ impl<'a, 'tcx> Visitor<'tcx> for BorrowDependencies<'a, 'tcx> { match &statement.kind { StatementKind::Assign(assign) => { let assign_place = assign.0; + debug!("assign_place_ty: {:?}", assign_place.ty(self.local_decls, self.tcx).ty); let assign_local = assign_place.local; self.current_local = Some(assign_local); debug!("set current_local to {:?}", self.current_local); @@ -305,12 +268,8 @@ impl<'a, 'tcx> Visitor<'tcx> for BorrowDependencies<'a, 'tcx> { let src_node_kind = NodeKind::Borrow(src_local); let src_node_idx = self.maybe_create_node(src_node_kind); - let node_idx = if let Some(node_idx) = self.locals_to_node_indexes.get(&place.local) - { - *node_idx - } else { - bug!("should have added a node for the moved borrowed place before"); - }; + let node_kind = NodeKind::Borrow(place.local); + let node_idx = self.maybe_create_node(node_kind); debug!( "adding edge from {:?}({:?}) -> {:?}({:?})", @@ -346,10 +305,12 @@ impl<'a, 'tcx> Visitor<'tcx> for BorrowDependencies<'a, 'tcx> { self.dep_graph.add_edge(src_node_idx, node_idx, ()); } - _ => { - // FIXME Need to also create edges for `Local`s that correspond to `NodeKind::LocalWithRefs` here + Rvalue::Cast(..) => { + // FIXME we probably should handle pointer casts here directly + self.super_rvalue(rvalue, location) } + _ => self.super_rvalue(rvalue, location), } } @@ -385,7 +346,8 @@ impl<'a, 'tcx> Visitor<'tcx> for BorrowDependencies<'a, 'tcx> { None => {} }, _ => { - // FIXME Need to also create edges for `Local`s that correspond to `NodeKind::LocalWithRefs` here + // FIXME I think we probably need to introduce edges here if `place.local` + // corresponds to a `NodeKind::LocalWithRefs` } } @@ -404,26 +366,12 @@ impl<'a, 'tcx> Visitor<'tcx> for BorrowDependencies<'a, 'tcx> { let dest_ty = destination.ty(self.local_decls, self.tcx).ty; debug!(?dest_ty); - let mut has_refs_or_pointers_visitor = MaybeHasRefsOrPointersVisitor::new(); - dest_ty.visit_with(&mut has_refs_or_pointers_visitor); - - let dest_might_include_refs_or_pointers = - if has_refs_or_pointers_visitor.has_refs_or_pointers { - true - } else if has_refs_or_pointers_visitor.has_params { - // if we pass in any references or raw pointers as arguments and the - // return type includes type parameters, these type parameters could - // refer to those refs/ptrs, so we have to be conservative here and possibly - // include an edge. - true - } else { - false - }; - - if dest_might_include_refs_or_pointers { - self.current_local = Some(destination.local); - debug!("self.current_local: {:?}", self.current_local); - } + // To ensure safety we need to add `destination` to the graph as a `Node` with `NodeKind::LocalWithRefs` + // if we pass in any refs/ptrs or `Local`s corresponding to `NodeKind::LocalWithRefs`. The reason for this + // is that the function could include those refs/ptrs in its return value. It's not sufficient + // to look for the existence of `ty::Ref` or `ty::RawPtr` in the type of the return type, since the + // function could also cast pointers to integers e.g. . + self.current_local = Some(destination.local); self.visit_operand(func, location); for arg in args { @@ -436,26 +384,9 @@ impl<'a, 'tcx> Visitor<'tcx> for BorrowDependencies<'a, 'tcx> { let resume_arg_ty = resume_arg.ty(self.local_decls, self.tcx).ty; debug!(?resume_arg_ty); - let mut has_refs_or_pointers_visitor = MaybeHasRefsOrPointersVisitor::new(); - resume_arg_ty.visit_with(&mut has_refs_or_pointers_visitor); - - let dest_might_include_refs_or_pointers = - if has_refs_or_pointers_visitor.has_refs_or_pointers { - true - } else if has_refs_or_pointers_visitor.has_params { - // if we pass in any references or raw pointers as arguments and the - // return type includes type parameters, these type parameters could - // refer to those refs/ptrs, so we have to be conservative here and possibly - // include an edge. - true - } else { - false - }; - - if dest_might_include_refs_or_pointers { - self.current_local = Some(resume_arg.local); - debug!("self.current_local: {:?}", self.current_local); - } + // We may need to add edges from `destination`, see the comment for this statement + // in `TerminatorKind::Call` for the rationale behind this. + self.current_local = Some(resume_arg.local); self.super_operand(value, location); } @@ -578,6 +509,7 @@ pub fn get_borrowed_locals_results<'mir, 'tcx>( ) } } + let live_borrows = LiveBorrows::new(body, tcx, borrow_deps); let results = live_borrows.into_engine(tcx, body).pass_name("borrowed_locals").iterate_to_fixpoint(); @@ -763,6 +695,15 @@ where self.visit_rvalue(&assign.1, location); } _ => { + if let Some(node_idx) = + self.borrow_deps.locals_to_node_indexes.get(&lhs_place.local) + { + let node = self.borrow_deps.dep_graph.node(*node_idx); + if let NodeKind::LocalWithRefs(_) = node.data { + debug!("killing {:?}", lhs_place.local); + self._trans.kill(lhs_place.local); + } + } self.super_assign(&assign.0, &assign.1, location); } } @@ -828,18 +769,32 @@ where #[instrument(skip(self), level = "debug")] fn visit_terminator(&mut self, terminator: &mir::Terminator<'tcx>, location: Location) { - match terminator.kind { - TerminatorKind::Call { destination, .. } => { - debug!("killing {:?}", destination.local); - self._trans.kill(destination.local); + match &terminator.kind { + TerminatorKind::Call { destination, args, .. } => { + match destination.projection.as_slice() { + &[] | &[ProjectionElem::OpaqueCast(_)] => { + debug!("killing {:?}", destination.local); + self._trans.kill(destination.local); + + for arg in args { + self.visit_operand(arg, location) + } + } + _ => self.super_terminator(terminator, location), + } } - TerminatorKind::Yield { resume_arg, .. } => { - debug!("killing {:?}", resume_arg.local); - self._trans.kill(resume_arg.local); + TerminatorKind::Yield { resume_arg, value, .. } => { + match resume_arg.projection.as_slice() { + &[] | &[ProjectionElem::OpaqueCast(_)] => { + debug!("killing {:?}", resume_arg.local); + self._trans.kill(resume_arg.local); + + self.visit_operand(value, location) + } + _ => self.super_terminator(terminator, location), + } } - _ => {} + _ => self.super_terminator(terminator, location), } - - self.super_terminator(terminator, location); } } diff --git a/compiler/rustc_mir_dataflow/src/impls/storage_liveness.rs b/compiler/rustc_mir_dataflow/src/impls/storage_liveness.rs index 112d289a13cbd..89eb7fa8a81b7 100644 --- a/compiler/rustc_mir_dataflow/src/impls/storage_liveness.rs +++ b/compiler/rustc_mir_dataflow/src/impls/storage_liveness.rs @@ -199,6 +199,7 @@ impl<'a, 'mir, 'tcx> crate::GenKillAnalysis<'tcx> for MaybeRequiresStorage<'a, ' for i in 0..self.body.local_decls().len() { let local = Local::from_usize(i); if borrowed_locals_at_loc.contains(local) { + debug!("{:?} is borrowed", local); trans.gen(local); } } diff --git a/tests/ui/async-await/drop-track-field-assign-nonsend.drop_tracking_mir.stderr b/tests/ui/async-await/drop-track-field-assign-nonsend.drop_tracking_mir.stderr index c4665ee26e450..d9141cf4e3642 100644 --- a/tests/ui/async-await/drop-track-field-assign-nonsend.drop_tracking_mir.stderr +++ b/tests/ui/async-await/drop-track-field-assign-nonsend.drop_tracking_mir.stderr @@ -5,11 +5,14 @@ LL | assert_send(agent.handle()); | ^^^^^^^^^^^^^^ future returned by `handle` is not `Send` | = help: within `impl Future`, the trait `Send` is not implemented for `Rc` -note: captured value is not `Send` because `&mut` references cannot be sent unless their referent is `Send` - --> $DIR/drop-track-field-assign-nonsend.rs:19:21 +note: future is not `Send` as this value is used across an await + --> $DIR/drop-track-field-assign-nonsend.rs:23:39 | -LL | async fn handle(&mut self) { - | ^^^^^^^^^ has type `&mut Agent` which is not `Send`, because `Agent` is not `Send` +LL | let mut info = self.info_result.clone(); + | -------- has type `InfoResult` which is not `Send` +... +LL | let _ = send_element(element).await; + | ^^^^^ await occurs here, with `mut info` maybe used later note: required by a bound in `assert_send` --> $DIR/drop-track-field-assign-nonsend.rs:40:19 | diff --git a/tests/ui/async-await/field-assign-nonsend.drop_tracking_mir.stderr b/tests/ui/async-await/field-assign-nonsend.drop_tracking_mir.stderr index bbf9a3fae7ad2..d1df8e91afa2c 100644 --- a/tests/ui/async-await/field-assign-nonsend.drop_tracking_mir.stderr +++ b/tests/ui/async-await/field-assign-nonsend.drop_tracking_mir.stderr @@ -5,11 +5,14 @@ LL | assert_send(agent.handle()); | ^^^^^^^^^^^^^^ future returned by `handle` is not `Send` | = help: within `impl Future`, the trait `Send` is not implemented for `Rc` -note: captured value is not `Send` because `&mut` references cannot be sent unless their referent is `Send` - --> $DIR/field-assign-nonsend.rs:19:21 +note: future is not `Send` as this value is used across an await + --> $DIR/field-assign-nonsend.rs:23:39 | -LL | async fn handle(&mut self) { - | ^^^^^^^^^ has type `&mut Agent` which is not `Send`, because `Agent` is not `Send` +LL | let mut info = self.info_result.clone(); + | -------- has type `InfoResult` which is not `Send` +... +LL | let _ = send_element(element).await; + | ^^^^^ await occurs here, with `mut info` maybe used later note: required by a bound in `assert_send` --> $DIR/field-assign-nonsend.rs:40:19 | From e8c6786c588b6d4f563a59d9d70bf95279159602 Mon Sep 17 00:00:00 2001 From: b-naber Date: Tue, 30 May 2023 20:32:02 +0000 Subject: [PATCH 11/20] more docs changes --- .../src/impls/live_borrows.rs | 100 +++++++++++++----- 1 file changed, 74 insertions(+), 26 deletions(-) diff --git a/compiler/rustc_mir_dataflow/src/impls/live_borrows.rs b/compiler/rustc_mir_dataflow/src/impls/live_borrows.rs index 6a27739f7f8e0..f86924f18f2e3 100644 --- a/compiler/rustc_mir_dataflow/src/impls/live_borrows.rs +++ b/compiler/rustc_mir_dataflow/src/impls/live_borrows.rs @@ -29,12 +29,13 @@ //! //! The `Node`s in the dependency graph include data values of type `NodeKind`. `NodeKind` has //! three variants: `Local`, `Borrow` and `LocalWithRefs`. -//! * `NodeKind::Local` is used for `Local`s that are borrowed somewhere (`_4` in our example) +//! * `NodeKind::Local` is used for `Local`s that are borrowed somewhere (`_4` in our example), but aren't +//! themselves references or pointers. //! * `NodeKind::Borrow` is used for `Local`s that correspond to borrows (`_5` in our example) and //! also `Local`s that result from re-borrows. //! * `NodeKind::LocalWithRefs` is used for `Local`s that aren't themselves borrows, but contain -//! borrowed `Local`s. We want to keep these `Local`s live and also any of the references/pointers -//! they might contain. Let's look at an example: +//! `Local`s that correspond to references, pointers or other `Local`s with `Node`s of kind +//! `NodeKind::LocalWithRef`s. Let's look at an example: //! //! ```ignore(rust) //! _4 = Bar {} @@ -47,9 +48,15 @@ //! In this example `_6` would be given `NodeKind::LocalWithRefs` and our graph would look //! as follows: //! -//! `_7 (NodeKind::Borrow) -> `_6` (NodeKind::LocalWithRefs) -> `_5` (NodeKind::Borrow) -> `_4` (NodeKind::Local) -//! -//! In addition to keeping `_6` alive over the range of `_7` we also keep `_4` alive (leaf node). +//! `_7` (NodeKind::Borrow) -> `_6` (NodeKind::LocalWithRefs) -> `_5` (NodeKind::Borrow) -> `_4` (NodeKind::Local) +//! +//! On the one hand we need to treat `Local`s with `Node`s of kind `NodeKind::LocalWithRefs` similarly +//! to how we treat `Local`s with `Node`s of kind `NodeKind::Local`, in the sense that if they are +//! borrowed we want to keep them live over the live range of the borrow. But on the other hand we +//! want to also treat them like `Local`s with `Node`s of kind `NodeKind::Borrow` as they ultimately +//! could also contain references or pointers that refer to other `Local`s. So we want a +//! path in the graph from a `NodeKind::LocalWithRef`s node to the `NodeKind::Local` nodes, whose borrows +//! they might contain. //! //! Additionally `NodeKind::LocalWithRefs` is also used for raw pointers that are cast to //! `usize`: @@ -66,13 +73,13 @@ //! * `_5` (Borrow) -> `_4` (Local) //! * `_6` (LocalWithRefs) -> `_5` (Borrow) //! * `_7` (LocalWithRefs) -> `_6` (LocalWithRefs) -//! * `_8` (LocalWithRefs) -> `_7` (LocalWithRefs) (FIXME this one is currently not being done) +//! * `_8` (LocalWithRefs) -> `_7` (LocalWithRefs) (FIXME this one is currently not being done, but unsafe) //! //! We also have to be careful when dealing with `Terminator`s. Whenever we pass references, -//! pointers or `Local`s with `NodeKind::LocalWithRefs` (FIXME currently not done) to -//! a `TerminatorKind::Call` or `TerminatorKind::Yield` and the destination `Place` or resume place, resp., -//! contains references/pointers or generic parameters we have to be careful and treat the -//! `Local`s corresponding to the `Place`s as `NodeKind::LocalWithRef`s. +//! pointers or `Local`s with `NodeKind::LocalWithRefs` to a `TerminatorKind::Call` or +//! `TerminatorKind::Yield`, the destination `Place` or resume place, resp., might contain +//! these references, pointers or `NodeKind::LocalWithRefs` `Local`s, hence we have to be conservative +//! and keep the `destination` `Local` and `resume_arg` `Local` live. //! //! 2. Liveness analysis for borrows //! @@ -86,15 +93,19 @@ //! 3. _5 = Ref(_3) //! 4. _6 = Ref(_4) //! 5. _7 = Aggregate(..)(move _5) -//! 6. _8 = Call(..)(move _6) (assume _8 contains no refs/ptrs or generic params) +//! 6. _8 = Call(..)(move _6) //! 7. _9 = (_8.0) -//! 8. (_7.0) = _9 +//! 8. _10 = const 5 +//! 9. (_7.0) = move _10 //! ``` //! //! * `_5` is live from stmt 3 to stmt 5 //! * `_6` is live from stmt 4 to stmt 6 -//! * `_7` is a `Local` of kind `LocalWithRef` so needs to be taken into account in the -//! analyis. It's live from stmt 5 to stmt 8 +//! * `_7` is a `Local` of kind `LocalWithRefs` so needs to be taken into account in the +//! analyis. It's live from stmt 5 to stmt 9 +//! * `_8` is a `Local` of kind `LocalWithRefs`. It's live from 6. to 7. +//! * `_9` is a `Local` of kind `LocalWithRefs` (FIXME this is currently not done, see FIXME above), +//! it's live at 7. //! //! 3. Determining which `Local`s are borrowed //! @@ -103,7 +114,9 @@ //! `_5` (Borrow) -> `_3` (Local) //! `_6` (Borrow) -> `_4` (Local) //! `_7` (LocalWithRef) -> `_5` (Borrow) -//! `_7` (LocalWithRef) -> `_9` (Local) +//! `_8` (LocalWithRef) -> `_6` (Borrow) +//! `_9` (LocalWithRef) -> `_8` (LocalWithRef) (FIXME currently not done) +//! `_7` (LocalWithRef) -> `_10` (Local) //! //! So at each of those statements we have the following `Local`s that are live due to borrows: //! @@ -112,9 +125,10 @@ //! 3. {_3} //! 4. {_3, _4} //! 5. {_3, _4, _7} -//! 6. {_3, _4, _7} -//! 7. {_3, _7} +//! 6. {_3, _4, _7, _8} +//! 7. {_3, _4, _7, _8} //! 8. {_3, _7} +//! 9. {_3, _7} //! use super::*; @@ -134,8 +148,7 @@ use either::Either; #[derive(Copy, Clone, Debug)] enum NodeKind { - // An node corresponding to the place of the borrowed place (`_4` in this case) in - // an assignment like `_3 = Ref(_, _, _4)`. + // An node corresponding to the place on the lhs of an assignment like `_3 = Ref(_, _, _4)`. Borrow(Local), // Nodes corresponding to the place on the lhs of a statement like @@ -143,7 +156,8 @@ enum NodeKind { // where _3 and _6 are places corresponding to references or raw pointers. LocalWithRefs(Local), - // Nodes corresponding to the place on the lhs of an assignment like `_2 = Ref(..)`. + // Nodes corresponding to the borrowed place of an assignment like `_2 = Ref(_, _, borrowed_place)`, + // if `borrowed_place` is a non-ref or non-ptr value. Local(Local), } @@ -159,11 +173,25 @@ impl NodeKind { /// Used to build a dependency graph between borrows/pointers and the `Local`s that /// they reference. -/// We add edges to the graph in two kinds of situations: -/// * direct assignment of reference or raw pointer (e.g. `_4 = Ref(..)` or `_4 = AddressOf`) +/// We add edges to the graph in following situations: +/// * direct assignment of reference or raw pointer (e.g. `_4 = Ref(_, _ , borrowed_place)` or +/// `_4 = AddressOf(_, borrowed_place)`). For this case we create a `Node` of kind +/// `NodeKind::Borrow` for the `Local` being assigned to and an edge to either an existing +/// `Node` or if none exists yet to a new `Node` of type `NodeKind::Local` corresponding to +/// a non-ref/ptr `Local`. /// * assignments to non-reference or non-pointer `Local`s, which themselves might contain /// references or pointers (e.g. `_2 = Aggregate(Adt(..), _, _, _, _), [move _3, move _6])`, -/// where `_3` and `_6` are places corresponding to references or raw pointers). +/// where `_3` and `_6` are places corresponding to references or raw pointers). In this case +/// we create a `Node` of kind `NodeKind::LocalWithRefs` for `_2`. Since `_3` and `_6` are +/// `Local`s that correspond to references, pointers or composite types that might contain +/// references or pointers (`NodeKind::LocalWithRefs`), there already exist `Node`s for these +/// `Local`s. We then add edges from the `Node` for `_2` to both the `Node` for `_3` and the +/// `Node` for `_6`. +/// * `destination` places for `TerminatorKind::Call` and the `resume_arg` places for +/// `TerminatorKind::Yield` if we pass in any references, pointers or composite values that +/// might correspond to references, pointers or exposed pointers (`NodeKind::LocalWithRef`s). +/// The rationale for this is that the return values of both of these terminators might themselves +/// contain any of the references or pointers passed as arguments. struct BorrowDependencies<'a, 'tcx> { tcx: TyCtxt<'tcx>, local_decls: &'a LocalDecls<'tcx>, @@ -396,7 +424,13 @@ impl<'a, 'tcx> Visitor<'tcx> for BorrowDependencies<'a, 'tcx> { } pub struct BorrowedLocalsResults<'mir, 'tcx> { + // the results of the liveness analysis of `LiveBorrows` borrows_analysis_results: Results<'tcx, LiveBorrows<'mir, 'tcx>>, + + // Maps each `Local` that corresponds to a reference, pointer or a node of kind + // `NodeKind::LocalWithRefs` (i.e. `Local`s which either correspond to refs, pointers or + // exposed pointers or a composite value that might include refs, pointers or exposed pointers) + // to the set of `Local`s that are borrowed through those references, pointers or composite values. borrowed_local_to_locals_to_keep_alive: FxHashMap>, } @@ -473,6 +507,8 @@ where } } +/// The function gets the results of the borrowed locals analysis in this module. See the module +/// doc-comment for information on what exactly this analysis does. #[instrument(skip(tcx), level = "debug")] pub fn get_borrowed_locals_results<'mir, 'tcx>( body: &'mir Body<'tcx>, @@ -517,9 +553,19 @@ pub fn get_borrowed_locals_results<'mir, 'tcx>( BorrowedLocalsResults::new(results) } +/// The `ResultsCursor` equivalent for the borrowed locals analysis. Since this analysis doesn't +/// require convergence, we expose the set of borrowed `Local`s for a `Location` directly via +/// the `get` method without the need for any prior 'seek' calls. pub struct BorrowedLocalsResultsCursor<'a, 'mir, 'tcx> { body: &'mir Body<'tcx>, + + // The cursor for the liveness analysis performed by `LiveBorrows` borrows_analysis_cursor: ResultsRefCursor<'a, 'mir, 'tcx, LiveBorrows<'mir, 'tcx>>, + + // Maps each `Local` corresponding to a reference or pointer to the set of `Local`s + // that are borrowed through the ref/ptr. Additionally contains entries for `Local`s + // corresponding to `NodeKind::LocalWithRefs` since they might contain refs, ptrs or + // exposed pointers and need to be treated equivalently to refs/ptrs borrowed_local_to_locals_to_keep_alive: &'a FxHashMap>, } @@ -586,7 +632,9 @@ impl<'a, 'mir, 'tcx> BorrowedLocalsResultsCursor<'a, 'mir, 'tcx> { } } -/// Performs a liveness analysis for borrows and raw pointers. +/// Performs a liveness analysis for borrows and raw pointers. This analysis also tracks `Local`s +/// corresponding to `Node`s of kind `NodeKind::LocalWithRefs`, as these could potentially refer to +/// or include references, pointers or exposed pointers. pub struct LiveBorrows<'mir, 'tcx> { body: &'mir Body<'tcx>, tcx: TyCtxt<'tcx>, @@ -662,7 +710,7 @@ impl<'a, 'tcx> GenKillAnalysis<'tcx> for LiveBorrows<'a, 'tcx> { } } -/// A `Visitor` that defines the transfer function for `MaybeBorrowedLocals`. +/// A `Visitor` that defines the transfer function for `LiveBorrows`. struct TransferFunction<'a, 'b, 'c, 'tcx, T> { body: &'a Body<'tcx>, tcx: TyCtxt<'tcx>, From d2b84b065d1b19d049535d81a8b59c85d875348f Mon Sep 17 00:00:00 2001 From: b-naber Date: Wed, 7 Jun 2023 16:34:18 +0000 Subject: [PATCH 12/20] keep LocalWithRefs operands live in calls and yields --- .../src/impls/live_borrows.rs | 56 ++++++++++++++----- 1 file changed, 41 insertions(+), 15 deletions(-) diff --git a/compiler/rustc_mir_dataflow/src/impls/live_borrows.rs b/compiler/rustc_mir_dataflow/src/impls/live_borrows.rs index f86924f18f2e3..c7f744438fbf0 100644 --- a/compiler/rustc_mir_dataflow/src/impls/live_borrows.rs +++ b/compiler/rustc_mir_dataflow/src/impls/live_borrows.rs @@ -786,20 +786,7 @@ where debug!("gen {:?}", local); self._trans.gen(local); } - _ => { - if let Some(node_idx) = self.borrow_deps.locals_to_node_indexes.get(&local) { - let node = self.borrow_deps.dep_graph.node(*node_idx); - - // these are `Local`s that contain references/pointers or are raw pointers - // that were assigned to raw pointers, which were cast to usize. Hence we - // need to treat them as uses of the references/pointers that they - // refer/correspond to. - if let NodeKind::LocalWithRefs(_) = node.data { - debug!("gen {:?}", local); - self._trans.gen(local); - } - } - } + _ => {} } self.super_place(place, context, location); @@ -825,7 +812,26 @@ where self._trans.kill(destination.local); for arg in args { - self.visit_operand(arg, location) + match arg { + Operand::Copy(place) | Operand::Move(place) => { + if let Some(node_idx) = + self.borrow_deps.locals_to_node_indexes.get(&place.local) + { + let node = self.borrow_deps.dep_graph.node(*node_idx); + + // these are `Local`s that contain references/pointers or are raw pointers + // that were assigned to raw pointers, which were cast to usize. Since the + // function call is free to use these in any form, we need to gen them here. + if let NodeKind::LocalWithRefs(_) = node.data { + debug!("gen {:?}", place.local); + self._trans.gen(place.local); + } + } else { + self.super_operand(arg, location) + } + } + _ => {} + } } } _ => self.super_terminator(terminator, location), @@ -837,6 +843,26 @@ where debug!("killing {:?}", resume_arg.local); self._trans.kill(resume_arg.local); + match value { + Operand::Copy(place) | Operand::Move(place) => { + if let Some(node_idx) = + self.borrow_deps.locals_to_node_indexes.get(&place.local) + { + let node = self.borrow_deps.dep_graph.node(*node_idx); + + // these are `Local`s that contain references/pointers or are raw pointers + // that were assigned to raw pointers, which were cast to usize. Since the + // function call is free to use these in any form, we need to gen them here. + if let NodeKind::LocalWithRefs(_) = node.data { + debug!("gen {:?}", place.local); + self._trans.gen(place.local); + } + } else { + self.super_operand(value, location) + } + } + _ => {} + } self.visit_operand(value, location) } _ => self.super_terminator(terminator, location), From 2f7aa5d5fc2c63d14182554deca0491d7f99d776 Mon Sep 17 00:00:00 2001 From: b-naber Date: Wed, 7 Jun 2023 18:48:23 +0000 Subject: [PATCH 13/20] account for cycles in dfs, add edges from LocalWithRefs --- .../src/impls/live_borrows.rs | 123 +++++++++++------- 1 file changed, 79 insertions(+), 44 deletions(-) diff --git a/compiler/rustc_mir_dataflow/src/impls/live_borrows.rs b/compiler/rustc_mir_dataflow/src/impls/live_borrows.rs index c7f744438fbf0..304e144faa9ee 100644 --- a/compiler/rustc_mir_dataflow/src/impls/live_borrows.rs +++ b/compiler/rustc_mir_dataflow/src/impls/live_borrows.rs @@ -73,7 +73,7 @@ //! * `_5` (Borrow) -> `_4` (Local) //! * `_6` (LocalWithRefs) -> `_5` (Borrow) //! * `_7` (LocalWithRefs) -> `_6` (LocalWithRefs) -//! * `_8` (LocalWithRefs) -> `_7` (LocalWithRefs) (FIXME this one is currently not being done, but unsafe) +//! * `_8` (LocalWithRefs) -> `_7` (LocalWithRefs) //! //! We also have to be careful when dealing with `Terminator`s. Whenever we pass references, //! pointers or `Local`s with `NodeKind::LocalWithRefs` to a `TerminatorKind::Call` or @@ -104,7 +104,7 @@ //! * `_7` is a `Local` of kind `LocalWithRefs` so needs to be taken into account in the //! analyis. It's live from stmt 5 to stmt 9 //! * `_8` is a `Local` of kind `LocalWithRefs`. It's live from 6. to 7. -//! * `_9` is a `Local` of kind `LocalWithRefs` (FIXME this is currently not done, see FIXME above), +//! * `_9` is a `Local` of kind `LocalWithRefs` //! it's live at 7. //! //! 3. Determining which `Local`s are borrowed @@ -115,7 +115,7 @@ //! `_6` (Borrow) -> `_4` (Local) //! `_7` (LocalWithRef) -> `_5` (Borrow) //! `_8` (LocalWithRef) -> `_6` (Borrow) -//! `_9` (LocalWithRef) -> `_8` (LocalWithRef) (FIXME currently not done) +//! `_9` (LocalWithRef) -> `_8` (LocalWithRef) //! `_7` (LocalWithRef) -> `_10` (Local) //! //! So at each of those statements we have the following `Local`s that are live due to borrows: @@ -266,7 +266,10 @@ impl<'a, 'tcx> Visitor<'tcx> for BorrowDependencies<'a, 'tcx> { } StatementKind::FakeRead(..) | StatementKind::StorageDead(_) - | StatementKind::StorageLive(_) => {} + | StatementKind::StorageLive(_) + | StatementKind::AscribeUserType(..) + | StatementKind::Deinit(_) + | StatementKind::Coverage(_) => {} _ => { self.super_statement(statement, location); } @@ -289,7 +292,7 @@ impl<'a, 'tcx> Visitor<'tcx> for BorrowDependencies<'a, 'tcx> { // hence we want to treat them as `NodeKind::Borrow` // FIXME Are these always Operand::Copy or is Operand::Move also possible for refs/ptrs? let Some(src_local) = self.current_local else { - bug!("Expected self.current_local to be set with Rvalue::Ref|Rvalue::AddressOf"); + bug!("Expected self.current_local to be set when encountering Rvalue"); }; // These are just moves of refs/ptrs, hence `NodeKind::Borrow`. @@ -344,39 +347,57 @@ impl<'a, 'tcx> Visitor<'tcx> for BorrowDependencies<'a, 'tcx> { #[instrument(skip(self), level = "debug")] fn visit_place(&mut self, place: &Place<'tcx>, context: PlaceContext, location: Location) { - // Add edges for places that correspond to references or raw pointers + // Add edges for places that correspond to references/raw pointers or nodes of kind `LocalWithRefs` let place_ty = place.ty(self.local_decls, self.tcx).ty; debug!(?place_ty); debug!("current local: {:?}", self.current_local); - match place_ty.kind() { - ty::Ref(..) | ty::RawPtr(..) => match self.current_local { - Some(src_local) => { - // If we haven't created a node for this before, then this must be a - // `NodeKind::LocalWithRefs` as we would have handled the - // other possible assignment case (`NodeKind::Local`) previously in - // `visit_rvalue`. - let src_node_kind = NodeKind::LocalWithRefs(src_local); - let src_node_idx = self.maybe_create_node(src_node_kind); - - let borrowed_node_kind = NodeKind::Borrow(place.local); - let node_idx = self.maybe_create_node(borrowed_node_kind); - - debug!( - "adding edge from {:?}({:?}) -> {:?}({:?})", - src_node_idx, - self.dep_graph.node(src_node_idx).data, - node_idx, - place.local - ); - - self.dep_graph.add_edge(src_node_idx, node_idx, ()); + + match self.current_local { + Some(src_local) => { + match place_ty.kind() { + ty::Ref(..) | ty::RawPtr(..) => { + // If we haven't created a node for this before, then this must be a + // `NodeKind::LocalWithRefs` as we would have handled the + // other possible assignment case (`NodeKind::Local`) previously in + // `visit_rvalue`. + let src_node_kind = NodeKind::LocalWithRefs(src_local); + let src_node_idx = self.maybe_create_node(src_node_kind); + + let borrowed_node_kind = NodeKind::Borrow(place.local); + let node_idx = self.maybe_create_node(borrowed_node_kind); + + debug!( + "adding edge from {:?}({:?}) -> {:?}({:?})", + src_node_idx, + self.dep_graph.node(src_node_idx).data, + node_idx, + place.local + ); + + self.dep_graph.add_edge(src_node_idx, node_idx, ()); + } + _ => { + if let Some(node_idx) = self.locals_to_node_indexes.get(&place.local) { + // LocalsWithRefs -> LocalWithRefs + + let node_idx = *node_idx; + let src_node_kind = NodeKind::LocalWithRefs(src_local); + let src_node_idx = self.maybe_create_node(src_node_kind); + + debug!( + "adding edge from {:?}({:?}) -> {:?}({:?})", + src_node_idx, + self.dep_graph.node(src_node_idx).data, + node_idx, + self.dep_graph.node(node_idx).data, + ); + + self.dep_graph.add_edge(src_node_idx, node_idx, ()); + } + } } - None => {} - }, - _ => { - // FIXME I think we probably need to introduce edges here if `place.local` - // corresponds to a `NodeKind::LocalWithRefs` } + _ => {} } self.super_place(place, context, location) @@ -450,10 +471,17 @@ where dep_graph: &'a Graph, ) -> FxHashMap> { let mut borrows_to_locals: FxHashMap> = Default::default(); + let mut locals_visited = vec![]; + for (node_idx, node) in dep_graph.enumerated_nodes() { let current_local = node.data.get_local(); if borrows_to_locals.get(¤t_local).is_none() { - Self::dfs_for_node(node_idx, &mut borrows_to_locals, dep_graph); + Self::dfs_for_node( + node_idx, + &mut borrows_to_locals, + dep_graph, + &mut locals_visited, + ); } } @@ -461,14 +489,15 @@ where borrows_to_locals } - // FIXME Account for cycles in the graph! fn dfs_for_node( node_idx: NodeIndex, borrows_to_locals: &mut FxHashMap>, dep_graph: &Graph, + locals_visited: &mut Vec, ) -> Vec { let src_node = dep_graph.node(node_idx); let current_local = src_node.data.get_local(); + locals_visited.push(current_local); if let Some(locals_to_keep_alive) = borrows_to_locals.get(¤t_local) { // already traversed this node return (*locals_to_keep_alive).clone(); @@ -480,6 +509,10 @@ where num_succs += 1; let target_node_idx = edge.target(); let target_node = dep_graph.node(target_node_idx); + let target_local = target_node.data.get_local(); + if locals_visited.contains(&target_local) { + continue; + } debug!( "edge {:?} ({:?}) -> {:?} ({:?})", @@ -487,23 +520,25 @@ where ); let mut locals_to_keep_alive_for_succ = - Self::dfs_for_node(target_node_idx, borrows_to_locals, dep_graph); + Self::dfs_for_node(target_node_idx, borrows_to_locals, dep_graph, locals_visited); locals_for_node.append(&mut locals_to_keep_alive_for_succ); } - if num_succs == 0 { - // base node to keep alive - vec![src_node.data.get_local()] - } else { - if matches!(src_node.data, NodeKind::LocalWithRefs(_)) { + match src_node.data { + NodeKind::Local(_) => { + assert!(num_succs == 0, "Local node with successors"); + return vec![src_node.data.get_local()]; + } + NodeKind::LocalWithRefs(_) => { // These are locals that we need to keep alive, but that also contain // successors in the graph since they contain other references/pointers. locals_for_node.push(current_local); } - - borrows_to_locals.insert(current_local, locals_for_node.clone()); - locals_for_node + NodeKind::Borrow(_) => {} } + + borrows_to_locals.insert(current_local, locals_for_node.clone()); + locals_for_node } } From 65de22612a161417b50c42b1672c53905e324d52 Mon Sep 17 00:00:00 2001 From: b-naber Date: Thu, 8 Jun 2023 20:12:01 +0000 Subject: [PATCH 14/20] add edges between operands, use MaybeBorrowedLocals as upper bound --- .../src/impls/live_borrows.rs | 155 ++++++++++++++++-- .../src/impls/storage_liveness.rs | 2 +- compiler/rustc_mir_transform/src/generator.rs | 9 +- 3 files changed, 151 insertions(+), 15 deletions(-) diff --git a/compiler/rustc_mir_dataflow/src/impls/live_borrows.rs b/compiler/rustc_mir_dataflow/src/impls/live_borrows.rs index 304e144faa9ee..95ad52d504a87 100644 --- a/compiler/rustc_mir_dataflow/src/impls/live_borrows.rs +++ b/compiler/rustc_mir_dataflow/src/impls/live_borrows.rs @@ -134,6 +134,7 @@ use super::*; use crate::framework::{Analysis, Results, ResultsCursor}; +use crate::impls::MaybeBorrowedLocals; use crate::{ AnalysisDomain, Backward, CallReturnPlaces, GenKill, GenKillAnalysis, ResultsRefCursor, }; @@ -145,6 +146,7 @@ use rustc_middle::mir::*; use rustc_middle::ty; use either::Either; +use std::cell::RefCell; #[derive(Copy, Clone, Debug)] enum NodeKind { @@ -234,6 +236,68 @@ impl<'a, 'tcx> BorrowDependencies<'a, 'tcx> { node_idx } } + + /// Panics if `local` doesn't have a `Node` in `self.dep_graph`. + fn get_node_idx_for_local(&self, local: Local) -> NodeIndex { + let node_idx = self + .locals_to_node_indexes + .get(&local) + .unwrap_or_else(|| bug!("expected {:?} to have a Node", local)); + + *node_idx + } + + #[instrument(skip(self), level = "debug")] + fn local_is_ref_ptr_or_localwithrefs(&self, place: &Place<'tcx>) -> bool { + let place_ty = place.ty(self.local_decls, self.tcx).ty; + let is_ref_or_ptr = matches!(place_ty.kind(), ty::Ref(..) | ty::RawPtr(..)); + + // Also account for `LocalWithRef`s + let is_local_with_refs = + if let Some(node_idx) = self.locals_to_node_indexes.get(&place.local) { + let node_for_place = self.dep_graph.node(*node_idx); + debug!(?node_for_place.data); + matches!(node_for_place.data, NodeKind::LocalWithRefs(_)) + } else { + false + }; + + debug!(?is_ref_or_ptr, ?is_local_with_refs); + is_ref_or_ptr || is_local_with_refs + } + + #[instrument(skip(self), level = "debug")] + fn maybe_create_edges_for_operands(&mut self, args: &Vec>) { + for i in 0..args.len() { + let outer_operand = &args[i]; + debug!(?outer_operand); + match outer_operand { + Operand::Copy(outer_place) | Operand::Move(outer_place) => { + if self.local_is_ref_ptr_or_localwithrefs(outer_place) { + for j in i + 1..args.len() { + let inner_operand = &args[j]; + debug!(?inner_operand); + match inner_operand { + Operand::Copy(inner_place) | Operand::Move(inner_place) => { + if self.local_is_ref_ptr_or_localwithrefs(inner_place) { + let node_idx_outer = + self.get_node_idx_for_local(outer_place.local); + let node_idx_inner = + self.get_node_idx_for_local(inner_place.local); + + self.dep_graph.add_edge(node_idx_outer, node_idx_inner, ()); + self.dep_graph.add_edge(node_idx_inner, node_idx_outer, ()); + } + } + Operand::Constant(_) => {} + } + } + } + } + Operand::Constant(_) => {} + } + } + } } impl<'a, 'tcx> Visitor<'tcx> for BorrowDependencies<'a, 'tcx> { @@ -427,6 +491,10 @@ impl<'a, 'tcx> Visitor<'tcx> for BorrowDependencies<'a, 'tcx> { self.visit_operand(arg, location); } + // Additionally we have to introduce edges between borrowed operands, since we could + // mutate those in the call (either through mutable references or interior mutability) + self.maybe_create_edges_for_operands(args); + self.current_local = None; } TerminatorKind::Yield { resume_arg, value, .. } => { @@ -444,7 +512,7 @@ impl<'a, 'tcx> Visitor<'tcx> for BorrowDependencies<'a, 'tcx> { } } -pub struct BorrowedLocalsResults<'mir, 'tcx> { +pub struct BorrowedLocalsResults<'a, 'mir, 'tcx> { // the results of the liveness analysis of `LiveBorrows` borrows_analysis_results: Results<'tcx, LiveBorrows<'mir, 'tcx>>, @@ -453,28 +521,49 @@ pub struct BorrowedLocalsResults<'mir, 'tcx> { // exposed pointers or a composite value that might include refs, pointers or exposed pointers) // to the set of `Local`s that are borrowed through those references, pointers or composite values. borrowed_local_to_locals_to_keep_alive: FxHashMap>, + + maybe_borrowed_locals_results_cursor: RefCell< + ResultsCursor<'mir, 'tcx, MaybeBorrowedLocals, &'a Results<'tcx, MaybeBorrowedLocals>>, + >, } -impl<'mir, 'tcx> BorrowedLocalsResults<'mir, 'tcx> +impl<'a, 'mir, 'tcx> BorrowedLocalsResults<'a, 'mir, 'tcx> where 'tcx: 'mir, + 'tcx: 'a, { - fn new(borrows_analysis_results: Results<'tcx, LiveBorrows<'mir, 'tcx>>) -> Self { + fn new( + borrows_analysis_results: Results<'tcx, LiveBorrows<'mir, 'tcx>>, + maybe_borrowed_locals_results_cursor: ResultsCursor< + 'mir, + 'tcx, + MaybeBorrowedLocals, + &'a Results<'tcx, MaybeBorrowedLocals>, + >, + ) -> Self { let dep_graph = &borrows_analysis_results.analysis.borrow_deps.dep_graph; let borrowed_local_to_locals_to_keep_alive = Self::get_locals_to_keep_alive_map(dep_graph); - Self { borrows_analysis_results, borrowed_local_to_locals_to_keep_alive } + Self { + borrows_analysis_results, + borrowed_local_to_locals_to_keep_alive, + maybe_borrowed_locals_results_cursor: RefCell::new( + maybe_borrowed_locals_results_cursor, + ), + } } /// Uses the dependency graph to find all locals that we need to keep live for a given /// `Node` (or more specically the `Local` corresponding to that `Node`). - fn get_locals_to_keep_alive_map<'a>( - dep_graph: &'a Graph, + #[instrument(skip(dep_graph), level = "debug")] + fn get_locals_to_keep_alive_map<'b>( + dep_graph: &'b Graph, ) -> FxHashMap> { let mut borrows_to_locals: FxHashMap> = Default::default(); let mut locals_visited = vec![]; for (node_idx, node) in dep_graph.enumerated_nodes() { let current_local = node.data.get_local(); + debug!(?current_local); if borrows_to_locals.get(¤t_local).is_none() { Self::dfs_for_node( node_idx, @@ -489,6 +578,7 @@ where borrows_to_locals } + #[instrument(skip(dep_graph), level = "debug")] fn dfs_for_node( node_idx: NodeIndex, borrows_to_locals: &mut FxHashMap>, @@ -499,6 +589,7 @@ where let current_local = src_node.data.get_local(); locals_visited.push(current_local); if let Some(locals_to_keep_alive) = borrows_to_locals.get(¤t_local) { + debug!("already prev. calculated: {:?}", locals_to_keep_alive); // already traversed this node return (*locals_to_keep_alive).clone(); } @@ -510,7 +601,17 @@ where let target_node_idx = edge.target(); let target_node = dep_graph.node(target_node_idx); let target_local = target_node.data.get_local(); + + // necessary to prevent loops if locals_visited.contains(&target_local) { + if let Some(locals_to_keep_alive) = borrows_to_locals.get(&target_local) { + debug!( + "prev. calculated locals to keep alive for {:?}: {:?}", + target_local, locals_to_keep_alive + ); + locals_for_node.append(&mut locals_to_keep_alive.clone()); + } + continue; } @@ -537,6 +638,7 @@ where NodeKind::Borrow(_) => {} } + debug!("locals for {:?}: {:?}", current_local, locals_for_node); borrows_to_locals.insert(current_local, locals_for_node.clone()); locals_for_node } @@ -544,11 +646,17 @@ where /// The function gets the results of the borrowed locals analysis in this module. See the module /// doc-comment for information on what exactly this analysis does. -#[instrument(skip(tcx), level = "debug")] -pub fn get_borrowed_locals_results<'mir, 'tcx>( +#[instrument(skip(tcx, maybe_borrowed_locals_cursor, body), level = "debug")] +pub fn get_borrowed_locals_results<'a, 'mir, 'tcx>( body: &'mir Body<'tcx>, tcx: TyCtxt<'tcx>, -) -> BorrowedLocalsResults<'mir, 'tcx> { + maybe_borrowed_locals_cursor: ResultsCursor< + 'mir, + 'tcx, + MaybeBorrowedLocals, + &'a Results<'tcx, MaybeBorrowedLocals>, + >, +) -> BorrowedLocalsResults<'a, 'mir, 'tcx> { debug!("body: {:#?}", body); let mut borrow_deps = BorrowDependencies::new(body.local_decls(), tcx); @@ -585,7 +693,7 @@ pub fn get_borrowed_locals_results<'mir, 'tcx>( let results = live_borrows.into_engine(tcx, body).pass_name("borrowed_locals").iterate_to_fixpoint(); - BorrowedLocalsResults::new(results) + BorrowedLocalsResults::new(results, maybe_borrowed_locals_cursor) } /// The `ResultsCursor` equivalent for the borrowed locals analysis. Since this analysis doesn't @@ -602,10 +710,15 @@ pub struct BorrowedLocalsResultsCursor<'a, 'mir, 'tcx> { // corresponding to `NodeKind::LocalWithRefs` since they might contain refs, ptrs or // exposed pointers and need to be treated equivalently to refs/ptrs borrowed_local_to_locals_to_keep_alive: &'a FxHashMap>, + + // the cursor of the conservative borrowed locals analysis + maybe_borrowed_locals_results_cursor: &'a RefCell< + ResultsCursor<'mir, 'tcx, MaybeBorrowedLocals, &'a Results<'tcx, MaybeBorrowedLocals>>, + >, } impl<'a, 'mir, 'tcx> BorrowedLocalsResultsCursor<'a, 'mir, 'tcx> { - pub fn new(body: &'mir Body<'tcx>, results: &'a BorrowedLocalsResults<'mir, 'tcx>) -> Self { + pub fn new(body: &'mir Body<'tcx>, results: &'a BorrowedLocalsResults<'a, 'mir, 'tcx>) -> Self { let mut cursor = ResultsCursor::new(body, &results.borrows_analysis_results); // We don't care about the order of the blocks, only about the result at a given location. @@ -617,6 +730,7 @@ impl<'a, 'mir, 'tcx> BorrowedLocalsResultsCursor<'a, 'mir, 'tcx> { body, borrows_analysis_cursor: cursor, borrowed_local_to_locals_to_keep_alive: &results.borrowed_local_to_locals_to_keep_alive, + maybe_borrowed_locals_results_cursor: &results.maybe_borrowed_locals_results_cursor, } } @@ -662,6 +776,16 @@ impl<'a, 'mir, 'tcx> BorrowedLocalsResultsCursor<'a, 'mir, 'tcx> { Either::Left(_) => {} } + // use results of conservative analysis as an "upper bound" on the borrowed locals. This + // is necessary since to guarantee soundness for this analysis requires us to be more conservative + // in some cases than the analysis performed by `MaybeBorrowedLocals`. + let mut maybe_borrowed_locals_cursor = + self.maybe_borrowed_locals_results_cursor.borrow_mut(); + maybe_borrowed_locals_cursor.allow_unreachable(); + maybe_borrowed_locals_cursor.seek_before_primary_effect(loc); + let upper_bound_borrowed_locals = maybe_borrowed_locals_cursor.get(); + borrowed_locals.intersect(upper_bound_borrowed_locals); + debug!(?borrowed_locals); borrowed_locals } @@ -821,7 +945,14 @@ where debug!("gen {:?}", local); self._trans.gen(local); } - _ => {} + _ => { + if let Some(node_idx) = self.borrow_deps.locals_to_node_indexes.get(&local) { + let node = self.borrow_deps.dep_graph.node(*node_idx); + if matches!(node.data, NodeKind::LocalWithRefs(_)) { + self._trans.gen(local); + } + } + } } self.super_place(place, context, location); diff --git a/compiler/rustc_mir_dataflow/src/impls/storage_liveness.rs b/compiler/rustc_mir_dataflow/src/impls/storage_liveness.rs index 89eb7fa8a81b7..90e13187a38b0 100644 --- a/compiler/rustc_mir_dataflow/src/impls/storage_liveness.rs +++ b/compiler/rustc_mir_dataflow/src/impls/storage_liveness.rs @@ -157,7 +157,7 @@ pub struct MaybeRequiresStorage<'a, 'mir, 'tcx> { impl<'a, 'mir, 'tcx> MaybeRequiresStorage<'a, 'mir, 'tcx> { pub fn new( body: &'mir Body<'tcx>, - borrowed_locals: &'a BorrowedLocalsResults<'mir, 'tcx>, + borrowed_locals: &'a BorrowedLocalsResults<'a, 'mir, 'tcx>, ) -> Self { MaybeRequiresStorage { body, diff --git a/compiler/rustc_mir_transform/src/generator.rs b/compiler/rustc_mir_transform/src/generator.rs index 7e39acca52ce0..8e9955635499e 100644 --- a/compiler/rustc_mir_transform/src/generator.rs +++ b/compiler/rustc_mir_transform/src/generator.rs @@ -67,7 +67,7 @@ use rustc_middle::mir::*; use rustc_middle::ty::{self, AdtDef, Ty, TyCtxt}; use rustc_middle::ty::{GeneratorSubsts, SubstsRef}; use rustc_mir_dataflow::impls::{ - get_borrowed_locals_results, BorrowedLocalsResultsCursor, MaybeLiveLocals, + get_borrowed_locals_results, BorrowedLocalsResultsCursor, MaybeBorrowedLocals, MaybeLiveLocals, MaybeRequiresStorage, MaybeStorageLive, }; use rustc_mir_dataflow::storage::always_storage_live_locals; @@ -593,8 +593,13 @@ fn locals_live_across_suspend_points<'tcx>( .iterate_to_fixpoint() .into_results_cursor(body_ref); + let borrowed_locals_results = + MaybeBorrowedLocals.into_engine(tcx, body_ref).pass_name("generator").iterate_to_fixpoint(); + let borrowed_locals_cursor = + rustc_mir_dataflow::ResultsCursor::new(body_ref, &borrowed_locals_results); + // Calculate the locals that are live due to outstanding references or pointers. - let live_borrows_results = get_borrowed_locals_results(body_ref, tcx); + let live_borrows_results = get_borrowed_locals_results(body_ref, tcx, borrowed_locals_cursor); let mut live_borrows_cursor = BorrowedLocalsResultsCursor::new(body_ref, &live_borrows_results); // Calculate the MIR locals that we actually need to keep storage around From b679a03ade8e5abf69f34043e74c42f003f065a1 Mon Sep 17 00:00:00 2001 From: b-naber Date: Mon, 12 Jun 2023 12:17:20 +0000 Subject: [PATCH 15/20] use singly connected components in analysis --- .../src/graph/implementation/mod.rs | 13 +- .../src/impls/live_borrows.rs | 386 ++++++++++++++---- 2 files changed, 316 insertions(+), 83 deletions(-) diff --git a/compiler/rustc_data_structures/src/graph/implementation/mod.rs b/compiler/rustc_data_structures/src/graph/implementation/mod.rs index 9ff401c3c7aad..c87c67917b63f 100644 --- a/compiler/rustc_data_structures/src/graph/implementation/mod.rs +++ b/compiler/rustc_data_structures/src/graph/implementation/mod.rs @@ -22,6 +22,7 @@ use crate::snapshot_vec::{SnapshotVec, SnapshotVecDelegate}; use rustc_index::bit_set::BitSet; +use rustc_index::Idx; use std::fmt::Debug; #[cfg(test)] @@ -59,9 +60,19 @@ impl SnapshotVecDelegate for Edge { fn reverse(_: &mut Vec>, _: ()) {} } -#[derive(Copy, Clone, PartialEq, Debug)] +#[derive(Copy, Clone, PartialEq, Eq, Hash, Debug, Ord, PartialOrd)] pub struct NodeIndex(pub usize); +impl Idx for NodeIndex { + fn new(idx: usize) -> Self { + NodeIndex(idx) + } + + fn index(self) -> usize { + self.0 + } +} + #[derive(Copy, Clone, PartialEq, Debug)] pub struct EdgeIndex(pub usize); diff --git a/compiler/rustc_mir_dataflow/src/impls/live_borrows.rs b/compiler/rustc_mir_dataflow/src/impls/live_borrows.rs index 95ad52d504a87..6e0bc412431b1 100644 --- a/compiler/rustc_mir_dataflow/src/impls/live_borrows.rs +++ b/compiler/rustc_mir_dataflow/src/impls/live_borrows.rs @@ -139,7 +139,9 @@ use crate::{ AnalysisDomain, Backward, CallReturnPlaces, GenKill, GenKillAnalysis, ResultsRefCursor, }; use rustc_data_structures::fx::FxHashMap; +use rustc_data_structures::graph; use rustc_data_structures::graph::implementation::{Graph, NodeIndex}; +use rustc_data_structures::graph::scc::Sccs; use rustc_middle::mir::visit::PlaceContext; use rustc_middle::mir::visit::Visitor; use rustc_middle::mir::*; @@ -147,6 +149,7 @@ use rustc_middle::ty; use either::Either; use std::cell::RefCell; +use std::ops::{Deref, DerefMut}; #[derive(Copy, Clone, Debug)] enum NodeKind { @@ -173,6 +176,48 @@ impl NodeKind { } } +struct BorrowDepGraph { + graph: Graph, + + // Maps `Local`s, for which we have nodes in the graph, to the `NodeIndex`es of those nodes. + locals_to_node_indexes: FxHashMap, +} + +impl Deref for BorrowDepGraph { + type Target = Graph; + + fn deref(&self) -> &Self::Target { + &self.graph + } +} + +impl DerefMut for BorrowDepGraph { + fn deref_mut(&mut self) -> &mut Self::Target { + &mut self.graph + } +} + +impl graph::DirectedGraph for BorrowDepGraph { + type Node = NodeIndex; +} + +impl graph::WithNumNodes for BorrowDepGraph { + fn num_nodes(&self) -> usize { + return self.len_nodes(); + } +} + +impl graph::WithSuccessors for BorrowDepGraph { + fn successors(&self, node: Self::Node) -> >::Iter { + Box::new(self.successor_nodes(node)) + } +} + +impl<'a> graph::GraphSuccessors<'a> for BorrowDepGraph { + type Item = NodeIndex; + type Iter = Box + 'a>; +} + /// Used to build a dependency graph between borrows/pointers and the `Local`s that /// they reference. /// We add edges to the graph in following situations: @@ -198,17 +243,18 @@ struct BorrowDependencies<'a, 'tcx> { tcx: TyCtxt<'tcx>, local_decls: &'a LocalDecls<'tcx>, - // Maps `Local`s, for which we have nodes in the graph, to the `NodeIndex`es of those nodes. - locals_to_node_indexes: FxHashMap, - // Tracks the dependencies of places and the references/pointers they may contain, // e.g. if we have `_3 = Ref(_, _, _2)` we add an edge from _3 to _2. We later use // this graph to allow us to infer which locals need to be kept live in the // liveness analysis. - dep_graph: Graph, + dep_graph: BorrowDepGraph, // Contains the `Local` to which we're currently assigning. current_local: Option, + + // Maps locals that correspond to re-borrows to the borrow that was re-borrowed, + // so for `_4 = &(*_3)` we include `_4 -> _3` in `reborrows_map`. + reborrows_map: FxHashMap, } impl<'a, 'tcx> BorrowDependencies<'a, 'tcx> { @@ -220,31 +266,51 @@ impl<'a, 'tcx> BorrowDependencies<'a, 'tcx> { BorrowDependencies { tcx, local_decls, - dep_graph: Graph::with_capacity(num_nodes, approx_num_edges), + dep_graph: BorrowDepGraph { + locals_to_node_indexes: Default::default(), + graph: Graph::with_capacity(num_nodes, approx_num_edges), + }, current_local: None, - locals_to_node_indexes: Default::default(), + reborrows_map: Default::default(), } } fn maybe_create_node(&mut self, node_kind: NodeKind) -> NodeIndex { let local = node_kind.get_local(); - if let Some(node_idx) = self.locals_to_node_indexes.get(&local) { + if let Some(node_idx) = self.dep_graph.locals_to_node_indexes.get(&local) { *node_idx } else { - let node_idx = self.dep_graph.add_node(node_kind); - self.locals_to_node_indexes.insert(local, node_idx); - node_idx + if let Some(reborrowed_local) = self.reborrows_map.get(&local) { + let Some(node_idx) = self.dep_graph.locals_to_node_indexes.get(&reborrowed_local) else { + bug!("reborrowed local should have a node ({:?})", reborrowed_local); + }; + + *node_idx + } else { + let node_idx = self.dep_graph.add_node(node_kind); + debug!( + "inserting {:?} into locals_to_node_indexes, node idx: {:?}", + local, node_idx + ); + self.dep_graph.locals_to_node_indexes.insert(local, node_idx); + node_idx + } } } /// Panics if `local` doesn't have a `Node` in `self.dep_graph`. fn get_node_idx_for_local(&self, local: Local) -> NodeIndex { - let node_idx = self - .locals_to_node_indexes - .get(&local) - .unwrap_or_else(|| bug!("expected {:?} to have a Node", local)); - - *node_idx + if let Some(reborrowed_local) = self.reborrows_map.get(&local) { + *self.dep_graph.locals_to_node_indexes.get(reborrowed_local).unwrap_or_else(|| { + bug!("should have created a Node for re-borrowed Local {:?}", reborrowed_local) + }) + } else { + *self + .dep_graph + .locals_to_node_indexes + .get(&local) + .unwrap_or_else(|| bug!("expected {:?} to have a Node", local)) + } } #[instrument(skip(self), level = "debug")] @@ -254,7 +320,7 @@ impl<'a, 'tcx> BorrowDependencies<'a, 'tcx> { // Also account for `LocalWithRef`s let is_local_with_refs = - if let Some(node_idx) = self.locals_to_node_indexes.get(&place.local) { + if let Some(node_idx) = self.dep_graph.locals_to_node_indexes.get(&place.local) { let node_for_place = self.dep_graph.node(*node_idx); debug!(?node_for_place.data); matches!(node_for_place.data, NodeKind::LocalWithRefs(_)) @@ -308,7 +374,7 @@ impl<'a, 'tcx> Visitor<'tcx> for BorrowDependencies<'a, 'tcx> { let local = Local::from_usize(i); let node_kind = NodeKind::Local(local); let node_idx = self.dep_graph.add_node(node_kind); - self.locals_to_node_indexes.insert(local, node_idx); + self.dep_graph.locals_to_node_indexes.insert(local, node_idx); } self.super_body(body) @@ -346,44 +412,141 @@ impl<'a, 'tcx> Visitor<'tcx> for BorrowDependencies<'a, 'tcx> { fn visit_rvalue(&mut self, rvalue: &Rvalue<'tcx>, location: Location) { debug!("self.current_local: {:?}", self.current_local); match rvalue { - Rvalue::Use(Operand::Move(place) | Operand::Copy(place)) + Rvalue::Use(Operand::Move(place) | Operand::Copy(place)) => { if matches!( place.ty(self.local_decls, self.tcx).ty.kind(), ty::Ref(..) | ty::RawPtr(..) - ) => - { - // these are just re-assignments of already outstanding refs or pointers, - // hence we want to treat them as `NodeKind::Borrow` - // FIXME Are these always Operand::Copy or is Operand::Move also possible for refs/ptrs? - let Some(src_local) = self.current_local else { - bug!("Expected self.current_local to be set when encountering Rvalue"); - }; + ) { + // these are just re-assignments of already outstanding refs or pointers, + // hence we want to treat them as `NodeKind::Borrow` + // FIXME Are these always Operand::Copy or is Operand::Move also possible for refs/ptrs? + let Some(src_local) = self.current_local else { + bug!("Expected self.current_local to be set when encountering Rvalue"); + }; - // These are just moves of refs/ptrs, hence `NodeKind::Borrow`. - let src_node_kind = NodeKind::Borrow(src_local); - let src_node_idx = self.maybe_create_node(src_node_kind); + // These are just moves of refs/ptrs, hence `NodeKind::Borrow`. + let src_node_kind = NodeKind::Borrow(src_local); + let src_node_idx = self.maybe_create_node(src_node_kind); - let node_kind = NodeKind::Borrow(place.local); - let node_idx = self.maybe_create_node(node_kind); + let node_kind = NodeKind::Borrow(place.local); + let node_idx = self.maybe_create_node(node_kind); - debug!( - "adding edge from {:?}({:?}) -> {:?}({:?})", - src_node_idx, - self.dep_graph.node(src_node_idx).data, - node_idx, - self.dep_graph.node(node_idx).data, - ); + debug!( + "adding edge from {:?}({:?}) -> {:?}({:?})", + src_node_idx, + self.dep_graph.node(src_node_idx).data, + node_idx, + self.dep_graph.node(node_idx).data, + ); - self.dep_graph.add_edge(src_node_idx, node_idx, ()); + self.dep_graph.add_edge(src_node_idx, node_idx, ()); + self.dep_graph.add_edge(node_idx, src_node_idx, ()); + } else { + // Don't introduce edges for moved/copied `Local`s that correspond to `NodeKind::Local` + if let Some(node_idx) = self.dep_graph.locals_to_node_indexes.get(&place.local) + { + if matches!(self.dep_graph.node(*node_idx).data, NodeKind::Local(_)) { + return; + } + } else { + return; + } + + self.super_rvalue(rvalue, location); + } } Rvalue::Ref(_, _, borrowed_place) | Rvalue::AddressOf(_, borrowed_place) => { let Some(src_local) = self.current_local else { bug!("Expected self.current_local to be set with Rvalue::Ref|Rvalue::AddressOf"); }; - // we're in a statement like `_4 = Ref(..)`, hence NodeKind::Borrow for `_4` - let src_node_kind = NodeKind::Borrow(src_local); - let src_node_idx = self.maybe_create_node(src_node_kind); + let src_node_idx = if borrowed_place.is_indirect() { + // Don't introduce new nodes for re-borrows. We need to treat Re-borrows the same as the original + // borrow. The reason for this is that we could e.g. use interior mutability on a re-borrow + // in a function call (which would require us to add an edge between the borrow and whatever is + // inserted (e.g. a `LocalWithRefs`)), but we then later need to also have this edge on the local + // that corresponds to the borrow that we re-borrowed. To make this more clear, let's say we have + // something like this: + // + // struct WithInteriorMut { + // a: RefCell, + // } + // + // fn switch_interior_mut<'a>(raw_ref_exposed: usize, int_mut: &'a WithInteriorMut) { + // let mut mut_ref = int_mut.a.borrow_mut(); + // *mut_ref = raw_ref_exposed; + // } + // + // fn gen() -> impl Generator { + // static move || { + // let x = Foo { a: 11 }; + // let p = &x as *const Foo; + // let exposed_p = p as usize; + // let int_mut = WithInteriorMut { a: RefCell::new(exposed_p) }; + // let x2 = Foo { a: 13 }; + // let p2 = &x2 as *const Foo; + // let exposed_p2 = p2 as usize; + // + // yield 12; + // + // switch_interior_mut(exposed_p2, &int_mut); + // + // yield 15; + // + // let int_mut_back = int_mut.a.borrow(); + // let ref_to_foo2 = unsafe { &*(*int_mut_back as *const Foo) }; + // } + // } + // + // with MIR that looks something like this (simplified): + // + // _3 = Foo { a: const 11_usize }, + // _5 = &_3, + // _4 = &raw const (*_5), + // _7 = _4 + // _6 = move _7 as usize (PointerExposeAddress) + // _10 = _6 + // Terminator(Call, kind: _9 = RefCell::::new(move _10)) + // _8 = WithInteriorMut { a: move _9 } + // _11 = Foo { a: const 13_usize} + // _13 = &_11 + // _12 = &raw const (*13) + // _15 = _12 + // _14 = move _15 as usize (PointerExposeAddress) + // Terminator(Yield, _16 = yield(const 12_u32)) + // _18 = _14 + // _20 = &8 + // _19 = &(*_20) + // Terminator(Call, _17 = switch_interior_mut(move _18, move _19)) + // Terminator(Yield, _21 = yield(const 15_u32)) + // _23 = &(_8.0) + // _22 = RefCell::::borrow(move _23) + // _28 = &_22 + // Terminator(Call, _27 = as Deref>::deref(move _28)) + // _26 = (*_27) + // _25 = move _26 as *const Foo (PointerFromExposedAddress) + // _24 = &(*_25) + // + // We need to keep `_11` alive across the second suspension point (yield Terminator). To enable + // us to do this, we introduce edges between `_18` and `_19` (the call operands in the + // `switch_interior_mut` call). Note that + + if let Some(_) = + self.dep_graph.locals_to_node_indexes.get(&borrowed_place.local) + { + self.reborrows_map.insert(src_local, borrowed_place.local); + + return; + } else { + // we're in a statement like `_4 = Ref(..)`, hence NodeKind::Borrow for `_4` + let src_node_kind = NodeKind::Borrow(src_local); + self.maybe_create_node(src_node_kind) + } + } else { + // we're in a statement like `_4 = Ref(..)`, hence NodeKind::Borrow for `_4` + let src_node_kind = NodeKind::Borrow(src_local); + self.maybe_create_node(src_node_kind) + }; // If we haven't previously added a node for `borrowed_place.local` then it can be neither // `NodeKind::Borrow` nor `NodeKind::LocalsWithRefs`. @@ -399,6 +562,7 @@ impl<'a, 'tcx> Visitor<'tcx> for BorrowDependencies<'a, 'tcx> { ); self.dep_graph.add_edge(src_node_idx, node_idx, ()); + self.dep_graph.add_edge(node_idx, src_node_idx, ()); } Rvalue::Cast(..) => { // FIXME we probably should handle pointer casts here directly @@ -439,9 +603,14 @@ impl<'a, 'tcx> Visitor<'tcx> for BorrowDependencies<'a, 'tcx> { ); self.dep_graph.add_edge(src_node_idx, node_idx, ()); + + // FIXME not sure whether these are correct + self.dep_graph.add_edge(node_idx, src_node_idx, ()); } _ => { - if let Some(node_idx) = self.locals_to_node_indexes.get(&place.local) { + if let Some(node_idx) = + self.dep_graph.locals_to_node_indexes.get(&place.local) + { // LocalsWithRefs -> LocalWithRefs let node_idx = *node_idx; @@ -457,6 +626,9 @@ impl<'a, 'tcx> Visitor<'tcx> for BorrowDependencies<'a, 'tcx> { ); self.dep_graph.add_edge(src_node_idx, node_idx, ()); + + // FIXME not sure whether these are correct + self.dep_graph.add_edge(node_idx, src_node_idx, ()); } } } @@ -484,6 +656,8 @@ impl<'a, 'tcx> Visitor<'tcx> for BorrowDependencies<'a, 'tcx> { // is that the function could include those refs/ptrs in its return value. It's not sufficient // to look for the existence of `ty::Ref` or `ty::RawPtr` in the type of the return type, since the // function could also cast pointers to integers e.g. . + // FIMXE: I don't think we really have to create edges from the destination place + // to the operands. Interior mutability isn't obviously a problem here. self.current_local = Some(destination.local); self.visit_operand(func, location); @@ -556,21 +730,41 @@ where /// `Node` (or more specically the `Local` corresponding to that `Node`). #[instrument(skip(dep_graph), level = "debug")] fn get_locals_to_keep_alive_map<'b>( - dep_graph: &'b Graph, + dep_graph: &'b BorrowDepGraph, ) -> FxHashMap> { let mut borrows_to_locals: FxHashMap> = Default::default(); - let mut locals_visited = vec![]; - for (node_idx, node) in dep_graph.enumerated_nodes() { - let current_local = node.data.get_local(); - debug!(?current_local); - if borrows_to_locals.get(¤t_local).is_none() { - Self::dfs_for_node( - node_idx, - &mut borrows_to_locals, - dep_graph, - &mut locals_visited, - ); + // create SCCs for dependency graph and map each local to its SCC. + let sccs: Sccs = Sccs::new(dep_graph); + let mut components: FxHashMap> = Default::default(); + let mut local_to_component: FxHashMap = Default::default(); + for (node_idx, scc_idx) in sccs.scc_indices().iter().enumerate() { + let node = dep_graph.node(NodeIndex(node_idx)); + let local = node.data.get_local(); + components.entry(scc_idx.0).or_default().push(local); + local_to_component.insert(local, scc_idx.0); + } + + debug!("components: {:#?}", components); + debug!("local_to_component: {:#?}", local_to_component); + + for (_, node) in dep_graph.enumerated_nodes() { + if matches!(node.data, NodeKind::Borrow(_) | NodeKind::LocalWithRefs(_)) { + let current_local = node.data.get_local(); + let scc = local_to_component + .get(¤t_local) + .unwrap_or_else(|| bug!("{:?} should have a component", current_local)); + + // add all locals that we need to keep alive for a given Borrow/LocalWithRefs (these are all + // `Local`s or `LocalWithRef`s in the SCC) + for local_in_scc in &components[scc] { + if let Some(node_idx) = dep_graph.locals_to_node_indexes.get(local_in_scc) { + let node = dep_graph.node(*node_idx); + if matches!(node.data, NodeKind::Local(_) | NodeKind::LocalWithRefs(_)) { + borrows_to_locals.entry(current_local).or_default().push(*local_in_scc); + } + } + } } } @@ -594,10 +788,8 @@ where return (*locals_to_keep_alive).clone(); } - let mut num_succs = 0; let mut locals_for_node = vec![]; for (_, edge) in dep_graph.outgoing_edges(node_idx) { - num_succs += 1; let target_node_idx = edge.target(); let target_node = dep_graph.node(target_node_idx); let target_local = target_node.data.get_local(); @@ -627,8 +819,7 @@ where match src_node.data { NodeKind::Local(_) => { - assert!(num_succs == 0, "Local node with successors"); - return vec![src_node.data.get_local()]; + locals_for_node.push(current_local); } NodeKind::LocalWithRefs(_) => { // These are locals that we need to keep alive, but that also contain @@ -877,6 +1068,33 @@ struct TransferFunction<'a, 'b, 'c, 'tcx, T> { borrow_deps: &'c BorrowDependencies<'a, 'tcx>, } +impl<'a, 'b, 'c, 'tcx, T> TransferFunction<'a, 'b, 'c, 'tcx, T> +where + T: GenKill, +{ + fn gen(&mut self, local: Local) { + debug!("gen {:?}", local); + if let Some(reborrowed_local) = self.borrow_deps.reborrows_map.get(&local) { + self._trans.gen(*reborrowed_local); + } else { + if self.borrow_deps.dep_graph.locals_to_node_indexes.get(&local).is_some() { + self._trans.gen(local) + } + } + } + + fn kill(&mut self, local: Local) { + debug!("killing {:?}", local); + if let Some(reborrowed_local) = self.borrow_deps.reborrows_map.get(&local) { + self._trans.kill(*reborrowed_local); + } else { + if self.borrow_deps.dep_graph.locals_to_node_indexes.get(&local).is_some() { + self._trans.kill(local) + } + } + } +} + impl<'a, 'tcx, T> Visitor<'tcx> for TransferFunction<'a, '_, '_, 'tcx, T> where T: GenKill, @@ -896,19 +1114,20 @@ where // kill the local. match lhs_place_ty.kind() { ty::Ref(..) | ty::RawPtr(..) => { - debug!("killing {:?}", lhs_place.local); - self._trans.kill(lhs_place.local); + self.kill(lhs_place.local); self.visit_rvalue(&assign.1, location); } _ => { - if let Some(node_idx) = - self.borrow_deps.locals_to_node_indexes.get(&lhs_place.local) + if let Some(node_idx) = self + .borrow_deps + .dep_graph + .locals_to_node_indexes + .get(&lhs_place.local) { let node = self.borrow_deps.dep_graph.node(*node_idx); if let NodeKind::LocalWithRefs(_) = node.data { - debug!("killing {:?}", lhs_place.local); - self._trans.kill(lhs_place.local); + self.kill(lhs_place.local); } } self.super_assign(&assign.0, &assign.1, location); @@ -942,14 +1161,15 @@ where match local_ty.kind() { ty::Ref(..) | ty::RawPtr(..) => { - debug!("gen {:?}", local); - self._trans.gen(local); + self.gen(local); } _ => { - if let Some(node_idx) = self.borrow_deps.locals_to_node_indexes.get(&local) { + if let Some(node_idx) = + self.borrow_deps.dep_graph.locals_to_node_indexes.get(&local) + { let node = self.borrow_deps.dep_graph.node(*node_idx); if matches!(node.data, NodeKind::LocalWithRefs(_)) { - self._trans.gen(local); + self.gen(local); } } } @@ -974,14 +1194,16 @@ where TerminatorKind::Call { destination, args, .. } => { match destination.projection.as_slice() { &[] | &[ProjectionElem::OpaqueCast(_)] => { - debug!("killing {:?}", destination.local); - self._trans.kill(destination.local); + self.kill(destination.local); for arg in args { match arg { Operand::Copy(place) | Operand::Move(place) => { - if let Some(node_idx) = - self.borrow_deps.locals_to_node_indexes.get(&place.local) + if let Some(node_idx) = self + .borrow_deps + .dep_graph + .locals_to_node_indexes + .get(&place.local) { let node = self.borrow_deps.dep_graph.node(*node_idx); @@ -989,8 +1211,7 @@ where // that were assigned to raw pointers, which were cast to usize. Since the // function call is free to use these in any form, we need to gen them here. if let NodeKind::LocalWithRefs(_) = node.data { - debug!("gen {:?}", place.local); - self._trans.gen(place.local); + self.gen(place.local); } } else { self.super_operand(arg, location) @@ -1006,13 +1227,15 @@ where TerminatorKind::Yield { resume_arg, value, .. } => { match resume_arg.projection.as_slice() { &[] | &[ProjectionElem::OpaqueCast(_)] => { - debug!("killing {:?}", resume_arg.local); - self._trans.kill(resume_arg.local); + self.kill(resume_arg.local); match value { Operand::Copy(place) | Operand::Move(place) => { - if let Some(node_idx) = - self.borrow_deps.locals_to_node_indexes.get(&place.local) + if let Some(node_idx) = self + .borrow_deps + .dep_graph + .locals_to_node_indexes + .get(&place.local) { let node = self.borrow_deps.dep_graph.node(*node_idx); @@ -1020,8 +1243,7 @@ where // that were assigned to raw pointers, which were cast to usize. Since the // function call is free to use these in any form, we need to gen them here. if let NodeKind::LocalWithRefs(_) = node.data { - debug!("gen {:?}", place.local); - self._trans.gen(place.local); + self.gen(place.local); } } else { self.super_operand(value, location) From d16bff463f901039488e8d8ca4b82db4e4a9ec87 Mon Sep 17 00:00:00 2001 From: b-naber Date: Wed, 14 Jun 2023 15:58:31 +0000 Subject: [PATCH 16/20] add visitor to detect interior mutability --- .../src/impls/live_borrows.rs | 76 ++++++++++++++++++- 1 file changed, 74 insertions(+), 2 deletions(-) diff --git a/compiler/rustc_mir_dataflow/src/impls/live_borrows.rs b/compiler/rustc_mir_dataflow/src/impls/live_borrows.rs index 6e0bc412431b1..b14f4aa1dfe37 100644 --- a/compiler/rustc_mir_dataflow/src/impls/live_borrows.rs +++ b/compiler/rustc_mir_dataflow/src/impls/live_borrows.rs @@ -145,11 +145,83 @@ use rustc_data_structures::graph::scc::Sccs; use rustc_middle::mir::visit::PlaceContext; use rustc_middle::mir::visit::Visitor; use rustc_middle::mir::*; -use rustc_middle::ty; +use rustc_middle::ty::TypeVisitable; +use rustc_middle::ty::{self, TypeSuperVisitable}; use either::Either; use std::cell::RefCell; -use std::ops::{Deref, DerefMut}; +use std::ops::{ControlFlow, Deref, DerefMut}; + +const DEPTH_LEVEL: u8 = 5; + +/// Checks whether a given type allows for interior mutability +struct MaybeContainsInteriorMutabilityVisitor<'tcx> { + tcx: TyCtxt<'tcx>, + has_interior_mut: bool, + reached_depth_limit: bool, + current_depth_level: u8, +} + +impl<'tcx> MaybeContainsInteriorMutabilityVisitor<'tcx> { + fn new(tcx: TyCtxt<'tcx>) -> Self { + MaybeContainsInteriorMutabilityVisitor { + tcx, + has_interior_mut: false, + reached_depth_limit: false, + current_depth_level: 0, + } + } +} + +impl<'tcx> ty::TypeVisitor> for MaybeContainsInteriorMutabilityVisitor<'tcx> { + type BreakTy = (); + + #[instrument(skip(self), level = "debug")] + fn visit_ty( + &mut self, + t: as ty::Interner>::Ty, + ) -> std::ops::ControlFlow + where + as ty::Interner>::Ty: ty::TypeSuperVisitable>, + { + self.current_depth_level += 1; + debug!(?self.current_depth_level); + if self.current_depth_level >= DEPTH_LEVEL { + self.reached_depth_limit = true; + return ControlFlow::Break(()); + } + + let control_flow = match t.kind() { + ty::Param(..) => { + // Need to be conservative here + self.has_interior_mut = true; + return ControlFlow::Break(()); + } + ty::Adt(adt_def, substs) => { + if adt_def.is_unsafe_cell() { + self.has_interior_mut = true; + return ControlFlow::Break(()); + } + + let mut control_flow = ControlFlow::Continue(()); + for field in adt_def.all_fields() { + let field_ty = field.ty(self.tcx, substs); + control_flow = field_ty.visit_with(self); + + if control_flow == ControlFlow::Break(()) { + return ControlFlow::Break(()); + } + } + + control_flow + } + _ => t.super_visit_with(self), + }; + + self.current_depth_level -= 1; + control_flow + } +} #[derive(Copy, Clone, Debug)] enum NodeKind { From 0b39afee4c920e5924008136e1181c89ef39d2fa Mon Sep 17 00:00:00 2001 From: b-naber Date: Tue, 27 Jun 2023 18:54:43 +0000 Subject: [PATCH 17/20] cleanup --- .../src/impls/live_borrows.rs | 748 +++++++++++------- .../rustc_mir_dataflow/src/impls/liveness.rs | 25 +- compiler/rustc_mir_transform/src/generator.rs | 6 + 3 files changed, 469 insertions(+), 310 deletions(-) diff --git a/compiler/rustc_mir_dataflow/src/impls/live_borrows.rs b/compiler/rustc_mir_dataflow/src/impls/live_borrows.rs index b14f4aa1dfe37..1758d527fef8c 100644 --- a/compiler/rustc_mir_dataflow/src/impls/live_borrows.rs +++ b/compiler/rustc_mir_dataflow/src/impls/live_borrows.rs @@ -1,25 +1,100 @@ -//! This module defines a more fine-grained analysis for `Local`s that are live due -//! to outstanding references or raw pointers than `MaybeBorrowedLocals`. +//! This module defines a liveness analysis for `Local`s that are live due to outstanding references +//! or raw pointers. We can, however, not solely rely on tracking references and pointers due to soundness +//! issues. Exposed raw pointers (i.e. those cast to `usize`) and function calls would make this simple +//! analysis unsound, so we have to handle them as follows: //! -//! The analysis consists of three steps: -//! 1. build a dependency graph that relates `Local`s based on their borrowing relationship. +//! * exposed pointers (i.e. a cast of a raw pointer to `usize`) +//! => These count towards the liveness of the `Local` that is behind the raw pointer, e.g. for +//! `_5 = _4 as usize` (where `_4 = AddressOf(_3)`), we keep `_3` alive on any use site of _5. +//! The same holds for any re-assignments of the variable containing the exposed pointer, e.g. +//! for `_6 = _5` or `_6.0 = _5` we keep `_3` alive for any use site of `_6`. +//! Note: we cover `Local`s corresponding to exposed pointers or re-assignments of those exposed pointers +//! under the concept of a `LocalWithRefs`. `LocalWithRefs` are `Local`s that might contain references or +//! pointers and that need to be kept alive if they're behind a reference or pointer. +//! E.g. for `struct Foo<'a> {p: &'a Bar}` and `_4 = Foo { p: _3 }`, where `_3 = Ref(_2)`, any reference or +//! pointer of `_4` requires both `_3` and `4` to be kept alive. `LocalWithRefs` are also used for `Local`s +//! corresponding to destination places of function calls or yield statements, since they could refer to a +//! reference, pointer or another `LocalWithRefs` that was returned from the function call or yield statement. +//! * function calls and yield statements could allow for any `Operand` corresponding to a borrow or `LocalWithRefs` +//! to be moved into the destination place. +//! => destination places count towards the liveness of the `Local` that could be behind any ref/ptr or `LocalWithRefs` +//! corresponding to an `Operand`, e.g. for `TerminatorKind::Call { func, args: [_3, _4], destination: _5, .. }`, +//! where `_3` has type `Ref` or `RawPtr` or is a `LocalWithRefs` we keep any `Local` which `_3` could refer +//! to live on any use site of `_5`. +//! * function calls could move refs/ptrs or `LocalWithRefs` in arguments into `Local`s behind mutable references or shared references +//! that allow for interior mutability. +//! => E.g. let `Op1` and `Op2` be `Operand`s of a function call and suppose that at least one corresponds to either a mutable +//! references or shared references that allow for interior mutability and the other is a shared reference or a `LocalWithRefs`. +//! Let `Locals2` be the set of `Local`s that need to be kept alive due to the borrows corresponding to `Op2` +//! Then we need to start tracking all `Local`s in `Locals2` for the borrow corresponding to `Op1`, since the `Place` +//! corresponding to `Op2` might be moved into `Op1` in then call. +//! +//! As an example for what this analysis does: +//! +//! ```ignore(rust) +//! use std::ops::Generator; +//! +//! struct Bar {} +//! struct Foo { +//! p: *const Bar, +//! } +//! +//! fn takes_and_returns_ref<'a, T>(arg: &'a T) -> &'a T { +//! arg +//! } +//! +//! fn takes_ptr(arg: *const T) {} +//! +//! fn gen() -> impl Generator { +//! static move || { +//! let bar = Bar {}; // `Local1` +//! let bar2 = Bar {}; // `Local2` +//! let bar_ptr = &bar as *const Bar; // live locals: [`Local1`] +//! +//! yield 1; // live locals: [`Local1`] +//! +//! let foo = Foo { p: bar_ptr }; // `Local3` is a `LocalWithRefs`, live_locals: [`Local1`] +//! let bar2_ref = takes_and_returns_ref(&bar2); // live locals: [`Local1`, `Local2`, `Local3`] +//! let bar_ptr = foo.p; // Assignment of `LocalWithRefs` (need to be conservative and keep those alive) +//! // live locals: [`Local1`, `Local3`] +//! takes_ptr(bar_ptr); // live locals: [`Local1`, `Local3`] +//! +//! yield 2; // live locals: [] +//! +//! let bar3 = Bar {}; // `Local4` +//! let bar3_ref = &bar3; // live locals: [`Local4`] +//! +//! yield 3; // live locals: [`Local4`] +//! +//! takes_and_returns_ref(bar3_ref); // live locals: [`Local4`] +//! } +//! } +//! ``` +//! +//! Following is a description of the algorithm: +//! 1. build a dependency graph that relates `Local`s based on their borrowing relationships. //! As an example if we have something like this (in a simplified MIR representation): //! -//! ```ignore(rust) +//! ```ignore //! _4 = Bar {} //! _5 = Ref(_4) +//! ... +//! _10 = f(_5) //! ``` //! -//! Then we add an edge from `_5` to `_4`. +//! Then we add edges from `_5` to `_4` and `_4` to `_5` (for an explanation of why the edge +//! from `_4` to `_5` is necessary see the comment in `handle_ravlue_or_ptr`) and from +//! `_10` to `_5`. //! 2. perform a liveness analysis for borrowed `Local`s. -//! Continuing our example from step 1, if we later have a use of `_5`, `_5` is -//! live at least from its definition to that use of it. +//! Continuing our example from step 1, `_5` is live from its definition to the function call and +//! `_10` is live (it's a `LocalWithRefs`, since it might refer to `_5`) only at the statement +//! corresponding to the call. //! 3. Combine the two analyses from step 1 and 2. For any `Local` that corresponds //! to a borrow (`_5` in our example), we want to keep the `Local` (`_4`), which is actually //! borrowed through it, live over the range at which the borrow is live. Hence for any point -//! in that range we traverse our dependency graph and look for leaf nodes. In our example -//! we would find an edge from `_5` to `_4`, which is a leaf node and hence we keep `_4` live -//! over that range. +//! in that range we traverse our dependency graph and look for nodes that correspond to borrowed +//! `Local`s ("leaf nodes"). In our example we would find an edge from `_5` to `_4`, which is a leaf +//! node and hence we keep `_4` live over that range. //! //! There are some corner cases we need to look out for to make this analysis sound. Let's look //! at each of the three steps in more detail and elaborate how these steps deal with these corner @@ -31,16 +106,17 @@ //! three variants: `Local`, `Borrow` and `LocalWithRefs`. //! * `NodeKind::Local` is used for `Local`s that are borrowed somewhere (`_4` in our example), but aren't //! themselves references or pointers. -//! * `NodeKind::Borrow` is used for `Local`s that correspond to borrows (`_5` in our example) and -//! also `Local`s that result from re-borrows. -//! * `NodeKind::LocalWithRefs` is used for `Local`s that aren't themselves borrows, but contain -//! `Local`s that correspond to references, pointers or other `Local`s with `Node`s of kind -//! `NodeKind::LocalWithRef`s. Let's look at an example: +//! * `NodeKind::Borrow` is used for `Local`s that correspond to borrows (`_5` in our example). We equate +//! re-borrows with the `Node` that corresponds to the original borrow. +//! * `NodeKind::LocalWithRefs` is used for `Local`s that aren't themselves refs/ptrs, but contain +//! `Local`s that correspond to refs/ptrs or other `Local`s with `Node`s of kind `NodeKind::LocalWithRef`s. +//! `LocalWithRefs` is also used for exposed pointers. +//! Let's look at an example: //! -//! ```ignore(rust) +//! ```ignore //! _4 = Bar {} //! _5 = Ref(_4) -//! _6 = Aggregate(..)(move _5) +//! _6 = Foo(..)(move _5) //! ... //! _7 = (_6.0) //! ``` @@ -48,7 +124,7 @@ //! In this example `_6` would be given `NodeKind::LocalWithRefs` and our graph would look //! as follows: //! -//! `_7` (NodeKind::Borrow) -> `_6` (NodeKind::LocalWithRefs) -> `_5` (NodeKind::Borrow) -> `_4` (NodeKind::Local) +//! `_7` (NodeKind::Borrow) <-> `_6` (NodeKind::LocalWithRefs) <-> `_5` (NodeKind::Borrow) <-> `_4` (NodeKind::Local) //! //! On the one hand we need to treat `Local`s with `Node`s of kind `NodeKind::LocalWithRefs` similarly //! to how we treat `Local`s with `Node`s of kind `NodeKind::Local`, in the sense that if they are @@ -61,7 +137,7 @@ //! Additionally `NodeKind::LocalWithRefs` is also used for raw pointers that are cast to //! `usize`: //! -//! ```ignore(rust) +//! ```ignore //! _4 = Bar {} //! _5 = AddressOf(_4) //! _6 = _5 as usize @@ -70,24 +146,24 @@ //! ``` //! //! In this example our graph would have the following edges: -//! * `_5` (Borrow) -> `_4` (Local) -//! * `_6` (LocalWithRefs) -> `_5` (Borrow) -//! * `_7` (LocalWithRefs) -> `_6` (LocalWithRefs) -//! * `_8` (LocalWithRefs) -> `_7` (LocalWithRefs) +//! * `_5` (Borrow) <-> `_4` (Local) +//! * `_6` (LocalWithRefs) <-> `_5` (Borrow) +//! * `_7` (LocalWithRefs) <-> `_6` (LocalWithRefs) +//! * `_8` (LocalWithRefs) <-> `_7` (LocalWithRefs) //! -//! We also have to be careful when dealing with `Terminator`s. Whenever we pass references, +//! We also have to be careful about dealing with `Terminator`s. Whenever we pass references, //! pointers or `Local`s with `NodeKind::LocalWithRefs` to a `TerminatorKind::Call` or //! `TerminatorKind::Yield`, the destination `Place` or resume place, resp., might contain -//! these references, pointers or `NodeKind::LocalWithRefs` `Local`s, hence we have to be conservative +//! these references, pointers or `LocalWithRefs`, hence we have to be conservative //! and keep the `destination` `Local` and `resume_arg` `Local` live. //! //! 2. Liveness analysis for borrows //! -//! We perform a standard liveness analysis on any outstanding references, pointers or `Local`s -//! with `NodeKind::LocalWithRefs`. So we `gen` at any use site, which are either direct uses -//! of these `Local`s or projections that contain these `Local`s. So e.g.: +//! We perform a standard liveness analysis on any outstanding references, pointers or `LocalWithRefs` +//! So we `gen` at any use site, which are either direct uses of these `Local`s or projections that contain +//! these `Local`s. So e.g.: //! -//! ```ignore(rust) +//! ```ignore //! 1. _3 = Foo {} //! 2. _4 = Bar {} //! 3. _5 = Ref(_3) @@ -99,25 +175,32 @@ //! 9. (_7.0) = move _10 //! ``` //! -//! * `_5` is live from stmt 3 to stmt 5 -//! * `_6` is live from stmt 4 to stmt 6 +//! * `_5` is live from stmt 3 to stmt 9 +//! * `_6` is live from stmt 4 to stmt 7 //! * `_7` is a `Local` of kind `LocalWithRefs` so needs to be taken into account in the //! analyis. It's live from stmt 5 to stmt 9 //! * `_8` is a `Local` of kind `LocalWithRefs`. It's live from 6. to 7. -//! * `_9` is a `Local` of kind `LocalWithRefs` -//! it's live at 7. +//! * `_9` is a `Local` of kind `LocalWithRefs`. It's live at 7. //! //! 3. Determining which `Local`s are borrowed //! //! Let's use our last example again. The dependency graph for that example looks as follows: //! -//! `_5` (Borrow) -> `_3` (Local) -//! `_6` (Borrow) -> `_4` (Local) -//! `_7` (LocalWithRef) -> `_5` (Borrow) +//! `_5` (Borrow) <-> `_3` (Local) +//! `_6` (Borrow) <-> `_4` (Local) +//! `_7` (LocalWithRef) <-> `_5` (Borrow) //! `_8` (LocalWithRef) -> `_6` (Borrow) -//! `_9` (LocalWithRef) -> `_8` (LocalWithRef) -//! `_7` (LocalWithRef) -> `_10` (Local) +//! `_9` (LocalWithRef) <-> `_8` (LocalWithRef) +//! `_7` (LocalWithRef) <-> `_10` (Local) +//! +//! We then construct a strongly connected components graph from the dependency graph, yielding: +//! +//! SCC1: [_3, _5, _7, _10] +//! SCC2: [_4, _6, _8, _9] //! +//! Now for each statement in the `Body` we check which refs/ptrs or `LocalWithRefs` are live at that statement +//! and then perform a depth-first search in the scc graph, collecting all `Local`s that need to be kept alive +//! (`Local`s that have `Node`s in the graph of either `NodeKind::Local` or `NodeKind::LocalWithRefs`). //! So at each of those statements we have the following `Local`s that are live due to borrows: //! //! 1. {} @@ -126,10 +209,36 @@ //! 4. {_3, _4} //! 5. {_3, _4, _7} //! 6. {_3, _4, _7, _8} -//! 7. {_3, _4, _7, _8} +//! 7. {_3, _4, _7, _8, _9} //! 8. {_3, _7} -//! 9. {_3, _7} +//! 9. {_3, _7, _10} +//! +//! Ensuring soundness in all cases requires us to be more conservative (i.e. keeping more `Local`s alive) than necessary +//! in most situations. To eliminate all the unnecessary `Local`s we use the fact that the analysis performed by +//! `MaybeBorrowedLocals` functions as an upper bound for which `Local`s need to be kept alive. Hence we take the intersection +//! of the two analyses at each statement. The results of `MaybeBorrowedLocals` for our example are: +//! +//! 1. {} +//! 2. {} +//! 3. {_3} +//! 4. {_3, _4} +//! 5. {_3, _4} +//! 6. {_3, _4} +//! 7. {_3, _4,} +//! 8. {_3, _4} +//! 9. {_3, _4} +//! +//! Taking the intersection hence yields: //! +//! 1. {} +//! 2. {} +//! 3. {_3} +//! 4. {_3, _4} +//! 5. {_3, _4} +//! 6. {_3, _4} +//! 7. {_3, _4} +//! 8. {_3} +//! 9. {_3} use super::*; @@ -138,7 +247,7 @@ use crate::impls::MaybeBorrowedLocals; use crate::{ AnalysisDomain, Backward, CallReturnPlaces, GenKill, GenKillAnalysis, ResultsRefCursor, }; -use rustc_data_structures::fx::FxHashMap; +use rustc_data_structures::fx::{FxHashMap, FxHashSet}; use rustc_data_structures::graph; use rustc_data_structures::graph::implementation::{Graph, NodeIndex}; use rustc_data_structures::graph::scc::Sccs; @@ -146,12 +255,12 @@ use rustc_middle::mir::visit::PlaceContext; use rustc_middle::mir::visit::Visitor; use rustc_middle::mir::*; use rustc_middle::ty::TypeVisitable; -use rustc_middle::ty::{self, TypeSuperVisitable}; +use rustc_middle::ty::{self, Ty, TypeSuperVisitable}; -use either::Either; use std::cell::RefCell; use std::ops::{ControlFlow, Deref, DerefMut}; +// FIXME Properly determine a reasonable value const DEPTH_LEVEL: u8 = 5; /// Checks whether a given type allows for interior mutability @@ -370,6 +479,18 @@ impl<'a, 'tcx> BorrowDependencies<'a, 'tcx> { } } + fn add_edge(&mut self, from_node_idx: NodeIndex, to_node_idx: NodeIndex) { + debug!( + "adding edge from {:?}({:?}) -> {:?}({:?})", + from_node_idx, + self.dep_graph.node(from_node_idx).data, + to_node_idx, + self.dep_graph.node(to_node_idx).data, + ); + + self.dep_graph.add_edge(from_node_idx, to_node_idx, ()); + } + /// Panics if `local` doesn't have a `Node` in `self.dep_graph`. fn get_node_idx_for_local(&self, local: Local) -> NodeIndex { if let Some(reborrowed_local) = self.reborrows_map.get(&local) { @@ -385,10 +506,21 @@ impl<'a, 'tcx> BorrowDependencies<'a, 'tcx> { } } + #[inline] + fn place_is_mut_ref_or_has_interior_mut(&self, place: &Place<'tcx>) -> bool { + let ty = place.ty(self.local_decls, self.tcx).ty; + match ty.kind() { + ty::Ref(_, inner_ty, mutbl) | ty::RawPtr(ty::TypeAndMut { ty: inner_ty, mutbl }) => { + *mutbl == Mutability::Mut || self.type_has_interior_mutability(*inner_ty) + } + _ => false, + } + } + #[instrument(skip(self), level = "debug")] - fn local_is_ref_ptr_or_localwithrefs(&self, place: &Place<'tcx>) -> bool { - let place_ty = place.ty(self.local_decls, self.tcx).ty; - let is_ref_or_ptr = matches!(place_ty.kind(), ty::Ref(..) | ty::RawPtr(..)); + /// Checks whether ty type of `place` is a ref/ptr or whether it's local is a `LocalWithRefs`. + fn operand_maybe_needs_edge(&self, place: &Place<'tcx>) -> bool { + let ty = place.ty(self.local_decls, self.tcx).ty; // Also account for `LocalWithRef`s let is_local_with_refs = @@ -400,8 +532,7 @@ impl<'a, 'tcx> BorrowDependencies<'a, 'tcx> { false }; - debug!(?is_ref_or_ptr, ?is_local_with_refs); - is_ref_or_ptr || is_local_with_refs + ty.is_ref() || ty.is_unsafe_ptr() || is_local_with_refs } #[instrument(skip(self), level = "debug")] @@ -411,20 +542,25 @@ impl<'a, 'tcx> BorrowDependencies<'a, 'tcx> { debug!(?outer_operand); match outer_operand { Operand::Copy(outer_place) | Operand::Move(outer_place) => { - if self.local_is_ref_ptr_or_localwithrefs(outer_place) { + if self.operand_maybe_needs_edge(outer_place) { for j in i + 1..args.len() { let inner_operand = &args[j]; debug!(?inner_operand); match inner_operand { Operand::Copy(inner_place) | Operand::Move(inner_place) => { - if self.local_is_ref_ptr_or_localwithrefs(inner_place) { + if self.operand_maybe_needs_edge(inner_place) { let node_idx_outer = self.get_node_idx_for_local(outer_place.local); let node_idx_inner = self.get_node_idx_for_local(inner_place.local); - self.dep_graph.add_edge(node_idx_outer, node_idx_inner, ()); - self.dep_graph.add_edge(node_idx_inner, node_idx_outer, ()); + if self.place_is_mut_ref_or_has_interior_mut(outer_place) { + self.add_edge(node_idx_outer, node_idx_inner); + } + + if self.place_is_mut_ref_or_has_interior_mut(inner_place) { + self.add_edge(node_idx_inner, node_idx_outer); + } } } Operand::Constant(_) => {} @@ -436,6 +572,129 @@ impl<'a, 'tcx> BorrowDependencies<'a, 'tcx> { } } } + + fn type_has_interior_mutability(&self, ty: Ty<'tcx>) -> bool { + // FIXME maybe cache those? + let mut visitor = MaybeContainsInteriorMutabilityVisitor::new(self.tcx); + ty.visit_with(&mut visitor); + + visitor.has_interior_mut || visitor.reached_depth_limit + } + + fn handle_rvalue_ref_or_ptr(&mut self, borrowed_place: &Place<'tcx>, mutbl: Mutability) { + let Some(src_local) = self.current_local else { + bug!("Expected self.current_local to be set with Rvalue::Ref|Rvalue::AddressOf"); + }; + + let src_node_idx = if borrowed_place.is_indirect() { + // Don't introduce new nodes for re-borrows. + if let Some(_) = self.dep_graph.locals_to_node_indexes.get(&borrowed_place.local) { + self.reborrows_map.insert(src_local, borrowed_place.local); + + return; + } else { + // we're in a statement like `_4 = Ref(..)` or `_4 = AddressOf(..)`, hence NodeKind::Borrow for `_4` + let src_node_kind = NodeKind::Borrow(src_local); + self.maybe_create_node(src_node_kind) + } + } else { + // we're in a statement like `_4 = Ref(..)`, hence NodeKind::Borrow for `_4` + let src_node_kind = NodeKind::Borrow(src_local); + self.maybe_create_node(src_node_kind) + }; + + // If we haven't previously added a node for `borrowed_place.local` then it can be neither + // `NodeKind::Borrow` nor `NodeKind::LocalsWithRefs`. + let borrowed_node_kind = NodeKind::Local(borrowed_place.local); + let node_idx = self.maybe_create_node(borrowed_node_kind); + + // We need to reach the borrowed local through this borrow + self.add_edge(src_node_idx, node_idx); + + // If this is a mutable borrow or the type might contain interior mutatability + // we need to also have a path from any other borrows of the locals to which + // `borrowed_place` refers and this borrow. As an example: + // + // struct WithInteriorMut { + // a: RefCell, + // } + // + // fn switch_interior_mut<'a>(raw_ref_exposed: usize, int_mut: &'a WithInteriorMut) { + // let mut mut_ref = int_mut.a.borrow_mut(); + // *mut_ref = raw_ref_exposed; + // } + // + // fn gen() -> impl Generator { + // static move || { + // let x = Foo { a: 11 }; + // let p = &x as *const Foo; + // let exposed_p = p as usize; + // let int_mut = WithInteriorMut { a: RefCell::new(exposed_p) }; + // let x2 = Foo { a: 13 }; + // let p2 = &x2 as *const Foo; + // let exposed_p2 = p2 as usize; + // + // yield 12; + // + // switch_interior_mut(exposed_p2, &int_mut); + // + // yield 15; + // + // let int_mut_back = int_mut.a.borrow(); + // let ref_to_foo2 = unsafe { &*(*int_mut_back as *const Foo) }; + // } + // } + // + // with MIR that looks something like this (simplified): + // + // _3 = Foo { a: const 11_usize }, + // _5 = &_3, + // _4 = &raw const (*_5), + // _7 = _4 + // _6 = move _7 as usize (PointerExposeAddress) + // _10 = _6 + // Terminator(Call, kind: _9 = RefCell::::new(move _10)) + // _8 = WithInteriorMut { a: move _9 } + // _11 = Foo { a: const 13_usize} + // _13 = &_11 + // _12 = &raw const (*13) + // _15 = _12 + // _14 = move _15 as usize (PointerExposeAddress) + // Terminator(Yield, _16 = yield(const 12_u32)) + // _18 = _14 + // _20 = &8 + // _19 = &(*_20) + // Terminator(Call, _17 = switch_interior_mut(move _18, move _19)) + // Terminator(Yield, _21 = yield(const 15_u32)) + // _23 = &(_8.0) + // _22 = RefCell::::borrow(move _23) + // _28 = &_22 + // Terminator(Call, _27 = as Deref>::deref(move _28)) + // _26 = (*_27) + // _25 = move _26 as *const Foo (PointerFromExposedAddress) + // _24 = &(*_25) + // + // Note how we introduce an immutable ref `_20` for `_8`, which we then + // mutate through the `switch_interior_mut` call by putting the exposed + // pointer that is contained in `_18` into `_8.0`. We therefore have the + // following edges in our graph: + // + // _20 -> _8 + // _8 <- _20 + // _18 -> _20 (_19 is the same as _20 since it's a reborrow) + // _20 <- _18 + // (ignoring _17 here since it's unused) + // + // We then take a reference of `8.0` via `_23` (remember `8.0` now contains + // an exposed pointer to `_11`) which we later cast/convert to a reference of + // `_11` again. So we need to also keep `_11` alive up to this point. + // If we only had an edge from `_20` to `_8` there would be no way to reach the + // node for `_18` from `_24`, so we wouldn't be able to keep `_11` alive. + let borrow_place_ty = borrowed_place.ty(self.local_decls, self.tcx).ty; + if matches!(mutbl, Mutability::Mut) || self.type_has_interior_mutability(borrow_place_ty) { + self.add_edge(node_idx, src_node_idx); + } + } } impl<'a, 'tcx> Visitor<'tcx> for BorrowDependencies<'a, 'tcx> { @@ -489,9 +748,6 @@ impl<'a, 'tcx> Visitor<'tcx> for BorrowDependencies<'a, 'tcx> { place.ty(self.local_decls, self.tcx).ty.kind(), ty::Ref(..) | ty::RawPtr(..) ) { - // these are just re-assignments of already outstanding refs or pointers, - // hence we want to treat them as `NodeKind::Borrow` - // FIXME Are these always Operand::Copy or is Operand::Move also possible for refs/ptrs? let Some(src_local) = self.current_local else { bug!("Expected self.current_local to be set when encountering Rvalue"); }; @@ -499,22 +755,14 @@ impl<'a, 'tcx> Visitor<'tcx> for BorrowDependencies<'a, 'tcx> { // These are just moves of refs/ptrs, hence `NodeKind::Borrow`. let src_node_kind = NodeKind::Borrow(src_local); let src_node_idx = self.maybe_create_node(src_node_kind); - let node_kind = NodeKind::Borrow(place.local); let node_idx = self.maybe_create_node(node_kind); - debug!( - "adding edge from {:?}({:?}) -> {:?}({:?})", - src_node_idx, - self.dep_graph.node(src_node_idx).data, - node_idx, - self.dep_graph.node(node_idx).data, - ); - - self.dep_graph.add_edge(src_node_idx, node_idx, ()); - self.dep_graph.add_edge(node_idx, src_node_idx, ()); + self.add_edge(src_node_idx, node_idx); + self.add_edge(node_idx, src_node_idx); } else { - // Don't introduce edges for moved/copied `Local`s that correspond to `NodeKind::Local` + // Don't introduce edges for moved/copied `Local`s that correspond to `NodeKind::Local` or of + // `Local`s that don't have a `Node` in the graph already. if let Some(node_idx) = self.dep_graph.locals_to_node_indexes.get(&place.local) { if matches!(self.dep_graph.node(*node_idx).data, NodeKind::Local(_)) { @@ -527,119 +775,18 @@ impl<'a, 'tcx> Visitor<'tcx> for BorrowDependencies<'a, 'tcx> { self.super_rvalue(rvalue, location); } } - Rvalue::Ref(_, _, borrowed_place) | Rvalue::AddressOf(_, borrowed_place) => { - let Some(src_local) = self.current_local else { - bug!("Expected self.current_local to be set with Rvalue::Ref|Rvalue::AddressOf"); - }; - - let src_node_idx = if borrowed_place.is_indirect() { - // Don't introduce new nodes for re-borrows. We need to treat Re-borrows the same as the original - // borrow. The reason for this is that we could e.g. use interior mutability on a re-borrow - // in a function call (which would require us to add an edge between the borrow and whatever is - // inserted (e.g. a `LocalWithRefs`)), but we then later need to also have this edge on the local - // that corresponds to the borrow that we re-borrowed. To make this more clear, let's say we have - // something like this: - // - // struct WithInteriorMut { - // a: RefCell, - // } - // - // fn switch_interior_mut<'a>(raw_ref_exposed: usize, int_mut: &'a WithInteriorMut) { - // let mut mut_ref = int_mut.a.borrow_mut(); - // *mut_ref = raw_ref_exposed; - // } - // - // fn gen() -> impl Generator { - // static move || { - // let x = Foo { a: 11 }; - // let p = &x as *const Foo; - // let exposed_p = p as usize; - // let int_mut = WithInteriorMut { a: RefCell::new(exposed_p) }; - // let x2 = Foo { a: 13 }; - // let p2 = &x2 as *const Foo; - // let exposed_p2 = p2 as usize; - // - // yield 12; - // - // switch_interior_mut(exposed_p2, &int_mut); - // - // yield 15; - // - // let int_mut_back = int_mut.a.borrow(); - // let ref_to_foo2 = unsafe { &*(*int_mut_back as *const Foo) }; - // } - // } - // - // with MIR that looks something like this (simplified): - // - // _3 = Foo { a: const 11_usize }, - // _5 = &_3, - // _4 = &raw const (*_5), - // _7 = _4 - // _6 = move _7 as usize (PointerExposeAddress) - // _10 = _6 - // Terminator(Call, kind: _9 = RefCell::::new(move _10)) - // _8 = WithInteriorMut { a: move _9 } - // _11 = Foo { a: const 13_usize} - // _13 = &_11 - // _12 = &raw const (*13) - // _15 = _12 - // _14 = move _15 as usize (PointerExposeAddress) - // Terminator(Yield, _16 = yield(const 12_u32)) - // _18 = _14 - // _20 = &8 - // _19 = &(*_20) - // Terminator(Call, _17 = switch_interior_mut(move _18, move _19)) - // Terminator(Yield, _21 = yield(const 15_u32)) - // _23 = &(_8.0) - // _22 = RefCell::::borrow(move _23) - // _28 = &_22 - // Terminator(Call, _27 = as Deref>::deref(move _28)) - // _26 = (*_27) - // _25 = move _26 as *const Foo (PointerFromExposedAddress) - // _24 = &(*_25) - // - // We need to keep `_11` alive across the second suspension point (yield Terminator). To enable - // us to do this, we introduce edges between `_18` and `_19` (the call operands in the - // `switch_interior_mut` call). Note that - - if let Some(_) = - self.dep_graph.locals_to_node_indexes.get(&borrowed_place.local) - { - self.reborrows_map.insert(src_local, borrowed_place.local); - - return; - } else { - // we're in a statement like `_4 = Ref(..)`, hence NodeKind::Borrow for `_4` - let src_node_kind = NodeKind::Borrow(src_local); - self.maybe_create_node(src_node_kind) + Rvalue::Ref(_, borrow_kind, borrowed_place) => { + let mutbl = match borrow_kind { + BorrowKind::Mut { .. } => Mutability::Mut, + BorrowKind::Shallow | BorrowKind::Shared | BorrowKind::Unique => { + Mutability::Not } - } else { - // we're in a statement like `_4 = Ref(..)`, hence NodeKind::Borrow for `_4` - let src_node_kind = NodeKind::Borrow(src_local); - self.maybe_create_node(src_node_kind) }; - // If we haven't previously added a node for `borrowed_place.local` then it can be neither - // `NodeKind::Borrow` nor `NodeKind::LocalsWithRefs`. - let borrowed_node_kind = NodeKind::Local(borrowed_place.local); - let node_idx = self.maybe_create_node(borrowed_node_kind); - - debug!( - "adding edge from {:?}({:?}) -> {:?}({:?})", - src_node_idx, - self.dep_graph.node(src_node_idx).data, - node_idx, - self.dep_graph.node(node_idx).data, - ); - - self.dep_graph.add_edge(src_node_idx, node_idx, ()); - self.dep_graph.add_edge(node_idx, src_node_idx, ()); + self.handle_rvalue_ref_or_ptr(borrowed_place, mutbl) } - Rvalue::Cast(..) => { - // FIXME we probably should handle pointer casts here directly - - self.super_rvalue(rvalue, location) + Rvalue::AddressOf(mutbl, borrowed_place) => { + self.handle_rvalue_ref_or_ptr(borrowed_place, *mutbl); } _ => self.super_rvalue(rvalue, location), } @@ -666,41 +813,30 @@ impl<'a, 'tcx> Visitor<'tcx> for BorrowDependencies<'a, 'tcx> { let borrowed_node_kind = NodeKind::Borrow(place.local); let node_idx = self.maybe_create_node(borrowed_node_kind); - debug!( - "adding edge from {:?}({:?}) -> {:?}({:?})", - src_node_idx, - self.dep_graph.node(src_node_idx).data, - node_idx, - place.local - ); - - self.dep_graph.add_edge(src_node_idx, node_idx, ()); + self.add_edge(src_node_idx, node_idx); - // FIXME not sure whether these are correct - self.dep_graph.add_edge(node_idx, src_node_idx, ()); + // FIXME don't think this edge is necessary in all cases. We should be able to do something + // similar to what is done in `handle_rvalue_ref_or_ptr`, i.e. only add this edge in a mutable + // context or if interior mutability is used. + self.add_edge(node_idx, src_node_idx); } _ => { if let Some(node_idx) = self.dep_graph.locals_to_node_indexes.get(&place.local) { - // LocalsWithRefs -> LocalWithRefs + // LocalsWithRefs = LocalWithRefs/Local assigment let node_idx = *node_idx; let src_node_kind = NodeKind::LocalWithRefs(src_local); let src_node_idx = self.maybe_create_node(src_node_kind); - debug!( - "adding edge from {:?}({:?}) -> {:?}({:?})", - src_node_idx, - self.dep_graph.node(src_node_idx).data, - node_idx, - self.dep_graph.node(node_idx).data, - ); + self.add_edge(src_node_idx, node_idx); - self.dep_graph.add_edge(src_node_idx, node_idx, ()); - - // FIXME not sure whether these are correct - self.dep_graph.add_edge(node_idx, src_node_idx, ()); + // FIXME not sure this edge is necessary (this edge is not necessary for moves, so we might + // want to handle this case in `visit_rvalue` instead). + // For copies does dest prop eliminate the possibility of using both the copy and the copied + // value after the assignment? + self.add_edge(node_idx, src_node_idx); } } } @@ -711,11 +847,6 @@ impl<'a, 'tcx> Visitor<'tcx> for BorrowDependencies<'a, 'tcx> { self.super_place(place, context, location) } - #[instrument(skip(self), level = "debug")] - fn visit_operand(&mut self, operand: &Operand<'tcx>, location: Location) { - self.super_operand(operand, location) - } - #[instrument(skip(self), level = "debug")] fn visit_terminator(&mut self, terminator: &Terminator<'tcx>, location: Location) { match &terminator.kind { @@ -753,21 +884,27 @@ impl<'a, 'tcx> Visitor<'tcx> for BorrowDependencies<'a, 'tcx> { self.super_operand(value, location); } + TerminatorKind::InlineAsm { .. } => { + // TO-DO + } _ => self.super_terminator(terminator, location), } } } pub struct BorrowedLocalsResults<'a, 'mir, 'tcx> { - // the results of the liveness analysis of `LiveBorrows` + /// the results of the liveness analysis of `LiveBorrows` borrows_analysis_results: Results<'tcx, LiveBorrows<'mir, 'tcx>>, - // Maps each `Local` that corresponds to a reference, pointer or a node of kind - // `NodeKind::LocalWithRefs` (i.e. `Local`s which either correspond to refs, pointers or - // exposed pointers or a composite value that might include refs, pointers or exposed pointers) - // to the set of `Local`s that are borrowed through those references, pointers or composite values. - borrowed_local_to_locals_to_keep_alive: FxHashMap>, + /// Maps each `Local` that corresponds to a reference, pointer or a node of kind + /// `NodeKind::LocalWithRefs` (i.e. `Local`s which either correspond to refs, pointers or + /// exposed pointers or a composite value that might include refs, pointers or exposed pointers) + /// to the set of `Local`s that are borrowed through those references, pointers or composite values. + borrowed_local_to_locals_to_keep_alive: FxHashMap>, + /// The results cursor of the `MaybeBorrowedLocals` analysis. Needed as an upper bound, since + /// to ensure soundness the `LiveBorrows` analysis would keep more `Local`s alive than + /// strictly necessary. maybe_borrowed_locals_results_cursor: RefCell< ResultsCursor<'mir, 'tcx, MaybeBorrowedLocals, &'a Results<'tcx, MaybeBorrowedLocals>>, >, @@ -803,21 +940,39 @@ where #[instrument(skip(dep_graph), level = "debug")] fn get_locals_to_keep_alive_map<'b>( dep_graph: &'b BorrowDepGraph, - ) -> FxHashMap> { - let mut borrows_to_locals: FxHashMap> = Default::default(); + ) -> FxHashMap> { + let mut borrows_to_locals: FxHashMap> = Default::default(); + let mut memoization_map: FxHashMap> = Default::default(); // create SCCs for dependency graph and map each local to its SCC. let sccs: Sccs = Sccs::new(dep_graph); - let mut components: FxHashMap> = Default::default(); - let mut local_to_component: FxHashMap = Default::default(); + + // Contains the Locals to keep alive for each scc. + let mut scc_to_locals_to_keep_alive: FxHashMap> = + Default::default(); + + // Maps each `Local` that has a node in the dependency graph to its SCC index. + let mut local_to_component: FxHashMap = Default::default(); + for (node_idx, scc_idx) in sccs.scc_indices().iter().enumerate() { + debug!(?node_idx, ?scc_idx); let node = dep_graph.node(NodeIndex(node_idx)); let local = node.data.get_local(); - components.entry(scc_idx.0).or_default().push(local); - local_to_component.insert(local, scc_idx.0); + + if matches!(node.data, NodeKind::LocalWithRefs(_) | NodeKind::Local(_)) { + scc_to_locals_to_keep_alive.entry(*scc_idx).or_default().insert(local); + } + + local_to_component.insert(local, *scc_idx); + } + + debug!("scc_to_locals_to_keep_alive: {:#?}", scc_to_locals_to_keep_alive); + + for scc in sccs.all_sccs() { + let succs = sccs.successors(scc); + debug!("succs of {:?}: {:?}", scc, succs); } - debug!("components: {:#?}", components); debug!("local_to_component: {:#?}", local_to_component); for (_, node) in dep_graph.enumerated_nodes() { @@ -827,16 +982,16 @@ where .get(¤t_local) .unwrap_or_else(|| bug!("{:?} should have a component", current_local)); - // add all locals that we need to keep alive for a given Borrow/LocalWithRefs (these are all - // `Local`s or `LocalWithRef`s in the SCC) - for local_in_scc in &components[scc] { - if let Some(node_idx) = dep_graph.locals_to_node_indexes.get(local_in_scc) { - let node = dep_graph.node(*node_idx); - if matches!(node.data, NodeKind::Local(_) | NodeKind::LocalWithRefs(_)) { - borrows_to_locals.entry(current_local).or_default().push(*local_in_scc); - } - } - } + let locals_to_keep_alive = Self::dfs_for_local( + current_local, + *scc, + &sccs, + &scc_to_locals_to_keep_alive, + &local_to_component, + &mut memoization_map, + ); + + borrows_to_locals.insert(current_local, locals_to_keep_alive); } } @@ -844,66 +999,55 @@ where borrows_to_locals } - #[instrument(skip(dep_graph), level = "debug")] - fn dfs_for_node( - node_idx: NodeIndex, - borrows_to_locals: &mut FxHashMap>, - dep_graph: &Graph, - locals_visited: &mut Vec, - ) -> Vec { - let src_node = dep_graph.node(node_idx); - let current_local = src_node.data.get_local(); - locals_visited.push(current_local); - if let Some(locals_to_keep_alive) = borrows_to_locals.get(¤t_local) { - debug!("already prev. calculated: {:?}", locals_to_keep_alive); - // already traversed this node - return (*locals_to_keep_alive).clone(); + #[instrument(skip(sccs, memoization_map), level = "debug")] + /// Performs a depth-first search on the singular components graph starting from the `Node` corresponding to + /// `local` (this is a `Local` corresponding to a `Ref`, `RawPtr` or `LocalWithRefs`). + /// Collects all `Local`s that need to be kept alive for the borrow corresponding to `local`. + fn dfs_for_local( + local: Local, + current_scc_idx: NodeIndex, + sccs: &Sccs, + scc_to_locals_to_keep_alive: &FxHashMap>, + local_to_scc: &FxHashMap, + memoization_map: &mut FxHashMap>, + ) -> FxHashSet { + if let Some(locals_to_keep_alive) = memoization_map.get(¤t_scc_idx) { + return locals_to_keep_alive.clone(); } - let mut locals_for_node = vec![]; - for (_, edge) in dep_graph.outgoing_edges(node_idx) { - let target_node_idx = edge.target(); - let target_node = dep_graph.node(target_node_idx); - let target_local = target_node.data.get_local(); - - // necessary to prevent loops - if locals_visited.contains(&target_local) { - if let Some(locals_to_keep_alive) = borrows_to_locals.get(&target_local) { - debug!( - "prev. calculated locals to keep alive for {:?}: {:?}", - target_local, locals_to_keep_alive - ); - locals_for_node.append(&mut locals_to_keep_alive.clone()); - } - - continue; + let mut locals_to_keep_alive: FxHashSet = Default::default(); + if let Some(locals_to_keep_alive_for_scc) = + scc_to_locals_to_keep_alive.get(¤t_scc_idx) + { + #[allow(rustc::potential_query_instability)] + // Order is irrelevant here. + for local_to_keep_alive in locals_to_keep_alive_for_scc { + locals_to_keep_alive.insert(*local_to_keep_alive); } + } - debug!( - "edge {:?} ({:?}) -> {:?} ({:?})", - node_idx, src_node.data, target_node_idx, target_node.data, + let succs = sccs.successors(current_scc_idx); + debug!(?succs); + for succ_scc_idx in succs { + let locals_to_keep_alive_for_succ = Self::dfs_for_local( + local, + *succ_scc_idx, + sccs, + scc_to_locals_to_keep_alive, + local_to_scc, + memoization_map, ); - let mut locals_to_keep_alive_for_succ = - Self::dfs_for_node(target_node_idx, borrows_to_locals, dep_graph, locals_visited); - locals_for_node.append(&mut locals_to_keep_alive_for_succ); - } - - match src_node.data { - NodeKind::Local(_) => { - locals_for_node.push(current_local); + #[allow(rustc::potential_query_instability)] + // Order is irrelevant here. + for local_to_keep_alive in locals_to_keep_alive_for_succ { + locals_to_keep_alive.insert(local_to_keep_alive); } - NodeKind::LocalWithRefs(_) => { - // These are locals that we need to keep alive, but that also contain - // successors in the graph since they contain other references/pointers. - locals_for_node.push(current_local); - } - NodeKind::Borrow(_) => {} } - debug!("locals for {:?}: {:?}", current_local, locals_for_node); - borrows_to_locals.insert(current_local, locals_for_node.clone()); - locals_for_node + memoization_map.insert(current_scc_idx, locals_to_keep_alive.clone()); + + locals_to_keep_alive } } @@ -963,8 +1107,6 @@ pub fn get_borrowed_locals_results<'a, 'mir, 'tcx>( /// require convergence, we expose the set of borrowed `Local`s for a `Location` directly via /// the `get` method without the need for any prior 'seek' calls. pub struct BorrowedLocalsResultsCursor<'a, 'mir, 'tcx> { - body: &'mir Body<'tcx>, - // The cursor for the liveness analysis performed by `LiveBorrows` borrows_analysis_cursor: ResultsRefCursor<'a, 'mir, 'tcx, LiveBorrows<'mir, 'tcx>>, @@ -972,7 +1114,7 @@ pub struct BorrowedLocalsResultsCursor<'a, 'mir, 'tcx> { // that are borrowed through the ref/ptr. Additionally contains entries for `Local`s // corresponding to `NodeKind::LocalWithRefs` since they might contain refs, ptrs or // exposed pointers and need to be treated equivalently to refs/ptrs - borrowed_local_to_locals_to_keep_alive: &'a FxHashMap>, + borrowed_local_to_locals_to_keep_alive: &'a FxHashMap>, // the cursor of the conservative borrowed locals analysis maybe_borrowed_locals_results_cursor: &'a RefCell< @@ -990,7 +1132,6 @@ impl<'a, 'mir, 'tcx> BorrowedLocalsResultsCursor<'a, 'mir, 'tcx> { cursor.allow_unreachable(); Self { - body, borrows_analysis_cursor: cursor, borrowed_local_to_locals_to_keep_alive: &results.borrowed_local_to_locals_to_keep_alive, maybe_borrowed_locals_results_cursor: &results.maybe_borrowed_locals_results_cursor, @@ -1011,37 +1152,17 @@ impl<'a, 'mir, 'tcx> BorrowedLocalsResultsCursor<'a, 'mir, 'tcx> { self.borrowed_local_to_locals_to_keep_alive.get(&borrowed_local) { debug!(?locals_to_keep_alive); + #[allow(rustc::potential_query_instability)] + // Order is irrelevant here. for local in locals_to_keep_alive.iter() { borrowed_locals.insert(*local); } } } - match self.body.stmt_at(loc) { - Either::Right(terminator) => { - match terminator.kind { - TerminatorKind::Drop { place: dropped_place, .. } => { - // Drop terminators may call custom drop glue (`Drop::drop`), which takes `&mut - // self` as a parameter. In the general case, a drop impl could launder that - // reference into the surrounding environment through a raw pointer, thus creating - // a valid `*mut` pointing to the dropped local. We are not yet willing to declare - // this particular case UB, so we must treat all dropped locals as mutably borrowed - // for now. See discussion on [#61069]. - // - // [#61069]: https://github.com/rust-lang/rust/pull/61069 - if !dropped_place.is_indirect() { - borrowed_locals.insert(dropped_place.local); - } - } - _ => {} - } - } - Either::Left(_) => {} - } - // use results of conservative analysis as an "upper bound" on the borrowed locals. This - // is necessary since to guarantee soundness for this analysis requires us to be more conservative - // in some cases than the analysis performed by `MaybeBorrowedLocals`. + // is necessary since to guarantee soundness for this analysis we would have to keep + // more `Local`s alive than strictly necessary. let mut maybe_borrowed_locals_cursor = self.maybe_borrowed_locals_results_cursor.borrow_mut(); maybe_borrowed_locals_cursor.allow_unreachable(); @@ -1328,6 +1449,19 @@ where _ => self.super_terminator(terminator, location), } } + TerminatorKind::Drop { place: dropped_place, .. } => { + // Drop terminators may call custom drop glue (`Drop::drop`), which takes `&mut + // self` as a parameter. In the general case, a drop impl could launder that + // reference into the surrounding environment through a raw pointer, thus creating + // a valid `*mut` pointing to the dropped local. We are not yet willing to declare + // this particular case UB, so we must treat all dropped locals as mutably borrowed + // for now. See discussion on [#61069]. + // + // [#61069]: https://github.com/rust-lang/rust/pull/61069 + if !dropped_place.is_indirect() { + self.gen(dropped_place.local); + } + } _ => self.super_terminator(terminator, location), } } diff --git a/compiler/rustc_mir_dataflow/src/impls/liveness.rs b/compiler/rustc_mir_dataflow/src/impls/liveness.rs index 9662c19777fe0..7b5214cee930b 100644 --- a/compiler/rustc_mir_dataflow/src/impls/liveness.rs +++ b/compiler/rustc_mir_dataflow/src/impls/liveness.rs @@ -43,6 +43,7 @@ impl<'tcx> AnalysisDomain<'tcx> for MaybeLiveLocals { impl<'tcx> GenKillAnalysis<'tcx> for MaybeLiveLocals { type Idx = Local; + #[instrument(skip(self, trans), level = "debug")] fn statement_effect( &mut self, trans: &mut impl GenKill, @@ -52,6 +53,7 @@ impl<'tcx> GenKillAnalysis<'tcx> for MaybeLiveLocals { TransferFunction(trans).visit_statement(statement, location); } + #[instrument(skip(self, trans), level = "debug")] fn terminator_effect( &mut self, trans: &mut impl GenKill, @@ -61,6 +63,7 @@ impl<'tcx> GenKillAnalysis<'tcx> for MaybeLiveLocals { TransferFunction(trans).visit_terminator(terminator, location); } + #[instrument(skip(self, trans, return_places), level = "debug")] fn call_return_effect( &mut self, trans: &mut impl GenKill, @@ -69,6 +72,7 @@ impl<'tcx> GenKillAnalysis<'tcx> for MaybeLiveLocals { ) { return_places.for_each(|place| { if let Some(local) = place.as_local() { + debug!("killing {:?}", local); trans.kill(local); } }); @@ -94,6 +98,7 @@ impl<'tcx, T> Visitor<'tcx> for TransferFunction<'_, T> where T: GenKill, { + #[instrument(skip(self), level = "debug")] fn visit_place(&mut self, place: &mir::Place<'tcx>, context: PlaceContext, location: Location) { if let PlaceContext::MutatingUse(MutatingUseContext::Yield) = context { // The resume place is evaluated and assigned to only after generator resumes, so its @@ -115,13 +120,17 @@ where self.0.kill(place.local); } } - Some(DefUse::Use) => self.0.gen(place.local), + Some(DefUse::Use) => { + debug!("gen {:?}", place.local); + self.0.gen(place.local); + } None => {} } self.visit_projection(place.as_ref(), context, location); } + #[instrument(skip(self), level = "debug")] fn visit_local(&mut self, local: Local, context: PlaceContext, _: Location) { DefUse::apply(self.0, local.into(), context); } @@ -133,11 +142,13 @@ impl<'tcx, T> Visitor<'tcx> for YieldResumeEffect<'_, T> where T: GenKill, { + #[instrument(skip(self), level = "debug")] fn visit_place(&mut self, place: &mir::Place<'tcx>, context: PlaceContext, location: Location) { DefUse::apply(self.0, *place, context); self.visit_projection(place.as_ref(), context, location); } + #[instrument(skip(self), level = "debug")] fn visit_local(&mut self, local: Local, context: PlaceContext, _: Location) { DefUse::apply(self.0, local.into(), context); } @@ -150,14 +161,22 @@ enum DefUse { } impl DefUse { + #[instrument(skip(trans), level = "debug")] fn apply(trans: &mut impl GenKill, place: Place<'_>, context: PlaceContext) { match DefUse::for_place(place, context) { - Some(DefUse::Def) => trans.kill(place.local), - Some(DefUse::Use) => trans.gen(place.local), + Some(DefUse::Def) => { + debug!("killing {:?}", place.local); + trans.kill(place.local) + } + Some(DefUse::Use) => { + debug!("gen {:?}", place.local); + trans.gen(place.local); + } None => {} } } + #[instrument(skip(), level = "debug")] fn for_place(place: Place<'_>, context: PlaceContext) -> Option { match context { PlaceContext::NonUse(_) => None, diff --git a/compiler/rustc_mir_transform/src/generator.rs b/compiler/rustc_mir_transform/src/generator.rs index 8e9955635499e..a579a9ad12138 100644 --- a/compiler/rustc_mir_transform/src/generator.rs +++ b/compiler/rustc_mir_transform/src/generator.rs @@ -584,6 +584,7 @@ fn locals_live_across_suspend_points<'tcx>( always_live_locals: &BitSet, movable: bool, ) -> LivenessInfo { + debug!("body: {:#?}", body); let body_ref: &Body<'_> = &body; // Calculate when MIR locals have live storage. This gives us an upper bound of their @@ -598,6 +599,8 @@ fn locals_live_across_suspend_points<'tcx>( let borrowed_locals_cursor = rustc_mir_dataflow::ResultsCursor::new(body_ref, &borrowed_locals_results); + let mut blc = rustc_mir_dataflow::ResultsCursor::new(body_ref, &borrowed_locals_results); + // Calculate the locals that are live due to outstanding references or pointers. let live_borrows_results = get_borrowed_locals_results(body_ref, tcx, borrowed_locals_cursor); let mut live_borrows_cursor = BorrowedLocalsResultsCursor::new(body_ref, &live_borrows_results); @@ -643,6 +646,8 @@ fn locals_live_across_suspend_points<'tcx>( // forever. Note that the final liveness is still bounded by the storage liveness // of the local, which happens using the `intersect` operation below. let live_borrowed_locals = live_borrows_cursor.get(loc); + blc.seek_before_primary_effect(loc); + let old_borrowed_locals = blc.get(); let mut live_locals_stmt: BitSet<_> = BitSet::new_empty(body.local_decls.len()); liveness.seek_before_primary_effect(loc); @@ -653,6 +658,7 @@ fn locals_live_across_suspend_points<'tcx>( storage_req.union(requires_storage_cursor.get()); debug!(?live_borrowed_locals); + debug!(?old_borrowed_locals); debug!(?live_locals_stmt); debug!(?storage_req); From 220898194f6462a70418fc975ff02fb7184f665a Mon Sep 17 00:00:00 2001 From: b-naber Date: Thu, 29 Jun 2023 15:30:12 +0000 Subject: [PATCH 18/20] make LiveBorrows Clone --- .../src/impls/live_borrows.rs | 111 +++++++++--------- 1 file changed, 56 insertions(+), 55 deletions(-) diff --git a/compiler/rustc_mir_dataflow/src/impls/live_borrows.rs b/compiler/rustc_mir_dataflow/src/impls/live_borrows.rs index 1758d527fef8c..df54d71531cd6 100644 --- a/compiler/rustc_mir_dataflow/src/impls/live_borrows.rs +++ b/compiler/rustc_mir_dataflow/src/impls/live_borrows.rs @@ -778,9 +778,7 @@ impl<'a, 'tcx> Visitor<'tcx> for BorrowDependencies<'a, 'tcx> { Rvalue::Ref(_, borrow_kind, borrowed_place) => { let mutbl = match borrow_kind { BorrowKind::Mut { .. } => Mutability::Mut, - BorrowKind::Shallow | BorrowKind::Shared | BorrowKind::Unique => { - Mutability::Not - } + BorrowKind::Shallow | BorrowKind::Shared => Mutability::Not, }; self.handle_rvalue_ref_or_ptr(borrowed_place, mutbl) @@ -923,8 +921,8 @@ where MaybeBorrowedLocals, &'a Results<'tcx, MaybeBorrowedLocals>, >, + dep_graph: BorrowDepGraph, ) -> Self { - let dep_graph = &borrows_analysis_results.analysis.borrow_deps.dep_graph; let borrowed_local_to_locals_to_keep_alive = Self::get_locals_to_keep_alive_map(dep_graph); Self { borrows_analysis_results, @@ -938,14 +936,14 @@ where /// Uses the dependency graph to find all locals that we need to keep live for a given /// `Node` (or more specically the `Local` corresponding to that `Node`). #[instrument(skip(dep_graph), level = "debug")] - fn get_locals_to_keep_alive_map<'b>( - dep_graph: &'b BorrowDepGraph, + fn get_locals_to_keep_alive_map( + dep_graph: BorrowDepGraph, ) -> FxHashMap> { let mut borrows_to_locals: FxHashMap> = Default::default(); let mut memoization_map: FxHashMap> = Default::default(); // create SCCs for dependency graph and map each local to its SCC. - let sccs: Sccs = Sccs::new(dep_graph); + let sccs: Sccs = Sccs::new(&dep_graph); // Contains the Locals to keep alive for each scc. let mut scc_to_locals_to_keep_alive: FxHashMap> = @@ -1096,11 +1094,28 @@ pub fn get_borrowed_locals_results<'a, 'mir, 'tcx>( } } - let live_borrows = LiveBorrows::new(body, tcx, borrow_deps); - let results = + let reborrows_map = borrow_deps.reborrows_map; + let local_to_nodekind = borrow_deps + .dep_graph + .all_nodes() + .iter() + .map(|node| { + let nodekind = node.data; + let local = nodekind.get_local(); + + (local, nodekind) + }) + .collect::>(); + + let live_borrows = LiveBorrows::new(body, tcx, local_to_nodekind, reborrows_map); + let live_borrows_results = live_borrows.into_engine(tcx, body).pass_name("borrowed_locals").iterate_to_fixpoint(); - BorrowedLocalsResults::new(results, maybe_borrowed_locals_cursor) + BorrowedLocalsResults::new( + live_borrows_results, + maybe_borrowed_locals_cursor, + borrow_deps.dep_graph, + ) } /// The `ResultsCursor` equivalent for the borrowed locals analysis. Since this analysis doesn't @@ -1178,19 +1193,22 @@ impl<'a, 'mir, 'tcx> BorrowedLocalsResultsCursor<'a, 'mir, 'tcx> { /// Performs a liveness analysis for borrows and raw pointers. This analysis also tracks `Local`s /// corresponding to `Node`s of kind `NodeKind::LocalWithRefs`, as these could potentially refer to /// or include references, pointers or exposed pointers. +#[derive(Clone)] pub struct LiveBorrows<'mir, 'tcx> { body: &'mir Body<'tcx>, tcx: TyCtxt<'tcx>, - borrow_deps: BorrowDependencies<'mir, 'tcx>, + local_to_nodekind: FxHashMap, + reborrows_map: FxHashMap, } impl<'mir, 'tcx> LiveBorrows<'mir, 'tcx> { fn new( body: &'mir Body<'tcx>, tcx: TyCtxt<'tcx>, - borrow_deps: BorrowDependencies<'mir, 'tcx>, + local_to_nodekind: FxHashMap, + reborrows_map: FxHashMap, ) -> Self { - LiveBorrows { body, tcx, borrow_deps } + LiveBorrows { body, tcx, local_to_nodekind, reborrows_map } } fn transfer_function<'b, T>( @@ -1201,11 +1219,18 @@ impl<'mir, 'tcx> LiveBorrows<'mir, 'tcx> { body: self.body, tcx: self.tcx, _trans: trans, - borrow_deps: &self.borrow_deps, + local_to_nodekind: &self.local_to_nodekind, + reborrows_map: &self.reborrows_map, } } } +impl<'mir, 'tcx> crate::CloneAnalysis for LiveBorrows<'mir, 'tcx> { + fn clone_analysis(&self) -> Self { + self.clone() + } +} + impl<'a, 'tcx> AnalysisDomain<'tcx> for LiveBorrows<'a, 'tcx> { type Domain = BitSet; type Direction = Backward; @@ -1258,7 +1283,8 @@ struct TransferFunction<'a, 'b, 'c, 'tcx, T> { body: &'a Body<'tcx>, tcx: TyCtxt<'tcx>, _trans: &'b mut T, - borrow_deps: &'c BorrowDependencies<'a, 'tcx>, + local_to_nodekind: &'c FxHashMap, + reborrows_map: &'c FxHashMap, } impl<'a, 'b, 'c, 'tcx, T> TransferFunction<'a, 'b, 'c, 'tcx, T> @@ -1267,10 +1293,10 @@ where { fn gen(&mut self, local: Local) { debug!("gen {:?}", local); - if let Some(reborrowed_local) = self.borrow_deps.reborrows_map.get(&local) { + if let Some(reborrowed_local) = self.reborrows_map.get(&local) { self._trans.gen(*reborrowed_local); } else { - if self.borrow_deps.dep_graph.locals_to_node_indexes.get(&local).is_some() { + if self.local_to_nodekind.get(&local).is_some() { self._trans.gen(local) } } @@ -1278,10 +1304,10 @@ where fn kill(&mut self, local: Local) { debug!("killing {:?}", local); - if let Some(reborrowed_local) = self.borrow_deps.reborrows_map.get(&local) { + if let Some(reborrowed_local) = self.reborrows_map.get(&local) { self._trans.kill(*reborrowed_local); } else { - if self.borrow_deps.dep_graph.locals_to_node_indexes.get(&local).is_some() { + if self.local_to_nodekind.get(&local).is_some() { self._trans.kill(local) } } @@ -1312,16 +1338,10 @@ where self.visit_rvalue(&assign.1, location); } _ => { - if let Some(node_idx) = self - .borrow_deps - .dep_graph - .locals_to_node_indexes - .get(&lhs_place.local) + if let Some(NodeKind::LocalWithRefs(_)) = + self.local_to_nodekind.get(&lhs_place.local) { - let node = self.borrow_deps.dep_graph.node(*node_idx); - if let NodeKind::LocalWithRefs(_) = node.data { - self.kill(lhs_place.local); - } + self.kill(lhs_place.local); } self.super_assign(&assign.0, &assign.1, location); } @@ -1357,13 +1377,8 @@ where self.gen(local); } _ => { - if let Some(node_idx) = - self.borrow_deps.dep_graph.locals_to_node_indexes.get(&local) - { - let node = self.borrow_deps.dep_graph.node(*node_idx); - if matches!(node.data, NodeKind::LocalWithRefs(_)) { - self.gen(local); - } + if let Some(NodeKind::LocalWithRefs(_)) = self.local_to_nodekind.get(&local) { + self.gen(local); } } } @@ -1392,20 +1407,13 @@ where for arg in args { match arg { Operand::Copy(place) | Operand::Move(place) => { - if let Some(node_idx) = self - .borrow_deps - .dep_graph - .locals_to_node_indexes - .get(&place.local) + if let Some(NodeKind::LocalWithRefs(_)) = + self.local_to_nodekind.get(&place.local) { - let node = self.borrow_deps.dep_graph.node(*node_idx); - // these are `Local`s that contain references/pointers or are raw pointers // that were assigned to raw pointers, which were cast to usize. Since the // function call is free to use these in any form, we need to gen them here. - if let NodeKind::LocalWithRefs(_) = node.data { - self.gen(place.local); - } + self.gen(place.local); } else { self.super_operand(arg, location) } @@ -1424,20 +1432,13 @@ where match value { Operand::Copy(place) | Operand::Move(place) => { - if let Some(node_idx) = self - .borrow_deps - .dep_graph - .locals_to_node_indexes - .get(&place.local) + if let Some(NodeKind::LocalWithRefs(_)) = + self.local_to_nodekind.get(&place.local) { - let node = self.borrow_deps.dep_graph.node(*node_idx); - // these are `Local`s that contain references/pointers or are raw pointers // that were assigned to raw pointers, which were cast to usize. Since the // function call is free to use these in any form, we need to gen them here. - if let NodeKind::LocalWithRefs(_) = node.data { - self.gen(place.local); - } + self.gen(place.local); } else { self.super_operand(value, location) } From 9fb6c16e9cbf10b1a8d0e1d910221f25b4e7521f Mon Sep 17 00:00:00 2001 From: b-naber Date: Thu, 29 Jun 2023 19:14:18 +0000 Subject: [PATCH 19/20] make BorrowedLocalsResultsCursor compatible with #108293 --- .../src/impls/live_borrows.rs | 89 ++++++++----------- .../src/impls/storage_liveness.rs | 23 +++-- compiler/rustc_mir_transform/src/generator.rs | 24 +++-- 3 files changed, 59 insertions(+), 77 deletions(-) diff --git a/compiler/rustc_mir_dataflow/src/impls/live_borrows.rs b/compiler/rustc_mir_dataflow/src/impls/live_borrows.rs index df54d71531cd6..4f64bb4178e3b 100644 --- a/compiler/rustc_mir_dataflow/src/impls/live_borrows.rs +++ b/compiler/rustc_mir_dataflow/src/impls/live_borrows.rs @@ -242,11 +242,9 @@ use super::*; -use crate::framework::{Analysis, Results, ResultsCursor}; +use crate::framework::{Analysis, Results, ResultsClonedCursor, ResultsCursor}; use crate::impls::MaybeBorrowedLocals; -use crate::{ - AnalysisDomain, Backward, CallReturnPlaces, GenKill, GenKillAnalysis, ResultsRefCursor, -}; +use crate::{AnalysisDomain, Backward, CallReturnPlaces, CloneAnalysis, GenKill, GenKillAnalysis}; use rustc_data_structures::fx::{FxHashMap, FxHashSet}; use rustc_data_structures::graph; use rustc_data_structures::graph::implementation::{Graph, NodeIndex}; @@ -257,7 +255,6 @@ use rustc_middle::mir::*; use rustc_middle::ty::TypeVisitable; use rustc_middle::ty::{self, Ty, TypeSuperVisitable}; -use std::cell::RefCell; use std::ops::{ControlFlow, Deref, DerefMut}; // FIXME Properly determine a reasonable value @@ -890,7 +887,7 @@ impl<'a, 'tcx> Visitor<'tcx> for BorrowDependencies<'a, 'tcx> { } } -pub struct BorrowedLocalsResults<'a, 'mir, 'tcx> { +pub struct BorrowedLocalsResults<'mir, 'tcx> { /// the results of the liveness analysis of `LiveBorrows` borrows_analysis_results: Results<'tcx, LiveBorrows<'mir, 'tcx>>, @@ -900,36 +897,26 @@ pub struct BorrowedLocalsResults<'a, 'mir, 'tcx> { /// to the set of `Local`s that are borrowed through those references, pointers or composite values. borrowed_local_to_locals_to_keep_alive: FxHashMap>, - /// The results cursor of the `MaybeBorrowedLocals` analysis. Needed as an upper bound, since + /// The results of the `MaybeBorrowedLocals` analysis. Needed as an upper bound, since /// to ensure soundness the `LiveBorrows` analysis would keep more `Local`s alive than /// strictly necessary. - maybe_borrowed_locals_results_cursor: RefCell< - ResultsCursor<'mir, 'tcx, MaybeBorrowedLocals, &'a Results<'tcx, MaybeBorrowedLocals>>, - >, + maybe_borrowed_locals_results: Results<'tcx, MaybeBorrowedLocals>, } -impl<'a, 'mir, 'tcx> BorrowedLocalsResults<'a, 'mir, 'tcx> +impl<'mir, 'tcx> BorrowedLocalsResults<'mir, 'tcx> where 'tcx: 'mir, - 'tcx: 'a, { fn new( borrows_analysis_results: Results<'tcx, LiveBorrows<'mir, 'tcx>>, - maybe_borrowed_locals_results_cursor: ResultsCursor< - 'mir, - 'tcx, - MaybeBorrowedLocals, - &'a Results<'tcx, MaybeBorrowedLocals>, - >, + maybe_borrowed_locals_results: Results<'tcx, MaybeBorrowedLocals>, dep_graph: BorrowDepGraph, ) -> Self { let borrowed_local_to_locals_to_keep_alive = Self::get_locals_to_keep_alive_map(dep_graph); Self { borrows_analysis_results, borrowed_local_to_locals_to_keep_alive, - maybe_borrowed_locals_results_cursor: RefCell::new( - maybe_borrowed_locals_results_cursor, - ), + maybe_borrowed_locals_results, } } @@ -1051,17 +1038,12 @@ where /// The function gets the results of the borrowed locals analysis in this module. See the module /// doc-comment for information on what exactly this analysis does. -#[instrument(skip(tcx, maybe_borrowed_locals_cursor, body), level = "debug")] -pub fn get_borrowed_locals_results<'a, 'mir, 'tcx>( +#[instrument(skip(tcx, maybe_borrowed_locals, body), level = "debug")] +pub fn get_borrowed_locals_results<'mir, 'tcx>( body: &'mir Body<'tcx>, tcx: TyCtxt<'tcx>, - maybe_borrowed_locals_cursor: ResultsCursor< - 'mir, - 'tcx, - MaybeBorrowedLocals, - &'a Results<'tcx, MaybeBorrowedLocals>, - >, -) -> BorrowedLocalsResults<'a, 'mir, 'tcx> { + maybe_borrowed_locals: Results<'tcx, MaybeBorrowedLocals>, +) -> BorrowedLocalsResults<'mir, 'tcx> { debug!("body: {:#?}", body); let mut borrow_deps = BorrowDependencies::new(body.local_decls(), tcx); @@ -1111,11 +1093,7 @@ pub fn get_borrowed_locals_results<'a, 'mir, 'tcx>( let live_borrows_results = live_borrows.into_engine(tcx, body).pass_name("borrowed_locals").iterate_to_fixpoint(); - BorrowedLocalsResults::new( - live_borrows_results, - maybe_borrowed_locals_cursor, - borrow_deps.dep_graph, - ) + BorrowedLocalsResults::new(live_borrows_results, maybe_borrowed_locals, borrow_deps.dep_graph) } /// The `ResultsCursor` equivalent for the borrowed locals analysis. Since this analysis doesn't @@ -1123,7 +1101,16 @@ pub fn get_borrowed_locals_results<'a, 'mir, 'tcx>( /// the `get` method without the need for any prior 'seek' calls. pub struct BorrowedLocalsResultsCursor<'a, 'mir, 'tcx> { // The cursor for the liveness analysis performed by `LiveBorrows` - borrows_analysis_cursor: ResultsRefCursor<'a, 'mir, 'tcx, LiveBorrows<'mir, 'tcx>>, + borrows_analysis_cursor: ResultsCursor< + 'mir, + 'tcx, + LiveBorrows<'mir, 'tcx>, + Results< + 'tcx, + LiveBorrows<'mir, 'tcx>, + &'a rustc_index::IndexVec>, + >, + >, // Maps each `Local` corresponding to a reference or pointer to the set of `Local`s // that are borrowed through the ref/ptr. Additionally contains entries for `Local`s @@ -1132,14 +1119,13 @@ pub struct BorrowedLocalsResultsCursor<'a, 'mir, 'tcx> { borrowed_local_to_locals_to_keep_alive: &'a FxHashMap>, // the cursor of the conservative borrowed locals analysis - maybe_borrowed_locals_results_cursor: &'a RefCell< - ResultsCursor<'mir, 'tcx, MaybeBorrowedLocals, &'a Results<'tcx, MaybeBorrowedLocals>>, - >, + maybe_borrowed_locals_results_cursor: ResultsClonedCursor<'a, 'mir, 'tcx, MaybeBorrowedLocals>, } impl<'a, 'mir, 'tcx> BorrowedLocalsResultsCursor<'a, 'mir, 'tcx> { - pub fn new(body: &'mir Body<'tcx>, results: &'a BorrowedLocalsResults<'a, 'mir, 'tcx>) -> Self { - let mut cursor = ResultsCursor::new(body, &results.borrows_analysis_results); + pub fn new(body: &'mir Body<'tcx>, results: &'a BorrowedLocalsResults<'mir, 'tcx>) -> Self { + let mut cursor = + ResultsCursor::new(body, results.borrows_analysis_results.clone_analysis()); // We don't care about the order of the blocks, only about the result at a given location. // This statement is necessary since we're performing a backward analysis in `LiveBorrows`, @@ -1149,7 +1135,10 @@ impl<'a, 'mir, 'tcx> BorrowedLocalsResultsCursor<'a, 'mir, 'tcx> { Self { borrows_analysis_cursor: cursor, borrowed_local_to_locals_to_keep_alive: &results.borrowed_local_to_locals_to_keep_alive, - maybe_borrowed_locals_results_cursor: &results.maybe_borrowed_locals_results_cursor, + maybe_borrowed_locals_results_cursor: ResultsClonedCursor::new( + body, + results.maybe_borrowed_locals_results.clone_analysis(), + ), } } @@ -1178,11 +1167,9 @@ impl<'a, 'mir, 'tcx> BorrowedLocalsResultsCursor<'a, 'mir, 'tcx> { // use results of conservative analysis as an "upper bound" on the borrowed locals. This // is necessary since to guarantee soundness for this analysis we would have to keep // more `Local`s alive than strictly necessary. - let mut maybe_borrowed_locals_cursor = - self.maybe_borrowed_locals_results_cursor.borrow_mut(); - maybe_borrowed_locals_cursor.allow_unreachable(); - maybe_borrowed_locals_cursor.seek_before_primary_effect(loc); - let upper_bound_borrowed_locals = maybe_borrowed_locals_cursor.get(); + self.maybe_borrowed_locals_results_cursor.allow_unreachable(); + self.maybe_borrowed_locals_results_cursor.seek_before_primary_effect(loc); + let upper_bound_borrowed_locals = self.maybe_borrowed_locals_results_cursor.get(); borrowed_locals.intersect(upper_bound_borrowed_locals); debug!(?borrowed_locals); @@ -1225,7 +1212,7 @@ impl<'mir, 'tcx> LiveBorrows<'mir, 'tcx> { } } -impl<'mir, 'tcx> crate::CloneAnalysis for LiveBorrows<'mir, 'tcx> { +impl<'mir, 'tcx> CloneAnalysis for LiveBorrows<'mir, 'tcx> { fn clone_analysis(&self) -> Self { self.clone() } @@ -1251,7 +1238,7 @@ impl<'a, 'tcx> GenKillAnalysis<'tcx> for LiveBorrows<'a, 'tcx> { #[instrument(skip(self, trans), level = "debug")] fn statement_effect( - &self, + &mut self, trans: &mut impl GenKill, statement: &mir::Statement<'tcx>, location: Location, @@ -1261,7 +1248,7 @@ impl<'a, 'tcx> GenKillAnalysis<'tcx> for LiveBorrows<'a, 'tcx> { #[instrument(skip(self, trans), level = "debug")] fn terminator_effect( - &self, + &mut self, trans: &mut impl GenKill, terminator: &mir::Terminator<'tcx>, location: Location, @@ -1270,7 +1257,7 @@ impl<'a, 'tcx> GenKillAnalysis<'tcx> for LiveBorrows<'a, 'tcx> { } fn call_return_effect( - &self, + &mut self, _trans: &mut impl GenKill, _block: mir::BasicBlock, _return_places: CallReturnPlaces<'_, 'tcx>, diff --git a/compiler/rustc_mir_dataflow/src/impls/storage_liveness.rs b/compiler/rustc_mir_dataflow/src/impls/storage_liveness.rs index 90e13187a38b0..31aae40eac89d 100644 --- a/compiler/rustc_mir_dataflow/src/impls/storage_liveness.rs +++ b/compiler/rustc_mir_dataflow/src/impls/storage_liveness.rs @@ -1,6 +1,6 @@ pub use super::*; -use crate::impls::{BorrowedLocalsResults, BorrowedLocalsResultsCursor}; +use crate::impls::BorrowedLocalsResultsCursor; use crate::{CallReturnPlaces, GenKill}; use rustc_middle::mir::visit::{NonMutatingUseContext, PlaceContext, Visitor}; use rustc_middle::mir::*; @@ -151,18 +151,15 @@ impl<'tcx> crate::GenKillAnalysis<'tcx> for MaybeStorageDead { /// given location; i.e. whether its storage can go away without being observed. pub struct MaybeRequiresStorage<'a, 'mir, 'tcx> { body: &'mir Body<'tcx>, - borrowed_locals: RefCell>, + borrowed_locals_cursor: BorrowedLocalsResultsCursor<'a, 'mir, 'tcx>, } impl<'a, 'mir, 'tcx> MaybeRequiresStorage<'a, 'mir, 'tcx> { pub fn new( body: &'mir Body<'tcx>, - borrowed_locals: &'a BorrowedLocalsResults<'a, 'mir, 'tcx>, + borrowed_locals_cursor: BorrowedLocalsResultsCursor<'a, 'mir, 'tcx>, ) -> Self { - MaybeRequiresStorage { - body, - borrowed_locals: RefCell::new(BorrowedLocalsResultsCursor::new(body, borrowed_locals)), - } + MaybeRequiresStorage { body, borrowed_locals_cursor } } } @@ -195,7 +192,7 @@ impl<'a, 'mir, 'tcx> crate::GenKillAnalysis<'tcx> for MaybeRequiresStorage<'a, ' loc: Location, ) { // If a place is borrowed in a statement, it needs storage for that statement. - let borrowed_locals_at_loc = self.borrowed_locals.borrow_mut().get(loc); + let borrowed_locals_at_loc = self.borrowed_locals_cursor.get(loc); for i in 0..self.body.local_decls().len() { let local = Local::from_usize(i); if borrowed_locals_at_loc.contains(local) { @@ -246,7 +243,7 @@ impl<'a, 'mir, 'tcx> crate::GenKillAnalysis<'tcx> for MaybeRequiresStorage<'a, ' loc: Location, ) { // If a place is borrowed in a statement, it needs storage for that statement. - let borrowed_locals_at_loc = self.borrowed_locals.borrow_mut().get(loc); + let borrowed_locals_at_loc = self.borrowed_locals_cursor.get(loc); for i in 0..self.body.local_decls().len() { let local = Local::from_usize(i); if borrowed_locals_at_loc.contains(local) { @@ -359,14 +356,14 @@ impl<'a, 'mir, 'tcx> crate::GenKillAnalysis<'tcx> for MaybeRequiresStorage<'a, ' impl<'a, 'mir, 'tcx> MaybeRequiresStorage<'a, 'mir, 'tcx> { /// Kill locals that are fully moved and have not been borrowed. fn check_for_move(&mut self, trans: &mut impl GenKill, loc: Location) { - let body = self.borrowed_locals.body(); - let mut visitor = MoveVisitor { trans, borrowed_locals: &mut self.borrowed_locals }; + let body = self.body; + let mut visitor = MoveVisitor { trans, borrowed_locals: &mut self.borrowed_locals_cursor }; visitor.visit_location(body, loc); } } struct MoveVisitor<'a, 'b, 'mir, 'tcx, T> { - borrowed_locals: &'a RefCell>, + borrowed_locals: &'a mut BorrowedLocalsResultsCursor<'b, 'mir, 'tcx>, trans: &'a mut T, } @@ -377,7 +374,7 @@ where #[instrument(skip(self), level = "debug")] fn visit_local(&mut self, local: Local, context: PlaceContext, loc: Location) { if PlaceContext::NonMutatingUse(NonMutatingUseContext::Move) == context { - let borrowed_locals = self.borrowed_locals.borrow_mut().get(loc); + let borrowed_locals = self.borrowed_locals.get(loc); debug!(?borrowed_locals); if !borrowed_locals.contains(local) { self.trans.kill(local); diff --git a/compiler/rustc_mir_transform/src/generator.rs b/compiler/rustc_mir_transform/src/generator.rs index a579a9ad12138..1426e12cbfaa7 100644 --- a/compiler/rustc_mir_transform/src/generator.rs +++ b/compiler/rustc_mir_transform/src/generator.rs @@ -594,25 +594,26 @@ fn locals_live_across_suspend_points<'tcx>( .iterate_to_fixpoint() .into_results_cursor(body_ref); + // conservative upper bound on borrowed locals that is needed in the `LiveBorrows` analysis let borrowed_locals_results = MaybeBorrowedLocals.into_engine(tcx, body_ref).pass_name("generator").iterate_to_fixpoint(); - let borrowed_locals_cursor = - rustc_mir_dataflow::ResultsCursor::new(body_ref, &borrowed_locals_results); - - let mut blc = rustc_mir_dataflow::ResultsCursor::new(body_ref, &borrowed_locals_results); // Calculate the locals that are live due to outstanding references or pointers. - let live_borrows_results = get_borrowed_locals_results(body_ref, tcx, borrowed_locals_cursor); + let live_borrows_results = get_borrowed_locals_results(body_ref, tcx, borrowed_locals_results); + let mut live_borrows_cursor = BorrowedLocalsResultsCursor::new(body_ref, &live_borrows_results); // Calculate the MIR locals that we actually need to keep storage around // for. - let requires_storage_results = MaybeRequiresStorage::new(body, &live_borrows_results) - .into_engine(tcx, body_ref) - .iterate_to_fixpoint(); + let mut requires_storage_results = MaybeRequiresStorage::new( + body, + BorrowedLocalsResultsCursor::new(body_ref, &live_borrows_results), + ) + .into_engine(tcx, body_ref) + .iterate_to_fixpoint(); let mut requires_storage_cursor = - rustc_mir_dataflow::ResultsCursor::new(body_ref, &requires_storage_results); + rustc_mir_dataflow::ResultsRefCursor::new(body_ref, &mut requires_storage_results); // Calculate the liveness of MIR locals ignoring borrows. let mut liveness = MaybeLiveLocals @@ -646,8 +647,6 @@ fn locals_live_across_suspend_points<'tcx>( // forever. Note that the final liveness is still bounded by the storage liveness // of the local, which happens using the `intersect` operation below. let live_borrowed_locals = live_borrows_cursor.get(loc); - blc.seek_before_primary_effect(loc); - let old_borrowed_locals = blc.get(); let mut live_locals_stmt: BitSet<_> = BitSet::new_empty(body.local_decls.len()); liveness.seek_before_primary_effect(loc); @@ -658,7 +657,6 @@ fn locals_live_across_suspend_points<'tcx>( storage_req.union(requires_storage_cursor.get()); debug!(?live_borrowed_locals); - debug!(?old_borrowed_locals); debug!(?live_locals_stmt); debug!(?storage_req); @@ -770,7 +768,7 @@ fn compute_storage_conflicts<'a, 'mir, 'tcx>( body: &'mir Body<'tcx>, saved_locals: &GeneratorSavedLocals, always_live_locals: BitSet, - requires_storage: rustc_mir_dataflow::Results<'tcx, MaybeRequiresStorage<'a, 'mir, 'tcx>>, + mut requires_storage: rustc_mir_dataflow::Results<'tcx, MaybeRequiresStorage<'a, 'mir, 'tcx>>, ) -> BitMatrix { assert_eq!(body.local_decls.len(), saved_locals.domain_size()); From 70b281fd11fd3beb00885bdeb059c7c521c83b00 Mon Sep 17 00:00:00 2001 From: b-naber Date: Thu, 29 Jun 2023 19:40:28 +0000 Subject: [PATCH 20/20] fix module doc comment --- .../src/impls/live_borrows.rs | 97 +++++++++++-------- 1 file changed, 55 insertions(+), 42 deletions(-) diff --git a/compiler/rustc_mir_dataflow/src/impls/live_borrows.rs b/compiler/rustc_mir_dataflow/src/impls/live_borrows.rs index 4f64bb4178e3b..2a9d3c72f06e5 100644 --- a/compiler/rustc_mir_dataflow/src/impls/live_borrows.rs +++ b/compiler/rustc_mir_dataflow/src/impls/live_borrows.rs @@ -3,6 +3,7 @@ //! issues. Exposed raw pointers (i.e. those cast to `usize`) and function calls would make this simple //! analysis unsound, so we have to handle them as follows: //! +//! ```text //! * exposed pointers (i.e. a cast of a raw pointer to `usize`) //! => These count towards the liveness of the `Local` that is behind the raw pointer, e.g. for //! `_5 = _4 as usize` (where `_4 = AddressOf(_3)`), we keep `_3` alive on any use site of _5. @@ -28,6 +29,7 @@ //! Let `Locals2` be the set of `Local`s that need to be kept alive due to the borrows corresponding to `Op2` //! Then we need to start tracking all `Local`s in `Locals2` for the borrow corresponding to `Op1`, since the `Place` //! corresponding to `Op2` might be moved into `Op1` in then call. +//! ``` //! //! As an example for what this analysis does: //! @@ -67,20 +69,19 @@ //! yield 3; // live locals: [`Local4`] //! //! takes_and_returns_ref(bar3_ref); // live locals: [`Local4`] -//! } +//! } //! } //! ``` //! //! Following is a description of the algorithm: +//! ```text //! 1. build a dependency graph that relates `Local`s based on their borrowing relationships. //! As an example if we have something like this (in a simplified MIR representation): //! -//! ```ignore //! _4 = Bar {} //! _5 = Ref(_4) //! ... //! _10 = f(_5) -//! ``` //! //! Then we add edges from `_5` to `_4` and `_4` to `_5` (for an explanation of why the edge //! from `_4` to `_5` is necessary see the comment in `handle_ravlue_or_ptr`) and from @@ -95,6 +96,7 @@ //! in that range we traverse our dependency graph and look for nodes that correspond to borrowed //! `Local`s ("leaf nodes"). In our example we would find an edge from `_5` to `_4`, which is a leaf //! node and hence we keep `_4` live over that range. +//! ``` //! //! There are some corner cases we need to look out for to make this analysis sound. Let's look //! at each of the three steps in more detail and elaborate how these steps deal with these corner @@ -104,6 +106,7 @@ //! //! The `Node`s in the dependency graph include data values of type `NodeKind`. `NodeKind` has //! three variants: `Local`, `Borrow` and `LocalWithRefs`. +//! ```text //! * `NodeKind::Local` is used for `Local`s that are borrowed somewhere (`_4` in our example), but aren't //! themselves references or pointers. //! * `NodeKind::Borrow` is used for `Local`s that correspond to borrows (`_5` in our example). We equate @@ -111,51 +114,51 @@ //! * `NodeKind::LocalWithRefs` is used for `Local`s that aren't themselves refs/ptrs, but contain //! `Local`s that correspond to refs/ptrs or other `Local`s with `Node`s of kind `NodeKind::LocalWithRef`s. //! `LocalWithRefs` is also used for exposed pointers. -//! Let's look at an example: +//! ``` //! -//! ```ignore -//! _4 = Bar {} -//! _5 = Ref(_4) -//! _6 = Foo(..)(move _5) -//! ... -//! _7 = (_6.0) -//! ``` +//! Let's look at an example: //! -//! In this example `_6` would be given `NodeKind::LocalWithRefs` and our graph would look -//! as follows: +//! ```text +//! _4 = Bar {} +//! _5 = Ref(_4) +//! _6 = Foo(..)(move _5) +//! ... +//! _7 = (_6.0) //! -//! `_7` (NodeKind::Borrow) <-> `_6` (NodeKind::LocalWithRefs) <-> `_5` (NodeKind::Borrow) <-> `_4` (NodeKind::Local) +//! In this example `_6` would be given `NodeKind::LocalWithRefs` and our graph would look +//! as follows: //! -//! On the one hand we need to treat `Local`s with `Node`s of kind `NodeKind::LocalWithRefs` similarly -//! to how we treat `Local`s with `Node`s of kind `NodeKind::Local`, in the sense that if they are -//! borrowed we want to keep them live over the live range of the borrow. But on the other hand we -//! want to also treat them like `Local`s with `Node`s of kind `NodeKind::Borrow` as they ultimately -//! could also contain references or pointers that refer to other `Local`s. So we want a -//! path in the graph from a `NodeKind::LocalWithRef`s node to the `NodeKind::Local` nodes, whose borrows -//! they might contain. +//! `_7` (NodeKind::Borrow) <-> `_6` (NodeKind::LocalWithRefs) <-> `_5` (NodeKind::Borrow) <-> `_4` (NodeKind::Local) //! -//! Additionally `NodeKind::LocalWithRefs` is also used for raw pointers that are cast to -//! `usize`: +//! On the one hand we need to treat `Local`s with `Node`s of kind `NodeKind::LocalWithRefs` similarly +//! to how we treat `Local`s with `Node`s of kind `NodeKind::Local`, in the sense that if they are +//! borrowed we want to keep them live over the live range of the borrow. But on the other hand we +//! want to also treat them like `Local`s with `Node`s of kind `NodeKind::Borrow` as they ultimately +//! could also contain references or pointers that refer to other `Local`s. So we want a +//! path in the graph from a `NodeKind::LocalWithRef`s node to the `NodeKind::Local` nodes, whose borrows +//! they might contain. //! -//! ```ignore -//! _4 = Bar {} -//! _5 = AddressOf(_4) -//! _6 = _5 as usize -//! _7 = Aggregate(..) (move _6) -//! _8 = (_7.0) -//! ``` +//! Additionally `NodeKind::LocalWithRefs` is also used for raw pointers that are cast to +//! `usize`: //! -//! In this example our graph would have the following edges: -//! * `_5` (Borrow) <-> `_4` (Local) -//! * `_6` (LocalWithRefs) <-> `_5` (Borrow) -//! * `_7` (LocalWithRefs) <-> `_6` (LocalWithRefs) -//! * `_8` (LocalWithRefs) <-> `_7` (LocalWithRefs) +//! _4 = Bar {} +//! _5 = AddressOf(_4) +//! _6 = _5 as usize +//! _7 = Aggregate(..) (move _6) +//! _8 = (_7.0) //! -//! We also have to be careful about dealing with `Terminator`s. Whenever we pass references, -//! pointers or `Local`s with `NodeKind::LocalWithRefs` to a `TerminatorKind::Call` or -//! `TerminatorKind::Yield`, the destination `Place` or resume place, resp., might contain -//! these references, pointers or `LocalWithRefs`, hence we have to be conservative -//! and keep the `destination` `Local` and `resume_arg` `Local` live. +//! In this example our graph would have the following edges: +//! * `_5` (Borrow) <-> `_4` (Local) +//! * `_6` (LocalWithRefs) <-> `_5` (Borrow) +//! * `_7` (LocalWithRefs) <-> `_6` (LocalWithRefs) +//! * `_8` (LocalWithRefs) <-> `_7` (LocalWithRefs) +//! +//! We also have to be careful about dealing with `Terminator`s. Whenever we pass references, +//! pointers or `Local`s with `NodeKind::LocalWithRefs` to a `TerminatorKind::Call` or +//! `TerminatorKind::Yield`, the destination `Place` or resume place, resp., might contain +//! these references, pointers or `LocalWithRefs`, hence we have to be conservative +//! and keep the `destination` `Local` and `resume_arg` `Local` live. +//! ``` //! //! 2. Liveness analysis for borrows //! @@ -163,7 +166,7 @@ //! So we `gen` at any use site, which are either direct uses of these `Local`s or projections that contain //! these `Local`s. So e.g.: //! -//! ```ignore +//! ```text //! 1. _3 = Foo {} //! 2. _4 = Bar {} //! 3. _5 = Ref(_3) @@ -173,7 +176,6 @@ //! 7. _9 = (_8.0) //! 8. _10 = const 5 //! 9. (_7.0) = move _10 -//! ``` //! //! * `_5` is live from stmt 3 to stmt 9 //! * `_6` is live from stmt 4 to stmt 7 @@ -181,28 +183,34 @@ //! analyis. It's live from stmt 5 to stmt 9 //! * `_8` is a `Local` of kind `LocalWithRefs`. It's live from 6. to 7. //! * `_9` is a `Local` of kind `LocalWithRefs`. It's live at 7. +//! ``` //! //! 3. Determining which `Local`s are borrowed //! //! Let's use our last example again. The dependency graph for that example looks as follows: //! +//! ```text //! `_5` (Borrow) <-> `_3` (Local) //! `_6` (Borrow) <-> `_4` (Local) //! `_7` (LocalWithRef) <-> `_5` (Borrow) //! `_8` (LocalWithRef) -> `_6` (Borrow) //! `_9` (LocalWithRef) <-> `_8` (LocalWithRef) //! `_7` (LocalWithRef) <-> `_10` (Local) +//! ``` //! //! We then construct a strongly connected components graph from the dependency graph, yielding: //! +//! ```text //! SCC1: [_3, _5, _7, _10] //! SCC2: [_4, _6, _8, _9] +//! ``` //! //! Now for each statement in the `Body` we check which refs/ptrs or `LocalWithRefs` are live at that statement //! and then perform a depth-first search in the scc graph, collecting all `Local`s that need to be kept alive //! (`Local`s that have `Node`s in the graph of either `NodeKind::Local` or `NodeKind::LocalWithRefs`). //! So at each of those statements we have the following `Local`s that are live due to borrows: //! +//! ```text //! 1. {} //! 2. {} //! 3. {_3} @@ -212,12 +220,14 @@ //! 7. {_3, _4, _7, _8, _9} //! 8. {_3, _7} //! 9. {_3, _7, _10} +//! ``` //! //! Ensuring soundness in all cases requires us to be more conservative (i.e. keeping more `Local`s alive) than necessary //! in most situations. To eliminate all the unnecessary `Local`s we use the fact that the analysis performed by //! `MaybeBorrowedLocals` functions as an upper bound for which `Local`s need to be kept alive. Hence we take the intersection //! of the two analyses at each statement. The results of `MaybeBorrowedLocals` for our example are: //! +//! ```text //! 1. {} //! 2. {} //! 3. {_3} @@ -227,9 +237,11 @@ //! 7. {_3, _4,} //! 8. {_3, _4} //! 9. {_3, _4} +//! ``` //! //! Taking the intersection hence yields: //! +//! ```text //! 1. {} //! 2. {} //! 3. {_3} @@ -239,6 +251,7 @@ //! 7. {_3, _4} //! 8. {_3} //! 9. {_3} +//! ``` use super::*;