Responded to all feedback as of 2020-10-30

Author: richkadel

compiler/rustc_mir/src/transform/coverage/counters.rs

@@ -1,3 +1,113 @@
+//! The `InstrumentCoverage` MIR pass implementation includes debugging tools and options
+//! to help developers understand and/or improve the analysis and instrumentation of a MIR.
+//!
+//! To enable coverage, include the rustc command line option:
+//!
+//!   * `-Z instrument-coverage`
+//!
+//! MIR Dump Files, with additional `CoverageGraph` graphviz and `CoverageSpan` spanview
+//! ------------------------------------------------------------------------------------
+//!
+//! Additional debugging options include:
+//!
+//!   * `-Z dump-mir=InstrumentCoverage` - Generate `.mir` files showing the state of the MIR,
+//!     before and after the `InstrumentCoverage` pass, for each compiled function.
+//!
+//!   * `-Z dump-mir-graphviz` - If `-Z dump-mir` is also enabled for the current MIR node path,
+//!     each MIR dump is accompanied by a before-and-after graphical view of the MIR, in Graphviz
+//!     `.dot` file format (which can be visually rendered as a graph using any of a number of free
+//!     Graphviz viewers and IDE extensions).
+//!
+//!     For the `InstrumentCoverage` pass, this option also enables generation of an additional
+//!     Graphviz `.dot` file for each function, rendering the `CoverageGraph`: the control flow
+//!     graph (CFG) of `BasicCoverageBlocks` (BCBs), as nodes, internally labeled to show the
+//!     `CoverageSpan`-based MIR elements each BCB represents (`BasicBlock`s, `Statement`s and
+//!     `Terminator`s), assigned coverage counters and/or expressions, and edge counters, as needed.
+//!
+//!     (Note the additional option, `-Z graphviz-dark-mode`, can be added, to change the rendered
+//!     output from its default black-on-white background to a dark color theme, if desired.)
+//!
+//!   * `-Z dump-mir-spanview` - If `-Z dump-mir` is also enabled for the current MIR node path,
+//!     each MIR dump is accompanied by a before-and-after `.html` document showing the function's
+//!     original source code, highlighted by it's MIR spans, at the `statement`-level (by default),
+//!     `terminator` only, or encompassing span for the `Terminator` plus all `Statement`s, in each
+//!     `block` (`BasicBlock`).
+//!
+//!     For the `InstrumentCoverage` pass, this option also enables generation of an additional
+//!     spanview `.html` file for each function, showing the aggregated `CoverageSpan`s that will
+//!     require counters (or counter expressions) for accurate coverage analysis.
+//!
+//! Debug Logging
+//! -------------
+//!
+//! The `InstrumentCoverage` pass includes debug logging messages at various phases and decision
+//! points, which can be enabled via environment variable:
+//!
+//! ```shell
+//! RUSTC_LOG=rustc_mir::transform::coverage=debug
+//! ```
+//!
+//! Other module paths with coverage-related debug logs may also be of interest, particularly for
+//! debugging the coverage map data, injected as global variables in the LLVM IR (during rustc's
+//! code generation pass). For example:
+//!
+//! ```shell
+//! RUSTC_LOG=rustc_mir::transform::coverage,rustc_codegen_ssa::coverageinfo,rustc_codegen_llvm::coverageinfo=debug
+//! ```
+//!
+//! Coverage Debug Options
+//! ---------------------------------
+//!
+//! Additional debugging options can be enabled using the environment variable:
+//!
+//! ```shell
+//! RUSTC_COVERAGE_DEBUG_OPTIONS=<options>
+//! ```
+//!
+//! These options are comma-separated, and specified in the format `option-name=value`. For example:
+//!
+//! ```shell
+//! $ RUSTC_COVERAGE_DEBUG_OPTIONS=counter-format=id+operation,allow-unused-expressions=yes cargo build
+//! ```
+//!
+//! Coverage debug options include:
+//!
+//!   * `allow-unused-expressions=yes` or `no` (default: `no`)
+//!
+//!     The `InstrumentCoverage` algorithms _should_ only create and assign expressions to a
+//!     `BasicCoverageBlock`, or an incoming edge, if that expression is either (a) required to
+//!     count a `CoverageSpan`, or (b) a dependency of some other required counter expression.
+//!
+//!     If an expression is generated that does not map to a `CoverageSpan` or dependency, this
+//!     probably indicates there was a bug in the algorithm that creates and assigns counters
+//!     and expressions.
+//!
+//!     When this kind of bug is encountered, the rustc compiler will panic by default. Setting:
+//!     `allow-unused-expressions=yes` will log a warning message instead of panicking (effectively
+//!     ignoring the unused expressions), which may be helpful when debugging the root cause of
+//!     the problem.
+//!
+//!   * `counter-format=<choices>`, where `<choices>` can be any plus-separated combination of `id`,
+//!     `block`, and/or `operation` (default: `block+operation`)
+//!
+//!     This option effects both the `CoverageGraph` (graphviz `.dot` files) and debug logging, when
+//!     generating labels for counters and expressions.
+//!
+//!     Depending on the values and combinations, counters can be labeled by:
+//!
+//!         * `id` - counter or expression ID (ascending counter IDs, starting at 1, or descending
+//!           expression IDs, starting at `u32:MAX`)
+//!         * `block` - the `BasicCoverageBlock` label (for example, `bcb0`) or edge label (for
+//!           example `bcb0->bcb1`), for counters or expressions assigned to count a
+//!           `BasicCoverageBlock` or edge. Intermediate expressions (not directly associated with
+//!           a BCB or edge) will be labeled by their expression ID, unless `operation` is also
+//!           specified.
+//!         * `operation` - applied to expressions only, labels include the left-hand-side counter
+//!           or expression label (lhs operand), the operator (`+` or `-`), and the right-hand-side
+//!           counter or expression (rhs operand). Expression operand labels are generated
+//!           recursively, generating labels with nested operations, enclosed in parentheses
+//!           (for example: `bcb2 + (bcb0 - bcb1)`).
+
 use super::graph::{BasicCoverageBlock, BasicCoverageBlockData, CoverageGraph};
 use super::spans::CoverageSpan;
 
