Page MenuHomePhabricator

Outline cleanup handlers for native Windows C++ exception handling

Authored by andrew.w.kaylor on Feb 24 2015, 12:32 PM.



This patch adds support for outlining cleanup handlers and refactors the existing catch handler outlining code to share common code as much as possible.

Diff Detail


Event Timeline

andrew.w.kaylor retitled this revision from to Outline cleanup handlers for native Windows C++ exception handling.
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, ABataev.
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.Feb 24 2015, 12:57 PM
285–287 ↗(On Diff #20617)

Now that I think about it, I'm not sure we actually need to handle that case. All optimizations have to assume that landingpads can be entered for reasons other than those that are listed. Consider this case where we might hoist a store:

void g();
static int r;
int f() {
  try { g(); }
  catch (int) { r = 42; }
  catch (float) { r = 42; }
  return r;

An optimization that hoisted the r = 42 store into the landingpad block would be invalid, because the landingpad can be entered for other reasons. In particular, if we did that hoist and then inlined f into some other function with cleanups, now the store would be unconditional.

288–292 ↗(On Diff #20617)

I'm not sure we can handle catches inside cleanups this way. Are you planning to switch to a more up front analysis in subsequent changes? I'm thinking of this case:

void g();
struct MyRAII { ~MyRAII(); };
void runs_before_dtor();
void f() {
  MyRAII x;
  try { g(); }
  catch (int) { runs_before_dtor(); }

We need to make sure runs_before_dtor() runs first, but I guess we haven't gotten to building eh.actions.

702–703 ↗(On Diff #20617)

Any reason not to map to unreachable in a cleanup?

708–737 ↗(On Diff #20617)

Similarly, I'd map this to unreachable. It isn't tested yet anyway.

285–287 ↗(On Diff #20617)

Well, I'd be happy to never see the hoisting case arise, but I don't think it's actually going to be very difficult to handle if we wanted to. It might be better to have code to handle it that never gets executed than to have code to flags it as a fatal error if it does happen. Either way, I can take this comment out for now and we can decide when we get to the block analysis review.

288–292 ↗(On Diff #20617)

Yeah, this only handles a single cleanup per landing pad and doesn't handle the case where a block of cleanup code is shared between landing pads. This is basically here as a place holder to move things along. I can add a comment explaining that.

To handle cases like the one you describe I need the block analysis stuff in place. The actually outlining works the same way except that it gets a specific starting block and has different termination conditions. I have that working in my sandbox, but it seemed like a bit much to put into this review.

702–703 ↗(On Diff #20617)

That's a good idea. I'll change it to that.

708–737 ↗(On Diff #20617)


rnk accepted this revision.Feb 24 2015, 1:32 PM
rnk edited edge metadata.


288–292 ↗(On Diff #20617)

OK, sounds good.

This revision is now accepted and ready to land.Feb 24 2015, 1:32 PM
andrew.w.kaylor edited edge metadata.

Added comments clarifying the state of cleanup support.
Switched begin/end catch replacement to unreachable for cleanup cloning.

Updating handler function linkage as suggested in the work-in-progress review.

majnemer accepted this revision.Mar 2 2015, 4:02 PM
majnemer edited edge metadata.


This revision was automatically updated to reflect the committed changes.