[Lldb-commits] [lldb] d20aa7c - [lldb] Report old modules from ModuleList::ReplaceEquivalent

[Joseph Tremoulet via lldb-commits]

Author: Joseph Tremoulet
Date: 2020-10-30T15:14:32-04:00
New Revision: d20aa7ca422145fb4d07e16c1d0aa7de9e3554ea

URL: https://github.com/llvm/llvm-project/commit/d20aa7ca422145fb4d07e16c1d0aa7de9e3554ea
DIFF: https://github.com/llvm/llvm-project/commit/d20aa7ca422145fb4d07e16c1d0aa7de9e3554ea.diff

LOG: [lldb] Report old modules from ModuleList::ReplaceEquivalent

This allows the Target to update its module list when loading a shared
module replaces an equivalent one.

A testcase is added which hits this codepath -- without the fix, the
target reports libbreakpad.so twice in its module list.

Reviewed By: jingham

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

Added: 
    

Modified: 
    lldb/include/lldb/Core/ModuleList.h
    lldb/source/Core/ModuleList.cpp
    lldb/test/API/functionalities/postmortem/minidump-new/TestMiniDumpUUID.py

Removed: 
    


################################################################################
diff  --git a/lldb/include/lldb/Core/ModuleList.h b/lldb/include/lldb/Core/ModuleList.h
index c62021b4bf6b..d90b27e474ac 100644
--- a/lldb/include/lldb/Core/ModuleList.h
+++ b/lldb/include/lldb/Core/ModuleList.h
@@ -139,7 +139,13 @@ class ModuleList {
   ///
   /// \param[in] module_sp
   ///     A shared pointer to a module to replace in this collection.
-  void ReplaceEquivalent(const lldb::ModuleSP &module_sp);
+  ///
+  /// \param[in] old_modules
+  ///     Optional pointer to a vector which, if provided, will have shared
+  ///     pointers to the replaced module(s) appended to it.
+  void ReplaceEquivalent(
+      const lldb::ModuleSP &module_sp,
+      llvm::SmallVectorImpl<lldb::ModuleSP> *old_modules = nullptr);
 
   /// Append a module to the module list, if it is not already there.
   ///

diff  --git a/lldb/source/Core/ModuleList.cpp b/lldb/source/Core/ModuleList.cpp
index a4b3b2b92f83..cf276ba65931 100644
--- a/lldb/source/Core/ModuleList.cpp
+++ b/lldb/source/Core/ModuleList.cpp
@@ -171,7 +171,9 @@ void ModuleList::Append(const ModuleSP &module_sp, bool notify) {
   AppendImpl(module_sp, notify);
 }
 
-void ModuleList::ReplaceEquivalent(const ModuleSP &module_sp) {
+void ModuleList::ReplaceEquivalent(
+    const ModuleSP &module_sp,
+    llvm::SmallVectorImpl<lldb::ModuleSP> *old_modules) {
   if (module_sp) {
     std::lock_guard<std::recursive_mutex> guard(m_modules_mutex);
 
@@ -184,11 +186,14 @@ void ModuleList::ReplaceEquivalent(const ModuleSP &module_sp) {
 
     size_t idx = 0;
     while (idx < m_modules.size()) {
-      ModuleSP module_sp(m_modules[idx]);
-      if (module_sp->MatchesModuleSpec(equivalent_module_spec))
+      ModuleSP test_module_sp(m_modules[idx]);
+      if (test_module_sp->MatchesModuleSpec(equivalent_module_spec)) {
+        if (old_modules)
+          old_modules->push_back(test_module_sp);
         RemoveImpl(m_modules.begin() + idx);
-      else
+      } else {
         ++idx;
+      }
     }
     // Now add the new module to the list
     Append(module_sp);
@@ -820,7 +825,7 @@ ModuleList::GetSharedModule(const ModuleSpec &module_spec, ModuleSP &module_sp,
           *did_create_ptr = true;
         }
 
-        shared_module_list.ReplaceEquivalent(module_sp);
+        shared_module_list.ReplaceEquivalent(module_sp, old_modules);
         return error;
       }
     }
@@ -857,7 +862,7 @@ ModuleList::GetSharedModule(const ModuleSpec &module_spec, ModuleSP &module_sp,
             if (did_create_ptr)
               *did_create_ptr = true;
 
-            shared_module_list.ReplaceEquivalent(module_sp);
+            shared_module_list.ReplaceEquivalent(module_sp, old_modules);
             return Status();
           }
         }
@@ -955,7 +960,7 @@ ModuleList::GetSharedModule(const ModuleSpec &module_spec, ModuleSP &module_sp,
           if (did_create_ptr)
             *did_create_ptr = true;
 
-          shared_module_list.ReplaceEquivalent(module_sp);
+          shared_module_list.ReplaceEquivalent(module_sp, old_modules);
         }
       } else {
         located_binary_modulespec.GetFileSpec().GetPath(path, sizeof(path));

diff  --git a/lldb/test/API/functionalities/postmortem/minidump-new/TestMiniDumpUUID.py b/lldb/test/API/functionalities/postmortem/minidump-new/TestMiniDumpUUID.py
index abd62500e7d6..dc28ec6d7acc 100644
--- a/lldb/test/API/functionalities/postmortem/minidump-new/TestMiniDumpUUID.py
+++ b/lldb/test/API/functionalities/postmortem/minidump-new/TestMiniDumpUUID.py
@@ -32,10 +32,10 @@ def verify_module(self, module, verify_path, verify_uuid):
             os.path.normcase(module.GetFileSpec().dirname or ""))
         self.assertEqual(verify_uuid, module.GetUUIDString())
 
-    def get_minidump_modules(self, yaml_file):
+    def get_minidump_modules(self, yaml_file, exe = None):
         minidump_path = self.getBuildArtifact(os.path.basename(yaml_file) + ".dmp")
         self.yaml2obj(yaml_file, minidump_path)
-        self.target = self.dbg.CreateTarget(None)
+        self.target = self.dbg.CreateTarget(exe)
         self.process = self.target.LoadCore(minidump_path)
         return self.target.modules
 
@@ -266,6 +266,24 @@ def test_breakpad_overflow_hash_match(self):
         # will check that this matches.
         self.verify_module(modules[0], so_path, "48EB9FD7")
 
+    def test_breakpad_hash_match_exe_outside_sysroot(self):
+        """
+            Check that we can match the breakpad .text section hash when the
+            module is specified as the exe during launch, and a syroot is
+            provided, which does not contain the exe.
+        """
+        sysroot_path = os.path.join(self.getBuildDir(), "mock_sysroot")
+        lldbutil.mkdir_p(sysroot_path)
+        so_dir = os.path.join(self.getBuildDir(), "binary")
+        so_path = os.path.join(so_dir, "libbreakpad.so")
+        lldbutil.mkdir_p(so_dir)
+        self.yaml2obj("libbreakpad.yaml", so_path)
+        self.runCmd("platform select remote-linux --sysroot '%s'" % sysroot_path)
+        modules = self.get_minidump_modules("linux-arm-breakpad-uuid-match.yaml", so_path)
+        self.assertEqual(1, len(modules))
+        # LLDB makes up its own UUID as well when there is no build ID so we
+        # will check that this matches.
+        self.verify_module(modules[0], so_path, "D9C480E8")
 
     def test_facebook_hash_match(self):
         """