[Coverage] Combine counts of expansion region if there are no code regions for the same area.

[ikudrin]

The motivation case for this patch is the following:

$ cat > sample.cpp << EOF
#define DO_LOOP1() \
    for (int K = 0; K < 2; ++K) {}

#define DO_LOOP2() \
    for (int K = 0; K < 2; ++K) {}

#define JUST_EXPAND() DO_LOOP2()

int main() {
    DO_LOOP1();
    JUST_EXPAND();
    return 0;
}
EOF
$ clang++ -fprofile-instr-generate -fcoverage-mapping sample.cpp 
$ ./a.out
$ llvm-profdata merge -o default.profdata default.profraw
$ llvm-cov show a.out -instr-profile default.profdata
       |    1|#define DO_LOOP1() \
      3|    2|    for (int K = 0; K < 2; ++K) {}
       |    3|
       |    4|#define DO_LOOP2() \
      3|    5|    for (int K = 0; K < 2; ++K) {}
       |    6|
      3|    7|#define JUST_EXPAND() DO_LOOP2()
       |    8|
      1|    9|int main() {
      3|   10|    DO_LOOP1();
      1|   11|    JUST_EXPAND();
      1|   12|    return 0;
      1|   13|}

As you may see, the counts for DO_LOOP macroses are 3, and these values are propagated to the outer code. As a result, counts for lines 7 and 10 are shown as 3 which looks quite weird.
After applying the patch, the output of llvm-cov looks more predictable:

$ llvm-cov show a.out -instr-profile default.profdata
       |    1|#define DO_LOOP1() \
      3|    2|    for (int K = 0; K < 2; ++K) {}
       |    3|
       |    4|#define DO_LOOP2() \
      3|    5|    for (int K = 0; K < 2; ++K) {}
       |    6|
      1|    7|#define JUST_EXPAND() DO_LOOP2()
       |    8|
      1|    9|int main() {
      1|   10|    DO_LOOP1();
      1|   11|    JUST_EXPAND();
      1|   12|    return 0;
      1|   13|}

[ikudrin]

Another example which is also fixed by this patch:

$ cat > sample.cpp << EOF
#define DO_LOOP() \
    for (int k = 0; k < 2; ++k) {}

#define DO_SOMETHING() \
    { \
        DO_LOOP(); \
    }

int main() {
    DO_SOMETHING();
    DO_SOMETHING();
    return 0;
}
EOF
$ clang++ -fprofile-instr-generate -fcoverage-mapping sample.cpp 
$ ./a.out
$ llvm-profdata merge -o default.profdata default.profraw
$ llvm-cov show a.out -instr-profile default.profdata
       |    1|#define DO_LOOP() \
      6|    2|    for (int k = 0; k < 2; ++k) {}
       |    3|
       |    4|#define DO_SOMETHING() \
      2|    5|    { \
      3|    6|        DO_LOOP(); \
      2|    7|    }
       |    8|
      1|    9|int main() {
      1|   10|    DO_SOMETHING();
      1|   11|    DO_SOMETHING();
      1|   12|    return 0;
      1|   13|}

Pay attention to line 6. The patched llvm-cov generates the following output:

$ llvm-cov show a.out -instr-profile default.profdata
       |    1|#define DO_LOOP() \
      6|    2|    for (int k = 0; k < 2; ++k) {}
       |    3|
       |    4|#define DO_SOMETHING() \
      2|    5|    { \
      2|    6|        DO_LOOP(); \
      2|    7|    }
       |    8|
      1|    9|int main() {
      1|   10|    DO_SOMETHING();
      1|   11|    DO_SOMETHING();
      1|   12|    return 0;
      1|   13|}

[davidxl]

Your little motivating examples are great -- please add them as test cases as well.

llvm/trunk/lib/ProfileData/CoverageMapping.cpp

327

what is this change about?

410

Can you add a comment here with more explanation?

llvm/trunk/unittests/ProfileData/CoverageMappingTest.cpp

526

Also add a test for "include".

[ikudrin]

• Reworked to correspond the changes of D18610.
• Expanded the test.
• Added some comments.

I'll add test cases for llvm-cov with the next update.

[ikudrin]

• Added a test for llvm-cov.

[davidxl]

The code looks correct, but after a second though, I wonder if the logic in
fixExpansionRegion

should really be done in Clang: tools/clang/lib/CodeGen/CoverageMappingGen.cpp

The counter expression for the expansion region should really be the counter of the enclosing region, instead of 'enclosing region counter + ....'.

[ikudrin]

In D18831#411576, @davidxl wrote:

The code looks correct, but after a second though, I wonder if the logic in
fixExpansionRegion

should really be done in Clang: tools/clang/lib/CodeGen/CoverageMappingGen.cpp

The counter expression for the expansion region should really be the counter of the enclosing region, instead of 'enclosing region counter + ....'.

Thanks! Sounds reasonable, I'll see if the issue can be fixed there.

[ikudrin]

• To fully fix the original issue, D19725 is required.
• Simplified the logic of combining regions.
• Improved comments.
• Reworked test cases to check only the issue with combining expansions.

[vsk]

Thanks, this is great!

unittests/ProfileData/CoverageMappingTest.cpp

455 ↗

