rsmith added a comment.
I've tried to think of some more exotic corner cases. I'd like to see a test
for this:
void f(); struct A { ~A() { [[clang::musttail]] return f(); } };
... even though we reject that for a reason unrelated to musttail. We should
also reject this:
struct B {};
void f(B b) { [[clang::musttail]] return b.~B(); }
... because the calling convention for a destructor can be different from the
calling convention for a regular function (eg, destructors sometimes implicitly
return `this` and sometimes have secret extra parameters).
Please also make sure we reject
struct A {}; void f(A a) { [[clang::musttail]] return a.A::A(); }
(which we would accept without the attribute under `-fms-extensions`):
constructors, like destructors, have unusual calling conventions.
================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:2845
+ "target function "
+ "%select{has different class%diff{ (expected $ but has $)|}1,2"
+ "|has different number of parameters (expected %1 but has %2)"
----------------
Might be clearer?
================
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:
> 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.
================
Comment at: clang/lib/CodeGen/CGExpr.cpp:4828
ReturnValueSlot ReturnValue) {
+ SaveAndRestore<bool> SaveMustTail(InMustTailCallExpr, E == MustTailCall);
+
----------------
The more I think about this, the more it makes me nervous: if any of the
`Emit*CallExpr` functions below incidentally emit a call on the way to
producing their results via the CGCall machinery, and do so without recursing
through this function, that incidental call will be emitted as a tail call
instead of the intended one. Specifically:
-- I could imagine a block call involving multiple function calls, depending on
the blocks ABI.
-- I could imagine a member call performing a function call to convert from
derived to virtual base in some ABIs.
-- A CUDA kernel call in general involves calling a setup function before the
actual function call happens (and it doesn't make sense for a CUDA kernel call
to be a tail call anyway...)
-- A call to a builtin can result in any number of function calls.
-- If any expression in the function arguments emits a call without calling
back into this function, we'll emit that call as a tail call instead of this
one. Eg, `[[clang::musttail]] return f(dynamic_cast<T*>(p));` might emit the
call to `__cxa_dynamic_cast` as the tail call instead of emitting the call to
`f` as the tail call, depending on whether the CGCall machinery is used when
emitting the `__cxa_dynamic_cast` call.
Is it feasible to sink this check into the `CodeGenFunction::EmitCall` overload
that takes a `CallExpr`, `CodeGenFunction::EmitCXXMemberOrOperatorCall`, and
`CodeGenFunction::EmitCXXMemberPointerCallExpr`, after we've emitted the callee
and call args? It looks like we might be able to check this immediately before
calling the CGCall overload of `EmitCall`, so we could pass in the 'musttail'
information as a flag or similar instead of using global state in the
`CodeGenFunction` object; if so, it'd be much easier to be confident that we're
applying the attribute to the right call.
================
Comment at: clang/lib/Sema/SemaStmt.cpp:581
+ if (EWC->cleanupsHaveSideEffects()) {
+ Diag(St->getBeginLoc(), diag::err_musttail_needs_trivial_args) << &MTA;
+ return false;
----------------
It's a bit awkward, but I think we should delay this check until after the
others -- complaining about non-trivial destruction seems beside the point if
the returned value isn't a function call.
Also, the diagnostic text for this error seems narrower than the cases it
covers. For example:
```
void f(const char*);
void g(const char *s) {
[[clang::musttail]] return f((s + "foo"s).c_str());
}
```
would be diagnosed as "attribute requires that the return type and all
arguments are trivially destructible", and they are; the problem is that the
return value creates a temporary object with non-trivial destruction.
================
Comment at: clang/test/CodeGen/attr-musttail.cpp:72
+
+// CHECK: musttail call void @_Z11ReturnsVoidv()
+
----------------
For completeness, can we also get a `CHECK-NEXT: ret void` here too?
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
