lldb: Use the DWARF linkage name when importing C++ methods

[nelhage]

Summary

When importing C++ methods into clang AST nodes from the DWARF symbol table, preserve the DW_AT_linkage_name and use it as the linker ("asm") name for the symbol.

Concretely, this enables expression to call into names that use the GNU abi_tag extension, and enables lldb to call into code using std::string or std::list from recent versions of libstdc++, as described in https://bugs.llvm.org/show_bug.cgi?id=35310 . This also seems potentially broadly more robust than relying on the DWARF->clang->codegen pipeline to roundtrip properly, but I'm not immediately aware of any other cases in which it makes a difference.

[labath]

This seems like a good idea to me (but Greg or Jim should review this).

Could you please also add a test case for this. It looks like you already have a simple program in the bug you linked, so it should be just a matter of turning that into a test. Let me know if you run into problems while doing that.

[nelhage]

Add a test case.

Verified that it passes on my machine and fails with this patch
reverted.

[nelhage]

Remove accidentally-added flycheck file.

[clayborg]

Jim will need to ok this. A few concerns follow. Most of the time when we don't get the DWARF -> AST conversion right, it can mean that we might code gen incorrect code. Is there not enough information for the GNU abi_tag in the DWARF to actually re-create these classes correctly in the AST? My first inclination would be to fix that if possible. If there is info missing from the DWARF such that we can't re-create the AST correctly, then it seems that this approach will help. Can you clarify what the GNU abi_tag stuff does? Will it change how code would be generated? If we have things in the AST that aren't correct, but have been fixed by correcting the "asm" tag, will we JIT up bad code?
-

[nelhage]

In D40283#936088, @clayborg wrote:

Jim will need to ok this. A few concerns follow. Most of the time when we don't get the DWARF -> AST conversion right, it can mean that we might code gen incorrect code. Is there not enough information for the GNU abi_tag in the DWARF to actually re-create these classes correctly in the AST?

Correct, as far as I can tell. Looking at a symbol definition from the test program on the issue via llvm-dwarfdump, I see

0x00000076:     DW_TAG_subprogram
                  DW_AT_linkage_name    ("_ZN1A16helloWorld_cxx11B5cxx11Ev")
                  DW_AT_name    ("helloWorld_cxx11")
                  DW_AT_decl_file       ("/tmp/test_abi_tag.cc")
                  DW_AT_decl_line       (5)
                  DW_AT_declaration     (true)
                  DW_AT_external        (true)
                  DW_AT_accessibility   (DW_ACCESS_public)

0x00000082:       DW_TAG_formal_parameter
                    DW_AT_type  (cu + 0x009b "A*")
                    DW_AT_artificial    (true)

0x00000087:       NULL

The function of the abi_tag annotation is to append the B5cxx11 to the mangled name; I can't find any other indication in the DWARF of the ABI tag.

You can read more about abi_tag here:
https://gcc.gnu.org/onlinedocs/gcc/C_002b_002b-Attributes.html
Or an explanation of why it exists and how it is used here:
https://davmac.wordpress.com/2015/07/19/tale-of-two-abis/

The effect of this patch on the code generated by expression is to change the names we emit into the LLVM IR; Previously, we would emit a call to _ZN1A16helloWorld_cxx11Ev, which would then fail when resolving symbols. Now we (correctly) emit a call to _ZN1A16helloWorld_cxx11B5cxx11Ev.

The test case I added in this patch also demonstrates that GCC and clang both support arbitrarily overriding a symbol's mangled name in the source using asm("new_name"). I can't recall having ever encountered a use of that in the wild, so I don't care strongly about supporting it, but if we want to I think this general approach is almost certainly the only approach that would do so.

[clayborg]

ok, sounds like the abi_tag stuff is meant to just affect the mangled named with no code gen implications. Jim should ok this, but I am ok if he is.

[jingham]

This patch is actually pretty broad, since it will make all C++ declarations coming from DWARF have overridden names so far as clang is concerned. So we are relying on the fact that as far as clang is concerned a method with an asm assigned name is equivalent to method whose name mangles to the same thing.

From what I can tell scanning through the clang sources, this attribute just overrides whatever mangling clang was intending to do for this decl. There shouldn't be any case where we have an incorrect mangled name in the DWARF so at first glance that should be okay.

But since this affects all C++ method names that we pass to clang, I'd feel better if we could get confirmation from a clang expert that this change has no other side-effects.

[nelhage]

Hey -- Is there anything I can do to move this patch forward? Would it help to do something like only setting the attribute if the mangled name that *would* be generated doesn't match the one from the DWARF? Any such symbol is already broken for lldb, so such a patch feels perhaps safer and more conservative, at the cost of a bit of added complexity.

