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

[zequanwu]

Summary

It would be beneficial to allow not_tail_called attribute to be applied to
virtual functions. I don't see any drawback of allowing this.

[aaron.ballman]

It was explicitly disallowed in the initial patch: https://reviews.llvm.org/D12922 and the original author said "I'm still trying to figure out the best way to handle c++ virtual functions: this attribute is not very useful for someone who is looking for a way to reliably prevent tail-call to a virtual function." and "I made this change because this attribute isn't useful when the compiler cannot resolve the function call statically at compile time and it isn't important for the use case I have." Has this situation changed in the backend?

[zequanwu]

In D96832#2568257, @aaron.ballman wrote:

It was explicitly disallowed in the initial patch: https://reviews.llvm.org/D12922 and the original author said "I'm still trying to figure out the best way to handle c++ virtual functions: this attribute is not very useful for someone who is looking for a way to reliably prevent tail-call to a virtual function." and "I made this change because this attribute isn't useful when the compiler cannot resolve the function call statically at compile time and it isn't important for the use case I have." Has this situation changed in the backend?

Oh, I didn't see that. But when I tested not_tail_called on normal functions, it seems like not working(https://godbolt.org/z/znr5b5, f1 is marked as not_tail_called, it still get inlined). Or, I misunderstand how to use it properly?

[ahatanak]

In D96832#2569481, @zequanwu wrote:

In D96832#2568257, @aaron.ballman wrote:

It was explicitly disallowed in the initial patch: https://reviews.llvm.org/D12922 and the original author said "I'm still trying to figure out the best way to handle c++ virtual functions: this attribute is not very useful for someone who is looking for a way to reliably prevent tail-call to a virtual function." and "I made this change because this attribute isn't useful when the compiler cannot resolve the function call statically at compile time and it isn't important for the use case I have." Has this situation changed in the backend?

Oh, I didn't see that. But when I tested not_tail_called on normal functions, it seems like not working(https://godbolt.org/z/znr5b5, f1 is marked as not_tail_called, it still get inlined). Or, I misunderstand how to use it properly?

I'm still trying to remember the discussions we had, but I think we banned the attribute on virtual functions because you can't in general promise a call to an annotated function won't be tail called if the function is virtual.

This patch doesn't prevent the call to method in the code below from being tail called, but I suppose users would except the attribute to prevent the tail call?

struct B {
  virtual void method();  
};

struct D : B {
  [[clang::not_tail_called]] void method() override; 
};

void test(D *d) {
  B *b = D;
  b->method();
}

[zequanwu]

In D96832#2569987, @ahatanak wrote:

In D96832#2569481, @zequanwu wrote:

In D96832#2568257, @aaron.ballman wrote:

It was explicitly disallowed in the initial patch: https://reviews.llvm.org/D12922 and the original author said "I'm still trying to figure out the best way to handle c++ virtual functions: this attribute is not very useful for someone who is looking for a way to reliably prevent tail-call to a virtual function." and "I made this change because this attribute isn't useful when the compiler cannot resolve the function call statically at compile time and it isn't important for the use case I have." Has this situation changed in the backend?

Oh, I didn't see that. But when I tested not_tail_called on normal functions, it seems like not working(https://godbolt.org/z/znr5b5, f1 is marked as not_tail_called, it still get inlined). Or, I misunderstand how to use it properly?

I'm still trying to remember the discussions we had, but I think we banned the attribute on virtual functions because you can't in general promise a call to an annotated function won't be tail called if the function is virtual.

This patch doesn't prevent the call to method in the code below from being tail called, but I suppose users would except the attribute to prevent the tail call?

struct B {
  virtual void method();  
};

struct D : B {
  [[clang::not_tail_called]] void method() override; 
};

void test(D *d) {
  B *b = D;
  b->method();
}

I got the point about why preventing it on virtual functions. But, on the above godbolt link, I provided a example where not_tail_called doesn't prevent a non-virtual function from tail call. I wonder if I use it correctly.

