Skip to content

Commit 1c2c6b6

Browse files
borsbors
committed
Auto merge of #84582 - richkadel:issue-84561, r=tmandry
Vastly improves coverage spans for macros Fixes: #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 3d67e07 + 0312bf5 commit 1c2c6b6

13 files changed

+861
-68
lines changed

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

+201-41
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.closure_macro.txt

+42
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,42 @@
1+
1| |// compile-flags: --edition=2018
2+
2| |#![feature(no_coverage)]
3+
3| |
4+
4| |macro_rules! bail {
5+
5| | ($msg:literal $(,)?) => {
6+
6| | if $msg.len() > 0 {
7+
7| | println!("no msg");
8+
8| | } else {
9+
9| | println!($msg);
10+
10| | }
11+
11| | return Err(String::from($msg));
12+
12| | };
13+
13| |}
14+
14| |
15+
15| |macro_rules! on_error {
16+
16| | ($value:expr, $error_message:expr) => {
17+
17| 0| $value.or_else(|e| {
18+
18| 0| let message = format!($error_message, e);
19+
19| 0| if message.len() > 0 {
20+
20| 0| println!("{}", message);
21+
21| 0| Ok(String::from("ok"))
22+
22| | } else {
23+
23| 0| bail!("error");
24+
24| | }
25+
25| 0| })
26+
26| | };
27+
27| |}
28+
28| |
29+
29| 1|fn load_configuration_files() -> Result<String, String> {
30+
30| 1| Ok(String::from("config"))
31+
31| 1|}
32+
32| |
33+
33| 1|pub fn main() -> Result<(), String> {
34+
34| 1| println!("Starting service");
35+
35| 1| let config = on_error!(load_configuration_files(), "Error loading configs: {}")?;
36+
^0
37+
36| |
38+
37| 1| let startup_delay_duration = String::from("arg");
39+
38| 1| let _ = (config, startup_delay_duration);
40+
39| 1| Ok(())
41+
40| 1|}
42+

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

+83
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,83 @@
1+
1| |// compile-flags: --edition=2018
2+
2| |#![feature(no_coverage)]
3+
3| |
4+
4| |macro_rules! bail {
5+
5| | ($msg:literal $(,)?) => {
6+
6| | if $msg.len() > 0 {
7+
7| | println!("no msg");
8+
8| | } else {
9+
9| | println!($msg);
10+
10| | }
11+
11| | return Err(String::from($msg));
12+
12| | };
13+
13| |}
14+
14| |
15+
15| |macro_rules! on_error {
16+
16| | ($value:expr, $error_message:expr) => {
17+
17| 0| $value.or_else(|e| {
18+
18| 0| let message = format!($error_message, e);
19+
19| 0| if message.len() > 0 {
20+
20| 0| println!("{}", message);
21+
21| 0| Ok(String::from("ok"))
22+
22| | } else {
23+
23| 0| bail!("error");
24+
24| | }
25+
25| 0| })
26+
26| | };
27+
27| |}
28+
28| |
29+
29| 1|fn load_configuration_files() -> Result<String, String> {
30+
30| 1| Ok(String::from("config"))
31+
31| 1|}
32+
32| |
33+
33| 1|pub async fn test() -> Result<(), String> {
34+
34| 1| println!("Starting service");
35+
35| 1| let config = on_error!(load_configuration_files(), "Error loading configs: {}")?;
36+
^0
37+
36| |
38+
37| 1| let startup_delay_duration = String::from("arg");
39+
38| 1| let _ = (config, startup_delay_duration);
40+
39| 1| Ok(())
41+
40| 1|}
42+
41| |
43+
42| |#[no_coverage]
44+
43| |fn main() {
45+
44| | executor::block_on(test());
46+
45| |}
47+
46| |
48+
47| |mod executor {
49+
48| | use core::{
50+
49| | future::Future,
51+
50| | pin::Pin,
52+
51| | task::{Context, Poll, RawWaker, RawWakerVTable, Waker},
53+
52| | };
54+
53| |
55+
54| | #[no_coverage]
56+
55| | pub fn block_on<F: Future>(mut future: F) -> F::Output {
57+
56| | let mut future = unsafe { Pin::new_unchecked(&mut future) };
58+
57| | use std::hint::unreachable_unchecked;
59+
58| | static VTABLE: RawWakerVTable = RawWakerVTable::new(
60+
59| |
61+
60| | #[no_coverage]
62+
61| | |_| unsafe { unreachable_unchecked() }, // clone
63+
62| |
64+
63| | #[no_coverage]
65+
64| | |_| unsafe { unreachable_unchecked() }, // wake
66+
65| |
67+
66| | #[no_coverage]
68+
67| | |_| unsafe { unreachable_unchecked() }, // wake_by_ref
69+
68| |
70+
69| | #[no_coverage]
71+
70| | |_| (),
72+
71| | );
73+
72| | let waker = unsafe { Waker::from_raw(RawWaker::new(core::ptr::null(), &VTABLE)) };
74+
73| | let mut context = Context::from_waker(&waker);
75+
74| |
76+
75| | loop {
77+
76| | if let Poll::Ready(val) = future.as_mut().poll(&mut context) {
78+
77| | break val;
79+
78| | }
80+
79| | }
81+
80| | }
82+
81| |}
83+

‎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-83601.txt

+2-2
Original file line numberDiff line numberDiff line change
@@ -12,9 +12,9 @@
1212
5| |
1313
6| 1|fn main() {
1414
7| 1| let bar = Foo(1);
15-
8| 0| assert_eq!(bar, Foo(1));
15+
8| 1| assert_eq!(bar, Foo(1));
1616
9| 1| let baz = Foo(0);
17-
10| 0| assert_ne!(baz, Foo(1));
17+
10| 1| assert_ne!(baz, Foo(1));
1818
11| 1| println!("{:?}", Foo(1));
1919
12| 1| println!("{:?}", bar);
2020
13| 1| println!("{:?}", baz);

0 commit comments

Comments
 (0)