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

Thu Apr 1 21:14:30 PDT 2021

haberman added a comment.

I added tests for all the cases you mentioned. PTAL.

================
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.
----------------
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
```

================
Comment at: clang/lib/CodeGen/CGExpr.cpp:4828
                                      ReturnValueSlot ReturnValue) {
+  SaveAndRestore<bool> SaveMustTail(InMustTailCallExpr, E == MustTailCall);
+
----------------
rsmith wrote:
> 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.
Done. It's feeling like `IsMustTail`, `callOrInvoke`, and `Loc` might want to get collapsed into an options struct, especially given the default parameters on the first two. Maybe could do as a follow up?

================
Comment at: clang/test/CodeGen/attr-musttail.cpp:72
+
+// CHECK: musttail call void @_Z11ReturnsVoidv()
+
----------------
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?

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99517/new/

https://reviews.llvm.org/D99517