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

[Alex Langford via Phabricator via lldb-commits]

bulbazord added a comment.

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



================
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})
----------------
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?


================
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());
+    }
+
----------------
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`.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141702