[LLDB][Minidump] Support minidumps where there are multiple exception streams #97470

Jlalond · 2024-07-02T19:56:22Z

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 only 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.~~

llvmbot · 2024-07-02T22:00:49Z

@llvm/pr-subscribers-objectyaml

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

Author: Jacob Lalonde (Jlalond)

Changes

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.

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

12 Files Affected:

(modified) lldb/source/Plugins/Process/minidump/MinidumpParser.cpp (+6-4)
(modified) lldb/source/Plugins/Process/minidump/MinidumpParser.h (+1-1)
(modified) lldb/source/Plugins/Process/minidump/ProcessMinidump.cpp (+66-55)
(modified) lldb/source/Plugins/Process/minidump/ProcessMinidump.h (+1-1)
(modified) lldb/source/Plugins/Process/minidump/ThreadMinidump.cpp (+15-3)
(modified) lldb/source/Plugins/Process/minidump/ThreadMinidump.h (+3-1)
(modified) lldb/test/API/functionalities/postmortem/minidump-new/TestMiniDumpNew.py (+14)
(added) lldb/test/API/functionalities/postmortem/minidump-new/multiple-sigsev.yaml (+39)
(modified) lldb/unittests/Process/minidump/MinidumpParserTest.cpp (+7-4)
(modified) llvm/include/llvm/Object/Minidump.h (+31-7)
(modified) llvm/lib/Object/Minidump.cpp (+36)
(modified) llvm/lib/ObjectYAML/MinidumpYAML.cpp (+2-2)

diff --git a/lldb/source/Plugins/Process/minidump/MinidumpParser.cpp b/lldb/source/Plugins/Process/minidump/MinidumpParser.cpp
index be9fae938e227..c51fed8d91b07 100644
--- a/lldb/source/Plugins/Process/minidump/MinidumpParser.cpp
+++ b/lldb/source/Plugins/Process/minidump/MinidumpParser.cpp
@@ -417,14 +417,16 @@ 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..9a7e481b92796 100644
--- a/lldb/source/Plugins/Process/minidump/ProcessMinidump.cpp
+++ b/lldb/source/Plugins/Process/minidump/ProcessMinidump.cpp
@@ -157,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_active_exception(nullptr),
-      m_is_wow64(false) {}
+      m_core_data(std::move(core_data)), m_is_wow64(false) {}
 
 ProcessMinidump::~ProcessMinidump() {
   Clear();
@@ -209,7 +208,20 @@ 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
+             .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;
+    }
+  }
 
   SetUnixSignals(UnixSignals::Create(GetArchitecture()));
 
@@ -232,60 +244,57 @@ 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;
+      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,10 +395,11 @@ 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;
@@ -398,7 +408,8 @@ bool ProcessMinidump::DoUpdateThreadList(ThreadList &old_thread_list,
     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..a1d7da376b02b 100644
--- a/lldb/source/Plugins/Process/minidump/ThreadMinidump.cpp
+++ b/lldb/source/Plugins/Process/minidump/ThreadMinidump.cpp
@@ -34,9 +34,10 @@ 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 +116,15 @@ ThreadMinidump::CreateRegisterContextForFrame(StackFrame *frame) {
   return reg_ctx_sp;
 }
 
-bool ThreadMinidump::CalculateStopInfo() { return false; }
+// 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));
+  return true;
+}
diff --git a/lldb/source/Plugins/Process/minidump/ThreadMinidump.h b/lldb/source/Plugins/Process/minidump/ThreadMinidump.h
index aed7cfbc1b161..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);
+                 llvm::ArrayRef<uint8_t> gpregset_data,
+                 std::optional<minidump::Exception> exception);
 
   ~ThreadMinidump() override;
 
