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

[Joseph Tremoulet via Phabricator via lldb-commits]

JosephTremoulet added a comment.

Thanks for the review!

I'm still unsure how to proceed w.r.t. D89156 <https://reviews.llvm.org/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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D89157/new/

https://reviews.llvm.org/D89157