[patch] printf "%p" gdb internal error fix

[patch] printf "%p" gdb internal error fix

[Jan Kratochvil]

[-- Attachment #1: Type: text/plain, Size: 469 bytes --]

Hi,

(gdb) printf "%p\n", (void *) 7
internal error in printf_command

Before 6.5 and after patching the regression:
	(gdb) printf "%p\n", (void *) 7
	0x7

Bugreport by the courtesy of Akira TAGOH <tagoh(at)redhat.com>.

2006-09-10  Jan Kratochvil  <jan.kratochvil@redhat.com>
	* printcmd.c (printf_command): Handle forgotten "%p".

2006-09-10  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* gdb.base/display.exp: New test of `printf' "%p" formatting.



Regards,
Jan

[-- Attachment #2: gdb-printf-p.patch --]
[-- Type: text/plain, Size: 1376 bytes --]

Index: printcmd.c
===================================================================
RCS file: /cvs/src/src/gdb/printcmd.c,v
retrieving revision 1.98
diff -u -p -r1.98 printcmd.c
--- printcmd.c	17 Jul 2006 22:15:55 -0000	1.98
+++ printcmd.c	10 Sep 2006 17:14:48 -0000
@@ -2077,6 +2077,12 @@ printf_command (char *arg, int from_tty)
 	      printf_filtered (current_substring, val);
 	      break;
 	    }
+	  case ptr_arg:
+	    {
+	      void *val = (void *) value_as_long (val_args[i]);
+	      printf_filtered (current_substring, val);
+	      break;
+	    }
 	  default:
 	    internal_error (__FILE__, __LINE__,
 			    _("failed internal consitency check"));
Index: testsuite/gdb.base/display.exp
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.base/display.exp,v
retrieving revision 1.8
diff -u -p -r1.8 display.exp
--- testsuite/gdb.base/display.exp	10 Aug 2006 05:27:20 -0000	1.8
+++ testsuite/gdb.base/display.exp	10 Sep 2006 17:14:48 -0000
@@ -179,6 +179,8 @@ gdb_test "printf \"\\\\!\\a\\f\\r\\t\\v\
 gdb_test "printf \"\"" ".*" "re-set term"
 gdb_test "printf \"\\w\"" ".*Unrecognized escape character.*"
 gdb_test "printf \"%d\" j" ".*Invalid argument syntax.*"
+# 0 or hex vs. dec printing may be platform dependent:
+gdb_test "printf \"<%p>\\n\", (void *)7" ".*7>.*"
 
 # play with "print", too
 #

Re: [patch] printf "%p" gdb internal error fix

[Mark Kettenis]

> Date: Sun, 10 Sep 2006 19:20:37 +0200
> From: Jan Kratochvil <jan.kratochvil@redhat.com>
> 
> Hi,
> 
> (gdb) printf "%p\n", (void *) 7
> internal error in printf_command
> 
> Before 6.5 and after patching the regression:
> 	(gdb) printf "%p\n", (void *) 7
> 	0x7
> 
> Bugreport by the courtesy of Akira TAGOH <tagoh(at)redhat.com>.
> 
> 2006-09-10  Jan Kratochvil  <jan.kratochvil@redhat.com>
> 	* printcmd.c (printf_command): Handle forgotten "%p".
> 
> 2006-09-10  Jan Kratochvil  <jan.kratochvil@redhat.com>
> 
> 	* gdb.base/display.exp: New test of `printf' "%p" formatting.

Hmm, this will not do the right thing when you try to print a 64-bit
pointer on a 32-bit host.

Mark

Re: [patch] printf "%p" gdb internal error fix

[Daniel Jacobowitz]

On Sun, Sep 10, 2006 at 09:31:15PM +0200, Mark Kettenis wrote:
> Hmm, this will not do the right thing when you try to print a 64-bit
> pointer on a 32-bit host.

True, though, neither will the rest of the printf code:

          case long_arg:
            {
              long val = value_as_long (val_args[i]);
              printf_filtered (current_substring, val);
              break;
            }

None of it is written to use target types at present, only host types.
It'd need even more surgery if you wanted it to, since
current_substring gets passed to the host's printf.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [patch] printf "%p" gdb internal error fix

[Daniel Jacobowitz]

On Sun, Sep 10, 2006 at 09:31:15PM +0200, Mark Kettenis wrote:
> > Date: Sun, 10 Sep 2006 19:20:37 +0200
> > From: Jan Kratochvil <jan.kratochvil@redhat.com>
> > 
> > Hi,
> > 
> > (gdb) printf "%p\n", (void *) 7
> > internal error in printf_command

> Hmm, this will not do the right thing when you try to print a 64-bit
> pointer on a 32-bit host.

Better late than never...

Here is a patch for %p which does not rely on the size of a host
pointer.  Instead of using the underlying system's printf, it chooses
the same implementation-defined behavior chosen by glibc (%#x for
non-zero pointers and "(nil)" for null pointers).

I tested this on x86_64-linux and checked it in.

-- 
Daniel Jacobowitz
CodeSourcery

2007-09-04  Daniel Jacobowitz  <dan@codesourcery.com>

	* printcmd.c (printf_command): Handle ptr_arg.  Correct typo
	in internal error message.

2007-09-04  Daniel Jacobowitz  <dan@codesourcery.com>

	* gdb.base/display.exp: Add tests for printf %p.

Index: printcmd.c
===================================================================
RCS file: /cvs/src/src/gdb/printcmd.c,v
retrieving revision 1.109
diff -u -p -r1.109 printcmd.c
--- printcmd.c	23 Aug 2007 18:08:36 -0000	1.109
+++ printcmd.c	4 Sep 2007 13:59:44 -0000
@@ -2079,9 +2079,68 @@ printf_command (char *arg, int from_tty)
 	      printf_filtered (current_substring, val);
 	      break;
 	    }
