[lldb][ClangExpression] Add asm() label to all FunctionDecls we create from DWARF

[Michael137]

Summary

When resolving symbols during IR execution, lldb makes a last effort attempt
to resolve external symbols from object files by approximate name matching.
It currently uses CPlusPlusNameParser to parse the demangled function name
and arguments for the unresolved symbol and its candidates. However, this
hand-rolled C++ parser doesn’t support ABI tags which, depending on the demangler,
get demangled into [abi:tag]. This lack of parsing support causes lldb to never
consider a candidate mangled function name that has ABI tags.

The issue reproduces by calling an ABI-tagged template function from the
expression evaluator. This is particularly problematic with the recent
addition of ABI tags to numerous libcxx APIs.

The issue stems from the fact that clang::CodeGen emits function
function calls using the mangled name inferred from the FunctionDecl
LLDB constructs from DWARF. Debug info often lacks information for
us to construct a perfect FunctionDecl resulting in subtle mangled
name inaccuracies.

This patch side-steps the problem of inaccurate FunctionDecls by
attaching an asm() label to each FunctionDecl LLDB creates from DWARF.
clang::CodeGen consults this label to get the mangled name as one of
the first courses of action when emitting a function call.

LLDB already does this for C++ member functions as of
675767a5910d2ec77ef8b51c78fe312cf9022896

Testing

• Added API tests

[Michael137]

This implements @labath's suggestion from https://reviews.llvm.org/D131335

Following tests fail with this:

lang/cpp/extern_c/TestExternCSymbols.py
lang/cpp/namespace/TestNamespaceLookup.py
macosx/rosetta/TestRosetta.py

Investigating currently...

[labath]

In D131974#3726438, @Michael137 wrote:

lang/cpp/extern_c/TestExternCSymbols.py

I think that's because extern "C" symbols don't have a DW_AT_linkage_name attribute. Generally speaking, I don't think we can assume all debug info entries will have one (e.g. -gsce makes clang omit those), and should add them only if available.

[Michael137]

This comment was removed by Michael137.

[Michael137]

In D131974#3726451, @labath wrote:

In D131974#3726438, @Michael137 wrote:

lang/cpp/extern_c/TestExternCSymbols.py

I think that's because extern "C" symbols don't have a DW_AT_linkage_name attribute. Generally speaking, I don't think we can assume all debug info entries will have one (e.g. -gsce makes clang omit those), and should add them only if available.

Yup makes sense. Checking the others. Wonder if it needs a check for attrs.storage == SC_Extern

[Michael137]

• Check mangled name and storage class before attaching label

[Michael137]

• Remove libcxx ABI-tag workaround from API test makefile

[aprantl]

Thanks! This looks like a fine incremental improvement. But we should aim at eventually getting rid of these XFAILs.

[labath]

For me this wins on simplicity grounds alone.

In D131974#3726475, @Michael137 wrote:

• Check mangled name and storage class before attaching label

I don't think that the storage class check is helping anything. Non-external functions can have DW_AT_linkage_names and ABI tags (although their usefulness is questionable) as well. Of course, that means that the local (indicated by the L in the name) mangled name is not necessarily unique (and that's why I said that going off of the address would be more correct). However, this is not something we can fix by letting clang deduce the name for itself.

[labath]

In D131974#3726618, @labath wrote:

For me this wins on simplicity grounds alone.

In D131974#3726475, @Michael137 wrote:

• Check mangled name and storage class before attaching label

I don't think that the storage class check is helping anything. Non-external functions can have DW_AT_linkage_names and ABI tags (although their usefulness is questionable) as well. Of course, that means that the local (indicated by the L in the name) mangled name is not necessarily unique (and that's why I said that going off of the address would be more correct). However, this is not something we can fix by letting clang deduce the name for itself.

Any particular reason for not removing the SC_Extern check? I'm pretty sure I could create a test case with a static abi-tagged function which we wouldn't be able to call with that check in place...

[Michael137]

In D131974#3732727, @labath wrote:

In D131974#3726618, @labath wrote:

For me this wins on simplicity grounds alone.

In D131974#3726475, @Michael137 wrote:

• Check mangled name and storage class before attaching label

I don't think that the storage class check is helping anything. Non-external functions can have DW_AT_linkage_names and ABI tags (although their usefulness is questionable) as well. Of course, that means that the local (indicated by the L in the name) mangled name is not necessarily unique (and that's why I said that going off of the address would be more correct). However, this is not something we can fix by letting clang deduce the name for itself.

Any particular reason for not removing the SC_Extern check? I'm pretty sure I could create a test case with a static abi-tagged function which we wouldn't be able to call with that check in place...

Boiled down to this comment:

the local (indicated by the L in the name) mangled name is not necessarily unique

Attaching it to non-external functions caused cpp/namespace/TestNamespaceLookup.py to fail. With a non-static and a static version of func() LLDB chose the wrong one. I can follow up on whether this is a bug elsewhere and how feasible it would be to unconditionally attach the label

[labath]

In D131974#3733024, @Michael137 wrote:

In D131974#3732727, @labath wrote:

