[PATCH] D102817: [Windows SEH]: HARDWARE EXCEPTION HANDLING (MSVC -EHa) - Part 2

[Ten Tzen via Phabricator via llvm-commits]

tentzen added inline comments.


================
Comment at: llvm/lib/CodeGen/BranchFolding.cpp:1207
     // If it is dead, remove it.
-    if (MBB->pred_empty()) {
+    if (MBB->pred_empty() && !MBB->hasAddressTaken()) {
       RemoveDeadBlock(MBB);
----------------
efriedma wrote:
> tentzen wrote:
> > efriedma wrote:
> > > efriedma wrote:
> > > > tentzen wrote:
> > > > > efriedma wrote:
> > > > > > tentzen wrote:
> > > > > > > JosephTremoulet wrote:
> > > > > > > > do the tests (either the new ones you're adding or existing ones) exercise the case where we need this?
> > > > > > > Yes, the **Dtor one.  it's how we found this bug. thanks.
> > > > > > We intentionally do not check for `hasAddressTaken()` here. Even if something refers to the address, it's not legal to branch to the block, so it can't contain any useful instructions.  AsmPrinter::emitFunctionBody has some code to print a fake label if there's a reference to an erased block.
> > > > > Hmm, really? there are at least three places where hasAddressTaken() is used to guard optimizations in the same pass (BranchFolding.cpp).  why is OptimizeBranches() special?
> > > > The other places in the pass are checking hasAddressTaken() to try to figure out if they can rewrite the predecessors.  Here, we can trivially rewrite all the predecessors: there aren't any.
> > > I guess given that we have references to an MBB stored in FuncInfo, we can't really delete the block.  So I guess this is right, sort of.  But it would still be undefined behavior to jump to it, so I'm not sure this really solves whatever problem you ran into.
> > it's MBB.pred_empty(), so there is no edge jumping to it.
> > Without this change, an EH Cleanuppad block could be removed and EH runtime would not be able to dtor a live object during unwinding. 
> That's a contradiction: you're saying there's no edge jumping to it, but somehow the "EH runtime" is executing the code in the block.
> 
> In particular, even if you preserve the block, anything involving dominance relationships, like register allocation, is likely to destroy unreachable code.  Maybe you can sort of get away with it in trivial cases.
> 
> 
I meant no edge jumps into it through IR's control flow graph during compilation. EH runtime invokes it via an indirect branch during execution time.

It's the whole point of 'address-taken' attribute; it tells optimizations/transformations that although this block is unreached via IR control flow, its address is taken and runtime (or some other code) can invoke this block via an indirect branch.

Register-allocation, or and all other optimizations (like this BranchFolding) should check whether or not it's address-taken before optimizing it away.
If more cases are revealed, we should fix them.  'Retpoline' and other code are also relying on this flag to guard an unreachable block.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D102817/new/

https://reviews.llvm.org/D102817