[lldb] [llvm] [DRAFT][LLDB][Minidump] Support minidumps where there are multiple exception streams (PR #97470)

Jacob Lalonde via llvm-commits llvm-commits at lists.llvm.org
Tue Jul 2 12:56:28 PDT 2024


https://github.com/Jlalond created https://github.com/llvm/llvm-project/pull/97470

Currently, LLDB assumes all minidumps will have unique sections. This is intuitive because almost all of the minidump sections are themselves lists. Exceptions including Signals are unique in that they are all individual sections with their own directory. 

This means LLDB fails to load minidumps with multiple exceptions due to them not being unique. This behavior is erroneous and this PR introduces support for an arbitrary number of exception streams. Additionally, stop info was calculated on for a single thread before, and now we properly support mapping exceptions to threads.

This PR is starting in DRAFT because implementing testing is still required.

>From 7e41ca79a09d67ff7bb76a9d95dda4e7ccfdba8b Mon Sep 17 00:00:00 2001
From: Jacob Lalonde <jalalonde at fb.com>
Date: Tue, 2 Jul 2024 12:41:02 -0700
Subject: [PATCH 1/2] Add support to read multiple exception streams in
 minidumps

---
 .../Process/minidump/MinidumpParser.cpp       |  11 +-
 .../Plugins/Process/minidump/MinidumpParser.h |   2 +-
 .../Process/minidump/ProcessMinidump.cpp      | 122 ++++++++++--------
 .../Process/minidump/ProcessMinidump.h        |   2 +-
 .../Process/minidump/ThreadMinidump.cpp       |  14 +-
 .../Plugins/Process/minidump/ThreadMinidump.h |   3 +-
 .../Process/minidump/MinidumpParserTest.cpp   |  11 +-
 llvm/include/llvm/Object/Minidump.h           |  34 ++++-
 llvm/lib/Object/Minidump.cpp                  |  37 ++++++
 llvm/lib/ObjectYAML/MinidumpYAML.cpp          |   4 +-
 10 files changed, 162 insertions(+), 78 deletions(-)

diff --git a/lldb/source/Plugins/Process/minidump/MinidumpParser.cpp b/lldb/source/Plugins/Process/minidump/MinidumpParser.cpp
index be9fae938e227..ac487a5ed0c0a 100644
--- a/lldb/source/Plugins/Process/minidump/MinidumpParser.cpp
+++ b/lldb/source/Plugins/Process/minidump/MinidumpParser.cpp
@@ -408,7 +408,7 @@ std::vector<const minidump::Module *> MinidumpParser::GetFilteredModuleList() {
         continue;
       }
       // This module has been seen. Modules are sometimes mentioned multiple
-      // times when they are mapped discontiguously, so find the module with
+      // times when they are mapped discontiguously, so find the module with 
       // the lowest "base_of_image" and use that as the filtered module.
       if (module.BaseOfImage < dup_module->BaseOfImage)
         filtered_modules[iter->second] = &module;
@@ -417,14 +417,15 @@ std::vector<const minidump::Module *> MinidumpParser::GetFilteredModuleList() {
   return filtered_modules;
 }
 
-const minidump::ExceptionStream *MinidumpParser::GetExceptionStream() {
-  auto ExpectedStream = GetMinidumpFile().getExceptionStream();
+const std::vector<minidump::ExceptionStream> MinidumpParser::GetExceptionStreams() {
+  auto ExpectedStream = GetMinidumpFile().getExceptionStreams();
   if (ExpectedStream)
-    return &*ExpectedStream;
+    return ExpectedStream.get();
 
   LLDB_LOG_ERROR(GetLog(LLDBLog::Process), ExpectedStream.takeError(),
                  "Failed to read minidump exception stream: {0}");
-  return nullptr;
+  // return empty on failure.
+  return std::vector<minidump::ExceptionStream>();
 }
 
 std::optional<minidump::Range>
diff --git a/lldb/source/Plugins/Process/minidump/MinidumpParser.h b/lldb/source/Plugins/Process/minidump/MinidumpParser.h
index 050ba086f46f5..e552c7210e330 100644
--- a/lldb/source/Plugins/Process/minidump/MinidumpParser.h
+++ b/lldb/source/Plugins/Process/minidump/MinidumpParser.h
@@ -82,7 +82,7 @@ class MinidumpParser {
   // have the same name, it keeps the copy with the lowest load address.
   std::vector<const minidump::Module *> GetFilteredModuleList();
 
-  const llvm::minidump::ExceptionStream *GetExceptionStream();
+  const std::vector<llvm::minidump::ExceptionStream> GetExceptionStreams();
 
   std::optional<Range> FindMemoryRange(lldb::addr_t addr);
 
diff --git a/lldb/source/Plugins/Process/minidump/ProcessMinidump.cpp b/lldb/source/Plugins/Process/minidump/ProcessMinidump.cpp
index 13599f4a1553f..9f707c0d8a7a7 100644
--- a/lldb/source/Plugins/Process/minidump/ProcessMinidump.cpp
+++ b/lldb/source/Plugins/Process/minidump/ProcessMinidump.cpp
@@ -39,6 +39,7 @@
 
 #include <memory>
 #include <optional>
+#include <iostream>
 
 using namespace lldb;
 using namespace lldb_private;
@@ -157,7 +158,7 @@ ProcessMinidump::ProcessMinidump(lldb::TargetSP target_sp,
                                  const FileSpec &core_file,
                                  DataBufferSP core_data)
     : PostMortemProcess(target_sp, listener_sp, core_file),
-      m_core_data(std::move(core_data)), m_active_exception(nullptr),
+      m_core_data(std::move(core_data)),
       m_is_wow64(false) {}
 
 ProcessMinidump::~ProcessMinidump() {
@@ -209,7 +210,19 @@ Status ProcessMinidump::DoLoadCore() {
   GetTarget().SetArchitecture(arch, true /*set_platform*/);
 
   m_thread_list = m_minidump_parser->GetThreads();
-  m_active_exception = m_minidump_parser->GetExceptionStream();
+  std::vector<minidump::ExceptionStream> exception_streams = m_minidump_parser->GetExceptionStreams();
+  for (const auto &exception_stream : exception_streams) {
+    if (m_exceptions_by_tid.count(exception_stream.ThreadId) > 0) {
+      // We only cast to avoid the warning around converting little endian in printf.
+      error.SetErrorStringWithFormat("duplicate exception stream for tid %" PRIu32, (uint32_t)exception_stream.ThreadId);
+      return error;
+    } else 
+      m_exceptions_by_tid[exception_stream.ThreadId] = exception_stream;
+
+
+    std::cout << "Adding Exception Stream # " << (uint32_t)exception_stream.ThreadId << std::endl;
+    std::cout << "Added index " << (uint32_t)m_exceptions_by_tid[exception_stream.ThreadId].ExceptionRecord.ExceptionCode << std::endl;
+  }
 
   SetUnixSignals(UnixSignals::Create(GetArchitecture()));
 
@@ -232,60 +245,59 @@ Status ProcessMinidump::DoDestroy() { return Status(); }
 
 void ProcessMinidump::RefreshStateAfterStop() {
 
-  if (!m_active_exception)
-    return;
-
-  constexpr uint32_t BreakpadDumpRequested = 0xFFFFFFFF;
-  if (m_active_exception->ExceptionRecord.ExceptionCode ==
-      BreakpadDumpRequested) {
-    // This "ExceptionCode" value is a sentinel that is sometimes used
-    // when generating a dump for a process that hasn't crashed.
-
-    // TODO: The definition and use of this "dump requested" constant
-    // in Breakpad are actually Linux-specific, and for similar use
-    // cases on Mac/Windows it defines different constants, referring
-    // to them as "simulated" exceptions; consider moving this check
-    // down to the OS-specific paths and checking each OS for its own
-    // constant.
-    return;
-  }
+  for (auto &[_, exception_stream] : m_exceptions_by_tid) {
+    constexpr uint32_t BreakpadDumpRequested = 0xFFFFFFFF;
+    if (exception_stream.ExceptionRecord.ExceptionCode ==
+        BreakpadDumpRequested) {
+      // This "ExceptionCode" value is a sentinel that is sometimes used
+      // when generating a dump for a process that hasn't crashed.
+
+      // TODO: The definition and use of this "dump requested" constant
+      // in Breakpad are actually Linux-specific, and for similar use
+      // cases on Mac/Windows it defines different constants, referring
+      // to them as "simulated" exceptions; consider moving this check
+      // down to the OS-specific paths and checking each OS for its own
+      // constant.
+      return;
+    }
 
-  lldb::StopInfoSP stop_info;
-  lldb::ThreadSP stop_thread;
+    lldb::StopInfoSP stop_info;
+    lldb::ThreadSP stop_thread;
 
-  Process::m_thread_list.SetSelectedThreadByID(m_active_exception->ThreadId);
-  stop_thread = Process::m_thread_list.GetSelectedThread();
-  ArchSpec arch = GetArchitecture();
+    Process::m_thread_list.SetSelectedThreadByID(exception_stream.ThreadId);
+    stop_thread = Process::m_thread_list.GetSelectedThread();
+    ArchSpec arch = GetArchitecture();
 
-  if (arch.GetTriple().getOS() == llvm::Triple::Linux) {
-    uint32_t signo = m_active_exception->ExceptionRecord.ExceptionCode;
+    if (arch.GetTriple().getOS() == llvm::Triple::Linux) {
+      uint32_t signo = exception_stream.ExceptionRecord.ExceptionCode;
+      std::cout << "Thread Id : " << exception_stream.ThreadId << " has signal " << signo << std::endl;
+      if (signo == 0) {
+        // No stop.
+        return;
+      }
 
-    if (signo == 0) {
-      // No stop.
-      return;
+      stop_info = StopInfo::CreateStopReasonWithSignal(
+          *stop_thread, signo);
+    } else if (arch.GetTriple().getVendor() == llvm::Triple::Apple) {
+      stop_info = StopInfoMachException::CreateStopReasonWithMachException(
+          *stop_thread, exception_stream.ExceptionRecord.ExceptionCode, 2,
+          exception_stream.ExceptionRecord.ExceptionFlags,
+          exception_stream.ExceptionRecord.ExceptionAddress, 0);
+    } else {
+      std::string desc;
+      llvm::raw_string_ostream desc_stream(desc);
+      desc_stream << "Exception "
+                  << llvm::format_hex(
+                        exception_stream.ExceptionRecord.ExceptionCode, 8)
+                  << " encountered at address "
+                  << llvm::format_hex(
+                        exception_stream.ExceptionRecord.ExceptionAddress, 8);
+      stop_info = StopInfo::CreateStopReasonWithException(
+          *stop_thread, desc_stream.str().c_str());
     }
 
-    stop_info = StopInfo::CreateStopReasonWithSignal(
-        *stop_thread, signo);
-  } else if (arch.GetTriple().getVendor() == llvm::Triple::Apple) {
-    stop_info = StopInfoMachException::CreateStopReasonWithMachException(
-        *stop_thread, m_active_exception->ExceptionRecord.ExceptionCode, 2,
-        m_active_exception->ExceptionRecord.ExceptionFlags,
-        m_active_exception->ExceptionRecord.ExceptionAddress, 0);
-  } else {
-    std::string desc;
-    llvm::raw_string_ostream desc_stream(desc);
-    desc_stream << "Exception "
-                << llvm::format_hex(
-                       m_active_exception->ExceptionRecord.ExceptionCode, 8)
-                << " encountered at address "
-                << llvm::format_hex(
-                       m_active_exception->ExceptionRecord.ExceptionAddress, 8);
-    stop_info = StopInfo::CreateStopReasonWithException(
-        *stop_thread, desc_stream.str().c_str());
-  }
-
-  stop_thread->SetStopInfo(stop_info);
+    stop_thread->SetStopInfo(stop_info);
+  }
 }
 
 bool ProcessMinidump::IsAlive() { return true; }
@@ -386,19 +398,21 @@ bool ProcessMinidump::DoUpdateThreadList(ThreadList &old_thread_list,
   for (const minidump::Thread &thread : m_thread_list) {
     LocationDescriptor context_location = thread.Context;
 
+    std::optional<minidump::Exception> exception;
     // If the minidump contains an exception context, use it
-    if (m_active_exception != nullptr &&
-        m_active_exception->ThreadId == thread.ThreadId) {
-      context_location = m_active_exception->ThreadContext;
+    if (m_exceptions_by_tid.count(thread.ThreadId) > 0) {
+      context_location = m_exceptions_by_tid[thread.ThreadId].ThreadContext;
+      exception = m_exceptions_by_tid[thread.ThreadId].ExceptionRecord;
     }
 
+
     llvm::ArrayRef<uint8_t> context;
     if (!m_is_wow64)
       context = m_minidump_parser->GetThreadContext(context_location);
     else
       context = m_minidump_parser->GetThreadContextWow64(thread);
 
-    lldb::ThreadSP thread_sp(new ThreadMinidump(*this, thread, context));
+    lldb::ThreadSP thread_sp(new ThreadMinidump(*this, thread, context, exception));
     new_thread_list.AddThread(thread_sp);
   }
   return new_thread_list.GetSize(false) > 0;
diff --git a/lldb/source/Plugins/Process/minidump/ProcessMinidump.h b/lldb/source/Plugins/Process/minidump/ProcessMinidump.h
index 3f3123a0a8b5d..0379de7bee5e6 100644
--- a/lldb/source/Plugins/Process/minidump/ProcessMinidump.h
+++ b/lldb/source/Plugins/Process/minidump/ProcessMinidump.h
@@ -109,7 +109,7 @@ class ProcessMinidump : public PostMortemProcess {
 private:
   lldb::DataBufferSP m_core_data;
   llvm::ArrayRef<minidump::Thread> m_thread_list;
-  const minidump::ExceptionStream *m_active_exception;
+  std::unordered_map<uint32_t, minidump::ExceptionStream> m_exceptions_by_tid;
   lldb::CommandObjectSP m_command_sp;
   bool m_is_wow64;
   std::optional<MemoryRegionInfos> m_memory_regions;
diff --git a/lldb/source/Plugins/Process/minidump/ThreadMinidump.cpp b/lldb/source/Plugins/Process/minidump/ThreadMinidump.cpp
index 1fbc52815238b..3cc154f1eac47 100644
--- a/lldb/source/Plugins/Process/minidump/ThreadMinidump.cpp
+++ b/lldb/source/Plugins/Process/minidump/ThreadMinidump.cpp
@@ -34,9 +34,9 @@ using namespace lldb_private;
 using namespace minidump;
 
 ThreadMinidump::ThreadMinidump(Process &process, const minidump::Thread &td,
-                               llvm::ArrayRef<uint8_t> gpregset_data)
+                               llvm::ArrayRef<uint8_t> gpregset_data, std::optional<minidump::Exception> exception)
     : Thread(process, td.ThreadId), m_thread_reg_ctx_sp(),
-      m_gpregset_data(gpregset_data) {}
+      m_gpregset_data(gpregset_data), m_exception(exception) {}
 
 ThreadMinidump::~ThreadMinidump() = default;
 
@@ -115,4 +115,12 @@ ThreadMinidump::CreateRegisterContextForFrame(StackFrame *frame) {
   return reg_ctx_sp;
 }
 
-bool ThreadMinidump::CalculateStopInfo() { return false; }
+bool ThreadMinidump::CalculateStopInfo() {
+  if (!m_exception)
+    return false;
+
+  minidump::Exception thread_exception = m_exception.value();
+  SetStopInfo(StopInfo::CreateStopReasonWithSignal(
+      *this, thread_exception.ExceptionCode, /*description=*/nullptr, thread_exception.ExceptionCode));
+  return true;
+}
diff --git a/lldb/source/Plugins/Process/minidump/ThreadMinidump.h b/lldb/source/Plugins/Process/minidump/ThreadMinidump.h
index aed7cfbc1b161..cc336ad2d7b3d 100644
--- a/lldb/source/Plugins/Process/minidump/ThreadMinidump.h
+++ b/lldb/source/Plugins/Process/minidump/ThreadMinidump.h
@@ -21,7 +21,7 @@ namespace minidump {
 class ThreadMinidump : public Thread {
 public:
   ThreadMinidump(Process &process, const minidump::Thread &td,
-                 llvm::ArrayRef<uint8_t> gpregset_data);
+                 llvm::ArrayRef<uint8_t> gpregset_data, std::optional<minidump::Exception> exception);
 
   ~ThreadMinidump() override;
 
@@ -35,6 +35,7 @@ class ThreadMinidump : public Thread {
 protected:
   lldb::RegisterContextSP m_thread_reg_ctx_sp;
   llvm::ArrayRef<uint8_t> m_gpregset_data;
+  std::optional<minidump::Exception> m_exception;
 
   bool CalculateStopInfo() override;
 };
diff --git a/lldb/unittests/Process/minidump/MinidumpParserTest.cpp b/lldb/unittests/Process/minidump/MinidumpParserTest.cpp
index 632a7fd4e4f8f..e5355e07e41fc 100644
--- a/lldb/unittests/Process/minidump/MinidumpParserTest.cpp
+++ b/lldb/unittests/Process/minidump/MinidumpParserTest.cpp
@@ -251,10 +251,13 @@ TEST_F(MinidumpParserTest, GetFilteredModuleList) {
 
 TEST_F(MinidumpParserTest, GetExceptionStream) {
   SetUpData("linux-x86_64.dmp");
-  const llvm::minidump::ExceptionStream *exception_stream =
-      parser->GetExceptionStream();
-  ASSERT_NE(nullptr, exception_stream);
-  ASSERT_EQ(11UL, exception_stream->ExceptionRecord.ExceptionCode);
+  llvm::Expected<std::vector<ExceptionStream>> exception_stream =
+      parser->GetExceptionStreams();
+  // LLVM::Expected has an explicit bool operator that determines if 
+  // the expected value is an error or not.
+  ASSERT_TRUE((bool)exception_stream);
+  ASSERT_EQ(1UL, exception_stream->size());
+  ASSERT_EQ(11UL, exception_stream.get()[0].ExceptionRecord.ExceptionCode);
 }
 
 void check_mem_range_exists(MinidumpParser &parser, const uint64_t range_start,
diff --git a/llvm/include/llvm/Object/Minidump.h b/llvm/include/llvm/Object/Minidump.h
index e45d4de0090de..87fdc555fb1ac 100644
--- a/llvm/include/llvm/Object/Minidump.h
+++ b/llvm/include/llvm/Object/Minidump.h
@@ -82,15 +82,22 @@ class MinidumpFile : public Binary {
     return getListStream<minidump::Thread>(minidump::StreamType::ThreadList);
   }
 
-  /// Returns the contents of the Exception stream.  An error is returned if the
-  /// file does not contain this stream, or the stream is smaller than the size
-  /// of the ExceptionStream structure.  The internal consistency of the stream
-  /// is not checked in any way.
-  Expected<const minidump::ExceptionStream &> getExceptionStream() const {
-    return getStream<minidump::ExceptionStream>(
-        minidump::StreamType::Exception);
+  /// Returns the contents of the Exception stream. An error is returned if the 
+  /// associated stream is smaller than the size of the ExceptionStream structure.  
+  /// Or the directory supplied is not of kind exception stream.
+  Expected<minidump::ExceptionStream> getExceptionStream(minidump::Directory Directory) const {
+    if (Directory.Type != minidump::StreamType::Exception) {
+      return createError("Not an exception stream");
+    }
+
+    return getStreamFromDirectory<minidump::ExceptionStream>(Directory);
   }
 
+  /// Returns the contents of the Exception streams.  An error is returned if
+  /// any of the streams are smaller than the size of the ExceptionStream structure.  
+  /// The internal consistency of the stream is not checked in any way.
+  Expected<std::vector<minidump::ExceptionStream>> getExceptionStreams() const;
+
   /// Returns the list of descriptors embedded in the MemoryList stream. The
   /// descriptors provide the content of interesting regions of memory at the
   /// time the minidump was taken. An error is returned if the file does not
@@ -172,6 +179,11 @@ class MinidumpFile : public Binary {
     return arrayRefFromStringRef(Data.getBuffer());
   }
 
+  /// Return the stream of the given type, cast to the appropriate type. Checks
+  /// that the stream is large enough to hold an object of this type.
+  template <typename T>
+  Expected<const T &> getStreamFromDirectory(minidump::Directory Directory) const;
+
   /// Return the stream of the given type, cast to the appropriate type. Checks
   /// that the stream is large enough to hold an object of this type.
   template <typename T>
@@ -187,6 +199,14 @@ class MinidumpFile : public Binary {
   DenseMap<minidump::StreamType, std::size_t> StreamMap;
 };
 
+template <typename T>
+Expected<const T &> MinidumpFile::getStreamFromDirectory(minidump::Directory Directory) const {
+  ArrayRef<uint8_t> Stream = getRawStream(Directory);
+  if (Stream.size() >= sizeof(T))
+    return *reinterpret_cast<const T *>(Stream.data());
+  return createEOFError();
+}
+
 template <typename T>
 Expected<const T &> MinidumpFile::getStream(minidump::StreamType Type) const {
   if (std::optional<ArrayRef<uint8_t>> Stream = getRawStream(Type)) {
diff --git a/llvm/lib/Object/Minidump.cpp b/llvm/lib/Object/Minidump.cpp
index 6febff89ac519..d366b4d3d579e 100644
--- a/llvm/lib/Object/Minidump.cpp
+++ b/llvm/lib/Object/Minidump.cpp
@@ -9,6 +9,7 @@
 #include "llvm/Object/Minidump.h"
 #include "llvm/Object/Error.h"
 #include "llvm/Support/ConvertUTF.h"
+#include <iostream>
 
 using namespace llvm;
 using namespace llvm::object;
@@ -53,6 +54,33 @@ Expected<std::string> MinidumpFile::getString(size_t Offset) const {
   return Result;
 }
 
+Expected<std::vector<minidump::ExceptionStream>> MinidumpFile::getExceptionStreams() const {
+  // Scan the directories for exceptions first
+  std::vector<Directory> exceptionStreams;
+  for (const auto &directory : Streams) {
+    if (directory.Type == StreamType::Exception)
+      exceptionStreams.push_back(directory);
+  }
+
+  if (exceptionStreams.empty())
+    return createError("No exception streams found");
+
+  std::vector<minidump::ExceptionStream> exceptionStreamList;
+  for (const auto &exceptionStream : exceptionStreams) {
+    llvm::Expected<minidump::ExceptionStream> ExpectedStream = getStreamFromDirectory<minidump::ExceptionStream>(exceptionStream);
+    if (!ExpectedStream)
+      return ExpectedStream.takeError();
+
+    std::cout << "Adding Exception Stream # " << exceptionStreamList.size() << std::endl;
+    std::cout << "Thread Id : " << ExpectedStream->ThreadId << std::endl;
+    std::cout << "Exception Code : " << ExpectedStream.get().ExceptionRecord.ExceptionCode << std::endl;
+    exceptionStreamList.push_back(ExpectedStream.get());
+    assert(exceptionStreamList.back().ThreadId == ExpectedStream->ThreadId);
+  }
+
+  return exceptionStreamList;
+}
+
 Expected<iterator_range<MinidumpFile::MemoryInfoIterator>>
 MinidumpFile::getMemoryInfoList() const {
   std::optional<ArrayRef<uint8_t>> Stream =
@@ -127,6 +155,7 @@ MinidumpFile::create(MemoryBufferRef Source) {
     return ExpectedStreams.takeError();
 
   DenseMap<StreamType, std::size_t> StreamMap;
+  std::vector<std::size_t> ExceptionStreams;
   for (const auto &StreamDescriptor : llvm::enumerate(*ExpectedStreams)) {
     StreamType Type = StreamDescriptor.value().Type;
     const LocationDescriptor &Loc = StreamDescriptor.value().Location;
@@ -142,6 +171,14 @@ MinidumpFile::create(MemoryBufferRef Source) {
       continue;
     }
 
+    // We treat exceptions differently here because the LLDB minidump
+    // makes some assumptions about uniqueness, all the streams other than exceptions
+    // are lists. But exceptions are not a list, they are single streams that point back to their thread
+    // So we will omit them here, and will find them when needed in the MinidumpFile.
+    if (Type == StreamType::Exception) {
+      continue;
+    }
+
     if (Type == DenseMapInfo<StreamType>::getEmptyKey() ||
         Type == DenseMapInfo<StreamType>::getTombstoneKey())
       return createError("Cannot handle one of the minidump streams");
diff --git a/llvm/lib/ObjectYAML/MinidumpYAML.cpp b/llvm/lib/ObjectYAML/MinidumpYAML.cpp
index fdbd2d8e6dcbc..e00feceea57af 100644
--- a/llvm/lib/ObjectYAML/MinidumpYAML.cpp
+++ b/llvm/lib/ObjectYAML/MinidumpYAML.cpp
@@ -464,8 +464,8 @@ Stream::create(const Directory &StreamDesc, const object::MinidumpFile &File) {
   StreamKind Kind = getKind(StreamDesc.Type);
   switch (Kind) {
   case StreamKind::Exception: {
-    Expected<const minidump::ExceptionStream &> ExpectedExceptionStream =
-        File.getExceptionStream();
+    Expected<minidump::ExceptionStream> ExpectedExceptionStream =
+        File.getExceptionStream(StreamDesc);
     if (!ExpectedExceptionStream)
       return ExpectedExceptionStream.takeError();
     Expected<ArrayRef<uint8_t>> ExpectedThreadContext =

>From 317fa13f62cd729b7aa29412ab06179110f8878d Mon Sep 17 00:00:00 2001
From: Jacob Lalonde <jalalonde at fb.com>
Date: Tue, 2 Jul 2024 12:44:59 -0700
Subject: [PATCH 2/2] Remove debug std::cout code. Run git-clang-format

---
 .../Process/minidump/MinidumpParser.cpp       |  5 ++-
 .../Process/minidump/ProcessMinidump.cpp      | 37 +++++++++----------
 .../Process/minidump/ThreadMinidump.cpp       |  8 +++-
 .../Plugins/Process/minidump/ThreadMinidump.h |  3 +-
 .../Process/minidump/MinidumpParserTest.cpp   |  2 +-
 llvm/include/llvm/Object/Minidump.h           | 20 ++++++----
 llvm/lib/Object/Minidump.cpp                  | 17 ++++-----
 7 files changed, 49 insertions(+), 43 deletions(-)

diff --git a/lldb/source/Plugins/Process/minidump/MinidumpParser.cpp b/lldb/source/Plugins/Process/minidump/MinidumpParser.cpp
index ac487a5ed0c0a..c51fed8d91b07 100644
--- a/lldb/source/Plugins/Process/minidump/MinidumpParser.cpp
+++ b/lldb/source/Plugins/Process/minidump/MinidumpParser.cpp
@@ -408,7 +408,7 @@ std::vector<const minidump::Module *> MinidumpParser::GetFilteredModuleList() {
         continue;
       }
       // This module has been seen. Modules are sometimes mentioned multiple
-      // times when they are mapped discontiguously, so find the module with 
+      // times when they are mapped discontiguously, so find the module with
       // the lowest "base_of_image" and use that as the filtered module.
       if (module.BaseOfImage < dup_module->BaseOfImage)
         filtered_modules[iter->second] = &module;
@@ -417,7 +417,8 @@ std::vector<const minidump::Module *> MinidumpParser::GetFilteredModuleList() {
   return filtered_modules;
 }
 
-const std::vector<minidump::ExceptionStream> MinidumpParser::GetExceptionStreams() {
+const std::vector<minidump::ExceptionStream>
+MinidumpParser::GetExceptionStreams() {
   auto ExpectedStream = GetMinidumpFile().getExceptionStreams();
   if (ExpectedStream)
     return ExpectedStream.get();
diff --git a/lldb/source/Plugins/Process/minidump/ProcessMinidump.cpp b/lldb/source/Plugins/Process/minidump/ProcessMinidump.cpp
index 9f707c0d8a7a7..9a7e481b92796 100644
--- a/lldb/source/Plugins/Process/minidump/ProcessMinidump.cpp
+++ b/lldb/source/Plugins/Process/minidump/ProcessMinidump.cpp
@@ -39,7 +39,6 @@
 
 #include <memory>
 #include <optional>
-#include <iostream>
 
 using namespace lldb;
 using namespace lldb_private;
@@ -158,8 +157,7 @@ ProcessMinidump::ProcessMinidump(lldb::TargetSP target_sp,
                                  const FileSpec &core_file,
                                  DataBufferSP core_data)
     : PostMortemProcess(target_sp, listener_sp, core_file),
-      m_core_data(std::move(core_data)),
-      m_is_wow64(false) {}
+      m_core_data(std::move(core_data)), m_is_wow64(false) {}
 
 ProcessMinidump::~ProcessMinidump() {
   Clear();
@@ -210,18 +208,19 @@ Status ProcessMinidump::DoLoadCore() {
   GetTarget().SetArchitecture(arch, true /*set_platform*/);
 
   m_thread_list = m_minidump_parser->GetThreads();
-  std::vector<minidump::ExceptionStream> exception_streams = m_minidump_parser->GetExceptionStreams();
+  std::vector<minidump::ExceptionStream> exception_streams =
+      m_minidump_parser->GetExceptionStreams();
   for (const auto &exception_stream : exception_streams) {
-    if (m_exceptions_by_tid.count(exception_stream.ThreadId) > 0) {
-      // We only cast to avoid the warning around converting little endian in printf.
-      error.SetErrorStringWithFormat("duplicate exception stream for tid %" PRIu32, (uint32_t)exception_stream.ThreadId);
+    if (!m_exceptions_by_tid
+             .try_emplace(exception_stream.ThreadId, exception_stream)
+             .second) {
+      // We only cast to avoid the warning around converting little endian in
+      // printf.
+      error.SetErrorStringWithFormat(
+          "duplicate exception stream for tid %" PRIu32,
+          (uint32_t)exception_stream.ThreadId);
       return error;
-    } else 
-      m_exceptions_by_tid[exception_stream.ThreadId] = exception_stream;
-
-
-    std::cout << "Adding Exception Stream # " << (uint32_t)exception_stream.ThreadId << std::endl;
-    std::cout << "Added index " << (uint32_t)m_exceptions_by_tid[exception_stream.ThreadId].ExceptionRecord.ExceptionCode << std::endl;
+    }
   }
 
   SetUnixSignals(UnixSignals::Create(GetArchitecture()));
@@ -270,14 +269,12 @@ void ProcessMinidump::RefreshStateAfterStop() {
 
     if (arch.GetTriple().getOS() == llvm::Triple::Linux) {
       uint32_t signo = exception_stream.ExceptionRecord.ExceptionCode;
-      std::cout << "Thread Id : " << exception_stream.ThreadId << " has signal " << signo << std::endl;
       if (signo == 0) {
         // No stop.
         return;
       }
 
-      stop_info = StopInfo::CreateStopReasonWithSignal(
-          *stop_thread, signo);
+      stop_info = StopInfo::CreateStopReasonWithSignal(*stop_thread, signo);
     } else if (arch.GetTriple().getVendor() == llvm::Triple::Apple) {
       stop_info = StopInfoMachException::CreateStopReasonWithMachException(
           *stop_thread, exception_stream.ExceptionRecord.ExceptionCode, 2,
@@ -288,10 +285,10 @@ void ProcessMinidump::RefreshStateAfterStop() {
       llvm::raw_string_ostream desc_stream(desc);
       desc_stream << "Exception "
                   << llvm::format_hex(
-                        exception_stream.ExceptionRecord.ExceptionCode, 8)
+                         exception_stream.ExceptionRecord.ExceptionCode, 8)
                   << " encountered at address "
                   << llvm::format_hex(
-                        exception_stream.ExceptionRecord.ExceptionAddress, 8);
+                         exception_stream.ExceptionRecord.ExceptionAddress, 8);
       stop_info = StopInfo::CreateStopReasonWithException(
           *stop_thread, desc_stream.str().c_str());
     }
@@ -405,14 +402,14 @@ bool ProcessMinidump::DoUpdateThreadList(ThreadList &old_thread_list,
       exception = m_exceptions_by_tid[thread.ThreadId].ExceptionRecord;
     }
 
-
     llvm::ArrayRef<uint8_t> context;
     if (!m_is_wow64)
       context = m_minidump_parser->GetThreadContext(context_location);
     else
       context = m_minidump_parser->GetThreadContextWow64(thread);
 
-    lldb::ThreadSP thread_sp(new ThreadMinidump(*this, thread, context, exception));
+    lldb::ThreadSP thread_sp(
+        new ThreadMinidump(*this, thread, context, exception));
     new_thread_list.AddThread(thread_sp);
   }
   return new_thread_list.GetSize(false) > 0;
diff --git a/lldb/source/Plugins/Process/minidump/ThreadMinidump.cpp b/lldb/source/Plugins/Process/minidump/ThreadMinidump.cpp
index 3cc154f1eac47..a1d7da376b02b 100644
--- a/lldb/source/Plugins/Process/minidump/ThreadMinidump.cpp
+++ b/lldb/source/Plugins/Process/minidump/ThreadMinidump.cpp
@@ -34,7 +34,8 @@ using namespace lldb_private;
 using namespace minidump;
 
 ThreadMinidump::ThreadMinidump(Process &process, const minidump::Thread &td,
-                               llvm::ArrayRef<uint8_t> gpregset_data, std::optional<minidump::Exception> exception)
+                               llvm::ArrayRef<uint8_t> gpregset_data,
+                               std::optional<minidump::Exception> exception)
     : Thread(process, td.ThreadId), m_thread_reg_ctx_sp(),
       m_gpregset_data(gpregset_data), m_exception(exception) {}
 
@@ -115,12 +116,15 @@ ThreadMinidump::CreateRegisterContextForFrame(StackFrame *frame) {
   return reg_ctx_sp;
 }
 
+// This method doesn't end up getting called for minidump
+// because the stopinfo is set in `ProcessMinidump::RefreshStateAfterStop`
 bool ThreadMinidump::CalculateStopInfo() {
   if (!m_exception)
     return false;
 
   minidump::Exception thread_exception = m_exception.value();
   SetStopInfo(StopInfo::CreateStopReasonWithSignal(
-      *this, thread_exception.ExceptionCode, /*description=*/nullptr, thread_exception.ExceptionCode));
+      *this, thread_exception.ExceptionCode, /*description=*/nullptr,
+      thread_exception.ExceptionCode));
   return true;
 }
diff --git a/lldb/source/Plugins/Process/minidump/ThreadMinidump.h b/lldb/source/Plugins/Process/minidump/ThreadMinidump.h
index cc336ad2d7b3d..abde13ccb3f2a 100644
--- a/lldb/source/Plugins/Process/minidump/ThreadMinidump.h
+++ b/lldb/source/Plugins/Process/minidump/ThreadMinidump.h
@@ -21,7 +21,8 @@ namespace minidump {
 class ThreadMinidump : public Thread {
 public:
   ThreadMinidump(Process &process, const minidump::Thread &td,
-                 llvm::ArrayRef<uint8_t> gpregset_data, std::optional<minidump::Exception> exception);
+                 llvm::ArrayRef<uint8_t> gpregset_data,
+                 std::optional<minidump::Exception> exception);
 
   ~ThreadMinidump() override;
 
diff --git a/lldb/unittests/Process/minidump/MinidumpParserTest.cpp b/lldb/unittests/Process/minidump/MinidumpParserTest.cpp
index e5355e07e41fc..aa74e690d45a5 100644
--- a/lldb/unittests/Process/minidump/MinidumpParserTest.cpp
+++ b/lldb/unittests/Process/minidump/MinidumpParserTest.cpp
@@ -253,7 +253,7 @@ TEST_F(MinidumpParserTest, GetExceptionStream) {
   SetUpData("linux-x86_64.dmp");
   llvm::Expected<std::vector<ExceptionStream>> exception_stream =
       parser->GetExceptionStreams();
-  // LLVM::Expected has an explicit bool operator that determines if 
+  // LLVM::Expected has an explicit bool operator that determines if
   // the expected value is an error or not.
   ASSERT_TRUE((bool)exception_stream);
   ASSERT_EQ(1UL, exception_stream->size());
diff --git a/llvm/include/llvm/Object/Minidump.h b/llvm/include/llvm/Object/Minidump.h
index 87fdc555fb1ac..14fad92d1d2b6 100644
--- a/llvm/include/llvm/Object/Minidump.h
+++ b/llvm/include/llvm/Object/Minidump.h
@@ -82,10 +82,11 @@ class MinidumpFile : public Binary {
     return getListStream<minidump::Thread>(minidump::StreamType::ThreadList);
   }
 
-  /// Returns the contents of the Exception stream. An error is returned if the 
-  /// associated stream is smaller than the size of the ExceptionStream structure.  
-  /// Or the directory supplied is not of kind exception stream.
-  Expected<minidump::ExceptionStream> getExceptionStream(minidump::Directory Directory) const {
+  /// Returns the contents of the Exception stream. An error is returned if the
+  /// associated stream is smaller than the size of the ExceptionStream
+  /// structure. Or the directory supplied is not of kind exception stream.
+  Expected<minidump::ExceptionStream>
+  getExceptionStream(minidump::Directory Directory) const {
     if (Directory.Type != minidump::StreamType::Exception) {
       return createError("Not an exception stream");
     }
@@ -94,8 +95,9 @@ class MinidumpFile : public Binary {
   }
 
   /// Returns the contents of the Exception streams.  An error is returned if
-  /// any of the streams are smaller than the size of the ExceptionStream structure.  
-  /// The internal consistency of the stream is not checked in any way.
+  /// any of the streams are smaller than the size of the ExceptionStream
+  /// structure. The internal consistency of the stream is not checked in any
+  /// way.
   Expected<std::vector<minidump::ExceptionStream>> getExceptionStreams() const;
 
   /// Returns the list of descriptors embedded in the MemoryList stream. The
@@ -182,7 +184,8 @@ class MinidumpFile : public Binary {
   /// Return the stream of the given type, cast to the appropriate type. Checks
   /// that the stream is large enough to hold an object of this type.
   template <typename T>
-  Expected<const T &> getStreamFromDirectory(minidump::Directory Directory) const;
+  Expected<const T &>
+  getStreamFromDirectory(minidump::Directory Directory) const;
 
   /// Return the stream of the given type, cast to the appropriate type. Checks
   /// that the stream is large enough to hold an object of this type.
@@ -200,7 +203,8 @@ class MinidumpFile : public Binary {
 };
 
 template <typename T>
-Expected<const T &> MinidumpFile::getStreamFromDirectory(minidump::Directory Directory) const {
+Expected<const T &>
+MinidumpFile::getStreamFromDirectory(minidump::Directory Directory) const {
   ArrayRef<uint8_t> Stream = getRawStream(Directory);
   if (Stream.size() >= sizeof(T))
     return *reinterpret_cast<const T *>(Stream.data());
diff --git a/llvm/lib/Object/Minidump.cpp b/llvm/lib/Object/Minidump.cpp
index d366b4d3d579e..43c2a4a949214 100644
--- a/llvm/lib/Object/Minidump.cpp
+++ b/llvm/lib/Object/Minidump.cpp
@@ -54,7 +54,8 @@ Expected<std::string> MinidumpFile::getString(size_t Offset) const {
   return Result;
 }
 
-Expected<std::vector<minidump::ExceptionStream>> MinidumpFile::getExceptionStreams() const {
+Expected<std::vector<minidump::ExceptionStream>>
+MinidumpFile::getExceptionStreams() const {
   // Scan the directories for exceptions first
   std::vector<Directory> exceptionStreams;
   for (const auto &directory : Streams) {
@@ -67,15 +68,12 @@ Expected<std::vector<minidump::ExceptionStream>> MinidumpFile::getExceptionStrea
 
   std::vector<minidump::ExceptionStream> exceptionStreamList;
   for (const auto &exceptionStream : exceptionStreams) {
-    llvm::Expected<minidump::ExceptionStream> ExpectedStream = getStreamFromDirectory<minidump::ExceptionStream>(exceptionStream);
+    llvm::Expected<minidump::ExceptionStream> ExpectedStream =
+        getStreamFromDirectory<minidump::ExceptionStream>(exceptionStream);
     if (!ExpectedStream)
       return ExpectedStream.takeError();
 
-    std::cout << "Adding Exception Stream # " << exceptionStreamList.size() << std::endl;
-    std::cout << "Thread Id : " << ExpectedStream->ThreadId << std::endl;
-    std::cout << "Exception Code : " << ExpectedStream.get().ExceptionRecord.ExceptionCode << std::endl;
     exceptionStreamList.push_back(ExpectedStream.get());
-    assert(exceptionStreamList.back().ThreadId == ExpectedStream->ThreadId);
   }
 
   return exceptionStreamList;
@@ -172,9 +170,10 @@ MinidumpFile::create(MemoryBufferRef Source) {
     }
 
     // We treat exceptions differently here because the LLDB minidump
-    // makes some assumptions about uniqueness, all the streams other than exceptions
-    // are lists. But exceptions are not a list, they are single streams that point back to their thread
-    // So we will omit them here, and will find them when needed in the MinidumpFile.
+    // makes some assumptions about uniqueness, all the streams other than
+    // exceptions are lists. But exceptions are not a list, they are single
+    // streams that point back to their thread So we will omit them here, and
+    // will find them when needed in the MinidumpFile.
     if (Type == StreamType::Exception) {
       continue;
     }



More information about the llvm-commits mailing list