[RFA 2/3] Allow - in %p for printf

[RFA 2/3] Allow - in %p for printf

[Tom Tromey]

PR cli/19918 points out that a printf format like "%-5p" will cause a
gdb crash.  The bug is problem is that printf_pointer doesn't take the
"-" flag into account.

gdb/ChangeLog
2018-02-14  Tom Tromey  <tom@tromey.com>

	PR cli/19918:
	* printcmd.c (printf_pointer): Allow "-" in format.

gdb/testsuite/ChangeLog
2018-02-14  Tom Tromey  <tom@tromey.com>

	PR cli/19918:
	* gdb.base/printcmds.exp (test_printf): Add printf test using '-'
	flag.
---
 gdb/ChangeLog                        | 5 +++++
 gdb/printcmd.c                       | 5 +++--
 gdb/testsuite/ChangeLog              | 6 ++++++
 gdb/testsuite/gdb.base/printcmds.exp | 4 ++++
 4 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/gdb/printcmd.c b/gdb/printcmd.c
index 13b967f0a2..d2d13895f6 100644
--- a/gdb/printcmd.c
+++ b/gdb/printcmd.c
@@ -2399,8 +2399,9 @@ printf_pointer (struct ui_file *stream, const char *format,
   if (val != 0)
     *fmt_p++ = '#';
 
-  /* Copy any width.  */
-  while (*p >= '0' && *p < '9')
+  /* Copy any width or flags.  Only the "-" flag is needed -- see the
+     format_pieces constructor.  */
+  while (*p == '-' || (*p >= '0' && *p < '9'))
     *fmt_p++ = *p++;
 
   gdb_assert (*p == 'p' && *(p + 1) == '\0');
diff --git a/gdb/testsuite/gdb.base/printcmds.exp b/gdb/testsuite/gdb.base/printcmds.exp
index 9402d97ef3..56cedb908f 100644
--- a/gdb/testsuite/gdb.base/printcmds.exp
+++ b/gdb/testsuite/gdb.base/printcmds.exp
@@ -776,6 +776,10 @@ proc test_printf {} {
 	"" \
 	"create hibob command"
     gdb_test "hibob" "hi bob zzz.*y" "run hibob command"
+
+    # PR cli/19918.
+    gdb_test "printf \"%-16dq\\n\", 0" "0               q"
+    gdb_test "printf \"%-16pq\\n\", 0" "\\(nil\\)           q"
 }
 
 #Test printing DFP values with printf
-- 
2.13.6

[RFA 0/3] a few minor printf fixes

[Tom Tromey]

This series fixes a few papercuts in the "printf" command.

Regression tested by the buildbot.

Tom

[RFA 1/3] Add usage to printf command

[Tom Tromey]

This patch adds the "Usage:" text to the printf command's help text,
and tries to improve the text a tiny bit.

gdb/ChangeLog
2018-02-14  Tom Tromey  <tom@tromey.com>

	* printcmd.c (_initialize_printcmd): Add usage to printf.
---
 gdb/ChangeLog  | 4 ++++
 gdb/printcmd.c | 5 +++--
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/gdb/printcmd.c b/gdb/printcmd.c
index fc9d7e4dd9..13b967f0a2 100644
--- a/gdb/printcmd.c
+++ b/gdb/printcmd.c
@@ -2680,8 +2680,9 @@ No argument means cancel all automatic-display expressions.\n\
 Do \"info display\" to see current list of code numbers."), &deletelist);
 
   add_com ("printf", class_vars, printf_command, _("\
-printf \"printf format string\", arg1, arg2, arg3, ..., argn\n\
-This is useful for formatted output in user-defined commands."));
+Formatted printing, like the C \"printf\" function.\n\
+Usage: printf \"format string\", arg1, arg2, arg3, ..., argn\n\
+This supports most C printf format specifications, like %s, %d, etc."));
 
   add_com ("output", class_vars, output_command, _("\
 Like \"print\" but don't put in value history and don't print newline.\n\
-- 
2.13.6

[RFA 3/3] Special case NULL when using printf's %s format

[Tom Tromey]

This changes the printf command's %s and %ls formats to special-case
NULL, and print "(null)" for these.  This is PR cli/14977.  This
behavior seems a bit friendlier; I was undecided on whether other
invalid pointers should be handled specially somehow, so for the time
being I've left those out.

gdb/ChangeLog
2018-02-14  Tom Tromey  <tom@tromey.com>

	PR cli/14977:
	* printcmd.c (printf_c_string, printf_wide_c_string): Special case
	for NULL.

gdb/gdbserver/ChangeLog
2018-02-14  Tom Tromey  <tom@tromey.com>

	PR cli/14977:
	* ax.c (ax_printf): Special case for NULL.

gdb/testsuite/ChangeLog
2018-02-14  Tom Tromey  <tom@tromey.com>

	PR cli/14977:
	* gdb.base/printcmds.exp (test_printf): Add printf test of %s with
	a null pointer.
	* gdb.base/wchar.exp: Likewise.
---
 gdb/ChangeLog                        |  6 ++++++
 gdb/gdbserver/ChangeLog              |  5 +++++
 gdb/gdbserver/ax.c                   |  5 +++++
 gdb/printcmd.c                       | 10 ++++++++++
 gdb/testsuite/ChangeLog              |  7 +++++++
 gdb/testsuite/gdb.base/printcmds.exp |  3 +++
 gdb/testsuite/gdb.base/wchar.exp     |  3 +++
 7 files changed, 39 insertions(+)

diff --git a/gdb/gdbserver/ax.c b/gdb/gdbserver/ax.c
index b42ee54c84..c754383df8 100644
--- a/gdb/gdbserver/ax.c
+++ b/gdb/gdbserver/ax.c
@@ -847,6 +847,11 @@ ax_printf (CORE_ADDR fn, CORE_ADDR chan, const char *format,
 	    int j;
 
 	    tem = args[i];
+	    if (tem == 0)
+	      {
+		printf (current_substring, "(null)");
+		break;
+	      }
 
 	    /* This is a %s argument.  Find the length of the string.  */
 	    for (j = 0;; j++)
diff --git a/gdb/printcmd.c b/gdb/printcmd.c
index d2d13895f6..a399ed761b 100644
--- a/gdb/printcmd.c
+++ b/gdb/printcmd.c
@@ -2220,6 +2220,11 @@ printf_c_string (struct ui_file *stream, const char *format,
   int j;
 
   tem = value_as_address (value);
+  if (tem == 0)
+    {
+      fprintf_filtered (stream, format, "(null)");
+      return;
+    }
 
   /* This is a %s argument.  Find the length of the string.  */
   for (j = 0;; j++)
@@ -2260,6 +2265,11 @@ printf_wide_c_string (struct ui_file *stream, const char *format,
   gdb_byte *buf = (gdb_byte *) alloca (wcwidth);
 
   tem = value_as_address (value);
+  if (tem == 0)
+    {
+      fprintf_filtered (stream, format, "(null)");
+      return;
+    }
 
   /* This is a %s argument.  Find the length of the string.  */
   for (j = 0;; j += wcwidth)
diff --git a/gdb/testsuite/gdb.base/printcmds.exp b/gdb/testsuite/gdb.base/printcmds.exp
index 56cedb908f..635bd621fd 100644
--- a/gdb/testsuite/gdb.base/printcmds.exp
+++ b/gdb/testsuite/gdb.base/printcmds.exp
@@ -780,6 +780,9 @@ proc test_printf {} {
     # PR cli/19918.
     gdb_test "printf \"%-16dq\\n\", 0" "0               q"
     gdb_test "printf \"%-16pq\\n\", 0" "\\(nil\\)           q"
+
+    # PR cli/14977.
+    gdb_test "printf \"%s\\n\", 0" "\\(null\\)"
 }
 
 #Test printing DFP values with printf
diff --git a/gdb/testsuite/gdb.base/wchar.exp b/gdb/testsuite/gdb.base/wchar.exp
index 80b6513e22..a4d9bce7d0 100644
--- a/gdb/testsuite/gdb.base/wchar.exp
+++ b/gdb/testsuite/gdb.base/wchar.exp
@@ -69,3 +69,6 @@ gdb_test "print repeat" "= L\"A$cent$cent\"\.\.\." \
 
 gdb_test "print repeat_p" "= $hex L\"A$cent$cent\"\.\.\." \
     "print repeat_p (print elements 3)"
+
+# From PR cli/14977, but here because it requires wchar_t.
+gdb_test "printf \"%ls\\n\", 0" "\\(null\\)"
-- 
2.13.6

Re: [RFA 2/3] Allow - in %p for printf

[Alan Hayward]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 2244 bytes --]



> On 15 Feb 2018, at 20:50, Tom Tromey <tom@tromey.com> wrote:
> 
> PR cli/19918 points out that a printf format like "%-5p" will cause a
> gdb crash.  The bug is problem is that printf_pointer doesn't take the
> "-" flag into account.
> 
> gdb/ChangeLog
> 2018-02-14  Tom Tromey  <tom@tromey.com>
> 
> 	PR cli/19918:
> 	* printcmd.c (printf_pointer): Allow "-" in format.
> 
> gdb/testsuite/ChangeLog
> 2018-02-14  Tom Tromey  <tom@tromey.com>
> 
> 	PR cli/19918:
> 	* gdb.base/printcmds.exp (test_printf): Add printf test using '-'
> 	flag.
> ---
> gdb/ChangeLog                        | 5 +++++
> gdb/printcmd.c                       | 5 +++--
> gdb/testsuite/ChangeLog              | 6 ++++++
> gdb/testsuite/gdb.base/printcmds.exp | 4 ++++
> 4 files changed, 18 insertions(+), 2 deletions(-)
> 
> diff --git a/gdb/printcmd.c b/gdb/printcmd.c
> index 13b967f0a2..d2d13895f6 100644
> --- a/gdb/printcmd.c
> +++ b/gdb/printcmd.c
> @@ -2399,8 +2399,9 @@ printf_pointer (struct ui_file *stream, const char *format,
>   if (val != 0)
>     *fmt_p++ = '#';
> 
> -  /* Copy any width.  */
> -  while (*p >= '0' && *p < '9')
> +  /* Copy any width or flags.  Only the "-" flag is needed -- see the
> +     format_pieces constructor.  */

I found this comment a little confusing. How about something like:
Copy and width or flags. “-“ is the only valid flag for pointers — see the format_pieces constructor.

Otherwise looks good to me.

> +  while (*p == '-' || (*p >= '0' && *p < '9'))
>     *fmt_p++ = *p++;
> 
>   gdb_assert (*p == 'p' && *(p + 1) == '\0');
> diff --git a/gdb/testsuite/gdb.base/printcmds.exp b/gdb/testsuite/gdb.base/printcmds.exp
> index 9402d97ef3..56cedb908f 100644
> --- a/gdb/testsuite/gdb.base/printcmds.exp
> +++ b/gdb/testsuite/gdb.base/printcmds.exp
> @@ -776,6 +776,10 @@ proc test_printf {} {
> 	"" \
> 	"create hibob command"
>     gdb_test "hibob" "hi bob zzz.*y" "run hibob command"
> +
> +    # PR cli/19918.
> +    gdb_test "printf \"%-16dq\\n\", 0" "0               q"
> +    gdb_test "printf \"%-16pq\\n\", 0" "\\(nil\\)           q"
> }
> 
> #Test printing DFP values with printf
> -- 
> 2.13.6
> 

\x16º&Öéj×!zÊÞ¶êç׎x×Ib²Ö«r\x18\x1dn–­r\x17¬

Re: [RFA 3/3] Special case NULL when using printf's %s format

[Pedro Alves]

On 02/16/2018 12:08 PM, Pedro Alves wrote:
> On 02/15/2018 08:50 PM, Tom Tromey wrote:
>> This changes the printf command's %s and %ls formats to special-case
>> NULL, and print "(null)" for these.  This is PR cli/14977.  This
>> behavior seems a bit friendlier; I was undecided on whether other
>> invalid pointers should be handled specially somehow, so for the time
>> being I've left those out.
> 
> A question here is what to do on targets that actually map things at
> address zero.  IIRC, some ARM chips do that.  For such ports, I think you'd
> want to be able to read/print that memory.  I thought we had a gdbarch hook
> for that, but I'm not finding it right now.  Maybe I was thinking of
> has_section_at_zero in dwarf2read.c.

Then again, I don't think it's likely that programs map
strings at 0 (I think it's more like interrupt vectors), so for %s,
it's likely that nobody would miss printing 0 as a string.

So with that in mind, this is fine with me.

Thanks,
Pedro Alves

Re: [RFA 3/3] Special case NULL when using printf's %s format

[Tom Tromey]

Pedro> Then again, I don't think it's likely that programs map
Pedro> strings at 0 (I think it's more like interrupt vectors), so for %s,
Pedro> it's likely that nobody would miss printing 0 as a string.

Another option would be to do a try-catch and only print "(null)" on
failure, if the pointer is 0.

Tom

Re: [RFA 1/3] Add usage to printf command

[Tom Tromey]

>>>>> "Alan" == Alan Hayward <Alan.Hayward@arm.com> writes:

Alan> I like the addition of a “usage”. But this will be the only command to have it.

Plenty of commands have it.

Alan> Probably not in scope for this patch series, but it would be nice for consistency
Alan> for usage statements to be added to all the commands in this file.

I can drop this patch.

Tom

Re: [RFA 1/3] Add usage to printf command

[Simon Marchi]

On 2018-02-16 16:58, Tom Tromey wrote:
>>>>>> "Alan" == Alan Hayward <Alan.Hayward@arm.com> writes:
> 
> Alan> I like the addition of a “usage”. But this will be the only
> command to have it.
> 
> Plenty of commands have it.
> 
> Alan> Probably not in scope for this patch series, but it would be
> nice for consistency
> Alan> for usage statements to be added to all the commands in this 
> file.
> 
> I can drop this patch.

Please don't :).  I think this is a welcome change.

Simon

Re: [RFA 1/3] Add usage to printf command

[Alan Hayward]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 714 bytes --]



> On 16 Feb 2018, at 23:09, Simon Marchi <simon.marchi@polymtl.ca> wrote:
> 
> On 2018-02-16 16:58, Tom Tromey wrote:
>>>>>>> "Alan" == Alan Hayward <Alan.Hayward@arm.com> writes:
>> Alan> I like the addition of a “usage”. But this will be the only
>> command to have it.
>> Plenty of commands have it.

I meant to say “the only command in printcmd.c to have it". :)

>> Alan> Probably not in scope for this patch series, but it would be
>> nice for consistency
>> Alan> for usage statements to be added to all the commands in this file.
>> I can drop this patch.
> 
> Please don't :).  I think this is a welcome change.
> 

Agree with Simon.


Alan.\x16º&Öéj×!zÊÞ¶êç׎xßyb²Ö«r\x18\x1dn–­r\x17¬

Re: [RFA 2/3] Allow - in %p for printf

[Tom Tromey]

>>>>> "Alan" == Alan Hayward <Alan.Hayward@arm.com> writes:

Alan> I found this comment a little confusing. How about something like:
Alan> Copy and width or flags. “-“ is the only valid flag for pointers —
Alan> see the format_pieces constructor.

I made this change.

Tom

Re: [RFA 0/3] a few minor printf fixes

[Tom Tromey]

>>>>> "Tom" == Tom Tromey <tom@tromey.com> writes:

Tom> This series fixes a few papercuts in the "printf" command.
Tom> Regression tested by the buildbot.

I'm finally checking this in.

Tom