Any particular reason for not removing the SC_Extern check? I'm pretty sure I could create a test case with a static abi-tagged function which we wouldn't be able to call with that check in place...

Boiled down to this comment:

the local (indicated by the L in the name) mangled name is not necessarily unique

Attaching it to non-external functions caused cpp/namespace/TestNamespaceLookup.py to fail. With a non-static and a static version of func() LLDB chose the wrong one. I can follow up on whether this is a bug elsewhere and how feasible it would be to unconditionally attach the label

Ah, this is a fun one. So this breaks the test_scope_lookup_with_run_command test in TestNamespaceLookup.py, but simultaneously "fixes" test_file_scope_lookup_with_run_command. I would say this is a pre-existing bug, that just happens to manifest itself differently with this change. The problem is that lldb is not able to correctly scope CU-local objects, as it ends up putting all files into one huge AST context. It was mostly accidental that lldb picked the external version of the function (because it happened to get its name mangling "right"). Now it happens to pick the static one as they both have the same signature (and I guess this is the one that gets parsed first).

I'd say we should just flip the xfails around, since this is already pretty broken, and the asm labels definitely help in some situations.

[Michael137]

In D131974#3734849, @labath wrote:

In D131974#3733024, @Michael137 wrote:

In D131974#3732727, @labath wrote:

Any particular reason for not removing the SC_Extern check? I'm pretty sure I could create a test case with a static abi-tagged function which we wouldn't be able to call with that check in place...

Boiled down to this comment:

the local (indicated by the L in the name) mangled name is not necessarily unique

Attaching it to non-external functions caused cpp/namespace/TestNamespaceLookup.py to fail. With a non-static and a static version of func() LLDB chose the wrong one. I can follow up on whether this is a bug elsewhere and how feasible it would be to unconditionally attach the label

Ah, this is a fun one. So this breaks the test_scope_lookup_with_run_command test in TestNamespaceLookup.py, but simultaneously "fixes" test_file_scope_lookup_with_run_command. I would say this is a pre-existing bug, that just happens to manifest itself differently with this change. The problem is that lldb is not able to correctly scope CU-local objects, as it ends up putting all files into one huge AST context. It was mostly accidental that lldb picked the external version of the function (because it happened to get its name mangling "right"). Now it happens to pick the static one as they both have the same signature (and I guess this is the one that gets parsed first).

