[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/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:
> > > > > 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. 
> > 
> There's not really any point to arguing what the person who originally wrote the comment meant. Sorry, I shouldn't have tried to take the discussion in that direction.
> 
> But I still think this is confusing: there's multiple different use-cases smashed together into one boolean.
> 
> How about this: we split up isBlockAddressTarget(), isAsyncSEHTarget(), and maybe isMachineLabelUsed() for the target-specific stuff that messes with MachineBasicBlock labels in weird ways.  And then as a transitional measure hasAddressTaken() checks all of them, maybe?  Really, I expect most of the relevant places that would check hasAddressTaken(), or something like that.
> 
> Then we can go through later and refine various places to check exactly what they need.  I suspect most places don't actually need all three.
I feel doing that is over-engineering and I don't see much value out of it.

note that in addition to BasicBlock.h, it's also commented in MachineBasicBlock.h :

  /// Test whether this block is **potentially the target of an indirect branch.**
  bool hasAddressTaken() const { return AddressTaken; }

Also, some present usages are far from what you described. 
See the usages in RetpolineThunkInserter where CaptureSpec and CallTarget blocks are not even address-taken.

The usage of hasAddressTaken() in this revision is much more appropriate and legitimate.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102817