[PATCH] Extend the `sentinel' attribute

[PATCH] Extend the `sentinel' attribute

[Ludovic Courtès]

[-- Attachment #1: Type: text/plain, Size: 1016 bytes --]

Hello,

The attached patch adds a second optional parameter to the `sentinel'
attribute, allowing the expected sentinel value to be specified.  IOW,
it allows `sentinel' to be used for non-NULL values and addresses bug
#28319:

  http://gcc.gnu.org/bugzilla/show_bug.cgi?id=28319

Does that seem like a reasonable change?

In addition, it would be nice to no limit sentinel values to pointer
types but this would be an incompatible change.  What do you think?

Thanks,
Ludovic.

PS: I have not (yet) assigned copyright to the FSF, but maybe the change
    is trivial enough that it's not necessary?


2008-04-16  Ludovic Courtès  <ludo@gnu.org>

	PR28319
        * c-common.c (c_common_attribute_table): Set max argument count
        to 2.
        (check_function_sentinel): Check SENTINEL against the
        user-provided value rather than against zero.
        (handle_sentinel_attribute): Validate second optional argument.
        * doc/extend.texi (Function Attributes): Document `sentinel'
        change.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: The patch --]
[-- Type: text/x-patch, Size: 3103 bytes --]

diff --git a/gcc/c-common.c b/gcc/c-common.c
index 2bc7434..14eb688 100644
--- a/gcc/c-common.c
+++ b/gcc/c-common.c
@@ -647,7 +647,7 @@ const struct attribute_spec c_common_attribute_table[] =
 			      handle_cleanup_attribute },
   { "warn_unused_result",     0, 0, false, true, true,
 			      handle_warn_unused_result_attribute },
-  { "sentinel",               0, 1, false, true, true,
+  { "sentinel",               0, 2, false, true, true,
 			      handle_sentinel_attribute },
   /* For internal use (marking of builtins) only.  The name contains space
      to prevent its usage in source code.  */
@@ -6214,7 +6214,7 @@ check_function_sentinel (tree attrs, int nargs, tree *argarray, tree typelist)
     {
       int len = 0;
       int pos = 0;
-      tree sentinel;
+      tree value, sentinel;
 
       /* Skip over the named arguments.  */
       while (typelist && len < nargs)
@@ -6227,6 +6227,14 @@ check_function_sentinel (tree attrs, int nargs, tree *argarray, tree typelist)
 	{
 	  tree p = TREE_VALUE (TREE_VALUE (attr));
 	  pos = TREE_INT_CST_LOW (p);
+
+	  value = TREE_CHAIN (TREE_VALUE (attr));
+	  if (value)
+	    /* Use the user-provided sentinel value.  */
+	    value = TREE_VALUE (value);
+	  else
+	    /* Default to zero.  */
+	    value = integer_zero_node;
 	}
 
       /* The sentinel must be one of the varargs, i.e.
@@ -6241,7 +6249,7 @@ check_function_sentinel (tree attrs, int nargs, tree *argarray, tree typelist)
       /* Validate the sentinel.  */
       sentinel = argarray[nargs - 1 - pos];
       if ((!POINTER_TYPE_P (TREE_TYPE (sentinel))
-	   || !integer_zerop (sentinel))
+	   || !tree_int_cst_equal (sentinel, value))
 	  /* Although __null (in C++) is only an integer we allow it
 	     nevertheless, as we are guaranteed that it's exactly
 	     as wide as a pointer, and we don't want to force
@@ -6434,6 +6442,21 @@ handle_sentinel_attribute (tree *node, tree name, tree args,
 		       "requested position is less than zero");
 	      *no_add_attrs = true;
 	    }
+
+	  args = TREE_CHAIN (args);
+	  if (args)
+	    {
+	      /* Check the user-provided sentinel value.  */
+	      tree value = TREE_VALUE (args);
+
+	      if (TREE_CODE (value) != INTEGER_CST)
+		{
+		  warning (OPT_Wattributes,
+			   "requested sentinel value is not "
+			   "an integer constant");
+		  *no_add_attrs = true;
+		}
+	    }
 	}
     }
 
diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi
index 5dfbbcf..f1f684b 100644
--- a/gcc/doc/extend.texi
+++ b/gcc/doc/extend.texi
@@ -2758,10 +2758,15 @@ last parameter of the function call.  If an optional integer position
 argument P is supplied to the attribute, the sentinel must be located at
 position P counting backwards from the end of the argument list.
 
+Optionally, a second integer constant can be passed that determines the
+expected sentinel value.
+
 @smallexample
 __attribute__ ((sentinel))
 is equivalent to
 __attribute__ ((sentinel(0)))
+and is equivalent to
+__attribute__ ((sentinel(0, 0)))
 @end smallexample
 
 The attribute is automatically set with a position of 0 for the built-in

Re: [PATCH] Extend the `sentinel' attribute

[Tom Tromey]

>>>>> "Ludovic" == Ludovic Courtès <ludo@gnu.org> writes:

Ludovic> The attached patch adds a second optional parameter to the
Ludovic> `sentinel' attribute, allowing the expected sentinel value to
Ludovic> be specified.  IOW, it allows `sentinel' to be used for
Ludovic> non-NULL values and addresses bug #28319:

The idea seems pretty reasonable to me.

Ludovic> In addition, it would be nice to no limit sentinel values to
Ludovic> pointer types but this would be an incompatible change.  What
Ludovic> do you think?

Is this a common pattern in real applications?

Ludovic> PS: I have not (yet) assigned copyright to the FSF, but maybe
Ludovic> the change is trivial enough that it's not necessary?

No, sorry, I think it is over the limit.

Ludovic>         * c-common.c (c_common_attribute_table): Set max argument count
Ludovic>         to 2.

You should mention "<sentinel>" before the colon here.

Ludovic>        if ((!POINTER_TYPE_P (TREE_TYPE (sentinel))
Ludovic> -	   || !integer_zerop (sentinel))
Ludovic> +	   || !tree_int_cst_equal (sentinel, value))

It looks to me like this will do the wrong thing if there is a
user-provided sentinel but -Wstrict-null-sentinel is passed.

I think this patch needs test cases as well, covering at least proper
operation of the new functionality, plus a test of the new error case
when the second argument is not a constant.

Tom

Re: [PATCH] Extend the `sentinel' attribute

[Ben Elliston]

> PS: I have not (yet) assigned copyright to the FSF, but maybe the change
>     is trivial enough that it's not necessary?

In my opinion, it's too large to be deemed trivial.

Cheers, Ben

Re: [PATCH] Extend the `sentinel' attribute

[Ludovic Courtès]

[-- Attachment #1: Type: text/plain, Size: 2322 bytes --]

Hi,

Tom Tromey <tromey@redhat.com> writes:

> Ludovic> In addition, it would be nice to no limit sentinel values to
> Ludovic> pointer types but this would be an incompatible change.  What
> Ludovic> do you think?
>
> Is this a common pattern in real applications?

Not sure.  In Guile, the `SCM' type can be defined to be either a union,
a pointer, or an integer, although in practice it's defined to be a
pointer type by default (see `SCM_DEBUG_TYPING_STRICTNESS' in
"libguile/tags.h").  I don't have any other example at hand.

Yet, it feels better to not limit ourselves to pointer types, as long as
we don't need to break anything to achieve this (see below).

> Ludovic> PS: I have not (yet) assigned copyright to the FSF, but maybe
> Ludovic> the change is trivial enough that it's not necessary?
>
> No, sorry, I think it is over the limit.

OK, will do.

> It looks to me like this will do the wrong thing if there is a
> user-provided sentinel but -Wstrict-null-sentinel is passed.

Indeed, I had overlooked this.

Here's an updated patch, with test cases.  The first difference with the
previous patch is that it should interact well with
`-Wstrict-null-sentinel'.  The second one is that it now checks whether
the "pointerness" of the sentinel matches that of the expected value.
This allows both regular integers and pointer types to be used as
sentinels.  Opinions?

(Speaking of which, I'm not completely satisfied with the wording in the
documentation patch.)

Thanks,
Ludovic.

gcc/ChangeLog
2008-04-17  Ludovic Courtès  <ludo@gnu.org>

	PR28319
        * c-common.c (c_common_attribute_table)[sentinel]: Set max
        argument count to 2.
        (check_function_sentinel): Check SENTINEL against the
        user-provided value rather than against zero.  Check 
        SENTINEL's "pointerness" against that of the user-specified
        value instead of always expecting a pointer type.
        (handle_sentinel_attribute): Validate second optional argument.
        * doc/extend.texi (Function Attributes): Document `sentinel'
        change.

gcc/testsuite/ChangeLog
2008-04-17  Ludovic Courtès  <ludo@gnu.org>

	PR28319
        * gcc.dg/format/sentinel-1.c (foo10): Add third `sentinel'
        argument.
        (foo11, foo12, foo13, foo14, foo15): New declarations.
        (bar): Use them.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: The patch --]
[-- Type: text/x-patch, Size: 8005 bytes --]

diff --git a/gcc/c-common.c b/gcc/c-common.c
index 2bc7434..343a514 100644
--- a/gcc/c-common.c
+++ b/gcc/c-common.c
@@ -647,7 +647,7 @@ const struct attribute_spec c_common_attribute_table[] =
 			      handle_cleanup_attribute },
   { "warn_unused_result",     0, 0, false, true, true,
 			      handle_warn_unused_result_attribute },
-  { "sentinel",               0, 1, false, true, true,
+  { "sentinel",               0, 2, false, true, true,
 			      handle_sentinel_attribute },
   /* For internal use (marking of builtins) only.  The name contains space
      to prevent its usage in source code.  */
@@ -6214,7 +6214,7 @@ check_function_sentinel (tree attrs, int nargs, tree *argarray, tree typelist)
     {
       int len = 0;
       int pos = 0;
-      tree sentinel;
+      tree sentinel, value = null_pointer_node;
 
       /* Skip over the named arguments.  */
       while (typelist && len < nargs)
@@ -6227,6 +6227,14 @@ check_function_sentinel (tree attrs, int nargs, tree *argarray, tree typelist)
 	{
 	  tree p = TREE_VALUE (TREE_VALUE (attr));
 	  pos = TREE_INT_CST_LOW (p);
+
+	  value = TREE_CHAIN (TREE_VALUE (attr));
+	  if (value)
+	    /* Use the user-provided sentinel value.  */
+	    value = TREE_VALUE (value);
+	  else
+	    /* Default to `NULL'.  */
+	    value = null_pointer_node;
 	}
 
       /* The sentinel must be one of the varargs, i.e.
@@ -6240,14 +6248,18 @@ check_function_sentinel (tree attrs, int nargs, tree *argarray, tree typelist)
 
       /* Validate the sentinel.  */
       sentinel = argarray[nargs - 1 - pos];
-      if ((!POINTER_TYPE_P (TREE_TYPE (sentinel))
-	   || !integer_zerop (sentinel))
-	  /* Although __null (in C++) is only an integer we allow it
-	     nevertheless, as we are guaranteed that it's exactly
-	     as wide as a pointer, and we don't want to force
-	     users to cast the NULL they have written there.
-	     We warn with -Wstrict-null-sentinel, though.  */
-	  && (warn_strict_null_sentinel || null_node != sentinel))
+      if ((((POINTER_TYPE_P (TREE_TYPE (sentinel))
+	     != POINTER_TYPE_P (TREE_TYPE (value)))
+	    /* Although __null (in C++) is only an integer we allow it
+	       nevertheless, as we are guaranteed that it's exactly
+	       as wide as a pointer, and we don't want to force
+	       users to cast the NULL they have written there.
+	       We warn with -Wstrict-null-sentinel, though.  */
+	    && ((value == null_pointer_node)
+		? (warn_strict_null_sentinel || null_node != sentinel)
+		: 1)))
+	  || !tree_int_cst_equal (sentinel, value))
+
 	warning (OPT_Wformat, "missing sentinel in function call");
     }
 }
@@ -6434,6 +6446,21 @@ handle_sentinel_attribute (tree *node, tree name, tree args,
 		       "requested position is less than zero");
 	      *no_add_attrs = true;
 	    }
