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:

In D62644#1543328, @uabelho wrote:

In D62644#1543279, @fhahn wrote:

It looks like this patch may have caused https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=15236. It would be great if you could have a look.

That assertion is also seen here:

https://bugs.llvm.org/show_bug.cgi?id=42280

However, as I wrote in that PR, with older opt versions it still crashes now and then, only that it often takes more attempts.
And then we hit a different assertion:

opt: ../include/llvm/ADT/DenseMap.h:404: void llvm::DenseMapBase<llvm::DenseMap<(anonymous namespace)::SimpleValue, llvm::ScopedHashTableVal<(anonymous namespace)::SimpleValue, llvm::Value *> *, llvm::DenseMapInfo<(anonymous namespace)::SimpleValue>, llvm::detail::DenseMapPair<(anonymous namespace)::SimpleValue, llvm::ScopedHashTableVal<(anonymous namespace)::SimpleValue, llvm::Value *> *> >, (anonymous namespace)::SimpleValue, llvm::ScopedHashTableVal<(anonymous namespace)::SimpleValue, llvm::Value *> *, llvm::DenseMapInfo<(anonymous namespace)::SimpleValue>, llvm::detail::DenseMapPair<(anonymous namespace)::SimpleValue, llvm::ScopedHashTableVal<(anonymous namespace)::SimpleValue, llvm::Value *> *> >::moveFromOldBuckets(BucketT *, BucketT *) [DerivedT = llvm::DenseMap<(anonymous namespace)::SimpleValue, llvm::ScopedHashTableVal<(anonymous namespace)::SimpleValue, llvm::Value *> *, llvm::DenseMapInfo<(anonymous namespace)::SimpleValue>, llvm::detail::DenseMapPair<(anonymous namespace)::SimpleValue, llvm::ScopedHashTableVal<(anonymous namespace)::SimpleValue, llvm::Value *> *> >, KeyT = (anonymous namespace)::SimpleValue, ValueT = llvm::ScopedHashTableVal<(anonymous namespace)::SimpleValue, llvm::Value *> *, KeyInfoT = llvm::DenseMapInfo<(anonymous namespace)::SimpleValue>, BucketT = llvm::detail::DenseMapPair<(anonymous namespace)::SimpleValue, llvm::ScopedHashTableVal<(anonymous namespace)::SimpleValue, llvm::Value *> *>]: Assertion `!FoundVal && "Key already in new map?"' failed.

• Remove useless assignment

• Add -earlycse-debug-hash flag, shorten new test

Fix style, rebase on top of test change

Switched to static function, and yes I do have commit access. Thanks for the review!

• Use static function rather than anonymous namespace

@labath, as discussed in D59991, I went with a local copy of the function since I didn't see an obvious opportunity to tweak the calling code, but maybe I overlooked something. My copy of cpuid.h matches https://github.com/gcc-mirror/gcc/blob/gcc-5-branch/gcc/config/i386/cpuid.h

In D59991#1475694, @labath wrote:

gcc-5.4 technically supported, so we can try to make things work for you. I'd be best if you can create a patch that will make things work for you (since it's kinda hard for me to test that). Or at least, can you paste the contents of your cpuid.h somewhere?

Thanks for the review!

Updated.

• Merge exe yaml with test file, remove more cruft

• Address review feedback

Names normalized, cruft removed, thanks for the feedback.

No, I hadn't seen the test files in lld/test/COFF; thanks for the pointers! Testcase added; I verified that it fails without the fix and passes without it, and that neither make check nor make check-lld regress.

• add testcase

In D60081#1450958, @ruiu wrote:

Can you tell me how to reproduce the issue? Maybe that's a good start to think about how to write a test.

I'm not sure how to update the tests for this change. Ideally I'd change test/tools/llvm-readobj/imports.test to use an input that has multiple delay-load DLL references. The inputs for that are (binary) coff files test/tools/llvm-readobj/Inputs/imports.exe.coff-x86-64 and test/tools/llvm-readobj/Inputs/imports.exe.coff-i386. @ruiu , do we have sources for those binaries checked in or otherwise available somewhere? Should I just generate new ones with similar import lists?

Thanks!

I don't see this or its unsigned counterpart added to the LangRef; is that intentional, or an oversight?

In D26270#586489, @majnemer wrote:

I don't think funclets need to worry about this.

• Add assertions guarding against over-propagation of unwind dest token

• fix typo
• fix bogus assertion, add testcase covering it

LGTM

HandleInlinedEHPad builds up the FuncletUnwindMap looking at the pre-inlining callee IR. Sharing it with the new call to getUnwindDestToken might be benign, but I'm not sure. I think it would be safer to use a separate FuncletUnwindMap for the new call. Probably also safer to compute the unwind dest in the caller before the code on line 1759 has had a chance to give the caller EH pad new uses in the partially-rewritten inlinee which might confuse the computation.

FYI, I've split this out into constituent changes (locally). I think at this point it makes sense to hold off uploading them individually to Phabricator until they're ready for real review (for fear I'd lose track of things otherwise), but I do have them pushed up to my GitHub fork for my own sake, so if someone happens to want to look at the changes that way, they are:

cosmetic tidying

I went to create the spill slots up-front. Looking at the code after doing that, I realized it makes it more readable to pull all of the spill/fill insertion into the new pass, so that its workings are akin to the rematerialization pass. So I did that.

switch back to inline ctor, as that seems to be the consensus