[LLDB][Minidump] Fix ProcessMinidump::GetMemoryRegions to include 64b regions when /proc/pid maps are missing.

[Jlalond]

This PR is in response to a bug my coworker @mbucko discovered where on MacOS Minidumps were being created where the 64b memory regions were readable, but were not being listed in SBProcess.GetMemoryRegionList(). This went unnoticed in #95312 due to all the linux testing including /proc/pid maps. On MacOS generated dumps (or any dump without access to /proc/pid) we would fail to properly map Memory Regions due to there being two independent methods for 32b and 64b mapping.

In this PR I addressed this minor bug and merged the methods, but in order to add test coverage required additions to obj2yaml and yaml2obj which make up the bulk of this patch.

Lastly, there are some non-required changes such as the addition of the Memory64ListHeader type, to make writing/reading the header section of the Memory64List easier.

[llvmbot]

@llvm/pr-subscribers-lldb
@llvm/pr-subscribers-objectyaml

@llvm/pr-subscribers-llvm-binary-utilities

Author: Jacob Lalonde (Jlalond)

Changes

This PR is in response to a bug my coworker @mbucko discovered where on MacOS Minidumps were being created where the 64b memory regions were readable, but were not being listed in SBProcess.GetMemoryRegionList(). This went unnoticed in !95312 due to all the linux testing including /proc/pid maps. On MacOS generated dumps (or any dump without access to /proc/pid) we would fail to properly map Memory Regions due to there being two independent methods for 32b and 64b mapping.

In this PR I addressed this minor bug and merged the methods, but in order to add test coverage required additions to obj2yaml and yaml2obj which make up the bulk of this patch.

Lastly, there are some non-required changes such as the addition of the Memory64ListHeader type, to make writing/reading the header section of the Memory64List easier.

---

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

11 Files Affected:

• (modified) lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp (+8-5)
• (modified) lldb/source/Plugins/Process/minidump/MinidumpParser.cpp (+29-29)
• (modified) lldb/test/API/functionalities/postmortem/minidump-new/TestMiniDumpNew.py (+13)
• (added) lldb/test/API/functionalities/postmortem/minidump-new/linux-x86_64_mem64.dmp ()
• (added) lldb/test/API/functionalities/postmortem/minidump-new/linux-x86_64_mem64.yaml (+36)
• (modified) llvm/include/llvm/BinaryFormat/Minidump.h (+12)
• (modified) llvm/include/llvm/Object/Minidump.h (+13-3)
• (modified) llvm/include/llvm/ObjectYAML/MinidumpYAML.h (+28)
• (modified) llvm/lib/Object/Minidump.cpp (+17-2)
• (modified) llvm/lib/ObjectYAML/MinidumpEmitter.cpp (+11)
• (modified) llvm/lib/ObjectYAML/MinidumpYAML.cpp (+34)

