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