[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:
> > > 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.
> > For Switch jump table, yes, pred. & succ. lists are well maintained because they are real control-flow.
> > 
> > We are talking about EH unwinding flow here. EH runtime cannot be in the list. it's why we rely on address-taken flag to convey this scenario to optimizations.
> > 
> We model EH unwinding flow similarly to any other control flow.  Take the following example:
> 
> ```
> echo "struct A {~A(); }; void f(); void g() { A a; f(); }" | clang++ --target=x86_64-pc-windows-msvc -x c++ - -o - -S -mllvm -stop-before=greedy
> ```
> 
> This dumps the MIR as it appears during register allocation.  If you look at the output, it contains:
> 
> ```
>   bb.0.entry:
>     successors: %bb.1(0x40000000), %bb.2(0x40000000)
> ```
> 
> bb.1 is the normal continuation, bb.2 is the cleanuppad.  bb.2 isn't directly reachable; the only way to jump to the block is through the exception unwinder.  But we still model the edge.
Yes, we all know that. that is the whole point of this design.
For Asynch Exception, this design only tracks EH state at scope (or block) level, not at instruction level. As such, after seh-scope-intrinsic is lowered, the design relies on address-taken flag to guard ehcleanup pads. 
Please review the design on the top.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102817