[PATCH v2] Add no_tail_call attribute

[PATCH v2] Add no_tail_call attribute

[Yuri Gribov]

[-- Attachment #1: Type: text/plain, Size: 907 bytes --]

On Mon, May 29, 2017 at 8:14 AM, Yuri Gribov <tetra2005@gmail.com> wrote:
> Hi all,
>
> As discussed in
> https://sourceware.org/ml/libc-alpha/2017-01/msg00455.html , some
> libdl functions rely on return address to figure out the calling
> DSO and then use this information in computation (e.g. output of dlsym
> depends on which library called it).
>
> As reported in https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66826 this
> may break under tailcall optimization i.e. in cases like
>
>   return dlsym(...);
>
> Carlos confirmed that they would prefer to have GCC attribute to
> prevent tailcalls
> (https://sourceware.org/ml/libc-alpha/2017-01/msg00502.html) so there
> you go.
>
> This was bootstrapped on x86_64. Given that this is a minor addition,
> I only ran newly added regtests. I hope that's enough (full testsuite
> would take a week on my notebook...).

Added docs, per Alex's suggestion.

-Y

[-- Attachment #2: 0001-Added-no_tail_call-attribute.patch --]
[-- Type: application/octet-stream, Size: 6610 bytes --]

From 1f4590e7a633c6335512b012578bddba7602b3c9 Mon Sep 17 00:00:00 2001
From: Yury Gribov <tetra2005@gmail.com>
Date: Sun, 28 May 2017 21:02:20 +0100
Subject: [PATCH] Added no_tail_call attribute.

gcc/
2017-05-29  Yury Gribov  <tetra2005@gmail.com>

	* cgraphunit.c (cgraph_node::expand_thunk): Prevent
	tailcalling functions marked with no_tail_call.
	* gcc/doc/extend.texi: Document no_tail_call.
	* tree-tailcall.c (find_tail_calls): Ditto.
	* tree.c (comp_type_attributes): Treat no_tail_call
	mismatch as error.

gcc/c-family/
2017-05-29  Yury Gribov  <tetra2005@gmail.com>

	* c-attribs.c: New attribute.

gcc/testsuite/
2017-05-29  Yury Gribov  <tetra2005@gmail.com>

	* gcc.dg/pr66826-1.c: New test.
	* gcc.dg/pr66826-2.c: New test.
---
 gcc/c-family/c-attribs.c         |  1 +
 gcc/cgraphunit.c                 |  6 ++++--
 gcc/doc/extend.texi              |  7 +++++++
 gcc/testsuite/gcc.dg/pr66826-1.c | 14 ++++++++++++++
 gcc/testsuite/gcc.dg/pr66826-2.c |  6 ++++++
 gcc/tree-tailcall.c              |  4 ++++
 gcc/tree.c                       |  7 ++++---
 7 files changed, 40 insertions(+), 5 deletions(-)
 create mode 100644 gcc/testsuite/gcc.dg/pr66826-1.c
 create mode 100644 gcc/testsuite/gcc.dg/pr66826-2.c

diff --git a/gcc/c-family/c-attribs.c b/gcc/c-family/c-attribs.c
index 695c58c..482db00 100644
--- a/gcc/c-family/c-attribs.c
+++ b/gcc/c-family/c-attribs.c
@@ -345,6 +345,7 @@ const struct attribute_spec c_common_attribute_table[] =
 			      handle_bnd_instrument, false },
   { "fallthrough",	      0, 0, false, false, false,
 			      handle_fallthrough_attribute, false },
+  { "no_tail_call",           0, 0, false, true, true, NULL, true },
   { NULL,                     0, 0, false, false, false, NULL, false }
 };
 
diff --git a/gcc/cgraphunit.c b/gcc/cgraphunit.c
index 4a949ca..e23fd21 100644
--- a/gcc/cgraphunit.c
+++ b/gcc/cgraphunit.c
@@ -1715,6 +1715,7 @@ cgraph_node::expand_thunk (bool output_asm_thunks, bool force_gimple_thunk)
       tree resdecl;
       tree restmp = NULL;
       tree resbnd = NULL;
+      bool no_tail_call_p;
 
       gcall *call;
       greturn *ret;
@@ -1823,6 +1824,7 @@ cgraph_node::expand_thunk (bool output_asm_thunks, bool force_gimple_thunk)
       callees->call_stmt = call;
       gimple_call_set_from_thunk (call, true);
       gimple_call_set_with_bounds (call, instrumentation_clone);
+      no_tail_call_p = lookup_attribute ("no_tail_call", TYPE_ATTRIBUTES (gimple_call_fntype (call)));
 
       /* Return slot optimization is always possible and in fact requred to
          return values with DECL_BY_REFERENCE.  */
@@ -1912,7 +1914,7 @@ cgraph_node::expand_thunk (bool output_asm_thunks, bool force_gimple_thunk)
 		  bsi = gsi_last_bb (return_bb);
 		}
 	    }
-	  else
+	  else if (!no_tail_call_p)
 	    gimple_call_set_tail (call, true);
 
 	  /* Build return value.  */
@@ -1924,7 +1926,7 @@ cgraph_node::expand_thunk (bool output_asm_thunks, bool force_gimple_thunk)
 
 	  gsi_insert_after (&bsi, ret, GSI_NEW_STMT);
 	}
