[lldb] Fix simple template names interaction with debug info declarations

[aeubanks]

Summary

Without checking template parameters, we would sometimes lookup the
wrong type definition for a type declaration because different
instantiations of the same template class had the same debug info name.

The added GetForwardDeclarationDIETemplateParams() shouldn't need a
cache because we'll cache the results of the declaration -> definition
lookup anyway. (DWARFASTParserClang::ParseStructureLikeDIE()
is_forward_declaration branch)

[aeubanks]

depends on D138612

[dblaikie]

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

747–749

Could invert this condition and use an early return ConstString(); to reduce indentation - but I guess this looks more similar to the definition handling case and so might be worth keeping in the same shape. (maybe ParseTemplateParameterInfos should return true only if the result is !empty or has packed args, so that test doesn't need to be repeated at multiple callers?)

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

3067–3077

might be worth flipping to reduce indentation:

auto i = test_base_name.find('<');
if (i == llvm::StringRef::npos)
  return true;
llvm::StringRef test_template_params = ...
if (test_template_params != template_params.GetStringRef())
  return true;

lldb/test/API/lang/cpp/unique-types3/main.cpp

2–15

maybe good to simplify this a bit - rather than using big/complex templates like std::atomic and std::vector, instead using test-only, simpler templates to keep the test a bit more focussed (less likely to fail for unrelated reasons/other regressions)?

[aeubanks]

don't use stl in test
simplify some code a bit

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

747–749

I'll clean this up after this patch lands

lldb/test/API/lang/cpp/unique-types3/main.cpp

2–15

hmm I swear I tried that and it ended up passing, although that might have been before I figured out the Makefile env var vs normal var issue. done

[aeubanks]

rebase

[dblaikie]

I think this is good - welcome to wait for a second opinion if you prefer, or folks can provide feedback post-commit.

[Michael137]

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

91

Can we add a docstring?

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

2988

So after this call we'll have an unnamed ClassTemplateDecl hanging off the AST? Doing this inside FindDefinitionTypeForDWARFDeclContext instead of ParseStructureLikeDIE feels a bit off. But will have to understand this change a bit deeper to know whether there is something we can do about that.

As a side-note, we seem to be accumulating a lot of blocks around LLDB that look like:

// Do X because -gsimple-template-names
if (name.contains('<')) {
   // Do something
}

Still wonder if there isn't some flag that can be set on, e.g., a CU, that would tell us this.

3062

[Michael137]

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

765

Ok so what we're doing is:

1. Create a ClassTemplateSpecializationDecl with an empty basename
2. Return the type-name and since the basename is empty we end up with just the template arguments

Why can't we just call GetTemplateParametersString(die) instead of creating this function?

[Michael137]

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

765

Can confirm that this works locally. Though required moving that function out of the DWARFASTParserClang, which seems OK

[aeubanks]

address comments

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

765

GetTemplateParametersString is specifically only used for GetCPlusPlusQualifiedName, which is used below

// For C++, we rely solely upon the one definition rule that says
// only one thing can exist at a given decl context. We ignore the
// file and line that things are declared on.
std::string qualified_name = GetCPlusPlusQualifiedName(die);

so it doesn't have to match clang's printing. but for the simple template name stuff, we are comparing clang-generated names, so everything needs to go through clang.

I've replaced GetTemplateParametersString with this code that goes through clang as well

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

2988

the effect is not really per-CU but rather per-DIE because in some cases (where DWARF roundtripping is lossy, @dblaikie would know best) we still put template params in the name. so it needs to be checked per-DIE

I agree that it's weird to create clang AST nodes here and then forget about them, but clang uses a bump allocator and we can't easily delete it. As long as we call this at most once or twice per DIE, I don't think it's worth caching the results, but if we do end up using this more, we do need to cache the created AST nodes. I've added a comment to the docstring.

[Michael137]

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

765

But didn't GetTemplateParametersString go through Clang's type printer too? Probably missing something.
What would be an example of the difference in output that could arise between the new vs. old method?

[aeubanks]

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

765

one thing that I was aware of even initially implementing GetTemplateParametersString was spacing between two >, clang will add a space (unless you tell it not to), whereas the naive implementation didn't

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

public:
   GetDeclContextForUIDFromDWARF(const DWARFDIE &die) = 0;
 
   virtual lldb_private::CompilerDeclContext
   GetDeclContextContainingUIDFromDWARF(const DWARFDIE &die) = 0;
 
   virtual void EnsureAllDIEsInDeclContextHaveBeenParsed(
       lldb_private::CompilerDeclContext decl_context) = 0;
 
+  virtual lldb_private::ConstString
+  GetDIEClassTemplateParams(const DWARFDIE &die) = 0;
+
   static llvm::Optional<lldb_private::SymbolFile::ArrayInfo>
   ParseChildArrayInfo(const DWARFDIE &parent_die,
                       const lldb_private::ExecutionContext *exe_ctx = nullptr);
 
   static lldb::AccessType GetAccessTypeFromDWARF(uint32_t dwarf_accessibility);
 };
 
 #endif // LLDB_SOURCE_PLUGINS_SYMBOLFILE_DWARF_DWARFASTPARSER_H

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

