[lldb/crashlog] Make module loading use Scripted Process affordance

[mib]

Summary

This patch changes the way modules get loaded in lldb when using
interactive crashlog. It takes advantage of a Scripted Process
affordance to let lldb fetch symbols in parallel and load modules itself.

This also allows us to report progress on module loading to the user.

Signed-off-by: Med Ismail Bennani <[e-mail]>

[bulbazord]

I'm somewhat unfamiliar with this code so I'll ask some clarifying questions. I think I understand the idea in general though.

lldb/examples/python/scripted_process/crashlog_scripted_process.py

34–38

I don't understand the intent of this part. It looks like you're changing the format of self.loaded_images here. It's still a List, but instead of containing images it contains specific information about specific sections of each image. If the format has changed, don't consumers of get_loaded_images need to be modified as well?

lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp

434–440

Shouldn't you be using the result of Symbols::DownloadObjectAndSymbolFile here? I've read through the implementation of DownloadObjectAndSymbolFile and here's what I've surmised:

• The non-Darwin implementation of DownloadObjectAndSymbolFile simply returns false, so error won't be populated.
• The Darwin implementation fills in error only as a result of invoking dsymForUUID and subsequent processing of the result. It's entirely possible for the function to return false before dsymForUUID is invoked or without any useful information in error.

[bulbazord]

Additionally, it looks like you're doing maybe 2-3 things in this patch (Progress reporting, adding a guard rail, changing the format of loaded_images in the scripted process example). You could probably break this up into multiple patches.

[mib]

lldb/examples/python/scripted_process/crashlog_scripted_process.py

34–38

The list for this specific scripted process class was wrong, it expects the current format. Previously we didn't make use of loaded_images for the crashlog_scripted_process class, we just relied on some ad hoc heuristic to load the modules.
The other consumers of get_loaded_images are fine.

lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp

434–440

Good catch!

[mib]

Address @bulbazord feedback

[bulbazord]

Seems okay to me.
@JDevlieghere How does this look to you?

lldb/examples/python/scripted_process/crashlog_scripted_process.py

34–38

Ah, that makes sense. If one wanted to know what the format of loaded_images *should* be, how would they find out? Is that information documented or encoded anywhere? Or is it more ad-hoc?

[mib]

lldb/examples/python/scripted_process/crashlog_scripted_process.py

34–38

It's documented in the get_loaded_images in the base ScriptedProcess python class.

[JDevlieghere]

lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp

435

The last argument here is force_lookup (you should probably add that as an inline comment). This means that for any scripted process (not just interactive crashlogs) we're going to do a dSYMForUUID call (like we do for loading a kernel). I suppose that's what you want for interactive crashlogs, but not for a generic scripted process implementation.

[mib]

Only force symbol lookup for crashlog scripted process.

[mib]

ping @JDevlieghere @bulbazord :p

[JDevlieghere]

lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp

392

This seems inverted: rather than the generic scripted process implementation having to know about a concrete implementation, why not make this a capability that the crashlog can opt into?

[bulbazord]

lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp

392

Instead of relying on the class name being CrashLog, why not ask the scripted metadata for its capabilities? Something like bool force_lookup = m_scripted_metadata.ShouldForceSymbolFileLookup(); or something to this effect?

[mib]

lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp

497–498

I'm just thinking about this but I don't see any reason not to run this loop in parallel to accelerate dependency loading ... That could potentially mess up the progress reporting but I think in parallel it might not even be needed. @JDevlieghere Any objections ?

[mib]

Address @JDevlieghere & @bulbazord comments

• Use D142059's ScriptedProcess::GetCapabilities to decide whether we should force symbol lookup.
• Make symbol fetching parallel.

[mib]

lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp

505–511

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 ?

[JDevlieghere]

lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp

505–511

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.

[mib]

Address @JDevlieghere comments.

[mib]

Fixed module loading issue.

[bulbazord]

Just a personal preference but making those lambdas into static functions may make it easier to read.

[bulbazord]

Sorry, this fell off my radar for a while. I re-read through it and it looks fine to me, the lambda vs static methods function thing doesn't really matter much. LGTM.

lldb/examples/python/scripted_process/crashlog_scripted_process.py

def parse_crashlog(self):
         self.exception = crash_log.exception
         self.app_specific_thread = None
         if hasattr(crash_log, 'asi'):
             self.metadata['asi'] = crash_log.asi
         if hasattr(crash_log, 'asb'):
             self.extended_thread_info = crash_log.asb
 
         def load_images(self, images):
