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