https://github.com/Jlalond created https://github.com/llvm/llvm-project/pull/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 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 `/proc/pid/maps`. There are often smaller pages between the larger heap regions, and those regions were often the first region. With no variables pointing there it wasn't apparent we corrupted the data.  ### 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::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 returns the number of regions there as the `SBMemoryRegionInfoList` instead of the content. ```SBSaveCoreOptions::AddFlag()`` I added a new API to define custom, plugin specific, flags when defining 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](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) ``` 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. 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. ### 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. 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 >From d17473cc32acb31935759012ca87342d750d68f7 Mon Sep 17 00:00:00 2001 From: Jacob Lalonde <jalalo...@fb.com> Date: Wed, 2 Jul 2025 09:18:59 -0700 Subject: [PATCH 1/7] Fix logs, prevent accidentally printing a partial read when that's not true, and fix where we write the base RVA 8 bytes earlier than it starts --- .../Minidump/MinidumpFileBuilder.cpp | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp index 806f256d9da48..34a71f41f3b84 100644 --- a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp +++ b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp @@ -977,6 +977,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 +985,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 +998,10 @@ 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 +1011,14 @@ 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) { + 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 +1065,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,7 +1136,7 @@ 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. offset_t base_rva = >From cbb5ee7914c201323730b73dee9d0394f012673c Mon Sep 17 00:00:00 2001 From: Jacob Lalonde <jalalo...@fb.com> Date: Wed, 2 Jul 2025 11:01:03 -0700 Subject: [PATCH 2/7] Add new iterator to SBSaveCoreOptions to enable better test asserts --- .../include/lldb/API/SBMemoryRegionInfoList.h | 1 + lldb/include/lldb/API/SBSaveCoreOptions.h | 8 ++++ lldb/include/lldb/Symbol/SaveCoreOptions.h | 2 + lldb/source/API/SBSaveCoreOptions.cpp | 19 ++++++++ .../Minidump/MinidumpFileBuilder.cpp | 2 +- lldb/source/Symbol/SaveCoreOptions.cpp | 23 +++++++++- .../TestSBSaveCoreOptions.py | 44 +++++++++++++++++++ 7 files changed, 97 insertions(+), 2 deletions(-) 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..a965f4448cbf0 100644 --- a/lldb/include/lldb/API/SBSaveCoreOptions.h +++ b/lldb/include/lldb/API/SBSaveCoreOptions.h @@ -15,6 +15,7 @@ #include "lldb/API/SBProcess.h" #include "lldb/API/SBThread.h" #include "lldb/API/SBThreadCollection.h" +#include "lldb/API/SBMemoryRegionInfoList.h" namespace lldb { @@ -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. diff --git a/lldb/include/lldb/Symbol/SaveCoreOptions.h b/lldb/include/lldb/Symbol/SaveCoreOptions.h index da66b184745db..2a171ba2d8ee2 100644 --- a/lldb/include/lldb/Symbol/SaveCoreOptions.h +++ b/lldb/include/lldb/Symbol/SaveCoreOptions.h @@ -10,6 +10,7 @@ #define LLDB_SOURCE_PLUGINS_OBJECTFILE_SaveCoreOPTIONS_H #include "lldb/Target/ThreadCollection.h" +#include "lldb/Target/CoreFileMemoryRanges.h" #include "lldb/Utility/FileSpec.h" #include "lldb/Utility/RangeMap.h" @@ -47,6 +48,7 @@ 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(); diff --git a/lldb/source/API/SBSaveCoreOptions.cpp b/lldb/source/API/SBSaveCoreOptions.cpp index 15584abaac013..2b71cb695e584 100644 --- a/lldb/source/API/SBSaveCoreOptions.cpp +++ b/lldb/source/API/SBSaveCoreOptions.cpp @@ -128,6 +128,25 @@ 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; +} + 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 34a71f41f3b84..3adcb2633a29e 100644 --- a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp +++ b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp @@ -1138,7 +1138,7 @@ MinidumpFileBuilder::AddMemoryList_64(std::vector<CoreFileMemoryRange> &ranges, offset_t starting_offset = 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..dfabe3a62ed1d 100644 --- a/lldb/source/Symbol/SaveCoreOptions.cpp +++ b/lldb/source/Symbol/SaveCoreOptions.cpp @@ -155,6 +155,23 @@ 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 +186,12 @@ 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; diff --git a/lldb/test/API/python_api/sbsavecoreoptions/TestSBSaveCoreOptions.py b/lldb/test/API/python_api/sbsavecoreoptions/TestSBSaveCoreOptions.py index 31e35e0285f17..ec889db983ea0 100644 --- a/lldb/test/API/python_api/sbsavecoreoptions/TestSBSaveCoreOptions.py +++ b/lldb/test/API/python_api/sbsavecoreoptions/TestSBSaveCoreOptions.py @@ -164,3 +164,47 @@ 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()) >From d5d443c49f47d2da93f06e7c672279e3978bd4bd Mon Sep 17 00:00:00 2001 From: Jacob Lalonde <jalalo...@fb.com> Date: Wed, 2 Jul 2025 11:16:17 -0700 Subject: [PATCH 3/7] Create flag api, and define it in MinidumpFileBuilder --- lldb/include/lldb/API/SBSaveCoreOptions.h | 8 ++++++++ lldb/include/lldb/Symbol/SaveCoreOptions.h | 7 ++++++- lldb/source/API/SBSaveCoreOptions.cpp | 6 ++++++ .../ObjectFile/Minidump/MinidumpFileBuilder.cpp | 5 +++++ .../ObjectFile/Minidump/MinidumpFileBuilder.h | 2 ++ lldb/source/Symbol/SaveCoreOptions.cpp | 14 ++++++++++++++ 6 files changed, 41 insertions(+), 1 deletion(-) diff --git a/lldb/include/lldb/API/SBSaveCoreOptions.h b/lldb/include/lldb/API/SBSaveCoreOptions.h index a965f4448cbf0..92c5c12a8ae33 100644 --- a/lldb/include/lldb/API/SBSaveCoreOptions.h +++ b/lldb/include/lldb/API/SBSaveCoreOptions.h @@ -140,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 specific to a plugin provider, 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 2a171ba2d8ee2..9ff4b494c2bf7 100644 --- a/lldb/include/lldb/Symbol/SaveCoreOptions.h +++ b/lldb/include/lldb/Symbol/SaveCoreOptions.h @@ -24,7 +24,7 @@ namespace lldb_private { class SaveCoreOptions { public: - SaveCoreOptions(){}; + SaveCoreOptions() = default; ~SaveCoreOptions() = default; lldb_private::Status SetPluginName(const char *name); @@ -53,6 +53,10 @@ class SaveCoreOptions { llvm::Expected<uint64_t> GetCurrentSizeInBytes(); + void AddFlag(const char *flag); + + bool ContainsFlag(const char *flag) const; + void Clear(); private: @@ -61,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 2b71cb695e584..7696912a56bbd 100644 --- a/lldb/source/API/SBSaveCoreOptions.cpp +++ b/lldb/source/API/SBSaveCoreOptions.cpp @@ -147,6 +147,12 @@ lldb::SBMemoryRegionInfoList SBSaveCoreOptions::GetMemoryRegionsToSave() { 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 3adcb2633a29e..349797c80376b 100644 --- a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp +++ b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp @@ -831,6 +831,11 @@ 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 diff --git a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h index 46b20f90138fe..978fc7b44a384 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 const char[10] 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 dfabe3a62ed1d..58d8b59f7b843 100644 --- a/lldb/source/Symbol/SaveCoreOptions.cpp +++ b/lldb/source/Symbol/SaveCoreOptions.cpp @@ -211,3 +211,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(); +} >From 3c4adde006fd6add000178b79cc4c571adfb220a Mon Sep 17 00:00:00 2001 From: Jacob Lalonde <jalalo...@fb.com> Date: Wed, 2 Jul 2025 13:13:20 -0700 Subject: [PATCH 4/7] Add new test class, and do some code fix-up --- lldb/include/lldb/API/SBSaveCoreOptions.h | 2 +- .../Minidump/MinidumpFileBuilder.cpp | 4 +- .../ObjectFile/Minidump/MinidumpFileBuilder.h | 2 +- .../TestProcessSaveCoreMinidump64b.py | 84 +++++++++++++++++++ 4 files changed, 88 insertions(+), 4 deletions(-) create mode 100644 lldb/test/API/functionalities/process_save_core_minidump/TestProcessSaveCoreMinidump64b.py diff --git a/lldb/include/lldb/API/SBSaveCoreOptions.h b/lldb/include/lldb/API/SBSaveCoreOptions.h index 92c5c12a8ae33..e42b6e2823775 100644 --- a/lldb/include/lldb/API/SBSaveCoreOptions.h +++ b/lldb/include/lldb/API/SBSaveCoreOptions.h @@ -140,7 +140,7 @@ class LLDB_API SBSaveCoreOptions { /// The expected size of the data contained in the core in bytes. uint64_t GetCurrentSizeInBytes(SBError &error); - /// Add a flag specific to a plugin provider, null or empty flags + /// Add a flag to be consumed by the specified plugin, null or empty flags /// will be ignored. /// /// \note diff --git a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp index 349797c80376b..cb1fb2cb5ca25 100644 --- a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp +++ b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp @@ -834,7 +834,7 @@ Status MinidumpFileBuilder::AddMemoryList() { // 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); + 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 @@ -895,7 +895,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 { diff --git a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h index 978fc7b44a384..cc2520d7f0b16 100644 --- a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h +++ b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h @@ -176,7 +176,7 @@ class MinidumpFileBuilder { static constexpr size_t HEADER_SIZE = sizeof(llvm::minidump::Header); static constexpr size_t DIRECTORY_SIZE = sizeof(llvm::minidump::Directory); - static const char[10] FORCE_64B_FLAG = "force_64b"; + 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 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..5a2300188f140 --- /dev/null +++ b/lldb/test/API/functionalities/process_save_core_minidump/TestProcessSaveCoreMinidump64b.py @@ -0,0 +1,84 @@ +""" +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) >From 6c762afea5a7e1a337cffec37838935743fe46e0 Mon Sep 17 00:00:00 2001 From: Jacob Lalonde <jalalo...@fb.com> Date: Wed, 2 Jul 2025 13:14:23 -0700 Subject: [PATCH 5/7] GCF --- lldb/include/lldb/API/SBSaveCoreOptions.h | 6 +++--- lldb/include/lldb/Symbol/SaveCoreOptions.h | 2 +- lldb/source/API/SBSaveCoreOptions.cpp | 9 +++++---- .../Minidump/MinidumpFileBuilder.cpp | 18 +++++++++++------- lldb/source/Symbol/SaveCoreOptions.cpp | 9 ++++++--- 5 files changed, 26 insertions(+), 18 deletions(-) diff --git a/lldb/include/lldb/API/SBSaveCoreOptions.h b/lldb/include/lldb/API/SBSaveCoreOptions.h index e42b6e2823775..e72dd53780ab9 100644 --- a/lldb/include/lldb/API/SBSaveCoreOptions.h +++ b/lldb/include/lldb/API/SBSaveCoreOptions.h @@ -12,10 +12,10 @@ #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" -#include "lldb/API/SBMemoryRegionInfoList.h" namespace lldb { @@ -123,7 +123,7 @@ class LLDB_API SBSaveCoreOptions { /// 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 + /// An unsorted copy of all memory regions to save. If no process or style /// is specified an empty collection will be returned. SBMemoryRegionInfoList GetMemoryRegionsToSave(); @@ -146,7 +146,7 @@ class LLDB_API SBSaveCoreOptions { /// \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); + 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 9ff4b494c2bf7..92e474c1131a1 100644 --- a/lldb/include/lldb/Symbol/SaveCoreOptions.h +++ b/lldb/include/lldb/Symbol/SaveCoreOptions.h @@ -9,8 +9,8 @@ #ifndef LLDB_SOURCE_PLUGINS_OBJECTFILE_SaveCoreOPTIONS_H #define LLDB_SOURCE_PLUGINS_OBJECTFILE_SaveCoreOPTIONS_H -#include "lldb/Target/ThreadCollection.h" #include "lldb/Target/CoreFileMemoryRanges.h" +#include "lldb/Target/ThreadCollection.h" #include "lldb/Utility/FileSpec.h" #include "lldb/Utility/RangeMap.h" diff --git a/lldb/source/API/SBSaveCoreOptions.cpp b/lldb/source/API/SBSaveCoreOptions.cpp index 7696912a56bbd..0d618bc1916b9 100644 --- a/lldb/source/API/SBSaveCoreOptions.cpp +++ b/lldb/source/API/SBSaveCoreOptions.cpp @@ -130,17 +130,18 @@ uint64_t SBSaveCoreOptions::GetCurrentSizeInBytes(SBError &error) { lldb::SBMemoryRegionInfoList SBSaveCoreOptions::GetMemoryRegionsToSave() { LLDB_INSTRUMENT_VA(this); - llvm::Expected<lldb_private::CoreFileMemoryRanges> memory_ranges = m_opaque_up->GetMemoryRegionsToSave(); + 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); + SBMemoryRegionInfo region_info( + nullptr, range.GetRangeBase(), range.GetRangeEnd(), + range.data.lldb_permissions, /*mapped=*/true); m_memory_region_infos.Append(region_info); } diff --git a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp index cb1fb2cb5ca25..8d8d04d1b1aba 100644 --- a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp +++ b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp @@ -831,10 +831,12 @@ 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); + // 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 @@ -1004,7 +1006,10 @@ Status MinidumpFileBuilder::ReadWriteMemoryInChunks( } 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); + 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 @@ -1019,8 +1024,7 @@ Status MinidumpFileBuilder::ReadWriteMemoryInChunks( 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() && - total_bytes_read != size) { + if (bytes_read != data_buffer.GetByteSize() && total_bytes_read != size) { LLDB_LOGF(log, "Memory region at: 0x%" PRIx64 " partial read 0x%" PRIx64 " bytes out of 0x%" PRIx64 " bytes.", diff --git a/lldb/source/Symbol/SaveCoreOptions.cpp b/lldb/source/Symbol/SaveCoreOptions.cpp index 58d8b59f7b843..6b6387f821590 100644 --- a/lldb/source/Symbol/SaveCoreOptions.cpp +++ b/lldb/source/Symbol/SaveCoreOptions.cpp @@ -155,7 +155,8 @@ SaveCoreOptions::GetThreadsToSave() const { return thread_collection; } -llvm::Expected<lldb_private::CoreFileMemoryRanges> SaveCoreOptions::GetMemoryRegionsToSave() { +llvm::Expected<lldb_private::CoreFileMemoryRanges> +SaveCoreOptions::GetMemoryRegionsToSave() { Status error; if (!m_process_sp) return Status::FromErrorString("Requires a process to be set.").takeError(); @@ -186,10 +187,12 @@ llvm::Expected<uint64_t> SaveCoreOptions::GetCurrentSizeInBytes() { if (error.Fail()) return error.takeError(); - llvm::Expected<lldb_private::CoreFileMemoryRanges> core_file_ranges_maybe = GetMemoryRegionsToSave(); + 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; + const lldb_private::CoreFileMemoryRanges &core_file_ranges = + *core_file_ranges_maybe; uint64_t total_in_bytes = 0; for (auto &core_range : core_file_ranges) total_in_bytes += core_range.data.range.size(); >From fa488d0608b961395f0cb41a5b8e2002d0a5fcff Mon Sep 17 00:00:00 2001 From: Jacob Lalonde <jalalo...@fb.com> Date: Wed, 2 Jul 2025 13:14:42 -0700 Subject: [PATCH 6/7] Python formatting --- .../TestProcessSaveCoreMinidump64b.py | 28 +++++++++++++++---- .../TestSBSaveCoreOptions.py | 1 - 2 files changed, 22 insertions(+), 7 deletions(-) diff --git a/lldb/test/API/functionalities/process_save_core_minidump/TestProcessSaveCoreMinidump64b.py b/lldb/test/API/functionalities/process_save_core_minidump/TestProcessSaveCoreMinidump64b.py index 5a2300188f140..b1ce13a047439 100644 --- a/lldb/test/API/functionalities/process_save_core_minidump/TestProcessSaveCoreMinidump64b.py +++ b/lldb/test/API/functionalities/process_save_core_minidump/TestProcessSaveCoreMinidump64b.py @@ -12,6 +12,7 @@ # Constant from MinidumpFileBuilder.h, this forces 64b for non threads FORCE_64B = "force_64b" + class ProcessSaveCoreMinidump64bTestCase(TestBase): def verify_minidump( @@ -33,19 +34,34 @@ def verify_minidump( 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) + 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) + 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") + 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()) + ( + 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") diff --git a/lldb/test/API/python_api/sbsavecoreoptions/TestSBSaveCoreOptions.py b/lldb/test/API/python_api/sbsavecoreoptions/TestSBSaveCoreOptions.py index ec889db983ea0..92ca44ecbbffc 100644 --- a/lldb/test/API/python_api/sbsavecoreoptions/TestSBSaveCoreOptions.py +++ b/lldb/test/API/python_api/sbsavecoreoptions/TestSBSaveCoreOptions.py @@ -189,7 +189,6 @@ def test_get_memory_regions_to_save(self): 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() >From 2dc4a46246325ea01cb8583661c412b37547c929 Mon Sep 17 00:00:00 2001 From: Jacob Lalonde <jalalo...@fb.com> Date: Wed, 2 Jul 2025 13:56:41 -0700 Subject: [PATCH 7/7] Add docstring --- .../bindings/interface/SBSaveCoreOptionsDocstrings.i | 12 ++++++++++++ 1 file changed, 12 insertions(+) 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." _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
