Skip to content

Commit aeb67ad

Browse files
richkadelrichkadel
committed
Drop branching blocks with same span as expanded macro
Fixes: #84561
1 parent 1919b3f commit aeb67ad

File tree

4 files changed

+276
-17
lines changed

4 files changed

+276
-17
lines changed

‎compiler/rustc_mir/src/transform/coverage/spans.rs‎

Lines changed: 69 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@ use rustc_middle::mir::{
1111
use rustc_middle::ty::TyCtxt;
1212

1313
use rustc_span::source_map::original_sp;
14-
use rustc_span::{BytePos, Span};
14+
use rustc_span::{BytePos, ExpnKind, MacroKind, Span};
1515

1616
use std::cmp::Ordering;
1717

@@ -67,19 +67,30 @@ impl CoverageStatement {
6767
#[derive(Debug, Clone)]
6868
pub(super) struct CoverageSpan {
6969
pub span: Span,
70+
pub is_macro_expansion: bool,
7071
pub bcb: BasicCoverageBlock,
7172
pub coverage_statements: Vec<CoverageStatement>,
7273
pub is_closure: bool,
7374
}
7475

7576
impl CoverageSpan {
7677
pub fn for_fn_sig(fn_sig_span: Span) -> Self {
77-
Self { span: fn_sig_span, bcb: START_BCB, coverage_statements: vec![], is_closure: false }
78+
// Whether the function signature is from a macro or not, it should not be treated like
79+
// macro-expanded statements and terminators.
80+
let is_macro_expansion = false;
81+
Self {
82+
span: fn_sig_span,
83+
is_macro_expansion,
84+
bcb: START_BCB,
85+
coverage_statements: vec![],
86+
is_closure: false,
87+
}
7888
}
7989

8090
pub fn for_statement(
8191
statement: &Statement<'tcx>,
8292
span: Span,
93+
is_macro_expansion: bool,
8394
bcb: BasicCoverageBlock,
8495
bb: BasicBlock,
8596
stmt_index: usize,
@@ -94,15 +105,22 @@ impl CoverageSpan {
94105

95106
Self {
96107
span,
108+
is_macro_expansion,
97109
bcb,
98110
coverage_statements: vec![CoverageStatement::Statement(bb, span, stmt_index)],
99111
is_closure,
100112
}
101113
}
102114

103-
pub fn for_terminator(span: Span, bcb: BasicCoverageBlock, bb: BasicBlock) -> Self {
115+
pub fn for_terminator(
116+
span: Span,
117+
is_macro_expansion: bool,
118+
bcb: BasicCoverageBlock,
119+
bb: BasicBlock,
120+
) -> Self {
104121
Self {
105122
span,
123+
is_macro_expansion,
106124
bcb,
107125
coverage_statements: vec![CoverageStatement::Terminator(bb, span)],
108126
is_closure: false,
@@ -344,7 +362,27 @@ impl<'a, 'tcx> CoverageSpans<'a, 'tcx> {
344362
} else if self.prev_original_span == self.curr().span {
345363
// Note that this compares the new span to `prev_original_span`, which may not
346364
// be the full `prev.span` (if merged during the previous iteration).
347-
self.hold_pending_dups_unless_dominated();
365+
if self.prev().is_macro_expansion && self.curr().is_macro_expansion {
366+
// Macros that expand to include branching (such as
367+
// `assert_eq!()`, `assert_ne!()`, `info!()`, `debug!()`, or
368+
// `trace!()) typically generate callee spans with identical
369+
// ranges (typically the full span of the macro) for all
370+
// `BasicBlocks`. This makes it impossible to distinguish
371+
// the condition (`if val1 != val2`) from the optional
372+
// branched statements (such as the call to `panic!()` on
373+
// assert failure). In this case it is better (or less
374+
// worse) to drop the optional branch bcbs and keep the
375+
// non-conditional statements, to count when reached.
376+
debug!(
377+
" curr and prev are part of a macro expansion, and curr has the same span \
378+
as prev, but is in a different bcb. Drop curr and keep prev for next iter. \
379+
prev={:?}",
380+
self.prev()
381+
);
382+
self.take_curr();
383+
} else {
384+
self.hold_pending_dups_unless_dominated();
385+
}
348386
} else {
349387
self.cutoff_prev_at_overlapping_curr();
350388
}
@@ -401,14 +439,24 @@ impl<'a, 'tcx> CoverageSpans<'a, 'tcx> {
401439
.iter()
402440
.enumerate()
403441
.filter_map(move |(index, statement)| {
404-
filtered_statement_span(statement, self.body_span).map(|span| {
405-
CoverageSpan::for_statement(statement, span, bcb, bb, index)
406-
})
442+
filtered_statement_span(statement, self.body_span).map(
443+
|(span, is_macro_expansion)| {
444+
CoverageSpan::for_statement(
445+
statement,
446+
span,
447+
is_macro_expansion,
448+
bcb,
449+
bb,
450+
index,
451+
)
452+
},
453+
)
407454
})
408-
.chain(
409-
filtered_terminator_span(data.terminator(), self.body_span)
410-
.map(|span| CoverageSpan::for_terminator(span, bcb, bb)),
411-
)
455+
.chain(filtered_terminator_span(data.terminator(), self.body_span).map(
456+
|(span, is_macro_expansion)| {
457+
CoverageSpan::for_terminator(span, is_macro_expansion, bcb, bb)
458+
},
459+
))
412460
})
413461
.collect()
414462
}
@@ -656,7 +704,7 @@ impl<'a, 'tcx> CoverageSpans<'a, 'tcx> {
656704
pub(super) fn filtered_statement_span(
657705
statement: &'a Statement<'tcx>,
658706
body_span: Span,
659-
) -> Option<Span> {
707+
) -> Option<(Span, bool)> {
660708
match statement.kind {
661709
// These statements have spans that are often outside the scope of the executed source code
662710
// for their parent `BasicBlock`.
@@ -701,7 +749,7 @@ pub(super) fn filtered_statement_span(
701749
pub(super) fn filtered_terminator_span(
702750
terminator: &'a Terminator<'tcx>,
703751
body_span: Span,
704-
) -> Option<Span> {
752+
) -> Option<(Span, bool)> {
705753
match terminator.kind {
706754
// These terminators have spans that don't positively contribute to computing a reasonable
707755
// span of actually executed source code. (For example, SwitchInt terminators extracted from
@@ -732,7 +780,13 @@ pub(super) fn filtered_terminator_span(
732780
}
733781

734782
#[inline]
735-
fn function_source_span(span: Span, body_span: Span) -> Span {
783+
fn function_source_span(span: Span, body_span: Span) -> (Span, bool) {
784+
let is_macro_expansion = span.ctxt() != body_span.ctxt()
785+
&& if let ExpnKind::Macro(MacroKind::Bang, _) = span.ctxt().outer_expn_data().kind {
786+
true
787+
} else {
788+
false
789+
};
736790
let span = original_sp(span, body_span).with_ctxt(body_span.ctxt());
737-
if body_span.contains(span) { span } else { body_span }
791+
(if body_span.contains(span) { span } else { body_span }, is_macro_expansion)
738792
}

‎compiler/rustc_mir/src/transform/coverage/tests.rs‎

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,10 @@
11
//! This crate hosts a selection of "unit tests" for components of the `InstrumentCoverage` MIR
22
//! pass.
33
//!
4+
//! ```shell
5+
//! ./x.py test --keep-stage 1 compiler/rustc_mir --test-args '--show-output coverage'
6+
//! ```
7+
//!
48
//! The tests construct a few "mock" objects, as needed, to support the `InstrumentCoverage`
59
//! functions and algorithms. Mocked objects include instances of `mir::Body`; including
610
//! `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() {
679683
let mut basic_coverage_blocks = graph::CoverageGraph::from_mir(&mir_body);
680684
let mut coverage_spans = Vec::new();
681685
for (bcb, data) in basic_coverage_blocks.iter_enumerated() {
682-
if let Some(span) =
686+
if let Some((span, is_macro_expansion)) =
683687
spans::filtered_terminator_span(data.terminator(&mir_body), body_span)
684688
{
685-
coverage_spans.push(spans::CoverageSpan::for_terminator(span, bcb, data.last_bb()));
689+
coverage_spans.push(spans::CoverageSpan::for_terminator(
690+
span,
691+
is_macro_expansion,
692+
bcb,
693+
data.last_bb(),
694+
));
686695
}
687696
}
688697
let mut coverage_counters = counters::CoverageCounters::new(0);

‎src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.issue-84561.txt‎

Lines changed: 102 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,102 @@
1+
1| |// FIXME(#84561): function-like macros produce unintuitive coverage results.
2+
2| |// This test demonstrates some of the problems.
3+
3| |
4+
4| 18|#[derive(Debug, PartialEq, Eq)]
5+
^5 ^0
6+
------------------
7+
| <issue_84561::Foo as core::cmp::PartialEq>::eq:
8+
| 4| 18|#[derive(Debug, PartialEq, Eq)]
9+
------------------
10+
| Unexecuted instantiation: <issue_84561::Foo as core::cmp::PartialEq>::ne
11+
------------------
12+
5| |struct Foo(u32);
13+
6| |
14+
7| 1|fn main() {
15+
8| 1| let bar = Foo(1);
16+
9| 1| assert_eq!(bar, Foo(1));
17+
10| 1| let baz = Foo(0);
18+
11| 1| assert_ne!(baz, Foo(1));
19+
12| 1| println!("{:?}", Foo(1));
20+
13| 1| println!("{:?}", bar);
21+
14| 1| println!("{:?}", baz);
22+
15| 1|
23+
16| 1| assert_eq!(Foo(1), Foo(1));
24+
17| 1| assert_ne!(Foo(0), Foo(1));
25+
18| 1| assert_eq!(Foo(2), Foo(2));
26+
19| 1| let bar = Foo(1);
27+
20| 1| assert_ne!(Foo(0), Foo(3));
28+
21| 1| assert_ne!(Foo(0), Foo(4));
29+
22| 1| assert_eq!(Foo(3), Foo(3));
30+
23| 1| assert_ne!(Foo(0), Foo(5));
31+
24| 1| println!("{:?}", bar);
32+
25| 1| println!("{:?}", Foo(1));
33+
26| 1|
34+
27| 1| let is_true = std::env::args().len() == 1;
35+
28| 1|
36+
29| 1| assert_eq!(
37+
30| 1| Foo(1),
38+
31| 1| Foo(1)
39+
32| 1| );
40+
33| 1| assert_ne!(
41+
34| 1| Foo(0),
42+
35| 1| Foo(1)
43+
36| 1| );
44+
37| 1| assert_eq!(
45+
38| 1| Foo(2),
46+
39| 1| Foo(2)
47+
40| 1| );
48+
41| 1| let bar = Foo(1
49+
42| 1| );
50+
43| 1| assert_ne!(
51+
44| 1| Foo(0),
52+
45| 1| Foo(3)
53+
46| 1| );
54+
47| 1| if is_true {
55+
48| 1| assert_ne!(
56+
49| 1| Foo(0),
57+
50| 1| Foo(4)
58+
51| 1| );
59+
52| | } else {
60+
53| 0| assert_eq!(
61+
54| 0| Foo(3),
62+
55| 0| Foo(3)
63+
56| 0| );
64+
57| | }
65+
58| | assert_ne!(
66+
59| 1| if is_true {
67+
60| 1| Foo(0)
68+
61| | } else {
69+
62| 0| Foo(1)
70+
63| | },
71+
64| | Foo(5)
72+
65| | );
73+
66| 1| assert_ne!(
74+
67| 1| Foo(5),
75+
68| 1| if is_true {
76+
69| 1| Foo(0)
77+
70| | } else {
78+
71| 0| Foo(1)
79+
72| | }
80+
73| | );
81+
74| | assert_ne!(
82+
75| 1| if is_true {
83+
76| 1| assert_eq!(
84+
77| 1| Foo(3),
85+
78| 1| Foo(3)
86+
79| 1| );
87+
80| 1| Foo(0)
88+
81| | } else {
89+
82| | assert_ne!(
90+
83| 0| if is_true {
91+
84| 0| Foo(0)
92+
85| | } else {
93+
86| 0| Foo(1)
94+
87| | },
95+
88| | Foo(5)
96+
89| | );
97+
90| 0| Foo(1)
98+
91| | },
99+
92| | Foo(5)
100+
93| | );
101+
94| 1|}
102+

‎src/test/run-make-fulldeps/coverage/issue-84561.rs‎

Lines changed: 94 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,94 @@
1+
// FIXME(#84561): function-like macros produce unintuitive coverage results.
2+
// This test demonstrates some of the problems.
3+
4+
#[derive(Debug, PartialEq, Eq)]
5+
struct Foo(u32);
6+
7+
fn main() {
8+
let bar = Foo(1);
9+
assert_eq!(bar, Foo(1));
10+
let baz = Foo(0);
11+
assert_ne!(baz, Foo(1));
12+
println!("{:?}", Foo(1));
13+
println!("{:?}", bar);
14+
println!("{:?}", baz);
15+
16+
assert_eq!(Foo(1), Foo(1));
17+
assert_ne!(Foo(0), Foo(1));
18+
assert_eq!(Foo(2), Foo(2));
19+
let bar = Foo(1);
20+
assert_ne!(Foo(0), Foo(3));
21+
assert_ne!(Foo(0), Foo(4));
22+
assert_eq!(Foo(3), Foo(3));
23+
assert_ne!(Foo(0), Foo(5));
24+
println!("{:?}", bar);
25+
println!("{:?}", Foo(1));
26+
27+
let is_true = std::env::args().len() == 1;
28+
29+
assert_eq!(
30+
Foo(1),
31+
Foo(1)
32+
);
33+
assert_ne!(
34+
Foo(0),
35+
Foo(1)
36+
);
37+
assert_eq!(
38+
Foo(2),
39+
Foo(2)
40+
);
41+
let bar = Foo(1
42+
);
43+
assert_ne!(
44+
Foo(0),
45+
Foo(3)
46+
);
47+
if is_true {
48+
assert_ne!(
49+
Foo(0),
50+
Foo(4)
51+
);
52+
} else {
53+
assert_eq!(
54+
Foo(3),
55+
Foo(3)
56+
);
57+
}
58+
assert_ne!(
59+
if is_true {
60+
Foo(0)
61+
} else {
62+
Foo(1)
63+
},
64+
Foo(5)
65+
);
66+
assert_ne!(
67+
Foo(5),
68+
if is_true {
69+
Foo(0)
70+
} else {
71+
Foo(1)
72+
}
73+
);
74+
assert_ne!(
75+
if is_true {
76+
assert_eq!(
77+
Foo(3),
78+
Foo(3)
79+
);
80+
Foo(0)
81+
} else {
82+
assert_ne!(
83+
if is_true {
84+
Foo(0)
85+
} else {
86+
Foo(1)
87+
},
88+
Foo(5)
89+
);
90+
Foo(1)
91+
},
92+
Foo(5)
93+
);
94+
}

0 commit comments

Comments
 (0)