[clayborg]

Sounds like finding a clang expert to clarify what Jim last asked for is the way forward. Do a source control "blame" command and see who worked on the code in the area of clang and maybe add them as reviewers so they can comment? I agree with Jim that this sounds like a good fix, but we should be careful. Is there no other way to detect this special "asm" name? No extra DWARF attributes?

[clayborg]

Added Adrian Prantl as a reviewer in case he has any input. Adrian: is there any way that a DIE is marked up with an extra attribute when the asm name is explicitly set? It would be great to know this from the DWARF so we don't have to end up setting the ASM name for every single possibly affected DIE...

[clayborg]

And I do agree with Jim that we don't want to have to mangle the typename to see if it matches, that is too much work.

[aprantl]

In D40283#972622, @clayborg wrote:

Added Adrian Prantl as a reviewer in case he has any input. Adrian: is there any way that a DIE is marked up with an extra attribute when the asm name is explicitly set? It would be great to know this from the DWARF so we don't have to end up setting the ASM name for every single possibly affected DIE...

I'm afraid DW_AT_linkage *is* that attribute. If you don't want to explicitly set the mangled name for each entity with a DW_AT_linkage_name, the way to do this is to add a DWARF extension like DW_AT_GNU_abi_tag or something. How does GDB/gcc handle this? Maybe they already invented an encoding for the attribute?

[nelhage]

Compiling my test program with g++ and looking at llvm-dwarfdump -all, I can see no reference to the ABI tag other than in the DW_AT_linkage_name. Skimming the gdb source, I see references to it knowing how to *demangle* ABI tags, but no references in the DWARF code or otherwise in the expression-handler. I strongly suspect that GDB implements essentially this solution, using DW_AT_linkage_name to populate linkage names in its symbol table, and uses its C++ demangler to find ABI tags for display purposes.

[nelhage]

Further evidence that the ABI tag is never stored separately:

$ strings -a -n4 test_abi_tag | grep cxx11
helloWorld_cxx11
_ZN1A16helloWorld_cxx11B5cxx11Ev
_ZN1A16helloWorld_cxx11B5cxx11Ev

Those three instances are, I presume, the DW_AT_name, and the DW_AT_linkage_name, and the ELF symbol table.

[nelhage]

I did a little bit of looking into performance implications today. It looks like DWARFASTParserClang::ParseTypeFromDWARF is only called lazily as symbols are needed, which alleviates some of my concerns, and also makes it a bit trickier to construct a convenient benchmark. Does someone with performance concerns have a specific usage pattern or operation or mode that they're concerned by the performance of for me to dig into?

[davide]

In D40283#983057, @nelhage wrote:

I did a little bit of looking into performance implications today. It looks like DWARFASTParserClang::ParseTypeFromDWARF is only called lazily as symbols are needed, which alleviates some of my concerns, and also makes it a bit trickier to construct a convenient benchmark. Does someone with performance concerns have a specific usage pattern or operation or mode that they're concerned by the performance of for me to dig into?

My take is that we should consider going ahead with this. I tried and I confirm your experience (basically std::string in libstdc++ is undebbuggable).
Please wait a couple of days in case somebody has comments otherwise I'd say this can go in. BTW, do you need somebody to commit this on your behalf?

Thanks!

Davide

[nelhage]

Yes, I will need someone else to commit on my behalf -- I don't have commit access.

[clayborg]

I am fine with checking this. The only issue on my end is the extra memory that will be needed to store these often huge mangled names in every AST context for the 0.0001% of the time it is actually needed.

[davide]

Thanks Greg. I'll wait another day and check this one in. Nelson, does the plan make sense to you?

[aprantl]

In D40283#983908, @clayborg wrote:

I am fine with checking this. The only issue on my end is the extra memory that will be needed to store these often huge mangled names in every AST context for the 0.0001% of the time it is actually needed.

Well since this is for libstdc++ only, should we make this a platform-specific behavior and, e.g., disable it on Darwin?

[davide]

In D40283#983946, @aprantl wrote:

In D40283#983908, @clayborg wrote:

I am fine with checking this. The only issue on my end is the extra memory that will be needed to store these often huge mangled names in every AST context for the 0.0001% of the time it is actually needed.

Well since this is for libstdc++ only, should we make this a platform-specific behavior and, e.g., disable it on Darwin?

Could be, but is it really worth the complexity? I'd rather defer this if we find it being a performance problem.
Either way is fine to me, FWIW.

[nelhage]

Thanks Greg. I'll wait another day and check this one in. Nelson, does the plan make sense to you?

Yep, sounds great. Thanks so much.