+
+	  args = TREE_CHAIN (args);
+	  if (args)
+	    {
+	      /* Check the user-provided sentinel value.  */
+	      tree value = TREE_VALUE (args);
+
+	      if (TREE_CODE (value) != INTEGER_CST)
+		{
+		  warning (OPT_Wattributes,
+			   "requested sentinel value is not "
+			   "an integer constant");
+		  *no_add_attrs = true;
+		}
+	    }
 	}
     }
 
diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi
index 5dfbbcf..0a71952 100644
--- a/gcc/doc/extend.texi
+++ b/gcc/doc/extend.texi
@@ -2752,26 +2752,37 @@ section, consider using the facilities of the linker instead.
 @item sentinel
 @cindex @code{sentinel} function attribute
 This function attribute ensures that a parameter in a function call is
-an explicit @code{NULL}.  The attribute is only valid on variadic
-functions.  By default, the sentinel is located at position zero, the
-last parameter of the function call.  If an optional integer position
-argument P is supplied to the attribute, the sentinel must be located at
-position P counting backwards from the end of the argument list.
+an explicit @code{NULL} or some other user-specified integer constant.
+The attribute is only valid on variadic functions.  By default, the
+sentinel is located at position zero, the last parameter of the function
+call.  If an optional integer position argument P is supplied to the
+attribute, the sentinel must be located at position P counting backwards
+from the end of the argument list.
+
+Optionally, a second integer constant can be passed that determines the
+expected sentinel value.  When this integer constant has a pointer type,
+the sentinel is expected to have pointer type as well.
 
 @smallexample
 __attribute__ ((sentinel))
 is equivalent to
 __attribute__ ((sentinel(0)))