When I initially looked at it, it seemed unintuitive for LLDB to pick the file-static function (from a context where it's usually not accessible) over the one in the header so I kept the check in.
But it's true that the storage-class check just works around a different bug. There seem to always be cases where we pick an incorrect overload no matter how you flip it.
Happy to remove the check and flip the XFAIL

[Michael137]

https://reviews.llvm.org/D132231

lldb/packages/Python/lldbsuite/test/make/Makefile.rules

ifeq (1,$(USE_LIBSTDCPP))
   # Clang requires an extra flag: -stdlib=libstdc++
   ifneq (,$(findstring clang,$(CC)))
     CXXFLAGS += -stdlib=libstdc++ -DLLDB_USING_LIBSTDCPP
     LDFLAGS += -stdlib=libstdc++
   endif
 endif
 
 ifeq (1,$(USE_LIBCPP))
-  # TODO: Teach LLDB to handle ABI tags in libc++ namespaces.
-  CXXFLAGS += -DLLDB_USING_LIBCPP -D_LIBCPP_NO_ABI_TAG
+  CXXFLAGS += -DLLDB_USING_LIBCPP
   ifeq "$(OS)" "Android"
     # Nothing to do, this is already handled in
     # Android.rules.
   else
     CXXFLAGS += -stdlib=libc++
     LDFLAGS += -stdlib=libc++
   endif
   ifneq (,$(filter $(OS), FreeBSD Linux NetBSD))

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

if (!type_handled) {
                   template_function_decl, template_param_infos);
           m_ast.CreateFunctionTemplateSpecializationInfo(
               template_function_decl, func_template_decl, template_param_infos);
         }
 
         lldbassert(function_decl);
 
         if (function_decl) {
+          // Attach an asm(<mangled_name>) label to the FunctionDecl.
+          // This ensures that clang::CodeGen emits function calls
+          // using symbols that are mangled according to the DW_AT_linkage_name.
+          // If we didn't do this, the external symbols wouldn't exactly
+          // match the mangled name LLDB knows about and the IRExecutionUnit
+          // would have to fall back to searching object files for
+          // approximately matching function names. The motivating
+          // example is generating calls to ABI-tagged template functions.
+          // This is done separately for member functions in
+          // AddMethodToCXXRecordType.
+          if (attrs.mangled_name && attrs.storage == clang::SC_Extern)
+            function_decl->addAttr(clang::AsmLabelAttr::CreateImplicit(
+                m_ast.getASTContext(), attrs.mangled_name, /*literal=*/false));
+
           LinkDeclContextToDIE(function_decl, die);
 
           if (!function_param_decls.empty()) {
             m_ast.SetFunctionParameters(function_decl, function_param_decls);
             if (template_function_decl)
               m_ast.SetFunctionParameters(template_function_decl,
                                           function_param_decls);
           }

lldb/test/API/lang/cpp/abi_tag_lookup/Makefile

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

lldb/test/API/lang/cpp/abi_tag_lookup/TestAbiTagLookup.py

+"""
+Test that we can call functions and use types
+annotated (and thus mangled) with ABI tags.
+"""
+
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+class AbiTagLookupTestCase(TestBase):
+
+    @skipIfWindows
+    @expectedFailureAll(debug_info=["dwarf", "gmodules", "dwo"])
+    def test_abi_tag_lookup(self):
+        self.build()
+        lldbutil.run_to_source_breakpoint(self, 'Break here',
+                lldb.SBFileSpec("main.cpp", False))
+
+        # Qualified/unqualified lookup to templates in namespace
+        self.expect_expr("operator<(b1, b2)", result_type="bool", result_value="true")
+        self.expect_expr("A::operator<(b1, b2)", result_type="bool", result_value="true")
+        self.expect_expr("b1 < b2", result_type="bool", result_value="true")
+
+        # Qualified/unqualified lookup to templates with ABI tags in namespace
+        self.expect_expr("operator>(b1, b2)", result_type="bool", result_value="true")
+        self.expect_expr("A::operator>(b1, b2)", result_type="bool", result_value="true")
+        self.expect_expr("b1 > b2", result_type="bool", result_value="true")
+
+        # Call non-operator templates with ABI tags
+        self.expect_expr("A::withAbiTagInNS(1, 1)", result_type="int", result_value="1")
+
+        self.expect_expr("A::withAbiTagInNS(1.0, 1.0)", result_type="int", result_value="2")
+        self.expect_expr("withAbiTagInNS(b1, b2)", result_type="int", result_value="2")
+        self.expect_expr("A::withAbiTagInNS(b1, b2)", result_type="int", result_value="2")
+
+        self.expect_expr("withAbiTag(b1, b2)", result_type="int", result_value="3")
+        self.expect_expr("withAbiTag(0, 0)", result_type="int", result_value="-3")
+
+        # Structures with ABI tags
+        self.expect_expr("t.Value()", result_type="const int", result_value="4")
+        self.expect_expr("tt.Value()", result_type="const int", result_value="5")
+
+        self.expect_expr("Tagged{.mem = 6}", result_type="Tagged",
+                         result_children=[ValueCheck(name="mem", value="6")])
+
+        # Inline namespaces with ABI tags
+        self.expect_expr("v1::withImplicitTag(Simple{.mem = 6})", result_type="int", result_value="6")
+        self.expect_expr("withImplicitTag(Simple{.mem = 6})", result_type="int", result_value="6")
+
+        self.expect_expr("v1::withImplicitTag(Tagged{.mem = 6})", result_type="int", result_value="6")
+        self.expect_expr("withImplicitTag(Tagged{.mem = 6})", result_type="int", result_value="6")

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

+#include <cstdio>
+
+namespace A {
+template <typename T> bool operator<(const T &, const T &) { return true; }
+
+template <typename T>
+[[gnu::abi_tag("Two", "Tags")]] bool operator>(const T &, const T &) {
+  return true;
+}
+
+template <typename T>
+[[gnu::abi_tag("OneTag")]] bool operator==(const T &, const T &) {
+  return true;
+}
+
+[[gnu::abi_tag("Foo")]] int withAbiTagInNS(const int &, const int &) {
+  return 1;
+}
+
+template <typename T>
+[[gnu::abi_tag("Bar")]] int withAbiTagInNS(const T &, const T &) {
+  return 2;
+}
+
+struct B {};
+} // namespace A
+
+template <typename T>
+[[gnu::abi_tag("Baz")]] int withAbiTag(const T &, const T &) {
+  return 3;
+}
+
+[[gnu::abi_tag("Baz")]] int withAbiTag(const int &, const int &) { return -3; }
+
+struct Simple {
+  int mem;
+};
+
+struct [[gnu::abi_tag("Qux")]] Tagged {
+  int mem;
+
+  int const &Value() const { return mem; }
+};
+
+template <typename T> struct [[gnu::abi_tag("Quux", "Quuux")]] TaggedTemplate {
+  T mem;
+
+  T const &Value() const { return mem; }
+};
+
+// clang-format off
+inline namespace [[gnu::abi_tag("Inline", "NS")]] v1 {
+template <typename T> int withImplicitTag(T const &t) { return t.mem; }
+} // namespace
+// clang-format on
+
+int main() {
+  A::B b1;
+  A::B b2;
+  Tagged t{.mem = 4};
+  TaggedTemplate<int> tt{.mem = 5};
+
+  int result = (b1 < b2) + (b1 > b2) + (b1 == b2) + withAbiTag(b1, b2) +
+               A::withAbiTagInNS(1.0, 2.0) + withAbiTagInNS(b1, b2) +
+               A::withAbiTagInNS(1, 2) + withImplicitTag(Tagged{.mem = 6}) +
+               withImplicitTag(Simple{.mem = 6}) + t.Value() + tt.Value();
+
+  std::puts("Break here");
+
+  return result;
+}