Message ID | AM4PR0701MB2162EA032484BB9F00425151E4370@AM4PR0701MB2162.eurprd07.prod.outlook.com |
---|---|
State | New |
Headers | show |
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).
Index: gcc/builtin-attrs.def =================================================================== --- gcc/builtin-attrs.def (revision 238382) +++ gcc/builtin-attrs.def (working copy) @@ -131,6 +131,8 @@ DEF_ATTR_TREE_LIST (ATTR_NORETURN_NOTHROW_LIST, AT ATTR_NULL, ATTR_NOTHROW_LIST) DEF_ATTR_TREE_LIST (ATTR_NORETURN_NOTHROW_LEAF_LIST, ATTR_NORETURN,\ ATTR_NULL, ATTR_NOTHROW_LEAF_LIST) +DEF_ATTR_TREE_LIST (ATTR_RT_NOTHROW_LEAF_LIST, ATTR_RETURNS_TWICE,\ + ATTR_NULL, ATTR_NOTHROW_LEAF_LIST) DEF_ATTR_TREE_LIST (ATTR_COLD_NOTHROW_LEAF_LIST, ATTR_COLD,\ ATTR_NULL, ATTR_NOTHROW_LEAF_LIST) DEF_ATTR_TREE_LIST (ATTR_COLD_NORETURN_NOTHROW_LEAF_LIST, ATTR_COLD,\ Index: gcc/builtins.def =================================================================== --- gcc/builtins.def (revision 238382) +++ gcc/builtins.def (working copy) @@ -796,6 +796,7 @@ DEF_EXT_LIB_BUILTIN (BUILT_IN_FINITED32, "finit DEF_EXT_LIB_BUILTIN (BUILT_IN_FINITED64, "finited64", BT_FN_INT_DFLOAT64, ATTR_CONST_NOTHROW_LEAF_LIST) DEF_EXT_LIB_BUILTIN (BUILT_IN_FINITED128, "finited128", BT_FN_INT_DFLOAT128, ATTR_CONST_NOTHROW_LEAF_LIST) DEF_GCC_BUILTIN (BUILT_IN_FPCLASSIFY, "fpclassify", BT_FN_INT_INT_INT_INT_INT_INT_VAR, ATTR_CONST_NOTHROW_TYPEGENERIC_LEAF) +DEF_EXT_LIB_BUILTIN (BUILT_IN_GETCONTEXT, "getcontext", BT_FN_INT_PTR, ATTR_RT_NOTHROW_LEAF_LIST) DEF_GCC_BUILTIN (BUILT_IN_ISFINITE, "isfinite", BT_FN_INT_VAR, ATTR_CONST_NOTHROW_TYPEGENERIC_LEAF) DEF_GCC_BUILTIN (BUILT_IN_ISINF_SIGN, "isinf_sign", BT_FN_INT_VAR, ATTR_CONST_NOTHROW_TYPEGENERIC_LEAF) DEF_C99_C90RES_BUILTIN (BUILT_IN_ISINF, "isinf", BT_FN_INT_VAR, ATTR_CONST_NOTHROW_TYPEGENERIC) @@ -837,7 +838,7 @@ DEF_LIB_BUILTIN (BUILT_IN_REALLOC, "realloc DEF_GCC_BUILTIN (BUILT_IN_RETURN, "return", BT_FN_VOID_PTR, ATTR_NORETURN_NOTHROW_LEAF_LIST) DEF_GCC_BUILTIN (BUILT_IN_RETURN_ADDRESS, "return_address", BT_FN_PTR_UINT, ATTR_LEAF_LIST) DEF_GCC_BUILTIN (BUILT_IN_SAVEREGS, "saveregs", BT_FN_PTR_VAR, ATTR_NULL) -DEF_GCC_BUILTIN (BUILT_IN_SETJMP, "setjmp", BT_FN_INT_PTR, ATTR_NOTHROW_LEAF_LIST) +DEF_GCC_BUILTIN (BUILT_IN_SETJMP, "setjmp", BT_FN_INT_PTR, ATTR_RT_NOTHROW_LEAF_LIST) DEF_EXT_LIB_BUILTIN (BUILT_IN_STRFMON, "strfmon", BT_FN_SSIZE_STRING_SIZE_CONST_STRING_VAR, ATTR_FORMAT_STRFMON_NOTHROW_3_4) DEF_LIB_BUILTIN (BUILT_IN_STRFTIME, "strftime", BT_FN_SIZE_STRING_SIZE_CONST_STRING_CONST_PTR, ATTR_FORMAT_STRFTIME_NOTHROW_3_0) DEF_GCC_BUILTIN (BUILT_IN_TRAP, "trap", BT_FN_VOID, ATTR_NORETURN_NOTHROW_LEAF_LIST) @@ -849,6 +850,7 @@ DEF_GCC_BUILTIN (BUILT_IN_VA_END, "va_end", DEF_GCC_BUILTIN (BUILT_IN_VA_START, "va_start", BT_FN_VOID_VALIST_REF_VAR, ATTR_NOTHROW_LEAF_LIST) DEF_GCC_BUILTIN (BUILT_IN_VA_ARG_PACK, "va_arg_pack", BT_FN_INT, ATTR_PURE_NOTHROW_LEAF_LIST) DEF_GCC_BUILTIN (BUILT_IN_VA_ARG_PACK_LEN, "va_arg_pack_len", BT_FN_INT, ATTR_PURE_NOTHROW_LEAF_LIST) +DEF_EXT_LIB_BUILTIN (BUILT_IN_VFORK, "vfork", BT_FN_PID, ATTR_RT_NOTHROW_LEAF_LIST) DEF_EXT_LIB_BUILTIN (BUILT_IN__EXIT, "_exit", BT_FN_VOID_INT, ATTR_NORETURN_NOTHROW_LEAF_LIST) DEF_C99_BUILTIN (BUILT_IN__EXIT2, "_Exit", BT_FN_VOID_INT, ATTR_NORETURN_NOTHROW_LEAF_LIST) Index: gcc/calls.c =================================================================== --- gcc/calls.c (revision 238382) +++ gcc/calls.c (working copy) @@ -474,8 +474,6 @@ emit_call_1 (rtx funexp, tree fntree ATTRIBUTE_UNU For example, if the function might return more than one time (setjmp), then set RETURNS_TWICE to a nonzero value. - Similarly set NORETURN if the function is in the longjmp family. - Set MAY_BE_ALLOCA for any memory allocation function that might allocate space from the stack such as alloca. */ @@ -491,7 +489,7 @@ special_function_p (const_tree fndecl, int flags) name_decl = DECL_NAME (cgraph_node::get (fndecl)->orig_decl); if (fndecl && name_decl - && IDENTIFIER_LENGTH (name_decl) <= 17 + && IDENTIFIER_LENGTH (name_decl) <= 12 /* Exclude functions not at the file scope, or not `extern', since they are not the magic functions we would otherwise think they are. @@ -506,55 +504,21 @@ special_function_p (const_tree fndecl, int flags) const char *name = IDENTIFIER_POINTER (name_decl); const char *tname = name; - /* We assume that alloca will always be called by name. It - makes no sense to pass it as a pointer-to-function to - anything that does not understand its behavior. */ - if (IDENTIFIER_LENGTH (name_decl) == 6 - && name[0] == 'a' - && ! strcmp (name, "alloca")) - flags |= ECF_MAY_BE_ALLOCA; - - /* Disregard prefix _, __, __x or __builtin_. */ + /* Disregard prefix _ or __. */ if (name[0] == '_') { - if (name[1] == '_' - && name[2] == 'b' - && !strncmp (name + 3, "uiltin_", 7)) - tname += 10; - else if (name[1] == '_' && name[2] == 'x') - tname += 3; - else if (name[1] == '_') + if (name[1] == '_') tname += 2; else tname += 1; } - if (tname[0] == 's') - { - if ((tname[1] == 'e' - && (! strcmp (tname, "setjmp") - || ! strcmp (tname, "setjmp_syscall"))) - || (tname[1] == 'i' - && ! strcmp (tname, "sigsetjmp")) - || (tname[1] == 'a' - && ! strcmp (tname, "savectx"))) - flags |= ECF_RETURNS_TWICE | ECF_LEAF; - - if (tname[1] == 'i' - && ! strcmp (tname, "siglongjmp")) - flags |= ECF_NORETURN; - } - else if ((tname[0] == 'q' && tname[1] == 's' - && ! strcmp (tname, "qsetjmp")) - || (tname[0] == 'v' && tname[1] == 'f' - && ! strcmp (tname, "vfork")) - || (tname[0] == 'g' && tname[1] == 'e' - && !strcmp (tname, "getcontext"))) - flags |= ECF_RETURNS_TWICE | ECF_LEAF; - - else if (tname[0] == 'l' && tname[1] == 'o' - && ! strcmp (tname, "longjmp")) - flags |= ECF_NORETURN; + /* ECF_RETURNS_TWICE is safe even for -ffrestanding. */ + if (! strcmp (tname, "setjmp") + || ! strcmp (tname, "sigsetjmp") + || ! strcmp (tname, "vfork") + || ! strcmp (tname, "getcontext")) + flags |= ECF_RETURNS_TWICE; } if (DECL_BUILT_IN_CLASS (fndecl) == BUILT_IN_NORMAL)