Centralize the code that figures out which memory ranges to save into core files #71772
Conversation
clayborg
requested review from
bulbazord,
kusmour,
jeffreytan81,
jimingham and
jasonmolenda
|
@llvm/pr-subscribers-lldb Author: Greg Clayton (clayborg) ChangesPrior 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: 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:
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]
|
You can test this locally with the following command:git-clang-format --diff d0da3d8393939790bb1a6b3b5a36daeeef000c9b 5b368d756ed631064d7836e9f0b5d580c427cf55 -- lldb/include/lldb/Target/MemoryRegionInfo.h lldb/include/lldb/Target/Process.h lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.cpp lldb/source/Target/Process.cpp lldb/source/Target/TraceDumper.cppView the diff from clang-format here.diff --git a/lldb/source/Target/Process.cpp b/lldb/source/Target/Process.cpp
index 21b80b8240ab..ee96c3e85f33 100644
--- a/lldb/source/Target/Process.cpp
+++ b/lldb/source/Target/Process.cpp
@@ -6311,7 +6311,7 @@ static void GetCoreFileSaveRangesFull(Process &process,
Process::CoreFileMemoryRanges &ranges) {
// Don't add only dirty pages, add full regions.
-const bool try_dirty_pages = false;
+ const bool try_dirty_pages = false;
for (const auto ®ion : regions)
AddRegion(region, try_dirty_pages, ranges);
}
|
There was a problem hiding this comment.
A couple of small nits but looks good to me. I do have a little concern about pulling in llvm/ADT/AddressRanges.h in Process.h to get llvm::AddressRange when we have an lldb_private::AddressRange, which is used in a number of places in lldb. I'm surprised you only needed to add lldb_private:: namespace qualifications in that TraceDumper after adding the llvm header in Process.h. I don't have a good suggestion here but it seems like the kind of thing that's going to annoy someone once a year or so when they stumble on the ambiguity. :)
lldb/include/lldb/Target/Process.h
Outdated
| 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 |
lldb/source/Target/Process.cpp
Outdated
| } | ||
|
|
||
| // Also check with our threads and get the regions for their stack pointers | ||
| // and add those regions if not already added above |
There was a problem hiding this comment.
This is a clever idea. Add a period at the end of the comment.
lldb/source/Target/Process.cpp
Outdated
| // and add those regions if not already added above | ||
| ThreadList &thread_list = process.GetThreadList(); | ||
| const size_t num_threads = thread_list.GetSize(); | ||
| for (size_t idx=0; idx<num_threads; ++idx) { |
There was a problem hiding this comment.
ThreadList has an interable interface, couldn't you do for (ThreadSP &thread_sp : thread_list.Threads())? I haven't actually tried this.
| Status CalculateCoreFileSaveRanges(lldb::SaveCoreStyle core_style, | ||
| CoreFileMemoryRanges &ranges); | ||
|
|
There was a problem hiding this comment.
What do you think of something like llvm::Expected<CoreFileMemoryRanges> instead? Status is kind of easy to ignore in terms of error handling and you won't need the out parameter.
There was a problem hiding this comment.
I did that initially but then it was always just converted to Status in the end so it seemed like using Expected when we didn't need to. I am happy to change to Expected<> if you really think it should be that way
| // 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, |
There was a problem hiding this comment.
It seems like there are multiple reasons why we may return false but false itself doesn't really accurately capture what went wrong. Maybe we can return an llvm::Error here?
There was a problem hiding this comment.
We aren't going to propagate the error, we just need to know if it succeeded or not, so I didn't have it return an error. Otherwise every single range from a linux lldb-server will return an error "dirty pages not supported". For Darwin we added special extra features to track dirty pages, but no one else has these. Again, I can add an error here, but I will just end up consuming the error because lack of dirty page information isn't going to stop a core file from being created. Also many regions on mac will not have any dirty pages if they don't have write permissions, so most regions would end up returning an error, which again, we won't do anything with, we will just ignore.
lldb/source/Target/Process.cpp
Outdated
| const bool try_dirty_pages = false; | ||
| for (const auto ®ion: regions) | ||
| AddRegion(region, try_dirty_pages, ranges); | ||
| return Status(); |
There was a problem hiding this comment.
Why does this function return a Status? Doesn't seem like it actually does much here?
There was a problem hiding this comment.
Each of the GetCoreFileSaveRanges* funtions all have the same return value to keep it consistent. When we ask explicitly for dirty pages only with the eSaveCoreDirtyOnly core style, that function returns an error if dirty pages are not supported by any regions. Not much can go wrong in this function. I can remove the Status return value if needed.
| static void AddRegion(const MemoryRegionInfo ®ion, bool try_dirty_pages, | ||
| Process::CoreFileMemoryRanges &ranges) { |
There was a problem hiding this comment.
Seems like you'd want this class to return something? There seems to be multiple failure conditions here
There was a problem hiding this comment.
If we have empty ranges or a memory region without permissions, we don't need to add it to the core file, and this function will determine if it needs to add the region to the list of regions to save to the core file. The user won't care or need to know about any of the error conditions.
| const addr_t addr = region.GetRange().GetRangeBase(); | ||
| llvm::AddressRange range(addr, addr + region.GetRange().GetByteSize()); |
There was a problem hiding this comment.
Do you think it'd be useful to be able to create an AddressRange from a MemoryRegionInfo object?
There was a problem hiding this comment.
This is an llvm::AddressRange here. We also have lldb_private::AddressRange which contains a lldb_private::Address (section offset address) + byte size. The llvm::AddressRange just has a start address and end address. I could add accessors to MemoryRegionInfo for these, but as Jason already eluded to we have llvm::AddressRange and lldb_private::AddressRange and if we include both header files in MemoryRegionInfo.h, we might end up with more qualifications needed on bare AddressRange types in source files.
| const addr_t page_size = region.GetPageSize(); | ||
| if (page_size == 0) | ||
| return false; |
There was a problem hiding this comment.
Why might the page size be 0? If the MemoryRegionInfo is "invalid"?
| range = llvm::AddressRange(page_addr, page_addr + page_size); | ||
| } else { | ||
| if (range.end() == page_addr) { | ||
| // Combine consective ranges. |
| // given region. If the region has dirty page information, only dirty pages | ||
| // will be added to \a ranges, else the entire range will be added to \a | ||
| // ranges. | ||
| static void AddRegion(const MemoryRegionInfo ®ion, bool try_dirty_pages, |
There was a problem hiding this comment.
I think try_dirty_pages is kind of a confusing name. The intent of the parameter is to only add dirty pages if AddDirtyPages succeeds right? What do you think about dirty_pages_only? Why might you want to try only dirty pages and do the regular thing only if it fails?
There was a problem hiding this comment.
try_dirty_pages is saying "if you can add only the dirty pages from the current region because the process plugin supports it, then please add those, if not please add the full region. If you note in the code below we only try to add dirty pages if we are asked to when try_dirty_pages == true
lldb/source/Target/Process.cpp
Outdated
| // doesn't support reporting stack memory. This function does unique the stack | ||
| // regions and won't add the same range twice. This function also attempts to |
There was a problem hiding this comment.
I think you can drop the part about uniquing stack regions and just say that it won't add the same range twice.
lldb/source/Target/Process.cpp
Outdated
| AddRegion(sp_region, try_dirty_pages, ranges); | ||
| } | ||
| } | ||
| return Status(); |
There was a problem hiding this comment.
This also always succeeds, why return a Status at all?
clayborg
force-pushed
the
core-style-unified
branch
from
9ff1e92 to
feea395
Compare
|
I addressed most of the feedback. Alex let me know if you still really want llvm::Error and llvm::Expected to be used as I can add that if you think it is required. I also ran clang format. |
|
@jasonmolenda I was wondering if we should modify Right now we have |
|
@bulbazord let me know if you require any changes after reading my inline comments. |
I think your answers make sense to me. I don't think you need to add them here since it all eventually turns into Status objects anyway... I generally prefer it over Status though. Thanks for checking. 👍 |
Another good idea. Do we get page permissions like that on Linux? I'm curious what system it would work on. I tried a full memory coredump on darwin and there ARE some regions that are marked |
You can test this locally with the following command:darker --check --diff -r d0da3d8393939790bb1a6b3b5a36daeeef000c9b..5b368d756ed631064d7836e9f0b5d580c427cf55 lldb/test/API/functionalities/process_save_core_minidump/TestProcessSaveCoreMinidump.pyView the diff from darker here.--- TestProcessSaveCoreMinidump.py 2023-11-11 19:16:35.000000 +0000
+++ TestProcessSaveCoreMinidump.py 2023-11-11 19:30:44.634320 +0000
@@ -9,13 +9,13 @@
from lldbsuite.test.lldbtest import *
from lldbsuite.test import lldbutil
class ProcessSaveCoreMinidumpTestCase(TestBase):
-
- def verify_core_file(self, core_path, expected_pid, expected_modules,
- expected_threads):
+ def verify_core_file(
+ self, core_path, expected_pid, expected_modules, expected_threads
+ ):
# To verify, we'll launch with the mini dump
target = self.dbg.CreateTarget(None)
process = target.LoadCore(core_path)
# check if the core is in desired state
@@ -75,54 +75,51 @@
thread_id = thread.GetThreadID()
expected_threads.append(thread_id)
# save core and, kill process and verify corefile existence
base_command = "process save-core --plugin-name=minidump "
- self.runCmd(
- base_command + " --style=stack '%s'" % (core_stack)
+ self.runCmd(base_command + " --style=stack '%s'" % (core_stack))
+ self.assertTrue(os.path.isfile(core_stack))
+ self.verify_core_file(
+ core_stack, expected_pid, expected_modules, expected_threads
)
- self.assertTrue(os.path.isfile(core_stack))
- self.verify_core_file(core_stack, expected_pid, expected_modules,
- expected_threads)
- self.runCmd(
- base_command + " --style=modified-memory '%s'" % (core_dirty)
+ self.runCmd(base_command + " --style=modified-memory '%s'" % (core_dirty))
+ self.assertTrue(os.path.isfile(core_dirty))
+ self.verify_core_file(
+ core_dirty, expected_pid, expected_modules, expected_threads
)
- self.assertTrue(os.path.isfile(core_dirty))
- self.verify_core_file(core_dirty, expected_pid, expected_modules,
- expected_threads)
- self.runCmd(
- base_command + " --style=full '%s'" % (core_full)
+ self.runCmd(base_command + " --style=full '%s'" % (core_full))
+ self.assertTrue(os.path.isfile(core_full))
+ self.verify_core_file(
+ core_full, expected_pid, expected_modules, expected_threads
)
- self.assertTrue(os.path.isfile(core_full))
- self.verify_core_file(core_full, expected_pid, expected_modules,
- expected_threads)
# validate saving via SBProcess
- error = process.SaveCore(core_sb_stack, "minidump",
- lldb.eSaveCoreStackOnly)
+ error = process.SaveCore(core_sb_stack, "minidump", lldb.eSaveCoreStackOnly)
self.assertTrue(error.Success())
self.assertTrue(os.path.isfile(core_sb_stack))
- self.verify_core_file(core_sb_stack, expected_pid,
- expected_modules, expected_threads)
+ self.verify_core_file(
+ core_sb_stack, expected_pid, expected_modules, expected_threads
+ )
- error = process.SaveCore(core_sb_dirty, "minidump",
- lldb.eSaveCoreDirtyOnly)
+ error = process.SaveCore(core_sb_dirty, "minidump", lldb.eSaveCoreDirtyOnly)
self.assertTrue(error.Success())
self.assertTrue(os.path.isfile(core_sb_dirty))
- self.verify_core_file(core_sb_dirty, expected_pid, expected_modules,
- expected_threads)
+ self.verify_core_file(
+ core_sb_dirty, expected_pid, expected_modules, expected_threads
+ )
# Minidump can now save full core files, but they will be huge and
# they might cause this test to timeout.
- error = process.SaveCore(core_sb_full, "minidump",
- lldb.eSaveCoreFull)
+ error = process.SaveCore(core_sb_full, "minidump", lldb.eSaveCoreFull)
self.assertTrue(error.Success())
self.assertTrue(os.path.isfile(core_sb_full))
- self.verify_core_file(core_sb_full, expected_pid, expected_modules,
- expected_threads)
+ self.verify_core_file(
+ core_sb_full, expected_pid, expected_modules, expected_threads
+ )
self.assertSuccess(process.Kill())
finally:
# Clean up the mini dump file.
self.assertTrue(self.dbg.DeleteTarget(target))
|
… core files. 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.
- If we are saving core style with eSaveCoreDirtyOnly and process doesn't support dirty ranges, save all regions with write permssions as a backup. - Modify the TestProcessSaveCoreMinidump.py test case to handle minidump now able to handle eSaveCoreDirtyOnly and eSaveCoreFull
… dirty page support is detected and do full testing on minidumps.
clayborg
force-pushed
the
core-style-unified
branch
from
5b368d7 to
62397fd
Compare
|
Hello @clayborg ! Looks like this patch broke lldb-aarch64-windows bot: https://lab.llvm.org/buildbot/#/builders/219/builds/6868 |
I looked at this and I didn't touch anything related to native windows core file saving. If this is a native windows build on arm64 for lldb, and we are debugging with COFF files, then this code will get run: The |
|
Does anyone have access to a native windows arm64 build of LLDB that could help look at this? |
|
@clayborg |
If you end up being able to reproduce, if you can send the minidump file that was produced I should be able to look at it to make sure it was not produced by the core file saver. But if this is debugging a COFF file, it should only be using the native windows API to produce a core file and then load it. And I don't see any changes in this PR that should affect this, but you never know! |
|
core_dmps.tar.gz |
It does indeed seem like my new core file creation code is being triggered on windows. I am working on a fix that adds ARM64 support to the built in minidump creator. |
This patch adds support for saving minidumps with the arm64 architecture. It also will cause unsupported architectures to emit an error where before this patch it would emit a minidump with partial information. This new code is tested by the arm64 windows buildbot that was failing: https://lab.llvm.org/buildbot/#/builders/219/builds/6868 This is needed following this PR: llvm#71772
|
@clayborg yes sure I will test this on the same machine |
This patch adds support for saving minidumps with the arm64 architecture. It also will cause unsupported architectures to emit an error where before this patch it would emit a minidump with partial information. This new code is tested by the arm64 windows buildbot that was failing: https://lab.llvm.org/buildbot/#/builders/219/builds/6868 This is needed following this PR: #71772
… core files (llvm#71772) 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.
…2315) This patch adds support for saving minidumps with the arm64 architecture. It also will cause unsupported architectures to emit an error where before this patch it would emit a minidump with partial information. This new code is tested by the arm64 windows buildbot that was failing: https://lab.llvm.org/buildbot/#/builders/219/builds/6868 This is needed following this PR: llvm#71772
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:
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.