@@ -35,6 +36,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/test/API/functionalities/postmortem/minidump-new/TestMiniDumpNew.py b/lldb/test/API/functionalities/postmortem/minidump-new/TestMiniDumpNew.py
index 91fd2439492b5..ba93eff6f1f82 100644
--- a/lldb/test/API/functionalities/postmortem/minidump-new/TestMiniDumpNew.py
+++ b/lldb/test/API/functionalities/postmortem/minidump-new/TestMiniDumpNew.py
@@ -491,3 +491,17 @@ def test_minidump_sysroot(self):
         spec_dir_norm = os.path.normcase(module.GetFileSpec().GetDirectory())
         exe_dir_norm = os.path.normcase(exe_dir)
         self.assertEqual(spec_dir_norm, exe_dir_norm)
+
+    def test_multiple_exceptions_or_signals(self):
+        """Test that lldb can read the exception information from the Minidump."""
+        print("Starting to read multiple-sigsev.yaml")
+        self.process_from_yaml("multiple-sigsev.yaml")
+        print("Done reading multiple-sigsev.yaml")
+        self.check_state()
+        # This process crashed due to a segmentation fault in both it's threads.
+        self.assertEqual(self.process.GetNumThreads(), 2)
+        for i in range(2):
+            thread = self.process.GetThreadAtIndex(i)
+            self.assertStopReason(thread.GetStopReason(), lldb.eStopReasonSignal)
+            stop_description = thread.GetStopDescription(256)
+            self.assertIn("SIGSEGV", stop_description)
diff --git a/lldb/test/API/functionalities/postmortem/minidump-new/multiple-sigsev.yaml b/lldb/test/API/functionalities/postmortem/minidump-new/multiple-sigsev.yaml
new file mode 100644
index 0000000000000..f6fcfdbf5c0eb
--- /dev/null
+++ b/lldb/test/API/functionalities/postmortem/minidump-new/multiple-sigsev.yaml
@@ -0,0 +1,39 @@
+--- !minidump
+Streams:
+  - Type:            ThreadList
+    Threads:
+      - Thread Id:       0x1B4F23
+        Context:         0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000B0010000000000033000000000000000000000006020100000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000010000000000000000000000000000000000000000000000010A234EBFC7F000010A234EBFC7F00000000000000000000F09C34EBFC7F0000C0A91ABCE97F00000000000000000000A0163FBCE97F00004602000000000000921C40000000000030A434EBFC7F000000000000000000000000000000000000C61D4000000000007F0300000000000000000000000000000000000000000000801F0000FFFF0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000FFFF00FFFFFFFFFFFFFF00FFFFFFFF25252525252525252525252525252525000000000000000000000000000000000000000000000000000000000000000000FFFF00FFFFFFFFFFFFFF00FFFFFFFF0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000FF00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
+        Stack:
+          Start of Memory Range: 0x7FFFFFFFD348
+          Content:         ''
+      - Thread Id:       0x1B6D22
+        Context:         0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000B0010000000000033000000000000000000000006020100000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000010000000000000000000000000000000000000000000000010A234EBFC7F000010A234EBFC7F00000000000000000000F09C34EBFC7F0000C0A91ABCE97F00000000000000000000A0163FBCE97F00004602000000000000921C40000000000030A434EBFC7F000000000000000000000000000000000000C61D4000000000007F0300000000000000000000000000000000000000000000801F0000FFFF0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000FFFF00FFFFFFFFFFFFFF00FFFFFFFF25252525252525252525252525252525000000000000000000000000000000000000000000000000000000000000000000FFFF00FFFFFFFFFFFFFF00FFFFFFFF0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000FF00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
+        Stack:
+          Start of Memory Range: 0x7FFFF75FDE28
+          Content:         ''
+  - Type:            ModuleList
+    Modules:
+      - Base of Image:   0x0000000000400000
+        Size of Image:   0x00017000
+        Module Name:     'a.out'
+        CodeView Record: ''
+  - Type:            SystemInfo
+    Processor Arch:  AMD64
+    Platform ID:     Linux
+    CSD Version:     'Linux 3.13'
+    CPU:
+      Vendor ID:       GenuineIntel
+      Version Info:    0x00000000
+      Feature Info:    0x00000000
+  - Type:            Exception
+    Thread ID:       0x1B4F23
+    Exception Record:
+      Exception Code:  0xB
+    Thread Context:  00000000
+  - Type:            Exception
+    Thread ID:       0x1B6D22
+    Exception Record:
+      Exception Code:  0xB
+    Thread Context:  00000000
+...
diff --git a/lldb/unittests/Process/minidump/MinidumpParserTest.cpp b/lldb/unittests/Process/minidump/MinidumpParserTest.cpp
index 632a7fd4e4f8f..aa74e690d45a5 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::...
[truncated]

