[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;
-    };
   }];
----------------
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D96832/new/

https://reviews.llvm.org/D96832