aaron.ballman added inline comments.
================
Comment at: clang/include/clang/Basic/AttrDocs.td:4109
- [[clang::not_tail_called]] int foo2() override;
- };
}];
----------------
rnk wrote:
> aaron.ballman wrote:
> > rnk wrote:
> > > aaron.ballman wrote:
> > > > Quuxplusone wrote:
> > > > > aaron.ballman wrote:
> > > > > > ahatanak wrote:
> > > > > > > Quuxplusone wrote:
> > > > > > > > (Moving into a thread)
> > > > > > > >
> > > > > > > > > This patch doesn't prevent the call to method in the code
> > > > > > > > > below from being tail called,
> > > > > > > > > but I suppose users would expect the attribute to prevent the
> > > > > > > > > tail call?
> > > > > > > > ```
> > > > > > > > struct B {
> > > > > > > > virtual void method();
> > > > > > > > };
> > > > > > > > struct D : B {
> > > > > > > > [[clang::not_tail_called]] void method() override;
> > > > > > > > };
> > > > > > > > ```
> > > > > > > >
> > > > > > > > The way virtual calls are handled in C++ is, all attributes and
> > > > > > > > properties of the call are determined based on the //static//
> > > > > > > > type at the call site; and then the //runtime destination// of
> > > > > > > > the call is determined from the pointer in the vtable.
> > > > > > > > Attributes and properties have no runtime existence, and so
> > > > > > > > they physically cannot affect anything at runtime. Consider
> > > > > > > > https://godbolt.org/z/P3799e :
> > > > > > > >
> > > > > > > > ```
> > > > > > > > struct Ba {
> > > > > > > > virtual Ba *method(int x = 1);
> > > > > > > > };
> > > > > > > > struct Da : Ba {
> > > > > > > > [[clang::not_tail_called]] [[nodiscard]] Da *method(int x =
> > > > > > > > 2) noexcept override;
> > > > > > > > };
> > > > > > > > auto callera(Da& da) {
> > > > > > > > Ba& ba = da;
> > > > > > > > ba.method();
> > > > > > > > }
> > > > > > > > ```
> > > > > > > > Here the call that is made is a //virtual// call (because
> > > > > > > > `Ba::method` is virtual); with a default argument value of `1`
> > > > > > > > (because `Ba::method`'s `x` parameter has a default value of
> > > > > > > > `1`); and it returns something of type `Ba*` (because that's
> > > > > > > > what `Ba::method` returns); and it is not considered to be
> > > > > > > > noexcept (because `Ba::method` isn't marked noexcept); and it's
> > > > > > > > okay to discard the result (because `Ba::method` is not
> > > > > > > > nodiscard) and it is tail-called (because `Ba::method` doesn't
> > > > > > > > disallow tail calls). All of these attributes and properties
> > > > > > > > are based on the //static// type of variable `ba`, despite the
> > > > > > > > fact that //at runtime// we'll end up jumping to the code for
> > > > > > > > `Da::method`. According to the source code, statically,
> > > > > > > > `Da::method` has a default argument of `2`, returns `Da*`, is
> > > > > > > > noexcept, and is nodiscard, and disallows tail-calls. But we're
> > > > > > > > not calling `da.method()`, we're calling `ba.method()`; so none
> > > > > > > > of that matters to our call site at `callera`.
> > > > > > > >
> > > > > > > > I think this patch is a good thing.
> > > > > > > OK, I see. I think this patch is fine then.
> > > > > > >
> > > > > > > Should we add an explanation of how virtual functions are
> > > > > > > handled? The doc currently just says the attribute prevents
> > > > > > > tail-call optimization on statically bound calls.
> > > > > > > I think this patch is a good thing.
> > > > > >
> > > > > > I agree with your explanation but I'm not certain I agree with your
> > > > > > conclusion. :-) I think the examples you point out are more often a
> > > > > > source of confusion for users than not because of the nature of
> > > > > > static vs dynamic dispatch, and so saying "this behavior can be
> > > > > > consistent with all of these other things people often get confused
> > > > > > by" may be justifiable but also seems a bit user-hostile.
> > > > > >
> > > > > > Taking a slightly different example:
> > > > > > ```
> > > > > > struct Ba {
> > > > > > [[clang::not_tail_called]] virtual Ba *method();
> > > > > > };
> > > > > > struct Da : Ba {
> > > > > > Ba *method() override;
> > > > > > };
> > > > > >
> > > > > > void callera(Da& da) {
> > > > > > Ba& ba = da;
> > > > > > ba.method();
> > > > > > }
> > > > > > ```
> > > > > > There's no guarantee that `Ba::method()` and `Da::method()` have
> > > > > > the same not-tail-called properties. The codegen for this case will
> > > > > > still attach `notail` to the call site even though the dynamic
> > > > > > target may not meet that requirement.
> > > > > >
> > > > > > tl;dr: I think `notail` is a property of the call expression and
> > > > > > the only way to know that's valid is when you know what's being
> > > > > > called, so the current behavior is more user-friendly for avoiding
> > > > > > optimization issues. I'd prefer not to relax that unless there was
> > > > > > a significantly motivating example beyond what's presented here so
> > > > > > far.
> > > > > > saying "this behavior can be consistent with all of these other
> > > > > > things people often get confused by" may be justifiable but also
> > > > > > seems a bit user-hostile.
> > > > >
> > > > > I disagree. In areas where (we agree that) users are already a bit
> > > > > confused, I want to treat consistency-of-interface as a virtue.
> > > > > Imagine a student being confused for weeks about the behavior of
> > > > > attributes on virtual methods — "I put
> > > > > `[[nodiscard]]`/`[[noreturn]]`/`[[deprecated]]` on the child method,
> > > > > but the compiler isn't warning me when I call the parent method!" —
> > > > > and then //finally// someone asks him to repeat it slowly, and the
> > > > > lightbulb goes on: "Oh, right, I'm calling the //parent// method..."
> > > > > So now he "gets it." Oh, except, the entire mental model breaks down
> > > > > again for the `[[not_tail_called]]` attribute, because that attribute
> > > > > uses different rules.
> > > > >
> > > > > Let's just skip that very last step. Let's have all attributes use
> > > > > the same rules, so that the mental model for one carries over to all
> > > > > the others.
> > > > >
> > > > > Btw, here's all the interesting attributes/properties/bells/whistles
> > > > > I was able to think of: https://godbolt.org/z/3PYe87 (Looks like
> > > > > Clang is more permissive than it should be with covariant return
> > > > > types.) It'd be interesting to see what a linter like PVS-Studio
> > > > > thinks of all these overriders. I hope it would catch m9 and m10 at
> > > > > least.
> > > > >
> > > > > I would support (but not myself attempt to implement) `-Wall`
> > > > > warnings for m3/m4, for m9, and for m5/m6/m11/m12.
> > > > > Let's just skip that very last step. Let's have all attributes use
> > > > > the same rules, so that the mental model for one carries over to all
> > > > > the others.
> > > >
> > > > Okay, that is sufficiently compelling to me, but I would point out that
> > > > there's a pretty big difference between "I don't get the warnings I'd
> > > > expect" for things like `[[deprecated]]` or `[[nodiscard]]` and
> > > > miscompiles where the backend is assuming a promise holds when it
> > > > doesn't. If this is purely an optimization hint to the backend so a
> > > > mismatch of expectations results in pessimized but correct code, I'm
> > > > not worried. If it can result in incorrect code execution, then I think
> > > > we should consider whether we need to (or could) add additional
> > > > diagnostics to the frontend to help users who do get confused by the
> > > > behavior.
> > > >
> > > > > I would support (but not myself attempt to implement) -Wall warnings
> > > > > for m3/m4, for m9, and for m5/m6/m11/m12.
> > > >
> > > > FWIW, m5, m6, m11, and m12 seem somewhat reasonable to me because the
> > > > derived class may have more information than the base class, but I tend
> > > > to think that derived classes should only ever add specificity, not
> > > > remove it.
> > > We've had a lot of discussion. To sum up, how do we unblock this patch?
> > >
> > > I still think there is a valid use case for making this work for virtual
> > > functions. Our use case is debugging: we want to be able to see call
> > > frame that calls a particular virtual destructor, even when that virtual
> > > destructor call is in a tail position.
> > >
> > > Do you want a warning when the user adds the [[not_tail_called]]
> > > attribute to a virtual method override? Do you want to declare that
> > > [[not_tail_called]] should only be applied when introducing a new virtual
> > > method? Or should we just document that the attribute only works when
> > > virtual calls happen through a specific-enough static type?
> > > We've had a lot of discussion. To sum up, how do we unblock this patch?
> >
> > I'd like verification on:
> >
> > > If this is purely an optimization hint to the backend so a mismatch of
> > > expectations results in pessimized but correct code, I'm not worried. If
> > > it can result in incorrect code execution, then I think we should
> > > consider whether we need to (or could) add additional diagnostics to the
> > > frontend to help users who do get confused by the behavior.
> >
> > Based on the current documentation, I *believe* there is no chance for a
> > miscompile, only a chance for a missed optimization opportunity. Is that
> > correct?
> >
> > If so, then I think adding some wording about static vs dynamic types to
> > the documentation is the appropriate route to go.
> > > We've had a lot of discussion. To sum up, how do we unblock this patch?
> >
> > I'd like verification on:
> >
> > > If this is purely an optimization hint to the backend so a mismatch of
> > > expectations results in pessimized but correct code, I'm not worried. If
> > > it can result in incorrect code execution, then I think we should
> > > consider whether we need to (or could) add additional diagnostics to the
> > > frontend to help users who do get confused by the behavior.
> >
> > Based on the current documentation, I *believe* there is no chance for a
> > miscompile, only a chance for a missed optimization opportunity. Is that
> > correct?
> >
> > If so, then I think adding some wording about static vs dynamic types to
> > the documentation is the appropriate route to go.
>
> I think this attribute only affects implementation-defined program behavior,
> so I think we can say that accidentally misusing the attribute doesn't result
> in a miscompile, it results in a program that is slightly harder to debug or
> profile.
>
> I believe that ARC relies on the LLVM IR notail marker for correctness, but
> not this source-level Clang attribute.
>
> If we did implement a warning, I'm not sure how useful it would be. The
> obvious way to warn would be on code like this:
> struct Foo { virtual void f(); };
> struct Bar : Foo {
> void [[clang::not_tail_called]] f() override;
> // warning: attribute may not work as expected when applied to method
> overrides
> };
>
> If the user doesn't control the Foo interface, that seems a bit unreasonable,
> and it's not clear to me how the user would silence the warning if they are
> OK with the existing behavior.
>
> ---
>
> So, I've convinced myself that this patch needs some doc updates, and perhaps
> some more complex inheritance tests, but we should proceed without a new
> warning. Hopefully you agree.
> So, I've convinced myself that this patch needs some doc updates, and perhaps
> some more complex inheritance tests, but we should proceed without a new
> warning. Hopefully you agree.
Yes, I agree. Thanks (everyone) for the discussion on this!
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D96832/new/
https://reviews.llvm.org/D96832
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
