[Lldb-commits] [PATCH] D134581: [lldb] Prevent re-adding a module that is already loaded

[Alvin Wong via Phabricator via lldb-commits]

alvinhochun added a comment.

In D134581#3815766 <https://reviews.llvm.org/D134581#3815766>, @jasonmolenda wrote:

> In D134581#3813757 <https://reviews.llvm.org/D134581#3813757>, @alvinhochun wrote:
>
>> Thanks for the comment.
>>
>> In D134581#3813484 <https://reviews.llvm.org/D134581#3813484>, @jasonmolenda wrote:
>>
>>> I probably misunderstand the situation DynamicLoaderWindows finds itself in, but in DynamicLoaderDarwin we have similar behavior - you run 'lldb binary' and lldb loads all the directly linked dynamic libraries into the target it creates, pre-execution.  When execution starts, the dynamic linker tells us where the binary is loaded in memory and we call Target::SetExecutableModule with it.  Target::SetExecutableModule has a side effect of clearing the Target's module list - you now have only one binary in the Target, your executable module.  (this is done so the 0th image in the image list is your executable module)
>>>
>>> Why aren't your pre-loaded DLLs unloaded when you call Target::SetExecutableModule?
>>
>> In `ProcessWindows::OnDebuggerConnected`, it first checks `GetTarget().GetExecutableModule()`. Only when the returned module is null (i.e. the binary hasn't already been loaded) then it calls `GetTarget().SetExecutableModule(module, eLoadDependentsNo)`. If I understood you correctly, then the right thing to do there should be to call `GetTarget().SetExecutableModule(module, eLoadDependentsNo)` in all cases, without checking `GetExecutableModule`, right?
>>
>> It seems to make sense, but I may need some comments from other reviewers on this.
>
> Just my opinion, but I know how DynamicDarwinLoader works is that when it starts the actual debug session, it clears the image list entirely (iirc or maybe it just calls Target::SetExecutableModule - which is effectively the same thing).  I don't know how Windows works, but on Darwin we pre-load the binaries we THINK will be loaded, but when the process actually runs, different binaries may end up getting loaded, and we need to use what the dynamic linker tells us instead of our original logic.  `GetTarget().SetExecutableModule(module, eLoadDependentsNo)` would be one option, or clear the list and start adding, effectively the same thing.

Sounds right. On Windows, events from the debug loop will tell us which DLLs are actually being loaded by the process and we add them to the module list.

> I think it would be more straightforward than adding this change to Target::GetOrAddModule.

Here's the thing, even if we change the Windows debugging support to clear the module list when starting the process, the logic of `Target::GetOrAddModule` still appears to be flawed if it can result in duplicated modules in the target module list, so IMO it needs to be fixed regardless.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134581