[Lldb-commits] [lldb] [llvm] [LLDB][Minidump] Add 64b support to LLDB's minidump file builder. (PR #95312)

Jacob Lalonde via lldb-commits lldb-commits at lists.llvm.org
Thu Jun 20 17:17:45 PDT 2024


https://github.com/Jlalond updated https://github.com/llvm/llvm-project/pull/95312

>From 810865580a4d8ea3f48e56f3964602fba6087ddf Mon Sep 17 00:00:00 2001
From: Jacob Lalonde <jalalonde at fb.com>
Date: Wed, 12 Jun 2024 13:57:10 -0700
Subject: [PATCH 01/14] Squash minidump-64-protoype.

This is a change to reorder LLDB's minidumps to support 64b memory lists/minidumps while still keeping all directories under the 32b limit.

Better O(N), not N^2 cleanup code
---
 .../Minidump/MinidumpFileBuilder.cpp          | 520 ++++++++++++++----
 .../ObjectFile/Minidump/MinidumpFileBuilder.h |  55 +-
 .../Minidump/ObjectFileMinidump.cpp           |  38 +-
 llvm/include/llvm/BinaryFormat/Minidump.h     |  11 +
 llvm/include/llvm/Object/Minidump.h           |   7 +
 llvm/lib/ObjectYAML/MinidumpEmitter.cpp       |   7 +
 6 files changed, 482 insertions(+), 156 deletions(-)

diff --git a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp
index 7231433619ffb..c08b5206720ab 100644
--- a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp
+++ b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp
@@ -20,35 +20,106 @@
 #include "lldb/Target/RegisterContext.h"
 #include "lldb/Target/StopInfo.h"
 #include "lldb/Target/ThreadList.h"
+#include "lldb/Utility/DataBufferHeap.h"
 #include "lldb/Utility/DataExtractor.h"
 #include "lldb/Utility/LLDBLog.h"
 #include "lldb/Utility/Log.h"
+#include "lldb/Utility/LLDBLog.h"
 #include "lldb/Utility/RegisterValue.h"
 
 #include "llvm/ADT/StringRef.h"
 #include "llvm/BinaryFormat/Minidump.h"
 #include "llvm/Support/ConvertUTF.h"
+#include "llvm/Support/Endian.h"
 #include "llvm/Support/Error.h"
 
 #include "Plugins/Process/minidump/MinidumpTypes.h"
+#include "lldb/lldb-enumerations.h"
+#include "lldb/lldb-forward.h"
+#include "lldb/lldb-types.h"
 
 #include <cinttypes>
+#include <climits>
+#include <cstdint>
+#include <iostream>
+#include <utility>
+#include <vector>
 
 using namespace lldb;
 using namespace lldb_private;
 using namespace llvm::minidump;
 
