This patch updates the code that calculates C++ exception states. The results after this patch still have some problems, but it is much better. In particular, this patch helps with rethrowing exceptions from within a catch handler.
Details
Details
Diff Detail
Diff Detail
- Repository
- rL LLVM
Event Timeline
| lib/CodeGen/AsmPrinter/Win64Exception.cpp | ||
|---|---|---|
| 391 ↗ | (On Diff #25001) | When is MaxState different from FuncInfo.UnwindMap.size(). |
| lib/CodeGen/SelectionDAG/FunctionLoweringInfo.cpp | ||
| 588 ↗ | (On Diff #25001) | calculateStateNumbers does the same basic traversal, how does stashing the first landing pad associated with the action help things? Is it just to reenable the optimization in processCallSite ? |
| lib/CodeGen/SelectionDAG/FunctionLoweringInfo.cpp | ||
|---|---|---|
| 588 ↗ | (On Diff #25001) | Yeah, if I read this right, all the RootLPad related changes are separable and are for reducing the number of TryBlockMapEntries. What do you think about separating the ip2state handler begin label from the rootlpad change? |
| lib/CodeGen/AsmPrinter/Win64Exception.cpp | ||
|---|---|---|
| 391 ↗ | (On Diff #25001) | I'd have to go back to my test cases to see if I can isolate the conditions that cause this. It was definitely happening. It's quite possible that this is an indication that the Unwind table was missing an entry. |
| lib/CodeGen/SelectionDAG/FunctionLoweringInfo.cpp | ||
| 588 ↗ | (On Diff #25001) | The purpose of the root lpad change is to allow us to recognize which handlers belong to the same try block. I wouldn't really just consider this an optimization. The numbering is logically wrong without it. That said, I'm not sure it's causing problems. I've tried a lot of variations of this code over the past week, and it's hard to keep straight which changes were necessary to fix issues with what's in trunk and which fixed problems I saw after making other changes. I know at some point things just didn't work at all without this change, but I just tested with it effectively disabled and it looks like it might be OK. I can pull this out of the current change set if you like. |
| lib/CodeGen/SelectionDAG/FunctionLoweringInfo.cpp | ||
|---|---|---|
| 588 ↗ | (On Diff #25001) | Like David, I expect there's a simpler way to figure out the state number of the first landingpad action list that mentioned any given handler, so I'd rather split it up and then we can bring it back with a more targetted test. The current numbering is consistent with rewriting trys with multiple catches like this: try { }
catch (A) { }
catch (B) { }
catch (C) { }Right now we number this like: try {
try {
try {
} catch (A) { /* except this is magically outside the catch B scope */ }
} catch (B) { /* ditto for C */ }
} catch (C) { }I think this numbering discrepancy doesn't result in a correctness problem, it just means our tables are weird. A TryBlockMap entry always has exactly one handler, instead of having three as MSVC would in this case. |
| lib/CodeGen/SelectionDAG/FunctionLoweringInfo.cpp | ||
|---|---|---|
| 588 ↗ | (On Diff #25001) | It's that "magically outside the scope" that concerns me. When I say that this is logically incorrect I am specifically trying to distinguish that from functionally incorrect. Until I have a specific test case that fails with our current behavior I'm willing to live with it. However, I think it's worth noting that our xdata tables are not strictly accurate. We are currently describing EH states that do not actually exist in the generated code. It may be that the current MSVCRT implementation works with this fictional description. That doesn't mean this isn't a potential fault line. So I'd like to eliminate this at some point, but I'm happy to put it off until everything else works. |
Comment Actions
Rebased to top of trunk
Removed code that wasn't required for the test case being added
Comment Actions
The main problem this change set is meant to address is that the EH states of catch handlers weren't being numbered correctly.
Remember the assertion that was removed earlier (CatchHigh > TryHigh). It turns out that is important.
| lib/CodeGen/AsmPrinter/Win64Exception.cpp | ||
|---|---|---|
| 338 ↗ | (On Diff #25100) | It isn't clear to me what this is doing. I strongly suspect that the hard-coded -1 here is incorrect. I added code which always adds an ip2state entry for the beginning of a function. It seems that this entry is meant to handle the case where this state is needed in the parent function. If this case is never hit, having the -1 state for the parent function may not be strictly necessary. I added it to mimic the behavior of MSVC. I suspect that this code will misbehave if we are in a catch handler. |
| lib/CodeGen/SelectionDAG/FunctionLoweringInfo.cpp | ||
| 433 ↗ | (On Diff #25100) | I removed this line of code because we weren't actually using a new state value here. The state number returned by currentEHNumber was previously claimed when the action at the top of the handler stack was pushed and the NextState value is incremented there. Incrementing it here results in an EH state value that doesn't appear anywhere in our tables. Honestly, I'm not entirely clear as to why an unwind entry is being created here. Near as I can tell we can only get here when a cleanup handler has been popped from the handler stack, as might happen at the first call site after a non-try scope ended in the middle of a try block. But wouldn't we have already created an unwind entry for that cleanup handler when it was pushed on the handler stack? |
| 481 ↗ | (On Diff #25100) | Here I'm claiming a new EH state but not creating an unwind entry. That may be the cause of the problem I saw with the MaxState value in the xdata table being larger than the size of the unwind map. Do I need to create an unwind entry here? |
Comment Actions
I see that this is wrong in the spots that I just commented on. Please ignore this review until I update it again.
| lib/CodeGen/AsmPrinter/Win64Exception.cpp | ||
|---|---|---|
| 338 ↗ | (On Diff #25100) | The -1 state always means "take no EH actions if this PC throws", so I think -1 is right. This code is basically searching for this pattern: invoke void @f() ; landingpad has EHState 0 ... call void @g() ; has *no* actions, so EHState -1 ... invoke void @h() ; landingpad has EHState 1 If we get to @h, and we saw a potentially throwing call instruction (@g) between the last invoke and the current invoke, then we should put a -1 entry in the table to prevent running actions from either @f or @h when @g throws. It doesn't matter if we're in a catch handler or the parent function. In practice, all calls in a deeply nested catch handler will have actions on the stack, so we'll never see a plain call that potentially throws. |
| lib/CodeGen/SelectionDAG/FunctionLoweringInfo.cpp | ||
| 433 ↗ | (On Diff #25100) | First, NextState should really be synced with UnwindMapEntries. States are really indices into the unwind map table, which gives you a cleanup action and a state transition that you follow. We should probably have createUnwindMapEntry return the next state instead of maintaining the number separately. I think the intention of this code was to recover from merging multiple catch action pushes into one, which optimistically assumes that two catch actions pushed at once must be part of a try / catch / catch sequence. In the code sample below, we see the first invoke, and only assign one new state number. We don't know until later that one catch is popped by itself, and that we'll need a new state number for the outer catch: try {
try {
may_throw(); // state 0
} catch (int) { }
may_throw(); // state 1
} catch (float) { }Until we start treating try / catch / catch specially, this code can probably be deleted. |
| 452 ↗ | (On Diff #25100) | wrap |
| 481 ↗ | (On Diff #25100) | Yes. States are indices into the UnwindMap. I don't think your base state assignment approach actually works in the face of basic block reordering. It's possible to push and pop a handler twice after CFG reordering. Suppose we have code like this: void foo() {
try {
f();
if (unlikely_cond)
g();
} catch (int) {
int_handler();
}
h();
}For whatever reason, IR passes have reordered the CFG like this: invoke @f() ; catch int br i1 unlikely_cond %later, %cont cont: call @h() ret void later: invoke @g() ; catch int br label %cont WinEHPrepare will outline exactly one catch handler, but WinEHNumbering will push and pop that one catch handler twice, and we'll assign the base state twice, when we only need to do it once. Also, I don't understand why catch handlers need their own new state, but maybe that's just because I don't understand what the runtime is really doing. It would seem like a catch handler is actually in the EH state of the code before the try, which is just -1 for code at the top level of a function. Maybe the runtime just needs a new state here. Anyway, inventing a new state is certainly a valid way to ensure that CatchHigh is greater than TryHigh. :) |
| lib/CodeGen/AsmPrinter/Win64Exception.cpp | ||
|---|---|---|
| 338 ↗ | (On Diff #25100) | I see what you're saying. I just put together a test case that hits this code in a catch handler, and I agree that using -1 here would produce the correct results. My only objection, and this is more principle than practical, is that the MS runtime seems to assume that catch handlers will have a range of states from TBME.TryHigh+1 to TBME.CatchHigh. I'm not sure there's anything that absolutely depends on this, but that seems to be the general expectation. My concern is that even if we can get away with not doing things that way now we may fail with the next version of the runtime. |
| lib/CodeGen/SelectionDAG/FunctionLoweringInfo.cpp | ||
| 433 ↗ | (On Diff #25100) | I'm not sure I understand your example. If we introduced code similar to what I had originally proposed where both catches in the following case would be assigned the same state try {
may_throw();
} catch (int) {
} catch (float) { }that code would also assign the same state to both catches in this case if nothing in the first catch handler used a landing pad: try {
try {
may_throw();
} catch (int) { }
} catch (float) { }But that would be OK because nothing needs the other state. However, in your example: try {
try {
may_throw(); // state 0
} catch (int) { }
may_throw(); // state 1
} catch (float) { }The code that pushes those handlers on the handler stack would give them different states. Am I missing something? In any event, I did delete this code in the new revision I uploaded last night. |
| 481 ↗ | (On Diff #25100) | Yes, my update last night add a call to create an unwind entry here. As for the catch handler getting a base state, partly this change is part of my efforts to get our numbering to behave more like I perceive the MS runtime to be expecting. I believe this also fixes some issues. In particular, if a catch handler contains no landing pads our current implementation will record an incorrect value for TBME.CatchHigh (which is based on NextState at the end of the handler processing). I've seen actual test cases where this caused the runtime to claim not to be able to find a handler for a state that had a handler. That said, there are serious problems with our current numbering. As I mentioned before, this patch was just meant to get some things I had in progress committed while I continued to work on other problems. The block re-ordering problem you describe is interesting. I have been working on a couple of other cases that break our current model even without unusual block ordering. This first one looks more or less like this: void test() {
try {
f();
try {
g();
} catch (...) {}
h();
} catch (int) {}
}In this case, the "catch (int)" handler disappears from the action list during the call to g() but then reappears at h(). The second case looks something like this: void test() {
try {
try {
try {
a();
} catch (int x) {}
b();
}
catch (char c) {}
e();
} catch(int x) {
} catch(...) {}
}There seems to be some variation in the code that clang generates for this and I haven't been able to isolate the conditions that determine it, but I have a test case like this where at a() the action list is: catch (int) // The first "catch (int)" handler catch (char) catch (...) Then at b() the action list is catch (char) catch (int) // The second "catch (int)" handler catch (...) This leads our current code to pop the "catch (char)" handler from the stack (and perform all the associated actions) and immediately re-push it because its position in the list changes. I have an idea for how I can fix these problems, but it isn't terribly elegant. If it works, I may want to put it in place anyway until we can design something better. |
| lib/CodeGen/AsmPrinter/Win64Exception.cpp | ||
|---|---|---|
| 338 ↗ | (On Diff #25100) | Hm, if they do assume catch states are in the range (TryHigh, CatchHigh], that could be a real problem. Man, this is the most inflexible, cantankerous scheme ever. :( |
| lib/CodeGen/SelectionDAG/FunctionLoweringInfo.cpp | ||
| 433 ↗ | (On Diff #25100) | I think we're on the same page. This code is a poor attempt to recover from a situation caused by our previous failed attempt to optimize TBME generation for try / catch / catch. If you have a better way to handle try / catch / catch, we don't need this. |
| 481 ↗ | (On Diff #25100) | I think in these situations it's just unrealistic to expect us to be able to get the same numbering with our current EH model. We might want to consider changing the model if these issues are insurmountable. |
| 468 ↗ | (On Diff #25131) | I guess there's no harm in giving out base states, but this means we're giving out a state that never gets used between the start of the catch handler and the first call site. No exception can occur there, so we don't really need anything in the ip2state table, right? We only need a base state if the catch handler has nothing in it. |