[PATCH] D96832: [Clang][Attributes] Allow not_tail_called attribute to be applied to virtual function.

[Aaron Ballman via Phabricator via cfe-commits]

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