+	  case ptr_arg:
+	    {
+	      /* We avoid the host's %p because pointers are too
+		 likely to be the wrong size.  The only interesting
+		 modifier for %p is a width; extract that, and then
+		 handle %p as glibc would: %#x or a literal "(nil)".  */
+
+	      char *p, *fmt, *fmt_p;
+#if defined (CC_HAS_LONG_LONG) && defined (PRINTF_HAS_LONG_LONG)
+	      long long val = value_as_long (val_args[i]);
+#else
+	      long val = value_as_long (val_args[i]);
+#endif
+
+	      fmt = alloca (strlen (current_substring) + 5);
+
+	      /* Copy up to the leading %.  */
+	      p = current_substring;
+	      fmt_p = fmt;
+	      while (*p)
+		{
+		  int is_percent = (*p == '%');
+		  *fmt_p++ = *p++;
+		  if (is_percent)
+		    {
+		      if (*p == '%')
+			*fmt_p++ = *p++;
+		      else
+			break;
+		    }
+		}
+
+	      if (val != 0)
+		*fmt_p++ = '#';
+
+	      /* Copy any width.  */
+	      while (*p >= '0' && *p < '9')
+		*fmt_p++ = *p++;
+
+	      gdb_assert (*p == 'p' && *(p + 1) == '\0');
+	      if (val != 0)
+		{
+#if defined (CC_HAS_LONG_LONG) && defined (PRINTF_HAS_LONG_LONG)
+		  *fmt_p++ = 'l';
+#endif
+		  *fmt_p++ = 'l';
+		  *fmt_p++ = 'x';
+		  *fmt_p++ = '\0';
+		  printf_filtered (fmt, val);
+		}
+	      else
+		{
+		  *fmt_p++ = 's';
+		  *fmt_p++ = '\0';
+		  printf_filtered (fmt, "(nil)");
+		}
+
+	      break;
+	    }
 	  default:
 	    internal_error (__FILE__, __LINE__,
-			    _("failed internal consitency check"));
+			    _("failed internal consistency check"));
 	  }
 	/* Skip to the next substring.  */
 	current_substring += strlen (current_substring) + 1;
