[NFC][LLDB] Refactor MachO to use options.GetMemoryRegionsToSave #149623
+303
−300
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@llvm/pr-subscribers-lldb Author: Jacob Lalonde (Jlalond) ChangesFollow up on (#146777), where we expose the memory ranges through the Save Core Options object, and simplify plugin manager to just take an options object with a process set. Mach-O Save core has a lot of nesting, so I converted this case into a guard clause. I wanted to do the same with make_core but I decided against it to keep the scope of the change as small as possible. Patch is 30.01 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/149623.diff 1 Files Affected:
diff --git a/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp b/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
index 13df6e2f26b53..bfc57322e137e 100644
--- a/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
+++ b/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
@@ -6525,334 +6525,337 @@ bool ObjectFileMachO::SaveCore(const lldb::ProcessSP &process_sp,
}
if (make_core) {
- CoreFileMemoryRanges core_ranges;
- error = process_sp->CalculateCoreFileSaveRanges(options, core_ranges);
- if (error.Success()) {
- const uint32_t addr_byte_size = target_arch.GetAddressByteSize();
- const ByteOrder byte_order = target_arch.GetByteOrder();
- std::vector<llvm::MachO::segment_command_64> segment_load_commands;
- for (const auto &core_range_info : core_ranges) {
- // TODO: Refactor RangeDataVector to have a data iterator.
- const auto &core_range = core_range_info.data;
- uint32_t cmd_type = LC_SEGMENT_64;
- uint32_t segment_size = sizeof(llvm::MachO::segment_command_64);
- if (addr_byte_size == 4) {
- cmd_type = LC_SEGMENT;
- segment_size = sizeof(llvm::MachO::segment_command);
- }
- // Skip any ranges with no read/write/execute permissions and empty
- // ranges.
- if (core_range.lldb_permissions == 0 || core_range.range.size() == 0)
- continue;
- uint32_t vm_prot = 0;
- if (core_range.lldb_permissions & ePermissionsReadable)
- vm_prot |= VM_PROT_READ;
- if (core_range.lldb_permissions & ePermissionsWritable)
- vm_prot |= VM_PROT_WRITE;
- if (core_range.lldb_permissions & ePermissionsExecutable)
- vm_prot |= VM_PROT_EXECUTE;
- const addr_t vm_addr = core_range.range.start();
- const addr_t vm_size = core_range.range.size();
- llvm::MachO::segment_command_64 segment = {
- cmd_type, // uint32_t cmd;
- segment_size, // uint32_t cmdsize;
- {0}, // char segname[16];
- vm_addr, // uint64_t vmaddr; // uint32_t for 32-bit Mach-O
- vm_size, // uint64_t vmsize; // uint32_t for 32-bit Mach-O
- 0, // uint64_t fileoff; // uint32_t for 32-bit Mach-O
- vm_size, // uint64_t filesize; // uint32_t for 32-bit Mach-O
- vm_prot, // uint32_t maxprot;
- vm_prot, // uint32_t initprot;
- 0, // uint32_t nsects;
- 0}; // uint32_t flags;
- segment_load_commands.push_back(segment);
- }
+ llvm::Expected<CoreFileMemoryRanges> core_ranges_maybe =
+ options.GetMemoryRegionsToSave();
+ if (!core_ranges_maybe) {
+ error = Status::FromError(core_ranges_maybe.takeError());
+ return false;
+ }
- StreamString buffer(Stream::eBinary, addr_byte_size, byte_order);
-
- llvm::MachO::mach_header_64 mach_header;
- mach_header.magic = addr_byte_size == 8 ? MH_MAGIC_64 : MH_MAGIC;
- mach_header.cputype = target_arch.GetMachOCPUType();
- mach_header.cpusubtype = target_arch.GetMachOCPUSubType();
- mach_header.filetype = MH_CORE;
- mach_header.ncmds = segment_load_commands.size();
- mach_header.flags = 0;
- mach_header.reserved = 0;
- ThreadList &thread_list = process_sp->GetThreadList();
- const uint32_t num_threads = thread_list.GetSize();
-
- // Make an array of LC_THREAD data items. Each one contains the
- // contents of the LC_THREAD load command. The data doesn't contain
- // the load command + load command size, we will add the load command
- // and load command size as we emit the data.
- std::vector<StreamString> LC_THREAD_datas(num_threads);
- for (auto &LC_THREAD_data : LC_THREAD_datas) {
- LC_THREAD_data.GetFlags().Set(Stream::eBinary);
- LC_THREAD_data.SetAddressByteSize(addr_byte_size);
- LC_THREAD_data.SetByteOrder(byte_order);
+ CoreFileMemoryRanges &core_ranges = *core_ranges_maybe;
+ const uint32_t addr_byte_size = target_arch.GetAddressByteSize();
+ const ByteOrder byte_order = target_arch.GetByteOrder();
+ std::vector<llvm::MachO::segment_command_64> segment_load_commands;
+ for (const auto &core_range_info : core_ranges) {
+ // TODO: Refactor RangeDataVector to have a data iterator.
+ const auto &core_range = core_range_info.data;
+ uint32_t cmd_type = LC_SEGMENT_64;
+ uint32_t segment_size = sizeof(llvm::MachO::segment_command_64);
+ if (addr_byte_size == 4) {
+ cmd_type = LC_SEGMENT;
+ segment_size = sizeof(llvm::MachO::segment_command);
}
- for (uint32_t thread_idx = 0; thread_idx < num_threads; ++thread_idx) {
- ThreadSP thread_sp(thread_list.GetThreadAtIndex(thread_idx));
- if (thread_sp) {
- switch (mach_header.cputype) {
- case llvm::MachO::CPU_TYPE_ARM64:
- case llvm::MachO::CPU_TYPE_ARM64_32:
- RegisterContextDarwin_arm64_Mach::Create_LC_THREAD(
- thread_sp.get(), LC_THREAD_datas[thread_idx]);
- break;
+ // Skip any ranges with no read/write/execute permissions and empty
+ // ranges.
+ if (core_range.lldb_permissions == 0 || core_range.range.size() == 0)
+ continue;
+ uint32_t vm_prot = 0;
+ if (core_range.lldb_permissions & ePermissionsReadable)
+ vm_prot |= VM_PROT_READ;
+ if (core_range.lldb_permissions & ePermissionsWritable)
+ vm_prot |= VM_PROT_WRITE;
+ if (core_range.lldb_permissions & ePermissionsExecutable)
+ vm_prot |= VM_PROT_EXECUTE;
+ const addr_t vm_addr = core_range.range.start();
+ const addr_t vm_size = core_range.range.size();
+ llvm::MachO::segment_command_64 segment = {
+ cmd_type, // uint32_t cmd;
+ segment_size, // uint32_t cmdsize;
+ {0}, // char segname[16];
+ vm_addr, // uint64_t vmaddr; // uint32_t for 32-bit Mach-O
+ vm_size, // uint64_t vmsize; // uint32_t for 32-bit Mach-O
+ 0, // uint64_t fileoff; // uint32_t for 32-bit Mach-O
+ vm_size, // uint64_t filesize; // uint32_t for 32-bit Mach-O
+ vm_prot, // uint32_t maxprot;
+ vm_prot, // uint32_t initprot;
+ 0, // uint32_t nsects;
+ 0}; // uint32_t flags;
+ segment_load_commands.push_back(segment);
+ }
- case llvm::MachO::CPU_TYPE_ARM:
- RegisterContextDarwin_arm_Mach::Create_LC_THREAD(
- thread_sp.get(), LC_THREAD_datas[thread_idx]);
- break;
+ StreamString buffer(Stream::eBinary, addr_byte_size, byte_order);
+
+ llvm::MachO::mach_header_64 mach_header;
+ mach_header.magic = addr_byte_size == 8 ? MH_MAGIC_64 : MH_MAGIC;
+ mach_header.cputype = target_arch.GetMachOCPUType();
+ mach_header.cpusubtype = target_arch.GetMachOCPUSubType();
+ mach_header.filetype = MH_CORE;
+ mach_header.ncmds = segment_load_commands.size();
+ mach_header.flags = 0;
+ mach_header.reserved = 0;
+ ThreadList &thread_list = process_sp->GetThreadList();
+ const uint32_t num_threads = thread_list.GetSize();
+
+ // Make an array of LC_THREAD data items. Each one contains the
+ // contents of the LC_THREAD load command. The data doesn't contain
+ // the load command + load command size, we will add the load command
+ // and load command size as we emit the data.
+ std::vector<StreamString> LC_THREAD_datas(num_threads);
+ for (auto &LC_THREAD_data : LC_THREAD_datas) {
+ LC_THREAD_data.GetFlags().Set(Stream::eBinary);
+ LC_THREAD_data.SetAddressByteSize(addr_byte_size);
+ LC_THREAD_data.SetByteOrder(byte_order);
+ }
+ for (uint32_t thread_idx = 0; thread_idx < num_threads; ++thread_idx) {
+ ThreadSP thread_sp(thread_list.GetThreadAtIndex(thread_idx));
+ if (thread_sp) {
+ switch (mach_header.cputype) {
+ case llvm::MachO::CPU_TYPE_ARM64:
+ case llvm::MachO::CPU_TYPE_ARM64_32:
+ RegisterContextDarwin_arm64_Mach::Create_LC_THREAD(
+ thread_sp.get(), LC_THREAD_datas[thread_idx]);
+ break;
- case llvm::MachO::CPU_TYPE_X86_64:
- RegisterContextDarwin_x86_64_Mach::Create_LC_THREAD(
- thread_sp.get(), LC_THREAD_datas[thread_idx]);
- break;
+ case llvm::MachO::CPU_TYPE_ARM:
+ RegisterContextDarwin_arm_Mach::Create_LC_THREAD(
+ thread_sp.get(), LC_THREAD_datas[thread_idx]);
+ break;
- case llvm::MachO::CPU_TYPE_RISCV:
- RegisterContextDarwin_riscv32_Mach::Create_LC_THREAD(
- thread_sp.get(), LC_THREAD_datas[thread_idx]);
- break;
- }
- }
- }
+ case llvm::MachO::CPU_TYPE_X86_64:
+ RegisterContextDarwin_x86_64_Mach::Create_LC_THREAD(
+ thread_sp.get(), LC_THREAD_datas[thread_idx]);
+ break;
- // The size of the load command is the size of the segments...
- if (addr_byte_size == 8) {
- mach_header.sizeofcmds = segment_load_commands.size() *
- sizeof(llvm::MachO::segment_command_64);
- } else {
- mach_header.sizeofcmds = segment_load_commands.size() *
- sizeof(llvm::MachO::segment_command);
+ case llvm::MachO::CPU_TYPE_RISCV:
+ RegisterContextDarwin_riscv32_Mach::Create_LC_THREAD(
+ thread_sp.get(), LC_THREAD_datas[thread_idx]);
+ break;
+ }
}
+ }
- // and the size of all LC_THREAD load command
- for (const auto &LC_THREAD_data : LC_THREAD_datas) {
- ++mach_header.ncmds;
- mach_header.sizeofcmds += 8 + LC_THREAD_data.GetSize();
- }
+ // The size of the load command is the size of the segments...
+ if (addr_byte_size == 8) {
+ mach_header.sizeofcmds = segment_load_commands.size() *
+ sizeof(llvm::MachO::segment_command_64);
+ } else {
+ mach_header.sizeofcmds =
+ segment_load_commands.size() * sizeof(llvm::MachO::segment_command);
+ }
- // Bits will be set to indicate which bits are NOT used in
- // addressing in this process or 0 for unknown.
- uint64_t address_mask = process_sp->GetCodeAddressMask();
- if (address_mask != LLDB_INVALID_ADDRESS_MASK) {
- // LC_NOTE "addrable bits"
- mach_header.ncmds++;
- mach_header.sizeofcmds += sizeof(llvm::MachO::note_command);
- }
+ // and the size of all LC_THREAD load command
+ for (const auto &LC_THREAD_data : LC_THREAD_datas) {
+ ++mach_header.ncmds;
+ mach_header.sizeofcmds += 8 + LC_THREAD_data.GetSize();
+ }
- // LC_NOTE "process metadata"
+ // Bits will be set to indicate which bits are NOT used in
+ // addressing in this process or 0 for unknown.
+ uint64_t address_mask = process_sp->GetCodeAddressMask();
+ if (address_mask != LLDB_INVALID_ADDRESS_MASK) {
+ // LC_NOTE "addrable bits"
mach_header.ncmds++;
mach_header.sizeofcmds += sizeof(llvm::MachO::note_command);
+ }
- // LC_NOTE "all image infos"
- mach_header.ncmds++;
- mach_header.sizeofcmds += sizeof(llvm::MachO::note_command);
+ // LC_NOTE "process metadata"
+ mach_header.ncmds++;
+ mach_header.sizeofcmds += sizeof(llvm::MachO::note_command);
+
+ // LC_NOTE "all image infos"
+ mach_header.ncmds++;
+ mach_header.sizeofcmds += sizeof(llvm::MachO::note_command);
+
+ // Write the mach header
+ buffer.PutHex32(mach_header.magic);
+ buffer.PutHex32(mach_header.cputype);
+ buffer.PutHex32(mach_header.cpusubtype);
+ buffer.PutHex32(mach_header.filetype);
+ buffer.PutHex32(mach_header.ncmds);
+ buffer.PutHex32(mach_header.sizeofcmds);
+ buffer.PutHex32(mach_header.flags);
+ if (addr_byte_size == 8) {
+ buffer.PutHex32(mach_header.reserved);
+ }
- // Write the mach header
- buffer.PutHex32(mach_header.magic);
- buffer.PutHex32(mach_header.cputype);
- buffer.PutHex32(mach_header.cpusubtype);
- buffer.PutHex32(mach_header.filetype);
- buffer.PutHex32(mach_header.ncmds);
- buffer.PutHex32(mach_header.sizeofcmds);
- buffer.PutHex32(mach_header.flags);
- if (addr_byte_size == 8) {
- buffer.PutHex32(mach_header.reserved);
- }
+ // Skip the mach header and all load commands and align to the next
+ // 0x1000 byte boundary
+ addr_t file_offset = buffer.GetSize() + mach_header.sizeofcmds;
- // Skip the mach header and all load commands and align to the next
- // 0x1000 byte boundary
- addr_t file_offset = buffer.GetSize() + mach_header.sizeofcmds;
-
- file_offset = llvm::alignTo(file_offset, 16);
- std::vector<std::unique_ptr<LCNoteEntry>> lc_notes;
-
- // Add "addrable bits" LC_NOTE when an address mask is available
- if (address_mask != LLDB_INVALID_ADDRESS_MASK) {
- std::unique_ptr<LCNoteEntry> addrable_bits_lcnote_up(
- new LCNoteEntry(addr_byte_size, byte_order));
- addrable_bits_lcnote_up->name = "addrable bits";
- addrable_bits_lcnote_up->payload_file_offset = file_offset;
- int bits = std::bitset<64>(~address_mask).count();
- addrable_bits_lcnote_up->payload.PutHex32(4); // version
- addrable_bits_lcnote_up->payload.PutHex32(
- bits); // # of bits used for low addresses
- addrable_bits_lcnote_up->payload.PutHex32(
- bits); // # of bits used for high addresses
- addrable_bits_lcnote_up->payload.PutHex32(0); // reserved
-
- file_offset += addrable_bits_lcnote_up->payload.GetSize();
-
- lc_notes.push_back(std::move(addrable_bits_lcnote_up));
- }
+ file_offset = llvm::alignTo(file_offset, 16);
+ std::vector<std::unique_ptr<LCNoteEntry>> lc_notes;
- // Add "process metadata" LC_NOTE
- std::unique_ptr<LCNoteEntry> thread_extrainfo_lcnote_up(
+ // Add "addrable bits" LC_NOTE when an address mask is available
+ if (address_mask != LLDB_INVALID_ADDRESS_MASK) {
+ std::unique_ptr<LCNoteEntry> addrable_bits_lcnote_up(
new LCNoteEntry(addr_byte_size, byte_order));
- thread_extrainfo_lcnote_up->name = "process metadata";
- thread_extrainfo_lcnote_up->payload_file_offset = file_offset;
+ addrable_bits_lcnote_up->name = "addrable bits";
+ addrable_bits_lcnote_up->payload_file_offset = file_offset;
+ int bits = std::bitset<64>(~address_mask).count();
+ addrable_bits_lcnote_up->payload.PutHex32(4); // version
+ addrable_bits_lcnote_up->payload.PutHex32(
+ bits); // # of bits used for low addresses
+ addrable_bits_lcnote_up->payload.PutHex32(
+ bits); // # of bits used for high addresses
+ addrable_bits_lcnote_up->payload.PutHex32(0); // reserved
+
+ file_offset += addrable_bits_lcnote_up->payload.GetSize();
+
+ lc_notes.push_back(std::move(addrable_bits_lcnote_up));
+ }
- StructuredData::DictionarySP dict(
+ // Add "process metadata" LC_NOTE
+ std::unique_ptr<LCNoteEntry> thread_extrainfo_lcnote_up(
+ new LCNoteEntry(addr_byte_size, byte_order));
+ thread_extrainfo_lcnote_up->name = "process metadata";
+ thread_extrainfo_lcnote_up->payload_file_offset = file_offset;
+
+ StructuredData::DictionarySP dict(
+ std::make_shared<StructuredData::Dictionary>());
+ StructuredData::ArraySP threads(
+ std::make_shared<StructuredData::Array>());
+ for (const ThreadSP &thread_sp :
+ process_sp->CalculateCoreFileThreadList(options)) {
+ StructuredData::DictionarySP thread(
std::make_shared<StructuredData::Dictionary>());
- StructuredData::ArraySP threads(
- std::make_shared<StructuredData::Array>());
- for (const ThreadSP &thread_sp :
- process_sp->CalculateCoreFileThreadList(options)) {
- StructuredData::DictionarySP thread(
- std::make_shared<StructuredData::Dictionary>());
- thread->AddIntegerItem("thread_id", thread_sp->GetID());
- threads->AddItem(thread);
- }
- dict->AddItem("threads", threads);
- StreamString strm;
- dict->Dump(strm, /* pretty */ false);
- thread_extrainfo_lcnote_up->payload.PutRawBytes(strm.GetData(),
- strm.GetSize());
-
- file_offset += thread_extrainfo_lcnote_up->payload.GetSize();
- file_offset = llvm::alignTo(file_offset, 16);
- lc_notes.push_back(std::move(thread_extrainfo_lcnote_up));
-
- // Add "all image infos" LC_NOTE
- std::unique_ptr<LCNoteEntry> all_image_infos_lcnote_up(
- new LCNoteEntry(addr_byte_size, byte_order));
- all_image_infos_lcnote_up->name = "all image infos";
- all_image_infos_lcnote_up->payload_file_offset = file_offset;
- file_offset = CreateAllImageInfosPayload(
- process_sp, file_offset, all_image_infos_lcnote_up->payload,
- options);
- lc_notes.push_back(std::move(all_image_infos_lcnote_up));
-
- // Add LC_NOTE load commands
- for (auto &lcnote : lc_notes) {
- // Add the LC_NOTE load command to the file.
- buffer.PutHex32(LC_NOTE);
- buffer.PutHex32(sizeof(llvm::MachO::note_command));
- char namebuf[16];
- memset(namebuf, 0, sizeof(namebuf));
- // This is the uncommon case where strncpy is exactly
- // the right one, doesn't need to be nul terminated.
- // LC_NOTE name field is char[16] and is not guaranteed to be
- // nul-terminated.
- // coverity[buffer_size_warning]
- strncpy(namebuf, lcnote->name.c_str(), sizeof(namebuf));
- buffer.PutRawBytes(namebuf, sizeof(namebuf));
- buffer.PutHex64(lcnote->payload_file_offset);
- buffer.PutHex64(lcnote->payload.GetSize());
- }
+ thread->AddIntegerItem("thread_id", thread_sp->GetID());
+ threads->AddItem(thread);
+ }
+ dict->AddItem("threads", threads);
+ StreamString strm;
+ dict->Dump(strm, /* pretty */ false);
+ thread_extrainfo_lcnote_up->payload.PutRawBytes(strm.GetData(),
+ strm.GetSize());
+
+ file_offset += thread_extrainfo_lcnote_up->payload.GetSize();
+ file_offset = llvm::alignTo(file_offset, 16);
+ lc_notes.push_back(std::move(thread_extrainfo_lcnote_up));
+
+ // Add "all image infos" LC_NOTE
+ std::unique_ptr<LCNoteEntry> all_image_infos_lcnote_up(
+ new LCNoteEntry(addr_byte_size, byte_order));
+ all_image_infos_lcnote_up->name = "all image infos";
+ all_image_infos_lcnote_up->payload_file_offset = file_offset;
+ file_offset = CreateAllImageInfosPayload(
+ process_sp, file_offset, all_image_infos_lcnote_up->payload, options);
+ lc_notes.push_back(std::move(all_image_infos_lcnote_up));
+
+ // Add LC_NOTE load commands
+ for (auto &lcnote : lc_notes) {
+ // Add the LC_NOTE load command to the file.
+ buffer.PutHex32(LC_NOTE);
+ buffer.PutHex32(sizeof(llvm::MachO::note_command));
+ char namebuf[16];
+ memset(namebuf, 0, sizeof(namebuf));
+ // This is the uncommon case where strncpy is exactly
+ // the right one, doesn't need to be nul terminated.
+ // LC_NOTE name field is char[16] and is not guaranteed to be
+ // nul-terminated.
+ // coverity[buffer_size_warning]
+ strncpy(namebuf, lcnote->name.c_str(), sizeof(namebuf));
+ ...
[truncated]
|
|
@JDevlieghere Hey Jonas, this change has a lot of LOC because I converted to a guard clause, because this is just an API change do you prefer if I keep the existing code flow and make the guard clause change in another patch? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Follow up on (#146777), where we expose the memory ranges through the Save Core Options object, and simplify plugin manager to just take an options object with a process set.
Mach-O Save core has a lot of nesting, so I converted this case into a guard clause.