RFC: Fix various problems with "printf" and warnings

RFC: Fix various problems with "printf" and warnings

[Daniel Jacobowitz]

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) \

Re: RFC: Fix various problems with "printf" and warnings

[Andreas Schwab]

Daniel Jacobowitz <drow@false.org> writes:

> +	    case 'o':
> +	    case 'x':
> +	    case 'X':
> +	      if (seen_space || seen_plus)
> +		bad = 1;

I don't think this should be rejected.  I've got only the C99 spec, but I
don't think that C89 is different here in that space and plus are ignored
for unsigned and non-numeric conversions.

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
PGP key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: RFC: Fix various problems with "printf" and warnings

[Daniel Jacobowitz]

On Sun, Jan 22, 2006 at 02:22:09AM +0100, Andreas Schwab wrote:
> Daniel Jacobowitz <drow@false.org> writes:
> 
> > +	    case 'o':
> > +	    case 'x':
> > +	    case 'X':
> > +	      if (seen_space || seen_plus)
> > +		bad = 1;
> 
> I don't think this should be rejected.  I've got only the C99 spec, but I
> don't think that C89 is different here in that space and plus are ignored
> for unsigned and non-numeric conversions.

That's what I figured too, but on the other hand, GCC warns about them:

drow@nevyn:~% gcc -Wformat -o a a.c
a.c: In function 'main':
a.c:7: warning: ' ' flag used with '%o' printf format

-- 
Daniel Jacobowitz
CodeSourcery

Re: RFC: Fix various problems with "printf" and warnings

[Eli Zaretskii]

> Date: Sat, 21 Jan 2006 19:31:17 -0500
> From: Daniel Jacobowitz <drow@false.org>
> 
> +	      else if (*f == ' ')
> +		seen_space = 1;
> +	      else if (*f == ' ')
> +		seen_plus = 1;

A typo in the second "else if" clause.

> +	  case long_double_arg:
> +#ifdef HAVE_LONG_DOUBLE
> +	    {
> +	      long double val = value_as_double (val_args[i]);
> +	      printf_filtered (current_substring, val);

Won't `value_as_double' lose accuracy here, possibly catastrophically
(i.e., a non-zero number coming out as zero, etc.)?

Re: RFC: Fix various problems with "printf" and warnings

[Daniel Jacobowitz]

On Sun, Jan 22, 2006 at 06:28:05AM +0200, Eli Zaretskii wrote:
> > Date: Sat, 21 Jan 2006 19:31:17 -0500
> > From: Daniel Jacobowitz <drow@false.org>
> > 
> > +	      else if (*f == ' ')
> > +		seen_space = 1;
> > +	      else if (*f == ' ')
> > +		seen_plus = 1;
> 
> A typo in the second "else if" clause.

Oops.  Thank you.

> > +	  case long_double_arg:
> > +#ifdef HAVE_LONG_DOUBLE
> > +	    {
> > +	      long double val = value_as_double (val_args[i]);
> > +	      printf_filtered (current_substring, val);
> 
> Won't `value_as_double' lose accuracy here, possibly catastrophically
> (i.e., a non-zero number coming out as zero, etc.)?

Not usually.  value_as_double is somewhat misnamed; it returns a
DOUBLEST, which will be long double if the host supports long double.
Now, if the target has a long double with a greater precision (or just
different precision, due to different format) than the host long
double, this will lose out a bit.  But this isn't the only place in GDB
that will lose in that case.

-- 
Daniel Jacobowitz
CodeSourcery

Re: RFC: Fix various problems with "printf" and warnings

[Jim Blandy]

Awesome.  That doesn't look like a terribly entertaining bit of work.  :)

Should we be using sed s/...//g?

I guess this patch shouldn't address such issues, but those Makefile
variable names are pretty wild.  The 'CFLAGS' lexeme sometimes appears
in the middle, sometimes at the end; we use 'INTERNAL' and 'GDB' to
mean essentially the same thing; and it seems sometimes we just drop
that altogether.

Re: RFC: Fix various problems with "printf" and warnings

[Daniel Jacobowitz]

On Sat, Jan 21, 2006 at 10:38:53PM -0800, Jim Blandy wrote:
> Should we be using sed s/...//g?

Might as well, thanks.  Also fixed up some quoting:
+GDB_WARN_CFLAGS_NO_FORMAT = `echo " $(GDB_WARN_CFLAGS) " | sed "s/ -Wformat-nonliteral / /g"`


-- 
Daniel Jacobowitz
CodeSourcery

Re: RFC: Fix various problems with "printf" and warnings

[Eli Zaretskii]

> Date: Sat, 21 Jan 2006 23:56:17 -0500
> From: Daniel Jacobowitz <drow@false.org>
> Cc: gdb-patches@sourceware.org
> 
> > > +	  case long_double_arg:
> > > +#ifdef HAVE_LONG_DOUBLE
> > > +	    {
> > > +	      long double val = value_as_double (val_args[i]);
> > > +	      printf_filtered (current_substring, val);
> > 
> > Won't `value_as_double' lose accuracy here, possibly catastrophically
> > (i.e., a non-zero number coming out as zero, etc.)?
> 
> Not usually.  value_as_double is somewhat misnamed; it returns a
> DOUBLEST, which will be long double if the host supports long double.

Ah, okay, thanks, I forgot about that.

Perhaps we should rename that function some day.  It is IMHO a bad
mantra for a function name to lie about what it does.
`value_as_doublest' sounds like a better name.

Re: RFC: Fix various problems with "printf" and warnings

[Daniel Jacobowitz]

On Sat, Jan 21, 2006 at 07:31:17PM -0500, Daniel Jacobowitz wrote:
> 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.

Thanks to everyone who commented; I've now checked this in.  A lot of
other targets aren't, but x86_64-linux native is now warning-free.


-- 
Daniel Jacobowitz
CodeSourcery