[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/AsmPrinter/AsmPrinter.cpp:1316
+        //  Div with variable opnds won't be the first instruction in
+        //  an EH region as it must be led by at least a Load
+        {
----------------
If the issue here that the "end" label for a region can accidentally include the following instruction after the region?  Would it make sense to explicitly insert a nop earlier, in that case?


================
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;
----------------
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.)


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp:1323
+    MachineBasicBlock *MBB = &*MBBI;
+    const BasicBlock *BB = MBB->getBasicBlock();
+    int State = EHInfo->BlockToStateMap[BB];
----------------
getBasicBlock() can return null, in general.  Maybe since this is running during isel, that isn't an issue, though.  Maybe worth adding a comment.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp:1341
+      MachineInstr *MIe = &*(--MBBe);
+      // insert before (possible multiple) terminators
+      while (MIe->isTerminator())
----------------
If a load is folded into the terminator, it can fault... but I guess that's unlikely to come up in practice.  The most likely scenario probably involves indirectbr.  Most other indirect jumps probably aren't terminators. I guess a jump table could theoretically fault, but if you cared you could probably just disable those.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102817