[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:
> > 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?


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:2916
+      if (CleanupMBB) // a block referenced by EH table
+        CleanupMBB->setHasAddressTaken(); // so dtor-funclet not removed by opts
       break;
----------------
efriedma wrote:
> tentzen wrote:
> > efriedma wrote:
> > > tentzen wrote:
> > > > efriedma wrote:
> > > > > I assume this is trying to trick optimizations into avoiding optimizations on the cleanup block if the __try involves multiple basic blocks.
> > > > > 
> > > > > Have you considered trying to fix the control flow graph?  I mean, I understand the difficulty of expressing the control flow at the IR level, given we don't have IR versions of "load" and "store" with an unwind edge, but those concerns don't really apply in the same way in a MachineFunction.  You can mark the cleanup block as a successor of any block that can throw an exception, without explicitly marking up the control flow on each individual instruction.  (This is how exceptions normally work.)
> > > > The Cleanup block is turned into a dtor funclet at the end. And the beginning address of that funclet will be recorded in EH table in .xdata section. So it's really address-taken by definition. If you verify this by checking Linker relocation records or the EH table in .asm file.  IMO this flag should be set regardless of the SEH feature.
> > > That's not what "address-taken" is supposed to mean.
> > > 
> > > It's supposed to be equivalent to BasicBlock::hasAddressTaken, which means that the block is the target of a blockaddress expression (used for indirectbr).  Primarily, this restricts certain optimizations because in general, we can't split a critical edge from an indirectbr.
> > > 
> > > hasAddressTaken() is not a general "will the address of the block eventually be emitted somewhere in the assembly".  That can happen for a variety of unrelated reasons, including exception handling, debug info, jump tables, and branch lowering.
> > I'm not sure about your interpretation.  the comment of this flag from the header is:
> >   
> >   /// Returns true if there are any uses of this basic block other than
> >   /// direct branches, switches, etc. to it.
> >   bool hasAddressTaken() const {
> >     return getBasicBlockBits().BlockAddressRefCount != 0;
> >   }
> > it does include "switch" which I think refers to jump-table.
> > Besides, the dtor-funclet in EH table is invoked via indirectbr/indirect-call.  Does not that also meet your interpretation?
> > 
> "other than direct branches, switches, etc." means MachineBasicBlock terminators, exception pads, and jump tables.  Anything which is *not* one of those, we mark up, because target-independent code can't analyze it.  But in almost all cases, the above list covers basically everything except indirectbr until you get very late in the compile process.
> 
> If we do end up performing some transform that materializes the address of a block outside of the above cases, it could make sense to mark a block address-taken.  (Some target-specific code does this.  Not sure if that code is really doing it correctly, but none of those calls are relevant to this patch.)  But we should not be preemptively marking basic block address-taken: we should wait until such a transform is actually performed.
> 
> For a dtor-funclet, we don't actually "take the address" of the funclet until the AsmPrinter, when we compute the exception tables.  We could add an address-taken marking at that point... but nothing would check it that late in the pipeline.
This is exactly the case that a target-independent code (like BranchFolding.cpp) must be aware of it. 
The dtor-funclet cleanup block is genuinely address-taken the moment it's created.
Yes, it's not emitted until AsmPrinter. but it does not mean the block is not address-taken until AsmPrinter. 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102817