Patch: Problem with type safety and the "sentinel" attribute
Stefan Westerfeld
stefan@space.twc.de
Thu Jun 15 13:30:00 GMT 2006
Hi!
There has been some discussion with the same subject on the gcc list,
but I am moving this to gcc-patches now, since I've got a patch now,
which you'll find at the end of the mail.
On Tue, Jun 13, 2006 at 09:18:06PM +0200, Stefan Westerfeld wrote:
> On Fri, Jun 09, 2006 at 07:30:25PM +0200, Tim Janik wrote:
> > On Fri, 9 Jun 2006, Kaveh R. Ghazi wrote:
> > >> void print_string_array (const char *array_name,
> > >> const char *string, ...) __attribute__
> > >> ((__sentinel__));
> > >>
> > >> print_string_array ("empty_array", NULL); /* gcc warns, but shouldn't
> > >*/
> > >>
> > >> The only way out for keeping the sentinel attribute and avoiding the
> > >> warning is using
> > >>
> > >> static void print_string_array (const char *array_name, ...)
> > >> __attribute__ ((__sentinel__));
> > >>
> > >>[...]
> > >>
> > >> so if it gets changed, then gcc might need to support both
> > >> - NULL termination within the last named parameter allowed
> > >> - NULL termination only allowed within varargs parameters (like it is
> > >> now)
> > >
> > >I'm not against this enhancement, but you need to specify a syntax
> > >that allows the old behavior but also allows doing it your way.
> > >
> > >Hmm, perhaps we could check for attribute "nonnull" on the last named
> > >argument, if it exists then that can't be the sentinel, if it's not
> > >there then it does what you want. This is not completely backwards
> > >compatible because anyone wanting the existing behavior has to add the
> > >attribute nonnull. But there's precedent for this when attribute
> > >printf split out whether the format specifier could be null...
> > >
> > >We could also create a new attribute name for the new behavior. This
> > >would preserve backwards compatibility. I like this idea better.
> >
> > i agree here. as far as the majority of the GLib and Gtk+ APIs are
> > concerned,
> > we don't really need the flexibility of the sentinel attribute but rather
> > a compiler check on whether the last argument used in a function call
> > is NULL or 0 (regardless of whether it's the last named arg or already part
> > of the varargs list).
> > that's also why the actual sentinel wrapper in GLib looks like this:
> >
> > #define G_GNUC_NULL_TERMINATED __attribute__((__sentinel__))
> >
> > so, if i was to make a call on this issue, i'd either introduce
> > __attribute__((__null_terminated__)) with the described semantics,
> > or have __attribute__((__sentinel__(-1))) work essentially like
> > __attribute__((__sentinel__(0))) while also accepting 0 in the position
> > of the last named argument.
>
> I also like the backwards compatible way better than trying to somehow
> modifying the attribute; it may break something now, or - which would
> also be bad - it may make something that somebody will want to check
> with the sentinel attribute in some future program impossible.
>
> The only case that I ever needed was NULL termination, so I think
> implementing one of the two proposals Tim made would be sufficient.
> Personally I like __attribute__((__null_terminated__)) better, because
> a -1 sentinel may be less intuitive to read in the source code. So this
> would be my suggestion for a "syntax specification".
So the provided patch adds the null_terminated attribute as suggested,
and works as I would expect it to work.
There is only one thing which I haven't explicitely implemented: a
varargs function with this prototype:
extern void ifoo (const char *name,
int i, ...) __attribute__ ((null_terminated));
should not allow NULL termination within the "i" parameter (which has
the wrong type to be suitable for NULL termination):
ifoo ("warn", NULL); /* <- should warn */
ifoo ("do not warn", 0, NULL); /* <- should not warn */
Although I wrote no code to handle this case explicitely, gcc does emit
the right warning anyway. So essentially even this case works, but I
don't know why it does. But I admit I don't fully understand those parts
of the check which I simply copypasted from the sentinel code.
Anyway I tested it with this code, on an x86_64 linux machine:
#include <stdio.h> /* NULL */
extern void sfoo (const char *name,
const char *string, ...) __attribute__ ((null_terminated));
extern void ifoo (const char *name,
int i, ...) __attribute__ ((null_terminated));
extern void sbar (const char *name,
const char *string, ...) __attribute__ ((sentinel));
extern void ibar (const char *name, int i, ...) __attribute__ ((sentinel));
int
main()
{
/* warnings: */
sfoo ("warn", "1"); /* <- line 18 */
sfoo ("warn", "1", "2");
ifoo ("warn", NULL);
ifoo ("warn", 0);
ifoo ("warn", 0, "1");
sbar ("warn", NULL);
sbar ("warn", "1");
sbar ("warn", "1", "2");
ibar ("warn", NULL);
ibar ("warn", 0);
ibar ("warn", 0, "1"); /* <- line 28 */
/* no warnings: */
sfoo ("do not warn", NULL);
sfoo ("do not warn", "1", NULL);
sfoo ("do not warn", "1", "2", NULL);
ifoo ("do not warn", 0, NULL);
ifoo ("do not warn", 0, "1", NULL);
sbar ("no not warn", "1", NULL);
sbar ("no not warn", "1", "2", NULL);
ibar ("do not warn", 0, NULL);
ibar ("do not warn", 0, "1", NULL);
return 0;
}
And it produces (as expected) these warnings:
/usr/local/gcc-snapshot/bin/gcc -Wall -xc nt.c -c
nt.c: In function 'main':
nt.c:18: warning: argument list is not properly null terminated
nt.c:19: warning: argument list is not properly null terminated
nt.c:20: warning: passing argument 2 of 'ifoo' makes integer from pointer without a cast
nt.c:20: warning: argument list is not properly null terminated
nt.c:21: warning: argument list is not properly null terminated
nt.c:22: warning: argument list is not properly null terminated
nt.c:23: warning: not enough variable arguments to fit a sentinel
nt.c:24: warning: not enough variable arguments to fit a sentinel
nt.c:25: warning: missing sentinel in function call
nt.c:26: warning: passing argument 2 of 'ibar' makes integer from pointer without a cast
nt.c:26: warning: not enough variable arguments to fit a sentinel
nt.c:27: warning: not enough variable arguments to fit a sentinel
nt.c:28: warning: missing sentinel in function call
/usr/local/gcc-snapshot/bin/gcc -Wall -xc++ nt.c -c
nt.c: In function 'int main()':
nt.c:18: warning: argument list is not properly null terminated
nt.c:19: warning: argument list is not properly null terminated
nt.c:20: warning: argument list is not properly null terminated
nt.c:21: warning: argument list is not properly null terminated
nt.c:22: warning: argument list is not properly null terminated
nt.c:23: warning: not enough variable arguments to fit a sentinel
nt.c:24: warning: not enough variable arguments to fit a sentinel
nt.c:25: warning: missing sentinel in function call
nt.c:26: warning: not enough variable arguments to fit a sentinel
nt.c:27: warning: not enough variable arguments to fit a sentinel
nt.c:28: warning: missing sentinel in function call
Finally, here is the patch itself:
Index: gcc/doc/extend.texi
===================================================================
--- gcc/doc/extend.texi (revision 114677)
+++ gcc/doc/extend.texi (working copy)
@@ -1561,8 +1561,8 @@
attribute specification inside double parentheses. The following
attributes are currently defined for functions on all targets:
@code{noreturn}, @code{returns_twice}, @code{noinline}, @code{always_inline},
-@code{flatten}, @code{pure}, @code{const}, @code{nothrow}, @code{sentinel},
-@code{format}, @code{format_arg}, @code{no_instrument_function},
+@code{flatten}, @code{pure}, @code{const}, @code{nothrow}, @code{null_terminated},
+@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},
@code{alias}, @code{warn_unused_result}, @code{nonnull}
@@ -2155,6 +2155,42 @@
take function pointer arguments. The @code{nothrow} attribute is not
implemented in GCC versions earlier than 3.3.
+@item null_terminated
+@cindex @code{null_terminated} 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. It is very similar to the @code{sentinel} attribute. However
+it allows calls to the function without any variadic arguments, so that
+the @code{NULL} termination occurs within the last named parameter.
+
+@smallexample
+static void print_string_array (const char *array_name,
+ const char *string, ...) __attribute__ ((null_terminated));
+
+void
+foo()
+@{
+ /* invalid with null_terminated attribute and invalid with sentinel attribute */
+ print_string_array ("bad_array", "bad1", "bad2");
+
+ /* valid with null_terminated attribute, but invalid with sentinel attribute */
+ print_string_array ("empty_array", NULL);
+
+ /* valid with null_terminated attribute and valid with sentinel attribute */
+ print_string_array ("foo_array", "string1", "string2", "string3", NULL);
+@}
+@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.
+
+The warnings for missing or incorrect NULL termination are enabled with
+@option{-Wformat}.
+
+See also @code{sentinel} attribute.
+
@item pure
@cindex @code{pure} function attribute
Many functions have no effects except the return value and their
@@ -2284,6 +2320,8 @@
The warnings for missing or incorrect sentinels are enabled with
@option{-Wformat}.
+See also @code{null_terminated} attribute.
+
@item short_call
See long_call/short_call.
Index: gcc/doc/invoke.texi
===================================================================
--- gcc/doc/invoke.texi (revision 114677)
+++ gcc/doc/invoke.texi (working copy)
@@ -1834,6 +1834,8 @@
to @code{__null}. Although it is a null pointer constant not a null pointer,
it is guaranteed to of the same size as a pointer. But this use is
not portable across different compilers.
+This option affects both: functions marked with the sentinel attribute and
+functions marked with the null_terminated attribute.
@item -Wno-non-template-friend @r{(C++ only)}
@opindex Wno-non-template-friend
Index: gcc/ChangeLog
===================================================================
--- gcc/ChangeLog (revision 114677)
+++ gcc/ChangeLog (working copy)
@@ -1,3 +1,12 @@
+2006-06-15 Stefan Westerfeld <stefan@space.twc.de>
+
+ * c-common.c: Implemented null_terminated attribute, which is similar
+ but somewhat different than sentinel(0).
+ * doc/extend.texi: Add documentation for the null_terminated
+ attribute.
+ * doc/invoke.texi: Document that -Wstrict-null-sentinel also affects
+ the null_terminated attribute.
+
2006-06-15 Richard Guenther <rguenther@suse.de>
* tree-ssa-structalias.c (alias_get_name): Avoid creating
Index: gcc/c-common.c
===================================================================
--- gcc/c-common.c (revision 114677)
+++ gcc/c-common.c (working copy)
@@ -543,6 +543,7 @@
static tree handle_warn_unused_result_attribute (tree *, tree, tree, int,
bool *);
static tree handle_sentinel_attribute (tree *, tree, tree, int, bool *);
+static tree handle_null_terminated_attribute (tree *, tree, tree, int, bool *);
static void check_function_nonnull (tree, tree);
static void check_nonnull_arg (void *, tree, unsigned HOST_WIDE_INT);
@@ -635,6 +636,8 @@
handle_warn_unused_result_attribute },
{ "sentinel", 0, 1, false, true, true,
handle_sentinel_attribute },
+ { "null_terminated", 0, 0, false, true, true,
+ handle_null_terminated_attribute },
{ NULL, 0, 0, false, false, false, NULL }
};
@@ -5445,6 +5448,43 @@
}
}
+/* Check that the argument list of a function call is terminated by
+ a (pointer)0. */
+
+static void
+check_function_null_terminated (tree attrs, tree params, tree ARG_UNUSED (typelist))
+{
+ tree attr = lookup_attribute ("null_terminated", attrs);
+
+ if (attr)
+ {
+ tree last_param = params;
+
+ if (!last_param)
+ {
+ warning (OPT_Wformat, "argument list is not properly null terminated");
+ }
+ else
+ {
+ /* Advance until we find the last parameter. */
+ while (TREE_CHAIN (last_param))
+ last_param = TREE_CHAIN (last_param);
+
+ /* Validate that the last parameter is (pointer)0. */
+ if ((!POINTER_TYPE_P (TREE_TYPE (TREE_VALUE (last_param)))
+ || !integer_zerop (TREE_VALUE (last_param)))
+ /* 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 != TREE_VALUE (last_param)))
+ warning (OPT_Wformat, "argument list is not properly null terminated");
+ }
+ }
+}
+
/* 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. */
@@ -5631,6 +5671,34 @@
return NULL_TREE;
}
+/* Handle a "null_terminated" attribute. */
+static tree
+handle_null_terminated_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)
+ {
+ warning (OPT_Wattributes,
+ "%qE attribute requires prototypes with named arguments", name);
+ *no_add_attrs = true;
+ }
+ else
+ {
+ while (TREE_CHAIN (params))
+ params = TREE_CHAIN (params);
+
+ if (VOID_TYPE_P (TREE_VALUE (params)))
+ {
+ warning (OPT_Wattributes,
+ "%qE attribute only applies to variadic functions", name);
+ *no_add_attrs = true;
+ }
+ }
+ return NULL_TREE;
+}
+
/* Check for valid arguments being passed to a function. */
void
check_function_arguments (tree attrs, tree params, tree typelist)
@@ -5647,7 +5715,10 @@
check_function_format (attrs, params);
if (warn_format)
- check_function_sentinel (attrs, params, typelist);
+ {
+ check_function_sentinel (attrs, params, typelist);
+ check_function_null_terminated (attrs, params, typelist);
+ }
}
/* Generic argument checking recursion routine. PARAM is the argument to
Cu... Stefan
--
Stefan Westerfeld, Hamburg/Germany, http://space.twc.de/~stefan
More information about the Gcc-patches
mailing list