SimplifyCFG of empty WinEH cleanup pads doesn't handle lifetime.end correctly

[andykaylor]

Bugzilla Link	44329
Version	trunk
OS	Windows NT
Attachments	Reproducer for SSA problem
CC	@efriedma-quic,@rnk

Extended Description

The SimplifyCFG utility doesn't correctly handle lifetime.end intrinsics when deleting empty WinEH cleanup pads. There are at least two issues.

1. The lifetime.end intrinsic is simply deleted with the empty cleanup pad. This leaves a path through the control graph in which a pointer appears to be alive longer than it should.

2. If the lifetime.end is the only user of a PHI node in the cleanup pad we will attempt to sink the PHI node into the cleanup pad's successor, which might not be dominated by the cleanup pad. If the PHI node had non-PHI users outside the cleanup pad this would be safe, but when the only user of the PHI node is a lifetime.end marker inside the cleanup pad it causes an SSA violation.

The attached IR file reproduces the second problem. The first problem can be seen in the existing (as of 12/17/2019) SimplifyCFG/empty-cleanuppad.ll test (in f9).

[efriedma-quic]

The first issue is sort of nasty; we don't want to keep around an empty cleanup, but in some cases we have nowhere to place the appropriate lifetime marker, besides the "empty" cleanup. Not sure what the right solution looks like. We could try to delay erasing the empty cleanups until after stack coloring, but that probably has other bad consequences.

[andykaylor]

Yeah, we've already done the state numbering before we get to stack coloring. I think it's too late to remove unwind handlers then.

[rnk]

Generally, I think our lifetime representation is not very good (I apologize, people tell me they are tired of hearing me say this), and this is just one of many ways that temporaries may end up with overly long lifetimes. So, in the tradeoff between removing cleanups and less lifetime precision, I think removing cleanups earlier is better. It unlocks many midlevel optimizations, so I wouldn't want to delay removing these otherwise empty cleanups.

For the second problem, I haven't figured it out yet. I see there was some discussion about this case back when the code was reviewed:
https://reviews.llvm.org/D12434?id=33817#inline-101835

I want to say that if the cleanuppad does not dominate its unwind destination, then the only uses of the phi can either be in the cleanuppad (lifetime in this case) or phis in the unwind destination block. Otherwise, there would be an SSA violation (def doesn't dominate use). What exactly are the cases where you should treat the non-dominating block as a loop backedge, and add the phi itself as the incoming value?

I tried to write this C++ code to exercise this edge case, but I wasn't able to trigger the bug:

void usex(int);
void maythrow();
struct Obj {
Obj() {}
~Obj() {}
int x = 0;
};
void f() {
int x = 0;
try {
{
Obj o;
++x;
maythrow();
++x;
maythrow();
}
++x;
maythrow();
} catch (...) {
usex(x);
}
}

[andykaylor]

I became aware of the second problem as a result of someone within my company trying to do a self-build of clang on Windows with exception handling enabled. This resulted in a build failure in DAGCombiner.cpp. The problem was seen with a private branch. I'm testing now to see if it can be reproduced with top of trunk. Even if it can't I'm sure the potential for the problem exists in trunk, as the attached reproducer demonstrates.

The failing IR from DAGCombiner.cpp was very messy, even after running it through bugpoint. After debugging the problem I understood it well enough to create the attached reproducer with hand-written IR. I'm not sure how to get to it from a simple C++ case, but I'm sure that it can happen with real code.

I'm not sure we'll ever actually encounter the back edge case. It would involve a jump into the middle of an exception handling scope that really shouldn't happen and certainly isn't legal in C++, but I'm not sure a hypothetical IR optimization pass would be able to notice it was doing that.

In any event, the current broken case arises because I missed something in the logic of Joseph's reasoning in the review you linked: "and since the empty cleanup was empty, there weren't any uses there." But the cleanup wasn't empty, it contained a lifetime.end marker that was the only user of the PHI. So if we're willing to just delete the lifetime.end marker we could fix this by just ignoring the PHI if it isn't used outside the cleanup pad we're removing.

Could we get into the same situation with debug info intrinsics in the cleanup handler?

[rnk]

Could we get into the same situation with debug info intrinsics in the
cleanup handler?

I don't think so. A dbg.value intrinsic isn't enough to keep around a PHI, it does not introduce real SSA uses, just metadata uses, which aren't verified for dominance.

[andykaylor]

I couldn't reproduce the self-build failure with trunk. Some local change must be exposing the problem.

Looking at the IR from the failing case, it doesn't contain any catchpads, only cleanup handlers. I was curious as to how we ended up with PHI nodes that are used by lifetime.end intrinsics. I'm seeing a bunch of PHIs that look like this:

%61 = phi i8* [ %14, %ehcleanup99 ], [ %14, %invoke.cont94 ], [ %14, %invoke.cont93 ]

If you're curious, this is in DAGCombiner::MergeStoresOfConstantsOrVecElts().

This had me wondering where all of these cleanup handlers came from. Apparently, clang likes to generate cleanup handlers. For example:

https://godbolt.org/z/QqTU4w

I know untouched IR output from the front end is very literal and often redundant, but I really don't understand why that little example with one object and one scope gets three cleanup pads.

In any event, it seems to me like a lifetime.end in a cleanup pad probably means that we're eventually going to unwind to the caller, so probably deleting the lifetime.end is inconsequential.

[andykaylor]

I've created https://reviews.llvm.org/D72540 to address the second problem.