LGTM, thanks.

• Use invalid_argument error code

• Change signature to return Expected<vector<BreakpointSP>>

• Rebase
• Use Twine instead of formatv

• punctuation fixes

• Move test

• Remove TODO, lit-ify negative test and tighten check

• ...and fix namespace...

• Fix Expected<> types

• Apply review feedback (-auto, -memset, +comments)

In D68096#1706258, @clayborg wrote:

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.

Just to make sure I'm understanding the feedback correctly, I'll try to summarize. Please let me know if this is off track:

Added Exception stream to minidump-basic.yaml as suggested.

• Remove useless comment and leftover debugging cruft

• Update test input yaml Exception stream

• Rebase
• Add testcase

In D68657#1703752, @labath wrote:

Therefore I think it would make sense to just spell out each member of that array as a separate member in the yaml representation, which should be a much simpler endeavour. We can use the "actual parameter count" field to suppress the fields that don't contain any value, if they really are zero, which should make the yaml output concise in the usual cases. I think something like this should be sufficient:
...
I think that would strike a good balance between code complexity, output brevity, and being able to generate interesting and potentially invalid inputs for other tools (which is one of the main goals of yaml2obj, and so interpreting the input too strictly is not desired/helpful).

• Add test with extraneous parameter

• Change Exception Information format per feedback

In D68656#1703711, @labath wrote:

Hmm.. maybe just keep those constants defined in lldb somewhere (e.g. right next to or inside the RefreshStateAfterStop function)? That's the place which does the decoding and also includes the OS switch, so it makes it easier to explain their purpose and why they are different for each OS...

• Define DumpRequested constant in-line, per feedback in D68656

• Remove ExceptionCode enumeration

rebase

• Remove the os-defined exception code enum values

In D68656#1701658, @labath wrote:

Thanks for taking your time to do this. I have one question: It looks like you're not using the exception code enum in the follow-up patch. I think that's completely reasonable given that the enum values are overloaded and system-dependent. But given this fact, and the fact that I am not convinced the enum values are completely right (e.g. the linux signal numbers depend also on the architecture -- though this may not manifest itself on the architectures that breakpad supports right now), what would you say to just dropping that enumeration?

Nit: Title says "thread" rather than "memory info"

In 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?

• Review feedback

• Use accessor for m_unix_signals_sp

• Move artificial SIGSTOP injection to RefreshStateAfterStop

Fortunately, for the functionality you're testing, I don't think you really need the executable file, so you can just ignore the elf bit and test with a plain lldb -c foo.dmp (obviously, you won't get the backtrace that way, but you don't really need that here.

I'm trying to fix an issue where opening a minidump created by breakpad in lldb just hangs, but if I use breakpad's minidump-2-core on that same dump then opening the core dump works fine. From debugging the two cases I can see that the critical logic that ProcessElfCore has which ProcessMinidump lacks is here, and the code in this patch is my attempt to replicate that.

LGTM. Probably good to have someone else weigh in before committing (I touched this code last, but only to fix a couple bugs).

In D64993#1612746, @JDevlieghere wrote:

Hey Joseph,

The test trap_frame_sym_ctx is failing on GreenDragon. Can you please have a look?

http://lab.llvm.org:8080/green/view/LLDB/job/lldb-cmake/32449/testReport/junit/LLDB/Unwind/trap_frame_sym_ctx_test/

• Expand comment about return trampolines
• rename PropagateTrapHandlerFlag -> PropagateTrapHandlerFlagFromUnwindPlan

In D64993#1608452, @labath wrote:

I haven't looked at the implementation, but I did run into the problem you are fixing in the past, and I am happy that someone is finally implementing this. I just wanted to say that for testing, I think you should be able to create some hand-written assembly that creates the kind of stack frames and unwind info you need to trigger this. You can look at existing tests in lldb/lit/Unwind for examples.

• add test

• Move resolution helper from RegisterContextLLDB to lldb_private::Address
• Defensively initialize out arguments

@jasonmolenda @clayborg ping. To clarify, the issue this fixes is, using test functionalities/signal/handle-abrt as an example:

#include <signal.h>
#include <stdio.h>
#include <stdlib.h>

In D63667#1594718, @jankratochvil wrote:

You did not have DWARF for glibc installed: dnf debuginfo-install glibc

Then in fact your test was better as only real frames matter for this patch. There is some LLDB bug/incompatibility decoding inline function from (GCC-produced) DWARF but that is unrelated to this patch.

I'm guessing on this Linux system, you've got a trap receiver function on the stack that is on its first instruction or something? So backing up the pc value for *that* frame is the problem you're solving.
...
Just just to check, we've got a backtrace like...

ping @jasonmolenda -- do the updates look like what you had in mind when you said " It'd be a good change to capture that information from the eh_frame though, even if we don't see a clear way to use it right now"?

That LLDB frame #5 is a bit bogus

In D63667#1561365, @jankratochvil wrote:

Shouldn't you add also symbol __restore_rt (Fedora 30 x86_64)?

• Include __restore_rt

• fix copy pasta

I've updated this with code to recognize the 'S' in the eh_frame augmentation and record it in a new bool member of UnwindPlan, m_plan_is_for_signal_trap. I haven't hooked up any consumers of the new bit; as you say, with the current code flow we don't parse the eh_frame info until after we've decided whether the current frame is a trap handler frame or not.

• Fix typos
• Convey 'S' eh_frame augmentation to UnwindPlan

FYI, I sent mail about this to lldb-dev.. I'll copy the contents here for the benefit of anybody who didn't see it there but could use the context: