[Lldb-commits] [PATCH] D89156: [lldb] GetSharedModule: Collect old modules in SmallVector

Tue Oct 13 19:21:25 PDT 2020

JosephTremoulet added a comment.

In D89156#2328772 <https://reviews.llvm.org/D89156#2328772>, @jingham wrote:

> IIUC, the problem with ReplaceModule will get fixed in a dependent patch, right?  So the last bit of this patch will get cleaned up by that change.  If that's right, then this LGTM.

No, I wasn't planning any changes to ReplaceModule (dependent patch D89157 <https://reviews.llvm.org/D89157>, if you're thinking of that one, fixes an issue in ReplaceEquivalent, which is orthogonal to the bit about ReplaceModule that I mentioned in this change description).

Let me back up.

I ran into a bug whereby I open a minidump and the module list has two entries for a particular module, one of which is stale/defunct.  I tracked down where the stale one is coming from; it's added in ResolveExecutable but then discarded by a call to ReplaceEquivalent when we see the executable's module in the minidump's module list.

I saw that Target::GetOrCreateModule already has logic to remove modules from the target's list as they become stale.  It identifies stale modules by way of an out parameter (`ModuleSP* old_module_sp`) on several of the utilities it calls.  So the fix I'm pursuing is to add such an out parameter to ResolveExecutable.  When I went to write that code, though, I found that I'd have to deal with the potential case of replacing more than one module, since ReplaceEquivalent loops through all old modules rather than assuming that only one is equivalent.  So, I looked at how the other methods handle the potential case of finding an old module when their out parameter is already pointing at another old module.  I found that they were inconsistent -- some would overwrite it (failing to report the first old module), others would leave it be (failing to report the second old module), and some would clear it (failing to report either old module),

So, I put together this patch to accumulate the potentially-multiple old modules into a vector, which Target::GetOrCreateModule can then decide how to process, and made the dependent patch to fix the bug (D89157 <https://reviews.llvm.org/D89157>) by adding the out parameter to ReplaceEquivalent.

In this patch, though, I still have to have code (now in Target::GetOrCreateModule) to handle the potential case of finding more than one old module.  The old code would call `m_images.ReplaceModule(old_module, new_module)`, which in turn may call `NotifyModuleUpdated(..., old_module, new_module)` on an external notifier.  Since I'm not sure we ever actually hit the case that we're replacing multiple old modules (if we replace any old one whenever we add a new one, we should never have more than one, right?), I didn't think that this merited changing the signatures of ReplaceModule and NotifyModuleUpdated to take a list of old modules, nor did I think that calling them multiple times with the same new module made sense.  That's what led me to the code I have in this patch, which will send the notification for the first old module and then just remove any potential further old modules (with logging).  I'm happy to revisit it if you think that's the wrong way to go, though I still don't know what alternative to prefer.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89156