[clang] [llvm] [coverage] skipping code coverage for 'if constexpr' and 'if consteval' (PR #78033)

Hana Dusíková via cfe-commits cfe-commits at lists.llvm.org
Mon Jan 22 00:16:34 PST 2024


https://github.com/hanickadot updated https://github.com/llvm/llvm-project/pull/78033

>From ae319fd34659c1373ce573346b93ffaa290a3312 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Hana=20Dusi=CC=81kova=CC=81?= <hanicka at hanicka.net>
Date: Mon, 22 Jan 2024 00:00:43 +0100
Subject: [PATCH 1/7] [coverage] skipping code coverage for 'if constexpr' and
 'if consteval'

---
 clang/lib/CodeGen/CoverageMappingGen.cpp      | 212 +++++++++++++++---
 .../CoverageMapping/branch-constfolded.cpp    |   8 +-
 clang/test/CoverageMapping/if.cpp             | 138 ++++++++----
 .../ProfileData/Coverage/CoverageMapping.cpp  |  14 +-
 .../ProfileData/CoverageMappingTest.cpp       |  24 +-
 5 files changed, 317 insertions(+), 79 deletions(-)

diff --git a/clang/lib/CodeGen/CoverageMappingGen.cpp b/clang/lib/CodeGen/CoverageMappingGen.cpp
index 4a44d113ddec9e..0f0f0459406fb3 100644
--- a/clang/lib/CodeGen/CoverageMappingGen.cpp
+++ b/clang/lib/CodeGen/CoverageMappingGen.cpp
@@ -119,12 +119,16 @@ class SourceMappingRegion {
   /// as the line execution count if there are no other regions on the line.
   bool GapRegion;
 
+  /// Whetever this region is skipped ('if constexpr' or 'if consteval' untaken
+  /// branch, or anything skipped but not empty line / comments)
+  bool SkippedRegion;
+
 public:
   SourceMappingRegion(Counter Count, std::optional<SourceLocation> LocStart,
                       std::optional<SourceLocation> LocEnd,
                       bool GapRegion = false)
-      : Count(Count), LocStart(LocStart), LocEnd(LocEnd), GapRegion(GapRegion) {
-  }
+      : Count(Count), LocStart(LocStart), LocEnd(LocEnd), GapRegion(GapRegion),
+        SkippedRegion(false) {}
 
   SourceMappingRegion(Counter Count, std::optional<Counter> FalseCount,
                       MCDCParameters MCDCParams,
@@ -132,13 +136,14 @@ class SourceMappingRegion {
                       std::optional<SourceLocation> LocEnd,
                       bool GapRegion = false)
       : Count(Count), FalseCount(FalseCount), MCDCParams(MCDCParams),
-        LocStart(LocStart), LocEnd(LocEnd), GapRegion(GapRegion) {}
+        LocStart(LocStart), LocEnd(LocEnd), GapRegion(GapRegion),
+        SkippedRegion(false) {}
 
   SourceMappingRegion(MCDCParameters MCDCParams,
                       std::optional<SourceLocation> LocStart,
                       std::optional<SourceLocation> LocEnd)
       : MCDCParams(MCDCParams), LocStart(LocStart), LocEnd(LocEnd),
-        GapRegion(false) {}
+        GapRegion(false), SkippedRegion(false) {}
 
   const Counter &getCounter() const { return Count; }
 
@@ -174,6 +179,10 @@ class SourceMappingRegion {
 
   void setGap(bool Gap) { GapRegion = Gap; }
 
+  bool isSkipped() const { return SkippedRegion; }
+
+  void setSkipped(bool Skipped) { SkippedRegion = Skipped; }
+
   bool isBranch() const { return FalseCount.has_value(); }
 
   bool isMCDCDecision() const { return MCDCParams.NumConditions != 0; }
@@ -468,6 +477,10 @@ class CoverageMappingBuilder {
         MappingRegions.push_back(CounterMappingRegion::makeGapRegion(
             Region.getCounter(), *CovFileID, SR.LineStart, SR.ColumnStart,
             SR.LineEnd, SR.ColumnEnd));
+      } else if (Region.isSkipped()) {
+        MappingRegions.push_back(CounterMappingRegion::makeSkipped(
+            *CovFileID, SR.LineStart, SR.ColumnStart, SR.LineEnd,
+            SR.ColumnEnd));
       } else if (Region.isBranch()) {
         MappingRegions.push_back(CounterMappingRegion::makeBranchRegion(
             Region.getCounter(), Region.getFalseCounter(),
@@ -1251,6 +1264,70 @@ struct CounterCoverageMappingBuilder
     popRegions(Index);
   }
 
+  /// Find a valid range starting with \p StartingLoc and ending before \p
+  /// BeforeLoc.
+  std::optional<SourceRange> findAreaStartingFromTo(SourceLocation StartingLoc,
+                                                    SourceLocation BeforeLoc) {
+    // If StartingLoc is in function-like macro, use its start location.
+    if (StartingLoc.isMacroID()) {
+      FileID FID = SM.getFileID(StartingLoc);
+      const SrcMgr::ExpansionInfo *EI = &SM.getSLocEntry(FID).getExpansion();
+      if (EI->isFunctionMacroExpansion())
+        StartingLoc = EI->getExpansionLocStart();
+    }
+
+    size_t StartDepth = locationDepth(StartingLoc);
+    size_t EndDepth = locationDepth(BeforeLoc);
+    while (!SM.isWrittenInSameFile(StartingLoc, BeforeLoc)) {
+      bool UnnestStart = StartDepth >= EndDepth;
+      bool UnnestEnd = EndDepth >= StartDepth;
+      if (UnnestEnd) {
+        assert(SM.isWrittenInSameFile(getStartOfFileOrMacro(BeforeLoc),
+                                      BeforeLoc));
+
+        BeforeLoc = getIncludeOrExpansionLoc(BeforeLoc);
+        assert(BeforeLoc.isValid());
+        EndDepth--;
+      }
+      if (UnnestStart) {
+        assert(SM.isWrittenInSameFile(StartingLoc,
+                                      getStartOfFileOrMacro(StartingLoc)));
+
+        StartingLoc = getIncludeOrExpansionLoc(StartingLoc);
+        assert(StartingLoc.isValid());
+        StartDepth--;
+      }
+    }
+    // If the start and end locations of the gap are both within the same macro
+    // file, the range may not be in source order.
+    if (StartingLoc.isMacroID() || BeforeLoc.isMacroID())
+      return std::nullopt;
+    if (!SM.isWrittenInSameFile(StartingLoc, BeforeLoc) ||
+        !SpellingRegion(SM, StartingLoc, BeforeLoc).isInSourceOrder())
+      return std::nullopt;
+    return {{StartingLoc, BeforeLoc}};
+  }
+
+  void markSkipped(SourceLocation StartLoc, SourceLocation BeforeLoc) {
+    const auto Skipped = findAreaStartingFromTo(StartLoc, BeforeLoc);
+
+    if (!Skipped) {
+      return;
+    }
+
+    const auto NewStartLoc = Skipped->getBegin();
+    const auto EndLoc = Skipped->getEnd();
+
+    if (NewStartLoc == EndLoc)
+      return;
+    assert(SpellingRegion(SM, NewStartLoc, EndLoc).isInSourceOrder());
+    handleFileExit(NewStartLoc);
+    size_t Index = pushRegion({}, NewStartLoc, EndLoc);
+    getRegion().setSkipped(true);
+    handleFileExit(EndLoc);
+    popRegions(Index);
+  }
+
   /// Keep counts of breaks and continues inside loops.
   struct BreakContinue {
     Counter BreakCount;
@@ -1700,43 +1777,116 @@ struct CounterCoverageMappingBuilder
     Visit(S->getSubStmt());
   }
 
+  void CoverIfConsteval(const IfStmt *S) {
+    assert(S->isConsteval());
+
+    const auto *Then = S->getThen();
+    const auto *Else = S->getElse();
+
+    // I'm using 'propagateCounts' later as new region is better and allows me
+    // to properly calculate line coverage in llvm-cov utility
+    const Counter ParentCount = getRegion().getCounter();
+
+    extendRegion(S);
+
+    if (S->isNegatedConsteval()) {
+      // ignore 'if consteval'
+      markSkipped(S->getIfLoc(), getStart(Then));
+      propagateCounts(ParentCount, Then);
+
+      if (Else) {
+        // ignore 'else <else>'
+        markSkipped(getEnd(Then), getEnd(Else));
+      }
+    } else {
+      assert(S->isNonNegatedConsteval());
+      // ignore 'if consteval <then> [else]'
+      markSkipped(S->getIfLoc(), Else ? getStart(Else) : getEnd(Then));
+
+      if (Else)
+        propagateCounts(ParentCount, Else);
+    }
+  }
+
+  void CoverIfConstexpr(const IfStmt *S) {
+    assert(S->isConstexpr());
+
+    // evaluate constant condition...
+    const auto *E = dyn_cast<ConstantExpr>(S->getCond());
+    assert(E != nullptr);
+    const bool isTrue = E->getResultAsAPSInt().getExtValue();
+
+    extendRegion(S);
+
+    const auto *Init = S->getInit();
+    const auto *Then = S->getThen();
+    const auto *Else = S->getElse();
+
+    // I'm using 'propagateCounts' later as new region is better and allows me
+    // to properly calculate line coverage in llvm-cov utility
+    const Counter ParentCount = getRegion().getCounter();
+
+    // ignore 'if constexpr ('
+    SourceLocation startOfSkipped = S->getIfLoc();
+
+    if (Init) {
+      // don't mark initialisation as ignored
+      markSkipped(startOfSkipped, getStart(Init));
+      propagateCounts(ParentCount, Init);
+      // ignore after initialisation: '; <condition>)'...
+      startOfSkipped = getEnd(Init);
+    }
+
+    if (isTrue) {
+      // ignore '<condition>)'
+      markSkipped(startOfSkipped, getStart(Then));
+      propagateCounts(ParentCount, Then);
+
+      if (Else)
+        // ignore 'else <else>'
+        markSkipped(getEnd(Then), getEnd(Else));
+    } else {
+      // ignore '<condition>) <then> [else]'
+      markSkipped(startOfSkipped, Else ? getStart(Else) : getEnd(Then));
+
+      if (Else) {
+        propagateCounts(ParentCount, Else);
+      }
+    }
+  }
+
   void VisitIfStmt(const IfStmt *S) {
+    // "if constexpr" and "if consteval" are not normal conditional statements,
+    // they should behave more like a preprocessor conditions
+    if (S->isConsteval())
+      return CoverIfConsteval(S);
+    else if (S->isConstexpr())
+      return CoverIfConstexpr(S);
+
     extendRegion(S);
     if (S->getInit())
       Visit(S->getInit());
 
     // Extend into the condition before we propagate through it below - this is
     // needed to handle macros that generate the "if" but not the condition.
-    if (!S->isConsteval())
-      extendRegion(S->getCond());
+    extendRegion(S->getCond());
 
     Counter ParentCount = getRegion().getCounter();
+    Counter ThenCount = getRegionCounter(S);
 
-    // If this is "if !consteval" the then-branch will never be taken, we don't
-    // need to change counter
-    Counter ThenCount =
-        S->isNegatedConsteval() ? ParentCount : getRegionCounter(S);
+    // Emitting a counter for the condition makes it easier to interpret the
+    // counter for the body when looking at the coverage.
+    propagateCounts(ParentCount, S->getCond());
 
-    if (!S->isConsteval()) {
-      // Emitting a counter for the condition makes it easier to interpret the
-      // counter for the body when looking at the coverage.
-      propagateCounts(ParentCount, S->getCond());
-
-      // The 'then' count applies to the area immediately after the condition.
-      std::optional<SourceRange> Gap =
-          findGapAreaBetween(S->getRParenLoc(), getStart(S->getThen()));
-      if (Gap)
-        fillGapAreaWithCount(Gap->getBegin(), Gap->getEnd(), ThenCount);
-    }
+    // The 'then' count applies to the area immediately after the condition.
+    std::optional<SourceRange> Gap =
+        findGapAreaBetween(S->getRParenLoc(), getStart(S->getThen()));
+    if (Gap)
+      fillGapAreaWithCount(Gap->getBegin(), Gap->getEnd(), ThenCount);
 
     extendRegion(S->getThen());
     Counter OutCount = propagateCounts(ThenCount, S->getThen());
-
-    // If this is "if consteval" the else-branch will never be taken, we don't
-    // need to change counter
-    Counter ElseCount = S->isNonNegatedConsteval()
-                            ? ParentCount
-                            : subtractCounters(ParentCount, ThenCount);
+    Counter ElseCount = subtractCounters(ParentCount, ThenCount);
 
     if (const Stmt *Else = S->getElse()) {
       bool ThenHasTerminateStmt = HasTerminateStmt;
@@ -1759,11 +1909,9 @@ struct CounterCoverageMappingBuilder
       GapRegionCounter = OutCount;
     }
 
-    if (!S->isConsteval()) {
-      // Create Branch Region around condition.
-      createBranchRegion(S->getCond(), ThenCount,
-                         subtractCounters(ParentCount, ThenCount));
-    }
+    // Create Branch Region around condition.
+    createBranchRegion(S->getCond(), ThenCount,
+                       subtractCounters(ParentCount, ThenCount));
   }
 
   void VisitCXXTryStmt(const CXXTryStmt *S) {
diff --git a/clang/test/CoverageMapping/branch-constfolded.cpp b/clang/test/CoverageMapping/branch-constfolded.cpp
index 4fdb640506c9d3..c8755d5d752b63 100644
--- a/clang/test/CoverageMapping/branch-constfolded.cpp
+++ b/clang/test/CoverageMapping/branch-constfolded.cpp
@@ -90,10 +90,10 @@ bool for_7(bool a) {       // MCDC: Decision,File 0, [[@LINE+1]]:10 -> [[@LINE+1
 }                          // CHECK: Branch,File 0, [[@LINE-1]]:15 -> [[@LINE-1]]:19 = 0, 0
 
 // CHECK-LABEL: _Z5for_8b:
-bool for_8(bool a) {       // MCDC: Decision,File 0, [[@LINE+3]]:17 -> [[@LINE+3]]:30 = M:0, C:2
-                           // CHECK: Branch,File 0, [[@LINE+2]]:17 -> [[@LINE+2]]:21 = 0, 0
-                           // CHECK: Branch,File 0, [[@LINE+1]]:25 -> [[@LINE+1]]:30 = 0, 0
-  if constexpr (true && false)
+bool for_8(bool a) {       // MCDC: Decision,File 0, [[@LINE+3]]:7 -> [[@LINE+3]]:20 = M:0, C:2
+                           // CHECK: Branch,File 0, [[@LINE+2]]:7 -> [[@LINE+2]]:11 = 0, 0
+                           // CHECK: Branch,File 0, [[@LINE+1]]:15 -> [[@LINE+1]]:20 = 0, 0
+  if (true && false)
       return true;
   else
       return false;
diff --git a/clang/test/CoverageMapping/if.cpp b/clang/test/CoverageMapping/if.cpp
index 92d560be01f3b7..bd23088ae65274 100644
--- a/clang/test/CoverageMapping/if.cpp
+++ b/clang/test/CoverageMapping/if.cpp
@@ -23,47 +23,99 @@ void foo() {                    // CHECK-NEXT: Gap,File 0, [[@LINE+1]]:21 -> [[@
 }                               // CHECK-NEXT: [[@LINE-2]]:9 -> [[@LINE-1]]:5 = #1
                                 // CHECK-NEXT: [[@LINE-2]]:5 -> [[@LINE-2]]:8 = #1
 
-// FIXME: Do not generate coverage for discarded branches in if constexpr
+// GH-57377
 // CHECK-LABEL: _Z30check_constexpr_true_with_elsei:
 int check_constexpr_true_with_else(int i) {   // CHECK-NEXT: [[@LINE]]:{{[0-9]+}} -> {{[0-9]+}}:2 = #0
-                                // CHECK-NEXT: [[@LINE+2]]:16 -> [[@LINE+2]]:20 = #0
-                                // CHECK-NEXT: Branch,File 0, [[@LINE+1]]:16 -> [[@LINE+1]]:20 = 0, 0
-  if constexpr(true) {          // CHECK-NEXT: Gap,File 0, [[@LINE]]:21 -> [[@LINE]]:22 = #1
-    i *= 3;                     // CHECK-NEXT: [[@LINE-1]]:22 -> [[@LINE+1]]:4 = #1
-  } else {                      // CHECK-NEXT: Gap,File 0, [[@LINE]]:4 -> [[@LINE]]:10 = (#0 - #1)
-    i *= 5;                     // CHECK-NEXT: [[@LINE-1]]:10 -> [[@LINE+1]]:4 = (#0 - #1)
+  if constexpr(true) {          // CHECK-NEXT: Skipped,File 0, [[@LINE]]:3 -> [[@LINE]]:22 = 0
+    i *= 3;                     // CHECK-NEXT: File 0, [[@LINE-1]]:22 -> [[@LINE+1]]:4 = #0
+  } else {                      // CHECK-NEXT: Skipped,File 0, [[@LINE]]:4 -> [[@LINE+2]]:4 = 0
+    i *= 5;
   }
   return i;
 }
 
+// GH-57377
 // CHECK-LABEL: _Z33check_constexpr_true_without_elsei:
 int check_constexpr_true_without_else(int i) {   // CHECK-NEXT: [[@LINE]]:{{[0-9]+}} -> {{[0-9]+}}:2 = #0
-                                // CHECK-NEXT: [[@LINE+2]]:16 -> [[@LINE+2]]:20 = #0
-                                // CHECK-NEXT: Branch,File 0, [[@LINE+1]]:16 -> [[@LINE+1]]:20 = 0, 0
-  if constexpr(true) {          // CHECK-NEXT: Gap,File 0, [[@LINE]]:21 -> [[@LINE]]:22 = #1
-    i *= 3;                     // CHECK-NEXT: [[@LINE-1]]:22 -> [[@LINE+1]]:4 = #1
+  if constexpr(true) {          // CHECK-NEXT: Skipped,File 0, [[@LINE]]:3 -> [[@LINE]]:22 = 0
+    i *= 3;                     // CHECK-NEXT: File 0, [[@LINE-1]]:22 -> [[@LINE+1]]:4 = #0
   }
   return i;
 }
 
+// GH-57377
 // CHECK-LABEL: _Z31check_constexpr_false_with_elsei:
 int check_constexpr_false_with_else(int i) {   // CHECK-NEXT: [[@LINE]]:{{[0-9]+}} -> {{[0-9]+}}:2 = #0
-                                // CHECK-NEXT: [[@LINE+2]]:16 -> [[@LINE+2]]:21 = #0
-                                // CHECK-NEXT: Branch,File 0, [[@LINE+1]]:16 -> [[@LINE+1]]:21 = 0, 0
-  if constexpr(false) {         // CHECK-NEXT: Gap,File 0, [[@LINE]]:22 -> [[@LINE]]:23 = #1
-    i *= 3;                     // CHECK-NEXT: File 0, [[@LINE-1]]:23 -> [[@LINE+1]]:4 = #1
-  } else {                      // CHECK-NEXT: Gap,File 0, [[@LINE]]:4 -> [[@LINE]]:10 = (#0 - #1)
-    i *= 5;                     // CHECK-NEXT: File 0, [[@LINE-1]]:10 -> [[@LINE+1]]:4 = (#0 - #1)
+  if constexpr(false) {         // CHECK-NEXT: Skipped,File 0, [[@LINE]]:3 -> [[@LINE+2]]:10 = 0
+    i *= 3;
+  } else {                      // CHECK-NEXT: File 0, [[@LINE]]:10 -> [[@LINE+2]]:4 = #0
+    i *= 5;
   }
   return i;
 }
 
+// GH-57377
 // CHECK-LABEL: _Z34check_constexpr_false_without_elsei:
 int check_constexpr_false_without_else(int i) {   // CHECK-NEXT: [[@LINE]]:{{[0-9]+}} -> {{[0-9]+}}:2 = #0
-                                // CHECK-NEXT: [[@LINE+2]]:16 -> [[@LINE+2]]:21 = #0
-                                // CHECK-NEXT: Branch,File 0, [[@LINE+1]]:16 -> [[@LINE+1]]:21 = 0, 0
-  if constexpr(false) {         // CHECK-NEXT: Gap,File 0, [[@LINE]]:22 -> [[@LINE]]:23 = #1
-    i *= 3;                     // CHECK-NEXT: File 0, [[@LINE-1]]:23 -> [[@LINE+1]]:4 = #1
+  if constexpr(false) {         // CHECK-NEXT: Skipped,File 0, [[@LINE]]:3 -> [[@LINE+2]]:4 = 0
+    i *= 3;
+  }
+  return i;
+}
+
+// GH-57377
+// CHECK-LABEL: _Z35check_constexpr_init_true_with_elsei:
+int check_constexpr_init_true_with_else(int i) {   // CHECK-NEXT: [[@LINE]]:{{[0-9]+}} -> {{[0-9]+}}:2 = #0
+  if constexpr(int j = i; true) {          // CHECK-NEXT: Skipped,File 0, [[@LINE]]:3 -> [[@LINE]]:16 = 0
+                                           // CHECK-NEXT: File 0, [[@LINE-1]]:16 -> [[@LINE-1]]:26 = #0
+                                           // CHECK-NEXT: Skipped,File 0, [[@LINE-2]]:26 -> [[@LINE-2]]:33 = 0
+                                           // CHECK-NEXT: File 0, [[@LINE-3]]:33 -> [[@LINE+2]]:4 = #0
+    i *= j;
+  } else {                                 // CHECK-NEXT: Skipped,File 0, [[@LINE]]:4 -> [[@LINE+2]]:4 = 0
+    i *= j;
+  }
+  return i;
+}
+
+// GH-57377
+// CHECK-LABEL: _Z38check_constexpr_init_true_without_elsei:
+int check_constexpr_init_true_without_else(int i) {   // CHECK-NEXT: [[@LINE]]:{{[0-9]+}} -> {{[0-9]+}}:2 = #0
+  if constexpr(int j = i; true) {          // CHECK-NEXT: Skipped,File 0, [[@LINE]]:3 -> [[@LINE]]:16 = 0
+                                           // CHECK-NEXT: File 0, [[@LINE-1]]:16 -> [[@LINE-1]]:26 = #0
+                                           // CHECK-NEXT: Skipped,File 0, [[@LINE-2]]:26 -> [[@LINE-2]]:33 = 0
+                                           // CHECK-NEXT: File 0, [[@LINE-3]]:33 -> [[@LINE+2]]:4 = #0
+    i *= j;
+  }
+  return i;
+}
+
+// GH-57377
+// CHECK-LABEL: _Z36check_constexpr_init_false_with_elsei:
+int check_constexpr_init_false_with_else(int i) {   // CHECK-NEXT: [[@LINE]]:{{[0-9]+}} -> {{[0-9]+}}:2 = #0
+  if constexpr(int j = i; false) {         // CHECK-NEXT: Skipped,File 0, [[@LINE]]:3 -> [[@LINE]]:16 = 0
+                                           // CHECK-NEXT: File 0, [[@LINE-1]]:16 -> [[@LINE-1]]:26 = #0
+    i *= j;                                // CHECK-NEXT: Skipped,File 0, [[@LINE-2]]:26 -> [[@LINE+1]]:10 = 0
+  } else {                                 // CHECK-NEXT: File 0, [[@LINE]]:10 -> [[@LINE+2]]:4 = #0
+    i *= j;
+  }
+  return i;
+}
+
+// GH-57377
+// CHECK-LABEL: _Z39check_constexpr_init_false_without_elsei:
+int check_constexpr_init_false_without_else(int i) {   // CHECK-NEXT: [[@LINE]]:{{[0-9]+}} -> {{[0-9]+}}:2 = #0
+  if constexpr(int j = i; false) {         // CHECK-NEXT: Skipped,File 0, [[@LINE]]:3 -> [[@LINE]]:16 = 0
+                                           // CHECK-NEXT: File 0, [[@LINE-1]]:16 -> [[@LINE-1]]:26 = #0
+    i *= j;                                // CHECK-NEXT: Skipped,File 0, [[@LINE-2]]:26 -> [[@LINE+1]]:4 = 0
+  }
+  return i;
+}
+
+// CHECK-LABEL: _Z32check_macro_constexpr_if_skippedi:
+int check_macro_constexpr_if_skipped(int i) {   // CHECK-NEXT: [[@LINE]]:{{[0-9]+}} -> {{[0-9]+}}:2 = #0
+#define IF_CONSTEXPR if constexpr               // CHECK-NEXT: Expansion,File 0, [[@LINE+1]]:3 -> [[@LINE+1]]:15 = #0 (Expanded file = 1)
+  IF_CONSTEXPR(false) {                         // CHECK-NEXT: Skipped,File 0, [[@LINE]]:3 -> [[@LINE+2]]:4 = 0
+    i *= 2;                                     // CHECK-NEXT: File 1, [[@LINE-2]]:22 -> [[@LINE-2]]:34 = #0
   }
   return i;
 }
@@ -127,48 +179,58 @@ void ternary() {
 
 // GH-57377
 // CHECK-LABEL: _Z40check_consteval_with_else_discarded_theni:
-// FIXME: Do not generate coverage for discarded <then> branch in if consteval
 constexpr int check_consteval_with_else_discarded_then(int i) { // CHECK-NEXT: [[@LINE]]:{{[0-9]+}} -> {{[0-9]+}}:2 = #0
-  if consteval {                        
-    i *= 3;                             // CHECK-NEXT: [[@LINE-1]]:16 -> [[@LINE+1]]:4 = #1
-  } else {                              // CHECK-NEXT: Gap,File 0, [[@LINE]]:4 -> [[@LINE]]:10 = #0
-    i *= 5;                             // CHECK-NEXT: [[@LINE-1]]:10 -> [[@LINE+1]]:4 = #0
+  if consteval {                        // CHECK-NEXT: Skipped,File 0, [[@LINE]]:3 -> [[@LINE+2]]:10 = 0
+    i *= 3;
+  } else {                              // CHECK-NEXT: File 0, [[@LINE]]:10 -> [[@LINE+2]]:4 = #0
+    i *= 5;
   }
-  return i;                             // CHECK-NEXT: [[@LINE]]:3 -> [[@LINE]]:11 = (#0 + #1)
+  return i;
 }
 
+// GH-57377
 // CHECK-LABEL: _Z43check_notconsteval_with_else_discarded_elsei:
-// FIXME: Do not generate coverage for discarded <else> branch in if consteval
 constexpr int check_notconsteval_with_else_discarded_else(int i) { // CHECK-NEXT: [[@LINE]]:{{[0-9]+}} -> {{[0-9]+}}:2 = #0
-  if !consteval {                        
-    i *= 3;                             // CHECK-NEXT: [[@LINE-1]]:17 -> [[@LINE+1]]:4 = #0
-  } else {                              // CHECK-NEXT: Gap,File 0, [[@LINE]]:4 -> [[@LINE]]:10 = 0
-    i *= 5;                             // CHECK-NEXT: [[@LINE-1]]:10 -> [[@LINE+1]]:4 = 0
+  if !consteval {                       // CHECK-NEXT: Skipped,File 0, [[@LINE]]:3 -> [[@LINE]]:17 = 0
+    i *= 3;                             
+  } else {                              // CHECK-NEXT: File 0, [[@LINE-2]]:17 -> [[@LINE]]:4 = #0
+    i *= 5;                             // CHECK-NEXT: Skipped,File 0, [[@LINE-1]]:4 -> [[@LINE+1]]:4 = 0
   }
   return i;
 }
 
+// GH-57377
 // CHECK-LABEL: _Z32check_consteval_branch_discardedi:
-// FIXME: Do not generate coverage for discarded <then> branch in if consteval
 constexpr int check_consteval_branch_discarded(int i) { // CHECK-NEXT: [[@LINE]]:{{[0-9]+}} -> {{[0-9]+}}:2 = #0
-  if consteval {                        
-    i *= 3;                             // CHECK-NEXT: [[@LINE-1]]:16 -> [[@LINE+1]]:4 = #1
+  if consteval {                        // CHECK-NEXT: Skipped,File 0, [[@LINE]]:3 -> [[@LINE+2]]:4 = 0 
+    i *= 3;      
   }
-  return i;                             // CHECK-NEXT: [[@LINE]]:3 -> [[@LINE]]:11 = (#0 + #1)
+  return i;      
 }
 
+// GH-57377
 // CHECK-LABEL: _Z30check_notconsteval_branch_kepti:
 constexpr int check_notconsteval_branch_kept(int i) { // CHECK-NEXT: [[@LINE]]:{{[0-9]+}} -> {{[0-9]+}}:2 = #0
-  if !consteval {                        
-    i *= 3;                             // CHECK-NEXT: [[@LINE-1]]:17 -> [[@LINE+1]]:4 = #0
+  if !consteval {                       // CHECK-NEXT: Skipped,File 0, [[@LINE]]:3 -> [[@LINE]]:17 = 0
+    i *= 3;                             // CHECK-NEXT: File 0, [[@LINE-1]]:17 -> [[@LINE+1]]:4 = #0
   }
   return i;
 }
 
+// CHECK-LABEL: _Z32check_macro_consteval_if_skippedi:
+constexpr int check_macro_consteval_if_skipped(int i) {   // CHECK-NEXT: [[@LINE]]:{{[0-9]+}} -> {{[0-9]+}}:2 = #0
+#define IF_RUNTIME if !consteval               // CHECK-NEXT: Expansion,File 0, [[@LINE+1]]:3 -> [[@LINE+1]]:13 = #0 (Expanded file = 1)
+  IF_RUNTIME {                                 // CHECK-NEXT: Skipped,File 0, [[@LINE]]:3 -> [[@LINE]]:14 = 0
+    i *= 2;                                    // CHECK-NEXT: File 0, [[@LINE-1]]:14 -> [[@LINE+1]]:4 = #0
+  }                                            // CHECK-NEXT: File 1, [[@LINE-3]]:20 -> [[@LINE-3]]:33 = #0
+  return i;
+}
+
 int instantiate_consteval(int i) {
   i *= check_consteval_with_else_discarded_then(i);
   i *= check_notconsteval_with_else_discarded_else(i);
   i *= check_consteval_branch_discarded(i);
   i *= check_notconsteval_branch_kept(i);
+  i *= check_macro_consteval_if_skipped(i);
   return i;
 }
diff --git a/llvm/lib/ProfileData/Coverage/CoverageMapping.cpp b/llvm/lib/ProfileData/Coverage/CoverageMapping.cpp
index eece6a2cc71797..2d7fcf2eac2d39 100644
--- a/llvm/lib/ProfileData/Coverage/CoverageMapping.cpp
+++ b/llvm/lib/ProfileData/Coverage/CoverageMapping.cpp
@@ -1319,8 +1319,20 @@ LineCoverageStats::LineCoverageStats(
       !StartOfSkippedRegion &&
       ((WrappedSegment && WrappedSegment->HasCount) || (MinRegionCount > 0));
 
-  if (!Mapped)
+  // if there is any starting segment at this line with a counter, it must be
+  // mapped
+  Mapped |= std::any_of(
+      LineSegments.begin(), LineSegments.end(),
+      [](const auto *Seq) { return Seq->IsRegionEntry && Seq->HasCount; });
+
+  // make sure last line of a block is covered if at that line there is also
+  // skipped region
+  Mapped |= WrappedSegment && WrappedSegment->IsRegionEntry &&
+            WrappedSegment->HasCount;
+
+  if (!Mapped) {
     return;
+  }
 
   // Pick the max count from the non-gap, region entry segments and the
   // wrapped count.
diff --git a/llvm/unittests/ProfileData/CoverageMappingTest.cpp b/llvm/unittests/ProfileData/CoverageMappingTest.cpp
index 1cf497cbdc2e61..23f66a0232ddbb 100644
--- a/llvm/unittests/ProfileData/CoverageMappingTest.cpp
+++ b/llvm/unittests/ProfileData/CoverageMappingTest.cpp
@@ -187,6 +187,11 @@ struct CoverageMappingTest : ::testing::TestWithParam<std::tuple<bool, bool>> {
                 : CounterMappingRegion::makeRegion(C, FileID, LS, CS, LE, CE));
   }
 
+  void addSkipped(StringRef File, unsigned LS, unsigned CS, unsigned LE,
+                  unsigned CE) {
+    addCMR(Counter::getZero(), File, LS, CS, LE, CE, true);
+  }
+
   void addMCDCDecisionCMR(unsigned Mask, unsigned NC, StringRef File,
                           unsigned LS, unsigned CS, unsigned LE, unsigned CE) {
     auto &Regions = InputFunctions.back().Regions;
@@ -700,22 +705,33 @@ TEST_P(CoverageMappingTest, test_line_coverage_iterator) {
 
   startFunction("func", 0x1234);
   addCMR(Counter::getCounter(0), "file1", 1, 1, 9, 9);
+  addSkipped("file1", 1, 3, 1, 8); // skipped region inside previous block
   addCMR(Counter::getCounter(1), "file1", 1, 1, 4, 7);
+  addSkipped("file1", 4, 1, 4, 20); // skipped line
   addCMR(Counter::getCounter(2), "file1", 5, 8, 9, 1);
+  addSkipped("file1", 10, 1, 12,
+             20); // skipped region which contains next region
   addCMR(Counter::getCounter(3), "file1", 10, 10, 11, 11);
   EXPECT_THAT_ERROR(loadCoverageMapping(), Succeeded());
-
   CoverageData Data = LoadedCoverage->getCoverageForFile("file1");
 
   unsigned Line = 0;
-  unsigned LineCounts[] = {20, 20, 20, 20, 30, 10, 10, 10, 10, 0, 0};
+  const unsigned LineCounts[] = {20, 20, 20, 0, 30, 10, 10, 10, 10, 0, 0, 0, 0};
+  const bool MappedLines[] = {1, 1, 1, 0, 1, 1, 1, 1, 1, 1, 1, 0, 0};
+  ASSERT_EQ(std::size(LineCounts), std::size(MappedLines));
+
   for (const auto &LCS : getLineCoverageStats(Data)) {
+    ASSERT_LT(Line, std::size(LineCounts));
+    ASSERT_LT(Line, std::size(MappedLines));
+
     ASSERT_EQ(Line + 1, LCS.getLine());
-    errs() << "Line: " << Line + 1 << ", count = " << LCS.getExecutionCount() << "\n";
+    errs() << "Line: " << Line + 1 << ", count = " << LCS.getExecutionCount()
+           << ", mapped = " << LCS.isMapped() << "\n";
     ASSERT_EQ(LineCounts[Line], LCS.getExecutionCount());
+    ASSERT_EQ(MappedLines[Line], LCS.isMapped());
     ++Line;
   }
-  ASSERT_EQ(11U, Line);
+  ASSERT_EQ(12U, Line);
 
   // Check that operator->() works / compiles.
   ASSERT_EQ(1U, LineCoverageIterator(Data)->getLine());

>From 73d0a8c30a19745226beaf163ca5dbdf93670014 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Hana=20Dusi=CC=81kova=CC=81?= <hanicka at hanicka.net>
Date: Mon, 22 Jan 2024 00:16:57 +0100
Subject: [PATCH 2/7] [coverage] don't show tooltips for skipped regions

---
 llvm/tools/llvm-cov/SourceCoverageView.cpp | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/llvm/tools/llvm-cov/SourceCoverageView.cpp b/llvm/tools/llvm-cov/SourceCoverageView.cpp
index b92c62df7950ac..71edd5fec4280a 100644
--- a/llvm/tools/llvm-cov/SourceCoverageView.cpp
+++ b/llvm/tools/llvm-cov/SourceCoverageView.cpp
@@ -130,6 +130,8 @@ bool SourceCoverageView::shouldRenderRegionMarkers(
     const auto *CurSeg = Segments[I];
     if (!CurSeg->IsRegionEntry || CurSeg->Count == LCS.getExecutionCount())
       continue;
+    if (!CurSeg->HasCount) // don't show tooltips for SkippedRegions
+      continue;
     return true;
   }
   return false;

>From 8e2e76d6b0a2e559251c6b1c5cf60d768026383f Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Hana=20Dusi=CC=81kova=CC=81?= <hanicka at hanicka.net>
Date: Mon, 22 Jan 2024 00:22:28 +0100
Subject: [PATCH 3/7] coding style fix

---
 clang/lib/CodeGen/CoverageMappingGen.cpp | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/clang/lib/CodeGen/CoverageMappingGen.cpp b/clang/lib/CodeGen/CoverageMappingGen.cpp
index 0f0f0459406fb3..b9705733fc03d6 100644
--- a/clang/lib/CodeGen/CoverageMappingGen.cpp
+++ b/clang/lib/CodeGen/CoverageMappingGen.cpp
@@ -1311,9 +1311,8 @@ struct CounterCoverageMappingBuilder
   void markSkipped(SourceLocation StartLoc, SourceLocation BeforeLoc) {
     const auto Skipped = findAreaStartingFromTo(StartLoc, BeforeLoc);
 
-    if (!Skipped) {
+    if (!Skipped)
       return;
-    }
 
     const auto NewStartLoc = Skipped->getBegin();
     const auto EndLoc = Skipped->getEnd();

>From c784703aa9642778e85c7a288bbe2c1a1494d2ea Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Hana=20Dusi=CC=81kova=CC=81?= <hanicka at hanicka.net>
Date: Mon, 22 Jan 2024 00:49:10 +0100
Subject: [PATCH 4/7] [coverage] fix for wrongly uncovered lines

---
 llvm/lib/ProfileData/Coverage/CoverageMapping.cpp | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/llvm/lib/ProfileData/Coverage/CoverageMapping.cpp b/llvm/lib/ProfileData/Coverage/CoverageMapping.cpp
index 2d7fcf2eac2d39..d8be7be6d7a59c 100644
--- a/llvm/lib/ProfileData/Coverage/CoverageMapping.cpp
+++ b/llvm/lib/ProfileData/Coverage/CoverageMapping.cpp
@@ -1325,11 +1325,6 @@ LineCoverageStats::LineCoverageStats(
       LineSegments.begin(), LineSegments.end(),
       [](const auto *Seq) { return Seq->IsRegionEntry && Seq->HasCount; });
 
-  // make sure last line of a block is covered if at that line there is also
-  // skipped region
-  Mapped |= WrappedSegment && WrappedSegment->IsRegionEntry &&
-            WrappedSegment->HasCount;
-
   if (!Mapped) {
     return;
   }

>From 14b985d8baf1704fa88e6f48d2b2cd5eef2d0e6b Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Hana=20Dusi=CC=81kova=CC=81?= <hanicka at hanicka.net>
Date: Mon, 22 Jan 2024 09:09:19 +0100
Subject: [PATCH 5/7] [coverage] fix for wrongly uncovered lines (fix nit about
 code style)

---
 clang/lib/CodeGen/CoverageMappingGen.cpp | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/clang/lib/CodeGen/CoverageMappingGen.cpp b/clang/lib/CodeGen/CoverageMappingGen.cpp
index b9705733fc03d6..4b85178de07dcf 100644
--- a/clang/lib/CodeGen/CoverageMappingGen.cpp
+++ b/clang/lib/CodeGen/CoverageMappingGen.cpp
@@ -1776,7 +1776,7 @@ struct CounterCoverageMappingBuilder
     Visit(S->getSubStmt());
   }
 
-  void CoverIfConsteval(const IfStmt *S) {
+  void coverIfConsteval(const IfStmt *S) {
     assert(S->isConsteval());
 
     const auto *Then = S->getThen();
@@ -1807,7 +1807,7 @@ struct CounterCoverageMappingBuilder
     }
   }
 
-  void CoverIfConstexpr(const IfStmt *S) {
+  void coverIfConstexpr(const IfStmt *S) {
     assert(S->isConstexpr());
 
     // evaluate constant condition...
@@ -1858,9 +1858,9 @@ struct CounterCoverageMappingBuilder
     // "if constexpr" and "if consteval" are not normal conditional statements,
     // they should behave more like a preprocessor conditions
     if (S->isConsteval())
-      return CoverIfConsteval(S);
+      return coverIfConsteval(S);
     else if (S->isConstexpr())
-      return CoverIfConstexpr(S);
+      return coverIfConstexpr(S);
 
     extendRegion(S);
     if (S->getInit())

>From 1b7b14fa5ab3e21d759e260024846947595ae48e Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Hana=20Dusi=CC=81kova=CC=81?= <hanicka at hanicka.net>
Date: Mon, 22 Jan 2024 09:11:11 +0100
Subject: [PATCH 6/7] [coverage] fix for wrongly uncovered lines (fix comments
 from Corentin)

---
 clang/lib/CodeGen/CoverageMappingGen.cpp | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/clang/lib/CodeGen/CoverageMappingGen.cpp b/clang/lib/CodeGen/CoverageMappingGen.cpp
index 4b85178de07dcf..eb6ae78473c9e6 100644
--- a/clang/lib/CodeGen/CoverageMappingGen.cpp
+++ b/clang/lib/CodeGen/CoverageMappingGen.cpp
@@ -1848,15 +1848,14 @@ struct CounterCoverageMappingBuilder
       // ignore '<condition>) <then> [else]'
       markSkipped(startOfSkipped, Else ? getStart(Else) : getEnd(Then));
 
-      if (Else) {
+      if (Else)
         propagateCounts(ParentCount, Else);
-      }
     }
   }
 
   void VisitIfStmt(const IfStmt *S) {
     // "if constexpr" and "if consteval" are not normal conditional statements,
-    // they should behave more like a preprocessor conditions
+    // their discarded statement should be skipped
     if (S->isConsteval())
       return coverIfConsteval(S);
     else if (S->isConstexpr())

>From 14ce2f3bdeeae371e1be43d815ca0362dfa2d473 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Hana=20Dusi=CC=81kova=CC=81?= <hanicka at hanicka.net>
Date: Mon, 22 Jan 2024 09:16:21 +0100
Subject: [PATCH 7/7] [coverage] fix for wrongly uncovered lines (fix comments
 from Corentin)

---
 clang/lib/CodeGen/CoverageMappingGen.cpp | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/clang/lib/CodeGen/CoverageMappingGen.cpp b/clang/lib/CodeGen/CoverageMappingGen.cpp
index eb6ae78473c9e6..2ace9f3ecf2428 100644
--- a/clang/lib/CodeGen/CoverageMappingGen.cpp
+++ b/clang/lib/CodeGen/CoverageMappingGen.cpp
@@ -1782,8 +1782,9 @@ struct CounterCoverageMappingBuilder
     const auto *Then = S->getThen();
     const auto *Else = S->getElse();
 
-    // I'm using 'propagateCounts' later as new region is better and allows me
-    // to properly calculate line coverage in llvm-cov utility
+    // It's better for llvm-cov to create a new region with same counter
+    // so line-coverage can be properly calculated for lines containing 
+    // a skipped region (without it the line is marked uncovered)
     const Counter ParentCount = getRegion().getCounter();
 
     extendRegion(S);
@@ -1811,16 +1812,11 @@ struct CounterCoverageMappingBuilder
     assert(S->isConstexpr());
 
     // evaluate constant condition...
-    const auto *E = dyn_cast<ConstantExpr>(S->getCond());
-    assert(E != nullptr);
+    const auto *E = cast<ConstantExpr>(S->getCond());
     const bool isTrue = E->getResultAsAPSInt().getExtValue();
 
     extendRegion(S);
 
-    const auto *Init = S->getInit();
-    const auto *Then = S->getThen();
-    const auto *Else = S->getElse();
-
     // I'm using 'propagateCounts' later as new region is better and allows me
     // to properly calculate line coverage in llvm-cov utility
     const Counter ParentCount = getRegion().getCounter();
@@ -1828,13 +1824,16 @@ struct CounterCoverageMappingBuilder
     // ignore 'if constexpr ('
     SourceLocation startOfSkipped = S->getIfLoc();
 
-    if (Init) {
+    if (const auto *Init = S->getInit()) {
       // don't mark initialisation as ignored
       markSkipped(startOfSkipped, getStart(Init));
       propagateCounts(ParentCount, Init);
       // ignore after initialisation: '; <condition>)'...
       startOfSkipped = getEnd(Init);
     }
+    
+    const auto *Then = S->getThen();
+    const auto *Else = S->getElse();
 
     if (isTrue) {
       // ignore '<condition>)'



More information about the cfe-commits mailing list