From c26afb765cb3047f2fa83b21739d9211af39f7ce Mon Sep 17 00:00:00 2001 From: Rich Kadel Date: Mon, 26 Apr 2021 02:45:46 -0700 Subject: [PATCH] Drop branching blocks with same span as expanded macro Fixes: #84561 --- .../rustc_mir/src/transform/coverage/spans.rs | 84 ++++++++++++--- .../rustc_mir/src/transform/coverage/tests.rs | 13 ++- .../expected_show_coverage.issue-84561.txt | 102 ++++++++++++++++++ .../run-make-fulldeps/coverage/issue-84561.rs | 94 ++++++++++++++++ 4 files changed, 276 insertions(+), 17 deletions(-) create mode 100644 src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.issue-84561.txt create mode 100644 src/test/run-make-fulldeps/coverage/issue-84561.rs diff --git a/compiler/rustc_mir/src/transform/coverage/spans.rs b/compiler/rustc_mir/src/transform/coverage/spans.rs index 2041109eb38..ddbd1680430 100644 --- a/compiler/rustc_mir/src/transform/coverage/spans.rs +++ b/compiler/rustc_mir/src/transform/coverage/spans.rs @@ -11,7 +11,7 @@ use rustc_middle::mir::{ use rustc_middle::ty::TyCtxt; use rustc_span::source_map::original_sp; -use rustc_span::{BytePos, Span}; +use rustc_span::{BytePos, ExpnKind, MacroKind, Span}; use std::cmp::Ordering; @@ -67,6 +67,7 @@ impl CoverageStatement { #[derive(Debug, Clone)] pub(super) struct CoverageSpan { pub span: Span, + pub is_macro_expansion: bool, pub bcb: BasicCoverageBlock, pub coverage_statements: Vec, pub is_closure: bool, @@ -74,12 +75,22 @@ pub(super) struct CoverageSpan { impl CoverageSpan { pub fn for_fn_sig(fn_sig_span: Span) -> Self { - Self { span: fn_sig_span, bcb: START_BCB, coverage_statements: vec![], is_closure: false } + // Whether the function signature is from a macro or not, it should not be treated like + // macro-expanded statements and terminators. + let is_macro_expansion = false; + Self { + span: fn_sig_span, + is_macro_expansion, + bcb: START_BCB, + coverage_statements: vec![], + is_closure: false, + } } pub fn for_statement( statement: &Statement<'tcx>, span: Span, + is_macro_expansion: bool, bcb: BasicCoverageBlock, bb: BasicBlock, stmt_index: usize, @@ -94,15 +105,22 @@ impl CoverageSpan { Self { span, + is_macro_expansion, bcb, coverage_statements: vec![CoverageStatement::Statement(bb, span, stmt_index)], is_closure, } } - pub fn for_terminator(span: Span, bcb: BasicCoverageBlock, bb: BasicBlock) -> Self { + pub fn for_terminator( + span: Span, + is_macro_expansion: bool, + bcb: BasicCoverageBlock, + bb: BasicBlock, + ) -> Self { Self { span, + is_macro_expansion, bcb, coverage_statements: vec![CoverageStatement::Terminator(bb, span)], is_closure: false, @@ -344,7 +362,27 @@ impl<'a, 'tcx> CoverageSpans<'a, 'tcx> { } else if self.prev_original_span == self.curr().span { // Note that this compares the new span to `prev_original_span`, which may not // be the full `prev.span` (if merged during the previous iteration). - self.hold_pending_dups_unless_dominated(); + if self.prev().is_macro_expansion && self.curr().is_macro_expansion { + // Macros that expand to include branching (such as + // `assert_eq!()`, `assert_ne!()`, `info!()`, `debug!()`, or + // `trace!()) typically generate callee spans with identical + // ranges (typically the full span of the macro) for all + // `BasicBlocks`. This makes it impossible to distinguish + // the condition (`if val1 != val2`) from the optional + // branched statements (such as the call to `panic!()` on + // assert failure). In this case it is better (or less + // worse) to drop the optional branch bcbs and keep the + // non-conditional statements, to count when reached. + debug!( + " curr and prev are part of a macro expansion, and curr has the same span \ + as prev, but is in a different bcb. Drop curr and keep prev for next iter. \ + prev={:?}", + self.prev() + ); + self.take_curr(); + } else { + self.hold_pending_dups_unless_dominated(); + } } else { self.cutoff_prev_at_overlapping_curr(); } @@ -401,14 +439,24 @@ impl<'a, 'tcx> CoverageSpans<'a, 'tcx> { .iter() .enumerate() .filter_map(move |(index, statement)| { - filtered_statement_span(statement, self.body_span).map(|span| { - CoverageSpan::for_statement(statement, span, bcb, bb, index) - }) + filtered_statement_span(statement, self.body_span).map( + |(span, is_macro_expansion)| { + CoverageSpan::for_statement( + statement, + span, + is_macro_expansion, + bcb, + bb, + index, + ) + }, + ) }) - .chain( - filtered_terminator_span(data.terminator(), self.body_span) - .map(|span| CoverageSpan::for_terminator(span, bcb, bb)), - ) + .chain(filtered_terminator_span(data.terminator(), self.body_span).map( + |(span, is_macro_expansion)| { + CoverageSpan::for_terminator(span, is_macro_expansion, bcb, bb) + }, + )) }) .collect() } @@ -656,7 +704,7 @@ impl<'a, 'tcx> CoverageSpans<'a, 'tcx> { pub(super) fn filtered_statement_span( statement: &'a Statement<'tcx>, body_span: Span, -) -> Option { +) -> Option<(Span, bool)> { match statement.kind { // These statements have spans that are often outside the scope of the executed source code // for their parent `BasicBlock`. @@ -701,7 +749,7 @@ pub(super) fn filtered_statement_span( pub(super) fn filtered_terminator_span( terminator: &'a Terminator<'tcx>, body_span: Span, -) -> Option { +) -> Option<(Span, bool)> { match terminator.kind { // These terminators have spans that don't positively contribute to computing a reasonable // span of actually executed source code. (For example, SwitchInt terminators extracted from @@ -742,7 +790,13 @@ pub(super) fn filtered_terminator_span( } #[inline] -fn function_source_span(span: Span, body_span: Span) -> Span { +fn function_source_span(span: Span, body_span: Span) -> (Span, bool) { + let is_macro_expansion = span.ctxt() != body_span.ctxt() + && if let ExpnKind::Macro(MacroKind::Bang, _) = span.ctxt().outer_expn_data().kind { + true + } else { + false + }; let span = original_sp(span, body_span).with_ctxt(body_span.ctxt()); - if body_span.contains(span) { span } else { body_span } + (if body_span.contains(span) { span } else { body_span }, is_macro_expansion) } diff --git a/compiler/rustc_mir/src/transform/coverage/tests.rs b/compiler/rustc_mir/src/transform/coverage/tests.rs index dee112443d3..dbbd677fd63 100644 --- a/compiler/rustc_mir/src/transform/coverage/tests.rs +++ b/compiler/rustc_mir/src/transform/coverage/tests.rs @@ -1,6 +1,10 @@ //! This crate hosts a selection of "unit tests" for components of the `InstrumentCoverage` MIR //! pass. //! +//! ```shell +//! ./x.py test --keep-stage 1 compiler/rustc_mir --test-args '--show-output coverage' +//! ``` +//! //! The tests construct a few "mock" objects, as needed, to support the `InstrumentCoverage` //! functions and algorithms. Mocked objects include instances of `mir::Body`; including //! `Terminator`s of various `kind`s, and `Span` objects. Some functions used by or used on @@ -679,10 +683,15 @@ fn test_make_bcb_counters() { let mut basic_coverage_blocks = graph::CoverageGraph::from_mir(&mir_body); let mut coverage_spans = Vec::new(); for (bcb, data) in basic_coverage_blocks.iter_enumerated() { - if let Some(span) = + if let Some((span, is_macro_expansion)) = spans::filtered_terminator_span(data.terminator(&mir_body), body_span) { - coverage_spans.push(spans::CoverageSpan::for_terminator(span, bcb, data.last_bb())); + coverage_spans.push(spans::CoverageSpan::for_terminator( + span, + is_macro_expansion, + bcb, + data.last_bb(), + )); } } let mut coverage_counters = counters::CoverageCounters::new(0); diff --git a/src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.issue-84561.txt b/src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.issue-84561.txt new file mode 100644 index 00000000000..34d584f9eae --- /dev/null +++ b/src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.issue-84561.txt @@ -0,0 +1,102 @@ + 1| |// FIXME(#84561): function-like macros produce unintuitive coverage results. + 2| |// This test demonstrates some of the problems. + 3| | + 4| 18|#[derive(Debug, PartialEq, Eq)] + ^5 ^0 + ------------------ + | ::eq: + | 4| 18|#[derive(Debug, PartialEq, Eq)] + ------------------ + | Unexecuted instantiation: ::ne + ------------------ + 5| |struct Foo(u32); + 6| | + 7| 1|fn main() { + 8| 1| let bar = Foo(1); + 9| 1| assert_eq!(bar, Foo(1)); + 10| 1| let baz = Foo(0); + 11| 1| assert_ne!(baz, Foo(1)); + 12| 1| println!("{:?}", Foo(1)); + 13| 1| println!("{:?}", bar); + 14| 1| println!("{:?}", baz); + 15| 1| + 16| 1| assert_eq!(Foo(1), Foo(1)); + 17| 1| assert_ne!(Foo(0), Foo(1)); + 18| 1| assert_eq!(Foo(2), Foo(2)); + 19| 1| let bar = Foo(1); + 20| 1| assert_ne!(Foo(0), Foo(3)); + 21| 1| assert_ne!(Foo(0), Foo(4)); + 22| 1| assert_eq!(Foo(3), Foo(3)); + 23| 1| assert_ne!(Foo(0), Foo(5)); + 24| 1| println!("{:?}", bar); + 25| 1| println!("{:?}", Foo(1)); + 26| 1| + 27| 1| let is_true = std::env::args().len() == 1; + 28| 1| + 29| 1| assert_eq!( + 30| 1| Foo(1), + 31| 1| Foo(1) + 32| 1| ); + 33| 1| assert_ne!( + 34| 1| Foo(0), + 35| 1| Foo(1) + 36| 1| ); + 37| 1| assert_eq!( + 38| 1| Foo(2), + 39| 1| Foo(2) + 40| 1| ); + 41| 1| let bar = Foo(1 + 42| 1| ); + 43| 1| assert_ne!( + 44| 1| Foo(0), + 45| 1| Foo(3) + 46| 1| ); + 47| 1| if is_true { + 48| 1| assert_ne!( + 49| 1| Foo(0), + 50| 1| Foo(4) + 51| 1| ); + 52| | } else { + 53| 0| assert_eq!( + 54| 0| Foo(3), + 55| 0| Foo(3) + 56| 0| ); + 57| | } + 58| | assert_ne!( + 59| 1| if is_true { + 60| 1| Foo(0) + 61| | } else { + 62| 0| Foo(1) + 63| | }, + 64| | Foo(5) + 65| | ); + 66| 1| assert_ne!( + 67| 1| Foo(5), + 68| 1| if is_true { + 69| 1| Foo(0) + 70| | } else { + 71| 0| Foo(1) + 72| | } + 73| | ); + 74| | assert_ne!( + 75| 1| if is_true { + 76| 1| assert_eq!( + 77| 1| Foo(3), + 78| 1| Foo(3) + 79| 1| ); + 80| 1| Foo(0) + 81| | } else { + 82| | assert_ne!( + 83| 0| if is_true { + 84| 0| Foo(0) + 85| | } else { + 86| 0| Foo(1) + 87| | }, + 88| | Foo(5) + 89| | ); + 90| 0| Foo(1) + 91| | }, + 92| | Foo(5) + 93| | ); + 94| 1|} + diff --git a/src/test/run-make-fulldeps/coverage/issue-84561.rs b/src/test/run-make-fulldeps/coverage/issue-84561.rs new file mode 100644 index 00000000000..a5a0e1dc758 --- /dev/null +++ b/src/test/run-make-fulldeps/coverage/issue-84561.rs @@ -0,0 +1,94 @@ +// FIXME(#84561): function-like macros produce unintuitive coverage results. +// This test demonstrates some of the problems. + +#[derive(Debug, PartialEq, Eq)] +struct Foo(u32); + +fn main() { + let bar = Foo(1); + assert_eq!(bar, Foo(1)); + let baz = Foo(0); + assert_ne!(baz, Foo(1)); + println!("{:?}", Foo(1)); + println!("{:?}", bar); + println!("{:?}", baz); + + assert_eq!(Foo(1), Foo(1)); + assert_ne!(Foo(0), Foo(1)); + assert_eq!(Foo(2), Foo(2)); + let bar = Foo(1); + assert_ne!(Foo(0), Foo(3)); + assert_ne!(Foo(0), Foo(4)); + assert_eq!(Foo(3), Foo(3)); + assert_ne!(Foo(0), Foo(5)); + println!("{:?}", bar); + println!("{:?}", Foo(1)); + + let is_true = std::env::args().len() == 1; + + assert_eq!( + Foo(1), + Foo(1) + ); + assert_ne!( + Foo(0), + Foo(1) + ); + assert_eq!( + Foo(2), + Foo(2) + ); + let bar = Foo(1 + ); + assert_ne!( + Foo(0), + Foo(3) + ); + if is_true { + assert_ne!( + Foo(0), + Foo(4) + ); + } else { + assert_eq!( + Foo(3), + Foo(3) + ); + } + assert_ne!( + if is_true { + Foo(0) + } else { + Foo(1) + }, + Foo(5) + ); + assert_ne!( + Foo(5), + if is_true { + Foo(0) + } else { + Foo(1) + } + ); + assert_ne!( + if is_true { + assert_eq!( + Foo(3), + Foo(3) + ); + Foo(0) + } else { + assert_ne!( + if is_true { + Foo(0) + } else { + Foo(1) + }, + Foo(5) + ); + Foo(1) + }, + Foo(5) + ); +}