[lldb] Remove template parameters from FunctionTemplateDecl names

[shafik]

Summary

Fix to get the AST we generate for function templates closer to what clang generates and expects. We fix which FuntionDecl we are passing to CreateFunctionTemplateSpecializationInfo and we strip template parameters from the name when creating the FunctionDecl and FunctionTemplateDecl.

These two fixes together fix asserts and ambiguous lookup issues for several cases which are added to the already existing small function template test. This fixes issues with overloads, overloads and ADL, variadic function templates and templated operator overloads.

The recent fix D75545 came out of this patch and there will be at least one more patch to deal with and edge case with templated operator< when it is in a namespace. We do C++ name parsing in CPlusPlusNameParser and we don’t deal with cases like A::operator<<A::B> properly.

Currently the fix applies mostly to dsym cases and I will be working on a fix that will apply to all cases but it may be a separate PR.

[aprantl]

The general mechanics look good, I have a few comments about the implementation.

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

823

Nit: llvm::Optional

However, usually just a plain StringRef is enough, since it can be empty().

825

Nit: This would be nicer as the doxygen comment of the function.

831

Should we scan for the complete "operator<" "operator<=>" instead, to be clearer and more future-proof?
Should we assert(Name.count("<")), too?

841

We are scanning the entire string multiple times here and that seems unnecessarily expensive since C++ template function names get looong. I think we could do this as a single for-loop with a state machine instead, without being too difficult to read?

852

This function would probably benefit from a unit test even if that means it can't be static any more. I just can't prove to myself that there isn't an off-by-one error lurking in it somewhere.

1172

Can you add a comment here explaining why the template parameters are being stripped? I don't think that's obvious to the casual reader.

lldb/test/API/lang/cpp/template-function/TestTemplateFunctions.py

63

Are all of these strings exercising the stripping function? Perhaps that's just as good as a unit test then? But the unit test would still be better to document the expectations of the function.

[labath]

It's a pity that the clang's DW_AT_name value is so ambiguous. For example, gcc would output the name in the commit message as operator< <A::B>, which is a lot more parsable (for computers and people). Have you looked at changing clang's output to make it more like gcc's ? I know it would only help new compilers, but maybe for such a special case, this does not matter?

Or, even if we do end up adding some compat parsing code, that would reduce the need for it to be super exact. I'm pretty sure that the current code does not handle all cases correctly. E.g., I believe it will break on something like operator<<<&operator>> > (mangled name _ZlsIXadL_Zrs1AS0_EEEvS0_S0_, godbolt link).

As an alternative, we could try extracting the same information from the mangled name (via llvm::ItaniumPartialDemangler::getFunctionBaseName), at least when DW_AT_linkage_name is present (not all compilers emit that attribute but I am not sure if anyone has tested if lldb actually works without it).

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

841

Would it be sufficient to always strip one < from the first sequence of <'s that you find? You can't have two template < one after the other, so in a valid name, the last < will be the template angle bracket, while everything else must be a part of the operator name.

So, something like return {Name.data(), Name.find_first_not_of('<', Name.find_first_of('<')) -1} (with some error checks and an explicit check for the spaceship operator).

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

1

Do we have a test for the spaceship operator?

[teemperor]

