[Coverage] Prevent creating a redundant counter if a nested body ends with a macro.

[ikudrin]

Summary

If there were several nested statements arranged in a way that all of them end up with the same macro,
then the expansion of this macro was assigned with all the corresponding counters of these statements.
As a result, the wrong counter value was shown for the macro in llvm-cov.

This patch fixes the issue by preventing adding a counter for an expanded source range
if it already has an assigned counter, which is expected to come from the most specific statement.

[ikudrin]

The motivation sample:

$ cat > test.cpp << EOF
void dummy() {}

#define MACRO dummy()

int main()
{
  int i = 0;
  while (i++ < 10)
    if (i < 5)
      MACRO;
  return 0;
}
EOF
$ clang++ -fprofile-instr-generate -fcoverage-mapping dummy.cpp test.cpp
$ ./a.out
$ llvm-profdata merge -o default.profdata default.profraw
$ llvm-cov show a.out -instr-profile --show-expansions default.profdata
test.cpp:
      4|    1|void dummy() {}
       |    2|
     14|    3|#define MACRO dummy()
       |    4|
       |    5|int main()
      1|    6|{
      1|    7|  int i = 0;
     11|    8|  while (i++ < 10)
     10|    9|    if (i < 5)
      4|   10|      MACRO;
  ------------------
  |  |     14|    3|#define MACRO dummy()
  ------------------
      1|   11|  return 0;
      1|   12|}

After applying this patch the counter for MACRO shows a reasonable value:

$ llvm-cov show a.out -instr-profile --show-expansions default.profdata
test.cpp:
      4|    1|void dummy() {}
       |    2|
      4|    3|#define MACRO dummy()
       |    4|
       |    5|int main()
      1|    6|{
      1|    7|  int i = 0;
     11|    8|  while (i++ < 10)
     10|    9|    if (i < 5)
      4|   10|      MACRO;
  ------------------
  |  |      4|    3|#define MACRO dummy()
  ------------------
      1|   11|  return 0;
      1|   12|}

[vsk]

Thanks for catching this! I couldn't really reduce your test case any further. This LGTM.

I guess it never makes sense to have two regions with the exact same start/end loc, and different counters. Do you think we should add assertions in llvm (either in llvm-cov, or in the coverage reader) which guard against this?

[vsk]

(@hans If there are no objections, and if it's still possible, it would be great to see this merged into 3.9 once it has landed.)

[ikudrin]

In D23160#506061, @vsk wrote:

I guess it never makes sense to have two regions with the exact same start/end loc, and different counters. Do you think we should add assertions in llvm (either in llvm-cov, or in the coverage reader) which guard against this?

Right now we can have several ranges with the same start and end locations in case of macro fully expanded into another macro. We consider this situation possible and handle it in class SegmentBuilder in llvm/lib/ProfileData/Coverage/CoverageMapping.cpp. Maybe something has to be fixed here too.

By the way, thinking about your words, I found another example which results in spawning not only redundant but also wrong counters. I'm postponing landing this patch until I finish the investigation.

    4|    1|void dummy() {}
     |    2|
    4|    3|#define M_INT dummy()
     |    4|
   11|    5|#define MACRO M_INT
     |    6|
     |    7|int main()
    1|    8|{
    1|    9|  int i = 0;
   11|   10|  while (i++ < 10)
   10|   11|    if (i < 5)
   10|   12|      MACRO;
------------------
|  |     11|    5|#define MACRO M_INT
|  |  ------------------
|  |  |  |      4|    3|#define M_INT dummy()
|  |  ------------------
------------------
    1|   13|  return 0;
    1|   14|}

[ikudrin]

I've reduced the last sample to the following:

    0|    1|void dummy() {}
     |    2|
    0|    3|#define MACRO_1 dummy()
     |    4|
    1|    5|#define MACRO_2 MACRO_1
     |    6|
    1|    7|int main() {
    1|    8|  int i = 0;
    1|    9|  if (i > 5)
    1|   10|    MACRO_2;
------------------
|  |      1|    5|#define MACRO_2 MACRO_1
|  |  ------------------
|  |  |  |      0|    3|#define MACRO_1 dummy()
|  |  ------------------
------------------
    1|   11|  return 0;
    1|   12|}

Right now, I don't have a good idea how to fix this.

[ikudrin]

I figured out that the last issue is an independent one, so I prepared a separate fix for it: D23987.

lib/CodeGen/CoverageMappingGen.cpp

while (RegionStack.size() > ParentIndex) {
                                     ? Region.getEndLoc()
                                     : RegionStack[ParentIndex].getEndLoc();
         while (!SM.isWrittenInSameFile(StartLoc, EndLoc)) {
           // The region ends in a nested file or macro expansion. Create a
           // separate region for each expansion.
           SourceLocation NestedLoc = getStartOfFileOrMacro(EndLoc);
           assert(SM.isWrittenInSameFile(NestedLoc, EndLoc));
 
+          if (!isRegionAlreadyAdded(NestedLoc, EndLoc))
             SourceRegions.emplace_back(Region.getCounter(), NestedLoc, EndLoc);
 
           EndLoc = getPreciseTokenLocEnd(getIncludeOrExpansionLoc(EndLoc));
           if (EndLoc.isInvalid())
             llvm::report_fatal_error("File exit not handled before popRegions");
         }
         Region.setEndLoc(EndLoc);
 
         MostRecentLocation = EndLoc;

test/CoverageMapping/macros.c

 // CHECK-NEXT: func3
 void func3() { // CHECK-NEXT: File 0, [[@LINE]]:14 -> [[@LINE+3]]:2 = #0
   MACRO_2; // CHECK-NEXT: Expansion,File 0, [[@LINE]]:3 -> [[@LINE]]:10 = #0
   MACRO_2; // CHECK-NEXT: Expansion,File 0, [[@LINE]]:3 -> [[@LINE]]:10 = #0
 }
 // CHECK-NEXT: File 1, 4:17 -> 4:22 = #0
 // CHECK-NEXT: File 2, 4:17 -> 4:22 = #0
 
+// CHECK-NEXT: func4
+void func4() { // CHECK-NEXT: File 0, [[@LINE]]:14 -> [[@LINE+6]]:2 = #0
+  int i = 0;
+  while (i++ < 10) // CHECK-NEXT: File 0, [[@LINE]]:10 -> [[@LINE]]:18 = (#0 + #1)
+    if (i < 5) // CHECK-NEXT: File 0, [[@LINE]]:5 -> [[@LINE+2]]:14 = #1
+               // CHECK-NEXT: File 0, [[@LINE-1]]:9 -> [[@LINE-1]]:14 = #1
+      MACRO_2; // CHECK-NEXT: Expansion,File 0, [[@LINE]]:7 -> [[@LINE]]:14 = #2
+}
+// CHECK-NEXT: File 1, 4:17 -> 4:22 = #2
+// CHECK-NOT: File 1
+
 int main(int argc, const char *argv[]) {
   func();
   func2();
   func3();
+  func4();
 }