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

bolshakov-a · 2024-03-19T18:17:22Z

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.

@erichkeane, @cor3ntin, @Endilll

llvmbot · 2024-03-19T18:17:53Z

@llvm/pr-subscribers-clang-codegen

Author: Andrey Ali Khan Bolshakov (bolshakov-a)

Changes

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.

@erichkeane, @cor3ntin, @Endilll

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

2 Files Affected:

(modified) clang/lib/CodeGen/CoverageMappingGen.cpp (+2-1)
(modified) clang/test/CoverageMapping/templates.cpp (+2-1)

diff --git a/clang/lib/CodeGen/CoverageMappingGen.cpp b/clang/lib/CodeGen/CoverageMappingGen.cpp
index 71215da362d3d0..3a87f935470673 100644
--- a/clang/lib/CodeGen/CoverageMappingGen.cpp
+++ b/clang/lib/CodeGen/CoverageMappingGen.cpp
@@ -2177,7 +2177,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 143e566a33cb85..7e7f2208f1145f 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<typename T>
 void unused(T x) {
@@ -30,5 +30,6 @@ namespace structural_value_crash {
 
   void test() {
     tpl_fn<arr>();
+    tpl_fn<&arr[1]>();
   }
 }

llvmbot · 2024-03-19T18:17:53Z

@llvm/pr-subscribers-clang

Author: Andrey Ali Khan Bolshakov (bolshakov-a)

Changes

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.

@erichkeane, @cor3ntin, @Endilll

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

2 Files Affected:

(modified) clang/lib/CodeGen/CoverageMappingGen.cpp (+2-1)
(modified) clang/test/CoverageMapping/templates.cpp (+2-1)

diff --git a/clang/lib/CodeGen/CoverageMappingGen.cpp b/clang/lib/CodeGen/CoverageMappingGen.cpp
index 71215da362d3d0..3a87f935470673 100644
--- a/clang/lib/CodeGen/CoverageMappingGen.cpp
+++ b/clang/lib/CodeGen/CoverageMappingGen.cpp
@@ -2177,7 +2177,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 143e566a33cb85..7e7f2208f1145f 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<typename T>
 void unused(T x) {
@@ -30,5 +30,6 @@ namespace structural_value_crash {
 
   void test() {
     tpl_fn<arr>();
+    tpl_fn<&arr[1]>();
   }
 }

bolshakov-a · 2024-03-19T18:19:29Z

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


Maybe, separate launch mode should be added instead of changing existing one? OTOH, it was irrelevant to any specific C++ standard.

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

efriedma-quic · 2024-03-27T21:15:01Z

clang/lib/CodeGen/CoverageMappingGen.cpp

@@ -2177,7 +2177,8 @@ struct CounterCoverageMappingBuilder
  }

  void VisitOpaqueValueExpr(const OpaqueValueExpr* OVE) {
-    Visit(OVE->getSourceExpr());
+    if (const Expr *SE = OVE->getSourceExpr())


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

Not all OpaqueValueExprs 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|}

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.

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/llvmorg-18.1.3/clang/lib/CodeGen/CoverageMappingGen.cpp#L1949).

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.

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.

Makes sense, thanks! Moreover, I've really found a couple of bugs when some expressions referred by OpaqueValueExpr are handled more than once (#88881 and #88898). Regarding BinaryConditionalOperator, I've decided to refactor it in a separate PR and then to introduce the isUnique() check in this PR, hoping that there will be no more regressions.

efriedma-quic · 2024-03-27T21:15:35Z

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


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

bolshakov-a · 2024-04-09T00:41:27Z

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

erichkeane

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

Only unique `OpaqueValueExpr`s should be handled in the mapping builder, as discussed in llvm#85837. However, `getCond()` returns non-unique `OpaqueValueExpr` for `BinaryConditionalOperator` (because it is also used as the "true" branch expression). Use `getCommon()` instead so as to bypass the `OpaqueValueExpr`.

Only unique `OpaqueValueExpr`s should be handled in the mapping builder, as [discussed](#85837 (comment)) in #85837. However, `getCond()` returns non-unique `OpaqueValueExpr` for `BinaryConditionalOperator` (because it is also used as the "true" branch expression). Use `getCommon()` instead so as to bypass the `OpaqueValueExpr`.

`ArrayInitLoopExpr` AST node has two occurences of its as-written initializing expression in its subexpressions through a non-unique `OpaqueValueExpr`. It causes double-visiting of the initializing expression if not handled explicitly, as discussed in #85837.

Introduced in llvm#78041, originally reported as llvm#79957 and fixed partially in llvm#80050. `OpaqueValueExpr` used with `TemplateArgument::StructuralValue` has no corresponding source expression. A test case with subobject-referring NTTP added.

bolshakov-a · 2024-05-24T20:00:32Z

@efriedma-quic ping.

efriedma-quic

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

bolshakov-a · 2024-05-24T20:04:33Z

Ok, got it. Thanks!

llvmbot added clang Clang issues not falling into any other category clang:codegen labels Mar 19, 2024

bolshakov-a commented Mar 19, 2024

View reviewed changes

bolshakov-a force-pushed the fix_coverage_nttp_crash branch from ec923ee to 398ad87 Compare March 20, 2024 02:56

bolshakov-a changed the title ~~Fix code coverage mapping crash with generalized NTTPs~~ [clang][c++20] Fix code coverage mapping crash with generalized NTTPs Mar 20, 2024

efriedma-quic requested review from ahatanak and efriedma-quic March 27, 2024 21:13

efriedma-quic reviewed Mar 27, 2024

View reviewed changes

erichkeane approved these changes Apr 9, 2024

View reviewed changes

bolshakov-a mentioned this pull request Apr 16, 2024

[Coverage] Handle array decomposition correctly #88881

Merged

bolshakov-a mentioned this pull request Apr 16, 2024

[Coverage][NFC] Avoid visiting non-unique OpaqueValueExpr #88910

Merged

bolshakov-a added 2 commits May 16, 2024 12:07

Fix up: check isUnique instead of source expr presence

9684fed

bolshakov-a force-pushed the fix_coverage_nttp_crash branch from 398ad87 to 9684fed Compare May 16, 2024 10:11

efriedma-quic approved these changes May 24, 2024

View reviewed changes

efriedma-quic merged commit 6be1a15 into llvm:main May 24, 2024
4 checks passed

bolshakov-a deleted the fix_coverage_nttp_crash branch May 24, 2024 20:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

bolshakov-a commented Mar 19, 2024

llvmbot commented Mar 19, 2024

llvmbot commented Mar 19, 2024

bolshakov-a Mar 19, 2024

efriedma-quic Mar 27, 2024

efriedma-quic Mar 27, 2024

bolshakov-a Mar 28, 2024

efriedma-quic Apr 9, 2024

bolshakov-a Apr 10, 2024 •

edited

Loading

efriedma-quic Apr 10, 2024

bolshakov-a Apr 16, 2024

efriedma-quic Mar 27, 2024

bolshakov-a commented Apr 9, 2024

erichkeane left a comment

bolshakov-a commented May 24, 2024

efriedma-quic left a comment

bolshakov-a commented May 24, 2024

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

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

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

Conversation

bolshakov-a commented Mar 19, 2024

llvmbot commented Mar 19, 2024

llvmbot commented Mar 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bolshakov-a Apr 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bolshakov-a commented Apr 9, 2024

erichkeane left a comment

Choose a reason for hiding this comment

bolshakov-a commented May 24, 2024

efriedma-quic left a comment

Choose a reason for hiding this comment

bolshakov-a commented May 24, 2024

bolshakov-a Apr 10, 2024 •

edited

Loading