[PATCH 3/3] Introduce field_unsigned

[PATCH 3/3] Introduce field_unsigned

[Tom Tromey]

This adds field_unsigned and changes various places using field_fmt
with "%u" to use this instead.  This also replaces an existing
equivalent helper function in record-btrace.c.

gdb/ChangeLog
2019-07-01  Tom Tromey  <tromey@adacore.com>

	* symfile.c (generic_load): Use field_unsigned.
	(print_transfer_performance): Likewise.
	* record-btrace.c (ui_out_field_uint): Remove.
	(btrace_call_history_insn_range, btrace_call_history): Use
	field_unsigned.
	* disasm.c (gdb_pretty_print_disassembler::pretty_print_insn): Use
	field_unsigned.
	* ui-out.h (class ui_out) <field_unsigned>: New method.
---
 gdb/ChangeLog       | 11 +++++++++++
 gdb/disasm.c        |  2 +-
 gdb/record-btrace.c | 14 +++-----------
 gdb/symfile.c       | 12 ++++++------
 gdb/ui-out.h        |  6 ++++++
 5 files changed, 27 insertions(+), 18 deletions(-)

diff --git a/gdb/disasm.c b/gdb/disasm.c
index 4e58cb67f93..927b092ce2c 100644
--- a/gdb/disasm.c
+++ b/gdb/disasm.c
@@ -209,7 +209,7 @@ gdb_pretty_print_disassembler::pretty_print_insn (struct ui_out *uiout,
 
     if (insn->number != 0)
       {
-	uiout->field_fmt ("insn-number", "%u", insn->number);
+	uiout->field_unsigned ("insn-number", insn->number);
 	uiout->text ("\t");
       }
 
diff --git a/gdb/record-btrace.c b/gdb/record-btrace.c
index 21085d5c62c..313f1ac7d1e 100644
--- a/gdb/record-btrace.c
+++ b/gdb/record-btrace.c
@@ -623,14 +623,6 @@ btrace_ui_out_decode_error (struct ui_out *uiout, int errcode,
   uiout->text (_("]\n"));
 }
 
-/* Print an unsigned int.  */
-
-static void
-ui_out_field_uint (struct ui_out *uiout, const char *fld, unsigned int val)
-{
-  uiout->field_fmt (fld, "%u", val);
-}
-
 /* A range of source lines.  */
 
 struct btrace_line_range
@@ -1032,9 +1024,9 @@ btrace_call_history_insn_range (struct ui_out *uiout,
   begin = bfun->insn_offset;
   end = begin + size - 1;
 
-  ui_out_field_uint (uiout, "insn begin", begin);
+  uiout->field_unsigned ("insn begin", begin);
   uiout->text (",");
-  ui_out_field_uint (uiout, "insn end", end);
+  uiout->field_unsigned ("insn end", end);
 }
 
 /* Compute the lowest and highest source line for the instructions in BFUN
@@ -1155,7 +1147,7 @@ btrace_call_history (struct ui_out *uiout,
       msym = bfun->msym;
 
       /* Print the function index.  */
-      ui_out_field_uint (uiout, "index", bfun->number);
+      uiout->field_unsigned ("index", bfun->number);
       uiout->text ("\t");
 
       /* Indicate gaps in the trace.  */
diff --git a/gdb/symfile.c b/gdb/symfile.c
index 58049a544b5..9b71ad6b3a4 100644
--- a/gdb/symfile.c
+++ b/gdb/symfile.c
@@ -2083,7 +2083,7 @@ generic_load (const char *args, int from_tty)
   uiout->text ("Start address ");
   uiout->field_core_addr ("address", target_gdbarch (), entry);
   uiout->text (", load size ");
-  uiout->field_fmt ("load-size", "%lu", total_progress.data_count);
+  uiout->field_unsigned ("load-size", total_progress.data_count);
   uiout->text ("\n");
   regcache_write_pc (get_current_regcache (), entry);
 
@@ -2126,29 +2126,29 @@ print_transfer_performance (struct ui_file *stream,
 
       if (uiout->is_mi_like_p ())
 	{
-	  uiout->field_fmt ("transfer-rate", "%lu", rate * 8);
+	  uiout->field_unsigned ("transfer-rate", rate * 8);
 	  uiout->text (" bits/sec");
 	}
       else if (rate < 1024)
 	{
-	  uiout->field_fmt ("transfer-rate", "%lu", rate);
+	  uiout->field_unsigned ("transfer-rate", rate);
 	  uiout->text (" bytes/sec");
 	}
       else
 	{
-	  uiout->field_fmt ("transfer-rate", "%lu", rate / 1024);
+	  uiout->field_unsigned ("transfer-rate", rate / 1024);
 	  uiout->text (" KB/sec");
 	}
     }
   else
     {
-      uiout->field_fmt ("transferred-bits", "%lu", (data_count * 8));
+      uiout->field_unsigned ("transferred-bits", (data_count * 8));
       uiout->text (" bits in <1 sec");
     }
   if (write_count > 0)
     {
       uiout->text (", ");
-      uiout->field_fmt ("write-rate", "%lu", data_count / write_count);
+      uiout->field_unsigned ("write-rate", data_count / write_count);
       uiout->text (" bytes/write");
     }
   uiout->text (".\n");
diff --git a/gdb/ui-out.h b/gdb/ui-out.h
index 9eba70eedac..cf074ebcef4 100644
--- a/gdb/ui-out.h
+++ b/gdb/ui-out.h
@@ -121,6 +121,12 @@ class ui_out
   void field_fmt (const char *fldname, const char *format, ...)
     ATTRIBUTE_PRINTF (3, 4);
 
+  /* Like field_int, but print an unsigned value.  */
+  void field_unsigned (const char *fldname, ULONGEST value)
+  {
+    field_string (fldname, pulongest (value));
+  }
+
   void spaces (int numspaces);
   void text (const char *string);
   void message (const char *format, ...) ATTRIBUTE_PRINTF (2, 3);
-- 
2.20.1

[PATCH 1/3] Use field_core_addr in more places

[Tom Tromey]

This changes a few spots that use field_fmt to use field_core_addr
instead.

gdb/ChangeLog
2019-07-01  Tom Tromey  <tromey@adacore.com>

	* target.c (flash_erase_command): Use field_core_addr.
	* symfile.c (generic_load): Use field_core_addr.
	* sparc64-linux-tdep.c (sparc64_linux_handle_segmentation_fault):
	Use field_core_addr.
	* i386-linux-tdep.c (i386_linux_handle_segmentation_fault): Use
	field_core_addr.
---
 gdb/ChangeLog            | 9 +++++++++
 gdb/i386-linux-tdep.c    | 6 +++---
 gdb/sparc64-linux-tdep.c | 6 +++---
 gdb/symfile.c            | 2 +-
 gdb/target.c             | 2 +-
 5 files changed, 17 insertions(+), 8 deletions(-)

diff --git a/gdb/i386-linux-tdep.c b/gdb/i386-linux-tdep.c
index 74f429b49af..b5c3ef2b71e 100644
--- a/gdb/i386-linux-tdep.c
+++ b/gdb/i386-linux-tdep.c
@@ -432,13 +432,13 @@ i386_linux_handle_segmentation_fault (struct gdbarch *gdbarch,
     uiout->field_string ("sigcode-meaning", _("Lower bound violation"));
 
   uiout->text (_(" while accessing address "));
-  uiout->field_fmt ("bound-access", "%s", paddress (gdbarch, access));
+  uiout->field_core_addr ("bound-access", gdbarch, access);
 
   uiout->text (_("\nBounds: [lower = "));
-  uiout->field_fmt ("lower-bound", "%s", paddress (gdbarch, lower_bound));
+  uiout->field_core_addr ("lower-bound", gdbarch, lower_bound);
 
   uiout->text (_(", upper = "));
-  uiout->field_fmt ("upper-bound", "%s", paddress (gdbarch, upper_bound));
+  uiout->field_core_addr ("upper-bound", gdbarch, upper_bound);
 
   uiout->text (_("]"));
 }
diff --git a/gdb/sparc64-linux-tdep.c b/gdb/sparc64-linux-tdep.c
index 158db97ec61..563d21e4b4c 100644
--- a/gdb/sparc64-linux-tdep.c
+++ b/gdb/sparc64-linux-tdep.c
@@ -150,19 +150,19 @@ sparc64_linux_handle_segmentation_fault (struct gdbarch *gdbarch,
       uiout->text ("\n");
       uiout->field_string ("sigcode-meaning", _("ADI disabled"));
       uiout->text (_(" while accessing address "));
-      uiout->field_fmt ("bound-access", "%s", paddress (gdbarch, addr));
+      uiout->field_core_addr ("bound-access", gdbarch, addr);
       break;
     case SEGV_ADIDERR:	/* disrupting mismatch */
       uiout->text ("\n");
       uiout->field_string ("sigcode-meaning", _("ADI deferred mismatch"));
       uiout->text (_(" while accessing address "));
-      uiout->field_fmt ("bound-access", "%s", paddress (gdbarch, addr));
+      uiout->field_core_addr ("bound-access", gdbarch, addr);
       break;
     case SEGV_ADIPERR:	/* precise mismatch */
       uiout->text ("\n");
       uiout->field_string ("sigcode-meaning", _("ADI precise mismatch"));
       uiout->text (_(" while accessing address "));
-      uiout->field_fmt ("bound-access", "%s", paddress (gdbarch, addr));
+      uiout->field_core_addr ("bound-access", gdbarch, addr);
       break;
     default:
       break;
diff --git a/gdb/symfile.c b/gdb/symfile.c
index 6f9c81c8b94..58049a544b5 100644
--- a/gdb/symfile.c
+++ b/gdb/symfile.c
@@ -2081,7 +2081,7 @@ generic_load (const char *args, int from_tty)
   CORE_ADDR entry = bfd_get_start_address (loadfile_bfd.get ());
   entry = gdbarch_addr_bits_remove (target_gdbarch (), entry);
   uiout->text ("Start address ");
-  uiout->field_fmt ("address", "%s", paddress (target_gdbarch (), entry));
+  uiout->field_core_addr ("address", target_gdbarch (), entry);
   uiout->text (", load size ");
   uiout->field_fmt ("load-size", "%lu", total_progress.data_count);
   uiout->text ("\n");
diff --git a/gdb/target.c b/gdb/target.c
index 1c04095fd63..febb3390339 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -3793,7 +3793,7 @@ flash_erase_command (const char *cmd, int from_tty)
 	  ui_out_emit_tuple tuple_emitter (current_uiout, "erased-regions");
 
           current_uiout->message (_("Erasing flash memory region at address "));
-          current_uiout->field_fmt ("address", "%s", paddress (gdbarch, m.lo));
+          current_uiout->field_core_addr ("address", gdbarch, m.lo);
           current_uiout->message (", size = ");
           current_uiout->field_fmt ("size", "%s", hex_string (m.hi - m.lo));
           current_uiout->message ("\n");
-- 
2.20.1

[PATCH 0/3] replace many uses of field_fmt

[Tom Tromey]

This series replaces many of the uses of field_fmt.  I believe it
fixes a few minor latent bugs along the way.

Tested on x86-64 Fedora 29, though note that this touches files not
tested in this configuration.

Let me know what you think.

Tom

[PATCH 2/3] Use field_string in more places

[Tom Tromey]

This replaces uses of field_fmt with a "%s" format string to use
field_string instead.

I think this fixes some latent bugs, in cases like:

-  current_uiout->field_fmt ("event_status", "0x%s", phex (event_status, 4));

Here, I think "0x" will be printed twice.

gdb/ChangeLog
2019-07-01  Tom Tromey  <tromey@adacore.com>

	* mi/mi-main.c (list_available_thread_groups): Use field_string.
	* mi/mi-interp.c (mi_memory_changed): Use field_string.
	* target.c (flash_erase_command): Use field_string.
	* spu-tdep.c (info_spu_event_command, info_spu_signal_command)
	(info_spu_mailbox_list, info_spu_dma_cmdlist)
	(info_spu_dma_command, info_spu_proxydma_command): Use
	field_string.
	* infrun.c (print_signal_received_reason): Use field_string.
	* i386-tdep.c (i386_mpx_print_bounds): Use field_string.
	* breakpoint.c (maybe_print_thread_hit_breakpoint): Use
	field_string.
	* ada-tasks.c (print_ada_task_info): Use field_string.
---
 gdb/ChangeLog      | 15 +++++++++++++++
 gdb/ada-tasks.c    |  7 +++----
 gdb/breakpoint.c   |  4 ++--
 gdb/i386-tdep.c    |  2 +-
 gdb/infrun.c       |  4 ++--
 gdb/mi/mi-interp.c |  2 +-
 gdb/mi/mi-main.c   |  2 +-
 gdb/spu-tdep.c     | 44 ++++++++++++++++++++++----------------------
 gdb/target.c       |  2 +-
 9 files changed, 48 insertions(+), 34 deletions(-)

diff --git a/gdb/ada-tasks.c b/gdb/ada-tasks.c
index 9c07f0ca226..a8fcc8ed43b 100644
--- a/gdb/ada-tasks.c
+++ b/gdb/ada-tasks.c
@@ -1129,10 +1129,9 @@ print_ada_task_info (struct ui_out *uiout,
 	uiout->field_string ("state", task_states[task_info->state]);
 
       /* Finally, print the task name.  */
-      uiout->field_fmt ("name",
-			"%s",
-			task_info->name[0] != '\0' ? task_info->name
-						   : _("<no name>"));
+      uiout->field_string ("name",
+			   task_info->name[0] != '\0' ? task_info->name
+			   : _("<no name>"));
 
       uiout->text ("\n");
     }
diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 8422db8b571..5cdebddfc5d 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -4490,13 +4490,13 @@ maybe_print_thread_hit_breakpoint (struct ui_out *uiout)
       struct thread_info *thr = inferior_thread ();
 
       uiout->text ("Thread ");
-      uiout->field_fmt ("thread-id", "%s", print_thread_id (thr));
+      uiout->field_string ("thread-id", print_thread_id (thr));
 
       name = thr->name != NULL ? thr->name : target_thread_name (thr);
       if (name != NULL)
 	{
 	  uiout->text (" \"");
-	  uiout->field_fmt ("name", "%s", name);
+	  uiout->field_string ("name", name);
 	  uiout->text ("\"");
 	}
 
diff --git a/gdb/i386-tdep.c b/gdb/i386-tdep.c
index 00c1f8d7499..42fb8b26463 100644
--- a/gdb/i386-tdep.c
+++ b/gdb/i386-tdep.c
@@ -8914,7 +8914,7 @@ i386_mpx_print_bounds (const CORE_ADDR bt_entry[4])
 
       size = (size > -1 ? size + 1 : size);
       uiout->text (", size = ");
-      uiout->field_fmt ("size", "%s", plongest (size));
+      uiout->field_string ("size", plongest (size));
 
       uiout->text (", metadata = ");
       uiout->field_core_addr ("metadata", gdbarch, bt_entry[3]);
diff --git a/gdb/infrun.c b/gdb/infrun.c
index 4fd92f1bac2..f1f10fd30ff 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -7690,13 +7690,13 @@ print_signal_received_reason (struct ui_out *uiout, enum gdb_signal siggnal)
       const char *name;
 
       uiout->text ("\nThread ");
-      uiout->field_fmt ("thread-id", "%s", print_thread_id (thr));
+      uiout->field_string ("thread-id", print_thread_id (thr));
 
       name = thr->name != NULL ? thr->name : target_thread_name (thr);
       if (name != NULL)
 	{
 	  uiout->text (" \"");
-	  uiout->field_fmt ("name", "%s", name);
+	  uiout->field_string ("name", name);
 	  uiout->text ("\"");
 	}
     }
diff --git a/gdb/mi/mi-interp.c b/gdb/mi/mi-interp.c
index ad1a06cae0b..76583ff494b 100644
--- a/gdb/mi/mi-interp.c
+++ b/gdb/mi/mi-interp.c
@@ -1172,7 +1172,7 @@ mi_memory_changed (struct inferior *inferior, CORE_ADDR memaddr,
 
       mi_uiout->field_fmt ("thread-group", "i%d", inferior->num);
       mi_uiout->field_core_addr ("addr", target_gdbarch (), memaddr);
-      mi_uiout->field_fmt ("len", "%s", hex_string (len));
+      mi_uiout->field_string ("len", hex_string (len));
 
       /* Append 'type=code' into notification if MEMADDR falls in the range of
 	 sections contain code.  */
diff --git a/gdb/mi/mi-main.c b/gdb/mi/mi-main.c
index 13c310d494c..dc4feedb6f4 100644
--- a/gdb/mi/mi-main.c
+++ b/gdb/mi/mi-main.c
@@ -746,7 +746,7 @@ list_available_thread_groups (const std::set<int> &ids, int recurse)
 
       ui_out_emit_tuple tuple_emitter (uiout, NULL);
 
-      uiout->field_fmt ("id", "%s", pid->c_str ());
+      uiout->field_string ("id", pid->c_str ());
       uiout->field_string ("type", "process");
       if (cmd)
 	uiout->field_string ("description", cmd->c_str ());
diff --git a/gdb/spu-tdep.c b/gdb/spu-tdep.c
index a2ac3149d4d..3ac709c1611 100644
--- a/gdb/spu-tdep.c
+++ b/gdb/spu-tdep.c
@@ -2096,10 +2096,10 @@ info_spu_event_command (const char *args, int from_tty)
   ui_out_emit_tuple tuple_emitter (current_uiout, "SPUInfoEvent");
 
   current_uiout->text (_("Event Status "));
-  current_uiout->field_fmt ("event_status", "0x%s", phex (event_status, 4));
+  current_uiout->field_string ("event_status", phex (event_status, 4));
   current_uiout->text ("\n");
   current_uiout->text (_("Event Mask   "));
-  current_uiout->field_fmt ("event_mask", "0x%s", phex (event_mask, 4));
+  current_uiout->field_string ("event_mask", phex (event_mask, 4));
   current_uiout->text ("\n");
 }
 
@@ -2168,10 +2168,10 @@ info_spu_signal_command (const char *args, int from_tty)
   if (current_uiout->is_mi_like_p ())
     {
       current_uiout->field_int ("signal1_pending", signal1_pending);
-      current_uiout->field_fmt ("signal1", "0x%s", phex_nz (signal1, 4));
+      current_uiout->field_string ("signal1", phex (signal1, 4));
       current_uiout->field_int ("signal1_type", signal1_type);
       current_uiout->field_int ("signal2_pending", signal2_pending);
-      current_uiout->field_fmt ("signal2", "0x%s", phex_nz (signal2, 4));
+      current_uiout->field_string ("signal2", phex (signal2, 4));
       current_uiout->field_int ("signal2_type", signal2_type);
     }
   else
@@ -2218,7 +2218,7 @@ info_spu_mailbox_list (gdb_byte *buf, int nr, enum bfd_endian byte_order,
 	ULONGEST val;
 	ui_out_emit_tuple tuple_emitter (current_uiout, "mbox");
 	val = extract_unsigned_integer (buf + 4*i, 4, byte_order);
-	current_uiout->field_fmt (field, "0x%s", phex (val, 4));
+	current_uiout->field_string (field, phex (val, 4));
       }
 
       current_uiout->text ("\n");
@@ -2421,7 +2421,7 @@ info_spu_dma_cmdlist (gdb_byte *buf, int nr, enum bfd_endian byte_order)
 	current_uiout->field_int ("rid", rclass_id);
 
 	if (ea_valid_p)
-	  current_uiout->field_fmt ("ea", "0x%s", phex (mfc_ea, 8));
+	  current_uiout->field_string ("ea", phex (mfc_ea, 8));
 	else
 	  current_uiout->field_skip ("ea");
 
@@ -2494,16 +2494,16 @@ info_spu_dma_command (const char *args, int from_tty)
 
   if (current_uiout->is_mi_like_p ())
     {
-      current_uiout->field_fmt ("dma_info_type", "0x%s",
-				phex_nz (dma_info_type, 4));
-      current_uiout->field_fmt ("dma_info_mask", "0x%s",
-				phex_nz (dma_info_mask, 4));
-      current_uiout->field_fmt ("dma_info_status", "0x%s",
-				phex_nz (dma_info_status, 4));
-      current_uiout->field_fmt ("dma_info_stall_and_notify", "0x%s",
-				phex_nz (dma_info_stall_and_notify, 4));
-      current_uiout->field_fmt ("dma_info_atomic_command_status", "0x%s",
-				phex_nz (dma_info_atomic_command_status, 4));
+      current_uiout->field_string ("dma_info_type",
+				   phex (dma_info_type, 4));
+      current_uiout->field_string ("dma_info_mask",
+				   phex (dma_info_mask, 4));
+      current_uiout->field_string ("dma_info_status",
+				   phex (dma_info_status, 4));
+      current_uiout->field_string ("dma_info_stall_and_notify",
+				   phex (dma_info_stall_and_notify, 4));
+      current_uiout->field_string ("dma_info_atomic_command_status",
+				   phex (dma_info_atomic_command_status, 4));
     }
   else
     {
@@ -2564,12 +2564,12 @@ info_spu_proxydma_command (const char *args, int from_tty)
 
   if (current_uiout->is_mi_like_p ())
     {
-      current_uiout->field_fmt ("proxydma_info_type", "0x%s",
-				phex_nz (dma_info_type, 4));
-      current_uiout->field_fmt ("proxydma_info_mask", "0x%s",
-				phex_nz (dma_info_mask, 4));
-      current_uiout->field_fmt ("proxydma_info_status", "0x%s",
-				phex_nz (dma_info_status, 4));
+      current_uiout->field_string ("proxydma_info_type",
+				   phex (dma_info_type, 4));
+      current_uiout->field_string ("proxydma_info_mask",
+				   phex (dma_info_mask, 4));
+      current_uiout->field_string ("proxydma_info_status",
+				   phex (dma_info_status, 4));
     }
   else
     {
diff --git a/gdb/target.c b/gdb/target.c
index febb3390339..417b795d47f 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -3795,7 +3795,7 @@ flash_erase_command (const char *cmd, int from_tty)
           current_uiout->message (_("Erasing flash memory region at address "));
           current_uiout->field_core_addr ("address", gdbarch, m.lo);
           current_uiout->message (", size = ");
-          current_uiout->field_fmt ("size", "%s", hex_string (m.hi - m.lo));
+          current_uiout->field_string ("size", hex_string (m.hi - m.lo));
           current_uiout->message ("\n");
         }
     }
-- 
2.20.1

RE: [PATCH 3/3] Introduce field_unsigned

[Metzger, Markus T]

Hello Tom,

> This adds field_unsigned and changes various places using field_fmt
> with "%u" to use this instead.  This also replaces an existing
> equivalent helper function in record-btrace.c.
> 
> gdb/ChangeLog
> 2019-07-01  Tom Tromey  <tromey@adacore.com>
> 
> 	* symfile.c (generic_load): Use field_unsigned.
> 	(print_transfer_performance): Likewise.
> 	* record-btrace.c (ui_out_field_uint): Remove.
> 	(btrace_call_history_insn_range, btrace_call_history): Use
> 	field_unsigned.
> 	* disasm.c (gdb_pretty_print_disassembler::pretty_print_insn): Use
> 	field_unsigned.
> 	* ui-out.h (class ui_out) <field_unsigned>: New method.
> ---
>  gdb/ChangeLog       | 11 +++++++++++
>  gdb/disasm.c        |  2 +-
>  gdb/record-btrace.c | 14 +++-----------
>  gdb/symfile.c       | 12 ++++++------
>  gdb/ui-out.h        |  6 ++++++
>  5 files changed, 27 insertions(+), 18 deletions(-)


> diff --git a/gdb/ui-out.h b/gdb/ui-out.h
> index 9eba70eedac..cf074ebcef4 100644
> --- a/gdb/ui-out.h
> +++ b/gdb/ui-out.h
> @@ -121,6 +121,12 @@ class ui_out
>    void field_fmt (const char *fldname, const char *format, ...)
>      ATTRIBUTE_PRINTF (3, 4);
> 
> +  /* Like field_int, but print an unsigned value.  */
> +  void field_unsigned (const char *fldname, ULONGEST value)
> +  {
> +    field_string (fldname, pulongest (value));
> +  }
> +
>    void spaces (int numspaces);
>    void text (const char *string);
>    void message (const char *format, ...) ATTRIBUTE_PRINTF (2, 3);

This is quite different from how other types are handled, e.g.

ui_out::field_int (const char *fldname, int value)
{
  int fldno;
  int width;
  ui_align align;

  verify_field (&fldno, &width, &align);

  do_field_int (fldno, width, align, fldname, value);
}

Is there a reason for not treating int and unsigned int in a similar way?

The rest looks good to me.

Regards,
Markus.
Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Christin Eisenschmid, Gary Kershaw
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

Re: [PATCH 2/3] Use field_string in more places

[Andrew Burgess]

* Tom Tromey <tromey@adacore.com> [2019-07-01 14:00:57 -0600]:

> This replaces uses of field_fmt with a "%s" format string to use
> field_string instead.
> 
> I think this fixes some latent bugs, in cases like:
> 
> -  current_uiout->field_fmt ("event_status", "0x%s", phex (event_status, 4));
> 
> Here, I think "0x" will be printed twice.

I don't think this is correct, at least experimenting on HEAD for me
doesn't print it twice.

I think you could replace phex with hex_string_custom to add the '0x'
for you, except that hex_string_custom seems to treat the width
differently to phex.  phex treats the width as a byte width, while
hex_string_custom treats the width as a character width, so

  phex (event_status, 4)

needs to become

  hex_string_custom (event_status, 8)

Thanks,
Andrew


> 
> gdb/ChangeLog
> 2019-07-01  Tom Tromey  <tromey@adacore.com>
> 
> 	* mi/mi-main.c (list_available_thread_groups): Use field_string.
> 	* mi/mi-interp.c (mi_memory_changed): Use field_string.
> 	* target.c (flash_erase_command): Use field_string.
> 	* spu-tdep.c (info_spu_event_command, info_spu_signal_command)
> 	(info_spu_mailbox_list, info_spu_dma_cmdlist)
> 	(info_spu_dma_command, info_spu_proxydma_command): Use
> 	field_string.
> 	* infrun.c (print_signal_received_reason): Use field_string.
> 	* i386-tdep.c (i386_mpx_print_bounds): Use field_string.
> 	* breakpoint.c (maybe_print_thread_hit_breakpoint): Use
> 	field_string.
> 	* ada-tasks.c (print_ada_task_info): Use field_string.
> ---
>  gdb/ChangeLog      | 15 +++++++++++++++
>  gdb/ada-tasks.c    |  7 +++----
>  gdb/breakpoint.c   |  4 ++--
>  gdb/i386-tdep.c    |  2 +-
>  gdb/infrun.c       |  4 ++--
>  gdb/mi/mi-interp.c |  2 +-
>  gdb/mi/mi-main.c   |  2 +-
>  gdb/spu-tdep.c     | 44 ++++++++++++++++++++++----------------------
>  gdb/target.c       |  2 +-
>  9 files changed, 48 insertions(+), 34 deletions(-)
> 
> diff --git a/gdb/ada-tasks.c b/gdb/ada-tasks.c
> index 9c07f0ca226..a8fcc8ed43b 100644
> --- a/gdb/ada-tasks.c
> +++ b/gdb/ada-tasks.c
> @@ -1129,10 +1129,9 @@ print_ada_task_info (struct ui_out *uiout,
>  	uiout->field_string ("state", task_states[task_info->state]);
>  
>        /* Finally, print the task name.  */
> -      uiout->field_fmt ("name",
> -			"%s",
> -			task_info->name[0] != '\0' ? task_info->name
> -						   : _("<no name>"));
> +      uiout->field_string ("name",
> +			   task_info->name[0] != '\0' ? task_info->name
> +			   : _("<no name>"));
>  
>        uiout->text ("\n");
>      }
> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
> index 8422db8b571..5cdebddfc5d 100644
> --- a/gdb/breakpoint.c
> +++ b/gdb/breakpoint.c
> @@ -4490,13 +4490,13 @@ maybe_print_thread_hit_breakpoint (struct ui_out *uiout)
>        struct thread_info *thr = inferior_thread ();
>  
>        uiout->text ("Thread ");
> -      uiout->field_fmt ("thread-id", "%s", print_thread_id (thr));
> +      uiout->field_string ("thread-id", print_thread_id (thr));
>  
>        name = thr->name != NULL ? thr->name : target_thread_name (thr);
>        if (name != NULL)
>  	{
>  	  uiout->text (" \"");
> -	  uiout->field_fmt ("name", "%s", name);
> +	  uiout->field_string ("name", name);
>  	  uiout->text ("\"");
>  	}
>  
> diff --git a/gdb/i386-tdep.c b/gdb/i386-tdep.c
> index 00c1f8d7499..42fb8b26463 100644
> --- a/gdb/i386-tdep.c
> +++ b/gdb/i386-tdep.c
> @@ -8914,7 +8914,7 @@ i386_mpx_print_bounds (const CORE_ADDR bt_entry[4])
>  
>        size = (size > -1 ? size + 1 : size);
>        uiout->text (", size = ");
> -      uiout->field_fmt ("size", "%s", plongest (size));
> +      uiout->field_string ("size", plongest (size));
>  
>        uiout->text (", metadata = ");
>        uiout->field_core_addr ("metadata", gdbarch, bt_entry[3]);
> diff --git a/gdb/infrun.c b/gdb/infrun.c
> index 4fd92f1bac2..f1f10fd30ff 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -7690,13 +7690,13 @@ print_signal_received_reason (struct ui_out *uiout, enum gdb_signal siggnal)
>        const char *name;
>  
>        uiout->text ("\nThread ");
> -      uiout->field_fmt ("thread-id", "%s", print_thread_id (thr));
> +      uiout->field_string ("thread-id", print_thread_id (thr));
>  
>        name = thr->name != NULL ? thr->name : target_thread_name (thr);
>        if (name != NULL)
>  	{
>  	  uiout->text (" \"");
> -	  uiout->field_fmt ("name", "%s", name);
> +	  uiout->field_string ("name", name);
>  	  uiout->text ("\"");
>  	}
>      }
> diff --git a/gdb/mi/mi-interp.c b/gdb/mi/mi-interp.c
> index ad1a06cae0b..76583ff494b 100644
> --- a/gdb/mi/mi-interp.c
> +++ b/gdb/mi/mi-interp.c
> @@ -1172,7 +1172,7 @@ mi_memory_changed (struct inferior *inferior, CORE_ADDR memaddr,
>  
>        mi_uiout->field_fmt ("thread-group", "i%d", inferior->num);
>        mi_uiout->field_core_addr ("addr", target_gdbarch (), memaddr);
> -      mi_uiout->field_fmt ("len", "%s", hex_string (len));
> +      mi_uiout->field_string ("len", hex_string (len));
>  
>        /* Append 'type=code' into notification if MEMADDR falls in the range of
>  	 sections contain code.  */
> diff --git a/gdb/mi/mi-main.c b/gdb/mi/mi-main.c
> index 13c310d494c..dc4feedb6f4 100644
> --- a/gdb/mi/mi-main.c
> +++ b/gdb/mi/mi-main.c
> @@ -746,7 +746,7 @@ list_available_thread_groups (const std::set<int> &ids, int recurse)
>  
>        ui_out_emit_tuple tuple_emitter (uiout, NULL);
>  
> -      uiout->field_fmt ("id", "%s", pid->c_str ());
> +      uiout->field_string ("id", pid->c_str ());
>        uiout->field_string ("type", "process");
>        if (cmd)
>  	uiout->field_string ("description", cmd->c_str ());
> diff --git a/gdb/spu-tdep.c b/gdb/spu-tdep.c
> index a2ac3149d4d..3ac709c1611 100644
> --- a/gdb/spu-tdep.c
> +++ b/gdb/spu-tdep.c
> @@ -2096,10 +2096,10 @@ info_spu_event_command (const char *args, int from_tty)
>    ui_out_emit_tuple tuple_emitter (current_uiout, "SPUInfoEvent");
>  
>    current_uiout->text (_("Event Status "));
> -  current_uiout->field_fmt ("event_status", "0x%s", phex (event_status, 4));
> +  current_uiout->field_string ("event_status", phex (event_status, 4));
>    current_uiout->text ("\n");
>    current_uiout->text (_("Event Mask   "));
> -  current_uiout->field_fmt ("event_mask", "0x%s", phex (event_mask, 4));
> +  current_uiout->field_string ("event_mask", phex (event_mask, 4));
>    current_uiout->text ("\n");
>  }
>  
> @@ -2168,10 +2168,10 @@ info_spu_signal_command (const char *args, int from_tty)
>    if (current_uiout->is_mi_like_p ())
>      {
>        current_uiout->field_int ("signal1_pending", signal1_pending);
> -      current_uiout->field_fmt ("signal1", "0x%s", phex_nz (signal1, 4));
> +      current_uiout->field_string ("signal1", phex (signal1, 4));
>        current_uiout->field_int ("signal1_type", signal1_type);
>        current_uiout->field_int ("signal2_pending", signal2_pending);
> -      current_uiout->field_fmt ("signal2", "0x%s", phex_nz (signal2, 4));
> +      current_uiout->field_string ("signal2", phex (signal2, 4));
>        current_uiout->field_int ("signal2_type", signal2_type);
>      }
>    else
> @@ -2218,7 +2218,7 @@ info_spu_mailbox_list (gdb_byte *buf, int nr, enum bfd_endian byte_order,
>  	ULONGEST val;
>  	ui_out_emit_tuple tuple_emitter (current_uiout, "mbox");
>  	val = extract_unsigned_integer (buf + 4*i, 4, byte_order);
> -	current_uiout->field_fmt (field, "0x%s", phex (val, 4));
> +	current_uiout->field_string (field, phex (val, 4));
>        }
>  
>        current_uiout->text ("\n");
> @@ -2421,7 +2421,7 @@ info_spu_dma_cmdlist (gdb_byte *buf, int nr, enum bfd_endian byte_order)
>  	current_uiout->field_int ("rid", rclass_id);
>  
>  	if (ea_valid_p)
> -	  current_uiout->field_fmt ("ea", "0x%s", phex (mfc_ea, 8));
> +	  current_uiout->field_string ("ea", phex (mfc_ea, 8));
>  	else
>  	  current_uiout->field_skip ("ea");
>  
> @@ -2494,16 +2494,16 @@ info_spu_dma_command (const char *args, int from_tty)
>  
>    if (current_uiout->is_mi_like_p ())
>      {
> -      current_uiout->field_fmt ("dma_info_type", "0x%s",
> -				phex_nz (dma_info_type, 4));
> -      current_uiout->field_fmt ("dma_info_mask", "0x%s",
> -				phex_nz (dma_info_mask, 4));
> -      current_uiout->field_fmt ("dma_info_status", "0x%s",
> -				phex_nz (dma_info_status, 4));
> -      current_uiout->field_fmt ("dma_info_stall_and_notify", "0x%s",
> -				phex_nz (dma_info_stall_and_notify, 4));
> -      current_uiout->field_fmt ("dma_info_atomic_command_status", "0x%s",
> -				phex_nz (dma_info_atomic_command_status, 4));
> +      current_uiout->field_string ("dma_info_type",
> +				   phex (dma_info_type, 4));
> +      current_uiout->field_string ("dma_info_mask",
> +				   phex (dma_info_mask, 4));
> +      current_uiout->field_string ("dma_info_status",
> +				   phex (dma_info_status, 4));
> +      current_uiout->field_string ("dma_info_stall_and_notify",
> +				   phex (dma_info_stall_and_notify, 4));
> +      current_uiout->field_string ("dma_info_atomic_command_status",
> +				   phex (dma_info_atomic_command_status, 4));
>      }
>    else
>      {
> @@ -2564,12 +2564,12 @@ info_spu_proxydma_command (const char *args, int from_tty)
>  
>    if (current_uiout->is_mi_like_p ())
>      {
> -      current_uiout->field_fmt ("proxydma_info_type", "0x%s",
> -				phex_nz (dma_info_type, 4));
> -      current_uiout->field_fmt ("proxydma_info_mask", "0x%s",
> -				phex_nz (dma_info_mask, 4));
> -      current_uiout->field_fmt ("proxydma_info_status", "0x%s",
> -				phex_nz (dma_info_status, 4));
> +      current_uiout->field_string ("proxydma_info_type",
> +				   phex (dma_info_type, 4));
> +      current_uiout->field_string ("proxydma_info_mask",
> +				   phex (dma_info_mask, 4));
> +      current_uiout->field_string ("proxydma_info_status",
> +				   phex (dma_info_status, 4));
>      }
>    else
>      {
> diff --git a/gdb/target.c b/gdb/target.c
> index febb3390339..417b795d47f 100644
> --- a/gdb/target.c
> +++ b/gdb/target.c
> @@ -3795,7 +3795,7 @@ flash_erase_command (const char *cmd, int from_tty)
>            current_uiout->message (_("Erasing flash memory region at address "));
>            current_uiout->field_core_addr ("address", gdbarch, m.lo);
>            current_uiout->message (", size = ");
> -          current_uiout->field_fmt ("size", "%s", hex_string (m.hi - m.lo));
> +          current_uiout->field_string ("size", hex_string (m.hi - m.lo));
>            current_uiout->message ("\n");
>          }
>      }
> -- 
> 2.20.1
> 

