[PATCH] D124642: [WIP] Add support for return from an SEH __finally block.

[Reid Kleckner via Phabricator via cfe-commits]

rnk added a comment.

The approach makes sense to me, but can you include test cases to illustrate the IR and code that gets generated?



================
Comment at: clang/lib/CodeGen/CGException.cpp:1833
+/// Find all local variable captures in the statement.
+struct ReturnStmtFinder : ConstStmtVisitor<ReturnStmtFinder> {
+  bool ContainsRetStmt = false;
----------------
We have the option to generalize the naming here to include other kinds of jumps that exit the scope of the finally. I think it's worth implementing `__leave` eventually, which I think users have asked for at some point. I could go either way, your choice.


================
Comment at: clang/lib/CodeGen/CGException.cpp:2205
+void CodeGenFunction::EnterSEHTryStmt(const SEHTryStmt &S,
+                                      bool &ContainsRetStmt) {
   CodeGenFunction HelperCGF(CGM, /*suppressNewContext=*/true);
----------------
If we were to generalize now, we'd want to name this something like `HasLocalUnwind`.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/WinException.cpp:604-614
   // Iterate over all the invoke try ranges. Unlike MSVC, LLVM currently only
   // models exceptions from invokes. LLVM also allows arbitrary reordering of
   // the code, so our tables end up looking a bit different. Rather than
   // trying to match MSVC's tables exactly, we emit a denormalized table.  For
   // each range of invokes in the same state, we emit table entries for all
   // the actions that would be taken in that state. This means our tables are
   // slightly bigger, which is OK.
----------------
Perhaps it's time to revisit this.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/WinException.cpp:631-634
+      // Mark up the destination of _local_unwind so it doesn't unwind
+      // too far.
+      //
+      // FIXME: Can this overlap with the EH_LABEL for an invoke?
----------------
I don't understand what this is doing, but I should look at the tests.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:2942
+      if (!isa<CatchSwitchInst>(EHPadBB->getTerminator())) {
+        report_fatal_error("localunwind doesn't point to catchswitch");
+      }
----------------
Since this isn't verifier checked yet, is it possible to emit a diagnostic via the LLVMContext and select this instruction to nothing, or would that leave behind disconnected unreachable MBBs that break too many invariants later?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124642