lldb/test/API/functionalities/postmortem/minidump-new/multiple-sigsev.yaml

jeffreytan81

One concern is that, whether minidump format specification supports multiple exception streams. If this is not supported by spec, we may generate minidump that only lldb can read/parse but can fail other consumers.

It would be interesting to copy the generated minidump with multiple exception steams to a Windows machine and try windbg/Visual Studio and see if they break.

jeffreytan81 · 2024-07-05T18:26:30Z

llvm/lib/Object/Minidump.cpp

+  std::vector<minidump::ExceptionStream> exceptionStreamList;
+  for (const auto &exceptionStream : exceptionStreams) {
+    llvm::Expected<minidump::ExceptionStream> ExpectedStream =
+        getStreamFromDirectory<minidump::ExceptionStream>(exceptionStream);
+    if (!ExpectedStream)
+      return ExpectedStream.takeError();
+
+    exceptionStreamList.push_back(ExpectedStream.get());


minidump::ExceptionStream is non-trivial in size. There are a lot of copying here which I do not think you need.
The ownership of minidump::ExceptionStream is still inside the stream, so let's return reference/pointer from these functions instead of copying by value.

jeffreytan81 · 2024-07-05T18:27:17Z

llvm/lib/Object/Minidump.cpp

+  std::vector<Directory> exceptionStreams;
+  for (const auto &directory : Streams) {
+    if (directory.Type == StreamType::Exception)
+      exceptionStreams.push_back(directory);
+  }


Similar, I do not think you need to make a copy of Directory, returning by reference is enough.

jeffreytan81 · 2024-07-05T18:28:26Z

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 =


Why are you changing this to return by value copy?

jeffreytan81 · 2024-07-05T18:30:04Z

llvm/include/llvm/Object/Minidump.h

+  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);


Why are changing the API not to return by const reference?

jeffreytan81 · 2024-07-05T18:31:17Z

lldb/source/Plugins/Process/minidump/ProcessMinidump.cpp

@@ -209,7 +208,20 @@ 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 =


Do not return by a vector of copies, instead return a value of const references/pointers.

jeffreytan81 · 2024-07-05T18:31:57Z

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;


Store const minidump::ExceptionStream& instead.

This will expose my noviceness in C++, but I struggled that you can't emplace a list of references, so instead I refactored this to return a vector of const pointers.

jeffreytan81 · 2024-07-05T18:34:08Z

llvm/include/llvm/Object/Minidump.h

+  /// 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;


Should this API return Expected<> instead of std::option<>? It seems perfect expected that a minidump does not have any exceptions.

I decided against this for Minidump.h because the existing API's expose Expected, for the Minidumparser I used optional upon your recommendation

jeffreytan81 · 2024-07-05T18:34:43Z

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>


Is this needed?

Jlalond · 2024-07-05T21:52:01Z

@jeffreytan81 I think the callout for multiple exception is a good question. I made a C# testbed to see what would happen if I had multiple simultaneous exceptions. Gist here. When manually collecting a minidump on my Windows system and then viewing it, I do get a warning that my application has multiple exceptions.

This leads me to believe multiple exceptions is acceptable, albeit rare.

jeffreytan81 · 2024-07-08T17:28:03Z

@jeffreytan81 I think the callout for multiple exception is a good question. I made a C# testbed to see what would happen if I had multiple simultaneous exceptions. Gist here. When manually collecting a minidump on my Windows system and then viewing it, I do get a warning that my application has multiple exceptions.

This leads me to believe multiple exceptions is acceptable, albeit rare.

That's good to know, but this is still an indirectly way to check. We would feel much convinced if you can copy the minidump generated by lldb to Windows and give windbg or Visual Studio a try.

Jlalond · 2024-07-08T20:39:08Z