-      else
+      else if (!no_tail_call_p)
 	{
 	  gimple_call_set_tail (call, true);
 	  remove_edge (single_succ_edge (bb));
diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi
index c737634..6fa9c66 100644
--- a/gcc/doc/extend.texi
+++ b/gcc/doc/extend.texi
@@ -3067,6 +3067,13 @@ the standard C library can be guaranteed not to throw an exception
 with the notable exceptions of @code{qsort} and @code{bsearch} that
 take function pointer arguments.
 
+@item no_tail_call
+@cindex @code{no_tail_call} function attribute
+The @code{no_tail_call} attribute is used to prohibit tail-call
+optimization of a function.  When taking address of function
+marked with @code{no_tail_call}, resulting function pointer type must
+have this attribute set too.
+
 @item optimize
 @cindex @code{optimize} function attribute
 The @code{optimize} attribute is used to specify that a function is to
diff --git a/gcc/testsuite/gcc.dg/pr66826-1.c b/gcc/testsuite/gcc.dg/pr66826-1.c
new file mode 100644
index 0000000..eac8ba9
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pr66826-1.c
@@ -0,0 +1,14 @@
+/* { dg-do compile { target x86_64-*-* } } */
+/* { dg-options "-O2" } */
+
+int tail();
+
+__attribute__((no_tail_call)) int notail();
+
+int foo1() {
+  return tail();  /* { dg-final { scan-assembler "jmp.*\\ytail" } } */
+}
+
+int foo2() {
+  return notail();  /* { dg-final { scan-assembler "call.*\\ynotail" } } */
+}
diff --git a/gcc/testsuite/gcc.dg/pr66826-2.c b/gcc/testsuite/gcc.dg/pr66826-2.c
new file mode 100644
index 0000000..a22df8b
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pr66826-2.c
@@ -0,0 +1,6 @@
+__attribute__((no_tail_call)) int foo();
+
+int bar() {
+  int (*p)() = foo; /* { dg-error "initialization from incompatible pointer type" } */
+  return p();
+}
diff --git a/gcc/tree-tailcall.c b/gcc/tree-tailcall.c
index f586edc..30a6fad 100644
--- a/gcc/tree-tailcall.c
+++ b/gcc/tree-tailcall.c
@@ -601,6 +601,10 @@ find_tail_calls (basic_block bb, struct tailcall **ret)
   if (m && POINTER_TYPE_P (TREE_TYPE (DECL_RESULT (current_function_decl))))
     return;
 
+  /* See if function does not want to be tailcalled.  */
+  if (lookup_attribute ("no_tail_call", TYPE_ATTRIBUTES (gimple_call_fntype (call))))
+    return;
+
   nw = XNEW (struct tailcall);
 
   nw->call_gsi = gsi;
diff --git a/gcc/tree.c b/gcc/tree.c
index b76b521..47e1ea4 100644
--- a/gcc/tree.c
+++ b/gcc/tree.c
@@ -5034,7 +5034,7 @@ comp_type_attributes (const_tree type1, const_tree type2)
       const_tree attr;
 
       as = lookup_attribute_spec (get_attribute_name (a));
-      if (!as || as->affects_type_identity == false)
+      if (!as || !as->affects_type_identity)
         continue;
 
       attr = lookup_attribute (as->name, CONST_CAST_TREE (a2));
@@ -5048,7 +5048,7 @@ comp_type_attributes (const_tree type1, const_tree type2)
 	  const struct attribute_spec *as;
 
 	  as = lookup_attribute_spec (get_attribute_name (a));
-	  if (!as || as->affects_type_identity == false)
+	  if (!as || !as->affects_type_identity)
 	    continue;
 
 	  if (!lookup_attribute (as->name, CONST_CAST_TREE (a1)))
@@ -5061,7 +5061,8 @@ comp_type_attributes (const_tree type1, const_tree type2)
       if (!a)
         return 1;
     }
-  if (lookup_attribute ("transaction_safe", CONST_CAST_TREE (a)))
+  if (lookup_attribute ("transaction_safe", CONST_CAST_TREE (a))
+      || lookup_attribute ("no_tail_call", CONST_CAST_TREE (a)))
     return 0;
   /* As some type combinations - like default calling-convention - might
      be compatible, we have to call the target hook to get the final result.  */
-- 
2.7.4

Re: [PATCH v2] Add no_tail_call attribute

[Jeff Law]

