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

Fri Oct 1 12:53:14 PDT 2021

JosephTremoulet added inline comments.

================
Comment at: llvm/include/llvm/CodeGen/SelectionDAGISel.h:332

+  // Mark and Report IPToState for each Block under IsEHa
+  void reportIPToStateForBlocks(MachineFunction *Fn);
----------------
In the previous checkin, you went through and replaced the term `EHa` with `asyncEH` or similar in all identifiers and comments, didn't you?  If so, I think it would be good to do the same with this one.

================
Comment at: llvm/include/llvm/CodeGen/WinEHFuncInfo.h:128-132
+// For IsEHa
+void calculateCXXStateForBlocks(const BasicBlock *BB, int State,
+                                WinEHFuncInfo &FuncInfo);
+void calculateSEHStateForBlocks(const BasicBlock *BB, int State,
+                                WinEHFuncInfo &FuncInfo);
----------------
Here you've got the signature and the name both indicating that this works at a block scope, and a comment indicating that it is for EHa.  I wonder if it wouldn't be better to have the signature indicate block scope and use the name to indicate EHa, something like `calculateCXXStateForAsyncEH`.

================
Comment at: llvm/include/llvm/IR/BasicBlock.h:198

+  /// Returns the first potential -EHa faulty instruction
+  const Instruction *getFirstFaultyInst() const;
----------------
To me, the word "faulty" implies that a fault definitely exists.  I think that `getFirstMayFaultInst`, though perhaps more awkward, would be more clear.

================
Comment at: llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:1305
+        //   (see MCConstantExpr::create(1,..) in WinException.cpp)
+        //  Ignore SDiv/UDiv bcause a DIV with Const-0 divisor
+        //    must have being turned into an UndefValue.
----------------
majnemer wrote:
> nit: bcause -> because
what if the divisior is not a constant but evaluates to 0 at run-time?  I assume that's handled differently, but without context I wouldn't know where, so maybe mentioning in the comment would be good.

================
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);
----------------
do the tests (either the new ones you're adding or existing ones) exercise the case where we need this?

================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:2887-2888
     case Intrinsic::seh_try_end:
     case Intrinsic::seh_scope_end:
+      if (CleanupMBB) // a CleanupPad, referenced by EH table
+        CleanupMBB->setHasAddressTaken(); // so dtor-funclet not removed by opts
----------------
How do we know that this is a CleanupPad and not a Catchswitch?

================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp:1325-1327
+      // insert before (possible multiple) terminators
+      while (MIb->isTerminator())
+        MIb = &*(--MBBe);
----------------
How do we know this doesn't wind back out of the block we care about?  Is it worth adding an assertion?

================
Comment at: llvm/lib/CodeGen/WinEHPrepare.cpp:253-256
+  // Continue traveling successors recursively
+  for (auto *SuccBB : successors(BB)) {
+    calculateCXXStateForBlocks(SuccBB, State, EHInfo);
+  }
----------------
I'm surprised this doesn't cause a problem for super-deep CFGs (pathological cases); consider using a worklist.

================
Comment at: llvm/lib/CodeGen/WinEHPrepare.cpp:267-269
+//   Side exits can ONLY jump into parent scopes (lower state number).
+//   Thus, when a block succeeds various states from its predecessors,
+//     the lowest State triumphs others.
----------------
I think you're saying here that it's legal for a side exit to target a successor with any ancestor state.  But also you're assigning each such successor the exact parent state of its least predecessor ("least" by comparing state numbers).  How do you know that it shouldn't be the grandparent (or any other ancestor) of the least predecessor?

================
Comment at: llvm/lib/CodeGen/WinEHPrepare.cpp:269
+//   Thus, when a block succeeds various states from its predecessors,
+//     the lowest State triumphs others.
+//   If some exits flow to unreachable, propagation on those paths terminate,
----------------
nit: should be "trumps others" or "triumphs over others"

================
Comment at: llvm/lib/CodeGen/WinEHPrepare.cpp:272
+//     not affecting remaining blocks.
+// NOLINTNEXTLINE: Suppress clang-tidy recursive warning
+void llvm::calculateSEHStateForBlocks(const BasicBlock *BB, int State,
----------------
Oh, I didn't notice this when I wrote the comment above.  I agree with clang-tidy on this one (for both of these functions).

================
Comment at: llvm/lib/IR/BasicBlock.cpp:212

+const Instruction *BasicBlock::getFirstFaultyInst() const {
+  if (InstList.empty())
----------------
I didn't notice when commenting on this name earlier that this method is on the IR `BasicBlock` class.  I don't know if there's a better term to use for this than "fault".  Regardless, I think it would be good to make the comment on the declaration very explicit that this checks for loads/stores (which may dereference a null pointer) and calls/invokes (which may propagate exceptions), since I don't think we have a well-defined IR concept for this particular property of those instructions.  (FWIW, The last time I went looking, the closest I could find was the logic in SimplifyCFG that decides how much code it can remove before an unconditional branch to `unreachable`, which was open-coded.)

================
Comment at: llvm/lib/IR/BasicBlock.cpp:216
+  for (const Instruction &I : *this)
+    if (isa<LoadInst>(I) || isa<StoreInst>(I) || isa<CallInst>(I))
+      return &I;
----------------
Did you mean `CallBase` here?  This will skip `InvokeInst`s (and `CallBrInst`s).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102817