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 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
