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

[Ten Tzen via Phabricator via cfe-commits]

tentzen marked an inline comment as not done.
tentzen added a comment.

thank you for the thorough review again.  My answer for each comment below:



================
Comment at: clang/lib/CodeGen/CGCleanup.cpp:1341
+  llvm::FunctionCallee SehCppScope =
+      CGM.CreateRuntimeFunction(FTy, "llvm.seh.scope.begin");
+  EmitSehScope(*this, SehCppScope);
----------------
rjmccall wrote:
> We generally prefer to get intrinsic functions with `CGM.getIntrinsic`.
Does this really matter? there are more than 200 uses of CGM.CreateRuntimeFunction().


================
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();
----------------
rjmccall wrote:
> 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?)
Yes, MSVC terminate() runtime bypasses HW exceptions to its caller.
Hmm, it's been a whole. I think I placed that code there because Clang's terminate runtime does not dispatch HW exception to caller when I tried a year ago.  It issues an unhandled exception.  I felt if a user explicitly specify -EHa, HW exception probably is more significant than C++ noexcept/nothrow directive.
Anyways, I can undo this code and let terminate-runtime handler it one way or the other.


================
Comment at: clang/lib/CodeGen/CGException.cpp:554
+  if (isNoexceptExceptionSpec(EST) && Proto->canThrow() == CT_Cannot &&
+      !EHStack.empty() /* possible empty when -EHa */) {
     EHStack.popTerminate();
----------------
rjmccall wrote:
> 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.
will do.


================
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));
----------------
rjmccall wrote:
> Please use `dyn_cast` for all of these.
ok will fix them. thanks.


================
Comment at: clang/lib/CodeGen/CGException.cpp:1678
+      VolatilizeTryBlocks(TI->getSuccessor(I), V);
+  }
+}
----------------
rjmccall wrote:
> 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.
A _try region is a Single Entry Multiple Exits regions. this code starts from Entry block and follows control-flow to reach all successors.  Yes a block could have multi-predecessors.  Note the 2nd line of this function: a visit flag is marked and checked
   !V.insert(BB).second /* already visited */ 

As long as it follows control-flows, it does not matter what the lexical order is.


================
Comment at: clang/lib/CodeGen/CGException.cpp:603
+
+      //  For IsEHa catch(...) must handle HW exception
+      //  Adjective = HT_IsStdDotDot (0x40), only catch C++ exceptions
----------------
rjmccall wrote:
> 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.
Yes, will do. thanks.


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


================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:2786
   Opts.CXXExceptions = Args.hasArg(OPT_fcxx_exceptions);
+  Opts.EHAsynch = Args.hasArg(OPT_feh_asynch);
 
----------------
rjmccall wrote:
> 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?
are you sure it's better to emit an error? I think some users would like it being ignored on non-Window platforms so they can have a single option set across platforms. 


================
Comment at: clang/lib/Sema/JumpDiagnostics.cpp:935
+    LabelStmt *Label = cast<LabelStmt>(To);
+    Label->setSideEntry(true);
   }
----------------
rjmccall wrote:
> 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`?
A _try scope must be a (SEME) Single-Entry-Multi-Exits region.  Branching into a _try is not allowed; it triggers an error. Clang handles it properly.
What we want to flag here is an branch (either initiated by a go-to/break/continue) from inner _try to outer _try. 
This flag is set in this If-statement because that is exactly the place issue the warning we want to catch. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80344