From 483ee1f14770343122e9fadfffc054a119587dbf Mon Sep 17 00:00:00 2001 From: Joshua Nelson Date: Sun, 26 Jun 2022 06:19:32 -0500 Subject: [PATCH 1/2] Add a `-Zdump-drop-tracking-cfg` debugging flag This is useful for debugging drop-tracking; previously, you had to recompile rustc from source and manually add a call to `write_graph_to_file`. This makes the option more discoverable and configurable at runtime. I also took the liberty of making the labels for the CFG nodes much easier to read: previously, they looked like `id(2), local_id: 48`, now they look like ``` expr from_config (hir_id=HirId { owner: DefId(0:10 ~ default_struct_update[79f9]::foo), local_id: 2}) ``` --- compiler/rustc_interface/src/tests.rs | 1 + compiler/rustc_session/src/options.rs | 2 + .../drop_ranges/cfg_build.rs | 3 ++ .../drop_ranges/cfg_visualize.rs | 46 +++++++++++-------- 4 files changed, 33 insertions(+), 19 deletions(-) diff --git a/compiler/rustc_interface/src/tests.rs b/compiler/rustc_interface/src/tests.rs index 30a29ed6ed38f..dd7ef360cc18b 100644 --- a/compiler/rustc_interface/src/tests.rs +++ b/compiler/rustc_interface/src/tests.rs @@ -649,6 +649,7 @@ fn test_debugging_options_tracking_hash() { untracked!(dlltool, Some(PathBuf::from("custom_dlltool.exe"))); untracked!(dont_buffer_diagnostics, true); untracked!(dump_dep_graph, true); + untracked!(dump_drop_tracking_cfg, Some("cfg.dot".to_string())); untracked!(dump_mir, Some(String::from("abc"))); untracked!(dump_mir_dataflow, true); untracked!(dump_mir_dir, String::from("abc")); diff --git a/compiler/rustc_session/src/options.rs b/compiler/rustc_session/src/options.rs index 441e1f9f6a2b8..2638faa9d6a47 100644 --- a/compiler/rustc_session/src/options.rs +++ b/compiler/rustc_session/src/options.rs @@ -1245,6 +1245,8 @@ options! { dump_dep_graph: bool = (false, parse_bool, [UNTRACKED], "dump the dependency graph to $RUST_DEP_GRAPH (default: /tmp/dep_graph.gv) \ (default: no)"), + dump_drop_tracking_cfg: Option = (None, parse_opt_string, [UNTRACKED], + "dump drop-tracking control-flow graph as a `.dot` file (default: no)"), dump_mir: Option = (None, parse_opt_string, [UNTRACKED], "dump MIR state to file. `val` is used to select which passes and functions to dump. For example: diff --git a/compiler/rustc_typeck/src/check/generator_interior/drop_ranges/cfg_build.rs b/compiler/rustc_typeck/src/check/generator_interior/drop_ranges/cfg_build.rs index dbc309b29ff11..0af866359e847 100644 --- a/compiler/rustc_typeck/src/check/generator_interior/drop_ranges/cfg_build.rs +++ b/compiler/rustc_typeck/src/check/generator_interior/drop_ranges/cfg_build.rs @@ -33,6 +33,9 @@ pub(super) fn build_control_flow_graph<'tcx>( intravisit::walk_body(&mut drop_range_visitor, body); drop_range_visitor.drop_ranges.process_deferred_edges(); + if let Some(filename) = &tcx.sess.opts.debugging_opts.dump_drop_tracking_cfg { + super::cfg_visualize::write_graph_to_file(&drop_range_visitor.drop_ranges, filename, tcx); + } (drop_range_visitor.drop_ranges, drop_range_visitor.places.borrowed_temporaries) } diff --git a/compiler/rustc_typeck/src/check/generator_interior/drop_ranges/cfg_visualize.rs b/compiler/rustc_typeck/src/check/generator_interior/drop_ranges/cfg_visualize.rs index 20aad7aedf775..afca7f7b531f4 100644 --- a/compiler/rustc_typeck/src/check/generator_interior/drop_ranges/cfg_visualize.rs +++ b/compiler/rustc_typeck/src/check/generator_interior/drop_ranges/cfg_visualize.rs @@ -2,6 +2,7 @@ //! flow graph when needed for debugging. use rustc_graphviz as dot; +use rustc_middle::ty::TyCtxt; use super::{DropRangesBuilder, PostOrderId}; @@ -9,22 +10,35 @@ use super::{DropRangesBuilder, PostOrderId}; /// /// It is not normally called, but is kept around to easily add debugging /// code when needed. -#[allow(dead_code)] -pub(super) fn write_graph_to_file(drop_ranges: &DropRangesBuilder, filename: &str) { - dot::render(drop_ranges, &mut std::fs::File::create(filename).unwrap()).unwrap(); +pub(super) fn write_graph_to_file( + drop_ranges: &DropRangesBuilder, + filename: &str, + tcx: TyCtxt<'_>, +) { + dot::render( + &DropRangesGraph { drop_ranges, tcx }, + &mut std::fs::File::create(filename).unwrap(), + ) + .unwrap(); } -impl<'a> dot::GraphWalk<'a> for DropRangesBuilder { +struct DropRangesGraph<'a, 'tcx> { + drop_ranges: &'a DropRangesBuilder, + tcx: TyCtxt<'tcx>, +} + +impl<'a> dot::GraphWalk<'a> for DropRangesGraph<'_, '_> { type Node = PostOrderId; type Edge = (PostOrderId, PostOrderId); fn nodes(&'a self) -> dot::Nodes<'a, Self::Node> { - self.nodes.iter_enumerated().map(|(i, _)| i).collect() + self.drop_ranges.nodes.iter_enumerated().map(|(i, _)| i).collect() } fn edges(&'a self) -> dot::Edges<'a, Self::Edge> { - self.nodes + self.drop_ranges + .nodes .iter_enumerated() .flat_map(|(i, node)| { if node.successors.len() == 0 { @@ -45,7 +59,7 @@ impl<'a> dot::GraphWalk<'a> for DropRangesBuilder { } } -impl<'a> dot::Labeller<'a> for DropRangesBuilder { +impl<'a> dot::Labeller<'a> for DropRangesGraph<'_, '_> { type Node = PostOrderId; type Edge = (PostOrderId, PostOrderId); @@ -60,18 +74,12 @@ impl<'a> dot::Labeller<'a> for DropRangesBuilder { fn node_label(&'a self, n: &Self::Node) -> dot::LabelText<'a> { dot::LabelText::LabelStr( - format!( - "{:?}, local_id: {}", - n, - self.post_order_map - .iter() - .find(|(_hir_id, &post_order_id)| post_order_id == *n) - .map_or("".into(), |(hir_id, _)| format!( - "{}", - hir_id.local_id.index() - )) - ) - .into(), + self.drop_ranges + .post_order_map + .iter() + .find(|(_hir_id, &post_order_id)| post_order_id == *n) + .map_or("".into(), |(hir_id, _)| self.tcx.hir().node_to_string(*hir_id)) + .into(), ) } } From 3164c2aa15a17ddbc4378b375ac822b6fef154b5 Mon Sep 17 00:00:00 2001 From: Joshua Nelson Date: Fri, 1 Jul 2022 00:11:39 -0500 Subject: [PATCH 2/2] Make logging for drop-tracking easier to read. Some of these are a little questionable because the output is so much longer, but I would really love to keep the bit that adds the pretty-printed expression to the generated CFG .dot file. Before: ``` DEBUG rustc_typeck::check::generator_interior::drop_ranges::record_consumed_borrow consume PlaceWithHirId { hir_id: HirId { owner: DefId(0:7 ~ default_struct_update[79f9]::foo), local_id: 15 }, place: Place { base_ty: impl std::future::Future, base: Rvalue, projections: [] } }; diag_expr_id=HirId { owner: DefId(0:7 ~ default_struct_update[79f9]::foo), local_id: 15 }, using parent expr HirId { owner: DefId(0:7 ~ default_struct_update[79f9]::foo), local_id: 49 } ``` After: ``` DEBUG rustc_typeck::check::generator_interior::drop_ranges::record_consumed_borrow consume PlaceWithHirId { hir_id: HirId { owner: DefId(0:7 ~ default_struct_update[79f9]::foo), local_id: 15 }, place: Place { base_ty: impl std::future::Future, base: Rvalue, projections: [] } }; diag_expr_id=expr from_config(Config { nickname: None, ..Default::default() }) (hir_id=HirId { owner: DefId(0:7 ~ default_struct_update[79f9]::foo), local_id: 15 }), using parent expr .await (hir_id=HirId { owner: DefId(0:7 ~ default_struct_update[79f9]::foo), local_id: 49 }) ``` --- .../check/generator_interior/drop_ranges.rs | 19 +++++++++++++++++-- .../drop_ranges/cfg_build.rs | 3 ++- .../drop_ranges/cfg_visualize.rs | 18 ++++++++++++------ .../drop_ranges/record_consumed_borrow.rs | 10 +++++++--- 4 files changed, 38 insertions(+), 12 deletions(-) diff --git a/compiler/rustc_typeck/src/check/generator_interior/drop_ranges.rs b/compiler/rustc_typeck/src/check/generator_interior/drop_ranges.rs index c2b4c478ba387..887c791af76c2 100644 --- a/compiler/rustc_typeck/src/check/generator_interior/drop_ranges.rs +++ b/compiler/rustc_typeck/src/check/generator_interior/drop_ranges.rs @@ -109,7 +109,7 @@ rustc_index::newtype_index! { } /// Identifies a value whose drop state we need to track. -#[derive(PartialEq, Eq, Hash, Debug, Clone, Copy)] +#[derive(PartialEq, Eq, Hash, Clone, Copy)] enum TrackedValue { /// Represents a named variable, such as a let binding, parameter, or upvar. /// @@ -121,6 +121,21 @@ enum TrackedValue { Temporary(HirId), } +impl Debug for TrackedValue { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + ty::tls::with_opt(|opt_tcx| { + if let Some(tcx) = opt_tcx { + write!(f, "{}", tcx.hir().node_to_string(self.hir_id())) + } else { + match self { + Self::Variable(hir_id) => write!(f, "Variable({:?})", hir_id), + Self::Temporary(hir_id) => write!(f, "Temporary({:?})", hir_id), + } + } + }) + } +} + impl TrackedValue { fn hir_id(&self) -> HirId { match self { @@ -148,7 +163,7 @@ enum TrackedValueConversionError { /// Place projects are not currently supported. /// /// The reasoning around these is kind of subtle, so we choose to be more - /// conservative around these for now. There is not reason in theory we + /// conservative around these for now. There is no reason in theory we /// cannot support these, we just have not implemented it yet. PlaceProjectionsNotSupported, } diff --git a/compiler/rustc_typeck/src/check/generator_interior/drop_ranges/cfg_build.rs b/compiler/rustc_typeck/src/check/generator_interior/drop_ranges/cfg_build.rs index 0af866359e847..5e363ef6f8444 100644 --- a/compiler/rustc_typeck/src/check/generator_interior/drop_ranges/cfg_build.rs +++ b/compiler/rustc_typeck/src/check/generator_interior/drop_ranges/cfg_build.rs @@ -129,13 +129,14 @@ impl<'a, 'tcx> DropRangeVisitor<'a, 'tcx> { /// ExprUseVisitor's consume callback doesn't go deep enough for our purposes in all /// expressions. This method consumes a little deeper into the expression when needed. fn consume_expr(&mut self, expr: &hir::Expr<'_>) { - debug!("consuming expr {:?}, count={:?}", expr.hir_id, self.expr_index); + debug!("consuming expr {:?}, count={:?}", expr.kind, self.expr_index); let places = self .places .consumed .get(&expr.hir_id) .map_or(vec![], |places| places.iter().cloned().collect()); for place in places { + trace!(?place, "consuming place"); for_each_consumable(self.hir, place, |value| self.record_drop(value)); } } diff --git a/compiler/rustc_typeck/src/check/generator_interior/drop_ranges/cfg_visualize.rs b/compiler/rustc_typeck/src/check/generator_interior/drop_ranges/cfg_visualize.rs index afca7f7b531f4..c0a0bfe8e1c00 100644 --- a/compiler/rustc_typeck/src/check/generator_interior/drop_ranges/cfg_visualize.rs +++ b/compiler/rustc_typeck/src/check/generator_interior/drop_ranges/cfg_visualize.rs @@ -74,12 +74,18 @@ impl<'a> dot::Labeller<'a> for DropRangesGraph<'_, '_> { fn node_label(&'a self, n: &Self::Node) -> dot::LabelText<'a> { dot::LabelText::LabelStr( - self.drop_ranges - .post_order_map - .iter() - .find(|(_hir_id, &post_order_id)| post_order_id == *n) - .map_or("".into(), |(hir_id, _)| self.tcx.hir().node_to_string(*hir_id)) - .into(), + format!( + "{n:?}: {}", + self.drop_ranges + .post_order_map + .iter() + .find(|(_hir_id, &post_order_id)| post_order_id == *n) + .map_or("".into(), |(hir_id, _)| self + .tcx + .hir() + .node_to_string(*hir_id)) + ) + .into(), ) } } diff --git a/compiler/rustc_typeck/src/check/generator_interior/drop_ranges/record_consumed_borrow.rs b/compiler/rustc_typeck/src/check/generator_interior/drop_ranges/record_consumed_borrow.rs index b22b791f629d7..67cc46f21f00b 100644 --- a/compiler/rustc_typeck/src/check/generator_interior/drop_ranges/record_consumed_borrow.rs +++ b/compiler/rustc_typeck/src/check/generator_interior/drop_ranges/record_consumed_borrow.rs @@ -75,6 +75,7 @@ impl<'tcx> ExprUseDelegate<'tcx> { if !self.places.consumed.contains_key(&consumer) { self.places.consumed.insert(consumer, <_>::default()); } + debug!(?consumer, ?target, "mark_consumed"); self.places.consumed.get_mut(&consumer).map(|places| places.insert(target)); } @@ -136,13 +137,16 @@ impl<'tcx> expr_use_visitor::Delegate<'tcx> for ExprUseDelegate<'tcx> { place_with_id: &expr_use_visitor::PlaceWithHirId<'tcx>, diag_expr_id: HirId, ) { - let parent = match self.tcx.hir().find_parent_node(place_with_id.hir_id) { + let hir = self.tcx.hir(); + let parent = match hir.find_parent_node(place_with_id.hir_id) { Some(parent) => parent, None => place_with_id.hir_id, }; debug!( - "consume {:?}; diag_expr_id={:?}, using parent {:?}", - place_with_id, diag_expr_id, parent + "consume {:?}; diag_expr_id={}, using parent {}", + place_with_id, + hir.node_to_string(diag_expr_id), + hir.node_to_string(parent) ); place_with_id .try_into()