[Coverage] Use gap regions to select better line exec counts

After clang started emitting deferred regions (r312818), llvm-cov has
had a hard time picking reasonable line execuction counts. There have
been one or two generic improvements in this area (e.g r310012), but
line counts can still report coverage for whitespace instead of code
(llvm.org/PR34612).

To fix the problem:

 * Introduce a new region kind so that frontends can explicitly label
   gap areas.

   This is done by changing the encoding of the columnEnd field of
   MappingRegion. This doesn't substantially increase binary size, and
   makes it easy to maintain backwards-compatibility.

 * Don't set the line count to a count from a gap area, unless the count
   comes from a wrapped segment.

 * Don't highlight gap areas as uncovered.

Fixes llvm.org/PR34612.

git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@313597 91177308-0d34-0410-b5e6-96231b3b80d8

diff --git a/docs/CoverageMappingFormat.rst b/docs/CoverageMappingFormat.rst
index 46cc9d10f3195b09a92d3ff694da946a32277b41..30b11fe2f31d6f0a41ef1a510471f29a6016885c 100644 (file)
--- a/docs/CoverageMappingFormat.rst
+++ b/docs/CoverageMappingFormat.rst
@@ -258,7 +258,7 @@ The coverage mapping variable generated by Clang has 3 fields:
       i32 2,  ; The number of function records
       i32 20, ; The length of the string that contains the encoded translation unit filenames
       i32 20, ; The length of the string that contains the encoded coverage mapping data
-      i32 1,  ; Coverage mapping format version
+      i32 2,  ; Coverage mapping format version
     },
     [2 x { i64, i32, i64 }] [ ; Function records
      { i64, i32, i64 } {
@@ -274,6 +274,8 @@ The coverage mapping variable generated by Clang has 3 fields:
    [40 x i8] c"..." ; Encoded data (dissected later)
   }, section "__llvm_covmap", align 8
 
+The current version of the format is version 3. The only difference from version 2 is that a special encoding for column end locations was introduced to indicate gap regions.
+
 The function record layout has evolved since version 1. In version 1, the function record for *foo* is defined as follows:
 
 .. code-block:: llvm
@@ -296,7 +298,7 @@ The coverage mapping header has the following fields:
 
 * The length of the string in the third field of *__llvm_coverage_mapping* that contains the encoded coverage mapping data.
 
-* The format version. The current version is 2 (encoded as a 1).
+* The format version. The current version is 3 (encoded as a 2).
 
 .. _function records:
 
@@ -602,4 +604,6 @@ The source range record contains the following fields:
 * *numLines*: The difference between the ending line and the starting line
   of the current mapping region.
 
-* *columnEnd*: The ending column of the mapping region.
+* *columnEnd*: The ending column of the mapping region. If the high bit is set,
+  the current mapping region is a gap area. A count for a gap area is only used
+  as the line execution count if there are no other regions on a line.

diff --git a/include/llvm/ProfileData/Coverage/CoverageMapping.h b/include/llvm/ProfileData/Coverage/CoverageMapping.h
index eb560d9c3dff14ae0a650602c9886e08995bed1a..13a9093bd84f886a3c09ca8dc2bb672ee29fee18 100644 (file)
--- a/include/llvm/ProfileData/Coverage/CoverageMapping.h
+++ b/include/llvm/ProfileData/Coverage/CoverageMapping.h
@@ -213,7 +213,11 @@ struct CounterMappingRegion {
 
     /// A SkippedRegion represents a source range with code that was skipped
     /// by a preprocessor or similar means.
-    SkippedRegion
+    SkippedRegion,
+
+    /// A GapRegion is like a CodeRegion, but its count is only set as the
+    /// line execution count when its the only region in the line.
+    GapRegion
   };
 
   Counter Count;
@@ -250,6 +254,13 @@ struct CounterMappingRegion {
                                 LineEnd, ColumnEnd, SkippedRegion);
   }
 
