Skip to content

Commit 844f638

Browse files
jackh726jackh726
authored
Rollup merge of rust-lang#84582 - richkadel:issue-84561, r=tmandry
Vastly improves coverage spans for macros Fixes: rust-lang#84561 This resolves problems where macros like `trace!(...)` would show zero coverage if tracing was disabled, and `assert_eq!(...)` would show zero coverage if the assertion did not fail, because only one coverage span was generated, for the branch. This PR started with an idea that I could just drop branching blocks with same span as expanded macro. (See the fixed issue for more details.) That did help, but it didn't resolve everything. I also needed to add a span specifically for the macro name (plus `!`) to ensure the macro gets coverage even if it's internal expansion adds conditional branching blocks that are retained, and would otherwise drop the outer span. Now that outer span is _only_ the `(argument, list)`, which can safely be dropped now), because the macro name has its own span. While testing, I also noticed the spanview debug output can cause an ICE on a function with no body. The workaround for this is included in this PR (separate commit). r? `````@tmandry````` cc? `````@wesleywiser`````
2 parents 3ad9b00 + fd85fd3 commit 844f638

File tree

8 files changed

+587
-56
lines changed

8 files changed

+587
-56
lines changed

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

+176-33
Large diffs are not rendered by default.

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

+11-2
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, expn_span)) =
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+
expn_span,
692+
bcb,
693+
data.last_bb(),
694+
));
686695
}
687696
}
688697
let mut coverage_counters = counters::CoverageCounters::new(0);

‎compiler/rustc_mir/src/util/spanview.rs

