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

[chapuni]

A synthesized identifier with ## is emitted to <scratch space>. llvm-cov cannot handle ` 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 #87000

[llvmbot]

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-codegen

Author: NAKAMURA Takumi (chapuni)

Changes

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 #87000

---

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

4 Files Affected:

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

diff --git a/clang/lib/CodeGen/CoverageMappingGen.cpp b/clang/lib/CodeGen/CoverageMappingGen.cpp
index 71215da362d3d0..643f3c5ab8d349 100644
--- a/clang/lib/CodeGen/CoverageMappingGen.cpp
+++ b/clang/lib/CodeGen/CoverageMappingGen.cpp
@@ -339,8 +339,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;
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..1b8735cd27445a
--- /dev/null
+++ b/clang/test/CoverageMapping/mcdc-scratch-space.c
@@ -0,0 +1,39 @@
+// 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 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]
+}

[whentojump]

Hi, thanks for the fix.

I can confirm it addressed your test case (https://godbolt.org/z/sMscqWeq7). But unfortunately it doesn't fix the one in my original post of #87000 (compiler explorer link).

In fact, I utilized creduce to automatically reduce that one and ended up with this (with a bit manual modification):

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

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

int main(void) { return 0; }

This time an expansion region is missing (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] 
main:
  File 0, 8:16 -> 8:29 = #0

As a result, addBranch() here

llvm-project/llvm/lib/ProfileData/Coverage/CoverageMapping.cpp

Lines 693 to 704 in 28d85e2

	Result addBranch(const CounterMappingRegion &Branch) {
	assert(Branch.Kind == CounterMappingRegion::MCDCBranchRegion);
	auto ConditionID = Branch.getBranchParams().ID;
	assert(ConditionID >= 0 && "ConditionID should be positive");
	if (ConditionIDs.contains(ConditionID) ||
	ConditionID >= DecisionParams.NumConditions)
	return NotProcessed;
	if (!this->dominates(Branch))
	return NotProcessed;

fails to associate the branch with the decision.

I will keep investigating it and posting my findings here.

[whentojump]

@chapuni Would you please take a look at #89869? Specifically, the extra one commit atop yours.

Essentially I did similar things to getIncludeOrExpansionLoc().

[chapuni]

@whentojump Thanks. Would you like to take this over? Then I will close this.

[whentojump]

Yes I can do it. Thank you as well!!

[chapuni]

See #89869 .