On 05/29/2017 11:24 PM, Yuri Gribov wrote:
> On Mon, May 29, 2017 at 8:14 AM, Yuri Gribov <tetra2005@gmail.com> wrote:
>> Hi all,
>>
>> As discussed in
>> https://sourceware.org/ml/libc-alpha/2017-01/msg00455.html , some
>> libdl functions rely on return address to figure out the calling
>> DSO and then use this information in computation (e.g. output of dlsym
>> depends on which library called it).
>>
>> As reported in https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66826 this
>> may break under tailcall optimization i.e. in cases like
>>
>>   return dlsym(...);
>>
>> Carlos confirmed that they would prefer to have GCC attribute to
>> prevent tailcalls
>> (https://sourceware.org/ml/libc-alpha/2017-01/msg00502.html) so there
>> you go.
>>
>> This was bootstrapped on x86_64. Given that this is a minor addition,
>> I only ran newly added regtests. I hope that's enough (full testsuite
>> would take a week on my notebook...).
> Added docs, per Alex's suggestion.
> 
> -Y
> 
> 
> 0001-Added-no_tail_call-attribute.patch
> 
> 
> From 1f4590e7a633c6335512b012578bddba7602b3c9 Mon Sep 17 00:00:00 2001
> From: Yury Gribov <tetra2005@gmail.com>
> Date: Sun, 28 May 2017 21:02:20 +0100
> Subject: [PATCH] Added no_tail_call attribute.
> 
> gcc/
> 2017-05-29  Yury Gribov  <tetra2005@gmail.com>
> 
> 	* cgraphunit.c (cgraph_node::expand_thunk): Prevent
> 	tailcalling functions marked with no_tail_call.
> 	* gcc/doc/extend.texi: Document no_tail_call.
> 	* tree-tailcall.c (find_tail_calls): Ditto.
> 	* tree.c (comp_type_attributes): Treat no_tail_call
> 	mismatch as error.
> 
> gcc/c-family/
> 2017-05-29  Yury Gribov  <tetra2005@gmail.com>
> 
> 	* c-attribs.c: New attribute.
> 
> gcc/testsuite/
> 2017-05-29  Yury Gribov  <tetra2005@gmail.com>
> 
> 	* gcc.dg/pr66826-1.c: New test.
> 	* gcc.dg/pr66826-2.c: New test.
I think a "no_tail_call" attribute is quite reasonable -- more so than
some asm hack to prevent tail calling.


> ---
>  gcc/c-family/c-attribs.c         |  1 +
>  gcc/cgraphunit.c                 |  6 ++++--
>  gcc/doc/extend.texi              |  7 +++++++
>  gcc/testsuite/gcc.dg/pr66826-1.c | 14 ++++++++++++++
>  gcc/testsuite/gcc.dg/pr66826-2.c |  6 ++++++
>  gcc/tree-tailcall.c              |  4 ++++
>  gcc/tree.c                       |  7 ++++---
>  7 files changed, 40 insertions(+), 5 deletions(-)
>  create mode 100644 gcc/testsuite/gcc.dg/pr66826-1.c
>  create mode 100644 gcc/testsuite/gcc.dg/pr66826-2.c
> 
> diff --git a/gcc/c-family/c-attribs.c b/gcc/c-family/c-attribs.c
> index 695c58c..482db00 100644
> --- a/gcc/c-family/c-attribs.c
> +++ b/gcc/c-family/c-attribs.c
> @@ -345,6 +345,7 @@ const struct attribute_spec c_common_attribute_table[] =
>  			      handle_bnd_instrument, false },
>    { "fallthrough",	      0, 0, false, false, false,
>  			      handle_fallthrough_attribute, false },
> +  { "no_tail_call",           0, 0, false, true, true, NULL, true },
Is no_tail_call supposed to be attached to the function's decl or type?

ISTM this is most similar to noclone, noinline, no_icf and friends which
seem to attach the attribute to the decl rather than to the type.



>    { NULL,                     0, 0, false, false, false, NULL, false }
>  };
>  
> diff --git a/gcc/cgraphunit.c b/gcc/cgraphunit.c
> index 4a949ca..e23fd21 100644
> --- a/gcc/cgraphunit.c
> +++ b/gcc/cgraphunit.c
> @@ -1823,6 +1824,7 @@ cgraph_node::expand_thunk (bool output_asm_thunks, bool force_gimple_thunk)
>        callees->call_stmt = call;
>        gimple_call_set_from_thunk (call, true);
>        gimple_call_set_with_bounds (call, instrumentation_clone);
> +      no_tail_call_p = lookup_attribute ("no_tail_call", TYPE_ATTRIBUTES (gimple_call_fntype (call)));
And I think the answer to the question above potentially impacts this
chunk of code.  If we were to change the attribute to apply to the decl,
then you'd need to look at the decl here rather than its type.


> index f586edc..30a6fad 100644
> --- a/gcc/tree-tailcall.c
> +++ b/gcc/tree-tailcall.c
> @@ -601,6 +601,10 @@ find_tail_calls (basic_block bb, struct tailcall **ret)
>    if (m && POINTER_TYPE_P (TREE_TYPE (DECL_RESULT (current_function_decl))))
>      return;
>  
> +  /* See if function does not want to be tailcalled.  */
> +  if (lookup_attribute ("no_tail_call", TYPE_ATTRIBUTES (gimple_call_fntype (call))))
> +    return;
Similarly and perhaps in other locations as well.

THoughts?

jeff

Re: [PATCH v2] Add no_tail_call attribute

[Yuri Gribov]

