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

Hana Dusíková via llvm-commits llvm-commits at lists.llvm.org
Sat Jan 13 05:44:05 PST 2024


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

`if constexpr` and `if consteval` conditional statements code coverage should behave more like a preprocesor `#if`-s than normal ConditionalStmt. This PR should fix that.

>From 50856e444d4fd9dc92dab34ed8ad302671ce3716 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Hana=20Dusi=CC=81kova=CC=81?= <hanicka at hanicka.net>
Date: Sat, 13 Jan 2024 14:41:21 +0100
Subject: [PATCH] [coverage] skipping code coverage for 'if constexpr' and 'if
 consteval'

---
 clang/lib/CodeGen/CoverageMappingGen.cpp      | 151 ++++++++++++++----
 .../ProfileData/Coverage/CoverageMapping.cpp  |  12 +-
 2 files changed, 132 insertions(+), 31 deletions(-)

diff --git a/clang/lib/CodeGen/CoverageMappingGen.cpp b/clang/lib/CodeGen/CoverageMappingGen.cpp
index b245abd16c3f4a..7e3d2b76b8646b 100644
--- a/clang/lib/CodeGen/CoverageMappingGen.cpp
+++ b/clang/lib/CodeGen/CoverageMappingGen.cpp
@@ -119,6 +119,10 @@ 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)
+  bool SkippedRegion{false};
+
 public:
   SourceMappingRegion(Counter Count, std::optional<SourceLocation> LocStart,
                       std::optional<SourceLocation> LocEnd,
@@ -174,6 +178,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 +476,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(),
@@ -1252,6 +1264,29 @@ struct CounterCoverageMappingBuilder
     popRegions(Index);
   }
 
+  /// Emit a skip region between \p StartLoc and \p EndLoc with the given count.
+  void markSkipArea(SourceLocation StartLoc, SourceLocation EndLoc) {
+    if (StartLoc == EndLoc)
+      return;
+    assert(SpellingRegion(SM, StartLoc, EndLoc).isInSourceOrder());
+    handleFileExit(StartLoc);
+    size_t Index = pushRegion({}, StartLoc, EndLoc);
+    getRegion().setSkipped(true);
+    handleFileExit(EndLoc);
+    popRegions(Index);
+  }
+
+  void findGapAreaBetweenAndMarkSkipArea(SourceLocation AfterLoc,
+                                         SourceLocation BeforeLoc) {
+    // const std::optional<SourceRange> Gap = findGapAreaBetween(AfterLoc,
+    // BeforeLoc);
+    //
+    // if (Gap) {
+    //   markSkipArea(Gap->getBegin(), Gap->getEnd());
+    // }
+    markSkipArea(AfterLoc, BeforeLoc);
+  }
+
   /// Keep counts of breaks and continues inside loops.
   struct BreakContinue {
     Counter BreakCount;
@@ -1701,43 +1736,106 @@ struct CounterCoverageMappingBuilder
     Visit(S->getSubStmt());
   }
 
+  void CoverIfConsteval(const IfStmt *S) {
+    assert(S->isConsteval());
+
+    const auto *Then = S->getThen();
+    const auto *Else = S->getElse();
+
+    extendRegion(S);
+
+    if (S->isNegatedConsteval()) {
+      // ignore 'if consteval'
+      findGapAreaBetweenAndMarkSkipArea(S->getIfLoc(), getStart(Then));
+      Visit(Then);
+
+      if (Else) {
+        // ignore 'else <else>'
+        findGapAreaBetweenAndMarkSkipArea(getEnd(Then), getEnd(Else));
+      }
+    } else {
+      assert(S->isNonNegatedConsteval());
+      // ignore 'if consteval <then> [else]'
+      findGapAreaBetweenAndMarkSkipArea(S->getIfLoc(),
+                                        Else ? getStart(Else) : getEnd(Then));
+
+      if (Else)
+        Visit(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 *Then = S->getThen();
+    const auto *Else = S->getElse();
+
+    // ignore 'if constexpr ('
+    SourceLocation startOfSkipped = S->getIfLoc();
+
+    if (S->getInit()) {
+      // don't mark initialisation as ignored
+      findGapAreaBetweenAndMarkSkipArea(startOfSkipped, getStart(S->getInit()));
+      Visit(S->getInit());
+      // ignore after initialisation: '; <condition>)'...
+      startOfSkipped = getEnd(S->getInit());
+    }
+
+    if (isTrue) {
+      // ignore '<condition>)'
+      findGapAreaBetweenAndMarkSkipArea(startOfSkipped, getStart(Then));
+      Visit(Then);
+
+      if (Else)
+        // ignore 'else <else>'
+        findGapAreaBetweenAndMarkSkipArea(getEnd(Then), getEnd(Else));
+    } else {
+      // ignore '<condition>) <then> [else]'
+      findGapAreaBetweenAndMarkSkipArea(startOfSkipped,
+                                        Else ? getStart(Else) : getEnd(Then));
+
+      if (Else)
+        Visit(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);
-
-    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());
+    // 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;
@@ -1760,11 +1858,8 @@ 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/llvm/lib/ProfileData/Coverage/CoverageMapping.cpp b/llvm/lib/ProfileData/Coverage/CoverageMapping.cpp
index eece6a2cc71797..40bc376f69af25 100644
--- a/llvm/lib/ProfileData/Coverage/CoverageMapping.cpp
+++ b/llvm/lib/ProfileData/Coverage/CoverageMapping.cpp
@@ -1310,9 +1310,15 @@ LineCoverageStats::LineCoverageStats(
     if (isStartOfRegion(LineSegments[I]))
       ++MinRegionCount;
 
-  bool StartOfSkippedRegion = !LineSegments.empty() &&
-                              !LineSegments.front()->HasCount &&
-                              LineSegments.front()->IsRegionEntry;
+  // if any of region starting at this line, we need to count this line with it
+  auto IsRegionWithoutCount =  [](const CoverageSegment * Segment) { 
+    assert(Segment != nullptr);
+    return !(Segment->HasCount) && Segment->IsRegionEntry;
+  };
+
+  // previously skipped region could be only one on line, this changed with
+  // improvement for 'if constexpr' and 'if consteval' code coverage
+  const bool StartOfSkippedRegion = !LineSegments.empty() && std::all_of(LineSegments.begin(), LineSegments.end(), IsRegionWithoutCount);
 
   HasMultipleRegions = MinRegionCount > 1;
   Mapped =



More information about the llvm-commits mailing list