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

Tue Sep 27 08:12:03 PDT 2022

labath added a comment.

In D134581#3817457 <https://reviews.llvm.org/D134581#3817457>, @alvinhochun wrote:

> In D134581#3815766 <https://reviews.llvm.org/D134581#3815766>, @jasonmolenda wrote:
>
>> In D134581#3813757 <https://reviews.llvm.org/D134581#3813757>, @alvinhochun wrote:
>>
>>> 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.

Seems reasonable to me. I'm not actually sure if we're doing this on linux/posix (as I still see duplicated vdso modules occasionaly -- but this could be caused by other problems as well), but if we're not -- we probably should.

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

I can totally believe that there is a bug in the GetOrAddModule, but I would not be able to tell you if this is the right fix or not. As for multiple modules, I can say that on linux it is actually possible to load the same shared library more than once (into different "linker namespaces"). LLDB does not currently support that, but I would like to support it one day. I don't know whether this change would make that harder or easier, though.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134581