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).

Differential Revision: https://reviews.llvm.org/D124579

10 files changed, 147 insertions, 12 deletions

diff --git a/lldb/include/lldb/Target/Language.h b/lldb/include/lldb/Target/Language.h
index ce2d273a8277..fa79aaee0574 100644
--- a/lldb/include/lldb/Target/Language.h
+++ b/lldb/include/lldb/Target/Language.h
@@ -217,6 +217,17 @@ public:
                                         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();
diff --git a/lldb/source/Core/Module.cpp b/lldb/source/Core/Module.cpp
index e89d5ec40178..7160c2386efd 100644
--- a/lldb/source/Core/Module.cpp
+++ b/lldb/source/Core/Module.cpp
@@ -739,13 +739,25 @@ void Module::LookupInfo::Prune(SymbolContextList &sc_list,
     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);
     }
   }
 
diff --git a/lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp b/lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp
index 56daf75ee0fa..08b6d89e55f7 100644
--- a/lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp
+++ b/lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp
@@ -268,6 +268,41 @@ std::string CPlusPlusLanguage::MethodName::GetScopeQualifiedName() {
   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
@@ -280,6 +315,12 @@ bool CPlusPlusLanguage::IsCPPMangledName(llvm::StringRef name) {
   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))
diff --git a/lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.h b/lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.h
index 5547864a3763..53a01cfc4799 100644
--- a/lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.h
+++ b/lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.h
@@ -55,6 +55,8 @@ public:
     llvm::StringRef GetArguments();
 
     llvm::StringRef GetQualifiers();
+    
+    bool ContainsPath(llvm::StringRef path);
 
   protected:
     void Parse();
@@ -105,6 +107,9 @@ public:
   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;
diff --git a/lldb/source/Target/Language.cpp b/lldb/source/Target/Language.cpp
index eee1ff1512d9..6df36aeeb7b7 100644
--- a/lldb/source/Target/Language.cpp
+++ b/lldb/source/Target/Language.cpp
@@ -428,6 +428,14 @@ bool Language::GetFormatterPrefixSuffix(ValueObject &valobj,
   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;
 }
diff --git a/lldb/test/API/functionalities/breakpoint/cpp/TestCPPBreakpointLocations.py b/lldb/test/API/functionalities/breakpoint/cpp/TestCPPBreakpointLocations.py
index be21c6eea138..8edd852cd8ec 100644
--- a/lldb/test/API/functionalities/breakpoint/cpp/TestCPPBreakpointLocations.py
+++ b/lldb/test/API/functionalities/breakpoint/cpp/TestCPPBreakpointLocations.py
@@ -28,7 +28,7 @@ class TestCPPBreakpointLocations(TestBase):
         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:
@@ -47,9 +47,9 @@ class TestCPPBreakpointLocations(TestBase):
         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()']},
diff --git a/lldb/test/API/functionalities/breakpoint/cpp/main.cpp b/lldb/test/API/functionalities/breakpoint/cpp/main.cpp
index f352084e7444..088e33c6a7c7 100644
--- a/lldb/test/API/functionalities/breakpoint/cpp/main.cpp
+++ b/lldb/test/API/functionalities/breakpoint/cpp/main.cpp
@@ -24,6 +24,29 @@ namespace a {
     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:
@@ -62,11 +85,15 @@ namespace c {
 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();
diff --git a/lldb/test/API/functionalities/return-value/TestReturnValue.py b/lldb/test/API/functionalities/return-value/TestReturnValue.py
index f2f50385b8bd..517ed46a4fe9 100644
--- a/lldb/test/API/functionalities/return-value/TestReturnValue.py
+++ b/lldb/test/API/functionalities/return-value/TestReturnValue.py
@@ -237,7 +237,7 @@ class ReturnValueTestCase(TestBase):
 
         # 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()
 
diff --git a/lldb/test/API/macosx/indirect_symbol/TestIndirectSymbols.py b/lldb/test/API/macosx/indirect_symbol/TestIndirectSymbols.py
index 5c76d7aabf1b..e2cebf020910 100644
--- a/lldb/test/API/macosx/indirect_symbol/TestIndirectSymbols.py
+++ b/lldb/test/API/macosx/indirect_symbol/TestIndirectSymbols.py
@@ -98,7 +98,7 @@ class TestIndirectFunctions(TestBase):
         # 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:
diff --git a/lldb/unittests/Language/CPlusPlus/CPlusPlusLanguageTest.cpp b/lldb/unittests/Language/CPlusPlus/CPlusPlusLanguageTest.cpp
index 20abd5a87e1b..bf887fb0777c 100644
--- a/lldb/unittests/Language/CPlusPlus/CPlusPlusLanguageTest.cpp
+++ b/lldb/unittests/Language/CPlusPlus/CPlusPlusLanguageTest.cpp
@@ -123,6 +123,37 @@ TEST(CPlusPlusLanguage, MethodNameParsing) {
   }
 }
 
+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;