@@ -20,21 +130,19 @@ const RUSTC_COVERAGE_DEBUG_OPTIONS: &str = "RUSTC_COVERAGE_DEBUG_OPTIONS";
 pub(crate) fn debug_options<'a>() -> &'a DebugOptions {
     static DEBUG_OPTIONS: SyncOnceCell<DebugOptions> = SyncOnceCell::new();
 
-    &DEBUG_OPTIONS.get_or_init(|| DebugOptions::new())
+    &DEBUG_OPTIONS.get_or_init(|| DebugOptions::from_env())
 }
 
 /// Parses and maintains coverage-specific debug options captured from the environment variable
-/// "RUSTC_COVERAGE_DEBUG_OPTIONS", if set. Options can be set on the command line by, for example:
-///
-///     $ RUSTC_COVERAGE_DEBUG_OPTIONS=counter-format=block,allow_unused_expressions=n cargo build
+/// "RUSTC_COVERAGE_DEBUG_OPTIONS", if set.
 #[derive(Debug, Clone)]
 pub(crate) struct DebugOptions {
     pub allow_unused_expressions: bool,
     counter_format: ExpressionFormat,
 }
 
 impl DebugOptions {
-    fn new() -> Self {
+    fn from_env() -> Self {
         let mut allow_unused_expressions = true;
         let mut counter_format = ExpressionFormat::default();
 
@@ -152,10 +260,11 @@ impl DebugCounters {
     }
 
     pub fn enable(&mut self) {
+        debug_assert!(!self.is_enabled());
         self.some_counters.replace(FxHashMap::default());
     }
 
-    pub fn is_enabled(&mut self) -> bool {
+    pub fn is_enabled(&self) -> bool {
         self.some_counters.is_some()
     }
 
@@ -294,12 +403,13 @@ impl GraphvizData {
     }
 
     pub fn enable(&mut self) {
+        debug_assert!(!self.is_enabled());
         self.some_bcb_to_coverage_spans_with_counters = Some(FxHashMap::default());
         self.some_bcb_to_dependency_counters = Some(FxHashMap::default());
         self.some_edge_to_counter = Some(FxHashMap::default());
     }
 
-    pub fn is_enabled(&mut self) -> bool {
+    pub fn is_enabled(&self) -> bool {
         self.some_bcb_to_coverage_spans_with_counters.is_some()
     }
 
@@ -399,11 +509,12 @@ impl UsedExpressions {
     }
 
     pub fn enable(&mut self) {
+        debug_assert!(!self.is_enabled());
         self.some_used_expression_operands = Some(FxHashMap::default());
         self.some_unused_expressions = Some(Vec::new());
     }
 
-    pub fn is_enabled(&mut self) -> bool {
+    pub fn is_enabled(&self) -> bool {
         self.some_used_expression_operands.is_some()
     }
 
@@ -416,7 +527,7 @@ impl UsedExpressions {
         }
     }
 
-    pub fn expression_is_used(&mut self, expression: &CoverageKind) -> bool {
+    pub fn expression_is_used(&self, expression: &CoverageKind) -> bool {
         if let Some(used_expression_operands) = self.some_used_expression_operands.as_ref() {
             used_expression_operands.contains_key(&expression.as_operand_id())
         } else {

compiler/rustc_mir/src/transform/coverage/debug.rs

@@ -82,6 +82,8 @@ impl CoverageGraph {
         // each block terminator's `successors()`. Coverage spans must map to actual source code,
         // so compiler generated blocks and paths can be ignored. To that end, the CFG traversal
         // intentionally omits unwind paths.
+        // FIXME(#78544): MIR InstrumentCoverage: Improve coverage of `#[should_panic]` tests and
+        // `catch_unwind()` handlers.
         let mir_cfg_without_unwind = ShortCircuitPreorder::new(&mir_body, bcb_filtered_successors);
 
         let mut basic_blocks = Vec::new();
@@ -288,7 +290,8 @@ rustc_index::newtype_index! {
 ///   * The BCB CFG ignores (trims) branches not relevant to coverage, such as unwind-related code,
 ///     that is injected by the Rust compiler but has no physical source code to count. This also
 ///     means a BasicBlock with a `Call` terminator can be merged into its primary successor target
-///     block, in the same BCB.
+///     block, in the same BCB. (But, note: Issue #78544: "MIR InstrumentCoverage: Improve coverage
+///     of `#[should_panic]` tests and `catch_unwind()` handlers")
 ///   * Some BasicBlock terminators support Rust-specific concerns--like borrow-checking--that are
 ///     not relevant to coverage analysis. `FalseUnwind`, for example, can be treated the same as
 ///     a `Goto`, and merged with its successor into the same BCB.
@@ -329,7 +332,6 @@ impl BasicCoverageBlockData {
         &mir_body[self.last_bb()].terminator()
     }
 
-    #[inline(always)]
     pub fn set_counter(
         &mut self,
         counter_kind: CoverageKind,
@@ -342,16 +344,15 @@ impl BasicCoverageBlockData {
             "attempt to add a `Counter` to a BCB target with existing incoming edge counters"
         );
         let operand = counter_kind.as_operand_id();
-        let expect_none = self.counter_kind.replace(counter_kind);
-        if expect_none.is_some() {
-            return Error::from_string(format!(
+        if let Some(replaced) = self.counter_kind.replace(counter_kind) {
+            Error::from_string(format!(
                 "attempt to set a BasicCoverageBlock coverage counter more than once; \
                 {:?} already had counter {:?}",
-                self,
-                expect_none.unwrap(),
-            ));
+                self, replaced,
+            ))
+        } else {
+            Ok(operand)
         }
-        Ok(operand)
     }
 
     #[inline(always)]
@@ -364,7 +365,6 @@ impl BasicCoverageBlockData {
         self.counter_kind.take()
     }
 
-    #[inline(always)]
     pub fn set_edge_counter_from(
         &mut self,
         from_bcb: BasicCoverageBlock,
@@ -383,22 +383,22 @@ impl BasicCoverageBlockData {
             }
         }
         let operand = counter_kind.as_operand_id();
-        let expect_none = self
+        if let Some(replaced) = self
             .edge_from_bcbs
             .get_or_insert_with(|| FxHashMap::default())
-            .insert(from_bcb, counter_kind);
-        if expect_none.is_some() {
-            return Error::from_string(format!(
+            .insert(from_bcb, counter_kind)
+        {
+            Error::from_string(format!(
                 "attempt to set an edge counter more than once; from_bcb: \
                 {:?} already had counter {:?}",
-                from_bcb,
-                expect_none.unwrap(),
-            ));
+                from_bcb, replaced,
+            ))
+        } else {
+            Ok(operand)
         }
-        Ok(operand)
     }
 
-    #[inline(always)]
+    #[inline]
     pub fn edge_counter_from(&self, from_bcb: BasicCoverageBlock) -> Option<&CoverageKind> {
         if let Some(edge_from_bcbs) = &self.edge_from_bcbs {
             edge_from_bcbs.get(&from_bcb)
@@ -407,7 +407,7 @@ impl BasicCoverageBlockData {
         }
     }
 
-    #[inline(always)]
+    #[inline]
     pub fn take_edge_counters(
         &mut self,
     ) -> Option<impl Iterator<Item = (BasicCoverageBlock, CoverageKind)>> {
@@ -476,6 +476,9 @@ impl std::fmt::Debug for BcbBranch {
     }
 }
 
+// Returns the `Terminator`s non-unwind successors.
+// FIXME(#78544): MIR InstrumentCoverage: Improve coverage of `#[should_panic]` tests and
+// `catch_unwind()` handlers.
 fn bcb_filtered_successors<'a, 'tcx>(
     body: &'tcx &'a mir::Body<'tcx>,
     term_kind: &'tcx TerminatorKind<'tcx>,
@@ -495,6 +498,7 @@ fn bcb_filtered_successors<'a, 'tcx>(
 /// Maintains separate worklists for each loop in the BasicCoverageBlock CFG, plus one for the
 /// CoverageGraph outside all loops. This supports traversing the BCB CFG in a way that
 /// ensures a loop is completely traversed before processing Blocks after the end of the loop.
+// FIXME(richkadel): Add unit tests for TraversalContext.
 #[derive(Debug)]
 pub(crate) struct TraversalContext {
     /// From one or more backedges returning to a loop header.
@@ -644,7 +648,27 @@ fn find_loop_backedges(
     let num_bcbs = basic_coverage_blocks.num_nodes();
     let mut backedges = IndexVec::from_elem_n(Vec::<BasicCoverageBlock>::new(), num_bcbs);
 
-    // Identify loops by their backedges
+    // Identify loops by their backedges.
+    //
+    // The computational complexity is bounded by: n(s) x d where `n` is the number of
+    // `BasicCoverageBlock` nodes (the simplified/reduced representation of the CFG derived from the
+    // MIR); `s` is the average number of successors per node (which is most likely less than 2, and
+    // independent of the size of the function, so it can be treated as a constant);
+    // and `d` is the average number of dominators per node.
+    //
+    // The average number of dominators depends on the size and complexity of the function, and
+    // nodes near the start of the function's control flow graph typically have less dominators
+    // than nodes near the end of the CFG. Without doing a detailed mathematical analysis, I
+    // think the resulting complexity has the characteristics of O(n log n).
+    //
+    // The overall complexity appears to be comparable to many other MIR transform algorithms, and I
+    // don't expect that this function is creating a performance hot spot, but if this becomes an
+    // issue, there may be ways to optimize the `is_dominated_by` algorithm (as indicated by an
+    // existing `FIXME` comment in that code), or possibly ways to optimize it's usage here, perhaps
+    // by keeping track of results for visited `BasicCoverageBlock`s if they can be used to short
+    // circuit downstream `is_dominated_by` checks.
+    //
+    // For now, that kind of optimization seems unnecessarily complicated.
     for (bcb, _) in basic_coverage_blocks.iter_enumerated() {
         for &successor in &basic_coverage_blocks.successors[bcb] {
             if basic_coverage_blocks.is_dominated_by(bcb, successor) {

compiler/rustc_mir/src/transform/coverage/graph.rs

@@ -74,9 +74,6 @@ impl<'tcx> MirPass<'tcx> for InstrumentCoverage {
             trace!("InstrumentCoverage skipped for {:?} (not an FnLikeNode)", mir_source.def_id());
             return;
         }
-        // FIXME(richkadel): By comparison, the MIR pass `ConstProp` includes associated constants,
-        // with functions, methods, and closures. I assume Miri is used for associated constants as
-        // well. If not, we may need to include them here too.
 
         trace!("InstrumentCoverage starting for {:?}", mir_source.def_id());
         Instrumentor::new(&self.name(), tcx, mir_body).inject_counters();
@@ -121,7 +118,10 @@ impl<'a, 'tcx> Instrumentor<'a, 'tcx> {
         let mut graphviz_data = debug::GraphvizData::new();
         let mut debug_used_expressions = debug::UsedExpressions::new();
 
-        let dump_graphviz = tcx.sess.opts.debugging_opts.dump_mir_graphviz;
+        let dump_mir = pretty::dump_enabled(tcx, self.pass_name, def_id);
+        let dump_graphviz = dump_mir && tcx.sess.opts.debugging_opts.dump_mir_graphviz;
+        let dump_spanview = dump_mir && tcx.sess.opts.debugging_opts.dump_mir_spanview.is_some();
+
         if dump_graphviz {
             graphviz_data.enable();
             self.coverage_counters.enable_debug();
@@ -139,7 +139,7 @@ impl<'a, 'tcx> Instrumentor<'a, 'tcx> {
             &self.basic_coverage_blocks,
         );
 
-        if pretty::dump_enabled(tcx, self.pass_name, def_id) {
+        if dump_spanview {
             debug::dump_coverage_spanview(
                 tcx,
                 self.mir_body,
@@ -174,6 +174,13 @@ impl<'a, 'tcx> Instrumentor<'a, 'tcx> {
                 ////////////////////////////////////////////////////
                 // Remove the counter or edge counter from of each `CoverageSpan`s associated
                 // `BasicCoverageBlock`, and inject a `Coverage` statement into the MIR.
+                //
+                // `Coverage` statements injected from `CoverageSpan`s will include the code regions
+                // (source code start and end positions) to be counted by the associated counter.
+                //
+                // These `CoverageSpan`-associated counters are removed from their associated
+                // `BasicCoverageBlock`s so that the only remaining counters in the `CoverageGraph`
+                // are indirect counters (to be injected next, without associated code regions).
                 self.inject_coverage_span_counters(
                     coverage_spans,
                     &mut graphviz_data,
@@ -262,6 +269,8 @@ impl<'a, 'tcx> Instrumentor<'a, 'tcx> {
                 bug!("Every BasicCoverageBlock should have a Counter or Expression");
             };
             graphviz_data.add_bcb_coverage_span_with_counter(bcb, &covspan, &counter_kind);
+            // FIXME(#78542): Can spans for `TerminatorKind::Goto` be improved to avoid special
+            // cases?
             let some_code_region = if self.is_code_region_redundant(bcb, span, body_span) {
                 None
             } else {
@@ -280,6 +289,7 @@ impl<'a, 'tcx> Instrumentor<'a, 'tcx> {
     ///
     /// If this method returns `true`, the counter (which other `Expressions` may depend on) is
     /// still injected, but without an associated code region.
+    // FIXME(#78542): Can spans for `TerminatorKind::Goto` be improved to avoid special cases?
     fn is_code_region_redundant(
         &self,
         bcb: BasicCoverageBlock,