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

[Greg Clayton via Phabricator via lldb-commits]

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

Nice changes. Just a few changes and should be good to go.



================
Comment at: lldb/include/lldb/Symbol/ObjectFile.h:675
+  ///
+  /// \param[in] target
+  ///     Target where to load binaries.
----------------
process


================
Comment at: lldb/include/lldb/Symbol/ObjectFile.h:676
+  /// \param[in] target
+  ///     Target where to load binaries.
+  ///
----------------
process


================
Comment at: lldb/source/API/SBProcess.cpp:1231
 
 lldb::SBError SBProcess::SaveCore(const char *file_name) {
   LLDB_RECORD_METHOD(lldb::SBError, SBProcess, SaveCore, (const char *),
----------------
We should add another SaveCore() includes the bool argument and have this one call that one.


================
Comment at: lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp:6600
+
+offset_t ObjectFileMachO::CreateAllImageInfosPayload(
+    const lldb::ProcessSP &process_sp, offset_t initial_file_offset,
----------------
Can this just be a static function in this file without being in the ObjectFileMachO class? That would keep the header file cleaner.


================
Comment at: lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.h:18
 #include "lldb/Utility/RangeMap.h"
+#include "lldb/Utility/StreamString.h"
 #include "lldb/Utility/UUID.h"
----------------
Don't need this include, it should be forward declared in lldb-forward.h. Move to .cpp file


================
Comment at: lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.h:215-216
 
+  typedef lldb_private::RangeDataVector<lldb::addr_t, lldb::addr_t, uint32_t>
+      PageObjects;
+
----------------
move to ObjectFileMachO.cpp, not needed in header file.


================
Comment at: lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.h:218-220
+  static lldb::offset_t CreateAllImageInfosPayload(
+      const lldb::ProcessSP &process_sp, lldb::offset_t file_offset,
+      lldb_private::StreamString &all_image_infos_payload);
----------------
Move to .cpp file? Not needed in header unless this static function takes avantage of being a part of the class which gives it access to non public stuff?


================
Comment at: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp:1252
+            int page_size;
+            if (!value.getAsInteger(0, page_size)) {
+              m_target_vm_page_size = page_size;
----------------
IS there a getAsUnsigned?


================
Comment at: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h:249
+  // 0 is returned for an un-set value.
+  int GetTargetVMPageSize();
+
----------------
uint32_t as the type? This should never be negative, so no need to use a signed value.


================
Comment at: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h:592
   std::chrono::seconds m_default_packet_timeout;
+  int m_target_vm_page_size; // target system VM page size; 0 if unspecified
   uint64_t m_max_packet_size;        // as returned by qSupported
----------------
uint32_t as the type? This should never be negative, so no need to use a signed value.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88387