[lldb][DWARFASTParserClang] Attach linkage name to ctors/dtors if missing

[Michael137]

Summary

Summary

This patch addresses the case where we have a DW_AT_external
subprogram for a constructor (and/or destructor) that doesn't carry
a DW_AT_linkage_name attribute. The corresponding DIE(s) that
represent the definition will have a linkage name, but if the name
contains constructs that LLDBs fallback mechanism for guessing mangled
names to resolve external symbols doesn't support (e.g., abi-tags)
then we end up failing to resolve the function call.

We address this by trying to find the linkage name before we create
the constructor/destructor decl, which will get attached using
an AsmLabelAttr to make symbol resolution easier.

This is useful for cases where we try to evaluate expressions that return libcxx
types whose constructors/destructors are labelled with ABI tags (e.g., std::shared_ptr)

Testing

• Added API test

[Michael137]

• clang-format
• Minor cleanup

[Michael137]

• add CHECK-NEXT to test
• reword commit

[Michael137]

• Simplify test

[aprantl]

Nice!

lldb/test/API/lang/cpp/external_ctor_dtor_lookup/TestExternalCtorDtorLookup.py

3

This sentence misses a verb :-)

[JDevlieghere]

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

914–915

[Michael137]

• Reduce scope of local var

[Michael137]

On a release build, for a type instantiated with 50,000 distinct types I get the following results:

Benchmark 1: ./lldb-build-release-with-patch/bin/lldb a.out -o "b main" -o "r" -o "p b.sinkWrapper(b.getWrapper())"
  Time (mean ± σ):      7.599 s ±  0.125 s    [User: 6.836 s, System: 0.371 s]
  Range (min … max):    7.392 s …  7.762 s    10 runs

Benchmark 1: ./lldb-build-release-no-patch/bin/lldb a.out -o "b main" -o "r" -o "p b.sinkWrapper(b.getWrapper())"
  Time (mean ± σ):      7.303 s ±  0.113 s    [User: 6.563 s, System: 0.348 s]
  Range (min … max):    7.073 s …  7.468 s    10 runs

So a 0.3s slowdown (but including startup)

[MaskRay]

This seems to cause many lldb failures https://lab.llvm.org/buildbot/#/builders/68/builds/47790

[Michael137]

In D143652#4119484, @MaskRay wrote:

This seems to cause many lldb failures https://lab.llvm.org/buildbot/#/builders/68/builds/47790

Argh thanks for reporting, let me revert for now and try on my linux machine

[labath]

lldb/test/API/lang/cpp/external_ctor_dtor_lookup/TestExternalCtorDtorLookup.py

29

