[Coverage] Handle array decomposition correctly

[bolshakov-a]

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.

@efriedma-quic

[llvmbot]

@llvm/pr-subscribers-clang-codegen

@llvm/pr-subscribers-clang

Author: Andrey Ali Khan Bolshakov (bolshakov-a)

Changes

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.

@efriedma-quic

---

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

2 Files Affected:

• (modified) clang/lib/CodeGen/CoverageMappingGen.cpp (+4)
• (added) clang/test/CoverageMapping/decomposition.cpp (+15)

diff --git a/clang/lib/CodeGen/CoverageMappingGen.cpp b/clang/lib/CodeGen/CoverageMappingGen.cpp
index 71215da362d3d0..569fd489dc8baa 100644
--- a/clang/lib/CodeGen/CoverageMappingGen.cpp
+++ b/clang/lib/CodeGen/CoverageMappingGen.cpp
@@ -2171,6 +2171,10 @@ struct CounterCoverageMappingBuilder
     // propagate counts into them.
   }
 
+  void VisitArrayInitLoopExpr(const ArrayInitLoopExpr *AILE) {
+    Visit(AILE->getCommonExpr()->getSourceExpr());
+  }
+
   void VisitPseudoObjectExpr(const PseudoObjectExpr *POE) {
     // Just visit syntatic expression as this is what users actually write.
     VisitStmt(POE->getSyntacticForm());
diff --git a/clang/test/CoverageMapping/decomposition.cpp b/clang/test/CoverageMapping/decomposition.cpp
new file mode 100644
index 00000000000000..31bd6cae2c4dec
--- /dev/null
+++ b/clang/test/CoverageMapping/decomposition.cpp
@@ -0,0 +1,15 @@
+// RUN: %clang_cc1 -mllvm -emptyline-comment-coverage=false -fprofile-instrument=clang -fcoverage-mapping -dump-coverage-mapping -emit-llvm-only %s | FileCheck %s
+
+// CHECK-LABEL:       _Z19array_decompositioni:
+// CHECK-NEXT:          File 0, [[@LINE+6]]:32 -> {{[0-9]+}}:2 = #0
+// CHECK-NEXT:          File 0, [[@LINE+8]]:20 -> [[@LINE+8]]:25 = #0
+// CHECK-NEXT:          Branch,File 0, [[@LINE+7]]:20 -> [[@LINE+7]]:25 = #1, (#0 - #1)
+// CHECK-NEXT:          Gap,File 0, [[@LINE+6]]:27 -> [[@LINE+6]]:28 = #1
+// CHECK-NEXT:          File 0, [[@LINE+5]]:28 -> [[@LINE+5]]:29 = #1
+// CHECK-NEXT:          File 0, [[@LINE+4]]:32 -> [[@LINE+4]]:33 = (#0 - #1)
+int array_decomposition(int i) {
+  int a[] = {1, 2, 3};
+  int b[] = {4, 5, 6};
+  auto [x, y, z] = i > 0 ? a : b;
+  return x + y + z;
+}

[efriedma-quic]

I don't think this works correctly? You need to evaluate both the getCommonExpr(), and the getSubExpr().

[bolshakov-a]

Honestly, I'm not very familiar with code coverage technique, but it seems to me that only explicitly written code is relevant for that. "Common expression" is exactly the explicitly written part. "Subexpression" is an implicitly generated per-element initializer which refers to the "common expression" (hence, without this patch, the counters for the conditional operator from the test are duplicated despite it is executed only once per function call). Which observable drawbacks do you expect from not visiting the implicit parts of the initializer?

[efriedma-quic]

Say you have:

int foo();
struct A { A(); A(const A&, int = foo()); };
struct B { A a[10]; };
void f(const B& b) { B bb = b; }

We want to visit the call to foo(), I think?

[bolshakov-a]

I don't see any difference on your example (with main() and function definitions added) with and without my patch both in the dumped coverage mapping and in the output of llvm-cov show ... --show-branches=count --show-expansions -show-line-counts-or-regions command. If the coverage mapping works with code regions, it looks quite natural that an implicit code (foo() call inside f() in the sample) has no corresponding region and hence no mapping (except macro expansions).

Maybe, @hyp can clarify?

[efriedma-quic]

I'm not deeply familiar with how the code decides when and when not to insert counters... the point was just that just because it's "implicit", it doesn't mean there aren't any interesting expressions.

But anyway, I think we end up doing the right thing automatically if you ignore non-unique OpaqueValueExprs.

[bolshakov-a]

But anyway, I think we end up doing the right thing automatically if you ignore non-unique OpaqueValueExprs.

The problem is that there is no initializing "common expression" in the AST besides non-unique OpaqueValueExpr, hence some code handling ArrayInitLoopExpr should be written so as not to lose it at all.

@chapuni, @gulfemsavrun, @evodius96, @vedantk, please clarify: should or should not the implicit stuff be traversed here, particularly for array element initialization?

[bolshakov-a]

@efriedma-quic, would it be OK to add "subexpression" visitation with a comment that I'm not sure whether it is actually needed?

[bolshakov-a]

@efriedma-quic ping. CC @AaronBallman

[bolshakov-a]

This looks like supporting my words:
https://github.com/llvm/llvm-project/blob/llvmorg-19-init/clang/lib/CodeGen/CodeGenPGO.cpp#L935-L936

[efriedma-quic]

LGTM.

I'm not completely convinced by your argument here... but let's try to move forward and land this and #88898 so we can do the refactor to use isUnique(). Then we can revisit later if necessary.

[bolshakov-a]

Could you please merge both of these?

[bolshakov-a]

Thanks!