+10-9
Original file line numberDiff line numberDiff line change
@@ -99,7 +99,11 @@ where
9999
W: Write,
100100
{
101101
let def_id = body.source.def_id();
102-
let body_span = hir_body(tcx, def_id).value.span;
102+
let hir_body = hir_body(tcx, def_id);
103+
if hir_body.is_none() {
104+
return Ok(());
105+
}
106+
let body_span = hir_body.unwrap().value.span;
103107
let mut span_viewables = Vec::new();
104108
for (bb, data) in body.basic_blocks().iter_enumerated() {
105109
match spanview {
@@ -664,19 +668,16 @@ fn fn_span<'tcx>(tcx: TyCtxt<'tcx>, def_id: DefId) -> Span {
664668
let hir_id =
665669
tcx.hir().local_def_id_to_hir_id(def_id.as_local().expect("expected DefId is local"));
666670
let fn_decl_span = tcx.hir().span(hir_id);
667-
let body_span = hir_body(tcx, def_id).value.span;
668-
if fn_decl_span.ctxt() == body_span.ctxt() {
669-
fn_decl_span.to(body_span)
671+
if let Some(body_span) = hir_body(tcx, def_id).map(|hir_body| hir_body.value.span) {
672+
if fn_decl_span.ctxt() == body_span.ctxt() { fn_decl_span.to(body_span) } else { body_span }
670673
} else {
671-
// This probably occurs for functions defined via macros
672-
body_span
674+
fn_decl_span
673675
}
674676
}
675677

676-
fn hir_body<'tcx>(tcx: TyCtxt<'tcx>, def_id: DefId) -> &'tcx rustc_hir::Body<'tcx> {
678+
fn hir_body<'tcx>(tcx: TyCtxt<'tcx>, def_id: DefId) -> Option<&'tcx rustc_hir::Body<'tcx>> {
677679
let hir_node = tcx.hir().get_if_local(def_id).expect("expected DefId is local");
678-
let fn_body_id = hir::map::associated_body(hir_node).expect("HIR node is a function with body");
679-
tcx.hir().body(fn_body_id)
680+
hir::map::associated_body(hir_node).map(|fn_body_id| tcx.hir().body(fn_body_id))
680681
}
681682

682683
fn escape_html(s: &str) -> String {

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

+3-3
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,9 @@
11
1| |#![allow(unused_assignments, unused_variables, dead_code)]
22
2| |
33
3| 1|fn main() {
4-
4| | // Initialize test constants in a way that cannot be determined at compile time, to ensure
5-
5| | // rustc and LLVM cannot optimize out statements (or coverage counters) downstream from
6-
6| | // dependent conditions.
4+
4| 1| // Initialize test constants in a way that cannot be determined at compile time, to ensure
5+
5| 1| // rustc and LLVM cannot optimize out statements (or coverage counters) downstream from
6+
6| 1| // dependent conditions.
77
7| 1| let is_true = std::env::args().len() == 1;
88
8| 1|
99
9| 1| let mut countdown = 0;

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

+196
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,196 @@
1+
1| |// This demonstrated Issue #84561: function-like macros produce unintuitive coverage results.
2+
2| |
3+
3| |// expect-exit-status-101
4+
4| 21|#[derive(PartialEq, Eq)]
5+
^0
6+
------------------
7+
| <issue_84561::Foo as core::cmp::PartialEq>::eq:
8+
| 4| 21|#[derive(PartialEq, Eq)]
9+
------------------
10+
| Unexecuted instantiation: <issue_84561::Foo as core::cmp::PartialEq>::ne
11+
------------------
12+
5| |struct Foo(u32);
13+
6| 1|fn test3() {
14+
7| 1| let is_true = std::env::args().len() == 1;
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(0);
27+
20| 1| assert_ne!(bar, Foo(3));
28+
21| 1| assert_ne!(Foo(0), Foo(4));
29+
22| 1| assert_eq!(Foo(3), Foo(3), "with a message");
30+
^0
31+
23| 1| println!("{:?}", bar);
32+
24| 1| println!("{:?}", Foo(1));
33+
25| 1|
34+
26| 1| assert_ne!(Foo(0), Foo(5), "{}", if is_true { "true message" } else { "false message" });
35+
^0 ^0 ^0
36+
27| 1| assert_ne!(
37+
28| | Foo(0)
38+
29| | ,
39+
30| | Foo(5)
40+
31| | ,
41+
32| 0| "{}"
42+
33| 0| ,
43+
34| 0| if
44+
35| 0| is_true
45+
36| | {
46+
37| 0| "true message"
47+
38| | } else {
48+
39| 0| "false message"
49+
40| | }
50+
41| | );
51+
42| |
52+
43| 1| let is_true = std::env::args().len() == 1;
53+
44| 1|
54+
45| 1| assert_eq!(
55+
46| 1| Foo(1),
56+
47| 1| Foo(1)
57+
48| 1| );
58+
49| 1| assert_ne!(
59+
50| 1| Foo(0),
60+
51| 1| Foo(1)
61+
52| 1| );
62+
53| 1| assert_eq!(
63+
54| 1| Foo(2),
64+
55| 1| Foo(2)
65+
56| 1| );
66+
57| 1| let bar = Foo(1);
67+
58| 1| assert_ne!(
68+
59| 1| bar,
69+
60| 1| Foo(3)
70+
61| 1| );
71+
62| 1| if is_true {
72+
63| 1| assert_ne!(
73+
64| 1| Foo(0),
74+
65| 1| Foo(4)
75+
66| 1| );
76+
67| | } else {
77+
68| 0| assert_eq!(
78+
69| 0| Foo(3),
79+
70| 0| Foo(3)
80+
71| 0| );
81+
72| | }
82+
73| 1| if is_true {
83+
74| 1| assert_ne!(
84+
75| | Foo(0),
85+
76| | Foo(4),
86+
77| 0| "with a message"
87+
78| | );
88+
79| | } else {
89+
80| 0| assert_eq!(
90+
81| | Foo(3),
91+
82| | Foo(3),
92+
83| 0| "with a message"
93+
84| | );
94+
85| | }
95+
86| 1| assert_ne!(
96+
87| 1| if is_true {
97+
88| 1| Foo(0)
98+
89| | } else {
99+
90| 0| Foo(1)
100+
91| | },
101+
92| | Foo(5)
102+
93| | );
103+
94| 1| assert_ne!(
104+
95| 1| Foo(5),
105+
96| 1| if is_true {
106+
97| 1| Foo(0)
107+
98| | } else {
108+
99| 0| Foo(1)
109+
100| | }
110+
101| | );
111+
102| 1| assert_ne!(
112+
103| 1| if is_true {
113+
104| 1| assert_eq!(
114+
105| 1| Foo(3),
115+
106| 1| Foo(3)
116+
107| 1| );
117+
108| 1| Foo(0)
118+
109| | } else {
119+
110| 0| assert_ne!(
120+
111| 0| if is_true {
121+
112| 0| Foo(0)
122+
113| | } else {
123+
114| 0| Foo(1)
124+
115| | },
125+
116| | Foo(5)
126+
117| | );
127+
118| 0| Foo(1)
128+
119| | },
129+
120| | Foo(5),
130+
121| 0| "with a message"
131+
122| | );
132+
123| 1| assert_eq!(
133+
124| | Foo(1),
134+
125| | Foo(3),
135+
126| 1| "this assert should fail"
136+
127| | );
137+
128| 0| assert_eq!(
138+
129| | Foo(3),
139+
130| | Foo(3),
140+
131| 0| "this assert should not be reached"
141+
132| | );
142+
133| 0|}
143+
134| |
144+
135| |impl std::fmt::Debug for Foo {
145+
136| | fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {
146+
137| 7| write!(f, "try and succeed")?;
147+
^0
148+
138| 7| Ok(())
149+
139| 7| }
150+
140| |}
151+
141| |
152+
142| |static mut DEBUG_LEVEL_ENABLED: bool = false;
153+
143| |
154+
144| |macro_rules! debug {
155+
145| | ($($arg:tt)+) => (
156+
146| | if unsafe { DEBUG_LEVEL_ENABLED } {
157+
147| | println!($($arg)+);
158+
148| | }
159+
149| | );
160+
150| |}
161+
151| |
162+
152| 1|fn test1() {
163+
153| 1| debug!("debug is enabled");
164+
^0
165+
154| 1| debug!("debug is enabled");
166+
^0
167+
155| 1| let _ = 0;
168+
156| 1| debug!("debug is enabled");
169+
^0
170+
157| 1| unsafe {
171+
158| 1| DEBUG_LEVEL_ENABLED = true;
172+
159| 1| }
173+
160| 1| debug!("debug is enabled");
174+
161| 1|}
175+
162| |
176+
163| |macro_rules! call_debug {
177+
164| | ($($arg:tt)+) => (
178+
165| 1| fn call_print(s: &str) {
179+
166| 1| print!("{}", s);
180+
167| 1| }
181+
168| |
182+
169| | call_print("called from call_debug: ");
183+
170| | debug!($($arg)+);
184+
171| | );
185+
172| |}
186+
173| |
187+
174| 1|fn test2() {
188+
175| 1| call_debug!("debug is enabled");
189+
176| 1|}
190+
177| |
191+
178| 1|fn main() {
192+
179| 1| test1();
193+
180| 1| test2();
194+
181| 1| test3();
195+
182| 1|}
196+

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

+3-3
Original file line numberDiff line numberDiff line change
@@ -3,9 +3,9 @@
33
3| |use std::fmt::Debug;
44
4| |
55
5| 1|pub fn used_function() {
6-
6| | // Initialize test constants in a way that cannot be determined at compile time, to ensure
7-
7| | // rustc and LLVM cannot optimize out statements (or coverage counters) downstream from
8-
8| | // dependent conditions.
6+
6| 1| // Initialize test constants in a way that cannot be determined at compile time, to ensure
7+
7| 1| // rustc and LLVM cannot optimize out statements (or coverage counters) downstream from
8+
8| 1| // dependent conditions.
99
9| 1| let is_true = std::env::args().len() == 1;
1010
10| 1| let mut countdown = 0;
1111
11| 1| if is_true {

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

+6-6
Original file line numberDiff line numberDiff line change
@@ -5,9 +5,9 @@
55
5| |use std::fmt::Debug;
66
6| |
77
7| 1|pub fn used_function() {
8-
8| | // Initialize test constants in a way that cannot be determined at compile time, to ensure
9-
9| | // rustc and LLVM cannot optimize out statements (or coverage counters) downstream from
10-
10| | // dependent conditions.
8+
8| 1| // Initialize test constants in a way that cannot be determined at compile time, to ensure
9+
9| 1| // rustc and LLVM cannot optimize out statements (or coverage counters) downstream from
10+
10| 1| // dependent conditions.
1111
11| 1| let is_true = std::env::args().len() == 1;
1212
12| 1| let mut countdown = 0;
1313
13| 1| if is_true {
@@ -19,9 +19,9 @@
1919
18| |
2020
19| |#[inline(always)]
2121
20| 1|pub fn used_inline_function() {
22-
21| | // Initialize test constants in a way that cannot be determined at compile time, to ensure
23-
22| | // rustc and LLVM cannot optimize out statements (or coverage counters) downstream from
24-
23| | // dependent conditions.
22+
21| 1| // Initialize test constants in a way that cannot be determined at compile time, to ensure
23+
22| 1| // rustc and LLVM cannot optimize out statements (or coverage counters) downstream from
24+
23| 1| // dependent conditions.
2525
24| 1| let is_true = std::env::args().len() == 1;
2626
25| 1| let mut countdown = 0;
2727
26| 1| if is_true {

0 commit comments

Comments
 (0)