On 05/29/2017 11:24 PM, Yuri Gribov wrote:
> On Mon, May 29, 2017 at 8:14 AM, Yuri Gribov <tetra2...@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 <tetra2...@gmail.com>
> Date: Sun, 28 May 2017 21:02:20 +0100
> Subject: [PATCH] Added no_tail_call attribute.
>
> gcc/
> 2017-05-29 Yury Gribov <tetra2...@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 <tetra2...@gmail.com>
>
> * c-attribs.c: New attribute.
>
> gcc/testsuite/
> 2017-05-29 Yury Gribov <tetra2...@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
