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