@jeffreytan81 I think the callout for multiple exception is a good question. I made a C# testbed to see what would happen if I had multiple simultaneous exceptions. Gist here. When manually collecting a minidump on my Windows system and then viewing it, I do get a warning that my application has multiple exceptions.

This leads me to believe multiple exceptions is acceptable, albeit rare.

That's good to know, but this is still an indirectly way to check. We would feel much convinced if you can copy the minidump generated by lldb to Windows and give windbg or Visual Studio a try.

Okay, I took the same program and then collected a dump with LLDB. As this was .net, we have an issue not collecting information about private core lib or the windows OS version, so Visual studio does encounter some issue. However I can still enter managed debug and see the exception objects on every thread.

labath

If other readers can handle these kinds of minidumps, then I guess this is fine. Like in the other patch, I think it'd be best to split out the llvm part into its own PR.

labath · 2024-08-14T14:10:36Z

llvm/lib/Object/Minidump.cpp

@@ -53,6 +53,27 @@ Expected<std::string> MinidumpFile::getString(size_t Offset) const {
  return Result;
 }

+Expected<std::vector<const minidump::ExceptionStream *>>
+MinidumpFile::getExceptionStreams() const {


This looks like a job for an (non-fallible) input iterator returning an Expected<const ExceptionStream&>. (non-fallible, because the iteration always suceeds, but reading of any stream can fail, and can do so independendly of all other streams.)

Alternatively, maybe we could just return an array of minidump::Directory descriptors, and have the caller retrieve them one at a time via something like getExceptionStream(Directory).

labath · 2024-08-14T14:12:38Z

llvm/lib/Object/Minidump.cpp

+    // We treat exceptions differently here because the LLDB minidump
+    // makes some assumptions about uniqueness, all the streams other than


I'm not sure which assumptions are you referring to (from what I can tell it's this code here, which is assuming there can be only one stream of a given type), but it'd be nice if this could be explained in a way which does not involve LLDB (say what do we want to do, not who we're doing it for)

labath · 2024-08-14T14:13:28Z

llvm/lib/Object/Minidump.cpp

+    if (Type == StreamType::Exception) {
+      continue;


How about storing the of exception streams inside some (vector) member?

Jlalond · 2024-08-14T17:12:30Z

@labath Agree on all counts, I'll split this into a different PR. With what I learned from our obj2yaml adventure I would like to do it better anyway.

A fork of #97470, splitting off the LLVM changes from the LLDB specific changes. This patch enables a minidump file to have multiple exceptions, exposed via an iterator of Expected streams.

github-actions · 2024-09-06T22:33:00Z

✅ With the latest revision this PR passed the C/C++ code formatter.

labath

Looks good to me. Just a few suggestions.

labath · 2024-09-09T07:04:41Z

lldb/source/Plugins/Process/minidump/ProcessMinidump.cpp

+      // We only cast to avoid the warning around converting little endian in
+      // printf.
+      return Status::FromErrorStringWithFormat(
+          "Duplicate exception stream for tid %" PRIu32,
+          (uint32_t)exception_stream->ThreadId);
+    }


This wasn't just an idle warning. The cast is actually necessary to make the code correct. But it's even better to use a formatting function that handles this for you.

Suggested change

// We only cast to avoid the warning around converting little endian in

// printf.

return Status::FromErrorStringWithFormat(

"Duplicate exception stream for tid %" PRIu32,

(uint32_t)exception_stream->ThreadId);

}

return Status::FromErrorStringWithFormatv(

"Duplicate exception stream for tid {0}",

exception_stream->ThreadId);

}

labath · 2024-09-09T07:07:41Z

lldb/source/Plugins/Process/minidump/ProcessMinidump.cpp

+    if (m_exceptions_by_tid.count(thread.ThreadId) > 0)
+      context_location = m_exceptions_by_tid[thread.ThreadId].ThreadContext;


Suggested change

if (m_exceptions_by_tid.count(thread.ThreadId) > 0)

context_location = m_exceptions_by_tid[thread.ThreadId].ThreadContext;

