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

Mon Sep 26 11:24:12 PDT 2022

jasonmolenda added a comment.

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.  I think it would be more straightforward than adding this change to Target::GetOrAddModule.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134581