Re: [PATCH 1/3] Use field_core_addr in more places

[Andrew Burgess]

* Tom Tromey <tromey@adacore.com> [2019-07-01 14:00:56 -0600]:

> This changes a few spots that use field_fmt to use field_core_addr
> instead.
> 
> gdb/ChangeLog
> 2019-07-01  Tom Tromey  <tromey@adacore.com>
> 
> 	* target.c (flash_erase_command): Use field_core_addr.
> 	* symfile.c (generic_load): Use field_core_addr.
> 	* sparc64-linux-tdep.c (sparc64_linux_handle_segmentation_fault):
> 	Use field_core_addr.
> 	* i386-linux-tdep.c (i386_linux_handle_segmentation_fault): Use
> 	field_core_addr.

Looks good to me.

Thanks,
Andrew
> ---
>  gdb/ChangeLog            | 9 +++++++++
>  gdb/i386-linux-tdep.c    | 6 +++---
>  gdb/sparc64-linux-tdep.c | 6 +++---
>  gdb/symfile.c            | 2 +-
>  gdb/target.c             | 2 +-
>  5 files changed, 17 insertions(+), 8 deletions(-)
> 
> diff --git a/gdb/i386-linux-tdep.c b/gdb/i386-linux-tdep.c
> index 74f429b49af..b5c3ef2b71e 100644
> --- a/gdb/i386-linux-tdep.c
> +++ b/gdb/i386-linux-tdep.c
> @@ -432,13 +432,13 @@ i386_linux_handle_segmentation_fault (struct gdbarch *gdbarch,
>      uiout->field_string ("sigcode-meaning", _("Lower bound violation"));
>  
>    uiout->text (_(" while accessing address "));
> -  uiout->field_fmt ("bound-access", "%s", paddress (gdbarch, access));
> +  uiout->field_core_addr ("bound-access", gdbarch, access);
>  
>    uiout->text (_("\nBounds: [lower = "));
> -  uiout->field_fmt ("lower-bound", "%s", paddress (gdbarch, lower_bound));
> +  uiout->field_core_addr ("lower-bound", gdbarch, lower_bound);
>  
>    uiout->text (_(", upper = "));
> -  uiout->field_fmt ("upper-bound", "%s", paddress (gdbarch, upper_bound));
> +  uiout->field_core_addr ("upper-bound", gdbarch, upper_bound);
>  
>    uiout->text (_("]"));
>  }
> diff --git a/gdb/sparc64-linux-tdep.c b/gdb/sparc64-linux-tdep.c
> index 158db97ec61..563d21e4b4c 100644
> --- a/gdb/sparc64-linux-tdep.c
> +++ b/gdb/sparc64-linux-tdep.c
> @@ -150,19 +150,19 @@ sparc64_linux_handle_segmentation_fault (struct gdbarch *gdbarch,
>        uiout->text ("\n");
>        uiout->field_string ("sigcode-meaning", _("ADI disabled"));
>        uiout->text (_(" while accessing address "));
> -      uiout->field_fmt ("bound-access", "%s", paddress (gdbarch, addr));
> +      uiout->field_core_addr ("bound-access", gdbarch, addr);
>        break;
>      case SEGV_ADIDERR:	/* disrupting mismatch */
>        uiout->text ("\n");
>        uiout->field_string ("sigcode-meaning", _("ADI deferred mismatch"));
>        uiout->text (_(" while accessing address "));
> -      uiout->field_fmt ("bound-access", "%s", paddress (gdbarch, addr));
> +      uiout->field_core_addr ("bound-access", gdbarch, addr);
>        break;
>      case SEGV_ADIPERR:	/* precise mismatch */
>        uiout->text ("\n");
>        uiout->field_string ("sigcode-meaning", _("ADI precise mismatch"));
>        uiout->text (_(" while accessing address "));
> -      uiout->field_fmt ("bound-access", "%s", paddress (gdbarch, addr));
> +      uiout->field_core_addr ("bound-access", gdbarch, addr);
>        break;
>      default:
>        break;
> diff --git a/gdb/symfile.c b/gdb/symfile.c
> index 6f9c81c8b94..58049a544b5 100644
> --- a/gdb/symfile.c
> +++ b/gdb/symfile.c
> @@ -2081,7 +2081,7 @@ generic_load (const char *args, int from_tty)
>    CORE_ADDR entry = bfd_get_start_address (loadfile_bfd.get ());
>    entry = gdbarch_addr_bits_remove (target_gdbarch (), entry);
>    uiout->text ("Start address ");
> -  uiout->field_fmt ("address", "%s", paddress (target_gdbarch (), entry));
> +  uiout->field_core_addr ("address", target_gdbarch (), entry);
>    uiout->text (", load size ");
>    uiout->field_fmt ("load-size", "%lu", total_progress.data_count);
>    uiout->text ("\n");
> diff --git a/gdb/target.c b/gdb/target.c
> index 1c04095fd63..febb3390339 100644
> --- a/gdb/target.c
> +++ b/gdb/target.c
> @@ -3793,7 +3793,7 @@ flash_erase_command (const char *cmd, int from_tty)
>  	  ui_out_emit_tuple tuple_emitter (current_uiout, "erased-regions");
>  
>            current_uiout->message (_("Erasing flash memory region at address "));
> -          current_uiout->field_fmt ("address", "%s", paddress (gdbarch, m.lo));
> +          current_uiout->field_core_addr ("address", gdbarch, m.lo);
>            current_uiout->message (", size = ");
>            current_uiout->field_fmt ("size", "%s", hex_string (m.hi - m.lo));
>            current_uiout->message ("\n");
> -- 
> 2.20.1
> 

Re: [PATCH 3/3] Introduce field_unsigned

[Tom Tromey]

>> +  void field_unsigned (const char *fldname, ULONGEST value)
>> +  {
>> +    field_string (fldname, pulongest (value));
>> +  }

[...]

>> This is quite different from how other types are handled, e.g.

>> ui_out::field_int (const char *fldname, int value)
>> {
>>   int fldno;
>>   int width;
>>   ui_align align;

>>   verify_field (&fldno, &width, &align);

>>   do_field_int (fldno, width, align, fldname, value);
>> }

>> Is there a reason for not treating int and unsigned int in a similar way?

There just didn't seem to be a benefit, and the field_int approach is
wordier.

In the end all the existing do_field_int methods actually just end up in
their respective do_field_string methods anyway.  E.g., mi-out.c:

    void
    mi_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? */

      xsnprintf (buffer, sizeof (buffer), "%d", value);
      do_field_string (fldno, width, alignment, fldname, buffer,
                       ui_out_style_kind::DEFAULT);
    }

