[llvm-cov] Reset ExecutionCount only if first line segment is start of a new region.

[zequanwu]

Summary

To fix the bug, introduced by D85036, llvm-cov shows the following:

1|       |#include <stdlib.h>
2|       |
3|      1|int main() {
4|      1|  return getenv(
5|      0|      "TEST") ? 1
6|      1|              : 0;
7|      1|}

[pirama]

Thanks for the patch Zequan. This fixes the C++ test case that prompted this issue.

The underlying Rust issue is not fixed though:

 1|       |fn notatest() -> Result<(), std::env::VarError> {
 2|      1|        let result = std::env::var("MEOW")
 3|      0|                    .or(Ok("a".to_string()))?;
 4|      1|            println!("{}", result);
 5|      1|                Ok(())
 6|      1|}
 7|       | 
 8|      1|fn main() {
 9|      1|        println!("Hello, world! {:?}", notatest());
10|      1|}

Here're the regions from -debug-only=coverage-mapping:

Segment at 2:13 (count = 1), RegionEntry
Segment at 2:19 (count = 0), Skipped                                                          
Segment at 2:22 (count = 1), RegionEntry                                                      
Segment at 3:45 (count = 0), RegionEntry
Segment at 3:46 (count = 0), Skipped
Segment at 4:13 (count = 1), RegionEntry
Segment at 5:23 (count = 0), Skipped
Segment at 6:1 (count = 1), RegionEntry
Segment at 6:2 (count = 0), Skipped
Segment at 8:11 (count = 1), RegionEntry
Segment at 10:2 (count = 0), Skipped

The ternary operator case in the C++ test case has a gap region corresponding to the ?. For the Rust ? operator, there is no similar gap region. In fact, the rust frontend associates the counter corresponding to the error path of the ? operator to the ? token . Is there a better sequence of Segments that the Rust frontend can emit to handle this case?

[zequanwu]

In D95987#2541091, @pirama wrote:

Thanks for the patch Zequan. This fixes the C++ test case that prompted this issue.

The underlying Rust issue is not fixed though:

 1|       |fn notatest() -> Result<(), std::env::VarError> {
 2|      1|        let result = std::env::var("MEOW")
 3|      0|                    .or(Ok("a".to_string()))?;
 4|      1|            println!("{}", result);
 5|      1|                Ok(())
 6|      1|}
 7|       | 
 8|      1|fn main() {
 9|      1|        println!("Hello, world! {:?}", notatest());
10|      1|}

Here're the regions from -debug-only=coverage-mapping:

Segment at 2:13 (count = 1), RegionEntry
Segment at 2:19 (count = 0), Skipped                                                          
Segment at 2:22 (count = 1), RegionEntry                                                      
Segment at 3:45 (count = 0), RegionEntry
Segment at 3:46 (count = 0), Skipped
Segment at 4:13 (count = 1), RegionEntry
Segment at 5:23 (count = 0), Skipped
Segment at 6:1 (count = 1), RegionEntry
Segment at 6:2 (count = 0), Skipped
Segment at 8:11 (count = 1), RegionEntry
Segment at 10:2 (count = 0), Skipped

The ternary operator case in the C++ test case has a gap region corresponding to the ?. For the Rust ? operator, there is no similar gap region. In fact, the rust frontend associates the counter corresponding to the error path of the ? operator to the ? token . Is there a better sequence of Segments that the Rust frontend can emit to handle this case?

Sorry, I don't know about Rust. I just read the ? operator doc on https://doc.rust-lang.org/edition-guide/rust-2018/error-handling-and-panics/the-question-mark-operator-for-easier-error-handling.html.

For the example you provided, I am not sure if it's a bug or not. Segment at 3:45 (count = 0), RegionEntry corresponds to the error path of the ? operator, which means error path is not executed. So, the 0 count looks reasonable. If we mark line 3 as executed regardless of which path is chosen, we may lost the information of path choices.

[zequanwu]

For the example you provided, I am not sure if it's a bug or not. Segment at 3:45 (count = 0), RegionEntry corresponds to the error path of the ? operator, which means error path is not executed. So, the 0 count looks reasonable. If we mark line 3 as executed regardless of which path is chosen, we may lost the information of path choices.

I just realized that this also applies to C++ ? operator. In the following case, if we always mark line 5 as executed, we don't know which branch is chosen (But when generating html report by llvm-cov, we will see that the not executed branch is marked with red color).

1|       |#include <stdlib.h>
2|       |
3|      1|int main() {
4|      1|  return getenv(
5|      0|      "TEST") ? 1
6|      1|              : 0;
7|      1|}

[pirama]

In D95987#2543866, @zequanwu wrote:

For the example you provided, I am not sure if it's a bug or not. Segment at 3:45 (count = 0), RegionEntry corresponds to the error path of the ? operator, which means error path is not executed. So, the 0 count looks reasonable. If we mark line 3 as executed regardless of which path is chosen, we may lost the information of path choices.

I just realized that this also applies to C++ ? operator. In the following case, if we always mark line 5 as executed, we don't know which branch is chosen (But when generating html report by llvm-cov, we will see that the not executed branch is marked with red color).

I agree and think this patch may be unnecessary. The region coverage information in the html reports are accurate than line coverage and we have to use some heuristics here when computing line coverage.

1|       |#include <stdlib.h>
2|       |
3|      1|int main() {
4|      1|  return getenv(
5|      0|      "TEST") ? 1
6|      1|              : 0;
7|      1|}

[zequanwu]

Abandoned since this is not a bug.

compiler-rt/test/profile/coverage_wrappedsegments.cpp

+// RUN: %clangxx_profgen -fcoverage-mapping -o %t %s
+// RUN: env LLVM_PROFILE_FILE=%t.profraw %run %t
+// RUN: llvm-profdata merge -o %t.profdata %t.profraw
+// RUN: llvm-cov show %t -instr-profile %t.profdata 2>&1 | FileCheck %s
+
+#include <stdlib.h>
+
+int f1() {                  // CHECK:      [[# @LINE]]| 1|int f1() {
+  return getenv(            // CHECK-NEXT: [[# @LINE]]| 1|
+             "TEST") ? 1    // CHECK-NEXT: [[# @LINE]]| 1|
+                     : 0;   // CHECK-NEXT: [[# @LINE]]| 1|
+}                           // CHECK-NEXT: [[# @LINE]]| 1|}
+
+int f2() {                  // CHECK:      [[# @LINE]]| 1|int f2() {
+  return getenv(            // CHECK-NEXT: [[# @LINE]]| 1|
+             "TEST"         // CHECK-NEXT: [[# @LINE]]| 1|
+             "TEST") ? 1    // CHECK-NEXT: [[# @LINE]]| 1|
+                     : 0;   // CHECK-NEXT: [[# @LINE]]| 1|
+}                           // CHECK-NEXT: [[# @LINE]]| 1|}
+
+int f3() {                  // CHECK:      [[# @LINE]]| 1|int f3() {
+  return 0;                 // CHECK-NEXT: [[# @LINE]]| 1|
+  return 1;                 // CHECK-NEXT: [[# @LINE]]| 0|
+}                           // CHECK-NEXT: [[# @LINE]]| 1|}
+
+int f4() {                  // CHECK:      [[# @LINE]]| 1|int f4() {
+  int a = 0;                // CHECK-NEXT: [[# @LINE]]| 1|
+  for (; a < 9;)            // CHECK-NEXT: [[# @LINE]]| 1|
+    return a;               // CHECK-NEXT: [[# @LINE]]| 1|
+  return a;                 // CHECK-NEXT: [[# @LINE]]| 0|
+}                           // CHECK-NEXT: [[# @LINE]]| 1|}
+
+int f5() {                  // CHECK:      [[# @LINE]]| 1|int f5() {
+  int i=1, j=1;             // CHECK-NEXT: [[# @LINE]]| 1|
+  if (i > 0) {              // CHECK-NEXT: [[# @LINE]]| 1|
+    if (j > 0)              // CHECK-NEXT: [[# @LINE]]| 1|
+      return 0;             // CHECK-NEXT: [[# @LINE]]| 1|
+    else                    // CHECK-NEXT: [[# @LINE]]| 0|
+      j++;                  // CHECK-NEXT: [[# @LINE]]| 0|
+  }                         // CHECK-NEXT: [[# @LINE]]| 1|
+  return 1;                 // CHECK-NEXT: [[# @LINE]]| 0|
+}                           // CHECK-NEXT: [[# @LINE]]| 1|}
+
+int main() {
+    f1();
+    f2();
+    f3();
+    f4();
+    f5();
+    return 0;
+}

llvm/lib/ProfileData/Coverage/CoverageMapping.cpp

if (!Mapped)
     return;
 
   // Pick the max count from the non-gap, region entry segments and the
   // wrapped count.
   if (WrappedSegment)
     ExecutionCount = WrappedSegment->Count;
   if (!MinRegionCount)
     return;
+  // If the first line segment is start of region, discard wrapped segment
+  // count.
+  if (isStartOfRegion(LineSegments[0]))
     ExecutionCount = 0;
   for (const auto *LS : LineSegments)
     if (isStartOfRegion(LS))
       ExecutionCount = std::max(ExecutionCount, LS->Count);
 }
 
 LineCoverageIterator &LineCoverageIterator::operator++() {
   if (Next == CD.end()) {
     Stats = LineCoverageStats();