[lldb] Report old modules from ModuleList::ReplaceEquivalent

[JosephTremoulet]

Summary

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.

[jingham]

The change makes sense.

If I run lldb on a binary, hit a breakpoint, run to it, then change a file, rebuild the executable outside lldb and rerun, I only see one instance of the executable. Why does that path not need your fix?

lldb/include/lldb/Core/ModuleList.h

142–148

You added another parameter, so you should add it to the docs.

[JosephTremoulet]

• Comment new parameter

[JosephTremoulet]

lldb/source/Core/ModuleList.cpp

779–781

If I run lldb on a binary, hit a breakpoint, run to it, then change a file, rebuild the executable outside lldb and rerun, I only see one instance of the executable. Why does that path not need your fix?

The codepath where I'm inserting this comment (ModuleList.cpp lines 779-781/784-786) is the one that handles that when I run those steps under the debugger.

[JosephTremoulet]

lldb/source/Core/ModuleList.cpp

779–781

(see also D89156 which this change depends on)

[JosephTremoulet]

I'd been hoping to get this bugfix into 11.0.1, but it (and D89156, which is just some cleanup that occurred to me while working on this code) have been stalled for a few weeks, and I don't have any insight into the reason. If there's anything i can do to help either one along, please let me know. One idea I had is that I could rebase this on master so it doesn't depend on the other, would that help?

Thanks.

[jingham]

I can't see anything wrong with this, it seems to follow the practice of other parts of GetSharedModule. I'm still not quite certain why I've never seen an instance where the absence of your fix has caused problems, and it isn't clear from the patch why that is. If there something special about handling breakpad files that moves you out of the normal code path for replacing overridden modules? So I still have a lurking feeling I'm missing something?

OTOH, seems like any time you find that one module is a newer version of one you held onto you should mention it, so this seems okay.

[JosephTremoulet]

Thanks for the review!

I'm still unsure how to proceed w.r.t. D89156 -- did you intend to include that when you approved this change, or do you have a sense when you'll have time to look at it, or should I change this to not depend on it, or find another reviewer, or ... ? I'd appreciate any advice.

I'm still not quite certain why I've never seen an instance where the absence of your fix has caused problems, and it isn't clear from the patch why that is. If there something special about handling breakpad files that moves you out of the normal code path for replacing overridden modules? So I still have a lurking feeling I'm missing something?

