[LLDB] Fix Incorrect offset for Memory64 RVA in Minidump (+ Testing changes) #146777
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…t true, and fix where we write the base RVA 8 bytes earlier than it starts
|
@llvm/pr-subscribers-lldb Author: Jacob Lalonde (Jlalond) ChangesContextOver 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 BugThis 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 actual memory by 8 bytes. However because this artifically introduced 8 bytes to the first region, all subsequent regions remained aligned and thus uncorrupted
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, because which regions that end up as the first 64b region is up to chance. We try to fit all regions into 32b, only greedily moving regions in 64b if adding them to the 32b bucket would cause an overflow. Additionally, the order matters, and we effectively implicity get the order from
Why is this showing up now?In #138206, as a part of my effort on SBSaveCore, I fixed some bugs where super regions and subregions were not being merged correctly. My hypothesis is because we're now merging adjacent regions with the same permission the chance of a valid data containing region ending up as the first region increased. Additionally, as I work on this we've had more users to report these errors that just appeared 'random' prior. How do we prevent future regressions?To prevent regressions, and honestly to save my sanity for figuring out where 4 bytes magically came from, I've added two new APIs to SBSaveCoreOptions.
```SBSaveCoreOptions::AddFlag()`` Leveraging these two new APIs, I added a new test class that creates a minidump and compares all the regions byte per byte to ensure we don't regress on the 64b region again. Snippet produced by minidump.py Showing we are forcing 64b on the Test Minidump. Can we fix existing MinidumpsYes. None of the data itself is corrupted, and we're simply reading the first element 8 byte too early. I don't think we can automate this, but any Minidump produced with the LLDB stream on a date before this patch lands can have it's RVA updated by 8 and will work fine. I want to repeat, we were not corrupting the data, simply the read pointer is 8 bytes short of where it should begin. MiscAs a part of this fix I had to look at LLDB logs a lot, you'll notice I added 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 Patch is 20.14 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/146777.diff 10 Files Affected:
diff --git a/lldb/bindings/interface/SBSaveCoreOptionsDocstrings.i b/lldb/bindings/interface/SBSaveCoreOptionsDocstrings.i
index 6907164a1b95c..a44c0569f40e6 100644
--- a/lldb/bindings/interface/SBSaveCoreOptionsDocstrings.i
+++ b/lldb/bindings/interface/SBSaveCoreOptionsDocstrings.i
@@ -63,6 +63,18 @@ 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 guaraunteed every region will be present. If called without a valid process or style set an empty
+ collection will be returned."
+) lldb::SBSaveCoreOptions::GetMemoryRegionsToSave;
+
+%feature("docstring", "
+ Add a plugin specific flag to the objects option."
+) lldb::SBSaveCoreOptions::AddFlag;
+
+
%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..e72dd53780ab9 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"
@@ -119,6 +120,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.
@@ -132,6 +140,14 @@ class LLDB_API SBSaveCoreOptions {
/// The expected size of the data contained in the core in bytes.
uint64_t GetCurrentSizeInBytes(SBError &error);
+ /// Add a flag to be consumed by the specified plugin, null or empty flags
+ /// will be ignored.
+ ///
+ /// \note
+ /// This API is currently only used for testing, with forcing Minidumps to
+ /// to 64b memory list the reason this api was added
+ void AddFlag(const char *flag);
+
/// Reset all options.
void Clear();
diff --git a/lldb/include/lldb/Symbol/SaveCoreOptions.h b/lldb/include/lldb/Symbol/SaveCoreOptions.h
index da66b184745db..92e474c1131a1 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);
@@ -47,10 +48,15 @@ class SaveCoreOptions {
void AddMemoryRegionToSave(const lldb_private::MemoryRegionInfo ®ion);
+ llvm::Expected<lldb_private::CoreFileMemoryRanges> GetMemoryRegionsToSave();
lldb_private::ThreadCollection::collection GetThreadsToSave() const;
llvm::Expected<uint64_t> GetCurrentSizeInBytes();
+ void AddFlag(const char *flag);
+
+ bool ContainsFlag(const char *flag) const;
+
void Clear();
private:
@@ -59,6 +65,7 @@ class SaveCoreOptions {
std::optional<std::string> m_plugin_name;
std::optional<lldb_private::FileSpec> m_file;
std::optional<lldb::SaveCoreStyle> m_style;
+ std::optional<std::unordered_set<std::string>> m_flags;
lldb::ProcessSP m_process_sp;
std::unordered_set<lldb::tid_t> m_threads_to_save;
MemoryRanges m_regions_to_save;
diff --git a/lldb/source/API/SBSaveCoreOptions.cpp b/lldb/source/API/SBSaveCoreOptions.cpp
index 15584abaac013..0d618bc1916b9 100644
--- a/lldb/source/API/SBSaveCoreOptions.cpp
+++ b/lldb/source/API/SBSaveCoreOptions.cpp
@@ -128,6 +128,32 @@ 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 m_memory_region_infos;
+ for (const auto &range : *memory_ranges) {
+ SBMemoryRegionInfo region_info(
+ nullptr, range.GetRangeBase(), range.GetRangeEnd(),
+ range.data.lldb_permissions, /*mapped=*/true);
+ m_memory_region_infos.Append(region_info);
+ }
+
+ return m_memory_region_infos;
+}
+
+void SBSaveCoreOptions::AddFlag(const char *flag) {
+ LLDB_INSTRUMENT_VA(this, flag);
+
+ m_opaque_up->AddFlag(flag);
+}
+
lldb_private::SaveCoreOptions &SBSaveCoreOptions::ref() const {
return *m_opaque_up;
}
diff --git a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp
index 806f256d9da48..8d8d04d1b1aba 100644
--- a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp
+++ b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp
@@ -831,6 +831,13 @@ Status MinidumpFileBuilder::AddLinuxFileStreams() {
Status MinidumpFileBuilder::AddMemoryList() {
Status error;
+ // Note this is here for testing. In the past there has been many occasions
+ // that the 64b code has regressed because it's wasteful and expensive to
+ // write a 4.2gb+ on every CI run to get around this and to exercise this
+ // codepath we define a flag in the options object.
+ bool force_64b_for_non_threads =
+ m_save_core_options.ContainsFlag(FORCE_64B_FLAG);
+
// We first save the thread stacks to ensure they fit in the first UINT32_MAX
// bytes of the core file. Thread structures in minidump files can only use
// 32 bit memory descriptiors, so we emit them first to ensure the memory is
@@ -890,7 +897,7 @@ Status MinidumpFileBuilder::AddMemoryList() {
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) {
+ if (!force_64b_for_non_threads && total_size + range_size < UINT32_MAX) {
ranges_32.push_back(core_range);
total_size += range_size;
} else {
@@ -977,6 +984,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 +992,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 +1005,13 @@ Status MinidumpFileBuilder::ReadWriteMemoryInChunks(
return lldb_private::IterationAction::Stop;
}
+ if (current_addr != addr + total_bytes_read) {
+ LLDB_LOGF(log,
+ "Current addr is at expected address, 0x%" PRIx64
+ ", expected at 0x%" PRIx64,
+ current_addr, addr + total_bytes_read);
+ }
+
// 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 +1021,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 +1074,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;
@@ -1130,9 +1145,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/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h
index 46b20f90138fe..cc2520d7f0b16 100644
--- a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h
+++ b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h
@@ -176,6 +176,8 @@ class MinidumpFileBuilder {
static constexpr size_t HEADER_SIZE = sizeof(llvm::minidump::Header);
static constexpr size_t DIRECTORY_SIZE = sizeof(llvm::minidump::Directory);
+ static constexpr const char FORCE_64B_FLAG[] = "force_64b";
+
// More that one place can mention the register thread context locations,
// so when we emit the thread contents, remember where it is so we don't have
// to duplicate it in the exception data.
diff --git a/lldb/source/Symbol/SaveCoreOptions.cpp b/lldb/source/Symbol/SaveCoreOptions.cpp
index f93b58f59cf96..6b6387f821590 100644
--- a/lldb/source/Symbol/SaveCoreOptions.cpp
+++ b/lldb/source/Symbol/SaveCoreOptions.cpp
@@ -155,6 +155,24 @@ 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(m_process_sp);
+ 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)
@@ -169,8 +187,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 (auto &core_range : core_file_ranges)
total_in_bytes += core_range.data.range.size();
return total_in_bytes;
@@ -190,3 +214,17 @@ void SaveCoreOptions::Clear() {
m_process_sp.reset();
m_regions_to_save.Clear();
}
+
+void SaveCoreOptions::AddFlag(const char *flag) {
+ if (!flag || !flag[0])
+ return;
+
+ if (!m_flags)
+ m_flags = std::unordered_set<std::string>();
+
+ m_flags->emplace(std::string(flag));
+}
+
+bool SaveCoreOptions::ContainsFlag(const char *flag) const {
+ return m_flags && m_flags->find(flag) != m_flags->end();
+}
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..b1ce13a047439
--- /dev/null
+++ b/lldb/test/API/functionalities/process_save_core_minidump/TestProcessSaveCoreMinidump64b.py
@@ -0,0 +1,100 @@
+"""
+Test saving a minidumps with the force 64b flag, and evaluate that every
+saved memory region is byte-wise 1:1 with the live process.
+"""
+
+import os
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+# Constant from MinidumpFileBuilder.h, this forces 64b for non threads
+FORCE_64B = "force_64b"
+
+
+class ProcessSaveCoreMinidump64bTestCase(TestBase):
+
+ def verify_minidump(
+ self,
+ core_proc,
+ live_proc,
+ options,
+ ):
+ """Verify that the minidump is the same byte for byte as the live process."""
+ # 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 differs from the live process.
+ if (
+ actual_process_read_error.Success()
+ and expected_process_read_error.Success()
+ ):
+ self.assertEqual(
+ actual, expected, "Bytes differ 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
+ self.assertTrue(
+ (
+ actual_process_read_error.Success()
+ and expected_process_read_error.Success()
+ )
+ or (
+ actual_process_read_error.Fail()
+ and expected_process_read_error.Fail()
+ )
+ )
+
+ @skipUnlessArch("x86_64")
+ @skipUnlessPlatform(["linux"])
+ def test_minidump_save_style_full(self):
+ """Test that a full minidump is the same byte for byte."""
+
+ self.build()
+ exe = self.getBuildArtifact("a.out")
+ minidump_path = self.getBuildArtifact("minidump_full_force64b.dmp")
+
+ try:
+ target = self.dbg.CreateTarget(exe)
+ live_process = target.LaunchSimple(
+ None, None, self.get_process_working_directory()
+ )
+ self.assertState(live_process.GetState(), lldb.eStateStopped)
+ options = lldb.SBSaveCoreOptions()
+
+ options.SetOutputFile(lldb.SBFileSpec(minidump_path))
+ options.SetStyle(lldb.eSaveCoreFull)
+ options.SetPluginName("minidump")
+ options.SetProcess(live_process)
+ options.AddFlag(FORCE_64B)
+
+ error = live_process.SaveCore(options)
+ self.assertTrue(error.Success(), error.GetCString())
+
+ target = self.dbg.CreateTarget(None)
+ core_proc = target.LoadCore(minidump_path)
+
+ self.verify_minidump(core_proc, live_process, options)
+ finally:
+ self.assertTrue(self.dbg.DeleteTarget(target))
+ 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(mem...
[truncated]
|
|
✅ With the latest revision this PR passed the Python code formatter. |
Jlalond
changed the title
|
Can you please split the PR into two? One for improving testing/diagnosibility, the other for the real fix? |
|
@jeffreytan81 Sure, but I'll have to revisit after I return from vacation |
|
Alternative proposal and one @clayborg and I talked about for awhile. Why don't we just put all the non-stacks into Memory64List? Originally I didn't want to change this out of not being familiar with how everything works, but now that we have a 64b list I think it makes sense to fill mem32 with everything that can only support 32b offsets then fill the 64b list. This removes the need to pass flags (which is not a design decision I like but I didn't see many alternatives). @labath any opposition to putting everything in mem64? |
I don't have an opinion on that. I'm not currently involved in minidumps. What I am not excited about is the idea of creating stringly-typed forever stable API for the purpose of testing a fix like this. As for alternatives, an "obvious" one is a unit tests. If you manage (I think it should be possible) instantiate the minidump writer with a mock process, then you can call any API you want, such as one that writes a 64-bit region (but still pass it a small memory block). |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 inspecting 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 two new APIs 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/mapsand returns the number of regions there as theSBMemoryRegionInfoListinstead of the content.SBSaveCoreOptions::AddFlag()I added a new API to define custom, plugin specific flags when creating your save core options. I'm leveraging this to explicitly force MinidumpFileBuilder to populate all non-thread memory regions into the 64b region. Note, threads have to be in 32b because the Minidump Thread object only supports a 32b offset.
Leveraging these two new APIs, I added a new test class that creates a minidump and compares all the regions byte per byte to ensure we don't regress on the 64b region again.
Snippet produced by minidump.py
Showing we are forcing 64b on the Test Minidump.
Can we fix existing Minidumps
Yes. None of the data itself is corrupted, and we're simply reading the first element 8 byte too early. We can even automate this for any Minidump produced with the LLDB stream on a date before this patch lands can have it's RVA updated by 8 and will work fine. I want to repeat, we were not corrupting the data, simply the read pointer is 8 bytes short of where it should begin.
Misc
As a part of this fix I had to look at LLDB logs a lot, you'll notice I added
0xto many of the PRIx64LLDB_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