I think this could be a bit of a problem, because (as you've probably found out by now) there are multiple versions of a single constructor, and the asm label seems to cause clang to coalesce them. In the simple test case below that doesn't matter, as the two constructors are identical, but things might be different if the class had virtual bases. (i.e., it could cause us to call the wrong constructor and blow up).

lldb/test/API/lang/cpp/external_ctor_dtor_lookup/lib.h

7

superfluous semicolon

[Michael137]

lldb/test/API/lang/cpp/external_ctor_dtor_lookup/TestExternalCtorDtorLookup.py

29

I may be misunderstanding, but wouldn't they just get added as extra CXXConstructorDecls on the AST with distinct AsmLabels? Each constructor subprogram DIE links to some specification, which is the definition of the constructor we should call. That's where we get the linkage name from. Playing around with virtual bases I didn't yet manage to come up with a counterexample of where we would pick the wrong constructor

Although I did now notice that there's an extra destructor call in some cases where I didn't expect one before. Maybe that's a manifestation of the issue you describe. Investigating...

[Michael137]

lldb/test/API/lang/cpp/external_ctor_dtor_lookup/TestExternalCtorDtorLookup.py

29

You probably mean multiple constructor definitions can point to the same specification DIE? In which case we would choose the first one happen to see. To what extent can these definitions differ?

[labath]

lldb/test/API/lang/cpp/external_ctor_dtor_lookup/TestExternalCtorDtorLookup.py

29

I mean the "subobject" and "full" constructor (and destructor) variants. There is only a single DIE for the constructor (presumably, because there is only one constructor at the source level), but there are two functions in the object.

You can try the following program to get an idea of what's happening:

struct A {
  A(int) {}
  A(float) {}
};

struct B : virtual A {
  B();
};
B::B() : A(47) {}

struct C : B {
  C() : A(4.7f) {}
};

int main() {
  B b;
  C c;
}

In this program, the b constructor calls the full object constructor for B (_ZN1BC1Ev), whereas the B object constructed as a part of the c full object calls the base object constructor for B (_ZN1BC2Ev).

[Michael137]

lldb/test/API/lang/cpp/external_ctor_dtor_lookup/TestExternalCtorDtorLookup.py

29

Ok that's very useful, thanks!
That does indeed look problematic. Will again revert for now. If we want to go down the linkage-name path I'd have to see if there's more context in DWARFASTParserClang that we can use to filter down to the correct definition. Maybe there's a better way to get these symbols resolved. Don't think the fallback to a guessed mangled name can save us here though

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

ConvertDWARFCallingConventionToClang(const ParsedDWARFTypeAttributes &attrs) {
 
   Log *log = GetLog(DWARFLog::TypeCompletion | DWARFLog::Lookups);
   LLDB_LOG(log, "Unsupported DW_AT_calling_convention value: {0}",
            attrs.calling_convention);
   // Use the default calling convention as a fallback.
   return clang::CC_C;
 }
 
+/// Given a DIE with an external definition (and thus no linkage name)
+/// find the definitions by lookup into the DWARF name index.
+/// We check the DW_AT_specification for each DIE in the index with
+/// the same name as the specified 'die' until we find one that references
+/// 'die'. Then return that linkage name. If no such DIE is found in the index,
+/// returns nullptr.
+static char const *FindLinkageName(DWARFDIE die) {
+  auto *dwarf = die.GetDWARF();
+  if (!dwarf)
+    return nullptr;
+
+  ConstString func_name(die.GetName());
+  if (!func_name)
+    return nullptr;
+
+  SymbolContextList sc_list;
+  Module::LookupInfo lookup_info(func_name,
+                                 FunctionNameType::eFunctionNameTypeMethod |
+                                     FunctionNameType::eFunctionNameTypeFull,
+                                 LanguageType::eLanguageTypeUnknown);
+  dwarf->FindFunctions(lookup_info, {}, true, sc_list);
+
+  for (auto const &sc : sc_list.SymbolContexts()) {
+    auto *func = sc.function;
+    if (func) {
+      auto func_die = dwarf->GetDIE(func->GetID());
+      if (!func_die.IsValid())
+        continue;
+
+      auto spec_die =
+          func_die.GetAttributeValueAsReferenceDIE(DW_AT_specification);
+      if (spec_die.IsValid() && spec_die == die) {
+        return func->GetMangled().GetMangledName().AsCString();
+      }
+    }
+  }
+
+  return nullptr;
+}
+
 TypeSP DWARFASTParserClang::ParseSubroutine(const DWARFDIE &die,
                            ParsedDWARFTypeAttributes &attrs) {
   Log *log = GetLog(DWARFLog::TypeCompletion | DWARFLog::Lookups);
 
   SymbolFileDWARF *dwarf = die.GetDWARF();
   const dw_tag_t tag = die.Tag();
 
   bool is_variadic = false;
if (tag == DW_TAG_subprogram || tag == DW_TAG_inlined_subroutine) {
 
                   const bool is_attr_used = false;
                   // Neither GCC 4.2 nor clang++ currently set a valid
                   // accessibility in the DWARF for C++ methods...
                   // Default to public for now...
                   if (attrs.accessibility == eAccessNone)
                     attrs.accessibility = eAccessPublic;
 
+                  // Make sure we find the linkage name here so it gets
+                  // attached to the member function inside
+                  // AddMethodToCXXRecordType below.
+                  if (!attrs.mangled_name && attrs.storage == clang::SC_Extern)
+                    attrs.mangled_name = FindLinkageName(die);
+
                   clang::CXXMethodDecl *cxx_method_decl =
                       m_ast.AddMethodToCXXRecordType(
                           class_opaque_type.GetOpaqueQualType(),
                           attrs.name.GetCString(), attrs.mangled_name,
                           clang_type, attrs.accessibility, attrs.is_virtual,
                           is_static, attrs.is_inline, attrs.is_explicit,
                           is_attr_used, attrs.is_artificial);
 

lldb/test/API/lang/cpp/external_ctor_dtor_lookup/Makefile

+CXX_SOURCES := main.cpp
+
+include Makefile.rules

lldb/test/API/lang/cpp/external_ctor_dtor_lookup/TestExternalCtorDtorLookup.py

+"""
+Test that we can constructors/destructors
+without a linkage name because they are
+marked DW_AT_external and the fallback
+mangled-name-guesser in LLDB doesn't account
+for ABI tags.
+"""
+
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+class ExternalCtorDtorLookupTestCase(TestBase):
+
+    def test(self):
+        self.build()
+        lldbutil.run_to_source_breakpoint(self, 'getTarget\(\)\)',
+                lldb.SBFileSpec('main.cpp', False))
+
+        self.expect_expr('s.foo(s.getTarget())', result_type='int', result_value='-1')
+        self.filecheck("target module dump ast", __file__)
+# CHECK:      ClassTemplateSpecializationDecl {{.*}} class Wrapper definition
+# CHECK:      |-TemplateArgument type 'Foo'
+# CHECK:      | `-RecordType {{.*}} 'Foo'
+# CHECK:      |   `-CXXRecord {{.*}} 'Foo'
+# CHECK:      |-CXXConstructorDecl {{.*}} Wrapper 'void ()'
+# CHECK-NEXT: | `-AsmLabelAttr {{.*}} Implicit "_ZN7WrapperI3FooEC1B4testEv"
+# CHECK-NEXT: `-CXXDestructorDecl {{.*}} ~Wrapper 'void ()'
+# CHECK-NEXT:   `-AsmLabelAttr {{.*}} Implicit "_ZN7WrapperI3FooED1B4testEv"

lldb/test/API/lang/cpp/external_ctor_dtor_lookup/lib.h

+#ifndef SPTR_H_IN
+#define SPTR_H_IN
+
+template <typename T> class Wrapper {
+public:
+  [[gnu::abi_tag("test")]] Wrapper(){};
+
+  [[gnu::abi_tag("test")]] ~Wrapper(){};
+};
+
+#endif // _H_IN

lldb/test/API/lang/cpp/external_ctor_dtor_lookup/main.cpp

+#include "lib.h"
+
+struct Foo {};
+
+using FooWrapped = Wrapper<Foo>;
+
+struct S {
+  FooWrapped getTarget() { return Wrapper<Foo>(); }
+  int foo(FooWrapped) const { return -1; }
+  void bar() { foo(getTarget()); }
+};
+
+int main() {
+  S s;
+  s.foo(s.getTarget());
+  return 0;
+}