Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

function-like macros, esp. with internal branching, produce unintuitive coverage results #84561

Closed
richkadel opened this issue Apr 25, 2021 · 0 comments · Fixed by #84582
Closed

Comments

@richkadel
Copy link
Contributor

When compiling with -Zinstrument-coverage, Rust code that uses functions like trace!("some trace message"); or assert_eq!(val1, val2; results in unintuitive coverage results.

Macros are not fully supported by the Rust -Zinstrument-coverage option because coverage regions are generated during a MIR pass only (InstrumentCoverage). The MIR includes the BasicBlocks (statements, terminators, and branching) from the expanded source AFTER processing the macros, and the coverage regions, shown in a function's coverage report, must relate to the source region of the visible source (including the macro invocations).

An invoked macro's source is not visible from the invoking function, so there is no practical way to map branching spans in a macro to the function's coverage regions.

As explained below, it is difficult to imagine a general solution to this problem, since macro syntax is so fluid.

Possible workaround

Perhaps a workaround would be to consider a macro invocation as if it was a function call, and ignore all MIR statements with the same span as the first statement encountered within the expanded macro.

Examples

For instance, if trace!("a message") equates to if trace_is_enabled() { println!("a message"); } then it's possible that the condition (the call to, and result from, trace_is_enabled()) will be executed more often than the println!(...). There should be two different coverage regions, but trace!("a message") only represents one coverage region for both BasicBlocks.

Due to the way MIR InstrumentCoverage works, the inner most condition will determine whether the macro invocation appears covered; that is, if trace_is_enabled(), the span for trace!("a message") will appear covered, and if trace is not enabled, the span for trace!("a message") will appear uncovered (zero executions).

trace!("a message") looks like a function call (similar to some_function("a message"), but it is not a function call. With some_function(...), there is no condition, and no branching, so if the function is reached, then entire span is covered. When trace!("a message") is reached, clearly we cannot say the entire hidden span (the internal expansion of trace! macro is covered, so this may appear non-intuitive when compared to an actual function.

assert_eq(val1, val2) is another example. If the optional failure message is included, assert_eq(val1, val2, "they are not equal") for example, the issue is similar to the problem with trace!(). It might be nice to associate the macro's internal condition (probably something like, val1 == val2) with the span val1, val2 within the macro, but there is no general way to make that association, that I know of.

But the assert_eq(val1, val2) also exposes another non-intuitive coverage behavior. Take the example:

#[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));
}

This partial coverage result (starting from the main()) shows something unexpected:

    4|      1|fn main() {
    5|      1|    let bar = Foo(1);
    6|      0|    assert_eq!(bar, Foo(1));
    7|      1|    let baz = Foo(0);
    8|      0|    assert_ne!(baz, Foo(1));
    9|      1|    println!("{:?}", Foo(1));
   10|      1|    println!("{:?}", bar);
   11|      1|    println!("{:?}", baz);
   12|       |
   13|      1|    assert_eq!(Foo(1), Foo(1));
   14|      1|    assert_ne!(Foo(0), Foo(1));
   15|      0|    assert_eq!(Foo(2), Foo(2));
   16|      1|    let bar = Foo(1);
   17|      1|    assert_ne!(Foo(0), Foo(3));
   18|      1|    assert_ne!(Foo(0), Foo(4));
   19|      1|    assert_eq!(Foo(3), Foo(3));
   20|      0|    assert_ne!(Foo(0), Foo(5));
   21|      1|    println!("{:?}", bar);
   22|      1|    println!("{:?}", Foo(1));
   23|      1|}

The assert_... invocations on lines 6 and 8 show as uncovered, which is unintuitive, but for the same reason that trace!() appears uncovered if trace is not enabled: The condition (comparison) is checked, but it returns false (because the assertion does not fail), so the internal block (the panic) is not executed, and internal blocks win the coverage battle when the spans are the same (in this case, all statements related to comparing the values and optionally panicking have the same span, because there's no general way to map the condition check to one part of the macro, and the optional panic block to a different part of the macro invocation.

But also see lines 13 and 14.

These invocations of assert_eq!() and assert_ne!() appear covered?

This is a slightly different problem, and maybe there is a general way to improve this, but I'm describing the problem here in this same issue because "improving" the result for lines 13 and 14 would at best be to make them consistent with lines 6 and 8, and the consistency is not particularly helpful. A workaround, therefore, may be a waste of time, depending on how we might someday improve macro handling in general.

The reason lines 13 and 14 appear covered is because those assert_* invocations occur immediately before one more assert_eq!() on line 15 (which appears uncovered).

compiler/rustc_mir/src/transform/coverage/spans.rs generates a reduced set of coverage regions (see to_refined_spans(), for example, by combining contiguous spans from the same BasicCoverageBlock. The algorithm works well for MIR statements based on actual source lines in the function.

The assert_* macro invocations on lines 13 and 14 have different spans from line 15, to put it simply, the existence of a different assert_* macro on a successive line (line 14, for line 13, and line 15 for line 14) allows a different coverage span to be generated that can be counted. (Debug logging for this module can help understand the exact logic.) As a result, if two or more assert_* macros are executed, contiguously, only the last one will appear uncovered (assuming all assertions were true).

richkadel added a commit to richkadel/rust that referenced this issue Apr 28, 2021
The Eq trait has a special hidden function. MIR `InstrumentCoverage`
would add this function to the coverage map, but it is never called, so
the `Eq` trait would always appear uncovered.

Fixes: rust-lang#83601

The fix required creating a new function attribute `no_coverage` to mark
functions that should be ignored by `InstrumentCoverage` and the
coverage `mapgen` (during codegen).

While testing, I also noticed two other issues:

* spanview debug file output ICEd on a function with no body. The
workaround for this is included in this PR.
* `assert_*!()` macro coverage can appear covered if followed by another
`assert_*!()` macro. Normally they appear uncovered. I submitted a new
Issue rust-lang#84561, and added a coverage test to demonstrate this issue.
richkadel added a commit to richkadel/rust that referenced this issue Apr 28, 2021
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Apr 29, 2021
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`
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Apr 29, 2021
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``
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Apr 29, 2021
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```
jackh726 added a commit to jackh726/rust that referenced this issue Apr 29, 2021
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````
jackh726 added a commit to jackh726/rust that referenced this issue Apr 29, 2021
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`````
bors added a commit to rust-lang-ci/rust that referenced this issue May 1, 2021
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`
@bors bors closed this as completed in c26afb7 May 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant