Preliminary mainline patch for __attribute__ ((sentinel))

["Kaveh R. Ghazi" <ghazi at caip dot rutgers dot edu>]

Based on the thread and proposal I made here:
http://gcc.gnu.org/ml/gcc/2004-08/msg01068.html
and the incremental approach which was agreed to here:
http://gcc.gnu.org/ml/gcc/2004-08/msg01100.html

I've gone ahead and implemented "step 1" of my proposal for
__attribute__ ((sentinel)), which is to accept the attribute with no
position or value arguments and assume that the sentinel goes in the
last argument and is a NULL.

In doing so, I encountered a problem and I'll explain my workaround.
I initially believed that GCC always defines NULL to (void*)0 on every
single platform because GCC completely replaces the system's stddef.h
with it's own copy which always "does the right thing(tm)" WRT
defining NULL.

However much to my chagrin I've since recalled and realized that some
systems define NULL with integer types in other header files (all
suitably guarded by #ifndef NULL).  Solaris for one defines NULL in no
less than 15 different headers!  It defines NULL to 0 for ILP32 mode
and 0L for LP64 like so in stdio.h/stdlib.h:
	
	#ifndef	NULL
	#if defined(_LP64) && !defined(__cplusplus)
	#define	NULL	0L
	#else
	#define	NULL	0
	#endif
	#endif

So depending on whether you include stddef.h first or stdio.h first,
you get different types for NULL.  In practice this works fine for
variadic functions, e.g. we've used NULL to terminate concat() for
years and I've yet to see a reported problem.  As long as we use the
system's definition of NULL we should be okay.

I do _not_ want to consider fixincludes for these 15 headers (they're
not all the same) plus similar hacks for every other system out there.
I also think a warning should avoid false positives to be useful.
Being too strict about requiring (void*)0 will make this warning
useless on non-glibc boxes.

My solution is to accept either (void*)0 or an integer zero with the
same width as a pointer.  To everyone who thinks this is bad, this is
a step forward and IMHO the best we can do.  The alternative is
another round of arguing and no progress.

The main objection I can foresee is that it won't catch unadorned zero
on ILP32 systems such as:

	execl ("/bin/ls", "-aFC", 0);

My thought is we already suffer such a situation in format checking
for this:

	 printf ("%u\n", (size_t)10);

The above printf won't get a -Wformat warning unless you are in LP64
mode and size_t is longer than an int.  Similarly the unadorned zero
used as a sentinel won't be caught unless you compile your code in
LP64 mode.  The lesson here is that if you want to check your code for
portability to LP64, compile it on such a system and look at new
warnings.

For ABIs where sizeof(0) always equals sizeof(void*0) but which do not
have the same variadic passing convention for ints and pointers, we
can perhaps add a stricter flag not included in -Wall that only
accepts (void*)0.  Perhaps activated at -Wformat=2, but that's an
incremental improvement for later.  For the moment, this is the best
compromise I can create that gets us this attribute into GCC.  (My
time for this is not unlimited.)


Here is my patch.  It compiles and passes "make" and a hand written
testcase in C and C++, but I haven't gone through the whole bootstrap
process yet.  I'd like to see if we can proceed with the above
compromise.  If we can achieve consensus, I'll create a real testcase
and go through the usual round of testing.

Comments?

		Thanks,
		--Kaveh


2004-08-28  Kaveh R. Ghazi  <ghazi@caip.rutgers.edu>

gcc:
	* builtin-attrs.def (ATTR_SENTINEL, ATTR_SENTINEL_NOTHROW_LIST):
	New.
	* builtins.def (BUILT_IN_EXECL, BUILT_IN_EXECLP): Add `sentinel'
	attribute.
	* c-common.c (handle_sentinel_attribute, check_function_sentinel):
	New functions.
	(c_common_attribute_table): Add `sentinel' attribute.
	(check_function_arguments): Handle `sentinel' attribute.
	* doc/extend.texi: Document `sentinel' attribute.
	
include:
	* ansidecl.h (ATTRIBUTE_SENTINEL): Define.
	* libiberty.h (concat, reconcat, concat_length, concat_copy,
	concat_copy2): Use ATTRIBUTE_SENTINEL.
	
diff -rup orig/egcc-CVS20040827/gcc/builtin-attrs.def egcc-CVS20040827/gcc/builtin-attrs.def
--- orig/egcc-CVS20040827/gcc/builtin-attrs.def	Tue May 11 16:59:12 2004
+++ egcc-CVS20040827/gcc/builtin-attrs.def	Sat Aug 28 10:08:04 2004
@@ -84,6 +84,7 @@ DEF_ATTR_IDENT (ATTR_GCC_CDIAG, "gcc_cdi
 DEF_ATTR_IDENT (ATTR_GCC_CXXDIAG, "gcc_cxxdiag")
 DEF_ATTR_IDENT (ATTR_PURE, "pure")
 DEF_ATTR_IDENT (ATTR_SCANF, "scanf")
+DEF_ATTR_IDENT (ATTR_SENTINEL, "sentinel")
 DEF_ATTR_IDENT (ATTR_STRFMON, "strfmon")
 DEF_ATTR_IDENT (ATTR_STRFTIME, "strftime")
 
@@ -96,6 +97,8 @@ DEF_ATTR_TREE_LIST (ATTR_PURE_NOTHROW_LI
 DEF_ATTR_TREE_LIST (ATTR_NORETURN_NOTHROW_LIST, ATTR_NORETURN,	\
 			ATTR_NULL, ATTR_NOTHROW_LIST)
 DEF_ATTR_TREE_LIST (ATTR_MALLOC_NOTHROW_LIST, ATTR_MALLOC,	\
+			ATTR_NULL, ATTR_NOTHROW_LIST)
+DEF_ATTR_TREE_LIST (ATTR_SENTINEL_NOTHROW_LIST, ATTR_SENTINEL,	\
 			ATTR_NULL, ATTR_NOTHROW_LIST)
 
 DEF_ATTR_TREE_LIST (ATTR_NOTHROW_NONNULL_1, ATTR_NONNULL, ATTR_LIST_1, \
diff -rup orig/egcc-CVS20040827/gcc/builtins.def egcc-CVS20040827/gcc/builtins.def
--- orig/egcc-CVS20040827/gcc/builtins.def	Wed Aug 11 00:15:55 2004
+++ egcc-CVS20040827/gcc/builtins.def	Sat Aug 28 10:08:04 2004
@@ -554,8 +554,8 @@ DEF_GCC_BUILTIN        (BUILT_IN_DWARF_C
 DEF_GCC_BUILTIN        (BUILT_IN_DWARF_SP_COLUMN, "dwarf_sp_column", BT_FN_UINT, ATTR_NULL)
 DEF_GCC_BUILTIN        (BUILT_IN_EH_RETURN, "eh_return", BT_FN_VOID_PTRMODE_PTR, ATTR_NORETURN_NOTHROW_LIST)
 DEF_GCC_BUILTIN        (BUILT_IN_EH_RETURN_DATA_REGNO, "eh_return_data_regno", BT_FN_INT_INT, ATTR_NULL)
-DEF_EXT_LIB_BUILTIN        (BUILT_IN_EXECL, "execl", BT_FN_INT_CONST_STRING_CONST_STRING_VAR, ATTR_NOTHROW_LIST)
-DEF_EXT_LIB_BUILTIN        (BUILT_IN_EXECLP, "execlp", BT_FN_INT_CONST_STRING_CONST_STRING_VAR, ATTR_NOTHROW_LIST)
+DEF_EXT_LIB_BUILTIN        (BUILT_IN_EXECL, "execl", BT_FN_INT_CONST_STRING_CONST_STRING_VAR, ATTR_SENTINEL_NOTHROW_LIST)
+DEF_EXT_LIB_BUILTIN        (BUILT_IN_EXECLP, "execlp", BT_FN_INT_CONST_STRING_CONST_STRING_VAR, ATTR_SENTINEL_NOTHROW_LIST)
 DEF_EXT_LIB_BUILTIN        (BUILT_IN_EXECLE, "execle", BT_FN_INT_CONST_STRING_CONST_STRING_VAR, ATTR_NOTHROW_LIST)
 DEF_EXT_LIB_BUILTIN        (BUILT_IN_EXECV, "execv", BT_FN_INT_CONST_STRING_PTR_CONST_STRING, ATTR_NOTHROW_LIST)
 DEF_EXT_LIB_BUILTIN        (BUILT_IN_EXECVP, "execvp", BT_FN_INT_CONST_STRING_PTR_CONST_STRING, ATTR_NOTHROW_LIST)
diff -rup orig/egcc-CVS20040827/gcc/c-common.c egcc-CVS20040827/gcc/c-common.c
--- orig/egcc-CVS20040827/gcc/c-common.c	Thu Aug 26 20:00:33 2004
+++ egcc-CVS20040827/gcc/c-common.c	Sat Aug 28 10:08:04 2004
@@ -552,6 +552,7 @@ static tree handle_nothrow_attribute (tr
 static tree handle_cleanup_attribute (tree *, tree, tree, int, bool *);
 static tree handle_warn_unused_result_attribute (tree *, tree, tree, int,
 						 bool *);
+static tree handle_sentinel_attribute (tree *, tree, tree, int, bool *);
 
 static void check_function_nonnull (tree, tree);
 static void check_nonnull_arg (void *, tree, unsigned HOST_WIDE_INT);
@@ -630,6 +631,8 @@ const struct attribute_spec c_common_att
 			      handle_cleanup_attribute },
   { "warn_unused_result",     0, 0, false, true, true,
 			      handle_warn_unused_result_attribute },
+  { "sentinel",               0, 0, false, true, true,
+			      handle_sentinel_attribute },
   { NULL,                     0, 0, false, false, false, NULL }
 };
 
@@ -5023,6 +5026,46 @@ check_function_nonnull (tree attrs, tree
     }
 }
 
+/* Check the last argument of a function call is NULL.  */
+
+static void
+check_function_sentinel (tree attrs, tree params)
+{
+  tree attr = lookup_attribute ("sentinel", attrs);
+
+  if (attr)
+    {
+      if (!params)
+	warning ("missing sentinel in function call");
+      else
+        {
+	  tree param, value;
+
+	  /* Find the last parameter.  */
+	  for (param = params; TREE_CHAIN (param); param = TREE_CHAIN (param))
+	    ;
+	  value = TREE_VALUE (param);
+
+	  if (! integer_zerop (value))
+	    {
+	      warning ("missing sentinel in function call");
+	      return;
+	    }
+
+	  /* Unfortunately, some systems define NULL as 0 (ILP32) or
+	     as 0L (LP64) instead of (void*)0.  So we avoid warning if
+	     the size of the integer zero we got equals the size of a
+	     pointer.  */
+	  if (! POINTER_TYPE_P (TREE_TYPE (value))
+	      && int_size_in_bytes (TREE_TYPE (value)) * BITS_PER_UNIT != POINTER_SIZE)
+	    {
+	      warning ("sentinel size does not equal pointer size");
+	      return;
+	    }
+	}
+    }
+}
+
 /* Helper for check_function_nonnull; given a list of operands which
    must be non-null in ARGS, determine if operand PARAM_NUM should be
    checked.  */
@@ -5164,6 +5207,31 @@ handle_warn_unused_result_attribute (tre
 
   return NULL_TREE;
 }
+
+/* Handle a "sentinel" attribute.  */
+
+static tree
+handle_sentinel_attribute (tree *node, tree name,
+			   tree ARG_UNUSED (args),
+			   int ARG_UNUSED (flags), bool *no_add_attrs)
+{
+  tree params = TYPE_ARG_TYPES (*node);
+
+  if (!params)
+    return NULL_TREE;
+
+  while (TREE_CHAIN (params))
+    params = TREE_CHAIN (params);
+
+  if (VOID_TYPE_P (TREE_VALUE (params)))
+    {
+      warning ("`%s' attribute only applies to variadic functions",
+	       IDENTIFIER_POINTER (name));
+      *no_add_attrs = true;
+    }
+  
+  return NULL_TREE;
+}
 
 /* Check for valid arguments being passed to a function.  */
 void
@@ -5178,7 +5246,10 @@ check_function_arguments (tree attrs, tr
   /* Check for errors in format strings.  */
 
   if (warn_format)
-    check_function_format (attrs, params);
+    {
+      check_function_format (attrs, params);
+      check_function_sentinel (attrs, params);
+    }
 }
 
 /* Generic argument checking recursion routine.  PARAM is the argument to
diff -rup orig/egcc-CVS20040827/gcc/doc/extend.texi egcc-CVS20040827/gcc/doc/extend.texi
--- orig/egcc-CVS20040827/gcc/doc/extend.texi	Tue Aug 17 18:51:28 2004
+++ egcc-CVS20040827/gcc/doc/extend.texi	Sat Aug 28 10:08:04 2004
@@ -1496,7 +1496,7 @@ attributes when making a declaration.  T
 attribute specification inside double parentheses.  The following
 attributes are currently defined for functions on all targets:
 @code{noreturn}, @code{noinline}, @code{always_inline},
-@code{pure}, @code{const}, @code{nothrow},
+@code{pure}, @code{const}, @code{nothrow}, @code{sentinel},
 @code{format}, @code{format_arg}, @code{no_instrument_function},
 @code{section}, @code{constructor}, @code{destructor}, @code{used},
 @code{unused}, @code{deprecated}, @code{weak}, @code{malloc},
@@ -2110,6 +2110,18 @@ Some file formats do not support arbitra
 attribute is not available on all platforms.
 If you need to map the entire contents of a module to a particular
 section, consider using the facilities of the linker instead.
+
+@item sentinel
+@cindex @code{sentinel} function attribute
+This function attribute ensures that the last parameter in a function
+call is an explicit @code{NULL}.  The attribute is only valid on
+variadic functions.  For example the attribute is automatically set for
+the built-in functions @code{execl} and @code{execlp} where @code{NULL}
+is the marker for argument list termination.  Because @code{NULL} is
+defined in various ways on different systems, an acceptable @code{NULL}
+is considered to be @code{(void*)0} or an integer zero whose size equals
+that of a pointer.  The warnings for missing or incorrect sentinels are
+enabled with @option{-Wformat}.
 
 @item short_call
 See long_call/short_call.
diff -rup orig/egcc-CVS20040827/include/ansidecl.h egcc-CVS20040827/include/ansidecl.h
--- orig/egcc-CVS20040827/include/ansidecl.h	Sat Jul 24 13:49:27 2004
+++ egcc-CVS20040827/include/ansidecl.h	Sat Aug 28 10:08:04 2004
@@ -322,6 +322,15 @@ So instead we use the macro below and te
 # define ATTRIBUTE_NULL_PRINTF_5 ATTRIBUTE_NULL_PRINTF(5, 6)
 #endif /* ATTRIBUTE_NULL_PRINTF */
 
+/* Attribute `sentinel' was valid as of gcc 3.5.  */
+#ifndef ATTRIBUTE_SENTINEL
+# if (GCC_VERSION >= 3005)
+#  define ATTRIBUTE_SENTINEL __attribute__ ((__sentinel__))
+# else
+#  define ATTRIBUTE_SENTINEL
+# endif /* GNUC >= 3.5 */
+#endif /* ATTRIBUTE_SENTINEL */
+
 /* We use __extension__ in some places to suppress -pedantic warnings
    about GCC extensions.  This feature didn't work properly before
    gcc 2.8.  */
diff -rup orig/egcc-CVS20040827/include/libiberty.h egcc-CVS20040827/include/libiberty.h
--- orig/egcc-CVS20040827/include/libiberty.h	Mon Aug  2 12:45:52 2004
+++ egcc-CVS20040827/include/libiberty.h	Sat Aug 28 10:08:04 2004
@@ -93,7 +93,7 @@ extern char *lrealpath PARAMS ((const ch
    the last argument of this function, to terminate the list of
    strings.  Allocates memory using xmalloc.  */
 
-extern char *concat PARAMS ((const char *, ...)) ATTRIBUTE_MALLOC;
+extern char *concat PARAMS ((const char *, ...)) ATTRIBUTE_MALLOC ATTRIBUTE_SENTINEL;
 
 /* Concatenate an arbitrary number of strings.  You must pass NULL as
    the last argument of this function, to terminate the list of
@@ -102,27 +102,27 @@ extern char *concat PARAMS ((const char 
    pointer to be freed after the new string is created, similar to the
    way xrealloc works.  */
 
-extern char *reconcat PARAMS ((char *, const char *, ...)) ATTRIBUTE_MALLOC;
+extern char *reconcat PARAMS ((char *, const char *, ...)) ATTRIBUTE_MALLOC ATTRIBUTE_SENTINEL;
 
 /* Determine the length of concatenating an arbitrary number of
    strings.  You must pass NULL as the last argument of this function,
    to terminate the list of strings.  */
 
-extern unsigned long concat_length PARAMS ((const char *, ...));
+extern unsigned long concat_length PARAMS ((const char *, ...)) ATTRIBUTE_SENTINEL;
 
 /* Concatenate an arbitrary number of strings into a SUPPLIED area of
    memory.  You must pass NULL as the last argument of this function,
    to terminate the list of strings.  The supplied memory is assumed
    to be large enough.  */
 
-extern char *concat_copy PARAMS ((char *, const char *, ...));
+extern char *concat_copy PARAMS ((char *, const char *, ...)) ATTRIBUTE_SENTINEL;
 
 /* Concatenate an arbitrary number of strings into a GLOBAL area of
    memory.  You must pass NULL as the last argument of this function,
    to terminate the list of strings.  The supplied memory is assumed
    to be large enough.  */
 
-extern char *concat_copy2 PARAMS ((const char *, ...));
+extern char *concat_copy2 PARAMS ((const char *, ...)) ATTRIBUTE_SENTINEL;
 
 /* This is the global area used by concat_copy2.  */