https://github.com/hanickadot updated https://github.com/llvm/llvm-project/pull/77214
From 413517b2a1d4e45b6c58ab282c7990e83f429ab9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hana=20Dusi=CC=81kova=CC=81?= <hani...@hanicka.net> Date: Mon, 8 Jan 2024 11:54:45 +0100 Subject: [PATCH 1/2] [coverage] fix incorrect coverage reporting for "if constexpr" and "if consteval" --- clang/docs/ReleaseNotes.rst | 3 + clang/include/clang/AST/Stmt.h | 6 +- clang/lib/CodeGen/CoverageMappingGen.cpp | 13 +++- clang/lib/Sema/TreeTransform.h | 10 ++- clang/test/CoverageMapping/if.cpp | 86 +++++++++++++++++++----- 5 files changed, 96 insertions(+), 22 deletions(-) diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst index c9b577bd549b1e..c36e051d055b72 100644 --- a/clang/docs/ReleaseNotes.rst +++ b/clang/docs/ReleaseNotes.rst @@ -696,6 +696,9 @@ Bug Fixes in This Version - Clang now accepts recursive non-dependent calls to functions with deduced return type. Fixes (`#71015 <https://github.com/llvm/llvm-project/issues/71015>`_) +- 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 diff --git a/clang/include/clang/AST/Stmt.h b/clang/include/clang/AST/Stmt.h index da7b37ce0e1211..e1fde24e647789 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) {} + + 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..19266a7ffa2fe0 100644 --- a/clang/lib/Sema/TreeTransform.h +++ b/clang/lib/Sema/TreeTransform.h @@ -7732,7 +7732,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. @@ -7741,6 +7745,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() && diff --git a/clang/test/CoverageMapping/if.cpp b/clang/test/CoverageMapping/if.cpp index 65e3d62df79db4..cc18f791c2f79e 100644 --- a/clang/test/CoverageMapping/if.cpp +++ b/clang/test/CoverageMapping/if.cpp @@ -23,19 +23,29 @@ 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: _Z16check_constexprAi: +int check_constexprA(int i) { // CHECK-NEXT: [[@LINE]]:29 -> {{[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: _Z16check_constexprBi: +int check_constexprB(int i) { // CHECK-NEXT: [[@LINE]]:29 -> {{[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: main: @@ -75,10 +85,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 @@ -98,3 +104,49 @@ int main() { // CHECK: File 0, [[@LINE]]:12 -> {{[0-9]+}}:2 = void ternary() { true ? FOO : FOO; // CHECK-NOT: Gap,{{.*}}, [[@LINE]]:8 -> } + +// FIXME: Do not generate coverage for discarded branches in if consteval +// GH-57377 +// CHECK-LABEL: _Z16check_constevalAi: +constexpr int check_constevalA(int i) { // CHECK-NEXT: [[@LINE]]:39 -> {{[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: _Z16check_constevalBi: +constexpr int check_constevalB(int i) { // CHECK-NEXT: [[@LINE]]:39 -> {{[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: _Z16check_constevalCi: +constexpr int check_constevalC(int i) { // CHECK-NEXT: [[@LINE]]:39 -> {{[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: _Z16check_constevalDi: +constexpr int check_constevalD(int i) { // CHECK-NEXT: [[@LINE]]:39 -> {{[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_constevalA(i); + i *= check_constevalB(i); + i *= check_constevalC(i); + i *= check_constevalD(i); + return i; +} From f46e9d6da2a3ec425c5ba2e8f9c1918aa0b1ea11 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hana=20Dusi=CC=81kova=CC=81?= <hani...@hanicka.net> Date: Tue, 9 Jan 2024 06:53:10 +0100 Subject: [PATCH 2/2] [coverage] fix incorrect coverage reporting for "if constexpr" and "if consteval" (updated tests and added comments) --- clang/lib/Sema/TreeTransform.h | 3 ++ clang/test/CoverageMapping/if.cpp | 56 +++++++++++++++++++++---------- 2 files changed, 42 insertions(+), 17 deletions(-) diff --git a/clang/lib/Sema/TreeTransform.h b/clang/lib/Sema/TreeTransform.h index 19266a7ffa2fe0..2e52729384a392 100644 --- a/clang/lib/Sema/TreeTransform.h +++ b/clang/lib/Sema/TreeTransform.h @@ -7747,6 +7747,9 @@ TreeTransform<Derived>::TransformIfStmt(IfStmt *S) { 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()); } diff --git a/clang/test/CoverageMapping/if.cpp b/clang/test/CoverageMapping/if.cpp index cc18f791c2f79e..92d560be01f3b7 100644 --- a/clang/test/CoverageMapping/if.cpp +++ b/clang/test/CoverageMapping/if.cpp @@ -24,8 +24,8 @@ void foo() { // CHECK-NEXT: Gap,File 0, [[@LINE+1]]:21 -> [[@ // CHECK-NEXT: [[@LINE-2]]:5 -> [[@LINE-2]]:8 = #1 // FIXME: Do not generate coverage for discarded branches in if constexpr -// CHECK-LABEL: _Z16check_constexprAi: -int check_constexprA(int i) { // CHECK-NEXT: [[@LINE]]:29 -> {{[0-9]+}}:2 = #0 +// 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 @@ -36,8 +36,18 @@ int check_constexprA(int i) { // CHECK-NEXT: [[@LINE]]:29 -> {{[0-9]+}}:2 = #0 return i; } -// CHECK-LABEL: _Z16check_constexprBi: -int check_constexprB(int i) { // CHECK-NEXT: [[@LINE]]:29 -> {{[0-9]+}}:2 = #0 +// 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 @@ -48,6 +58,16 @@ int check_constexprB(int i) { // CHECK-NEXT: [[@LINE]]:29 -> {{[0-9]+}}:2 = #0 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: int main() { // CHECK: File 0, [[@LINE]]:12 -> {{[0-9]+}}:2 = #0 int i = 0; @@ -105,10 +125,10 @@ void ternary() { true ? FOO : FOO; // CHECK-NOT: Gap,{{.*}}, [[@LINE]]:8 -> } -// FIXME: Do not generate coverage for discarded branches in if consteval // GH-57377 -// CHECK-LABEL: _Z16check_constevalAi: -constexpr int check_constevalA(int i) { // CHECK-NEXT: [[@LINE]]:39 -> {{[0-9]+}}:2 = #0 +// 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 @@ -117,8 +137,9 @@ constexpr int check_constevalA(int i) { // CHECK-NEXT: [[@LINE]]:39 -> {{[0-9]+} return i; // CHECK-NEXT: [[@LINE]]:3 -> [[@LINE]]:11 = (#0 + #1) } -// CHECK-LABEL: _Z16check_constevalBi: -constexpr int check_constevalB(int i) { // CHECK-NEXT: [[@LINE]]:39 -> {{[0-9]+}}:2 = #0 +// 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 @@ -127,16 +148,17 @@ constexpr int check_constevalB(int i) { // CHECK-NEXT: [[@LINE]]:39 -> {{[0-9]+} return i; } -// CHECK-LABEL: _Z16check_constevalCi: -constexpr int check_constevalC(int i) { // CHECK-NEXT: [[@LINE]]:39 -> {{[0-9]+}}:2 = #0 +// 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: _Z16check_constevalDi: -constexpr int check_constevalD(int i) { // CHECK-NEXT: [[@LINE]]:39 -> {{[0-9]+}}:2 = #0 +// 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 } @@ -144,9 +166,9 @@ constexpr int check_constevalD(int i) { // CHECK-NEXT: [[@LINE]]:39 -> {{[0-9]+} } int instantiate_consteval(int i) { - i *= check_constevalA(i); - i *= check_constevalB(i); - i *= check_constevalC(i); - i *= check_constevalD(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; } _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits