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