[lldb][Breakpoint] Fix setting breakpoints on templates by basename

[Michael137]

Summary

This patch fixes a regression with setting breakpoints on template
functions by name. E.g.,:

$ cat main.cpp
template<typename T>
struct Foo {
  template<typename U>
  void func() {}
};

int main() {
  Foo<int> f;
  f.func<double>();
}

(lldb) br se -n func

This has regressed since 3339000e0bda696c2e29173d15958c0a4978a143
where we started using the CPlusPlusNameParser for getting the
basename of the function symbol and match it exactly against
the name in the breakpoint command. The parser will include template
parameters in the basename, so the exact match will always fail

Testing

• Added API tests
• Added unit-tests

[Michael137]

lldb/test/API/functionalities/breakpoint/cpp/TestCPPBreakpointLocations.py

63

These didn't work before this patch (or in lldb-14) either. So may xfail them for now

[Michael137]

• Fix comment

[Michael137]

lldb/test/API/functionalities/breakpoint/cpp/TestCPPBreakpointLocations.py

63

This turns out to be a discrepancy between how we parse basenames for templates and the DW_AT_name that gets generated for these nested templates. In DWARF, the name contains a space between angle brackets (i.e., func<ns::Foo<int> >). So setting a breakpoint without the space fails to find the function name in the DWARF index. However, adding the space still doesn't work because it trips over something around the parser, haven't checked what exactly yet. Will try address this in a separate patch

[Michael137]

lldb/test/API/functionalities/breakpoint/cpp/TestCPPBreakpointLocations.py

63

Ah, the demangler gives us back a string without the space between angle brackets. So we don't match.

[labath]

lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp

280

I think this should return basename here. Otherwise, all non-templated names will match the empty string.

lldb/unittests/Language/CPlusPlus/CPlusPlusLanguageTest.cpp

170

Is this actually expected? Like, I don't think it's completely wrong, but I definitely did not expect it to do that.

[Michael137]

lldb/unittests/Language/CPlusPlus/CPlusPlusLanguageTest.cpp

170

I agree, it's not intuitive. This happens because operator is a reserved keyword in the eyes of the CPlusPlusNameParser. It will just consume the entire token and return an empty string.

This won't work for the breakpoint matching logic. I thought we'd just test it here in case the someone ever decides to change this behaviour

[Michael137]

lldb/unittests/Language/CPlusPlus/CPlusPlusLanguageTest.cpp

170

This won't work for the breakpoint matching logic.

I.e., we don't allow matching breakpoints on just operator. IIRC something else in the breakpoint resolver accounts for this. Seems fragile though

[Michael137]

lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp

280

Very true

[Michael137]

• Add more test cases
• Remove FIXMEs
• Add docs
• Don't return empty string for non-templates

lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.h

public:
     llvm::StringRef GetContext();
 
     llvm::StringRef GetArguments();
 
     llvm::StringRef GetQualifiers();
     
     bool ContainsPath(llvm::StringRef path);
 
+  private:
+    /// Returns the Basename of this method without a template parameter
+    /// list, if any.
+    ///
+    // Examples:
+    //
+    //   +--------------------------------+---------+
+    //   | MethodName                     | Returns |
+    //   +--------------------------------+---------+
+    //   | void func()                    | func    |
+    //   | void func<int>()               | func    |
+    //   | void func<std::vector<int>>()  | func    |
+    //   +--------------------------------+---------+
+    llvm::StringRef GetBasenameNoTemplateParameters();
+
   protected:
     void Parse();
     bool TrySimplifiedParse();
 
     ConstString m_full; // Full name:
                         // "lldb::SBTarget::GetBreakpointAtIndex(unsigned int)
                         // const"
     llvm::StringRef m_basename;   // Basename:     "GetBreakpointAtIndex"

lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp

std::string CPlusPlusLanguage::MethodName::GetScopeQualifiedName() {
 
   std::string res;
   res += m_context;
   res += "::";
   res += m_basename;
   return res;
 }
 
