[clang] [clang][c++20] Fix code coverage mapping crash with generalized NTTPs (PR #85837)

[Andrey Ali Khan Bolshakov via cfe-commits]

bolshakov-a wrote:

Ok, got it. Thanks!

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

[clang] [clang][c++20] Fix code coverage mapping crash with generalized NTTPs (PR #85837)

[Eli Friedman via cfe-commits]

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

[clang] [clang][c++20] Fix code coverage mapping crash with generalized NTTPs (PR #85837)

[Eli Friedman via cfe-commits]

https://github.com/efriedma-quic approved this pull request.

LGTM

(In the future, please leave a note on the pull request when you push an 
update; as far as 
I can tell, GitHub doesn't generate a notification when you force-push to the 
branch.)

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

[clang] [clang][c++20] Fix code coverage mapping crash with generalized NTTPs (PR #85837)

[Andrey Ali Khan Bolshakov via cfe-commits]

bolshakov-a wrote:

@efriedma-quic ping.

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

[clang] [clang][c++20] Fix code coverage mapping crash with generalized NTTPs (PR #85837)

[Andrey Ali Khan Bolshakov via cfe-commits]

https://github.com/bolshakov-a updated 
https://github.com/llvm/llvm-project/pull/85837

>From 61abd7b6089ecb87eaf6886e251969d4db87e223 Mon Sep 17 00:00:00 2001
From: Bolshakov 
Date: Tue, 19 Mar 2024 19:05:36 +0300
Subject: [PATCH 1/2] [clang][c++20] Fix code coverage mapping crash with
 generalized NTTPs

Introduced in #78041, originally reported as #79957 and fixed partially
in #80050.

`OpaqueValueExpr` used with `TemplateArgument::StructuralValue` has no
corresponding source expression.

A test case with subobject-referring NTTP added.
---
 clang/lib/CodeGen/CoverageMappingGen.cpp | 3 ++-
 clang/test/CoverageMapping/templates.cpp | 3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/clang/lib/CodeGen/CoverageMappingGen.cpp 
b/clang/lib/CodeGen/CoverageMappingGen.cpp
index cc8ab7a5b4369..48896fa03d9ff 100644
--- a/clang/lib/CodeGen/CoverageMappingGen.cpp
+++ b/clang/lib/CodeGen/CoverageMappingGen.cpp
@@ -2187,7 +2187,8 @@ struct CounterCoverageMappingBuilder
   }
 
   void VisitOpaqueValueExpr(const OpaqueValueExpr* OVE) {
-Visit(OVE->getSourceExpr());
+if (const Expr *SE = OVE->getSourceExpr())
+  Visit(SE);
   }
 };
 
diff --git a/clang/test/CoverageMapping/templates.cpp 
b/clang/test/CoverageMapping/templates.cpp
index 143e566a33cb8..7e7f2208f1145 100644
--- a/clang/test/CoverageMapping/templates.cpp
+++ b/clang/test/CoverageMapping/templates.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -mllvm -emptyline-comment-coverage=false 
-fprofile-instrument=clang -fcoverage-mapping -dump-coverage-mapping 
-emit-llvm-only -main-file-name templates.cpp %s | FileCheck %s
+// RUN: %clang_cc1 -std=c++20 -mllvm -emptyline-comment-coverage=false 
-fprofile-instrument=clang -fcoverage-mapping -dump-coverage-mapping 
-emit-llvm-only -main-file-name templates.cpp %s | FileCheck %s
 
 template
 void unused(T x) {
@@ -30,5 +30,6 @@ namespace structural_value_crash {
 
   void test() {
 tpl_fn();
+tpl_fn<[1]>();
   }
 }

>From 9684fed244607e3c7923c0a3118945c559e2d5d5 Mon Sep 17 00:00:00 2001
From: Bolshakov 
Date: Thu, 16 May 2024 12:18:17 +0300
Subject: [PATCH 2/2] Fix up: check `isUnique` instead of source expr presence

---
 clang/lib/CodeGen/CoverageMappingGen.cpp | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/clang/lib/CodeGen/CoverageMappingGen.cpp 
b/clang/lib/CodeGen/CoverageMappingGen.cpp
index 48896fa03d9ff..5f62d2ae46bdb 100644
--- a/clang/lib/CodeGen/CoverageMappingGen.cpp
+++ b/clang/lib/CodeGen/CoverageMappingGen.cpp
@@ -2187,8 +2187,8 @@ struct CounterCoverageMappingBuilder
   }
 
   void VisitOpaqueValueExpr(const OpaqueValueExpr* OVE) {
-if (const Expr *SE = OVE->getSourceExpr())
-  Visit(SE);
+if (OVE->isUnique())
+  Visit(OVE->getSourceExpr());
   }
 };
 

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

[clang] [clang][c++20] Fix code coverage mapping crash with generalized NTTPs (PR #85837)

[Andrey Ali Khan Bolshakov via cfe-commits]

@@ -2177,7 +2177,8 @@ struct CounterCoverageMappingBuilder
   }
 
   void VisitOpaqueValueExpr(const OpaqueValueExpr* OVE) {
-Visit(OVE->getSourceExpr());
+if (const Expr *SE = OVE->getSourceExpr())

bolshakov-a wrote:

Makes sense, thanks! Moreover, I've really found a couple of bugs when some 
expressions referred by `OpaqueValueExpr` are handled more than once (#1 
and #88898). Regarding `BinaryConditionalOperator`, I've decided to refactor it 
[in a separate PR](https://github.com/llvm/llvm-project/pull/88910) and then to 
introduce the `isUnique()` check in this PR, hoping that there will be no more 
regressions.

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

[clang] [clang][c++20] Fix code coverage mapping crash with generalized NTTPs (PR #85837)

[Eli Friedman via cfe-commits]

@@ -2177,7 +2177,8 @@ struct CounterCoverageMappingBuilder
   }
 
   void VisitOpaqueValueExpr(const OpaqueValueExpr* OVE) {
-Visit(OVE->getSourceExpr());
+if (const Expr *SE = OVE->getSourceExpr())

efriedma-quic wrote:

The point of the "unique" flag is precisely to indicate whether a subexpression 
is naturally part of the tree at that point, or it's referring to an expression 
which should have been evaluated elsewhere.  So not visiting a non-unique 
expression should be the right default.

I didn't realize the existing AbstractConditionalOperator code already had a 
workaround for this... we should be able to adjust it to compensate.

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

[clang] [clang][c++20] Fix code coverage mapping crash with generalized NTTPs (PR #85837)

[Andrey Ali Khan Bolshakov via cfe-commits]

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

[clang] [clang][c++20] Fix code coverage mapping crash with generalized NTTPs (PR #85837)

[Andrey Ali Khan Bolshakov via cfe-commits]

@@ -2177,7 +2177,8 @@ struct CounterCoverageMappingBuilder
   }
 
   void VisitOpaqueValueExpr(const OpaqueValueExpr* OVE) {
-Visit(OVE->getSourceExpr());
+if (const Expr *SE = OVE->getSourceExpr())

bolshakov-a wrote:

> If I'm following correctly, you end up visiting the condition twice

No, the "true" branch visitation (`propagateCounts`) is performed only when it 
is the ordinary `ConditionalOperator` 
(https://github.com/llvm/llvm-project/blob/main/clang/lib/CodeGen/CoverageMappingGen.cpp#L2018).

Do you think it should be rewritten first? It's just the first case found by me 
where a non-unique OVE has a source expression which is visited. There may be 
more cases.

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

[clang] [clang][c++20] Fix code coverage mapping crash with generalized NTTPs (PR #85837)

[Eli Friedman via cfe-commits]

@@ -2177,7 +2177,8 @@ struct CounterCoverageMappingBuilder
   }
 
   void VisitOpaqueValueExpr(const OpaqueValueExpr* OVE) {
-Visit(OVE->getSourceExpr());
+if (const Expr *SE = OVE->getSourceExpr())

efriedma-quic wrote:

If you have a BinaryConditionalOperator, I'm not sure this does the right 
thing.  If I'm following correctly, you end up visiting the condition twice, 
which is not accurately reflecting the way this actually executes.  The correct 
way to visit a BinaryConditionalOperator is to visit the "common" expression 
(BinaryConditionalOperator::getCommon()) first, then don't recurse into the 
OpaqueValueExpr.

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

[clang] [clang][c++20] Fix code coverage mapping crash with generalized NTTPs (PR #85837)

[Erich Keane via cfe-commits]

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

This seems fine to me, but please give @efriedma-quic a few days to do a final 
pass through.

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

[clang] [clang][c++20] Fix code coverage mapping crash with generalized NTTPs (PR #85837)

[Andrey Ali Khan Bolshakov via cfe-commits]

bolshakov-a wrote:

@erichkeane, @cor3ntin, @Endilll, @efriedma-quic ping.

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

[clang] [clang][c++20] Fix code coverage mapping crash with generalized NTTPs (PR #85837)

[Andrey Ali Khan Bolshakov via cfe-commits]

@@ -2177,7 +2177,8 @@ struct CounterCoverageMappingBuilder
   }
 
   void VisitOpaqueValueExpr(const OpaqueValueExpr* OVE) {
-Visit(OVE->getSourceExpr());
+if (const Expr *SE = OVE->getSourceExpr())

bolshakov-a wrote:

Not all `OpaqueValueExpr`s having a source expression are marked as "unique". 
Checking `isUnique()` instead of `getSourceExpr()` breaks at least handling of 
the GNU extension of the `?:` operator:
```
1|  1|int main() {
2|  1|int i = 1;
3|  1|return (i ? 1 : 0)
  ^0<-- disappears if isUnique() is used
  --
  |  Branch (3:12): [True: 1, False: 0]
  |  Branch (3:13): [True: 1, False: 0] <-- disappears if isUnique() is used
  --
4|  1|?: 10;
 ^0
5|  1|}
```

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

[clang] [clang][c++20] Fix code coverage mapping crash with generalized NTTPs (PR #85837)

[Eli Friedman via cfe-commits]

@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -mllvm -emptyline-comment-coverage=false 
-fprofile-instrument=clang -fcoverage-mapping -dump-coverage-mapping 
-emit-llvm-only -main-file-name templates.cpp %s | FileCheck %s
+// RUN: %clang_cc1 -std=c++20 -mllvm -emptyline-comment-coverage=false 
-fprofile-instrument=clang -fcoverage-mapping -dump-coverage-mapping 
-emit-llvm-only -main-file-name templates.cpp %s | FileCheck %s

efriedma-quic wrote:

If you don't have any reason to expect the shared cases might differently in 
earlier standard versions, this is fine.

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

[clang] [clang][c++20] Fix code coverage mapping crash with generalized NTTPs (PR #85837)

[Eli Friedman via cfe-commits]

@@ -2177,7 +2177,8 @@ struct CounterCoverageMappingBuilder
   }
 
   void VisitOpaqueValueExpr(const OpaqueValueExpr* OVE) {
-Visit(OVE->getSourceExpr());
+if (const Expr *SE = OVE->getSourceExpr())

efriedma-quic wrote:

I suspect the correct check here is `if (OVE->isUnique())`?  See 
https://reviews.llvm.org/D39562 .  CC @ahatanak.

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

[clang] [clang][c++20] Fix code coverage mapping crash with generalized NTTPs (PR #85837)

[Andrey Ali Khan Bolshakov via cfe-commits]

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