This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
RFC: Fix various problems with "printf" and warnings
- From: Daniel Jacobowitz <drow at false dot org>
- To: gdb-patches at sourceware dot org
- Date: Sat, 21 Jan 2006 19:31:17 -0500
- Subject: RFC: Fix various problems with "printf" and warnings
This patch solves the same general problem three different ways: the
annoying warnings while building printcmd.o and the underlying problem
they represent.
First, a Debian user pointed out that calling "nomem" because vasprintf
failed isn't a good choice. printf is allowed to fail for other
reasons, including bad format strings - and sometimes it does.
Second, the existing code passes a lot of bogus format strings on to
the system printf. If we're going to suppress the warnings about our
use of user-provided format strings we at least ought to sanity check
them better. So this patch overhauls printf_command to match ISO C89,
with the exception of long long support (autoconf enabled; I didn't
touch that bit).
Third, the right way to build this file is to remove
-Wformat-nonliteral now that we're convinced the warnings do not
represent a real problem.
Any comments on, or objections to, this patch?
--
Daniel Jacobowitz
CodeSourcery
2006-01-21 Daniel Jacobowitz <dan@codesourcery.com>
* printcmd.c (printf_command): Make format string checking
stricter. Add separate cases for long_arg, ptr_arg, and
long_double_arg.
* utils.c (xstrvprintf): Improve the error message issued
for a bad format string.
* Makefile.in (GDB_WARN_CFLAGS_NO_FORMAT, INTERNAL_CFLAGS_BASE):
New variables.
(gnu-v3-abi.o, monitor.o, procfs.o, linux-thread-db.o): Remove
$(NO_WERROR_CFLAGS).
(printcmd.o): Likewise. Use $(GDB_WARN_CFLAGS_NO_FORMAT) and
enable -Werror.
Index: printcmd.c
===================================================================
RCS file: /cvs/src/src/gdb/printcmd.c,v
retrieving revision 1.96
diff -u -p -r1.96 printcmd.c
--- printcmd.c 15 Jan 2006 19:50:03 -0000 1.96
+++ printcmd.c 22 Jan 2006 00:08:39 -0000
@@ -1836,13 +1836,13 @@ printf_command (char *arg, int from_tty)
enum argclass
{
- no_arg, int_arg, string_arg, double_arg, long_long_arg
+ int_arg, long_arg, long_long_arg, ptr_arg, string_arg,
+ double_arg, long_double_arg
};
enum argclass *argclass;
enum argclass this_argclass;
char *last_arg;
int nargs_wanted;
- int lcount;
int i;
argclass = (enum argclass *) alloca (strlen (s) * sizeof *argclass);
@@ -1852,23 +1852,136 @@ printf_command (char *arg, int from_tty)
while (*f)
if (*f++ == '%')
{
- lcount = 0;
- while (strchr ("0123456789.hlL-+ #", *f))
+ int seen_hash = 0, seen_zero = 0, lcount = 0, seen_prec = 0;
+ int seen_space = 0, seen_plus = 0;
+ int seen_big_l = 0, seen_h = 0;
+ int bad = 0;
+
+ /* Check the validity of the format specifier, and work
+ out what argument it expects. We only accept C89
+ format strings, with the exception of long long (which
+ we autoconf for). */
+
+ /* Skip over "%%". */
+ if (*f == '%')
+ {
+ f++;
+ continue;
+ }
+
+ /* The first part of a format specifier is a set of flag
+ characters. */
+ while (strchr ("0-+ #", *f))
+ {
+ if (*f == '#')
+ seen_hash = 1;
+ else if (*f == '0')
+ seen_zero = 1;
+ else if (*f == ' ')
+ seen_space = 1;
+ else if (*f == ' ')
+ seen_plus = 1;
+ f++;
+ }
+
+ /* The next part of a format specifier is a width. */
+ while (strchr ("0123456789", *f))
+ f++;
+
+ /* The next part of a format specifier is a precision. */
+ if (*f == '.')
+ {
+ seen_prec = 1;
+ f++;
+ while (strchr ("0123456789", *f))
+ f++;
+ }
+
+ /* The next part of a format specifier is a length modifier. */
+ if (*f == 'h')
{
- if (*f == 'l' || *f == 'L')
- lcount++;
+ seen_h = 1;
f++;
}
+ else if (*f == 'l')
+ {
+ f++;
+ lcount++;
+ if (*f == 'l')
+ {
+ f++;
+ lcount++;
+ }
+ }
+ else if (*f == 'L')
+ {
+ seen_big_l = 1;
+ f++;
+ }
+
switch (*f)
{
+ case 'u':
+ if (seen_hash)
+ bad = 1;
+ /* FALLTHROUGH */
+
+ case 'o':
+ case 'x':
+ case 'X':
+ if (seen_space || seen_plus)
+ bad = 1;
+ /* FALLTHROUGH */
+
+ case 'd':
+ case 'i':
+ if (lcount == 0)
+ this_argclass = int_arg;
+ else if (lcount == 1)
+ this_argclass = long_arg;
+ else
+ this_argclass = long_long_arg;
+
+ if (seen_big_l)
+ bad = 1;
+ break;
+
+ case 'c':
+ this_argclass = int_arg;
+ if (lcount || seen_h || seen_big_l)
+ bad = 1;
+ if (seen_prec || seen_zero || seen_space || seen_plus)
+ bad = 1;
+ break;
+
+ case 'p':
+ this_argclass = ptr_arg;
+ if (lcount || seen_h || seen_big_l)
+ bad = 1;
+ if (seen_prec || seen_zero || seen_space || seen_plus)
+ bad = 1;
+ break;
+
case 's':
this_argclass = string_arg;
+ if (lcount || seen_h || seen_big_l)
+ bad = 1;
+ if (seen_zero || seen_space || seen_plus)
+ bad = 1;
break;
case 'e':
case 'f':
case 'g':
- this_argclass = double_arg;
+ case 'E':
+ case 'G':
+ if (seen_big_l)
+ this_argclass = long_double_arg;
+ else
+ this_argclass = double_arg;
+
+ if (lcount || seen_h)
+ bad = 1;
break;
case '*':
@@ -1877,26 +1990,23 @@ printf_command (char *arg, int from_tty)
case 'n':
error (_("Format specifier `n' not supported in printf"));
- case '%':
- this_argclass = no_arg;
- break;
+ case '\0':
+ error (_("Incomplete format specifier at end of format string"));
default:
- if (lcount > 1)
- this_argclass = long_long_arg;
- else
- this_argclass = int_arg;
- break;
+ error (_("Unrecognized format specifier '%c' in printf"), *f);
}
+
+ if (bad)
+ error (_("Inappropriate modifiers to format specifier '%c' in printf"),
+ *f);
+
f++;
- if (this_argclass != no_arg)
- {
- strncpy (current_substring, last_arg, f - last_arg);
- current_substring += f - last_arg;
- *current_substring++ = '\0';
- last_arg = f;
- argclass[nargs_wanted++] = this_argclass;
- }
+ strncpy (current_substring, last_arg, f - last_arg);
+ current_substring += f - last_arg;
+ *current_substring++ = '\0';
+ last_arg = f;
+ argclass[nargs_wanted++] = this_argclass;
}
/* Now, parse all arguments and evaluate them.
@@ -1970,6 +2080,16 @@ printf_command (char *arg, int from_tty)
printf_filtered (current_substring, val);
break;
}
+ case long_double_arg:
+#ifdef HAVE_LONG_DOUBLE
+ {
+ long double val = value_as_double (val_args[i]);
+ printf_filtered (current_substring, val);
+ break;
+ }
+#else
+ error (_("long double not supported in printf"));
+#endif
case long_long_arg:
#if defined (CC_HAS_LONG_LONG) && defined (PRINTF_HAS_LONG_LONG)
{
@@ -1982,7 +2102,12 @@ printf_command (char *arg, int from_tty)
#endif
case int_arg:
{
- /* FIXME: there should be separate int_arg and long_arg. */
+ int val = value_as_long (val_args[i]);
+ printf_filtered (current_substring, val);
+ break;
+ }
+ case long_arg:
+ {
long val = value_as_long (val_args[i]);
printf_filtered (current_substring, val);
break;
Index: utils.c
===================================================================
RCS file: /cvs/src/src/gdb/utils.c,v
retrieving revision 1.163
diff -u -p -r1.163 utils.c
--- utils.c 17 Dec 2005 22:34:03 -0000 1.163
+++ utils.c 22 Jan 2006 00:09:48 -0000
@@ -1068,14 +1068,12 @@ xstrvprintf (const char *format, va_list
{
char *ret = NULL;
int status = vasprintf (&ret, format, ap);
- /* NULL is returned when there was a memory allocation problem. */
- if (ret == NULL)
- nomem (0);
- /* A negative status (the printed length) with a non-NULL buffer
- should never happen, but just to be sure. */
- if (status < 0)
- internal_error (__FILE__, __LINE__,
- _("vasprintf call failed (errno %d)"), errno);
+ /* NULL is returned when there was a memory allocation problem, or
+ any other error (for instance, a bad format string). A negative
+ status (the printed length) with a non-NULL buffer should never
+ happen, but just to be sure. */
+ if (ret == NULL || status < 0)
+ internal_error (__FILE__, __LINE__, _("vasprintf call failed"));
return ret;
}
Index: Makefile.in
===================================================================
RCS file: /cvs/src/src/gdb/Makefile.in,v
retrieving revision 1.777
diff -u -p -r1.777 Makefile.in
--- Makefile.in 21 Jan 2006 01:29:03 -0000 1.777
+++ Makefile.in 22 Jan 2006 00:22:53 -0000
@@ -132,6 +132,8 @@ WERROR_CFLAGS = @WERROR_CFLAGS@
GDB_WARN_CFLAGS = $(WARN_CFLAGS)
GDB_WERROR_CFLAGS = $(WERROR_CFLAGS)
+GDB_WARN_CFLAGS_NO_FORMAT = `echo $(GDB_WARN_CFLAGS) | sed s/-Wformat-nonliteral//`
+
# Where is the INTL library? Typically in ../intl.
INTL_DIR = ../intl
INTL = @INTLLIBS@
@@ -344,12 +346,12 @@ CFLAGS = @CFLAGS@
CXXFLAGS = -g -O
# INTERNAL_CFLAGS is the aggregate of all other *CFLAGS macros.
-INTERNAL_WARN_CFLAGS = \
+INTERNAL_CFLAGS_BASE = \
$(CFLAGS) $(GLOBAL_CFLAGS) $(PROFILE_CFLAGS) \
$(GDB_CFLAGS) $(OPCODES_CFLAGS) $(READLINE_CFLAGS) \
$(BFD_CFLAGS) $(INCLUDE_CFLAGS) \
- $(INTL_CFLAGS) $(ENABLE_CFLAGS) \
- $(GDB_WARN_CFLAGS)
+ $(INTL_CFLAGS) $(ENABLE_CFLAGS)
+INTERNAL_WARN_CFLAGS = $(INTERNAL_CFLAGS_BASE) $(GDB_WARN_CFLAGS)
INTERNAL_CFLAGS = $(INTERNAL_WARN_CFLAGS) $(GDB_WERROR_CFLAGS)
# LDFLAGS is specifically reserved for setting from the command line
@@ -1479,8 +1481,7 @@ main.o: main.c
# return types - "enum gnu_v3_dtor_kinds" vs "enum ctor_kinds" -
# conflict.
gnu-v3-abi.o: $(srcdir)/gnu-v3-abi.c
- $(CC) -c $(INTERNAL_WARN_CFLAGS) $(NO_WERROR_CFLAGS) \
- $(srcdir)/gnu-v3-abi.c
+ $(CC) -c $(INTERNAL_WARN_CFLAGS) $(srcdir)/gnu-v3-abi.c
# FIXME: cagney/2003-08-10: "monitor.c" gets -Wformat-nonliteral
# errors. It turns out that that is the least of monitor.c's
@@ -1489,25 +1490,23 @@ gnu-v3-abi.o: $(srcdir)/gnu-v3-abi.c
# definitly will not work. "monitor.c" needs to be rewritten so that
# it doesn't use format strings and instead uses callbacks.
monitor.o: $(srcdir)/monitor.c
- $(CC) -c $(INTERNAL_WARN_CFLAGS) $(NO_WERROR_CFLAGS) $(srcdir)/monitor.c
+ $(CC) -c $(INTERNAL_WARN_CFLAGS) $(srcdir)/monitor.c
-# FIXME: cagney/2003-08-10: Do not try to build "printcmd.c" with
-# -Wformat-nonliteral. It needs to be overhauled so that it doesn't
-# pass user input strings as the format parameter to host printf
-# function calls.
+# Do not try to build "printcmd.c" with -Wformat-nonliteral. It manually
+# checks format strings.
printcmd.o: $(srcdir)/printcmd.c
- $(CC) -c $(INTERNAL_WARN_CFLAGS) $(NO_WERROR_CFLAGS) $(srcdir)/printcmd.c
+ $(CC) -c $(INTERNAL_CFLAGS_BASE) $(GDB_WARN_CFLAGS_NO_FORMAT) \
+ $(GDB_WERROR_CFLAGS) $(srcdir)/printcmd.c
# FIXME: Procfs.o gets -Wformat errors because things like pid_t don't
# match output format strings.
procfs.o: $(srcdir)/procfs.c
- $(CC) -c $(INTERNAL_WARN_CFLAGS) $(NO_WERROR_CFLAGS) $(srcdir)/procfs.c
+ $(CC) -c $(INTERNAL_WARN_CFLAGS) $(srcdir)/procfs.c
# FIXME: Thread-db.o gets warnings because the definitions of the register
# sets are different from kernel to kernel.
linux-thread-db.o: $(srcdir)/linux-thread-db.c
- $(CC) -c $(INTERNAL_WARN_CFLAGS) $(NO_WERROR_CFLAGS) \
- $(srcdir)/linux-thread-db.c
+ $(CC) -c $(INTERNAL_WARN_CFLAGS) $(srcdir)/linux-thread-db.c
v850ice.o: $(srcdir)/v850ice.c
$(CC) -c $(INTERNAL_CFLAGS) $(IDE_CFLAGS) $(ITCL_CFLAGS) \