[coverage] skipping code coverage for 'if constexpr' and 'if consteval' #78033

hanickadot · 2024-01-13T13:43:59Z

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.

llvmbot · 2024-01-13T13:44:28Z

@llvm/pr-subscribers-pgo
@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-codegen

Author: Hana Dusíková (hanickadot)

Changes

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.

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

2 Files Affected:

(modified) clang/lib/CodeGen/CoverageMappingGen.cpp (+123-28)
(modified) llvm/lib/ProfileData/Coverage/CoverageMapping.cpp (+9-3)

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 =

github-actions · 2024-01-13T13:46:46Z

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

hanickadot · 2024-01-13T20:10:03Z

I need to skip the whitespace before if constexpr and if consteval (in circles).

UPDATED: keeping them there.

ornata · 2024-01-15T06:36:00Z

I think that skipping whitespace-only regions in llvm-cov intuitively makes sense. In general, showing coverage information about empty spaces doesn't give the user any useful information.

I think I'd do that part in a separate patch though.

hanickadot · 2024-01-21T23:02:55Z

I deleted the screenshot showing skipped whitespace regions as I reverted that part of the patch.

hanickadot · 2024-01-21T23:04:00Z

I kept the whitespace in front of if constexpr and if consteval marked as uncovered in case whole region containing the ifs is uncovered. It's an esthetic thing and I will do it with a next patch to limit scope of this one.

hanickadot · 2024-01-21T23:19:37Z

clang/test/CoverageMapping/branch-constfolded.cpp

-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)


if constexpr is not a branch now

clang/lib/CodeGen/CoverageMappingGen.cpp

hanickadot · 2024-01-21T23:21:46Z

clang/lib/CodeGen/CoverageMappingGen.cpp

@@ -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,


this is modified function findGapAreaBetween but we need to start at StartingLoc, not after

cor3ntin

Looks great!
I added a few comments
This probably needs another edit of the release notes

clang/lib/CodeGen/CoverageMappingGen.cpp

cor3ntin · 2024-01-22T07:59:50Z

clang/lib/CodeGen/CoverageMappingGen.cpp

+      markSkipped(startOfSkipped, getStart(Init));
+      propagateCounts(ParentCount, Init);
+      // ignore after initialisation: '; <condition>)'...
+      startOfSkipped = getEnd(Init);


I don't know if we care but you can technically write things like

