Page MenuHomePhabricator

[Coverage] Emit gap region between statements if first statements contains terminate statements.
ClosedPublic

Authored by zequanwu on Feb 19 2021, 4:54 PM.

Details

Summary

This solves the problems that D85036 tried to solve, but that patch introduced
new bugs and those bugs should be fixed in the clang coverage rather than
llvm-cov. I'll revert D85036 after this is landed to avoid reintroduce the bugs.

Diff Detail

Event Timeline

zequanwu created this revision.Feb 19 2021, 4:54 PM
zequanwu requested review of this revision.Feb 19 2021, 4:54 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 19 2021, 4:54 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
rnk added a comment.Feb 22 2021, 11:11 AM

Are the check-profile presubmit test failures related to this patch? Do we have to revert D85036 in the same patch in order to keep the integration tests green?

zequanwu updated this revision to Diff 325516.Feb 22 2021, 11:39 AM

Rebase and fix runtime-counter-relocation.c

Do we have to revert D85036 in the same patch in order to keep the integration tests green?

I didn't revert D85036. I don't know why phabricator thinks I did.

Herald added a project: Restricted Project. · View Herald TranscriptFeb 22 2021, 11:39 AM
Herald added a subscriber: Restricted Project. · View Herald Transcript
vsk added inline comments.Feb 22 2021, 6:06 PM
clang/lib/CodeGen/CoverageMappingGen.cpp
546–550

nit: Whether the visitor is at a ... stmt?

583

Does this mean a deferred gap region following a break; statement won't be completed before the next region is pushed?

870

What's supposed to be the difference between the zero region pushed after a return; vs. after a break;?

987

Can this replace all of the machinery around DeferredRegions? It'd be nice to just have one way to set up gaps between statements.

zequanwu updated this revision to Diff 325947.Feb 23 2021, 5:57 PM
zequanwu marked an inline comment as done.
  • Revert the deletion of completeDeferred in pushRegion.
  • Emit zero gap region only if OutCounter is different from parentCounter in whil, for statements.
  • Update test cases.
zequanwu added inline comments.Feb 23 2021, 6:11 PM
clang/lib/CodeGen/CoverageMappingGen.cpp
583

I reverted this change.

870

What I think is DeferredRegion is only used for break; and continue. Other terminate statements like return;, throw etc will use the logic in VisitStmt to emit gap region. So, I added this condition to separate the two cases.

987

See above comments.

vsk added inline comments.Mar 1 2021, 10:44 AM
clang/lib/CodeGen/CoverageMappingGen.cpp
870

What do you think of the notion of using the gaps inserted in VisitStmt to replace the whole deferred region system? Is it something that might be feasible (if perhaps out of scope for this patch), or do you see a fundamental reason it can't/shouldn't be done?

zequanwu added inline comments.Mar 1 2021, 5:00 PM
clang/lib/CodeGen/CoverageMappingGen.cpp
870

I think it is feasible. For break and continue, they only affect the statements after them in the same block. For other terminate statements, they affect all the statements after them even outside the block, see example below. Also instead of emitting a zero gap region in VisitStmt, we need emit gap region with (previous counter - current statement counter).

while (cond) {
...
break; // This affects statements' count until the end of the while body.
...
}

while (cond) {
...
return; // This affects statements' count until the end of the function body.
...
}
vsk added a comment.Mar 2 2021, 4:32 PM

Thanks for the patient explanation.

clang/lib/CodeGen/CoverageMappingGen.cpp
870

Thanks, that's really helpful. It's clicking now that the main difference is that the deferred region sticks around after we're done visiting a block.

1311

Is the reason why we don't need to complete it because the logic in VisitStmt handles filling in the gap region? What happens if this isn't cleared, or if there are nested switches?

clang/test/CoverageMapping/classtemplate.cpp
83

Nice, this is from the special handling of noreturn calls.

clang/test/CoverageMapping/deferred-region.cpp
0

Nice, this should amount to the same end result, it's a smaller encoding.

0

I'm confused by this change. Do we lose the gap here? I assumed it was needed to prevent the condition count from if (true) from kicking in after the return?

clang/test/CoverageMapping/switch.cpp
62

The blank space after default: break; and before the closing '}' should have the same count as the switch condition, I thought (this goes for all of the unreachable code within a switch body)? The idea is to prevent 'not-executed' regions from appearing after the break stmt.

zequanwu updated this revision to Diff 327661.Mar 2 2021, 8:39 PM
zequanwu marked an inline comment as done.
  • Deprecate deferred region. Use the notion of gap region inserted in VisitStmt to replace deferred region.
  • Emit gap region with OutCounter from last statement if exists.
  • Move test cases from deferred-region.cpp to terminate-statements.cpp
clang/test/CoverageMapping/deferred-region.cpp
0

Fixed.

clang/test/CoverageMapping/switch.cpp
62

Yes, they are unreachable. So, shouldn't they always have zero count instead of same count as switch condition?

zequanwu retitled this revision from [Coverage] Emit zero count gap region between statements if first statements contains return, throw, goto, co_return to [Coverage] Emit gap region between statements if first statements contains terminate statements..Mar 2 2021, 8:54 PM
vsk accepted this revision.Mar 3 2021, 9:43 AM

Thanks, this looks great.

clang/test/CoverageMapping/switch.cpp
62

I see, you're right. We use a gap region with zero count to cover the area after break. That's actually documented here: https://clang.llvm.org/docs/SourceBasedCodeCoverage.html#switch-statements. One interesting point is that there's only one gap region that covers the whole body; in this example, that would be Gap,File 0, [[@LINE]]:13 -> [[@LINE+8]]:10 = 0. So, perhaps the new gaps emitted in switches are redundant, but getting rid of them is just a performance optimization.

This revision is now accepted and ready to land.Mar 3 2021, 9:43 AM
zequanwu updated this revision to Diff 327859.Mar 3 2021, 11:14 AM

use itanium abi in terminate-statements.cpp

Thanks for reviewing.

clang/test/CoverageMapping/switch.cpp
62

Yes, they are redundant.

This revision was landed with ongoing or failed builds.Mar 3 2021, 11:26 AM
This revision was automatically updated to reflect the committed changes.

This broke ContinuousSyncMode/basic.c in check-profile on macOS (see e.g. end of https://logs.chromium.org/logs/chromium/buildbucket/cr-buildbucket.appspot.com/8853690069583264896/+/steps/package_clang/0/stdout?format=raw), so I reverted it for now.

It repros on my laptop, so let me know if you want me to try things :)

This broke ContinuousSyncMode/basic.c in check-profile on macOS (see e.g. end of https://logs.chromium.org/logs/chromium/buildbucket/cr-buildbucket.appspot.com/8853690069583264896/+/steps/package_clang/0/stdout?format=raw), so I reverted it for now.

It repros on my laptop, so let me know if you want me to try things :)

Relanded with update on the broken test case.