[LLDB] Fix Incorrect offset for Memory64 RVA in Minidump (+ Testing changes)

lldb/bindings/interface/SBSaveCoreOptionsDocstrings.i

@@ -63,6 +63,18 @@ Note that currently ELF Core files are not supported."
     Get an SBThreadCollection of all threads marked to be saved. This collection is not sorted according to insertion order."
 ) lldb::SBSaveCoreOptions::GetThreadsToSave;
 
+
+%feature("docstring", "
+    Get an SBMemoryRegionInfoList of all the Regions that LLDB will attempt to write into the Core. Note, reading from these
+    regions can fail, and it's guaraunteed every region will be present. If called without a valid process or style set an empty
+    collection will be returned."
+) lldb::SBSaveCoreOptions::GetMemoryRegionsToSave;
+
+%feature("docstring", "
+    Add a plugin specific flag to the objects option."
+) lldb::SBSaveCoreOptions::AddFlag;
+
+
 %feature("docstring", "
     Get the current total number of bytes the core is expected to have, excluding the overhead of the core file format.
     Requires both a Process and a Style to be specified. An error will be returned if the provided options would result in no data being saved."

lldb/include/lldb/API/SBMemoryRegionInfoList.h

@@ -45,6 +45,7 @@ class LLDB_API SBMemoryRegionInfoList {
 
 private:
   friend class SBProcess;
+  friend class SBSaveCoreOptions;
 
   lldb_private::MemoryRegionInfos &ref();
 

lldb/include/lldb/API/SBSaveCoreOptions.h

@@ -12,6 +12,7 @@
 #include "lldb/API/SBDefines.h"
 #include "lldb/API/SBError.h"
 #include "lldb/API/SBFileSpec.h"
+#include "lldb/API/SBMemoryRegionInfoList.h"
 #include "lldb/API/SBProcess.h"
 #include "lldb/API/SBThread.h"
 #include "lldb/API/SBThreadCollection.h"
@@ -119,6 +120,13 @@ class LLDB_API SBSaveCoreOptions {
   ///   an empty collection will be returned.
   SBThreadCollection GetThreadsToSave() const;
 
+  /// Get an unsorted copy of all memory regions to save
+  ///
+  /// \returns
+  ///   An unsorted copy of all memory regions to save. If no process or style
+  ///   is specified an empty collection will be returned.
+  SBMemoryRegionInfoList GetMemoryRegionsToSave();
+
   /// Get the current total number of bytes the core is expected to have
   /// excluding the overhead of the core file format. Requires a Process and
   /// Style to be specified.
@@ -132,6 +140,14 @@ class LLDB_API SBSaveCoreOptions {
   ///   The expected size of the data contained in the core in bytes.
   uint64_t GetCurrentSizeInBytes(SBError &error);
 
+  /// Add a flag to be consumed by the specified plugin, null or empty flags
+  /// will be ignored.
+  ///
+  /// \note
+  ///   This API is currently only used for testing, with forcing Minidumps to
+  ///   to 64b memory list the reason this api was added
+  void AddFlag(const char *flag);
+
   /// Reset all options.
   void Clear();
 

lldb/include/lldb/Symbol/SaveCoreOptions.h

@@ -9,6 +9,7 @@
 #ifndef LLDB_SOURCE_PLUGINS_OBJECTFILE_SaveCoreOPTIONS_H
 #define LLDB_SOURCE_PLUGINS_OBJECTFILE_SaveCoreOPTIONS_H
 
+#include "lldb/Target/CoreFileMemoryRanges.h"
 #include "lldb/Target/ThreadCollection.h"
 #include "lldb/Utility/FileSpec.h"
 #include "lldb/Utility/RangeMap.h"
@@ -23,7 +24,7 @@ namespace lldb_private {
 
 class SaveCoreOptions {
 public:
-  SaveCoreOptions(){};
+  SaveCoreOptions() = default;
   ~SaveCoreOptions() = default;
 
   lldb_private::Status SetPluginName(const char *name);
@@ -47,10 +48,15 @@ class SaveCoreOptions {
 
   void AddMemoryRegionToSave(const lldb_private::MemoryRegionInfo &region);
 
+  llvm::Expected<lldb_private::CoreFileMemoryRanges> GetMemoryRegionsToSave();
   lldb_private::ThreadCollection::collection GetThreadsToSave() const;
 
   llvm::Expected<uint64_t> GetCurrentSizeInBytes();
 
+  void AddFlag(const char *flag);
+
+  bool ContainsFlag(const char *flag) const;
+
   void Clear();
 
 private:
@@ -59,6 +65,7 @@ class SaveCoreOptions {
   std::optional<std::string> m_plugin_name;
   std::optional<lldb_private::FileSpec> m_file;
   std::optional<lldb::SaveCoreStyle> m_style;
+  std::optional<std::unordered_set<std::string>> m_flags;
   lldb::ProcessSP m_process_sp;
   std::unordered_set<lldb::tid_t> m_threads_to_save;
   MemoryRanges m_regions_to_save;

lldb/source/API/SBSaveCoreOptions.cpp

@@ -128,6 +128,32 @@ uint64_t SBSaveCoreOptions::GetCurrentSizeInBytes(SBError &error) {
   return *expected_bytes;
 }
 
+lldb::SBMemoryRegionInfoList SBSaveCoreOptions::GetMemoryRegionsToSave() {
+  LLDB_INSTRUMENT_VA(this);
+  llvm::Expected<lldb_private::CoreFileMemoryRanges> memory_ranges =
+      m_opaque_up->GetMemoryRegionsToSave();
+  if (!memory_ranges) {
+    llvm::consumeError(memory_ranges.takeError());
+    return SBMemoryRegionInfoList();
+  }
+
+  SBMemoryRegionInfoList m_memory_region_infos;
+  for (const auto &range : *memory_ranges) {
+    SBMemoryRegionInfo region_info(
+        nullptr, range.GetRangeBase(), range.GetRangeEnd(),
+        range.data.lldb_permissions, /*mapped=*/true);
+    m_memory_region_infos.Append(region_info);
+  }
+
+  return m_memory_region_infos;
+}
+
+void SBSaveCoreOptions::AddFlag(const char *flag) {
+  LLDB_INSTRUMENT_VA(this, flag);
+
+  m_opaque_up->AddFlag(flag);
+}
+
 lldb_private::SaveCoreOptions &SBSaveCoreOptions::ref() const {
   return *m_opaque_up;
 }

lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp

@@ -831,6 +831,13 @@ Status MinidumpFileBuilder::AddLinuxFileStreams() {
 Status MinidumpFileBuilder::AddMemoryList() {
   Status error;
 
+  // Note this is here for testing. In the past there has been many occasions
+  // that the 64b code has regressed because it's wasteful and expensive to
+  // write a 4.2gb+ on every CI run to get around this and to exercise this
+  // codepath we define a flag in the options object.
+  bool force_64b_for_non_threads =
+      m_save_core_options.ContainsFlag(FORCE_64B_FLAG);
+
   // We first save the thread stacks to ensure they fit in the first UINT32_MAX
   // bytes of the core file. Thread structures in minidump files can only use
   // 32 bit memory descriptiors, so we emit them first to ensure the memory is
@@ -890,7 +897,7 @@ Status MinidumpFileBuilder::AddMemoryList() {
     const addr_t range_size = core_range.range.size();
     // We don't need to check for stacks here because we already removed them
     // from all_core_memory_ranges.
-    if (total_size + range_size < UINT32_MAX) {
+    if (!force_64b_for_non_threads && total_size + range_size < UINT32_MAX) {
       ranges_32.push_back(core_range);
       total_size += range_size;
     } else {
@@ -977,14 +984,15 @@ Status MinidumpFileBuilder::ReadWriteMemoryInChunks(
   const lldb::addr_t addr = range.range.start();
   const lldb::addr_t size = range.range.size();
   Log *log = GetLog(LLDBLog::Object);
+  uint64_t total_bytes_read = 0;
   Status addDataError;
   Process::ReadMemoryChunkCallback callback =
       [&](Status &error, lldb::addr_t current_addr, const void *buf,
           uint64_t bytes_read) -> lldb_private::IterationAction {
     if (error.Fail() || bytes_read == 0) {
       LLDB_LOGF(log,
                 "Failed to read memory region at: 0x%" PRIx64
-                ". Bytes read: %" PRIx64 ", error: %s",
+                ". Bytes read: 0x%" PRIx64 ", error: %s",
                 current_addr, bytes_read, error.AsCString());
 
       // If we failed in a memory read, we would normally want to skip
@@ -997,6 +1005,13 @@ Status MinidumpFileBuilder::ReadWriteMemoryInChunks(
       return lldb_private::IterationAction::Stop;
     }
 
+    if (current_addr != addr + total_bytes_read) {
+      LLDB_LOGF(log,
+                "Current addr is at expected address, 0x%" PRIx64
+                ", expected at 0x%" PRIx64,
+                current_addr, addr + total_bytes_read);
+    }
+
     // Write to the minidump file with the chunk potentially flushing to
     // disk.
     // This error will be captured by the outer scope and is considered fatal.
@@ -1006,13 +1021,13 @@ Status MinidumpFileBuilder::ReadWriteMemoryInChunks(
     if (addDataError.Fail())
       return lldb_private::IterationAction::Stop;
 
+    total_bytes_read += bytes_read;
     // If we have a partial read, report it, but only if the partial read
     // didn't finish reading the entire region.
-    if (bytes_read != data_buffer.GetByteSize() &&
-        current_addr + bytes_read != size) {
+    if (bytes_read != data_buffer.GetByteSize() && total_bytes_read != size) {
       LLDB_LOGF(log,
-                "Memory region at: %" PRIx64 " partiall read 0x%" PRIx64
-                " bytes out of %" PRIx64 " bytes.",
+                "Memory region at: 0x%" PRIx64 " partial read 0x%" PRIx64
+                " bytes out of 0x%" PRIx64 " bytes.",
                 current_addr, bytes_read,
                 data_buffer.GetByteSize() - bytes_read);
 
@@ -1059,7 +1074,7 @@ MinidumpFileBuilder::AddMemoryList_32(std::vector<CoreFileMemoryRange> &ranges,
 
     LLDB_LOGF(log,
               "AddMemoryList %zu/%zu reading memory for region "
-              "(%" PRIx64 " bytes) [%" PRIx64 ", %" PRIx64 ")",
+              "(0x%" PRIx64 " bytes) [0x%" PRIx64 ", 0x%" PRIx64 ")",
               region_index, ranges.size(), size, addr, addr + size);
     ++region_index;
 
@@ -1130,9 +1145,9 @@ MinidumpFileBuilder::AddMemoryList_64(std::vector<CoreFileMemoryRange> &ranges,
   // Capture the starting offset for all the descriptors so we can clean them up
   // if needed.
   offset_t starting_offset =
-      GetCurrentDataEndOffset() + sizeof(llvm::support::ulittle64_t);
+      GetCurrentDataEndOffset() + sizeof(llvm::minidump::Memory64ListHeader);
   // The base_rva needs to start after the directories, which is right after
-  // this 8 byte variable.
+  // the descriptors + the size of the header.
   offset_t base_rva =
       starting_offset +
       (ranges.size() * sizeof(llvm::minidump::MemoryDescriptor_64));

lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h

@@ -176,6 +176,8 @@ class MinidumpFileBuilder {
   static constexpr size_t HEADER_SIZE = sizeof(llvm::minidump::Header);
   static constexpr size_t DIRECTORY_SIZE = sizeof(llvm::minidump::Directory);
 
+  static constexpr const char FORCE_64B_FLAG[] = "force_64b";
+
   // More that one place can mention the register thread context locations,
   // so when we emit the thread contents, remember where it is so we don't have
   // to duplicate it in the exception data.

lldb/source/Symbol/SaveCoreOptions.cpp

@@ -155,6 +155,24 @@ SaveCoreOptions::GetThreadsToSave() const {
   return thread_collection;
 }
 
+llvm::Expected<lldb_private::CoreFileMemoryRanges>
+SaveCoreOptions::GetMemoryRegionsToSave() {
+  Status error;
+  if (!m_process_sp)
+    return Status::FromErrorString("Requires a process to be set.").takeError();
+
+  error = EnsureValidConfiguration(m_process_sp);
+  if (error.Fail())
+    return error.takeError();
+
+  CoreFileMemoryRanges ranges;
+  error = m_process_sp->CalculateCoreFileSaveRanges(*this, ranges);
+  if (error.Fail())
+    return error.takeError();
+
+  return ranges;
+}
+
 llvm::Expected<uint64_t> SaveCoreOptions::GetCurrentSizeInBytes() {
   Status error;
   if (!m_process_sp)
@@ -169,8 +187,14 @@ llvm::Expected<uint64_t> SaveCoreOptions::GetCurrentSizeInBytes() {
   if (error.Fail())
     return error.takeError();
 
+  llvm::Expected<lldb_private::CoreFileMemoryRanges> core_file_ranges_maybe =
+      GetMemoryRegionsToSave();
+  if (!core_file_ranges_maybe)
+    return core_file_ranges_maybe.takeError();
+  const lldb_private::CoreFileMemoryRanges &core_file_ranges =
+      *core_file_ranges_maybe;
   uint64_t total_in_bytes = 0;
-  for (auto &core_range : ranges)
+  for (auto &core_range : core_file_ranges)
     total_in_bytes += core_range.data.range.size();
 
   return total_in_bytes;
@@ -190,3 +214,17 @@ void SaveCoreOptions::Clear() {
   m_process_sp.reset();
   m_regions_to_save.Clear();
 }
+
+void SaveCoreOptions::AddFlag(const char *flag) {
+  if (!flag || !flag[0])
+    return;
+
+  if (!m_flags)
+    m_flags = std::unordered_set<std::string>();
+
+  m_flags->emplace(std::string(flag));
+}
+
+bool SaveCoreOptions::ContainsFlag(const char *flag) const {
+  return m_flags && m_flags->find(flag) != m_flags->end();
+}