On Mon, Jul 3, 2017 at 6:03 PM, Jeff Law <law@redhat.com> wrote:
> On 05/29/2017 11:24 PM, Yuri Gribov wrote:
>> On Mon, May 29, 2017 at 8:14 AM, Yuri Gribov <tetra2005@gmail.com> wrote:
>>> Hi all,
>>>
>>> As discussed in
>>> https://sourceware.org/ml/libc-alpha/2017-01/msg00455.html , some
>>> libdl functions rely on return address to figure out the calling
>>> DSO and then use this information in computation (e.g. output of dlsym
>>> depends on which library called it).
>>>
>>> As reported in https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66826 this
>>> may break under tailcall optimization i.e. in cases like
>>>
>>>   return dlsym(...);
>>>
>>> Carlos confirmed that they would prefer to have GCC attribute to
>>> prevent tailcalls
>>> (https://sourceware.org/ml/libc-alpha/2017-01/msg00502.html) so there
>>> you go.
>>>
>>> This was bootstrapped on x86_64. Given that this is a minor addition,
>>> I only ran newly added regtests. I hope that's enough (full testsuite
>>> would take a week on my notebook...).
>> Added docs, per Alex's suggestion.
>>
>> -Y
>>
>>
>> 0001-Added-no_tail_call-attribute.patch
>>
>>
>> From 1f4590e7a633c6335512b012578bddba7602b3c9 Mon Sep 17 00:00:00 2001
>> From: Yury Gribov <tetra2005@gmail.com>
>> Date: Sun, 28 May 2017 21:02:20 +0100
>> Subject: [PATCH] Added no_tail_call attribute.
>>
>> gcc/
>> 2017-05-29  Yury Gribov  <tetra2005@gmail.com>
>>
>>       * cgraphunit.c (cgraph_node::expand_thunk): Prevent
>>       tailcalling functions marked with no_tail_call.
>>       * gcc/doc/extend.texi: Document no_tail_call.
>>       * tree-tailcall.c (find_tail_calls): Ditto.
>>       * tree.c (comp_type_attributes): Treat no_tail_call
>>       mismatch as error.
>>
>> gcc/c-family/
>> 2017-05-29  Yury Gribov  <tetra2005@gmail.com>
>>
>>       * c-attribs.c: New attribute.
>>
>> gcc/testsuite/
>> 2017-05-29  Yury Gribov  <tetra2005@gmail.com>
>>
>>       * gcc.dg/pr66826-1.c: New test.
>>       * gcc.dg/pr66826-2.c: New test.
> I think a "no_tail_call" attribute is quite reasonable -- more so than
> some asm hack to prevent tail calling.

Thanks! Frankly I lost my hope on this...

>> ---
>>  gcc/c-family/c-attribs.c         |  1 +
>>  gcc/cgraphunit.c                 |  6 ++++--
>>  gcc/doc/extend.texi              |  7 +++++++
>>  gcc/testsuite/gcc.dg/pr66826-1.c | 14 ++++++++++++++
>>  gcc/testsuite/gcc.dg/pr66826-2.c |  6 ++++++
>>  gcc/tree-tailcall.c              |  4 ++++
>>  gcc/tree.c                       |  7 ++++---
>>  7 files changed, 40 insertions(+), 5 deletions(-)
>>  create mode 100644 gcc/testsuite/gcc.dg/pr66826-1.c
>>  create mode 100644 gcc/testsuite/gcc.dg/pr66826-2.c
>>
>> diff --git a/gcc/c-family/c-attribs.c b/gcc/c-family/c-attribs.c
>> index 695c58c..482db00 100644
>> --- a/gcc/c-family/c-attribs.c
>> +++ b/gcc/c-family/c-attribs.c
>> @@ -345,6 +345,7 @@ const struct attribute_spec c_common_attribute_table[] =
>>                             handle_bnd_instrument, false },
>>    { "fallthrough",         0, 0, false, false, false,
>>                             handle_fallthrough_attribute, false },
>> +  { "no_tail_call",           0, 0, false, true, true, NULL, true },
> Is no_tail_call supposed to be attached to the function's decl or type?
>
> ISTM this is most similar to noclone, noinline, no_icf and friends which
> seem to attach the attribute to the decl rather than to the type.

Glibc people were worried that attribute would be lost when taking a
pointer to function
(https://sourceware.org/ml/libc-alpha/2017-01/msg00482.html). I think
their reasoning was that return address is a shadow argument for
dlsym-like functions so this would cause a (most likely inadvertent)
ABI error.

>>    { NULL,                     0, 0, false, false, false, NULL, false }
>>  };
>>
>> diff --git a/gcc/cgraphunit.c b/gcc/cgraphunit.c
>> index 4a949ca..e23fd21 100644
>> --- a/gcc/cgraphunit.c
>> +++ b/gcc/cgraphunit.c
>> @@ -1823,6 +1824,7 @@ cgraph_node::expand_thunk (bool output_asm_thunks, bool force_gimple_thunk)
>>        callees->call_stmt = call;
>>        gimple_call_set_from_thunk (call, true);
>>        gimple_call_set_with_bounds (call, instrumentation_clone);
>> +      no_tail_call_p = lookup_attribute ("no_tail_call", TYPE_ATTRIBUTES (gimple_call_fntype (call)));
> And I think the answer to the question above potentially impacts this
> chunk of code.  If we were to change the attribute to apply to the decl,
> then you'd need to look at the decl here rather than its type.
>
>
>> index f586edc..30a6fad 100644
>> --- a/gcc/tree-tailcall.c
>> +++ b/gcc/tree-tailcall.c
>> @@ -601,6 +601,10 @@ find_tail_calls (basic_block bb, struct tailcall **ret)
>>    if (m && POINTER_TYPE_P (TREE_TYPE (DECL_RESULT (current_function_decl))))
>>      return;
>>
>> +  /* See if function does not want to be tailcalled.  */
>> +  if (lookup_attribute ("no_tail_call", TYPE_ATTRIBUTES (gimple_call_fntype (call))))
>> +    return;
> Similarly and perhaps in other locations as well.
>
> THoughts?
>
> jeff

Re: [PATCH v2] Add no_tail_call attribute

[Jeff Law]