[• Quuxplusone]

clang/include/clang/Basic/AttrDocs.td

4109

(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.

[ahatanak]

In D96832#2570009, @zequanwu wrote:

In D96832#2569987, @ahatanak wrote:

In D96832#2569481, @zequanwu wrote:

In D96832#2568257, @aaron.ballman wrote:

It was explicitly disallowed in the initial patch: https://reviews.llvm.org/D12922 and the original author said "I'm still trying to figure out the best way to handle c++ virtual functions: this attribute is not very useful for someone who is looking for a way to reliably prevent tail-call to a virtual function." and "I made this change because this attribute isn't useful when the compiler cannot resolve the function call statically at compile time and it isn't important for the use case I have." Has this situation changed in the backend?

Oh, I didn't see that. But when I tested not_tail_called on normal functions, it seems like not working(https://godbolt.org/z/znr5b5, f1 is marked as not_tail_called, it still get inlined). Or, I misunderstand how to use it properly?

I'm still trying to remember the discussions we had, but I think we banned the attribute on virtual functions because you can't in general promise a call to an annotated function won't be tail called if the function is virtual.

This patch doesn't prevent the call to method in the code below from being tail called, but I suppose users would except the attribute to prevent the tail call?

struct B {
  virtual void method();  
};

struct D : B {
  [[clang::not_tail_called]] void method() override; 
};

void test(D *d) {
  B *b = D;
  b->method();
}

I got the point about why preventing it on virtual functions. But, on the above godbolt link, I provided a example where not_tail_called doesn't prevent a non-virtual function from tail call. I wonder if I use it correctly.

not_tail_called adds notail to the call c->f1 in the IR, but doesn't do anything to prevent the calls inside f1 from being tail called. You can use disable-tail-calls instead for that purpose.

[ahatanak]

clang/include/clang/Basic/AttrDocs.td

4109

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.

[aaron.ballman]

clang/include/clang/Basic/AttrDocs.td

4109

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.

[• Quuxplusone]

clang/include/clang/Basic/AttrDocs.td

4109

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.

[aaron.ballman]

clang/include/clang/Basic/AttrDocs.td

4109

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.

[rnk]

clang/include/clang/Basic/AttrDocs.td

4083–4084

This paragraph about indirect function calls should probably be retained. I think all this information is still correct, the notail marker is a property of the call site, and it is only applied if the frontend can directly see the declaration of the callee with the attribute.

4088

We will need to replace this paragraph with documentation about how the attribute works on virtual methods. It should at least describe the expected behavior in the case where the attribute is applied to a method override in a derived class.

4109

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?

[aaron.ballman]

clang/include/clang/Basic/AttrDocs.td

4109

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.

[rnk]

clang/include/clang/Basic/AttrDocs.td

4109

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.

[aaron.ballman]

clang/include/clang/Basic/AttrDocs.td

4109

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!

[zequanwu]

Add a doc example and CodeGen testcase about not_tail_called being applied to method override in a derived class.

[aaron.ballman]

LGTM aside from a minor wording suggestion on the documentation.

clang/include/clang/Basic/AttrDocs.td

4087–4092

[• Quuxplusone]

clang/include/clang/Basic/AttrDocs.td

4087–4092

/will not have effect/will have no effect/
However, this phrasing is easy to interpret the wrong way around: actually marking a (base-class) virtual function will affect overriding functions in derived classes! You meant that marking the overrider wouldn't retroactively affect the overridden function from the base class.
I think the correct explanation would be more like this:

Generally, marking an overriding virtual function as ``not_tail_called`` is
not useful, because this attribute is a property of the static type. Calls
made through a pointer or reference to the base class type will respect
the ``no_tail_called`` attribute of the base class's member function,
regardless of the runtime destination of the call.

I think it'd also be correct and helpful to add:

Similarly, calls made through a function pointer will respect the
``no_tail_called`` attribute of the function pointer, not of its
runtime destination.

(I admit this is mildly redundant with the foo2 example above.)

[ahatanak]

clang/include/clang/Basic/AttrDocs.td

4087–4092

calls made through a function pointer will respect the `no_tail_called` attribute of the function pointer

But function pointers currently can't be annotated with no_tail_called, right?

[• Quuxplusone]

clang/include/clang/Basic/AttrDocs.td

4087–4092

Ah, maybe. By "will respect the not-tail-called attribute of the function pointer," I meant both "will not-tail-call it if it has the attribute" and also "may tail-call it if it lacks the attribute." Sounds like only the latter is hittable today (as shown in the foo2 example).

Also, I notice that everyone (including me) keeps misspelling the name of this attribute. I wish it were spelled as a command, like "nodiscard" — e.g. "notailcall" or "do_not_tail_call" — but I assume that ship has sailed? Anyway, apparently not_tail_called is the current spelling, and we should make sure the docs use it consistently :)

