This is an archive of the discontinued LLVM Phabricator instance.

[lldb] GetSharedModule: Collect old modules in SmallVector
ClosedPublic

Authored by JosephTremoulet on Oct 9 2020, 12:15 PM.

Details

Summary

The various GetSharedModule methods have an optional out parameter for
the old module when a file has changed or been replaced, which the
Target uses to keep its module list current/correct. We've been using
a single ModuleSP to track "the" old module, and this change switches
to using a SmallVector of ModuleSP, which has a couple benefits:

  • There are multiple codepaths which may discover an old module, and this centralizes the code for how to handle multiples in one place, in the Target code. With the single ModuleSP, each place that may discover an old module is responsible for how it handles multiples, and the current code is inconsistent (some code paths drop the first old module, others drop the second).
  • The API will be more natural for identifying old modules in routines that work on sets, like ModuleList::ReplaceEquivalent (which I plan on updating to report old module(s) in a subsequent change to fix a bug).

I'm not convinced we can ever actually run into the case that multiple
old modules are found in the same GetOrCreateModule call, but I think
this change makes sense regardless, in light of the above.

When an old module is reported, Target::GetOrCreateModule calls
m_images.ReplaceModule, which doesn't allow multiple "old" modules; the
new code calls ReplaceModule for the first "old" module, and for any
subsequent old modules it logs the event and calls m_images.Remove.

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptOct 9 2020, 12:15 PM
JosephTremoulet requested review of this revision.Oct 9 2020, 12:15 PM

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.

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

  • Expand logging as suggested in D89157
lldb/source/Target/Target.cpp
2131–2132

Oops, the new logging code needs this condition. Will fix...

  • Ignore old modules not in target list

OK, I believe this is correct now.

This revision is now accepted and ready to land.Oct 30 2020, 11:17 AM
This revision was landed with ongoing or failed builds.Oct 30 2020, 12:14 PM
This revision was automatically updated to reflect the committed changes.

Shouldn't this also update PlatformiOSSimulator.h?

Shouldn't this also update PlatformiOSSimulator.h?

There is no PlatformOSSimulator.h file (or I can't find it). There is a type PlatformiOSSumulator, but it doesn't have its own GetSharedModule method. Its CreateInstance method returns a PlatformAppleSimulator[1], and this change does update PlatformAppleSimulator. So unless I'm misunderstanding your point, I don't think any changes specific to PlatformiOSSimulator are needed

1 - https://github.com/llvm/llvm-project/blob/82847436e9258a12503dcfadb5dc373cb42fea43/lldb/source/Plugins/Platform/MacOSX/PlatformAppleSimulator.cpp#L538

Shouldn't this also update PlatformiOSSimulator.h?

There is no PlatformOSSimulator.h file (or I can't find it). There is a type PlatformiOSSumulator, but it doesn't have its own GetSharedModule method. Its CreateInstance method returns a PlatformAppleSimulator[1], and this change does update PlatformAppleSimulator. So unless I'm misunderstanding your point, I don't think any changes specific to PlatformiOSSimulator are needed

1 - https://github.com/llvm/llvm-project/blob/82847436e9258a12503dcfadb5dc373cb42fea43/lldb/source/Plugins/Platform/MacOSX/PlatformAppleSimulator.cpp#L538

Well, there is https://github.com/llvm/llvm-project/tree/llvmorg-11.0.1/lldb/source/Plugins/Platform/MacOSX
And if you look here https://github.com/llvm/llvm-project/blob/llvmorg-11.0.1/lldb/source/Plugins/Platform/MacOSX/PlatformiOSSimulator.h#L57 you will see why the build will crash.
Also this applies for the AppleTV implementation.

Oh, I see. I made this change in the master branch and then ported it to the release/11.x branch. It changed the signature of a virtual method, which had a couple more overrides in the release/11.x branch, and when I ported the change I didn't realize those other overrides existed. So now the release/11.x builds that include those overrides (btw, what build flags are needed to include those?) are broken. I'm not sure what the options are now that 11.0.1 has been released and 11.1.0 is at rc2 -- @tstellar, thoughts?

Oh, I see. I made this change in the master branch and then ported it to the release/11.x branch. It changed the signature of a virtual method, which had a couple more overrides in the release/11.x branch, and when I ported the change I didn't realize those other overrides existed. So now the release/11.x builds that include those overrides (btw, what build flags are needed to include those?) are broken. I'm not sure what the options are now that 11.0.1 has been released and 11.1.0 is at rc2 -- @tstellar, thoughts?

I have a fix for this ready, I can push it to 11.0.1.

Oh, I see. I made this change in the master branch and then ported it to the release/11.x branch. It changed the signature of a virtual method, which had a couple more overrides in the release/11.x branch, and when I ported the change I didn't realize those other overrides existed. So now the release/11.x builds that include those overrides (btw, what build flags are needed to include those?) are broken. I'm not sure what the options are now that 11.0.1 has been released and 11.1.0 is at rc2 -- @tstellar, thoughts?

I have a fix for this ready, I can push it to 11.0.1.

Can you file a bug for this or re-open the merge request bug?

Oh, I see. I made this change in the master branch and then ported it to the release/11.x branch. It changed the signature of a virtual method, which had a couple more overrides in the release/11.x branch, and when I ported the change I didn't realize those other overrides existed. So now the release/11.x builds that include those overrides (btw, what build flags are needed to include those?) are broken. I'm not sure what the options are now that 11.0.1 has been released and 11.1.0 is at rc2 -- @tstellar, thoughts?

I have a fix for this ready, I can push it to 11.0.1.

Can you file a bug for this or re-open the merge request bug?

I have D95683 and create release blocker bug https://bugs.llvm.org/show_bug.cgi?id=48944.