-            #TODO: Add to self.loaded_images and load images in lldb
+            self.loaded_images.clear()
             if images:
                 for image in images:
                     if image not in self.loaded_images:
                         if image.uuid == uuid.UUID(int=0):
                             continue
-                        err = image.add_module(self.target)
-                        if err:
-                            # Append to SBCommandReturnObject
-                            print(err)
-                        else:
-                            self.loaded_images.append(image)
+                        for section in image.section_infos:
+                            if section.start_addr and section.name == "__TEXT":
+                                self.loaded_images.append({"uuid": str(image.uuid),
+                                                           "load_addr": section.start_addr})
 
         for thread in crash_log.threads:
             if self.load_all_images:
                 load_images(self, crash_log.images)
             elif thread.did_crash():
                 for ident in thread.idents:
                     load_images(self, crash_log.find_images_with_identifier(ident))
 
             if hasattr(thread, 'app_specific_backtrace') and thread.app_specific_backtrace:
def __init__(self, exe_ctx: lldb.SBExecutionContext, args : lldb.SBStructuredData):
         if not self.load_all_images:
             self.load_all_images = False
 
         self.pid = super().get_process_id()
         self.crashed_thread_idx = 0
         self.exception = None
         self.extended_thread_info = None
         self.parse_crashlog()
+        self.capabilities['force_symbol_lookup'] = True
 
     def get_memory_region_containing_address(self, addr: int) -> lldb.SBMemoryRegionInfo:
         return None
 
     def get_thread_with_id(self, tid: int):
         return {}
 
     def get_registers_for_thread(self, tid: int):

lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp

 //===-- ScriptedProcess.cpp -----------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "ScriptedProcess.h"
 
 #include "lldb/Core/Debugger.h"
 #include "lldb/Core/Module.h"
 #include "lldb/Core/PluginManager.h"
-
+#include "lldb/Core/Progress.h"
 #include "lldb/Host/OptionParser.h"
 #include "lldb/Host/ThreadLauncher.h"
 #include "lldb/Interpreter/CommandInterpreter.h"
 #include "lldb/Interpreter/OptionArgParser.h"
 #include "lldb/Interpreter/OptionGroupBoolean.h"
 #include "lldb/Interpreter/ScriptInterpreter.h"
 #include "lldb/Interpreter/ScriptedMetadata.h"
+#include "lldb/Symbol/LocateSymbolFile.h"
 #include "lldb/Target/MemoryRegionInfo.h"
 #include "lldb/Target/Queue.h"
 #include "lldb/Target/RegisterContext.h"
 #include "lldb/Utility/LLDBLog.h"
 #include "lldb/Utility/State.h"
 