+llvm::StringRef
+CPlusPlusLanguage::MethodName::GetBasenameNoTemplateParameters() {
+  llvm::StringRef basename = GetBasename();
+  size_t arg_start, arg_end;
+  llvm::StringRef parens("<>", 2);
+  if (ReverseFindMatchingChars(basename, parens, arg_start, arg_end))
+    return basename.substr(0, arg_start);
+
+  return basename;
+}
+
 bool CPlusPlusLanguage::MethodName::ContainsPath(llvm::StringRef path) {
   if (!m_parsed)
     Parse();
+
   // If we can't parse the incoming name, then just check that it contains path.
   if (m_parse_error)
     return m_full.GetStringRef().contains(path);
     
   llvm::StringRef identifier;
   llvm::StringRef context;
   std::string path_str = path.str();
   bool success 
       = CPlusPlusLanguage::ExtractContextAndIdentifier(path_str.c_str(),
                                                        context,
                                                        identifier);
   if (!success)
     return m_full.GetStringRef().contains(path);
 
-  if (identifier != GetBasename())
+  // Basename may include template arguments.
+  // E.g.,
+  // GetBaseName(): func<int>
+  // identifier   : func
+  //
+  // ...but we still want to account for identifiers with template parameter
+  // lists, e.g., when users set breakpoints on template specializations.
+  //
+  // E.g.,
+  // GetBaseName(): func<uint32_t>
+  // identifier   : func<int32_t*>
+  //
+  // Try to match the basename with or without template parameters.
+  if (GetBasename() != identifier &&
+      GetBasenameNoTemplateParameters() != identifier)
     return false;
+
   // Incoming path only had an identifier, so we match.
   if (context.empty())
     return true;
   // Incoming path has context but this method does not, no match.
   if (m_context.empty())
     return false;
 
   llvm::StringRef haystack = m_context;

lldb/test/API/functionalities/breakpoint/cpp/Makefile

 CXX_SOURCES := main.cpp
+CXXFLAGS_EXTRAS := -std=c++14
 
 ifneq (,$(findstring icc,$(CC)))
     CXXFLAGS_EXTRAS := -debug inline-debug-info
 endif
 
 
 include Makefile.rules

lldb/test/API/functionalities/breakpoint/cpp/TestCPPBreakpointLocations.py

def breakpoint_id_tests(self):
             {'name': 'c::func2', 'loc_names': ['a::c::func2()']},
             {'name': 'c::func3', 'loc_names': ['a::c::func3()', 'b::c::func3()']},
             {'name': 'a::c::func1', 'loc_names': ['a::c::func1()']},
             {'name': 'b::c::func1', 'loc_names': ['b::c::func1()']},
             {'name': 'c::d::func2', 'loc_names': ['c::d::func2()']},
             {'name': 'a::c::func1()', 'loc_names': ['a::c::func1()']},
             {'name': 'b::c::func1()', 'loc_names': ['b::c::func1()']},
             {'name': 'c::d::func2()', 'loc_names': ['c::d::func2()']},
+
+            # Template cases
+            {'name': 'func<float>', 'loc_names': []},
+            {'name': 'func<int>', 'loc_names': ['auto ns::Foo<double>::func<int>()']},
+            {'name': 'func', 'loc_names': ['auto ns::Foo<double>::func<int>()',
+                                           'auto ns::Foo<double>::func<ns::Foo<int>>()']},
+
+            {'name': 'operator', 'loc_names': []},
+            {'name': 'ns::Foo<double>::operator bool', 'loc_names': ['ns::Foo<double>::operator bool()']},
+
+            {'name': 'operator a::c', 'loc_names': ['ns::Foo<double>::operator a::c<a::c>()']},
+            {'name': 'operator ns::Foo<int>', 'loc_names': ['ns::Foo<double>::operator ns::Foo<int><ns::Foo<int>>()']},
+
+            {'name': 'operator<<<a::c>', 'loc_names': []},
+            {'name': 'operator<<<int>', 'loc_names': ['void ns::Foo<double>::operator<<<int>(int)']},
+            {'name': 'ns::Foo<double>::operator<<', 'loc_names': ['void ns::Foo<double>::operator<<<int>(int)',
+                                                                  'void ns::Foo<double>::operator<<<ns::Foo<int>>(ns::Foo<int>)']},
         ]
 
         for bp_dict in bp_dicts:
             self.verify_breakpoint_locations(target, bp_dict)
 
     @expectedFailureAll(oslist=["windows"], bugnumber="llvm.org/pr24764")
     def test_destructors(self):
         self.build()

lldb/test/API/functionalities/breakpoint/cpp/main.cpp

public:
         }
         void func3() 
         {
             puts (__PRETTY_FUNCTION__);
         }
     };
 }
 
