https://github.com/Jlalond updated https://github.com/llvm/llvm-project/pull/146777
>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 01/16] 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 02/16] 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 03/16] 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 04/16] 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 05/16] 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 06/16] 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 07/16] 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." >From 082ecc6cb60cc856eb746a3f4a43b84761bee41f Mon Sep 17 00:00:00 2001 From: Jacob Lalonde <jalalo...@fb.com> Date: Wed, 2 Jul 2025 14:02:35 -0700 Subject: [PATCH 08/16] Manual python formatting --- .../process_save_core_minidump/TestProcessSaveCoreMinidump64b.py | 1 - 1 file changed, 1 deletion(-) 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 b1ce13a047439..b672629ad7453 100644 --- a/lldb/test/API/functionalities/process_save_core_minidump/TestProcessSaveCoreMinidump64b.py +++ b/lldb/test/API/functionalities/process_save_core_minidump/TestProcessSaveCoreMinidump64b.py @@ -14,7 +14,6 @@ class ProcessSaveCoreMinidump64bTestCase(TestBase): - def verify_minidump( self, core_proc, >From 5ef1482a4e792fdf00a629dfd3a5b8ea9c28ea15 Mon Sep 17 00:00:00 2001 From: Jacob Lalonde <jalalo...@fb.com> Date: Wed, 2 Jul 2025 14:16:31 -0700 Subject: [PATCH 09/16] Add the mixed pages test to 64b --- .../TestProcessSaveCoreMinidump64b.py | 35 +++++++++++++++++++ 1 file changed, 35 insertions(+) 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 b672629ad7453..69d7abf1533a6 100644 --- a/lldb/test/API/functionalities/process_save_core_minidump/TestProcessSaveCoreMinidump64b.py +++ b/lldb/test/API/functionalities/process_save_core_minidump/TestProcessSaveCoreMinidump64b.py @@ -97,3 +97,38 @@ def test_minidump_save_style_full(self): self.assertTrue(self.dbg.DeleteTarget(target)) 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.""" + + self.build() + exe = self.getBuildArtifact("a.out") + minidump_path = self.getBuildArtifact("minidump_mixed_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.eSaveCoreDirtyOnly) + 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 dbf641b5c3dde79cadbbc9dc612da0bd9f72e9a0 Mon Sep 17 00:00:00 2001 From: Jacob Lalonde <jalalo...@fb.com> Date: Mon, 14 Jul 2025 14:49:08 -0700 Subject: [PATCH 10/16] Drop Flags API, move all non-stack memory to memory64list --- lldb/include/lldb/API/SBSaveCoreOptions.h | 8 ---- lldb/include/lldb/Symbol/SaveCoreOptions.h | 7 +--- lldb/source/API/SBSaveCoreOptions.cpp | 6 --- .../Minidump/MinidumpFileBuilder.cpp | 40 ++++--------------- .../ObjectFile/Minidump/MinidumpFileBuilder.h | 2 - lldb/source/Symbol/SaveCoreOptions.cpp | 14 ------- .../TestProcessSaveCoreMinidump64b.py | 9 ++--- 7 files changed, 12 insertions(+), 74 deletions(-) diff --git a/lldb/include/lldb/API/SBSaveCoreOptions.h b/lldb/include/lldb/API/SBSaveCoreOptions.h index e72dd53780ab9..647ab62dcf46c 100644 --- a/lldb/include/lldb/API/SBSaveCoreOptions.h +++ b/lldb/include/lldb/API/SBSaveCoreOptions.h @@ -140,14 +140,6 @@ 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 92e474c1131a1..bfd6d9b532066 100644 --- a/lldb/include/lldb/Symbol/SaveCoreOptions.h +++ b/lldb/include/lldb/Symbol/SaveCoreOptions.h @@ -52,11 +52,7 @@ class SaveCoreOptions { 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: @@ -65,7 +61,6 @@ 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 0d618bc1916b9..a143c00d6eb4b 100644 --- a/lldb/source/API/SBSaveCoreOptions.cpp +++ b/lldb/source/API/SBSaveCoreOptions.cpp @@ -148,12 +148,6 @@ 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 8d8d04d1b1aba..d81dbd53beb93 100644 --- a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp +++ b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp @@ -831,19 +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 // 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); @@ -875,6 +867,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 " @@ -883,35 +879,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 (!force_64b_for_non_threads && 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; } diff --git a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h index cc2520d7f0b16..46b20f90138fe 100644 --- a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h +++ b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h @@ -176,8 +176,6 @@ 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 6b6387f821590..3b541da8cca0a 100644 --- a/lldb/source/Symbol/SaveCoreOptions.cpp +++ b/lldb/source/Symbol/SaveCoreOptions.cpp @@ -214,17 +214,3 @@ 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 index 69d7abf1533a6..76361747b1bde 100644 --- a/lldb/test/API/functionalities/process_save_core_minidump/TestProcessSaveCoreMinidump64b.py +++ b/lldb/test/API/functionalities/process_save_core_minidump/TestProcessSaveCoreMinidump64b.py @@ -9,9 +9,6 @@ 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( @@ -51,6 +48,7 @@ def verify_minidump( 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( ( @@ -60,7 +58,8 @@ def verify_minidump( or ( actual_process_read_error.Fail() and expected_process_read_error.Fail() - ) + ), + f"Address range {hex(start_addr)} - {hex(end_addr)} failed to read from live process and core for different reasons", ) @skipUnlessArch("x86_64") @@ -84,7 +83,6 @@ def test_minidump_save_style_full(self): 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()) @@ -119,7 +117,6 @@ def test_minidump_save_style_mixed_memory(self): options.SetStyle(lldb.eSaveCoreDirtyOnly) options.SetPluginName("minidump") options.SetProcess(live_process) - options.AddFlag(FORCE_64B) error = live_process.SaveCore(options) self.assertTrue(error.Success(), error.GetCString()) >From cf112e90e83191f410f389dfe04d9087a1eaff87 Mon Sep 17 00:00:00 2001 From: Jacob Lalonde <jalalo...@fb.com> Date: Mon, 14 Jul 2025 15:10:07 -0700 Subject: [PATCH 11/16] Add a new API to get the Process set for the save core object, check the process in PluginManager --- lldb/include/lldb/API/SBSaveCoreOptions.h | 6 ++++++ lldb/include/lldb/Symbol/SaveCoreOptions.h | 1 + lldb/source/API/SBSaveCoreOptions.cpp | 5 +++++ lldb/source/Core/PluginManager.cpp | 11 +++++++++++ .../ObjectFile/Minidump/MinidumpFileBuilder.cpp | 9 ++++----- 5 files changed, 27 insertions(+), 5 deletions(-) diff --git a/lldb/include/lldb/API/SBSaveCoreOptions.h b/lldb/include/lldb/API/SBSaveCoreOptions.h index 647ab62dcf46c..ac8d699c7528b 100644 --- a/lldb/include/lldb/API/SBSaveCoreOptions.h +++ b/lldb/include/lldb/API/SBSaveCoreOptions.h @@ -79,6 +79,12 @@ 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 diff --git a/lldb/include/lldb/Symbol/SaveCoreOptions.h b/lldb/include/lldb/Symbol/SaveCoreOptions.h index bfd6d9b532066..0a72839768d0b 100644 --- a/lldb/include/lldb/Symbol/SaveCoreOptions.h +++ b/lldb/include/lldb/Symbol/SaveCoreOptions.h @@ -37,6 +37,7 @@ 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); diff --git a/lldb/source/API/SBSaveCoreOptions.cpp b/lldb/source/API/SBSaveCoreOptions.cpp index a143c00d6eb4b..6f3d1542b9ebc 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()); diff --git a/lldb/source/Core/PluginManager.cpp b/lldb/source/Core/PluginManager.cpp index 3f20a96edc187..ba0c4cdad0737 100644 --- a/lldb/source/Core/PluginManager.cpp +++ b/lldb/source/Core/PluginManager.cpp @@ -964,6 +964,17 @@ Status PluginManager::SaveCore(const lldb::ProcessSP &process_sp, return error; } + // Set the process sp if not already set. + ProcessSP options_sp = options.GetProcess(); + if (!options_sp) + options.SetProcess(process_sp); + + // Make sure the process sp is the same as the one we are using. + if (options_sp != process_sp) { + error = Status::FromErrorString("Save Core Options configured for a different process."); + return error; + } + error = options.EnsureValidConfiguration(process_sp); if (error.Fail()) return error; diff --git a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp index d81dbd53beb93..c76cd23892cc2 100644 --- a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp +++ b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp @@ -836,12 +836,11 @@ 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; - 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()); >From f748e4f956958318a374376de30cc85d182ab0ed Mon Sep 17 00:00:00 2001 From: Jacob Lalonde <jalalo...@fb.com> Date: Mon, 14 Jul 2025 15:11:18 -0700 Subject: [PATCH 12/16] Add doc string --- lldb/bindings/interface/SBSaveCoreOptionsDocstrings.i | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/lldb/bindings/interface/SBSaveCoreOptionsDocstrings.i b/lldb/bindings/interface/SBSaveCoreOptionsDocstrings.i index a44c0569f40e6..e3746a83438ea 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 undefined, an invalid SBProcess will be returned." +) lldb::SBSaveCoreOptions::GetProcess; + %feature("docstring", " Add an SBThread to be saved, an error will be returned if an SBThread from a different process is specified. The process is set either by the first SBThread added to the options container, or explicitly by the SetProcess call." @@ -63,18 +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 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." >From 9b524492ee55b6cf042a5dc1dabd977da8e0fd7c Mon Sep 17 00:00:00 2001 From: Jacob Lalonde <jalalo...@fb.com> Date: Mon, 14 Jul 2025 15:11:41 -0700 Subject: [PATCH 13/16] Run GCF --- lldb/include/lldb/API/SBSaveCoreOptions.h | 3 ++- lldb/include/lldb/Symbol/SaveCoreOptions.h | 2 +- lldb/source/Core/PluginManager.cpp | 3 ++- .../ObjectFile/Minidump/MinidumpFileBuilder.cpp | 10 ++++++---- .../TestProcessSaveCoreMinidump64b.py | 1 - 5 files changed, 11 insertions(+), 8 deletions(-) diff --git a/lldb/include/lldb/API/SBSaveCoreOptions.h b/lldb/include/lldb/API/SBSaveCoreOptions.h index ac8d699c7528b..7b05377966965 100644 --- a/lldb/include/lldb/API/SBSaveCoreOptions.h +++ b/lldb/include/lldb/API/SBSaveCoreOptions.h @@ -79,7 +79,8 @@ 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. + /// 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. diff --git a/lldb/include/lldb/Symbol/SaveCoreOptions.h b/lldb/include/lldb/Symbol/SaveCoreOptions.h index 0a72839768d0b..7fec58e2d1b03 100644 --- a/lldb/include/lldb/Symbol/SaveCoreOptions.h +++ b/lldb/include/lldb/Symbol/SaveCoreOptions.h @@ -53,7 +53,7 @@ class SaveCoreOptions { lldb_private::ThreadCollection::collection GetThreadsToSave() const; llvm::Expected<uint64_t> GetCurrentSizeInBytes(); - + void Clear(); private: diff --git a/lldb/source/Core/PluginManager.cpp b/lldb/source/Core/PluginManager.cpp index ba0c4cdad0737..055a92334e72c 100644 --- a/lldb/source/Core/PluginManager.cpp +++ b/lldb/source/Core/PluginManager.cpp @@ -971,7 +971,8 @@ Status PluginManager::SaveCore(const lldb::ProcessSP &process_sp, // Make sure the process sp is the same as the one we are using. if (options_sp != process_sp) { - error = Status::FromErrorString("Save Core Options configured for a different process."); + error = Status::FromErrorString( + "Save Core Options configured for a different process."); return error; } diff --git a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp index c76cd23892cc2..689bde42ec585 100644 --- a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp +++ b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp @@ -836,11 +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; - llvm::Expected<CoreFileMemoryRanges> all_core_memory_ranges_maybe = m_save_core_options.GetMemoryRegionsToSave(); + 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()); - const CoreFileMemoryRanges &all_core_memory_ranges = *all_core_memory_ranges_maybe; + const CoreFileMemoryRanges &all_core_memory_ranges = + *all_core_memory_ranges_maybe; lldb_private::Progress progress("Saving Minidump File", "", all_core_memory_ranges.GetSize()); @@ -878,8 +880,8 @@ Status MinidumpFileBuilder::AddMemoryList() { return error; } - // Save only the thread stacks to the 32b memory list. Everything else will get put in - // Memory64, this simplifies tracking + // 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; 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 76361747b1bde..7c311cb2b0a03 100644 --- a/lldb/test/API/functionalities/process_save_core_minidump/TestProcessSaveCoreMinidump64b.py +++ b/lldb/test/API/functionalities/process_save_core_minidump/TestProcessSaveCoreMinidump64b.py @@ -48,7 +48,6 @@ def verify_minidump( 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( ( >From c2885a30bdfc72dcb55b006c2788dc5272caf6f2 Mon Sep 17 00:00:00 2001 From: Jacob Lalonde <jalalo...@fb.com> Date: Mon, 14 Jul 2025 15:53:59 -0700 Subject: [PATCH 14/16] Fix logic in PluginManager --- lldb/source/Core/PluginManager.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lldb/source/Core/PluginManager.cpp b/lldb/source/Core/PluginManager.cpp index 055a92334e72c..129c0dc48f3e9 100644 --- a/lldb/source/Core/PluginManager.cpp +++ b/lldb/source/Core/PluginManager.cpp @@ -970,7 +970,7 @@ Status PluginManager::SaveCore(const lldb::ProcessSP &process_sp, options.SetProcess(process_sp); // Make sure the process sp is the same as the one we are using. - if (options_sp != process_sp) { + if (options_sp && options_sp != process_sp) { error = Status::FromErrorString( "Save Core Options configured for a different process."); return error; >From 81075b43788951d556146ae18f6df39ed749cf4f Mon Sep 17 00:00:00 2001 From: Jacob Lalonde <jalalo...@fb.com> Date: Mon, 14 Jul 2025 16:08:16 -0700 Subject: [PATCH 15/16] Update test file description --- .../TestProcessSaveCoreMinidump64b.py | 4 ++-- 1 file changed, 2 insertions(+), 2 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 7c311cb2b0a03..605bd76871f95 100644 --- a/lldb/test/API/functionalities/process_save_core_minidump/TestProcessSaveCoreMinidump64b.py +++ b/lldb/test/API/functionalities/process_save_core_minidump/TestProcessSaveCoreMinidump64b.py @@ -1,6 +1,6 @@ """ -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. +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 >From 313fb2f395eac5401086d241b0b8f351bf656df2 Mon Sep 17 00:00:00 2001 From: Jacob Lalonde <jalalo...@fb.com> Date: Tue, 15 Jul 2025 09:11:59 -0700 Subject: [PATCH 16/16] Switch from 2x u64 to sizeof header --- lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp index 689bde42ec585..a331704c7a51f 100644 --- a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp +++ b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp @@ -1109,7 +1109,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()) _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits