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 <michaelbuc...@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) { _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
