Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Landing pad wrongly removed #1794

Closed
llvmbot opened this issue May 15, 2007 · 9 comments
Closed

Landing pad wrongly removed #1794

llvmbot opened this issue May 15, 2007 · 9 comments
Labels
bugzilla Issues migrated from bugzilla

Comments

@llvmbot
Copy link
Collaborator

llvmbot commented May 15, 2007

Bugzilla Link 1422
Resolution FIXED
Resolved on Feb 22, 2010 12:50
Version unspecified
OS Linux
Attachments testcase .ll
Reporter LLVM Bugzilla Contributor
CC @asl

Extended Description

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...

@llvmbot
Copy link
Collaborator Author

llvmbot commented May 25, 2007

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.

@llvmbot
Copy link
Collaborator Author

llvmbot commented May 26, 2007

I will look.

@llvmbot
Copy link
Collaborator Author

llvmbot commented May 31, 2007

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).

@llvmbot
Copy link
Collaborator Author

llvmbot commented May 31, 2007

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.

@llvmbot
Copy link
Collaborator Author

llvmbot commented May 31, 2007

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.

@llvmbot
Copy link
Collaborator Author

llvmbot commented Jun 1, 2007

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.

@llvmbot
Copy link
Collaborator Author

llvmbot commented Jun 1, 2007

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).

@llvmbot
Copy link
Collaborator Author

llvmbot commented Nov 26, 2021

mentioned in issue llvm/llvm-bugzilla-archive#1444

@llvmbot llvmbot transferred this issue from llvm/llvm-bugzilla-archive Dec 3, 2021
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugzilla Issues migrated from bugzilla
Projects
None yet
Development

No branches or pull requests

1 participant