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

[Phoebe Wang via Phabricator via llvm-commits]

pengfei added a comment.

Reverse ping @tentzen. Is there any block issues for it moving on? I don't see serious from a quick review, just some nits.



================
Comment at: llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:1624
+        //   (see MCConstantExpr::create(1,..) in WinException.cpp)
+        //  Ignore SDiv/UDiv because a DIV with Const-0 divisor
+        //  SDiv/UDiv because a DIV with Const-0 divisor
----------------
duplicated.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:1632
+          if (IsEHa && MI2 != MBB.end() &&
+              (MI2->mayLoadOrStore() || MI2->mayRaiseFPException()))
+            emitNops(1);
----------------
Check for call instructions?


================
Comment at: llvm/lib/CodeGen/AsmPrinter/WinException.cpp:774
+  if (MMI->getModule()->getModuleFlag("eh-asynch")) {
+    OS.emitInt32(0);
+  } else {
----------------
Check it in test case?


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:2964-2965
     case Intrinsic::seh_scope_end:
+      if (EHPadMBB)                     // a block referenced by EH table
+          // so dtor-funclet not removed by opts
+          EHPadMBB->setMachineBlockAddressTaken();
----------------
The comment format seems uncommon, reformat it?


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp:1289
+    return;
+  for (auto MBBI = MF->begin(); MBBI != MF->end(); ++MBBI) {
+    MachineBasicBlock *MBB = &*MBBI;
----------------
It's more preferred to use `.. E = MF->end(); MBBI != E; ..`


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp:1293
+    int State = EHInfo->BlockToStateMap[BB];
+    if (BB->getFirstMayFaultInst()) {
+      // Report IP range only for blocks with Faulty inst
----------------
Any reason this has to be done in `SelectionDAGISel`. If we do it after ISel, we can check `mayRaiseFPException` for FP instructions too.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp:1302-1303
+      MachineInstr *MIb = &*MBBb;
+      if (MIb->isTerminator())
+        continue;
+      BuildMI(*MBB, MBBb, SDB->getCurDebugLoc(),
----------------
Is there any chance a terminator will raise exception?
If we don't emit begin/end labels, why adding them to EHInfo by line 1297?


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp:1309-1311
+      // insert before (possible multiple) terminators
+      while (MIe->isTerminator())
+        MIe = &*(--MBBe);
----------------
You can use `getFirstTerminator`.


================
Comment at: llvm/lib/CodeGen/WinEHPrepare.cpp:232
+  SmallVector<struct WorkItem *, 8> WorkList;
+  struct WorkItem *WI = new WorkItem(BB, State);
+  WorkList.push_back(WI);
----------------
Didn't see its free. Would there be memory leak?


================
Comment at: llvm/lib/CodeGen/WinEHPrepare.cpp:254-256
+      if (Fn && Fn->isIntrinsic() &&
+          (Fn->getIntrinsicID() == Intrinsic::seh_scope_begin ||
+           Fn->getIntrinsicID() == Intrinsic::seh_try_begin))
----------------
There are many function calls, would be better to simplify to
```
if (Fn && Fn->isIntrinsic()) {
  Intrinsic::ID IID = Fn->getIntrinsicID();
  if (IID == ...)
```


================
Comment at: llvm/lib/CodeGen/WinEHPrepare.cpp:270
+    for (auto *SuccBB : successors(BB)) {
+      WI = new WorkItem(SuccBB, State);
+      WorkList.push_back(WI);
----------------
ditto.


================
Comment at: llvm/lib/IR/BasicBlock.cpp:212
 
+const Instruction *BasicBlock::getFirstFaultyInst() const {
+  if (InstList.empty())
----------------
tentzen wrote:
> 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!
We'd better use machine instruction infos.


================
Comment at: llvm/test/CodeGen/X86/windows-seh-EHa-CppCatchDotDotDot.ll:290
+!1 = !{i32 2, !"eh-asynch", i32 1}
+!2 = !{!"clang version 11.0.0 (https://github.com/llvm/llvm-project 7ee479a760e0a4402b4eb7fb6168768a44f66945)"}
+
----------------
This is useless and misleading. Ditto for other tests.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102817