if (auto it = m_exceptions_by_tid.find(thread.ThreadId); it != m_exceptions_by_tid.end())

context_location = it->ThreadContext;

Avoids double container lookup. Not that it matters here, but nice to get in the habit of it.

Agreed, I need to get into the habit of using find

jeffreytan81

LGTM

Jlalond assigned clayborg, jeffreytan81 and kusmour Jul 2, 2024

Jlalond changed the title ~~[DRAFT][LLDB][Minidump] Support minidumps where there are multiple exception streams~~ [LLDB][Minidump] Support minidumps where there are multiple exception streams Jul 2, 2024

Jlalond force-pushed the minidump-exception-reading branch from 7a008b1 to f91ff4b Compare July 2, 2024 22:00

Jlalond marked this pull request as ready for review July 2, 2024 22:00

Jlalond requested a review from JDevlieghere as a code owner July 2, 2024 22:00

llvmbot added lldb objectyaml llvm:binary-utilities labels Jul 2, 2024

Jlalond commented Jul 2, 2024

View reviewed changes

lldb/test/API/functionalities/postmortem/minidump-new/multiple-sigsev.yaml Show resolved Hide resolved

Jlalond force-pushed the minidump-exception-reading branch from f91ff4b to 9bddd55 Compare July 3, 2024 18:32

Jlalond assigned GeorgeHuyubo Jul 3, 2024

jeffreytan81 requested changes Jul 5, 2024

View reviewed changes

jeffreytan81 requested review from labath, clayborg, kusmour and GeorgeHuyubo July 5, 2024 18:40

Jlalond force-pushed the minidump-exception-reading branch from 613dbd0 to 5b6da6c Compare July 9, 2024 17:06

Jlalond closed this Aug 6, 2024

Jlalond deleted the minidump-exception-reading branch August 6, 2024 18:46

Jlalond restored the minidump-exception-reading branch August 8, 2024 16:45

Jlalond reopened this Aug 8, 2024

labath reviewed Aug 14, 2024

View reviewed changes

Jlalond mentioned this pull request Sep 4, 2024

[Minidump] Support multiple exceptions in a minidump #107319

Merged

Jlalond added 3 commits September 6, 2024 13:09

MSquash merge lldb changes and drop llvm changes

5d7f2a6

Migrate code over to the new iterator api

9c90aa6

Edit tests

bfb8f4f

Jlalond force-pushed the minidump-exception-reading branch from a8c1683 to bfb8f4f Compare September 6, 2024 22:29

Run formatters

06e493f

labath reviewed Sep 9, 2024

View reviewed changes

Some cleanup that Pavel suggested

a32c516

Jlalond force-pushed the minidump-exception-reading branch from f870d51 to a32c516 Compare September 9, 2024 18:30

jeffreytan81 approved these changes Sep 10, 2024

View reviewed changes

Jlalond merged commit 4926835 into llvm:main Sep 10, 2024
7 checks passed

Jlalond deleted the minidump-exception-reading branch March 7, 2025 19:20

		// We treat exceptions differently here because the LLDB minidump
		// makes some assumptions about uniqueness, all the streams other than

		if (m_exceptions_by_tid.count(thread.ThreadId) > 0)
		context_location = m_exceptions_by_tid[thread.ThreadId].ThreadContext;

[LLDB][Minidump] Support minidumps where there are multiple exception streams #97470

[LLDB][Minidump] Support minidumps where there are multiple exception streams #97470

Uh oh!

Conversation

Jlalond commented Jul 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Jul 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

jeffreytan81 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Jlalond commented Jul 5, 2024

Uh oh!

jeffreytan81 commented Jul 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Jlalond commented Jul 8, 2024

Uh oh!

labath left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Jlalond commented Aug 14, 2024

Uh oh!

github-actions bot commented Sep 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

labath left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jeffreytan81 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Jlalond commented Jul 2, 2024 •

edited

Loading

llvmbot commented Jul 2, 2024 •

edited

Loading

jeffreytan81 commented Jul 8, 2024 •

edited

Loading

github-actions bot commented Sep 6, 2024 •

edited

Loading