[Lldb-commits] [lldb] 6a7f572 - [LLDB] Fix Memory64 BaseRVA, move all non-stack memory to Mem64. (#146777)
via lldb-commits
lldb-commits at lists.llvm.org
Fri Jul 18 13:05:19 PDT 2025
Author: Jacob Lalonde
Date: 2025-07-18T13:05:15-07:00
New Revision: 6a7f572ef9758f49fcf9e178ce1cb95aa3069415
URL: https://github.com/llvm/llvm-project/commit/6a7f572ef9758f49fcf9e178ce1cb95aa3069415
DIFF: https://github.com/llvm/llvm-project/commit/6a7f572ef9758f49fcf9e178ce1cb95aa3069415.diff
LOG: [LLDB] Fix Memory64 BaseRVA, move all non-stack memory to Mem64. (#146777)
### Context
Over a year ago, I landed support for 64b Memory ranges in Minidump
(#95312). In this patch we added the Memory64 list stream, which is
effectively a Linked List on disk. The layout is a sixteen byte header
and then however many Memory descriptors.
### The Bug
This is a classic off-by one error, where I added 8 bytes instead of 16
for the header. This caused the first region to start 8 bytes before the
correct RVA, thus shifting all memory reads by 8 bytes. We are correctly
writing all the regions to disk correctly, with no physical corruption
but the RVA is defined wrong, meaning we were incorrectly reading memory
### Why wasn't this caught?
One problem we've had is forcing Minidump to actually use the 64b mode,
it would be a massive waste of resources to have a test that actually
wrote >4.2gb of IO to validate the 64b regions, and so almost all
validation has been manual. As a weakness of manual testing, this issue
is psuedo non-deterministic, as what regions end up in 64b or 32b is
handled greedily and iterated in the order it's laid out in
/proc/pid/maps. We often validated 64b was written correctly by
hexdumping the Minidump itself, which was not corrupted (other than the
BaseRVA)
### Why is this showing up now?
During internal usage, we had a bug report that the Minidump wasn't
displaying values. I was unable to repro the issue, but during my
investigation I saw the variables were in the 64b regions which resulted
in me identifying the bug.
### How do we prevent future regressions?
To prevent regressions, and honestly to save my sanity for figuring out
where 8 bytes magically came from, I've added a new API to
SBSaveCoreOptions.
```SBSaveCoreOptions::GetMemoryRegionsToSave()```
The ability to get the memory regions that we intend to include in the Coredump. I added this so we can compare what we intended to include versus what was actually included. Traditionally we've always had issues comparing regions because Minidump includes `/proc/pid/maps` and it can be difficult to know what memoryregion read failure was a genuine error or just a page that wasn't meant to be included.
We are also leveraging this API to choose the memory regions to be generated, as well as for testing what regions should be bytewise 1:1.
After much debate with @clayborg, I've moved all non-stack memory to the Memory64 List. This list doesn't incur us any meaningful overhead and Greg originally suggested doing this in the original 64b PR. This also means we're exercising the 64b path every single time we save a Minidump, preventing regressions on this feature from slipping through testing in the future.
Snippet produced by [minidump.py](https://github.com/clayborg/scripts)
```
MINIDUMP_MEMORY_LIST:
NumberOfMemoryRanges = 0x00000002
MemoryRanges[0] = [0x00007f61085ff9f0 - 0x00007f6108601000) @ 0x0003f655
MemoryRanges[1] = [0x00007ffe47e50910 - 0x00007ffe47e52000) @ 0x00040c65
MINIDUMP_MEMORY64_LIST:
NumberOfMemoryRanges = 0x000000000000002e
BaseRva = 0x0000000000042669
MemoryRanges[0] = [0x00005584162d8000 - 0x00005584162d9000)
MemoryRanges[1] = [0x00005584162d9000 - 0x00005584162db000)
MemoryRanges[2] = [0x00005584162db000 - 0x00005584162dd000)
MemoryRanges[3] = [0x00005584162dd000 - 0x00005584162ff000)
MemoryRanges[4] = [0x00007f6100000000 - 0x00007f6100021000)
MemoryRanges[5] = [0x00007f6108800000 - 0x00007f6108828000)
MemoryRanges[6] = [0x00007f6108828000 - 0x00007f610899d000)
MemoryRanges[7] = [0x00007f610899d000 - 0x00007f61089f9000)
MemoryRanges[8] = [0x00007f61089f9000 - 0x00007f6108a08000)
MemoryRanges[9] = [0x00007f6108bf5000 - 0x00007f6108bf7000)
```
### Misc
As a part of this fix I had to look at LLDB logs a lot, you'll notice I added `0x` to many of the PRIx64 `LLDB_LOGF`. This is so the user (or I) can directly copy paste the address in the logs instead of adding the hex prefix themselves.
Added some SBSaveCore tests for the new GetMemoryAPI, and Docstrings.
CC: @DavidSpickett, @da-viper @labath because we've been working together on save-core plugins, review it optional and I didn't tag you but figured you'd want to know
Added:
lldb/test/API/functionalities/process_save_core_minidump/TestProcessSaveCoreMinidump64b.py
Modified:
lldb/bindings/interface/SBSaveCoreOptionsDocstrings.i
lldb/include/lldb/API/SBMemoryRegionInfoList.h
lldb/include/lldb/API/SBSaveCoreOptions.h
lldb/include/lldb/Core/PluginManager.h
lldb/include/lldb/Symbol/SaveCoreOptions.h
lldb/source/API/SBProcess.cpp
lldb/source/API/SBSaveCoreOptions.cpp
lldb/source/Commands/CommandObjectProcess.cpp
lldb/source/Core/PluginManager.cpp
lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp
lldb/source/Symbol/SaveCoreOptions.cpp
lldb/test/API/python_api/sbsavecoreoptions/TestSBSaveCoreOptions.py
Removed:
################################################################################
diff --git a/lldb/bindings/interface/SBSaveCoreOptionsDocstrings.i b/lldb/bindings/interface/SBSaveCoreOptionsDocstrings.i
index 6907164a1b95c..1df4d2b26212d 100644
--- a/lldb/bindings/interface/SBSaveCoreOptionsDocstrings.i
+++ b/lldb/bindings/interface/SBSaveCoreOptionsDocstrings.i
@@ -45,6 +45,10 @@ Note that currently ELF Core files are not supported."
Resetting will result in the reset of all process specific options, such as Threads to save."
) lldb::SBSaveCoreOptions::SetProcess;
+%feature("docstring", "
+ Get the process to save. If a process is not defined, whether by calling clear or by not setting a process, an invalid process will be returned."
+) lldb::SBSaveCoreOptions::GetProcess;
+
%feature("docstring", "
Add an SBThread to be saved, an error will be returned if an SBThread from a
diff erent process is specified.
The process is set either by the first SBThread added to the options container, or explicitly by the SetProcess call."
@@ -63,6 +67,12 @@ Note that currently ELF Core files are not supported."
Get an SBThreadCollection of all threads marked to be saved. This collection is not sorted according to insertion order."
) lldb::SBSaveCoreOptions::GetThreadsToSave;
+%feature("docstring", "
+ Get an SBMemoryRegionInfoList of all the Regions that LLDB will attempt to write into the Core. Note, reading from these
+ regions can fail, and it's not guaraunteed every region will be present in the resulting core. If called without a valid process or style set an empty
+ collection will be returned."
+) lldb::SBSaveCoreOptions::GetMemoryRegionsToSave;
+
%feature("docstring", "
Get the current total number of bytes the core is expected to have, excluding the overhead of the core file format.
Requires both a Process and a Style to be specified. An error will be returned if the provided options would result in no data being saved."
diff --git a/lldb/include/lldb/API/SBMemoryRegionInfoList.h b/lldb/include/lldb/API/SBMemoryRegionInfoList.h
index 1d939dff55faa..8ac9c1aceb6f6 100644
--- a/lldb/include/lldb/API/SBMemoryRegionInfoList.h
+++ b/lldb/include/lldb/API/SBMemoryRegionInfoList.h
@@ -45,6 +45,7 @@ class LLDB_API SBMemoryRegionInfoList {
private:
friend class SBProcess;
+ friend class SBSaveCoreOptions;
lldb_private::MemoryRegionInfos &ref();
diff --git a/lldb/include/lldb/API/SBSaveCoreOptions.h b/lldb/include/lldb/API/SBSaveCoreOptions.h
index 37552c13d0f36..7b05377966965 100644
--- a/lldb/include/lldb/API/SBSaveCoreOptions.h
+++ b/lldb/include/lldb/API/SBSaveCoreOptions.h
@@ -12,6 +12,7 @@
#include "lldb/API/SBDefines.h"
#include "lldb/API/SBError.h"
#include "lldb/API/SBFileSpec.h"
+#include "lldb/API/SBMemoryRegionInfoList.h"
#include "lldb/API/SBProcess.h"
#include "lldb/API/SBThread.h"
#include "lldb/API/SBThreadCollection.h"
@@ -78,6 +79,13 @@ class LLDB_API SBSaveCoreOptions {
/// api, or implicitly from any function that requires a process.
SBError SetProcess(lldb::SBProcess process);
+ /// Get the process to save, if the process is not set an invalid SBProcess
+ /// will be returned.
+ ///
+ /// \return
+ /// The set process, or an invalid SBProcess if no process is set.
+ SBProcess GetProcess();
+
/// Add a thread to save in the core file.
///
/// \param thread
@@ -119,6 +127,13 @@ class LLDB_API SBSaveCoreOptions {
/// an empty collection will be returned.
SBThreadCollection GetThreadsToSave() const;
+ /// Get an unsorted copy of all memory regions to save
+ ///
+ /// \returns
+ /// An unsorted copy of all memory regions to save. If no process or style
+ /// is specified an empty collection will be returned.
+ SBMemoryRegionInfoList GetMemoryRegionsToSave();
+
/// Get the current total number of bytes the core is expected to have
/// excluding the overhead of the core file format. Requires a Process and
/// Style to be specified.
diff --git a/lldb/include/lldb/Core/PluginManager.h b/lldb/include/lldb/Core/PluginManager.h
index 369785ceea5a5..aa60b7c6693ca 100644
--- a/lldb/include/lldb/Core/PluginManager.h
+++ b/lldb/include/lldb/Core/PluginManager.h
@@ -261,8 +261,7 @@ class PluginManager {
static ObjectFileCreateMemoryInstance
GetObjectFileCreateMemoryCallbackForPluginName(llvm::StringRef name);
- static Status SaveCore(const lldb::ProcessSP &process_sp,
- lldb_private::SaveCoreOptions &core_options);
+ static Status SaveCore(lldb_private::SaveCoreOptions &core_options);
static std::vector<llvm::StringRef> GetSaveCorePluginNames();
diff --git a/lldb/include/lldb/Symbol/SaveCoreOptions.h b/lldb/include/lldb/Symbol/SaveCoreOptions.h
index da66b184745db..697549706ed07 100644
--- a/lldb/include/lldb/Symbol/SaveCoreOptions.h
+++ b/lldb/include/lldb/Symbol/SaveCoreOptions.h
@@ -9,6 +9,7 @@
#ifndef LLDB_SOURCE_PLUGINS_OBJECTFILE_SaveCoreOPTIONS_H
#define LLDB_SOURCE_PLUGINS_OBJECTFILE_SaveCoreOPTIONS_H
+#include "lldb/Target/CoreFileMemoryRanges.h"
#include "lldb/Target/ThreadCollection.h"
#include "lldb/Utility/FileSpec.h"
#include "lldb/Utility/RangeMap.h"
@@ -23,7 +24,7 @@ namespace lldb_private {
class SaveCoreOptions {
public:
- SaveCoreOptions(){};
+ SaveCoreOptions() = default;
~SaveCoreOptions() = default;
lldb_private::Status SetPluginName(const char *name);
@@ -36,17 +37,19 @@ class SaveCoreOptions {
const std::optional<lldb_private::FileSpec> GetOutputFile() const;
Status SetProcess(lldb::ProcessSP process_sp);
+ lldb::ProcessSP GetProcess() { return m_process_sp; }
Status AddThread(lldb::ThreadSP thread_sp);
bool RemoveThread(lldb::ThreadSP thread_sp);
bool ShouldThreadBeSaved(lldb::tid_t tid) const;
bool HasSpecifiedThreads() const;
- Status EnsureValidConfiguration(lldb::ProcessSP process_sp) const;
+ Status EnsureValidConfiguration() const;
const MemoryRanges &GetCoreFileMemoryRanges() const;
void AddMemoryRegionToSave(const lldb_private::MemoryRegionInfo ®ion);
+ llvm::Expected<lldb_private::CoreFileMemoryRanges> GetMemoryRegionsToSave();
lldb_private::ThreadCollection::collection GetThreadsToSave() const;
llvm::Expected<uint64_t> GetCurrentSizeInBytes();
diff --git a/lldb/source/API/SBProcess.cpp b/lldb/source/API/SBProcess.cpp
index 4de5929d6b230..d4be64b815369 100644
--- a/lldb/source/API/SBProcess.cpp
+++ b/lldb/source/API/SBProcess.cpp
@@ -1263,6 +1263,15 @@ lldb::SBError SBProcess::SaveCore(SBSaveCoreOptions &options) {
return error;
}
+ if (!options.GetProcess())
+ options.SetProcess(process_sp);
+
+ if (options.GetProcess().GetSP() != process_sp) {
+ error = Status::FromErrorString(
+ "Save Core Options configured for a
diff erent process.");
+ return error;
+ }
+
std::lock_guard<std::recursive_mutex> guard(
process_sp->GetTarget().GetAPIMutex());
@@ -1271,7 +1280,7 @@ lldb::SBError SBProcess::SaveCore(SBSaveCoreOptions &options) {
return error;
}
- error.ref() = PluginManager::SaveCore(process_sp, options.ref());
+ error.ref() = PluginManager::SaveCore(options.ref());
return error;
}
diff --git a/lldb/source/API/SBSaveCoreOptions.cpp b/lldb/source/API/SBSaveCoreOptions.cpp
index 15584abaac013..e8b81ee57f5a9 100644
--- a/lldb/source/API/SBSaveCoreOptions.cpp
+++ b/lldb/source/API/SBSaveCoreOptions.cpp
@@ -81,6 +81,11 @@ SBError SBSaveCoreOptions::SetProcess(lldb::SBProcess process) {
return m_opaque_up->SetProcess(process.GetSP());
}
+SBProcess SBSaveCoreOptions::GetProcess() {
+ LLDB_INSTRUMENT_VA(this);
+ return SBProcess(m_opaque_up->GetProcess());
+}
+
SBError SBSaveCoreOptions::AddThread(lldb::SBThread thread) {
LLDB_INSTRUMENT_VA(this, thread);
return m_opaque_up->AddThread(thread.GetSP());
@@ -128,6 +133,26 @@ uint64_t SBSaveCoreOptions::GetCurrentSizeInBytes(SBError &error) {
return *expected_bytes;
}
+lldb::SBMemoryRegionInfoList SBSaveCoreOptions::GetMemoryRegionsToSave() {
+ LLDB_INSTRUMENT_VA(this);
+ llvm::Expected<lldb_private::CoreFileMemoryRanges> memory_ranges =
+ m_opaque_up->GetMemoryRegionsToSave();
+ if (!memory_ranges) {
+ llvm::consumeError(memory_ranges.takeError());
+ return SBMemoryRegionInfoList();
+ }
+
+ SBMemoryRegionInfoList memory_region_infos;
+ for (const auto &range : *memory_ranges) {
+ SBMemoryRegionInfo region_info(
+ nullptr, range.GetRangeBase(), range.GetRangeEnd(),
+ range.data.lldb_permissions, /*mapped=*/true);
+ memory_region_infos.Append(region_info);
+ }
+
+ return memory_region_infos;
+}
+
lldb_private::SaveCoreOptions &SBSaveCoreOptions::ref() const {
return *m_opaque_up;
}
diff --git a/lldb/source/Commands/CommandObjectProcess.cpp b/lldb/source/Commands/CommandObjectProcess.cpp
index 1181b2d95c8b4..84c576e721e71 100644
--- a/lldb/source/Commands/CommandObjectProcess.cpp
+++ b/lldb/source/Commands/CommandObjectProcess.cpp
@@ -1354,7 +1354,8 @@ class CommandObjectProcessSaveCore : public CommandObjectParsed {
FileSystem::Instance().Resolve(output_file);
auto &core_dump_options = m_options.m_core_dump_options;
core_dump_options.SetOutputFile(output_file);
- Status error = PluginManager::SaveCore(process_sp, core_dump_options);
+ core_dump_options.SetProcess(process_sp);
+ Status error = PluginManager::SaveCore(core_dump_options);
if (error.Success()) {
if (core_dump_options.GetStyle() ==
SaveCoreStyle::eSaveCoreDirtyOnly ||
diff --git a/lldb/source/Core/PluginManager.cpp b/lldb/source/Core/PluginManager.cpp
index bece690a85fa5..588736715f817 100644
--- a/lldb/source/Core/PluginManager.cpp
+++ b/lldb/source/Core/PluginManager.cpp
@@ -952,27 +952,26 @@ PluginManager::GetObjectFileCreateMemoryCallbackForPluginName(
return nullptr;
}
-Status PluginManager::SaveCore(const lldb::ProcessSP &process_sp,
- lldb_private::SaveCoreOptions &options) {
+Status PluginManager::SaveCore(lldb_private::SaveCoreOptions &options) {
Status error;
if (!options.GetOutputFile()) {
error = Status::FromErrorString("No output file specified");
return error;
}
- if (!process_sp) {
+ if (!options.GetProcess()) {
error = Status::FromErrorString("Invalid process");
return error;
}
- error = options.EnsureValidConfiguration(process_sp);
+ error = options.EnsureValidConfiguration();
if (error.Fail())
return error;
if (!options.GetPluginName().has_value()) {
// Try saving core directly from the process plugin first.
llvm::Expected<bool> ret =
- process_sp->SaveCore(options.GetOutputFile()->GetPath());
+ options.GetProcess()->SaveCore(options.GetOutputFile()->GetPath());
if (!ret)
return Status::FromError(ret.takeError());
if (ret.get())
@@ -984,7 +983,10 @@ Status PluginManager::SaveCore(const lldb::ProcessSP &process_sp,
auto instances = GetObjectFileInstances().GetSnapshot();
for (auto &instance : instances) {
if (plugin_name.empty() || instance.name == plugin_name) {
- if (instance.save_core && instance.save_core(process_sp, options, error))
+ // TODO: Refactor the instance.save_core() to not require a process and
+ // get it from options instead.
+ if (instance.save_core &&
+ instance.save_core(options.GetProcess(), options, error))
return error;
}
}
diff --git a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp
index 806f256d9da48..fe28213c49740 100644
--- a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp
+++ b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp
@@ -836,13 +836,13 @@ Status MinidumpFileBuilder::AddMemoryList() {
// 32 bit memory descriptiors, so we emit them first to ensure the memory is
// in accessible with a 32 bit offset.
std::vector<CoreFileMemoryRange> ranges_32;
- std::vector<CoreFileMemoryRange> ranges_64;
- CoreFileMemoryRanges all_core_memory_ranges;
- error = m_process_sp->CalculateCoreFileSaveRanges(m_save_core_options,
- all_core_memory_ranges);
+ llvm::Expected<CoreFileMemoryRanges> all_core_memory_ranges_maybe =
+ m_save_core_options.GetMemoryRegionsToSave();
+ if (!all_core_memory_ranges_maybe)
+ return Status::FromError(all_core_memory_ranges_maybe.takeError());
- if (error.Fail())
- return error;
+ const CoreFileMemoryRanges &all_core_memory_ranges =
+ *all_core_memory_ranges_maybe;
lldb_private::Progress progress("Saving Minidump File", "",
all_core_memory_ranges.GetSize());
@@ -868,6 +868,10 @@ Status MinidumpFileBuilder::AddMemoryList() {
}
}
+ // The header has to be in 32b memory, as it needs to be addressable by a 32b
+ // RVA. Everything else can be 64b.
+ total_size += sizeof(llvm::minidump::MemoryListHeader);
+
if (total_size >= UINT32_MAX) {
error = Status::FromErrorStringWithFormat(
"Unable to write minidump. Stack memory "
@@ -876,35 +880,15 @@ Status MinidumpFileBuilder::AddMemoryList() {
return error;
}
- // After saving the stacks, we start packing as much as we can into 32b.
- // We apply a generous padding here so that the Directory, MemoryList and
- // Memory64List sections all begin in 32b addressable space.
- // Then anything overflow extends into 64b addressable space.
- // all_core_memory_vec will either contain all stack regions at this point,
- // or be empty if it's a stack only minidump.
- if (!all_core_memory_vec.empty())
- total_size += 256 + (all_core_memory_vec.size() *
- sizeof(llvm::minidump::MemoryDescriptor_64));
-
- for (const auto &core_range : all_core_memory_vec) {
- const addr_t range_size = core_range.range.size();
- // We don't need to check for stacks here because we already removed them
- // from all_core_memory_ranges.
- if (total_size + range_size < UINT32_MAX) {
- ranges_32.push_back(core_range);
- total_size += range_size;
- } else {
- ranges_64.push_back(core_range);
- }
- }
-
+ // Save only the thread stacks to the 32b memory list. Everything else will
+ // get put in Memory64, this simplifies tracking
error = AddMemoryList_32(ranges_32, progress);
if (error.Fail())
return error;
// Add the remaining memory as a 64b range.
- if (!ranges_64.empty()) {
- error = AddMemoryList_64(ranges_64, progress);
+ if (!all_core_memory_ranges.IsEmpty()) {
+ error = AddMemoryList_64(all_core_memory_vec, progress);
if (error.Fail())
return error;
}
@@ -977,6 +961,7 @@ Status MinidumpFileBuilder::ReadWriteMemoryInChunks(
const lldb::addr_t addr = range.range.start();
const lldb::addr_t size = range.range.size();
Log *log = GetLog(LLDBLog::Object);
+ uint64_t total_bytes_read = 0;
Status addDataError;
Process::ReadMemoryChunkCallback callback =
[&](Status &error, lldb::addr_t current_addr, const void *buf,
@@ -984,7 +969,7 @@ Status MinidumpFileBuilder::ReadWriteMemoryInChunks(
if (error.Fail() || bytes_read == 0) {
LLDB_LOGF(log,
"Failed to read memory region at: 0x%" PRIx64
- ". Bytes read: %" PRIx64 ", error: %s",
+ ". Bytes read: 0x%" PRIx64 ", error: %s",
current_addr, bytes_read, error.AsCString());
// If we failed in a memory read, we would normally want to skip
@@ -997,6 +982,21 @@ Status MinidumpFileBuilder::ReadWriteMemoryInChunks(
return lldb_private::IterationAction::Stop;
}
+ if (current_addr != addr + total_bytes_read) {
+ LLDB_LOGF(log,
+ "Current addr is at unexpected address, 0x%" PRIx64
+ ", expected at 0x%" PRIx64,
+ current_addr, addr + total_bytes_read);
+
+ // Something went wrong and the address is not where it should be
+ // we'll error out of this Minidump generation.
+ addDataError = Status::FromErrorStringWithFormat(
+ "Unexpected address encounterd when reading memory in chunks "
+ "0x%" PRIx64 " expected 0x%" PRIx64,
+ current_addr, addr + total_bytes_read);
+ return lldb_private::IterationAction::Stop;
+ }
+
// Write to the minidump file with the chunk potentially flushing to
// disk.
// This error will be captured by the outer scope and is considered fatal.
@@ -1006,13 +1006,13 @@ Status MinidumpFileBuilder::ReadWriteMemoryInChunks(
if (addDataError.Fail())
return lldb_private::IterationAction::Stop;
+ total_bytes_read += bytes_read;
// If we have a partial read, report it, but only if the partial read
// didn't finish reading the entire region.
- if (bytes_read != data_buffer.GetByteSize() &&
- current_addr + bytes_read != size) {
+ if (bytes_read != data_buffer.GetByteSize() && total_bytes_read != size) {
LLDB_LOGF(log,
- "Memory region at: %" PRIx64 " partiall read 0x%" PRIx64
- " bytes out of %" PRIx64 " bytes.",
+ "Memory region at: 0x%" PRIx64 " partial read 0x%" PRIx64
+ " bytes out of 0x%" PRIx64 " bytes.",
current_addr, bytes_read,
data_buffer.GetByteSize() - bytes_read);
@@ -1059,7 +1059,7 @@ MinidumpFileBuilder::AddMemoryList_32(std::vector<CoreFileMemoryRange> &ranges,
LLDB_LOGF(log,
"AddMemoryList %zu/%zu reading memory for region "
- "(%" PRIx64 " bytes) [%" PRIx64 ", %" PRIx64 ")",
+ "(0x%" PRIx64 " bytes) [0x%" PRIx64 ", 0x%" PRIx64 ")",
region_index, ranges.size(), size, addr, addr + size);
++region_index;
@@ -1117,7 +1117,7 @@ MinidumpFileBuilder::AddMemoryList_64(std::vector<CoreFileMemoryRange> &ranges,
return error;
error = AddDirectory(StreamType::Memory64List,
- (sizeof(llvm::support::ulittle64_t) * 2) +
+ (sizeof(llvm::minidump::Memory64ListHeader)) +
ranges.size() *
sizeof(llvm::minidump::MemoryDescriptor_64));
if (error.Fail())
@@ -1130,9 +1130,9 @@ MinidumpFileBuilder::AddMemoryList_64(std::vector<CoreFileMemoryRange> &ranges,
// Capture the starting offset for all the descriptors so we can clean them up
// if needed.
offset_t starting_offset =
- GetCurrentDataEndOffset() + sizeof(llvm::support::ulittle64_t);
+ GetCurrentDataEndOffset() + sizeof(llvm::minidump::Memory64ListHeader);
// The base_rva needs to start after the directories, which is right after
- // this 8 byte variable.
+ // the descriptors + the size of the header.
offset_t base_rva =
starting_offset +
(ranges.size() * sizeof(llvm::minidump::MemoryDescriptor_64));
diff --git a/lldb/source/Symbol/SaveCoreOptions.cpp b/lldb/source/Symbol/SaveCoreOptions.cpp
index f93b58f59cf96..6d762a66181cf 100644
--- a/lldb/source/Symbol/SaveCoreOptions.cpp
+++ b/lldb/source/Symbol/SaveCoreOptions.cpp
@@ -124,16 +124,14 @@ void SaveCoreOptions::AddMemoryRegionToSave(
const MemoryRanges &SaveCoreOptions::GetCoreFileMemoryRanges() const {
return m_regions_to_save;
}
-Status
-SaveCoreOptions::EnsureValidConfiguration(lldb::ProcessSP process_sp) const {
+Status SaveCoreOptions::EnsureValidConfiguration() const {
Status error;
std::string error_str;
if (!m_threads_to_save.empty() && GetStyle() == lldb::eSaveCoreFull)
error_str += "Cannot save a full core with a subset of threads\n";
- if (m_process_sp && m_process_sp != process_sp)
- error_str += "Cannot save core for process using supplied core options. "
- "Options were constructed targeting a
diff erent process. \n";
+ if (!m_process_sp)
+ error_str += "Need to assign a valid process\n";
if (!error_str.empty())
error = Status(error_str);
@@ -155,12 +153,30 @@ SaveCoreOptions::GetThreadsToSave() const {
return thread_collection;
}
+llvm::Expected<lldb_private::CoreFileMemoryRanges>
+SaveCoreOptions::GetMemoryRegionsToSave() {
+ Status error;
+ if (!m_process_sp)
+ return Status::FromErrorString("Requires a process to be set.").takeError();
+
+ error = EnsureValidConfiguration();
+ if (error.Fail())
+ return error.takeError();
+
+ CoreFileMemoryRanges ranges;
+ error = m_process_sp->CalculateCoreFileSaveRanges(*this, ranges);
+ if (error.Fail())
+ return error.takeError();
+
+ return ranges;
+}
+
llvm::Expected<uint64_t> SaveCoreOptions::GetCurrentSizeInBytes() {
Status error;
if (!m_process_sp)
return Status::FromErrorString("Requires a process to be set.").takeError();
- error = EnsureValidConfiguration(m_process_sp);
+ error = EnsureValidConfiguration();
if (error.Fail())
return error.takeError();
@@ -169,8 +185,14 @@ llvm::Expected<uint64_t> SaveCoreOptions::GetCurrentSizeInBytes() {
if (error.Fail())
return error.takeError();
+ llvm::Expected<lldb_private::CoreFileMemoryRanges> core_file_ranges_maybe =
+ GetMemoryRegionsToSave();
+ if (!core_file_ranges_maybe)
+ return core_file_ranges_maybe.takeError();
+ const lldb_private::CoreFileMemoryRanges &core_file_ranges =
+ *core_file_ranges_maybe;
uint64_t total_in_bytes = 0;
- for (auto &core_range : ranges)
+ for (const auto &core_range : core_file_ranges)
total_in_bytes += core_range.data.range.size();
return total_in_bytes;
diff --git a/lldb/test/API/functionalities/process_save_core_minidump/TestProcessSaveCoreMinidump64b.py b/lldb/test/API/functionalities/process_save_core_minidump/TestProcessSaveCoreMinidump64b.py
new file mode 100644
index 0000000000000..b86b69c8399f2
--- /dev/null
+++ b/lldb/test/API/functionalities/process_save_core_minidump/TestProcessSaveCoreMinidump64b.py
@@ -0,0 +1,102 @@
+"""
+Test that saved memory regions is byte-wise 1:1 with the live process. Specifically
+that the memory regions that will be populated in the Memory64List are the same byte for byte.
+"""
+
+import os
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+
+class ProcessSaveCoreMinidump64bTestCase(TestBase):
+ def verify_minidump(
+ self,
+ options,
+ ):
+ """Verify that the minidump is the same byte for byte as the live process."""
+ self.build()
+ exe = self.getBuildArtifact("a.out")
+ target = self.dbg.CreateTarget(exe)
+ core_target = None
+ live_proc = target.LaunchSimple(
+ None, None, self.get_process_working_directory()
+ )
+ try:
+ self.assertState(live_proc.GetState(), lldb.eStateStopped)
+ error = live_proc.SaveCore(options)
+ self.assertTrue(error.Success(), error.GetCString())
+ core_target = self.dbg.CreateTarget(None)
+ core_proc = target.LoadCore(options.GetOutputFile().fullpath)
+ # Get the memory regions we saved off in this core, we can't compare to the core
+ # because we pull from /proc/pid/maps, so even ranges that don't get mapped in will show up
+ # as ranges in the minidump.
+ #
+ # Instead, we have an API that returns to us the number of regions we planned to save from the live process
+ # and we compare those
+ memory_regions_to_compare = options.GetMemoryRegionsToSave()
+
+ for region in memory_regions_to_compare:
+ start_addr = region.GetRegionBase()
+ end_addr = region.GetRegionEnd()
+ actual_process_read_error = lldb.SBError()
+ actual = live_proc.ReadMemory(
+ start_addr, end_addr - start_addr, actual_process_read_error
+ )
+ expected_process_read_error = lldb.SBError()
+ expected = core_proc.ReadMemory(
+ start_addr, end_addr - start_addr, expected_process_read_error
+ )
+
+ # Both processes could fail to read a given memory region, so if they both pass
+ # compare, then we'll fail them if the core
diff ers from the live process.
+ if (
+ actual_process_read_error.Success()
+ and expected_process_read_error.Success()
+ ):
+ self.assertEqual(
+ actual, expected, "Bytes
diff er between live process and core"
+ )
+
+ # Now we check if the error is the same, error isn't abnormal but they should fail for the same reason
+ # Success will be false if they both fail
+ self.assertTrue(
+ actual_process_read_error.Success()
+ == expected_process_read_error.Success(),
+ f"Address range {hex(start_addr)} - {hex(end_addr)} failed to read from live process and core for
diff erent reasons",
+ )
+ finally:
+ self.assertTrue(self.dbg.DeleteTarget(target))
+ if core_target is not None:
+ self.assertTrue(self.dbg.DeleteTarget(core_target))
+
+ @skipUnlessArch("x86_64")
+ @skipUnlessPlatform(["linux"])
+ def test_minidump_save_style_full(self):
+ """Test that a full minidump is the same byte for byte."""
+ minidump_path = self.getBuildArtifact("minidump_full_force64b.dmp")
+ try:
+ options = lldb.SBSaveCoreOptions()
+ options.SetOutputFile(lldb.SBFileSpec(minidump_path))
+ options.SetStyle(lldb.eSaveCoreFull)
+ options.SetPluginName("minidump")
+ self.verify_minidump(options)
+ finally:
+ if os.path.isfile(minidump_path):
+ os.unlink(minidump_path)
+
+ @skipUnlessArch("x86_64")
+ @skipUnlessPlatform(["linux"])
+ def test_minidump_save_style_mixed_memory(self):
+ """Test that a mixed memory minidump is the same byte for byte."""
+ minidump_path = self.getBuildArtifact("minidump_mixed_force64b.dmp")
+ try:
+ options = lldb.SBSaveCoreOptions()
+ options.SetOutputFile(lldb.SBFileSpec(minidump_path))
+ options.SetStyle(lldb.eSaveCoreDirtyOnly)
+ options.SetPluginName("minidump")
+ self.verify_minidump(options)
+ finally:
+ if os.path.isfile(minidump_path):
+ os.unlink(minidump_path)
diff --git a/lldb/test/API/python_api/sbsavecoreoptions/TestSBSaveCoreOptions.py b/lldb/test/API/python_api/sbsavecoreoptions/TestSBSaveCoreOptions.py
index 31e35e0285f17..92ca44ecbbffc 100644
--- a/lldb/test/API/python_api/sbsavecoreoptions/TestSBSaveCoreOptions.py
+++ b/lldb/test/API/python_api/sbsavecoreoptions/TestSBSaveCoreOptions.py
@@ -164,3 +164,46 @@ def test_get_total_in_bytes_missing_requirements(self):
options.SetStyle(lldb.eSaveCoreCustomOnly)
total = options.GetCurrentSizeInBytes(error)
self.assertTrue(error.Fail(), error.GetCString())
+
+ def test_get_memory_regions_to_save(self):
+ """
+ Tests the matrix of responses for GetMemoryRegionsToSave
+ """
+
+ options = lldb.SBSaveCoreOptions()
+
+ # Not specifying plugin or process should return an empty list.
+ memory_list = options.GetMemoryRegionsToSave()
+ self.assertEqual(0, memory_list.GetSize())
+
+ # No style returns an empty list
+ process = self.get_basic_process()
+ options.SetProcess(process)
+ memory_list = options.GetMemoryRegionsToSave()
+ self.assertEqual(0, memory_list.GetSize())
+ options.Clear()
+
+ # No Process returns an empty list
+ options.SetStyle(lldb.eSaveCoreCustomOnly)
+ memory_list = options.GetMemoryRegionsToSave()
+ self.assertEqual(0, memory_list.GetSize())
+ options.Clear()
+
+ # Validate we get back the single region we populate
+ options.SetStyle(lldb.eSaveCoreCustomOnly)
+ process = self.get_basic_process()
+ options.SetProcess(process)
+ memory_range = lldb.SBMemoryRegionInfo()
+
+ # Add the memory range of 0x1000-0x1100
+ process.GetMemoryRegionInfo(0x1000, memory_range)
+ options.AddMemoryRegionToSave(memory_range)
+ memory_list = options.GetMemoryRegionsToSave()
+ self.assertEqual(1, memory_list.GetSize())
+ read_region = lldb.SBMemoryRegionInfo()
+ memory_list.GetMemoryRegionAtIndex(0, read_region)
+
+ # Permissions from Process getLLDBRegion aren't matching up with
+ # the live process permissions, so we're just checking the range for now.
+ self.assertEqual(memory_range.GetRegionBase(), read_region.GetRegionBase())
+ self.assertEqual(memory_range.GetRegionEnd(), read_region.GetRegionEnd())
More information about the lldb-commits
mailing list