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