Small nit pick: Use expect_expr( over expect("expr ... if you do a Python test.

[shafik]

Moving to using ItaniumPartialDemangler for now.

[shafik]

In D75761#1912038, @labath wrote:

It's a pity that the clang's DW_AT_name value is so ambiguous. For example, gcc would output the name in the commit message as operator< <A::B>, which is a lot more parsable (for computers and people). Have you looked at changing clang's output to make it more like gcc's ? I know it would only help new compilers, but maybe for such a special case, this does not matter?

Or, even if we do end up adding some compat parsing code, that would reduce the need for it to be super exact. I'm pretty sure that the current code does not handle all cases correctly. E.g., I believe it will break on something like operator<<<&operator>> > (mangled name _ZlsIXadL_Zrs1AS0_EEEvS0_S0_, godbolt link).

As an alternative, we could try extracting the same information from the mangled name (via llvm::ItaniumPartialDemangler::getFunctionBaseName), at least when DW_AT_linkage_name is present (not all compilers emit that attribute but I am not sure if anyone has tested if lldb actually works without it).

I was planning on looking into removing the template parameters from the names altogether for lldb but we would still need to do this on the lldb side to support older compilers. I have to try this and see how much it breaks.

I am going to move to use llvm::ItaniumPartialDemangler::getFunctionBaseName) and I am going to see if I can test subroutines w/o DW_AT_linkage_name and see if it works at all. If it does not work then maybe we don't need to support the case where we don't have it.

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

831

I don't believe looking for the full name buys us anything correctness wise, we would have to be getting malformed names AFAIK but perhaps I am missing some odd case.

I don't think the assert makes sense b/c we may end up sharing this code w/ dsymutil where we don't know before hand if we have template params in the name. Also perhaps in the future we may change clang to not generate DW_AT_names like this but still support older compilers that do.

841

You correct, I was moving toward a state machine before you pointed out ItaniumPartialDemangler

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

1

We currently don't support C++2a but when we do we should add a test.

[labath]

(This looks fine to me.)

In D75761#1913585, @shafik wrote:

I was planning on looking into removing the template parameters from the names altogether for lldb but we would still need to do this on the lldb side to support older compilers. I have to try this and see how much it breaks.

This is a issue that has come up multiple times now. The presence of template parameters makes it very hard to use accelerator tables. I think it would be great if we could get rid of them.

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

1

How is this lack of support "implemented"? Would it make sense to test that we do something reasonable (e.g. ignore it) if we do happen to run into it?

Given that the new version of this patch doesn't treat the spaceship operator specially, I don't think it needs to/should be a part of this patch, but it is something to think about...

[shafik]

Move to using expect_expr in the test.

[teemperor]

I only have some minor comments but otherwise I think this can land. The title could use some updating as "Fix to get the AST we generate for function templates to be closer to what clang generates and expects" seems very abstract. What about "[lldb] Remove template parameters from FunctionTemplateDecl names" or something like that?

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

1182

You can just pass a nullptr instead of &Size if you don't use the value.

lldb/test/API/lang/cpp/template-function/TestTemplateFunctions.py

16–18

This line can just be lldbutil.run_to_source_breakpoint(self, '// break here', lldb.SBFileSpec("main.cpp")), no need to assign self.anything. The SBFileSpec constructor is curious as it seems the constructor without the bool is deprecated since 2010 (?) but we're using it in many other places (as the bool parameter doesn't appear to be documented.....)

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

1

Well the expression evaluator doesn't activate C++20 so we would just not parse the <=> as the space ship operator in an expression (and therefore not call it). We could still get it into our AST in which case we would probably handle it as if C++20 was activated (at least on the Clang side, not sure about the other parts of LLDB).

I don't think this review is blocked by this though. We first need to add proper C++20 support and then we can actually test this. But I'm also not opposed to having a test that tries calling <=> and just makes sure that Clang handles it and then errors out without crashing anything.

15

I wish this function had a more descriptive name. Maybe something like FuncWithOverload and g would be FuncWithoutOverload. Or a comment that the f here is intended to have the same name as A::f or something like that. Same for the other single-letter function names in here (as they all seem to serve a special purpose and are not just generic functions).

42

This isn't related to A::C from what I understand, so could this have another name than C?

73

Can you move the comments what those expressions are testing to TestTemplateFunctions.py? If the expression fails it would be good to have the respective comment next to the expression instead of the copy in the .cpp file.

[teemperor]

