[Lldb-commits] [lldb] Centralize the code that figures out which memory ranges to save into core files (PR #71772)
via lldb-commits
lldb-commits at lists.llvm.org
Wed Nov 8 21:20:22 PST 2023
llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-lldb
Author: Greg Clayton (clayborg)
<details>
<summary>Changes</summary>
Prior to this patch, each core file plugin (ObjectFileMachO.cpp and ObjectFileMinindump.cpp) would calculate the address ranges to save in different ways. This patch adds a new function to Process.h/.cpp:
```
Status Process::CalculateCoreFileSaveRanges(lldb::SaveCoreStyle core_style, CoreFileMemoryRanges &ranges);
```
The patch updates the ObjectFileMachO::SaveCore(...) and ObjectFileMinindump::SaveCore(...) to use same code. This will allow core files to be consistent with the lldb::SaveCoreStyle across different core file creators and will allow us to add new core file saving features that do more complex things in future patches.
---
Patch is 27.10 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/71772.diff
8 Files Affected:
- (modified) lldb/include/lldb/Target/MemoryRegionInfo.h (+4-4)
- (modified) lldb/include/lldb/Target/Process.h (+37-6)
- (modified) lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp (+28-109)
- (modified) lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp (+9-28)
- (modified) lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h (+2-1)
- (modified) lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.cpp (+4-5)
- (modified) lldb/source/Target/Process.cpp (+186-10)
- (modified) lldb/source/Target/TraceDumper.cpp (+4-3)
``````````diff
diff --git a/lldb/include/lldb/Target/MemoryRegionInfo.h b/lldb/include/lldb/Target/MemoryRegionInfo.h
index 47d4c9d6398728c..66a4b3ed1b42d5f 100644
--- a/lldb/include/lldb/Target/MemoryRegionInfo.h
+++ b/lldb/include/lldb/Target/MemoryRegionInfo.h
@@ -81,11 +81,11 @@ class MemoryRegionInfo {
// lldb::Permissions
uint32_t GetLLDBPermissions() const {
uint32_t permissions = 0;
- if (m_read)
+ if (m_read == eYes)
permissions |= lldb::ePermissionsReadable;
- if (m_write)
+ if (m_write == eYes)
permissions |= lldb::ePermissionsWritable;
- if (m_execute)
+ if (m_execute == eYes)
permissions |= lldb::ePermissionsExecutable;
return permissions;
}
@@ -151,7 +151,7 @@ class MemoryRegionInfo {
int m_pagesize = 0;
std::optional<std::vector<lldb::addr_t>> m_dirty_pages;
};
-
+
inline bool operator<(const MemoryRegionInfo &lhs,
const MemoryRegionInfo &rhs) {
return lhs.GetRange() < rhs.GetRange();
diff --git a/lldb/include/lldb/Target/Process.h b/lldb/include/lldb/Target/Process.h
index a6d3e6c2d16926e..335cb64768a682d 100644
--- a/lldb/include/lldb/Target/Process.h
+++ b/lldb/include/lldb/Target/Process.h
@@ -54,6 +54,7 @@
#include "lldb/Utility/UserIDResolver.h"
#include "lldb/lldb-private.h"
+#include "llvm/ADT/AddressRanges.h"
#include "llvm/ADT/ArrayRef.h"
#include "llvm/Support/Threading.h"
#include "llvm/Support/VersionTuple.h"
@@ -354,11 +355,11 @@ class Process : public std::enable_shared_from_this<Process>,
};
// This is all the event bits the public process broadcaster broadcasts.
// The process shadow listener signs up for all these bits...
- static constexpr int g_all_event_bits = eBroadcastBitStateChanged
+ static constexpr int g_all_event_bits = eBroadcastBitStateChanged
| eBroadcastBitInterrupt
- | eBroadcastBitSTDOUT
+ | eBroadcastBitSTDOUT
| eBroadcastBitSTDERR
- | eBroadcastBitProfileData
+ | eBroadcastBitProfileData
| eBroadcastBitStructuredData;
enum {
@@ -390,7 +391,7 @@ class Process : public std::enable_shared_from_this<Process>,
ConstString &GetBroadcasterClass() const override {
return GetStaticBroadcasterClass();
}
-
+
/// A notification structure that can be used by clients to listen
/// for changes in a process's lifetime.
///
@@ -579,7 +580,7 @@ class Process : public std::enable_shared_from_this<Process>,
/// of CommandObject like CommandObjectRaw, CommandObjectParsed,
/// or CommandObjectMultiword.
virtual CommandObject *GetPluginCommandObject() { return nullptr; }
-
+
/// The underlying plugin might store the low-level communication history for
/// this session. Dump it into the provided stream.
virtual void DumpPluginHistory(Stream &s) { return; }
@@ -614,7 +615,7 @@ class Process : public std::enable_shared_from_this<Process>,
return error;
}
- /// The "ShadowListener" for a process is just an ordinary Listener that
+ /// The "ShadowListener" for a process is just an ordinary Listener that
/// listens for all the Process event bits. It's convenient because you can
/// specify it in the LaunchInfo or AttachInfo, so it will get events from
/// the very start of the process.
@@ -704,7 +705,37 @@ class Process : public std::enable_shared_from_this<Process>,
/// is not supported by the plugin, error otherwise.
virtual llvm::Expected<bool> SaveCore(llvm::StringRef outfile);
+ struct CoreFileMemoryRange {
+ llvm::AddressRange range; /// The address range to save into the core file.
+ uint32_t lldb_permissions; /// A bit set of lldb::Permissions bits.
+
+ bool operator==(const CoreFileMemoryRange &rhs) const {
+ return range == rhs.range && lldb_permissions == rhs.lldb_permissions;
+ }
+
+ bool operator!=(const CoreFileMemoryRange &rhs) const {
+ return !(*this == rhs);
+ }
+
+ bool operator<(const CoreFileMemoryRange &rhs) const {
+ if (range < rhs.range)
+ return true;
+ if (range == rhs.range)
+ return lldb_permissions < rhs.lldb_permissions;
+ return false;
+ }
+ };
+
+ using CoreFileMemoryRanges = std::vector<CoreFileMemoryRange>;
+
+ /// Helper function for Process::SaveCore(...) that calculates the address
+ /// ranges that should be save. This allows all core file plug-ins to save
+ /// consistent memory ranges given a \a core_style.
+ Status CalculateCoreFileSaveRanges(lldb::SaveCoreStyle core_style,
+ CoreFileMemoryRanges &ranges);
+
protected:
+
virtual JITLoaderList &GetJITLoaders();
public:
diff --git a/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp b/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
index 3714e37ec5c57d0..73b338639b13163 100644
--- a/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
+++ b/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
@@ -6474,9 +6474,8 @@ bool ObjectFileMachO::SaveCore(const lldb::ProcessSP &process_sp,
return false;
// Default on macOS is to create a dirty-memory-only corefile.
- if (core_style == SaveCoreStyle::eSaveCoreUnspecified) {
+ if (core_style == SaveCoreStyle::eSaveCoreUnspecified)
core_style = SaveCoreStyle::eSaveCoreDirtyOnly;
- }
Target &target = process_sp->GetTarget();
const ArchSpec target_arch = target.GetArchitecture();
@@ -6505,115 +6504,42 @@ bool ObjectFileMachO::SaveCore(const lldb::ProcessSP &process_sp,
}
if (make_core) {
- std::vector<llvm::MachO::segment_command_64> segment_load_commands;
- // uint32_t range_info_idx = 0;
- MemoryRegionInfo range_info;
- Status range_error = process_sp->GetMemoryRegionInfo(0, range_info);
- const uint32_t addr_byte_size = target_arch.GetAddressByteSize();
- const ByteOrder byte_order = target_arch.GetByteOrder();
- std::vector<page_object> pages_to_copy;
-
- if (range_error.Success()) {
- while (range_info.GetRange().GetRangeBase() != LLDB_INVALID_ADDRESS) {
- // Calculate correct protections
- uint32_t prot = 0;
- if (range_info.GetReadable() == MemoryRegionInfo::eYes)
- prot |= VM_PROT_READ;
- if (range_info.GetWritable() == MemoryRegionInfo::eYes)
- prot |= VM_PROT_WRITE;
- if (range_info.GetExecutable() == MemoryRegionInfo::eYes)
- prot |= VM_PROT_EXECUTE;
-
- const addr_t addr = range_info.GetRange().GetRangeBase();
- const addr_t size = range_info.GetRange().GetByteSize();
-
- if (size == 0)
- break;
-
- bool include_this_region = true;
- bool dirty_pages_only = false;
- if (core_style == SaveCoreStyle::eSaveCoreStackOnly) {
- dirty_pages_only = true;
- if (range_info.IsStackMemory() != MemoryRegionInfo::eYes) {
- include_this_region = false;
- }
- }
- if (core_style == SaveCoreStyle::eSaveCoreDirtyOnly) {
- dirty_pages_only = true;
- }
-
- if (prot != 0 && include_this_region) {
- addr_t pagesize = range_info.GetPageSize();
- const std::optional<std::vector<addr_t>> &dirty_page_list =
- range_info.GetDirtyPageList();
- if (dirty_pages_only && dirty_page_list) {
- for (addr_t dirtypage : *dirty_page_list) {
- page_object obj;
- obj.addr = dirtypage;
- obj.size = pagesize;
- obj.prot = prot;
- pages_to_copy.push_back(obj);
- }
- } else {
- page_object obj;
- obj.addr = addr;
- obj.size = size;
- obj.prot = prot;
- pages_to_copy.push_back(obj);
- }
- }
-
- range_error = process_sp->GetMemoryRegionInfo(
- range_info.GetRange().GetRangeEnd(), range_info);
- if (range_error.Fail())
- break;
- }
-
- // 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;
- last_obj.size = 0;
- for (page_object obj : pages_to_copy) {
- if (last_obj.addr == LLDB_INVALID_ADDRESS) {
- last_obj = obj;
- continue;
- }
- if (last_obj.addr + last_obj.size == obj.addr &&
- last_obj.prot == obj.prot) {
- last_obj.size += obj.size;
- continue;
- }
- combined_page_objects.push_back(last_obj);
- last_obj = obj;
- }
- // Add the last entry we were looking to combine
- // on to the array.
- if (last_obj.addr != LLDB_INVALID_ADDRESS && last_obj.size != 0)
- combined_page_objects.push_back(last_obj);
-
- for (page_object obj : combined_page_objects) {
+ Process::CoreFileMemoryRanges core_ranges;
+ error = process_sp->CalculateCoreFileSaveRanges(core_style, 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 : core_ranges) {
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];
- obj.addr, // uint64_t vmaddr; // uint32_t for 32-bit
- // Mach-O
- obj.size, // uint64_t vmsize; // uint32_t for 32-bit
- // Mach-O
- 0, // uint64_t fileoff; // uint32_t for 32-bit Mach-O
- obj.size, // uint64_t filesize; // uint32_t for 32-bit
- // Mach-O
- obj.prot, // uint32_t maxprot;
- obj.prot, // uint32_t initprot;
+ 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);
@@ -6622,11 +6548,7 @@ bool ObjectFileMachO::SaveCore(const lldb::ProcessSP &process_sp,
StreamString buffer(Stream::eBinary, addr_byte_size, byte_order);
llvm::MachO::mach_header_64 mach_header;
- if (addr_byte_size == 8) {
- mach_header.magic = MH_MAGIC_64;
- } else {
- mach_header.magic = MH_MAGIC;
- }
+ 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;
@@ -6911,9 +6833,6 @@ bool ObjectFileMachO::SaveCore(const lldb::ProcessSP &process_sp,
}
}
}
- } else {
- error.SetErrorString(
- "process doesn't support getting memory region info");
}
}
return true; // This is the right plug to handle saving core files for
diff --git a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp
index c396cb061c01776..e8e0d09b5324d0f 100644
--- a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp
+++ b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp
@@ -557,43 +557,24 @@ Status MinidumpFileBuilder::AddException(const lldb::ProcessSP &process_sp) {
}
lldb_private::Status
-MinidumpFileBuilder::AddMemoryList(const lldb::ProcessSP &process_sp) {
+MinidumpFileBuilder::AddMemoryList(const lldb::ProcessSP &process_sp,
+ lldb::SaveCoreStyle core_style) {
Status error;
-
+ Process::CoreFileMemoryRanges core_ranges;
+ error = process_sp->CalculateCoreFileSaveRanges(core_style, core_ranges);
if (error.Fail()) {
error.SetErrorString("Process doesn't support getting memory region info.");
return error;
}
- // Get interesting addresses
- std::vector<size_t> interesting_addresses;
- auto thread_list = process_sp->GetThreadList();
- for (size_t i = 0; i < thread_list.GetSize(); ++i) {
- ThreadSP thread_sp(thread_list.GetThreadAtIndex(i));
- RegisterContextSP reg_ctx_sp(thread_sp->GetRegisterContext());
- RegisterContext *reg_ctx = reg_ctx_sp.get();
-
- interesting_addresses.push_back(read_register_u64(reg_ctx, "rsp"));
- interesting_addresses.push_back(read_register_u64(reg_ctx, "rip"));
- }
-
DataBufferHeap helper_data;
std::vector<MemoryDescriptor> mem_descriptors;
-
- std::set<addr_t> visited_region_base_addresses;
- for (size_t interesting_address : interesting_addresses) {
- MemoryRegionInfo range_info;
- error = process_sp->GetMemoryRegionInfo(interesting_address, range_info);
- // Skip failed memory region requests or any regions with no permissions.
- if (error.Fail() || range_info.GetLLDBPermissions() == 0)
- continue;
- const addr_t addr = range_info.GetRange().GetRangeBase();
- // Skip any regions we have already saved out.
- if (visited_region_base_addresses.insert(addr).second == false)
- continue;
- const addr_t size = range_info.GetRange().GetByteSize();
- if (size == 0)
+ for (const auto &core_range : core_ranges) {
+ // Skip empty memory regions or any regions with no permissions.
+ if (core_range.range.empty() || core_range.lldb_permissions == 0)
continue;
+ const addr_t addr = core_range.range.start();
+ const addr_t size = core_range.range.size();
auto data_up = std::make_unique<DataBufferHeap>(size, 0);
const size_t bytes_read =
process_sp->ReadMemory(addr, data_up->GetBytes(), size, error);
diff --git a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h
index f4017fb663840ec..cae355799fa7247 100644
--- a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h
+++ b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h
@@ -64,7 +64,8 @@ class MinidumpFileBuilder {
// failed status.
lldb_private::Status AddException(const lldb::ProcessSP &process_sp);
// Add MemoryList stream, containing dumps of important memory segments
- lldb_private::Status AddMemoryList(const lldb::ProcessSP &process_sp);
+ lldb_private::Status AddMemoryList(const lldb::ProcessSP &process_sp,
+ lldb::SaveCoreStyle core_style);
// Add MiscInfo stream, mainly providing ProcessId
void AddMiscInfo(const lldb::ProcessSP &process_sp);
// Add informative files about a Linux process
diff --git a/lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.cpp b/lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.cpp
index 17b37afe557d914..f5294b2f08c66e1 100644
--- a/lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.cpp
+++ b/lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.cpp
@@ -57,10 +57,9 @@ bool ObjectFileMinidump::SaveCore(const lldb::ProcessSP &process_sp,
const lldb_private::FileSpec &outfile,
lldb::SaveCoreStyle &core_style,
lldb_private::Status &error) {
- if (core_style != SaveCoreStyle::eSaveCoreStackOnly) {
- error.SetErrorString("Only stack minidumps supported yet.");
- return false;
- }
+ // Set default core style if it isn't set.
+ if (core_style == SaveCoreStyle::eSaveCoreUnspecified)
+ core_style = SaveCoreStyle::eSaveCoreStackOnly;
if (!process_sp)
return false;
@@ -88,7 +87,7 @@ bool ObjectFileMinidump::SaveCore(const lldb::ProcessSP &process_sp,
if (error.Fail())
return false;
- error = builder.AddMemoryList(process_sp);
+ error = builder.AddMemoryList(process_sp, core_style);
if (error.Fail())
return false;
}
diff --git a/lldb/source/Target/Process.cpp b/lldb/source/Target/Process.cpp
index f82ab05362fbee9..4277555e52195e0 100644
--- a/lldb/source/Target/Process.cpp
+++ b/lldb/source/Target/Process.cpp
@@ -2404,16 +2404,7 @@ bool Process::GetLoadAddressPermissions(lldb::addr_t load_addr,
range_info.GetExecutable() == MemoryRegionInfo::eDontKnow) {
return false;
}
-
- if (range_info.GetReadable() == MemoryRegionInfo::eYes)
- permissions |= lldb::ePermissionsReadable;
-
- if (range_info.GetWritable() == MemoryRegionInfo::eYes)
- permissions |= lldb::ePermissionsWritable;
-
- if (range_info.GetExecutable() == MemoryRegionInfo::eYes)
- permissions |= lldb::ePermissionsExecutable;
-
+ permissions = range_info.GetLLDBPermissions();
return true;
}
@@ -6252,3 +6243,188 @@ Status Process::WriteMemoryTags(lldb::addr_t addr, size_t len,
return DoWriteMemoryTags(addr, len, tag_manager->GetAllocationTagType(),
*packed_tags);
}
+
+// Create a CoreFileMemoryRange from a MemoryRegionInfo
+static Process::CoreFileMemoryRange
+CreateCoreFileMemoryRange(const MemoryRegionInfo ®ion) {
+ const addr_t addr = region.GetRange().GetRangeBase();
+ llvm::AddressRange range(addr, addr + region.GetRange().GetByteSize());
+ return {range, region.GetLLDBPermissions()};
+}
+
+// Add dirty pages to the core file ranges and return true if dirty pages
+// were added. Return false if the dirty page information is not valid or in
+// the region.
+static bool AddDirtyPages(const MemoryRegionInfo ®ion,
+ Process::CoreFileMemoryRanges &ranges) {
+ const auto &dirty_page_list = region.GetDirtyPageList();
+ if (!dirty_page_list)
+ return false;
+ const uint32_t lldb_permissions = region.GetLLDBPermissions();
+ const addr_t page_size = region.GetPageSize();
+ if (page_size == 0)
+ return false;
+ llvm::AddressRange range(0, 0);
+ for (addr_t page_addr : *dirty_page_list) {
+ if (range.empty()) {
+ // No range yet, initialize the range with the current dirty page.
+ range = llvm::AddressRange(page_addr, page_addr + page_size);
+ } else {
+ if (range.end() == page_addr) {
+ // Combine consective ranges.
+ range = llvm::AddressRange(range.start(), page_addr + page_size);
+ } else {
+ // Add previous contiguous range and init the new range with the
+ // current dirty page.
+ ranges.push_back({range, lldb_permissions});
+ range = llvm::AddressRange(page_addr, page_addr + page_size);
+ }
+ }
+ }
+ // The last range
+ if (!range.empty())
+ ranges.push_back({range, lldb_permiss...
[truncated]
``````````
</details>
https://github.com/llvm/llvm-project/pull/71772
More information about the lldb-commits
mailing list