On 07/03/2017 12:08 PM, Yuri Gribov wrote:
>>> 0001-Added-no_tail_call-attribute.patch
>>>
>>>
>>> From 1f4590e7a633c6335512b012578bddba7602b3c9 Mon Sep 17 00:00:00 2001
>>> From: Yury Gribov <tetra2005@gmail.com>
>>> Date: Sun, 28 May 2017 21:02:20 +0100
>>> Subject: [PATCH] Added no_tail_call attribute.
>>>
>>> gcc/
>>> 2017-05-29  Yury Gribov  <tetra2005@gmail.com>
>>>
>>>       * cgraphunit.c (cgraph_node::expand_thunk): Prevent
>>>       tailcalling functions marked with no_tail_call.
>>>       * gcc/doc/extend.texi: Document no_tail_call.
>>>       * tree-tailcall.c (find_tail_calls): Ditto.
>>>       * tree.c (comp_type_attributes): Treat no_tail_call
>>>       mismatch as error.
>>>
>>> gcc/c-family/
>>> 2017-05-29  Yury Gribov  <tetra2005@gmail.com>
>>>
>>>       * c-attribs.c: New attribute.
>>>
>>> gcc/testsuite/
>>> 2017-05-29  Yury Gribov  <tetra2005@gmail.com>
>>>
>>>       * gcc.dg/pr66826-1.c: New test.
>>>       * gcc.dg/pr66826-2.c: New test.
>> I think a "no_tail_call" attribute is quite reasonable -- more so than
>> some asm hack to prevent tail calling.
> 
> Thanks! Frankly I lost my hope on this...
Sorry that happened.  Sometimes things take a while, but we do try to
get to everything.


> 
>>> ---
>>>  gcc/c-family/c-attribs.c         |  1 +
>>>  gcc/cgraphunit.c                 |  6 ++++--
>>>  gcc/doc/extend.texi              |  7 +++++++
>>>  gcc/testsuite/gcc.dg/pr66826-1.c | 14 ++++++++++++++
>>>  gcc/testsuite/gcc.dg/pr66826-2.c |  6 ++++++
>>>  gcc/tree-tailcall.c              |  4 ++++
>>>  gcc/tree.c                       |  7 ++++---
>>>  7 files changed, 40 insertions(+), 5 deletions(-)
>>>  create mode 100644 gcc/testsuite/gcc.dg/pr66826-1.c
>>>  create mode 100644 gcc/testsuite/gcc.dg/pr66826-2.c
>>>
>>> diff --git a/gcc/c-family/c-attribs.c b/gcc/c-family/c-attribs.c
>>> index 695c58c..482db00 100644
>>> --- a/gcc/c-family/c-attribs.c
>>> +++ b/gcc/c-family/c-attribs.c
>>> @@ -345,6 +345,7 @@ const struct attribute_spec c_common_attribute_table[] =
>>>                             handle_bnd_instrument, false },
>>>    { "fallthrough",         0, 0, false, false, false,
>>>                             handle_fallthrough_attribute, false },
>>> +  { "no_tail_call",           0, 0, false, true, true, NULL, true },
>> Is no_tail_call supposed to be attached to the function's decl or type?
>>
>> ISTM this is most similar to noclone, noinline, no_icf and friends which
>> seem to attach the attribute to the decl rather than to the type.
> 
> Glibc people were worried that attribute would be lost when taking a
> pointer to function
> (https://sourceware.org/ml/libc-alpha/2017-01/msg00482.html). I think
> their reasoning was that return address is a shadow argument for
> dlsym-like functions so this would cause a (most likely inadvertent)
> ABI error.
Fair enough, but does the right thing happen if the function's
definition is decorated?  Can you add a test for that in the testsuite?

Assuming it works with the definition decorated, then this is OK for the
trunk.

jeff

Re: [PATCH v2] Add no_tail_call attribute

[Alexander Monakov]

On Wed, 19 Jul 2017, Jeff Law wrote:
> > Glibc people were worried that attribute would be lost when taking a
> > pointer to function
> > (https://sourceware.org/ml/libc-alpha/2017-01/msg00482.html). I think
> > their reasoning was that return address is a shadow argument for
> > dlsym-like functions so this would cause a (most likely inadvertent)
> > ABI error.
> Fair enough, but does the right thing happen if the function's
> definition is decorated?  Can you add a test for that in the testsuite?

How would passing pointers to dlsym via a 'void *' work after this patch?

Honestly, if the goal is to assist users of GCC (and not only Glibc users,
but also people compiling against Bionic/musl/BSD libcs, which probably
wouldn't adopt this attribute), may I suggest that a clearer course of
action would be to:

1) recognize dlsym by name and suppress tailcalls to it

   this would solve >99% cases because calling dlsym by pointer would be rare,
   and has the benefit of not requiring libc header changes;

and
2) if we really want to offer some generic solution, can we give the users
a way to suppress tailcalls via a builtin? That is, introduce

    foo = __builtin_notailcall (func (args))

I think this would allow libc headers to do

    #define dlsym(sym, mod) __builtin_notailcall (dlsym (sym, mod))

I'm sorry for bringing up objections late, but I really ask for alternatives
to be considered, and I can take a stab at implementing either of the above
if the general course is agreed upon.

Alexander

Re: [PATCH v2] Add no_tail_call attribute

[Jakub Jelinek]

On Wed, Jul 19, 2017 at 06:30:40PM +0300, Alexander Monakov wrote:
> On Wed, 19 Jul 2017, Jeff Law wrote:
> > > Glibc people were worried that attribute would be lost when taking a
> > > pointer to function
> > > (https://sourceware.org/ml/libc-alpha/2017-01/msg00482.html). I think
> > > their reasoning was that return address is a shadow argument for
> > > dlsym-like functions so this would cause a (most likely inadvertent)
> > > ABI error.
> > Fair enough, but does the right thing happen if the function's
> > definition is decorated?  Can you add a test for that in the testsuite?
> 
> How would passing pointers to dlsym via a 'void *' work after this patch?
> 
> Honestly, if the goal is to assist users of GCC (and not only Glibc users,
> but also people compiling against Bionic/musl/BSD libcs, which probably
> wouldn't adopt this attribute), may I suggest that a clearer course of
> action would be to:
> 
> 1) recognize dlsym by name and suppress tailcalls to it
> 
>    this would solve >99% cases because calling dlsym by pointer would be rare,
>    and has the benefit of not requiring libc header changes;

Recognizing by name is IMNSHO undesirable.  Adding dlsym to builtins.def
with the attribute is certainly possible, and people can use
-fno-builtin-dlsym.  On the other side, does Bionic/musl/BSD dlsym really
need that attribute (i.e. does it similar computation of the return address
and base behavior on that)?

> and
> 2) if we really want to offer some generic solution, can we give the users
> a way to suppress tailcalls via a builtin? That is, introduce
> 
>     foo = __builtin_notailcall (func (args))

We already have that, just do:
  void *volatile foo = dlsym (args);
  return foo;
or
  void *foo = dlsym (args);
  __asm ("" : "+g" (foo));
  return foo;
I think adding yet another way to express that isn't needed.

	Jakub

Re: [PATCH v2] Add no_tail_call attribute

[Alexander Monakov]

On Wed, 19 Jul 2017, Jakub Jelinek wrote:
> > 1) recognize dlsym by name and suppress tailcalls to it
> > 
> >    this would solve >99% cases because calling dlsym by pointer would be rare,
> >    and has the benefit of not requiring libc header changes;
> 
> Recognizing by name is IMNSHO undesirable.  Adding dlsym to builtins.def
> with the attribute is certainly possible, and people can use
> -fno-builtin-dlsym.  On the other side, does Bionic/musl/BSD dlsym really
> need that attribute (i.e. does it similar computation of the return address
> and base behavior on that)?

