[lldb] Don't recognize template classes by the angle in their DW_AT_name

[teemperor]

Summary

Right now LLDB's DWARF parser tests if a DIE represents a C++ template class by looking for a
'<' character in its DW_AT_name attribute. This changes this check to instead look for a child DIE
that is any kind of template parameter.

I actually implemented this as part of a WIP patch for template variable support in LLDB (as they
don't have their template arguments in their DW_AT_name field), but as part of a discussion
to make DW_AT_name contents shorter by omitting template arguments [1] this seems now also
useful as a standalone patch.

[1] https://groups.google.com/g/llvm-dev/c/ekLMllbLIZg

[teemperor]

Mostly putting this up in case the linked llvm-dev thread moves towards actually changing the DW_AT_name contents. From what I understand no compiler currently omits the template arguments in DW_AT_name (?) so this also needs a test.

[teemperor]

Actually this wouldn't correctly handle class templates that aren't defined (well, right now we turn those into templates with no parameters, but now they would be normal classes with a potential <> in their name which seems worse).

[dblaikie]

In D108634#2962574, @teemperor wrote:

Actually this wouldn't correctly handle class templates that aren't defined (well, right now we turn those into templates with no parameters, but now they would be normal classes with a potential <> in their name which seems worse).

Yep, when using simplified template names it has to come along with the -debug-forward-template-params (or keep the template parameters in the name on declarations - could do that separately - but I /think/ it tips in favor of adding the template parameter DIEs rather than keeping them in the name)

This could use a test? A bit of manually modified assembly - or this patch can wait until I've committed the simplified names under a flag & that can be used to test the behavior in lldb?

[dblaikie]

I actually implemented this as part of a WIP patch for template variable support in LLDB (as they don't have their template arguments in their DW_AT_name field)

That seems buggy/inconsistent, though, yeah?

[dblaikie]

Yeah, I see what you mean - but yeah, that looks like something where we should probably change the DWARF to match other templates. (& then separately opt in to that behavior across all templates with the simplified template name work)

But not sure if you need to support current clang in newer/fixed lldbs, regardless of whether clang is changed to put the parameters on the DW_AT_name?

[teemperor]

In D108634#2963659, @dblaikie wrote:

In D108634#2962574, @teemperor wrote:

Actually this wouldn't correctly handle class templates that aren't defined (well, right now we turn those into templates with no parameters, but now they would be normal classes with a potential <> in their name which seems worse).

Yep, when using simplified template names it has to come along with the -debug-forward-template-params (or keep the template parameters in the name on declarations - could do that separately - but I /think/ it tips in favor of adding the template parameter DIEs rather than keeping them in the name)

I think both options should work in LLDB. FWIW, proper template parameter DIEs usually lead to a nicer AST inside LLDB as we can convert DIEs relatively straightforward back to Clang template parameters.

This could use a test? A bit of manually modified assembly - or this patch can wait until I've committed the simplified names under a flag & that can be used to test the behavior in lldb?

Is there a review open for that patch? I'm mostly curious about the results of running the LLDB test suit against it (with the flag enabled by default so that we can see what breaks).

Also yes, a test for the new/old behaviour still WIP as mentioned above.

In D108634#2963664, @dblaikie wrote:

Yeah, I see what you mean - but yeah, that looks like something where we should probably change the DWARF to match other templates. (& then separately opt in to that behavior across all templates with the simplified template name work)

But not sure if you need to support current clang in newer/fixed lldbs, regardless of whether clang is changed to put the parameters on the DW_AT_name?

If you mean whether a future LLDB (that has variable template support) should support parsing DWARF from Clang 12, then that's a supported use case. But from what I can see there is no special code needed for variable templates before/after the DW_AT_name change as the detection anyway can just be based on whether there are children DIEs? Unless there is some way to avoid emitting any template parameter DIEs for a variable template.

[dblaikie]

In D108634#2966778, @teemperor wrote:

In D108634#2963659, @dblaikie wrote:

In D108634#2962574, @teemperor wrote:

Actually this wouldn't correctly handle class templates that aren't defined (well, right now we turn those into templates with no parameters, but now they would be normal classes with a potential <> in their name which seems worse).

Yep, when using simplified template names it has to come along with the -debug-forward-template-params (or keep the template parameters in the name on declarations - could do that separately - but I /think/ it tips in favor of adding the template parameter DIEs rather than keeping them in the name)

I think both options should work in LLDB. FWIW, proper template parameter DIEs usually lead to a nicer AST inside LLDB as we can convert DIEs relatively straightforward back to Clang template parameters.

Fair enough - is it nicer enough that you'd want to carry the parameter DIEs on declarations even in cases where the name had to carry the parameter list too (there are some cases that are hard to rebuild - like pointer non-type template parameters).

This could use a test? A bit of manually modified assembly - or this patch can wait until I've committed the simplified names under a flag & that can be used to test the behavior in lldb?

Is there a review open for that patch? I'm mostly curious about the results of running the LLDB test suit against it (with the flag enabled by default so that we can see what breaks).

Not yet. Working on cleaning up some clang AST printing bugs/quirks/inconsistencies that I discovered while doing round-trip testing - then I should be able to start trying to tidy up the patches themselves for upload.

Also yes, a test for the new/old behaviour still WIP as mentioned above.

In D108634#2963664, @dblaikie wrote:

Yeah, I see what you mean - but yeah, that looks like something where we should probably change the DWARF to match other templates. (& then separately opt in to that behavior across all templates with the simplified template name work)

But not sure if you need to support current clang in newer/fixed lldbs, regardless of whether clang is changed to put the parameters on the DW_AT_name?

If you mean whether a future LLDB (that has variable template support) should support parsing DWARF from Clang 12, then that's a supported use case.

I guess what I mean is: Would it be sufficient to treat this as a bug in clang, and fix it there (adding the template parameters to the name of the variable, instead of adjusting lldb to handle the absence)? I feel like the inconsistency should be fixed. Though I certainly would be happy to also see more support for the mode I'm trying to create - with the parameters not being in the name & ultimately that code would need to be dynamic anyway, because some things will still need the parameters in the name, so maybe my question/suggestion (fixing clang rather than lldb) is moot - that I'll need the fixes in lldb anyway, so the current inconsistency in clang isn't really all that important to address.

But from what I can see there is no special code needed for variable templates before/after the DW_AT_name change as the detection anyway can just be based on whether there are children DIEs? Unless there is some way to avoid emitting any template parameter DIEs for a variable template.

[dblaikie]

Side note, came across some discussion of the variable template issue back in: https://reviews.llvm.org/D52057?id=165788#inline-459961

And also I've submitted at least some of the simplified template naming work - you should be able to test it with clang++ -gsimple-template-names -g x.cpp for instance - this will omit template argument lists in DW_AT_name any place where it can be reasonably reconstructed. There are several places where it can't at the moment - mentioned in the commits/comments/test cases, these include, especially, non-type-template parameters of pointer type. They point to real objects - I'm not sure what lldb does with them, but at least to reconstruct a string name would be tricky because we'd have to do symbol lookup, and hope there's a complete symbol table, etc. Lambdas V unnamed classes are ambiguous (I guess LLDB probably just rebuilds them both as unnamed classes) - oh, anything that takes a non-type template parameter that's a function pointer. We don't actually encode a value for this type of NTTP at all... - so hard to know what a debugger should do there if there's also no data in the DW_AT_name.

lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp

TypeSP DWARFASTParserClang::UpdateSymbolContextScopeForType(
   // We are ready to put this type into the uniqued list up at the module
   // level.
   type_list.Insert(type_sp);
 
   dwarf->GetDIEToType()[die.GetDIE()] = type_sp.get();
   return type_sp;
 }
 
+/// Returns true iff the DWARFDIE represents any kind of function, class or
+/// variable template instantiation.
+static bool IsTemplateDIE(const DWARFDIE &die) {
+  return llvm::any_of(die.children(), [](const DWARFDIE &child) {
+    switch (child.Tag()) {
+    case DW_TAG_template_type_parameter:
+    case DW_TAG_template_value_parameter:
+    case DW_TAG_GNU_template_template_param:
+    case DW_TAG_GNU_template_parameter_pack:
+      return true;
+    default:
+      return false;
+    }
+  });
+}
+
 TypeSP
 DWARFASTParserClang::ParseStructureLikeDIE(const SymbolContext &sc,
                                            const DWARFDIE &die,
                                            ParsedDWARFTypeAttributes &attrs) {
   TypeSP type_sp;
   CompilerType clang_type;
   const dw_tag_t tag = die.Tag();
   SymbolFileDWARF *dwarf = die.GetDWARF();
if (attrs.accessibility == eAccessNone && decl_ctx) {
       if (DeclKindIsCXXClass(containing_decl_kind))
         attrs.accessibility = default_accessibility;
     }
 
     ClangASTMetadata metadata;
     metadata.SetUserID(die.GetID());
     metadata.SetIsDynamicCXXType(dwarf->ClassOrStructIsVirtual(die));
 
-    if (attrs.name.GetStringRef().contains('<')) {
+    if (IsTemplateDIE(die)) {
       TypeSystemClang::TemplateParameterInfos template_param_infos;
       if (ParseTemplateParameterInfos(die, template_param_infos)) {
         clang::ClassTemplateDecl *class_template_decl =
             m_ast.ParseClassTemplateDecl(
                 decl_ctx, GetOwningClangModule(die), attrs.accessibility,
                 attrs.name.GetCString(), tag_decl_kind, template_param_infos);
         if (!class_template_decl) {
           if (log) {