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