Bionic and musl certainly do, and I see no other approach for supporting
RTLD_NEXT.

> >     foo = __builtin_notailcall (func (args))
> 
> We already have that, just do:
>   void *volatile foo = dlsym (args);
>   return foo;

(this has a small codesize penalty for round-tripping 'foo' via stack)

> or
>   void *foo = dlsym (args);
>   __asm ("" : "+g" (foo));
>   return foo;
> I think adding yet another way to express that isn't needed.

Well, a builtin as proposed above would also work for functions
that don't return a value.  I think that for dlsym in particular
such a builtin is not very useful, and there the choice is between
the attribute, or no additions at all.

My main concern about the attribute is that the apparent goal is
enforcing that taking the address doesn't lose the attribute.
Forbidding

   void *p = &dlsym;

seems questionable.

Alexander

Re: [PATCH v2] Add no_tail_call attribute

[Yuri Gribov]

On Wed, Jul 19, 2017 at 4:30 PM, Alexander Monakov <amonakov@ispras.ru> wrote:
> On Wed, 19 Jul 2017, Jeff Law wrote:
>> > Glibc people were worried that attribute would be lost when taking a
>> > pointer to function
>> > (https://sourceware.org/ml/libc-alpha/2017-01/msg00482.html). I think
>> > their reasoning was that return address is a shadow argument for
>> > dlsym-like functions so this would cause a (most likely inadvertent)
>> > ABI error.
>> Fair enough, but does the right thing happen if the function's
>> definition is decorated?  Can you add a test for that in the testsuite?
>
> How would passing pointers to dlsym via a 'void *' work after this patch?

Hi Alex,

Interesting, do people really do this sort of stuff?

So to reiterate, your logic here is that someone would wipe dlsym type
(e.g. by casting to void *), then later cast to another type which
lacks tailcall attribute. So proposed solution won't protect against
situation like this.

But isn't it similar to a situation when someone casts unaligned int *
(e.g. from a packed struct) to void * and then casts back to (aligned)
int *, expecting it to work (rather than SIGBUSing)?  If user dumps
important semantic attribute, it's his responsibility to properly
restore it before using the object.  In this particular case, he'd
need to cast void * to void (*)(void) __attribute__((notailcall));

> Honestly, if the goal is to assist users of GCC (and not only Glibc users,
> but also people compiling against Bionic/musl/BSD libcs, which probably
> wouldn't adopt this attribute),

Could you give more details here - why would they have problems with
notailcall approach?

> may I suggest that a clearer course of
> action would be to:
>
> 1) recognize dlsym by name and suppress tailcalls to it
>
>    this would solve >99% cases because calling dlsym by pointer would be rare,
>    and has the benefit of not requiring libc header changes;

I'm not sure how this is going to save above problem with casting to void *.

> and
> 2) if we really want to offer some generic solution, can we give the users
> a way to suppress tailcalls via a builtin? That is, introduce
>
>     foo = __builtin_notailcall (func (args))
> I think this would allow libc headers to do
>
>     #define dlsym(sym, mod) __builtin_notailcall (dlsym (sym, mod))

Could be done but what is the advantage compared to attribute?  It
does not seem to fix issue you posted in the beginning.