FWIW, I don't think the error cases should use expect_expr as it currently doesn't have a reasonable way to handle errors (the error_msg thing is really too strict and didn't land on purpose. That was more of a side effect of all the refactoring that went into the expect_expr patch).

[shafik]

Addressing comments

[teemperor]

LGTM

lldb/test/API/lang/cpp/template-function/TestTemplateFunctions.py

37

ultra nit pick: technically there should be a space after the "," in the expression.

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

6

hae -> have

[teemperor]

(Also my nit pick about the revision title is still stands)

[shafik]

Addressing minor comments

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

 #include "lldb/Symbol/SymbolFile.h"
 #include "lldb/Symbol/TypeList.h"
 #include "lldb/Symbol/TypeMap.h"
 #include "lldb/Target/Language.h"
 #include "lldb/Utility/LLDBAssert.h"
 #include "lldb/Utility/Log.h"
 #include "lldb/Utility/StreamString.h"
 
+#include "llvm/Demangle/Demangle.h"
+
 #include "clang/AST/CXXInheritance.h"
 #include "clang/AST/DeclCXX.h"
 #include "clang/AST/DeclObjC.h"
 #include "clang/AST/DeclTemplate.h"
 
 #include <map>
 #include <memory>
 #include <vector>
dwarf->GetObjectFile()->GetModule()->ReportError(
         "DWARF DIE at 0x%8.8x named \"%s\" was not able to start its "
         "definition.\nPlease file a bug and attach the file at the "
         "start of this error message",
         die.GetOffset(), attrs.name.GetCString());
   }
   return type_sp;
 }
 
 TypeSP DWARFASTParserClang::ParseSubroutine(const DWARFDIE &die,
                            ParsedDWARFTypeAttributes &attrs) {
   Log *log(LogChannelDWARF::GetLogIfAny(DWARF_LOG_TYPE_COMPLETION |
                                         DWARF_LOG_LOOKUPS));
 
   SymbolFileDWARF *dwarf = die.GetDWARF();
   const dw_tag_t tag = die.Tag();
 
   bool is_variadic = false;
   bool is_static = false;
   bool has_template_params = false;
 
   unsigned type_quals = 0;
 
   std::string object_pointer_name;
   if (attrs.object_pointer) {
     const char *object_pointer_name_cstr = attrs.object_pointer.GetName();
     if (object_pointer_name_cstr)
       object_pointer_name = object_pointer_name_cstr;
   }
 
   DEBUG_PRINTF("0x%8.8" PRIx64 ": %s (\"%s\")\n", die.GetID(),
                DW_TAG_value_to_name(tag), type_name_cstr);
 
   CompilerType return_clang_type;
   Type *func_type = NULL;
 
   if (attrs.type.IsValid())
     func_type = dwarf->ResolveTypeUID(attrs.type.Reference(), true);
 
   if (func_type)
     return_clang_type = func_type->GetForwardCompilerType();
   else
     return_clang_type = m_ast.GetBasicType(eBasicTypeVoid);
 
   std::vector<CompilerType> function_param_types;
   std::vector<clang::ParmVarDecl *> function_param_decls;
 
if (!type_handled) {
 
           if (function_decl) {
             LinkDeclContextToDIE(function_decl, die);
           }
         }
       }
 
       if (!function_decl) {
+        const char *name = attrs.name.GetCString();
+
+        // We currently generate function templates with template parameters in
+        // their name. In order to get closer to the AST that clang generates
+        // we want to strip these from the name when creating the AST.
+        if (attrs.mangled_name) {
+          llvm::ItaniumPartialDemangler D;
+          if (!D.partialDemangle(attrs.mangled_name))
+            name = D.getFunctionBaseName(nullptr, nullptr);
+        }
+
         // We just have a function that isn't part of a class
         function_decl = m_ast.CreateFunctionDeclaration(
             ignore_containing_context ? m_ast.GetTranslationUnitDecl()
                                       : containing_decl_ctx,
-            attrs.name.GetCString(), clang_type, attrs.storage,
-            attrs.is_inline);
+            name, clang_type, attrs.storage, attrs.is_inline);
 
         if (has_template_params) {
           TypeSystemClang::TemplateParameterInfos template_param_infos;
           ParseTemplateParameterInfos(die, template_param_infos);
           template_function_decl = m_ast.CreateFunctionDeclaration(
               ignore_containing_context ? m_ast.GetTranslationUnitDecl()
                                         : containing_decl_ctx,
-              attrs.name.GetCString(), clang_type, attrs.storage,
-              attrs.is_inline);
+              name, clang_type, attrs.storage, attrs.is_inline);
+
           clang::FunctionTemplateDecl *func_template_decl =
