[PATCH] D35925: [Coverage] Precise region termination with deferred regions
vsk added a comment. Reverted in r310154, because llvm-cov isn't doing a good job of displaying the new regions properly. I'll re-land this after llvm-cov is fixed. https://reviews.llvm.org/D35925 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D35925: [Coverage] Precise region termination with deferred regions
vsk closed this revision. vsk added a comment. Committed in r310010 https://reviews.llvm.org/D35925 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D35925: [Coverage] Precise region termination with deferred regions
vsk updated this revision to Diff 109142.
vsk marked 2 inline comments as done.
vsk edited the summary of this revision.
vsk added a comment.
Thanks for the review! I'll hold off on landing this until the llvm-cov changes
are in, so that people don't hit the issue where unexpected line execution
counts are displayed.
https://reviews.llvm.org/D35925
Files:
lib/CodeGen/CoverageMappingGen.cpp
test/CoverageMapping/deferred-region.cpp
test/CoverageMapping/label.cpp
test/CoverageMapping/moremacros.c
test/CoverageMapping/return.c
test/CoverageMapping/switch.cpp
test/CoverageMapping/switchmacro.c
test/CoverageMapping/trycatch.cpp
Index: test/CoverageMapping/trycatch.cpp
===
--- test/CoverageMapping/trycatch.cpp
+++ test/CoverageMapping/trycatch.cpp
@@ -18,7 +18,7 @@
// CHECK-NEXT: File 0, [[@LINE+1]]:10 -> [[@LINE+2]]:27 = (#0 - #1)
} else if(i == 8) // CHECK-NEXT: File 0, [[@LINE]]:13 -> [[@LINE]]:19 = (#0 - #1)
throw ImportantError(); // CHECK-NEXT: File 0, [[@LINE]]:5 -> [[@LINE]]:27 = #2
-}
+} // CHECK-NEXT: File 0, [[@LINE-1]]:27 -> [[@LINE]]:2 = ((#0 - #1) - #2)
// CHECK-NEXT: main
int main() { // CHECK-NEXT: File 0, [[@LINE]]:12 -> [[@LINE+13]]:2 = #0
Index: test/CoverageMapping/switchmacro.c
===
--- test/CoverageMapping/switchmacro.c
+++ test/CoverageMapping/switchmacro.c
@@ -8,6 +8,7 @@
default: // CHECK-NEXT: File 0, [[@LINE]]:3 -> {{[0-9]+}}:11 = #2
if (i == 1) // CHECK-NEXT: File 0, [[@LINE]]:9 -> [[@LINE]]:15 = #2
return 0; // CHECK-NEXT: File 0, [[@LINE]]:7 -> [[@LINE]]:15 = #3
+ // CHECK-NEXT: File 0, [[@LINE-1]]:15 -> [[@LINE+3]]:5 = (#2 - #3)
// CHECK-NEXT: Expansion,File 0, [[@LINE+2]]:5 -> [[@LINE+2]]:8 = (#2 - #3) (Expanded file = 1)
// CHECK-NEXT: File 0, [[@LINE+1]]:8 -> {{[0-9]+}}:11 = (#2 - #3)
FOO(1);
Index: test/CoverageMapping/switch.cpp
===
--- test/CoverageMapping/switch.cpp
+++ test/CoverageMapping/switch.cpp
@@ -6,7 +6,7 @@
case 1: // CHECK-NEXT: File 0, [[@LINE]]:3 -> [[@LINE+3]]:10 = #2
return;
case 2: // CHECK-NEXT: File 0, [[@LINE]]:3 -> [[@LINE+1]]:10 = #3
-break;
+break; // CHECK-NEXT: File 0, [[@LINE]]:10 -> [[@LINE+2]]:3 = #1
}
int x = 0;// CHECK-NEXT: File 0, [[@LINE]]:3 -> [[@LINE+1]]:2 = #1
}
@@ -55,16 +55,16 @@
i = 2;
break;
default: // CHECK-NEXT: File 0, [[@LINE]]:3 -> [[@LINE+1]]:10 = #4
-break;
+break; // CHECK-NEXT: File 0, [[@LINE]]:10 -> [[@LINE+2]]:3 = #1
}
switch(i) { // CHECK-NEXT: File 0, [[@LINE]]:3 -> [[@LINE+23]]:2 = #1
case 0: // CHECK-NEXT: File 0, [[@LINE]]:3 -> [[@LINE+6]]:10 = #6
i = 1;
break;
case 1: // CHECK-NEXT: File 0, [[@LINE]]:3 -> [[@LINE+3]]:10 = #7
i = 2;
default: // CHECK-NEXT: File 0, [[@LINE]]:3 -> [[@LINE+1]]:10 = (#7 + #8)
-break;
+break; // CHECK-NEXT: File 0, [[@LINE]]:10 -> [[@LINE+3]]:3 = #5
}
switch(i) { // CHECK-NEXT: File 0, [[@LINE]]:3 -> [[@LINE+13]]:2 = #5
Index: test/CoverageMapping/return.c
===
--- test/CoverageMapping/return.c
+++ test/CoverageMapping/return.c
@@ -13,7 +13,7 @@
for(int i = 0; i < 10; ++i) { // CHECK-NEXT: File 0, [[@LINE]]:31 -> {{[0-9]+}}:4 = #1
// CHECK-NEXT: File 0, [[@LINE+1]]:8 -> [[@LINE+1]]:13 = #1
if(i > 2) { // CHECK-NEXT: File 0, [[@LINE]]:15 -> [[@LINE+2]]:6 = #2
- return;
+ return; // CHECK-NEXT: File 0, [[@LINE+1]]:6 -> [[@LINE+3]]:5 = (#1 - #2)
} // CHECK-NEXT: File 0, [[@LINE+2]]:5 -> {{[0-9]+}}:4 = (#1 - #2)
// CHECK-NEXT: File 0, [[@LINE+1]]:8 -> [[@LINE+1]]:14 = (#1 - #2)
if(i == 3) {// CHECK-NEXT: File 0, [[@LINE]]:16 -> [[@LINE+2]]:6 = #3
Index: test/CoverageMapping/moremacros.c
===
--- test/CoverageMapping/moremacros.c
+++ test/CoverageMapping/moremacros.c
@@ -15,23 +15,23 @@
if (!argc) LBRAC
return 0;
// CHECK-NEXT: Expansion,File 0, [[@LINE+1]]:3 -> [[@LINE+1]]:8 = #2
- RBRAC
+ RBRAC // CHECK-NEXT: [[@LINE]]:8 -> [[@LINE+6]]:3 = (#0 - #2)
// CHECK-NEXT: File 0, [[@LINE+4]]:3 -> [[@LINE+15]]:2 = (#0 - #2)
// CHECK-NEXT: File 0, [[@LINE+3]]:7 -> [[@LINE+3]]:12 = (#0 - #2)
// CHECK-NEXT: Expansion,File 0, [[@LINE+2]]:14 -> [[@LINE+2]]:19 = #3
// CHECK-NEXT: File 0, [[@LINE+1]]:19 -> [[@LINE+3]]:4 = #3
if (!argc) LBRAC
return 0;
-
[PATCH] D35925: [Coverage] Precise region termination with deferred regions
arphaman accepted this revision.
arphaman added a comment.
This revision is now accepted and ready to land.
Thanks, it does make sense to update llvm-cov.
LGTM:
Comment at: lib/CodeGen/CoverageMappingGen.cpp:554
+ // useful to try and create a deferred region inside of one.
+ UpdateDeferredRegion &= SM.getFileID(EndLoc) == SM.getMainFileID();
+
Might be better to use `&&` to avoid extra work.
Comment at: lib/CodeGen/CoverageMappingGen.cpp:561
+ } else {
+if (Region.isDeferred()) {
+ assert(!ParentOfDeferredRegion && "Consecutive deferred regions");
You can use `else if` here.
https://reviews.llvm.org/D35925
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D35925: [Coverage] Precise region termination with deferred regions
vsk added a comment.
In https://reviews.llvm.org/D35925#823978, @arphaman wrote:
> This is awesome!
>
> I noticed in the sample output that llvm-cov is now forced to print some new
> region markers because the terminator introduces a new region on the same
> line, e.g.
>
> |// CHECK-LABEL: _Z10while_loopv:
> 88| 1|void while_loop() {
> 89| 1| if (false)
> 90| 1|return; // CHECK: [[@LINE]]:11 -> [[@LINE+2]]:3 = (#0 -
> #1)
> ^0^1 // Previously, llvm-cov didn't show region
> markers for this line
> 91| 1|
>
>
> Do you think this can be avoided? Should llvm-cov even try to avoid emitting
> the region markers? It seems to me that this situation affects just the
> command-line output of llvm-cov, and region highlighting in HTML won't be
> impacted by this.
Great question! The region highlighting for the html emitter matches exactly
with the textual emitter's, so there is an impact. I think we can/should make
two general improvements to llvm-cov to address the issue, which would be
useful even without this patch. The first (as you brought up) is to stop
emitting region markers for segments which start, but do not end, on one line.
This fixes an existing annoyance with loops:
for (...; ...; ...) -> { <- This singular curly brace gets a region marker
because the body's region starts here. That's a little distracting.
It would also suppress highlighting for deferred regions, removing a visual
distraction. The second improvement would be to to pick better line execution
counts. In the sample output that you quoted, you see that the line with the
return gets an execution count of 1. What's happening is that llvm-cov picks
the maximum execution count from the regions on (or before) the line. The end
result is a little weird, because we associate the line execution count with
the first region on a line, and we know the "return" is never executed. I have
a patch ready to fix this: it corrects a display issue in the existing llvm-cov
test suite, and it helps even more with deferred regions enabled.
https://reviews.llvm.org/D35925
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D35925: [Coverage] Precise region termination with deferred regions
arphaman added a comment.
This is awesome!
I noticed in the sample output that llvm-cov is now forced to print some new
region markers because the terminator introduces a new region on the same line,
e.g.
|// CHECK-LABEL: _Z10while_loopv:
88| 1|void while_loop() {
89| 1| if (false)
90| 1|return; // CHECK: [[@LINE]]:11 -> [[@LINE+2]]:3 = (#0 - #1)
^0^1 // Previously, llvm-cov didn't show region
markers for this line
91| 1|
Do you think this can be avoided? Should llvm-cov even try to avoid emitting
the region markers? It seems to me that this situation affects just the
command-line output of llvm-cov, and region highlighting in HTML won't be
impacted by this.
https://reviews.llvm.org/D35925
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D35925: [Coverage] Precise region termination with deferred regions
vsk created this revision.
The current coverage implementation doesn't handle region termination
very precisely. Take for example an `if' statement with a `return':
void f() {
if (true) {
return; // The `if' body's region is terminated here.
}
// This line gets the same coverage as the `if' condition.
}
If the function `f' is called, the line containing the comment will be
marked as having executed once, which is not correct.
The solution here is to create a deferred region after terminating a
region. The deferred region is completed once the start location of the
next statement is known, and is then pushed onto the region stack.
In the cases where it's not possible to complete a deferred region, it
can safely be dropped.
Example output (pre-patch): F3800158: deferred-regions-before.txt
Example output (post-patch): F3800159: deferred-regions-after.txt
Testing: lit test updates, a stage2 coverage-enabled build of clang
https://reviews.llvm.org/D35925
Files:
lib/CodeGen/CoverageMappingGen.cpp
test/CoverageMapping/deferred-region.cpp
test/CoverageMapping/label.cpp
test/CoverageMapping/moremacros.c
test/CoverageMapping/return.c
test/CoverageMapping/switch.cpp
test/CoverageMapping/switchmacro.c
test/CoverageMapping/trycatch.cpp
Index: test/CoverageMapping/trycatch.cpp
===
--- test/CoverageMapping/trycatch.cpp
+++ test/CoverageMapping/trycatch.cpp
@@ -18,7 +18,7 @@
// CHECK-NEXT: File 0, [[@LINE+1]]:10 -> [[@LINE+2]]:27 = (#0 - #1)
} else if(i == 8) // CHECK-NEXT: File 0, [[@LINE]]:13 -> [[@LINE]]:19 = (#0 - #1)
throw ImportantError(); // CHECK-NEXT: File 0, [[@LINE]]:5 -> [[@LINE]]:27 = #2
-}
+} // CHECK-NEXT: File 0, [[@LINE-1]]:27 -> [[@LINE]]:2 = ((#0 - #1) - #2)
// CHECK-NEXT: main
int main() { // CHECK-NEXT: File 0, [[@LINE]]:12 -> [[@LINE+13]]:2 = #0
Index: test/CoverageMapping/switchmacro.c
===
--- test/CoverageMapping/switchmacro.c
+++ test/CoverageMapping/switchmacro.c
@@ -8,6 +8,7 @@
default: // CHECK-NEXT: File 0, [[@LINE]]:3 -> {{[0-9]+}}:11 = #2
if (i == 1) // CHECK-NEXT: File 0, [[@LINE]]:9 -> [[@LINE]]:15 = #2
return 0; // CHECK-NEXT: File 0, [[@LINE]]:7 -> [[@LINE]]:15 = #3
+ // CHECK-NEXT: File 0, [[@LINE-1]]:15 -> [[@LINE+3]]:5 = (#2 - #3)
// CHECK-NEXT: Expansion,File 0, [[@LINE+2]]:5 -> [[@LINE+2]]:8 = (#2 - #3) (Expanded file = 1)
// CHECK-NEXT: File 0, [[@LINE+1]]:8 -> {{[0-9]+}}:11 = (#2 - #3)
FOO(1);
Index: test/CoverageMapping/switch.cpp
===
--- test/CoverageMapping/switch.cpp
+++ test/CoverageMapping/switch.cpp
@@ -6,7 +6,7 @@
case 1: // CHECK-NEXT: File 0, [[@LINE]]:3 -> [[@LINE+3]]:10 = #2
return;
case 2: // CHECK-NEXT: File 0, [[@LINE]]:3 -> [[@LINE+1]]:10 = #3
-break;
+break; // CHECK-NEXT: File 0, [[@LINE]]:10 -> [[@LINE+2]]:3 = #1
}
int x = 0;// CHECK-NEXT: File 0, [[@LINE]]:3 -> [[@LINE+1]]:2 = #1
}
@@ -55,16 +55,16 @@
i = 2;
break;
default: // CHECK-NEXT: File 0, [[@LINE]]:3 -> [[@LINE+1]]:10 = #4
-break;
+break; // CHECK-NEXT: File 0, [[@LINE]]:10 -> [[@LINE+2]]:3 = #1
}
switch(i) { // CHECK-NEXT: File 0, [[@LINE]]:3 -> [[@LINE+23]]:2 = #1
case 0: // CHECK-NEXT: File 0, [[@LINE]]:3 -> [[@LINE+6]]:10 = #6
i = 1;
break;
case 1: // CHECK-NEXT: File 0, [[@LINE]]:3 -> [[@LINE+3]]:10 = #7
i = 2;
default: // CHECK-NEXT: File 0, [[@LINE]]:3 -> [[@LINE+1]]:10 = (#7 + #8)
-break;
+break; // CHECK-NEXT: File 0, [[@LINE]]:10 -> [[@LINE+3]]:3 = #5
}
switch(i) { // CHECK-NEXT: File 0, [[@LINE]]:3 -> [[@LINE+13]]:2 = #5
Index: test/CoverageMapping/return.c
===
--- test/CoverageMapping/return.c
+++ test/CoverageMapping/return.c
@@ -13,7 +13,7 @@
for(int i = 0; i < 10; ++i) { // CHECK-NEXT: File 0, [[@LINE]]:31 -> {{[0-9]+}}:4 = #1
// CHECK-NEXT: File 0, [[@LINE+1]]:8 -> [[@LINE+1]]:13 = #1
if(i > 2) { // CHECK-NEXT: File 0, [[@LINE]]:15 -> [[@LINE+2]]:6 = #2
- return;
+ return; // CHECK-NEXT: File 0, [[@LINE+1]]:6 -> [[@LINE+3]]:5 = (#1 - #2)
} // CHECK-NEXT: File 0, [[@LINE+2]]:5 -> {{[0-9]+}}:4 = (#1 - #2)
// CHECK-NEXT: File 0, [[@LINE+1]]:8 -> [[@LINE+1]]:14 = (#1 - #2)
if(i == 3) {// CHECK-NEXT: File 0, [[@LINE]]:16 ->
