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

Sun Sep 25 08:40:09 PDT 2022

alvinhochun added a reviewer: DavidSpickett.
alvinhochun added a comment.

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.

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

> I'll have to admit, a quick read through of Target::GetOrAddModule() worries me about adding this mid-function return - the code clearly has a mix of cases where it has found a Module on disk that is new to lldb and is loading it, it has found a module in lldb's global module cache which has the same UUID and/or filepath, and something in there about a Target which already has a module and the newly loaded module is used to replace it - not sure what that's about, but I only read it quickly.  My initial impression is that this change is unlikely to be the right thing to do, tbh, regardless of the more basic "why are we in this situation in the first place".   Again, I might be mistaken/not understanding the issue properly though.

I checked again and I think you are probably correct. Other than `!did_create_module` it seems an additional condition `&& m_images.FindModule(module_sp.get())` will be needed for it to do the right thing.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134581