Index: testsuite/gdb.base/display.exp
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.base/display.exp,v
retrieving revision 1.11
diff -u -p -r1.11 display.exp
--- testsuite/gdb.base/display.exp	23 Aug 2007 18:14:16 -0000	1.11
+++ testsuite/gdb.base/display.exp	4 Sep 2007 13:59:45 -0000
@@ -178,6 +178,8 @@ gdb_test "printf \"\\\\!\\a\\f\\r\\t\\v\
 gdb_test "printf \"\"" ".*" "re-set term"
 gdb_test "printf \"\\w\"" ".*Unrecognized escape character.*"
 gdb_test "printf \"%d\" j" ".*Invalid argument syntax.*"
+gdb_test "printf \"%p\\n\", 0" "\\(nil\\)"
+gdb_test "printf \"%p\\n\", 1" "0x1"
 
 # play with "print", too
 #

Re: [patch] printf "%p" gdb internal error fix

[Eli Zaretskii]

> Date: Tue, 4 Sep 2007 10:19:26 -0400
> From: Daniel Jacobowitz <drow@false.org>
> Cc: jan.kratochvil@redhat.com, gdb-patches@sourceware.org
> 
> On Sun, Sep 10, 2006 at 09:31:15PM +0200, Mark Kettenis wrote:
> > > Date: Sun, 10 Sep 2006 19:20:37 +0200
> > > From: Jan Kratochvil <jan.kratochvil@redhat.com>
> > > 
> > > Hi,
> > > 
> > > (gdb) printf "%p\n", (void *) 7
> > > internal error in printf_command
> 
> > Hmm, this will not do the right thing when you try to print a 64-bit
> > pointer on a 32-bit host.
> 
> Better late than never...
> 
> Here is a patch for %p which does not rely on the size of a host
> pointer.

Thanks, but what about a companion patch for the manual?

Re: [patch] printf "%p" gdb internal error fix

[Daniel Jacobowitz]

On Tue, Sep 04, 2007 at 11:43:14PM +0300, Eli Zaretskii wrote:
> Thanks, but what about a companion patch for the manual?

I don't believe that one is necessary; the manual says only "exactly
as if your program were to execute the C subroutine".  If there's
somewhere that should have more detail, please let me know.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [patch] printf "%p" gdb internal error fix

[Eli Zaretskii]

> Date: Tue, 4 Sep 2007 16:53:07 -0400
> From: Daniel Jacobowitz <drow@false.org>
> Cc: mark.kettenis@xs4all.nl, jan.kratochvil@redhat.com,
> 	gdb-patches@sourceware.org
> 
> On Tue, Sep 04, 2007 at 11:43:14PM +0300, Eli Zaretskii wrote:
> > Thanks, but what about a companion patch for the manual?
> 
> I don't believe that one is necessary; the manual says only "exactly
> as if your program were to execute the C subroutine".

