This is an archive of the discontinued LLVM Phabricator instance.

Handle nested landing pads in outlined catch handlers for Windows C++ EH
ClosedPublic

Authored by andrew.w.kaylor on Mar 24 2015, 5:11 PM.

Details

Summary

This patch implements the code to fill in nested landing pads that were stubbed out during initial catch handler outlining.

The nested landing pad may introduce new exception variables that need to be recovered in the handler in which the landing pad is nested in order for the value to be passed to eh.actions. If the variable isn't otherwise referenced in the handler, I think we will want to drop the recovered value during code gen.

A catch handler that is called from the nested landing pad may need to return to an address in the outlined handler. I've added code to detect this case and remap the return instruction. This will not work correctly if the cloning code optimizes away the target block or if the target block is shared between multiple catch handlers. I think both of these are possible, but I don't have test cases to show it yet.

Diff Detail

Repository
rL LLVM

Event Timeline

andrew.w.kaylor retitled this revision from to Handle nested landing pads in outlined catch handlers for Windows C++ EH.
andrew.w.kaylor updated this object.
andrew.w.kaylor edited the test plan for this revision. (Show Details)
andrew.w.kaylor added reviewers: rnk, majnemer.
andrew.w.kaylor set the repository for this revision to rL LLVM.
andrew.w.kaylor added a subscriber: Unknown Object (MLST).
rnk added inline comments.Mar 25 2015, 10:47 AM
lib/CodeGen/WinEHPrepare.cpp
99 ↗(On Diff #22617)

Can you add a comment about which is the key and which is the value? This goes from cloned LP to original LP if I read the code right. Maybe ClonedLPToOriginalLP would be a better name.

479 ↗(On Diff #22617)

I *think* LPadPair.second = NewLPad will update the container. The subscript operation is making me nervous because it looks like mutation during iteration, even though we already know that the key exists in the container. DenseMap doesn't have this bug, but just the other day we were looking at an old implementation of TR1 hash_map which had a bug where it would potentially resize on this exact operation.

537 ↗(On Diff #22617)

In general, you shouldn't need to reach for getInstList(). This can be written:

assert(&OutlinedBB->back() == OutlinedLPad);
558 ↗(On Diff #22617)

You could probably just cast the result of the clone() call and save a local variable.

562–564 ↗(On Diff #22617)

I have an idea for how we can completely eliminate the need for this. What if we changed the catch object argument from an AllocaInst to the i32 value that would be used with llvm.framerecover? That way the i32 would mean the same thing in the parent function and the child functions.

575 ↗(On Diff #22617)

break?

585 ↗(On Diff #22617)

Both the AllocaInst and Function cases of are pretty heavy. What do you think of pulling them out into helper functions? prepareExceptionHandlers is pretty long.

586–591 ↗(On Diff #22617)

We have a parseEHActions function implementation in a pending patch. It goes from llvm.eh.actions call back into a list of ActionHandlers. It may ultimately be cleaner to use that, but I don't want to block your patch on ours. Let's throw a FIXME in here to come around and do that.

592 ↗(On Diff #22617)

You can exit early if it's not i8*.

600–601 ↗(On Diff #22617)

You can continue if it's not ReturnInst

608 ↗(On Diff #22617)

Ditto, continue

888–893 ↗(On Diff #22617)

These "End" comments seem excessive, for this short of a loop.

1196–1198 ↗(On Diff #22617)

I wouldn't say "We shouldn't encounter landing pads in the actual cleanup code." This is currently only true because the outlining process unconditionally turns invokes in cleanups into calls. In general, destructors with real invokes can be inlined, and we will need to handle that one day. For now, I would rephrase this comment along the lines of "Cleanup outlining currently demotes all invokes to calls, so all landingpads are trivially unreachable."

lib/CodeGen/WinEHPrepare.cpp
99 ↗(On Diff #22617)

I'm not sure ClonedLPToOriginalLP is especially helpful, as it doesn't indicate why the landing pads in question were mapped.

I do agree that something needs to be done to make this more clear and that a comment doesn't seem quite sufficient.

How about NestedLPToOriginalLP?

Incidentally, I did the mapping in this way because I think that it is possible for the same landing pad to be nested within multiple handlers.

479 ↗(On Diff #22617)

Alright, I'll give that a try.

562–564 ↗(On Diff #22617)

Yeah, that should work and would clean up a lot of mess here. It's not as readable, but hopefully no one will be reading these things much anyway.

575 ↗(On Diff #22617)

Yes, if we have to keep this code.

585 ↗(On Diff #22617)

Sounds reasonable.

586–591 ↗(On Diff #22617)

OK

888–893 ↗(On Diff #22617)

I just really hate seeing that many consecutive end brackets. They'll go away after I clean up the loops as you suggest above.

1196–1198 ↗(On Diff #22617)

You mentioned this before, and I think I said then that I don't believe the MS runtime will allow an exception to be thrown in a cleanup handler. Do you disagree?

Implemented suggested changes from earlier review.

rnk edited edge metadata.Apr 1 2015, 11:11 AM

This seems pretty close to done. Sorry for the delay, I was staring at .xdata tables all day long. =P

lib/CodeGen/WinEHPrepare.cpp
105–124 ↗(On Diff #22767)

Thanks, these comments help. :)

697–698 ↗(On Diff #22767)

parseEHActions is in tree in FunctionLoweringInfo, but if we share this code, we should probably move it over to this pass and declare it in WinEHFuncInfo.h.

Do you want to do this now or later? I'm OK either way.

702–723 ↗(On Diff #22767)

Going forward, I would really like to change llvm.eh.actions to take an i32 frameescape index for the catch object instead. Doing this actually makes things easier for WinEHNumbering, which is where we build the try block map entries.

If we make this change first, you can remove this entire alloca case, because the index is the same in the parent function as in the outlined handler. Do you think it's worth defering this again, or would you rather try to get this in first and come back later?

lib/CodeGen/WinEHPrepare.cpp
697–698 ↗(On Diff #22767)

It probably doesn't make sense to check in code that's going to be removed almost immediately. I'm in the process of merging this patch with the latest trunk revision now. I'll go ahead and make this change while I'm at it.

702–723 ↗(On Diff #22767)

If the changes to frameescape can be made quickly, I think it's best to have that in first.

andrew.w.kaylor edited edge metadata.

Rebased to merge with latest WinEH changes.
Moved parseEHActions into WinEHPrepare and updated it to save the exception object frameescape index.
Updated the code that was previously parsing eh.actions calls in a different way.

majnemer added inline comments.Apr 2 2015, 5:27 PM
lib/CodeGen/WinEHPrepare.cpp
699 ↗(On Diff #23188)

Can you please clarify this FIXME?

andrew.w.kaylor added inline comments.Apr 2 2015, 5:28 PM
lib/CodeGen/WinEHPrepare.cpp
699 ↗(On Diff #23188)

Oops. That was there from a previous revision. It should have been deleted.

majnemer added inline comments.Apr 2 2015, 5:47 PM
include/llvm/CodeGen/WinEHFuncInfo.h
77 ↗(On Diff #23188)

There is no getExceptionVarIndex and code which loads from ExceptionObjectIndex. Is this code from a prior revision or do you have future intentions for it?

andrew.w.kaylor added inline comments.Apr 2 2015, 5:49 PM
include/llvm/CodeGen/WinEHFuncInfo.h
77 ↗(On Diff #23188)

I expected that the code generating the .xdata tables would use that, but I didn't want to add the function until it was going to be called.

Adding updates to cppeh-prepared-catch.ll, which I missed previously.
Removing stale FIXME comment.

rnk accepted this revision.Apr 3 2015, 10:41 AM
rnk edited edge metadata.

lgtm

lib/CodeGen/WinEHPrepare.cpp
705 ↗(On Diff #23224)

Early continue if !Handler?

This revision is now accepted and ready to land.Apr 3 2015, 10:41 AM
majnemer accepted this revision.Apr 3 2015, 12:38 PM
majnemer edited edge metadata.
This revision was automatically updated to reflect the committed changes.