This is the mail archive of the
gcc@gcc.gnu.org
mailing list for the GCC project.
Re: Warning for unadorned 0 in varargs lists?
On Mon, Aug 23, 2004 at 12:36:34PM +0200, Paolo Bonzini wrote:
> >I just haven't had time to port it over to gcc-current, as the
> >implementation we use is for gcc 2.95 or gcc 3.3...
>
> You can post the diff against gcc 3.3, it is better than nothing.
>
Correction, it's a bit worse than I thought. I have it for 2.95, and
I didn't find the time to write more than a stub for gcc 3.3...
Anyways, here's the gist of the code... There's also some syslog format
stuff, but I'm sure you'll be able to separate them.
Judging by the huge thread this is generating, I'm seeing, as usual,
lots of discussion about whether this is worth it, or not.
I can assure you that it is: sentinel has caught myriads of misuses of 0
in exec(), which were tending to crash 64 bits architectures in no
uncertain way...
However you decide to deal with sentinel, in the end, just make it so
that adorning a function with __attribute__((sentinel)) will just
catch the most common idiom: that of a list that should end with a
null pointer.
You can discuss fancy extensions all you want (for instance, make
it possible for sentinel to take arguments), or extra checks all you
want...
The basic functionality is just to walk the arguments lists, and if it
ends with a constant, check that it is indeed a constant pointer
(e.g., C++'s NULL, or a bona-fide null pointer).
Otherwise, you can probably warn: if it ends with non-zero, always warn.
If it ends with an unadorned 0, at least warn in pedantic mode, or on
64 bits architecture.
I'm not too optimistic about this. I predict we'll see an extra 50 messages
in this thread, with more and more esoteric cases being considered, no
conclusion being reached, no consensus being achieved, and any
functionality like sentinel provides staying on the wayside for an extra
three years.
Index: c-common.c
===================================================================
RCS file: /spare/mirrors/openbsd/cvs/src/gnu/egcs/gcc/c-common.c,v
retrieving revision 1.2
retrieving revision 1.5
diff -u -p -r1.2 -r1.5
--- c-common.c 19 Jul 2001 13:59:24 -0000 1.2
+++ c-common.c 7 Jan 2003 18:07:33 -0000 1.5
@@ -45,6 +45,8 @@ static enum cpp_token cpp_token;
#endif
#endif
+tree null_node;
+
extern struct obstack permanent_obstack;
/* Nonzero means the expression being parsed will never be evaluated.
@@ -54,10 +56,11 @@ int skip_evaluation;
enum attrs {A_PACKED, A_NOCOMMON, A_COMMON, A_NORETURN, A_CONST, A_T_UNION,
A_NO_CHECK_MEMORY_USAGE, A_NO_INSTRUMENT_FUNCTION,
A_CONSTRUCTOR, A_DESTRUCTOR, A_MODE, A_SECTION, A_ALIGNED,
- A_UNUSED, A_FORMAT, A_FORMAT_ARG, A_WEAK, A_ALIAS, A_NONNULL};
+ A_UNUSED, A_FORMAT, A_FORMAT_ARG, A_WEAK, A_ALIAS, A_NONNULL,
+ A_SENTINEL };
enum format_type { printf_format_type, scanf_format_type,
- strftime_format_type };
+ strftime_format_type, syslog_format_type };
static void declare_hidden_char_array PROTO((const char *, const char *));
static void add_attribute PROTO((enum attrs, const char *,
@@ -389,6 +392,7 @@ init_attributes ()
add_attribute (A_FORMAT, "format", 3, 3, 1);
add_attribute (A_FORMAT_ARG, "format_arg", 1, 1, 1);
add_attribute (A_NONNULL, "nonnull", 0, -1, 1);
+ add_attribute (A_SENTINEL, "sentinel", 0, 0, 1);
add_attribute (A_WEAK, "weak", 0, 0, 1);
add_attribute (A_ALIAS, "alias", 1, 1, 1);
add_attribute (A_NO_INSTRUMENT_FUNCTION, "no_instrument_function", 0, 0, 1);
@@ -433,6 +437,7 @@ static function_attribute_info *new_func
PROTO((enum format_type, int, int));
static function_attribute_info *new_international_format PROTO((int));
static function_attribute_info *new_nonnull_info PROTO((int));
+static function_attribute_info *new_sentinel_info PROTO((int));
static function_attributes_info *insert_function_attribute
PROTO((function_attributes_info *, tree, tree, function_attribute_info *));
@@ -763,6 +768,8 @@ decl_attributes (node, attributes, prefi
if (!strcmp (p, "printf") || !strcmp (p, "__printf__"))
format_type = printf_format_type;
+ else if (!strcmp (p, "syslog") || !strcmp (p, "__syslog__"))
+ format_type = syslog_format_type;
else if (!strcmp (p, "scanf") || !strcmp (p, "__scanf__"))
format_type = scanf_format_type;
else if (!strcmp (p, "strftime")
@@ -989,6 +996,42 @@ decl_attributes (node, attributes, prefi
}
break;
+ case A_SENTINEL:
+ if (TREE_CODE (decl) != FUNCTION_DECL)
+ {
+ error_with_decl (decl,
+ "sentinel attribute specified for non-function `%s'");
+ continue;
+ }
+
+ if (args)
+ {
+ error_with_decl (decl,
+ "sentinel attribute does not take arguments");
+ continue;
+ }
+ /* No argument number: assume a full prototype, find ellipsis
+ location. */
+ if (!args)
+ {
+ tree argument;
+ int arg_num;
+
+ arg_num = 0;
+ argument = TYPE_ARG_TYPES (type);
+ if (!argument)
+ {
+ error_with_decl (decl,
+ "sentinel attribute without arguments on a non-prototype `%s'");
+ continue;
+ }
+ while (argument)
+ arg_num++, argument = TREE_CHAIN (argument);
+ list = insert_function_attribute (list, DECL_NAME (decl),
+ DECL_ASSEMBLER_NAME (decl), new_sentinel_info (arg_num));
+ }
+ break;
+
case A_WEAK:
declare_weak (decl);
break;
@@ -1281,11 +1324,19 @@ typedef struct nonnull_info
int argument_num; /* number of non-null argument */
} nonnull_info;
+typedef struct sentinel_info
+{
+ struct function_attribute_info *next;
+ enum attrs type;
+ int argument_num; /* number of non-null argument */
+} sentinel_info;
+
static function_attribute_info *find_function_attribute
PROTO((tree, tree, enum attrs));
static void check_format_info PROTO((function_format_info *, tree));
static void check_nonnull_info PROTO((nonnull_info *, tree));
+static void check_sentinel_info PROTO((sentinel_info *, tree));
/* Helper function for setting up initial attribute for printf-like
functions, since the format argument is also non-null checked for
@@ -1399,6 +1450,23 @@ new_nonnull_info (argument_num)
return (function_attribute_info *) (info);
}
+/* Create information record for functions with sentinel parameters
+ ARGUMENT_NUM is the number of the argument to check. */
+
+static function_attribute_info *
+new_sentinel_info (argument_num)
+ int argument_num;
+{
+ sentinel_info *info;
+
+ info = (sentinel_info *)
+ xmalloc (sizeof (nonnull_info));
+ info->next = NULL;
+ info->type = A_SENTINEL;
+ info->argument_num = argument_num;
+ return (function_attribute_info *) (info);
+}
+
/* Record attribute information for the names of function. Used as:
* newlist = insert_function_attribute (old, name, asm, new_xxx (...) );
* In reality, newlist == old if old != NULL, but clients don't need to
@@ -1490,6 +1558,9 @@ check_function_format (name, assembler_n
case A_NONNULL:
check_nonnull_info ((nonnull_info *)ck, params);
break;
+ case A_SENTINEL:
+ check_sentinel_info ((sentinel_info *)ck, params);
+ break;
}
}
break;
@@ -1606,6 +1677,53 @@ check_nonnull_info (info, params)
}
}
+/* Check the argument list for sentinel values.
+ INFO points to the sentinel_info structure.
+ PARAMS is the list of argument values. */
+
+static void
+check_sentinel_info (info, params)
+ sentinel_info *info;
+ tree params;
+{
+ tree arg;
+ int arg_num;
+ int found_zero = 0;
+
+ /* Skip to first checked argument. If the argument isn't available, there's
+ no work for us to do; prototype checking will catch the problem. */
+ for (arg_num = 1; ; ++arg_num)
+ {
+ if (params == 0)
+ return;
+ if (arg_num == info->argument_num)
+ break;
+ params = TREE_CHAIN (params);
+ }
+
+ while (params != 0)
+ {
+ arg = TREE_VALUE (params);
+ if (arg == 0)
+ break;
+ while (TREE_CODE (arg) == NOP_EXPR)
+ arg = TREE_OPERAND (arg, 0); /* strip coercion */
+ /* Special C++ check */
+ if (arg == null_node)
+ return;
+ if (POINTER_TYPE_P (TREE_TYPE (arg)) && integer_zerop (arg))
+ return;
+ if (integer_zerop (arg))
+ found_zero = 1;
+ params = TREE_CHAIN (params);
+ }
+
+
+ warning("couldn't find null pointer sentinel value starting at %d", arg_num);
+ if (found_zero)
+ warning("(integer 0 is not a null pointer in varargs context)");
+}
+
/* Check the argument list of a call to printf, scanf, etc.
INFO points to the function_format_info structure.
PARAMS is the list of argument values. */
@@ -1745,7 +1863,8 @@ check_format_info (info, params)
}
}
}
- else if (info->format_type == printf_format_type)
+ else if ((info->format_type == printf_format_type) ||
+ (info->format_type == syslog_format_type))
{
/* See if we have a number followed by a dollar sign. If we do,
it is an operand number, so set PARAMS to that operand. */
@@ -1924,7 +2043,8 @@ check_format_info (info, params)
}
/* The m, C, and S formats are GNU extensions. */
if (pedantic && info->format_type != strftime_format_type
- && (format_char == 'm' || format_char == 'C' || format_char == 'S'))
+ && ((format_char == 'm' && info->format_type != syslog_format_type)
+ || format_char == 'C' || format_char == 'S'))
warning ("ANSI C does not support the `%c' format", format_char);
/* ??? The a and A formats are C9X extensions, and should be allowed
when a C9X option is added. */
@@ -1935,6 +2055,7 @@ check_format_info (info, params)
switch (info->format_type)
{
case printf_format_type:
+ case syslog_format_type:
fci = print_char_table;
break;
case scanf_format_type:
@@ -2078,7 +2199,8 @@ check_format_info (info, params)
/* See if this is an attempt to write into a const type with
scanf or with printf "%n". */
if ((info->format_type == scanf_format_type
- || (info->format_type == printf_format_type
+ || ((info->format_type == printf_format_type ||
+ info->format_type == syslog_format_type)
&& format_char == 'n'))
&& i == fci->pointer_count + aflag
&& wanted_type != 0