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

Phoebe Wang via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Mon Nov 7 19:17:18 PST 2022


pengfei 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);
----------------
tentzen wrote:
> pengfei wrote:
> > ditto.
> ditto.
Still not see where it been freed. Should we push the object instead?


================
Comment at: llvm/lib/IR/BasicBlock.cpp:212
 
+const Instruction *BasicBlock::getFirstFaultyInst() const {
+  if (InstList.empty())
----------------
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.


================
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)"}
+
----------------
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 ...`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102817



More information about the llvm-commits mailing list