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 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
