Skip to content

Centralize the code that figures out which memory ranges to save into core files #71772

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Nov 11, 2023
Merged

Centralize the code that figures out which memory ranges to save into core files #71772

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f1bd931
Centralize the code that figures out which memory ranges to save into…
clayborg Nov 9, 2023
5e87a05
Make changes according to feedback
clayborg Nov 10, 2023
62397fd
Make eSaveCoreDirtyOnly save all regions with write permissions if no…
clayborg Nov 11, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions lldb/include/lldb/Target/MemoryRegionInfo.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down Expand Up @@ -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();
Expand Down
46 changes: 37 additions & 9 deletions lldb/include/lldb/Target/Process.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -354,12 +355,10 @@ 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
| eBroadcastBitInterrupt
| eBroadcastBitSTDOUT
| eBroadcastBitSTDERR
| eBroadcastBitProfileData
| eBroadcastBitStructuredData;
static constexpr int g_all_event_bits =
eBroadcastBitStateChanged | eBroadcastBitInterrupt | eBroadcastBitSTDOUT |
eBroadcastBitSTDERR | eBroadcastBitProfileData |
eBroadcastBitStructuredData;

enum {
eBroadcastInternalStateControlStop = (1 << 0),
Expand Down Expand Up @@ -390,7 +389,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.
///
Expand Down Expand Up @@ -579,7 +578,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; }
Expand Down Expand Up @@ -614,7 +613,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.
Expand Down Expand Up @@ -704,6 +703,35 @@ 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 saved. 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);

Comment on lines +732 to +734
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

protected:
virtual JITLoaderList &GetJITLoaders();

Expand Down
137 changes: 28 additions & 109 deletions lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6476,9 +6476,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();
Expand Down Expand Up @@ -6507,115 +6506,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);
Expand All @@ -6624,11 +6550,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;
Expand Down Expand Up @@ -6913,9 +6835,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
Expand Down
37 changes: 9 additions & 28 deletions lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
3 changes: 2 additions & 1 deletion lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
9 changes: 4 additions & 5 deletions lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
}
Expand Down
Loading