Implement new logic to minidump and move objectmacho to the new API. Run git-clang-format

Author: Jlalond

lldb/include/lldb/Target/Process.h

@@ -738,13 +738,13 @@ class Process : public std::enable_shared_from_this<Process>,
   /// Helper function for Process::SaveCore(...) that calculates the address
   /// ranges that should be saved. This allows all core file plug-ins to save
   /// consistent memory ranges given a \a core_style.
-  Status CalculateCoreFileSaveRanges(lldb::SaveCoreStyle core_style,
+  Status CalculateCoreFileSaveRanges(const SaveCoreOptions &core_options,
                                      CoreFileMemoryRanges &ranges);
 
   /// Helper function for Process::SaveCore(...) that calculates the thread list
   /// based upon options set within a given \a core_options object.
-  ThreadCollection::ThreadIterable
-  CalculateCoreFileThreadList(SaveCoreOptions &core_options);
+  std::vector<lldb::ThreadSP>
+  CalculateCoreFileThreadList(const SaveCoreOptions &core_options);
 
 protected:
   virtual JITLoaderList &GetJITLoaders();

lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp

@@ -6558,7 +6558,7 @@ bool ObjectFileMachO::SaveCore(const lldb::ProcessSP &process_sp,
 
     if (make_core) {
       Process::CoreFileMemoryRanges core_ranges;
-      error = process_sp->CalculateCoreFileSaveRanges(core_style, core_ranges);
+      error = process_sp->CalculateCoreFileSaveRanges(options, core_ranges);
       if (error.Success()) {
         const uint32_t addr_byte_size = target_arch.GetAddressByteSize();
         const ByteOrder byte_order = target_arch.GetByteOrder();
@@ -6608,8 +6608,9 @@ bool ObjectFileMachO::SaveCore(const lldb::ProcessSP &process_sp,
         mach_header.ncmds = segment_load_commands.size();
         mach_header.flags = 0;
         mach_header.reserved = 0;
-        ThreadList &thread_list = process_sp->GetThreadList();
-        const uint32_t num_threads = thread_list.GetSize();
+        std::vector<ThreadSP> thread_list =
+            process_sp->CalculateCoreFileThreadList(options);
+        const uint32_t num_threads = thread_list.size();
 
         // Make an array of LC_THREAD data items. Each one contains the
         // contents of the LC_THREAD load command. The data doesn't contain
@@ -6622,7 +6623,7 @@ bool ObjectFileMachO::SaveCore(const lldb::ProcessSP &process_sp,
           LC_THREAD_data.SetByteOrder(byte_order);
         }
         for (uint32_t thread_idx = 0; thread_idx < num_threads; ++thread_idx) {
-          ThreadSP thread_sp(thread_list.GetThreadAtIndex(thread_idx));
+          ThreadSP thread_sp = thread_list.at(thread_idx);
           if (thread_sp) {
             switch (mach_header.cputype) {
             case llvm::MachO::CPU_TYPE_ARM64:
@@ -6730,7 +6731,7 @@ bool ObjectFileMachO::SaveCore(const lldb::ProcessSP &process_sp,
         StructuredData::ArraySP threads(
             std::make_shared<StructuredData::Array>());
         for (uint32_t thread_idx = 0; thread_idx < num_threads; ++thread_idx) {
-          ThreadSP thread_sp(thread_list.GetThreadAtIndex(thread_idx));
+          ThreadSP thread_sp = thread_list.at(thread_idx);
           StructuredData::DictionarySP thread(
               std::make_shared<StructuredData::Dictionary>());
           thread->AddIntegerItem("thread_id", thread_sp->GetID());

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

@@ -588,12 +588,13 @@ Status MinidumpFileBuilder::FixThreadStacks() {
 
 Status MinidumpFileBuilder::AddThreadList() {
   constexpr size_t minidump_thread_size = sizeof(llvm::minidump::Thread);
-  lldb_private::ThreadList thread_list = m_process_sp->GetThreadList();
+  std::vector<ThreadSP> thread_list =
+      m_process_sp->CalculateCoreFileThreadList(m_save_core_options);
 
   // size of the entire thread stream consists of:
   // number of threads and threads array
   size_t thread_stream_size = sizeof(llvm::support::ulittle32_t) +
-                              thread_list.GetSize() * minidump_thread_size;
+                              thread_list.size() * minidump_thread_size;
   // save for the ability to set up RVA
   size_t size_before = GetCurrentDataEndOffset();
   Status error;
@@ -602,17 +603,17 @@ Status MinidumpFileBuilder::AddThreadList() {
     return error;
 
   llvm::support::ulittle32_t thread_count =
-      static_cast<llvm::support::ulittle32_t>(thread_list.GetSize());
+      static_cast<llvm::support::ulittle32_t>(thread_list.size());
   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();
+  const uint32_t num_threads = thread_list.size();
   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));
+    ThreadSP thread_sp = thread_list.at(thread_idx);
     RegisterContextSP reg_ctx_sp(thread_sp->GetRegisterContext());
 
     if (!reg_ctx_sp) {
@@ -819,7 +820,7 @@ Status MinidumpFileBuilder::AddLinuxFileStreams() {
   return error;
 }
 
-Status MinidumpFileBuilder::AddMemoryList(SaveCoreStyle core_style) {
+Status MinidumpFileBuilder::AddMemoryList() {
   Status error;
 
   // We first save the thread stacks to ensure they fit in the first UINT32_MAX
@@ -828,18 +829,26 @@ Status MinidumpFileBuilder::AddMemoryList(SaveCoreStyle core_style) {
   // in accessible with a 32 bit offset.
   Process::CoreFileMemoryRanges ranges_32;
   Process::CoreFileMemoryRanges ranges_64;
-  error = m_process_sp->CalculateCoreFileSaveRanges(
-      SaveCoreStyle::eSaveCoreStackOnly, ranges_32);
+  Process::CoreFileMemoryRanges all_core_memory_ranges;
+  error = m_process_sp->CalculateCoreFileSaveRanges(m_save_core_options,
+                                                    all_core_memory_ranges);
   if (error.Fail())
     return error;
 
-  // Calculate totalsize including the current offset.
+  // Start by saving all of the stacks and ensuring they fit under the 32b
+  // limit.
   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());
-    total_size += core_range.range.size();
+  auto iterator = all_core_memory_ranges.begin();
+  while (iterator != all_core_memory_ranges.end()) {
+    if (m_saved_stack_ranges.count(iterator->range.start()) > 0) {
+      // We don't save stacks twice.
+      ranges_32.push_back(*iterator);
+      total_size +=
+          iterator->range.size() + sizeof(llvm::minidump::MemoryDescriptor);
+      iterator = all_core_memory_ranges.erase(iterator);
+    } else {
+      iterator++;
+    }
   }
 
   if (total_size >= UINT32_MAX) {
@@ -849,31 +858,20 @@ Status MinidumpFileBuilder::AddMemoryList(SaveCoreStyle core_style) {
     return error;
   }
 
-  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;
-  }
-
   // After saving the stacks, we start packing as much as we can into 32b.
   // We apply a generous padding here so that the Directory, MemoryList and
   // Memory64List sections all begin in 32b addressable space.
   // Then anything overflow extends into 64b addressable space.
   // All core 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);
+    total_size += 256 + (all_core_memory_ranges.size() *
+                         sizeof(llvm::minidump::MemoryDescriptor_64));
 
   for (const auto &core_range : all_core_memory_ranges) {
     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;
-
+    // 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) {
       ranges_32.push_back(core_range);
       total_size += range_size;

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

@@ -75,8 +75,10 @@ lldb_private::Status WriteString(const std::string &to_write,
 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)){};
+                      const lldb::ProcessSP &process_sp,
+                      const lldb_private::SaveCoreOptions &save_core_options)
+      : m_process_sp(process_sp), m_core_file(std::move(core_file)),
+        m_save_core_options(save_core_options){};
 
   MinidumpFileBuilder(const MinidumpFileBuilder &) = delete;
   MinidumpFileBuilder &operator=(const MinidumpFileBuilder &) = delete;
@@ -103,7 +105,7 @@ class MinidumpFileBuilder {
   // Add Exception streams for any threads that stopped with exceptions.
   lldb_private::Status AddExceptions();
   // Add MemoryList stream, containing dumps of important memory segments
-  lldb_private::Status AddMemoryList(lldb::SaveCoreStyle core_style);
+  lldb_private::Status AddMemoryList();
   // Add MiscInfo stream, mainly providing ProcessId
   lldb_private::Status AddMiscInfo();
   // Add informative files about a Linux process
@@ -163,7 +165,9 @@ class MinidumpFileBuilder {
   // to duplicate it in the exception data.
   std::unordered_map<lldb::tid_t, llvm::minidump::LocationDescriptor>
       m_tid_to_reg_ctx;
+  std::unordered_set<lldb::addr_t> m_saved_stack_ranges;
   lldb::FileUP m_core_file;
+  lldb_private::SaveCoreOptions m_save_core_options;
 };
 
 #endif // LLDB_SOURCE_PLUGINS_OBJECTFILE_MINIDUMP_MINIDUMPFILEBUILDER_H

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

@@ -74,7 +74,8 @@ bool ObjectFileMinidump::SaveCore(const lldb::ProcessSP &process_sp,
     error = maybe_core_file.takeError();
     return false;
   }
-  MinidumpFileBuilder builder(std::move(maybe_core_file.get()), process_sp);
+  MinidumpFileBuilder builder(std::move(maybe_core_file.get()), process_sp,
+                              options);
 
   Log *log = GetLog(LLDBLog::Object);
   error = builder.AddHeaderAndCalculateDirectories();
@@ -121,7 +122,7 @@ bool ObjectFileMinidump::SaveCore(const lldb::ProcessSP &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.AddMemoryList(core_style);
+  error = builder.AddMemoryList();
   if (error.Fail()) {
     LLDB_LOGF(log, "AddMemoryList failed: %s", error.AsCString());
     return false;

lldb/source/Symbol/SaveCoreOptions.cpp

@@ -77,6 +77,9 @@ int64_t SaveCoreOptions::GetThreadAtIndex(size_t index) const {
 }
 
 bool SaveCoreOptions::ShouldSaveThread(lldb::tid_t tid) const {
+  // If the user specified no threads to save, then we save all threads.
+  if (m_threads_to_save.empty())
+    return true;
   return m_threads_to_save.count(tid) > 0;
 }
 

lldb/source/Target/Process.cpp

@@ -6532,8 +6532,9 @@ static void AddRegion(const MemoryRegionInfo &region, bool try_dirty_pages,
 }
 
 static void SaveOffRegionsWithStackPointers(
-    Process &process, const MemoryRegionInfos &regions,
-    Process::CoreFileMemoryRanges &ranges, std::set<addr_t> &stack_ends) {
+    Process &process, const SaveCoreOptions &core_options,
+    const MemoryRegionInfos &regions, Process::CoreFileMemoryRanges &ranges,
+    std::set<addr_t> &stack_ends) {
   const bool try_dirty_pages = true;
 
   // Before we take any dump, we want to save off the used portions of the
@@ -6555,10 +6556,16 @@ static void SaveOffRegionsWithStackPointers(
     if (process.GetMemoryRegionInfo(sp, sp_region).Success()) {
       const size_t stack_head = (sp - red_zone);
       const size_t stack_size = sp_region.GetRange().GetRangeEnd() - stack_head;
+      // Even if the SaveCoreOption doesn't want us to save the stack
+      // we still need to populate the stack_ends set so it doesn't get saved
+      // off in other calls
       sp_region.GetRange().SetRangeBase(stack_head);
       sp_region.GetRange().SetByteSize(stack_size);
       stack_ends.insert(sp_region.GetRange().GetRangeEnd());
-      AddRegion(sp_region, try_dirty_pages, ranges);
+      // This will return true if the threadlist the user specified is empty,
+      // or contains the thread id from thread_sp.
+      if (core_options.ShouldSaveThread(thread_sp->GetID()))
+        AddRegion(sp_region, try_dirty_pages, ranges);
     }
   }
 }
@@ -6627,10 +6634,11 @@ static void GetCoreFileSaveRangesStackOnly(
   }
 }
 
-Status Process::CalculateCoreFileSaveRanges(lldb::SaveCoreStyle core_style,
+Status Process::CalculateCoreFileSaveRanges(const SaveCoreOptions &options,
                                             CoreFileMemoryRanges &ranges) {
   lldb_private::MemoryRegionInfos regions;
   Status err = GetMemoryRegions(regions);
+  SaveCoreStyle core_style = options.GetStyle();
   if (err.Fail())
     return err;
   if (regions.empty())
@@ -6640,7 +6648,7 @@ Status Process::CalculateCoreFileSaveRanges(lldb::SaveCoreStyle core_style,
                   "eSaveCoreUnspecified");
 
   std::set<addr_t> stack_ends;
-  SaveOffRegionsWithStackPointers(*this, regions, ranges, stack_ends);
+  SaveOffRegionsWithStackPointers(*this, options, regions, ranges, stack_ends);
 
   switch (core_style) {
   case eSaveCoreUnspecified:
@@ -6668,16 +6676,16 @@ Status Process::CalculateCoreFileSaveRanges(lldb::SaveCoreStyle core_style,
   return Status(); // Success!
 }
 
-ThreadCollection::ThreadIterable
-Process::CalculateCoreFileThreadList(SaveCoreOptions &core_options) {
-  ThreadCollection thread_list;
+std::vector<ThreadSP>
+Process::CalculateCoreFileThreadList(const SaveCoreOptions &core_options) {
+  std::vector<ThreadSP> thread_list;
   for (const auto &thread : m_thread_list.Threads()) {
     if (core_options.ShouldSaveThread(thread->GetID())) {
-      thread_list.AddThread(thread);
+      thread_list.push_back(thread);
     }
   }
 
-  return thread_list.Threads();
+  return thread_list;
 }
 
 void Process::SetAddressableBitMasks(AddressableBits bit_masks) {