[zequanwu]

Adopt Quuxplusone's phrasing.

[• Quuxplusone]

Marking "accepted" from me, merely to indicate that my grammar stuff is not blocking and I don't expect I'll have any further substantive comments.

clang/include/clang/Basic/AttrDocs.td

4101

Same grammar thing here: not_tail_called has no effect here, even though the

clang/include/clang/Basic/AttrDocs.td

.. code-block:: c
 
     }];
 }
 
 def NotTailCalledDocs : Documentation {
   let Category = DocCatFunction;
   let Content = [{
 The ``not_tail_called`` attribute prevents tail-call optimization on statically
-bound calls. It has no effect on indirect calls. Virtual functions, objective-c
-methods, and functions marked as ``always_inline`` cannot be marked as
-``not_tail_called``.
+bound calls. Objective-c methods, and functions marked as ``always_inline``
+cannot be marked as ``not_tail_called``.
 
 For example, it prevents tail-call optimization in the following case:
 
   .. code-block:: c
 
     int __attribute__((not_tail_called)) foo1(int);
 
     int foo2(int a) {
       return foo1(a); // No tail-call optimization on direct calls.
     }
 
 However, it doesn't prevent tail-call optimization in this case:
 
   .. code-block:: c
 
     int __attribute__((not_tail_called)) foo1(int);
 
     int foo2(int a) {
       int (*fn)(int) = &foo1;
 
       // not_tail_called has no effect on an indirect call even if the call can
       // be resolved at compile time.
       return (*fn)(a);
     }
 
-Marking virtual functions as ``not_tail_called`` is an error:
+Generally, marking an overriding virtual function as ``not_tail_called`` is
+not useful, because this attribute is a property of the static type. Calls
+made through a pointer or reference to the base class type will respect
+the ``not_tail_called`` attribute of the base class's member function,
+regardless of the runtime destination of the call:
 
   .. code-block:: c++
 
-    class Base {
-    public:
-      // not_tail_called on a virtual function is an error.
-      [[clang::not_tail_called]] virtual int foo1();
-
-      virtual int foo2();
-
-      // Non-virtual functions can be marked ``not_tail_called``.
-      [[clang::not_tail_called]] int foo3();
-    };
-
-    class Derived1 : public Base {
-    public:
-      int foo1() override;
-
-      // not_tail_called on a virtual function is an error.
-      [[clang::not_tail_called]] int foo2() override;
+    struct Foo { virtual void f(); };
+    struct Bar : Foo {
+      [[clang::not_tail_called]] void f() override;
     };
+    void callera(Bar& bar) {
+      Foo& foo = bar;
+      // not_tail_called has no effect on here, even though the
+      // underlying method is f from Bar.
+      foo.f();
+      bar.f(); // No tail-call optimization on here.
+    }
   }];
 }
 
 def NoThrowDocs : Documentation {
   let Category = DocCatFunction;
   let Content = [{
 Clang supports the GNU style ``__attribute__((nothrow))`` and Microsoft style
 ``__declspec(nothrow)`` attribute as an equivalent of ``noexcept`` on function

clang/lib/Sema/SemaDecl.cpp

if ((ND.isExternallyVisible() && AnonNSInMicrosoftMode) ||
         (!AnonNSInMicrosoftMode &&
          (!ND.isExternallyVisible() || (VD && VD->isStaticLocal())))) {
       S.Diag(ND.getLocation(), diag::err_attribute_dll_not_extern)
         << &ND << Attr;
       ND.setInvalidDecl();
     }
   }
 
-  // Virtual functions cannot be marked as 'notail'.
-  if (auto *Attr = ND.getAttr<NotTailCalledAttr>())
-    if (auto *MD = dyn_cast<CXXMethodDecl>(&ND))
-      if (MD->isVirtual()) {
-        S.Diag(ND.getLocation(),
-               diag::err_invalid_attribute_on_virtual_function)
-            << Attr;
-        ND.dropAttr<NotTailCalledAttr>();
-      }
-
   // Check the attributes on the function type, if any.
   if (const auto *FD = dyn_cast<FunctionDecl>(&ND)) {
     // Don't declare this variable in the second operand of the for-statement;
     // GCC miscompiles that by ending its lifetime before evaluating the
     // third operand. See gcc.gnu.org/PR86769.
     AttributedTypeLoc ATL;
     for (TypeLoc TL = FD->getTypeSourceInfo()->getTypeLoc();
          (ATL = TL.getAsAdjusted<AttributedTypeLoc>());

clang/test/CodeGenCXX/attr-notail.cpp

 // RUN: %clang_cc1 -triple=x86_64-apple-darwin -std=c++11 -emit-llvm -o - %s | FileCheck %s
 
 class Class1 {
 public:
   [[clang::not_tail_called]] int m1();
   int m2();
+  [[clang::not_tail_called]] virtual int m3();
+  virtual int m4();
 };
 
-int foo1(int a, Class1 *c1) {
+class Class2: public Class1 {
+public:
+  [[clang::not_tail_called]] int m4() override;
+};
+
+int foo1(int a, Class1 *c1, Class2 &c2) {
   if (a)
     return c1->m1();
+  c1->m3();
+  Class1 &c = c2;
+  c.m4();
+  c2.m4();
   return c1->m2();
 }
 
-// CHECK-LABEL: define{{.*}} i32 @_Z4foo1iP6Class1(
+// CHECK-LABEL: define{{.*}} i32 @_Z4foo1iP6Class1R6Class2(
 // CHECK: %{{[a-z0-9]+}} = notail call i32 @_ZN6Class12m1Ev(%class.Class1*
+// CHECK: %{{[a-z0-9]+}} = notail call i32 %{{[0-9]+}}(%class.Class1*
+// CHECK-NOT: %{{[a-z0-9]+}} = notail call i32 %{{[0-9]+}}(%class.Class1*
+// CHECK: %{{[a-z0-9]+}} = notail call i32 %{{[0-9]+}}(%class.Class2* 
 // CHECK: %{{[a-z0-9]+}} = call i32 @_ZN6Class12m2Ev(%class.Class1*

clang/test/SemaCXX/attr-notail.cpp

 // RUN: %clang_cc1 -std=c++11 -fsyntax-only -verify %s
+// expected-no-diagnostics
 
 class Base {
 public:
-  [[clang::not_tail_called]] virtual int foo1(); // expected-error {{'not_tail_called' attribute cannot be applied to virtual functions}}
+  [[clang::not_tail_called]] virtual int foo1();
   virtual int foo2();
   [[clang::not_tail_called]] int foo3();
   virtual ~Base() {}
 };
 
 class Derived1 : public Base {
 public:
   int foo1() override;
-  [[clang::not_tail_called]] int foo2() override; // expected-error {{'not_tail_called' attribute cannot be applied to virtual functions}}
+  [[clang::not_tail_called]] int foo2() override;
   [[clang::not_tail_called]] int foo4();
 };