+  static CounterMappingRegion
+  makeGapRegion(Counter Count, unsigned FileID, unsigned LineStart,
+                unsigned ColumnStart, unsigned LineEnd, unsigned ColumnEnd) {
+    return CounterMappingRegion(Count, FileID, 0, LineStart, ColumnStart,
+                                LineEnd, (1U << 31) | ColumnEnd, GapRegion);
+  }
+
   inline LineColPair startLoc() const {
     return LineColPair(LineStart, ColumnStart);
   }
@@ -377,19 +388,23 @@ struct CoverageSegment {
   bool HasCount;
   /// Whether this enters a new region or returns to a previous count.
   bool IsRegionEntry;
+  /// Whether this enters a gap region.
+  bool IsGapRegion;
 
   CoverageSegment(unsigned Line, unsigned Col, bool IsRegionEntry)
       : Line(Line), Col(Col), Count(0), HasCount(false),
-        IsRegionEntry(IsRegionEntry) {}
+        IsRegionEntry(IsRegionEntry), IsGapRegion(false) {}
 
   CoverageSegment(unsigned Line, unsigned Col, uint64_t Count,
-                  bool IsRegionEntry)
+                  bool IsRegionEntry, bool IsGapRegion = false)
       : Line(Line), Col(Col), Count(Count), HasCount(true),
-        IsRegionEntry(IsRegionEntry) {}
+        IsRegionEntry(IsRegionEntry), IsGapRegion(IsGapRegion) {}
 
   friend bool operator==(const CoverageSegment &L, const CoverageSegment &R) {
-    return std::tie(L.Line, L.Col, L.Count, L.HasCount, L.IsRegionEntry) ==
-           std::tie(R.Line, R.Col, R.Count, R.HasCount, R.IsRegionEntry);
+    return std::tie(L.Line, L.Col, L.Count, L.HasCount, L.IsRegionEntry,
+                    L.IsGapRegion) == std::tie(R.Line, R.Col, R.Count,
+                                               R.HasCount, R.IsRegionEntry,
+                                               R.IsGapRegion);
   }
 };
 
@@ -660,7 +675,10 @@ enum CovMapVersion {
   // name string pointer to MD5 to support name section compression. Name
   // section is also compressed.
   Version2 = 1,
-  // The current version is Version2
+  // A new interpretation of the columnEnd field is added in order to mark
+  // regions as gap areas.
+  Version3 = 2,
+  // The current version is Version3
   CurrentVersion = INSTR_PROF_COVMAP_VERSION
 };
 

