In the attached testcase, consider the following call to __gnat_rcheck_12: invoke void @__gnat_rcheck_12( i8* getelementptr ([13 x i8]* @.str, i32 0, i32 0), i32 103 ) to label %UnifiedUnreachableBlock unwind label %unwind869 [This is the only call taking 103 as last parameter]. After codegen with llc -enable-eh, this invoke has no entry in the eh table and the unwind block %unwind869 seems to have been deleted. This is unfortunate because when run this call is executed and raises an exception which is not caught...
Created attachment 843 [details] testcase .ll
I think I know what's going on, though I'm not sure. The landing pad MBB is empty: all the real work gets done in its successor. BranchFolding rewrites all the landing pad predecessors to go to its successor and then deletes the landing pad. However the landing pad was marked as being an eh landing pad, while its successor was not, so this information has been thrown away! [Probably the landing pad's fallthrough should simply be marked as being a landing pad if the previous one was too - but what about all the associated landing pad info, typeinfos and so forth?] Anyway, the fallthrough block is then optimized, and I notice that several adjustments are done to it that normally shouldn't be done because it is a landing pad (but not marked as one). I'm talking of this stuff: if (!MBB->isLandingPad()) { // Check all the predecessors of this block. If one of them has no fall // throughs, move this block right after it. At some point the block is removed entirely.
I will look.
> I will look. Thanks. I now think the origin of the problem is much earlier: the IsLandingPad flag and the landing pad label have become disassociated - the flag is on one (empty) block while the label is in another (the successor of the empty block).
Sorry not to have gotten to this sooner; I've been sick. The last thing you said is true, and this won't work until that is fixed. Whoever creates cond_true868.unwind869_crit_edge needs to fix up the EH tables; I think that's in the loop optimizer somewhere.
But the first thing you said is also true, and the easiest way to fix it is not to optimize away a forwarder block that's a landing pad. The extra branch, at most, is not going to be critical in exception-handling code. I'll get that in shortly. It's possible there are other issues with EH in there, but it's probably good to get the loop opt fixed next and see if that helps.
Hi Dale, I hope you're feeling better! > Whoever creates cond_true868.unwind869_crit_edge needs to fix up the > EH tables It looks to me that this problem comes from instruction lowering. When the invoke is lowered we already have this situation with an empty block inserted (presumably by the loop stuff): invoke_bb: ... invoke ... unwind %unwind.crit ... unwind.crit: ; preds %invoke_bb unwind: ; preds unwind.crit, ... call eh.exception... ... The lowering of invoke marks %unwind.crit as a landing pad. This seems perfectly logical to me. When eh.exception is lowered it does several things: (1) it adds a ISD::LABEL to %unwind. This is the label that the exception unwinder will jump to. This seems illogical to me. Why is the label being inserted by this intrinsic? It should surely be inserted by the invoke, and inserted in %unwind.crit. (2) it marks the exception register as live in on %unwind. Wouldn't it be more logical for the invoke lowering to do this an mark the register as live in on %unwind.crit? After all, this should be marked on the block which the unwinder jumps to, i.e. the same place the ISD::LABEL is inserted. (3) it inserts an EXCEPTIONADDR instruction (I think this just reads from the exception register). This seems ok to me. The only danger is if something clobbers the exception register between %unwind.crit and %unwind - not sure if there is really anything to worry about here. > But the first thing you said is also true... Yes, I agree. My understanding is that a MBB B is marked as a landing pad in order to indicate that one of its predecessors P jumps to it not from the end of P but maybe from some other point of P. For example, this means that if all predecessors of B finish with an "unreachable" instruction then you can't remove B if it is marked as being a landing pad. Given this, if B is a landing pad and you rewrite any predecessor of B so it now jumps to some other MBB then you need to mark that new MBB as being a landing pad too (it might not be - this is the conservative assumption). The question is, is it enough to copy the landing pad flag or do you have to worry about updating other funky exception handling stuff? I think the following invariant should hold: if a MBB contains an ISD::LABEL then the MBB must be marked as a landing pad. (However it is ok to have a MBB marked as a landing pad without containing an ISD::LABEL, because (a) ISD::LABEL guys are only created when you have a non-null MachineModuleInfo, and (b) if you copy landing pad flags around conservatively you could end up marking a MBB as a landing pad without it containing an ISD::LABEL). This invariant doesn't hold right now, see the first part of this comment, but I reckon that that is a bug. Suppose this invariant holds. Recall that the unwinder will jump to the MBB holding the ISD::LABEL. The question becomes: is it possible for BranchFolding to cause the ISD::LABEL to finish in a MBB which is not marked as a landing pad? No, because instructions don't get moved between MBBs (at least I don't think they do), and if a MBB is ever marked as a landing pad it never gets unmarked. It seems to me that this means that there is nothing to worry about: if you keep proper track of landing pad flags then the rest will take care of itself. OK, so now let me comment on this part of your patch (thanks for doing this!): // If this block is empty, make everyone use its fall-through, not the block - // explicitly. - if (MBB->empty()) { + // explicitly. Landing pads should not do this since the landing-pad table + // points to this block. + if (MBB->empty() && !MBB->isLandingPad()) { // Dead block? Leave for cleanup later. if (MBB->pred_empty()) return; I think you could instead just copy the landing pad flag to the new basic block. There are two cases: there is an ISD::LABEL in MBB. In this case the logic will never fire at all because MBB will not be empty! Or, there is no ISD::LABEL in MBB, in which case it doesn't much matter what you do! By the way, for the sake of uniformity I reckon it would be wise to always place an ISD::LABEL in every landing pad, whether or not we have any MachineModuleInfo. Then a "landing pad" is by definition something that contains such a label. It might even be possible to automatically maintain the landing pad flag: every time an ISD::LABEL is added to a MBB a counter is incremented in the MBB; everytime one is deleted from a MBB the counter is decremented. Then IsLandingPad can be implemented in terms of this counter: is it > 0 or not? I don't know if this kind of thing is feasible in LLVM.
You may be correct that it's safe just to transfer the landing-pad bit; the change I put in is conservatively correct. I'd suggest getting this to work first and worrying about that optimization later (there is at most one extra jump instruction).
Fixed by: http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20070528/050117.html http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20070604/050276.html Testcase: http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20070604/050275.html