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

[Ten Tzen via Phabricator via llvm-commits]

tentzen added a comment.

Joseph, 
I greatly appreciate your thorough review and insightful feedbacks.
new revision is provided. also review it again. 
thank you!



================
Comment at: llvm/include/llvm/CodeGen/SelectionDAGISel.h:332
 
+  // Mark and Report IPToState for each Block under IsEHa
+  void reportIPToStateForBlocks(MachineFunction *Fn);
----------------
JosephTremoulet wrote:
> 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.
good point. done. 


================
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);
----------------
JosephTremoulet wrote:
> 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`.
yes makes sense. thanks.


================
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.
----------------
JosephTremoulet wrote:
> 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.
as stated in last line comment, in that case the DIV will not be the first instruction in the EH region. we can skip Nop.. 


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


================
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
----------------
JosephTremoulet wrote:
> How do we know that this is a CleanupPad and not a Catchswitch?
fixed. thanks.


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


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


================
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.
----------------
JosephTremoulet wrote:
> 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?
the new state only assigned to successor when it's smaller than successor's current state.  See the first line in the while loop. 
if ( ... && EHInfo.BlockToStateMap[BB] <= State) ..


================
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,
----------------
JosephTremoulet wrote:
> 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).
done. recursion is replaced by a worklist loop..


================
Comment at: llvm/lib/IR/BasicBlock.cpp:212
 
+const Instruction *BasicBlock::getFirstFaultyInst() const {
+  if (InstList.empty())
----------------
JosephTremoulet wrote:
> 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.)
renamed it with getFirstMayFaultInst(). also added more comments in header.  thanks!


================
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;
----------------
JosephTremoulet wrote:
> Did you mean `CallBase` here?  This will skip `InvokeInst`s (and `CallBrInst`s).
good catch! done.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102817