diff --git a/include/llvm/ProfileData/InstrProfData.inc b/include/llvm/ProfileData/InstrProfData.inc
index be0dd4ad04bf611b3922fa6dec0112a0d72c5ea3..66d63a462a07106f48b636faaefc4689dd273b34 100644 (file)
--- a/include/llvm/ProfileData/InstrProfData.inc
+++ b/include/llvm/ProfileData/InstrProfData.inc
@@ -630,7 +630,7 @@ serializeValueProfDataFrom(ValueProfRecordClosure *Closure,
 /* Indexed profile format version (start from 1). */
 #define INSTR_PROF_INDEX_VERSION 4
 /* Coverage mapping format vresion (start from 0). */
-#define INSTR_PROF_COVMAP_VERSION 1
+#define INSTR_PROF_COVMAP_VERSION 2
 
 /* Profile version is always of type uint64_t. Reserve the upper 8 bits in the
  * version for other variants of profile. We set the lowest bit of the upper 8

diff --git a/lib/ProfileData/Coverage/CoverageMapping.cpp b/lib/ProfileData/Coverage/CoverageMapping.cpp
index 81e1c6bc6450dd4227ad33c13d4dca03b4570e24..4c257cf38e59af8d1dec530e7ac529d95afff74d 100644 (file)
--- a/lib/ProfileData/Coverage/CoverageMapping.cpp
+++ b/lib/ProfileData/Coverage/CoverageMapping.cpp
@@ -320,7 +320,7 @@ class SegmentBuilder {
 
   /// Emit a segment with the count from \p Region starting at \p StartLoc.
   //
-  /// \p IsRegionEntry: The segment is at the start of a new region.
+  /// \p IsRegionEntry: The segment is at the start of a new non-gap region.
   /// \p EmitSkippedRegion: The segment must be emitted as a skipped region.
   void startSegment(const CountedRegion &Region, LineColPair StartLoc,
                     bool IsRegionEntry, bool EmitSkippedRegion = false) {
@@ -337,7 +337,8 @@ class SegmentBuilder {
 
     if (HasCount)
       Segments.emplace_back(StartLoc.first, StartLoc.second,
-                            Region.ExecutionCount, IsRegionEntry);
+                            Region.ExecutionCount, IsRegionEntry,
+                            Region.Kind == CounterMappingRegion::GapRegion);
     else
       Segments.emplace_back(StartLoc.first, StartLoc.second, IsRegionEntry);
 
@@ -346,7 +347,8 @@ class SegmentBuilder {
       dbgs() << "Segment at " << Last.Line << ":" << Last.Col
              << " (count = " << Last.Count << ")"
              << (Last.IsRegionEntry ? ", RegionEntry" : "")
-             << (!Last.HasCount ? ", Skipped" : "") << "\n";
+             << (!Last.HasCount ? ", Skipped" : "")
+             << (Last.IsGapRegion ? ", Gap" : "") << "\n";
     });
   }
 
@@ -419,20 +421,22 @@ class SegmentBuilder {
         completeRegionsUntil(CurStartLoc, FirstCompletedRegion);
       }
 
+      bool GapRegion = CR.value().Kind == CounterMappingRegion::GapRegion;
+
       // Try to emit a segment for the current region.
       if (CurStartLoc == CR.value().endLoc()) {
         // Avoid making zero-length regions active. If it's the last region,
         // emit a skipped segment. Otherwise use its predecessor's count.
         const bool Skipped = (CR.index() + 1) == Regions.size();
         startSegment(ActiveRegions.empty() ? CR.value() : *ActiveRegions.back(),
-                     CurStartLoc, true, Skipped);
+                     CurStartLoc, !GapRegion, Skipped);
         continue;
       }
       if (CR.index() + 1 == Regions.size() ||
           CurStartLoc != Regions[CR.index() + 1].startLoc()) {
         // Emit a segment if the next region doesn't start at the same location
         // as this one.
-        startSegment(CR.value(), CurStartLoc, true);
+        startSegment(CR.value(), CurStartLoc, !GapRegion);
       }
 
       // This region is active (i.e not completed).

diff --git a/lib/ProfileData/Coverage/CoverageMappingReader.cpp b/lib/ProfileData/Coverage/CoverageMappingReader.cpp
index 9bf70c2e3aadbf12a39ec5dcbd30c977fe1adcb7..467a36ca748353dc9bfb7f57fceab5501b67b405 100644 (file)
--- a/lib/ProfileData/Coverage/CoverageMappingReader.cpp
+++ b/lib/ProfileData/Coverage/CoverageMappingReader.cpp
@@ -216,6 +216,13 @@ Error RawCoverageMappingReader::readMappingRegionsSubArray(
     if (auto Err = readIntMax(ColumnEnd, std::numeric_limits<unsigned>::max()))
       return Err;
     LineStart += LineStartDelta;
+
+    // If the high bit of ColumnEnd is set, this is a gap region.
+    if (ColumnEnd & (1U << 31)) {
+      Kind = CounterMappingRegion::GapRegion;
+      ColumnEnd &= ~(1U << 31);
+    }
+
     // Adjust the column locations for the empty regions that are supposed to
     // cover whole lines. Those regions should be encoded with the
     // column range (1 -> std::numeric_limits<unsigned>::max()), but because
@@ -534,11 +541,16 @@ Expected<std::unique_ptr<CovMapFuncRecordReader>> CovMapFuncRecordReader::get(
     return llvm::make_unique<VersionedCovMapFuncRecordReader<
         CovMapVersion::Version1, IntPtrT, Endian>>(P, R, F);
   case CovMapVersion::Version2:
+  case CovMapVersion::Version3:
     // Decompress the name data.
     if (Error E = P.create(P.getNameData()))
       return std::move(E);
-    return llvm::make_unique<VersionedCovMapFuncRecordReader<
-        CovMapVersion::Version2, IntPtrT, Endian>>(P, R, F);
+    if (Version == CovMapVersion::Version2)
+      return llvm::make_unique<VersionedCovMapFuncRecordReader<
+          CovMapVersion::Version2, IntPtrT, Endian>>(P, R, F);
+    else
+      return llvm::make_unique<VersionedCovMapFuncRecordReader<
+          CovMapVersion::Version3, IntPtrT, Endian>>(P, R, F);
   }
   llvm_unreachable("Unsupported version");
 }

diff --git a/lib/ProfileData/Coverage/CoverageMappingWriter.cpp b/lib/ProfileData/Coverage/CoverageMappingWriter.cpp
index 4a1ce16c486975ae542899e2c04367f7e2ccf31a..49e82e48105913bc4464da9059cc036a6d066455 100644 (file)
--- a/lib/ProfileData/Coverage/CoverageMappingWriter.cpp
+++ b/lib/ProfileData/Coverage/CoverageMappingWriter.cpp
@@ -171,6 +171,7 @@ void CoverageMappingWriter::write(raw_ostream &OS) {
     Counter Count = Minimizer.adjust(I->Count);
     switch (I->Kind) {
     case CounterMappingRegion::CodeRegion:
+    case CounterMappingRegion::GapRegion:
       writeCounter(MinExpressions, Count, OS);
       break;
     case CounterMappingRegion::ExpansionRegion: {

diff --git a/test/tools/llvm-cov/Inputs/deferred-regions.covmapping b/test/tools/llvm-cov/Inputs/deferred-regions.covmapping
index 757f3eea7204b1cb3bccd68918bcbf81a03a7292..4434b66513c07171fbf64b26e3198152582a359d 100644 (file)
Binary files a/test/tools/llvm-cov/Inputs/deferred-regions.covmapping and b/test/tools/llvm-cov/Inputs/deferred-regions.covmapping differ

diff --git a/test/tools/llvm-cov/deferred-region.cpp b/test/tools/llvm-cov/deferred-region.cpp
index 1b191ac0f1a78ddab4d137bba1c432b8657f53bd..6f6da7ec804b3f091f545e3986b43c241c5376ec 100644 (file)
--- a/test/tools/llvm-cov/deferred-region.cpp
+++ b/test/tools/llvm-cov/deferred-region.cpp
@@ -1,7 +1,7 @@
 // RUN: llvm-cov show %S/Inputs/deferred-regions.covmapping -instr-profile %S/Inputs/deferred-regions.profdata -show-line-counts-or-regions -dump -path-equivalence=/Users/vk/src/llvm.org-coverage-braces/llvm/test/tools,%S/.. %s 2>%t.markers > %t.out && FileCheck %s -input-file %t.out && FileCheck %s -input-file %t.markers -check-prefix=MARKER
 
 void foo(int x) {
-  if (x == 0) {
+  if (x == 0) { // CHECK: [[@LINE]]|{{ +}}2|
     return; // CHECK: [[@LINE]]|{{ +}}1|
   }
 
@@ -17,10 +17,10 @@ void for_loop() {
     return; // CHECK: [[@LINE]]|{{ +}}0|
 
   for (int i = 0; i < 10; ++i) { // CHECK: [[@LINE]]|{{ +}}2|
-    if (i % 2 == 0)
+    if (i % 2 == 0) // CHECK: [[@LINE]]|{{ +}}2|
       continue; // CHECK: [[@LINE]]|{{ +}}1|
 
-    if (i % 5 == 0)
+    if (i % 5 == 0) // CHECK: [[@LINE]]|{{ +}}1|
       break; // CHECK: [[@LINE]]|{{ +}}0|
 
     int x = i;
@@ -37,19 +37,19 @@ void while_loop() {
 
   int x = 0;
   while (++x < 10) { // CHECK: [[@LINE]]|{{ +}}3|
-    if (x == 1)
+    if (x == 1) // CHECK: [[@LINE]]|{{ +}}2|
       continue; // CHECK: [[@LINE]]|{{ +}}1|
 
     while (++x < 4) { // CHECK: [[@LINE]]|{{ +}}1|
-      if (x == 3)
+      if (x == 3) // CHECK: [[@LINE]]|{{ +}}1|
         break; // CHECK: [[@LINE]]|{{ +}}1|
                // CHECK: [[@LINE]]|{{ +}}0|
       while (++x < 5) {} // CHECK: [[@LINE]]|{{ +}}0|
     } // CHECK: [[@LINE]]|{{ +}}1|
 
-    if (x == 0)
+    if (x == 0) // CHECK: [[@LINE]]|{{ +}}1|
       throw Error(); // CHECK: [[@LINE]]|{{ +}}0|
-
+                // CHECK: [[@LINE]]|{{ +}}1|
     while (++x < 9) { // CHECK: [[@LINE]]|{{ +}}6|
       if (x == 0) // CHECK: [[@LINE]]|{{ +}}5|
         break; // CHECK: [[@LINE]]|{{ +}}0|
@@ -59,12 +59,12 @@ void while_loop() {
 }
 
 void gotos() {
-  if (false)
+  if (false) // CHECK: [[@LINE]]|{{ +}}1|
     goto out; // CHECK: [[@LINE]]|{{ +}}0|
+          // CHECK: [[@LINE]]|{{ +}}1|
+  return; // CHECK: [[@LINE]]|{{ +}}1|
 
-  return;
-
-out: // CHECK: [[@LINE]]|{{ +}}1|
+out: // CHECK: [[@LINE]]|{{ +}}0|
        return;
 }
 
@@ -80,23 +80,16 @@ int main() {
 
 // MARKER: Marker at 4:7 = 2
 // MARKER-NEXT: Highlighted line 17, 5 -> 11
-// MARKER-NEXT: Marker at 17:5 = 0
 // MARKER-NEXT: Marker at 19:3 = 1
 // MARKER-NEXT: Marker at 19:19 = 2
 // MARKER-NEXT: Marker at 19:27 = 1
-// MARKER-NEXT: Marker at 21:7 = 1
 // MARKER-NEXT: Marker at 23:5 = 1
 // MARKER-NEXT: Marker at 23:9 = 1
 // MARKER-NEXT: Highlighted line 24, 7 -> 12
-// MARKER-NEXT: Marker at 24:7 = 0
 // MARKER-NEXT: Highlighted line 36, 5 -> 11
-// MARKER-NEXT: Marker at 36:5 = 0
 // MARKER-NEXT: Marker at 39:10 = 3
-// MARKER-NEXT: Marker at 41:7 = 1
 // MARKER-NEXT: Marker at 43:5 = 1
 // MARKER-NEXT: Marker at 43:12 = 1
-// MARKER-NEXT: Highlighted line 45, 14 -> ?
-// MARKER-NEXT: Marker at 45:9 = 1
 // MARKER-NEXT: Highlighted line 46, 1 -> ?
 // MARKER-NEXT: Highlighted line 47, 1 -> 7
 // MARKER-NEXT: Highlighted line 47, 7 -> 14
@@ -107,13 +100,10 @@ int main() {
 // MARKER-NEXT: Marker at 47:14 = 0
 // MARKER-NEXT: Marker at 47:23 = 0
 // MARKER-NEXT: Highlighted line 51, 7 -> 20
-// MARKER-NEXT: Marker at 51:7 = 0
 // MARKER-NEXT: Marker at 53:5 = 1
 // MARKER-NEXT: Marker at 53:12 = 6
 // MARKER-NEXT: Highlighted line 55, 9 -> 14
 // MARKER-NEXT: Highlighted line 63, 5 -> 13
-// MARKER-NEXT: Marker at 63:5 = 0
 // MARKER-NEXT: Highlighted line 67, 1 -> ?
 // MARKER-NEXT: Highlighted line 68, 1 -> 8
-// MARKER-NEXT: Highlighted line 68, 8 -> ?
 // MARKER-NEXT: Highlighted line 69, 1 -> 2

diff --git a/tools/llvm-cov/SourceCoverageView.cpp b/tools/llvm-cov/SourceCoverageView.cpp
index 79c2c69571c85e005e44e67b2a637deb2bb5e7f1..1965595cdb7fab1188dec885cb98915030eadc11 100644 (file)
--- a/tools/llvm-cov/SourceCoverageView.cpp
+++ b/tools/llvm-cov/SourceCoverageView.cpp
@@ -89,7 +89,7 @@ LineCoverageStats::LineCoverageStats(
   // Find the minimum number of regions which start in this line.
   unsigned MinRegionCount = 0;
   auto isStartOfRegion = [](const coverage::CoverageSegment *S) {
-    return S->HasCount && S->IsRegionEntry;
+    return !S->IsGapRegion && S->HasCount && S->IsRegionEntry;
   };
   for (unsigned I = 0; I < LineSegments.size() && MinRegionCount < 2; ++I)
     if (isStartOfRegion(LineSegments[I]))
@@ -112,16 +112,19 @@ LineCoverageStats::LineCoverageStats(
   // avoid erroneously using the wrapped count, and to avoid picking region
   // counts which come from deferred regions.
   if (LineSegments.size() > 1) {
-    for (unsigned I = 0; I < LineSegments.size() - 1; ++I)
-      ExecutionCount = std::max(ExecutionCount, LineSegments[I]->Count);
+    for (unsigned I = 0; I < LineSegments.size() - 1; ++I) {
+      if (!LineSegments[I]->IsGapRegion)
+        ExecutionCount = std::max(ExecutionCount, LineSegments[I]->Count);
+    }
     return;
   }
 
-  // Just pick the maximum count.
-  if (WrappedSegment && WrappedSegment->HasCount)
+  // If a non-gap region starts here, use its count. Otherwise use the wrapped
+  // count.
+  if (MinRegionCount == 1)
+    ExecutionCount = LineSegments[0]->Count;
+  else
     ExecutionCount = WrappedSegment->Count;
-  if (!LineSegments.empty())
-    ExecutionCount = std::max(ExecutionCount, LineSegments[0]->Count);
 }
 
 unsigned SourceCoverageView::getFirstUncoveredLineNo() {

diff --git a/tools/llvm-cov/SourceCoverageViewHTML.cpp b/tools/llvm-cov/SourceCoverageViewHTML.cpp
index 1a21b72b14e2eaace3fa95592bc43d9773a3e020..300a0162bd11798aa8b39df9a8d4c35ee12b9e2d 100644 (file)
--- a/tools/llvm-cov/SourceCoverageViewHTML.cpp
+++ b/tools/llvm-cov/SourceCoverageViewHTML.cpp
@@ -527,7 +527,7 @@ void SourceCoverageViewHTML::renderLine(
     const auto *CurSeg = Segments[I];
     if (CurSeg->Col == ExpansionCol)
       Color = "cyan";
-    else if (CheckIfUncovered(CurSeg))
+    else if (!CurSeg->IsGapRegion && CheckIfUncovered(CurSeg))
       Color = "red";
     else
       Color = None;

diff --git a/tools/llvm-cov/SourceCoverageViewText.cpp b/tools/llvm-cov/SourceCoverageViewText.cpp
index a88558fceca57089a74bc4f09599738411827864..a78c0575cdc0315dada669780db4e7dbe1439ef7 100644 (file)
--- a/tools/llvm-cov/SourceCoverageViewText.cpp
+++ b/tools/llvm-cov/SourceCoverageViewText.cpp
@@ -120,7 +120,7 @@ void SourceCoverageViewText::renderLine(
     Col = End;
     if (Col == ExpansionCol)
       Highlight = raw_ostream::CYAN;
-    else if (S->HasCount && S->Count == 0)
+    else if (!S->IsGapRegion && S->HasCount && S->Count == 0)
       Highlight = raw_ostream::RED;
     else
       Highlight = None;