+#include "llvm/Support/ThreadPool.h"
+
 #include <mutex>
 
 LLDB_PLUGIN_DEFINE(ScriptedProcess)
 
 using namespace lldb;
 using namespace lldb_private;
 
 llvm::StringRef ScriptedProcess::GetPluginDescriptionStatic() {
ScriptedProcess::GetLoadedDynamicLibrariesInfos() {
   };
 
   StructuredData::ArraySP loaded_images_sp = GetInterface().GetLoadedImages();
 
   if (!loaded_images_sp || !loaded_images_sp->GetSize())
     return ScriptedInterface::ErrorWithMessage<StructuredData::ObjectSP>(
         LLVM_PRETTY_FUNCTION, "No loaded images.", error);
 
+  size_t num_images_to_load = loaded_images_sp->GetSize();
   ModuleList module_list;
   Target &target = GetTarget();
 
-  auto reload_image = [&target, &module_list, &error_with_message](
-                          StructuredData::Object *obj) -> bool {
-    StructuredData::Dictionary *dict = obj->GetAsDictionary();
+  StructuredData::DictionarySP capabilities_sp =
+      GetInterface().GetCapabilities();
+  if (!capabilities_sp)
+    ScriptedInterface::ErrorWithMessage<StructuredData::ObjectSP>(
+        LLVM_PRETTY_FUNCTION,
+        "Couldn't fetch scripted process capabilities.\nContinuing loading "
+        "dynamic libraries.",
+        error);
+
+  bool force_lookup = false;
+  capabilities_sp->GetValueForKeyAsBoolean("force_symbol_lookup", force_lookup);
 
+  std::mutex m;
+  std::vector<ModuleSpec> module_specs(num_images_to_load);
+  Progress progress("Fetching external dependencies", num_images_to_load);
+  auto fetch_symbols =
+      [&error_with_message, &progress, &force_lookup, &m, &module_specs,
+       &target](StructuredData::Dictionary *dict, size_t index) -> bool {
     if (!dict)
       return error_with_message("Couldn't cast image object into dictionary.");
 
-    ModuleSpec module_spec;
-    llvm::StringRef value;
-
     bool has_path = dict->HasKey("path");
     bool has_uuid = dict->HasKey("uuid");
     if (!has_path && !has_uuid)
       return error_with_message("Dictionary should have key 'path' or 'uuid'");
     if (!dict->HasKey("load_addr"))
       return error_with_message("Dictionary is missing key 'load_addr'");
 
+    ModuleSpec module_spec;
+    llvm::StringRef value;
     if (has_path) {
       dict->GetValueForKeyAsString("path", value);
       module_spec.GetFileSpec().SetPath(value);
     }
 
     if (has_uuid) {
       dict->GetValueForKeyAsString("uuid", value);
       module_spec.GetUUID().SetFromStringRef(value);
     }
     module_spec.GetArchitecture() = target.GetArchitecture();
 
+    Status error;
+    bool try_download_object_and_symbols =
+        Symbols::DownloadObjectAndSymbolFile(module_spec, error, force_lookup);
+    bool has_download_succeeded =
+        FileSystem::Instance().Exists(module_spec.GetFileSpec());
+    bool try_fetching_symbols = (try_download_object_and_symbols ||
+                                 error.Success() || has_download_succeeded);
+
+    {
+      std::lock_guard<std::mutex> lock(m);
+      // We need to increment progress and append the module spec to the vector
+      // even if symbol fetching failed.
+      progress.Increment();
+      module_specs[index] = module_spec;
+    }
+
+    if (!try_fetching_symbols)
+      return error_with_message(error.AsCString());
+
+    return true;
+  };
+
+  auto load_modules = [&target, &module_list,
+                       &error_with_message](StructuredData::Dictionary *dict,
+                                            ModuleSpec &module_spec) -> bool {
+    if (!dict)
+      return error_with_message("Structured data object is not a dictionary.");
+
     ModuleSP module_sp =
-        target.GetOrCreateModule(module_spec, true /* notify */);
+        target.GetOrCreateModule(module_spec, true /*=notify*/);
 
     if (!module_sp)
       return error_with_message("Couldn't create or get module.");
 
+    Debugger::ReportSymbolChange(module_spec);
+
+    llvm::StringRef value;
     lldb::addr_t load_addr = LLDB_INVALID_ADDRESS;
     lldb::addr_t slide = LLDB_INVALID_OFFSET;
     dict->GetValueForKeyAsInteger("load_addr", load_addr);
     dict->GetValueForKeyAsInteger("slide", slide);
     if (load_addr == LLDB_INVALID_ADDRESS)
       return error_with_message(
           "Couldn't get valid load address or slide offset.");
 
     if (slide != LLDB_INVALID_OFFSET)
       load_addr += slide;
 
     bool changed = false;
     module_sp->SetLoadAddress(target, load_addr, false /*=value_is_offset*/,
                               changed);
 
     if (!changed && !module_sp->GetObjectFile())
       return error_with_message("Couldn't set the load address for module.");
 
+    if (dict->HasKey("path")) {
       dict->GetValueForKeyAsString("path", value);
       FileSpec objfile(value);
       module_sp->SetFileSpecAndObjectName(objfile, objfile.GetFilename());
+    }
 
     return module_list.AppendIfNeeded(module_sp);
   };
 
-  if (!loaded_images_sp->ForEach(reload_image))
-    return ScriptedInterface::ErrorWithMessage<StructuredData::ObjectSP>(
-        LLVM_PRETTY_FUNCTION, "Couldn't reload all images.", error);
+  // Share one thread pool across operations to avoid the overhead of
+  // recreating the threads.
+  llvm::ThreadPoolTaskGroup task_group(Debugger::GetThreadPool());
+
+  // Create a task runner that fetches symbols and imports dynamic libraries in
+  // separate threads.
+  StructuredData::Dictionary *item = nullptr;
+  for (size_t i = 0; i < num_images_to_load; i++)
+    if (loaded_images_sp->GetItemAtIndexAsDictionary(i, item))
+      task_group.async(fetch_symbols, item, i);
+  task_group.wait();
+
+  for (size_t i = 0; i < num_images_to_load; i++)
+    if (loaded_images_sp->GetItemAtIndexAsDictionary(i, item))
+      load_modules(item, module_specs[i]);
 
   target.ModulesDidLoad(module_list);
 
   return loaded_images_sp;
 }
 
 lldb_private::StructuredData::DictionarySP ScriptedProcess::GetMetadata() {
   StructuredData::DictionarySP metadata_sp = GetInterface().GetMetadata();