I'd prefer to not gate this on !Darwin, since the added complexity seems unfortunate, and this isn't, strictly-speaking, only about libstdc++ (although in practice that's the ~only place where it's likely to manifest). But I'm happy to write the patch if it turns out to be an issue and the maintainers think that's the way forward.

[aprantl]

As Greg pointed out C++ (and Swift) mangled names can be enormous, so it would definitely have an impact on the memory footprint (unless we are only passing StringRefs around. Are we?)

[clayborg]

In D40283#983997, @aprantl wrote:

As Greg pointed out C++ (and Swift) mangled names can be enormous, so it would definitely have an impact on the memory footprint (unless we are only passing StringRefs around. Are we?)

Most strings seemed to be created using clang identifiers... Not sure what this API uses for the asm name...

[davide]

As there are no strong objections, I'm going to check this in tomorrow PST and see how it goes.

Thanks for your contribution.

include/lldb/Symbol/ClangASTContext.h

#define LLDB_INVALID_DECL_LEVEL UINT32_MAX
 
   static clang::VarDecl *AddVariableToRecordType(const CompilerType &type,
                                                  const char *name,
                                                  const CompilerType &var_type,
                                                  lldb::AccessType access);
 
   clang::CXXMethodDecl *
   AddMethodToCXXRecordType(lldb::opaque_compiler_type_t type, const char *name,
+                           const char *mangled_name,
                            const CompilerType &method_type,
                            lldb::AccessType access, bool is_virtual,
                            bool is_static, bool is_inline, bool is_explicit,
                            bool is_attr_used, bool is_artificial);
 
   // C++ Base Classes
   clang::CXXBaseSpecifier *
   CreateBaseClassSpecifier(lldb::opaque_compiler_type_t type,

packages/Python/lldbsuite/test/expression_command/issue_35310/Makefile

+LEVEL = ../../make
+
+CXX_SOURCES := main.cpp
+
+include $(LEVEL)/Makefile.rules

packages/Python/lldbsuite/test/expression_command/issue_35310/TestExprsBug35310.py

+from __future__ import print_function
+
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+class ExprBug35310(TestBase):
+
+    mydir = TestBase.compute_mydir(__file__)
+
+    def setUp(self):
+        # Call super's setUp().
+        TestBase.setUp(self)
+
+        self.main_source = "main.cpp"
+        self.main_source_spec = lldb.SBFileSpec(self.main_source)
+
+    def test_issue35310(self):
+        """Test invoking functions with non-standard linkage names.
+
+        The GNU abi_tag extension used by libstdc++ is a common source
+        of these, but they could originate from other reasons as well.
+        """
+        self.build()
+
+        (target, process, thread, bkpt) = lldbutil.run_to_source_breakpoint(self, 
+                                          '// Break here', self.main_source_spec)
+        frame = thread.GetFrameAtIndex(0)
+
+        value = frame.EvaluateExpression("a.test_abi_tag()")
+        self.assertTrue(value.IsValid())
+        self.assertTrue(value.GetError().Success())
+        self.assertEqual(value.GetValueAsSigned(0), 1)
+
+        value = frame.EvaluateExpression("a.test_asm_name()")
+        self.assertTrue(value.IsValid())
+        self.assertTrue(value.GetError().Success())
+        self.assertEqual(value.GetValueAsSigned(0), 2)

packages/Python/lldbsuite/test/expression_command/issue_35310/flycheck_main.plist

+<?xml version="1.0" encoding="UTF-8"?>
+<!DOCTYPE plist PUBLIC "-//Apple Computer//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd">
+<plist version="1.0">
+<dict>
+ <key>clang_version</key>
+<string>clang version 4.0.0-1ubuntu1 (tags/RELEASE_400/rc1)</string>
+ <key>files</key>
+ <array>
+ </array>
+ <key>diagnostics</key>
+ <array>
+ </array>
+</dict>
+</plist>
 No newline at end of file

packages/Python/lldbsuite/test/expression_command/issue_35310/main.cpp

+#include <stdio.h>
+
+class A {
+public:
+  int __attribute__((abi_tag("cxx11"))) test_abi_tag() {
+      return 1;
+  }
+  int test_asm_name() asm("A_test_asm") {
+      return 2;
+  }
+};
+
+int main(int argc, char **argv) {
+  A a;
+  // Break here
+  a.test_abi_tag();
+  a.test_asm_name();
+  return 0;
+}

source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp

if (copied_clang_type.IsAggregateType() &&
     const bool is_inline = false;
     const bool is_explicit = false;
     const bool is_attr_used = true;
     const bool is_artificial = false;
 
     CXXMethodDecl *method_decl =
         ClangASTContext::GetASTContext(m_ast_context)
             ->AddMethodToCXXRecordType(
-                copied_clang_type.GetOpaqueQualType(), "$__lldb_expr",
+                copied_clang_type.GetOpaqueQualType(), "$__lldb_expr", NULL,
                 method_type, lldb::eAccessPublic, is_virtual, is_static,
                 is_inline, is_explicit, is_attr_used, is_artificial);
 
     if (log) {
       ASTDumper method_ast_dumper((clang::Decl *)method_decl);
       ASTDumper type_ast_dumper(copied_clang_type);
 
       log->Printf("  CEDM::AddThisType Added function $__lldb_expr "

source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp

if (type_ptr == NULL) {
       if (type_is_new_ptr)
         *type_is_new_ptr = true;
 
       const dw_tag_t tag = die.Tag();
 
       bool is_forward_declaration = false;
       DWARFAttributes attributes;
       const char *type_name_cstr = NULL;
+      const char *mangled_name_cstr = NULL;
       ConstString type_name_const_str;
       Type::ResolveState resolve_state = Type::eResolveStateUnresolved;
       uint64_t byte_size = 0;
       Declaration decl;
 
       Type::EncodingDataType encoding_data_type = Type::eEncodingIsUID;
       CompilerType clang_type;
       DWARFFormValue form_value;
if (type_ptr == NULL) {
                 break;
               case DW_AT_name:
                 type_name_cstr = form_value.AsCString();
                 type_name_const_str.SetCString(type_name_cstr);
                 break;
 
               case DW_AT_linkage_name:
               case DW_AT_MIPS_linkage_name:
-                break; // mangled =
-                       // form_value.AsCString(&dwarf->get_debug_str_data());
-                       // break;
+                mangled_name_cstr = form_value.AsCString();
+                break;
               case DW_AT_type:
                 type_die_form = form_value;
                 break;
               case DW_AT_accessibility:
                 accessibility = DW_ACCESS_to_AccessType(form_value.Unsigned());
                 break;
               case DW_AT_declaration:
                 break; // is_forward_declaration = form_value.Boolean(); break;

                           // in the DWARF for C++ methods... Default to public
                           // for now...
                           if (accessibility == eAccessNone)
                             accessibility = eAccessPublic;
 
                           clang::CXXMethodDecl *cxx_method_decl =
                               m_ast.AddMethodToCXXRecordType(
                                   class_opaque_type.GetOpaqueQualType(),
-                                  type_name_cstr, clang_type, accessibility,
-                                  is_virtual, is_static, is_inline, is_explicit,
-                                  is_attr_used, is_artificial);
+                                  type_name_cstr, mangled_name_cstr, clang_type,
+                                  accessibility, is_virtual, is_static,
+                                  is_inline, is_explicit, is_attr_used,
+                                  is_artificial);
 
                           type_handled = cxx_method_decl != NULL;
 
                           if (type_handled) {
                             LinkDeclContextToDIE(
                                 ClangASTContext::GetAsDeclContext(
                                     cxx_method_decl),
                                 die);

source/Symbol/ClangASTContext.cpp

#ifdef LLDB_CONFIGURATION_DEBUG
       VerifyDecl(var_decl);
 #endif
     }
   }
   return var_decl;
 }
 
 clang::CXXMethodDecl *ClangASTContext::AddMethodToCXXRecordType(
-    lldb::opaque_compiler_type_t type, const char *name,
+    lldb::opaque_compiler_type_t type, const char *name, const char *mangled_name,
     const CompilerType &method_clang_type, lldb::AccessType access,
     bool is_virtual, bool is_static, bool is_inline, bool is_explicit,
     bool is_attr_used, bool is_artificial) {
   if (!type || !method_clang_type.IsValid() || name == nullptr ||
       name[0] == '\0')
     return nullptr;
 
   clang::QualType record_qual_type(GetCanonicalQualType(type));
clang::AccessSpecifier access_specifier =
       ClangASTContext::ConvertAccessTypeToAccessSpecifier(access);
 
   cxx_method_decl->setAccess(access_specifier);
   cxx_method_decl->setVirtualAsWritten(is_virtual);
 
   if (is_attr_used)
     cxx_method_decl->addAttr(clang::UsedAttr::CreateImplicit(*getASTContext()));
 
+  if (mangled_name != NULL) {
+    cxx_method_decl->addAttr(
+        clang::AsmLabelAttr::CreateImplicit(*getASTContext(), mangled_name));
+  }
+
   // Populate the method decl with parameter decls
 
   llvm::SmallVector<clang::ParmVarDecl *, 12> params;
 
   for (unsigned param_index = 0; param_index < num_params; ++param_index) {
     params.push_back(clang::ParmVarDecl::Create(
         *getASTContext(), cxx_method_decl, clang::SourceLocation(),
         clang::SourceLocation(),