llvmbot wrote:

<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-lldb

Author: Jacob Lalonde (Jlalond)

<details>
<summary>Changes</summary>

### 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

![image](https://github.com/user-attachments/assets/ffc04464-515e-4303-842c-da295ae9e651)


### 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 
&gt;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.

![image](https://github.com/user-attachments/assets/b599e3be-2d59-47e2-8a2d-75f182bb0b1d)

### 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

---

Patch is 20.14 KiB, truncated to 20.00 KiB below, full version: 
https://github.com/llvm/llvm-project/pull/146777.diff


10 Files Affected:

- (modified) lldb/bindings/interface/SBSaveCoreOptionsDocstrings.i (+12) 
- (modified) lldb/include/lldb/API/SBMemoryRegionInfoList.h (+1) 
- (modified) lldb/include/lldb/API/SBSaveCoreOptions.h (+16) 
- (modified) lldb/include/lldb/Symbol/SaveCoreOptions.h (+8-1) 
- (modified) lldb/source/API/SBSaveCoreOptions.cpp (+26) 
- (modified) lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp 
(+24-9) 
- (modified) lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h (+2) 
- (modified) lldb/source/Symbol/SaveCoreOptions.cpp (+39-1) 
- (added) 
lldb/test/API/functionalities/process_save_core_minidump/TestProcessSaveCoreMinidump64b.py
 (+100) 
- (modified) 
lldb/test/API/python_api/sbsavecoreoptions/TestSBSaveCoreOptions.py (+43) 


``````````diff
diff --git a/lldb/bindings/interface/SBSaveCoreOptionsDocstrings.i 
b/lldb/bindings/interface/SBSaveCoreOptionsDocstrings.i
index 6907164a1b95c..a44c0569f40e6 100644
--- a/lldb/bindings/interface/SBSaveCoreOptionsDocstrings.i
+++ b/lldb/bindings/interface/SBSaveCoreOptionsDocstrings.i
@@ -63,6 +63,18 @@ Note that currently ELF Core files are not supported."
     Get an SBThreadCollection of all threads marked to be saved. This 
collection is not sorted according to insertion order."
 ) lldb::SBSaveCoreOptions::GetThreadsToSave;
 
+
+%feature("docstring", "
+    Get an SBMemoryRegionInfoList of all the Regions that LLDB will attempt to 
write into the Core. Note, reading from these
+    regions can fail, and it's guaraunteed every region will be present. If 
called without a valid process or style set an empty
+    collection will be returned."
+) lldb::SBSaveCoreOptions::GetMemoryRegionsToSave;
+
+%feature("docstring", "
+    Add a plugin specific flag to the objects option."
+) lldb::SBSaveCoreOptions::AddFlag;
+
+
 %feature("docstring", "
     Get the current total number of bytes the core is expected to have, 
excluding the overhead of the core file format.
     Requires both a Process and a Style to be specified. An error will be 
returned if the provided options would result in no data being saved."
diff --git a/lldb/include/lldb/API/SBMemoryRegionInfoList.h 
b/lldb/include/lldb/API/SBMemoryRegionInfoList.h
index 1d939dff55faa..8ac9c1aceb6f6 100644
--- a/lldb/include/lldb/API/SBMemoryRegionInfoList.h
+++ b/lldb/include/lldb/API/SBMemoryRegionInfoList.h
@@ -45,6 +45,7 @@ class LLDB_API SBMemoryRegionInfoList {
 
 private:
   friend class SBProcess;
+  friend class SBSaveCoreOptions;
 
   lldb_private::MemoryRegionInfos &ref();
 
diff --git a/lldb/include/lldb/API/SBSaveCoreOptions.h 
b/lldb/include/lldb/API/SBSaveCoreOptions.h
index 37552c13d0f36..e72dd53780ab9 100644
--- a/lldb/include/lldb/API/SBSaveCoreOptions.h
+++ b/lldb/include/lldb/API/SBSaveCoreOptions.h
@@ -12,6 +12,7 @@
 #include "lldb/API/SBDefines.h"
 #include "lldb/API/SBError.h"
 #include "lldb/API/SBFileSpec.h"
+#include "lldb/API/SBMemoryRegionInfoList.h"
 #include "lldb/API/SBProcess.h"
 #include "lldb/API/SBThread.h"
 #include "lldb/API/SBThreadCollection.h"
@@ -119,6 +120,13 @@ class LLDB_API SBSaveCoreOptions {
   ///   an empty collection will be returned.
   SBThreadCollection GetThreadsToSave() const;
 
+  /// Get an unsorted copy of all memory regions to save
+  ///
+  /// \returns
+  ///   An unsorted copy of all memory regions to save. If no process or style
+  ///   is specified an empty collection will be returned.
+  SBMemoryRegionInfoList GetMemoryRegionsToSave();
+
   /// Get the current total number of bytes the core is expected to have
   /// excluding the overhead of the core file format. Requires a Process and
   /// Style to be specified.
@@ -132,6 +140,14 @@ class LLDB_API SBSaveCoreOptions {
   ///   The expected size of the data contained in the core in bytes.
   uint64_t GetCurrentSizeInBytes(SBError &error);
 
+  /// Add a flag to be consumed by the specified plugin, null or empty flags
+  /// will be ignored.
+  ///
+  /// \note
+  ///   This API is currently only used for testing, with forcing Minidumps to
+  ///   to 64b memory list the reason this api was added
+  void AddFlag(const char *flag);
+
   /// Reset all options.
   void Clear();
 
diff --git a/lldb/include/lldb/Symbol/SaveCoreOptions.h 
b/lldb/include/lldb/Symbol/SaveCoreOptions.h
index da66b184745db..92e474c1131a1 100644
--- a/lldb/include/lldb/Symbol/SaveCoreOptions.h
+++ b/lldb/include/lldb/Symbol/SaveCoreOptions.h
@@ -9,6 +9,7 @@
 #ifndef LLDB_SOURCE_PLUGINS_OBJECTFILE_SaveCoreOPTIONS_H
 #define LLDB_SOURCE_PLUGINS_OBJECTFILE_SaveCoreOPTIONS_H
 
+#include "lldb/Target/CoreFileMemoryRanges.h"
 #include "lldb/Target/ThreadCollection.h"
 #include "lldb/Utility/FileSpec.h"
 #include "lldb/Utility/RangeMap.h"
@@ -23,7 +24,7 @@ namespace lldb_private {
 
 class SaveCoreOptions {
 public:
-  SaveCoreOptions(){};
+  SaveCoreOptions() = default;
   ~SaveCoreOptions() = default;
 
   lldb_private::Status SetPluginName(const char *name);
@@ -47,10 +48,15 @@ class SaveCoreOptions {
 
   void AddMemoryRegionToSave(const lldb_private::MemoryRegionInfo &region);
 
+  llvm::Expected<lldb_private::CoreFileMemoryRanges> GetMemoryRegionsToSave();
   lldb_private::ThreadCollection::collection GetThreadsToSave() const;
 
   llvm::Expected<uint64_t> GetCurrentSizeInBytes();
 
+  void AddFlag(const char *flag);
+
+  bool ContainsFlag(const char *flag) const;
+
   void Clear();
 
 private:
@@ -59,6 +65,7 @@ class SaveCoreOptions {
   std::optional<std::string> m_plugin_name;
   std::optional<lldb_private::FileSpec> m_file;
   std::optional<lldb::SaveCoreStyle> m_style;
+  std::optional<std::unordered_set<std::string>> m_flags;
   lldb::ProcessSP m_process_sp;
   std::unordered_set<lldb::tid_t> m_threads_to_save;
   MemoryRanges m_regions_to_save;
diff --git a/lldb/source/API/SBSaveCoreOptions.cpp 
b/lldb/source/API/SBSaveCoreOptions.cpp
index 15584abaac013..0d618bc1916b9 100644
--- a/lldb/source/API/SBSaveCoreOptions.cpp
+++ b/lldb/source/API/SBSaveCoreOptions.cpp
@@ -128,6 +128,32 @@ uint64_t SBSaveCoreOptions::GetCurrentSizeInBytes(SBError 
&error) {
   return *expected_bytes;
 }
 
+lldb::SBMemoryRegionInfoList SBSaveCoreOptions::GetMemoryRegionsToSave() {
+  LLDB_INSTRUMENT_VA(this);
+  llvm::Expected<lldb_private::CoreFileMemoryRanges> memory_ranges =
+      m_opaque_up->GetMemoryRegionsToSave();
+  if (!memory_ranges) {
+    llvm::consumeError(memory_ranges.takeError());
+    return SBMemoryRegionInfoList();
+  }
+
+  SBMemoryRegionInfoList m_memory_region_infos;
+  for (const auto &range : *memory_ranges) {
+    SBMemoryRegionInfo region_info(
+        nullptr, range.GetRangeBase(), range.GetRangeEnd(),
+        range.data.lldb_permissions, /*mapped=*/true);
+    m_memory_region_infos.Append(region_info);
+  }
+
+  return m_memory_region_infos;
+}
+
+void SBSaveCoreOptions::AddFlag(const char *flag) {
+  LLDB_INSTRUMENT_VA(this, flag);
+
+  m_opaque_up->AddFlag(flag);
+}
+
 lldb_private::SaveCoreOptions &SBSaveCoreOptions::ref() const {
   return *m_opaque_up;
 }
diff --git a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp 
b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp
index 806f256d9da48..8d8d04d1b1aba 100644
--- a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp
+++ b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp
@@ -831,6 +831,13 @@ Status MinidumpFileBuilder::AddLinuxFileStreams() {
 Status MinidumpFileBuilder::AddMemoryList() {
   Status error;
 
+  // Note this is here for testing. In the past there has been many occasions
+  // that the 64b code has regressed because it's wasteful and expensive to
+  // write a 4.2gb+ on every CI run to get around this and to exercise this
+  // codepath we define a flag in the options object.
+  bool force_64b_for_non_threads =
+      m_save_core_options.ContainsFlag(FORCE_64B_FLAG);
+
   // We first save the thread stacks to ensure they fit in the first UINT32_MAX
   // bytes of the core file. Thread structures in minidump files can only use
   // 32 bit memory descriptiors, so we emit them first to ensure the memory is
@@ -890,7 +897,7 @@ Status MinidumpFileBuilder::AddMemoryList() {
     const addr_t range_size = core_range.range.size();
     // We don't need to check for stacks here because we already removed them
     // from all_core_memory_ranges.
-    if (total_size + range_size < UINT32_MAX) {
+    if (!force_64b_for_non_threads && total_size + range_size < UINT32_MAX) {
       ranges_32.push_back(core_range);
       total_size += range_size;
     } else {
@@ -977,6 +984,7 @@ Status MinidumpFileBuilder::ReadWriteMemoryInChunks(
   const lldb::addr_t addr = range.range.start();
   const lldb::addr_t size = range.range.size();
   Log *log = GetLog(LLDBLog::Object);
+  uint64_t total_bytes_read = 0;
   Status addDataError;
   Process::ReadMemoryChunkCallback callback =
       [&](Status &error, lldb::addr_t current_addr, const void *buf,
@@ -984,7 +992,7 @@ Status MinidumpFileBuilder::ReadWriteMemoryInChunks(
     if (error.Fail() || bytes_read == 0) {
       LLDB_LOGF(log,
                 "Failed to read memory region at: 0x%" PRIx64
-                ". Bytes read: %" PRIx64 ", error: %s",
+                ". Bytes read: 0x%" PRIx64 ", error: %s",
                 current_addr, bytes_read, error.AsCString());
 
       // If we failed in a memory read, we would normally want to skip
@@ -997,6 +1005,13 @@ Status MinidumpFileBuilder::ReadWriteMemoryInChunks(
       return lldb_private::IterationAction::Stop;
     }
 
+    if (current_addr != addr + total_bytes_read) {
+      LLDB_LOGF(log,
+                "Current addr is at expected address, 0x%" PRIx64
+                ", expected at 0x%" PRIx64,
+                current_addr, addr + total_bytes_read);
+    }
+
     // Write to the minidump file with the chunk potentially flushing to
     // disk.
     // This error will be captured by the outer scope and is considered fatal.
@@ -1006,13 +1021,13 @@ Status MinidumpFileBuilder::ReadWriteMemoryInChunks(
     if (addDataError.Fail())
       return lldb_private::IterationAction::Stop;
 
+    total_bytes_read += bytes_read;
     // If we have a partial read, report it, but only if the partial read
     // didn't finish reading the entire region.
-    if (bytes_read != data_buffer.GetByteSize() &&
-        current_addr + bytes_read != size) {
+    if (bytes_read != data_buffer.GetByteSize() && total_bytes_read != size) {
       LLDB_LOGF(log,
-                "Memory region at: %" PRIx64 " partiall read 0x%" PRIx64
-                " bytes out of %" PRIx64 " bytes.",
+                "Memory region at: 0x%" PRIx64 " partial read 0x%" PRIx64
+                " bytes out of 0x%" PRIx64 " bytes.",
                 current_addr, bytes_read,
                 data_buffer.GetByteSize() - bytes_read);
 
@@ -1059,7 +1074,7 @@ 
MinidumpFileBuilder::AddMemoryList_32(std::vector<CoreFileMemoryRange> &ranges,
 
     LLDB_LOGF(log,
               "AddMemoryList %zu/%zu reading memory for region "
-              "(%" PRIx64 " bytes) [%" PRIx64 ", %" PRIx64 ")",
+              "(0x%" PRIx64 " bytes) [0x%" PRIx64 ", 0x%" PRIx64 ")",
               region_index, ranges.size(), size, addr, addr + size);
     ++region_index;
 
@@ -1130,9 +1145,9 @@ 
MinidumpFileBuilder::AddMemoryList_64(std::vector<CoreFileMemoryRange> &ranges,
   // Capture the starting offset for all the descriptors so we can clean them 
up
   // if needed.
   offset_t starting_offset =
-      GetCurrentDataEndOffset() + sizeof(llvm::support::ulittle64_t);
+      GetCurrentDataEndOffset() + sizeof(llvm::minidump::Memory64ListHeader);
   // The base_rva needs to start after the directories, which is right after
-  // this 8 byte variable.
+  // the descriptors + the size of the header.
   offset_t base_rva =
       starting_offset +
       (ranges.size() * sizeof(llvm::minidump::MemoryDescriptor_64));
diff --git a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h 
b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h
index 46b20f90138fe..cc2520d7f0b16 100644
--- a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h
+++ b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h
@@ -176,6 +176,8 @@ class MinidumpFileBuilder {
   static constexpr size_t HEADER_SIZE = sizeof(llvm::minidump::Header);
   static constexpr size_t DIRECTORY_SIZE = sizeof(llvm::minidump::Directory);
 
+  static constexpr const char FORCE_64B_FLAG[] = "force_64b";
+
   // More that one place can mention the register thread context locations,
   // so when we emit the thread contents, remember where it is so we don't have
   // to duplicate it in the exception data.
diff --git a/lldb/source/Symbol/SaveCoreOptions.cpp 
b/lldb/source/Symbol/SaveCoreOptions.cpp
index f93b58f59cf96..6b6387f821590 100644
--- a/lldb/source/Symbol/SaveCoreOptions.cpp
+++ b/lldb/source/Symbol/SaveCoreOptions.cpp
@@ -155,6 +155,24 @@ SaveCoreOptions::GetThreadsToSave() const {
   return thread_collection;
 }
 
+llvm::Expected<lldb_private::CoreFileMemoryRanges>
+SaveCoreOptions::GetMemoryRegionsToSave() {
+  Status error;
+  if (!m_process_sp)
+    return Status::FromErrorString("Requires a process to be 
set.").takeError();
+
+  error = EnsureValidConfiguration(m_process_sp);
+  if (error.Fail())
+    return error.takeError();
+
+  CoreFileMemoryRanges ranges;
+  error = m_process_sp->CalculateCoreFileSaveRanges(*this, ranges);
+  if (error.Fail())
+    return error.takeError();
+
+  return ranges;
+}
+
 llvm::Expected<uint64_t> SaveCoreOptions::GetCurrentSizeInBytes() {
   Status error;
   if (!m_process_sp)
@@ -169,8 +187,14 @@ llvm::Expected<uint64_t> 
SaveCoreOptions::GetCurrentSizeInBytes() {
   if (error.Fail())
     return error.takeError();
 
+  llvm::Expected<lldb_private::CoreFileMemoryRanges> core_file_ranges_maybe =
+      GetMemoryRegionsToSave();
+  if (!core_file_ranges_maybe)
+    return core_file_ranges_maybe.takeError();
+  const lldb_private::CoreFileMemoryRanges &core_file_ranges =
+      *core_file_ranges_maybe;
   uint64_t total_in_bytes = 0;
-  for (auto &core_range : ranges)
+  for (auto &core_range : core_file_ranges)
     total_in_bytes += core_range.data.range.size();
 
   return total_in_bytes;
@@ -190,3 +214,17 @@ void SaveCoreOptions::Clear() {
   m_process_sp.reset();
   m_regions_to_save.Clear();
 }
+
+void SaveCoreOptions::AddFlag(const char *flag) {
+  if (!flag || !flag[0])
+    return;
+
+  if (!m_flags)
+    m_flags = std::unordered_set<std::string>();
+
+  m_flags->emplace(std::string(flag));
+}
+
+bool SaveCoreOptions::ContainsFlag(const char *flag) const {
+  return m_flags && m_flags->find(flag) != m_flags->end();
+}
diff --git 
a/lldb/test/API/functionalities/process_save_core_minidump/TestProcessSaveCoreMinidump64b.py
 
b/lldb/test/API/functionalities/process_save_core_minidump/TestProcessSaveCoreMinidump64b.py
new file mode 100644
index 0000000000000..b1ce13a047439
--- /dev/null
+++ 
b/lldb/test/API/functionalities/process_save_core_minidump/TestProcessSaveCoreMinidump64b.py
@@ -0,0 +1,100 @@
+"""
+Test saving a minidumps with the force 64b flag, and evaluate that every
+saved memory region is byte-wise 1:1 with the live process.
+"""
+
+import os
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+# Constant from MinidumpFileBuilder.h, this forces 64b for non threads
+FORCE_64B = "force_64b"
+
+
+class ProcessSaveCoreMinidump64bTestCase(TestBase):
+
+    def verify_minidump(
+        self,
+        core_proc,
+        live_proc,
+        options,
+    ):
+        """Verify that the minidump is the same byte for byte as the live 
process."""
+        # Get the memory regions we saved off in this core, we can't compare 
to the core
+        # because we pull from /proc/pid/maps, so even ranges that don't get 
mapped in will show up
+        # as ranges in the minidump.
+        #
+        # Instead, we have an API that returns to us the number of regions we 
planned to save from the live process
+        # and we compare those
+        memory_regions_to_compare = options.GetMemoryRegionsToSave()
+
+        for region in memory_regions_to_compare:
+            start_addr = region.GetRegionBase()
+            end_addr = region.GetRegionEnd()
+            actual_process_read_error = lldb.SBError()
+            actual = live_proc.ReadMemory(
+                start_addr, end_addr - start_addr, actual_process_read_error
+            )
+            expected_process_read_error = lldb.SBError()
+            expected = core_proc.ReadMemory(
+                start_addr, end_addr - start_addr, expected_process_read_error
+            )
+
+            # Both processes could fail to read a given memory region, so if 
they both pass
+            # compare, then we'll fail them if the core differs from the live 
process.
+            if (
+                actual_process_read_error.Success()
+                and expected_process_read_error.Success()
+            ):
+                self.assertEqual(
+                    actual, expected, "Bytes differ between live process and 
core"
+                )
+
+            # Now we check if the error is the same, error isn't abnormal but 
they should fail for the same reason
+            self.assertTrue(
+                (
+                    actual_process_read_error.Success()
+                    and expected_process_read_error.Success()
+                )
+                or (
+                    actual_process_read_error.Fail()
+                    and expected_process_read_error.Fail()
+                )
+            )
+
+    @skipUnlessArch("x86_64")
+    @skipUnlessPlatform(["linux"])
+    def test_minidump_save_style_full(self):
+        """Test that a full minidump is the same byte for byte."""
+
+        self.build()
+        exe = self.getBuildArtifact("a.out")
+        minidump_path = self.getBuildArtifact("minidump_full_force64b.dmp")
+
+        try:
+            target = self.dbg.CreateTarget(exe)
+            live_process = target.LaunchSimple(
+                None, None, self.get_process_working_directory()
+            )
+            self.assertState(live_process.GetState(), lldb.eStateStopped)
+            options = lldb.SBSaveCoreOptions()
+
+            options.SetOutputFile(lldb.SBFileSpec(minidump_path))
+            options.SetStyle(lldb.eSaveCoreFull)
+            options.SetPluginName("minidump")
+            options.SetProcess(live_process)
+            options.AddFlag(FORCE_64B)
+
+            error = live_process.SaveCore(options)
+            self.assertTrue(error.Success(), error.GetCString())
+
+            target = self.dbg.CreateTarget(None)
+            core_proc = target.LoadCore(minidump_path)
+
+            self.verify_minidump(core_proc, live_process, options)
+        finally:
+            self.assertTrue(self.dbg.DeleteTarget(target))
+            if os.path.isfile(minidump_path):
+                os.unlink(minidump_path)
diff --git 
a/lldb/test/API/python_api/sbsavecoreoptions/TestSBSaveCoreOptions.py 
b/lldb/test/API/python_api/sbsavecoreoptions/TestSBSaveCoreOptions.py
index 31e35e0285f17..92ca44ecbbffc 100644
--- a/lldb/test/API/python_api/sbsavecoreoptions/TestSBSaveCoreOptions.py
+++ b/lldb/test/API/python_api/sbsavecoreoptions/TestSBSaveCoreOptions.py
@@ -164,3 +164,46 @@ def test_get_total_in_bytes_missing_requirements(self):
         options.SetStyle(lldb.eSaveCoreCustomOnly)
         total = options.GetCurrentSizeInBytes(error)
         self.assertTrue(error.Fail(), error.GetCString())
+
+    def test_get_memory_regions_to_save(self):
+        """
+        Tests the matrix of responses for GetMemoryRegionsToSave
+        """
+
+        options = lldb.SBSaveCoreOptions()
+
+        # Not specifying plugin or process should return an empty list.
+        memory_list = options.GetMemoryRegionsToSave()
+        self.assertEqual(0, memory_list.GetSize())
+
+        # No style returns an empty list
+        process = self.get_basic_process()
+        options.SetProcess(process)
+        memory_list = options.GetMemoryRegionsToSave()
+        self.assertEqual(0, memory_list.GetSize())
+        options.Clear()
+
+        # No Process returns an empty list
+        options.SetStyle(lldb.eSaveCoreCustomOnly)
+        memory_list = options.GetMemoryRegionsToSave()
+        self.assertEqual(0, memory_list.GetSize())
+        options.Clear()
+
+        # Validate we get back the single region we populate
+        options.SetStyle(lldb.eSaveCoreCustomOnly)
+        process = self.get_basic_process()
+        options.SetProcess(process)
+        memory_range = lldb.SBMemoryRegionInfo()
+
+        # Add the memory range of 0x1000-0x1100
+        process.GetMemoryRegionInfo(0x1000, memory_range)
+        options.AddMemoryRegionToSave(memory_range)
+        memory_list = options.GetMemoryRegionsToSave()
+        self.assertEqual(1, memory_list.GetSize())
+        read_region = lldb.SBMemoryRegionInfo()
+        memory_list.GetMemoryRegionAtIndex(0, read_region)
+
+        # Permissions from Process getLLDBRegion aren't matching up with
+        # the live process permissions, so we're just checking the range for 
now.
+        self.assertEqual(mem...
[truncated]

``````````

</details>


https://github.com/llvm/llvm-project/pull/146777
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Reply via email to