-void MinidumpFileBuilder::AddDirectory(StreamType type, size_t stream_size) {
+Status MinidumpFileBuilder::AddHeaderAndCalculateDirectories(const lldb_private::Target &target, const lldb::ProcessSP &process_sp) {
+  // First set the offset on the file, and on the bytes saved
+  //TODO: advance the core file.
+  m_saved_data_size += header_size;
+  // We know we will have at least Misc, SystemInfo, Modules, and ThreadList (corresponding memory list for stacks)
+  // And an additional memory list for non-stacks.
+  m_expected_directories = 6;
+  // Check if OS is linux
+  if (target.GetArchitecture().GetTriple().getOS() ==
+      llvm::Triple::OSType::Linux)
+    m_expected_directories += 9;
+
+  // Go through all of the threads and check for exceptions.
+  lldb_private::ThreadList thread_list = process_sp->GetThreadList();
+  const uint32_t num_threads = thread_list.GetSize();
+  for (uint32_t thread_idx = 0; thread_idx < num_threads; ++thread_idx) {
+    ThreadSP thread_sp(thread_list.GetThreadAtIndex(thread_idx));
+    StopInfoSP stop_info_sp = thread_sp->GetStopInfo();
+    if (stop_info_sp && stop_info_sp->GetStopReason() == StopReason::eStopReasonException) {
+      m_expected_directories++;
+    }
+  }
+
+  // Now offset the file by the directores so we can write them in later.
+  offset_t directory_offset = m_expected_directories * directory_size;
+  //TODO: advance the core file.
+  m_saved_data_size += directory_offset;
+  // Replace this when we make a better way to do this.
+  Status error;
+  Header empty_header;
+  size_t bytes_written;
+  bytes_written = header_size;
+  error = m_core_file->Write(&empty_header, bytes_written);
+  if (error.Fail() || bytes_written != header_size) {
+    if (bytes_written != header_size)
+      error.SetErrorStringWithFormat(
+          "unable to write the header (written %zd/%zd)", bytes_written,
+          header_size);
+    return error;
+  }
+
+  for (uint i = 0; i < m_expected_directories; i++) {
+    size_t bytes_written;
+    bytes_written = directory_size;
+    Directory empty_directory;
+    error = m_core_file->Write(&empty_directory, bytes_written);
+    if (error.Fail() || bytes_written != directory_size) {
+      if (bytes_written != directory_size)
+        error.SetErrorStringWithFormat(
+            "unable to write the directory (written %zd/%zd)", bytes_written,
+            directory_size);
+      return error;
+    }
+  }
+
+  return error;
+}
+
+
+void MinidumpFileBuilder::AddDirectory(StreamType type, uint64_t stream_size) {
   LocationDescriptor loc;
-  loc.DataSize = static_cast<llvm::support::ulittle32_t>(stream_size);
+  loc.DataSize = static_cast<llvm::support::ulittle64_t>(stream_size);
   // Stream will begin at the current end of data section
-  loc.RVA = static_cast<llvm::support::ulittle32_t>(GetCurrentDataEndOffset());
+  loc.RVA = static_cast<llvm::support::ulittle64_t>(GetCurrentDataEndOffset());
 
   Directory dir;
   dir.Type = static_cast<llvm::support::little_t<StreamType>>(type);
   dir.Location = loc;
 
   m_directories.push_back(dir);
+  assert (m_expected_directories >= m_directories.size());
 }
 
 Status MinidumpFileBuilder::AddSystemInfo(const llvm::Triple &target_triple) {
@@ -114,7 +185,7 @@ Status MinidumpFileBuilder::AddSystemInfo(const llvm::Triple &target_triple) {
   sys_info.ProcessorArch =
       static_cast<llvm::support::little_t<ProcessorArchitecture>>(arch);
   // Global offset to beginning of a csd_string in a data section
-  sys_info.CSDVersionRVA = static_cast<llvm::support::ulittle32_t>(
+  sys_info.CSDVersionRVA = static_cast<llvm::support::ulittle64_t>(
       GetCurrentDataEndOffset() + sizeof(llvm::minidump::SystemInfo));
   sys_info.PlatformId = platform_id;
   m_data.AppendData(&sys_info, sizeof(llvm::minidump::SystemInfo));
@@ -165,7 +236,6 @@ llvm::Expected<uint64_t> getModuleFileSize(Target &target,
     }
     return SizeOfImage;
   }
-
   SectionSP sect_sp = mod->GetObjectFile()->GetBaseAddress().GetSection();
 
   if (!sect_sp) {
@@ -238,12 +308,10 @@ Status MinidumpFileBuilder::AddModuleList(Target &target) {
     auto maybe_mod_size = getModuleFileSize(target, mod);
     if (!maybe_mod_size) {
       llvm::Error mod_size_err = maybe_mod_size.takeError();
-      llvm::handleAllErrors(std::move(mod_size_err),
-                            [&](const llvm::ErrorInfoBase &E) {
-                              error.SetErrorStringWithFormat(
-                                  "Unable to get the size of module %s: %s.",
-                                  module_name.c_str(), E.message().c_str());
-                            });
+      llvm::handleAllErrors(std::move(mod_size_err), [&](const llvm::ErrorInfoBase &E) {
+        error.SetErrorStringWithFormat("Unable to get the size of module %s: %s.",
+                                     module_name.c_str(), E.message().c_str());
+      });
       return error;
     }
 
@@ -513,6 +581,32 @@ findStackHelper(const lldb::ProcessSP &process_sp, uint64_t rsp) {
   return std::make_pair(addr, size);
 }
 
+Status MinidumpFileBuilder::FixThreads() {
+  Status error;
+  // If we have anything in the heap flush it.
+  DumpToFile();
+
+  m_core_file->SeekFromStart(thread_list_file_offset);
+  for (auto &pair : m_thread_by_range_start) {
+    // The thread objects will get a new memory descriptor added
+    // When we are emitting the memory list and then we write it here
+    llvm::minidump::Thread thread = pair.second;
+    size_t bytes_to_write = sizeof(llvm::minidump::Thread);
+    size_t bytes_written = bytes_to_write;
+    error = m_core_file->Write(&thread, bytes_written);
+    if (error.Fail() || bytes_to_write != bytes_written) {
+        error.SetErrorStringWithFormat(
+                  "Wrote incorrect number of bytes to minidump file. (written %zd/%zd)", bytes_written,
+                  bytes_to_write);
+      return error;
+    }
+  }
+
+  // Seek the core file back to the end.
+  m_core_file->SeekFromEnd(0);
+  return error;
+}
+
 Status MinidumpFileBuilder::AddThreadList(const lldb::ProcessSP &process_sp) {
   constexpr size_t minidump_thread_size = sizeof(llvm::minidump::Thread);
   lldb_private::ThreadList thread_list = process_sp->GetThreadList();
@@ -533,7 +627,8 @@ Status MinidumpFileBuilder::AddThreadList(const lldb::ProcessSP &process_sp) {
   DataBufferHeap helper_data;
 
   const uint32_t num_threads = thread_list.GetSize();
-
+  thread_list_file_offset = GetCurrentDataEndOffset();
+  Log *log = GetLog(LLDBLog::Object);
   for (uint32_t thread_idx = 0; thread_idx < num_threads; ++thread_idx) {
     ThreadSP thread_sp(thread_list.GetThreadAtIndex(thread_idx));
     RegisterContextSP reg_ctx_sp(thread_sp->GetRegisterContext());
@@ -553,38 +648,18 @@ Status MinidumpFileBuilder::AddThreadList(const lldb::ProcessSP &process_sp) {
           arch.GetTriple().getArchName().str().c_str());
       return error;
     }
-    uint64_t sp = reg_ctx->GetSP();
-    auto expected_address_range = findStackHelper(process_sp, sp);
-
-    if (!expected_address_range) {
-      consumeError(expected_address_range.takeError());
-      error.SetErrorString("Unable to get the stack address.");
-      return error;
-    }
-
-    std::pair<uint64_t, uint64_t> range = std::move(*expected_address_range);
-    uint64_t addr = range.first;
-    uint64_t size = range.second;
 
-    auto data_up = std::make_unique<DataBufferHeap>(size, 0);
-    const size_t stack_bytes_read =
-        process_sp->ReadMemory(addr, data_up->GetBytes(), size, error);
-
-    if (error.Fail())
-      return error;
-
-    LocationDescriptor stack_memory;
-    stack_memory.DataSize =
-        static_cast<llvm::support::ulittle32_t>(stack_bytes_read);
-    stack_memory.RVA = static_cast<llvm::support::ulittle32_t>(
-        size_before + thread_stream_size + helper_data.GetByteSize());
+    uint64_t sp = reg_ctx->GetSP();
+    MemoryRegionInfo sp_region;
+    process_sp->GetMemoryRegionInfo(sp, sp_region);
 
+    // Emit a blank descriptor
     MemoryDescriptor stack;
-    stack.StartOfMemoryRange = static_cast<llvm::support::ulittle64_t>(addr);
-    stack.Memory = stack_memory;
-
-    helper_data.AppendData(data_up->GetBytes(), stack_bytes_read);
-
+    LocationDescriptor empty_label;
+    empty_label.DataSize = 0;
+    empty_label.RVA = 0;
+    stack.Memory = empty_label;
+    stack.StartOfMemoryRange = 0;
     LocationDescriptor thread_context_memory_locator;
     thread_context_memory_locator.DataSize =
         static_cast<llvm::support::ulittle32_t>(thread_context.size());
@@ -593,6 +668,8 @@ Status MinidumpFileBuilder::AddThreadList(const lldb::ProcessSP &process_sp) {
     // Cache thie thread context memory so we can reuse for exceptions.
     m_tid_to_reg_ctx[thread_sp->GetID()] = thread_context_memory_locator;
 
+    LLDB_LOGF(log, "AddThreadList for thread %d: thread_context %zu bytes",
+              thread_idx, thread_context.size());
     helper_data.AppendData(thread_context.data(), thread_context.size());
 
     llvm::minidump::Thread t;
@@ -604,9 +681,13 @@ Status MinidumpFileBuilder::AddThreadList(const lldb::ProcessSP &process_sp) {
     t.EnvironmentBlock = static_cast<llvm::support::ulittle64_t>(0);
     t.Stack = stack, t.Context = thread_context_memory_locator;
 
+    // We save off the stack object so we can circle back and clean it up.
+    m_thread_by_range_start[sp_region.GetRange().GetRangeBase()] = t;
     m_data.AppendData(&t, sizeof(llvm::minidump::Thread));
   }
 
+  LLDB_LOGF(log, "AddThreadList(): total helper_data %zu bytes",
+            helper_data.GetByteSize());
   m_data.AppendData(helper_data.GetBytes(), helper_data.GetByteSize());
   return Status();
 }
@@ -662,64 +743,6 @@ void MinidumpFileBuilder::AddExceptions(const lldb::ProcessSP &process_sp) {
   }
 }
 
-lldb_private::Status
-MinidumpFileBuilder::AddMemoryList(const lldb::ProcessSP &process_sp,
-                                   lldb::SaveCoreStyle core_style) {
-  Status error;
-  Process::CoreFileMemoryRanges core_ranges;
-  error = process_sp->CalculateCoreFileSaveRanges(core_style, core_ranges);
-  if (error.Fail()) {
-    error.SetErrorString("Process doesn't support getting memory region info.");
-    return error;
-  }
-
-  DataBufferHeap helper_data;
-  std::vector<MemoryDescriptor> mem_descriptors;
-  for (const auto &core_range : core_ranges) {
-    // Skip empty memory regions.
-    if (core_range.range.empty())
-      continue;
-    const addr_t addr = core_range.range.start();
-    const addr_t size = core_range.range.size();
-    auto data_up = std::make_unique<DataBufferHeap>(size, 0);
-    const size_t bytes_read =
-        process_sp->ReadMemory(addr, data_up->GetBytes(), size, error);
-    if (error.Fail()) {
-      Log *log = GetLog(LLDBLog::Object);
-      LLDB_LOGF(log, "Failed to read memory region. Bytes read: %zu, error: %s",
-                bytes_read, error.AsCString());
-      error.Clear();
-    }
-    if (bytes_read == 0)
-      continue;
-    // We have a good memory region with valid bytes to store.
-    LocationDescriptor memory_dump;
-    memory_dump.DataSize = static_cast<llvm::support::ulittle32_t>(bytes_read);
-    memory_dump.RVA =
-        static_cast<llvm::support::ulittle32_t>(GetCurrentDataEndOffset());
-    MemoryDescriptor memory_desc;
-    memory_desc.StartOfMemoryRange =
-        static_cast<llvm::support::ulittle64_t>(addr);
-    memory_desc.Memory = memory_dump;
-    mem_descriptors.push_back(memory_desc);
-    m_data.AppendData(data_up->GetBytes(), bytes_read);
-  }
-
-  AddDirectory(StreamType::MemoryList,
-               sizeof(llvm::support::ulittle32_t) +
-                   mem_descriptors.size() *
-                       sizeof(llvm::minidump::MemoryDescriptor));
-  llvm::support::ulittle32_t memory_ranges_num(mem_descriptors.size());
-
-  m_data.AppendData(&memory_ranges_num, sizeof(llvm::support::ulittle32_t));
-  for (auto memory_descriptor : mem_descriptors) {
-    m_data.AppendData(&memory_descriptor,
-                      sizeof(llvm::minidump::MemoryDescriptor));
-  }
-
-  return error;
-}
-
 void MinidumpFileBuilder::AddMiscInfo(const lldb::ProcessSP &process_sp) {
   AddDirectory(StreamType::MiscInfo,
                sizeof(lldb_private::minidump::MinidumpMiscInfo));
@@ -782,6 +805,7 @@ void MinidumpFileBuilder::AddLinuxFileStreams(
         {StreamType::LinuxProcFD, "/proc/" + pid_str + "/fd"});
   }
 
+  Status error;
   for (const auto &entry : files_with_stream_types) {
     StreamType stream = entry.first;
     std::string path = entry.second;
@@ -797,10 +821,57 @@ void MinidumpFileBuilder::AddLinuxFileStreams(
   }
 }
 
-Status MinidumpFileBuilder::Dump(lldb::FileUP &core_file) const {
-  constexpr size_t header_size = sizeof(llvm::minidump::Header);
-  constexpr size_t directory_size = sizeof(llvm::minidump::Directory);
+Status MinidumpFileBuilder::AddMemory(const ProcessSP &process_sp, SaveCoreStyle core_style) {
+  Status error;
+
+  Process::CoreFileMemoryRanges ranges;
+  error = process_sp->CalculateCoreFileSaveRanges(core_style, ranges);
+  if (error.Fail()) {
+    return error;
+  }
+
+  Process::CoreFileMemoryRanges ranges_for_memory_list;
+  // We leave a little padding for dictionary and any other metadata we would want.
+  // Also so that we can put the header of the memory list 64 in 32b land, because the directory
+  // requires a 32b RVA.
+  uint64_t total_size = 256 + (ranges.size() * sizeof(llvm::minidump::MemoryDescriptor_64));
+  // Enumerate all threads and save them off for the 32b range first
+  for (int i = ranges.size() - 1; i >= 0; i--) {
+    ranges_for_memory_list.push_back(ranges[i]);
+    total_size += ranges[i].range.size();
+    total_size += sizeof(llvm::minidump::MemoryDescriptor);
+    ranges.erase(ranges.begin() + i);
+  }
+
+  // Take all the memory that will fit in the 32b range.
+  for (int i = ranges.size() - 1; i >= 0; i--) {
+    addr_t size_to_add = ranges[i].range.size() + sizeof(llvm::minidump::MemoryDescriptor);
+    if (total_size + size_to_add < UINT32_MAX) {
+      ranges_for_memory_list.push_back(ranges[i]);
+      total_size += ranges[i].range.size();
+      total_size += sizeof(llvm::minidump::MemoryDescriptor);
+      ranges.erase(ranges.begin() + i);
+    }
+    else {
+      break;
+    }
+  }
+
+  error = AddMemoryList_32(process_sp, ranges_for_memory_list);
+  if (error.Fail()) 
+    return error;
 
+  // Add the remaining memory as a 64b range.
+  if (ranges.size() > 0) {
+    error = AddMemoryList_64(process_sp, ranges);
+    if (error.Fail()) 
+      return error;
+  }
+
+  return FixThreads();
+}
+
+Status MinidumpFileBuilder::DumpHeader() const {
   // write header
   llvm::minidump::Header header;
   header.Signature = static_cast<llvm::support::ulittle32_t>(
@@ -808,9 +879,10 @@ Status MinidumpFileBuilder::Dump(lldb::FileUP &core_file) const {
   header.Version = static_cast<llvm::support::ulittle32_t>(
       llvm::minidump::Header::MagicVersion);
   header.NumberOfStreams =
-      static_cast<llvm::support::ulittle32_t>(GetDirectoriesNum());
+      static_cast<llvm::support::ulittle32_t>(m_directories.size());
+  // We write the directories right after the header.
   header.StreamDirectoryRVA =
-      static_cast<llvm::support::ulittle32_t>(GetCurrentDataEndOffset());
+      static_cast<llvm::support::ulittle32_t>(header_size);
   header.Checksum = static_cast<llvm::support::ulittle32_t>(
       0u), // not used in most of the writers
       header.TimeDateStamp =
@@ -821,8 +893,9 @@ Status MinidumpFileBuilder::Dump(lldb::FileUP &core_file) const {
   Status error;
   size_t bytes_written;
 
+  m_core_file->SeekFromStart(0);
   bytes_written = header_size;
-  error = core_file->Write(&header, bytes_written);
+  error = m_core_file->Write(&header, bytes_written);
   if (error.Fail() || bytes_written != header_size) {
     if (bytes_written != header_size)
       error.SetErrorStringWithFormat(
@@ -830,22 +903,20 @@ Status MinidumpFileBuilder::Dump(lldb::FileUP &core_file) const {
           header_size);
     return error;
   }
+  return error;
+}
 
-  // write data
-  bytes_written = m_data.GetByteSize();
-  error = core_file->Write(m_data.GetBytes(), bytes_written);
-  if (error.Fail() || bytes_written != m_data.GetByteSize()) {
-    if (bytes_written != m_data.GetByteSize())
-      error.SetErrorStringWithFormat(
-          "unable to write the data (written %zd/%" PRIu64 ")", bytes_written,
-          m_data.GetByteSize());
-    return error;
-  }
+size_t MinidumpFileBuilder::GetCurrentDataEndOffset() const {
+  return m_data.GetByteSize() + m_saved_data_size;
+}
 
-  // write directories
+Status MinidumpFileBuilder::DumpDirectories() const {
+  Status error;
+  size_t bytes_written;
+  m_core_file->SeekFromStart(header_size);
   for (const Directory &dir : m_directories) {
     bytes_written = directory_size;
-    error = core_file->Write(&dir, bytes_written);
+    error = m_core_file->Write(&dir, bytes_written);
     if (error.Fail() || bytes_written != directory_size) {
       if (bytes_written != directory_size)
         error.SetErrorStringWithFormat(
@@ -858,10 +929,217 @@ Status MinidumpFileBuilder::Dump(lldb::FileUP &core_file) const {
   return error;
 }
 
-size_t MinidumpFileBuilder::GetDirectoriesNum() const {
-  return m_directories.size();
+Status MinidumpFileBuilder::AddMemoryList_32(const ProcessSP &process_sp, const Process::CoreFileMemoryRanges &ranges) {
+  std::vector<MemoryDescriptor> descriptors;
+  Status error;
+  Log *log = GetLog(LLDBLog::Object);
+  int region_index = 0;
+  for (const auto &core_range : ranges) {
+    // Take the offset before we write.
+    const size_t offset_for_data = GetCurrentDataEndOffset();
+    const addr_t addr = core_range.range.start();
+    const addr_t size = core_range.range.size();
+    std::cout << "Adding Memory Desciptor at " << std::hex << core_range.range.start() << std::dec << " with size " << core_range.range.size() << std::endl;
+    auto data_up = std::make_unique<DataBufferHeap>(size, 0);
+
+    LLDB_LOGF(log, "AddMemoryList %d/%d reading memory for region (%zu bytes) [%zx, %zx)",
+              region_index, ranges.size(), size, addr, addr+ size);
+    ++region_index;
+
+    const size_t bytes_read =
+        process_sp->ReadMemory(addr, data_up->GetBytes(), size, error);
+    if (error.Fail()) {
+      LLDB_LOGF(log, "Failed to read memory region. Bytes read: %zu, error: %s",
+                bytes_read, error.AsCString());
+      // Add the cleanup offset as the difference of bytes not read
+      descriptors.at(region_index).Memory.DataSize = 0;
+    }
+    else if (bytes_read != size) {
+      LLDB_LOGF(log, "Memory region at: %zu failed to read %zu bytes", addr, size);
+      // Add the cleanup offset as the difference of bytes not read
+      descriptors.at(region_index).Memory.DataSize = bytes_read;
+    }
+
+    MemoryDescriptor descriptor;
+    descriptor.StartOfMemoryRange = static_cast<llvm::support::ulittle64_t>(addr);
+    descriptor.Memory.DataSize = static_cast<llvm::support::ulittle32_t>(bytes_read);
+    descriptor.Memory.RVA = static_cast<llvm::support::ulittle32_t>(offset_for_data);
+    descriptors.push_back(descriptor);
+
+    // Add the data to the buffer, flush as needed.
+    error = AddData(data_up->GetBytes(), bytes_read);
+    if (error.Fail())
+      return error;
+  }
+
+  // Add a directory that references this list
+  // With a size of the number of ranges as a 32 bit num
+  // And then the size of all the ranges
+  AddDirectory(StreamType::MemoryList,
+            sizeof(llvm::support::ulittle32_t) +
+                ranges.size() *
+                    sizeof(llvm::minidump::MemoryDescriptor));
+
+  llvm::support::ulittle32_t memory_ranges_num = static_cast<llvm::support::ulittle32_t>(ranges.size());
+  m_data.AppendData(&memory_ranges_num, sizeof(llvm::support::ulittle32_t));
+  // 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(), descriptors.size() * sizeof(MemoryDescriptor));
+
+  return error;
 }
 
-size_t MinidumpFileBuilder::GetCurrentDataEndOffset() const {
-  return sizeof(llvm::minidump::Header) + m_data.GetByteSize();
+Status MinidumpFileBuilder::AddMemoryList_64(const ProcessSP &process_sp, const Process::CoreFileMemoryRanges &ranges) {
+  // Add a directory that references this list
+  // With a size of the number of ranges as a 32 bit num
+  // And then the size of all the ranges
+  // This is done first so that we can keep Directories are 32b.
+  AddDirectory(StreamType::Memory64List,
+            sizeof(llvm::support::ulittle32_t) +
+                ranges.size() *
+                    sizeof(llvm::minidump::MemoryDescriptor_64));
+
+  llvm::support::ulittle32_t memory_ranges_num = static_cast<llvm::support::ulittle32_t>(ranges.size());
+  m_data.AppendData(&memory_ranges_num, sizeof(llvm::support::ulittle32_t));
+  uint64_t current_offset = 0;
+  // Capture the starting offset, so we don't depend on the expansion of m_data.
+  uint64_t starting_offset = GetCurrentDataEndOffset();
+
+  std::map<uint32_t, addr_t> descriptors_to_clean_up_with_offset_fixes;
+  std::vector<MemoryDescriptor_64> descriptors;
+  // Enumerate the ranges and create the memory descriptors so we can append them first
+  for (const auto core_range : ranges) {
+    // Add the space required to store the memory descriptor
+    current_offset += sizeof(MemoryDescriptor_64);
+    LocationDescriptor_64 memory_dump;
+    memory_dump.DataSize = static_cast<llvm::support::ulittle64_t>(core_range.range.size());
+    memory_dump.RVA =
+        static_cast<llvm::support::ulittle64_t>(starting_offset + current_offset);
+    MemoryDescriptor_64 memory_desc;
+    memory_desc.StartOfMemoryRange =
+        static_cast<llvm::support::ulittle64_t>(core_range.range.start());
+    memory_desc.Memory = memory_dump;
+    descriptors.push_back(memory_desc);
+    // Now write this memory descriptor to the buffer.
+    m_data.AppendData(&memory_desc, sizeof(MemoryDescriptor_64));
+    // Add the data size to the current offset
+    // so subsequent RVA's are accurate.
+    current_offset += core_range.range.size();
+  }
+
+  Status error;
+  Log *log = GetLog(LLDBLog::Object);
+  int region_index = 0;
+  for (const auto &core_range : ranges) {
+    const addr_t addr = core_range.range.start();
+    const addr_t size = core_range.range.size();
+    auto data_up = std::make_unique<DataBufferHeap>(size, 0);
+
+
+    LLDB_LOGF(log, "AddMemoryList %d/%d reading memory for region (%zu bytes) [%zx, %zx)",
+              region_index, ranges.size(), size, addr, addr+ size);
+    ++region_index;
+
+    // TODO: Add a fixup functionality, when we fail to read the correct number of bytes or anything at all
+    // we need to go back and fix the memory descriptors and the subsequent RVA's.
+    const size_t bytes_read =
+        process_sp->ReadMemory(addr, data_up->GetBytes(), size, error);
+    if (error.Fail()) {
+      LLDB_LOGF(log, "Failed to read memory region. Bytes read: %zu, error: %s",
+                bytes_read, error.AsCString());
+      descriptors_to_clean_up_with_offset_fixes[region_index] = size;
+      descriptors[region_index].Memory.DataSize = 0;
+    }
+    if (bytes_read != size) {
+      error.SetErrorStringWithFormat("Memory region at: %zu failed to read %zu bytes", addr, size);
+      descriptors_to_clean_up_with_offset_fixes[region_index] = size - bytes_read;
+      descriptors[region_index].Memory.DataSize = bytes_read;
+    }
+
+    // Add the data to the buffer, flush as needed.
+    error = AddData(data_up->GetBytes(), bytes_read);
+    if (error.Fail())
+      return error;
+  }
+
+  // Early return if there is no cleanup needed.
+  if (descriptors.size() == 0) {
+    return error;
+  }
+  else {
+    // Flush to disk we can make the fixes in place.
+    DumpToFile();
+    // Iterate through the list, moving up (subtracting) the RVA by the current fix up offset
+    // Then check if the current item is in the map
+    // If it is, add it's offset to the fix up delta.
+    // This is so if we fail to write a certain number of bytes, we fix the rest of the RVA's
+    //
+    addr_t current_shiftup_delta = 0;
+    for (uint i = 0; i < descriptors.size(); i++) {
+      MemoryDescriptor_64 &desc = descriptors[i];
+      desc.Memory.RVA -= current_shiftup_delta;
+      // We update the delta afterwards because the RVA of the current 
+      // descriptor doesn't depend on it's size. Only the subsequent Descriptors
+      // would depend on this delta.
+      if (descriptors_to_clean_up_with_offset_fixes.count(i) > 0) {
+        current_shiftup_delta += descriptors_to_clean_up_with_offset_fixes[i];
+      }
+    }
+
+    // Fixup the descriptors that were not read correctly.
+    m_core_file->SeekFromStart(starting_offset);
+    size_t bytes_written = sizeof(MemoryDescriptor) * descriptors.size();
+    error = m_core_file->Write(descriptors.data(), bytes_written);
+    if (error.Fail() || bytes_written != sizeof(MemoryDescriptor) * descriptors.size()) {
+        error.SetErrorStringWithFormat(
+            "unable to write the memory descriptors (written %zd/%zd)",
+            bytes_written, sizeof(MemoryDescriptor) * descriptors.size());
+    }
+
+    return error;
+  }
+}
+
+Status MinidumpFileBuilder::AddData(const void* data, size_t size) {
+  m_data.AppendData(data, size);
+  // TODO: Refactor to be a parameter.
+  if (m_data.GetByteSize() > 1024 * 1024 * 1024) {
+    return FlushToDisk();
+  }
+
+  return Status();
+}
+
+Status MinidumpFileBuilder::FlushToDisk() const {
+  Status error;
+  size_t bytes_to_write = m_data.GetByteSize();
+  size_t bytes_written = bytes_to_write;
+  error = m_core_file->Write(m_data.GetBytes(), bytes_to_write);
+  if (error.Fail() || bytes_written != bytes_to_write) {
+      error.SetErrorStringWithFormat(
+          "Wrote incorrect number of bytes to minidump file. (written %zd/%zd)", bytes_written,
+          bytes_to_write);
+    return error;
+  }
+
+  return error;
+}
+
+Status MinidumpFileBuilder::DumpToFile() {
+  Status error;
+  // If anything is left unsaved, dump it.
+  error = FlushToDisk();
+  if (error.Fail())
+    return error;
+
+  // Overwrite the header which we filled in earlier.
+  error = DumpHeader();
+  if (error.Fail())
+    return error;
+
+  // Overwrite the space saved for directories
+  error = DumpDirectories();
+  if (error.Fail())
+    return error;
+
+  return error;
 }
diff --git a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h
index b2e984191983f..5f34c5da10078 100644
--- a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h
+++ b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h
@@ -17,12 +17,18 @@
 #define LLDB_SOURCE_PLUGINS_OBJECTFILE_MINIDUMP_MINIDUMPFILEBUILDER_H
 
 #include <cstddef>
+#include <cstdint>
 #include <map>
+#include <variant>
 
+#include "lldb/Target/Process.h"
 #include "lldb/Target/Target.h"
 #include "lldb/Utility/DataBufferHeap.h"
 #include "lldb/Utility/Status.h"
+#include "lldb/lldb-forward.h"
+#include "lldb/lldb-types.h"
 
+#include "llvm/BinaryFormat/Minidump.h"
 #include "llvm/Object/Minidump.h"
 
 // Write std::string to minidump in the UTF16 format(with null termination char)
@@ -40,7 +46,7 @@ lldb_private::Status WriteString(const std::string &to_write,
 /// the data on heap.
 class MinidumpFileBuilder {
 public:
-  MinidumpFileBuilder() = default;
+  MinidumpFileBuilder(lldb::FileUP&& core_file): m_core_file(std::move(core_file)) {};
 
   MinidumpFileBuilder(const MinidumpFileBuilder &) = delete;
   MinidumpFileBuilder &operator=(const MinidumpFileBuilder &) = delete;
@@ -50,6 +56,7 @@ class MinidumpFileBuilder {
 
   ~MinidumpFileBuilder() = default;
 
+  lldb_private::Status AddHeaderAndCalculateDirectories(const lldb_private::Target &target, const lldb::ProcessSP &process_sp);
   // Add SystemInfo stream, used for storing the most basic information
   // about the system, platform etc...
   lldb_private::Status AddSystemInfo(const llvm::Triple &target_triple);
@@ -59,39 +66,57 @@ class MinidumpFileBuilder {
   // Add ThreadList stream, containing information about all threads running
   // at the moment of core saving. Contains information about thread
   // contexts.
-  lldb_private::Status AddThreadList(const lldb::ProcessSP &process_sp);
-  // Add Exception streams for any threads that stopped with exceptions.
-  void AddExceptions(const lldb::ProcessSP &process_sp);
-  // Add MemoryList stream, containing dumps of important memory segments
-  lldb_private::Status AddMemoryList(const lldb::ProcessSP &process_sp,
-                                     lldb::SaveCoreStyle core_style);
   // Add MiscInfo stream, mainly providing ProcessId
   void AddMiscInfo(const lldb::ProcessSP &process_sp);
   // Add informative files about a Linux process
   void AddLinuxFileStreams(const lldb::ProcessSP &process_sp);
+  // Add Exception streams for any threads that stopped with exceptions.
+  void AddExceptions(const lldb::ProcessSP &process_sp);
   // Dump the prepared data into file. In case of the failure data are
   // intact.
-  lldb_private::Status Dump(lldb::FileUP &core_file) const;
-  // Returns the current number of directories(streams) that have been so far
-  // created. This number of directories will be dumped when calling Dump()
-  size_t GetDirectoriesNum() const;
+  lldb_private::Status AddThreadList(const lldb::ProcessSP &process_sp);
+
+  lldb_private::Status AddMemory(const lldb::ProcessSP &process_sp,
+                                     lldb::SaveCoreStyle core_style);
+
+  lldb_private::Status DumpToFile();
 
 private:
+  // Add data to the end of the buffer, if the buffer exceeds the flush level, trigger a flush.
+  lldb_private::Status AddData(const void *data, size_t size);
+  // Add MemoryList stream, containing dumps of important memory segments
+  lldb_private::Status AddMemoryList_64(const lldb::ProcessSP &process_sp, const lldb_private::Process::CoreFileMemoryRanges &ranges);
+  lldb_private::Status AddMemoryList_32(const lldb::ProcessSP &process_sp, const lldb_private::Process::CoreFileMemoryRanges &ranges);
+  lldb_private::Status FixThreads();
+  lldb_private::Status FlushToDisk() const;
+
+  lldb_private::Status DumpHeader() const;
+  lldb_private::Status DumpDirectories() const;
+  bool CheckIf_64Bit(const size_t size);
   // Add directory of StreamType pointing to the current end of the prepared
   // file with the specified size.
-  void AddDirectory(llvm::minidump::StreamType type, size_t stream_size);
-  size_t GetCurrentDataEndOffset() const;
-
-  // Stores directories to later put them at the end of minidump file
+  void AddDirectory(llvm::minidump::StreamType type, uint64_t stream_size);
+  lldb::addr_t GetCurrentDataEndOffset() const;
+  // Stores directories to fill in later
   std::vector<llvm::minidump::Directory> m_directories;
+  // When we write off the threads for the first time, we need to clean them up
+  // and give them the correct RVA once we write the stack memory list.
+  std::map<lldb::addr_t, llvm::minidump::Thread> m_thread_by_range_start;
   // Main data buffer consisting of data without the minidump header and
   // directories
   lldb_private::DataBufferHeap m_data;
+  uint m_expected_directories = 0;
+  uint64_t m_saved_data_size = 0;
+  size_t thread_list_file_offset = 0;
+
+  static constexpr size_t header_size = sizeof(llvm::minidump::Header);
+  static constexpr size_t directory_size = sizeof(llvm::minidump::Directory);
 
   // 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.
   std::map<lldb::tid_t, llvm::minidump::LocationDescriptor> m_tid_to_reg_ctx;
+  lldb::FileUP m_core_file;
 };
 
 #endif // LLDB_SOURCE_PLUGINS_OBJECTFILE_MINIDUMP_MINIDUMPFILEBUILDER_H
diff --git a/lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.cpp b/lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.cpp
index 1af5d99f0b160..d64c1d7bfeef0 100644
--- a/lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.cpp
+++ b/lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.cpp
@@ -17,6 +17,7 @@
 #include "lldb/Utility/LLDBLog.h"
 
 #include "llvm/Support/FileSystem.h"
+#include <unistd.h>
 
 using namespace lldb;
 using namespace lldb_private;
@@ -65,37 +66,40 @@ bool ObjectFileMinidump::SaveCore(const lldb::ProcessSP &process_sp,
   if (!process_sp)
     return false;
 
-  MinidumpFileBuilder builder;
+  llvm::Expected<lldb::FileUP> maybe_core_file = FileSystem::Instance().Open(
+      outfile, File::eOpenOptionWriteOnly | File::eOpenOptionCanCreate);
+  if (!maybe_core_file) {
+    error = maybe_core_file.takeError();
+    return false;
+  }
+  MinidumpFileBuilder builder(std::move(maybe_core_file.get()));
+
 
   Target &target = process_sp->GetTarget();
+    builder.AddHeaderAndCalculateDirectories(target, process_sp);
 
   Log *log = GetLog(LLDBLog::Object);
   error = builder.AddSystemInfo(target.GetArchitecture().GetTriple());
   if (error.Fail()) {
-    LLDB_LOG(log, "AddSystemInfo failed: %s", error.AsCString());
-    return false;
-  }
-
-  error = builder.AddModuleList(target);
-  if (error.Fail()) {
-    LLDB_LOG(log, "AddModuleList failed: %s", error.AsCString());
+    LLDB_LOGF(log, "AddSystemInfo failed: %s", error.AsCString());
     return false;
   }
 
+  builder.AddModuleList(target);
   builder.AddMiscInfo(process_sp);
 
   error = builder.AddThreadList(process_sp);
   if (error.Fail()) {
-    LLDB_LOG(log, "AddThreadList failed: %s", error.AsCString());
+    LLDB_LOGF(log, "AddThreadList failed: %s", error.AsCString());
     return false;
   }
 
   // Add any exceptions but only if there are any in any threads.
   builder.AddExceptions(process_sp);
 
-  error = builder.AddMemoryList(process_sp, core_style);
+  error = builder.AddMemory(process_sp, core_style);
   if (error.Fail()) {
-    LLDB_LOG(log, "AddMemoryList failed: %s", error.AsCString());
+    LLDB_LOGF(log, "AddMemoryList failed: %s", error.AsCString());
     return false;
   }
 
@@ -104,17 +108,11 @@ bool ObjectFileMinidump::SaveCore(const lldb::ProcessSP &process_sp,
     builder.AddLinuxFileStreams(process_sp);
   }
 
-  llvm::Expected<lldb::FileUP> maybe_core_file = FileSystem::Instance().Open(
-      outfile, File::eOpenOptionWriteOnly | File::eOpenOptionCanCreate);
-  if (!maybe_core_file) {
-    error = maybe_core_file.takeError();
+  error = builder.DumpToFile();
+  if (error.Fail()) {
+    LLDB_LOGF(log, "DumpToFile failed: %s", error.AsCString());
     return false;
   }
-  lldb::FileUP core_file = std::move(maybe_core_file.get());
-
-  error = builder.Dump(core_file);
-  if (error.Fail())
-    return false;
 
   return true;
 }
diff --git a/llvm/include/llvm/BinaryFormat/Minidump.h b/llvm/include/llvm/BinaryFormat/Minidump.h
index bc303929498d0..53cd0deb1b18b 100644
--- a/llvm/include/llvm/BinaryFormat/Minidump.h
+++ b/llvm/include/llvm/BinaryFormat/Minidump.h
@@ -62,6 +62,12 @@ struct LocationDescriptor {
 };
 static_assert(sizeof(LocationDescriptor) == 8);
 
+struct LocationDescriptor_64 {
+  support::ulittle64_t DataSize;
+  support::ulittle64_t RVA;
+};
+static_assert(sizeof(LocationDescriptor_64) == 16);
+
 /// Describes a single memory range (both its VM address and where to find it in
 /// the file) of the process from which this minidump file was generated.
 struct MemoryDescriptor {
@@ -70,6 +76,11 @@ struct MemoryDescriptor {
 };
 static_assert(sizeof(MemoryDescriptor) == 16);
 
+struct MemoryDescriptor_64 {
+  support::ulittle64_t StartOfMemoryRange;
+  LocationDescriptor_64 Memory;
+};
+
 struct MemoryInfoListHeader {
   support::ulittle32_t SizeOfHeader;
   support::ulittle32_t SizeOfEntry;
diff --git a/llvm/include/llvm/Object/Minidump.h b/llvm/include/llvm/Object/Minidump.h
index e45d4de0090de..a388fa3349772 100644
--- a/llvm/include/llvm/Object/Minidump.h
+++ b/llvm/include/llvm/Object/Minidump.h
@@ -52,6 +52,13 @@ class MinidumpFile : public Binary {
     return getDataSlice(getData(), Desc.RVA, Desc.DataSize);
   }
 
+  /// Returns the raw contents of an object given by the LocationDescriptor. An
+  /// error is returned if the descriptor points outside of the minidump file.
+  Expected<ArrayRef<uint8_t>>
+  getRawData(minidump::LocationDescriptor_64 Desc) const {
+    return getDataSlice(getData(), Desc.RVA, Desc.DataSize);
+  }
+
   /// Returns the minidump string at the given offset. An error is returned if
   /// we fail to parse the string, or the string is invalid UTF16.
   Expected<std::string> getString(size_t Offset) const;
diff --git a/llvm/lib/ObjectYAML/MinidumpEmitter.cpp b/llvm/lib/ObjectYAML/MinidumpEmitter.cpp
index 24b521a9925c7..a967e422bef67 100644
--- a/llvm/lib/ObjectYAML/MinidumpEmitter.cpp
+++ b/llvm/lib/ObjectYAML/MinidumpEmitter.cpp
@@ -6,9 +6,11 @@
 //
 //===----------------------------------------------------------------------===//
 
+#include "llvm/BinaryFormat/Minidump.h"
 #include "llvm/ObjectYAML/MinidumpYAML.h"
 #include "llvm/ObjectYAML/yaml2obj.h"
 #include "llvm/Support/ConvertUTF.h"
+#include "llvm/Support/Endian.h"
 #include "llvm/Support/raw_ostream.h"
 #include <optional>
 
@@ -119,6 +121,11 @@ static LocationDescriptor layout(BlobAllocator &File, yaml::BinaryRef Data) {
           support::ulittle32_t(File.allocateBytes(Data))};
 }
 
+static LocationDescriptor_64 layout_64(BlobAllocator &File, yaml::BinaryRef Data) {
+  return {support::ulittle64_t(Data.binary_size()),
+          support::ulittle64_t(File.allocateBytes(Data))};
+}
+
 static size_t layout(BlobAllocator &File, MinidumpYAML::ExceptionStream &S) {
   File.allocateObject(S.MDExceptionStream);
 

>From 2885cb3ec6e672a5eacc7d8a962d9790cc29f9ea Mon Sep 17 00:00:00 2001
From: Jacob Lalonde <jalalonde at fb.com>
Date: Wed, 12 Jun 2024 14:03:37 -0700
Subject: [PATCH 02/14] Remove debug changes to testprocesssavecore

---
 .../Minidump/MinidumpFileBuilder.cpp          | 165 +++++++++---------
 .../ObjectFile/Minidump/MinidumpFileBuilder.h |   9 +-
 .../Minidump/ObjectFileMinidump.cpp           |  11 +-
 llvm/include/llvm/BinaryFormat/Minidump.h     |   8 +-
 llvm/include/llvm/Object/Minidump.h           |   7 -
 5 files changed, 93 insertions(+), 107 deletions(-)

diff --git a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp
index c08b5206720ab..ecb16d2fbd388 100644
--- a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp
+++ b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp
@@ -38,10 +38,13 @@
 #include "lldb/lldb-forward.h"
 #include "lldb/lldb-types.h"
 
+#include <algorithm>
 #include <cinttypes>
 #include <climits>
+#include <cstddef>
 #include <cstdint>
 #include <iostream>
+#include <set>
 #include <utility>
 #include <vector>
 
@@ -584,9 +587,8 @@ findStackHelper(const lldb::ProcessSP &process_sp, uint64_t rsp) {
 Status MinidumpFileBuilder::FixThreads() {
   Status error;
   // If we have anything in the heap flush it.
-  DumpToFile();
-
-  m_core_file->SeekFromStart(thread_list_file_offset);
+  FlushToDisk();
+  m_core_file->SeekFromStart(m_thread_list_start);
   for (auto &pair : m_thread_by_range_start) {
     // The thread objects will get a new memory descriptor added
     // When we are emitting the memory list and then we write it here
@@ -602,8 +604,6 @@ Status MinidumpFileBuilder::FixThreads() {
     }
   }
 
-  // Seek the core file back to the end.
-  m_core_file->SeekFromEnd(0);
   return error;
 }
 
@@ -624,10 +624,11 @@ Status MinidumpFileBuilder::AddThreadList(const lldb::ProcessSP &process_sp) {
       static_cast<llvm::support::ulittle32_t>(thread_list.GetSize());
   m_data.AppendData(&thread_count, sizeof(llvm::support::ulittle32_t));
 
+  // Take the offset after the thread count.
+  m_thread_list_start = GetCurrentDataEndOffset();
   DataBufferHeap helper_data;
 
   const uint32_t num_threads = thread_list.GetSize();
-  thread_list_file_offset = GetCurrentDataEndOffset();
   Log *log = GetLog(LLDBLog::Object);
   for (uint32_t thread_idx = 0; thread_idx < num_threads; ++thread_idx) {
     ThreadSP thread_sp(thread_list.GetThreadAtIndex(thread_idx));
@@ -824,29 +825,34 @@ void MinidumpFileBuilder::AddLinuxFileStreams(
 Status MinidumpFileBuilder::AddMemory(const ProcessSP &process_sp, SaveCoreStyle core_style) {
   Status error;
 
-  Process::CoreFileMemoryRanges ranges;
-  error = process_sp->CalculateCoreFileSaveRanges(core_style, ranges);
+  Process::CoreFileMemoryRanges ranges_for_memory_list;
+  error = process_sp->CalculateCoreFileSaveRanges(SaveCoreStyle::eSaveCoreStackOnly, ranges_for_memory_list);
   if (error.Fail()) {
     return error;
   }
 
-  Process::CoreFileMemoryRanges ranges_for_memory_list;
+  std::set<addr_t> stack_ranges;
+  for (const auto &core_range : ranges_for_memory_list) {
+    stack_ranges.insert(core_range.range.start());
+  }
   // We leave a little padding for dictionary and any other metadata we would want.
   // Also so that we can put the header of the memory list 64 in 32b land, because the directory
   // requires a 32b RVA.
-  uint64_t total_size = 256 + (ranges.size() * sizeof(llvm::minidump::MemoryDescriptor_64));
-  // Enumerate all threads and save them off for the 32b range first
-  for (int i = ranges.size() - 1; i >= 0; i--) {
-    ranges_for_memory_list.push_back(ranges[i]);
-    total_size += ranges[i].range.size();
-    total_size += sizeof(llvm::minidump::MemoryDescriptor);
-    ranges.erase(ranges.begin() + i);
+  Process::CoreFileMemoryRanges ranges;
+  error = process_sp->CalculateCoreFileSaveRanges(core_style, ranges);
+  if (error.Fail()) {
+    return error;
   }
 
+  uint64_t total_size = 256 + (ranges.size() * sizeof(llvm::minidump::MemoryDescriptor_64));
   // Take all the memory that will fit in the 32b range.
   for (int i = ranges.size() - 1; i >= 0; i--) {
     addr_t size_to_add = ranges[i].range.size() + sizeof(llvm::minidump::MemoryDescriptor);
-    if (total_size + size_to_add < UINT32_MAX) {
+    // filter out the stacks and check if it's below 32b max.
+    if (stack_ranges.count(ranges[i].range.start()) > 0) {
+      ranges.erase(ranges.begin() + i);
+    }
+    else if (total_size + size_to_add < UINT32_MAX) {
       ranges_for_memory_list.push_back(ranges[i]);
       total_size += ranges[i].range.size();
       total_size += sizeof(llvm::minidump::MemoryDescriptor);
@@ -948,16 +954,14 @@ Status MinidumpFileBuilder::AddMemoryList_32(const ProcessSP &process_sp, const
 
     const size_t bytes_read =
         process_sp->ReadMemory(addr, data_up->GetBytes(), size, error);
-    if (error.Fail()) {
+    if (error.Fail() || bytes_read == 0) {
       LLDB_LOGF(log, "Failed to read memory region. Bytes read: %zu, error: %s",
                 bytes_read, error.AsCString());
-      // Add the cleanup offset as the difference of bytes not read
-      descriptors.at(region_index).Memory.DataSize = 0;
+      // Just skip sections with errors or zero bytes in 32b mode
+      continue; 
     }
     else if (bytes_read != size) {
       LLDB_LOGF(log, "Memory region at: %zu failed to read %zu bytes", addr, size);
-      // Add the cleanup offset as the difference of bytes not read
-      descriptors.at(region_index).Memory.DataSize = bytes_read;
     }
 
     MemoryDescriptor descriptor;
@@ -965,6 +969,9 @@ Status MinidumpFileBuilder::AddMemoryList_32(const ProcessSP &process_sp, const
     descriptor.Memory.DataSize = static_cast<llvm::support::ulittle32_t>(bytes_read);
     descriptor.Memory.RVA = static_cast<llvm::support::ulittle32_t>(offset_for_data);
     descriptors.push_back(descriptor);
+    if (m_thread_by_range_start.count(addr) > 0) {
+      m_thread_by_range_start[addr].Stack = descriptor;
+    }
 
     // Add the data to the buffer, flush as needed.
     error = AddData(data_up->GetBytes(), bytes_read);
@@ -977,10 +984,10 @@ Status MinidumpFileBuilder::AddMemoryList_32(const ProcessSP &process_sp, const
   // And then the size of all the ranges
   AddDirectory(StreamType::MemoryList,
             sizeof(llvm::support::ulittle32_t) +
-                ranges.size() *
+                descriptors.size() *
                     sizeof(llvm::minidump::MemoryDescriptor));
 
-  llvm::support::ulittle32_t memory_ranges_num = static_cast<llvm::support::ulittle32_t>(ranges.size());
+  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));
   // 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(), descriptors.size() * sizeof(MemoryDescriptor));
@@ -989,41 +996,30 @@ Status MinidumpFileBuilder::AddMemoryList_32(const ProcessSP &process_sp, const
 }
 
 Status MinidumpFileBuilder::AddMemoryList_64(const ProcessSP &process_sp, const Process::CoreFileMemoryRanges &ranges) {
-  // Add a directory that references this list
-  // With a size of the number of ranges as a 32 bit num
-  // And then the size of all the ranges
-  // This is done first so that we can keep Directories are 32b.
   AddDirectory(StreamType::Memory64List,
-            sizeof(llvm::support::ulittle32_t) +
+            (sizeof(llvm::support::ulittle64_t) * 2) +
                 ranges.size() *
                     sizeof(llvm::minidump::MemoryDescriptor_64));
 
-  llvm::support::ulittle32_t memory_ranges_num = static_cast<llvm::support::ulittle32_t>(ranges.size());
-  m_data.AppendData(&memory_ranges_num, sizeof(llvm::support::ulittle32_t));
-  uint64_t current_offset = 0;
-  // Capture the starting offset, so we don't depend on the expansion of m_data.
+  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));
+  llvm::support::ulittle64_t memory_ranges_base_rva = static_cast<llvm::support::ulittle64_t>(GetCurrentDataEndOffset());
+  m_data.AppendData(&memory_ranges_base_rva, sizeof(llvm::support::ulittle64_t));
+  // Capture the starting offset, so we can do cleanup later if needed.
   uint64_t starting_offset = GetCurrentDataEndOffset();
 
-  std::map<uint32_t, addr_t> descriptors_to_clean_up_with_offset_fixes;
+  bool cleanup_required = false;
   std::vector<MemoryDescriptor_64> descriptors;
   // Enumerate the ranges and create the memory descriptors so we can append them first
   for (const auto core_range : ranges) {
     // Add the space required to store the memory descriptor
-    current_offset += sizeof(MemoryDescriptor_64);
-    LocationDescriptor_64 memory_dump;
-    memory_dump.DataSize = static_cast<llvm::support::ulittle64_t>(core_range.range.size());
-    memory_dump.RVA =
-        static_cast<llvm::support::ulittle64_t>(starting_offset + current_offset);
     MemoryDescriptor_64 memory_desc;
     memory_desc.StartOfMemoryRange =
         static_cast<llvm::support::ulittle64_t>(core_range.range.start());
-    memory_desc.Memory = memory_dump;
+    memory_desc.DataSize = static_cast<llvm::support::ulittle64_t>(core_range.range.size());
     descriptors.push_back(memory_desc);
     // Now write this memory descriptor to the buffer.
     m_data.AppendData(&memory_desc, sizeof(MemoryDescriptor_64));
-    // Add the data size to the current offset
-    // so subsequent RVA's are accurate.
-    current_offset += core_range.range.size();
   }
 
   Status error;
@@ -1034,25 +1030,23 @@ Status MinidumpFileBuilder::AddMemoryList_64(const ProcessSP &process_sp, const
     const addr_t size = core_range.range.size();
     auto data_up = std::make_unique<DataBufferHeap>(size, 0);
 
-
-    LLDB_LOGF(log, "AddMemoryList %d/%d reading memory for region (%zu bytes) [%zx, %zx)",
+    LLDB_LOGF(log, "AddMemoryList_64 %d/%d reading memory for region (%zu bytes) [%zx, %zx)",
               region_index, ranges.size(), size, addr, addr+ size);
     ++region_index;
 
-    // TODO: Add a fixup functionality, when we fail to read the correct number of bytes or anything at all
-    // we need to go back and fix the memory descriptors and the subsequent RVA's.
     const size_t bytes_read =
         process_sp->ReadMemory(addr, data_up->GetBytes(), size, error);
     if (error.Fail()) {
       LLDB_LOGF(log, "Failed to read memory region. Bytes read: %zu, error: %s",
                 bytes_read, error.AsCString());
-      descriptors_to_clean_up_with_offset_fixes[region_index] = size;
-      descriptors[region_index].Memory.DataSize = 0;
+      error.Clear();
+      cleanup_required = true;
+      descriptors[region_index].DataSize = 0;
     }
     if (bytes_read != size) {
-      error.SetErrorStringWithFormat("Memory region at: %zu failed to read %zu bytes", addr, size);
-      descriptors_to_clean_up_with_offset_fixes[region_index] = size - bytes_read;
-      descriptors[region_index].Memory.DataSize = bytes_read;
+      LLDB_LOGF(log, "Memory region at: %zu failed to read %zu bytes", addr, size);
+      cleanup_required = true;
+      descriptors[region_index].DataSize = bytes_read;
     }
 
     // Add the data to the buffer, flush as needed.
@@ -1062,65 +1056,64 @@ Status MinidumpFileBuilder::AddMemoryList_64(const ProcessSP &process_sp, const
   }
 
   // Early return if there is no cleanup needed.
-  if (descriptors.size() == 0) {
+  if (!cleanup_required) {
     return error;
   }
   else {
     // Flush to disk we can make the fixes in place.
-    DumpToFile();
-    // Iterate through the list, moving up (subtracting) the RVA by the current fix up offset
-    // Then check if the current item is in the map
-    // If it is, add it's offset to the fix up delta.
-    // This is so if we fail to write a certain number of bytes, we fix the rest of the RVA's
-    //
-    addr_t current_shiftup_delta = 0;
-    for (uint i = 0; i < descriptors.size(); i++) {
-      MemoryDescriptor_64 &desc = descriptors[i];
-      desc.Memory.RVA -= current_shiftup_delta;
-      // We update the delta afterwards because the RVA of the current 
-      // descriptor doesn't depend on it's size. Only the subsequent Descriptors
-      // would depend on this delta.
-      if (descriptors_to_clean_up_with_offset_fixes.count(i) > 0) {
-        current_shiftup_delta += descriptors_to_clean_up_with_offset_fixes[i];
-      }
-    }
-
+    FlushToDisk();
     // Fixup the descriptors that were not read correctly.
     m_core_file->SeekFromStart(starting_offset);
-    size_t bytes_written = sizeof(MemoryDescriptor) * descriptors.size();
+    size_t bytes_written = sizeof(MemoryDescriptor_64) * descriptors.size();
     error = m_core_file->Write(descriptors.data(), bytes_written);
-    if (error.Fail() || bytes_written != sizeof(MemoryDescriptor) * descriptors.size()) {
+    if (error.Fail() || bytes_written != sizeof(MemoryDescriptor_64) * descriptors.size()) {
         error.SetErrorStringWithFormat(
             "unable to write the memory descriptors (written %zd/%zd)",
-            bytes_written, sizeof(MemoryDescriptor) * descriptors.size());
+            bytes_written, sizeof(MemoryDescriptor_64) * descriptors.size());
     }
 
     return error;
   }
 }
 
-Status MinidumpFileBuilder::AddData(const void* data, size_t size) {
+Status MinidumpFileBuilder::AddData(const void* data, addr_t size) {
+  // This should also get chunked, because worst case we copy over a big 
+  // object / memory range, say 5gb. In that case, we'd have to allocate 10gb
+  // 5 gb for the buffer we're copying from, and then 5gb for the buffer we're copying to.
+  // Which will be short lived and immedaitely go to disk, the goal here is to limit the number
+  // of bytes we need to host in memory at any given time.
   m_data.AppendData(data, size);
-  // TODO: Refactor to be a parameter.
-  if (m_data.GetByteSize() > 1024 * 1024 * 1024) {
+  if (m_data.GetByteSize() > m_write_chunk_max) {
     return FlushToDisk();
   }
 
   return Status();
 }
 
-Status MinidumpFileBuilder::FlushToDisk() const {
+Status MinidumpFileBuilder::FlushToDisk() {
   Status error;
-  size_t bytes_to_write = m_data.GetByteSize();
-  size_t bytes_written = bytes_to_write;
-  error = m_core_file->Write(m_data.GetBytes(), bytes_to_write);
-  if (error.Fail() || bytes_written != bytes_to_write) {
-      error.SetErrorStringWithFormat(
-          "Wrote incorrect number of bytes to minidump file. (written %zd/%zd)", bytes_written,
-          bytes_to_write);
-    return error;
+  // Set the stream to it's end.
+  m_core_file->SeekFromEnd(0);
+  addr_t starting_size = m_data.GetByteSize();
+  addr_t remaining_bytes = starting_size;
+  size_t offset = 0;
+  while (remaining_bytes > 0) {
+    size_t chunk_size = std::min(remaining_bytes, m_write_chunk_max);
+    size_t bytes_written = chunk_size;
+    error = m_core_file->Write(m_data.GetBytes() + offset, bytes_written);
+    if (error.Fail() || bytes_written != chunk_size) {
+        error.SetErrorStringWithFormat(
+            "Wrote incorrect number of bytes to minidump file. (written %zd/%zd)", bytes_written,
+            chunk_size);
+      return error;
+    }
+
+    offset += bytes_written;
+    remaining_bytes -= bytes_written;
   }
 
+  m_saved_data_size += starting_size;
+  m_data.Clear();
   return error;
 }
 
diff --git a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h
index 5f34c5da10078..c5fe9836f06a5 100644
--- a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h
+++ b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h
@@ -88,7 +88,7 @@ class MinidumpFileBuilder {
   lldb_private::Status AddMemoryList_64(const lldb::ProcessSP &process_sp, const lldb_private::Process::CoreFileMemoryRanges &ranges);
   lldb_private::Status AddMemoryList_32(const lldb::ProcessSP &process_sp, const lldb_private::Process::CoreFileMemoryRanges &ranges);
   lldb_private::Status FixThreads();
-  lldb_private::Status FlushToDisk() const;
+  lldb_private::Status FlushToDisk();
 
   lldb_private::Status DumpHeader() const;
   lldb_private::Status DumpDirectories() const;
@@ -107,7 +107,12 @@ class MinidumpFileBuilder {
   lldb_private::DataBufferHeap m_data;
   uint m_expected_directories = 0;
   uint64_t m_saved_data_size = 0;
-  size_t thread_list_file_offset = 0;
+  size_t m_thread_list_start = 0;
+  // We set the max write amount to 128 mb
+  // Linux has a signed 32b - some buffer on writes
+  // and we have guarauntee a user memory region / 'object' could be over 2gb
+  // now that we support 64b memory dumps.
+  static constexpr size_t m_write_chunk_max = (1024 * 1024 * 128);
 
   static constexpr size_t header_size = sizeof(llvm::minidump::Header);
   static constexpr size_t directory_size = sizeof(llvm::minidump::Directory);
diff --git a/lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.cpp b/lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.cpp
index d64c1d7bfeef0..3435d89accc49 100644
--- a/lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.cpp
+++ b/lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.cpp
@@ -93,21 +93,22 @@ bool ObjectFileMinidump::SaveCore(const lldb::ProcessSP &process_sp,
     LLDB_LOGF(log, "AddThreadList failed: %s", error.AsCString());
     return false;
   }
+  if (target.GetArchitecture().GetTriple().getOS() ==
+      llvm::Triple::OSType::Linux) {
+    builder.AddLinuxFileStreams(process_sp);
+  }
 
   // Add any exceptions but only if there are any in any threads.
   builder.AddExceptions(process_sp);
 
+  // Note: add memory HAS to be the last thing we do. It can overflow into 64b land
+  // and many RVA's only support 32b
   error = builder.AddMemory(process_sp, core_style);
   if (error.Fail()) {
     LLDB_LOGF(log, "AddMemoryList failed: %s", error.AsCString());
     return false;
   }
 
-  if (target.GetArchitecture().GetTriple().getOS() ==
-      llvm::Triple::OSType::Linux) {
-    builder.AddLinuxFileStreams(process_sp);
-  }
-
   error = builder.DumpToFile();
   if (error.Fail()) {
     LLDB_LOGF(log, "DumpToFile failed: %s", error.AsCString());
diff --git a/llvm/include/llvm/BinaryFormat/Minidump.h b/llvm/include/llvm/BinaryFormat/Minidump.h
index 53cd0deb1b18b..9669303252615 100644
--- a/llvm/include/llvm/BinaryFormat/Minidump.h
+++ b/llvm/include/llvm/BinaryFormat/Minidump.h
@@ -62,12 +62,6 @@ struct LocationDescriptor {
 };
 static_assert(sizeof(LocationDescriptor) == 8);
 
-struct LocationDescriptor_64 {
-  support::ulittle64_t DataSize;
-  support::ulittle64_t RVA;
-};
-static_assert(sizeof(LocationDescriptor_64) == 16);
-
 /// Describes a single memory range (both its VM address and where to find it in
 /// the file) of the process from which this minidump file was generated.
 struct MemoryDescriptor {
@@ -78,7 +72,7 @@ static_assert(sizeof(MemoryDescriptor) == 16);
 
 struct MemoryDescriptor_64 {
   support::ulittle64_t StartOfMemoryRange;
-  LocationDescriptor_64 Memory;
+  support::ulittle64_t DataSize;
 };
 
 struct MemoryInfoListHeader {
diff --git a/llvm/include/llvm/Object/Minidump.h b/llvm/include/llvm/Object/Minidump.h
index a388fa3349772..e45d4de0090de 100644
--- a/llvm/include/llvm/Object/Minidump.h
+++ b/llvm/include/llvm/Object/Minidump.h
@@ -52,13 +52,6 @@ class MinidumpFile : public Binary {
     return getDataSlice(getData(), Desc.RVA, Desc.DataSize);
   }
 
-  /// Returns the raw contents of an object given by the LocationDescriptor. An
-  /// error is returned if the descriptor points outside of the minidump file.
-  Expected<ArrayRef<uint8_t>>
-  getRawData(minidump::LocationDescriptor_64 Desc) const {
-    return getDataSlice(getData(), Desc.RVA, Desc.DataSize);
-  }
-
   /// Returns the minidump string at the given offset. An error is returned if
   /// we fail to parse the string, or the string is invalid UTF16.
   Expected<std::string> getString(size_t Offset) const;

>From 762fb82eb05976444e154e508ad456c234dafa83 Mon Sep 17 00:00:00 2001
From: Jacob Lalonde <jalalonde at fb.com>
Date: Wed, 12 Jun 2024 13:33:03 -0700
Subject: [PATCH 03/14] Add some comments and remove changes to ulittle64

---
 .../ObjectFile/Minidump/MinidumpFileBuilder.cpp       | 11 ++++++-----
 llvm/lib/ObjectYAML/MinidumpEmitter.cpp               |  5 -----
 2 files changed, 6 insertions(+), 10 deletions(-)

diff --git a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp
index ecb16d2fbd388..a90b6ec05b838 100644
--- a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp
+++ b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp
@@ -54,7 +54,6 @@ using namespace llvm::minidump;
 
 Status MinidumpFileBuilder::AddHeaderAndCalculateDirectories(const lldb_private::Target &target, const lldb::ProcessSP &process_sp) {
   // First set the offset on the file, and on the bytes saved
-  //TODO: advance the core file.
   m_saved_data_size += header_size;
   // We know we will have at least Misc, SystemInfo, Modules, and ThreadList (corresponding memory list for stacks)
   // And an additional memory list for non-stacks.
@@ -77,7 +76,6 @@ Status MinidumpFileBuilder::AddHeaderAndCalculateDirectories(const lldb_private:
 
   // Now offset the file by the directores so we can write them in later.
   offset_t directory_offset = m_expected_directories * directory_size;
-  //TODO: advance the core file.
   m_saved_data_size += directory_offset;
   // Replace this when we make a better way to do this.
   Status error;
@@ -113,9 +111,9 @@ Status MinidumpFileBuilder::AddHeaderAndCalculateDirectories(const lldb_private:
 
 void MinidumpFileBuilder::AddDirectory(StreamType type, uint64_t stream_size) {
   LocationDescriptor loc;
-  loc.DataSize = static_cast<llvm::support::ulittle64_t>(stream_size);
+  loc.DataSize = static_cast<llvm::support::ulittle32_t>(stream_size);
   // Stream will begin at the current end of data section
-  loc.RVA = static_cast<llvm::support::ulittle64_t>(GetCurrentDataEndOffset());
+  loc.RVA = static_cast<llvm::support::ulittle32_t>(GetCurrentDataEndOffset());
 
   Directory dir;
   dir.Type = static_cast<llvm::support::little_t<StreamType>>(type);
@@ -188,7 +186,7 @@ Status MinidumpFileBuilder::AddSystemInfo(const llvm::Triple &target_triple) {
   sys_info.ProcessorArch =
       static_cast<llvm::support::little_t<ProcessorArchitecture>>(arch);
   // Global offset to beginning of a csd_string in a data section
-  sys_info.CSDVersionRVA = static_cast<llvm::support::ulittle64_t>(
+  sys_info.CSDVersionRVA = static_cast<llvm::support::ulittle32_t>(
       GetCurrentDataEndOffset() + sizeof(llvm::minidump::SystemInfo));
   sys_info.PlatformId = platform_id;
   m_data.AppendData(&sys_info, sizeof(llvm::minidump::SystemInfo));
@@ -1097,6 +1095,9 @@ Status MinidumpFileBuilder::FlushToDisk() {
   addr_t starting_size = m_data.GetByteSize();
   addr_t remaining_bytes = starting_size;
   size_t offset = 0;
+
+  // Unix/Linux OS's return SSIZE for operations, this means a max of 2gb per IO operation
+  // so we chunk it here. We keep the file size small to try to minimize memory use.
   while (remaining_bytes > 0) {
     size_t chunk_size = std::min(remaining_bytes, m_write_chunk_max);
     size_t bytes_written = chunk_size;
diff --git a/llvm/lib/ObjectYAML/MinidumpEmitter.cpp b/llvm/lib/ObjectYAML/MinidumpEmitter.cpp
index a967e422bef67..6ca626f7842d2 100644
--- a/llvm/lib/ObjectYAML/MinidumpEmitter.cpp
+++ b/llvm/lib/ObjectYAML/MinidumpEmitter.cpp
@@ -121,11 +121,6 @@ static LocationDescriptor layout(BlobAllocator &File, yaml::BinaryRef Data) {
           support::ulittle32_t(File.allocateBytes(Data))};
 }
 
-static LocationDescriptor_64 layout_64(BlobAllocator &File, yaml::BinaryRef Data) {
-  return {support::ulittle64_t(Data.binary_size()),
-          support::ulittle64_t(File.allocateBytes(Data))};
-}
-
 static size_t layout(BlobAllocator &File, MinidumpYAML::ExceptionStream &S) {
   File.allocateObject(S.MDExceptionStream);
 

>From 5bf2dbb9e082b315deff223bd83f34244a3de192 Mon Sep 17 00:00:00 2001
From: Jacob Lalonde <jalalonde at fb.com>
Date: Wed, 12 Jun 2024 13:33:53 -0700
Subject: [PATCH 04/14] Run git clang format

---
 .../Minidump/MinidumpFileBuilder.cpp          | 159 ++++++++++--------
 .../ObjectFile/Minidump/MinidumpFileBuilder.h |  17 +-
 .../Minidump/ObjectFileMinidump.cpp           |   7 +-
 3 files changed, 107 insertions(+), 76 deletions(-)

diff --git a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp
index a90b6ec05b838..664d7100fa9ee 100644
--- a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp
+++ b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp
@@ -52,11 +52,13 @@ using namespace lldb;
 using namespace lldb_private;
 using namespace llvm::minidump;
 
-Status MinidumpFileBuilder::AddHeaderAndCalculateDirectories(const lldb_private::Target &target, const lldb::ProcessSP &process_sp) {
+Status MinidumpFileBuilder::AddHeaderAndCalculateDirectories(
+    const lldb_private::Target &target, const lldb::ProcessSP &process_sp) {
   // First set the offset on the file, and on the bytes saved
   m_saved_data_size += header_size;
-  // We know we will have at least Misc, SystemInfo, Modules, and ThreadList (corresponding memory list for stacks)
-  // And an additional memory list for non-stacks.
+  // We know we will have at least Misc, SystemInfo, Modules, and ThreadList
+  // (corresponding memory list for stacks) And an additional memory list for
+  // non-stacks.
   m_expected_directories = 6;
   // Check if OS is linux
   if (target.GetArchitecture().GetTriple().getOS() ==
@@ -69,7 +71,8 @@ Status MinidumpFileBuilder::AddHeaderAndCalculateDirectories(const lldb_private:
   for (uint32_t thread_idx = 0; thread_idx < num_threads; ++thread_idx) {
     ThreadSP thread_sp(thread_list.GetThreadAtIndex(thread_idx));
     StopInfoSP stop_info_sp = thread_sp->GetStopInfo();
-    if (stop_info_sp && stop_info_sp->GetStopReason() == StopReason::eStopReasonException) {
+    if (stop_info_sp &&
+        stop_info_sp->GetStopReason() == StopReason::eStopReasonException) {
       m_expected_directories++;
     }
   }
@@ -108,7 +111,6 @@ Status MinidumpFileBuilder::AddHeaderAndCalculateDirectories(const lldb_private:
   return error;
 }
 
-
 void MinidumpFileBuilder::AddDirectory(StreamType type, uint64_t stream_size) {
   LocationDescriptor loc;
   loc.DataSize = static_cast<llvm::support::ulittle32_t>(stream_size);
@@ -120,7 +122,7 @@ void MinidumpFileBuilder::AddDirectory(StreamType type, uint64_t stream_size) {
   dir.Location = loc;
 
   m_directories.push_back(dir);
-  assert (m_expected_directories >= m_directories.size());
+  assert(m_expected_directories >= m_directories.size());
 }
 
 Status MinidumpFileBuilder::AddSystemInfo(const llvm::Triple &target_triple) {
@@ -595,9 +597,9 @@ Status MinidumpFileBuilder::FixThreads() {
     size_t bytes_written = bytes_to_write;
     error = m_core_file->Write(&thread, bytes_written);
     if (error.Fail() || bytes_to_write != bytes_written) {
-        error.SetErrorStringWithFormat(
-                  "Wrote incorrect number of bytes to minidump file. (written %zd/%zd)", bytes_written,
-                  bytes_to_write);
+      error.SetErrorStringWithFormat(
+          "Wrote incorrect number of bytes to minidump file. (written %zd/%zd)",
+          bytes_written, bytes_to_write);
       return error;
     }
   }
@@ -820,11 +822,13 @@ void MinidumpFileBuilder::AddLinuxFileStreams(
   }
 }
 
-Status MinidumpFileBuilder::AddMemory(const ProcessSP &process_sp, SaveCoreStyle core_style) {
+Status MinidumpFileBuilder::AddMemory(const ProcessSP &process_sp,
+                                      SaveCoreStyle core_style) {
   Status error;
 
   Process::CoreFileMemoryRanges ranges_for_memory_list;
-  error = process_sp->CalculateCoreFileSaveRanges(SaveCoreStyle::eSaveCoreStackOnly, ranges_for_memory_list);
+  error = process_sp->CalculateCoreFileSaveRanges(
+      SaveCoreStyle::eSaveCoreStackOnly, ranges_for_memory_list);
   if (error.Fail()) {
     return error;
   }
@@ -833,42 +837,42 @@ Status MinidumpFileBuilder::AddMemory(const ProcessSP &process_sp, SaveCoreStyle
   for (const auto &core_range : ranges_for_memory_list) {
     stack_ranges.insert(core_range.range.start());
   }
-  // We leave a little padding for dictionary and any other metadata we would want.
-  // Also so that we can put the header of the memory list 64 in 32b land, because the directory
-  // requires a 32b RVA.
+  // We leave a little padding for dictionary and any other metadata we would
+  // want. Also so that we can put the header of the memory list 64 in 32b land,
+  // because the directory requires a 32b RVA.
   Process::CoreFileMemoryRanges ranges;
   error = process_sp->CalculateCoreFileSaveRanges(core_style, ranges);
   if (error.Fail()) {
     return error;
   }
 
-  uint64_t total_size = 256 + (ranges.size() * sizeof(llvm::minidump::MemoryDescriptor_64));
+  uint64_t total_size =
+      256 + (ranges.size() * sizeof(llvm::minidump::MemoryDescriptor_64));
   // Take all the memory that will fit in the 32b range.
   for (int i = ranges.size() - 1; i >= 0; i--) {
-    addr_t size_to_add = ranges[i].range.size() + sizeof(llvm::minidump::MemoryDescriptor);
+    addr_t size_to_add =
+        ranges[i].range.size() + sizeof(llvm::minidump::MemoryDescriptor);
     // filter out the stacks and check if it's below 32b max.
     if (stack_ranges.count(ranges[i].range.start()) > 0) {
       ranges.erase(ranges.begin() + i);
-    }
-    else if (total_size + size_to_add < UINT32_MAX) {
+    } else if (total_size + size_to_add < UINT32_MAX) {
       ranges_for_memory_list.push_back(ranges[i]);
       total_size += ranges[i].range.size();
       total_size += sizeof(llvm::minidump::MemoryDescriptor);
       ranges.erase(ranges.begin() + i);
-    }
-    else {
+    } else {
       break;
     }
   }
 
   error = AddMemoryList_32(process_sp, ranges_for_memory_list);
-  if (error.Fail()) 
+  if (error.Fail())
     return error;
 
   // Add the remaining memory as a 64b range.
   if (ranges.size() > 0) {
     error = AddMemoryList_64(process_sp, ranges);
-    if (error.Fail()) 
+    if (error.Fail())
       return error;
   }
 
@@ -933,7 +937,8 @@ Status MinidumpFileBuilder::DumpDirectories() const {
   return error;
 }
 
-Status MinidumpFileBuilder::AddMemoryList_32(const ProcessSP &process_sp, const Process::CoreFileMemoryRanges &ranges) {
+Status MinidumpFileBuilder::AddMemoryList_32(
+    const ProcessSP &process_sp, const Process::CoreFileMemoryRanges &ranges) {
   std::vector<MemoryDescriptor> descriptors;
   Status error;
   Log *log = GetLog(LLDBLog::Object);
@@ -943,11 +948,15 @@ Status MinidumpFileBuilder::AddMemoryList_32(const ProcessSP &process_sp, const
     const size_t offset_for_data = GetCurrentDataEndOffset();
     const addr_t addr = core_range.range.start();
     const addr_t size = core_range.range.size();
-    std::cout << "Adding Memory Desciptor at " << std::hex << core_range.range.start() << std::dec << " with size " << core_range.range.size() << std::endl;
+    std::cout << "Adding Memory Desciptor at " << std::hex
+              << core_range.range.start() << std::dec << " with size "
+              << core_range.range.size() << std::endl;
     auto data_up = std::make_unique<DataBufferHeap>(size, 0);
 
-    LLDB_LOGF(log, "AddMemoryList %d/%d reading memory for region (%zu bytes) [%zx, %zx)",
-              region_index, ranges.size(), size, addr, addr+ size);
+    LLDB_LOGF(
+        log,
+        "AddMemoryList %d/%d reading memory for region (%zu bytes) [%zx, %zx)",
+        region_index, ranges.size(), size, addr, addr + size);
     ++region_index;
 
     const size_t bytes_read =
@@ -956,16 +965,19 @@ Status MinidumpFileBuilder::AddMemoryList_32(const ProcessSP &process_sp, const
       LLDB_LOGF(log, "Failed to read memory region. Bytes read: %zu, error: %s",
                 bytes_read, error.AsCString());
       // Just skip sections with errors or zero bytes in 32b mode
-      continue; 
-    }
-    else if (bytes_read != size) {
-      LLDB_LOGF(log, "Memory region at: %zu failed to read %zu bytes", addr, size);
+      continue;
+    } else if (bytes_read != size) {
+      LLDB_LOGF(log, "Memory region at: %zu failed to read %zu bytes", addr,
+                size);
     }
 
     MemoryDescriptor descriptor;
-    descriptor.StartOfMemoryRange = static_cast<llvm::support::ulittle64_t>(addr);
-    descriptor.Memory.DataSize = static_cast<llvm::support::ulittle32_t>(bytes_read);
-    descriptor.Memory.RVA = static_cast<llvm::support::ulittle32_t>(offset_for_data);
+    descriptor.StartOfMemoryRange =
+        static_cast<llvm::support::ulittle64_t>(addr);
+    descriptor.Memory.DataSize =
+        static_cast<llvm::support::ulittle32_t>(bytes_read);
+    descriptor.Memory.RVA =
+        static_cast<llvm::support::ulittle32_t>(offset_for_data);
     descriptors.push_back(descriptor);
     if (m_thread_by_range_start.count(addr) > 0) {
       m_thread_by_range_start[addr].Stack = descriptor;
@@ -981,40 +993,48 @@ Status MinidumpFileBuilder::AddMemoryList_32(const ProcessSP &process_sp, const
   // With a size of the number of ranges as a 32 bit num
   // And then the size of all the ranges
   AddDirectory(StreamType::MemoryList,
-            sizeof(llvm::support::ulittle32_t) +
-                descriptors.size() *
-                    sizeof(llvm::minidump::MemoryDescriptor));
+               sizeof(llvm::support::ulittle32_t) +
+                   descriptors.size() *
+                       sizeof(llvm::minidump::MemoryDescriptor));
 
-  llvm::support::ulittle32_t memory_ranges_num = static_cast<llvm::support::ulittle32_t>(descriptors.size());
+  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));
-  // 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(), descriptors.size() * sizeof(MemoryDescriptor));
+  // 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(),
+                    descriptors.size() * sizeof(MemoryDescriptor));
 
   return error;
 }
 
-Status MinidumpFileBuilder::AddMemoryList_64(const ProcessSP &process_sp, const Process::CoreFileMemoryRanges &ranges) {
+Status MinidumpFileBuilder::AddMemoryList_64(
+    const ProcessSP &process_sp, const Process::CoreFileMemoryRanges &ranges) {
   AddDirectory(StreamType::Memory64List,
-            (sizeof(llvm::support::ulittle64_t) * 2) +
-                ranges.size() *
-                    sizeof(llvm::minidump::MemoryDescriptor_64));
+               (sizeof(llvm::support::ulittle64_t) * 2) +
+                   ranges.size() * sizeof(llvm::minidump::MemoryDescriptor_64));
 
-  llvm::support::ulittle64_t memory_ranges_num = static_cast<llvm::support::ulittle64_t>(ranges.size());
+  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));
-  llvm::support::ulittle64_t memory_ranges_base_rva = static_cast<llvm::support::ulittle64_t>(GetCurrentDataEndOffset());
-  m_data.AppendData(&memory_ranges_base_rva, sizeof(llvm::support::ulittle64_t));
+  llvm::support::ulittle64_t memory_ranges_base_rva =
+      static_cast<llvm::support::ulittle64_t>(GetCurrentDataEndOffset());
+  m_data.AppendData(&memory_ranges_base_rva,
+                    sizeof(llvm::support::ulittle64_t));
   // Capture the starting offset, so we can do cleanup later if needed.
   uint64_t starting_offset = GetCurrentDataEndOffset();
 
   bool cleanup_required = false;
   std::vector<MemoryDescriptor_64> descriptors;
-  // Enumerate the ranges and create the memory descriptors so we can append them first
+  // Enumerate the ranges and create the memory descriptors so we can append
+  // them first
   for (const auto core_range : ranges) {
     // Add the space required to store the memory descriptor
     MemoryDescriptor_64 memory_desc;
     memory_desc.StartOfMemoryRange =
         static_cast<llvm::support::ulittle64_t>(core_range.range.start());
-    memory_desc.DataSize = static_cast<llvm::support::ulittle64_t>(core_range.range.size());
+    memory_desc.DataSize =
+        static_cast<llvm::support::ulittle64_t>(core_range.range.size());
     descriptors.push_back(memory_desc);
     // Now write this memory descriptor to the buffer.
     m_data.AppendData(&memory_desc, sizeof(MemoryDescriptor_64));
@@ -1028,8 +1048,10 @@ Status MinidumpFileBuilder::AddMemoryList_64(const ProcessSP &process_sp, const
     const addr_t size = core_range.range.size();
     auto data_up = std::make_unique<DataBufferHeap>(size, 0);
 
-    LLDB_LOGF(log, "AddMemoryList_64 %d/%d reading memory for region (%zu bytes) [%zx, %zx)",
-              region_index, ranges.size(), size, addr, addr+ size);
+    LLDB_LOGF(log,
+              "AddMemoryList_64 %d/%d reading memory for region (%zu bytes) "
+              "[%zx, %zx)",
+              region_index, ranges.size(), size, addr, addr + size);
     ++region_index;
 
     const size_t bytes_read =
@@ -1042,7 +1064,8 @@ Status MinidumpFileBuilder::AddMemoryList_64(const ProcessSP &process_sp, const
       descriptors[region_index].DataSize = 0;
     }
     if (bytes_read != size) {
-      LLDB_LOGF(log, "Memory region at: %zu failed to read %zu bytes", addr, size);
+      LLDB_LOGF(log, "Memory region at: %zu failed to read %zu bytes", addr,
+                size);
       cleanup_required = true;
       descriptors[region_index].DataSize = bytes_read;
     }
@@ -1056,30 +1079,31 @@ Status MinidumpFileBuilder::AddMemoryList_64(const ProcessSP &process_sp, const
   // Early return if there is no cleanup needed.
   if (!cleanup_required) {
     return error;
-  }
-  else {
+  } else {
     // Flush to disk we can make the fixes in place.
     FlushToDisk();
     // Fixup the descriptors that were not read correctly.
     m_core_file->SeekFromStart(starting_offset);
     size_t bytes_written = sizeof(MemoryDescriptor_64) * descriptors.size();
     error = m_core_file->Write(descriptors.data(), bytes_written);
-    if (error.Fail() || bytes_written != sizeof(MemoryDescriptor_64) * descriptors.size()) {
-        error.SetErrorStringWithFormat(
-            "unable to write the memory descriptors (written %zd/%zd)",
-            bytes_written, sizeof(MemoryDescriptor_64) * descriptors.size());
+    if (error.Fail() ||
+        bytes_written != sizeof(MemoryDescriptor_64) * descriptors.size()) {
+      error.SetErrorStringWithFormat(
+          "unable to write the memory descriptors (written %zd/%zd)",
+          bytes_written, sizeof(MemoryDescriptor_64) * descriptors.size());
     }
 
     return error;
   }
 }
 
-Status MinidumpFileBuilder::AddData(const void* data, addr_t size) {
-  // This should also get chunked, because worst case we copy over a big 
+Status MinidumpFileBuilder::AddData(const void *data, addr_t size) {
+  // This should also get chunked, because worst case we copy over a big
   // object / memory range, say 5gb. In that case, we'd have to allocate 10gb
-  // 5 gb for the buffer we're copying from, and then 5gb for the buffer we're copying to.
-  // Which will be short lived and immedaitely go to disk, the goal here is to limit the number
-  // of bytes we need to host in memory at any given time.
+  // 5 gb for the buffer we're copying from, and then 5gb for the buffer we're
+  // copying to. Which will be short lived and immedaitely go to disk, the goal
+  // here is to limit the number of bytes we need to host in memory at any given
+  // time.
   m_data.AppendData(data, size);
   if (m_data.GetByteSize() > m_write_chunk_max) {
     return FlushToDisk();
@@ -1096,16 +1120,17 @@ Status MinidumpFileBuilder::FlushToDisk() {
   addr_t remaining_bytes = starting_size;
   size_t offset = 0;
 
-  // Unix/Linux OS's return SSIZE for operations, this means a max of 2gb per IO operation
-  // so we chunk it here. We keep the file size small to try to minimize memory use.
+  // Unix/Linux OS's return SSIZE for operations, this means a max of 2gb per IO
+  // operation so we chunk it here. We keep the file size small to try to
+  // minimize memory use.
   while (remaining_bytes > 0) {
     size_t chunk_size = std::min(remaining_bytes, m_write_chunk_max);
     size_t bytes_written = chunk_size;
     error = m_core_file->Write(m_data.GetBytes() + offset, bytes_written);
     if (error.Fail() || bytes_written != chunk_size) {
-        error.SetErrorStringWithFormat(
-            "Wrote incorrect number of bytes to minidump file. (written %zd/%zd)", bytes_written,
-            chunk_size);
+      error.SetErrorStringWithFormat(
+          "Wrote incorrect number of bytes to minidump file. (written %zd/%zd)",
+          bytes_written, chunk_size);
       return error;
     }
 
diff --git a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h
index c5fe9836f06a5..178318a2d6d4b 100644
--- a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h
+++ b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h
@@ -56,7 +56,9 @@ class MinidumpFileBuilder {
 
   ~MinidumpFileBuilder() = default;
 
-  lldb_private::Status AddHeaderAndCalculateDirectories(const lldb_private::Target &target, const lldb::ProcessSP &process_sp);
+  lldb_private::Status
+  AddHeaderAndCalculateDirectories(const lldb_private::Target &target,
+                                   const lldb::ProcessSP &process_sp);
   // Add SystemInfo stream, used for storing the most basic information
   // about the system, platform etc...
   lldb_private::Status AddSystemInfo(const llvm::Triple &target_triple);
@@ -77,16 +79,21 @@ class MinidumpFileBuilder {
   lldb_private::Status AddThreadList(const lldb::ProcessSP &process_sp);
 
   lldb_private::Status AddMemory(const lldb::ProcessSP &process_sp,
-                                     lldb::SaveCoreStyle core_style);
+                                 lldb::SaveCoreStyle core_style);
 
   lldb_private::Status DumpToFile();
 
 private:
-  // Add data to the end of the buffer, if the buffer exceeds the flush level, trigger a flush.
+  // Add data to the end of the buffer, if the buffer exceeds the flush level,
+  // trigger a flush.
   lldb_private::Status AddData(const void *data, size_t size);
   // Add MemoryList stream, containing dumps of important memory segments
-  lldb_private::Status AddMemoryList_64(const lldb::ProcessSP &process_sp, const lldb_private::Process::CoreFileMemoryRanges &ranges);
-  lldb_private::Status AddMemoryList_32(const lldb::ProcessSP &process_sp, const lldb_private::Process::CoreFileMemoryRanges &ranges);
+  lldb_private::Status
+  AddMemoryList_64(const lldb::ProcessSP &process_sp,
+                   const lldb_private::Process::CoreFileMemoryRanges &ranges);
+  lldb_private::Status
+  AddMemoryList_32(const lldb::ProcessSP &process_sp,
+                   const lldb_private::Process::CoreFileMemoryRanges &ranges);
   lldb_private::Status FixThreads();
   lldb_private::Status FlushToDisk();
 
diff --git a/lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.cpp b/lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.cpp
index 3435d89accc49..b490ff3e17ad3 100644
--- a/lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.cpp
+++ b/lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.cpp
@@ -74,9 +74,8 @@ bool ObjectFileMinidump::SaveCore(const lldb::ProcessSP &process_sp,
   }
   MinidumpFileBuilder builder(std::move(maybe_core_file.get()));
 
-
   Target &target = process_sp->GetTarget();
-    builder.AddHeaderAndCalculateDirectories(target, process_sp);
+  builder.AddHeaderAndCalculateDirectories(target, process_sp);
 
   Log *log = GetLog(LLDBLog::Object);
   error = builder.AddSystemInfo(target.GetArchitecture().GetTriple());
@@ -101,8 +100,8 @@ bool ObjectFileMinidump::SaveCore(const lldb::ProcessSP &process_sp,
   // Add any exceptions but only if there are any in any threads.
   builder.AddExceptions(process_sp);
 
-  // Note: add memory HAS to be the last thing we do. It can overflow into 64b land
-  // and many RVA's only support 32b
+  // Note: add memory HAS to be the last thing we do. It can overflow into 64b
+  // land and many RVA's only support 32b
   error = builder.AddMemory(process_sp, core_style);
   if (error.Fail()) {
     LLDB_LOGF(log, "AddMemoryList failed: %s", error.AsCString());

>From c592936dedb0dc494b03144d741ce57ac0b27809 Mon Sep 17 00:00:00 2001
From: Jacob Lalonde <jalalonde at fb.com>
Date: Wed, 12 Jun 2024 13:41:39 -0700
Subject: [PATCH 05/14] Change log to zu and region index to size_t

---
 .../ObjectFile/Minidump/MinidumpFileBuilder.cpp       | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp
index 664d7100fa9ee..3c1d88652cc92 100644
--- a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp
+++ b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp
@@ -942,20 +942,17 @@ Status MinidumpFileBuilder::AddMemoryList_32(
   std::vector<MemoryDescriptor> descriptors;
   Status error;
   Log *log = GetLog(LLDBLog::Object);
-  int region_index = 0;
+  size_t region_index = 0;
   for (const auto &core_range : ranges) {
     // Take the offset before we write.
     const size_t offset_for_data = GetCurrentDataEndOffset();
     const addr_t addr = core_range.range.start();
     const addr_t size = core_range.range.size();
-    std::cout << "Adding Memory Desciptor at " << std::hex
-              << core_range.range.start() << std::dec << " with size "
-              << core_range.range.size() << std::endl;
     auto data_up = std::make_unique<DataBufferHeap>(size, 0);
 
     LLDB_LOGF(
         log,
-        "AddMemoryList %d/%d reading memory for region (%zu bytes) [%zx, %zx)",
+        "AddMemoryList %zu/%zu reading memory for region (%zu bytes) [%zx, %zx)",
         region_index, ranges.size(), size, addr, addr + size);
     ++region_index;
 
@@ -1042,14 +1039,14 @@ Status MinidumpFileBuilder::AddMemoryList_64(
 
   Status error;
   Log *log = GetLog(LLDBLog::Object);
-  int region_index = 0;
+  size_t region_index = 0;
   for (const auto &core_range : ranges) {
     const addr_t addr = core_range.range.start();
     const addr_t size = core_range.range.size();
     auto data_up = std::make_unique<DataBufferHeap>(size, 0);
 
     LLDB_LOGF(log,
-              "AddMemoryList_64 %d/%d reading memory for region (%zu bytes) "
+              "AddMemoryList_64 %zu/%zu reading memory for region (%zu bytes) "
               "[%zx, %zx)",
               region_index, ranges.size(), size, addr, addr + size);
     ++region_index;

>From 051ebab836a32eeb60be32e4284b65c5f04140d6 Mon Sep 17 00:00:00 2001
From: Jacob Lalonde <jalalonde at fb.com>
Date: Thu, 13 Jun 2024 13:17:14 -0700
Subject: [PATCH 06/14] Code review feedback and an attempt to undo unintended
 changes

---
 .../Minidump/MinidumpFileBuilder.cpp          | 174 ++++++++----------
 .../ObjectFile/Minidump/MinidumpFileBuilder.h |  39 ++--
 .../Minidump/ObjectFileMinidump.cpp           |  19 +-
 llvm/lib/ObjectYAML/MinidumpEmitter.cpp       |   2 -
 4 files changed, 103 insertions(+), 131 deletions(-)

diff --git a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp
index 3c1d88652cc92..f379f4d1d7898 100644
--- a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp
+++ b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp
@@ -32,6 +32,7 @@
 #include "llvm/Support/ConvertUTF.h"
 #include "llvm/Support/Endian.h"
 #include "llvm/Support/Error.h"
+#include "llvm/TargetParser/Triple.h"
 
 #include "Plugins/Process/minidump/MinidumpTypes.h"
 #include "lldb/lldb-enumerations.h"
@@ -52,13 +53,14 @@ using namespace lldb;
 using namespace lldb_private;
 using namespace llvm::minidump;
 
-Status MinidumpFileBuilder::AddHeaderAndCalculateDirectories(
-    const lldb_private::Target &target, const lldb::ProcessSP &process_sp) {
+Status MinidumpFileBuilder::AddHeaderAndCalculateDirectories() {
   // First set the offset on the file, and on the bytes saved
   m_saved_data_size += header_size;
   // We know we will have at least Misc, SystemInfo, Modules, and ThreadList
   // (corresponding memory list for stacks) And an additional memory list for
   // non-stacks.
+
+  lldb_private::Target& target = m_process_sp->GetTarget();
   m_expected_directories = 6;
   // Check if OS is linux
   if (target.GetArchitecture().GetTriple().getOS() ==
@@ -66,7 +68,7 @@ Status MinidumpFileBuilder::AddHeaderAndCalculateDirectories(
     m_expected_directories += 9;
 
   // Go through all of the threads and check for exceptions.
-  lldb_private::ThreadList thread_list = process_sp->GetThreadList();
+  lldb_private::ThreadList thread_list = m_process_sp->GetThreadList();
   const uint32_t num_threads = thread_list.GetSize();
   for (uint32_t thread_idx = 0; thread_idx < num_threads; ++thread_idx) {
     ThreadSP thread_sp(thread_list.GetThreadAtIndex(thread_idx));
@@ -125,8 +127,9 @@ void MinidumpFileBuilder::AddDirectory(StreamType type, uint64_t stream_size) {
   assert(m_expected_directories >= m_directories.size());
 }
 
-Status MinidumpFileBuilder::AddSystemInfo(const llvm::Triple &target_triple) {
+Status MinidumpFileBuilder::AddSystemInfo() {
   Status error;
+  const llvm::Triple &target_triple = m_process_sp->GetTarget().GetArchitecture().GetTriple();
   AddDirectory(StreamType::SystemInfo, sizeof(llvm::minidump::SystemInfo));
 
   llvm::minidump::ProcessorArchitecture arch;
@@ -276,10 +279,11 @@ llvm::Expected<uint64_t> getModuleFileSize(Target &target,
 // single module. Additional data of variable length, such as module's names,
 // are stored just after the ModuleList stream. The llvm::minidump::Module
 // structures point to this helper data by global offset.
-Status MinidumpFileBuilder::AddModuleList(Target &target) {
+Status MinidumpFileBuilder::AddModuleList() {
   constexpr size_t minidump_module_size = sizeof(llvm::minidump::Module);
   Status error;
 
+  lldb_private::Target& target = m_process_sp->GetTarget();
   const ModuleList &modules = target.GetImages();
   llvm::support::ulittle32_t modules_count =
       static_cast<llvm::support::ulittle32_t>(modules.GetSize());
@@ -551,39 +555,6 @@ class ArchThreadContexts {
   }
 };
 
-// Function returns start and size of the memory region that contains
-// memory location pointed to by the current stack pointer.
-llvm::Expected<std::pair<addr_t, addr_t>>
-findStackHelper(const lldb::ProcessSP &process_sp, uint64_t rsp) {
-  MemoryRegionInfo range_info;
-  Status error = process_sp->GetMemoryRegionInfo(rsp, range_info);
-  // Skip failed memory region requests or any regions with no permissions.
-  if (error.Fail() || range_info.GetLLDBPermissions() == 0)
-    return llvm::createStringError(
-        std::errc::not_supported,
-        "unable to load stack segment of the process");
-
-  // This is a duplicate of the logic in
-  // Process::SaveOffRegionsWithStackPointers but ultimately, we need to only
-  // save up from the start of the stack down to the stack pointer
-  const addr_t range_end = range_info.GetRange().GetRangeEnd();
-  const addr_t red_zone = process_sp->GetABI()->GetRedZoneSize();
-  const addr_t stack_head = rsp - red_zone;
-  if (stack_head > range_info.GetRange().GetRangeEnd()) {
-    range_info.GetRange().SetRangeBase(stack_head);
-    range_info.GetRange().SetByteSize(range_end - stack_head);
-  }
-
-  const addr_t addr = range_info.GetRange().GetRangeBase();
-  const addr_t size = range_info.GetRange().GetByteSize();
-
-  if (size == 0)
-    return llvm::createStringError(std::errc::not_supported,
-                                   "stack segment of the process is empty");
-
-  return std::make_pair(addr, size);
-}
-
 Status MinidumpFileBuilder::FixThreads() {
   Status error;
   // If we have anything in the heap flush it.
@@ -607,9 +578,9 @@ Status MinidumpFileBuilder::FixThreads() {
   return error;
 }
 
-Status MinidumpFileBuilder::AddThreadList(const lldb::ProcessSP &process_sp) {
+Status MinidumpFileBuilder::AddThreadList() {
   constexpr size_t minidump_thread_size = sizeof(llvm::minidump::Thread);
-  lldb_private::ThreadList thread_list = process_sp->GetThreadList();
+  lldb_private::ThreadList thread_list = m_process_sp->GetThreadList();
 
   // size of the entire thread stream consists of:
   // number of threads and threads array
@@ -640,7 +611,7 @@ Status MinidumpFileBuilder::AddThreadList(const lldb::ProcessSP &process_sp) {
       return error;
     }
     RegisterContext *reg_ctx = reg_ctx_sp.get();
-    Target &target = process_sp->GetTarget();
+    Target &target = m_process_sp->GetTarget();
     const ArchSpec &arch = target.GetArchitecture();
     ArchThreadContexts thread_context(arch.GetMachine());
     if (!thread_context.prepareRegisterContext(reg_ctx)) {
@@ -652,7 +623,7 @@ Status MinidumpFileBuilder::AddThreadList(const lldb::ProcessSP &process_sp) {
 
     uint64_t sp = reg_ctx->GetSP();
     MemoryRegionInfo sp_region;
-    process_sp->GetMemoryRegionInfo(sp, sp_region);
+    m_process_sp->GetMemoryRegionInfo(sp, sp_region);
 
     // Emit a blank descriptor
     MemoryDescriptor stack;
@@ -693,8 +664,8 @@ Status MinidumpFileBuilder::AddThreadList(const lldb::ProcessSP &process_sp) {
   return Status();
 }
 
-void MinidumpFileBuilder::AddExceptions(const lldb::ProcessSP &process_sp) {
-  lldb_private::ThreadList thread_list = process_sp->GetThreadList();
+void MinidumpFileBuilder::AddExceptions() {
+  lldb_private::ThreadList thread_list = m_process_sp->GetThreadList();
 
   const uint32_t num_threads = thread_list.GetSize();
   for (uint32_t thread_idx = 0; thread_idx < num_threads; ++thread_idx) {
@@ -744,7 +715,7 @@ void MinidumpFileBuilder::AddExceptions(const lldb::ProcessSP &process_sp) {
   }
 }
 
-void MinidumpFileBuilder::AddMiscInfo(const lldb::ProcessSP &process_sp) {
+void MinidumpFileBuilder::AddMiscInfo() {
   AddDirectory(StreamType::MiscInfo,
                sizeof(lldb_private::minidump::MinidumpMiscInfo));
 
@@ -756,7 +727,7 @@ void MinidumpFileBuilder::AddMiscInfo(const lldb::ProcessSP &process_sp) {
   misc_info.flags1 = static_cast<llvm::support::ulittle32_t>(0);
 
   lldb_private::ProcessInstanceInfo process_info;
-  process_sp->GetProcessInfo(process_info);
+  m_process_sp->GetProcessInfo(process_info);
   if (process_info.ProcessIDIsValid()) {
     // Set flags1 to reflect that PID is filled in
     misc_info.flags1 =
@@ -778,15 +749,14 @@ getFileStreamHelper(const std::string &path) {
   return std::move(maybe_stream.get());
 }
 
-void MinidumpFileBuilder::AddLinuxFileStreams(
-    const lldb::ProcessSP &process_sp) {
+void MinidumpFileBuilder::AddLinuxFileStreams() {
   std::vector<std::pair<StreamType, std::string>> files_with_stream_types = {
       {StreamType::LinuxCPUInfo, "/proc/cpuinfo"},
       {StreamType::LinuxLSBRelease, "/etc/lsb-release"},
   };
 
   lldb_private::ProcessInstanceInfo process_info;
-  process_sp->GetProcessInfo(process_info);
+  m_process_sp->GetProcessInfo(process_info);
   if (process_info.ProcessIDIsValid()) {
     lldb::pid_t pid = process_info.GetProcessID();
     std::string pid_str = std::to_string(pid);
@@ -822,56 +792,66 @@ void MinidumpFileBuilder::AddLinuxFileStreams(
   }
 }
 
-Status MinidumpFileBuilder::AddMemory(const ProcessSP &process_sp,
-                                      SaveCoreStyle core_style) {
+Status MinidumpFileBuilder::AddMemory(SaveCoreStyle core_style) {
   Status error;
 
-  Process::CoreFileMemoryRanges ranges_for_memory_list;
-  error = process_sp->CalculateCoreFileSaveRanges(
-      SaveCoreStyle::eSaveCoreStackOnly, ranges_for_memory_list);
-  if (error.Fail()) {
+  Process::CoreFileMemoryRanges ranges_32;
+  Process::CoreFileMemoryRanges ranges_64;
+  error = m_process_sp->CalculateCoreFileSaveRanges(
+      SaveCoreStyle::eSaveCoreStackOnly, ranges_32);
+  if (error.Fail())
     return error;
-  }
 
-  std::set<addr_t> stack_ranges;
-  for (const auto &core_range : ranges_for_memory_list) {
-    stack_ranges.insert(core_range.range.start());
-  }
-  // We leave a little padding for dictionary and any other metadata we would
-  // want. Also so that we can put the header of the memory list 64 in 32b land,
-  // because the directory requires a 32b RVA.
-  Process::CoreFileMemoryRanges ranges;
-  error = process_sp->CalculateCoreFileSaveRanges(core_style, ranges);
-  if (error.Fail()) {
+  std::set<addr_t> stack_start_addresses;
+  for (const auto &core_range : ranges_32)
+    stack_start_addresses.insert(core_range.range.start());
+
+
+
+
+  uint64_t total_size = ranges_32.size() * sizeof(llvm::minidump::MemoryDescriptor);
+  for (const auto &core_range : ranges_32)
+    total_size += core_range.range.size();
+
+  if (total_size >= UINT32_MAX) {
+    error.SetErrorStringWithFormat("Unable to write minidump. Stack memory exceeds 32b limit. (Num Stacks %zu)", ranges_32.size());
     return error;
   }
 
-  uint64_t total_size =
-      256 + (ranges.size() * sizeof(llvm::minidump::MemoryDescriptor_64));
-  // Take all the memory that will fit in the 32b range.
-  for (int i = ranges.size() - 1; i >= 0; i--) {
-    addr_t size_to_add =
-        ranges[i].range.size() + sizeof(llvm::minidump::MemoryDescriptor);
-    // filter out the stacks and check if it's below 32b max.
-    if (stack_ranges.count(ranges[i].range.start()) > 0) {
-      ranges.erase(ranges.begin() + i);
+  Process::CoreFileMemoryRanges all_core_memory_ranges;
+  if (core_style != SaveCoreStyle::eSaveCoreStackOnly) {
+    error = m_process_sp->CalculateCoreFileSaveRanges(core_style, all_core_memory_ranges);
+    if (error.Fail())
+      return error;
+  }
+
+  // We need to calculate the MemoryDescriptor size in the worst case
+  // Where all memory descriptors are 64b. We also leave some additional padding
+  // So that we convert over to 64b with space to spare. This does not waste space in the dump
+  // But instead converts some memory from being in the memorylist_32 to the memorylist_64.
+  total_size += 256 + (ranges_64.size() - stack_start_addresses.size()) * sizeof(llvm::minidump::MemoryDescriptor_64);
+
+  for (const auto &core_range : all_core_memory_ranges) {
+    addr_t size_to_add = core_range.range.size() + sizeof(llvm::minidump::MemoryDescriptor);
+    if (stack_start_addresses.count(core_range.range.start()) > 0) {
+      // Don't double save stacks.
+      continue;
     } else if (total_size + size_to_add < UINT32_MAX) {
-      ranges_for_memory_list.push_back(ranges[i]);
-      total_size += ranges[i].range.size();
+      ranges_32.push_back(core_range);
+      total_size += core_range.range.size();
       total_size += sizeof(llvm::minidump::MemoryDescriptor);
-      ranges.erase(ranges.begin() + i);
     } else {
-      break;
+      ranges_64.push_back(core_range);
     }
   }
 
-  error = AddMemoryList_32(process_sp, ranges_for_memory_list);
+  error = AddMemoryList_32(ranges_32);
   if (error.Fail())
     return error;
 
   // Add the remaining memory as a 64b range.
-  if (ranges.size() > 0) {
-    error = AddMemoryList_64(process_sp, ranges);
+  if (ranges_64.size() > 0) {
+    error = AddMemoryList_64(ranges_64);
     if (error.Fail())
       return error;
   }
@@ -907,7 +887,7 @@ Status MinidumpFileBuilder::DumpHeader() const {
   if (error.Fail() || bytes_written != header_size) {
     if (bytes_written != header_size)
       error.SetErrorStringWithFormat(
-          "unable to write the header (written %zd/%zd)", bytes_written,
+          "Unable to write the minidump header (written %zd/%zd)", bytes_written,
           header_size);
     return error;
   }
@@ -937,8 +917,7 @@ Status MinidumpFileBuilder::DumpDirectories() const {
   return error;
 }
 
-Status MinidumpFileBuilder::AddMemoryList_32(
-    const ProcessSP &process_sp, const Process::CoreFileMemoryRanges &ranges) {
+Status MinidumpFileBuilder::AddMemoryList_32(const Process::CoreFileMemoryRanges &ranges) {
   std::vector<MemoryDescriptor> descriptors;
   Status error;
   Log *log = GetLog(LLDBLog::Object);
@@ -952,12 +931,12 @@ Status MinidumpFileBuilder::AddMemoryList_32(
 
     LLDB_LOGF(
         log,
-        "AddMemoryList %zu/%zu reading memory for region (%zu bytes) [%zx, %zx)",
+        "/AddMemoryList/AddMemory/ %zu/%zu reading memory for region (%zu bytes) [%zx, %zx)",
         region_index, ranges.size(), size, addr, addr + size);
     ++region_index;
 
     const size_t bytes_read =
-        process_sp->ReadMemory(addr, data_up->GetBytes(), size, error);
+        m_process_sp->ReadMemory(addr, data_up->GetBytes(), size, error);
     if (error.Fail() || bytes_read == 0) {
       LLDB_LOGF(log, "Failed to read memory region. Bytes read: %zu, error: %s",
                 bytes_read, error.AsCString());
@@ -1005,8 +984,7 @@ Status MinidumpFileBuilder::AddMemoryList_32(
   return error;
 }
 
-Status MinidumpFileBuilder::AddMemoryList_64(
-    const ProcessSP &process_sp, const Process::CoreFileMemoryRanges &ranges) {
+Status MinidumpFileBuilder::AddMemoryList_64(const Process::CoreFileMemoryRanges &ranges) {
   AddDirectory(StreamType::Memory64List,
                (sizeof(llvm::support::ulittle64_t) * 2) +
                    ranges.size() * sizeof(llvm::minidump::MemoryDescriptor_64));
@@ -1046,13 +1024,13 @@ Status MinidumpFileBuilder::AddMemoryList_64(
     auto data_up = std::make_unique<DataBufferHeap>(size, 0);
 
     LLDB_LOGF(log,
-              "AddMemoryList_64 %zu/%zu reading memory for region (%zu bytes) "
+              "/AddMemoryList_64/AddMemory %zu/%zu reading memory for region (%zu bytes) "
               "[%zx, %zx)",
               region_index, ranges.size(), size, addr, addr + size);
     ++region_index;
 
     const size_t bytes_read =
-        process_sp->ReadMemory(addr, data_up->GetBytes(), size, error);
+        m_process_sp->ReadMemory(addr, data_up->GetBytes(), size, error);
     if (error.Fail()) {
       LLDB_LOGF(log, "Failed to read memory region. Bytes read: %zu, error: %s",
                 bytes_read, error.AsCString());
@@ -1115,19 +1093,17 @@ Status MinidumpFileBuilder::FlushToDisk() {
   m_core_file->SeekFromEnd(0);
   addr_t starting_size = m_data.GetByteSize();
   addr_t remaining_bytes = starting_size;
-  size_t offset = 0;
+  offset_t offset = 0;
 
-  // Unix/Linux OS's return SSIZE for operations, this means a max of 2gb per IO
-  // operation so we chunk it here. We keep the file size small to try to
-  // minimize memory use.
   while (remaining_bytes > 0) {
-    size_t chunk_size = std::min(remaining_bytes, m_write_chunk_max);
-    size_t bytes_written = chunk_size;
+    size_t bytes_written = 0;
+    // We don't care how many bytes we wrote unless we got an error
+    // so just decrement the remaining bytes.
     error = m_core_file->Write(m_data.GetBytes() + offset, bytes_written);
-    if (error.Fail() || bytes_written != chunk_size) {
+    if (error.Fail()) {
       error.SetErrorStringWithFormat(
           "Wrote incorrect number of bytes to minidump file. (written %zd/%zd)",
-          bytes_written, chunk_size);
+          starting_size - remaining_bytes, starting_size);
       return error;
     }
 
diff --git a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h
index 178318a2d6d4b..8f863956c21db 100644
--- a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h
+++ b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h
@@ -19,6 +19,7 @@
 #include <cstddef>
 #include <cstdint>
 #include <map>
+#include <utility>
 #include <variant>
 
 #include "lldb/Target/Process.h"
@@ -46,7 +47,8 @@ lldb_private::Status WriteString(const std::string &to_write,
 /// the data on heap.
 class MinidumpFileBuilder {
 public:
-  MinidumpFileBuilder(lldb::FileUP&& core_file): m_core_file(std::move(core_file)) {};
+  MinidumpFileBuilder(lldb::FileUP&& core_file, const lldb::ProcessSP &process_sp) 
+    : m_process_sp(std::move(process_sp)), m_core_file(std::move(core_file)) {};
 
   MinidumpFileBuilder(const MinidumpFileBuilder &) = delete;
   MinidumpFileBuilder &operator=(const MinidumpFileBuilder &) = delete;
@@ -56,31 +58,28 @@ class MinidumpFileBuilder {
 
   ~MinidumpFileBuilder() = default;
 
+  lldb_private::Status AddThreadList();
+  // Add Exception streams for any threads that stopped with exceptions.
+  void AddExceptions();
   lldb_private::Status
-  AddHeaderAndCalculateDirectories(const lldb_private::Target &target,
-                                   const lldb::ProcessSP &process_sp);
+  AddHeaderAndCalculateDirectories();
   // Add SystemInfo stream, used for storing the most basic information
   // about the system, platform etc...
-  lldb_private::Status AddSystemInfo(const llvm::Triple &target_triple);
+  lldb_private::Status AddSystemInfo();
   // Add ModuleList stream, containing information about all loaded modules
   // at the time of saving minidump.
-  lldb_private::Status AddModuleList(lldb_private::Target &target);
+  lldb_private::Status AddModuleList();
   // Add ThreadList stream, containing information about all threads running
   // at the moment of core saving. Contains information about thread
   // contexts.
   // Add MiscInfo stream, mainly providing ProcessId
-  void AddMiscInfo(const lldb::ProcessSP &process_sp);
+  void AddMiscInfo();
   // Add informative files about a Linux process
-  void AddLinuxFileStreams(const lldb::ProcessSP &process_sp);
-  // Add Exception streams for any threads that stopped with exceptions.
-  void AddExceptions(const lldb::ProcessSP &process_sp);
-  // Dump the prepared data into file. In case of the failure data are
-  // intact.
-  lldb_private::Status AddThreadList(const lldb::ProcessSP &process_sp);
+  void AddLinuxFileStreams();
 
-  lldb_private::Status AddMemory(const lldb::ProcessSP &process_sp,
-                                 lldb::SaveCoreStyle core_style);
+  lldb_private::Status AddMemory(lldb::SaveCoreStyle core_style);
 
+  // Run cleanup and write all remaining bytes to file
   lldb_private::Status DumpToFile();
 
 private:
@@ -89,11 +88,9 @@ class MinidumpFileBuilder {
   lldb_private::Status AddData(const void *data, size_t size);
   // Add MemoryList stream, containing dumps of important memory segments
   lldb_private::Status
-  AddMemoryList_64(const lldb::ProcessSP &process_sp,
-                   const lldb_private::Process::CoreFileMemoryRanges &ranges);
+  AddMemoryList_64(const lldb_private::Process::CoreFileMemoryRanges &ranges);
   lldb_private::Status
-  AddMemoryList_32(const lldb::ProcessSP &process_sp,
-                   const lldb_private::Process::CoreFileMemoryRanges &ranges);
+  AddMemoryList_32(const lldb_private::Process::CoreFileMemoryRanges &ranges);
   lldb_private::Status FixThreads();
   lldb_private::Status FlushToDisk();
 
@@ -103,7 +100,7 @@ class MinidumpFileBuilder {
   // Add directory of StreamType pointing to the current end of the prepared
   // file with the specified size.
   void AddDirectory(llvm::minidump::StreamType type, uint64_t stream_size);
-  lldb::addr_t GetCurrentDataEndOffset() const;
+  lldb::offset_t GetCurrentDataEndOffset() const;
   // Stores directories to fill in later
   std::vector<llvm::minidump::Directory> m_directories;
   // When we write off the threads for the first time, we need to clean them up
@@ -112,9 +109,11 @@ class MinidumpFileBuilder {
   // Main data buffer consisting of data without the minidump header and
   // directories
   lldb_private::DataBufferHeap m_data;
+  lldb::ProcessSP m_process_sp;
+
   uint m_expected_directories = 0;
   uint64_t m_saved_data_size = 0;
-  size_t m_thread_list_start = 0;
+  lldb::offset_t m_thread_list_start = 0;
   // We set the max write amount to 128 mb
   // Linux has a signed 32b - some buffer on writes
   // and we have guarauntee a user memory region / 'object' could be over 2gb
diff --git a/lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.cpp b/lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.cpp
index b490ff3e17ad3..c054077437613 100644
--- a/lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.cpp
+++ b/lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.cpp
@@ -72,37 +72,36 @@ bool ObjectFileMinidump::SaveCore(const lldb::ProcessSP &process_sp,
     error = maybe_core_file.takeError();
     return false;
   }
-  MinidumpFileBuilder builder(std::move(maybe_core_file.get()));
+  MinidumpFileBuilder builder(std::move(maybe_core_file.get()), process_sp);
 
   Target &target = process_sp->GetTarget();
-  builder.AddHeaderAndCalculateDirectories(target, process_sp);
-
+  builder.AddHeaderAndCalculateDirectories();
   Log *log = GetLog(LLDBLog::Object);
-  error = builder.AddSystemInfo(target.GetArchitecture().GetTriple());
+  error = builder.AddSystemInfo();
   if (error.Fail()) {
     LLDB_LOGF(log, "AddSystemInfo failed: %s", error.AsCString());
     return false;
   }
 
-  builder.AddModuleList(target);
-  builder.AddMiscInfo(process_sp);
+  builder.AddModuleList();
+  builder.AddMiscInfo();
 
-  error = builder.AddThreadList(process_sp);
+  error = builder.AddThreadList();
   if (error.Fail()) {
     LLDB_LOGF(log, "AddThreadList failed: %s", error.AsCString());
     return false;
   }
   if (target.GetArchitecture().GetTriple().getOS() ==
       llvm::Triple::OSType::Linux) {
-    builder.AddLinuxFileStreams(process_sp);
+    builder.AddLinuxFileStreams();
   }
 
   // Add any exceptions but only if there are any in any threads.
-  builder.AddExceptions(process_sp);
+  builder.AddExceptions();
 
   // Note: add memory HAS to be the last thing we do. It can overflow into 64b
   // land and many RVA's only support 32b
-  error = builder.AddMemory(process_sp, core_style);
+  error = builder.AddMemory(core_style);
   if (error.Fail()) {
     LLDB_LOGF(log, "AddMemoryList failed: %s", error.AsCString());
     return false;
diff --git a/llvm/lib/ObjectYAML/MinidumpEmitter.cpp b/llvm/lib/ObjectYAML/MinidumpEmitter.cpp
index 6ca626f7842d2..24b521a9925c7 100644
--- a/llvm/lib/ObjectYAML/MinidumpEmitter.cpp
+++ b/llvm/lib/ObjectYAML/MinidumpEmitter.cpp
@@ -6,11 +6,9 @@
 //
 //===----------------------------------------------------------------------===//
 
-#include "llvm/BinaryFormat/Minidump.h"
 #include "llvm/ObjectYAML/MinidumpYAML.h"
 #include "llvm/ObjectYAML/yaml2obj.h"
 #include "llvm/Support/ConvertUTF.h"
-#include "llvm/Support/Endian.h"
 #include "llvm/Support/raw_ostream.h"
 #include <optional>
 

>From b51cdbfddaed2de974746f89782c39c4b0addd01 Mon Sep 17 00:00:00 2001
From: Jacob Lalonde <jalalonde at fb.com>
Date: Thu, 13 Jun 2024 13:49:03 -0700
Subject: [PATCH 07/14] Try to fix MinidumpFilerbuilder.h to minimize changes,
 and fix the flush to disk loop.

---
 .../ObjectFile/Minidump/MinidumpFileBuilder.cpp    |  2 +-
 .../ObjectFile/Minidump/MinidumpFileBuilder.h      | 14 +++++++-------
 .../ObjectFile/Minidump/ObjectFileMinidump.cpp     |  2 +-
 3 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp
index f379f4d1d7898..0196ed191a01e 100644
--- a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp
+++ b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp
@@ -1096,7 +1096,7 @@ Status MinidumpFileBuilder::FlushToDisk() {
   offset_t offset = 0;
 
   while (remaining_bytes > 0) {
-    size_t bytes_written = 0;
+    size_t bytes_written = remaining_bytes;
     // We don't care how many bytes we wrote unless we got an error
     // so just decrement the remaining bytes.
     error = m_core_file->Write(m_data.GetBytes() + offset, bytes_written);
diff --git a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h
index 8f863956c21db..52f2af97be8b3 100644
--- a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h
+++ b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h
@@ -58,9 +58,6 @@ class MinidumpFileBuilder {
 
   ~MinidumpFileBuilder() = default;
 
-  lldb_private::Status AddThreadList();
-  // Add Exception streams for any threads that stopped with exceptions.
-  void AddExceptions();
   lldb_private::Status
   AddHeaderAndCalculateDirectories();
   // Add SystemInfo stream, used for storing the most basic information
@@ -72,6 +69,9 @@ class MinidumpFileBuilder {
   // Add ThreadList stream, containing information about all threads running
   // at the moment of core saving. Contains information about thread
   // contexts.
+  lldb_private::Status AddThreadList();
+  // Add Exception streams for any threads that stopped with exceptions.
+  void AddExceptions();
   // Add MiscInfo stream, mainly providing ProcessId
   void AddMiscInfo();
   // Add informative files about a Linux process
@@ -114,10 +114,10 @@ class MinidumpFileBuilder {
   uint m_expected_directories = 0;
   uint64_t m_saved_data_size = 0;
   lldb::offset_t m_thread_list_start = 0;
-  // We set the max write amount to 128 mb
-  // Linux has a signed 32b - some buffer on writes
-  // and we have guarauntee a user memory region / 'object' could be over 2gb
-  // now that we support 64b memory dumps.
+  // We set the max write amount to 128 mb, this is arbitrary
+  // but we want to try to keep the size of m_data small
+  // and we will only exceed a 128 mb buffer if we get a memory region
+  // that is larger than 128 mb.
   static constexpr size_t m_write_chunk_max = (1024 * 1024 * 128);
 
   static constexpr size_t header_size = sizeof(llvm::minidump::Header);
diff --git a/lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.cpp b/lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.cpp
index c054077437613..62fad1b3c4160 100644
--- a/lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.cpp
+++ b/lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.cpp
@@ -114,4 +114,4 @@ bool ObjectFileMinidump::SaveCore(const lldb::ProcessSP &process_sp,
   }
 
   return true;
-}
+} 

>From 53da9f58a9cccc7871af228d1f6b41ac42603600 Mon Sep 17 00:00:00 2001
From: Jacob Lalonde <jalalonde at fb.com>
Date: Thu, 13 Jun 2024 13:49:27 -0700
Subject: [PATCH 08/14] Run git clang format

---
 .../Minidump/MinidumpFileBuilder.cpp          | 63 +++++++++++--------
 .../ObjectFile/Minidump/MinidumpFileBuilder.h |  9 +--
 .../Minidump/ObjectFileMinidump.cpp           |  2 +-
 3 files changed, 42 insertions(+), 32 deletions(-)

diff --git a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp
index 0196ed191a01e..1360762c53a1e 100644
--- a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp
+++ b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp
@@ -24,7 +24,6 @@
 #include "lldb/Utility/DataExtractor.h"
 #include "lldb/Utility/LLDBLog.h"
 #include "lldb/Utility/Log.h"
-#include "lldb/Utility/LLDBLog.h"
 #include "lldb/Utility/RegisterValue.h"
 
 #include "llvm/ADT/StringRef.h"
@@ -60,7 +59,7 @@ Status MinidumpFileBuilder::AddHeaderAndCalculateDirectories() {
   // (corresponding memory list for stacks) And an additional memory list for
   // non-stacks.
 
-  lldb_private::Target& target = m_process_sp->GetTarget();
+  lldb_private::Target &target = m_process_sp->GetTarget();
   m_expected_directories = 6;
   // Check if OS is linux
   if (target.GetArchitecture().GetTriple().getOS() ==
@@ -129,7 +128,8 @@ void MinidumpFileBuilder::AddDirectory(StreamType type, uint64_t stream_size) {
 
 Status MinidumpFileBuilder::AddSystemInfo() {
   Status error;
-  const llvm::Triple &target_triple = m_process_sp->GetTarget().GetArchitecture().GetTriple();
+  const llvm::Triple &target_triple =
+      m_process_sp->GetTarget().GetArchitecture().GetTriple();
   AddDirectory(StreamType::SystemInfo, sizeof(llvm::minidump::SystemInfo));
 
   llvm::minidump::ProcessorArchitecture arch;
@@ -283,7 +283,7 @@ Status MinidumpFileBuilder::AddModuleList() {
   constexpr size_t minidump_module_size = sizeof(llvm::minidump::Module);
   Status error;
 
-  lldb_private::Target& target = m_process_sp->GetTarget();
+  lldb_private::Target &target = m_process_sp->GetTarget();
   const ModuleList &modules = target.GetImages();
   llvm::support::ulittle32_t modules_count =
       static_cast<llvm::support::ulittle32_t>(modules.GetSize());
@@ -315,10 +315,12 @@ Status MinidumpFileBuilder::AddModuleList() {
     auto maybe_mod_size = getModuleFileSize(target, mod);
     if (!maybe_mod_size) {
       llvm::Error mod_size_err = maybe_mod_size.takeError();
-      llvm::handleAllErrors(std::move(mod_size_err), [&](const llvm::ErrorInfoBase &E) {
-        error.SetErrorStringWithFormat("Unable to get the size of module %s: %s.",
-                                     module_name.c_str(), E.message().c_str());
-      });
+      llvm::handleAllErrors(std::move(mod_size_err),
+                            [&](const llvm::ErrorInfoBase &E) {
+                              error.SetErrorStringWithFormat(
+                                  "Unable to get the size of module %s: %s.",
+                                  module_name.c_str(), E.message().c_str());
+                            });
       return error;
     }
 
@@ -806,33 +808,37 @@ Status MinidumpFileBuilder::AddMemory(SaveCoreStyle core_style) {
   for (const auto &core_range : ranges_32)
     stack_start_addresses.insert(core_range.range.start());
 
-
-
-
-  uint64_t total_size = ranges_32.size() * sizeof(llvm::minidump::MemoryDescriptor);
+  uint64_t total_size =
+      ranges_32.size() * sizeof(llvm::minidump::MemoryDescriptor);
   for (const auto &core_range : ranges_32)
     total_size += core_range.range.size();
 
   if (total_size >= UINT32_MAX) {
-    error.SetErrorStringWithFormat("Unable to write minidump. Stack memory exceeds 32b limit. (Num Stacks %zu)", ranges_32.size());
+    error.SetErrorStringWithFormat("Unable to write minidump. Stack memory "
+                                   "exceeds 32b limit. (Num Stacks %zu)",
+                                   ranges_32.size());
     return error;
   }
 
   Process::CoreFileMemoryRanges all_core_memory_ranges;
   if (core_style != SaveCoreStyle::eSaveCoreStackOnly) {
-    error = m_process_sp->CalculateCoreFileSaveRanges(core_style, all_core_memory_ranges);
+    error = m_process_sp->CalculateCoreFileSaveRanges(core_style,
+                                                      all_core_memory_ranges);
     if (error.Fail())
       return error;
   }
 
   // We need to calculate the MemoryDescriptor size in the worst case
   // Where all memory descriptors are 64b. We also leave some additional padding
-  // So that we convert over to 64b with space to spare. This does not waste space in the dump
-  // But instead converts some memory from being in the memorylist_32 to the memorylist_64.
-  total_size += 256 + (ranges_64.size() - stack_start_addresses.size()) * sizeof(llvm::minidump::MemoryDescriptor_64);
+  // So that we convert over to 64b with space to spare. This does not waste
+  // space in the dump But instead converts some memory from being in the
+  // memorylist_32 to the memorylist_64.
+  total_size += 256 + (ranges_64.size() - stack_start_addresses.size()) *
+                          sizeof(llvm::minidump::MemoryDescriptor_64);
 
   for (const auto &core_range : all_core_memory_ranges) {
-    addr_t size_to_add = core_range.range.size() + sizeof(llvm::minidump::MemoryDescriptor);
+    addr_t size_to_add =
+        core_range.range.size() + sizeof(llvm::minidump::MemoryDescriptor);
     if (stack_start_addresses.count(core_range.range.start()) > 0) {
       // Don't double save stacks.
       continue;
@@ -887,8 +893,8 @@ Status MinidumpFileBuilder::DumpHeader() const {
   if (error.Fail() || bytes_written != header_size) {
     if (bytes_written != header_size)
       error.SetErrorStringWithFormat(
-          "Unable to write the minidump header (written %zd/%zd)", bytes_written,
-          header_size);
+          "Unable to write the minidump header (written %zd/%zd)",
+          bytes_written, header_size);
     return error;
   }
   return error;
@@ -917,7 +923,8 @@ Status MinidumpFileBuilder::DumpDirectories() const {
   return error;
 }
 
-Status MinidumpFileBuilder::AddMemoryList_32(const Process::CoreFileMemoryRanges &ranges) {
+Status MinidumpFileBuilder::AddMemoryList_32(
+    const Process::CoreFileMemoryRanges &ranges) {
   std::vector<MemoryDescriptor> descriptors;
   Status error;
   Log *log = GetLog(LLDBLog::Object);
@@ -929,10 +936,10 @@ Status MinidumpFileBuilder::AddMemoryList_32(const Process::CoreFileMemoryRanges
     const addr_t size = core_range.range.size();
     auto data_up = std::make_unique<DataBufferHeap>(size, 0);
 
-    LLDB_LOGF(
-        log,
-        "/AddMemoryList/AddMemory/ %zu/%zu reading memory for region (%zu bytes) [%zx, %zx)",
-        region_index, ranges.size(), size, addr, addr + size);
+    LLDB_LOGF(log,
+              "/AddMemoryList/AddMemory/ %zu/%zu reading memory for region "
+              "(%zu bytes) [%zx, %zx)",
+              region_index, ranges.size(), size, addr, addr + size);
     ++region_index;
 
     const size_t bytes_read =
@@ -984,7 +991,8 @@ Status MinidumpFileBuilder::AddMemoryList_32(const Process::CoreFileMemoryRanges
   return error;
 }
 
-Status MinidumpFileBuilder::AddMemoryList_64(const Process::CoreFileMemoryRanges &ranges) {
+Status MinidumpFileBuilder::AddMemoryList_64(
+    const Process::CoreFileMemoryRanges &ranges) {
   AddDirectory(StreamType::Memory64List,
                (sizeof(llvm::support::ulittle64_t) * 2) +
                    ranges.size() * sizeof(llvm::minidump::MemoryDescriptor_64));
@@ -1024,7 +1032,8 @@ Status MinidumpFileBuilder::AddMemoryList_64(const Process::CoreFileMemoryRanges
     auto data_up = std::make_unique<DataBufferHeap>(size, 0);
 
     LLDB_LOGF(log,
-              "/AddMemoryList_64/AddMemory %zu/%zu reading memory for region (%zu bytes) "
+              "/AddMemoryList_64/AddMemory %zu/%zu reading memory for region "
+              "(%zu bytes) "
               "[%zx, %zx)",
               region_index, ranges.size(), size, addr, addr + size);
     ++region_index;
diff --git a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h
index 52f2af97be8b3..d5d2782a9c6c2 100644
--- a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h
+++ b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h
@@ -47,8 +47,10 @@ lldb_private::Status WriteString(const std::string &to_write,
 /// the data on heap.
 class MinidumpFileBuilder {
 public:
-  MinidumpFileBuilder(lldb::FileUP&& core_file, const lldb::ProcessSP &process_sp) 
-    : m_process_sp(std::move(process_sp)), m_core_file(std::move(core_file)) {};
+  MinidumpFileBuilder(lldb::FileUP &&core_file,
+                      const lldb::ProcessSP &process_sp)
+      : m_process_sp(std::move(process_sp)),
+        m_core_file(std::move(core_file)){};
 
   MinidumpFileBuilder(const MinidumpFileBuilder &) = delete;
   MinidumpFileBuilder &operator=(const MinidumpFileBuilder &) = delete;
@@ -58,8 +60,7 @@ class MinidumpFileBuilder {
 
   ~MinidumpFileBuilder() = default;
 
-  lldb_private::Status
-  AddHeaderAndCalculateDirectories();
+  lldb_private::Status AddHeaderAndCalculateDirectories();
   // Add SystemInfo stream, used for storing the most basic information
   // about the system, platform etc...
   lldb_private::Status AddSystemInfo();
diff --git a/lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.cpp b/lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.cpp
index 62fad1b3c4160..c054077437613 100644
--- a/lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.cpp
+++ b/lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.cpp
@@ -114,4 +114,4 @@ bool ObjectFileMinidump::SaveCore(const lldb::ProcessSP &process_sp,
   }
 
   return true;
-} 
+}

>From e3841d81cdcfecb53630af40e5f77de15c846d3e Mon Sep 17 00:00:00 2001
From: Jacob Lalonde <jalalonde at fb.com>
Date: Thu, 13 Jun 2024 15:45:46 -0700
Subject: [PATCH 09/14] Linting, and add some sorting so that we can determine
 the largest possible buffer ahead of time

---
 .../Minidump/MinidumpFileBuilder.cpp          | 28 ++++++++++++++-----
 .../ObjectFile/Minidump/MinidumpFileBuilder.h | 11 ++++----
 .../Minidump/ObjectFileMinidump.cpp           |  2 +-
 3 files changed, 27 insertions(+), 14 deletions(-)

diff --git a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp
index 1360762c53a1e..ed85ad72e096a 100644
--- a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp
+++ b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp
@@ -43,6 +43,7 @@
 #include <climits>
 #include <cstddef>
 #include <cstdint>
+#include <functional>
 #include <iostream>
 #include <set>
 #include <utility>
@@ -828,6 +829,9 @@ Status MinidumpFileBuilder::AddMemory(SaveCoreStyle core_style) {
       return error;
   }
 
+  // Sort the ranges so we try to fit all the small ranges in 32b.
+  std::sort(all_core_memory_ranges.begin(), all_core_memory_ranges.end());
+
   // We need to calculate the MemoryDescriptor size in the worst case
   // Where all memory descriptors are 64b. We also leave some additional padding
   // So that we convert over to 64b with space to spare. This does not waste
@@ -924,20 +928,24 @@ Status MinidumpFileBuilder::DumpDirectories() const {
 }
 
 Status MinidumpFileBuilder::AddMemoryList_32(
-    const Process::CoreFileMemoryRanges &ranges) {
+    Process::CoreFileMemoryRanges &ranges) {
   std::vector<MemoryDescriptor> descriptors;
   Status error;
+  if (ranges.size() == 0)
+    return error;
+
+  std::sort(ranges.begin(), ranges.end());
   Log *log = GetLog(LLDBLog::Object);
   size_t region_index = 0;
+  auto data_up = std::make_unique<DataBufferHeap>(ranges.back().range.size(), 0);
   for (const auto &core_range : ranges) {
     // Take the offset before we write.
     const size_t offset_for_data = GetCurrentDataEndOffset();
     const addr_t addr = core_range.range.start();
     const addr_t size = core_range.range.size();
-    auto data_up = std::make_unique<DataBufferHeap>(size, 0);
 
     LLDB_LOGF(log,
-              "/AddMemoryList/AddMemory/ %zu/%zu reading memory for region "
+              "AddMemoryList %zu/%zu reading memory for region "
               "(%zu bytes) [%zx, %zx)",
               region_index, ranges.size(), size, addr, addr + size);
     ++region_index;
@@ -992,7 +1000,13 @@ Status MinidumpFileBuilder::AddMemoryList_32(
 }
 
 Status MinidumpFileBuilder::AddMemoryList_64(
-    const Process::CoreFileMemoryRanges &ranges) {
+    Process::CoreFileMemoryRanges &ranges) {
+  Status error;
+  if (ranges.empty()) 
+    return error;
+
+  // Sort the ranges so we can preallocate a buffer big enough for the largest item.
+  std::sort(ranges.begin(), ranges.end());
   AddDirectory(StreamType::Memory64List,
                (sizeof(llvm::support::ulittle64_t) * 2) +
                    ranges.size() * sizeof(llvm::minidump::MemoryDescriptor_64));
@@ -1023,16 +1037,16 @@ Status MinidumpFileBuilder::AddMemoryList_64(
     m_data.AppendData(&memory_desc, sizeof(MemoryDescriptor_64));
   }
 
-  Status error;
+
   Log *log = GetLog(LLDBLog::Object);
   size_t region_index = 0;
+  auto data_up = std::make_unique<DataBufferHeap>(ranges.back().range.size(), 0);
   for (const auto &core_range : ranges) {
     const addr_t addr = core_range.range.start();
     const addr_t size = core_range.range.size();
-    auto data_up = std::make_unique<DataBufferHeap>(size, 0);
 
     LLDB_LOGF(log,
-              "/AddMemoryList_64/AddMemory %zu/%zu reading memory for region "
+              "AddMemoryList_64 %zu/%zu reading memory for region "
               "(%zu bytes) "
               "[%zx, %zx)",
               region_index, ranges.size(), size, addr, addr + size);
diff --git a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h
index d5d2782a9c6c2..2ae966fb6bf38 100644
--- a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h
+++ b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h
@@ -49,8 +49,8 @@ class MinidumpFileBuilder {
 public:
   MinidumpFileBuilder(lldb::FileUP &&core_file,
                       const lldb::ProcessSP &process_sp)
-      : m_process_sp(std::move(process_sp)),
-        m_core_file(std::move(core_file)){};
+      : m_process_sp(process_sp),
+        m_core_file(std::move(core_file)) {};
 
   MinidumpFileBuilder(const MinidumpFileBuilder &) = delete;
   MinidumpFileBuilder &operator=(const MinidumpFileBuilder &) = delete;
@@ -73,7 +73,7 @@ class MinidumpFileBuilder {
   lldb_private::Status AddThreadList();
   // Add Exception streams for any threads that stopped with exceptions.
   void AddExceptions();
-  // Add MiscInfo stream, mainly providing ProcessId
+  // Add MiscInfo stream, mainly providing ProcessId  8
   void AddMiscInfo();
   // Add informative files about a Linux process
   void AddLinuxFileStreams();
@@ -89,15 +89,14 @@ class MinidumpFileBuilder {
   lldb_private::Status AddData(const void *data, size_t size);
   // Add MemoryList stream, containing dumps of important memory segments
   lldb_private::Status
-  AddMemoryList_64(const lldb_private::Process::CoreFileMemoryRanges &ranges);
+  AddMemoryList_64(lldb_private::Process::CoreFileMemoryRanges &ranges);
   lldb_private::Status
-  AddMemoryList_32(const lldb_private::Process::CoreFileMemoryRanges &ranges);
+  AddMemoryList_32(lldb_private::Process::CoreFileMemoryRanges &ranges);
   lldb_private::Status FixThreads();
   lldb_private::Status FlushToDisk();
 
   lldb_private::Status DumpHeader() const;
   lldb_private::Status DumpDirectories() const;
-  bool CheckIf_64Bit(const size_t size);
   // Add directory of StreamType pointing to the current end of the prepared
   // file with the specified size.
   void AddDirectory(llvm::minidump::StreamType type, uint64_t stream_size);
diff --git a/lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.cpp b/lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.cpp
index c054077437613..66fdda5313229 100644
--- a/lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.cpp
+++ b/lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.cpp
@@ -103,7 +103,7 @@ bool ObjectFileMinidump::SaveCore(const lldb::ProcessSP &process_sp,
   // land and many RVA's only support 32b
   error = builder.AddMemory(core_style);
   if (error.Fail()) {
-    LLDB_LOGF(log, "AddMemoryList failed: %s", error.AsCString());
+    LLDB_LOGF(log, "AddMemory failed: %s", error.AsCString());
     return false;
   }
 

>From 5e375eaca4c31167edb531be7fd94e00c311923f Mon Sep 17 00:00:00 2001
From: Jacob Lalonde <jalalonde at fb.com>
Date: Fri, 14 Jun 2024 17:03:44 -0700
Subject: [PATCH 10/14] Implement fut further feedback and safety checks. Fix
 bug with 64b being off by 8 + descirptor count * sizeof(descriptor_64)

---
 .../Minidump/MinidumpFileBuilder.cpp          | 177 +++++++++++-------
 .../ObjectFile/Minidump/MinidumpFileBuilder.h |  23 ++-
 .../Minidump/ObjectFileMinidump.cpp           |  42 +++--
 3 files changed, 151 insertions(+), 91 deletions(-)

diff --git a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp
index ed85ad72e096a..00e47b86eae59 100644
--- a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp
+++ b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp
@@ -24,6 +24,7 @@
 #include "lldb/Utility/DataExtractor.h"
 #include "lldb/Utility/LLDBLog.h"
 #include "lldb/Utility/Log.h"
+#include "lldb/Utility/RangeMap.h"
 #include "lldb/Utility/RegisterValue.h"
 
 #include "llvm/ADT/StringRef.h"
@@ -59,10 +60,9 @@ Status MinidumpFileBuilder::AddHeaderAndCalculateDirectories() {
   // We know we will have at least Misc, SystemInfo, Modules, and ThreadList
   // (corresponding memory list for stacks) And an additional memory list for
   // non-stacks.
-
   lldb_private::Target &target = m_process_sp->GetTarget();
   m_expected_directories = 6;
-  // Check if OS is linux
+  // Check if OS is linux and reserve directory space for all linux specific breakpad extension directories.
   if (target.GetArchitecture().GetTriple().getOS() ==
       llvm::Triple::OSType::Linux)
     m_expected_directories += 9;
@@ -82,38 +82,42 @@ Status MinidumpFileBuilder::AddHeaderAndCalculateDirectories() {
   // Now offset the file by the directores so we can write them in later.
   offset_t directory_offset = m_expected_directories * directory_size;
   m_saved_data_size += directory_offset;
-  // Replace this when we make a better way to do this.
   Status error;
-  Header empty_header;
-  size_t bytes_written;
-  bytes_written = header_size;
-  error = m_core_file->Write(&empty_header, bytes_written);
-  if (error.Fail() || bytes_written != header_size) {
-    if (bytes_written != header_size)
+  size_t zeroes = 0; // 8 0's
+  size_t remaining_bytes = m_saved_data_size;
+  while (remaining_bytes > 0) {
+    // Keep filling in zero's until we preallocate enough space for the header
+    // and directory sections.
+    size_t bytes_written = std::min(remaining_bytes, sizeof(size_t));
+    error = m_core_file->Write(&zeroes, bytes_written);
+    if (error.Fail()) {
       error.SetErrorStringWithFormat(
-          "unable to write the header (written %zd/%zd)", bytes_written,
-          header_size);
-    return error;
-  }
-
-  for (uint i = 0; i < m_expected_directories; i++) {
-    size_t bytes_written;
-    bytes_written = directory_size;
-    Directory empty_directory;
-    error = m_core_file->Write(&empty_directory, bytes_written);
-    if (error.Fail() || bytes_written != directory_size) {
-      if (bytes_written != directory_size)
-        error.SetErrorStringWithFormat(
-            "unable to write the directory (written %zd/%zd)", bytes_written,
-            directory_size);
-      return error;
+        "Unable to write header and directory padding (written %zd/%zd)",
+        remaining_bytes - m_saved_data_size, m_saved_data_size);
+      break;
     }
+
+    remaining_bytes -= bytes_written;
   }
 
   return error;
 }
 
-void MinidumpFileBuilder::AddDirectory(StreamType type, uint64_t stream_size) {
+Status MinidumpFileBuilder::AddDirectory(StreamType type, uint64_t stream_size) {
+  Status error;
+  if (GetCurrentDataEndOffset() > UINT32_MAX) {
+    error.SetErrorStringWithFormat(
+        "Unable to add directory for stream type %x, offset is greater then 32 bit limit.", type);
+    return error;
+  }
+
+  if (m_directories.size() + 1 > m_expected_directories) {
+    error.SetErrorStringWithFormat(
+        "Unable to add directory for stream type %x, exceeded expected number of directories %d.",
+        type, m_expected_directories);
+    return error;
+  }
+
   LocationDescriptor loc;
   loc.DataSize = static_cast<llvm::support::ulittle32_t>(stream_size);
   // Stream will begin at the current end of data section
@@ -124,14 +128,16 @@ void MinidumpFileBuilder::AddDirectory(StreamType type, uint64_t stream_size) {
   dir.Location = loc;
 
   m_directories.push_back(dir);
-  assert(m_expected_directories >= m_directories.size());
+  return error;
 }
 
 Status MinidumpFileBuilder::AddSystemInfo() {
   Status error;
   const llvm::Triple &target_triple =
       m_process_sp->GetTarget().GetArchitecture().GetTriple();
-  AddDirectory(StreamType::SystemInfo, sizeof(llvm::minidump::SystemInfo));
+  error = AddDirectory(StreamType::SystemInfo, sizeof(llvm::minidump::SystemInfo));
+  if (error.Fail())
+    return error;
 
   llvm::minidump::ProcessorArchitecture arch;
   switch (target_triple.getArch()) {
@@ -301,7 +307,9 @@ Status MinidumpFileBuilder::AddModuleList() {
       sizeof(llvm::support::ulittle32_t) + modules_count * minidump_module_size;
 
   // Adding directory describing this stream.
-  AddDirectory(StreamType::ModuleList, module_stream_size);
+  error = AddDirectory(StreamType::ModuleList, module_stream_size);
+  if (error.Fail())
+    return error;
 
   m_data.AppendData(&modules_count, sizeof(llvm::support::ulittle32_t));
 
@@ -561,12 +569,12 @@ class ArchThreadContexts {
 Status MinidumpFileBuilder::FixThreads() {
   Status error;
   // If we have anything in the heap flush it.
-  FlushToDisk();
+  FlushBufferToDisk();
   m_core_file->SeekFromStart(m_thread_list_start);
-  for (auto &pair : m_thread_by_range_start) {
+  for (auto &pair : m_thread_by_range_end) {
     // The thread objects will get a new memory descriptor added
     // When we are emitting the memory list and then we write it here
-    llvm::minidump::Thread thread = pair.second;
+    const llvm::minidump::Thread &thread = pair.second;
     size_t bytes_to_write = sizeof(llvm::minidump::Thread);
     size_t bytes_written = bytes_to_write;
     error = m_core_file->Write(&thread, bytes_written);
@@ -591,8 +599,10 @@ Status MinidumpFileBuilder::AddThreadList() {
                               thread_list.GetSize() * minidump_thread_size;
   // save for the ability to set up RVA
   size_t size_before = GetCurrentDataEndOffset();
-
-  AddDirectory(StreamType::ThreadList, thread_stream_size);
+  Status error;
+  error = AddDirectory(StreamType::ThreadList, thread_stream_size);
+  if (error.Fail())
+    return error;
 
   llvm::support::ulittle32_t thread_count =
       static_cast<llvm::support::ulittle32_t>(thread_list.GetSize());
@@ -607,7 +617,6 @@ Status MinidumpFileBuilder::AddThreadList() {
   for (uint32_t thread_idx = 0; thread_idx < num_threads; ++thread_idx) {
     ThreadSP thread_sp(thread_list.GetThreadAtIndex(thread_idx));
     RegisterContextSP reg_ctx_sp(thread_sp->GetRegisterContext());
-    Status error;
 
     if (!reg_ctx_sp) {
       error.SetErrorString("Unable to get the register context.");
@@ -657,7 +666,7 @@ Status MinidumpFileBuilder::AddThreadList() {
     t.Stack = stack, t.Context = thread_context_memory_locator;
 
     // We save off the stack object so we can circle back and clean it up.
-    m_thread_by_range_start[sp_region.GetRange().GetRangeBase()] = t;
+    m_thread_by_range_end[sp_region.GetRange().GetRangeEnd()] = t;
     m_data.AppendData(&t, sizeof(llvm::minidump::Thread));
   }
 
@@ -667,9 +676,9 @@ Status MinidumpFileBuilder::AddThreadList() {
   return Status();
 }
 
-void MinidumpFileBuilder::AddExceptions() {
+Status MinidumpFileBuilder::AddExceptions() {
   lldb_private::ThreadList thread_list = m_process_sp->GetThreadList();
-
+  Status error;
   const uint32_t num_threads = thread_list.GetSize();
   for (uint32_t thread_idx = 0; thread_idx < num_threads; ++thread_idx) {
     ThreadSP thread_sp(thread_list.GetThreadAtIndex(thread_idx));
@@ -688,7 +697,10 @@ void MinidumpFileBuilder::AddExceptions() {
     if (add_exception) {
       constexpr size_t minidump_exception_size =
           sizeof(llvm::minidump::ExceptionStream);
-      AddDirectory(StreamType::Exception, minidump_exception_size);
+      error = AddDirectory(StreamType::Exception, minidump_exception_size);
+      if (error.Fail())
+        return error;
+
       StopInfoSP stop_info_sp = thread_sp->GetStopInfo();
       RegisterContextSP reg_ctx_sp(thread_sp->GetRegisterContext());
       Exception exp_record = {};
@@ -716,11 +728,16 @@ void MinidumpFileBuilder::AddExceptions() {
       m_data.AppendData(&exp_stream, minidump_exception_size);
     }
   }
+
+  return error;
 }
 
-void MinidumpFileBuilder::AddMiscInfo() {
-  AddDirectory(StreamType::MiscInfo,
+lldb_private::Status MinidumpFileBuilder::AddMiscInfo() {
+  Status error;
+  error = AddDirectory(StreamType::MiscInfo,
                sizeof(lldb_private::minidump::MinidumpMiscInfo));
+  if (error.Fail())
+    return error;
 
   lldb_private::minidump::MinidumpMiscInfo misc_info;
   misc_info.size = static_cast<llvm::support::ulittle32_t>(
@@ -742,6 +759,7 @@ void MinidumpFileBuilder::AddMiscInfo() {
 
   m_data.AppendData(&misc_info,
                     sizeof(lldb_private::minidump::MinidumpMiscInfo));
+  return error;
 }
 
 std::unique_ptr<llvm::MemoryBuffer>
@@ -752,7 +770,12 @@ getFileStreamHelper(const std::string &path) {
   return std::move(maybe_stream.get());
 }
 
-void MinidumpFileBuilder::AddLinuxFileStreams() {
+Status MinidumpFileBuilder::AddLinuxFileStreams() {
+  Status error;
+  // No-op if we are not on linux.
+  if (m_process_sp->GetTarget().GetArchitecture().GetTriple().getOS() != llvm::Triple::Linux)
+    return error;
+
   std::vector<std::pair<StreamType, std::string>> files_with_stream_types = {
       {StreamType::LinuxCPUInfo, "/proc/cpuinfo"},
       {StreamType::LinuxLSBRelease, "/etc/lsb-release"},
@@ -779,7 +802,7 @@ void MinidumpFileBuilder::AddLinuxFileStreams() {
         {StreamType::LinuxProcFD, "/proc/" + pid_str + "/fd"});
   }
 
-  Status error;
+
   for (const auto &entry : files_with_stream_types) {
     StreamType stream = entry.first;
     std::string path = entry.second;
@@ -789,13 +812,17 @@ void MinidumpFileBuilder::AddLinuxFileStreams() {
       size_t size = memory_buffer->getBufferSize();
       if (size == 0)
         continue;
-      AddDirectory(stream, size);
+      error = AddDirectory(stream, size);
+      if (error.Fail())
+        return error;
       m_data.AppendData(memory_buffer->getBufferStart(), size);
     }
   }
+
+  return error;
 }
 
-Status MinidumpFileBuilder::AddMemory(SaveCoreStyle core_style) {
+Status MinidumpFileBuilder::AddMemoryList(SaveCoreStyle core_style) {
   Status error;
 
   Process::CoreFileMemoryRanges ranges_32;
@@ -829,24 +856,23 @@ Status MinidumpFileBuilder::AddMemory(SaveCoreStyle core_style) {
       return error;
   }
 
-  // Sort the ranges so we try to fit all the small ranges in 32b.
-  std::sort(all_core_memory_ranges.begin(), all_core_memory_ranges.end());
 
   // We need to calculate the MemoryDescriptor size in the worst case
   // Where all memory descriptors are 64b. We also leave some additional padding
   // So that we convert over to 64b with space to spare. This does not waste
   // space in the dump But instead converts some memory from being in the
   // memorylist_32 to the memorylist_64.
-  total_size += 256 + (ranges_64.size() - stack_start_addresses.size()) *
+  total_size += 256 + (all_core_memory_ranges.size() - stack_start_addresses.size()) *
                           sizeof(llvm::minidump::MemoryDescriptor_64);
 
   for (const auto &core_range : all_core_memory_ranges) {
     addr_t size_to_add =
         core_range.range.size() + sizeof(llvm::minidump::MemoryDescriptor);
-    if (stack_start_addresses.count(core_range.range.start()) > 0) {
+    if (stack_start_addresses.count(core_range.range.start()) > 0) 
       // Don't double save stacks.
       continue;
-    } else if (total_size + size_to_add < UINT32_MAX) {
+
+    if (total_size + size_to_add < UINT32_MAX) {
       ranges_32.push_back(core_range);
       total_size += core_range.range.size();
       total_size += sizeof(llvm::minidump::MemoryDescriptor);
@@ -927,22 +953,30 @@ Status MinidumpFileBuilder::DumpDirectories() const {
   return error;
 }
 
+static size_t GetLargestRange(const Process::CoreFileMemoryRanges &ranges) {
+  size_t max_size = 0;
+  for (const auto &core_range : ranges) {
+    max_size = std::max(max_size, core_range.range.size());
+  }
+  return max_size;
+}
+
 Status MinidumpFileBuilder::AddMemoryList_32(
-    Process::CoreFileMemoryRanges &ranges) {
+  Process::CoreFileMemoryRanges &ranges) {
   std::vector<MemoryDescriptor> descriptors;
   Status error;
   if (ranges.size() == 0)
     return error;
 
-  std::sort(ranges.begin(), ranges.end());
   Log *log = GetLog(LLDBLog::Object);
   size_t region_index = 0;
-  auto data_up = std::make_unique<DataBufferHeap>(ranges.back().range.size(), 0);
+  auto data_up = std::make_unique<DataBufferHeap>(GetLargestRange(ranges), 0);
   for (const auto &core_range : ranges) {
     // Take the offset before we write.
     const size_t offset_for_data = GetCurrentDataEndOffset();
     const addr_t addr = core_range.range.start();
     const addr_t size = core_range.range.size();
+    const addr_t end = core_range.range.end();
 
     LLDB_LOGF(log,
               "AddMemoryList %zu/%zu reading memory for region "
@@ -970,9 +1004,8 @@ Status MinidumpFileBuilder::AddMemoryList_32(
     descriptor.Memory.RVA =
         static_cast<llvm::support::ulittle32_t>(offset_for_data);
     descriptors.push_back(descriptor);
-    if (m_thread_by_range_start.count(addr) > 0) {
-      m_thread_by_range_start[addr].Stack = descriptor;
-    }
+    if (m_thread_by_range_end.count(end) > 0)
+      m_thread_by_range_end[end].Stack = descriptor;
 
     // Add the data to the buffer, flush as needed.
     error = AddData(data_up->GetBytes(), bytes_read);
@@ -983,10 +1016,12 @@ Status MinidumpFileBuilder::AddMemoryList_32(
   // Add a directory that references this list
   // With a size of the number of ranges as a 32 bit num
   // And then the size of all the ranges
-  AddDirectory(StreamType::MemoryList,
+  error = AddDirectory(StreamType::MemoryList,
                sizeof(llvm::support::ulittle32_t) +
                    descriptors.size() *
                        sizeof(llvm::minidump::MemoryDescriptor));
+  if (error.Fail())
+    return error;
 
   llvm::support::ulittle32_t memory_ranges_num =
       static_cast<llvm::support::ulittle32_t>(descriptors.size());
@@ -1005,21 +1040,25 @@ Status MinidumpFileBuilder::AddMemoryList_64(
   if (ranges.empty()) 
     return error;
 
-  // Sort the ranges so we can preallocate a buffer big enough for the largest item.
-  std::sort(ranges.begin(), ranges.end());
-  AddDirectory(StreamType::Memory64List,
+  error = AddDirectory(StreamType::Memory64List,
                (sizeof(llvm::support::ulittle64_t) * 2) +
                    ranges.size() * sizeof(llvm::minidump::MemoryDescriptor_64));
+  if (error.Fail())
+    return error;
 
   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));
+  // 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);
+  // The base_rva needs to start after the directories, which is right after
+  // this 8 byte variable.
+  offset_t base_rva = starting_offset + (ranges.size() * sizeof(llvm::minidump::MemoryDescriptor_64));
   llvm::support::ulittle64_t memory_ranges_base_rva =
-      static_cast<llvm::support::ulittle64_t>(GetCurrentDataEndOffset());
+      static_cast<llvm::support::ulittle64_t>(base_rva);
   m_data.AppendData(&memory_ranges_base_rva,
                     sizeof(llvm::support::ulittle64_t));
-  // Capture the starting offset, so we can do cleanup later if needed.
-  uint64_t starting_offset = GetCurrentDataEndOffset();
 
   bool cleanup_required = false;
   std::vector<MemoryDescriptor_64> descriptors;
@@ -1040,7 +1079,7 @@ Status MinidumpFileBuilder::AddMemoryList_64(
 
   Log *log = GetLog(LLDBLog::Object);
   size_t region_index = 0;
-  auto data_up = std::make_unique<DataBufferHeap>(ranges.back().range.size(), 0);
+  auto data_up = std::make_unique<DataBufferHeap>(GetLargestRange(ranges), 0);
   for (const auto &core_range : ranges) {
     const addr_t addr = core_range.range.start();
     const addr_t size = core_range.range.size();
@@ -1079,7 +1118,7 @@ Status MinidumpFileBuilder::AddMemoryList_64(
     return error;
   } else {
     // Flush to disk we can make the fixes in place.
-    FlushToDisk();
+    FlushBufferToDisk();
     // Fixup the descriptors that were not read correctly.
     m_core_file->SeekFromStart(starting_offset);
     size_t bytes_written = sizeof(MemoryDescriptor_64) * descriptors.size();
@@ -1104,13 +1143,13 @@ Status MinidumpFileBuilder::AddData(const void *data, addr_t size) {
   // time.
   m_data.AppendData(data, size);
   if (m_data.GetByteSize() > m_write_chunk_max) {
-    return FlushToDisk();
+    return FlushBufferToDisk();
   }
 
   return Status();
 }
 
-Status MinidumpFileBuilder::FlushToDisk() {
+Status MinidumpFileBuilder::FlushBufferToDisk() {
   Status error;
   // Set the stream to it's end.
   m_core_file->SeekFromEnd(0);
@@ -1139,10 +1178,10 @@ Status MinidumpFileBuilder::FlushToDisk() {
   return error;
 }
 
-Status MinidumpFileBuilder::DumpToFile() {
+Status MinidumpFileBuilder::DumpFile() {
   Status error;
   // If anything is left unsaved, dump it.
-  error = FlushToDisk();
+  error = FlushBufferToDisk();
   if (error.Fail())
     return error;
 
diff --git a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h
index 2ae966fb6bf38..31a587813b43e 100644
--- a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h
+++ b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h
@@ -19,6 +19,7 @@
 #include <cstddef>
 #include <cstdint>
 #include <map>
+#include <unordered_map>
 #include <utility>
 #include <variant>
 
@@ -72,16 +73,16 @@ class MinidumpFileBuilder {
   // contexts.
   lldb_private::Status AddThreadList();
   // Add Exception streams for any threads that stopped with exceptions.
-  void AddExceptions();
-  // Add MiscInfo stream, mainly providing ProcessId  8
-  void AddMiscInfo();
+  lldb_private::Status AddExceptions();
+  // Add MemoryList stream, containing dumps of important memory segments
+  lldb_private::Status AddMemoryList(lldb::SaveCoreStyle core_style);
+  // Add MiscInfo stream, mainly providing ProcessId
+  lldb_private::Status AddMiscInfo();
   // Add informative files about a Linux process
-  void AddLinuxFileStreams();
-
-  lldb_private::Status AddMemory(lldb::SaveCoreStyle core_style);
+  lldb_private::Status AddLinuxFileStreams();
 
   // Run cleanup and write all remaining bytes to file
-  lldb_private::Status DumpToFile();
+  lldb_private::Status DumpFile();
 
 private:
   // Add data to the end of the buffer, if the buffer exceeds the flush level,
@@ -93,19 +94,21 @@ class MinidumpFileBuilder {
   lldb_private::Status
   AddMemoryList_32(lldb_private::Process::CoreFileMemoryRanges &ranges);
   lldb_private::Status FixThreads();
-  lldb_private::Status FlushToDisk();
+  lldb_private::Status FlushBufferToDisk();
 
   lldb_private::Status DumpHeader() const;
   lldb_private::Status DumpDirectories() const;
   // Add directory of StreamType pointing to the current end of the prepared
   // file with the specified size.
-  void AddDirectory(llvm::minidump::StreamType type, uint64_t stream_size);
+  lldb_private::Status AddDirectory(llvm::minidump::StreamType type, uint64_t stream_size);
   lldb::offset_t GetCurrentDataEndOffset() const;
   // Stores directories to fill in later
   std::vector<llvm::minidump::Directory> m_directories;
   // When we write off the threads for the first time, we need to clean them up
   // and give them the correct RVA once we write the stack memory list.
-  std::map<lldb::addr_t, llvm::minidump::Thread> m_thread_by_range_start;
+  // We save by the end because we only take from the stack pointer up
+  // So the saved off range base can differ from the memory region the stack pointer is in.
+  std::unordered_map<lldb::addr_t, llvm::minidump::Thread> m_thread_by_range_end;
   // Main data buffer consisting of data without the minidump header and
   // directories
   lldb_private::DataBufferHeap m_data;
diff --git a/lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.cpp b/lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.cpp
index 66fdda5313229..5174d73e4f579 100644
--- a/lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.cpp
+++ b/lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.cpp
@@ -15,6 +15,7 @@
 #include "lldb/Core/Section.h"
 #include "lldb/Target/Process.h"
 #include "lldb/Utility/LLDBLog.h"
+#include "lldb/Utility/Log.h"
 
 #include "llvm/Support/FileSystem.h"
 #include <unistd.h>
@@ -74,42 +75,59 @@ bool ObjectFileMinidump::SaveCore(const lldb::ProcessSP &process_sp,
   }
   MinidumpFileBuilder builder(std::move(maybe_core_file.get()), process_sp);
 
-  Target &target = process_sp->GetTarget();
-  builder.AddHeaderAndCalculateDirectories();
   Log *log = GetLog(LLDBLog::Object);
+  error = builder.AddHeaderAndCalculateDirectories();
+  if (error.Fail()) {
+    LLDB_LOGF(log, "AddHeaderAndCalculateDirectories failed: %s", error.AsCString());
+    return false;
+  };
   error = builder.AddSystemInfo();
   if (error.Fail()) {
     LLDB_LOGF(log, "AddSystemInfo failed: %s", error.AsCString());
     return false;
   }
 
-  builder.AddModuleList();
-  builder.AddMiscInfo();
+  error = builder.AddModuleList();
+  if (error.Fail()) {
+    LLDB_LOGF(log, "AddModuleList failed: %s", error.AsCString());
+    return false;
+  }
+  error = builder.AddMiscInfo();
+  if (error.Fail()) {
+    LLDB_LOGF(log, "AddMiscInfo failed: %s", error.AsCString());
+    return false;
+  }
 
   error = builder.AddThreadList();
   if (error.Fail()) {
     LLDB_LOGF(log, "AddThreadList failed: %s", error.AsCString());
     return false;
   }
-  if (target.GetArchitecture().GetTriple().getOS() ==
-      llvm::Triple::OSType::Linux) {
-    builder.AddLinuxFileStreams();
+  
+  error = builder.AddLinuxFileStreams();
+  if (error.Fail()) {
+    LLDB_LOGF(log, "AddLinuxFileStreams failed: %s", error.AsCString());
+    return false;
   }
 
   // Add any exceptions but only if there are any in any threads.
-  builder.AddExceptions();
+  error = builder.AddExceptions();
+  if (error.Fail()) {
+    LLDB_LOGF(log, "AddExceptions failed: %s", error.AsCString());
+    return false;
+  }
 
   // Note: add memory HAS to be the last thing we do. It can overflow into 64b
   // land and many RVA's only support 32b
-  error = builder.AddMemory(core_style);
+  error = builder.AddMemoryList(core_style);
   if (error.Fail()) {
-    LLDB_LOGF(log, "AddMemory failed: %s", error.AsCString());
+    LLDB_LOGF(log, "AddMemoryList failed: %s", error.AsCString());
     return false;
   }
 
-  error = builder.DumpToFile();
+  error = builder.DumpFile();
   if (error.Fail()) {
-    LLDB_LOGF(log, "DumpToFile failed: %s", error.AsCString());
+    LLDB_LOGF(log, "DumpFile failed: %s", error.AsCString());
     return false;
   }
 

>From f5de3f1e30acd9f3229caf00d8a4bb0774895159 Mon Sep 17 00:00:00 2001
From: Jacob Lalonde <jalalonde at fb.com>
Date: Mon, 17 Jun 2024 13:18:31 -0700
Subject: [PATCH 11/14] Implement seek logic instead of explicitly writing
 bytes. Fix  bug and instead keep tracrack internally. Add comments explaining
 serialization formating and process as requested by Jeffrey. Run Git clang
 format.

---
 .../Minidump/MinidumpFileBuilder.cpp          | 131 +++++++++---------
 .../ObjectFile/Minidump/MinidumpFileBuilder.h |  57 ++++++--
 .../Minidump/ObjectFileMinidump.cpp           |   5 +-
 3 files changed, 110 insertions(+), 83 deletions(-)

diff --git a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp
index 00e47b86eae59..9c7b23cc0926f 100644
--- a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp
+++ b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp
@@ -56,13 +56,14 @@ using namespace llvm::minidump;
 
 Status MinidumpFileBuilder::AddHeaderAndCalculateDirectories() {
   // First set the offset on the file, and on the bytes saved
-  m_saved_data_size += header_size;
+  m_saved_data_size += HEADER_SIZE;
   // We know we will have at least Misc, SystemInfo, Modules, and ThreadList
   // (corresponding memory list for stacks) And an additional memory list for
   // non-stacks.
   lldb_private::Target &target = m_process_sp->GetTarget();
   m_expected_directories = 6;
-  // Check if OS is linux and reserve directory space for all linux specific breakpad extension directories.
+  // Check if OS is linux and reserve directory space for all linux specific
+  // breakpad extension directories.
   if (target.GetArchitecture().GetTriple().getOS() ==
       llvm::Triple::OSType::Linux)
     m_expected_directories += 9;
@@ -79,41 +80,32 @@ Status MinidumpFileBuilder::AddHeaderAndCalculateDirectories() {
     }
   }
 
-  // Now offset the file by the directores so we can write them in later.
-  offset_t directory_offset = m_expected_directories * directory_size;
-  m_saved_data_size += directory_offset;
+  m_saved_data_size +=
+      m_expected_directories * sizeof(llvm::minidump::Directory);
   Status error;
-  size_t zeroes = 0; // 8 0's
-  size_t remaining_bytes = m_saved_data_size;
-  while (remaining_bytes > 0) {
-    // Keep filling in zero's until we preallocate enough space for the header
-    // and directory sections.
-    size_t bytes_written = std::min(remaining_bytes, sizeof(size_t));
-    error = m_core_file->Write(&zeroes, bytes_written);
-    if (error.Fail()) {
-      error.SetErrorStringWithFormat(
-        "Unable to write header and directory padding (written %zd/%zd)",
-        remaining_bytes - m_saved_data_size, m_saved_data_size);
-      break;
-    }
-
-    remaining_bytes -= bytes_written;
-  }
+  offset_t new_offset = m_core_file->SeekFromStart(m_saved_data_size);
+  if (new_offset != m_saved_data_size)
+    error.SetErrorStringWithFormat("Failed to fill in header and directory "
+                                   "sections. Written / Expected (%zu / %zu)",
+                                   new_offset, m_saved_data_size);
 
   return error;
 }
 
-Status MinidumpFileBuilder::AddDirectory(StreamType type, uint64_t stream_size) {
+Status MinidumpFileBuilder::AddDirectory(StreamType type,
+                                         uint64_t stream_size) {
   Status error;
   if (GetCurrentDataEndOffset() > UINT32_MAX) {
-    error.SetErrorStringWithFormat(
-        "Unable to add directory for stream type %x, offset is greater then 32 bit limit.", type);
+    error.SetErrorStringWithFormat("Unable to add directory for stream type "
+                                   "%x, offset is greater then 32 bit limit.",
+                                   type);
     return error;
   }
 
   if (m_directories.size() + 1 > m_expected_directories) {
     error.SetErrorStringWithFormat(
-        "Unable to add directory for stream type %x, exceeded expected number of directories %d.",
+        "Unable to add directory for stream type %x, exceeded expected number "
+        "of directories %d.",
         type, m_expected_directories);
     return error;
   }
@@ -135,7 +127,8 @@ Status MinidumpFileBuilder::AddSystemInfo() {
   Status error;
   const llvm::Triple &target_triple =
       m_process_sp->GetTarget().GetArchitecture().GetTriple();
-  error = AddDirectory(StreamType::SystemInfo, sizeof(llvm::minidump::SystemInfo));
+  error =
+      AddDirectory(StreamType::SystemInfo, sizeof(llvm::minidump::SystemInfo));
   if (error.Fail())
     return error;
 
@@ -566,7 +559,7 @@ class ArchThreadContexts {
   }
 };
 
-Status MinidumpFileBuilder::FixThreads() {
+Status MinidumpFileBuilder::FixThreadStacks() {
   Status error;
   // If we have anything in the heap flush it.
   FlushBufferToDisk();
@@ -735,7 +728,7 @@ Status MinidumpFileBuilder::AddExceptions() {
 lldb_private::Status MinidumpFileBuilder::AddMiscInfo() {
   Status error;
   error = AddDirectory(StreamType::MiscInfo,
-               sizeof(lldb_private::minidump::MinidumpMiscInfo));
+                       sizeof(lldb_private::minidump::MinidumpMiscInfo));
   if (error.Fail())
     return error;
 
@@ -773,7 +766,8 @@ getFileStreamHelper(const std::string &path) {
 Status MinidumpFileBuilder::AddLinuxFileStreams() {
   Status error;
   // No-op if we are not on linux.
-  if (m_process_sp->GetTarget().GetArchitecture().GetTriple().getOS() != llvm::Triple::Linux)
+  if (m_process_sp->GetTarget().GetArchitecture().GetTriple().getOS() !=
+      llvm::Triple::Linux)
     return error;
 
   std::vector<std::pair<StreamType, std::string>> files_with_stream_types = {
@@ -802,7 +796,6 @@ Status MinidumpFileBuilder::AddLinuxFileStreams() {
         {StreamType::LinuxProcFD, "/proc/" + pid_str + "/fd"});
   }
 
-
   for (const auto &entry : files_with_stream_types) {
     StreamType stream = entry.first;
     std::string path = entry.second;
@@ -832,14 +825,13 @@ Status MinidumpFileBuilder::AddMemoryList(SaveCoreStyle core_style) {
   if (error.Fail())
     return error;
 
-  std::set<addr_t> stack_start_addresses;
-  for (const auto &core_range : ranges_32)
-    stack_start_addresses.insert(core_range.range.start());
-
   uint64_t total_size =
       ranges_32.size() * sizeof(llvm::minidump::MemoryDescriptor);
-  for (const auto &core_range : ranges_32)
+  std::unordered_set<addr_t> stack_start_addresses;
+  for (const auto &core_range : ranges_32) {
+    stack_start_addresses.insert(core_range.range.start());
     total_size += core_range.range.size();
+  }
 
   if (total_size >= UINT32_MAX) {
     error.SetErrorStringWithFormat("Unable to write minidump. Stack memory "
@@ -856,26 +848,25 @@ Status MinidumpFileBuilder::AddMemoryList(SaveCoreStyle core_style) {
       return error;
   }
 
-
   // We need to calculate the MemoryDescriptor size in the worst case
   // Where all memory descriptors are 64b. We also leave some additional padding
   // So that we convert over to 64b with space to spare. This does not waste
   // space in the dump But instead converts some memory from being in the
   // memorylist_32 to the memorylist_64.
-  total_size += 256 + (all_core_memory_ranges.size() - stack_start_addresses.size()) *
-                          sizeof(llvm::minidump::MemoryDescriptor_64);
+  total_size +=
+      256 + (all_core_memory_ranges.size() - stack_start_addresses.size()) *
+                sizeof(llvm::minidump::MemoryDescriptor_64);
 
   for (const auto &core_range : all_core_memory_ranges) {
     addr_t size_to_add =
         core_range.range.size() + sizeof(llvm::minidump::MemoryDescriptor);
-    if (stack_start_addresses.count(core_range.range.start()) > 0) 
+    if (stack_start_addresses.count(core_range.range.start()) > 0)
       // Don't double save stacks.
       continue;
 
     if (total_size + size_to_add < UINT32_MAX) {
       ranges_32.push_back(core_range);
       total_size += core_range.range.size();
-      total_size += sizeof(llvm::minidump::MemoryDescriptor);
     } else {
       ranges_64.push_back(core_range);
     }
@@ -886,13 +877,13 @@ Status MinidumpFileBuilder::AddMemoryList(SaveCoreStyle core_style) {
     return error;
 
   // Add the remaining memory as a 64b range.
-  if (ranges_64.size() > 0) {
+  if (!ranges_64.empty()) {
     error = AddMemoryList_64(ranges_64);
     if (error.Fail())
       return error;
   }
 
-  return FixThreads();
+  return FixThreadStacks();
 }
 
 Status MinidumpFileBuilder::DumpHeader() const {
@@ -906,7 +897,7 @@ Status MinidumpFileBuilder::DumpHeader() const {
       static_cast<llvm::support::ulittle32_t>(m_directories.size());
   // We write the directories right after the header.
   header.StreamDirectoryRVA =
-      static_cast<llvm::support::ulittle32_t>(header_size);
+      static_cast<llvm::support::ulittle32_t>(HEADER_SIZE);
   header.Checksum = static_cast<llvm::support::ulittle32_t>(
       0u), // not used in most of the writers
       header.TimeDateStamp =
@@ -918,13 +909,13 @@ Status MinidumpFileBuilder::DumpHeader() const {
   size_t bytes_written;
 
   m_core_file->SeekFromStart(0);
-  bytes_written = header_size;
+  bytes_written = HEADER_SIZE;
   error = m_core_file->Write(&header, bytes_written);
-  if (error.Fail() || bytes_written != header_size) {
-    if (bytes_written != header_size)
+  if (error.Fail() || bytes_written != HEADER_SIZE) {
+    if (bytes_written != HEADER_SIZE)
       error.SetErrorStringWithFormat(
           "Unable to write the minidump header (written %zd/%zd)",
-          bytes_written, header_size);
+          bytes_written, HEADER_SIZE);
     return error;
   }
   return error;
@@ -937,15 +928,15 @@ size_t MinidumpFileBuilder::GetCurrentDataEndOffset() const {
 Status MinidumpFileBuilder::DumpDirectories() const {
   Status error;
   size_t bytes_written;
-  m_core_file->SeekFromStart(header_size);
+  m_core_file->SeekFromStart(HEADER_SIZE);
   for (const Directory &dir : m_directories) {
-    bytes_written = directory_size;
+    bytes_written = DIRECTORY_SIZE;
     error = m_core_file->Write(&dir, bytes_written);
-    if (error.Fail() || bytes_written != directory_size) {
-      if (bytes_written != directory_size)
+    if (error.Fail() || bytes_written != DIRECTORY_SIZE) {
+      if (bytes_written != DIRECTORY_SIZE)
         error.SetErrorStringWithFormat(
             "unable to write the directory (written %zd/%zd)", bytes_written,
-            directory_size);
+            DIRECTORY_SIZE);
       return error;
     }
   }
@@ -955,14 +946,13 @@ Status MinidumpFileBuilder::DumpDirectories() const {
 
 static size_t GetLargestRange(const Process::CoreFileMemoryRanges &ranges) {
   size_t max_size = 0;
-  for (const auto &core_range : ranges) {
+  for (const auto &core_range : ranges)
     max_size = std::max(max_size, core_range.range.size());
-  }
   return max_size;
 }
 
-Status MinidumpFileBuilder::AddMemoryList_32(
-  Process::CoreFileMemoryRanges &ranges) {
+Status
+MinidumpFileBuilder::AddMemoryList_32(Process::CoreFileMemoryRanges &ranges) {
   std::vector<MemoryDescriptor> descriptors;
   Status error;
   if (ranges.size() == 0)
@@ -1017,9 +1007,9 @@ Status MinidumpFileBuilder::AddMemoryList_32(
   // 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) +
-                   descriptors.size() *
-                       sizeof(llvm::minidump::MemoryDescriptor));
+                       sizeof(llvm::support::ulittle32_t) +
+                           descriptors.size() *
+                               sizeof(llvm::minidump::MemoryDescriptor));
   if (error.Fail())
     return error;
 
@@ -1034,15 +1024,16 @@ Status MinidumpFileBuilder::AddMemoryList_32(
   return error;
 }
 
-Status MinidumpFileBuilder::AddMemoryList_64(
-    Process::CoreFileMemoryRanges &ranges) {
+Status
+MinidumpFileBuilder::AddMemoryList_64(Process::CoreFileMemoryRanges &ranges) {
   Status error;
-  if (ranges.empty()) 
+  if (ranges.empty())
     return error;
 
   error = AddDirectory(StreamType::Memory64List,
-               (sizeof(llvm::support::ulittle64_t) * 2) +
-                   ranges.size() * sizeof(llvm::minidump::MemoryDescriptor_64));
+                       (sizeof(llvm::support::ulittle64_t) * 2) +
+                           ranges.size() *
+                               sizeof(llvm::minidump::MemoryDescriptor_64));
   if (error.Fail())
     return error;
 
@@ -1051,10 +1042,13 @@ Status MinidumpFileBuilder::AddMemoryList_64(
   m_data.AppendData(&memory_ranges_num, sizeof(llvm::support::ulittle64_t));
   // 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);
+  offset_t starting_offset =
+      GetCurrentDataEndOffset() + sizeof(llvm::support::ulittle64_t);
   // The base_rva needs to start after the directories, which is right after
   // this 8 byte variable.
-  offset_t base_rva = starting_offset + (ranges.size() * sizeof(llvm::minidump::MemoryDescriptor_64));
+  offset_t base_rva =
+      starting_offset +
+      (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,
@@ -1076,7 +1070,6 @@ Status MinidumpFileBuilder::AddMemoryList_64(
     m_data.AppendData(&memory_desc, sizeof(MemoryDescriptor_64));
   }
 
-
   Log *log = GetLog(LLDBLog::Object);
   size_t region_index = 0;
   auto data_up = std::make_unique<DataBufferHeap>(GetLargestRange(ranges), 0);
@@ -1142,7 +1135,7 @@ Status MinidumpFileBuilder::AddData(const void *data, addr_t size) {
   // here is to limit the number of bytes we need to host in memory at any given
   // time.
   m_data.AppendData(data, size);
-  if (m_data.GetByteSize() > m_write_chunk_max) {
+  if (m_data.GetByteSize() > MAX_WRITE_CHUNK_SIZE) {
     return FlushBufferToDisk();
   }
 
@@ -1152,7 +1145,7 @@ Status MinidumpFileBuilder::AddData(const void *data, addr_t size) {
 Status MinidumpFileBuilder::FlushBufferToDisk() {
   Status error;
   // Set the stream to it's end.
-  m_core_file->SeekFromEnd(0);
+  m_core_file->SeekFromStart(m_saved_data_size);
   addr_t starting_size = m_data.GetByteSize();
   addr_t remaining_bytes = starting_size;
   offset_t offset = 0;
diff --git a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h
index 31a587813b43e..f2fdfacf9045d 100644
--- a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h
+++ b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h
@@ -44,14 +44,39 @@ lldb_private::Status WriteString(const std::string &to_write,
 /// Minidump writer for Linux
 ///
 /// This class provides a Minidump writer that is able to
-/// snapshot the current process state. For the whole time, it stores all
-/// the data on heap.
+/// snapshot the current process state.
+///
+/// Minidumps are a Microsoft format for dumping process state.
+/// This class constructs the minidump on disk starting with
+/// Headers and Directories are written at the top of the file,
+/// with the amount of bytes being precalculates before any writing takes place
+/// Then the smaller data sections are written
+/// SystemInfo, ModuleList, Misc Info.
+/// Then Threads are emitted, threads are the first section that needs to be
+/// 'fixed up' this happens when later we emit the memory stream, we identify if
+/// that stream is the expected stack, and if so we update the stack with the
+/// current RVA. Lastly the Memory lists are added. For Memory List, this will
+/// contain everything that can fit within 4.2gb. MemoryList has it's
+/// descriptors written at the end so it cannot be allowed to overflow.
+///
+/// Memory64List is a special case where it has to be begin before 4.2gb but can
+/// expand forever The difference in Memory64List is there are no RVA's and all
+/// the addresses are figured out by starting at the base RVA, and adding the
+/// antecedent memory sections.
+///
+/// Because Memory64List can be arbitrarily large, this class has to write
+/// chunks to disk this means we have to precalculate the descriptors and write
+/// them first, and if we encounter any error, or are unable to read the same
+/// number of bytes we have to go back and update them on disk.
+///
+/// And as the last step, after all the directories have been added, we go back
+/// to the top of the file to fill in the header and the redirectory sections
+/// that we preallocated.
 class MinidumpFileBuilder {
 public:
   MinidumpFileBuilder(lldb::FileUP &&core_file,
                       const lldb::ProcessSP &process_sp)
-      : m_process_sp(process_sp),
-        m_core_file(std::move(core_file)) {};
+      : m_process_sp(process_sp), m_core_file(std::move(core_file)){};
 
   MinidumpFileBuilder(const MinidumpFileBuilder &) = delete;
   MinidumpFileBuilder &operator=(const MinidumpFileBuilder &) = delete;
@@ -61,6 +86,9 @@ class MinidumpFileBuilder {
 
   ~MinidumpFileBuilder() = default;
 
+  // This method only calculates the amount of bytes the header and directories
+  // will take up. It does not write the directories or headers. This function
+  // must be called with a followup to fill in the data.
   lldb_private::Status AddHeaderAndCalculateDirectories();
   // Add SystemInfo stream, used for storing the most basic information
   // about the system, platform etc...
@@ -93,22 +121,26 @@ class MinidumpFileBuilder {
   AddMemoryList_64(lldb_private::Process::CoreFileMemoryRanges &ranges);
   lldb_private::Status
   AddMemoryList_32(lldb_private::Process::CoreFileMemoryRanges &ranges);
-  lldb_private::Status FixThreads();
+  // Update the thread list on disk with the newly emitted stack RVAs.
+  lldb_private::Status FixThreadStacks();
   lldb_private::Status FlushBufferToDisk();
 
   lldb_private::Status DumpHeader() const;
   lldb_private::Status DumpDirectories() const;
   // Add directory of StreamType pointing to the current end of the prepared
   // file with the specified size.
-  lldb_private::Status AddDirectory(llvm::minidump::StreamType type, uint64_t stream_size);
+  lldb_private::Status AddDirectory(llvm::minidump::StreamType type,
+                                    uint64_t stream_size);
   lldb::offset_t GetCurrentDataEndOffset() const;
   // Stores directories to fill in later
   std::vector<llvm::minidump::Directory> m_directories;
   // When we write off the threads for the first time, we need to clean them up
   // and give them the correct RVA once we write the stack memory list.
   // We save by the end because we only take from the stack pointer up
-  // So the saved off range base can differ from the memory region the stack pointer is in.
-  std::unordered_map<lldb::addr_t, llvm::minidump::Thread> m_thread_by_range_end;
+  // So the saved off range base can differ from the memory region the stack
+  // pointer is in.
+  std::unordered_map<lldb::addr_t, llvm::minidump::Thread>
+      m_thread_by_range_end;
   // Main data buffer consisting of data without the minidump header and
   // directories
   lldb_private::DataBufferHeap m_data;
@@ -121,15 +153,16 @@ class MinidumpFileBuilder {
   // but we want to try to keep the size of m_data small
   // and we will only exceed a 128 mb buffer if we get a memory region
   // that is larger than 128 mb.
-  static constexpr size_t m_write_chunk_max = (1024 * 1024 * 128);
+  static constexpr size_t MAX_WRITE_CHUNK_SIZE = (1024 * 1024 * 128);
 
-  static constexpr size_t header_size = sizeof(llvm::minidump::Header);
-  static constexpr size_t directory_size = sizeof(llvm::minidump::Directory);
+  static constexpr size_t HEADER_SIZE = sizeof(llvm::minidump::Header);
+  static constexpr size_t DIRECTORY_SIZE = sizeof(llvm::minidump::Directory);
 
   // 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.
-  std::map<lldb::tid_t, llvm::minidump::LocationDescriptor> m_tid_to_reg_ctx;
+  std::unordered_map<lldb::tid_t, llvm::minidump::LocationDescriptor>
+      m_tid_to_reg_ctx;
   lldb::FileUP m_core_file;
 };
 
diff --git a/lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.cpp b/lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.cpp
index 5174d73e4f579..3668c37c5191d 100644
--- a/lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.cpp
+++ b/lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.cpp
@@ -78,7 +78,8 @@ bool ObjectFileMinidump::SaveCore(const lldb::ProcessSP &process_sp,
   Log *log = GetLog(LLDBLog::Object);
   error = builder.AddHeaderAndCalculateDirectories();
   if (error.Fail()) {
-    LLDB_LOGF(log, "AddHeaderAndCalculateDirectories failed: %s", error.AsCString());
+    LLDB_LOGF(log, "AddHeaderAndCalculateDirectories failed: %s",
+              error.AsCString());
     return false;
   };
   error = builder.AddSystemInfo();
@@ -103,7 +104,7 @@ bool ObjectFileMinidump::SaveCore(const lldb::ProcessSP &process_sp,
     LLDB_LOGF(log, "AddThreadList failed: %s", error.AsCString());
     return false;
   }
-  
+
   error = builder.AddLinuxFileStreams();
   if (error.Fail()) {
     LLDB_LOGF(log, "AddLinuxFileStreams failed: %s", error.AsCString());

>From 95a0a6c5f9652f6c536dd1d557813542079b462e Mon Sep 17 00:00:00 2001
From: Jacob Lalonde <jalalonde at fb.com>
Date: Mon, 17 Jun 2024 18:05:16 -0700
Subject: [PATCH 12/14] Code review feedback on size_t, fix printf for uint64.
 Cast type to uint32_t to avoid warnings. Run git clang format.

---
 .../Minidump/MinidumpFileBuilder.cpp          | 59 +++++++++++--------
 1 file changed, 33 insertions(+), 26 deletions(-)

diff --git a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp
index 9c7b23cc0926f..cd486f7947c3d 100644
--- a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp
+++ b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp
@@ -56,7 +56,7 @@ using namespace llvm::minidump;
 
 Status MinidumpFileBuilder::AddHeaderAndCalculateDirectories() {
   // First set the offset on the file, and on the bytes saved
-  m_saved_data_size += HEADER_SIZE;
+  m_saved_data_size = HEADER_SIZE;
   // We know we will have at least Misc, SystemInfo, Modules, and ThreadList
   // (corresponding memory list for stacks) And an additional memory list for
   // non-stacks.
@@ -86,7 +86,8 @@ Status MinidumpFileBuilder::AddHeaderAndCalculateDirectories() {
   offset_t new_offset = m_core_file->SeekFromStart(m_saved_data_size);
   if (new_offset != m_saved_data_size)
     error.SetErrorStringWithFormat("Failed to fill in header and directory "
-                                   "sections. Written / Expected (%zu / %zu)",
+                                   "sections. Written / Expected (%" PRIx64
+                                   " / %zu)",
                                    new_offset, m_saved_data_size);
 
   return error;
@@ -94,11 +95,12 @@ Status MinidumpFileBuilder::AddHeaderAndCalculateDirectories() {
 
 Status MinidumpFileBuilder::AddDirectory(StreamType type,
                                          uint64_t stream_size) {
+  // We explicitly cast type, an 32b enum, to uint32_t to avoid warnings.
   Status error;
   if (GetCurrentDataEndOffset() > UINT32_MAX) {
     error.SetErrorStringWithFormat("Unable to add directory for stream type "
                                    "%x, offset is greater then 32 bit limit.",
-                                   type);
+                                   (uint32_t)type);
     return error;
   }
 
@@ -106,7 +108,7 @@ Status MinidumpFileBuilder::AddDirectory(StreamType type,
     error.SetErrorStringWithFormat(
         "Unable to add directory for stream type %x, exceeded expected number "
         "of directories %d.",
-        type, m_expected_directories);
+        (uint32_t)type, m_expected_directories);
     return error;
   }
 
@@ -818,6 +820,10 @@ Status MinidumpFileBuilder::AddLinuxFileStreams() {
 Status MinidumpFileBuilder::AddMemoryList(SaveCoreStyle core_style) {
   Status error;
 
+  // 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.
   Process::CoreFileMemoryRanges ranges_32;
   Process::CoreFileMemoryRanges ranges_64;
   error = m_process_sp->CalculateCoreFileSaveRanges(
@@ -848,23 +854,21 @@ Status MinidumpFileBuilder::AddMemoryList(SaveCoreStyle core_style) {
       return error;
   }
 
-  // We need to calculate the MemoryDescriptor size in the worst case
-  // Where all memory descriptors are 64b. We also leave some additional padding
-  // So that we convert over to 64b with space to spare. This does not waste
-  // space in the dump But instead converts some memory from being in the
-  // memorylist_32 to the memorylist_64.
+  // 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.
   total_size +=
       256 + (all_core_memory_ranges.size() - stack_start_addresses.size()) *
                 sizeof(llvm::minidump::MemoryDescriptor_64);
 
   for (const auto &core_range : all_core_memory_ranges) {
-    addr_t size_to_add =
-        core_range.range.size() + sizeof(llvm::minidump::MemoryDescriptor);
+    addr_t range_size = core_range.range.size();
     if (stack_start_addresses.count(core_range.range.start()) > 0)
       // Don't double save stacks.
       continue;
 
-    if (total_size + size_to_add < UINT32_MAX) {
+    if (total_size + range_size < UINT32_MAX) {
       ranges_32.push_back(core_range);
       total_size += core_range.range.size();
     } else {
@@ -944,8 +948,9 @@ Status MinidumpFileBuilder::DumpDirectories() const {
   return error;
 }
 
-static size_t GetLargestRange(const Process::CoreFileMemoryRanges &ranges) {
-  size_t max_size = 0;
+static uint64_t
+GetLargestRangeSize(const Process::CoreFileMemoryRanges &ranges) {
+  uint64_t max_size = 0;
   for (const auto &core_range : ranges)
     max_size = std::max(max_size, core_range.range.size());
   return max_size;
@@ -960,17 +965,18 @@ MinidumpFileBuilder::AddMemoryList_32(Process::CoreFileMemoryRanges &ranges) {
 
   Log *log = GetLog(LLDBLog::Object);
   size_t region_index = 0;
-  auto data_up = std::make_unique<DataBufferHeap>(GetLargestRange(ranges), 0);
+  auto data_up =
+      std::make_unique<DataBufferHeap>(GetLargestRangeSize(ranges), 0);
   for (const auto &core_range : ranges) {
     // Take the offset before we write.
-    const size_t offset_for_data = GetCurrentDataEndOffset();
+    const offset_t offset_for_data = GetCurrentDataEndOffset();
     const addr_t addr = core_range.range.start();
     const addr_t size = core_range.range.size();
     const addr_t end = core_range.range.end();
 
     LLDB_LOGF(log,
               "AddMemoryList %zu/%zu reading memory for region "
-              "(%zu bytes) [%zx, %zx)",
+              "(%" PRIx64 " bytes) [%" PRIx64 ", %" PRIx64 ")",
               region_index, ranges.size(), size, addr, addr + size);
     ++region_index;
 
@@ -982,8 +988,9 @@ MinidumpFileBuilder::AddMemoryList_32(Process::CoreFileMemoryRanges &ranges) {
       // Just skip sections with errors or zero bytes in 32b mode
       continue;
     } else if (bytes_read != size) {
-      LLDB_LOGF(log, "Memory region at: %zu failed to read %zu bytes", addr,
-                size);
+      LLDB_LOGF(
+          log, "Memory region at: %" PRIx64 " failed to read %" PRIx64 " bytes",
+          addr, size);
     }
 
     MemoryDescriptor descriptor;
@@ -1072,15 +1079,16 @@ MinidumpFileBuilder::AddMemoryList_64(Process::CoreFileMemoryRanges &ranges) {
 
   Log *log = GetLog(LLDBLog::Object);
   size_t region_index = 0;
-  auto data_up = std::make_unique<DataBufferHeap>(GetLargestRange(ranges), 0);
+  auto data_up =
+      std::make_unique<DataBufferHeap>(GetLargestRangeSize(ranges), 0);
   for (const auto &core_range : ranges) {
     const addr_t addr = core_range.range.start();
     const addr_t size = core_range.range.size();
 
     LLDB_LOGF(log,
               "AddMemoryList_64 %zu/%zu reading memory for region "
-              "(%zu bytes) "
-              "[%zx, %zx)",
+              "(%" PRIx64 "bytes) "
+              "[%" PRIx64 ", %" PRIx64 ")",
               region_index, ranges.size(), size, addr, addr + size);
     ++region_index;
 
@@ -1094,8 +1102,8 @@ MinidumpFileBuilder::AddMemoryList_64(Process::CoreFileMemoryRanges &ranges) {
       descriptors[region_index].DataSize = 0;
     }
     if (bytes_read != size) {
-      LLDB_LOGF(log, "Memory region at: %zu failed to read %zu bytes", addr,
-                size);
+      LLDB_LOGF(log, "Memory region at: %" PRIx64 " failed to read %zu bytes",
+                addr, size);
       cleanup_required = true;
       descriptors[region_index].DataSize = bytes_read;
     }
@@ -1135,9 +1143,8 @@ Status MinidumpFileBuilder::AddData(const void *data, addr_t size) {
   // here is to limit the number of bytes we need to host in memory at any given
   // time.
   m_data.AppendData(data, size);
-  if (m_data.GetByteSize() > MAX_WRITE_CHUNK_SIZE) {
+  if (m_data.GetByteSize() > MAX_WRITE_CHUNK_SIZE)
     return FlushBufferToDisk();
-  }
 
   return Status();
 }

>From 487c7fc5cd793af7afe0c1cc9bd44f0e5ada165b Mon Sep 17 00:00:00 2001
From: Jacob Lalonde <jalalonde at fb.com>
Date: Tue, 18 Jun 2024 09:54:55 -0700
Subject: [PATCH 13/14] Extra cleanup that Greg pointed out, ensure full use of
 variable in loop and make it const. Fix m_total_siz_size formatting. Run
 git-clang-format

---
 .../ObjectFile/Minidump/MinidumpFileBuilder.cpp   | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp
index cd486f7947c3d..e6018bc08cfb4 100644
--- a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp
+++ b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp
@@ -87,7 +87,7 @@ Status MinidumpFileBuilder::AddHeaderAndCalculateDirectories() {
   if (new_offset != m_saved_data_size)
     error.SetErrorStringWithFormat("Failed to fill in header and directory "
                                    "sections. Written / Expected (%" PRIx64
-                                   " / %zu)",
+                                   " / %" PRIx64 ")",
                                    new_offset, m_saved_data_size);
 
   return error;
@@ -858,19 +858,22 @@ Status MinidumpFileBuilder::AddMemoryList(SaveCoreStyle core_style) {
   // 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.
-  total_size +=
-      256 + (all_core_memory_ranges.size() - stack_start_addresses.size()) *
-                sizeof(llvm::minidump::MemoryDescriptor_64);
+  // All core memeroy ranges will either container nothing on stacks only
+  // or all the memory ranges including stacks
+  if (!all_core_memory_ranges.empty())
+    total_size +=
+        256 + (all_core_memory_ranges.size() - stack_start_addresses.size()) *
+                  sizeof(llvm::minidump::MemoryDescriptor_64);
 
   for (const auto &core_range : all_core_memory_ranges) {
-    addr_t range_size = core_range.range.size();
+    const addr_t range_size = core_range.range.size();
     if (stack_start_addresses.count(core_range.range.start()) > 0)
       // Don't double save stacks.
       continue;
 
     if (total_size + range_size < UINT32_MAX) {
       ranges_32.push_back(core_range);
-      total_size += core_range.range.size();
+      total_size += range_size;
     } else {
       ranges_64.push_back(core_range);
     }

>From 86b0be17fbf04bf01201d5fb561bc77281ddef9c Mon Sep 17 00:00:00 2001
From: Jacob Lalonde <jalalonde at fb.com>
Date: Thu, 20 Jun 2024 17:16:55 -0700
Subject: [PATCH 14/14] Start total_size at current offset, make exception
 directory entries for stop signals. Run git clang format

---
 .../ObjectFile/Minidump/MinidumpFileBuilder.cpp     | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp
index e6018bc08cfb4..39d428a1e5396 100644
--- a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp
+++ b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp
@@ -74,9 +74,11 @@ Status MinidumpFileBuilder::AddHeaderAndCalculateDirectories() {
   for (uint32_t thread_idx = 0; thread_idx < num_threads; ++thread_idx) {
     ThreadSP thread_sp(thread_list.GetThreadAtIndex(thread_idx));
     StopInfoSP stop_info_sp = thread_sp->GetStopInfo();
-    if (stop_info_sp &&
-        stop_info_sp->GetStopReason() == StopReason::eStopReasonException) {
-      m_expected_directories++;
+    if (stop_info_sp) {
+      const StopReason &stop_reason = stop_info_sp->GetStopReason();
+      if (stop_reason == StopReason::eStopReasonException ||
+          stop_reason == StopReason::eStopReasonSignal)
+        m_expected_directories++;
     }
   }
 
@@ -831,8 +833,9 @@ Status MinidumpFileBuilder::AddMemoryList(SaveCoreStyle core_style) {
   if (error.Fail())
     return error;
 
-  uint64_t total_size =
-      ranges_32.size() * sizeof(llvm::minidump::MemoryDescriptor);
+  // Calculate totalsize including the current offset.
+  uint64_t total_size = GetCurrentDataEndOffset();
+  total_size += ranges_32.size() * sizeof(llvm::minidump::MemoryDescriptor);
   std::unordered_set<addr_t> stack_start_addresses;
   for (const auto &core_range : ranges_32) {
     stack_start_addresses.insert(core_range.range.start());



More information about the lldb-commits mailing list