aaron.ballman added inline comments.
================
Comment at: clang/include/clang/Basic/AttrDocs.td:4109
- [[clang::not_tail_called]] int foo2() override;
- };
}];
----------------
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.
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
