[clang][DebugInfo] Revert to printing canonical typenames for template aliases #110767

Michael137 · 2024-10-01T23:56:41Z

This was originally added in https://reviews.llvm.org/D142268 to have LLDB display variable typenames that benefit from suppressing defaulted template arguments.

We currently represent template aliases as DW_AT_typedefs instead of DW_TAG_template_alias. This means for types like:

template <class _Tp>
using __remove_cv_t = __remove_cv(_Tp);

template <class _Tp>
using remove_cv_t = __remove_cv_t<_Tp>;

template<typename T>
class optional {
  using value_type = T;
  remove_cv_t<value_type> __val_;
}

we would generate DWARF like:

0x0000274f:       DW_TAG_typedef
                    DW_AT_type  (0x0000000000002758 "__remove_cv_t<value_type>")
                    DW_AT_name  ("remove_cv_t<value_type>")

This is an actual libc++ type layout introduced in #110355, and uncovered a shortcoming of LLDB's data-formatter infrastructure, where we cache formatters on the contents of DW_AT_name (which currently wouldn't be a fully resolved typename for template specializations).

To unblock the libc++ change, I think we can revert this without much fallout.

Then we have two options for follow-up (or do both):

reland this but adjust the LLDB formatter cache so it doesn't cache formatters for template specializations
implement support for DW_TAG_template_alias in LLDB (and make Clang generate them by default).

…e aliases This was originally added in https://reviews.llvm.org/D142268 have LLDB display variable typenames that benefit from suppressing defaulted template arguments. We currently represent template aliases as `DW_AT_typedef`s instead of `DW_TAG_template_alias`. This means for types like: ``` template <class _Tp> using __remove_cv_t = __remove_cv(_Tp); template <class _Tp> using remove_cv_t = __remove_cv_t<_Tp>; template<typename T> class optional { using value_type = T; remove_cv_t<value_type> __val_; } ``` we would generate DWARF like: ``` 0x0000274f: DW_TAG_typedef DW_AT_type (0x0000000000002758 "__remove_cv_t<value_type>") DW_AT_name ("remove_cv_t<value_type>") ``` This is an actual libc++ type layout introduced in llvm#110355, which uncovered a shortcoming of LLDB's data-formatter infrastructure, which caches formatters on the contents of `DW_AT_name`. To unblock the libc++ change, we can revert this without much fallout. Then we have two options: 1. reland this but adjust the LLDB formatter cache so it doesn't cache formatters for template specializations 2. implement support for `DW_TAG_template_alias` in LLDB (and make Clang generate them by default).

llvmbot · 2024-10-01T23:57:15Z

@llvm/pr-subscribers-debuginfo

@llvm/pr-subscribers-clang

Author: Michael Buch (Michael137)

Changes

This was originally added in https://reviews.llvm.org/D142268 have LLDB display variable typenames that benefit from suppressing defaulted template arguments.

We currently represent template aliases as DW_AT_typedefs instead of DW_TAG_template_alias. This means for types like:

template &lt;class _Tp&gt;
using __remove_cv_t = __remove_cv(_Tp);

template &lt;class _Tp&gt;
using remove_cv_t = __remove_cv_t&lt;_Tp&gt;;

template&lt;typename T&gt;
class optional {
  using value_type = T;
  remove_cv_t&lt;value_type&gt; __val_;
}

we would generate DWARF like:

0x0000274f:       DW_TAG_typedef
                    DW_AT_type  (0x0000000000002758 "__remove_cv_t&lt;value_type&gt;")
                    DW_AT_name  ("remove_cv_t&lt;value_type&gt;")

This is an actual libc++ type layout introduced in #110355, which uncovered a shortcoming of LLDB's data-formatter infrastructure, which caches formatters on the contents of DW_AT_name.

To unblock the libc++ change, I think we can revert this without much fallout.

Then we have two options for follow-up (or do both):

reland this but adjust the LLDB formatter cache so it doesn't cache formatters for template specializations
implement support for DW_TAG_template_alias in LLDB (and make Clang generate them by default).

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

