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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
