[RFA 0/3] a few minor printf fixes

[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 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 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]

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

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

[Alan Hayward]

> On 15 Feb 2018, at 20:49, Tom Tromey <tom@tromey.com> wrote:
> 
> 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."));
> 

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

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



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

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

[Pedro Alves]

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.

Thanks,
Pedro Alves

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]

> 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.

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