public:
   /// \return An APInt containing the same integer value as the given
   ///         DWARFFormValue with the bit width of the given integer type.
   ///         Returns an error if the value in the DWARFFormValue does not fit
   ///         into the given integer type or the integer type isn't supported.
   llvm::Expected<llvm::APInt>
   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;
   typedef std::vector<DelayedAddObjCClassProperty> DelayedPropertyList;
 
   typedef llvm::DenseMap<const DWARFDebugInfoEntry *, clang::DeclContext *>
       DIEToDeclContextMap;

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

type_sp = std::make_shared<Type>(
       die.GetID(), dwarf, attrs.name, attrs.byte_size, nullptr,
       dwarf->GetUID(attrs.type.Reference()), encoding_data_type, &attrs.decl,
       clang_type, resolve_state, TypePayloadClang(GetOwningClangModule(die)));
 
   dwarf->GetDIEToType()[die.GetDIE()] = type_sp.get();
   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) &&
+      (!template_param_infos.args.empty() ||
+       template_param_infos.packed_args)) {
+    // 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();
+}
+
 TypeSP DWARFASTParserClang::ParseEnum(const SymbolContext &sc,
                                       const DWARFDIE &die,
                                       ParsedDWARFTypeAttributes &attrs) {
   Log *log = GetLog(DWARFLog::TypeCompletion | DWARFLog::Lookups);
   SymbolFileDWARF *dwarf = die.GetDWARF();
   const dw_tag_t tag = die.Tag();
   TypeSP type_sp;
 
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::GetTemplateParametersString(const DWARFDIE &die) {
-  if (llvm::StringRef(die.GetName()).contains("<"))
-    return std::string();
-  TypeSystemClang::TemplateParameterInfos template_param_infos;
-  if (!ParseTemplateParameterInfos(die, template_param_infos))
-    return std::string();
-  std::string all_template_names;
-  llvm::SmallVector<clang::TemplateArgument, 2> args =
-      template_param_infos.args;
-  if (template_param_infos.hasParameterPack())
-    args.append(template_param_infos.packed_args->args);
-  if (args.empty())
-    return std::string();
-  for (auto &arg : args) {
-    std::string template_name;
-    llvm::raw_string_ostream os(template_name);
-    arg.print(m_ast.getASTContext().getPrintingPolicy(), os, true);
-
-    if (!template_name.empty()) {
-      if (all_template_names.empty()) {
-        all_template_names.append("<");
-      } else {
-        all_template_names.append(", ");
-      }
-      all_template_names.append(template_name);
-    }
-  }
-  assert(!all_template_names.empty() && "no template parameters?");
-  // Spacing doesn't matter as long as we're consistent because we're only using
-  // this to deduplicate C++ symbols.
-  all_template_names.append(">");
-  return all_template_names;
-}
-
-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();
case DW_TAG_namespace: {
       parent_decl_ctx_die = parent_decl_ctx_die.GetParentDeclContextDIE();
       break;
     }
 
     case DW_TAG_class_type:
     case DW_TAG_structure_type:
     case DW_TAG_union_type: {
       if (const char *class_union_struct_name = parent_decl_ctx_die.GetName()) {
-        qualified_name.insert(0,
-                              GetTemplateParametersString(parent_decl_ctx_die));
+        qualified_name.insert(
+            0, GetDIEClassTemplateParams(parent_decl_ctx_die).AsCString(""));
         qualified_name.insert(0, "::");
         qualified_name.insert(0, class_union_struct_name);
       }
       parent_decl_ctx_die = parent_decl_ctx_die.GetParentDeclContextDIE();
       break;
     }
 
     default:
       parent_decl_ctx_die.Clear();
       break;
     }
   }
 
   if (qualified_name.empty())
     qualified_name.append("::");
 
   qualified_name.append(name);
