[PATCH] D97101: [Coverage] Emit zero count gap region between statements if first statements contains return, throw, goto, co_return

[Vedant Kumar via Phabricator via cfe-commits]

vsk added a comment.

Thanks for the patient explanation.



================
Comment at: clang/lib/CodeGen/CoverageMappingGen.cpp:1391
+    // Clear DeferredRegion because we don't need to complete it after switch.
+    DeferredRegion = None;
+
----------------
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?


================
Comment at: clang/lib/CodeGen/CoverageMappingGen.cpp:942
     pushRegion(Counter::getZero());
-    auto &ZeroRegion = getRegion();
-    ZeroRegion.setDeferred(true);
-    LastTerminatedRegion = {EndLoc, RegionStack.size()};
+    if (!HasTerminateStmt) {
+      auto &ZeroRegion = getRegion();
----------------
zequanwu wrote:
> vsk wrote:
> > zequanwu wrote:
> > > vsk wrote:
> > > > What's supposed to be the difference between the zero region pushed after a `return;` vs. after a `break;`?
> > > 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.
> > 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?
> 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, 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.


================
Comment at: clang/test/CoverageMapping/classtemplate.cpp:83
 std::map<int, int> Test4() { // CHECK-INIT-LIST: File 0, [[@LINE]]:28 -> [[@LINE+3]]:2 = #0
-  abort();
+  abort();                           // CHECK-INIT-LIST-NEXT: Gap,File 0, [[@LINE]]:11 -> [[@LINE+1]]:3 = 0
   return std::map<int, int>{{0, 0}}; // CHECK-INIT-LIST-NEXT: [[@LINE]]:3 -> [[@LINE]]:36 = 0
----------------
Nice, this is from the special handling of noreturn calls.


================
Comment at: clang/test/CoverageMapping/deferred-region.cpp:19
     return;
-  } // CHECK: Gap,File 0, [[@LINE]]:4 -> [[@LINE+2]]:3 = (#0 - #1)
+  } // CHECK: Gap,File 0, [[@LINE]]:4 -> [[@LINE+2]]:3 = 0
 
----------------
Nice, this should amount to the same end result, it's a smaller encoding.


================
Comment at: clang/test/CoverageMapping/deferred-region.cpp:43
   if (true)
-    return; // CHECK: Gap,File 0, [[@LINE]]:11
+    return;
   else
----------------
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`?


================
Comment at: clang/test/CoverageMapping/switch.cpp:62
   default:          // CHECK-NEXT: File 0, [[@LINE]]:3 -> [[@LINE+1]]:10 = #4
     break;          // CHECK-NEXT: Branch,File 0, [[@LINE-1]]:3 -> [[@LINE-1]]:10 = #4, (#0 - #4)
+  }
----------------
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D97101/new/

https://reviews.llvm.org/D97101