[Lldb-commits] [PATCH] D88387: Create "skinny corefiles" for Mach-O with process save-core / reading

Mon Sep 28 14:08:07 PDT 2020

clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.

Many inline comments that should explain everything.

================
Comment at: lldb/include/lldb/Symbol/ObjectFile.h:669-692
+  /// A corefile may include metadata about all of the binaries that were
+  /// present in the process when the corefile was taken.  This is only
+  /// implemented for Mach-O files for now; we'll generalize it when we
+  /// have other systems that can include the same.
+  struct MachOCorefileImageEntry {
+    std::string filename;
+    UUID uuid;
----------------
We shouldn't be exposing mach-o specific stuff here. I think an API that lets the ObjectFile (if it is a core file) load all needed images at the right addresses might be better encapsulated:

```
virtual llvm::Error LoadCoreFileImages(lldb_private::Target &target);
```
It returns an error if something the ObjectFile is not a core file or something fatal happens. Then you don't need to expose any of the mach-o specific things here, and we can probably remove the GetCorefileExecutingUUIDs() as well below.

================
Comment at: lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp:6157-6162
+// identical permissions.
+struct page_object {
+  addr_t addr;
+  addr_t size;
+  uint32_t prot;
+};
----------------
Use RangeDataVector from RangeMap.h:

```
typedef lldb_private::RangeDataVector<lldb::addr_t, lldb::addr_t, uint32_t> PageObjects;
```

================
Comment at: lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp:6164-6165
+
 bool ObjectFileMachO::SaveCore(const lldb::ProcessSP &process_sp,
                                const FileSpec &outfile, Status &error) {
   if (!process_sp)
----------------
We should add an extra boolean as an argument to allow saving all memory regions, or just the dirty pages?

================
Comment at: lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp:6202
       const ByteOrder byte_order = target_arch.GetByteOrder();
+      std::vector<page_object> pages_to_copy;
       if (range_error.Success()) {
----------------
Use PageObjects as defined above:

```
PageObjects pages_to_copy;
```

================
Comment at: lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp:6223
+              range_info.GetDirtyPageList();
+          if (dirty_page_list.hasValue()) {
+            for (addr_t dirtypage : dirty_page_list.getValue()) {
----------------
If we add a bool argument, we might need to return an error if the lldb-server doesn't support the dirty page list stuff. Some regions won't have dirty pages, but we might need add detection for any dirty pages and then error out at the end if user requested a minimal core file

================
Comment at: lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp:6225-6230
+              page_object obj;
+              obj.addr = dirtypage;
+              obj.size = pagesize;
+              obj.prot = prot;
+              if (prot != 0)
+                pages_to_copy.push_back(obj);
----------------
```
if (prot != 0) 
  pages_to_copy.Append({dirtypage, pagesize, prot});
```

================
Comment at: lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp:6233-6238
+            page_object obj;
+            obj.addr = addr;
+            obj.size = size;
+            obj.prot = prot;
+            if (prot != 0)
+              pages_to_copy.push_back(obj);
----------------
```
if (prot != 0) 
  pages_to_copy.Append({addr, size, prot});
```

================
Comment at: lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp:6246-6264
+        // Combine contiguous entries that have the same
+        // protections so we don't have an excess of
+        // load commands.
+        std::vector<page_object> combined_page_objects;
+        page_object last_obj;
+        last_obj.addr = LLDB_INVALID_ADDRESS;
+        for (page_object obj : pages_to_copy) {
----------------
Remove all of this and do:

```
page_objects.CombineConsecutiveEntriesWithEqualData();
```

================
Comment at: lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp:6266
+
+        for (page_object obj : combined_page_objects) {
+          uint32_t cmd_type = LC_SEGMENT_64;
----------------
use paige_objects directly now since we sorted it above:

```
for (page_object obj : page_objects) {
```

================
Comment at: lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp:6582
+  uint32_t segment_count;    // The number of segments for this binary.
+  uint32_t unused;
+
----------------
Can we get rid of the UUID array we save and add a flag here that states that the library was loaded instead of saving it in a separate load command?

================
Comment at: lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp:6811
+          uint32_t segment_count = m_data.GetU32(&offset);
+          offset += 4; // unused
+
----------------
Can we read a "uint32_t flags" field here where one of the bits is a "I was part of the thread stacks"?

================
Comment at: lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp:6841
+
+offset_t ObjectFileMachO::CreateExecutingUUIDsPayload(
+    const lldb::ProcessSP &process_sp, offset_t file_offset,
----------------
We can get rid of this entire load command if we put a bit into the all image info struct. We already have an unused uint32_t field available.

================
Comment at: lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp:6884
+
+std::vector<UUID> ObjectFileMachO::GetCorefileExecutingUUIDs() {
+  std::vector<UUID> uuids;
----------------
We can get rid of this entire load command if we put a bit into the all image info struct. We already have an unused uint32_t field available.

================
Comment at: lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.h:120-124
+  lldb_private::ObjectFile::MachOCorefileAllImageInfos
+  GetCorefileAllImageInfos() override;
+
+  std::vector<lldb_private::UUID> GetCorefileExecutingUUIDs() override;
+
----------------
Remove and just do:

```
llvm::Error LoadCoreFileImages(lldb_private::Target &target) override;
```

================
Comment at: lldb/source/Plugins/Process/mach-core/ProcessMachCore.cpp:279-293
+  // If we have an "executing uuids" LC_NOTE, force a dsymForUUID
+  // style lookup for those binaries / dSYMs.  The corefile may have
+  // hundreds of binary images included in the process, but only a
+  // handful of them were actually executing code when the corefile was
+  // taken.  We can do an expensive search for this more limited set of
+  // images.
+  std::vector<UUID> executing_uuids = core_objfile->GetCorefileExecutingUUIDs();
----------------
We can get rid of this entire load command if we put a bit into the all image info struct. We already have an unused uint32_t field available.

================
Comment at: lldb/source/Plugins/Process/mach-core/ProcessMachCore.cpp:421-455
+  // If we have a "all image infos" LC_NOTE, try to load all of the
+  // binaries listed, and set their Section load addresses in the Target.
+  ObjectFile::MachOCorefileAllImageInfos image_infos =
+      core_objfile->GetCorefileAllImageInfos();
+  if (found_main_binary_definitively == false && image_infos.IsValid()) {
+    m_dyld_plugin_name = DynamicLoaderDarwinKernel::GetPluginNameStatic();
+    found_main_binary_definitively = true;
----------------
This functionality can be moved to the "ObjectFileMachO::LoadCoreFileImages(lldb_private::Target &target)" function. If we have flags in the all image infos for each library that states if it was part of an active stack, then we can also call the object and symbols download:

```
Symbols::DownloadObjectAndSymbolFile(module_spec, true);
if (FileSystem::Instance().Exists(module_spec.GetFileSpec()))
  GetTarget().GetOrCreateModule(module_spec, false);
else {
 ...
}
```

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88387