Skip to content

Commit

Permalink
[clang][coverage] Fix "if constexpr" and "if consteval" coverage repo…
Browse files Browse the repository at this point in the history 
…rt (#77214)

Replace the discarded statement by an empty compound statement so we can keep track of the
whole source range we need to skip in coverage

Fixes #54419
  • Loading branch information
@hanickadot
hanickadot committed Jan 10, 2024
1 parent e22cb93 commit a26cc75
Show file tree
Hide file tree
Showing 5 changed files with 121 additions and 22 deletions.
3 changes: 3 additions & 0 deletions clang/docs/ReleaseNotes.rst
View file
Original file line number Diff line number Diff line change
Expand Up @@ -705,6 +705,9 @@ Bug Fixes in This Version
- Fix assertion crash due to failed scope restoring caused by too-early VarDecl
invalidation by invalid initializer Expr.
Fixes (`#30908 <https://github.com/llvm/llvm-project/issues/30908>`_)
- Clang now emits correct source location for code-coverage regions in `if constexpr`
and `if consteval` branches.
Fixes (`#54419 <https://github.com/llvm/llvm-project/issues/54419>`_)


Bug Fixes to Compiler Builtins
Expand Down
6 changes: 4 additions & 2 deletions clang/include/clang/AST/Stmt.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {}

CompoundStmt(SourceLocation Loc, SourceLocation EndLoc)
: Stmt(CompoundStmtClass), LBraceLoc(Loc), RBraceLoc(EndLoc) {
CompoundStmtBits.NumStmts = 0;
CompoundStmtBits.HasFPFeatures = 0;
}
Expand Down
13 changes: 11 additions & 2 deletions clang/lib/CodeGen/CoverageMappingGen.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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;
Expand Down
13 changes: 12 additions & 1 deletion clang/lib/Sema/TreeTransform.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7739,7 +7739,11 @@ TreeTransform<Derived>::TransformIfStmt(IfStmt *S) {
if (Then.isInvalid())
return StmtError();
} else {
Then = new (getSema().Context) NullStmt(S->getThen()->getBeginLoc());
// Discarded branch is replaced with empty CompoundStmt so we can keep
// proper source location for start and end of original branch, so
// subsequent transformations like CoverageMapping work properly
Then = new (getSema().Context)
CompoundStmt(S->getThen()->getBeginLoc(), S->getThen()->getEndLoc());
}

// Transform the "else" branch.
Expand All @@ -7748,6 +7752,13 @@ TreeTransform<Derived>::TransformIfStmt(IfStmt *S) {
Else = getDerived().TransformStmt(S->getElse());
if (Else.isInvalid())
return StmtError();
} else if (S->getElse() && ConstexprConditionValue &&
*ConstexprConditionValue) {
// Same thing here as with <then> branch, we are discarding it, we can't
// replace it with NULL nor NullStmt as we need to keep for source location
// range, for CoverageMapping
Else = new (getSema().Context)
CompoundStmt(S->getElse()->getBeginLoc(), S->getElse()->getEndLoc());
}

if (!getDerived().AlwaysRebuild() &&
Expand Down
108 changes: 91 additions & 17 deletions clang/test/CoverageMapping/if.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,19 +23,49 @@ 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 consteval and if constexpr statements
constexpr int check_consteval(int i) {
if consteval {
i++;
}
if !consteval {
i++;
}
if consteval {
return 42;
} else {
return i;
}
// FIXME: Do not generate coverage for discarded branches in if constexpr
// 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)
}
return i;
}

// 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
}
return i;
}

// 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)
}
return i;
}

// 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
}
return i;
}

// CHECK-LABEL: main:
Expand Down Expand Up @@ -75,10 +105,6 @@ int main() { // CHECK: File 0, [[@LINE]]:12 -> {{[0-9]+}}:2 =
// CHECK-NEXT: File 0, [[@LINE+1]]:14 -> [[@LINE+1]]:20 = #6
i = i == 0?i + 12:i + 10; // CHECK-NEXT: File 0, [[@LINE]]:21 -> [[@LINE]]:27 = (#0 - #6)

// GH-57377
constexpr int c_i = check_consteval(0);
check_consteval(i);

// GH-45481
S s;
s.the_prop = 0? 1 : 2; // CHECK-NEXT: File 0, [[@LINE]]:16 -> [[@LINE]]:17 = #0
Expand All @@ -98,3 +124,51 @@ int main() { // CHECK: File 0, [[@LINE]]:12 -> {{[0-9]+}}:2 =
void ternary() {
true ? FOO : FOO; // CHECK-NOT: Gap,{{.*}}, [[@LINE]]:8 ->
}

// 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
}
return i; // CHECK-NEXT: [[@LINE]]:3 -> [[@LINE]]:11 = (#0 + #1)
}

// 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
}
return i;
}

// 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
}
return i; // CHECK-NEXT: [[@LINE]]:3 -> [[@LINE]]:11 = (#0 + #1)
}

// 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
}
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);
return i;
}

0 comments on commit a26cc75

Please sign in to comment.