[clang] [clang][Sema] Don't mark VarDecls of bindings in tuple-like decompositions as implicit (PR #100355)
Michael Buch via cfe-commits
cfe-commits at lists.llvm.org
Wed Jul 24 05:02:38 PDT 2024
https://github.com/Michael137 created https://github.com/llvm/llvm-project/pull/100355
This patch is motivated by the debug-info issue in https://github.com/llvm/llvm-project/issues/48909. Clang is currently emitting the `DW_AT_artificial` attribute on debug-info entries for structured bindings whereas GCC does not. GCC's interpretation of the DWARF spec is more user-friendly in this regard, so we would like to do the same in Clang. [`CGDebugInfo` uses `isImplicit` to decide which variables to mark artificial](https://github.com/llvm/llvm-project/blob/0c4023ae3b64c54ff51947e9776aee0e963c5635/clang/lib/CodeGen/CGDebugInfo.cpp#L4783-L4784) (e.g., `ImplicitParamDecls`, compiler-generated variables, etc.). But for the purposes of debug-info, when we say "artificial", what we really mean in many cases is: "not explicitly spelled out in source". `VarDecl`s that back tuple-like bindings are [technically compiler-generated](https://github.com/llvm/llvm-project/issues/48909#issuecomment-2238976579), but that is a confusing notion for debug-info, since these bindings *are* spelled out in source. The [documentation for `isImplicit`](https://github.com/llvm/llvm-project/blob/68a0d0c76223736351fd7c452bca3ba9d80ca342/clang/include/clang/AST/DeclBase.h#L596-L600) does to some extent imply that implicit variables aren't written in source.
This patch avoids marking the variables introduced as part of a decomposition as artificial, resolving the debug-info confusion.
**Affects on AST matchers/traversal**
1. Matchers/visitors that previously ignored these `VarDecl`s using `TK_IgnoreUnlessSpelledInSource` now wouldn't (this is tested in the newly added unit-test)
2. In [MatchASTVisitor::TraverseDecl](https://github.com/llvm/llvm-project/blob/7fad04e94b7b594389111ae7eca0883ef18dc90b/clang/lib/ASTMatchers/ASTMatchFinder.cpp#L1463-L1465), the children of `BindingDecl`s aren't visited anyway in `TK_IgnoreUnlessSpelledInSource` mode, so not having implicit `VarDecl`s won't affect visitation of the `BindingDecl`s they back. This is already tested in the [ASTTraverser unit-tests](https://github.com/llvm/llvm-project/blob/893a303962608469ec5bd01fe44e82c935152e9c/clang/unittests/AST/ASTTraverserTest.cpp#L1578-L1585).
**Main alternatives considered**
1. Don't use `isImplicit` in `CGDebugInfo` when determining whether to add `DW_AT_artificial`. Instead use some other property of the AST that would tell us whether a node was explicitly spelled out in source or not
* Considered using `SourceRange` or `SourceLocation` to tell us this, but didn't find a good way to, e.g., correctly identify that the implicit `this` parameter wasn't spelled out in source (as opposed to an unnamed parameter in a function declaration)
2. In CGDebugInfo, don't mark `VarDecl`s as artificial if they are the holding vars for a `BindingDecl`
* We could detect that a `VarDecl` is actually a tuple-like decomposition using something like the following (modulo error handling/null-checks):
```
auto * RefExpr = llvm::cast<DeclRefExpr>(Decl->getInit()->IgnoreUnlessSpelledInSource());
bool IsDecomposition = llvm::isa<DecompositionDecl>(RefExpr->getDecl());
```
Though this felt like we'd be over-relying on the structure of the AST (but maybe that's stable enough?). Alternatively we could've also added a bit to `VarDeclBitFields` that indicates that a `VarDecl` is a holding var, but the use-case didn't feel like good enough justification for this.
Fixes https://github.com/llvm/llvm-project/issues/48909
>From 2b1255de05856e4c79f58d3e4071384ba80a881d Mon Sep 17 00:00:00 2001
From: Michael Buch <michaelbuch12 at gmail.com>
Date: Thu, 18 Jul 2024 16:26:16 -0500
Subject: [PATCH] [clang][Sema] Don't mark VarDecls of bindings in tuple-like
decompositions as implicit
---
clang/lib/AST/DeclCXX.cpp | 1 -
clang/lib/Sema/SemaDeclCXX.cpp | 1 -
.../debug-info-structured-binding.cpp | 36 ++++++++++--
.../ASTMatchers/ASTMatchersTraversalTest.cpp | 57 +++++++++++++++++++
4 files changed, 88 insertions(+), 7 deletions(-)
diff --git a/clang/lib/AST/DeclCXX.cpp b/clang/lib/AST/DeclCXX.cpp
index 72d68f39a97a5..2c22f42a5e0a8 100644
--- a/clang/lib/AST/DeclCXX.cpp
+++ b/clang/lib/AST/DeclCXX.cpp
@@ -3327,7 +3327,6 @@ VarDecl *BindingDecl::getHoldingVar() const {
return nullptr;
auto *VD = cast<VarDecl>(DRE->getDecl());
- assert(VD->isImplicit() && "holding var for binding decl not implicit");
return VD;
}
diff --git a/clang/lib/Sema/SemaDeclCXX.cpp b/clang/lib/Sema/SemaDeclCXX.cpp
index f24912cde275a..f4cc1fc4583aa 100644
--- a/clang/lib/Sema/SemaDeclCXX.cpp
+++ b/clang/lib/Sema/SemaDeclCXX.cpp
@@ -1316,7 +1316,6 @@ static bool checkTupleLikeDecomposition(Sema &S,
S.Context.getTrivialTypeSourceInfo(T, Loc), Src->getStorageClass());
RefVD->setLexicalDeclContext(Src->getLexicalDeclContext());
RefVD->setTSCSpec(Src->getTSCSpec());
- RefVD->setImplicit();
if (Src->isInlineSpecified())
RefVD->setInlineSpecified();
RefVD->getLexicalDeclContext()->addHiddenDecl(RefVD);
diff --git a/clang/test/CodeGenCXX/debug-info-structured-binding.cpp b/clang/test/CodeGenCXX/debug-info-structured-binding.cpp
index c88a5cdaa20e7..4a3126a36598d 100644
--- a/clang/test/CodeGenCXX/debug-info-structured-binding.cpp
+++ b/clang/test/CodeGenCXX/debug-info-structured-binding.cpp
@@ -5,20 +5,46 @@
// CHECK: #dbg_declare(ptr %{{[0-9]+}}, ![[VAR_2:[0-9]+]], !DIExpression(DW_OP_plus_uconst, 4),
// CHECK: #dbg_declare(ptr %{{[0-9]+}}, ![[VAR_3:[0-9]+]], !DIExpression(DW_OP_deref),
// CHECK: #dbg_declare(ptr %{{[0-9]+}}, ![[VAR_4:[0-9]+]], !DIExpression(DW_OP_deref, DW_OP_plus_uconst, 4),
+// CHECK: #dbg_declare(ptr %z1, ![[VAR_5:[0-9]+]], !DIExpression()
+// CHECK: #dbg_declare(ptr %z2, ![[VAR_6:[0-9]+]], !DIExpression()
// CHECK: ![[VAR_0]] = !DILocalVariable(name: "a"
-// CHECK: ![[VAR_1]] = !DILocalVariable(name: "x1"
-// CHECK: ![[VAR_2]] = !DILocalVariable(name: "y1"
-// CHECK: ![[VAR_3]] = !DILocalVariable(name: "x2"
-// CHECK: ![[VAR_4]] = !DILocalVariable(name: "y2"
+// CHECK: ![[VAR_1]] = !DILocalVariable(name: "x1", scope: !{{[0-9]+}}, file: !{{[0-9]+}}, line: {{[0-9]+}}, type: !{{[0-9]+}})
+// CHECK: ![[VAR_2]] = !DILocalVariable(name: "y1", scope: !{{[0-9]+}}, file: !{{[0-9]+}}, line: {{[0-9]+}}, type: !{{[0-9]+}})
+// CHECK: ![[VAR_3]] = !DILocalVariable(name: "x2", scope: !{{[0-9]+}}, file: !{{[0-9]+}}, line: {{[0-9]+}}, type: !{{[0-9]+}})
+// CHECK: ![[VAR_4]] = !DILocalVariable(name: "y2", scope: !{{[0-9]+}}, file: !{{[0-9]+}}, line: {{[0-9]+}}, type: !{{[0-9]+}})
+// CHECK: ![[VAR_5]] = !DILocalVariable(name: "z1", scope: !{{[0-9]+}}, file: !{{[0-9]+}}, line: {{[0-9]+}}, type: !{{[0-9]+}})
+// CHECK: ![[VAR_6]] = !DILocalVariable(name: "z2", scope: !{{[0-9]+}}, file: !{{[0-9]+}}, line: {{[0-9]+}}, type: !{{[0-9]+}})
struct A {
int x;
int y;
};
+struct B {
+ int w;
+ int z;
+ template<int> int get();
+ template<> int get<0>() { return w; }
+ template<> int get<1>() { return z; }
+};
+
+namespace std {
+template<typename T> struct tuple_size {
+};
+template<>
+struct tuple_size<B> {
+ static constexpr int value = 2;
+};
+
+template<int, typename T> struct tuple_element {};
+template<> struct tuple_element<0, B> { using type = int; };
+template<> struct tuple_element<1, B> { using type = int; };
+}
+
int f() {
A a{10, 20};
auto [x1, y1] = a;
auto &[x2, y2] = a;
- return x1 + y1 + x2 + y2;
+ auto [z1, z2] = B{1, 2};
+ return x1 + y1 + x2 + y2 + z1 + z2;
}
diff --git a/clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp b/clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
index 47a71134d5027..06ac07cb12509 100644
--- a/clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
+++ b/clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
@@ -3163,6 +3163,63 @@ void foo()
matchesConditionally(Code, traverse(TK_IgnoreUnlessSpelledInSource, M),
true, {"-std=c++17"}));
}
+
+ Code = R"cpp(
+struct Pair
+{
+ int x, y;
+};
+
+// Note: these utilities are required to force binding to tuple like structure
+namespace std
+{
+ template <typename E>
+ struct tuple_size
+ {
+ };
+
+ template <>
+ struct tuple_size<Pair>
+ {
+ static constexpr unsigned value = 2;
+ };
+
+ template <unsigned I, class T>
+ struct tuple_element
+ {
+ using type = int;
+ };
+
+};
+
+template <unsigned I>
+int &&get(Pair &&p);
+
+void decompTuple()
+{
+ Pair p{1, 2};
+ auto [a, b] = p;
+
+ a = 3;
+}
+ )cpp";
+
+ {
+ auto M = bindingDecl(hasName("a"));
+ EXPECT_TRUE(
+ matchesConditionally(Code, traverse(TK_AsIs, M), true, {"-std=c++17"}));
+ EXPECT_TRUE(
+ matchesConditionally(Code, traverse(TK_IgnoreUnlessSpelledInSource, M),
+ true, {"-std=c++17"}));
+ }
+ {
+ auto M = varDecl(hasName("a"));
+ EXPECT_TRUE(
+ matchesConditionally(Code, traverse(TK_AsIs, M), true, {"-std=c++17"}));
+ EXPECT_TRUE(
+ matchesConditionally(Code, traverse(TK_IgnoreUnlessSpelledInSource, M),
+ true, {"-std=c++17"}));
+ }
}
TEST(Traversal, traverseNoImplicit) {
More information about the cfe-commits
mailing list