[Coverage][Expansion] handle nested macros in scratch space #89869

whentojump · 2024-04-24T04:04:56Z

The problematic program is as follows:

#define pre_a 0
#define PRE(x) pre_##x

void f(void) {
    PRE(a) && 0;
}

int main(void) { return 0; }

in which after token concatenation (##), there's another nested macro pre_a.

Currently only the outer expansion region will be produced. (compiler explorer link)

f:
  File 0, 4:14 -> 6:2 = #0
  Decision,File 0, 5:5 -> 5:16 = M:0, C:2
  Expansion,File 0, 5:5 -> 5:8 = #0 (Expanded file = 1)
  File 0, 5:15 -> 5:16 = #1
  Branch,File 0, 5:15 -> 5:16 = 0, 0 [2,0,0] 
  File 1, 2:16 -> 2:23 = #0
  File 2, 1:15 -> 1:16 = #0
  File 2, 1:15 -> 1:16 = #0
  Branch,File 2, 1:15 -> 1:16 = 0, 0 [1,2,0]

The inner expansion region isn't produced because:

In the range-based for loop quoted below, each sloc is processed and possibly emit a corresponding expansion region.

For our sloc in question, its direct parent returned by getIncludeOrExpansionLoc() is a <scratch space>, because that's how ## is processed.

llvm-project/clang/lib/CodeGen/CoverageMappingGen.cpp

Lines 518 to 520 in 88b6186

    
           for (const auto &FM : FileIDMapping) { 
        
             SourceLocation ExpandedLoc = FM.second.second; 
        
             SourceLocation ParentLoc = getIncludeOrExpansionLoc(ExpandedLoc);

This <scratch space> cannot be found in the FileID mapping so ParentFileID will be assigned an std::nullopt

llvm-project/clang/lib/CodeGen/CoverageMappingGen.cpp

Lines 521 to 526 in 88b6186

    
           if (ParentLoc.isInvalid()) 
        
             continue; 
        
           auto ParentFileID = getCoverageFileID(ParentLoc); 
        
           if (!ParentFileID) 
        
             continue;

As a result this iteration of for loop finishes early and no expansion region is added for the sloc.

This problem gets worse with MC/DC: as the example shows, there's a branch from File 2 but File 2 itself is missing. This will trigger assertion failures.

The fix is more or less a workaround and takes a similar approach as #89573.

~~Depends on #89573.~~ This includes #89573. Kudos to @chapuni!
This and #89573 together fix #87000: I tested locally, both the reduced program and my original use case (fwiw, Linux kernel) can run successfully.

github-actions · 2024-04-24T04:07:52Z

✅ With the latest revision this PR passed the C/C++ code formatter.

llvmbot · 2024-04-24T06:18:29Z

@llvm/pr-subscribers-clang-codegen

Author: Wentao Zhang (whentojump)

Changes

The problematic program is as follows:

#define pre_a 0
#define PRE(x) pre_##x

void f(void) {
    PRE(a) &amp;&amp; 0;
}

int main(void) { return 0; }

in which after token concatenation (##), there's another nested macro pre_a.

Currently only the outer expansion region will be produced. (compiler explorer link)

f:
  File 0, 4:14 -&gt; 6:2 = #<!-- -->0
  Decision,File 0, 5:5 -&gt; 5:16 = M:0, C:2
  Expansion,File 0, 5:5 -&gt; 5:8 = #<!-- -->0 (Expanded file = 1)
  File 0, 5:15 -&gt; 5:16 = #<!-- -->1
  Branch,File 0, 5:15 -&gt; 5:16 = 0, 0 [2,0,0] 
  File 1, 2:16 -&gt; 2:23 = #<!-- -->0
  File 2, 1:15 -&gt; 1:16 = #<!-- -->0
  File 2, 1:15 -&gt; 1:16 = #<!-- -->0
  Branch,File 2, 1:15 -&gt; 1:16 = 0, 0 [1,2,0]

The inner expansion region isn't produced because:

In the range-based for loop quoted below, each sloc is processed and possibly emit a corresponding expansion region.

For our sloc in question, its direct parent returned by getIncludeOrExpansionLoc() is a <scratch space>, because that's how ## is processed.

llvm-project/clang/lib/CodeGen/CoverageMappingGen.cpp

Lines 518 to 520 in 88b6186

    
           for (const auto &FM : FileIDMapping) { 
        
             SourceLocation ExpandedLoc = FM.second.second; 
        
             SourceLocation ParentLoc = getIncludeOrExpansionLoc(ExpandedLoc);

This <scratch space> cannot be found in the FileID mapping so ParentFileID will be assigned an std::nullopt

llvm-project/clang/lib/CodeGen/CoverageMappingGen.cpp

Lines 521 to 526 in 88b6186

    
           if (ParentLoc.isInvalid()) 
        
             continue; 
        
           auto ParentFileID = getCoverageFileID(ParentLoc); 
        
           if (!ParentFileID) 
        
             continue;

As a result this iteration of for loop finishes early and no expansion region is added for the sloc.

This problem gets worse with MC/DC: as the example shows, there's a branch from File 2 but File 2 itself is missing. This will trigger assertion failures.

The fix is more or less a workaround and takes a similar approach as #89573.

Depends on #89573.
This and #89573 together fix #87000.

Full diff: https://github.com/llvm/llvm-project/pull/89869.diff

4 Files Affected:

(modified) clang/lib/CodeGen/CoverageMappingGen.cpp (+29-2)
(modified) clang/test/CoverageMapping/builtinmacro.c (+1-1)
(modified) clang/test/CoverageMapping/macros.c (+5-3)
(added) clang/test/CoverageMapping/mcdc-scratch-space.c (+65)

diff --git a/clang/lib/CodeGen/CoverageMappingGen.cpp b/clang/lib/CodeGen/CoverageMappingGen.cpp
index 733686d4946b3c..9268ac5b4c61a3 100644
--- a/clang/lib/CodeGen/CoverageMappingGen.cpp
+++ b/clang/lib/CodeGen/CoverageMappingGen.cpp
@@ -298,6 +298,22 @@ class CoverageMappingBuilder {
                            : SM.getIncludeLoc(SM.getFileID(Loc));
   }
 
+  /// Find out where the current file is included or macro is expanded. If the
+  /// found expansion is a <scratch space>, keep looking.
+  SourceLocation getIncludeOrNonScratchExpansionLoc(SourceLocation Loc) {
+    if (Loc.isMacroID()) {
+      Loc = SM.getImmediateExpansionRange(Loc).getBegin();
+      while (Loc.isMacroID() &&
+             SM.isWrittenInScratchSpace(SM.getSpellingLoc(Loc))) {
+        auto ExpansionRange = SM.getImmediateExpansionRange(Loc);
+        Loc = ExpansionRange.getBegin();
+      }
+    } else {
+      Loc = SM.getIncludeLoc(SM.getFileID(Loc));
+    }
+    return Loc;
+  }
+
   /// Return true if \c Loc is a location in a built-in macro.
   bool isInBuiltin(SourceLocation Loc) {
     return SM.getBufferName(SM.getSpellingLoc(Loc)) == "<built-in>";
@@ -339,8 +355,18 @@ class CoverageMappingBuilder {
 
     llvm::SmallSet<FileID, 8> Visited;
     SmallVector<std::pair<SourceLocation, unsigned>, 8> FileLocs;
-    for (const auto &Region : SourceRegions) {
+    for (auto &Region : SourceRegions) {
       SourceLocation Loc = Region.getBeginLoc();
+
+      // Replace Region with its definition if it is in <scratch space>.
+      while (Loc.isMacroID() &&
+             SM.isWrittenInScratchSpace(SM.getSpellingLoc(Loc))) {
+        auto ExpansionRange = SM.getImmediateExpansionRange(Loc);
+        Loc = ExpansionRange.getBegin();
+        Region.setStartLoc(Loc);
+        Region.setEndLoc(ExpansionRange.getEnd());
+      }
+
       FileID File = SM.getFileID(Loc);
       if (!Visited.insert(File).second)
         continue;
@@ -517,7 +543,8 @@ class CoverageMappingBuilder {
     SourceRegionFilter Filter;
     for (const auto &FM : FileIDMapping) {
       SourceLocation ExpandedLoc = FM.second.second;
-      SourceLocation ParentLoc = getIncludeOrExpansionLoc(ExpandedLoc);
+      SourceLocation ParentLoc =
+          getIncludeOrNonScratchExpansionLoc(ExpandedLoc);
       if (ParentLoc.isInvalid())
         continue;
 
diff --git a/clang/test/CoverageMapping/builtinmacro.c b/clang/test/CoverageMapping/builtinmacro.c
index abcdc191523a5d..5d5a176aa7d87e 100644
--- a/clang/test/CoverageMapping/builtinmacro.c
+++ b/clang/test/CoverageMapping/builtinmacro.c
@@ -4,7 +4,7 @@
 
 // CHECK: filename
 const char *filename (const char *name) { // CHECK-NEXT: File 0, [[@LINE]]:41 -> [[@LINE+3]]:2 = #0
-  static const char this_file[] = __FILE__;
+  static const char this_file[] = __FILE__; // CHECK-NEXT: File 0, [[@LINE]]:35 -> [[@LINE]]:35 = #0
   return this_file;
 }
 
diff --git a/clang/test/CoverageMapping/macros.c b/clang/test/CoverageMapping/macros.c
index 6bd3be434139a2..fcf21170ef135c 100644
--- a/clang/test/CoverageMapping/macros.c
+++ b/clang/test/CoverageMapping/macros.c
@@ -80,12 +80,14 @@ void func7(void) { // CHECK-NEXT: File 0, [[@LINE]]:18 -> [[@LINE+6]]:2 = #0
   int kk,ll;       // CHECK-NEXT: File 0, [[@LINE+1]]:7 -> [[@LINE+1]]:8 = #0
   if (k)           // CHECK-NEXT: Branch,File 0, [[@LINE]]:7 -> [[@LINE]]:8 = #1
     m(k);          // CHECK-NEXT: Gap,File 0, [[@LINE-1]]:9 -> [[@LINE]]:5 = #1
-  else             // CHECK-NEXT: Expansion,File 0, [[@LINE-1]]:5 -> [[@LINE-1]]:6 = #0
+  else             // CHECK-NEXT: Expansion,File 0, [[@LINE-1]]:5 -> [[@LINE-1]]:6 = #1
     l = m(l);      // CHECK-NEXT: Gap,File 0, [[@LINE-2]]:7 -> [[@LINE]]:5 = (#0 - #1)
 }                  // CHECK-NEXT: File 0, [[@LINE-1]]:5 -> [[@LINE-1]]:10 = (#0 - #1)
                    // CHECK-NEXT: Expansion,File 0, [[@LINE-2]]:9 -> [[@LINE-2]]:10 = (#0 - #1)
-                   // CHECK-NEXT: File 1, [[@LINE-9]]:14 -> [[@LINE-9]]:18 = #0
-                   // CHECK-NEXT: File 2, [[@LINE-10]]:14 -> [[@LINE-10]]:15 = (#0 - #1)
+                   // CHECK-NEXT: File 1, [[@LINE-9]]:14 -> [[@LINE-9]]:17 = #1
+                   // CHECK-NEXT: File 1, [[@LINE-10]]:14 -> [[@LINE-10]]:18 = #0
+                   // CHECK-NEXT: File 2, [[@LINE-11]]:14 -> [[@LINE-11]]:17 = (#0 - #1)
+                   // CHECK-NEXT: File 2, [[@LINE-12]]:14 -> [[@LINE-12]]:15 = (#0 - #1)
 
 int main(int argc, const char *argv[]) {
   func();
diff --git a/clang/test/CoverageMapping/mcdc-scratch-space.c b/clang/test/CoverageMapping/mcdc-scratch-space.c
new file mode 100644
index 00000000000000..2b5b12d9dcad65
--- /dev/null
+++ b/clang/test/CoverageMapping/mcdc-scratch-space.c
@@ -0,0 +1,65 @@
+// RUN: %clang_cc1 -triple %itanium_abi_triple -std=c99 -fcoverage-mcdc -fprofile-instrument=clang -fcoverage-mapping -dump-coverage-mapping -emit-llvm-only %s | FileCheck %s
+
+// CHECK: builtin_macro0:
+int builtin_macro0(int a) {
+  // CHECK: Decision,File 0, [[@LINE+1]]:11 -> [[@LINE+2]]:15 = M:0, C:2
+  return (__LINE__ // CHECK: Branch,File 0, [[@LINE]]:11 -> [[@LINE]]:11 = 0, 0 [1,2,0]
+          && a); //   CHECK: Branch,File 0, [[@LINE]]:14 -> [[@LINE]]:15 = #2, (#1 - #2) [2,0,0]
+}
+
+// CHECK: builtin_macro1:
+int builtin_macro1(int a) {
+  // CHECK: Decision,File 0, [[@LINE+1]]:11 -> [[@LINE+2]]:22 = M:0, C:2
+  return (a // CHECK: Branch,File 0, [[@LINE]]:11 -> [[@LINE]]:12 = (#0 - #1), #1 [1,0,2]
+          || __LINE__); // CHECK: Branch,File 0, [[@LINE]]:14 -> [[@LINE]]:14 = 0, 0 [2,0,0]
+}
+
+#define PRE(x) pre_##x
+
+// CHECK: pre0:
+int pre0(int pre_a, int b_post) {
+  // CHECK: Decision,File 0, [[@LINE+2]]:11 -> [[@LINE+3]]:20 = M:0, C:2
+  // CHECK: Expansion,File 0, [[@LINE+1]]:11 -> [[@LINE+1]]:14 = #0 (Expanded file = 1)
+  return (PRE(a)
+          && b_post);
+  // CHECK: Branch,File 0, [[@LINE-1]]:14 -> [[@LINE-1]]:20 = #2, (#1 - #2) [2,0,0]
+  // CHECK: Branch,File 1, [[@LINE-9]]:16 -> [[@LINE-9]]:22 = #1, (#0 - #1) [1,2,0]
+}
+
+#define pre_foo pre_a
+
+// CHECK: pre1:
+int pre1(int pre_a, int b_post) {
+  // CHECK: Decision,File 0, [[@LINE+3]]:11 -> [[@LINE+4]]:20 = M:0, C:2
+  // CHECK: Expansion,File 0, [[@LINE+2]]:11 -> [[@LINE+2]]:14 = #0 (Expanded file = 1)
+  // CHECK: Branch,File 0, [[@LINE+2]]:14 -> [[@LINE+2]]:20 = #2, (#1 - #2) [2,0,0]
+  return (PRE(foo)
+          && b_post);
+  // CHECK: Expansion,File 1, 17:16 -> 17:20 = #0 (Expanded file = 2)
+  // CHECK: Branch,File 2, 29:17 -> 29:22 = #1, (#0 - #1) [1,2,0]
+}
+
+#define POST(x) x##_post
+
+// CHECK: post0:
+int post0(int pre_a, int b_post) {
+  // CHECK: Decision,File 0, [[@LINE+2]]:11 -> [[@LINE+3]]:18 = M:0, C:2
+  // CHECK: Branch,File 0, [[@LINE+1]]:11 -> [[@LINE+1]]:16 = (#0 - #1), #1 [1,0,2]
+  return (pre_a
+          || POST(b));
+  // CHECK: Expansion,File 0, [[@LINE-1]]:14 -> [[@LINE-1]]:18 = #1 (Expanded file = 1)
+  // CHECK: Branch,File 1, [[@LINE-9]]:17 -> [[@LINE-9]]:20 = (#1 - #2), #2 [2,0,0]
+}
+
+#define bar_post b_post
+
+// CHECK: post1:
+int post1(int pre_a, int b_post) {
+  // CHECK: Decision,File 0, [[@LINE+3]]:11 -> [[@LINE+4]]:18 = M:0, C:2
+  // CHECK: Branch,File 0, [[@LINE+2]]:11 -> [[@LINE+2]]:16 = (#0 - #1), #1 [1,0,2]
+  // CHECK: Expansion,File 0, [[@LINE+2]]:14 -> [[@LINE+2]]:18 = 0 (Expanded file = 1)
+  return (pre_a
+          || POST(bar));
+  // CHECK: Expansion,File 1, 42:17 -> 42:18 = #1 (Expanded file = 2)
+  // CHECK: Branch,File 2, 54:18 -> 54:24 = (#1 - #2), #2 [2,0,0]
+}

llvmbot · 2024-04-24T06:18:30Z

@llvm/pr-subscribers-clang

Author: Wentao Zhang (whentojump)

Changes

The problematic program is as follows:

#define pre_a 0
#define PRE(x) pre_##x

void f(void) {
    PRE(a) &amp;&amp; 0;
}

int main(void) { return 0; }

in which after token concatenation (##), there's another nested macro pre_a.

Currently only the outer expansion region will be produced. (compiler explorer link)

f:
  File 0, 4:14 -&gt; 6:2 = #<!-- -->0
  Decision,File 0, 5:5 -&gt; 5:16 = M:0, C:2
  Expansion,File 0, 5:5 -&gt; 5:8 = #<!-- -->0 (Expanded file = 1)
  File 0, 5:15 -&gt; 5:16 = #<!-- -->1
  Branch,File 0, 5:15 -&gt; 5:16 = 0, 0 [2,0,0] 
  File 1, 2:16 -&gt; 2:23 = #<!-- -->0
  File 2, 1:15 -&gt; 1:16 = #<!-- -->0
  File 2, 1:15 -&gt; 1:16 = #<!-- -->0
  Branch,File 2, 1:15 -&gt; 1:16 = 0, 0 [1,2,0]

The inner expansion region isn't produced because:

In the range-based for loop quoted below, each sloc is processed and possibly emit a corresponding expansion region.

For our sloc in question, its direct parent returned by getIncludeOrExpansionLoc() is a <scratch space>, because that's how ## is processed.

llvm-project/clang/lib/CodeGen/CoverageMappingGen.cpp

Lines 518 to 520 in 88b6186

    
           for (const auto &FM : FileIDMapping) { 
        
             SourceLocation ExpandedLoc = FM.second.second; 
        
             SourceLocation ParentLoc = getIncludeOrExpansionLoc(ExpandedLoc);

This <scratch space> cannot be found in the FileID mapping so ParentFileID will be assigned an std::nullopt

llvm-project/clang/lib/CodeGen/CoverageMappingGen.cpp

Lines 521 to 526 in 88b6186

    
           if (ParentLoc.isInvalid()) 
        
             continue; 
        
           auto ParentFileID = getCoverageFileID(ParentLoc); 
        
           if (!ParentFileID) 
        
             continue;

As a result this iteration of for loop finishes early and no expansion region is added for the sloc.

This problem gets worse with MC/DC: as the example shows, there's a branch from File 2 but File 2 itself is missing. This will trigger assertion failures.

The fix is more or less a workaround and takes a similar approach as #89573.

Depends on #89573.
This and #89573 together fix #87000.

Full diff: https://github.com/llvm/llvm-project/pull/89869.diff

4 Files Affected:

(modified) clang/lib/CodeGen/CoverageMappingGen.cpp (+29-2)
(modified) clang/test/CoverageMapping/builtinmacro.c (+1-1)
(modified) clang/test/CoverageMapping/macros.c (+5-3)
(added) clang/test/CoverageMapping/mcdc-scratch-space.c (+65)

diff --git a/clang/lib/CodeGen/CoverageMappingGen.cpp b/clang/lib/CodeGen/CoverageMappingGen.cpp
index 733686d4946b3c..9268ac5b4c61a3 100644
--- a/clang/lib/CodeGen/CoverageMappingGen.cpp
+++ b/clang/lib/CodeGen/CoverageMappingGen.cpp
@@ -298,6 +298,22 @@ class CoverageMappingBuilder {
                            : SM.getIncludeLoc(SM.getFileID(Loc));
   }
 
+  /// Find out where the current file is included or macro is expanded. If the
+  /// found expansion is a <scratch space>, keep looking.
+  SourceLocation getIncludeOrNonScratchExpansionLoc(SourceLocation Loc) {
+    if (Loc.isMacroID()) {
+      Loc = SM.getImmediateExpansionRange(Loc).getBegin();
+      while (Loc.isMacroID() &&
+             SM.isWrittenInScratchSpace(SM.getSpellingLoc(Loc))) {
+        auto ExpansionRange = SM.getImmediateExpansionRange(Loc);
+        Loc = ExpansionRange.getBegin();
+      }
+    } else {
+      Loc = SM.getIncludeLoc(SM.getFileID(Loc));
+    }
+    return Loc;
+  }
+
   /// Return true if \c Loc is a location in a built-in macro.
   bool isInBuiltin(SourceLocation Loc) {
     return SM.getBufferName(SM.getSpellingLoc(Loc)) == "<built-in>";
@@ -339,8 +355,18 @@ class CoverageMappingBuilder {
 
     llvm::SmallSet<FileID, 8> Visited;
     SmallVector<std::pair<SourceLocation, unsigned>, 8> FileLocs;
-    for (const auto &Region : SourceRegions) {
+    for (auto &Region : SourceRegions) {
       SourceLocation Loc = Region.getBeginLoc();
+
+      // Replace Region with its definition if it is in <scratch space>.
+      while (Loc.isMacroID() &&
+             SM.isWrittenInScratchSpace(SM.getSpellingLoc(Loc))) {
+        auto ExpansionRange = SM.getImmediateExpansionRange(Loc);
+        Loc = ExpansionRange.getBegin();
+        Region.setStartLoc(Loc);
+        Region.setEndLoc(ExpansionRange.getEnd());
+      }
+
       FileID File = SM.getFileID(Loc);
       if (!Visited.insert(File).second)
         continue;
@@ -517,7 +543,8 @@ class CoverageMappingBuilder {
     SourceRegionFilter Filter;
     for (const auto &FM : FileIDMapping) {
       SourceLocation ExpandedLoc = FM.second.second;
-      SourceLocation ParentLoc = getIncludeOrExpansionLoc(ExpandedLoc);
+      SourceLocation ParentLoc =
+          getIncludeOrNonScratchExpansionLoc(ExpandedLoc);
       if (ParentLoc.isInvalid())
         continue;
 
diff --git a/clang/test/CoverageMapping/builtinmacro.c b/clang/test/CoverageMapping/builtinmacro.c
index abcdc191523a5d..5d5a176aa7d87e 100644
--- a/clang/test/CoverageMapping/builtinmacro.c
+++ b/clang/test/CoverageMapping/builtinmacro.c
@@ -4,7 +4,7 @@
 
 // CHECK: filename
 const char *filename (const char *name) { // CHECK-NEXT: File 0, [[@LINE]]:41 -> [[@LINE+3]]:2 = #0
-  static const char this_file[] = __FILE__;
+  static const char this_file[] = __FILE__; // CHECK-NEXT: File 0, [[@LINE]]:35 -> [[@LINE]]:35 = #0
   return this_file;
 }
 
diff --git a/clang/test/CoverageMapping/macros.c b/clang/test/CoverageMapping/macros.c
index 6bd3be434139a2..fcf21170ef135c 100644
--- a/clang/test/CoverageMapping/macros.c
+++ b/clang/test/CoverageMapping/macros.c
@@ -80,12 +80,14 @@ void func7(void) { // CHECK-NEXT: File 0, [[@LINE]]:18 -> [[@LINE+6]]:2 = #0
   int kk,ll;       // CHECK-NEXT: File 0, [[@LINE+1]]:7 -> [[@LINE+1]]:8 = #0
   if (k)           // CHECK-NEXT: Branch,File 0, [[@LINE]]:7 -> [[@LINE]]:8 = #1
     m(k);          // CHECK-NEXT: Gap,File 0, [[@LINE-1]]:9 -> [[@LINE]]:5 = #1
-  else             // CHECK-NEXT: Expansion,File 0, [[@LINE-1]]:5 -> [[@LINE-1]]:6 = #0
+  else             // CHECK-NEXT: Expansion,File 0, [[@LINE-1]]:5 -> [[@LINE-1]]:6 = #1
     l = m(l);      // CHECK-NEXT: Gap,File 0, [[@LINE-2]]:7 -> [[@LINE]]:5 = (#0 - #1)
 }                  // CHECK-NEXT: File 0, [[@LINE-1]]:5 -> [[@LINE-1]]:10 = (#0 - #1)
                    // CHECK-NEXT: Expansion,File 0, [[@LINE-2]]:9 -> [[@LINE-2]]:10 = (#0 - #1)
-                   // CHECK-NEXT: File 1, [[@LINE-9]]:14 -> [[@LINE-9]]:18 = #0
-                   // CHECK-NEXT: File 2, [[@LINE-10]]:14 -> [[@LINE-10]]:15 = (#0 - #1)
+                   // CHECK-NEXT: File 1, [[@LINE-9]]:14 -> [[@LINE-9]]:17 = #1
+                   // CHECK-NEXT: File 1, [[@LINE-10]]:14 -> [[@LINE-10]]:18 = #0
+                   // CHECK-NEXT: File 2, [[@LINE-11]]:14 -> [[@LINE-11]]:17 = (#0 - #1)
+                   // CHECK-NEXT: File 2, [[@LINE-12]]:14 -> [[@LINE-12]]:15 = (#0 - #1)
 
 int main(int argc, const char *argv[]) {
   func();
diff --git a/clang/test/CoverageMapping/mcdc-scratch-space.c b/clang/test/CoverageMapping/mcdc-scratch-space.c
new file mode 100644
index 00000000000000..2b5b12d9dcad65
--- /dev/null
+++ b/clang/test/CoverageMapping/mcdc-scratch-space.c
@@ -0,0 +1,65 @@
+// RUN: %clang_cc1 -triple %itanium_abi_triple -std=c99 -fcoverage-mcdc -fprofile-instrument=clang -fcoverage-mapping -dump-coverage-mapping -emit-llvm-only %s | FileCheck %s
+
+// CHECK: builtin_macro0:
+int builtin_macro0(int a) {
+  // CHECK: Decision,File 0, [[@LINE+1]]:11 -> [[@LINE+2]]:15 = M:0, C:2
+  return (__LINE__ // CHECK: Branch,File 0, [[@LINE]]:11 -> [[@LINE]]:11 = 0, 0 [1,2,0]
+          && a); //   CHECK: Branch,File 0, [[@LINE]]:14 -> [[@LINE]]:15 = #2, (#1 - #2) [2,0,0]
+}
+
+// CHECK: builtin_macro1:
+int builtin_macro1(int a) {
+  // CHECK: Decision,File 0, [[@LINE+1]]:11 -> [[@LINE+2]]:22 = M:0, C:2
+  return (a // CHECK: Branch,File 0, [[@LINE]]:11 -> [[@LINE]]:12 = (#0 - #1), #1 [1,0,2]
+          || __LINE__); // CHECK: Branch,File 0, [[@LINE]]:14 -> [[@LINE]]:14 = 0, 0 [2,0,0]
+}
+
+#define PRE(x) pre_##x
+
+// CHECK: pre0:
+int pre0(int pre_a, int b_post) {
+  // CHECK: Decision,File 0, [[@LINE+2]]:11 -> [[@LINE+3]]:20 = M:0, C:2
+  // CHECK: Expansion,File 0, [[@LINE+1]]:11 -> [[@LINE+1]]:14 = #0 (Expanded file = 1)
+  return (PRE(a)
+          && b_post);
+  // CHECK: Branch,File 0, [[@LINE-1]]:14 -> [[@LINE-1]]:20 = #2, (#1 - #2) [2,0,0]
+  // CHECK: Branch,File 1, [[@LINE-9]]:16 -> [[@LINE-9]]:22 = #1, (#0 - #1) [1,2,0]
+}
+
+#define pre_foo pre_a
+
+// CHECK: pre1:
+int pre1(int pre_a, int b_post) {
+  // CHECK: Decision,File 0, [[@LINE+3]]:11 -> [[@LINE+4]]:20 = M:0, C:2
+  // CHECK: Expansion,File 0, [[@LINE+2]]:11 -> [[@LINE+2]]:14 = #0 (Expanded file = 1)
+  // CHECK: Branch,File 0, [[@LINE+2]]:14 -> [[@LINE+2]]:20 = #2, (#1 - #2) [2,0,0]
+  return (PRE(foo)
+          && b_post);
+  // CHECK: Expansion,File 1, 17:16 -> 17:20 = #0 (Expanded file = 2)
+  // CHECK: Branch,File 2, 29:17 -> 29:22 = #1, (#0 - #1) [1,2,0]
+}
+
+#define POST(x) x##_post
+
+// CHECK: post0:
+int post0(int pre_a, int b_post) {
+  // CHECK: Decision,File 0, [[@LINE+2]]:11 -> [[@LINE+3]]:18 = M:0, C:2
+  // CHECK: Branch,File 0, [[@LINE+1]]:11 -> [[@LINE+1]]:16 = (#0 - #1), #1 [1,0,2]
+  return (pre_a
+          || POST(b));
+  // CHECK: Expansion,File 0, [[@LINE-1]]:14 -> [[@LINE-1]]:18 = #1 (Expanded file = 1)
+  // CHECK: Branch,File 1, [[@LINE-9]]:17 -> [[@LINE-9]]:20 = (#1 - #2), #2 [2,0,0]
+}
+
+#define bar_post b_post
+
+// CHECK: post1:
+int post1(int pre_a, int b_post) {
+  // CHECK: Decision,File 0, [[@LINE+3]]:11 -> [[@LINE+4]]:18 = M:0, C:2
+  // CHECK: Branch,File 0, [[@LINE+2]]:11 -> [[@LINE+2]]:16 = (#0 - #1), #1 [1,0,2]
+  // CHECK: Expansion,File 0, [[@LINE+2]]:14 -> [[@LINE+2]]:18 = 0 (Expanded file = 1)
+  return (pre_a
+          || POST(bar));
+  // CHECK: Expansion,File 1, 42:17 -> 42:18 = #1 (Expanded file = 2)
+  // CHECK: Branch,File 2, 54:18 -> 54:24 = (#1 - #2), #2 [2,0,0]
+}

chapuni

I've confirmed this works. I'd like to wait for other opinions.

My comments are just my preferences.

clang/lib/CodeGen/CoverageMappingGen.cpp

chapuni

They could be simpler if SourceRegion were compatible to std::pair<Loc,Loc>.
(I don't require you to work)

clang/lib/CodeGen/CoverageMappingGen.cpp

chapuni · 2024-04-25T05:02:30Z

clang/lib/CodeGen/CoverageMappingGen.cpp

+      EndLoc = ExpansionRange.getEnd();
+    }
+    if (EndLoc.has_value())
+      return SourceRange(Loc, EndLoc.value());


I wrongly supposed isWrittenInScratchSpace() would return SourceRange. Actually returns CharSourceRange.

You may rewind them with your previous change, if you prefer. The current implementation is not clean a bit.

Actually returns CharSourceRange.

True. Ideally we can circulate around CharSourceRange without converting back and forth too much but I found it hard to make both two callers clean and straightforward.

You may rewind them

I can do that, meanwhile I'll also seek if there's a better way

I meant;

auto getNonScratchExpansionLoc(SourceLocation Loc) { std::optional<CharSourceRange> ExpansionRange = std::nullopt; while (Loc.isMacroID() && SM.isWrittenInScratchSpace(SM.getSpellingLoc(Loc))) { ExpansionRange = SM.getImmediateExpansionRange(Loc); Loc = ExpansionRange->getBegin(); } return ExpansionRange; }

(No need to follow)

clang/lib/CodeGen/CoverageMappingGen.cpp

chapuni · 2024-05-21T21:22:37Z

@whentojump Any updates? Or Is it better for me to take this over?

whentojump · 2024-05-22T13:42:37Z

Sorry for getting back this late.

I reverted the latest commit (using SourceRange) and addressed the earlier comment about updating region sloc only when they are changed.

And yes if you have better ideas about getNonScratchExpansion implementation you can please post a new PR.

clang/lib/CodeGen/CoverageMappingGen.cpp

clang/test/CoverageMapping/mcdc-scratch-space.c

A synthesized identifier with `##` is emitted to `<scratch space>`. `llvm-cov` cannot handle `<scratch space> since it doesn't have actual files. As a workaround, peel `<scratch space>` to the actual definition if the definition is present. This affects predefined built-in macros, like __LINE__. Fixes llvm#87000

…by a flag

…en needed

… only when needed" This reverts commit 0eeb0d3e392b1ea1ae47bc8c977eb709cd1c264c.

chapuni

I'd like other approvals by clang guys. @AaronBallman Could you nominate anyone?

hanickadot

lgtm

whentojump · 2024-05-24T03:24:17Z

Thanks everyone!

I notice that #87000 has been tagged with release milestone. I can handle the backport there. Maybe in a few days? Or sooner if that's needed.

chapuni · 2024-05-24T05:19:28Z

@whentojump AFAIK, both of them missed boarding 18.1.6. I'll wait for opportunities.

@chapuni

The problematic program is as follows: ```shell void f(void) { PRE(a) && 0; } int main(void) { return 0; } ``` in which after token concatenation (`##`), there's another nested macro `pre_a`. Currently only the outer expansion region will be produced. ([compiler explorer link](https://godbolt.org/#g:!((g:!((g:!((h:codeEditor,i:(filename:'1',fontScale:14,fontUsePx:'0',j:1,lang:___c,selection:(endColumn:29,endLineNumber:8,positionColumn:29,positionLineNumber:8,selectionStartColumn:29,selectionStartLineNumber:8,startColumn:29,startLineNumber:8),source:'%23define+pre_a+0%0A%23define+PRE(x)+pre_%23%23x%0A%0Avoid+f(void)+%7B%0A++++PRE(a)+%26%26+0%3B%0A%7D%0A%0Aint+main(void)+%7B+return+0%3B+%7D'),l:'5',n:'0',o:'C+source+%231',t:'0')),k:51.69491525423727,l:'4',n:'0',o:'',s:0,t:'0'),(g:!((g:!((h:compiler,i:(compiler:cclang_assertions_trunk,filters:(b:'0',binary:'1',binaryObject:'1',commentOnly:'0',debugCalls:'1',demangle:'0',directives:'0',execute:'0',intel:'0',libraryCode:'1',trim:'1',verboseDemangling:'0'),flagsViewOpen:'1',fontScale:14,fontUsePx:'0',j:2,lang:___c,libs:!(),options:'-fprofile-instr-generate+-fcoverage-mapping+-fcoverage-mcdc+-Xclang+-dump-coverage-mapping+',overrides:!(),selection:(endColumn:1,endLineNumber:1,positionColumn:1,positionLineNumber:1,selectionStartColumn:1,selectionStartLineNumber:1,startColumn:1,startLineNumber:1),source:1),l:'5',n:'0',o:'+x86-64+clang+(assertions+trunk)+(Editor+%231)',t:'0')),k:34.5741843594503,l:'4',m:28.903654485049834,n:'0',o:'',s:0,t:'0'),(g:!((h:output,i:(compilerName:'x86-64+clang+(trunk)',editorid:1,fontScale:14,fontUsePx:'0',j:2,wrap:'1'),l:'5',n:'0',o:'Output+of+x86-64+clang+(assertions+trunk)+(Compiler+%232)',t:'0')),header:(),l:'4',m:71.09634551495017,n:'0',o:'',s:0,t:'0')),k:48.30508474576271,l:'3',n:'0',o:'',t:'0')),l:'2',m:100,n:'0',o:'',t:'0')),version:4)) ```text f: File 0, 4:14 -> 6:2 = #0 Decision,File 0, 5:5 -> 5:16 = M:0, C:2 Expansion,File 0, 5:5 -> 5:8 = #0 (Expanded file = 1) File 0, 5:15 -> 5:16 = llvm#1 Branch,File 0, 5:15 -> 5:16 = 0, 0 [2,0,0] File 1, 2:16 -> 2:23 = #0 File 2, 1:15 -> 1:16 = #0 File 2, 1:15 -> 1:16 = #0 Branch,File 2, 1:15 -> 1:16 = 0, 0 [1,2,0] ``` The inner expansion region isn't produced because: 1. In the range-based for loop quoted below, each sloc is processed and possibly emit a corresponding expansion region. 2. For our sloc in question, its direct parent returned by `getIncludeOrExpansionLoc()` is a `<scratch space>`, because that's how `##` is processed. https://github.com/llvm/llvm-project/blob/88b6186af3908c55b357858eb348b5143f21c289/clang/lib/CodeGen/CoverageMappingGen.cpp#L518-L520 3. This `<scratch space>` cannot be found in the FileID mapping so `ParentFileID` will be assigned an `std::nullopt` https://github.com/llvm/llvm-project/blob/88b6186af3908c55b357858eb348b5143f21c289/clang/lib/CodeGen/CoverageMappingGen.cpp#L521-L526 4. As a result this iteration of for loop finishes early and no expansion region is added for the sloc. This problem gets worse with MC/DC: as the example shows, there's a branch from File 2 but File 2 itself is missing. This will trigger assertion failures. The fix is more or less a workaround and takes a similar approach as ~~Depends on llvm#89573.~~ This includes llvm#89573. Kudos to @chapuni! This and llvm#89573 together fix llvm#87000: I tested locally, both the reduced program and my original use case (fwiw, Linux kernel) can run successfully. --------- Co-authored-by: NAKAMURA Takumi <geek4civic@gmail.com> llvmorg-19-init-12274-gf9e9e599c013

whentojump mentioned this pull request Apr 24, 2024

[MC/DC][Coverage] Workaround for ## conditions #89573

Closed

whentojump force-pushed the test-chapuni branch from 2010fb3 to ee6c8b6 Compare April 24, 2024 04:12

whentojump marked this pull request as ready for review April 24, 2024 06:18

llvmbot added clang Clang issues not falling into any other category clang:codegen labels Apr 24, 2024

whentojump requested review from chapuni, AaronBallman, ornata and evodius96 April 24, 2024 07:25

chapuni reviewed Apr 24, 2024

View reviewed changes

clang/lib/CodeGen/CoverageMappingGen.cpp Outdated Show resolved Hide resolved

clang/lib/CodeGen/CoverageMappingGen.cpp Outdated Show resolved Hide resolved

chapuni reviewed Apr 24, 2024

View reviewed changes

clang/lib/CodeGen/CoverageMappingGen.cpp Outdated Show resolved Hide resolved

whentojump force-pushed the test-chapuni branch from 978e311 to 93ace1a Compare April 24, 2024 21:28

chapuni reviewed Apr 24, 2024

View reviewed changes

clang/lib/CodeGen/CoverageMappingGen.cpp Outdated Show resolved Hide resolved

clang/lib/CodeGen/CoverageMappingGen.cpp Show resolved Hide resolved

chapuni reviewed Apr 25, 2024

View reviewed changes

whentojump mentioned this pull request May 15, 2024

[llvm-cov][MC/DC] "Branch not found in Decisions" when handling variadic macros #92216

Open

ornata reviewed May 20, 2024

View reviewed changes

clang/lib/CodeGen/CoverageMappingGen.cpp Show resolved Hide resolved

chapuni reviewed May 22, 2024

View reviewed changes

clang/lib/CodeGen/CoverageMappingGen.cpp Show resolved Hide resolved

chapuni reviewed May 22, 2024

View reviewed changes

clang/lib/CodeGen/CoverageMappingGen.cpp Outdated Show resolved Hide resolved

chapuni reviewed May 23, 2024

View reviewed changes

clang/test/CoverageMapping/mcdc-scratch-space.c Show resolved Hide resolved

chapuni and others added 6 commits May 23, 2024 09:58

[Coverage][Expansion] handle nested macros in scratch space

0c22a24

address comments: one getIncludeOrExpansionLoc() function controlled …

17b6484

…by a flag

address comments: extract common code

7250b77

address comments: return early

819103d

address comments: return optional<SourceRange>; update region only wh…

14e52c3

…en needed

whentojump added 2 commits May 23, 2024 09:58

Revert "address comments: return optional<SourceRange>; update region…

108a9ac

… only when needed" This reverts commit 0eeb0d3e392b1ea1ae47bc8c977eb709cd1c264c.

update region only when needed

6629cf1

whentojump force-pushed the test-chapuni branch from 68a3dfb to 6629cf1 Compare May 23, 2024 15:17

address comments: sink updating Loc should into then clause

0eace69

chapuni requested a review from hanickadot May 23, 2024 21:05

chapuni approved these changes May 23, 2024

View reviewed changes

hanickadot approved these changes May 23, 2024

View reviewed changes

chapuni merged commit f9e9e59 into llvm:main May 24, 2024
6 checks passed

whentojump deleted the test-chapuni branch May 24, 2024 03:18

whentojump mentioned this pull request Jun 5, 2024

[Coverage] Handles macros from system headers and nested macros in scratch space #94438

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Coverage][Expansion] handle nested macros in scratch space #89869

[Coverage][Expansion] handle nested macros in scratch space #89869

whentojump commented Apr 24, 2024 •

edited

Loading

github-actions bot commented Apr 24, 2024 •

edited

Loading

llvmbot commented Apr 24, 2024

llvmbot commented Apr 24, 2024

chapuni left a comment

chapuni left a comment

chapuni Apr 25, 2024

whentojump Apr 25, 2024

chapuni May 22, 2024

chapuni commented May 21, 2024

whentojump commented May 22, 2024

chapuni left a comment

hanickadot left a comment

whentojump commented May 24, 2024

chapuni commented May 24, 2024

	for (const auto &FM : FileIDMapping) {
	SourceLocation ExpandedLoc = FM.second.second;
	SourceLocation ParentLoc = getIncludeOrExpansionLoc(ExpandedLoc);

	if (ParentLoc.isInvalid())
	continue;

	auto ParentFileID = getCoverageFileID(ParentLoc);
	if (!ParentFileID)
	continue;

[Coverage][Expansion] handle nested macros in scratch space #89869

[Coverage][Expansion] handle nested macros in scratch space #89869

Conversation

whentojump commented Apr 24, 2024 • edited Loading

github-actions bot commented Apr 24, 2024 • edited Loading

llvmbot commented Apr 24, 2024

llvmbot commented Apr 24, 2024

chapuni left a comment

Choose a reason for hiding this comment

chapuni left a comment

Choose a reason for hiding this comment

chapuni Apr 25, 2024

Choose a reason for hiding this comment

whentojump Apr 25, 2024

Choose a reason for hiding this comment

chapuni May 22, 2024

Choose a reason for hiding this comment

chapuni commented May 21, 2024

whentojump commented May 22, 2024

chapuni left a comment

Choose a reason for hiding this comment

hanickadot left a comment

Choose a reason for hiding this comment

whentojump commented May 24, 2024

chapuni commented May 24, 2024

whentojump commented Apr 24, 2024 •

edited

Loading

github-actions bot commented Apr 24, 2024 •

edited

Loading