[PATCH] D80344: [Windows SEH]: HARDWARE EXCEPTION HANDLING (MSVC -EHa) - Part 1

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

I've made some brief comments about the code, but I think I have much larger concerns here.

The first is whether LLVM really intends to satisfy the constraints necessary to make these exceptions work, which I don't think you've gotten clear consensus about at all.  Unfortunately, llvm-dev is not an effective place to gather this kind of consensus.  For a change of this magnitude, I think you need to be proactively gathering a group of interested parties to sort out whether this is something we should support, not just expecting people to reply to an RFC.

The second is that I think the concept of block-level tracking state / control flow is potentially really problematic for LLVM, and the entire design here seems contingent on it.  Again, this is something we need to ensure consensus on, not something we can really just sign off on in code review.

The third is that I'm not really convinced that the way we handle cleanups in Clang today is sufficiently "atomic" in design to accommodate this.  I need to think about it.



================
Comment at: clang/include/clang/Basic/LangOptions.def:131
 LANGOPT(CXXExceptions     , 1, 0, "C++ exceptions")
+LANGOPT(EHAsynch          , 1, 0, "C/C++ EH Asynch exceptions")
 LANGOPT(DWARFExceptions   , 1, 0, "dwarf exception handling")
----------------
This is usually shortened as "async", but I'm not sure why it's being shortened at all.  Also, hardware faults are synchronous.  Also, it applies in all language modes.


================
Comment at: clang/lib/CodeGen/CGCleanup.cpp:769
+    // Under -EHa, invoke eha_scope_end() to mark scope end before dtor
+    bool IsEHa = getLangOpts().EHAsynch && !Scope.isLifetimeMarker();
+    const EHPersonality &Personality = EHPersonality::get(*this);
----------------
Readers of this code shouldn't have to remember what cl.exe command-line flags mean.  Please call this something like `RequiresSEHScopeEnd`.


================
Comment at: clang/lib/CodeGen/CGCleanup.cpp:783
+          EmitSehTryScopeEnd();
+        }
 
----------------
Please put braces around the outer `if`, and please be consistent about braces for the inner clauses.


================
Comment at: clang/lib/CodeGen/CGCleanup.cpp:1305
+static void EmitSehEHaScope(CodeGenFunction &CGF,
+                            llvm::FunctionCallee &SehCppScope) {
+  llvm::BasicBlock *InvokeDest = CGF.getInvokeDest();
----------------
I think you should take the callee by value.  And please rename this to not use "EHa".


================
Comment at: clang/lib/CodeGen/CGDecl.cpp:2008
+      EmitSehCppScopeBegin();
+  }
+
----------------
This seems like a pretty random place to do this.  Presumably this need to be tied to basically any scope with EH protection?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80344