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

[Ten Tzen via Phabricator via llvm-commits]

tentzen marked 3 inline comments as done.
tentzen added inline comments.


================
Comment at: llvm/lib/CodeGen/WinEHPrepare.cpp:270
+    for (auto *SuccBB : successors(BB)) {
+      WI = new WorkItem(SuccBB, State);
+      WorkList.push_back(WI);
----------------
pengfei wrote:
> tentzen wrote:
> > pengfei wrote:
> > > ditto.
> > ditto.
> Still not see where it been freed. Should we push the object instead?
Oh, my overlooked. done.


================
Comment at: llvm/lib/IR/BasicBlock.cpp:212
 
+const Instruction *BasicBlock::getFirstFaultyInst() const {
+  if (InstList.empty())
----------------
pengfei wrote:
> tentzen wrote:
> > pengfei wrote:
> > > 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.
> > this is the style copied from neighborhood code. do you see any problem or potential bug?
> I'm concerned about the FP exceptions as explained above. Especially FP instructions are more target specific, e.g., given an integet to FP conversion, `CVTDQ2PD` doesn't raise any FP exception but `CVTDQ2PS` does.
> But I'm fine to improve it in a follow up.
ok, thanks.


================
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)"}
+
----------------
pengfei wrote:
> tentzen wrote:
> > pengfei wrote:
> > > This is useless and misleading. Ditto for other tests.
> > this is testing SEH related table and flag is put properly.
> > which part is misleading?
> I mean the `!2 = !{!"clang version 11.0.0 ...`
I see. done.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102817