2 Files Affected:

(modified) clang/lib/CodeGen/CGDebugInfo.cpp (-9)
(modified) clang/test/CodeGenCXX/debug-info-alias.cpp (+1-1)

diff --git a/clang/lib/CodeGen/CGDebugInfo.cpp b/clang/lib/CodeGen/CGDebugInfo.cpp
index 8887c4de7c4c81..609957b75d6e7e 100644
--- a/clang/lib/CodeGen/CGDebugInfo.cpp
+++ b/clang/lib/CodeGen/CGDebugInfo.cpp
@@ -1472,15 +1472,6 @@ llvm::DIType *CGDebugInfo::CreateType(const TemplateSpecializationType *Ty,
     return AliasTy;
   }
 
-  // Disable PrintCanonicalTypes here because we want
-  // the DW_AT_name to benefit from the TypePrinter's ability
-  // to skip defaulted template arguments.
-  //
-  // FIXME: Once -gsimple-template-names is enabled by default
-  // and we attach template parameters to alias template DIEs
-  // we don't need to worry about customizing the PrintingPolicy
-  // here anymore.
-  PP.PrintCanonicalTypes = false;
   printTemplateArgumentList(OS, Ty->template_arguments(), PP,
                             TD->getTemplateParameters());
   return DBuilder.createTypedef(Src, OS.str(), getOrCreateFile(Loc),
diff --git a/clang/test/CodeGenCXX/debug-info-alias.cpp b/clang/test/CodeGenCXX/debug-info-alias.cpp
index bf2dbee4659592..3cd1446d25270e 100644
--- a/clang/test/CodeGenCXX/debug-info-alias.cpp
+++ b/clang/test/CodeGenCXX/debug-info-alias.cpp
@@ -24,7 +24,7 @@ x::bar<int> bi;
 // CHECK: [[BFLOAT]] = !DIDerivedType(tag: DW_TAG_typedef, name: "bar<float>"
 x::bar<float> bf;
 // CHECK: !DIGlobalVariable(name: "bz",{{.*}} type: [[BBAZ:![0-9]+]]
-// CHECK: [[BBAZ]] = !DIDerivedType(tag: DW_TAG_typedef, name: "bar<baz<int> >"
+// CHECK: [[BBAZ]] = !DIDerivedType(tag: DW_TAG_typedef, name: "bar<baz<int, int> >"
 x::bar<baz<int>> bz;
 
 using

llvmbot · 2024-10-01T23:57:16Z

@llvm/pr-subscribers-clang-codegen

Author: Michael Buch (Michael137)

Changes

This was originally added in https://reviews.llvm.org/D142268 have LLDB display variable typenames that benefit from suppressing defaulted template arguments.

We currently represent template aliases as DW_AT_typedefs instead of DW_TAG_template_alias. This means for types like:

template &lt;class _Tp&gt;
using __remove_cv_t = __remove_cv(_Tp);

template &lt;class _Tp&gt;
using remove_cv_t = __remove_cv_t&lt;_Tp&gt;;

template&lt;typename T&gt;
class optional {
  using value_type = T;
  remove_cv_t&lt;value_type&gt; __val_;
}

we would generate DWARF like:

0x0000274f:       DW_TAG_typedef
                    DW_AT_type  (0x0000000000002758 "__remove_cv_t&lt;value_type&gt;")
                    DW_AT_name  ("remove_cv_t&lt;value_type&gt;")

This is an actual libc++ type layout introduced in #110355, which uncovered a shortcoming of LLDB's data-formatter infrastructure, which caches formatters on the contents of DW_AT_name.

To unblock the libc++ change, I think we can revert this without much fallout.

Then we have two options for follow-up (or do both):

reland this but adjust the LLDB formatter cache so it doesn't cache formatters for template specializations
implement support for DW_TAG_template_alias in LLDB (and make Clang generate them by default).

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

2 Files Affected:

(modified) clang/lib/CodeGen/CGDebugInfo.cpp (-9)
(modified) clang/test/CodeGenCXX/debug-info-alias.cpp (+1-1)

diff --git a/clang/lib/CodeGen/CGDebugInfo.cpp b/clang/lib/CodeGen/CGDebugInfo.cpp
index 8887c4de7c4c81..609957b75d6e7e 100644
--- a/clang/lib/CodeGen/CGDebugInfo.cpp
+++ b/clang/lib/CodeGen/CGDebugInfo.cpp
@@ -1472,15 +1472,6 @@ llvm::DIType *CGDebugInfo::CreateType(const TemplateSpecializationType *Ty,
     return AliasTy;
   }
 
-  // Disable PrintCanonicalTypes here because we want
-  // the DW_AT_name to benefit from the TypePrinter's ability
-  // to skip defaulted template arguments.
-  //
-  // FIXME: Once -gsimple-template-names is enabled by default
-  // and we attach template parameters to alias template DIEs
-  // we don't need to worry about customizing the PrintingPolicy
-  // here anymore.
-  PP.PrintCanonicalTypes = false;
   printTemplateArgumentList(OS, Ty->template_arguments(), PP,
                             TD->getTemplateParameters());
   return DBuilder.createTypedef(Src, OS.str(), getOrCreateFile(Loc),
diff --git a/clang/test/CodeGenCXX/debug-info-alias.cpp b/clang/test/CodeGenCXX/debug-info-alias.cpp
index bf2dbee4659592..3cd1446d25270e 100644
--- a/clang/test/CodeGenCXX/debug-info-alias.cpp
+++ b/clang/test/CodeGenCXX/debug-info-alias.cpp
@@ -24,7 +24,7 @@ x::bar<int> bi;
 // CHECK: [[BFLOAT]] = !DIDerivedType(tag: DW_TAG_typedef, name: "bar<float>"
 x::bar<float> bf;
 // CHECK: !DIGlobalVariable(name: "bz",{{.*}} type: [[BBAZ:![0-9]+]]
-// CHECK: [[BBAZ]] = !DIDerivedType(tag: DW_TAG_typedef, name: "bar<baz<int> >"
+// CHECK: [[BBAZ]] = !DIDerivedType(tag: DW_TAG_typedef, name: "bar<baz<int, int> >"
 x::bar<baz<int>> bz;
 
 using

Michael137 · 2024-10-02T09:38:44Z

Test failures in Sema/aarch64-sve-vector-trig-ops.c unrelated

adrian-prantl

Sounds unfortunate, but if you think that's the best choice for unblocking libc++, let's do that.

dwblaikie · 2024-10-02T16:22:23Z

Not quite sure how we get to caching and template specializations and pretty printers.

If we're still producing the typedef-style DWARF for these alias template specializations - perhaps lldb could not cache pretty printers for typedefs? (I guess the pretty printers shouldn't be typedef-specific, right, since typedefs are transparent anyway - but I guess maybe pretty printers can be typedef-specific because typedefs can be intended to communicate what kind of a thing is and possibly how to render it?) - it'll cache teh pretty printer for the underlying type anyway, yeah?

Michael137 · 2024-10-02T17:05:08Z

If we're still producing the typedef-style DWARF for these alias template specializations - perhaps lldb could not cache pretty printers for typedefs? (I guess the pretty printers shouldn't be typedef-specific, right, since typedefs are transparent anyway - but I guess maybe pretty printers can be typedef-specific because typedefs can be intended to communicate what kind of a thing is and possibly how to render it?) - it'll cache teh pretty printer for the underlying type anyway, yeah?

