[lldb] Don't create Clang AST nodes in GetDIEClassTemplateParams

[aeubanks]

Summary

Otherwise we may be inserting a decl into a DeclContext that's not fully defined yet.

This simplifies/removes some clang AST node creation code. Instead, use
clang::printTemplateArgumentList().

[aeubanks]

there may be an alternate solution involving making some declarations into definitions (at all? earlier?), but I don't have that level of understanding of lldb

[Michael137]

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

1514

Ah the function was declared but not defined (presumably a leftover from https://reviews.llvm.org/D138834)

Can we elaborate in the function docstring how this differs from GetTemplateParameters?

1524–1525

While you're here mind correcting this?

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

100

Btw, what's guaranteeing that we don't call this more than once per DIE? Is there any way we could add a test that would notify us of that kind of regression? Or perhaps an assert somewhere in the DWARFASTParserClang?

lldb/test/API/lang/cpp/nested-template/TestNestedTemplate.py

14

Nit: technically don't need to run the program to test image lookup?

lldb/test/API/lang/cpp/nested-template/main.cpp

11

Nit: we typically use std::puts("Set breakpoint here") or return 0 for source regex breakpoints. The rationale was that breaking on comments isn't always reliable/future-proof.

[Michael137]

In D142413#4075198, @aeubanks wrote:

there may be an alternate solution involving making some declarations into definitions (at all? earlier?), but I don't have that level of understanding of lldb

One idea that came to mind is that instead of creating a dummy template decl in GetDIEClassTemplateParams, you could perhaps construct a clang::TemplateArgumentList from the TemplateArguments stored in TemplateParameterInfos and use the clang::printTemplateArgumentList to stringify the template arguments, passing it the printing-policy which is used in TypeSystemClang to print type-names in your current implementation.

[Michael137]

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

1514

Meant to say GetDIEClassTemplateParams

[aeubanks]

use suggested alternative

[aeubanks]

In D142413#4076749, @Michael137 wrote:

In D142413#4075198, @aeubanks wrote:

there may be an alternate solution involving making some declarations into definitions (at all? earlier?), but I don't have that level of understanding of lldb

One idea that came to mind is that instead of creating a dummy template decl in GetDIEClassTemplateParams, you could perhaps construct a clang::TemplateArgumentList from the TemplateArguments stored in TemplateParameterInfos and use the clang::printTemplateArgumentList to stringify the template arguments, passing it the printing-policy which is used in TypeSystemClang to print type-names in your current implementation.

this worked perfectly and simplified a lot of code, thanks!

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

1514

yeah I forgot to remove the declaration in that patch :|

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

100

now we don't create a clang AST node

lldb/test/API/lang/cpp/nested-template/TestNestedTemplate.py

14

last time I couldn't find the right thing to call to load the binary but not set a breakpoint, but perhaps this works?

lldb/test/API/lang/cpp/nested-template/main.cpp

11

obsolete now that we don't set a breakpoint

[Michael137]

awesome, looks much better now
thanks!

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

ExtractIntFromFormValue(const lldb_private::CompilerType &int_type,
                           const DWARFFormValue &form_value) const;
 
   /// Returns the template parameters of a class DWARFDIE as a string.
   ///
   /// This is mostly useful for -gsimple-template-names which omits template
   /// parameters from the DIE name and instead always adds template parameter
   /// children DIEs.
   ///
-  /// Currently this is only called in two places, when uniquing C++ classes and
-  /// when looking up the definition for a declaration (which is then cached).
-  /// If this is ever called more than twice per DIE, we need to start caching
-  /// the results to prevent unbounded growth of the created clang AST nodes.
-  ///
   /// \param die The struct/class DWARFDIE containing template parameters.
   /// \return A string, including surrounding '<>', of the template parameters.
   /// If the DIE's name already has '<>', returns an empty ConstString because
   /// it's assumed that the caller is using the DIE name anyway.
   lldb_private::ConstString
   GetDIEClassTemplateParams(const DWARFDIE &die) override;
 
 protected:
   /// Protected typedefs and members.
   /// @{
   class DelayedAddObjCClassProperty;
bool ParseTemplateDIE(const DWARFDIE &die,
                         lldb_private::TypeSystemClang::TemplateParameterInfos
                             &template_param_infos);
 
   bool ParseTemplateParameterInfos(
       const DWARFDIE &parent_die,
       lldb_private::TypeSystemClang::TemplateParameterInfos
           &template_param_infos);
 
-  /// Get the template parameters of a die as a string if the die name does not
-  /// already contain them. This happens with -gsimple-template-names.
-  std::string GetTemplateParametersString(const DWARFDIE &die);
-
   std::string GetCPlusPlusQualifiedName(const DWARFDIE &die);
 
   bool ParseChildMembers(
       const DWARFDIE &die, lldb_private::CompilerType &class_compiler_type,
       std::vector<std::unique_ptr<clang::CXXBaseSpecifier>> &base_classes,
       std::vector<DWARFDIE> &member_function_dies,
       DelayedPropertyList &delayed_properties,
       const lldb::AccessType default_accessibility,

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

DWARFASTParserClang::ParseTypeModifier(const SymbolContext &sc,
   return type_sp;
 }
 
 ConstString
 DWARFASTParserClang::GetDIEClassTemplateParams(const DWARFDIE &die) {
   if (llvm::StringRef(die.GetName()).contains("<"))
     return ConstString();
 
-  clang::DeclContext *decl_ctx = GetClangDeclContextContainingDIE(die, nullptr);
   TypeSystemClang::TemplateParameterInfos template_param_infos;
   if (ParseTemplateParameterInfos(die, template_param_infos)) {
-    // Most of the parameters here don't matter, but we make sure the base name
-    // is empty so when we print the name we only get the template parameters.
-    clang::ClassTemplateDecl *class_template_decl =
-        m_ast.ParseClassTemplateDecl(decl_ctx, GetOwningClangModule(die),
-                                     eAccessPublic, "", clang::TTK_Struct,
-                                     template_param_infos);
-    if (!class_template_decl)
-      return ConstString();
-
-    clang::ClassTemplateSpecializationDecl *class_specialization_decl =
-        m_ast.CreateClassTemplateSpecializationDecl(
-            decl_ctx, GetOwningClangModule(die), class_template_decl,
-            clang::TTK_Struct, template_param_infos);
-    if (!class_specialization_decl)
-      return ConstString();
-    CompilerType clang_type =
-        m_ast.CreateClassTemplateSpecializationType(class_specialization_decl);
-    ConstString name = clang_type.GetTypeName(/*BaseOnly*/ true);
-    return name;
+    return ConstString(m_ast.PrintTemplateParams(template_param_infos));
   }
   return ConstString();
 }
 
 TypeSP DWARFASTParserClang::ParseEnum(const SymbolContext &sc,
                                       const DWARFDIE &die,
                                       ParsedDWARFTypeAttributes &attrs) {
   Log *log = GetLog(DWARFLog::TypeCompletion | DWARFLog::Lookups);
TypeSP DWARFASTParserClang::UpdateSymbolContextScopeForType(
 
   if (symbol_context_scope != nullptr)
     type_sp->SetSymbolContextScope(symbol_context_scope);
 
   dwarf->GetDIEToType()[die.GetDIE()] = type_sp.get();
   return type_sp;
 }
 
 std::string
 DWARFASTParserClang::GetCPlusPlusQualifiedName(const DWARFDIE &die) {
   if (!die.IsValid())
     return "";
   const char *name = die.GetName();
   if (!name)
     return "";
   std::string qualified_name;
   DWARFDIE parent_decl_ctx_die = die.GetParentDeclContextDIE();
   // TODO: change this to get the correct decl context parent....
   while (parent_decl_ctx_die) {
-    // The name may not contain template parameters due to simplified template
-    // names; we must reconstruct the full name from child template parameter
-    // dies via GetTemplateParametersString().
+    // The name may not contain template parameters due to
+    // -gsimple-template-names; we must reconstruct the full name from child
+    // template parameter dies via GetDIEClassTemplateParams().
     const dw_tag_t parent_tag = parent_decl_ctx_die.Tag();
     switch (parent_tag) {
     case DW_TAG_namespace: {
       if (const char *namespace_name = parent_decl_ctx_die.GetName()) {
         qualified_name.insert(0, "::");
         qualified_name.insert(0, namespace_name);
       } else {
         qualified_name.insert(0, "(anonymous namespace)::");

lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h

#endif
   /// Complete a type from debug info, or mark it as forcefully completed if
   /// there is no definition of the type in the current Module. Call this
   /// function in contexts where the usual C++ rules require a type to be
   /// complete (base class, member, etc.).
   static void RequireCompleteType(CompilerType type);
 
   bool SetDeclIsForcefullyCompleted(const clang::TagDecl *td);
 
+  /// Return the template parameters (including surrounding <>) in string form.
+  std::string
+  PrintTemplateParams(const TemplateParameterInfos &template_param_infos);
+
 private:
   /// Returns the PrintingPolicy used when generating the internal type names.
   /// These type names are mostly used for the formatter selection.
   clang::PrintingPolicy GetTypePrintingPolicy();
   /// Returns the internal type name for the given NamedDecl using the
   /// type printing policy.
   std::string GetTypeNameForDecl(const clang::NamedDecl *named_decl,
                                  bool qualified = true);

lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp

static TemplateParameterList *CreateTemplateParameterList(
   }
   clang::Expr *const requires_clause = nullptr; // TODO: Concepts
   TemplateParameterList *template_param_list = TemplateParameterList::Create(
       ast, SourceLocation(), SourceLocation(), template_param_decls,
       SourceLocation(), requires_clause);
   return template_param_list;
 }
 
+std::string TypeSystemClang::PrintTemplateParams(
+    const TemplateParameterInfos &template_param_infos) {
+  llvm::SmallVector<NamedDecl *, 8> ignore;
+  clang::TemplateParameterList *template_param_list =
+      CreateTemplateParameterList(getASTContext(), template_param_infos,
+                                  ignore);
+  llvm::SmallVector<clang::TemplateArgument, 2> args =
+      template_param_infos.args;
+  if (template_param_infos.hasParameterPack()) {
+    args.append(template_param_infos.packed_args->args);
+  }
+  std::string str;
+  llvm::raw_string_ostream os(str);
+  clang::printTemplateArgumentList(os, args, GetTypePrintingPolicy(),
+                                   template_param_list);
+  return str;
+}
+
 clang::FunctionTemplateDecl *TypeSystemClang::CreateFunctionTemplateDecl(
     clang::DeclContext *decl_ctx, OptionalClangModuleID owning_module,
     clang::FunctionDecl *func_decl,
     const TemplateParameterInfos &template_param_infos) {
   //    /// Create a function template node.
   ASTContext &ast = getASTContext();
 
   llvm::SmallVector<NamedDecl *, 8> template_param_decls;

lldb/test/API/lang/cpp/nested-template/Makefile

+CXX_SOURCES := main.cpp
+
+CFLAGS_EXTRAS = $(TEST_CFLAGS_EXTRAS) $(LIMIT_DEBUG_INFO_FLAGS)
+
+include Makefile.rules

lldb/test/API/lang/cpp/nested-template/TestNestedTemplate.py

+"""
+Test that a nested template parameter works with simple template names.
+"""
+
+import lldb
+import lldbsuite.test.lldbutil as lldbutil
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+
+class NestedTemplateTestCase(TestBase):
+    def do_test(self, debug_flags):
+        self.build(dictionary=debug_flags)
+        self.dbg.CreateTarget(self.getBuildArtifact("a.out"))
+        self.expect("image lookup -A -t 'Inner<int>'", DATA_TYPES_DISPLAYED_CORRECTLY, substrs=["1 match found"])
+
+    @skipIf(compiler=no_match("clang"))
+    @skipIf(compiler_version=["<", "15.0"])
+    def test_simple_template_names(self):
+        self.do_test(dict(TEST_CFLAGS_EXTRAS="-gsimple-template-names"))
+
+    @skipIf(compiler=no_match("clang"))
+    @skipIf(compiler_version=["<", "15.0"])
+    def test_no_simple_template_names(self):
+        self.do_test(dict(TEST_CFLAGS_EXTRAS="-gno-simple-template-names"))

lldb/test/API/lang/cpp/nested-template/main.cpp

+struct Outer {
+  Outer() {}
+
+  template <class T>
+  struct Inner {};
+};
+
+int main() {
+  Outer::Inner<int> oi;
+}