[PATCH 0/2] Changes to cli-out.c for field alignment and formatting

[PATCH 0/2] Changes to cli-out.c for field alignment and formatting

[Andrew Burgess]

A couple of changes in cli-out that change how fields are formatted.

--

Andrew Burgess (2):
  gdb: Respect field width and alignment for 'fmt' fields in CLI output
  gdb: Use string_printf to format int fields instead of a fixed size
    buffer

 gdb/ChangeLog                          | 13 ++++++++++++
 gdb/breakpoint.c                       |  3 ---
 gdb/cli-out.c                          | 14 ++++++-------
 gdb/testsuite/ChangeLog                |  5 +++++
 gdb/testsuite/gdb.opt/inline-break.exp | 36 ++++++++++++++++++++++++++++++++++
 5 files changed, 60 insertions(+), 11 deletions(-)

-- 
2.14.5

[PATCH 2/2] gdb: Use string_printf to format int fields instead of a fixed size buffer

[Andrew Burgess]

This patch removes a FIXME comment from cli-out.c, now instead of
formatting integers into a fixed size buffer we build a std::string
and extract the formatted integer from that.

The old code using a fixed size buffer was probably fine (the integer
was not going to overflow it) and probably slightly more efficient
(avoids building a std::string) however, given we already have utility
code in GDB that will allow the 'FIXME' comment to be removed, it
seems like an easy improvement.

gdb/ChangeLog:

	* cli-out.c (cli_ui_out::do_field_int): Use string_printf rather
	than a fixed size buffer.
---
 gdb/ChangeLog | 5 +++++
 gdb/cli-out.c | 6 ++----
 2 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/gdb/cli-out.c b/gdb/cli-out.c
index 3fe131fef37..57687cd663f 100644
--- a/gdb/cli-out.c
+++ b/gdb/cli-out.c
@@ -94,14 +94,12 @@ void
 cli_ui_out::do_field_int (int fldno, int width, ui_align alignment,
 			  const char *fldname, int value)
 {
-  char buffer[20];	/* FIXME: how many chars long a %d can become? */
-
   if (m_suppress_output)
     return;
 
-  xsnprintf (buffer, sizeof (buffer), "%d", value);
+  std::string str = string_printf ("%d", value);
 
-  do_field_string (fldno, width, alignment, fldname, buffer);
+  do_field_string (fldno, width, alignment, fldname, str.c_str ());
 }
 
 /* used to omit a field */
-- 
2.14.5

[PATCH 1/2] gdb: Respect field width and alignment for 'fmt' fields in CLI output

[Andrew Burgess]

Currently the method 'cli_ui_out::do_field_fmt' has this comment:

  /* This is the only field function that does not align.  */

The reality is even slightly worse, the 'fmt' field type doesn't
respect either the field alignment or the field width.  In at least
one place in GDB we attempt to work around this lack of respect for
field width by adding additional padding manually.  But, as is often
the case, this is leading to knock on problems.

Conside the output for 'info breakpoints' when a breakpoint has
multiple locations.  This example is taken from the testsuite, from
test gdb.opt/inline-break.exp:

  (gdb) info breakpoints
  Num     Type           Disp Enb Address            What
  1       breakpoint     keep y   <MULTIPLE>
  1.1                     y     0x00000000004004ae in func4b at /src/gdb/testsuite/gdb.opt/inline-break.c:64
  1.2                     y     0x0000000000400682 in func4b at /src/gdb/testsuite/gdb.opt/inline-break.c:64

The miss-alignment of the fields shown here is exactly as GDB
currently produces.

With this patch 'fmt' style fields are now first written into a
temporary buffer, and then written out as a 'string' field.  The
result is that the field width, and alignment should now be respected.
With this patch in place the output from GDB now looks like this:

  (gdb) info breakpoints
  Num     Type           Disp Enb Address            What
  1       breakpoint     keep y   <MULTIPLE>
  1.1                         y   0x00000000004004ae in func4b at /src/gdb/testsuite/gdb.opt/inline-break.c:64
  1.2                         y   0x0000000000400682 in func4b at /src/gdb/testsuite/gdb.opt/inline-break.c:64

This patch has been tested on x86-64/Linux with no regressions,
however, the testsuite doesn't always spot broken output formatting or
alignment.  I have also audited all uses of 'fmt' fields that I could
find, and I don't think there are any other places that specifically
try to work around the lack of width/alignment, however, I could have
missed something.

gdb/ChangeLog:

	* breakpoint.c (print_one_breakpoint_location): Reduce whitespace,
	and remove insertion of extra spaces in GDB's output.
	* cli-out.c (cli_ui_out::do_field_fmt): Update header comment.
	Layout field into a temporary buffer, and then output it as a
	string field.

gdb/testsuite/ChangeLog:

	* gdb.opt/inline-break.exp: Add test that info breakpoint output
	is correctly aligned.
---
 gdb/ChangeLog                          |  8 ++++++++
 gdb/breakpoint.c                       |  3 ---
 gdb/cli-out.c                          |  8 ++++----
 gdb/testsuite/ChangeLog                |  5 +++++
 gdb/testsuite/gdb.opt/inline-break.exp | 36 ++++++++++++++++++++++++++++++++++
 5 files changed, 53 insertions(+), 7 deletions(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 3d254344f2f..6bd456ebbb8 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -6058,16 +6058,13 @@ print_one_breakpoint_location (struct breakpoint *b,
   else
     uiout->field_string ("disp", bpdisp_text (b->disposition));
 
-
   /* 4 */
   annotate_field (3);
   if (part_of_multiple)
     uiout->field_string ("enabled", loc->enabled ? "y" : "n");
   else
     uiout->field_fmt ("enabled", "%c", bpenables[(int) b->enable_state]);
-  uiout->spaces (2);
 
-  
   /* 5 and 6 */
   if (b->ops != NULL && b->ops->print_one != NULL)
     {
diff --git a/gdb/cli-out.c b/gdb/cli-out.c
index ad0a34ed39f..3fe131fef37 100644
--- a/gdb/cli-out.c
+++ b/gdb/cli-out.c
@@ -165,7 +165,7 @@ cli_ui_out::do_field_string (int fldno, int width, ui_align align,
     field_separator ();
 }
 
-/* This is the only field function that does not align.  */
+/* Output field containing ARGS using printf formatting in FORMAT.  */
 
 void
 cli_ui_out::do_field_fmt (int fldno, int width, ui_align align,
@@ -175,10 +175,10 @@ cli_ui_out::do_field_fmt (int fldno, int width, ui_align align,
   if (m_suppress_output)
     return;
 
-  vfprintf_filtered (m_streams.back (), format, args);
+  string_file stb;
+  stb.vprintf (format, args);
 
-  if (align != ui_noalign)
-    field_separator ();
+  do_field_string (fldno, width, align, fldname, stb.string ().c_str ());
 }
 
 void
diff --git a/gdb/testsuite/gdb.opt/inline-break.exp b/gdb/testsuite/gdb.opt/inline-break.exp
index aed38ed631f..06a08612371 100644
--- a/gdb/testsuite/gdb.opt/inline-break.exp
+++ b/gdb/testsuite/gdb.opt/inline-break.exp
@@ -304,4 +304,40 @@ with_test_prefix "address" {
 	"breakpoint hit presents stop at inlined function"
 }
 
+with_test_prefix "check alignment" {
+
+    clean_restart $binfile
+
+    if {![runto main]} {
+	untested "could not run to main"
+	continue
+    }
+
+    gdb_test "break func4b" \
+	"Breakpoint.*at.*func4b.*\\(2 locations\\)"
+
+    set expected_line_length -1
+    gdb_test_multiple "info break \$bpnum" "xxxx" {
+	-re "Num     Type           Disp Enb Address            What\r\n" {
+	    exp_continue
+	}
+	-re "($decimal \[^\r\n\]+)<MULTIPLE>\[^\r\n\]+\r\n" {
+	    if {$expected_line_length != -1} {
+		fail "multiple header lines seen"
+	    }
+	    set expected_line_length [string length $expect_out(1,string)]
+	    exp_continue
+	}
+	-re "($decimal\.($decimal) \[^\r\n\]+)$hex\[^\r\n\]+\r\n" {
+	    set len [string length $expect_out(1,string)]
+	    set loc $expect_out(2,string)
+	    gdb_assert {$len == $expected_line_length} \
+		"check alignment of location line $loc"
+	    exp_continue
+	}
+	-re "$gdb_prompt $" {
+	}
+    }
+}
+
 unset -nocomplain results
-- 
2.14.5

Re: [PATCH 1/2] gdb: Respect field width and alignment for 'fmt' fields in CLI output

[Tom Tromey]

>>>>> "Andrew" == Andrew Burgess <andrew.burgess@embecosm.com> writes:

Andrew> This patch has been tested on x86-64/Linux with no regressions,
Andrew> however, the testsuite doesn't always spot broken output formatting or
Andrew> alignment.  I have also audited all uses of 'fmt' fields that I could
Andrew> find, and I don't think there are any other places that specifically
Andrew> try to work around the lack of width/alignment, however, I could have
Andrew> missed something.

Thanks for the patch.

Andrew> -  vfprintf_filtered (m_streams.back (), format, args);
Andrew> +  string_file stb;
Andrew> +  stb.vprintf (format, args);

I think just using string_vprintf directly is simpler here.
This is ok with that change.

Tom

Re: [PATCH 2/2] gdb: Use string_printf to format int fields instead of a fixed size buffer

[Tom Tromey]

>>>>> "Andrew" == Andrew Burgess <andrew.burgess@embecosm.com> writes:

Andrew> This patch removes a FIXME comment from cli-out.c, now instead of
Andrew> formatting integers into a fixed size buffer we build a std::string
Andrew> and extract the formatted integer from that.

Andrew> The old code using a fixed size buffer was probably fine (the integer
Andrew> was not going to overflow it) and probably slightly more efficient
Andrew> (avoids building a std::string) however, given we already have utility
Andrew> code in GDB that will allow the 'FIXME' comment to be removed, it
Andrew> seems like an easy improvement.

Andrew> gdb/ChangeLog:

Andrew> 	* cli-out.c (cli_ui_out::do_field_int): Use string_printf rather
Andrew> 	than a fixed size buffer.

Thanks.  This is ok.

Tom