Here the new approach is actually a bit better because pulongest doesn't
have this FIXME.  Perhaps we should instead switch field_int to take a
LONGEST and follow the approach used in this new patch.

Tom

Re: [PATCH 2/3] Use field_string in more places

[Tom Tromey]

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

Andrew> I don't think this is correct, at least experimenting on HEAD for me
Andrew> doesn't print it twice.

Aha, thanks.  I will update everything.

I think I always get confused by the naming here and think that
"phex_nz" implies "no 0x", but it really doesn't.

Tom

RE: [PATCH 3/3] Introduce field_unsigned

[Metzger, Markus T]

Hello Tom,

> In the end all the existing do_field_int methods actually just end up in their
> respective do_field_string methods anyway.  E.g., mi-out.c:
> 
>     void
>     mi_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? */
> 
>       xsnprintf (buffer, sizeof (buffer), "%d", value);
>       do_field_string (fldno, width, alignment, fldname, buffer,
>                        ui_out_style_kind::DEFAULT);
>     }
> 
> Here the new approach is actually a bit better because pulongest doesn't have this
> FIXME.  Perhaps we should instead switch field_int to take a LONGEST and follow
> the approach used in this new patch.

If subclasses don't need to know that the field was originally an integer or a float,
all of ui_out can be reduced to outputting strings and sub-classes need only provide
that one member function for all field types.

I don't have any preference one way or another, I was mainly commenting on the
apparent discrepancy how similar field types are handled.

Markus.

Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Christin Eisenschmid, Gary Kershaw
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

Re: [PATCH 3/3] Introduce field_unsigned

[Tom Tromey]

>> If subclasses don't need to know that the field was originally an integer or a float,
>> all of ui_out can be reduced to outputting strings and sub-classes need only provide
>> that one member function for all field types.

>> I don't have any preference one way or another, I was mainly commenting on the
>> apparent discrepancy how similar field types are handled.

I suppose if we ever want to, say, switch MI to use JSON, then
preserving the difference would make sense.  I'll change it.

Tom