diff mbox

Fix unsafe function attributes for special functions (PR 71876)

Message ID AM4PR0701MB21627909D37FCAE8D4590037E4090@AM4PR0701MB2162.eurprd07.prod.outlook.com
State New
Headers show

Commit Message

Bernd Edlinger July 21, 2016, 9:57 p.m. UTC 
Hi,

based on the discussion here, I have updated my patch again...

This is the rest of the patch, which removes outdated function names,
and creates built-in definitions for vfork, getcontext, savectx.
These built-ins have the return_twice attribute but not the
leaf attribute, because we do not really know, what these functions
do.

The reason for ceating the builtin functions is, that I would like
to get a warning about conflicting builtin definition if someone
accidentally picks the name of one of these less well known special
functions, which are _not_ reserved names in most environments.

I do not define builtins (without __builtin_ prefix) for setjmp and
sigsetjmp because these are like wildcards, and they fall in the
well-known category anyways.

I still retain the handling of these functions in special_function_p
because even in a free standing environment, returning
ECF_RETURNS_TWICE is on the safe side.



Is it OK for trunk after boot-strap and regression-testing?


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

	PR middle-end/71876
	* builtin-attrs.def (ATTR_RT_NOTHROW_LIST): New return twice
	no leaf attribute.
	* builtins.def (BUILT_IN_GETCONTEXT, BUILT_IN_SAVECTX,
	BUILT_IN_VFORK): New builtins using ATTR_RT_NOTHROW_LIST.
	* calls.c (special_function_p): Remove special handling of
	"setjmp_syscall", "qsetjmp", "longjmp", "siglongjmp" and the
	prefix "__x".  Recognize "savectx", "vfork" and "getcontext" only
	without prefix.  Remove potentially unsafe ECF_LEAF and ECF_NORETURN.

Comments

Richard Biener July 22, 2016, 12:28 p.m. UTC | #1 
On Thu, 21 Jul 2016, Bernd Edlinger wrote:

> Hi,
> 
> based on the discussion here, I have updated my patch again...
> 
> This is the rest of the patch, which removes outdated function names,
> and creates built-in definitions for vfork, getcontext, savectx.
> These built-ins have the return_twice attribute but not the
> leaf attribute, because we do not really know, what these functions
> do.
> 
> The reason for ceating the builtin functions is, that I would like
> to get a warning about conflicting builtin definition if someone
> accidentally picks the name of one of these less well known special
> functions, which are _not_ reserved names in most environments.
> 
> I do not define builtins (without __builtin_ prefix) for setjmp and
> sigsetjmp because these are like wildcards, and they fall in the
> well-known category anyways.
> 
> I still retain the handling of these functions in special_function_p
> because even in a free standing environment, returning
> ECF_RETURNS_TWICE is on the safe side.
> 
> 
> 
> Is it OK for trunk after boot-strap and regression-testing?

As DEF_EXT_LIB_BUILTIN won't have any effect with -std=c99 for example
I don't think having the builtins helps much.  I think we have
this name-matching function for correctness purposes (and even my
local glibc doesn't declare [v]fork as returning twice nor does
the existing BUILT_IN_FORK btw).

So please simply leave special_function_p as-is, and simply just
return the for-correctness flags - ECF_RETURNS_TWICE and 
ECF_MAY_BE_ALLOCA.

I think all callers but flags_from_decl_or_type should vanish
(calling that function instead).

Richard.
Jakub Jelinek July 22, 2016, 12:36 p.m. UTC | #2 
On Fri, Jul 22, 2016 at 02:28:15PM +0200, Richard Biener wrote:
> As DEF_EXT_LIB_BUILTIN won't have any effect with -std=c99 for example
> I don't think having the builtins helps much.  I think we have

Well, -std=gnu11 is the default, so it will help.

	Jakub
Bernd Edlinger July 22, 2016, 1:03 p.m. UTC | #3 
On 07/22/16 14:28, Richard Biener wrote:
> On Thu, 21 Jul 2016, Bernd Edlinger wrote:
>
>> Hi,
>>
>> based on the discussion here, I have updated my patch again...
>>
>> This is the rest of the patch, which removes outdated function names,
>> and creates built-in definitions for vfork, getcontext, savectx.
>> These built-ins have the return_twice attribute but not the
>> leaf attribute, because we do not really know, what these functions
>> do.
>>
>> The reason for ceating the builtin functions is, that I would like
>> to get a warning about conflicting builtin definition if someone
>> accidentally picks the name of one of these less well known special
>> functions, which are _not_ reserved names in most environments.
>>
>> I do not define builtins (without __builtin_ prefix) for setjmp and
>> sigsetjmp because these are like wildcards, and they fall in the
>> well-known category anyways.
>>
>> I still retain the handling of these functions in special_function_p
>> because even in a free standing environment, returning
>> ECF_RETURNS_TWICE is on the safe side.
>>
>>
>>
>> Is it OK for trunk after boot-strap and regression-testing?
>
> As DEF_EXT_LIB_BUILTIN won't have any effect with -std=c99 for example
> I don't think having the builtins helps much.  I think we have
> this name-matching function for correctness purposes (and even my
> local glibc doesn't declare [v]fork as returning twice nor does
> the existing BUILT_IN_FORK btw).
>

fork does not have to, but it is a no-leaf function because it
calls pthread_atfork handlers.  So it has to be __TROWNL.

I think in the long run the header files just should be fixed.
But that will take a while...


Bernd.
diff mbox

Patch

Index: gcc/builtin-attrs.def
===================================================================
--- gcc/builtin-attrs.def	(revision 238611)
+++ 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_LIST, ATTR_RETURNS_TWICE,\
+			ATTR_NULL, ATTR_NOTHROW_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,\
Index: gcc/builtins.def
===================================================================
--- gcc/builtins.def	(revision 238611)
+++ 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_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)
@@ -836,6 +837,7 @@  DEF_GCC_BUILTIN        (BUILT_IN_PREFETCH, "prefet
 DEF_LIB_BUILTIN        (BUILT_IN_REALLOC, "realloc", BT_FN_PTR_PTR_SIZE, ATTR_NOTHROW_LEAF_LIST)
 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_EXT_LIB_BUILTIN    (BUILT_IN_SAVECTX, "savectx", BT_FN_VOID_PTR, ATTR_RT_NOTHROW_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_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)
@@ -849,6 +851,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_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 238611)
+++ 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) <= 11
       /* Exclude functions not at the file scope, or not `extern',
 	 since they are not the magic functions we would otherwise
 	 think they are.
@@ -514,43 +512,22 @@  special_function_p (const_tree fndecl, int flags)
 	  && ! strcmp (name, "alloca"))
 	flags |= ECF_MAY_BE_ALLOCA;
 
-      /* Disregard prefix _, __ or __x.  */
+      /* Disregard prefix _ or __.  */
       if (name[0] == '_')
 	{
-	  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 -ffreestanding.  */
+      if (! strcmp (tname, "setjmp")
+	  || ! strcmp (tname, "sigsetjmp")
+	  || ! strcmp (name, "savectx")
+	  || ! strcmp (name, "vfork")
+	  || ! strcmp (name, "getcontext"))
+	flags |= ECF_RETURNS_TWICE;
     }
 
   if (DECL_BUILT_IN_CLASS (fndecl) == BUILT_IN_NORMAL)