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

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

rsmith added inline comments.


================
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:
> > 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.
Or maybe instead of "is optional / skippable", the right question is, "is this redundant if we're about to return?" That way we could potentially one day reuse the same mechanism to also skip emitting such cleanups when emitting a cleanup path into the return block.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99517