[Lldb-commits] [PATCH] D68096: ProcessMinidump: Suppress reporting stop for signal '0'

Mon Sep 30 09:01:03 PDT 2019

JosephTremoulet marked 2 inline comments as done.
JosephTremoulet added a comment.

In D68096#1687790 <https://reviews.llvm.org/D68096#1687790>, @labath wrote:

> It doesn't look like it should be too hard to add yaml support for the exceptions stream -- it should only be a matter of adapting the patterns already used for other stream to work for this particular case. Could you give a go at that?

Yes, will do, thanks for the pointers.

================
Comment at: lldb/source/Plugins/Process/minidump/ProcessMinidump.cpp:258-265
+    uint32_t signo = m_active_exception->exception_record.exception_code;
+
+    if (signo == 0) {
+      // Artifically inject a SIGSTOP so that we'll find a stop reason
+      // when we process the stop event.
+      signo = GetUnixSignals()->GetSignalNumberFromName("SIGSTOP");
+    }
----------------
labath wrote:
> After some investigation, I re-learned about the DumpRequested constant, which is already checked for in this code (and it is tested (TestMiniDumpNew.py:test_snapshow_minidump) and works/does not hang). It seems to me like it would make sense to treat this case the same way as DumpRequested, and just don't return any stop info (instead of returning a stop info with signal 0 or SIGSTOP). IOW, you would just put `if (signo==0) return;` here. Eventually we could apply the same change to process elf core as well..
> 
> WDYT?
SGTM, and seems to work for my use case.  I wasn't sure though whether to leave the `if (signo==0) return` under `if (arch.GetTripe().getOS() == llvm::Triple::Linux)`.  Originally I'd put logic here just because I was copying logic from Process**Elf**Core.  It looks like the Apple case here would maybe similarly have an issue with exception code zero (?), but the `else` case (Windows?) doesn't reference the exception_code at all so I'd hesitate to apply this change there... I've left it under the check for Linux, but happy to move it if that seems wrong.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68096/new/

https://reviews.llvm.org/D68096