Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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?
| 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. | |
- 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.
| 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. | |
| 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? | |
| 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.
...
} | |
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. | |
- 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? | |
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. | |
Thanks for reviewing.
| clang/test/CoverageMapping/switch.cpp | ||
|---|---|---|
| 62 | Yes, they are redundant. | |
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 :)
nit: Whether the visitor is at a ... stmt?