The one and only advantage of attribute compared to Jakubs approach
(or yours, they share the same idea of wrapping dlsym calls) is that
it forces user to carry it around when taking address of function.

> I'm sorry for bringing up objections late, but I really ask for alternatives
> to be considered, and I can take a stab at implementing either of the above
> if the general course is agreed upon.

It's totally fine to bring up objections as long as we clearly list
advantages and disadvantages of alternatives.

-Y

Re: [PATCH v2] Add no_tail_call attribute

[Alexander Monakov]

On Wed, 19 Jul 2017, Yuri Gribov wrote:
> So to reiterate, your logic here is that someone would wipe dlsym type
> (e.g. by casting to void *), then later cast to another type which
> lacks tailcall attribute. So proposed solution won't protect against
> situation like this.

No, it's not "my logic" per se.  As you said, we've arrived to this
situation after Glibc people raised concerns about solutions akin to

    #define dlsym(mod, sym) ({ \
      void *__r = dlsym (mod, sym); \
      __asm__ ("" : "+g" (__r)); \
      __r; })

not suppressing tailcalls in situations where dlsym is called via a pointer.
I'm saying that it's not possible to solve that entirely, because the
user will always be able to lose the attribute.  It is simply not possible
to enforce that dlsym is never tailcalled.

Compare how people are always able to workaround warn_unused_result.

> But isn't it similar to a situation when someone casts unaligned int *
> (e.g. from a packed struct) to void * and then casts back to (aligned)
> int *, expecting it to work (rather than SIGBUSing)?  If user dumps
> important semantic attribute, it's his responsibility to properly
> restore it before using the object.  In this particular case, he'd
> need to cast void * to void (*)(void) __attribute__((notailcall));

... which won't work in any compiler except bleeding-edge GCC, unlike
the asm trick that works rather broadly.  If you're saying that the
user will need to change their code, won't they prefer to change it
in a fashion that is compatible with a wider range of compilers?

> > Honestly, if the goal is to assist users of GCC (and not only Glibc users,
> > but also people compiling against Bionic/musl/BSD libcs, which probably
> > wouldn't adopt this attribute),
> 
> Could you give more details here - why would they have problems with
> notailcall approach?

Obviously I can't speak authoritatively here, but I'm thinking that Bionic and
BSD people won't be very interested in GCC extensions that Clang doesn't
support, and musl generally aims to avoid using compiler extensions unless
absolutely required (and here adding the attribute doesn't solve the problem
in all cases anyway).

> > may I suggest that a clearer course of
> > action would be to:
> >
> > 1) recognize dlsym by name and suppress tailcalls to it
> >
> >    this would solve >99% cases because calling dlsym by pointer would be rare,
> >    and has the benefit of not requiring libc header changes;
> 
> I'm not sure how this is going to save above problem with casting to void *.

It wouldn't, and that's exactly what I said - it would solve direct calls, and
indirect calls are rare and not 100% solvable in the first place.

> > and
> > 2) if we really want to offer some generic solution, can we give the users
> > a way to suppress tailcalls via a builtin? That is, introduce
> >
> >     foo = __builtin_notailcall (func (args))
> > I think this would allow libc headers to do
> >
> >     #define dlsym(sym, mod) __builtin_notailcall (dlsym (sym, mod))
> 
> Could be done but what is the advantage compared to attribute?  It
> does not seem to fix issue you posted in the beginning.

One advantage is that the builtin is more fine-grained, you can use it to
suppress at individual call sites, so you can easily express the attribute
in terms of the builtin using a macro as shown above or an inline function,
but not the other way around.

Of course it doesn't do the impossible, but the point was that if we want
to add something to the compiler, we should consider if there are more
generic/useful alternatives.

> The one and only advantage of attribute compared to Jakubs approach
> (or yours, they share the same idea of wrapping dlsym calls) is that
> it forces user to carry it around when taking address of function.

It's an inconvenience.  People won't like it when their code suddenly
stops compiling after they update libc headers.  And what's the solution
for them?  Adding GCC-8-specific attributes in their code, even if it
never had dlsym in tail position anyway?

Alexander

Re: [PATCH v2] Add no_tail_call attribute

[Alexander Monakov]

On Wed, 19 Jul 2017, Alexander Monakov wrote:
> > The one and only advantage of attribute compared to Jakubs approach
> > (or yours, they share the same idea of wrapping dlsym calls) is that
> > it forces user to carry it around when taking address of function.
> 
> It's an inconvenience.  People won't like it when their code suddenly
> stops compiling after they update libc headers.  And what's the solution
> for them?  Adding GCC-8-specific attributes in their code, even if it
> never had dlsym in tail position anyway?

Sorry, a better argument to bring up here is that dlsym is specified in
POSIX which spells out the exact function prototype, so it is simply
wrong to diagnose an error for standards-conforming code such as

    void *(*ptr)(void *, const char *) = dlsym;

So Glibc could not add this attribute unconditionally, maybe only under
#ifdef _GNU_SOURCE (and even then it seems like a bad idea).
I'm not sure how GCC could gate it for the builtin.

Alexander

Re: [PATCH v2] Add no_tail_call attribute

[Yuri Gribov]

