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

[Greg Clayton via Phabricator via lldb-commits]

clayborg accepted this revision.
clayborg added a comment.
This revision is now accepted and ready to land.

In D68096#1706076 <https://reviews.llvm.org/D68096#1706076>, @JosephTremoulet wrote:

> Just to make sure I'm understanding the feedback correctly, I'll try to summarize.  Please let me know if this is off track:
>
> - When one uses breakpad to generate a minidump for a process that *hasn't* crashed, depending which of its methods one calls to write the dump, the generated dump's exception stream's exception code is either the "dump requested" sentinel or zero.


I believe so.

> - We have a bug, which this patch is meant to fix, wherein loading one of those dumps with exception code zero creates a stop with signal zero, resulting in lldb hanging.

If this patch is solely meant to fix this issue, then I remove my "Requires Changes" and this patch is good to go if it no longer hangs LLDB.

> - We have some "prior art" in ProcessElfCore, which is how we successfully load the core dump you get by running a zero-exception-code minidump through breakpad's minidump-2-core, which is to artificially report SIGSTOP.  @labath, your feedback is that this is a hack that we shouldn't extend and should ideally/eventually remove from ProcessElfCore

I agree on this as long as no hang happens!

> - We also have some "prior art" in ProcessMinidump, which is how we successfully load a minidump with the "dump requested" sentinel, which is to create no stop at all.  That's what this patch currently does when it sees signal zero.  @clayborg, your feedback is that simply checking that the error code is zero is insufficient grounds to suppress stop creation, that we should probably check other fields in the exception stream and create a stop if *any* of them are reporting anything non-null, and/or verify that the only way breakpad/crashpad will produce a linux minidump with null signal is if there's no exception whatsoever

As Pavel mentioned, I was asking for other fields in the exception info to be passed along for other signals, like SIGSEGV. After reading his comments, I agree this can be done in another patch if it isn't already being done.

> I may need to set this aside for a while and come back to it to address @clayborg's concerns (if I've understood them correctly), but I can live with my downstream changes in the meantime, so that's seeming like the best path forward.

If this patch fixes the hang, then it is good to go.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68096