-  qualified_name.append(GetTemplateParametersString(die));
+  qualified_name.append(GetDIEClassTemplateParams(die).AsCString(""));
 
   return qualified_name;
 }
 
 TypeSP
 DWARFASTParserClang::ParseStructureLikeDIE(const SymbolContext &sc,
                                            const DWARFDIE &die,
                                            ParsedDWARFTypeAttributes &attrs) {

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

if (language != eLanguageTypeUnknown) {
         LLDB_LOG_ERROR(GetLog(LLDBLog::Symbols), std::move(err),
                        "Cannot get TypeSystem for language {}",
                        Language::GetNameForLanguageType(language));
       } else {
         type_system = *type_system_or_err;
       }
     }
 
+    // See comments below about -gsimple-template-names for why we attempt to
+    // compute missing template parameter names.
+    ConstString template_params;
+    if (type_system) {
+      DWARFASTParser *dwarf_ast = type_system->GetDWARFParser();
+      if (dwarf_ast)
+        template_params = dwarf_ast->GetDIEClassTemplateParams(die);
+    }
+
     m_index->GetTypes(GetDWARFDeclContext(die), [&](DWARFDIE type_die) {
       // Make sure type_die's language matches the type system we are
       // looking for. We don't want to find a "Foo" type from Java if we
       // are looking for a "Foo" type for C, C++, ObjC, or ObjC++.
       if (type_system &&
           !type_system->SupportsLanguage(GetLanguage(*type_die.GetCU())))
         return true;
       bool try_resolving_type = false;
m_index->GetTypes(GetDWARFDeclContext(die), [&](DWARFDIE type_die) {
       // Make sure the decl contexts match all the way up
       if (GetDWARFDeclContext(die) != type_dwarf_decl_ctx)
         return true;
 
       Type *resolved_type = ResolveType(type_die, false);
       if (!resolved_type || resolved_type == DIE_IS_BEING_PARSED)
         return true;
 
+      // With -gsimple-template-names, the DIE name may not contain the template
+      // parameters. If the declaration has template parameters but doesn't
+      // contain '<', check that the child template parameters match.
+      if (template_params) {
+        llvm::StringRef test_base_name =
+            GetTypeForDIE(type_die)->GetBaseName().GetStringRef();
+        auto i = test_base_name.find('<');
+
+        // Full name from clang AST doesn't contain '<' so this type_die isn't
+        // a template parameter, but we're expecting template parameters, so
+        // bail.
+        if (i == llvm::StringRef::npos)
+          return true;
+
+        llvm::StringRef test_template_params =
+            test_base_name.slice(i, test_base_name.size());
+        // Bail if template parameters don't match.
+        if (test_template_params != template_params.GetStringRef())
+          return true;
+      }
+
       type_sp = resolved_type->shared_from_this();
       return false;
     });
   }
   return type_sp;
 }
 
 TypeSP SymbolFileDWARF::ParseType(const SymbolContext &sc, const DWARFDIE &die,

lldb/test/API/lang/cpp/unique-types3/Makefile

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

lldb/test/API/lang/cpp/unique-types3/TestUniqueTypes3.py

+"""
+Test that we return only the requested template instantiation.
+"""
+
+import lldb
+import lldbsuite.test.lldbutil as lldbutil
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+
+class UniqueTypesTestCase3(TestBase):
+    def do_test(self, debug_flags):
+        """Test that we display the correct template instantiation."""
+        self.build(dictionary=debug_flags)
+        lldbutil.run_to_source_breakpoint(self, "// Set breakpoint here", lldb.SBFileSpec("a.cpp"))
+        self.expect_expr("a", result_type="S<int>")
+
+    @skipIf(compiler=no_match("clang"))
+    @skipIf(compiler_version=["<", "15.0"])
+    def test_simple_template_names(self):
+        # Can't directly set CFLAGS_EXTRAS here because the Makefile can't
+        # override an environment variable.
+        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(CFLAGS_EXTRAS="-gno-simple-template-names"))

lldb/test/API/lang/cpp/unique-types3/a.h

+template <typename T> struct S {
+  T t;
+};

lldb/test/API/lang/cpp/unique-types3/a.cpp

+#include "a.h"
+
+void f(S<int> &a) {
+  (void)a; // Set breakpoint here
+}

lldb/test/API/lang/cpp/unique-types3/main.cpp

+#include "a.h"
+
+S<double> a1;
+S<int> a2;
+S<float> a3;
+
+void f(S<int> &);
+
+int main() { f(a2); }