On Wed, Jul 19, 2017 at 8:19 PM, Alexander Monakov <amonakov@ispras.ru> wrote:
> On Wed, 19 Jul 2017, Yuri Gribov wrote:
>> So to reiterate, your logic here is that someone would wipe dlsym type
>> (e.g. by casting to void *), then later cast to another type which
>> lacks tailcall attribute. So proposed solution won't protect against
>> situation like this.
>
> No, it's not "my logic" per se.  As you said, we've arrived to this
> situation after Glibc people raised concerns about solutions akin to
>
>     #define dlsym(mod, sym) ({ \
>       void *__r = dlsym (mod, sym); \
>       __asm__ ("" : "+g" (__r)); \
>       __r; })

Actually this was a contrived example invented by Jakub to illustrate
how we can never fully prevent tailcalls :)

> not suppressing tailcalls in situations where dlsym is called via a pointer.
> I'm saying that it's not possible to solve that entirely, because the
> user will always be able to lose the attribute.  It is simply not possible
> to enforce that dlsym is never tailcalled.
>
> Compare how people are always able to workaround warn_unused_result.

Sure, the best we can target for is preventing it in (hopefully vast)
majority of cases (speaking of which, I didn't see any really
problematic cases of "&dlsym" in
https://codesearch.debian.net/search?q=%26dlsym).

>> But isn't it similar to a situation when someone casts unaligned int *
>> (e.g. from a packed struct) to void * and then casts back to (aligned)
>> int *, expecting it to work (rather than SIGBUSing)?  If user dumps
>> important semantic attribute, it's his responsibility to properly
>> restore it before using the object.  In this particular case, he'd
>> need to cast void * to void (*)(void) __attribute__((notailcall));
>
> ... which won't work in any compiler except bleeding-edge GCC, unlike
> the asm trick that works rather broadly.  If you're saying that the
> user will need to change their code, won't they prefer to change it
> in a fashion that is compatible with a wider range of compilers?

Note that with inline asm trick users will not be aware of the problem
to begin with.  So if we go for it, we completely trade away detection
of tailcalls in function pointers (not just the extreme ones mentioned
above) and this was something Glibc people were specifically
interested in (https://sourceware.org/ml/libc-alpha/2017-01/msg00482.html).

Thanks for bringing up the issue of convenience.  I agree that it's
important (may make sense to check this with Clang community).  We
could change no_tail_call attribute to bind to declarations, rather
than types (as Jeff suggested).  But this makes it pretty much
equivalent to asm trick (or __builtin_notailcall), with all its pros
and cons.

>> > Honestly, if the goal is to assist users of GCC (and not only Glibc users,
>> > but also people compiling against Bionic/musl/BSD libcs, which probably
>> > wouldn't adopt this attribute),
>>
>> Could you give more details here - why would they have problems with
>> notailcall approach?
>
> Obviously I can't speak authoritatively here, but I'm thinking that Bionic and
> BSD people won't be very interested in GCC extensions that Clang doesn't
> support, and musl generally aims to avoid using compiler extensions unless
> absolutely required (and here adding the attribute doesn't solve the problem
> in all cases anyway).

Agreed.  I was tempted to say that this applies to any compiler
extension (including __builtin_notailcall) but it's not the case.
Notailcall attribute in its present form requires changes to programs
which take address of dlsym (in contrast to __builtin_notailcall).

>> > may I suggest that a clearer course of
>> > action would be to:
>> >
>> > 1) recognize dlsym by name and suppress tailcalls to it
>> >
>> >    this would solve >99% cases because calling dlsym by pointer would be rare,
>> >    and has the benefit of not requiring libc header changes;
>>
>> I'm not sure how this is going to save above problem with casting to void *.
>
> It wouldn't, and that's exactly what I said - it would solve direct calls, and
> indirect calls are rare and not 100% solvable in the first place.
>
>> > and
>> > 2) if we really want to offer some generic solution, can we give the users
>> > a way to suppress tailcalls via a builtin? That is, introduce
>> >
>> >     foo = __builtin_notailcall (func (args))
>> > I think this would allow libc headers to do
>> >
>> >     #define dlsym(sym, mod) __builtin_notailcall (dlsym (sym, mod))
>>
>> Could be done but what is the advantage compared to attribute?  It
>> does not seem to fix issue you posted in the beginning.
>
> One advantage is that the builtin is more fine-grained, you can use it to
> suppress at individual call sites, so you can easily express the attribute
> in terms of the builtin using a macro as shown above or an inline function,
> but not the other way around.

I'm not sure why fine-grainedness would be important for fixing dlsym
and friends.  They should simply never be tailcalled.

> Of course it doesn't do the impossible, but the point was that if we want
> to add something to the compiler, we should consider if there are more
> generic/useful alternatives.
>
>> The one and only advantage of attribute compared to Jakubs approach
>> (or yours, they share the same idea of wrapping dlsym calls) is that
>> it forces user to carry it around when taking address of function.
>
> It's an inconvenience.  People won't like it when their code suddenly
> stops compiling after they update libc headers.  And what's the solution
> for them?  Adding GCC-8-specific attributes in their code, even if it
> never had dlsym in tail position anyway?

Right, that's reiteration of above.

So I think net conclusion is that even though typed attribute detects
more problems with dlsym-like functions, it will potentially require
modification of users code in really ugly ways (#if defined __GLIBC__
&& __GLIBC_MINOR > 28 && ...).  Attribute can be relaxed but this
makes it more or less equivalent to plain and simple asm trick
proposed by Jakub.  Your opinion then is that additional benefits of
typed attribute do not justify the burden.

I tend to agree, even though I'd be interested how much problematic
code is out there.  Any idea how to grep it?

-Y