[clang] [clang][coverage] fixing "if constexpr" and "if consteval" coverage report (PR #77214)

[via cfe-commits]

https://github.com/cor3ntin closed 
https://github.com/llvm/llvm-project/pull/77214
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][coverage] fixing "if constexpr" and "if consteval" coverage report (PR #77214)

[Hana Dusíková via cfe-commits]

https://github.com/hanickadot updated 
https://github.com/llvm/llvm-project/pull/77214

From 766df92436569a924ef1c3860b3e1019c2048b53 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Hana=20Dusi=CC=81kova=CC=81?= 
Date: Wed, 10 Jan 2024 10:00:31 +0100
Subject: [PATCH] [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   |  13 ++-
 clang/test/CoverageMapping/if.cpp| 108 +++
 5 files changed, 121 insertions(+), 22 deletions(-)

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 20872f7ddb81e9..37f8bbc89d8949 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -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 `_)
+- Clang now emits correct source location for code-coverage regions in `if 
constexpr`
+  and `if consteval` branches.
+  Fixes (`#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 e7a6550b1c993c..1a1bc87d2b3203 100644
--- a/clang/lib/Sema/TreeTransform.h
+++ b/clang/lib/Sema/TreeTransform.h
@@ -7739,7 +7739,11 @@ TreeTransform::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.
@@ -7748,6 +7752,13 @@ TreeTransform::TransformIfStmt(IfStmt *S) {
 Else = getDerived().TransformStmt(S->getElse());
 if (Else.isInvalid())
   return StmtError();
+  } else if (S->getElse() && ConstexprConditionValue &&
+ *ConstexprConditionValue) {
+// Same thing here as with  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() &&
diff --git a/clang/test/CoverageMapping/if.cpp 
b/clang/test/CoverageMapping/if.cpp
index 

[clang] [clang][coverage] fixing "if constexpr" and "if consteval" coverage report (PR #77214)

[via cfe-commits]

Hana =?utf-8?q?Dusi=CC=81kova=CC=81?= 
Message-ID:
In-Reply-To: 


cor3ntin wrote:

Oups, can you rebase?

https://github.com/llvm/llvm-project/pull/77214
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][coverage] fixing "if constexpr" and "if consteval" coverage report (PR #77214)

[Hana Dusíková via cfe-commits]

hanickadot wrote:

yes please, I don't have merge rights

https://github.com/llvm/llvm-project/pull/77214
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][coverage] fixing "if constexpr" and "if consteval" coverage report (PR #77214)

[via cfe-commits]

Hana =?utf-8?q?Dusi=CC=81kova=CC=81?= 
Message-ID:
In-Reply-To: 


https://github.com/cor3ntin approved this pull request.

LGTM
@hanickadot do you need me to merge that for you?

https://github.com/llvm/llvm-project/pull/77214
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][coverage] fixing "if constexpr" and "if consteval" coverage report (PR #77214)

[Jessica Paquette via cfe-commits]

Hana =?utf-8?q?Dusi=CC=81kova=CC=81?= 
Message-ID:
In-Reply-To: 


https://github.com/ornata approved this pull request.


https://github.com/llvm/llvm-project/pull/77214
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][coverage] fixing "if constexpr" and "if consteval" coverage report (PR #77214)

[Jessica Paquette via cfe-commits]

Hana =?utf-8?q?Dusi=CC=81kova=CC=81?= 
Message-ID:
In-Reply-To: 


ornata wrote:

LGTM

https://github.com/llvm/llvm-project/pull/77214
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][coverage] fixing "if constexpr" and "if consteval" coverage report (PR #77214)

[Hana Dusíková via cfe-commits]

@@ -7732,7 +7732,11 @@ TreeTransform::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

hanickadot wrote:

IMHO There is nothing to fix later. This will stay, it replaces former using of 
NullStmt in  branch and NULL in  branch.

https://github.com/llvm/llvm-project/pull/77214
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][coverage] fixing "if constexpr" and "if consteval" coverage report (PR #77214)

[Hana Dusíková via cfe-commits]

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?= 
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 `_)
+- Clang now emits correct source location for code-coverage regions in `if 
constexpr`
+  and `if consteval` branches.
+  Fixes (`#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::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::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 -> [[@
 }  

[clang] [clang][coverage] fixing "if constexpr" and "if consteval" coverage report (PR #77214)

[Jessica Paquette via cfe-commits]

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

ornata wrote:

Maybe `check_if_consteval_with_discarded_branch` would be a better name?

https://github.com/llvm/llvm-project/pull/77214
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][coverage] fixing "if constexpr" and "if consteval" coverage report (PR #77214)

[Jessica Paquette via cfe-commits]

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

ornata wrote:

Similarly, this test and the below one could just be called 
`check_if_consteval` and `check_if_not_consteval`

https://github.com/llvm/llvm-project/pull/77214
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][coverage] fixing "if constexpr" and "if consteval" coverage report (PR #77214)

[Jessica Paquette via cfe-commits]

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

ornata wrote:

Similarly, this would be better-named something like `check_if_constexpr_false`

https://github.com/llvm/llvm-project/pull/77214
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][coverage] fixing "if constexpr" and "if consteval" coverage report (PR #77214)

[Jessica Paquette via cfe-commits]

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

ornata wrote:

Also does this one need a FIXME like in the above test?

https://github.com/llvm/llvm-project/pull/77214
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][coverage] fixing "if constexpr" and "if consteval" coverage report (PR #77214)

[Jessica Paquette via cfe-commits]

@@ -7741,6 +7745,10 @@ TreeTransform::TransformIfStmt(IfStmt *S) {
 Else = getDerived().TransformStmt(S->getElse());
 if (Else.isInvalid())
   return StmtError();
+  } else if (S->getElse() && ConstexprConditionValue &&

ornata wrote:

Could you add a comment similar to the one in the `else` above?

https://github.com/llvm/llvm-project/pull/77214
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][coverage] fixing "if constexpr" and "if consteval" coverage report (PR #77214)

[Jessica Paquette via cfe-commits]

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

ornata wrote:

I think this test should be named `check_if_constexpr_true` or something similar

https://github.com/llvm/llvm-project/pull/77214
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][coverage] fixing "if constexpr" and "if consteval" coverage report (PR #77214)

[Jessica Paquette via cfe-commits]

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

ornata wrote:

Specifically, it's the `else` region that shouldn't generate any coverage 
mapping data, right?

It'd be good to mention that in the `FIXME`; something like

> FIXME: Do not generate coverage for discarded branches in if consteval. The 
> else case is discarded and should not be covered.

https://github.com/llvm/llvm-project/pull/77214
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][coverage] fixing "if constexpr" and "if consteval" coverage report (PR #77214)

[Jessica Paquette via cfe-commits]

https://github.com/ornata commented:

Added some comments, mostly nits on the test.

https://github.com/llvm/llvm-project/pull/77214
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][coverage] fixing "if constexpr" and "if consteval" coverage report (PR #77214)

[Jessica Paquette via cfe-commits]

@@ -7732,7 +7732,11 @@ TreeTransform::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

ornata wrote:

In the test, you mention that we're generating coverage for the discarded 
regions. Is the reason that we're generating a `CompoundStmt` here? Could this 
use a `FIXME`?

https://github.com/llvm/llvm-project/pull/77214
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][coverage] fixing "if constexpr" and "if consteval" coverage report (PR #77214)

[Jessica Paquette via cfe-commits]

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

ornata wrote:

Maybe `check_if_not_consteval_with_discarded_branch` would be a more 
descriptive name?

https://github.com/llvm/llvm-project/pull/77214
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][coverage] fixing "if constexpr" and "if consteval" coverage report (PR #77214)

[Jessica Paquette via cfe-commits]

https://github.com/ornata edited https://github.com/llvm/llvm-project/pull/77214
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][coverage] fixing "if constexpr" and "if consteval" coverage report (PR #77214)

[Hana Dusíková via cfe-commits]

hanickadot wrote:

Added some tests. And also adding @ornata for review.

https://github.com/llvm/llvm-project/pull/77214
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][coverage] fixing "if constexpr" and "if consteval" coverage report (PR #77214)

[Hana Dusíková via cfe-commits]

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?= 
Date: Mon, 8 Jan 2024 11:54:45 +0100
Subject: [PATCH] [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 `_)
+- Clang now emits correct source location for code-coverage regions in `if 
constexpr`
+  and `if consteval` branches.
+  Fixes (`#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::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::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 -> [[@
 }  

[clang] [clang][coverage] fixing "if constexpr" and "if consteval" coverage report (PR #77214)

[Hana Dusíková via cfe-commits]

https://github.com/hanickadot updated 
https://github.com/llvm/llvm-project/pull/77214

From 2dc9d821aa3d100f99ac603a0880498226c94323 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Hana=20Dusi=CC=81kova=CC=81?= 
Date: Sun, 7 Jan 2024 00:13:08 +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 +-
 4 files changed, 27 insertions(+), 5 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 `_)
+- Clang now emits correct source location for code-coverage regions in `if 
constexpr`
+  and `if consteval` branches.
+  Fixes (`#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::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::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() &&

From 37879a5b7ec3f98ce423672d2bf43efe826ab5af Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Hana=20Dusi=CC=81kova=CC=81?= 
Date: Sun, 7 Jan 2024 00:13:08 +0100
Subject: [PATCH 2/2] [coverage] fix incorrect coverage reporting for "if
 constexpr" and "if consteval"

---
 clang/test/CoverageMapping/if.cpp | 86 +--
 1 file changed, 69 insertions(+), 17 deletions(-)

[clang] [clang][coverage] fixing "if constexpr" and "if consteval" coverage report (PR #77214)

[Hana Dusíková via cfe-commits]

https://github.com/hanickadot updated 
https://github.com/llvm/llvm-project/pull/77214

From 5aa041529997724e19109c45cf1483eb72d7dab6 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Hana=20Dusi=CC=81kova=CC=81?= 
Date: Sun, 7 Jan 2024 00:13:08 +0100
Subject: [PATCH] [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 +-
 4 files changed, 27 insertions(+), 5 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 `_)
+- Clang now emits correct source location for code-coverage regions in `if 
constexpr`
+  and `if consteval` branches.
+  Fixes (`#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::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::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() &&

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][coverage] fixing "if constexpr" and "if consteval" coverage report (PR #77214)

[Hana Dusíková via cfe-commits]

hanickadot wrote:

Yes, and it will be a bit bigger change. This is currently my biggest change 
yet :) But I want to do it as next. This fix's intention is to make the source 
location of regions properly done.

https://github.com/llvm/llvm-project/pull/77214
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][coverage] fixing "if constexpr" and "if consteval" coverage report (PR #77214)

[Paweł Bylica via cfe-commits]

chfast wrote:

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

This would be very useful not to treat compile-time unreachable code as 
uncovered. Can the skipped code be marked separately in the reports?

https://github.com/llvm/llvm-project/pull/77214
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][coverage] fixing "if constexpr" and "if consteval" coverage report (PR #77214)

[Hana Dusíková via cfe-commits]

https://github.com/hanickadot updated 
https://github.com/llvm/llvm-project/pull/77214

From fb2efdecce3a629c801d1f179297b1189e1ec287 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Hana=20Dusi=CC=81kova=CC=81?= 
Date: Sun, 7 Jan 2024 00:13:08 +0100
Subject: [PATCH] [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 +-
 4 files changed, 27 insertions(+), 5 deletions(-)

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index c9b577bd549b1e..04617e9236efc6 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 `_)
+- 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::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::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() &&

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][coverage] fixing "if constexpr" and "if consteval" coverage report (PR #77214)

[Hana Dusíková via cfe-commits]

@@ -7732,7 +7732,8 @@ TreeTransform::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());

hanickadot wrote:

Comment added :)

https://github.com/llvm/llvm-project/pull/77214
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][coverage] fixing "if constexpr" and "if consteval" coverage report (PR #77214)

[via cfe-commits]

cor3ntin wrote:

Adding a few folks to the review (We do not seem to update the coverage mapping 
very often)

https://github.com/llvm/llvm-project/pull/77214
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][coverage] fixing "if constexpr" and "if consteval" coverage report (PR #77214)

[Hana Dusíková via cfe-commits]

https://github.com/hanickadot updated 
https://github.com/llvm/llvm-project/pull/77214

From 1891847f50311c4ffbaa40577900eca2245c0a7c Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Hana=20Dusi=CC=81kova=CC=81?= 
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   | 10 +-
 3 files changed, 24 insertions(+), 5 deletions(-)

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::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::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() &&

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][coverage] fixing "if constexpr" and "if consteval" coverage report (PR #77214)

[via cfe-commits]

@@ -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) {}

cor3ntin wrote:

Hum, never mind, I misread the diff

https://github.com/llvm/llvm-project/pull/77214
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][coverage] fixing "if constexpr" and "if consteval" coverage report (PR #77214)

[Hana Dusíková via cfe-commits]

hanickadot wrote:

Fixes https://github.com/llvm/llvm-project/issues/54419.

https://github.com/llvm/llvm-project/pull/77214
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][coverage] fixing "if constexpr" and "if consteval" coverage report (PR #77214)

[via cfe-commits]

@@ -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) {}

cor3ntin wrote:

That does not seem to be used

https://github.com/llvm/llvm-project/pull/77214
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][coverage] fixing "if constexpr" and "if consteval" coverage report (PR #77214)

[via cfe-commits]

https://github.com/cor3ntin edited 
https://github.com/llvm/llvm-project/pull/77214
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][coverage] fixing "if constexpr" and "if consteval" coverage report (PR #77214)

[via cfe-commits]

https://github.com/cor3ntin commented:

Thanks for the patch!

I think this might need a release note (referencing the couple open issues 
about this bug), and probably some tests.

https://github.com/llvm/llvm-project/pull/77214
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][coverage] fixing "if constexpr" and "if consteval" coverage report (PR #77214)

[via cfe-commits]

@@ -7732,7 +7732,8 @@ TreeTransform::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());

cor3ntin wrote:

I think this is fine ( I don't see a better solution) but it probably warrants 
a comment.

https://github.com/llvm/llvm-project/pull/77214
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][coverage] fixing "if constexpr" and "if consteval" coverage report (PR #77214)

[Hana Dusíková via cfe-commits]

https://github.com/hanickadot updated 
https://github.com/llvm/llvm-project/pull/77214

From bde3362e609a129f410890745b1dcfd635ba2ec0 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Hana=20Dusi=CC=81kova=CC=81?= 
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..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..0033c851b618a1 100644
--- a/clang/lib/Sema/TreeTransform.h
+++ b/clang/lib/Sema/TreeTransform.h
@@ -7732,7 +7732,8 @@ TreeTransform::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::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() &&

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[mlir] [llvm] [clang] [clang][coverage] fixing "if constexpr" and "if consteval" coverage report (PR #77214)

[Hana Dusíková via cfe-commits]

https://github.com/hanickadot updated 
https://github.com/llvm/llvm-project/pull/77214

From 8f1370aae4db2048c35516a85fb72c742557942b Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Hana=20Dusi=CC=81kova=CC=81?= 
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..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..0033c851b618a1 100644
--- a/clang/lib/Sema/TreeTransform.h
+++ b/clang/lib/Sema/TreeTransform.h
@@ -7732,7 +7732,8 @@ TreeTransform::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::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() &&

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][coverage] fixing "if constexpr" and "if consteval" coverage report (PR #77214)

[Hana Dusíková via cfe-commits]

https://github.com/hanickadot edited 
https://github.com/llvm/llvm-project/pull/77214
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][coverage] fixing "if constexpr" and "if consteval" coverage report (PR #77214)

[Hana Dusíková via cfe-commits]

https://github.com/hanickadot updated 
https://github.com/llvm/llvm-project/pull/77214

From 8f1370aae4db2048c35516a85fb72c742557942b Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Hana=20Dusi=CC=81kova=CC=81?= 
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..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..0033c851b618a1 100644
--- a/clang/lib/Sema/TreeTransform.h
+++ b/clang/lib/Sema/TreeTransform.h
@@ -7732,7 +7732,8 @@ TreeTransform::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::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() &&

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][coverage] fixing "if constexpr" and "if consteval" coverage report (PR #77214)

[Hana Dusíková via cfe-commits]

hanickadot wrote:

https://github.com/llvm/llvm-project/assets/6557263/47ff77c0-9101-44cf-b2d5-ffea514bfc0c;>

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



https://github.com/llvm/llvm-project/pull/77214
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][coverage] fixing "if constexpr" and "if consteval" coverage report (PR #77214)

[Hana Dusíková via cfe-commits]

https://github.com/hanickadot edited 
https://github.com/llvm/llvm-project/pull/77214
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][coverage] fixing "if constexpr" and "if consteval" coverage report (PR #77214)

[via cfe-commits]

llvmbot wrote:




@llvm/pr-subscribers-clang

Author: Hana Dusíková (hanickadot)


Changes

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.

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


3 Files Affected:

- (modified) clang/include/clang/AST/Stmt.h (+4-2) 
- (modified) clang/lib/CodeGen/CoverageMappingGen.cpp (+11-2) 
- (modified) clang/lib/Sema/TreeTransform.h (+6-1) 


``diff
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::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::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() &&

``




https://github.com/llvm/llvm-project/pull/77214
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][coverage] fixing "if constexpr" and "if consteval" coverage report (PR #77214)

[Hana Dusíková via cfe-commits]

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).
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:
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?= 
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::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::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() &&

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits