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:
> > 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.
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
