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