I'm confused by that too. When reducing the repro / creating the testcase, I was surprised how tough it was to hit exactly this codepath. I don't know what the scenario was that these calls to ReplaceEquivalent were originally put in for (I couldn't figure it out from source history) to be able to say why it didn't matter there about not reporting the old module. There is some fallback code in Target::GetOrCreateModule to deal with the case that GetSharedModule didn't report the old module:

// GetSharedModule is not guaranteed to find the old shared module, for
// instance in the common case where you pass in the UUID, it is only
// going to find the one module matching the UUID.  In fact, it has no
// good way to know what the "old module" relevant to this target is,
// since there might be many copies of a module with this file spec in
// various running debug sessions, but only one of them will belong to
// this target. So let's remove the UUID from the module list, and look
// in the target's module list. Only do this if there is SOMETHING else
// in the module spec...
if (!old_module_sp) {
  if (module_spec.GetUUID().IsValid() &&
      !module_spec.GetFileSpec().GetFilename().IsEmpty() &&
      !module_spec.GetFileSpec().GetDirectory().IsEmpty()) {
    ModuleSpec module_spec_copy(module_spec.GetFileSpec());
    module_spec_copy.GetUUID().Clear();

    ModuleList found_modules;
    m_images.FindModules(module_spec_copy, found_modules);
    if (found_modules.GetSize() == 1)
      old_module_sp = found_modules.GetModuleAtIndex(0);
  }

ProcessMinidump::ReadModuleList calls into GetOrCreateModule without a UUID because it needs to handle the case that the thing reported in the uuid field of the minidump is actually a hash of the text section rather than a standard uuid, so it's possible that other cases usually have uuids and get saved by that fallback code, perhaps?

Another thing that's potentially unique for minidumps is that they will often try to resolve the executable twice -- once in ResolveExecutable, but then a second time if the executable appears in the module list in the minidump (which it should).

I've looked a bit at the codepaths used when loading ELF core dumps, and they don't have this module list iteration but rather call DidAttach on the dynamic loader to populate the target module list. I'm not familiar with the details of the dynamic loader code or how it avoids running into this issue.

I'm also under the impression that opening dumps with user-specified sysroots on Linux is not a frequently-used scenario, because this is the fourth bug in that area that my team has discovered, and I don't think we were otherwise doing anything particularly exotic. This patch isn't sysroot-specific, but having a sysroot specified alters how GetSharedModule is called (with different directories and search paths), and I wasn't hitting the key codepath when I tried to reduce the repro by removing the sysroot specification.

[jingham]

All the parts of D89156 that are just being consistent about always passing old_modules as a vector seem okay. But then you get to a point where you shouldn't really have multiple modules replacing a single one so you aren't really sure what to do about it. That part makes me a little uneasy.

[JosephTremoulet]

But then you get to a point where you shouldn't really have multiple modules replacing a single one so you aren't really sure what to do about it. That part makes me a little uneasy.

Yeah, I wouldn't claim that the handling of the multiple-old-module case there is ideal. My thinking is that it makes an incremental step in the right direction, though -- the same potential for having multiple old modules is there with or without the change; the change makes the issue more apparent to readers of the code, makes note of it in the relevant log if we hit the issue at runtime, and refactors the code so there is one place to handle the issue. If you think it's best to hold off on that until we have a better way to actually handle the case, I can see the logic in that, just let me know and I'll rebase this to fix the bug in the meantime.

Thanks!

[jingham]

In D89157#2362697, @JosephTremoulet wrote:

But then you get to a point where you shouldn't really have multiple modules replacing a single one so you aren't really sure what to do about it. That part makes me a little uneasy.

Yeah, I wouldn't claim that the handling of the multiple-old-module case there is ideal. My thinking is that it makes an incremental step in the right direction, though -- the same potential for having multiple old modules is there with or without the change; the change makes the issue more apparent to readers of the code, makes note of it in the relevant log if we hit the issue at runtime, and refactors the code so there is one place to handle the issue. If you think it's best to hold off on that until we have a better way to actually handle the case, I can see the logic in that, just let me know and I'll rebase this to fix the bug in the meantime.

Thanks!

Since we'd just be dropping all the other modules that we don't know how they are coming about on the floor before your patch, that seems strictly better. Can you emit more info when this happens, like the module that's replacing this set of old modules, and the UUID of the duplicate entries. That way when this happens for some reason we didn't expect - which will invariably be on some proprietary code that we can't debug locally - we have a better chance of figuring out what went wrong. Then this should be fine.

lldb/include/lldb/Core/ModuleList.h

public:
   /// Append a module to the module list and remove any equivalent modules.
   /// Equivalent modules are ones whose file, platform file and architecture
   /// matches.
   ///
   /// Replaces the module to the collection.
   ///
   /// \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.
   ///
   /// \param[in] notify
   ///     If true, and a notifier function is set, the notifier function
   ///     will be called.  Defaults to true.
   ///
   ///     When this ModuleList is the Target's ModuleList, the notifier

lldb/source/Core/ModuleList.cpp

if (use_notifier && m_notifier)
       m_notifier->NotifyModuleAdded(*this, module_sp);
   }
 }
 
 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);
 
     // First remove any equivalent modules. Equivalent modules are modules
     // whose path, platform path and architecture match.
     ModuleSpec equivalent_module_spec(module_sp->GetFileSpec(),
                                       module_sp->GetArchitecture());
     equivalent_module_spec.GetPlatformFileSpec() =
         module_sp->GetPlatformFileSpec();
 
     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);
   }
 }
 
 bool ModuleList::AppendIfNeeded(const ModuleSP &module_sp, bool notify) {
   if (module_sp) {
     std::lock_guard<std::recursive_mutex> guard(m_modules_mutex);
if (!always_create) {
     const size_t num_matching_modules = matching_module_list.GetSize();
 
     if (num_matching_modules > 0) {
       for (size_t module_idx = 0; module_idx < num_matching_modules;
            ++module_idx) {
         module_sp = matching_module_list.GetModuleAtIndex(module_idx);
 
         // Make sure the file for the module hasn't been modified
         if (module_sp->FileHasChanged()) {
           if (old_modules)
             old_modules->push_back(module_sp);
 
           Log *log(lldb_private::GetLogIfAnyCategoriesSet(LIBLLDB_LOG_MODULES));
           if (log != nullptr)
             LLDB_LOGF(
                 log, "%p '%s' module changed: removing from global module list",
                 static_cast<void *>(module_sp.get()),
                 module_sp->GetFileSpec().GetFilename().GetCString());
 
if (uuid_ptr && *uuid_ptr != module_sp->GetUUID()) {
           module_sp->GetObjectFile()->GetType() ==
               ObjectFile::eTypeStubLibrary) {
         module_sp.reset();
       } else {
         if (did_create_ptr) {
           *did_create_ptr = true;
         }
 
-        shared_module_list.ReplaceEquivalent(module_sp);
+        shared_module_list.ReplaceEquivalent(module_sp, old_modules);
         return error;
       }
     }
   } else {
     module_sp.reset();
   }
 
   if (module_search_paths_ptr) {
for (size_t idx = 0; idx < num_directories; ++idx) {
         } else {
           if (module_sp->GetObjectFile()->GetType() ==
               ObjectFile::eTypeStubLibrary) {
             module_sp.reset();
           } else {
             if (did_create_ptr)
               *did_create_ptr = true;
 
-            shared_module_list.ReplaceEquivalent(module_sp);
+            shared_module_list.ReplaceEquivalent(module_sp, old_modules);
             return Status();
           }
         }
       } else {
         module_sp.reset();
       }
     }
   }
if (!module_sp) {
       if (module_sp && module_sp->GetObjectFile()) {
         if (module_sp->GetObjectFile()->GetType() ==
             ObjectFile::eTypeStubLibrary) {
           module_sp.reset();
         } else {
           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));
 
         if (located_binary_modulespec.GetFileSpec()) {
           if (arch.IsValid())
             error.SetErrorStringWithFormat(
                 "unable to open %s architecture in '%s'",

lldb/test/API/functionalities/postmortem/minidump-new/TestMiniDumpUUID.py

def verify_module(self, module, verify_path, verify_uuid):
         # minidump appended to a Windows-style root path from the host.
         self.assertEqual(
             os.path.basename(verify_path), module.GetFileSpec().basename)
         self.assertEqual(
             os.path.normcase(os.path.dirname(verify_path)),
             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
 
     def test_zero_uuid_modules(self):
         """
             Test multiple modules having a MINIDUMP_MODULE.CvRecord that is valid,
             but contains a PDB70 value whose age is zero and whose UUID values are
             all zero. Prior to a fix all such modules would be duplicated to the
def test_breakpad_overflow_hash_match(self):
         cmd = 'settings set target.exec-search-paths "%s"' % (os.path.dirname(so_path))
         self.dbg.HandleCommand(cmd)
         modules = self.get_minidump_modules("linux-arm-breakpad-uuid-match.yaml")
         self.assertEqual(1, len(modules))
         # LLDB makes up it own UUID as well when there is no build ID so we
         # 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):
         """
             Breakpad creates minidump files using CvRecord in each module whose
             signature is set to PDB70 where the UUID is a hash generated by
             breakpad of the .text section and Facebook modified this hash to
             avoid collisions. This is only done when the executable has no ELF
             build ID.