| Message ID | AM4PR0701MB2162F941CAB008360282B6B5E4080@AM4PR0701MB2162.eurprd07.prod.outlook.com |
|---|---|
| State | New |
| Headers | show |
On Wed, 20 Jul 2016, Bernd Edlinger wrote: > On 07/20/16 12:46, Richard Biener wrote: > > On Wed, 20 Jul 2016, Richard Biener wrote: > > > >> On Tue, 19 Jul 2016, Bernd Edlinger wrote: > >> > >>> 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. > >> > >> Returning ECF_MAY_BE_ALLOCA is safe. Just wanted to mention this, > >> regardless of the followups you already received. > > > > Oh, and maybe you can factor out the less controversical parts, > > namely ignoring the __builtin_ prefix. I don't think that > > calling __builtin_setjmp in an environment where setjmp is not a > > builtin should beave like setjmp (it will call a function named > > '__builtin_setjmp'). > > > I wonder how I manage to dig out such contriversical things ;) > > But you are right, that would at least be a start. > > So this patch is what you requested: > > Remove the handling of the __builtin_ prefix from special_function_p > and add the returns_twice attribute to the __builtin_setjmp declaration > instead. > > Is it OK after boot-strap and regression-testing? I think the __builtin_setjmp change is wrong - __builtin_setjmp is _not_ 'setjmp' it is part of the GCC internal machinery (using setjmp and longjmp in the end) for SJLJ exception handing. Am I correct Eric? Thanks, Richard. > > Thanks > Bernd. >
On 07/20/16 13:53, Richard Biener wrote: > On Wed, 20 Jul 2016, Bernd Edlinger wrote: > >> On 07/20/16 12:46, Richard Biener wrote: >>> On Wed, 20 Jul 2016, Richard Biener wrote: >>> >>>> On Tue, 19 Jul 2016, Bernd Edlinger wrote: >>>> >>>>> 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. >>>> >>>> Returning ECF_MAY_BE_ALLOCA is safe. Just wanted to mention this, >>>> regardless of the followups you already received. >>> >>> Oh, and maybe you can factor out the less controversical parts, >>> namely ignoring the __builtin_ prefix. I don't think that >>> calling __builtin_setjmp in an environment where setjmp is not a >>> builtin should beave like setjmp (it will call a function named >>> '__builtin_setjmp'). >> >> >> I wonder how I manage to dig out such contriversical things ;) >> >> But you are right, that would at least be a start. >> >> So this patch is what you requested: >> >> Remove the handling of the __builtin_ prefix from special_function_p >> and add the returns_twice attribute to the __builtin_setjmp declaration >> instead. >> >> Is it OK after boot-strap and regression-testing? > > I think the __builtin_setjmp change is wrong - __builtin_setjmp is > _not_ 'setjmp' it is part of the GCC internal machinery (using setjmp > and longjmp in the end) for SJLJ exception handing. > I do think that part is correct. DEF_GCC_BUILTIN adds the __builtin_ prefix, but does not overload the standard setjmp function. /* A GCC builtin (like __builtin_saveregs) is provided by the compiler, but does not correspond to a function in the standard library. */ #undef DEF_GCC_BUILTIN #define DEF_GCC_BUILTIN(ENUM, NAME, TYPE, ATTRS) \ DEF_BUILTIN (ENUM, "__builtin_" NAME, BUILT_IN_NORMAL, TYPE, BT_LAST, \ false, false, false, ATTRS, true, true) And to define a builtin without __builtin_, there is a DEF_SYNC_BUILTIN macro. And DEF_LIB_BUILTIN defines both ways. However, there is no simple way to define setjmp and __builtin_setjmp to different builtin functions. Thanks Bernd. > Am I correct Eric? > > Thanks, > Richard. > >> >> Thanks >> Bernd. >> >
On 07/20/2016 05:53 AM, Richard Biener wrote: >> Is it OK after boot-strap and regression-testing? > > I think the __builtin_setjmp change is wrong - __builtin_setjmp is > _not_ 'setjmp' it is part of the GCC internal machinery (using setjmp > and longjmp in the end) for SJLJ exception handing. > > Am I correct Eric? That is correct. __builtin_setjmp (and friends) are part of the SJLJ exception handling code. They use a fixed sized buffer (5 words) to store the key items (as opposed to the OS defined jmp_buf structure which is usually considerably larger). jeff
On 07/20/16 18:15, Jeff Law wrote: > On 07/20/2016 05:53 AM, Richard Biener wrote: >>> Is it OK after boot-strap and regression-testing? >> >> I think the __builtin_setjmp change is wrong - __builtin_setjmp is >> _not_ 'setjmp' it is part of the GCC internal machinery (using setjmp >> and longjmp in the end) for SJLJ exception handing. >> >> Am I correct Eric? > That is correct. __builtin_setjmp (and friends) are part of the SJLJ > exception handling code. They use a fixed sized buffer (5 words) to > store the key items (as opposed to the OS defined jmp_buf structure > which is usually considerably larger). > > jeff Yes. __builtin_setjmp is declared in builtins.def: DEF_GCC_BUILTIN (BUILT_IN_SETJMP, "setjmp", BT_FN_INT_PTR, ATTR_NOTHROW_LEAF_LIST) It is visible in C as __builtin_setjmp, and it special_function_p adds the ECF_RETURNS_TWICE | ECF_LEAF. So it becomes equivalent to this: int __builtin_setjmp(void*) __attribute__((returns_twice, nothrow, leaf)) after special_function_p does it's magic. If I remove the recognition of "__builtin_" from special_function_p I have to add the returns_twice attribute in the DEF_GCC_BUILTIN. Otherwise, I would get wrong code on all platforms, because __builtin_setjmp saves only IP, SP, and FP registers. Everything in the normal test suite keeps on going with the patch, but is there anything that I have to do to make sure that the SJLJ eh is still working? It is not the default on x86_64, right? Bernd.
On 07/20/2016 10:30 AM, Bernd Edlinger wrote: > On 07/20/16 18:15, Jeff Law wrote: >> On 07/20/2016 05:53 AM, Richard Biener wrote: >>>> Is it OK after boot-strap and regression-testing? >>> >>> I think the __builtin_setjmp change is wrong - __builtin_setjmp is >>> _not_ 'setjmp' it is part of the GCC internal machinery (using setjmp >>> and longjmp in the end) for SJLJ exception handing. >>> >>> Am I correct Eric? >> That is correct. __builtin_setjmp (and friends) are part of the SJLJ >> exception handling code. They use a fixed sized buffer (5 words) to >> store the key items (as opposed to the OS defined jmp_buf structure >> which is usually considerably larger). >> >> jeff > > Yes. __builtin_setjmp is declared in builtins.def: > > DEF_GCC_BUILTIN (BUILT_IN_SETJMP, "setjmp", BT_FN_INT_PTR, > ATTR_NOTHROW_LEAF_LIST) > > It is visible in C as __builtin_setjmp, and it special_function_p > adds the ECF_RETURNS_TWICE | ECF_LEAF. > > So it becomes equivalent to this: > > int __builtin_setjmp(void*) __attribute__((returns_twice, nothrow, > leaf)) > > after special_function_p does it's magic. > > If I remove the recognition of "__builtin_" from special_function_p > I have to add the returns_twice attribute in the DEF_GCC_BUILTIN. > Otherwise, I would get wrong code on all platforms, because > __builtin_setjmp saves only IP, SP, and FP registers. > > Everything in the normal test suite keeps on going with the patch, > but is there anything that I have to do to make sure that the > SJLJ eh is still working? It is not the default on x86_64, right? Very few targets continue to use SJLJ eh (perhaps just cygwin/mingw). *But* I think the Ada front-end explicitly uses SJLJ EH, so if you want to get some smoke testing, the Ada testsuite is probably the place to go. Jeff
On 07/20/16 21:00, Jeff Law wrote: > On 07/20/2016 10:30 AM, Bernd Edlinger wrote: >> On 07/20/16 18:15, Jeff Law wrote: >>> On 07/20/2016 05:53 AM, Richard Biener wrote: >>>>> Is it OK after boot-strap and regression-testing? >>>> >>>> I think the __builtin_setjmp change is wrong - __builtin_setjmp is >>>> _not_ 'setjmp' it is part of the GCC internal machinery (using setjmp >>>> and longjmp in the end) for SJLJ exception handing. >>>> >>>> Am I correct Eric? >>> That is correct. __builtin_setjmp (and friends) are part of the SJLJ >>> exception handling code. They use a fixed sized buffer (5 words) to >>> store the key items (as opposed to the OS defined jmp_buf structure >>> which is usually considerably larger). >>> >>> jeff >> >> Yes. __builtin_setjmp is declared in builtins.def: >> >> DEF_GCC_BUILTIN (BUILT_IN_SETJMP, "setjmp", BT_FN_INT_PTR, >> ATTR_NOTHROW_LEAF_LIST) >> >> It is visible in C as __builtin_setjmp, and it special_function_p >> adds the ECF_RETURNS_TWICE | ECF_LEAF. >> >> So it becomes equivalent to this: >> >> int __builtin_setjmp(void*) __attribute__((returns_twice, nothrow, >> leaf)) >> >> after special_function_p does it's magic. >> >> If I remove the recognition of "__builtin_" from special_function_p >> I have to add the returns_twice attribute in the DEF_GCC_BUILTIN. >> Otherwise, I would get wrong code on all platforms, because >> __builtin_setjmp saves only IP, SP, and FP registers. >> >> Everything in the normal test suite keeps on going with the patch, >> but is there anything that I have to do to make sure that the >> SJLJ eh is still working? It is not the default on x86_64, right? > Very few targets continue to use SJLJ eh (perhaps just cygwin/mingw). > *But* I think the Ada front-end explicitly uses SJLJ EH, so if you want > to get some smoke testing, the Ada testsuite is probably the place to go. > > Jeff Good. I always include ada and go, just to be on the safe side. The reg-test of the __builtin-setjmp patch is still running but the ada part is already complete: === acats Summary === # of expected passes 2320 # of unexpected failures 0 Native configuration is x86_64-pc-linux-gnu === gnat tests === Running target unix FAIL: gnat.dg/vect3.adb scan-tree-dump-times vect "vectorized 1 loops" 15 FAIL: gnat.dg/vect6.adb scan-tree-dump-times vect "vectorized 1 loops" 15 === gnat Summary === # of expected passes 2511 # of unexpected failures 2 # of expected failures 22 # of unsupported tests 3 /home/ed/gnu/gcc-build/gcc/gnatmake version 7.0.0 20160720 (experimental) the failures are already there since a few months. Bernd.
> Very few targets continue to use SJLJ eh (perhaps just cygwin/mingw). > *But* I think the Ada front-end explicitly uses SJLJ EH, so if you want > to get some smoke testing, the Ada testsuite is probably the place to go. Right, the Ada front-end uses an EH scheme directly based on __builtin_setjmp (which is similar but distinct from the regular SJLJ EH because the front-end directly manages the SJLJ buffers) for internal EH. Note that it's on the host only, for the target it uses the same EH scheme as C++/Java/etc.
On Wed, Jul 20, 2016 at 1:02 PM, Eric Botcazou <ebotcazou@adacore.com> wrote: >> Very few targets continue to use SJLJ eh (perhaps just cygwin/mingw). >> *But* I think the Ada front-end explicitly uses SJLJ EH, so if you want >> to get some smoke testing, the Ada testsuite is probably the place to go. > > Right, the Ada front-end uses an EH scheme directly based on __builtin_setjmp > (which is similar but distinct from the regular SJLJ EH because the front-end > directly manages the SJLJ buffers) for internal EH. Note that it's on the > host only, for the target it uses the same EH scheme as C++/Java/etc. > libcilkrts/include/internal/abi.h:# define CILK_SETJMP(X) __builtin_setjmp(X)
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_SETJMP): Use ATTR_RT_NOTHROW_LEAF_LIST here. * calls.c (special_function_p): Remove the special handling of the "__builtin_" prefix. Index: gcc/builtin-attrs.def =================================================================== --- gcc/builtin-attrs.def (Revision 238382) +++ gcc/builtin-attrs.def (Arbeitskopie) @@ -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 (Arbeitskopie) @@ -837,7 +837,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) Index: gcc/calls.c =================================================================== --- gcc/calls.c (Revision 238382) +++ gcc/calls.c (Arbeitskopie) @@ -514,14 +514,10 @@ special_function_p (const_tree fndecl, int flags) && ! strcmp (name, "alloca")) flags |= ECF_MAY_BE_ALLOCA; - /* Disregard prefix _, __, __x or __builtin_. */ + /* Disregard prefix _, __ or __x. */ if (name[0] == '_') { - if (name[1] == '_' - && name[2] == 'b' - && !strncmp (name + 3, "uiltin_", 7)) - tname += 10; - else if (name[1] == '_' && name[2] == 'x') + if (name[1] == '_' && name[2] == 'x') tname += 3; else if (name[1] == '_') tname += 2;