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

[Med Ismail Bennani via Phabricator via lldb-commits]

mib marked an inline comment as done.
mib added inline comments.


================
Comment at: lldb/examples/python/scripted_process/crashlog_scripted_process.py:34-37
+                        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})
----------------
bulbazord wrote:
> 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?
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.


================
Comment at: lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp:452-458
+    Status error;
+    Symbols::DownloadObjectAndSymbolFile(module_spec, error, true);
+    if (error.Fail() ||
+        !FileSystem::Instance().Exists(module_spec.GetFileSpec())) {
+      return error_with_message(error.AsCString());
+    }
+
----------------
bulbazord wrote:
> 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`.
> 
> 
Good catch!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141702