This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH PING²] Extend the `sentinel' attribute


Hi,

"Joseph S. Myers" <joseph@codesourcery.com> writes:

> If an integer constant is used as the sentinel value in the attribute, I 
> think you need to check that the value in the call has compatible type 
> (after removing any qualifiers), or at least the same width (so that 
> passing 1 in one place and 1LL in the other is diagnosed, for example).

Good point.  Here's an updated patch that takes this into account.

There's a problem, though.  The "foo12 ("a", 0);" at sentinel-1.c:79
doesn't pass.  Strangely enough, "POINTER_TYPE_P (value)" wrongfully
returns true for the "0" in there, which triggers the "missing sentinel"
warning; even more strangely, commenting the `foo11 ()' declaration or
moving it a couple of lines down fixes the bug.  Any idea?

> A -Wstrict-null-sentinel test of the case
>
> +  foo12 ("a", (void *) 0); /* Unless `-Wstrict-null-sentinel' is passed,
> +			      using a null pointer instead of zero is OK.  */
>
> would also be a good idea (and if the value 0 in the attribute was not the 
> same width as pointers, I don't think -Wstrict-null-sentinel should be 
> required for the warning).

Right.  However, it would be a C++ test, and I'm not sure whether/how it
can be run under `gcc.dg/format' (I tried adding "{ dg-option "-x c++" }"
but that doesn't work since "-x c++" appears too late on the command
line).  Any idea?

> What's the copyright assignment status?

It falls under the INRIA copyright assignment (I'm an INRIA employee).

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.

diff --git a/gcc/c-common.c b/gcc/c-common.c
index 2bc7434..4c3cfc5 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,28 @@ 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))
+
+	  /* SENTINEL and VALUE have an integral type but these are different
+	     types or types that don't have the same width.  */
+	  || (!POINTER_TYPE_P (TREE_TYPE (sentinel))
+	      && !POINTER_TYPE_P (TREE_TYPE (value))
+	      ? (TREE_CODE (value) != TREE_CODE (sentinel)
+		 || (!tree_int_cst_equal (TYPE_SIZE (TREE_TYPE (value)),
+					  TYPE_SIZE (TREE_TYPE (sentinel)))))
+	      : 0)
+
+	  || !tree_int_cst_equal (sentinel, value))
+
 	warning (OPT_Wformat, "missing sentinel in function call");
     }
 }
@@ -6434,6 +6456,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..be22ecd 100644
--- a/gcc/testsuite/gcc.dg/format/sentinel-1.c
+++ b/gcc/testsuite/gcc.dg/format/sentinel-1.c
@@ -10,6 +10,11 @@ extern int execlp (const char *, const char *, ...);
 extern int execle (const char *, const char *, ...);
 extern char *envp[];
 
+enum foo
+  {
+    the_enum = 0
+  };
+
 #define ATTR __attribute__ ((__sentinel__))
 
 extern int a ATTR; /* { dg-warning "applies to function types" "sentinel" } */
@@ -23,7 +28,15 @@ 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__(2,777LL)));
+extern void foo16 (const char *, ...) __attribute__ ((__sentinel__(1,the_enum)));
+extern void foo17 (const char *, ...) __attribute__ ((__sentinel__(0,"a"))); /* { dg-warning "not an integer constant" "sentinel" } */
 
 extern void bar(void)
 {
@@ -59,6 +72,25 @@ 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 (C++),
+			      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);
+
+  foo15 ("a", 2, 3, 777); /* { dg-warning "missing sentinel" "sentinel" } */
+  foo15 ("a", 2, 3, 777, 4, 5); /* { dg-warning "missing sentinel" "sentinel" } */
+  foo15 ("a", 2, 3, 777LL, 4, 5);
+  foo16 ("a", 0ULL, -1); /* { dg-warning "missing sentinel" "sentinel" } */
+  foo16 ("a", the_enum, -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);

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]