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

[Eli Friedman via Phabricator via llvm-commits]

efriedma 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);
----------------
tentzen wrote:
> 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.
I think you're misunderstanding what I mean by "control flow graph".

Every MachineBasicBlock has a "successor" list, and a "predecessor" list. These can be accessed by MachineBasicBlock::successors() and MachineBasicBlock::predecessors().  MachineBasicBlock::pred_empty() queries the predecessor list. This isn't directly tied to any instructions in the block itself; they're just `std::vector<MachineBasicBlock*>`.  But they reflect the possible directions for control flow.  For example, suppose you have an indirectbr.  This gets lowered to an indirect jump instruction; none of the destinations are directly encoded into the basic block.  However, the block has a successor list; the jump must jump to one of those successors, or the behavior is undefined.  And the destination has a predecessor list; the block with the indirectbr must be in that list.

This is a bit different from LLVM IR, where every control flow edge is explicitly attached to a terminator instruction.

This successor list is essential to the way MachineFunction IR works. Before register allocation, the IR is in SSA form; computing where an SSA value is live requires an accurate control flow graph.

If the EH runtime is jumping to a MachineBasicBlock with no predecessors, that means there's a missing call to MachineBasicBlock::addSuccessor() somewhere.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102817