diff --git a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp
index de212c6b20da7..b941748bd7ccb 100644
--- a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp
+++ b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp
@@ -1020,15 +1020,17 @@ MinidumpFileBuilder::AddMemoryList_32(Process::CoreFileMemoryRanges &ranges) {
   // With a size of the number of ranges as a 32 bit num
   // And then the size of all the ranges
   error = AddDirectory(StreamType::MemoryList,
-                       sizeof(llvm::support::ulittle32_t) +
+                       sizeof(llvm::minidump::MemoryListHeader) +
                            descriptors.size() *
                                sizeof(llvm::minidump::MemoryDescriptor));
   if (error.Fail())
     return error;
 
+  llvm::minidump::MemoryListHeader list_header;
   llvm::support::ulittle32_t memory_ranges_num =
       static_cast<llvm::support::ulittle32_t>(descriptors.size());
-  m_data.AppendData(&memory_ranges_num, sizeof(llvm::support::ulittle32_t));
+  list_header.NumberOfMemoryRanges = memory_ranges_num;
+  m_data.AppendData(&list_header, sizeof(llvm::minidump::MemoryListHeader));
   // For 32b we can get away with writing off the descriptors after the data.
   // This means no cleanup loop needed.
   m_data.AppendData(descriptors.data(),
@@ -1050,9 +1052,10 @@ MinidumpFileBuilder::AddMemoryList_64(Process::CoreFileMemoryRanges &ranges) {
   if (error.Fail())
     return error;
 
+  llvm::minidump::Memory64ListHeader list_header;
   llvm::support::ulittle64_t memory_ranges_num =
       static_cast<llvm::support::ulittle64_t>(ranges.size());
-  m_data.AppendData(&memory_ranges_num, sizeof(llvm::support::ulittle64_t));
+  list_header.NumberOfMemoryRanges = memory_ranges_num;
   // Capture the starting offset for all the descriptors so we can clean them up
   // if needed.
   offset_t starting_offset =
@@ -1064,8 +1067,8 @@ MinidumpFileBuilder::AddMemoryList_64(Process::CoreFileMemoryRanges &ranges) {
       (ranges.size() * sizeof(llvm::minidump::MemoryDescriptor_64));
   llvm::support::ulittle64_t memory_ranges_base_rva =
       static_cast<llvm::support::ulittle64_t>(base_rva);
-  m_data.AppendData(&memory_ranges_base_rva,
-                    sizeof(llvm::support::ulittle64_t));
+  list_header.BaseRVA = memory_ranges_base_rva;
+  m_data.AppendData(&list_header, sizeof(llvm::minidump::Memory64ListHeader));
 
   bool cleanup_required = false;
   std::vector<MemoryDescriptor_64> descriptors;
diff --git a/lldb/source/Plugins/Process/minidump/MinidumpParser.cpp b/lldb/source/Plugins/Process/minidump/MinidumpParser.cpp
index be9fae938e227..7e19acd3d0834 100644
--- a/lldb/source/Plugins/Process/minidump/MinidumpParser.cpp
+++ b/lldb/source/Plugins/Process/minidump/MinidumpParser.cpp
@@ -553,43 +553,45 @@ static bool
 CreateRegionsCacheFromMemoryList(MinidumpParser &parser,
                                  std::vector<MemoryRegionInfo> &regions) {
   Log *log = GetLog(LLDBLog::Modules);
+  // Cache the expected memory32 into an optional
+  // because double checking the expected triggers the unchecked warning.
+  std::optional<llvm::ArrayRef<MemoryDescriptor>> memory32_list;
   auto ExpectedMemory = parser.GetMinidumpFile().getMemoryList();
   if (!ExpectedMemory) {
     LLDB_LOG_ERROR(log, ExpectedMemory.takeError(),
                    "Failed to read memory list: {0}");
-    return false;
-  }
-  regions.reserve(ExpectedMemory->size());
-  for (const MemoryDescriptor &memory_desc : *ExpectedMemory) {
-    if (memory_desc.Memory.DataSize == 0)
-      continue;
-    MemoryRegionInfo region;
-    region.GetRange().SetRangeBase(memory_desc.StartOfMemoryRange);
-    region.GetRange().SetByteSize(memory_desc.Memory.DataSize);
-    region.SetReadable(MemoryRegionInfo::eYes);
-    region.SetMapped(MemoryRegionInfo::eYes);
-    regions.push_back(region);
+  } else {
+    memory32_list = *ExpectedMemory;
   }
-  regions.shrink_to_fit();
-  return !regions.empty();
-}
 
-static bool
-CreateRegionsCacheFromMemory64List(MinidumpParser &parser,
-                                   std::vector<MemoryRegionInfo> &regions) {
+  size_t num_regions = memory32_list ? memory32_list->size() : 0;
+
   llvm::ArrayRef<uint8_t> data =
       parser.GetStream(StreamType::Memory64List);
-  if (data.empty())
-    return false;
+
   llvm::ArrayRef<MinidumpMemoryDescriptor64> memory64_list;
-  uint64_t base_rva;
-  std::tie(memory64_list, base_rva) =
-      MinidumpMemoryDescriptor64::ParseMemory64List(data);
+  if (!data.empty()) {
+    uint64_t base_rva;
+    std::tie(memory64_list, base_rva) =
+        MinidumpMemoryDescriptor64::ParseMemory64List(data);
 
-  if (memory64_list.empty())
-    return false;
+    num_regions += memory64_list.size();
+  }
+
+  regions.reserve(num_regions);
+  if (memory32_list) {
+    for (const MemoryDescriptor &memory_desc : *memory32_list) {
+      if (memory_desc.Memory.DataSize == 0)
+        continue;
+      MemoryRegionInfo region;
+      region.GetRange().SetRangeBase(memory_desc.StartOfMemoryRange);
+      region.GetRange().SetByteSize(memory_desc.Memory.DataSize);
+      region.SetReadable(MemoryRegionInfo::eYes);
+      region.SetMapped(MemoryRegionInfo::eYes);
+      regions.push_back(region);
+    }
+  }
 
-  regions.reserve(memory64_list.size());
   for (const auto &memory_desc : memory64_list) {
     if (memory_desc.data_size == 0)
       continue;
@@ -620,9 +622,7 @@ std::pair<MemoryRegionInfos, bool> MinidumpParser::BuildMemoryRegions() {
     return return_sorted(true);
   if (CreateRegionsCacheFromMemoryInfoList(*this, result))
     return return_sorted(true);
-  if (CreateRegionsCacheFromMemoryList(*this, result))
-    return return_sorted(false);
-  CreateRegionsCacheFromMemory64List(*this, result);
+  CreateRegionsCacheFromMemoryList(*this, result);
   return return_sorted(false);
 }
 
diff --git a/lldb/test/API/functionalities/postmortem/minidump-new/TestMiniDumpNew.py b/lldb/test/API/functionalities/postmortem/minidump-new/TestMiniDumpNew.py
index 91fd2439492b5..03f4de4120da4 100644
--- a/lldb/test/API/functionalities/postmortem/minidump-new/TestMiniDumpNew.py
+++ b/lldb/test/API/functionalities/postmortem/minidump-new/TestMiniDumpNew.py
@@ -491,3 +491,16 @@ def test_minidump_sysroot(self):
         spec_dir_norm = os.path.normcase(module.GetFileSpec().GetDirectory())
         exe_dir_norm = os.path.normcase(exe_dir)
         self.assertEqual(spec_dir_norm, exe_dir_norm)
+
+    def test_minidump_memory64list(self):
+        """Test that lldb can read from the memory64list in a minidump."""
+        self.process_from_yaml("linux-x86_64_mem64.yaml")
+
+        region_count = 1
+        region_info_list = self.process.GetMemoryRegions()
+        self.assertEqual(region_info_list.GetSize(), region_count)
+
+        region = lldb.SBMemoryRegionInfo()
+        self.assertTrue(region_info_list.GetMemoryRegionAtIndex(0, region))
+        self.assertEqual(region.GetRegionBase(), 0x7FFF12A84030)
+        self.assertTrue(region.GetRegionEnd(), 0x2FD0)
diff --git a/lldb/test/API/functionalities/postmortem/minidump-new/linux-x86_64_mem64.dmp b/lldb/test/API/functionalities/postmortem/minidump-new/linux-x86_64_mem64.dmp
new file mode 100644
index 0000000000000..a1069ef45ad35
Binary files /dev/null and b/lldb/test/API/functionalities/postmortem/minidump-new/linux-x86_64_mem64.dmp differ
diff --git a/lldb/test/API/functionalities/postmortem/minidump-new/linux-x86_64_mem64.yaml b/lldb/test/API/functionalities/postmortem/minidump-new/linux-x86_64_mem64.yaml
new file mode 100644
index 0000000000000..2c0687eeaaeb7
--- /dev/null
+++ b/lldb/test/API/functionalities/postmortem/minidump-new/linux-x86_64_mem64.yaml
@@ -0,0 +1,36 @@
+--- !minidump
+Streams:
+  - Type:            SystemInfo
+    Processor Arch:  AMD64
+    Processor Level: 6
+    Processor Revision: 15876
+    Number of Processors: 40
+    Platform ID:     Linux
+    CSD Version:     'Linux 3.13.0-91-generic'
+    CPU:
+      Vendor ID:       GenuineIntel
+      Version Info:    0x00000000
+      Feature Info:    0x00000000
+  - Type:            LinuxProcStatus
+    Text:             |
+      Name:	linux-x86_64
+      State:	t (tracing stop)
+      Tgid:	29917
+      Ngid:	0
+      Pid:	29917
+      PPid:	29370
+      TracerPid:	29918
+      Uid:	1001	1001	1001	1001
+      Gid:	1001	1001	1001	1001
+  - Type:            ThreadList
+    Threads:
+      - Thread Id:       0x2896BB
+        Context:         0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000700100000000000FFFFFFFF0000FFFFFFFFFFFFFFFFFFFF0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000B040A812FF7F00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000050D0A75BBA7F00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
+        Stack:
+          Start of Memory Range: 0x0
+          Content:         ''
+  - Type:            Memory64List
+    Memory Ranges:
+      - Start of memory range: 0x7FFF12A84030
+        Data Size:       0x2FD0
+...
diff --git a/llvm/include/llvm/BinaryFormat/Minidump.h b/llvm/include/llvm/BinaryFormat/Minidump.h
index 9669303252615..8054e81322a92 100644
--- a/llvm/include/llvm/BinaryFormat/Minidump.h
+++ b/llvm/include/llvm/BinaryFormat/Minidump.h
@@ -74,6 +74,18 @@ struct MemoryDescriptor_64 {
   support::ulittle64_t StartOfMemoryRange;
   support::ulittle64_t DataSize;
 };
+static_assert(sizeof(MemoryDescriptor_64) == 16);
+
+struct MemoryListHeader {
+  support::ulittle32_t NumberOfMemoryRanges;
+};
+static_assert(sizeof(MemoryListHeader) == 4);
+
+struct Memory64ListHeader {
+  support::ulittle64_t NumberOfMemoryRanges;
+  support::ulittle64_t BaseRVA;
+};
+static_assert(sizeof(Memory64ListHeader) == 16);
 
 struct MemoryInfoListHeader {
   support::ulittle32_t SizeOfHeader;
diff --git a/llvm/include/llvm/Object/Minidump.h b/llvm/include/llvm/Object/Minidump.h
index e45d4de0090de..6ca9eb2afe150 100644
--- a/llvm/include/llvm/Object/Minidump.h
+++ b/llvm/include/llvm/Object/Minidump.h
@@ -103,6 +103,15 @@ class MinidumpFile : public Binary {
         minidump::StreamType::MemoryList);
   }
 
+  /// Returns the header to the memory 64 list stream. An error is returned if
+  /// the file does not contain this stream.
+  Expected<minidump::Memory64ListHeader> getMemoryList64Header() const {
+    return getStream<minidump::Memory64ListHeader>(
+        minidump::StreamType::Memory64List);
+  }
+
+  Expected<ArrayRef<minidump::MemoryDescriptor_64>> getMemory64List() const;
+
   class MemoryInfoIterator
       : public iterator_facade_base<MemoryInfoIterator,
                                     std::forward_iterator_tag,
@@ -152,15 +161,15 @@ class MinidumpFile : public Binary {
   }
 
   /// Return a slice of the given data array, with bounds checking.
-  static Expected<ArrayRef<uint8_t>> getDataSlice(ArrayRef<uint8_t> Data,
-                                                  size_t Offset, size_t Size);
+  static Expected<ArrayRef<uint8_t>>
+  getDataSlice(ArrayRef<uint8_t> Data, uint64_t Offset, uint64_t Size);
 
   /// Return the slice of the given data array as an array of objects of the
   /// given type. The function checks that the input array is large enough to
   /// contain the correct number of objects of the given type.
   template <typename T>
   static Expected<ArrayRef<T>> getDataSliceAs(ArrayRef<uint8_t> Data,
-                                              size_t Offset, size_t Count);
+                                              uint64_t Offset, uint64_t Count);
 
   MinidumpFile(MemoryBufferRef Source, const minidump::Header &Header,
                ArrayRef<minidump::Directory> Streams,
@@ -208,6 +217,7 @@ Expected<ArrayRef<T>> MinidumpFile::getDataSliceAs(ArrayRef<uint8_t> Data,
       getDataSlice(Data, Offset, sizeof(T) * Count);
   if (!Slice)
     return Slice.takeError();
+
   return ArrayRef<T>(reinterpret_cast<const T *>(Slice->data()), Count);
 }
 
diff --git a/llvm/include/llvm/ObjectYAML/MinidumpYAML.h b/llvm/include/llvm/ObjectYAML/MinidumpYAML.h
index b0cee541cef20..d6aab6df1ca6f 100644
--- a/llvm/include/llvm/ObjectYAML/MinidumpYAML.h
+++ b/llvm/include/llvm/ObjectYAML/MinidumpYAML.h
@@ -29,6 +29,7 @@ struct Stream {
     Exception,
     MemoryInfoList,
     MemoryList,
+    Memory64List,
     ModuleList,
     RawContent,
     SystemInfo,
@@ -104,6 +105,25 @@ using ModuleListStream = detail::ListStream<detail::ParsedModule>;
 using ThreadListStream = detail::ListStream<detail::ParsedThread>;
 using MemoryListStream = detail::ListStream<detail::ParsedMemoryDescriptor>;
 
+/// Memory64ListStream minidump stream.
+struct Memory64ListStream : public Stream {
+  std::vector<minidump::MemoryDescriptor_64> Entries;
+  yaml::BinaryRef Content;
+  minidump::Memory64ListHeader Header;
+
+  Memory64ListStream()
+      : Stream(StreamKind::Memory64List, minidump::StreamType::Memory64List) {}
+
+  explicit Memory64ListStream(
+      std::vector<minidump::MemoryDescriptor_64> Entries)
+      : Stream(StreamKind::Memory64List, minidump::StreamType::Memory64List),
+        Entries(Entries) {}
+
+  static bool classof(const Stream *S) {
+    return S->Kind == StreamKind::Memory64List;
+  }
+};
+
 /// ExceptionStream minidump stream.
 struct ExceptionStream : public Stream {
   minidump::ExceptionStream MDExceptionStream;
@@ -244,6 +264,12 @@ template <> struct MappingContextTraits<minidump::MemoryDescriptor, BinaryRef> {
                       BinaryRef &Content);
 };
 
+template <>
+struct MappingContextTraits<minidump::MemoryDescriptor_64, BinaryRef> {
+  static void mapping(IO &IO, minidump::MemoryDescriptor_64 &Memory,
+                      BinaryRef &Content);
+};
+
 } // namespace yaml
 
 } // namespace llvm
@@ -262,6 +288,7 @@ LLVM_YAML_DECLARE_MAPPING_TRAITS(llvm::minidump::CPUInfo::X86Info)
 LLVM_YAML_DECLARE_MAPPING_TRAITS(llvm::minidump::Exception)
 LLVM_YAML_DECLARE_MAPPING_TRAITS(llvm::minidump::MemoryInfo)
 LLVM_YAML_DECLARE_MAPPING_TRAITS(llvm::minidump::VSFixedFileInfo)
+LLVM_YAML_DECLARE_MAPPING_TRAITS(llvm::minidump::MemoryDescriptor_64)
 
 LLVM_YAML_DECLARE_MAPPING_TRAITS(
     llvm::MinidumpYAML::MemoryListStream::entry_type)
@@ -275,6 +302,7 @@ LLVM_YAML_IS_SEQUENCE_VECTOR(llvm::MinidumpYAML::MemoryListStream::entry_type)
 LLVM_YAML_IS_SEQUENCE_VECTOR(llvm::MinidumpYAML::ModuleListStream::entry_type)
 LLVM_YAML_IS_SEQUENCE_VECTOR(llvm::MinidumpYAML::ThreadListStream::entry_type)
 LLVM_YAML_IS_SEQUENCE_VECTOR(llvm::minidump::MemoryInfo)
+LLVM_YAML_IS_SEQUENCE_VECTOR(llvm::minidump::MemoryDescriptor_64)
 
 LLVM_YAML_DECLARE_MAPPING_TRAITS(llvm::MinidumpYAML::Object)
 
diff --git a/llvm/lib/Object/Minidump.cpp b/llvm/lib/Object/Minidump.cpp
index 6febff89ac519..92aa4f66a4b35 100644
--- a/llvm/lib/Object/Minidump.cpp
+++ b/llvm/lib/Object/Minidump.cpp
@@ -99,8 +99,9 @@ template Expected<ArrayRef<Thread>>
 template Expected<ArrayRef<MemoryDescriptor>>
     MinidumpFile::getListStream(StreamType) const;
 
-Expected<ArrayRef<uint8_t>>
-MinidumpFile::getDataSlice(ArrayRef<uint8_t> Data, size_t Offset, size_t Size) {
+Expected<ArrayRef<uint8_t>> MinidumpFile::getDataSlice(ArrayRef<uint8_t> Data,
+                                                       uint64_t Offset,
+                                                       uint64_t Size) {
   // Check for overflow.
   if (Offset + Size < Offset || Offset + Size < Size ||
       Offset + Size > Data.size())
@@ -154,3 +155,17 @@ MinidumpFile::create(MemoryBufferRef Source) {
   return std::unique_ptr<MinidumpFile>(
       new MinidumpFile(Source, Hdr, *ExpectedStreams, std::move(StreamMap)));
 }
+
+Expected<ArrayRef<MemoryDescriptor_64>> MinidumpFile::getMemory64List() const {
+  Expected<minidump::Memory64ListHeader> MemoryList64 = getMemoryList64Header();
+  if (!MemoryList64)
+    return MemoryList64.takeError();
+
+  std::optional<ArrayRef<uint8_t>> Stream =
+      getRawStream(StreamType::Memory64List);
+  if (!Stream)
+    return createError("No such stream");
+
+  return getDataSliceAs<minidump::MemoryDescriptor_64>(
+      *Stream, sizeof(Memory64ListHeader), MemoryList64->NumberOfMemoryRanges);
+}
diff --git a/llvm/lib/ObjectYAML/MinidumpEmitter.cpp b/llvm/lib/ObjectYAML/MinidumpEmitter.cpp
index 24b521a9925c7..f992c4e3bde76 100644
--- a/llvm/lib/ObjectYAML/MinidumpEmitter.cpp
+++ b/llvm/lib/ObjectYAML/MinidumpEmitter.cpp
@@ -136,6 +136,14 @@ static size_t layout(BlobAllocator &File, MinidumpYAML::ExceptionStream &S) {
   return DataEnd;
 }
 
+static void layout(BlobAllocator &File, MinidumpYAML::Memory64ListStream &S) {
+  size_t BaseRVA = File.tell() + sizeof(minidump::Memory64ListHeader);
+  S.Header.BaseRVA = BaseRVA;
+  S.Header.NumberOfMemoryRanges = S.Entries.size();
+  File.allocateObject(S.Header);
+  File.allocateArray(ArrayRef(S.Entries));
+}
+
 static void layout(BlobAllocator &File, MemoryListStream::entry_type &Range) {
   Range.Entry.Memory = layout(File, Range.Content);
 }
@@ -190,6 +198,9 @@ static Directory layout(BlobAllocator &File, Stream &S) {
   case Stream::StreamKind::MemoryList:
     DataEnd = layout(File, cast<MemoryListStream>(S));
     break;
+  case Stream::StreamKind::Memory64List:
+    layout(File, cast<Memory64ListStream>(S));
+    break;
   case Stream::StreamKind::ModuleList:
     DataEnd = layout(File, cast<ModuleListStream>(S));
     break;
diff --git a/llvm/lib/ObjectYAML/MinidumpYAML.cpp b/llvm/lib/ObjectYAML/MinidumpYAML.cpp
index fdbd2d8e6dcbc..aee17c520a010 100644
--- a/llvm/lib/ObjectYAML/MinidumpYAML.cpp
+++ b/llvm/lib/ObjectYAML/MinidumpYAML.cpp
@@ -75,6 +75,8 @@ Stream::StreamKind Stream::getKind(StreamType Type) {
     return StreamKind::MemoryInfoList;
   case StreamType::MemoryList:
     return StreamKind::MemoryList;
+  case StreamType::Memory64List:
+    return StreamKind::Memory64List;
   case StreamType::ModuleList:
     return StreamKind::ModuleList;
   case StreamType::SystemInfo:
@@ -103,6 +105,8 @@ std::unique_ptr<Stream> Stream::create(StreamType Type) {
     return std::make_unique<MemoryInfoListStream>();
   case StreamKind::MemoryList:
     return std::make_unique<MemoryListStream>();
+  case StreamKind::Memory64List:
+    return std::make_unique<Memory64ListStream>();
   case StreamKind::ModuleList:
     return std::make_unique<ModuleListStream>();
   case StreamKind::RawContent:
@@ -256,6 +260,12 @@ void yaml::MappingTraits<MemoryInfo>::mapping(IO &IO, MemoryInfo &Info) {
   mapOptionalHex(IO, "Reserved1", Info.Reserved1, 0);
 }
 
+void yaml::MappingTraits<MemoryDescriptor_64>::mapping(
+    IO &IO, MemoryDescriptor_64 &Mem) {
+  mapRequiredHex(IO, "Start of memory range", Mem.StartOfMemoryRange);
+  mapRequiredHex(IO, "Data Size", Mem.DataSize);
+}
+
 void yaml::MappingTraits<VSFixedFileInfo>::mapping(IO &IO,
                                                    VSFixedFileInfo &Info) {
   mapOptionalHex(IO, "Signature", Info.Signature, 0);
@@ -312,6 +322,10 @@ static void streamMapping(yaml::IO &IO, MemoryListStream &Stream) {
   IO.mapRequired("Memory Ranges", Stream.Entries);
 }
 
+static void streamMapping(yaml::IO &IO, Memory64ListStream &Stream) {
+  IO.mapRequired("Memory Ranges", Stream.Entries);
+}
+
 static void streamMapping(yaml::IO &IO, ModuleListStream &Stream) {
   IO.mapRequired("Modules", ...
[truncated]

[Jlalond]

@labath Would you mind if I added you as a reviewer for the Obj2Yaml changes for minidump?

[clayborg]

There must be some minidump yaml tests somewhere. We should add some to test the YAML support. Also need to test for multiple MemoryDescriptor_64 entries in a file using your new YAML.

[clayborg]

move this up two lines to appear before Entries?

[Jlalond]

Good recommendation, added it after my footgun.

[clayborg]

The 32 bit memory list just does:

  Expected<ArrayRef<minidump::MemoryDescriptor>> getMemoryList() const {
    return getListStream<minidump::MemoryDescriptor>(
        minidump::StreamType::MemoryList);
  }

Should this be the same kind of thing:

  Expected<ArrayRef<minidump:: MemoryDescriptor_64>> getMemoryList64() const {
    return getListStream< minidump::MemoryDescriptor_64>(
        minidump::StreamType::MemoryList_64);
  }

[Jlalond]

Correct, but getListStream() makes an assumption all lists are

4 byte size
[Content]

Vs the Memory64List

16 byte header
[Content]

So we cannot reuse this call

[labath]

Makes sense. I'd just drop a small comment about this into the code.

[clayborg]

This gets just a single memory descriptor and it needs to be an array. There should be a test for this to ensure we can load multiple 64 bit ranges where you use your YAML to produce a small core file with only a few memory ranges that are in the MemoryDescriptor_64 formats and verify we can read the data.

[Jlalond]

getDataSlice under the hood does a reinterpret cast of an array of uint8_t to a pointer of your respective type, and then makes an array ref out of that. So this does in fact return an array

It is NOT clear that getDataSliceAs always returns an ArrayRef to <T> IMO.

[labath]

It is NOT clear that getDataSliceAs always returns an ArrayRef to IMO.

I'd be open to changing this around to clarify things (in another patch). I'm sure it was clear to me when I was writing this, but I agree that may not be the case for a casual reader.

[Jlalond]

I was a bit flippant, but I think the only confusion for me is that the pointers are always being converted to an ArrayRef,

getDataSliceAsArrayOf<> could work but is particularly verbose

[Jlalond]

There must be some minidump yaml tests somewhere. We should add some to test the YAML support. Also need to test for multiple MemoryDescriptor_64 entries in a file using your new YAML.

Anything in particular you have in mind? All the tests in TestMinidumpNew call yaml2obj to generate every test run. I can add more Mem64 tests if that is what you'd like

[github-actions]

⚠️ Python code formatter, darker found issues in your code. ⚠️

You can test this locally with the following command:

darker --check --diff -r 4e078e3797098daa40d254447c499bcf61415308...32e008c9d3efe3fd74a0091b2db4153046f7b694 lldb/test/API/functionalities/postmortem/minidump-new/TestMiniDumpNew.py

View the diff from darker here.

--- TestMiniDumpNew.py	2024-08-15 21:08:25.000000 +0000
+++ TestMiniDumpNew.py	2024-08-16 15:10:33.435957 +0000
@@ -503,10 +503,10 @@
         region = lldb.SBMemoryRegionInfo()
         self.assertTrue(region_info_list.GetMemoryRegionAtIndex(0, region))
         self.assertEqual(region.GetRegionBase(), 0x7FFF12A84030)
         self.assertTrue(region.GetRegionEnd(), 0x7FFF12A84030 + 0x2FD0)
         self.assertTrue(region_info_list.GetMemoryRegionAtIndex(1, region))
-        self.assertEqual(region.GetRegionBase(), 0x00007fff12a87000)
-        self.assertTrue(region.GetRegionEnd(), 0x00007fff12a87000 + 0x00000018)
+        self.assertEqual(region.GetRegionBase(), 0x00007FFF12A87000)
+        self.assertTrue(region.GetRegionEnd(), 0x00007FFF12A87000 + 0x00000018)
         self.assertTrue(region_info_list.GetMemoryRegionAtIndex(2, region))
-        self.assertEqual(region.GetRegionBase(), 0x00007fff12a87018)
-        self.assertTrue(region.GetRegionEnd(), 0x00007fff12a87018 + 0x00000400)
+        self.assertEqual(region.GetRegionBase(), 0x00007FFF12A87018)
+        self.assertTrue(region.GetRegionEnd(), 0x00007FFF12A87018 + 0x00000400)

[labath]

There must be some minidump yaml tests somewhere. We should add some to test the YAML support. Also need to test for multiple MemoryDescriptor_64 entries in a file using your new YAML.

There are tests in test/tools/yaml2obj/Minidump/, which test the yaml writing functionality, test/tools/obj2yaml/Minidump/basic.yaml tests yaml->obj->yaml round-tripping, and also unittests/ObjectYAML/MinidumpYAMLTest.cpp which tests the object API. You could probably sprinkle some references to the new stream type to each of these.

@labath Would you mind if I added you as a reviewer for the Obj2Yaml changes for minidump?

Sounds good. Could you split off the lldb parts to a separate review though?

[labath]

It is NOT clear that getDataSliceAs always returns an ArrayRef to IMO.

I'd be open to changing this around to clarify things (in another patch). I'm sure it was clear to me when I was writing this, but I agree that may not be the case for a casual reader.

[labath]

Makes sense. I'd just drop a small comment about this into the code.

[labath]

Suggested change

	std::optional<ArrayRef<uint8_t>> Stream =
	getRawStream(StreamType::Memory64List);
	if (!Stream)
	return createError("No such stream");
	ArrayRef<uint8_t> Stream =
	getRawStream(StreamType::Memory64List).value();

The error can never happen as the first call checks for stream existence.

I can also imagine a pattern like:

Stream = getRawStream(...);
Header = getDataSliceAs<Header>(Stream, 0, 1);
return getDataSliceAs<List>(Stream, sizeof(Header), Header->NumberOfMemoryRanges);

which might make it clearer that the header and body come from the same stream.

[Jlalond]

Sounds good. Could you split off the lldb parts to a separate review though?

@labath I think we need both, in order to fix SBProcess to return all memory regions we need the LLDB change, which enables us to test if the yaml2obj generates correctly

[labath]

Sounds good. Could you split off the lldb parts to a separate review though?

@labath I think we need both, in order to fix SBProcess to return all memory regions we need the LLDB change, which enables us to test if the yaml2obj generates correctly

I can believe you need the llvm changes for the lldb test. That's fine and expected. You can make those changes on a separate PR. However, there shouldn't code in llvm which is only tested from lldb. That's why I pointed you to all the existing tests for yaml2minidump functionality in llvm. You can test the llvm functionality through those.

[github-actions]

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:

git-clang-format --diff 4e078e3797098daa40d254447c499bcf61415308 32e008c9d3efe3fd74a0091b2db4153046f7b694 --extensions cpp,h -- lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp lldb/source/Plugins/Process/minidump/MinidumpParser.cpp lldb/source/Plugins/Process/minidump/MinidumpParser.h lldb/source/Plugins/Process/minidump/MinidumpTypes.cpp lldb/source/Plugins/Process/minidump/MinidumpTypes.h

View the diff from clang-format here.

diff --git a/lldb/source/Plugins/Process/minidump/MinidumpParser.cpp b/lldb/source/Plugins/Process/minidump/MinidumpParser.cpp
index c099c28a62..7518332b0d 100644
--- a/lldb/source/Plugins/Process/minidump/MinidumpParser.cpp
+++ b/lldb/source/Plugins/Process/minidump/MinidumpParser.cpp
@@ -458,10 +458,12 @@ MinidumpParser::FindMemoryRange(lldb::addr_t addr) {
 
   if (!GetStream(StreamType::Memory64List).empty()) {
     llvm::Error err = llvm::Error::success();
-    for (const auto &memory_desc :  GetMinidumpFile().getMemory64List(err)) {
-      if (memory_desc.first.StartOfMemoryRange <= addr 
-          && addr < memory_desc.first.StartOfMemoryRange + memory_desc.first.DataSize) {
-        return minidump::Range(memory_desc.first.StartOfMemoryRange, memory_desc.second);
+    for (const auto &memory_desc : GetMinidumpFile().getMemory64List(err)) {
+      if (memory_desc.first.StartOfMemoryRange <= addr &&
+          addr < memory_desc.first.StartOfMemoryRange +
+                     memory_desc.first.DataSize) {
+        return minidump::Range(memory_desc.first.StartOfMemoryRange,
+                               memory_desc.second);
       }
     }
 
@@ -495,7 +497,8 @@ llvm::ArrayRef<uint8_t> MinidumpParser::GetMemory(lldb::addr_t addr,
   return range->range_ref.slice(offset, overlap);
 }
 
-llvm::iterator_range<FallibleMemory64Iterator> MinidumpParser::GetMemory64Iterator(llvm::Error &err) {
+llvm::iterator_range<FallibleMemory64Iterator>
+MinidumpParser::GetMemory64Iterator(llvm::Error &err) {
   llvm::ErrorAsOutParameter ErrAsOutParam(&err);
   return m_file->getMemory64List(err);
 }
diff --git a/lldb/source/Plugins/Process/minidump/MinidumpParser.h b/lldb/source/Plugins/Process/minidump/MinidumpParser.h
index 222c0ef47f..46596081eb 100644
--- a/lldb/source/Plugins/Process/minidump/MinidumpParser.h
+++ b/lldb/source/Plugins/Process/minidump/MinidumpParser.h
@@ -47,7 +47,8 @@ struct Range {
   }
 };
 
-using FallibleMemory64Iterator = llvm::object::MinidumpFile::FallibleMemory64Iterator;
+using FallibleMemory64Iterator =
+    llvm::object::MinidumpFile::FallibleMemory64Iterator;
 
 class MinidumpParser {
 public:
@@ -94,7 +95,8 @@ public:
   /// complete (includes all regions mapped into the process memory).
   std::pair<MemoryRegionInfos, bool> BuildMemoryRegions();
 
-  llvm::iterator_range<FallibleMemory64Iterator> GetMemory64Iterator(llvm::Error &err);
+  llvm::iterator_range<FallibleMemory64Iterator>
+  GetMemory64Iterator(llvm::Error &err);
 
   static llvm::StringRef GetStreamTypeAsString(StreamType stream_type);
 

[Jlalond]

@labath as an FYI, using the new fallible iterator API

[clayborg]

Looks good to me. Lets give Pavel a day to respond, if not I will approve.

[labath]

A couple of small things, but I think it's fine overall. I'm going to be OOO next week, no need to wait for me.

[Jlalond]

@labath appreciate the review, I've addressed the points. CC: @clayborg