Fix unsafe function attributes for special functions (PR 71876)

Hi!

As discussed at https://gcc.gnu.org/bugzilla/show_bug.cgi?id=71876,
we have a _very_ old hack in gcc, that recognizes certain functions by
name, and inserts in some cases unsafe attributes, that don't work for
a freestanding environment.

It is unsafe to return ECF_MAY_BE_ALLOCA, ECF_LEAF and ECF_NORETURN
from special_function_p, just by the name of the function, especially
for less well known functions, like "getcontext" or "savectx", which
could easily used for something completely different.

Moreover, from the backend library we cannot check flag_hosted, or if
the function has "C" or "C++" binding.

So all these functions would get the leaf attribute, which makes it very
easy to construct some kind of wrong code examples.

This patch removes the unsafe flags, and adds them to built-in
functions instead.

The patch removes also support for some completely unknown function
names from the middle-end, because these functions were apparently in
use at some point in time, but are certainly dead by now.

It is however not possible to remove the special handling by name
altogether, because the glibc does not add the return_twice function
attribute on _setjmp, __sigsetjmp and getcontext until today; a glibc
BZ is filed at: https://sourceware.org/bugzilla/show_bug.cgi?id=20382

Without the return_twice attribute we would loose the -Wclobbered
warning, and some targets (spark, ia64, maybe others too) would even
generate wrong code.


Boot-strapped and reg-tested on x86_64-pc-linux-gnu.
OK for trunk?


Thanks
Bernd.
2016-07-19  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	PR middle-end/71876
	* builtin-attrs.def (ATTR_RT_NOTHROW_LEAF_LIST): New return twice
	attribute.
	* builtins.def (BUILT_IN_GETCONTEXT, BUILT_IN_VFORK): New built-in
	using ATTR_RT_NOTHROW_LEAF_LIST.
	(BUILT_IN_SETJMP): Use ATTR_RT_NOTHROW_LEAF_LIST here.
	* calls.c (special_function_p): Remove special handling of "alloca"
	by name, as well as "setjmp_syscall", "savectx", "qsetjmp" and the
	prefixes "__x" and "__builtin_".  Remove potentially unsafe ECF_LEAF
	and ECF_NORETURN from here, use attributes of built-in instead.

On Tue, Jul 19, 2016 at 04:20:55PM +0000, Bernd Edlinger wrote:
> As discussed at https://gcc.gnu.org/bugzilla/show_bug.cgi?id=71876,
> we have a _very_ old hack in gcc, that recognizes certain functions by
> name, and inserts in some cases unsafe attributes, that don't work for
> a freestanding environment.
> 
> It is unsafe to return ECF_MAY_BE_ALLOCA, ECF_LEAF and ECF_NORETURN
> from special_function_p, just by the name of the function, especially
> for less well known functions, like "getcontext" or "savectx", which
> could easily used for something completely different.
> 
> Moreover, from the backend library we cannot check flag_hosted, or if
> the function has "C" or "C++" binding.