-              m_ast.CreateFunctionTemplateDecl(
-                  containing_decl_ctx, template_function_decl,
-                  attrs.name.GetCString(), template_param_infos);
+              m_ast.CreateFunctionTemplateDecl(containing_decl_ctx,
+                                               template_function_decl, name,
+                                               template_param_infos);
           m_ast.CreateFunctionTemplateSpecializationInfo(
-              function_decl, func_template_decl, template_param_infos);
+              template_function_decl, func_template_decl, template_param_infos);
         }
 
         lldbassert(function_decl);
 
         if (function_decl) {
           LinkDeclContextToDIE(function_decl, die);
 
           if (!function_param_decls.empty()) {

lldb/test/API/lang/cpp/template-function/TestTemplateFunctions.py

 """
 Test that we can call C++ template fucntions.
 """
 import lldb
 from lldbsuite.test.decorators import *
 from lldbsuite.test.lldbtest import *
 from lldbsuite.test import lldbutil
 
 
 class TemplateFunctionsTestCase(TestBase):
 
     mydir = TestBase.compute_mydir(__file__)
 
     def do_test_template_function(self, add_cast):
         self.build()
-        (_, _, thread, _) = lldbutil.run_to_name_breakpoint(self, "main")
-        frame = thread.GetSelectedFrame()
-        expr = "foo(42)"
+        lldbutil.run_to_source_breakpoint(self, '// break here',
+                lldb.SBFileSpec("main.cpp", False))
+
         if add_cast:
-            expr = "(int)" + expr
-        expr_result = frame.EvaluateExpression(expr)
-        self.assertTrue(expr_result.IsValid())
-        self.assertEqual(expr_result.GetValue(), "42")
+          self.expect_expr("(int) foo(42)", result_type="int", result_value="42")
+        else:
+          self.expect("expr b1 <=> b2",  error=True, substrs=["warning: <user expression 0>:1:4: '<=>' is a single token in C++20; add a space to avoid a change in behavior"])
+
+          self.expect_expr("foo(42)", result_type="int", result_value="42")
+
+          # overload with template case
+          self.expect_expr("h(10)", result_type="int", result_value="10")
+
+          # ADL lookup case
+          self.expect_expr("f(A::C{})", result_type="int", result_value="4")
+
+          # ADL lookup but no overload
+          self.expect_expr("g(A::C{})", result_type="int", result_value="4")
+
+          # variadic function cases
+          self.expect_expr("var(1)", result_type="int", result_value="10")
+          self.expect_expr("var(1, 2)", result_type="int", result_value="10")
+
+          # Overloaded templated operator case
+          self.expect_expr("b1 > b2", result_type="bool", result_value="true")
+          self.expect_expr("b1 >> b2", result_type="bool", result_value="true")
+          self.expect_expr("b1 << b2", result_type="bool", result_value="true")
+          self.expect_expr("b1 == b2", result_type="bool", result_value="true")
+
+          # Overloaded operator case
+          self.expect_expr("d1 > d2", result_type="bool", result_value="true")
+          self.expect_expr("d1 >> d2", result_type="bool", result_value="true")
+          self.expect_expr("d1 << d2", result_type="bool", result_value="true")
+          self.expect_expr("d1 == d2", result_type="bool", result_value="true")
 
     @skipIfWindows
     def test_template_function_with_cast(self):
         self.do_test_template_function(True)
 
     @skipIfWindows
     @expectedFailureAll(debug_info=["dwarf", "gmodules", "dwo"])
     def test_template_function_without_cast(self):
         self.do_test_template_function(False)

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

 template<typename T>
 int foo(T t1) {
         return int(t1);
 }
 
+// Some cases to cover ADL, we have two cases:
+//
+// - f which will have a overload in the global namespace if unqualified lookup
+// find f(int) and f(T) is found via ADL.
+//
+// - g which does not have an overload in the global namespace.
+namespace A {
+struct C {};
+
+template <typename T> int f(T) { return 4; }
+
+template <typename T> int g(T) { return 4; }
+} // namespace A
+
+// Meant to overload A::f(T) which may be found via ADL
+int f(int) { return 1; }
+
+// Regular overloaded functions case h(T) and h(double).
+template <class T> int h(T x) { return x; }
+int h(double d) { return 5; }
+
+template <class... Us> int var(Us... pargs) { return 10; }
+
+// Having the templated overloaded operators in a namespace effects the
+// mangled name generated in the IR e.g. _ZltRK1BS1_ Vs _ZN1AltERKNS_1BES2_
+// One will be in the symbol table but the other won't. This results in a
+// different code path that will result in CPlusPlusNameParser being used.
+// This allows us to cover that code as well.
+namespace A {
+template <typename T> bool operator<(const T &, const T &) { return true; }
+
+template <typename T> bool operator>(const T &, const T &) { return true; }
+
+template <typename T> bool operator<<(const T &, const T &) { return true; }
+
+template <typename T> bool operator>>(const T &, const T &) { return true; }
+
+template <typename T> bool operator==(const T &, const T &) { return true; }
+
+struct B {};
+} // namespace A
+
+struct D {};
+
+// Make sure we cover more straight forward cases as well.
+bool operator<(const D &, const D &) { return true; }
+bool operator>(const D &, const D &) { return true; }
+bool operator>>(const D &, const D &) { return true; }
+bool operator<<(const D &, const D &) { return true; }
+bool operator==(const D &, const D &) { return true; }
+
 int main() {
-        return foo(42);
+  A::B b1;
+  A::B b2;
+  D d1;
+  D d2;
+
+  bool result_b = b1 < b2 && b1 << b2 && b1 == b2 && b1 > b2 && b1 >> b2;
+  bool result_c = d1 < d2 && d1 << d2 && d1 == d2 && d1 > d2 && d1 >> d2;
+
+  return foo(42) + result_b + result_c + f(A::C{}) + g(A::C{}) + h(10) + h(1.) +
+         var(1) + var(1, 2); // break here
 }