[PATCH] D99517: Implemented [[clang::musttail]] attribute for guaranteed tail calls.

[Richard Smith - zygoloid via Phabricator via cfe-commits]

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