Never liked such references too much, but I guess a full-fledged
description of conversion specifiers will have to wait for another
rainy day.  Until then, here's what I committed, in order to bring
the manual in line with the code, and at least tell what features of a
C-standard `printf' are _not_ supported:


2007-09-15  Eli Zaretskii  <eliz@gnu.org>

	* gdb.texinfo (Output): Spell out which features of C's printf are
	not supported by GDB's printf.


Index: gdb.texinfo
===================================================================
RCS file: /cvs/src/src/gdb/doc/gdb.texinfo,v
retrieving revision 1.429
diff -u -r1.429 gdb.texinfo
--- gdb.texinfo	5 Sep 2007 00:51:48 -0000	1.429
+++ gdb.texinfo	15 Sep 2007 08:53:16 -0000
@@ -16418,20 +16418,24 @@
 Formats}, for more information.
 
 @kindex printf
-@item printf @var{string}, @var{expressions}@dots{}
-Print the values of the @var{expressions} under the control of
-@var{string}.  The @var{expressions} are separated by commas and may be
-either numbers or pointers.  Their values are printed as specified by
-@var{string}, exactly as if your program were to execute the C
-subroutine
-@c FIXME: the above implies that at least all ANSI C formats are
-@c supported, but it isn't true: %E and %G don't work (or so it seems).
-@c Either this is a bug, or the manual should document what formats are
-@c supported.
-
-@smallexample
-printf (@var{string}, @var{expressions}@dots{});
-@end smallexample
+@item printf @var{template}, @var{expressions}@dots{}
+Print the values of one or more @var{expressions} under the control of
+the string @var{template}.  To print several values, make
+@var{expressions} be a comma-separated list of individual expressions,
+which may be either numbers or pointers.  Their values are printed as
+specified by @var{template}, exactly as a C program would do by
+executing the code below:
+
+@smallexample
+printf (@var{template}, @var{expressions}@dots{});
+@end smallexample
+
+As in @code{C} @code{printf}, ordinary characters in @var{template}
+are printed verbatim, while @dfn{conversion specification} introduced
+by the @samp{%} character cause subsequent @var{expressions} to be
+evaluated, their values converted and formatted according to type and
+style information encoded in the conversion specifications, and then
+printed.
 
 For example, you can print two values in hex like this:
 
@@ -16439,9 +16443,44 @@
 printf "foo, bar-foo = 0x%x, 0x%x\n", foo, bar-foo
 @end smallexample
 
-The only backslash-escape sequences that you can use in the format
-string are the simple ones that consist of backslash followed by a
-letter.
+@code{printf} supports all the standard @code{C} conversion
+specifications, including the flags and modifiers between the @samp{%}
+character and the conversion letter, with the following exceptions:
+
+@itemize @bullet
+@item
+The argument-ordering modifiers, such as @samp{2$}, are not supported.
+
+@item
+The modifier @samp{*} is not supported for specifying precision or
+width.
+
+@item
+The @samp{'} flag (for separation of digits into groups according to
+@code{LC_NUMERIC'}) is not supported.
+
+@item
+The type modifiers @samp{hh}, @samp{j}, @samp{t}, and @samp{z} are not
+supported.
+
+@item
+The conversion letter @samp{n} (as in @samp{%n}) is not supported.
+
+@item
+The conversion letters @samp{a} and @samp{A} are not supported.
+@end itemize
+
+@noindent
+Note that the @samp{ll} type modifier is supported only if the
+underlying @code{C} implementation used to build @value{GDBN} supports
+the @code{long long int} type, and the @samp{L} type modifier is
+supported only if @code{long double} type is available.
+
+As in @code{C}, @code{printf} supports simple backslash-escape
+sequences, such as @code{\n}, @samp{\t}, @samp{\\}, @samp{\"},
+@samp{\a}, and @samp{\f}, that consist of backslash followed by a
+single character.  Octal and hexadecimal escape sequences are not
+supported.
 @end table
 
 @node Interpreters

Re: [patch] printf "%p" gdb internal error fix

[Daniel Jacobowitz]

On Sat, Sep 15, 2007 at 11:59:52AM +0300, Eli Zaretskii wrote:
> Never liked such references too much, but I guess a full-fledged
> description of conversion specifiers will have to wait for another
> rainy day.  Until then, here's what I committed, in order to bring
> the manual in line with the code, and at least tell what features of a
> C-standard `printf' are _not_ supported:

Thanks!  May I suggest we reference a particular C or POSIX standard
if we are going to list exceptions?  When I wrote the current
implementation, I did it with a copy of C89 open next to me (well, I
think - I can't find the exact manual I was using) and GCC's format
string checker, which is a fairly definitive reference.

Of the exceptions you've found, * and n are present in C89.  1$,
', hh, j, t, z, a, and A are not (actually I can't find a reference
for a/A; what do they do?).

