Make partial function name matching match on context boundaries

[jingham]

Summary

We don't require users to type out the full context of a function, for symbol name matches. Instead, we extract the incoming path's base name, look up all the symbols with that base name, and then compare the rest of the context that the user provided to make sure it matches. However, we do this comparison using just a strstr. So for instance:

break set -n foo::bar

will match not only "a::foo::bar" but "notherfoo::bar". The former is pretty clearly the user's intent, but I don't think the latter is, and results in breakpoints picking up too many matches.

This change adds a Language::DemangledNameContainsPath API which can do a language aware match against the path provided. If the language doesn't provide this we fall back to the strstr (though that's changed to StringRef::contains in the patch).

[shafik]

You might want to try fun cases like operator< and operator()() from a lambda. They should work but might be worth throwing them in.

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

324

dead code?

lldb/tools/CMakeLists.txt

11 ↗

(On Diff #425658)

Did you mean to delete this line?

[labath]

Yeah, this was very surprising.

The fact that we're still doing substring matches on the context is suspicious, and might cause false positives, but given that we first verify that the context is valid, I was not able to come up with counterexamples.

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

293–309

StringRef haystack = m_context;
if (!haystack.consume_back(context))
  return false;
if (haystack.empty() || !isalnum(haystack.back()))
  return true;
return false;

lldb/unittests/Language/CPlusPlus/CPlusPlusLanguageTest.cpp

151–155

what about these?

157

This saves very little compared to spelling out the individual checks (EXPECT_TRUE/FALSE(reference_1.ContainsPath("whatever"))), and makes the error messages a lot more cryptic.

[clayborg]

lldb/include/lldb/Target/Language.h

225

The comment should be adding something extra to "A" right? See suggested code change

lldb/source/Core/Module.cpp

753–754

Remove this, SymbolContext::GetFunctionName() already returns the symbol name if there is no block with inlined function name or concrete function name.

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

324–353

delete commented out code

[jingham]

Address review comments.

[jingham]

lldb/include/lldb/Target/Language.h

225

Yes, the comment was wrong. I had a fancier example that was taking too many characters and I didn't delete enough when simplifying. But I'm not sure what code change you are referring to? I don't see how this is related to GetFunctionName.

lldb/source/Core/Module.cpp

753–754

Yeah, I swear I saw a case debugging through this where GetFunctionName was returning a null string but going to the Symbol worked, but reading through GetFunctionName I don't see how that could be and I can't reproduce the problem, so I must have been seeing some unrelated error.

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

293–309

Much nicer, thanks!

324

Yup, that was the not very good (but much easier to implement) first try. It's gone...

[clayborg]

lldb/include/lldb/Target/Language.h

225

It isn't related to GetFunctionName, I have a code change that you can see on the web page that suggests the edit to make to fix the comment. I see you fixed the comment, so nothing more needed for this.

lldb/include/lldb/Target/Language.h

public:
   // language boundary it makes sense for individual languages to want to
   // customize the printing of values of that type by appending proper
   // prefix/suffix information in language-specific ways
   virtual bool GetFormatterPrefixSuffix(ValueObject &valobj,
                                         ConstString type_hint,
                                         std::string &prefix,
                                         std::string &suffix);
 
+  // When looking up functions, we take a user provided string which may be a
+  // partial match to the full demangled name and compare it to the actual
+  // demangled name to see if it matches as much as the user specified.  An
+  // example of this is if the user provided A::my_function, but the
+  // symbol was really B::A::my_function.  We want that to be
+  // a match.  But we wouldn't want this to match AnotherA::my_function.  The
+  // user is specifying a truncated path, not a truncated set of characters.
+  // This function does a language-aware comparison for those purposes.
+  virtual bool DemangledNameContainsPath(llvm::StringRef path, 
+                                         ConstString demangled) const;
+
   // if a language has a custom format for printing variable declarations that
   // it wants LLDB to honor it should return an appropriate closure here
   virtual DumpValueObjectOptions::DeclPrintingHelper GetDeclPrintingHelper();
 
   virtual LazyBool IsLogicalTrue(ValueObject &valobj, Status &error);
 
   // for a ValueObject of some "reference type", if the value points to the
   // nil/null object, this method returns true

lldb/source/Core/Module.cpp

 void Module::LookupInfo::Prune(SymbolContextList &sc_list,
                                size_t start_idx) const {
   if (m_match_name_after_lookup && m_name) {
     SymbolContext sc;
     size_t i = start_idx;
     while (i < sc_list.GetSize()) {
       if (!sc_list.GetContextAtIndex(i, sc))
         break;
-      ConstString full_name(sc.GetFunctionName());
-      if (full_name &&
-          ::strstr(full_name.GetCString(), m_name.GetCString()) == nullptr) {
-        sc_list.RemoveContextAtIndex(i);
-      } else {
-        ++i;
+      
+      llvm::StringRef user_name = m_name.GetStringRef();
+      bool keep_it = true;
+      Language *language = Language::FindPlugin(sc.GetLanguage());
+      // If the symbol has a language, then let the language make the match.
+      // Otherwise just check that the demangled name contains the user name.
+      if (language)
+        keep_it = language->DemangledNameContainsPath(m_name.GetStringRef(),
+                sc.GetFunctionName());
+      else {
+        llvm::StringRef full_name = sc.GetFunctionName().GetStringRef();
+        // We always keep unnamed symbols:
+        if (!full_name.empty())
+          keep_it = full_name.contains(user_name);
       }
+      if (keep_it)
+        ++i;
+      else
+        sc_list.RemoveContextAtIndex(i);
     }
   }
 
   // If we have only full name matches we might have tried to set breakpoint on
   // "func" and specified eFunctionNameTypeFull, but we might have found
   // "a::func()", "a::b::func()", "c::func()", "func()" and "func". Only
   // "func()" and "func" should end up matching.
   if (m_name_type_mask == eFunctionNameTypeFull) {

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

public:
 
     llvm::StringRef GetBasename();
 
     llvm::StringRef GetContext();
 
     llvm::StringRef GetArguments();
 
     llvm::StringRef GetQualifiers();
     
+    bool ContainsPath(llvm::StringRef path);
+
   protected:
     void Parse();
     bool TrySimplifiedParse();
 
     ConstString m_full; // Full name:
                         // "lldb::SBTarget::GetBreakpointAtIndex(unsigned int)
                         // const"
     llvm::StringRef m_basename;   // Basename:     "GetBreakpointAtIndex"
public:
 
   static void Terminate();
 
   static lldb_private::Language *CreateInstance(lldb::LanguageType language);
 
   static llvm::StringRef GetPluginNameStatic() { return "cplusplus"; }
 
   bool SymbolNameFitsToLanguage(Mangled mangled) const override;
   
+  bool DemangledNameContainsPath(llvm::StringRef path, 
+                                 ConstString demangled) const override;
+
   ConstString
   GetDemangledFunctionNameWithoutArguments(Mangled mangled) const override;
 
   static bool IsCPPMangledName(llvm::StringRef name);
 
   // Extract C++ context and identifier from a string using heuristic matching
   // (as opposed to
   // CPlusPlusLanguage::MethodName which has to have a fully qualified C++ name

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

std::string CPlusPlusLanguage::MethodName::GetScopeQualifiedName() {
 
   std::string res;
   res += m_context;
   res += "::";
   res += m_basename;
   return res;
 }
 
+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())
+    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;
+  if (!haystack.consume_back(context))
+    return false;
+  if (haystack.empty() || !isalnum(haystack.back()))
+    return true;
+    
+  return false;
+}
+
 bool CPlusPlusLanguage::IsCPPMangledName(llvm::StringRef name) {
   // FIXME!! we should really run through all the known C++ Language plugins
   // and ask each one if this is a C++ mangled name
 
   Mangled::ManglingScheme scheme = Mangled::GetManglingScheme(name);
 
   if (scheme == Mangled::eManglingSchemeNone)
     return false;
 
   return true;
 }
 
+bool CPlusPlusLanguage::DemangledNameContainsPath(llvm::StringRef path, 
+                                                  ConstString demangled) const {
+  MethodName demangled_name(demangled);
+  return demangled_name.ContainsPath(path);
+}
+
 bool CPlusPlusLanguage::ExtractContextAndIdentifier(
     const char *name, llvm::StringRef &context, llvm::StringRef &identifier) {
   if (MSVCUndecoratedNameParser::IsMSVCUndecoratedName(name))
     return MSVCUndecoratedNameParser::ExtractContextAndIdentifier(name, context,
                                                                   identifier);
 
   CPlusPlusNameParser parser(name);
   if (auto full_name = parser.ParseAsFullName()) {
     identifier = full_name.getValue().basename;
     context = full_name.getValue().context;
     return true;
   }
   return false;
 }
 
 namespace {
 class NodeAllocator {
   llvm::BumpPtrAllocator Alloc;
 
 public:
   void reset() { Alloc.Reset(); }
 
   template <typename T, typename... Args> T *makeNode(Args &&... args) {
     return new (Alloc.Allocate(sizeof(T), alignof(T)))
         T(std::forward<Args>(args)...);
   }
 
   void *allocateNodeArray(size_t sz) {
     return Alloc.Allocate(sizeof(llvm::itanium_demangle::Node *) * sz,
                           alignof(llvm::itanium_demangle::Node *));
   }
 };
 
 template <typename Derived>
 class ManglingSubstitutor
     : public llvm::itanium_demangle::AbstractManglingParser<Derived,
                                                             NodeAllocator> {
   using Base =

lldb/source/Target/Language.cpp

 
 bool Language::GetFormatterPrefixSuffix(ValueObject &valobj,
                                         ConstString type_hint,
                                         std::string &prefix,
                                         std::string &suffix) {
   return false;
 }
 
+bool Language::DemangledNameContainsPath(llvm::StringRef path, 
+                                         ConstString demangled) const {
+  // The base implementation does a simple contains comparision:
+  if (path.empty())
+    return false;
+  return demangled.GetStringRef().contains(path);                                         
+}
+
 DumpValueObjectOptions::DeclPrintingHelper Language::GetDeclPrintingHelper() {
   return nullptr;
 }
 
 LazyBool Language::IsLogicalTrue(ValueObject &valobj, Status &error) {
   return eLazyBoolCalculate;
 }
 

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

class TestCPPBreakpointLocations(TestBase):
     def verify_breakpoint_locations(self, target, bp_dict):
 
         name = bp_dict['name']
         names = bp_dict['loc_names']
         bp = target.BreakpointCreateByName(name)
         self.assertEquals(
             bp.GetNumLocations(),
             len(names),
-            "Make sure we find the right number of breakpoint locations")
+            "Make sure we find the right number of breakpoint locations for {}".format(name))
 
         bp_loc_names = list()
         for bp_loc in bp:
             bp_loc_names.append(bp_loc.GetAddress().GetFunction().GetName())
 
         for name in names:
             found = name in bp_loc_names
             if not found:
                 print("Didn't find '%s' in: %s" % (name, bp_loc_names))
             self.assertTrue(found, "Make sure we find all required locations")
 
     def breakpoint_id_tests(self):
 
         # Create a target by the debugger.
         exe = self.getBuildArtifact("a.out")
         target = self.dbg.CreateTarget(exe)
         self.assertTrue(target, VALID_TARGET)
         bp_dicts = [
-            {'name': 'func1', 'loc_names': ['a::c::func1()', 'b::c::func1()']},
-            {'name': 'func2', 'loc_names': ['a::c::func2()', 'c::d::func2()']},
-            {'name': 'func3', 'loc_names': ['a::c::func3()', 'b::c::func3()', 'c::d::func3()']},
+            {'name': 'func1', 'loc_names': ['a::c::func1()', 'aa::cc::func1()', 'b::c::func1()']},
+            {'name': 'func2', 'loc_names': ['a::c::func2()', 'aa::cc::func2()', 'c::d::func2()']},
+            {'name': 'func3', 'loc_names': ['a::c::func3()', 'aa::cc::func3()', 'b::c::func3()', 'c::d::func3()']},
             {'name': 'c::func1', 'loc_names': ['a::c::func1()', 'b::c::func1()']},
             {'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()']},

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

public:
             puts (__PRETTY_FUNCTION__);
         }
     };
 
     c::c() {}
     c::~c() {}
 }
 