I believe this will regress handling of various functions.
E.g. for alloca (as opposed to __builtin_alloca/__builtin_alloca_with_align,
this means EFC_MAY_BE_ALLOCA will not be set anymore.

_longjmp/siglongjmp will no longer be ECF_NORETURN (glibc
doesn't declare them as such), __sigsetjmp will no longer be ECF_LEAF.

	Jakub

On 07/19/16 18:37, Jakub Jelinek wrote:
> On Tue, Jul 19, 2016 at 04:20:55PM +0000, Bernd Edlinger wrote:
>> As discussed at https://gcc.gnu.org/bugzilla/show_bug.cgi?id=71876,
>> we have a _very_ old hack in gcc, that recognizes certain functions by
>> name, and inserts in some cases unsafe attributes, that don't work for
>> a freestanding environment.
>>
>> It is unsafe to return ECF_MAY_BE_ALLOCA, ECF_LEAF and ECF_NORETURN
>> from special_function_p, just by the name of the function, especially
>> for less well known functions, like "getcontext" or "savectx", which
>> could easily used for something completely different.
>>
>> Moreover, from the backend library we cannot check flag_hosted, or if
>> the function has "C" or "C++" binding.
>
> I believe this will regress handling of various functions.
> E.g. for alloca (as opposed to __builtin_alloca/__builtin_alloca_with_align,
> this means EFC_MAY_BE_ALLOCA will not be set anymore.
>

That depends on which options are used: with -ansi and -ffreestanding, 
alloca is just a normal function, which is kind of correct.

If you include glibc's <alloca.h>, alloca is directly defined to 
__builtin_alloca(x), which should work always.

If alloca is declared as void *alloca(size_t); it is also recognized as
built-in unless -ansi or -ffreestanding is used, so that handling was
in a way duplicated already.

So I see no regression here.

> _longjmp/siglongjmp will no longer be ECF_NORETURN (glibc
> doesn't declare them as such), __sigsetjmp will no longer be ECF_LEAF.

Which version of glibc do you refer to?

My 2.19-0ubuntu6.9 has:

extern void _longjmp (struct __jmp_buf_tag __env[1], int __val)
      __THROWNL __attribute__ ((__noreturn__));

extern int __sigsetjmp (struct __jmp_buf_tag __env[1], int __savemask) 
__THROWNL;

So yes, __THROWNL is "__attribute__ ((__nothrow__))".

But they also have __THROW around which is "__attribute__ ((__nothrow__
__LEAF))", so that is just a minor bug in the glibc header, the header
should declare it __THROW if it is no leaf.

If you are concerned about the leaf attribute, it would
be easy to add a builtin for _longjmp, and __sigsetjmp, as
the _ is reserved anyway.  However I considered it an implementation
detail of glibc, that could change, and I did not check newlib on that
either.

Should I add built-in for _longjmp and __sigsetjmp, and check if
that works for newlib too?


Thanks
Bernd.

On 07/19/16 19:30, Bernd Edlinger wrote:
> On 07/19/16 18:37, Jakub Jelinek wrote:
>> On Tue, Jul 19, 2016 at 04:20:55PM +0000, Bernd Edlinger wrote:
>>> As discussed at https://gcc.gnu.org/bugzilla/show_bug.cgi?id=71876,
>>> we have a _very_ old hack in gcc, that recognizes certain functions by
>>> name, and inserts in some cases unsafe attributes, that don't work for
>>> a freestanding environment.
>>>
>>> It is unsafe to return ECF_MAY_BE_ALLOCA, ECF_LEAF and ECF_NORETURN
>>> from special_function_p, just by the name of the function, especially
>>> for less well known functions, like "getcontext" or "savectx", which
>>> could easily used for something completely different.
>>>
>>> Moreover, from the backend library we cannot check flag_hosted, or if
>>> the function has "C" or "C++" binding.
>>
>> I believe this will regress handling of various functions.
>> E.g. for alloca (as opposed to
>> __builtin_alloca/__builtin_alloca_with_align,
>> this means EFC_MAY_BE_ALLOCA will not be set anymore.
>>
>
> That depends on which options are used: with -ansi and -ffreestanding,
> alloca is just a normal function, which is kind of correct.
>
> If you include glibc's <alloca.h>, alloca is directly defined to
> __builtin_alloca(x), which should work always.
>
> If alloca is declared as void *alloca(size_t); it is also recognized as
> built-in unless -ansi or -ffreestanding is used, so that handling was
> in a way duplicated already.
>
> So I see no regression here.
>
>> _longjmp/siglongjmp will no longer be ECF_NORETURN (glibc
>> doesn't declare them as such), __sigsetjmp will no longer be ECF_LEAF.
>
> Which version of glibc do you refer to?
>
> My 2.19-0ubuntu6.9 has:
>
> extern void _longjmp (struct __jmp_buf_tag __env[1], int __val)
>       __THROWNL __attribute__ ((__noreturn__));
>
> extern int __sigsetjmp (struct __jmp_buf_tag __env[1], int __savemask)
> __THROWNL;
>
> So yes, __THROWNL is "__attribute__ ((__nothrow__))".
>
> But they also have __THROW around which is "__attribute__ ((__nothrow__
> __LEAF))", so that is just a minor bug in the glibc header, the header
> should declare it __THROW if it is no leaf.
>
> If you are concerned about the leaf attribute, it would
> be easy to add a builtin for _longjmp, and __sigsetjmp, as
> the _ is reserved anyway.  However I considered it an implementation
> detail of glibc, that could change, and I did not check newlib on that
> either.
>
> Should I add built-in for _longjmp and __sigsetjmp, and check if
> that works for newlib too?
>
>
> Thanks
> Bernd.


I have tried to find a test case with setjmp/longjmp where the leaf
attribute on the setjmp makes a difference, and the most aggressive
test case I could think of was this:

cat test1.c
static long env0[16], env1[16];
static int x = 0;

int jmp(void*) __attribute__((returns_twice, nothrow, leaf));
void go(void*, void*, int) __attribute__((nothrow));
void doit()
{
   x++;
}

int
test()
{
   static  int xx;
   xx = x;
   jmp (env0);
   xx += x;
   jmp (env1);
   go (env0, env1, xx + x);
   return xx;
}

gcc -O3 -S test.c && inspect assembler code.

Here jmp is marked leaf and returns_twice, but go is not leaf
and can either call doit, or jump to env0 or env1, or simply
return. It would be wrong to expect "x" not to change between
the jmp calls.  gcc-4.8.4 generates invalid code out of this,
and correct code if leaf is not in the function declaration.
However on trunk the code is correct, and the assembler code is
completely identical with or without leaf.

I wondered when that was fixed...

So I googled a bit around "gcc returns_twice leaf", and found
something interesting, where you argumented that it would
be wrong to put the leaf attribute at the setjmp function
in glibc: https://bugzilla.redhat.com/show_bug.cgi?id=752905

"Jakub Jelinek 2011-11-10 14:38:50 EST

setjmp/__sigsetjmp/_setjmp definitely must be __THROWNL rather than __THROW.
Similarly setcontext and swapcontext.  They all have side-effects which 
can modify module static variables that don't have address taken."


I completely agree with you.  But how can it be, that special_function_p
seems to do the completely opposite thing, and add leaf to the setjmp
declaration even if that is not written in the header file?


Thanks
Bernd.

On Tue, 19 Jul 2016, Bernd Edlinger wrote:

> It is however not possible to remove the special handling by name
> altogether, because the glibc does not add the return_twice function
> attribute on _setjmp, __sigsetjmp and getcontext until today; a glibc
> BZ is filed at: https://sourceware.org/bugzilla/show_bug.cgi?id=20382

Once you have the attribute (__returns_twice__, of course, to be 
namespace-clean) in the headers, fixing old versions of the headers with 
fixincludes would seem appropriate (see what projects/beginner.html says 
about the special_function_p issue).