[Lldb-commits] [PATCH] D141702: [lldb/crashlog] Make module loading use Scripted Process affordance

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added inline comments.


================
Comment at: lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp:537-543
+  for (size_t i = 0; i < loaded_images_sp->GetSize(); i++)
+    if (loaded_images_sp->GetItemAtIndexAsDictionary(i, item))
+      task_group.async(fetch_symbols, item);
+  task_group.wait();
+
+  if (!loaded_images_sp->ForEach(load_modules))
+    return {};
----------------
mib wrote:
> This is not behaving as expected:
> 
> 1. During the symbol fetching stage (in parallel), I can see from the progress report messages that it goes pretty fast originally, but when it gets close to end, it gets stuck for few seconds. When I profile LLDB at that point, I can still see multiple calls to `DownloadObjectAndSymbolFile` which lets me think that the progress reporting doesn't reflect the completion of the symbol fetching but rather just to the creation of `dsymForUUID` process.
> 
> 2. I don't know if this is related to 1. but for some reason, when I try to load the modules, only the module executable gets reloaded. I need to investigate more but may be @JDevlieghere you can see a problem with this implementation ?
1. In `fetch_symbols` you don't increment the progress count when the dsymForUUID calls fails, so as soon as one fails, the progress will never complete. You'll need to either increment the count in the failure path or scope the progress to match the scope of the  thread pool. But that doesn't explain why you're still seeing `DownloadObjectAndSymbolFile` instances. 
2. You need to call `debugger::ReportSymbolChange(module_spec);` to let the debugger know it needs to reload the symbols. 


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

https://reviews.llvm.org/D141702