+namespace aa {
+    class cc {
+    public:
+        cc();
+        ~cc();
+        void func1() 
+        {
+            puts (__PRETTY_FUNCTION__);
+        }
+        void func2() 
+        {
+            puts (__PRETTY_FUNCTION__);
+        }
+        void func3() 
+        {
+            puts (__PRETTY_FUNCTION__);
+        }
+    };
+
+    cc::cc() {}
+    cc::~cc() {}
+}
+
 namespace b {
     class c {
     public:
         c();
         ~c();
         void func1() 
         {
             puts (__PRETTY_FUNCTION__);
public:
             puts (__PRETTY_FUNCTION__);
         }
     };
 }
 
 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();
     return 0;
 }

lldb/test/API/functionalities/return-value/TestReturnValue.py

def test_for_cpp_support(self):
         self.return_and_test_struct_value("return_derived_class")
 
     @skipIf(compiler="clang", compiler_version=['<', '7.0'])
     def return_and_test_struct_value(self, func_name):
         """Pass in the name of the function to return from - takes in value, returns value."""
 
         # Set the breakpoint, run to it, finish out.
         bkpt = self.target.BreakpointCreateByName(func_name)
-        self.assertTrue(bkpt.GetNumResolvedLocations() > 0)
+        self.assertTrue(bkpt.GetNumResolvedLocations() > 0, "Got wrong number of locations for {0}".format(func_name))
 
         self.process.Continue()
 
         thread_list = lldbutil.get_threads_stopped_at_breakpoint(
             self.process, bkpt)
 
         self.assertEquals(len(thread_list), 1)
         thread = thread_list[0]