Basically what was happening is that LLDB saw the new std::removecv_ref_t<value_type> member of std::optional, which resolved to std::string. Then formatted it as std::string. And then cached the DW_AT_name, i.e., std::removecv_ref_t<value_type>, to point to the std::string formatter. But that meant that the next time we saw a std::removecv_ref_t<value_type> (even if it didn't resolve to std::string) would use the cached formatter. The reason this patch works around that situation is that now the DW_AT_name will have the canonical type in DW_AT_name, not the unresolved std::removecv_ref_t<value_type>.

@jimingham will be able to speak more confidently about LLDB's intention here but I think caching typedefs is intentionally supported. Jim and I came to the conclusion yesterday that we probably don't want to cache template specializations.

I think we can (and should) fix the caching behaviour, but that would be a longer undertaking and I wanted to unblock the libc++ patch (#110355) as soon as possible.

It's on my list of follow-ups to reland this and fix the cache (and also investigate what it would take to support DW_TAG_template_alias in LLDB, so we can enable those by default).

jimingham · 2024-10-02T17:14:37Z

On Oct 2, 2024, at 9:22 AM, David Blaikie ***@***.***> wrote: Not quite sure how we get to caching and template specializations and pretty printers. If we're still producing the typedef-style DWARF for these alias template specializations - perhaps lldb could not cache pretty printers for typedefs? (I guess the pretty printers shouldn't be typedef-specific, right, since typedefs are transparent anyway - but I guess maybe pretty printers can be typedef-specific because typedefs can be intended to communicate what kind of a thing is and possibly how to render it?) - it'll cache teh pretty printer for the underlying type anyway, yeah?

lldb has always allowed you to match formatters to typedef's, regardless of whether something past that typedef in the chain has it's own formatter. That way, for instance, if you have some constraint you intend for the values of the typedef, you can alert yourself (e.g. color the summary red) when the values don't follow those constraints. And when you do that you shouldn't have to worry about formatters lower down in the typedef chain. OTOH, if you have a variable whose immediate type is a typedef that doesn't itself have a formatter match, but that typedef has a type lower in the typedef chain that DOES have a formatter, we will use that formatter for the typedef. Again, if this typedef is telling the author about some intent for that set of instances of the type, we shouldn't make using typedef's that way annoying by forcing people to recreate the useful formatter every time they introduce such a typedef. To be clear, neither of these design decisions is causing any problem here. The problem comes in here because we try to speed up the type recognizer matching by caching hits we found by inserting the matched type name to the found formatter in our "exact name" lookup table. In this case, we can save ourselves from having to reconstruct this typedef chain every time we do the match on the original typedef in the future by caching the "original typedef"->"formatter from lower in the typedef chain" as an exact match. Normally, that's a fine thing to do, but in this case, we saw a typedef named "std::remove_cv_t<value_type>" that was a typedef for `std::string`. So we registered std::string as the formatter for the name "std::remove_cv_t<value_type>". Then the next time we saw something named this way, we found it in our cache and tried to use the `std::string` formatter. That failed because this time round <value_type> is in fact an "int"... Prior to this, these templated types wouldn't have had this generic looking name but would have had the resolved type in the name, which wouldn't have been ambiguous. The narrowest solution is that when you go to cache a type name -> formatter pair, you should first check whether the type name is generic. If it is, then you can't guarantee it will resolve to the same base type in the future, so you should not cache it. IIUC, Michael started down this path but found there wasn't a way to tell straightforwardly whether a type was generic. Since this is just an optimization, we should first prove to ourselves that failing to put one of these type names in the exact matches really makes enough difference to bother with a complex solution. If the gains are some but not huge, some boneheaded solution like never caching anything where the from type has a '<' and a '>' is probably sufficient. Hope that helps... Jim

…

— Reply to this email directly, view it on GitHub <#110767 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADUPVW4EZX3ZDOJGESSRMVDZZQMVTAVCNFSM6AAAAABPGTLBXSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGOBZGA4DSOBZHA>. You are receiving this because your review was requested.

dwblaikie · 2024-10-02T17:59:12Z

Thanks @jimingham - yeah, that makes sense. My original argument for being OK with the de-canonicalization of typedefs was that they aren't load bearing anyway. But if lldb puts load on them, that assumption doesn't hold.

Yeah, could check for angles or template parameter DIEs and cache typedef->prettyprinter only if none of those features appear.

Michael137 · 2024-10-02T18:38:40Z

Test failure unrelated. Will merge to unblock libc++ and investigate a path forward as a follow-up

…e aliases (llvm#110767) This was originally added in https://reviews.llvm.org/D142268 to have LLDB display variable typenames that benefit from suppressing defaulted template arguments. We currently represent template aliases as `DW_AT_typedef`s instead of `DW_TAG_template_alias`. This means for types like: ``` template <class _Tp> using __remove_cv_t = __remove_cv(_Tp); template <class _Tp> using remove_cv_t = __remove_cv_t<_Tp>; template<typename T> class optional { using value_type = T; remove_cv_t<value_type> __val_; } ``` we would generate DWARF like: ``` 0x0000274f: DW_TAG_typedef DW_AT_type (0x0000000000002758 "__remove_cv_t<value_type>") DW_AT_name ("remove_cv_t<value_type>") ``` This is an actual libc++ type layout introduced in llvm#110355, and uncovered a shortcoming of LLDB's data-formatter infrastructure, where we cache formatters on the contents of `DW_AT_name` (which currently wouldn't be a fully resolved typename for template specializations). To unblock the libc++ change, I think we can revert this without much fallout. Then we have two options for follow-up (or do both): 1. reland this but adjust the LLDB formatter cache so it doesn't cache formatters for template specializations 2. implement support for `DW_TAG_template_alias` in LLDB (and make Clang generate them by default).

Michael137 requested review from jimingham, dwblaikie and adrian-prantl October 1, 2024 23:56

llvmbot added clang Clang issues not falling into any other category clang:codegen IR generation bugs: mangling, exceptions, etc. debuginfo labels Oct 1, 2024

Michael137 mentioned this pull request Oct 2, 2024

[libc++] LWG3870: Remove voidify #110355

Merged

adrian-prantl approved these changes Oct 2, 2024

View reviewed changes

fixup! fix whitespace expectation in test for Windows

f87dd63

fixup! fix test regex

0a21cd5

Michael137 merged commit 52a9ba7 into llvm:main Oct 2, 2024
5 of 7 checks passed

Michael137 deleted the clang/template-alias-printing branch October 2, 2024 18:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[clang][DebugInfo] Revert to printing canonical typenames for template aliases #110767

[clang][DebugInfo] Revert to printing canonical typenames for template aliases #110767

Uh oh!

Michael137 commented Oct 1, 2024 •

edited

Loading

Uh oh!

llvmbot commented Oct 1, 2024 •

edited

Loading

Uh oh!

llvmbot commented Oct 1, 2024

Uh oh!

Michael137 commented Oct 2, 2024

Uh oh!

adrian-prantl left a comment

Uh oh!

dwblaikie commented Oct 2, 2024

Uh oh!

Michael137 commented Oct 2, 2024

Uh oh!

jimingham commented Oct 2, 2024 via email

Uh oh!

dwblaikie commented Oct 2, 2024

Uh oh!

Michael137 commented Oct 2, 2024

Uh oh!

Uh oh!

Uh oh!

[clang][DebugInfo] Revert to printing canonical typenames for template aliases #110767

[clang][DebugInfo] Revert to printing canonical typenames for template aliases #110767

Uh oh!

Conversation

Michael137 commented Oct 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Oct 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Oct 1, 2024

Uh oh!

Michael137 commented Oct 2, 2024

Uh oh!

adrian-prantl left a comment

Choose a reason for hiding this comment

Uh oh!

dwblaikie commented Oct 2, 2024

Uh oh!

Michael137 commented Oct 2, 2024

Uh oh!

jimingham commented Oct 2, 2024 via email

Uh oh!

dwblaikie commented Oct 2, 2024

Uh oh!

Michael137 commented Oct 2, 2024

Uh oh!

Uh oh!

Uh oh!

Michael137 commented Oct 1, 2024 •

edited

Loading

llvmbot commented Oct 1, 2024 •

edited

Loading