[clang] [clang][coverage] fixing "if constexpr" and "if consteval" coverage report (PR #77214)
Hana Dusíková via cfe-commits
cfe-commits at lists.llvm.org
Sat Jan 6 15:30:50 PST 2024
https://github.com/hanickadot created https://github.com/llvm/llvm-project/pull/77214
It was a while since I noticed coverage report is broken for "if constexpr" and "if consteval" (as shown on first picture).
<img width="453" alt="Screenshot 2024-01-07 at 00 29 17" src="https://github.com/llvm/llvm-project/assets/6557263/dbdbc8a6-ad16-44da-882d-8e229ee69093">
(notice wrong coverage "if constexpr" for positive one, and completely missing for negative one, also notice "if consteval" marking always the same branch as uncovered)
Report after this change:
<img width="453" alt="Screenshot 2024-01-07 at 00 25 32" src="https://github.com/llvm/llvm-project/assets/6557263/d7ca85b0-34c7-40b5-9cc7-4efd8c18649b">
Main problem was replacement of non-taken "if constexpr" branch with `NullStmt` but such object doesn't have begin/end for source location properly. So I introduce a new constructor for empty `CompoundStmt` and used it.
With "if consteval" I'm no longer introducing new branch counter for non-taken "branch". But in future it would be useful to mark whole gap there as skipped instead. If there is interest I would do it in another PR.
>From 5c6d6b34e544fba61188ab64363cfb1dd29e50ac Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Hana=20Dusi=CC=81kova=CC=81?= <hanicka at hanicka.net>
Date: Sun, 7 Jan 2024 00:13:08 +0100
Subject: [PATCH] [coverage] fix incorrect coverage reporting for "if
constexpr" and "if consteval"
---
clang/include/clang/AST/Stmt.h | 6 ++++--
clang/lib/CodeGen/CoverageMappingGen.cpp | 13 +++++++++++--
clang/lib/Sema/TreeTransform.h | 7 ++++++-
3 files changed, 21 insertions(+), 5 deletions(-)
diff --git a/clang/include/clang/AST/Stmt.h b/clang/include/clang/AST/Stmt.h
index da7b37ce0e1211..fb50212083316e 100644
--- a/clang/include/clang/AST/Stmt.h
+++ b/clang/include/clang/AST/Stmt.h
@@ -1631,8 +1631,10 @@ class CompoundStmt final
SourceLocation RB);
// Build an empty compound statement with a location.
- explicit CompoundStmt(SourceLocation Loc)
- : Stmt(CompoundStmtClass), LBraceLoc(Loc), RBraceLoc(Loc) {
+ explicit CompoundStmt(SourceLocation Loc) : CompoundStmt(Loc, Loc) {}
+
+ explicit CompoundStmt(SourceLocation Loc, SourceLocation EndLoc)
+ : Stmt(CompoundStmtClass), LBraceLoc(Loc), RBraceLoc(EndLoc) {
CompoundStmtBits.NumStmts = 0;
CompoundStmtBits.HasFPFeatures = 0;
}
diff --git a/clang/lib/CodeGen/CoverageMappingGen.cpp b/clang/lib/CodeGen/CoverageMappingGen.cpp
index bf227386a71b78..b245abd16c3f4a 100644
--- a/clang/lib/CodeGen/CoverageMappingGen.cpp
+++ b/clang/lib/CodeGen/CoverageMappingGen.cpp
@@ -1712,7 +1712,11 @@ struct CounterCoverageMappingBuilder
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
@@ -1729,7 +1733,12 @@ struct CounterCoverageMappingBuilder
extendRegion(S->getThen());
Counter OutCount = propagateCounts(ThenCount, S->getThen());
- Counter ElseCount = subtractCounters(ParentCount, ThenCount);
+ // 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);
+
if (const Stmt *Else = S->getElse()) {
bool ThenHasTerminateStmt = HasTerminateStmt;
HasTerminateStmt = false;
diff --git a/clang/lib/Sema/TreeTransform.h b/clang/lib/Sema/TreeTransform.h
index c8c5a51bf9f94e..0033c851b618a1 100644
--- a/clang/lib/Sema/TreeTransform.h
+++ b/clang/lib/Sema/TreeTransform.h
@@ -7732,7 +7732,8 @@ TreeTransform<Derived>::TransformIfStmt(IfStmt *S) {
if (Then.isInvalid())
return StmtError();
} else {
- Then = new (getSema().Context) NullStmt(S->getThen()->getBeginLoc());
+ Then = new (getSema().Context)
+ CompoundStmt(S->getThen()->getBeginLoc(), S->getThen()->getEndLoc());
}
// Transform the "else" branch.
@@ -7741,6 +7742,10 @@ TreeTransform<Derived>::TransformIfStmt(IfStmt *S) {
Else = getDerived().TransformStmt(S->getElse());
if (Else.isInvalid())
return StmtError();
+ } else if (S->getElse() && ConstexprConditionValue &&
+ *ConstexprConditionValue) {
+ Else = new (getSema().Context)
+ CompoundStmt(S->getElse()->getBeginLoc(), S->getElse()->getEndLoc());
}
if (!getDerived().AlwaysRebuild() &&
More information about the cfe-commits
mailing list