lldb/test/API/macosx/indirect_symbol/TestIndirectSymbols.py

def test_with_python_api(self):
         self.assertEqual(
             curr_function, "call_through_indirect_hidden",
             "Stepped into indirect symbols.")
 
         # And the last bit is to set a breakpoint on the re-exported symbol and
         # make sure we are again in out target function.
         break_reexported = target.BreakpointCreateByName(
             "reexport_to_indirect")
-        self.assertTrue(break_reexported, VALID_BREAKPOINT)
+        self.assertEqual(break_reexported.GetNumLocations(), 1, VALID_BREAKPOINT)
 
         # Now continue should take us to the second call through the indirect
         # symbol:
 
         threads = lldbutil.continue_to_breakpoint(process, break_reexported)
         self.assertEqual(
             len(threads), 1,
             "Stopped at breakpoint in reexported function target.")
         curr_function = thread.GetFrameAtIndex(0).GetFunctionName()
         self.assertEqual(
             curr_function, "call_through_indirect_hidden",
             "Stepped into indirect symbols.")

lldb/unittests/Language/CPlusPlus/CPlusPlusLanguageTest.cpp

if (method.IsValid()) {
       EXPECT_EQ(test.basename, method.GetBasename().str());
       EXPECT_EQ(test.arguments, method.GetArguments().str());
       EXPECT_EQ(test.qualifiers, method.GetQualifiers().str());
       EXPECT_EQ(test.scope_qualified_name, method.GetScopeQualifiedName());
     }
   }
 }
 
+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()"));
+  
+  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("foofoo::bar::func01"));
+  EXPECT_FALSE(reference_2.ContainsPath("foo::bar::func01"));
+  
+  EXPECT_TRUE(reference_3.ContainsPath("func01"));
+  EXPECT_FALSE(reference_3.ContainsPath("func"));
+  EXPECT_FALSE(reference_3.ContainsPath("bar::func01"));
+
+  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"));
+}
+
 TEST(CPlusPlusLanguage, ExtractContextAndIdentifier) {
   struct TestCase {
     std::string input;
     std::string context, basename;
   };
 
   TestCase test_cases[] = {
       {"main", "", "main"},
       {"main     ", "", "main"},