-- 
Daniel Jacobowitz
CodeSourcery

Re: [patch] printf "%p" gdb internal error fix

[Andreas Schwab]

Daniel Jacobowitz <drow@false.org> writes:

> Of the exceptions you've found, * and n are present in C89.  1$,
> ', hh, j, t, z, a, and A are not (actually I can't find a reference
> for a/A; what do they do?).

%a/%A is for printing floating point in hexadecimal (a C99 feature).

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: [patch] printf "%p" gdb internal error fix

[Eli Zaretskii]

> From: Andreas Schwab <schwab@suse.de>
> Cc: mark.kettenis@xs4all.nl, jan.kratochvil@redhat.com,
> 	gdb-patches@sourceware.org
> Date: Sat, 15 Sep 2007 16:54:51 +0200
> 
> Daniel Jacobowitz <drow@false.org> writes:
> 
> > Of the exceptions you've found, * and n are present in C89.  1$,
> > ', hh, j, t, z, a, and A are not (actually I can't find a reference
> > for a/A; what do they do?).
> 
> %a/%A is for printing floating point in hexadecimal (a C99 feature).

Yes, and the type modifiers Daniel mentioned above (hh, j, etc.) are
also from C99, AFAIR.

Re: [patch] printf "%p" gdb internal error fix

[Eli Zaretskii]

> Date: Sat, 15 Sep 2007 09:52:29 -0400
> From: Daniel Jacobowitz <drow@false.org>
> Cc: mark.kettenis@xs4all.nl, jan.kratochvil@redhat.com,
> 	gdb-patches@sourceware.org
> 
> May I suggest we reference a particular C or POSIX standard if we
> are going to list exceptions?

Fine with me, but I'm not aware of the C99 document that is freely
accessible on the net.  If you have a URL, by all means let's @uref
it.

> When I wrote the current
> implementation, I did it with a copy of C89 open next to me (well, I
> think - I can't find the exact manual I was using) and GCC's format
> string checker, which is a fairly definitive reference.

I looked at the glibc Info manual and a draft of C99.

Re: [patch] printf "%p" gdb internal error fix

[Daniel Jacobowitz]

On Sat, Sep 15, 2007 at 06:18:23PM +0300, Eli Zaretskii wrote:
> > Date: Sat, 15 Sep 2007 09:52:29 -0400
> > From: Daniel Jacobowitz <drow@false.org>
> > Cc: mark.kettenis@xs4all.nl, jan.kratochvil@redhat.com,
> > 	gdb-patches@sourceware.org
> > 
> > May I suggest we reference a particular C or POSIX standard if we
> > are going to list exceptions?
> 
> Fine with me, but I'm not aware of the C99 document that is freely
> accessible on the net.  If you have a URL, by all means let's @uref
> it.

I don't believe there is one, but this is a well-known and published
international standard.  Can't we reference the printed version?

What I was trying to say was that our printf is an implementation of
the C89 printf function, not the C99 or Single Unix printf; that's why
it has all the missing features you noticed - in fact it has none of
the C99 additions.  Can't we describe it as a mostly complete C89
printf instead of a mostly incomplete C99 printf?

-- 
Daniel Jacobowitz
CodeSourcery

Re: [patch] printf "%p" gdb internal error fix

[Eli Zaretskii]

> Date: Sat, 15 Sep 2007 12:12:20 -0400
> From: Daniel Jacobowitz <drow@false.org>
> Cc: gdb-patches@sourceware.org
> 
> > > May I suggest we reference a particular C or POSIX standard if we
> > > are going to list exceptions?
> > 
> > Fine with me, but I'm not aware of the C99 document that is freely
> > accessible on the net.  If you have a URL, by all means let's @uref
> > it.
> 
> I don't believe there is one, but this is a well-known and published
> international standard.  Can't we reference the printed version?

We can, if we have the exact title and other details.

> What I was trying to say was that our printf is an implementation of
> the C89 printf function, not the C99 or Single Unix printf; that's why
> it has all the missing features you noticed - in fact it has none of
> the C99 additions.

Well, we do have L and ll, so it's no longer true that we don't
support any of C99.  (Admittedly, they were implemented when they were
GNU extensions, but today's readers of the manual do not need to know
this piece of history.)

> Can't we describe it as a mostly complete C89 printf instead of a
> mostly incomplete C99 printf?

I don't think it's right to ask our readers to be familiar with the
history of the C standards.  That is why I didn't even mention C99 (or
any other standard).

