[Lldb-commits] [PATCH] D62504: Avoid calling LoadModules twice when modules change

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

Sorry, I missed this patch. Overall, this seems fine to me. I have only two comments about it:

- I'm not sure the `CanLoadModules` functions is really needed. It seems like we could just call `LoadModules` directly and check for error result. The `XMLEnabled() && ...` check could be done at the start of the `LoadModules` function. It looks like it already is there `ProcessGDBRemote::GetLoadedModuleList`, but it looks like that function accidentally returns a success error code instead of failure.
- Looking at the implementation of `ProcessGDBRemote::LoadModules`, I get the feeling it will return 0 if you call it when a module gets unloaded (although it will correctly unload the module, and correctly *not* load any new module). This is not a bug in this patch, so I'd be fine with checking it in without fixing that, but this sounds like something you would want to fix, as it will cause you to stop using the gdb-remote packet as soon as the first module is unloaded.

It looks like a single solution fixing both of these issues would be to change the `LoadModules` function to return `Expected<size_t>`. That way it could report the error states explicitly (such as when the svr packet is not supported or it just returns bogus data), and zero when it correctly did not load anything. (Or, given that the number of loaded modules is not particularly useful, and it does even not capture the effect of the function very precisely, the result could just be an `Error`).



================
Comment at: lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DYLDRendezvous.cpp:177
+    if (m_current.state == eConsistent) {
+      auto modules_loaded = m_process->LoadModules();
+      // If LoadModules fails do not try to use it again.
----------------
use a real type here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62504