Vastly improves coverage spans for macros #84582

richkadel · 2021-04-26T09:50:30Z

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

scole66 · 2021-04-27T01:28:49Z

Also fixes #82853, I believe.

richkadel · 2021-04-27T04:31:58Z

@tmandry I moved the spanview debug output ICE fix to this PR instead of the Eq #[no_coverage] PR since this one also caused the problem (during debugging only), and this PR should land much more quickly, pending your review.

Thanks!

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

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

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

richkadel · 2021-04-28T04:52:51Z

@tmandry - Great review comments. I resolved the simple requests. Check out my replies (and changes, where relevant).

Thanks!

richkadel · 2021-04-28T04:55:42Z

(To be a little more clear, I addressed all of your comments. I just marked the simple ones resolved, to reduce the clutter on this page.)

tmandry · 2021-04-29T01:18:46Z

Looks good, thanks for making those changes!

@bors r+

bors · 2021-04-29T01:18:48Z

📌 Commit fd85fd3 has been approved by tmandry

Fixes: rust-lang#84561

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`

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``

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````

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 · 2021-04-29T23:20:43Z

Possibly failed in #84720 (comment)

@bors rollup=iffy

richkadel · 2021-04-30T04:21:59Z

Possibly failed in #84720 (comment)

@bors rollup=iffy

Oh, darn. This test output changed after the new #[no_coverage] attribute was added a couple of days ago,

The Eq no longer shows 0 coverage.

But this PR was uploaded before the Eq coverage issue was fixed.

Will fix.

     2|       |
     3|       |// expect-exit-status-101
     4|     21|#[derive(PartialEq, Eq)]
-                                  ^0
   ------------------

richkadel · 2021-04-30T04:43:36Z

@bors r-

I may have found one more edge case, plus I'm not sure if I need to re-bless one other test result.

richkadel · 2021-04-30T07:24:22Z

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

            self.prev_mut().span = self.prev().span.with_lo(right_cutoff);
-            self.prev_original_span = self.prev().span;


This line updating prev_original_span to match the mutated self.prev().span has been wrong for a long time. It's clear, now, that the original_span should never be mutated, and this was always a bug.

I think recent changes elsewhere may have exposed this bug, but even so, it still occurs very rarely.

I added some tests to try to replicate the failures I saw when compiling a very large codebase, but it's very hard to replicate. (It seems to occur when calling a macro that contains a closure and returns a Result, checked with the ? try operator. But even that is not enough to replicate the problem. It also requires that the macro code branches, and that it produces just the right order of dominator relationship between basic blocks, with just the right nested but equal spans.

The bug is clear to me even if the specific situation leading up to that bug is not as clear. I validated this against the code that triggered the error (at least 3 instances) and removing this line corrected the problem, allowing the codebase to compile to completion.

richkadel · 2021-04-30T07:25:26Z

r? @tmandry - can you take a quick look and re-approve / re-submit to bors?

richkadel · 2021-04-30T07:27:40Z

@rustbot label -S-waiting-on-author +S-waiting-on-review

tmandry · 2021-04-30T17:49:03Z

@bors r+

bors · 2021-04-30T17:49:04Z

📌 Commit 0312bf5 has been approved by tmandry

bors · 2021-05-01T07:48:28Z

⌛ Testing commit 0312bf5 with merge 1c2c6b6...

bors · 2021-05-01T09:59:49Z

☀️ Test successful - checks-actions
Approved by: tmandry
Pushing 1c2c6b6 to master...

rust-highfive assigned tmandry Apr 26, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 26, 2021

richkadel force-pushed the issue-84561 branch from a484cd4 to e2c44f5 Compare April 26, 2021 09:52

This comment has been minimized.

Sign in to view

richkadel force-pushed the issue-84561 branch 2 times, most recently from b76f133 to 2d72f54 Compare April 26, 2021 10:11

This comment has been minimized.

Sign in to view

richkadel force-pushed the issue-84561 branch from 2d72f54 to 9e02746 Compare April 26, 2021 16:57

richkadel changed the title ~~Drop branching blocks with same span as expanded macro~~ Vastly improves coverage spans for macros Apr 26, 2021

richkadel mentioned this pull request Apr 27, 2021

Adds feature-gated #[no_coverage] function attribute, to fix derived Eq 0 coverage issue #83601 #84562

Merged

tmandry reviewed Apr 28, 2021

View reviewed changes

richkadel force-pushed the issue-84561 branch from dea2048 to fd85fd3 Compare April 28, 2021 04:47

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 29, 2021

richkadel added 4 commits April 28, 2021 20:27

Drop branching blocks with same span as expanded macro

c26afb7

Fixes: rust-lang#84561

More improvements to macro coverage

f12795f

spanview debug output caused ICE when a function had no body

c97d899

addressed review feedback

eef546a

Dylan-DPC-zz mentioned this pull request Apr 29, 2021

Rollup of 8 pull requests #84689

Closed

Dylan-DPC-zz mentioned this pull request Apr 29, 2021

Rollup of 7 pull requests #84691

Closed

Dylan-DPC-zz mentioned this pull request Apr 29, 2021

Rollup of 8 pull requests #84693

Closed

jackh726 mentioned this pull request Apr 29, 2021

Rollup of 11 pull requests #84718

Closed

jackh726 mentioned this pull request Apr 29, 2021

Rollup of 10 pull requests #84720

Closed

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Apr 30, 2021

richkadel force-pushed the issue-84561 branch from 0292728 to 187b9bc Compare April 30, 2021 07:16

richkadel commented Apr 30, 2021

View reviewed changes

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 30, 2021

Rebuilt out of date tests and fixed an old bug now exposed

0312bf5

richkadel force-pushed the issue-84561 branch from 187b9bc to 0312bf5 Compare April 30, 2021 08:10

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 30, 2021

bors added the merged-by-bors This PR was explicitly merged by bors. label May 1, 2021

bors merged commit 1c2c6b6 into rust-lang:master May 1, 2021

rustbot added this to the 1.54.0 milestone May 1, 2021

richkadel mentioned this pull request May 1, 2021

LLVM Coverage and "assert_eq!" #82853

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Vastly improves coverage spans for macros #84582

Vastly improves coverage spans for macros #84582

richkadel commented Apr 26, 2021 •

edited

Loading

This comment has been minimized.

This comment has been minimized.

scole66 commented Apr 27, 2021

richkadel commented Apr 27, 2021

richkadel commented Apr 28, 2021

richkadel commented Apr 28, 2021

tmandry commented Apr 29, 2021

bors commented Apr 29, 2021

jackh726 commented Apr 29, 2021

richkadel commented Apr 30, 2021

richkadel commented Apr 30, 2021

richkadel Apr 30, 2021

richkadel commented Apr 30, 2021

richkadel commented Apr 30, 2021

tmandry commented Apr 30, 2021

bors commented Apr 30, 2021

bors commented May 1, 2021

bors commented May 1, 2021

		self.prev_mut().span = self.prev().span.with_lo(right_cutoff);
		self.prev_original_span = self.prev().span;

Vastly improves coverage spans for macros #84582

Vastly improves coverage spans for macros #84582

Conversation

richkadel commented Apr 26, 2021 • edited Loading

This comment has been minimized.

This comment has been minimized.

scole66 commented Apr 27, 2021

richkadel commented Apr 27, 2021

richkadel commented Apr 28, 2021

richkadel commented Apr 28, 2021

tmandry commented Apr 29, 2021

bors commented Apr 29, 2021

jackh726 commented Apr 29, 2021

richkadel commented Apr 30, 2021

richkadel commented Apr 30, 2021

richkadel Apr 30, 2021

Choose a reason for hiding this comment

richkadel commented Apr 30, 2021

richkadel commented Apr 30, 2021

tmandry commented Apr 30, 2021

bors commented Apr 30, 2021

bors commented May 1, 2021

bors commented May 1, 2021

richkadel commented Apr 26, 2021 •

edited

Loading