if(using foo = int; true) {}```

yes, and it will be dealt as with declaration, in propagateCounts it will recurse into it

can you add a test?

test added :) it catched a crash as for some reason using foo = int there doesn't have right source location

clang/lib/CodeGen/CoverageMappingGen.cpp

ornata

LGTM

clang/docs/ReleaseNotes.rst

cor3ntin

LGTM (with a nit)
Thanks!

…constexpr init)

Co-authored-by: cor3ntin <[email protected]>

gkotheim · 2024-01-30T13:58:35Z

clang/lib/CodeGen/CoverageMappingGen.cpp

+    assert(S->isConstexpr());
+
+    // evaluate constant condition...
+    const auto *E = cast<ConstantExpr>(S->getCond());


@hanickadot The assumption that the condition of an if contexpr must be castable to ConstantExpr seems to be incorrect, see this counterexample.

hanickadot marked this pull request as draft January 13, 2024 13:44

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

hanickadot force-pushed the main branch from 50856e4 to 7e09f8a Compare January 13, 2024 13:54

cor3ntin added the coverage label Jan 13, 2024

hanickadot mentioned this pull request Jan 13, 2024

Source-based code coverage incorrectly handles if constexpr block #54419

Closed

hanickadot force-pushed the main branch from 90f17a2 to efcab4d Compare January 13, 2024 22:27

hanickadot force-pushed the main branch 3 times, most recently from cc8350e to ae319fd Compare January 21, 2024 23:00

hanickadot marked this pull request as ready for review January 21, 2024 23:02

llvmbot added the PGO Profile Guided Optimizations label Jan 21, 2024

hanickadot requested a review from chapuni January 21, 2024 23:04

hanickadot commented Jan 21, 2024

View reviewed changes

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

hanickadot commented Jan 21, 2024

View reviewed changes

hanickadot requested review from ornata and cor3ntin January 21, 2024 23:23

hanickadot changed the title ~~[coverage] skipping code coverage for 'if constexpr' and 'if consteval' [WIP]~~ [coverage] skipping code coverage for 'if constexpr' and 'if consteval' Jan 21, 2024

cor3ntin reviewed Jan 22, 2024

View reviewed changes

ornata reviewed Jan 22, 2024

View reviewed changes

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

ornata reviewed Jan 22, 2024

View reviewed changes

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

ornata reviewed Jan 22, 2024

View reviewed changes

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

ornata reviewed Jan 22, 2024

View reviewed changes

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

hanickadot force-pushed the main branch from 14ce2f3 to d48a5ea Compare January 22, 2024 08:25

ornata approved these changes Jan 22, 2024

View reviewed changes

cor3ntin reviewed Jan 22, 2024

View reviewed changes

clang/docs/ReleaseNotes.rst Outdated Show resolved Hide resolved

cor3ntin approved these changes Jan 22, 2024

View reviewed changes

hanickadot and others added 9 commits January 22, 2024 10:40

[coverage] skipping code coverage for 'if constexpr' and 'if consteval'

e2e0eca

[coverage] don't show tooltips for skipped regions

aa9cae2

coding style fix

b6618b4

[coverage] fix for wrongly uncovered lines

34c603c

[coverage] fix for wrongly uncovered lines (fix comments from review)

cd57c72

[coverage] fix for wrongly uncovered lines (add info to release notes)

9f55892

[coverage] fix for wrongly uncovered lines (add test for using in if …

d20e6d7

…constexpr init)

[coverage] fix for wrongly uncovered lines (add test for using in if …

27c5286

…constexpr init)

Update clang/docs/ReleaseNotes.rst

00f4356

Co-authored-by: cor3ntin <[email protected]>

hanickadot force-pushed the main branch from 0e69e3a to 00f4356 Compare January 22, 2024 09:40

hanickadot merged commit 865e4a1 into llvm:main Jan 22, 2024
5 checks passed

gkotheim mentioned this pull request Jan 30, 2024

Weird Clang's branch coverage with macros from system headers #78920

Closed

gkotheim reviewed Jan 30, 2024

View reviewed changes

chapuni mentioned this pull request Jan 30, 2024

[Coverage] Map regions from system headers #76950

Merged

This was referenced Feb 1, 2024

[coverage] crash with ExprWithCleanups in if constexpr #80285

Closed

reporting bad coverage for code in unevaluated context #75751

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[coverage] skipping code coverage for 'if constexpr' and 'if consteval' #78033

[coverage] skipping code coverage for 'if constexpr' and 'if consteval' #78033

hanickadot commented Jan 13, 2024

llvmbot commented Jan 13, 2024 •

edited

Loading

github-actions bot commented Jan 13, 2024 •

edited

Loading

hanickadot commented Jan 13, 2024 •

edited

Loading

ornata commented Jan 15, 2024

hanickadot commented Jan 21, 2024

hanickadot commented Jan 21, 2024

hanickadot Jan 21, 2024

hanickadot Jan 21, 2024

cor3ntin left a comment

cor3ntin Jan 22, 2024

hanickadot Jan 22, 2024

cor3ntin Jan 22, 2024

hanickadot Jan 22, 2024

ornata left a comment

cor3ntin left a comment

gkotheim Jan 30, 2024

[coverage] skipping code coverage for 'if constexpr' and 'if consteval' #78033

[coverage] skipping code coverage for 'if constexpr' and 'if consteval' #78033

Conversation

hanickadot commented Jan 13, 2024

llvmbot commented Jan 13, 2024 • edited Loading

github-actions bot commented Jan 13, 2024 • edited Loading

hanickadot commented Jan 13, 2024 • edited Loading

ornata commented Jan 15, 2024

hanickadot commented Jan 21, 2024

hanickadot commented Jan 21, 2024

hanickadot Jan 21, 2024

Choose a reason for hiding this comment

hanickadot Jan 21, 2024

Choose a reason for hiding this comment

cor3ntin left a comment

Choose a reason for hiding this comment

cor3ntin Jan 22, 2024

Choose a reason for hiding this comment

hanickadot Jan 22, 2024

Choose a reason for hiding this comment

cor3ntin Jan 22, 2024

Choose a reason for hiding this comment

hanickadot Jan 22, 2024

Choose a reason for hiding this comment

ornata left a comment

Choose a reason for hiding this comment

cor3ntin left a comment

Choose a reason for hiding this comment

gkotheim Jan 30, 2024

Choose a reason for hiding this comment

llvmbot commented Jan 13, 2024 •

edited

Loading

github-actions bot commented Jan 13, 2024 •

edited

Loading

hanickadot commented Jan 13, 2024 •

edited

Loading