[MC/DC][Coverage] Add assertions into emitSourceRegions()

[chapuni]

emitSourceRegions() has bugs to emit malformed MC/DC coverage mappings. They were detected in llvm-cov as the crash.

Detect inconsistencies earlier in clang with assertions.

• mcdc-scratch-space.c covers [llvm-cov][MC/DC] "Branch not found in Decisions" when handling complicated macros #87000.

[llvmbot]

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-codegen

Author: NAKAMURA Takumi (chapuni)

Changes

emitSourceRegions() has bugs to emit malformed MC/DC coverage mappings. They were detected in llvm-cov as the crash.

Detect inconsistencies earlier in clang with assertions.

• mcdc-system-headers.cpp covers #78920.
• mcdc-scratch-space.c covers #87000.

---

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

3 Files Affected:

• (modified) clang/lib/CodeGen/CoverageMappingGen.cpp (+24-5)
• (added) clang/test/CoverageMapping/mcdc-scratch-space.c (+27)
• (added) clang/test/CoverageMapping/mcdc-system-headers.cpp (+47)

diff --git a/clang/lib/CodeGen/CoverageMappingGen.cpp b/clang/lib/CodeGen/CoverageMappingGen.cpp
index 71215da362d3d0..95ad1967c8b673 100644
--- a/clang/lib/CodeGen/CoverageMappingGen.cpp
+++ b/clang/lib/CodeGen/CoverageMappingGen.cpp
@@ -190,11 +190,21 @@ class SourceMappingRegion {
 
   bool isBranch() const { return FalseCount.has_value(); }
 
+  bool isMCDCBranch() const {
+    const auto *BranchParams = std::get_if<mcdc::BranchParameters>(&MCDCParams);
+    assert(BranchParams == nullptr || BranchParams->ID >= 0);
+    return (BranchParams != nullptr);
+  }
+
+  const auto &getMCDCBranchParams() const {
+    return mcdc::getParams<const mcdc::BranchParameters>(MCDCParams);
+  }
+
   bool isMCDCDecision() const {
     const auto *DecisionParams =
         std::get_if<mcdc::DecisionParameters>(&MCDCParams);
-    assert(!DecisionParams || DecisionParams->NumConditions > 0);
-    return DecisionParams;
+    assert(DecisionParams == nullptr || DecisionParams->NumConditions > 0);
+    return (DecisionParams != nullptr);
   }
 
   const auto &getMCDCDecisionParams() const {
@@ -464,13 +474,19 @@ class CoverageMappingBuilder {
       // Ignore regions from system headers unless collecting coverage from
       // system headers is explicitly enabled.
       if (!SystemHeadersCoverage &&
-          SM.isInSystemHeader(SM.getSpellingLoc(LocStart)))
+          SM.isInSystemHeader(SM.getSpellingLoc(LocStart))) {
+        assert(!Region.isMCDCBranch() && !Region.isMCDCDecision() &&
+               "Don't suppress the condition in system headers");
         continue;
+      }
 
       auto CovFileID = getCoverageFileID(LocStart);
       // Ignore regions that don't have a file, such as builtin macros.
-      if (!CovFileID)
+      if (!CovFileID) {
+        assert(!Region.isMCDCBranch() && !Region.isMCDCDecision() &&
+               "Don't suppress the condition in non-file regions");
         continue;
+      }
 
       SourceLocation LocEnd = Region.getEndLoc();
       assert(SM.isWrittenInSameFile(LocStart, LocEnd) &&
@@ -480,8 +496,11 @@ class CoverageMappingBuilder {
       // This not only suppresses redundant regions, but sometimes prevents
       // creating regions with wrong counters if, for example, a statement's
       // body ends at the end of a nested macro.
-      if (Filter.count(std::make_pair(LocStart, LocEnd)))
+      if (Filter.count(std::make_pair(LocStart, LocEnd))) {
+        assert(!Region.isMCDCBranch() && !Region.isMCDCDecision() &&
+               "Don't suppress the condition");
         continue;
+      }
 
       // Find the spelling locations for the mapping region.
       SpellingRegion SR{SM, LocStart, LocEnd};
diff --git a/clang/test/CoverageMapping/mcdc-scratch-space.c b/clang/test/CoverageMapping/mcdc-scratch-space.c
new file mode 100644
index 00000000000000..962d10653a028b
--- /dev/null
+++ b/clang/test/CoverageMapping/mcdc-scratch-space.c
@@ -0,0 +1,27 @@
+// RUN: %clang_cc1 -triple %itanium_abi_triple -std=c99 -fcoverage-mcdc -fprofile-instrument=clang -fcoverage-mapping -dump-coverage-mapping -emit-llvm-only %s
+// XFAIL: *
+// REQUIRES: asserts
+
+int builtin_macro0(int a) {
+  return (__LINE__
+          && a);
+}
+
+int builtin_macro1(int a) {
+  return (a
+          || __LINE__);
+}
+
+#define PRE(x) pre_##x
+
+int pre0(int pre_a, int b_post) {
+  return (PRE(a)
+          && b_post);
+}
+
+#define POST(x) x##_post
+
+int post0(int pre_a, int b_post) {
+  return (pre_a
+          || POST(b));
+}
diff --git a/clang/test/CoverageMapping/mcdc-system-headers.cpp b/clang/test/CoverageMapping/mcdc-system-headers.cpp
new file mode 100644
index 00000000000000..329bb37822a9ea
--- /dev/null
+++ b/clang/test/CoverageMapping/mcdc-system-headers.cpp
@@ -0,0 +1,47 @@
+// RUN: %clang_cc1 -std=c++11 -fprofile-instrument=clang -fcoverage-mapping -dump-coverage-mapping  -fcoverage-mcdc -mllvm -system-headers-coverage -emit-llvm-only -o - %s | FileCheck %s
+
+// Will crash w/o -system-headers-coverage
+// RUN: not --crash %clang_cc1 -std=c++11 -fprofile-instrument=clang -fcoverage-mapping -dump-coverage-mapping -fcoverage-mcdc -emit-llvm-only -o - %s
+// REQUIRES: asserts
+
+#ifdef IS_SYSHEADER
+
+#pragma clang system_header
+#define CONST 42
+#define EXPR1(x) (x)
+#define EXPR2(x) ((x) * (x))
+
+#else
+
+#define IS_SYSHEADER
+#include __FILE__
+
+// CHECK: _Z5func0i:
+int func0(int a) {
+  // CHECK: Decision,File 0, [[@LINE+2]]:11 -> [[@LINE+2]]:21 = M:0, C:2
+  // CHECK: Expansion,File 0, [[@LINE+1]]:11 -> [[@LINE+1]]:16 = #0 (Expanded file = 1)
+  return (CONST && a);
+  // CHECK: Branch,File 0, [[@LINE-1]]:20 -> [[@LINE-1]]:21 = #2, (#1 - #2) [2,0,0]
+  // CHECK: Branch,File 1, [[@LINE-15]]:15 -> [[@LINE-15]]:17 = 0, 0 [1,2,0]
+}
+
+// CHECK: _Z5func1ii:
+int func1(int a, int b) {
+  // CHECK: Decision,File 0, [[@LINE+2]]:11 -> [[@LINE+2]]:21 = M:0, C:2
+  // CHECK: Branch,File 0, [[@LINE+1]]:11 -> [[@LINE+1]]:12 = (#0 - #1), #1 [1,0,2]
+  return (a || EXPR1(b));
+  // CHECK: Expansion,File 0, [[@LINE-1]]:16 -> [[@LINE-1]]:21 = #1 (Expanded file = 1)
+  // CHECK: Branch,File 1, [[@LINE-23]]:18 -> [[@LINE-23]]:21 = (#1 - #2), #2 [2,0,0]
+}
+
+// CHECK: _Z5func2ii:
+int func2(int a, int b) {
+  // Decision,File 0, [[@LINE+3]]:11 -> [[@LINE+3]]:28 = M:0, C:2
+  // Expansion,File 0, [[@LINE+2]]:11 -> [[@LINE+2]]:16 = #0 (Expanded file = 1)
+  // Expansion,File 0, [[@LINE+1]]:23 -> [[@LINE+1]]:28 = #1 (Expanded file = 2)
+  return (EXPR2(a) && EXPR1(a));
+  // CHECK: Branch,File 1, [[@LINE-31]]:18 -> [[@LINE-31]]:29 = #1, (#0 - #1) [1,2,0]
+  // CHECK: Branch,File 2, [[@LINE-33]]:18 -> [[@LINE-33]]:21 = #2, (#1 - #2) [2,0,0]
+}
+
+#endif

[ornata]

LGTM