rsmith added a comment.
Thanks, I think this is looking really good.
@rjmccall, no explicit need to review; I just wanted to make sure you'd seen
this and had a chance to express any concerns before we go ahead.
================
Comment at: clang/include/clang/Basic/AttrDocs.td:452
+optimizations are disabled. This guarantees that the call will not cause
+unbounded stack growth if it is part of a recursive cycle in the call graph.
+
----------------
One thing I'd add:
> If the callee is a virtual function that is implemented by a thunk, there is
> no guarantee in general that the thunk tail-calls the implementation of the
> virtual function, so such a call in a recursive cycle can still result in
> unbounded stack growth.
================
Comment at: clang/lib/CodeGen/CGCall.cpp:5318-5319
+ EHCleanupScope *Cleanup = dyn_cast<EHCleanupScope>(&*it);
+ assert(Cleanup && Cleanup->getCleanup()->isCallLifetimeEnd() &&
+ "found unexpected cleanup generating musttail exit");
+ }
----------------
Given the potential for mismatch between the JumpDiagnostics checks and this
one, especially as new more exotic kinds of cleanup are added, I wonder if we
should use an `ErrorUnsupported` here instead of an `assert`.
I strongly suspect we can still reach the problematic case here for a tail call
in a statement expression. I don't think it's feasible to check for all the
ways that an arbitrary expression context can have pending cleanups, which we'd
need in order to produce precise `Sema` diagnostics for that, so either we
handle that here or we blanket reject all `musttail` returns in statement
expressions. I think either approach is probably acceptable.
================
Comment at: clang/lib/CodeGen/CGCall.cpp:5315-5317
+ // TODO(haberman): insert checks/assertions to verify that this early exit
+ // is safe. We tried to verify this in Sema but we should double-check
+ // here.
----------------
haberman wrote:
> rsmith wrote:
> > haberman wrote:
> > > aaron.ballman wrote:
> > > > Are you planning to handle this TODO in the patch? If not, can you
> > > > switch to a FIXME without a name associated with it?
> > > I am interested in feedback on the best way to proceed here.
> > >
> > > - Is my assessment correct that we should have an assertion that
> > > validates this?
> > > - Is such an assertion reasonably feasible to implement?
> > > - Is it ok to defer with FIXME, or should I try to fix it in this patch?
> > >
> > > I've changed it to a FIXME for now.
> > Yes, I think we should validate this by an assertion if we can. We can
> > check this by walking the cleanup scope stack (walk from
> > `CurrentCleanupScopeDepth` to `EHScopeStack::stable_end()`) and making sure
> > that there is no "problematic" enclosing cleanup scope. Here, "problematic"
> > would mean any scope other than an `EHCleanupScope` containing only
> > `CallLifetimeEnd` cleanups.
> >
> > Looking at the kinds of cleanups that we might encounter here, I think
> > there may be a few more things that Sema needs to check in order to not get
> > in the way of exception handling. In particular, I think we should reject
> > if the callee is potentially-throwing and the musttail call is inside a try
> > block or a function that's either noexcept or has a dynamic exception
> > specification.
> >
> > Oh, also, we should disallow musttail calls inside statement expressions,
> > in order to defend against cleanups that exist transiently within an
> > expression.
> I'm having trouble implementing the check because there doesn't appear to be
> any discriminator in `EHScopeStack::Cleanup` that will let you test if it is
> a `CallLifetimeEnd`. (The actual code just does virtual dispatch through
> `EHScopeStack::Cleanup::Emit()`.
>
> I temporarily implemented this by adding an extra virtual function to act as
> discriminator. The check fires if a VLA is in scope:
>
> ```
> int Func14(int x) {
> int vla[x];
> [[clang::musttail]] return Bar(x);
> }
> ```
>
> Do we need to forbid VLAs or do I need to refine the check?
>
> It appears that `JumpDiagnostics.cpp` is already diagnosing statement
> expressions and `try`. However I could not get testing to work. I tried
> adding a test with `try` but even with `-fexceptions` I am getting:
>
> ```
> cannot use 'try' with exceptions disabled
> ```
> Do we need to forbid VLAs or do I need to refine the check?
Assuming that LLVM supports musttail calls from functions where a dynamic
alloca is in scope, I think we should allow VLAs. The `musttail` documentation
doesn't mention this, so I think its OK, and I can't think of a good reason why
you wouldn't be able to `musttail` call due to a variably-sized frame.
Perhaps a good model would be to add a virtual function to permit asking a
cleanup whether it's optional / skippable.
> I could not get testing to work.
You need `-fcxx-exceptions` to use `try`. At the `-cc1` level, we have
essentially-orthogonal settings for "it's valid for exceptions to unwind
through this code" (`-fexceptions`) and "C++ exception handling syntax is
permitted" (`-fcxx-exceptions`), and you usually need to enable both for
CodeGen tests involving exceptions.
================
Comment at: clang/lib/Sema/SemaStmt.cpp:583-586
+ if (!Ex) {
+ assert(hasUncompilableErrorOccurred() && "expected previous error");
+ return false;
+ }
----------------
I would have thought this assert would fire for `void f() { [[clang::musttail]]
return; }`. If so, we should reject this case with a diagnostic.
================
Comment at: clang/lib/Sema/SemaStmt.cpp:588
+
+ const CallExpr *CE = dyn_cast<CallExpr>(Ex->IgnoreUnlessSpelledInSource());
+
----------------
`IgnoreUnlessSpelledInSource` is a syntactic check that's only really intended
for tooling use cases; I think we want something a bit more semantic here, so
`IgnoreImplicitAsWritten` would be more appropriate.
I think it would be reasonable to also skip "parentheses" here (which we treat
as also including things like C's `_Generic`). Would
`Ex->IgnoreImplicitAsWritten()->IgnoreParens()` work?
If we're going to skip elidable copy construction of the result here (which I
think we should), should we also reflect that in the AST? Perhaps we should
strip the return value down to being just the call expression? I'm thinking in
particular of things like building in C++14 or before with
`-fno-elide-constructors`, where code generation for a by-value return of a
class object will synthesize a local temporary to hold the result, with a final
destination copy emitted after the call. (Testcase: `struct A { A(const A&); };
A f(); A g() { [[clang::musttail]] return f(); }` with
`-fno-elide-constructors`.)
================
Comment at: clang/lib/Sema/SemaStmt.cpp:596
+ if (!CE->getCalleeDecl()) {
+ assert(hasUncompilableErrorOccurred() && "expected previous error");
+ return false;
----------------
A call expression doesn't necessarily have a known callee declaration. I would
expect this assert to fire on a case like:
```
void f() {
void (*p)() = f;
[[clang::musttail]] return p();
}
```
We should reject this with a diagnostic.
================
Comment at: clang/lib/Sema/SemaStmt.cpp:619-620
+ if (dyn_cast<CXXConstructorDecl>(CMD) || dyn_cast<CXXDestructorDecl>(CMD))
{
+ Diag(St->getBeginLoc(), diag::err_musttail_structors_forbidden) << &MTA;
+ Diag(CMD->getBeginLoc(), diag::note_musttail_structors_forbidden);
+ return false;
----------------
Please pass in a flag here so the diagnostic can `%select` and produce a more
specific description of the problem.
================
Comment at: clang/lib/Sema/SemaStmt.cpp:631
+ // Caller is an Obj-C block decl: ^(void) { /* ... */ }
+ assert(dyn_cast<BlockDecl>(CurContext) && "unexpected decl context");
+ Diag(St->getBeginLoc(), diag::err_musttail_from_block_forbidden) << &MTA;
----------------
There are a couple of other contexts that can include a return statement: the
caller could also be an `ObjCMethodDecl` (an Objective-C method) or a
`CapturedDecl` (the body of a `#pragma omp` parallel region). I'd probably use
a specific diagnostic ("cannot be used from a block" / "cannot be used from an
Objective-C function") for the block and ObjCMethod case, and a
nonsepcific-but-correct "cannot be used from this context" for anything else.
================
Comment at: clang/test/CodeGen/attr-musttail.cpp:72
+
+// CHECK: musttail call void @_Z11ReturnsVoidv()
+
----------------
haberman wrote:
> rsmith wrote:
> > For completeness, can we also get a `CHECK-NEXT: ret void` here too?
> I turned on LLVM verification via `opt` so I think this should get verified
> by the IR verifier. Is that sufficient?
We don't like Clang's tests depending on `opt` in general, but I think in this
case it's an acceptable crutch until we fix Clang to run the verifier on its IR
output again (as discussed offline, it looks like we lost that as part of the
transition to the new pass manager). Please add a FIXME to remove the call to
`opt` once that bug is fixed. Other than that, I'm fine with this approach.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D99517/new/
https://reviews.llvm.org/D99517
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
