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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
