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

[Zequan Wu via Phabricator via cfe-commits]

zequanwu added inline comments.



Comment at: clang/test/CoverageMapping/deferred-region.cpp:43
   if (true)
-return; // CHECK: Gap,File 0, [[@LINE]]:11
+return;
   else

vsk wrote:
> 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`?
Fixed.



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)
+  }

vsk wrote:
> 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.
Yes, they are unreachable. So, shouldn't they always have zero count instead of 
same count as switch condition?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97101

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Zequan Wu via Phabricator via cfe-commits]

zequanwu updated this revision to Diff 327661.
zequanwu marked an inline comment as done.
zequanwu added a comment.

- 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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97101

Files:
  clang/lib/CodeGen/CoverageMappingGen.cpp
  clang/test/CoverageMapping/break.c
  clang/test/CoverageMapping/classtemplate.cpp
  clang/test/CoverageMapping/continue.c
  clang/test/CoverageMapping/coroutine.cpp
  clang/test/CoverageMapping/deferred-region.cpp
  clang/test/CoverageMapping/label.cpp
  clang/test/CoverageMapping/return.c
  clang/test/CoverageMapping/switch.cpp
  clang/test/CoverageMapping/switchmacro.c
  clang/test/CoverageMapping/terminate-statements.cpp
  clang/test/CoverageMapping/trycatch.cpp
  clang/test/CoverageMapping/unreachable-macro.c
  compiler-rt/test/profile/ContinuousSyncMode/runtime-counter-relocation.c

Index: compiler-rt/test/profile/ContinuousSyncMode/runtime-counter-relocation.c
===
--- compiler-rt/test/profile/ContinuousSyncMode/runtime-counter-relocation.c
+++ compiler-rt/test/profile/ContinuousSyncMode/runtime-counter-relocation.c
@@ -21,9 +21,9 @@
 
 // CHECK-COVERAGE: Filename RegionsMissed Regions Cover   Functions  Missed Functions  Executed   Lines  Missed Lines Cover
 // CHECK-COVERAGE-NEXT: ---
-// CHECK-COVERAGE-NEXT: runtime-counter-relocation.c  4 175.00%   1 0   100.00%   5 260.00%
+// CHECK-COVERAGE-NEXT: runtime-counter-relocation.c  4 175.00%   1 0   100.00%   5 180.00%
 // CHECK-COVERAGE-NEXT: ---
-// CHECK-COVERAGE-NEXT: TOTAL 4 175.00%   1 0   100.00%   5 260.00%
+// CHECK-COVERAGE-NEXT: TOTAL 4 175.00%   1 0   100.00%   5 180.00%
 
 extern int __llvm_profile_is_continuous_mode_enabled(void);
 
Index: clang/test/CoverageMapping/unreachable-macro.c
===
--- clang/test/CoverageMapping/unreachable-macro.c
+++ clang/test/CoverageMapping/unreachable-macro.c
@@ -6,6 +6,7 @@
 void counters_in_macro_following_unreachable() {
   // CHECK-NEXT: File 0, [[@LINE-1]]:48 -> {{[0-9]+}}:2 = #0
   return;
+  // CHECK-NEXT: Gap,File 0, [[@LINE-1]]:10 -> [[@LINE+3]]:3 = 0
   // CHECK-NEXT: Expansion,File 0, [[@LINE+2]]:3 -> [[@LINE+2]]:8 = 0
   // CHECK-NEXT: File 0, [[@LINE+1]]:8 -> [[@LINE+2]]:2 = 0
   WHILE
Index: clang/test/CoverageMapping/trycatch.cpp
===
--- clang/test/CoverageMapping/trycatch.cpp
+++ clang/test/CoverageMapping/trycatch.cpp
@@ -13,7 +13,7 @@
 void func(int i) {// CHECK-NEXT: File 0, [[@LINE]]:18 -> {{[0-9]+}}:2 = #0
   // CHECK-NEXT: File 0, [[@LINE+1]]:6 -> [[@LINE+1]]:11 = #0
   if(i % 2) { // CHECK: File 0, [[@LINE]]:13 -> [[@LINE+4]]:4 = #1
-throw Error();
+throw Error();// CHECK-NEXT: Gap,File 0, [[@LINE]]:19 -> [[@LINE+1]]:5 = 0
 int j = 0;// CHECK-NEXT: File 0, [[@LINE]]:5 -> [[@LINE+2]]:4 = 0
   // CHECK: File 0, [[@LINE+1]]:10 -> [[@LINE+2]]:27 = (#0 - #1)
   } else if(i == 8)   // CHECK-NEXT: File 0, [[@LINE]]:13 -> [[@LINE]]:19 = (#0 - #1)
Index: clang/test/CoverageMapping/terminate-statements.cpp
===
--- /dev/null
+++ clang/test/CoverageMapping/terminate-statements.cpp
@@ -0,0 +1,343 @@
+// RUN: %clang_cc1 -mllvm -emptyline-comment-coverage=false -fprofile-instrument=clang -fcoverage-mapping -dump-coverage-mapping -fexceptions -fcxx-exceptions -emit-llvm-only -main-file-name deferred-region.cpp -I %S/Inputs %s | FileCheck %s
+
+int f1() {
+  return 0;
+  return 0; // CHECK: Gap,File 0, [[@LINE-1]]:12 -> [[@LINE]]:3 = 0
+}
+
+int f2(int i) {
+  if (i)
+return 0;
+  else
+;   // CHECK: Gap,File 0, [[@LINE]]:6 -> [[@LINE+1]]:3 = (#0 - #1)
+  return 1; // CHECK-NEXT: File 0, [[@LINE]]:3 -> [[@LINE]]:11 = (#0 - #1)
+}
+
+int f3() {
+  for (int a = 1; a < 9; a--)
+return a; // CHECK: Gap,File 0, [[@LINE]]:14 -> [[@LINE+1]]:3 = (#0 - #1)
+  return 0;   // CHECK-NEXT: File 0, [[@LINE]]:3 -> [[@LINE]]:11 = (#0 - #1)
+}
+
+int f4(int i) {
+  while (i > 0) {
+i++;
+return i;
+  } // CHECK: File 

[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  = getRegion();
-ZeroRegion.setDeferred(true);
-LastTerminatedRegion = {EndLoc, RegionStack.size()};
+if (!HasTerminateStmt) {
+  auto  = 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 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{{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

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Zequan Wu via Phabricator via cfe-commits]

zequanwu added inline comments.



Comment at: clang/lib/CodeGen/CoverageMappingGen.cpp:942
 pushRegion(Counter::getZero());
-auto  = getRegion();
-ZeroRegion.setDeferred(true);
-LastTerminatedRegion = {EndLoc, RegionStack.size()};
+if (!HasTerminateStmt) {
+  auto  = getRegion();

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



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97101

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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 inline comments.



Comment at: clang/lib/CodeGen/CoverageMappingGen.cpp:942
 pushRegion(Counter::getZero());
-auto  = getRegion();
-ZeroRegion.setDeferred(true);
-LastTerminatedRegion = {EndLoc, RegionStack.size()};
+if (!HasTerminateStmt) {
+  auto  = getRegion();

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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97101

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Zequan Wu via Phabricator via cfe-commits]

zequanwu added inline comments.



Comment at: clang/lib/CodeGen/CoverageMappingGen.cpp:593
   MostRecentLocation = *StartLoc;
-  completeDeferred(Count, MostRecentLocation);
 }
 

vsk wrote:
> Does this mean a deferred gap region following a `break;` statement won't be 
> completed before the next region is pushed?
I reverted this change.



Comment at: clang/lib/CodeGen/CoverageMappingGen.cpp:942
 pushRegion(Counter::getZero());
-auto  = getRegion();
-ZeroRegion.setDeferred(true);
-LastTerminatedRegion = {EndLoc, RegionStack.size()};
+if (!HasTerminateStmt) {
+  auto  = getRegion();

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.



Comment at: clang/lib/CodeGen/CoverageMappingGen.cpp:1057
+if (SaveTerminateStmt)
+  HasTerminateStmt = true;
 handleFileExit(getEnd(S));

vsk wrote:
> Can this replace all of the machinery around DeferredRegions? It'd be nice to 
> just have one way to set up gaps between statements.
See above comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97101

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Zequan Wu via Phabricator via cfe-commits]

zequanwu updated this revision to Diff 325947.
zequanwu marked an inline comment as done.
zequanwu added a comment.

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97101

Files:
  clang/lib/CodeGen/CoverageMappingGen.cpp
  clang/test/CoverageMapping/classtemplate.cpp
  clang/test/CoverageMapping/coroutine.cpp
  clang/test/CoverageMapping/deferred-region.cpp
  clang/test/CoverageMapping/label.cpp
  clang/test/CoverageMapping/moremacros.c
  clang/test/CoverageMapping/return.c
  clang/test/CoverageMapping/switch.cpp
  clang/test/CoverageMapping/switchmacro.c
  clang/test/CoverageMapping/terminate-statements.cpp
  clang/test/CoverageMapping/trycatch.cpp
  clang/test/CoverageMapping/unreachable-macro.c
  compiler-rt/test/profile/ContinuousSyncMode/runtime-counter-relocation.c

Index: compiler-rt/test/profile/ContinuousSyncMode/runtime-counter-relocation.c
===
--- compiler-rt/test/profile/ContinuousSyncMode/runtime-counter-relocation.c
+++ compiler-rt/test/profile/ContinuousSyncMode/runtime-counter-relocation.c
@@ -21,9 +21,9 @@
 
 // CHECK-COVERAGE: Filename RegionsMissed Regions Cover   Functions  Missed Functions  Executed   Lines  Missed Lines Cover
 // CHECK-COVERAGE-NEXT: ---
-// CHECK-COVERAGE-NEXT: runtime-counter-relocation.c  4 175.00%   1 0   100.00%   5 260.00%
+// CHECK-COVERAGE-NEXT: runtime-counter-relocation.c  4 175.00%   1 0   100.00%   5 180.00%
 // CHECK-COVERAGE-NEXT: ---
-// CHECK-COVERAGE-NEXT: TOTAL 4 175.00%   1 0   100.00%   5 260.00%
+// CHECK-COVERAGE-NEXT: TOTAL 4 175.00%   1 0   100.00%   5 180.00%
 
 extern int __llvm_profile_is_continuous_mode_enabled(void);
 
Index: clang/test/CoverageMapping/unreachable-macro.c
===
--- clang/test/CoverageMapping/unreachable-macro.c
+++ clang/test/CoverageMapping/unreachable-macro.c
@@ -6,6 +6,7 @@
 void counters_in_macro_following_unreachable() {
   // CHECK-NEXT: File 0, [[@LINE-1]]:48 -> {{[0-9]+}}:2 = #0
   return;
+  // CHECK-NEXT: Gap,File 0, [[@LINE-1]]:10 -> [[@LINE+3]]:3 = 0
   // CHECK-NEXT: Expansion,File 0, [[@LINE+2]]:3 -> [[@LINE+2]]:8 = 0
   // CHECK-NEXT: File 0, [[@LINE+1]]:8 -> [[@LINE+2]]:2 = 0
   WHILE
Index: clang/test/CoverageMapping/trycatch.cpp
===
--- clang/test/CoverageMapping/trycatch.cpp
+++ clang/test/CoverageMapping/trycatch.cpp
@@ -13,7 +13,7 @@
 void func(int i) {// CHECK-NEXT: File 0, [[@LINE]]:18 -> {{[0-9]+}}:2 = #0
   // CHECK-NEXT: File 0, [[@LINE+1]]:6 -> [[@LINE+1]]:11 = #0
   if(i % 2) { // CHECK: File 0, [[@LINE]]:13 -> [[@LINE+4]]:4 = #1
-throw Error();
+throw Error();// CHECK-NEXT: Gap,File 0, [[@LINE]]:19 -> [[@LINE+1]]:5 = 0
 int j = 0;// CHECK-NEXT: File 0, [[@LINE]]:5 -> [[@LINE+2]]:4 = 0
   // CHECK: File 0, [[@LINE+1]]:10 -> [[@LINE+2]]:27 = (#0 - #1)
   } else if(i == 8)   // CHECK-NEXT: File 0, [[@LINE]]:13 -> [[@LINE]]:19 = (#0 - #1)
Index: clang/test/CoverageMapping/terminate-statements.cpp
===
--- /dev/null
+++ clang/test/CoverageMapping/terminate-statements.cpp
@@ -0,0 +1,138 @@
+// RUN: %clang_cc1 -mllvm -emptyline-comment-coverage=false -fprofile-instrument=clang -fcoverage-mapping -dump-coverage-mapping -emit-llvm-only -main-file-name deferred-region.cpp -I %S/Inputs %s | FileCheck %s
+
+int f1() {
+  return 0;
+  return 0; // CHECK: Gap,File 0, [[@LINE-1]]:12 -> [[@LINE]]:3 = 0
+}
+
+int f2(int i) {
+  if (i)
+return 0;
+  else
+;
+  return 1; // CHECK: Gap,File 0, [[@LINE-1]]:6 -> [[@LINE]]:3 = 0
+}
+
+int f3() {
+  for (int a = 1; a < 9; a--)
+return a;
+  return 0; // CHECK: Gap,File 0, [[@LINE-1]]:14 -> [[@LINE]]:3 = 0
+}
+
+int f4(int i) {
+  while (i > 0) {
+i++;
+return i;
+  }
+  return 0; // CHECK: Gap,File 0, [[@LINE-1]]:4 -> [[@LINE]]:3 = 0
+}
+
+int f5(int i) {
+  do {
+return i;
+  } while (i > 0);
+  return 0; // CHECK: Gap,File 0, [[@LINE-1]]:19 -> [[@LINE]]:3 = 0
+}
+
+int f6() {
+  int arr[] = {1, 2, 3, 4};
+  for (int i : arr) {
+return i;
+  }
+  return 0; // 

[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 inline comments.



Comment at: clang/lib/CodeGen/CoverageMappingGen.cpp:559
 
-  /// Location of the last terminated region.
-  Optional> LastTerminatedRegion;
+  /// If there is a return, co_return, goto or throw inside.
+  bool HasTerminateStmt = false;

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



Comment at: clang/lib/CodeGen/CoverageMappingGen.cpp:593
   MostRecentLocation = *StartLoc;
-  completeDeferred(Count, MostRecentLocation);
 }
 

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



Comment at: clang/lib/CodeGen/CoverageMappingGen.cpp:942
 pushRegion(Counter::getZero());
-auto  = getRegion();
-ZeroRegion.setDeferred(true);
-LastTerminatedRegion = {EndLoc, RegionStack.size()};
+if (!HasTerminateStmt) {
+  auto  = getRegion();

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



Comment at: clang/lib/CodeGen/CoverageMappingGen.cpp:1057
+if (SaveTerminateStmt)
+  HasTerminateStmt = true;
 handleFileExit(getEnd(S));

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97101

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Zequan Wu via Phabricator via cfe-commits]

zequanwu updated this revision to Diff 325516.
zequanwu added a comment.
Herald added a project: Sanitizers.
Herald added a subscriber: Sanitizers.

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97101

Files:
  clang/lib/CodeGen/CoverageMappingGen.cpp
  clang/test/CoverageMapping/classtemplate.cpp
  clang/test/CoverageMapping/coroutine.cpp
  clang/test/CoverageMapping/deferred-region.cpp
  clang/test/CoverageMapping/label.cpp
  clang/test/CoverageMapping/moremacros.c
  clang/test/CoverageMapping/return.c
  clang/test/CoverageMapping/switch.cpp
  clang/test/CoverageMapping/switchmacro.c
  clang/test/CoverageMapping/terminate-statements.cpp
  clang/test/CoverageMapping/trycatch.cpp
  clang/test/CoverageMapping/unreachable-macro.c
  compiler-rt/test/profile/ContinuousSyncMode/runtime-counter-relocation.c

Index: compiler-rt/test/profile/ContinuousSyncMode/runtime-counter-relocation.c
===
--- compiler-rt/test/profile/ContinuousSyncMode/runtime-counter-relocation.c
+++ compiler-rt/test/profile/ContinuousSyncMode/runtime-counter-relocation.c
@@ -21,9 +21,9 @@
 
 // CHECK-COVERAGE: Filename RegionsMissed Regions Cover   Functions  Missed Functions  Executed   Lines  Missed Lines Cover
 // CHECK-COVERAGE-NEXT: ---
-// CHECK-COVERAGE-NEXT: runtime-counter-relocation.c  4 175.00%   1 0   100.00%   5 260.00%
+// CHECK-COVERAGE-NEXT: runtime-counter-relocation.c  4 175.00%   1 0   100.00%   5 180.00%
 // CHECK-COVERAGE-NEXT: ---
-// CHECK-COVERAGE-NEXT: TOTAL 4 175.00%   1 0   100.00%   5 260.00%
+// CHECK-COVERAGE-NEXT: TOTAL 4 175.00%   1 0   100.00%   5 180.00%
 
 extern int __llvm_profile_is_continuous_mode_enabled(void);
 
Index: clang/test/CoverageMapping/unreachable-macro.c
===
--- clang/test/CoverageMapping/unreachable-macro.c
+++ clang/test/CoverageMapping/unreachable-macro.c
@@ -6,6 +6,7 @@
 void counters_in_macro_following_unreachable() {
   // CHECK-NEXT: File 0, [[@LINE-1]]:48 -> {{[0-9]+}}:2 = #0
   return;
+  // CHECK-NEXT: Gap,File 0, [[@LINE-1]]:10 -> [[@LINE+3]]:3 = 0
   // CHECK-NEXT: Expansion,File 0, [[@LINE+2]]:3 -> [[@LINE+2]]:8 = 0
   // CHECK-NEXT: File 0, [[@LINE+1]]:8 -> [[@LINE+2]]:2 = 0
   WHILE
Index: clang/test/CoverageMapping/trycatch.cpp
===
--- clang/test/CoverageMapping/trycatch.cpp
+++ clang/test/CoverageMapping/trycatch.cpp
@@ -13,7 +13,7 @@
 void func(int i) {// CHECK-NEXT: File 0, [[@LINE]]:18 -> {{[0-9]+}}:2 = #0
   // CHECK-NEXT: File 0, [[@LINE+1]]:6 -> [[@LINE+1]]:11 = #0
   if(i % 2) { // CHECK: File 0, [[@LINE]]:13 -> [[@LINE+4]]:4 = #1
-throw Error();
+throw Error();// CHECK-NEXT: Gap,File 0, [[@LINE]]:19 -> [[@LINE+1]]:5 = 0
 int j = 0;// CHECK-NEXT: File 0, [[@LINE]]:5 -> [[@LINE+2]]:4 = 0
   // CHECK: File 0, [[@LINE+1]]:10 -> [[@LINE+2]]:27 = (#0 - #1)
   } else if(i == 8)   // CHECK-NEXT: File 0, [[@LINE]]:13 -> [[@LINE]]:19 = (#0 - #1)
Index: clang/test/CoverageMapping/terminate-statements.cpp
===
--- /dev/null
+++ clang/test/CoverageMapping/terminate-statements.cpp
@@ -0,0 +1,130 @@
+// RUN: %clang_cc1 -mllvm -emptyline-comment-coverage=false -fprofile-instrument=clang -fcoverage-mapping -dump-coverage-mapping -emit-llvm-only -main-file-name deferred-region.cpp -I %S/Inputs %s | FileCheck %s
+
+int f1() {
+  return 0;
+  return 0; // CHECK: Gap,File 0, [[@LINE-1]]:12 -> [[@LINE]]:3 = 0
+}
+
+int f2(int i) {
+  if (i)
+return 0;
+  else
+;
+  return 1; // CHECK: Gap,File 0, [[@LINE-1]]:6 -> [[@LINE]]:3 = 0
+}
+
+int f3() {
+  for (int a = 1; a < 9; a--)
+return a;
+  return 0; // CHECK: Gap,File 0, [[@LINE-1]]:14 -> [[@LINE]]:3 = 0
+}
+
+int f4(int i) {
+  while (i > 0) {
+i++;
+return i;
+  }
+  return 0; // CHECK: Gap,File 0, [[@LINE-1]]:4 -> [[@LINE]]:3 = 0
+}
+
+int f5(int i) {
+  do {
+return i;
+  } while (i > 0);
+  return 0; // CHECK: Gap,File 0, 

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

[Reid Kleckner via Phabricator via cfe-commits]

rnk added a comment.

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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97101

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Zequan Wu via Phabricator via cfe-commits]

zequanwu created this revision.
zequanwu added reviewers: vsk, efriedma, rnk, pirama.
Herald added a subscriber: lxfind.
zequanwu requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

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.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D97101

Files:
  clang/lib/CodeGen/CoverageMappingGen.cpp
  clang/test/CoverageMapping/classtemplate.cpp
  clang/test/CoverageMapping/coroutine.cpp
  clang/test/CoverageMapping/deferred-region.cpp
  clang/test/CoverageMapping/label.cpp
  clang/test/CoverageMapping/moremacros.c
  clang/test/CoverageMapping/return.c
  clang/test/CoverageMapping/switch.cpp
  clang/test/CoverageMapping/switchmacro.c
  clang/test/CoverageMapping/terminate-statements.cpp
  clang/test/CoverageMapping/trycatch.cpp
  clang/test/CoverageMapping/unreachable-macro.c

Index: clang/test/CoverageMapping/unreachable-macro.c
===
--- clang/test/CoverageMapping/unreachable-macro.c
+++ clang/test/CoverageMapping/unreachable-macro.c
@@ -6,6 +6,7 @@
 void counters_in_macro_following_unreachable() {
   // CHECK-NEXT: File 0, [[@LINE-1]]:48 -> {{[0-9]+}}:2 = #0
   return;
+  // CHECK-NEXT: Gap,File 0, [[@LINE-1]]:10 -> [[@LINE+3]]:3 = 0
   // CHECK-NEXT: Expansion,File 0, [[@LINE+2]]:3 -> [[@LINE+2]]:8 = 0
   // CHECK-NEXT: File 0, [[@LINE+1]]:8 -> [[@LINE+2]]:2 = 0
   WHILE
Index: clang/test/CoverageMapping/trycatch.cpp
===
--- clang/test/CoverageMapping/trycatch.cpp
+++ clang/test/CoverageMapping/trycatch.cpp
@@ -13,7 +13,7 @@
 void func(int i) {// CHECK-NEXT: File 0, [[@LINE]]:18 -> {{[0-9]+}}:2 = #0
   // CHECK-NEXT: File 0, [[@LINE+1]]:6 -> [[@LINE+1]]:11 = #0
   if(i % 2) { // CHECK: File 0, [[@LINE]]:13 -> [[@LINE+4]]:4 = #1
-throw Error();
+throw Error();// CHECK-NEXT: Gap,File 0, [[@LINE]]:19 -> [[@LINE+1]]:5 = 0
 int j = 0;// CHECK-NEXT: File 0, [[@LINE]]:5 -> [[@LINE+2]]:4 = 0
   // CHECK: File 0, [[@LINE+1]]:10 -> [[@LINE+2]]:27 = (#0 - #1)
   } else if(i == 8)   // CHECK-NEXT: File 0, [[@LINE]]:13 -> [[@LINE]]:19 = (#0 - #1)
Index: clang/test/CoverageMapping/terminate-statements.cpp
===
--- /dev/null
+++ clang/test/CoverageMapping/terminate-statements.cpp
@@ -0,0 +1,130 @@
+// RUN: %clang_cc1 -mllvm -emptyline-comment-coverage=false -fprofile-instrument=clang -fcoverage-mapping -dump-coverage-mapping -emit-llvm-only -main-file-name deferred-region.cpp -I %S/Inputs %s | FileCheck %s
+
+int f1() {
+  return 0;
+  return 0; // CHECK: Gap,File 0, [[@LINE-1]]:12 -> [[@LINE]]:3 = 0
+}
+
+int f2(int i) {
+  if (i)
+return 0;
+  else
+;
+  return 1; // CHECK: Gap,File 0, [[@LINE-1]]:6 -> [[@LINE]]:3 = 0
+}
+
+int f3() {
+  for (int a = 1; a < 9; a--)
+return a;
+  return 0; // CHECK: Gap,File 0, [[@LINE-1]]:14 -> [[@LINE]]:3 = 0
+}
+
+int f4(int i) {
+  while (i > 0) {
+i++;
+return i;
+  }
+  return 0; // CHECK: Gap,File 0, [[@LINE-1]]:4 -> [[@LINE]]:3 = 0
+}
+
+int f5(int i) {
+  do {
+return i;
+  } while (i > 0);
+  return 0; // CHECK: Gap,File 0, [[@LINE-1]]:19 -> [[@LINE]]:3 = 0
+}
+
+int f6() {
+  int arr[] = {1, 2, 3, 4};
+  for (int i : arr) {
+return i;
+  }
+  return 0; // CHECK: Gap,File 0, [[@LINE-1]]:4 -> [[@LINE]]:3 = 0
+}
+
+int f7() {
+  {
+{
+  return 0;
+}
+return 0; // CHECK: Gap,File 0, [[@LINE-1]]:6 -> [[@LINE]]:5 = 0
+  }
+  return 0; // CHECK: Gap,File 0, [[@LINE-1]]:4 -> [[@LINE]]:3 = 0
+}
+
+int f8(int i) {
+  if (i == 1)
+return 1;
+  if (i == 2) // CHECK: Gap,File 0, [[@LINE-1]]:14 -> [[@LINE]]:3 = 0
+return 2;
+  if (i == 3) // CHECK: Gap,File 0, [[@LINE-1]]:14 -> [[@LINE]]:3 = 0
+return 3;
+  return 4; // CHECK: Gap,File 0, [[@LINE-1]]:14 -> [[@LINE]]:3 = 0
+}
+
+int f9(int i) {
+  if (i == 1)
+return 1;
+  else if (i == 2)
+return 2;
+  return 3; // CHECK: Gap,File 0, [[@LINE-1]]:14 -> [[@LINE]]:3 = 0
+}
+
+int f10(int i) {
+  if (i == 1) {
+return 0;
+if (i == 2) // CHECK: Gap,File 0, [[@LINE-1]]:14 -> [[@LINE]]:5 = 0
+  return 0;
+  }
+  return 0; // CHECK: Gap,File 0, [[@LINE-1]]:4 -> [[@LINE]]:3 = 0
+}
+
+int f11(int i) {
+  if (i == 1)
+i = 2;
+  else
+return 0;
+  return 0; // CHECK: Gap,File 0, [[@LINE-1]]:14 -> [[@LINE]]:3 = 0
+}
+
+int f12(int i) {
+  int x = 1;
+  if (x == 1) {
+if (x == 1) {
+  return 0;
+}
+  } else if (x == 2) {
+x