(On Diff #55606)

Could you use distinct counters here (e.g {2,3,5,7})? It would help show that the right regions are merged together.

[davidxl]

other than vsk's comments, lgtm.

lib/ProfileData/CoverageMapping.cpp

378 ↗

(On Diff #55606)

Clarify that ExpansionRegions from expanding a macro nested in another macro will have the same source locations. For such a case, the expansion regions's counts need to be combined.

llvm/trunk/lib/ProfileData/CoverageMapping.cpp

public:
   std::vector<CoverageSegment> buildSegments(ArrayRef<CountedRegion> Regions) {
     const CountedRegion *ReferenceRegion = nullptr;
     Optional<uint64_t> Count = None;
     for (const auto &Region : Regions) {
       // Combine counters of the regions which cover the same area.
       if (ReferenceRegion && ReferenceRegion->startLoc() == Region.startLoc() &&
           ReferenceRegion->endLoc() == Region.endLoc()) {
         if (Region.Kind == coverage::CounterMappingRegion::CodeRegion)
-          Count = Count.getValueOr(0) + Region.ExecutionCount;
+          Count = Count.getValue() + Region.ExecutionCount;
         continue;
       }
       // Add the previous region with the combined counter to the stack.
       if (ReferenceRegion)
         pushRegion(ReferenceRegion, Count);
       // Pop any regions that end before this one starts.
       while (!ActiveRegions.empty() &&
              ActiveRegions.back().first->endLoc() <= Region.startLoc())
         popRegion();
       ReferenceRegion = &Region;
-      if (Region.Kind != coverage::CounterMappingRegion::SkippedRegion)
+      switch (Region.Kind) {
+      case coverage::CounterMappingRegion::CodeRegion:
         Count = Region.ExecutionCount;
-      else
+        break;
+      case coverage::CounterMappingRegion::ExpansionRegion:
+        // Preserve the count of the outer region.
+        break;
+      case coverage::CounterMappingRegion::SkippedRegion:
         Count = None;
+        break;
+      }
     }
     // Add the last region to the stack.
     if (ReferenceRegion)
       pushRegion(ReferenceRegion, Count);
     // Pop any regions that are left in the stack.
     while (!ActiveRegions.empty())
       popRegion();
     return Segments;
static Optional<unsigned> findMainViewFileID(StringRef SourceFile,
   Optional<unsigned> I = findMainViewFileID(Function);
   if (I && SourceFile != Function.Filenames[*I])
     I.reset();
   return I;
 }
 
 /// Sort a nested sequence of regions from a single file.
 template <class It> static void sortNestedRegions(It First, It Last) {
-  std::sort(First, Last,
-            [](const CountedRegion &LHS, const CountedRegion &RHS) {
-    if (LHS.startLoc() == RHS.startLoc())
+  std::sort(
+      First, Last, [](const CountedRegion &LHS, const CountedRegion &RHS) {
+        if (LHS.startLoc() != RHS.startLoc())
+          return LHS.startLoc() < RHS.startLoc();
+        if (LHS.endLoc() != RHS.endLoc())
           // When LHS completely contains RHS, we sort LHS first.
           return RHS.endLoc() < LHS.endLoc();
-    return LHS.startLoc() < RHS.startLoc();
+        // If LHS and RHS cover the same region,
+        // prefer CodeRegion to ExpansionRegion
+        // and ExpansionRegion to SkippedRegion.
+        static_assert(coverage::CounterMappingRegion::CodeRegion <
+                              coverage::CounterMappingRegion::ExpansionRegion &&
+                          coverage::CounterMappingRegion::ExpansionRegion <
+                              coverage::CounterMappingRegion::SkippedRegion,
+                      "Not expected order of region kind values");
+        return LHS.Kind < RHS.Kind;
       });
 }
 
 static bool isExpansion(const CountedRegion &R, unsigned FileID) {
   return R.Kind == CounterMappingRegion::ExpansionRegion && R.FileID == FileID;
 }
 
 CoverageData CoverageMapping::getCoverageForFile(StringRef Filename) {
   CoverageData FileCoverage(Filename);

llvm/trunk/unittests/ProfileData/CoverageMappingTest.cpp

TEST_P(MaybeSparseCoverageMappingTest, load_coverage_for_expanded_file) {
 
   CoverageData Data = LoadedCoverage->getCoverageForFile("expanded");
   std::vector<CoverageSegment> Segments(Data.begin(), Data.end());
   ASSERT_EQ(2U, Segments.size());
   EXPECT_EQ(CoverageSegment(1, 1, 10, true), Segments[0]);
   EXPECT_EQ(CoverageSegment(1, 10, false), Segments[1]);
 }
 
+TEST_P(MaybeSparseCoverageMappingTest,
+       expansion_region_uses_count_of_outer_region) {
+  InstrProfRecord Record("func", 0x1234, {10, 20});
+  ProfileWriter.addRecord(std::move(Record));
+  readProfCounts();
+
+  startFunction("func", 0x1234);
+  addCMR(Counter::getCounter(1), "include", 1, 1, 1, 10);
+  addCMR(Counter::getCounter(0), "file", 1, 1, 5, 5);
+  addExpansionCMR("file", "include", 3, 1, 3, 5);
+
+  loadCoverageMapping();
+
+  CoverageData Data = LoadedCoverage->getCoverageForFile("file");
+  std::vector<CoverageSegment> Segments(Data.begin(), Data.end());
+  ASSERT_EQ(4U, Segments.size());
+  EXPECT_EQ(CoverageSegment(1, 1, 10, true), Segments[0]);
+  EXPECT_EQ(CoverageSegment(3, 1, 10, true), Segments[1]);
+  EXPECT_EQ(CoverageSegment(3, 5, 10, false), Segments[2]);
+  EXPECT_EQ(CoverageSegment(5, 5, false), Segments[3]);
+}
+
 INSTANTIATE_TEST_CASE_P(MaybeSparse, MaybeSparseCoverageMappingTest,
                         ::testing::Bool());
 
 } // end anonymous namespace