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

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.


================
Comment at: lldb/include/lldb/Target/MemoryRegionInfo.h:43-45
+    if (m_dirty_pages.hasValue()) {
+      m_dirty_pages.getValue().clear();
+    }
----------------
This bit is unnecessary. In fact, I'd implement the entire function as `*this = MemoryRegionInfo();`


================
Comment at: lldb/include/lldb/Target/MemoryRegionInfo.h:118
+  /// detail this.
+  llvm::Optional<std::vector<lldb::addr_t>> GetDirtyPageList() {
+    return m_dirty_pages;
----------------
`const Optional<...> &` to avoid a copy.


================
Comment at: lldb/include/lldb/Target/MemoryRegionInfo.h:125-127
+    if (m_dirty_pages.hasValue())
+      m_dirty_pages.getValue().clear();
+    m_dirty_pages = pagelist;
----------------
`m_dirty_pages = std::move(pagelist);`


================
Comment at: lldb/include/lldb/lldb-private-interfaces.h:58-59
+                                   const FileSpec &outfile,
+                                   lldb::SaveCoreStyle requested_core_style,
+                                   lldb::SaveCoreStyle &created_core_style,
+                                   Status &error);
----------------
Maybe just have one argument which will be set on exit to reflect the actual style that was used?


================
Comment at: lldb/source/API/SBMemoryRegionInfo.cpp:142
+  addr_t dirty_page_addr = LLDB_INVALID_ADDRESS;
+  llvm::Optional<std::vector<addr_t>> dirty_page_list =
+      m_opaque_up->GetDirtyPageList();
----------------
`const T &`


================
Comment at: lldb/source/Commands/CommandObjectMemory.cpp:1742
           name, section_name ? " " : "", section_name);
+      llvm::Optional<std::vector<addr_t>> dirty_page_list =
+          range_info.GetDirtyPageList();
----------------
const T &


================
Comment at: lldb/source/Commands/CommandObjectMemory.cpp:1750-1760
+          bool print_comma = false;
+          result.AppendMessageWithFormat("Dirty pages: ");
+          for (size_t i = 0; i < page_count; i++) {
+            if (print_comma)
+              result.AppendMessageWithFormat(", ");
+            else
+              print_comma = true;
----------------
I think something like this ought to do:
`AppendMessageWithFormatv("Dirty pages: {0:@[x]}.\n", llvm::make_range(dirty_page_list->begin(), dirty_page_list->end()));`


================
Comment at: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp:225
                                 lldb_private::Status &error) {
+  created_core_style = SaveCoreStyle::eSaveCoreMiniDump;
   return SaveMiniDump(process_sp, outfile, error);
----------------
I don't think this is a good use of that constant. Minidumps can also come in "full" styles, which includes all memory, or some smaller ones, where only the heap (or it's modified portion, or just stack, etc.) is included.

In essence, "minidump" is just a format, just like "elf" and "macho". It's peculiarity is that it is different from the native object file format, but that's not relevant here.

I think that the best way to handle this is to make the choice of format should be orthogonal to the choice of what goes into the dump. Currently we don't allow the user to pick a format, and that's fine -- we'll just pick whatever is the native format for the given platform. But I don't think we should start mixing the two.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88387