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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