+and is equivalent to
+__attribute__ ((sentinel(0, (void *) 0)))
 @end smallexample
 
+A valid @code{NULL} in this context is defined as zero with any pointer
+type.  If your system defines the @code{NULL} macro with an integer type
+then you need to add an explicit cast.  GCC replaces @code{stddef.h}
+with a copy that redefines NULL appropriately.  An exception is the C++
+@code{__null} value, which, although it is an integer constant, is
+considered a valid sentinel when @code{(void *) 0} is expected
+(@pxref{C++ Dialect Options, @code{-Wstrict-null-sentinel} option}).
+
 The attribute is automatically set with a position of 0 for the built-in
 functions @code{execl} and @code{execlp}.  The built-in function
 @code{execle} has the attribute set with a position of 1.
 
-A valid @code{NULL} in this context is defined as zero with any pointer
-type.  If your system defines the @code{NULL} macro with an integer type
-then you need to add an explicit cast.  GCC replaces @code{stddef.h}
-with a copy that redefines NULL appropriately.
 
 The warnings for missing or incorrect sentinels are enabled with
 @option{-Wformat}.
diff --git a/gcc/testsuite/gcc.dg/format/sentinel-1.c b/gcc/testsuite/gcc.dg/format/sentinel-1.c
index 1dc0908..9cedf81 100644
--- a/gcc/testsuite/gcc.dg/format/sentinel-1.c
+++ b/gcc/testsuite/gcc.dg/format/sentinel-1.c
@@ -23,7 +23,13 @@ extern void foo6 (const char *, ...) __attribute__ ((__sentinel__(5)));
 extern void foo7 (const char *, ...) __attribute__ ((__sentinel__(0)));
 extern void foo8 (const char *, ...) __attribute__ ((__sentinel__("a"))); /* { dg-warning "not an integer constant" "sentinel" } */
 extern void foo9 (const char *, ...) __attribute__ ((__sentinel__(-1))); /* { dg-warning "less than zero" "sentinel" } */
-extern void foo10 (const char *, ...) __attribute__ ((__sentinel__(1,3))); /* { dg-error "wrong number of arguments" "sentinel" } */
+extern void foo10 (const char *, ...) __attribute__ ((__sentinel__(1,2,3))); /* { dg-error "wrong number of arguments" "sentinel" } */
+
+extern void foo11 (const char *, ...) __attribute__ ((__sentinel__(0,NULL)));
+extern void foo12 (const char *, ...) __attribute__ ((__sentinel__(0,0)));
+extern void foo13 (const char *, ...) __attribute__ ((__sentinel__(0,-77)));
+extern void foo14 (const char *, ...) __attribute__ ((__sentinel__(0,(void *) -1)));
+extern void foo15 (const char *, ...) __attribute__ ((__sentinel__(0,"a"))); /* { dg-warning "not an integer constant" "sentinel" } */
 
 extern void bar(void)
 {
@@ -59,6 +65,18 @@ extern void bar(void)
 
   foo7 ("a", 1, 2, 3, NULL);
 
+  foo11 ("a", 1, 2, 3, NULL);
+  foo11 ("a", 1); /* { dg-warning "missing sentinel" "sentinel" } */
+  foo11 ("a", 0); /* { dg-warning "missing sentinel" "sentinel" } */
+  foo12 ("a", 0);
+  foo12 ("a", 1); /* { dg-warning "missing sentinel" "sentinel" } */
+  foo12 ("a", (void *) 0); /* Unless `-Wstrict-null-sentinel' is passed,
+			      using a null pointer instead of zero is OK.  */
+  foo13 ("a", 1, 2, 3, 4, -77);
+  foo13 ("a", 1, 2); /* { dg-warning "missing sentinel" "sentinel" } */
+  foo14 ("a", 1, -1); /* { dg-warning "missing sentinel" "sentinel" } */
+  foo14 ("a", 1, (void *) -1);
+
   execl ("/bin/ls", "/bin/ls", "-aFC"); /* { dg-warning "missing sentinel" "sentinel" } */
   execl ("/bin/ls", "/bin/ls", "-aFC", 0); /* { dg-warning "missing sentinel" "sentinel" } */
   execl ("/bin/ls", "/bin/ls", "-aFC", NULL);

Re: [PATCH] Extend the `sentinel' attribute

[Ludovic Courtès]

Hello,

This is a reminder for the following patch:

  http://gcc.gnu.org/ml/gcc-patches/2008-04/msg01376.html

This patch falls under the INRIA copyright assignment, being myself an
INRIA employee.

Thanks,
Ludovic.