Re: [patch] printf "%p" gdb internal error fix

[Daniel Jacobowitz]

On Sat, Sep 15, 2007 at 09:12:20PM +0300, Eli Zaretskii wrote:
> We can, if we have the exact title and other details.

All I know is:

  http://www.open-std.org/jtc1/sc22/wg14/www/standards

Sorry.

> > Can't we describe it as a mostly complete C89 printf instead of a
> > mostly incomplete C99 printf?
> 
> I don't think it's right to ask our readers to be familiar with the
> history of the C standards.  That is why I didn't even mention C99 (or
> any other standard).

I don't think it's right to have it both ways.  If we say it behaves
like the C "printf" function - which I personally think is the most
useful way to describe it, for readers' benefit - then it behooves us
to say which one.  There are probably a dozen or more variations.  I
can name five off the top of my head: C89, C99, SUSv2, Microsoft VC++
(%I64), C++ (which has wchar_t but not some other C99 features).

I don't feel terribly strongly about this, though, and you're
definitely the expert.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [patch] printf "%p" gdb internal error fix

[Eli Zaretskii]

> Date: Sat, 15 Sep 2007 14:40:02 -0400
> From: Daniel Jacobowitz <drow@false.org>
> Cc: gdb-patches@sourceware.org
> 
> > > Can't we describe it as a mostly complete C89 printf instead of a
> > > mostly incomplete C99 printf?
> > 
> > I don't think it's right to ask our readers to be familiar with the
> > history of the C standards.  That is why I didn't even mention C99 (or
> > any other standard).
> 
> I don't think it's right to have it both ways.

Yes, there's no single way that is right here.  I've chosen to talk
vaguely about a "C standard" because saying something precise would be
very hard without going into a lot of detail.  And I used C99
(implicitly) as the baseline because I believe more and more C
programmers regard it as _the_ standard as time passes.

Re: [patch] printf "%p" gdb internal error fix

[Daniel Jacobowitz]

On Sun, Sep 16, 2007 at 12:59:43AM +0300, Eli Zaretskii wrote:
> Yes, there's no single way that is right here.  I've chosen to talk
> vaguely about a "C standard" because saying something precise would be
> very hard without going into a lot of detail.  And I used C99
> (implicitly) as the baseline because I believe more and more C
> programmers regard it as _the_ standard as time passes.

Thanks, makes sense to me now.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [patch] printf "%p" gdb internal error fix

[Joseph S. Myers]

On Sat, 15 Sep 2007, Eli Zaretskii wrote:

> > Date: Sat, 15 Sep 2007 09:52:29 -0400
> > From: Daniel Jacobowitz <drow@false.org>
> > Cc: mark.kettenis@xs4all.nl, jan.kratochvil@redhat.com,
> > 	gdb-patches@sourceware.org
> > 
> > May I suggest we reference a particular C or POSIX standard if we
> > are going to list exceptions?
> 
> Fine with me, but I'm not aware of the C99 document that is freely
> accessible on the net.  If you have a URL, by all means let's @uref
> it.

<http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1256.pdf> is C99 with 
TC1, TC2 and TC3 incorporated into it and will be the base document for 
the C1x revision.  (TC3 - N1235 - is still going through the ISO 
publication process.)

-- 
Joseph S. Myers
joseph@codesourcery.com