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

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.


================
Comment at: clang/lib/CodeGen/CGCleanup.cpp:1341
+  llvm::FunctionCallee SehCppScope =
+      CGM.CreateRuntimeFunction(FTy, "llvm.seh.scope.begin");
+  EmitSehScope(*this, SehCppScope);
----------------
We generally prefer to get intrinsic functions with `CGM.getIntrinsic`.


================
Comment at: clang/lib/CodeGen/CGException.cpp:465
     if (const CapturedDecl* CD = dyn_cast_or_null<CapturedDecl>(D)) {
-      if (CD->isNothrow())
+      if (CD->isNothrow() && !getLangOpts().EHAsynch /* !IsEHa */)
         EHStack.pushTerminate();
----------------
Please remove the comment here.  The option name should be sufficiently self-descriptive.

Anyway, I don't think this change is right, because we *do* still need to push a terminate scope: we need C++ exceptions to trigger a call to `std::terminate`.  It's just that such scopes aren't fully terminal when async exceptions are enabled, because MSVC defines those exceptions as passing through `noexcept` and so on.  (I assume that's true; can you link to documentation about it?)


================
Comment at: clang/lib/CodeGen/CGException.cpp:554
+  if (isNoexceptExceptionSpec(EST) && Proto->canThrow() == CT_Cannot &&
+      !EHStack.empty() /* possible empty when -EHa */) {
     EHStack.popTerminate();
----------------
Again, please try to refer to this in a more driver-agnostic way: "under async exceptions" rather than "when -EHa".  But actually as mentioned above I think this is incorrect.


================
Comment at: clang/lib/CodeGen/CGException.cpp:1668
+      } else if (isa<llvm::MemIntrinsic>(J)) {
+        auto *MCI = cast<llvm::MemIntrinsic>(J);
+        MCI->setVolatile(llvm::ConstantInt::get(Builder.getInt1Ty(), 1));
----------------
Please use `dyn_cast` for all of these.


================
Comment at: clang/lib/CodeGen/CGException.cpp:1678
+      VolatilizeTryBlocks(TI->getSuccessor(I), V);
+  }
+}
----------------
Volatilizing every block that's reachable from the `try` block seems like it makes a lot of assumptions about where branches within the `try` can reach.  For example, a `goto` could certainly go to a block that's already been emitted, as could `break` or `continue` if the emitter just makes slightly different decisions about emission order.  Please look at how `FragileHazards` (in the ObjC emission code) collects blocks in order to do its transforms — I think you can probably extract a reasonable common base out.  Alternatively, I think you could handle this directly in the insertion callback (`CodeGenFunction::InsertHelper`) when we're in an appropriate `try` scope.


================
Comment at: clang/lib/CodeGen/CGException.cpp:603
+
+      //  For IsEHa catch(...) must handle HW exception
+      //  Adjective = HT_IsStdDotDot (0x40), only catch C++ exceptions
----------------
asmith wrote:
> nit - extra space after //
The comment here isn't explaining anything, it's just repeating what the code is doing.  If you want a useful comment, you could explain why it's important to mark the scope.


================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:6584
+    if (EH.Asynch)
+      CmdArgs.push_back("-feh-asynch");
   }
----------------
For consistency with the existing options, please spell this option `-fasync-exceptions`, and please spell the corresponding LangOption `AsyncExceptions`.


================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:2786
   Opts.CXXExceptions = Args.hasArg(OPT_fcxx_exceptions);
+  Opts.EHAsynch = Args.hasArg(OPT_feh_asynch);
 
----------------
You should emit an error if this is enabled on targets that are not in the appropriate Windows environment, since we don't (yet) support it there.  I assume that's just the MSVC Windows environment and that this can't easily be supported on e.g. MinGW?

Does it have other target restrictions, like i386-only?


================
Comment at: clang/lib/Sema/JumpDiagnostics.cpp:935
+    LabelStmt *Label = cast<LabelStmt>(To);
+    Label->setSideEntry(true);
   }
----------------
This doesn't seem like a reasonable assertion in the abstract.  Even if we really only currently emit warnings with jumps to labels, that doesn't seem like something we should write code that relies on.  And I'm sure this problem can come up with switch cases, unless that's structurally outlawed in some other way.

Also, you're making the correct setting of this flag dependent on whether we're emitting a warning vs. an error.  Seems like we should be setting it regardless.

What conditions exactly do you want this flag set on?  I would naturally assume it's specifically branches from a block outside the `try`, and you don't care about branches within the `try`?  If the label is used in multiple places, do you need to be careful to only set the flag on those branches that come from outside the `try`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80344