+namespace ns {
+template <typename Type> struct Foo {
+  template <typename T> void import() {}
+
+  template <typename T> auto func() {}
+
+  operator bool() { return true; }
+
+  template <typename T> operator T() { return {}; }
+
+  template <typename T> void operator<<(T t) {}
+};
+} // namespace ns
+
 int main (int argc, char const *argv[])
 {
     a::c ac;
     aa::cc aac;
     b::c bc;
     c::d cd;
     ac.func1();
     ac.func2();
     ac.func3();
     aac.func1();
     aac.func2();
     aac.func3();
     bc.func1();
     bc.func3();
     cd.func2();
     cd.func3();
+
+    ns::Foo<double> f;
+    f.import <int>();
+    f.func<int>();
+    f.func<ns::Foo<int>>();
+    f.operator bool();
+    f.operator a::c();
+    f.operator ns::Foo<int>();
+    f.operator<<(5);
+    f.operator<< <ns::Foo<int>>({});
+
     return 0;
 }

lldb/unittests/Language/CPlusPlus/CPlusPlusLanguageTest.cpp

 TEST(CPlusPlusLanguage, ContainsPath) {
   CPlusPlusLanguage::MethodName 
       reference_1(ConstString("int foo::bar::func01(int a, double b)"));
   CPlusPlusLanguage::MethodName
       reference_2(ConstString("int foofoo::bar::func01(std::string a, int b)"));
   CPlusPlusLanguage::MethodName reference_3(ConstString("int func01()"));
   CPlusPlusLanguage::MethodName 
       reference_4(ConstString("bar::baz::operator bool()"));
-  
+  CPlusPlusLanguage::MethodName reference_5(
+      ConstString("bar::baz::operator bool<int, Type<double>>()"));
+  CPlusPlusLanguage::MethodName reference_6(ConstString(
+      "bar::baz::operator<<<Type<double>, Type<std::vector<double>>>()"));
+  EXPECT_TRUE(reference_1.ContainsPath(""));
   EXPECT_TRUE(reference_1.ContainsPath("func01"));
   EXPECT_TRUE(reference_1.ContainsPath("bar::func01"));
   EXPECT_TRUE(reference_1.ContainsPath("foo::bar::func01"));
   EXPECT_FALSE(reference_1.ContainsPath("func"));
   EXPECT_FALSE(reference_1.ContainsPath("baz::func01"));
   EXPECT_FALSE(reference_1.ContainsPath("::bar::func01"));
   EXPECT_FALSE(reference_1.ContainsPath("::foo::baz::func01"));
   EXPECT_FALSE(reference_1.ContainsPath("foo::bar::baz::func01"));
   
+  EXPECT_TRUE(reference_2.ContainsPath(""));
   EXPECT_TRUE(reference_2.ContainsPath("foofoo::bar::func01"));
   EXPECT_FALSE(reference_2.ContainsPath("foo::bar::func01"));
   
+  EXPECT_TRUE(reference_3.ContainsPath(""));
   EXPECT_TRUE(reference_3.ContainsPath("func01"));
   EXPECT_FALSE(reference_3.ContainsPath("func"));
   EXPECT_FALSE(reference_3.ContainsPath("bar::func01"));
 
+  EXPECT_TRUE(reference_4.ContainsPath(""));
+  EXPECT_TRUE(reference_4.ContainsPath("operator"));
   EXPECT_TRUE(reference_4.ContainsPath("operator bool"));
   EXPECT_TRUE(reference_4.ContainsPath("baz::operator bool"));
   EXPECT_TRUE(reference_4.ContainsPath("bar::baz::operator bool"));
   EXPECT_FALSE(reference_4.ContainsPath("az::operator bool"));
+
+  EXPECT_TRUE(reference_5.ContainsPath(""));
+  EXPECT_TRUE(reference_5.ContainsPath("operator"));
+  EXPECT_TRUE(reference_5.ContainsPath("operator bool"));
+  EXPECT_TRUE(reference_5.ContainsPath("operator bool<int, Type<double>>"));
+  EXPECT_FALSE(reference_5.ContainsPath("operator bool<int, double>"));
+  EXPECT_FALSE(reference_5.ContainsPath("operator bool<int, Type<int>>"));
+
+  EXPECT_TRUE(reference_6.ContainsPath(""));
+  EXPECT_TRUE(reference_6.ContainsPath("operator"));
+  EXPECT_TRUE(reference_6.ContainsPath("operator<<"));
+  EXPECT_TRUE(reference_6.ContainsPath(
+      "bar::baz::operator<<<Type<double>, Type<std::vector<double>>>()"));
+  EXPECT_FALSE(reference_6.ContainsPath("operator<<<Type<double>>"));
 }
 
 TEST(CPlusPlusLanguage, ExtractContextAndIdentifier) {
   struct TestCase {
     std::string input;
     std::string context, basename;
   };