[RFA,2/7] Remove make_cleanup_ui_out_table_begin_end

Message ID 20170909153540.15008-3-tom@tromey.com
State New, archived
Headers

Commit Message

Tom Tromey Sept. 9, 2017, 3:35 p.m. UTC
  This changes the few remaining uses of
make_cleanup_ui_out_table_begin_end to use ui_out_emit_table instead,
and then removes the cleanup.

ChangeLog
2017-09-09  Tom Tromey  <tom@tromey.com>

	* ui-out.h (make_cleanup_ui_out_table_begin_end): Remove.
	(class ui_out_emit_table): Update comment.
	* ui-out.c (do_cleanup_table_end)
	(make_cleanup_ui_out_table_begin_end): Remove.
	* spu-tdep.c (info_spu_mailbox_list): Use ui_out_emit_table.
	(info_spu_dma_cmdlist): Likewise.
	* probe.c (info_probes_for_ops): Use ui_out_emit_table.
	* darwin-nat-info.c (darwin_debug_regions_recurse): Use
	ui_out_emit_table.
---
 gdb/ChangeLog         |  12 ++++
 gdb/darwin-nat-info.c |  49 +++++++--------
 gdb/probe.c           | 170 +++++++++++++++++++++++++-------------------------
 gdb/spu-tdep.c        |  90 ++++++++++++--------------
 gdb/ui-out.c          |  16 -----
 gdb/ui-out.h          |   8 +--
 6 files changed, 164 insertions(+), 181 deletions(-)
  

Comments

Tom Tromey Sept. 9, 2017, 4:02 p.m. UTC | #1
I forgot to mention something I had meant to mention when submitting
this series:

Tom>	* spu-tdep.c (info_spu_mailbox_list): Use ui_out_emit_table.
Tom>	(info_spu_dma_cmdlist): Likewise.
[..]
Tom>	* darwin-nat-info.c (darwin_debug_regions_recurse): Use
Tom>	ui_out_emit_table.

I don't have any way to test these, or even compile the darwin patch.
So this requires extra careful review at least, or someone to volunteer
to try them out.

It would be very nice to have a darwin builder hooked up to the
buildbot.  I'm not sure how practical this is ... there are so many Macs
around Moz that maybe I could acquire one somehow, but I don't know
that I could host it anywhere.

Tom
  
Simon Marchi Sept. 9, 2017, 6:22 p.m. UTC | #2
On 2017-09-09 18:02, Tom Tromey wrote:
> I forgot to mention something I had meant to mention when submitting
> this series:
> 
> Tom>	* spu-tdep.c (info_spu_mailbox_list): Use ui_out_emit_table.
> Tom>	(info_spu_dma_cmdlist): Likewise.
> [..]
> Tom>	* darwin-nat-info.c (darwin_debug_regions_recurse): Use
> Tom>	ui_out_emit_table.
> 
> I don't have any way to test these, or even compile the darwin patch.
> So this requires extra careful review at least, or someone to volunteer
> to try them out.
> 
> It would be very nice to have a darwin builder hooked up to the
> buildbot.  I'm not sure how practical this is ... there are so many 
> Macs
> around Moz that maybe I could acquire one somehow, but I don't know
> that I could host it anywhere.

The patch LGTM.

I built-tested your series on a Mac, it builds fine (if we ignore all 
the current hurdles to get it to build).  Now when I try actually 
running, I get this:

(gdb) r
Starting program: /Users/simark/build/binutils-gdb/gdb/test_clang
[New Thread 0x1503 of process 79554]
[New Thread 0x1803 of process 79554]
Warning:
Cannot insert breakpoint -1.
Cannot access memory at address 0xeb19

Command aborted.

But it also does that before your patch.

Simon
  

Patch

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index b37089e..4c9c419 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,17 @@ 
 2017-09-09  Tom Tromey  <tom@tromey.com>
 
+	* ui-out.h (make_cleanup_ui_out_table_begin_end): Remove.
+	(class ui_out_emit_table): Update comment.
+	* ui-out.c (do_cleanup_table_end)
+	(make_cleanup_ui_out_table_begin_end): Remove.
+	* spu-tdep.c (info_spu_mailbox_list): Use ui_out_emit_table.
+	(info_spu_dma_cmdlist): Likewise.
+	* probe.c (info_probes_for_ops): Use ui_out_emit_table.
+	* darwin-nat-info.c (darwin_debug_regions_recurse): Use
+	ui_out_emit_table.
+
+2017-09-09  Tom Tromey  <tom@tromey.com>
+
 	* thread.c (print_thread_info_1): Use ui_out_emit_table,
 	ui_out_emit_list, gdb::optional.
 
diff --git a/gdb/darwin-nat-info.c b/gdb/darwin-nat-info.c
index 91bc5b3..8951789 100644
--- a/gdb/darwin-nat-info.c
+++ b/gdb/darwin-nat-info.c
@@ -617,10 +617,9 @@  darwin_debug_regions_recurse (task_t task)
   vm_region_submap_short_info_data_64_t r_info;
   kern_return_t kret;
   int ret;
-  struct cleanup *table_chain;
   struct ui_out *uiout = current_uiout;
 
-  table_chain = make_cleanup_ui_out_table_begin_end (uiout, 9, -1, "regions");
+  ui_out_emit_table table_emitter (uiout, 9, -1, "regions");
 
   if (gdbarch_addr_bit (target_gdbarch ()) <= 32)
     {
@@ -647,7 +646,6 @@  darwin_debug_regions_recurse (task_t task)
   while (1)
     {
       const char *tag;
-      struct cleanup *row_chain;
 
       r_info_size = VM_REGION_SUBMAP_SHORT_INFO_COUNT_64;
       r_size = -1;
@@ -656,28 +654,29 @@  darwin_debug_regions_recurse (task_t task)
 				     &r_info_size);
       if (kret != KERN_SUCCESS)
 	break;
-      row_chain = make_cleanup_ui_out_tuple_begin_end (uiout, "regions-row");
-
-      uiout->field_core_addr ("start", target_gdbarch (), r_start);
-      uiout->field_core_addr ("end", target_gdbarch (), r_start + r_size);
-      uiout->field_string ("min-prot",
-			   unparse_protection (r_info.protection));
-      uiout->field_string ("max-prot",
-			   unparse_protection (r_info.max_protection));
-      uiout->field_string ("inheritence",
-			   unparse_inheritance (r_info.inheritance));
-      uiout->field_string ("share-mode",
-			   unparse_share_mode (r_info.share_mode));
-      uiout->field_int ("depth", r_depth);
-      uiout->field_string ("submap",
-			   r_info.is_submap ? _("sm ") : _("obj"));
-      tag = unparse_user_tag (r_info.user_tag);
-      if (tag)
-	uiout->field_string ("tag", tag);
-      else
-	uiout->field_int ("tag", r_info.user_tag);
 
-      do_cleanups (row_chain);
+      {
+	ui_out_emit_tuple tuple_emitter (uiout, "regions-row");
+
+	uiout->field_core_addr ("start", target_gdbarch (), r_start);
+	uiout->field_core_addr ("end", target_gdbarch (), r_start + r_size);
+	uiout->field_string ("min-prot",
+			     unparse_protection (r_info.protection));
+	uiout->field_string ("max-prot",
+			     unparse_protection (r_info.max_protection));
+	uiout->field_string ("inheritence",
+			     unparse_inheritance (r_info.inheritance));
+	uiout->field_string ("share-mode",
+			     unparse_share_mode (r_info.share_mode));
+	uiout->field_int ("depth", r_depth);
+	uiout->field_string ("submap",
+			     r_info.is_submap ? _("sm ") : _("obj"));
+	tag = unparse_user_tag (r_info.user_tag);
+	if (tag)
+	  uiout->field_string ("tag", tag);
+	else
+	  uiout->field_int ("tag", r_info.user_tag);
+      }
 
       if (!uiout->is_mi_like_p ())
 	uiout->text ("\n");
@@ -687,8 +686,6 @@  darwin_debug_regions_recurse (task_t task)
       else
 	r_start += r_size;
     }
-  do_cleanups (table_chain);
-
 }
 
 
diff --git a/gdb/probe.c b/gdb/probe.c
index ce28361..a2e204a 100644
--- a/gdb/probe.c
+++ b/gdb/probe.c
@@ -616,90 +616,92 @@  info_probes_for_ops (const char *arg, int from_tty,
   else
     ui_out_extra_fields = get_number_extra_fields (pops);
 
-  make_cleanup_ui_out_table_begin_end (current_uiout,
-				       5 + ui_out_extra_fields,
-				       VEC_length (bound_probe_s, probes),
-				       "StaticProbes");
-
-  if (!VEC_empty (bound_probe_s, probes))
-    qsort (VEC_address (bound_probe_s, probes),
-	   VEC_length (bound_probe_s, probes),
-	   sizeof (bound_probe_s), compare_probes);
-
-  /* What's the size of an address in our architecture?  */
-  size_addr = gdbarch_addr_bit (gdbarch) == 64 ? 18 : 10;
-
-  /* Determining the maximum size of each field (`type', `provider',
-     `name' and `objname').  */
-  for (i = 0; VEC_iterate (bound_probe_s, probes, i, probe); ++i)
-    {
-      const char *probe_type = probe->probe->pops->type_name (probe->probe);
-
-      size_type = std::max (strlen (probe_type), size_type);
-      size_name = std::max (strlen (probe->probe->name), size_name);
-      size_provider = std::max (strlen (probe->probe->provider), size_provider);
-      size_objname = std::max (strlen (objfile_name (probe->objfile)),
-			       size_objname);
-    }
-
-  current_uiout->table_header (size_type, ui_left, "type", _("Type"));
-  current_uiout->table_header (size_provider, ui_left, "provider",
-			       _("Provider"));
-  current_uiout->table_header (size_name, ui_left, "name", _("Name"));
-  current_uiout->table_header (size_addr, ui_left, "addr", _("Where"));
-
-  if (pops == NULL)
-    {
-      const struct probe_ops *po;
-      int ix;
-
-      /* We have to generate the table header for each new probe type
-	 that we will print.  Note that this excludes probe types not
-	 having any defined probe with the search criteria.  */
-      for (ix = 0; VEC_iterate (probe_ops_cp, all_probe_ops, ix, po); ++ix)
-	if (exists_probe_with_pops (probes, po))
-	  gen_ui_out_table_header_info (probes, po);
-    }
-  else
-    gen_ui_out_table_header_info (probes, pops);
-
-  current_uiout->table_header (size_objname, ui_left, "object", _("Object"));
-  current_uiout->table_body ();
-
-  for (i = 0; VEC_iterate (bound_probe_s, probes, i, probe); ++i)
-    {
-      const char *probe_type = probe->probe->pops->type_name (probe->probe);
-
-      ui_out_emit_tuple tuple_emitter (current_uiout, "probe");
-
-      current_uiout->field_string ("type",probe_type);
-      current_uiout->field_string ("provider", probe->probe->provider);
-      current_uiout->field_string ("name", probe->probe->name);
-      current_uiout->field_core_addr (
-	"addr", probe->probe->arch,
-	get_probe_address (probe->probe, probe->objfile));
-
-      if (pops == NULL)
-	{
-	  const struct probe_ops *po;
-	  int ix;
-
-	  for (ix = 0; VEC_iterate (probe_ops_cp, all_probe_ops, ix, po);
-	       ++ix)
-	    if (probe->probe->pops == po)
-	      print_ui_out_info (probe->probe);
-	    else if (exists_probe_with_pops (probes, po))
-	      print_ui_out_not_applicables (po);
-	}
-      else
-	print_ui_out_info (probe->probe);
-
-      current_uiout->field_string ("object",
-			   objfile_name (probe->objfile));
-      current_uiout->text ("\n");
-    }
-
-  any_found = !VEC_empty (bound_probe_s, probes);
+  {
+    ui_out_emit_table table_emitter (current_uiout,
+				     5 + ui_out_extra_fields,
+				     VEC_length (bound_probe_s, probes),
+				     "StaticProbes");
+
+    if (!VEC_empty (bound_probe_s, probes))
+      qsort (VEC_address (bound_probe_s, probes),
+	     VEC_length (bound_probe_s, probes),
+	     sizeof (bound_probe_s), compare_probes);
+
+    /* What's the size of an address in our architecture?  */
+    size_addr = gdbarch_addr_bit (gdbarch) == 64 ? 18 : 10;
+
+    /* Determining the maximum size of each field (`type', `provider',
+       `name' and `objname').  */
+    for (i = 0; VEC_iterate (bound_probe_s, probes, i, probe); ++i)
+      {
+	const char *probe_type = probe->probe->pops->type_name (probe->probe);
+
+	size_type = std::max (strlen (probe_type), size_type);
+	size_name = std::max (strlen (probe->probe->name), size_name);
+	size_provider = std::max (strlen (probe->probe->provider), size_provider);
+	size_objname = std::max (strlen (objfile_name (probe->objfile)),
+				 size_objname);
+      }
+
+    current_uiout->table_header (size_type, ui_left, "type", _("Type"));
+    current_uiout->table_header (size_provider, ui_left, "provider",
+				 _("Provider"));
+    current_uiout->table_header (size_name, ui_left, "name", _("Name"));
+    current_uiout->table_header (size_addr, ui_left, "addr", _("Where"));
+
+    if (pops == NULL)
+      {
+	const struct probe_ops *po;
+	int ix;
+
+	/* We have to generate the table header for each new probe type
+	   that we will print.  Note that this excludes probe types not
+	   having any defined probe with the search criteria.  */
+	for (ix = 0; VEC_iterate (probe_ops_cp, all_probe_ops, ix, po); ++ix)
+	  if (exists_probe_with_pops (probes, po))
+	    gen_ui_out_table_header_info (probes, po);
+      }
+    else
+      gen_ui_out_table_header_info (probes, pops);
+
+    current_uiout->table_header (size_objname, ui_left, "object", _("Object"));
+    current_uiout->table_body ();
+
+    for (i = 0; VEC_iterate (bound_probe_s, probes, i, probe); ++i)
+      {
+	const char *probe_type = probe->probe->pops->type_name (probe->probe);
+
+	ui_out_emit_tuple tuple_emitter (current_uiout, "probe");
+
+	current_uiout->field_string ("type",probe_type);
+	current_uiout->field_string ("provider", probe->probe->provider);
+	current_uiout->field_string ("name", probe->probe->name);
+	current_uiout->field_core_addr (
+					"addr", probe->probe->arch,
+					get_probe_address (probe->probe, probe->objfile));
+
+	if (pops == NULL)
+	  {
+	    const struct probe_ops *po;
+	    int ix;
+
+	    for (ix = 0; VEC_iterate (probe_ops_cp, all_probe_ops, ix, po);
+		 ++ix)
+	      if (probe->probe->pops == po)
+		print_ui_out_info (probe->probe);
+	      else if (exists_probe_with_pops (probes, po))
+		print_ui_out_not_applicables (po);
+	  }
+	else
+	  print_ui_out_info (probe->probe);
+
+	current_uiout->field_string ("object",
+				     objfile_name (probe->objfile));
+	current_uiout->text ("\n");
+      }
+
+    any_found = !VEC_empty (bound_probe_s, probes);
+  }
   do_cleanups (cleanup);
 
   if (!any_found)
diff --git a/gdb/spu-tdep.c b/gdb/spu-tdep.c
index 59c51b2..221f6de 100644
--- a/gdb/spu-tdep.c
+++ b/gdb/spu-tdep.c
@@ -2209,31 +2209,28 @@  static void
 info_spu_mailbox_list (gdb_byte *buf, int nr, enum bfd_endian byte_order,
 		       const char *field, const char *msg)
 {
-  struct cleanup *chain;
   int i;
 
   if (nr <= 0)
     return;
 
-  chain = make_cleanup_ui_out_table_begin_end (current_uiout, 1, nr, "mbox");
+  ui_out_emit_table table_emitter (current_uiout, 1, nr, "mbox");
 
   current_uiout->table_header (32, ui_left, field, msg);
   current_uiout->table_body ();
 
   for (i = 0; i < nr; i++)
     {
-      struct cleanup *val_chain;
-      ULONGEST val;
-      val_chain = make_cleanup_ui_out_tuple_begin_end (current_uiout, "mbox");
-      val = extract_unsigned_integer (buf + 4*i, 4, byte_order);
-      current_uiout->field_fmt (field, "0x%s", phex (val, 4));
-      do_cleanups (val_chain);
+      {
+	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));
+      }
 
       if (!current_uiout->is_mi_like_p ())
 	printf_filtered ("\n");
     }
-
-  do_cleanups (chain);
 }
 
 static void
@@ -2333,7 +2330,6 @@  info_spu_dma_cmdlist (gdb_byte *buf, int nr, enum bfd_endian byte_order)
 
   int *seq = XALLOCAVEC (int, nr);
   int done = 0;
-  struct cleanup *chain;
   int i, j;
 
 
@@ -2371,8 +2367,7 @@  info_spu_dma_cmdlist (gdb_byte *buf, int nr, enum bfd_endian byte_order)
   nr = i;
 
 
-  chain = make_cleanup_ui_out_table_begin_end (current_uiout, 10, nr,
-					       "dma_cmd");
+  ui_out_emit_table table_emitter (current_uiout, 10, nr, "dma_cmd");
 
   current_uiout->table_header (7, ui_left, "opcode", "Opcode");
   current_uiout->table_header (3, ui_left, "tag", "Tag");
@@ -2389,7 +2384,6 @@  info_spu_dma_cmdlist (gdb_byte *buf, int nr, enum bfd_endian byte_order)
 
   for (i = 0; i < nr; i++)
     {
-      struct cleanup *cmd_chain;
       ULONGEST mfc_cq_dw0;
       ULONGEST mfc_cq_dw1;
       ULONGEST mfc_cq_dw2;
@@ -2425,51 +2419,49 @@  info_spu_dma_cmdlist (gdb_byte *buf, int nr, enum bfd_endian byte_order)
       ea_valid_p = spu_mfc_get_bitfield (mfc_cq_dw2, 39, 39);
       cmd_error_p = spu_mfc_get_bitfield (mfc_cq_dw2, 40, 40);
 
-      cmd_chain = make_cleanup_ui_out_tuple_begin_end (current_uiout, "cmd");
-
-      if (spu_mfc_opcode[mfc_cmd_opcode])
-	current_uiout->field_string ("opcode", spu_mfc_opcode[mfc_cmd_opcode]);
-      else
-	current_uiout->field_int ("opcode", mfc_cmd_opcode);
+      {
+	ui_out_emit_tuple tuple_emitter (current_uiout, "cmd");
 
-      current_uiout->field_int ("tag", mfc_cmd_tag);
-      current_uiout->field_int ("tid", tclass_id);
-      current_uiout->field_int ("rid", rclass_id);
+	if (spu_mfc_opcode[mfc_cmd_opcode])
+	  current_uiout->field_string ("opcode", spu_mfc_opcode[mfc_cmd_opcode]);
+	else
+	  current_uiout->field_int ("opcode", mfc_cmd_opcode);
 
-      if (ea_valid_p)
-	current_uiout->field_fmt ("ea", "0x%s", phex (mfc_ea, 8));
-      else
-	current_uiout->field_skip ("ea");
+	current_uiout->field_int ("tag", mfc_cmd_tag);
+	current_uiout->field_int ("tid", tclass_id);
+	current_uiout->field_int ("rid", rclass_id);
 
-      current_uiout->field_fmt ("lsa", "0x%05x", mfc_lsa << 4);
-      if (qw_valid_p)
-	current_uiout->field_fmt ("size", "0x%05x", mfc_size << 4);
-      else
-	current_uiout->field_fmt ("size", "0x%05x", mfc_size);
+	if (ea_valid_p)
+	  current_uiout->field_fmt ("ea", "0x%s", phex (mfc_ea, 8));
+	else
+	  current_uiout->field_skip ("ea");
 
-      if (list_valid_p)
-	{
-	  current_uiout->field_fmt ("lstaddr", "0x%05x", list_lsa << 3);
-	  current_uiout->field_fmt ("lstsize", "0x%05x", list_size << 3);
-	}
-      else
-	{
-	  current_uiout->field_skip ("lstaddr");
-	  current_uiout->field_skip ("lstsize");
-	}
+	current_uiout->field_fmt ("lsa", "0x%05x", mfc_lsa << 4);
+	if (qw_valid_p)
+	  current_uiout->field_fmt ("size", "0x%05x", mfc_size << 4);
+	else
+	  current_uiout->field_fmt ("size", "0x%05x", mfc_size);
 
-      if (cmd_error_p)
-	current_uiout->field_string ("error_p", "*");
-      else
-	current_uiout->field_skip ("error_p");
+	if (list_valid_p)
+	  {
+	    current_uiout->field_fmt ("lstaddr", "0x%05x", list_lsa << 3);
+	    current_uiout->field_fmt ("lstsize", "0x%05x", list_size << 3);
+	  }
+	else
+	  {
+	    current_uiout->field_skip ("lstaddr");
+	    current_uiout->field_skip ("lstsize");
+	  }
 
-      do_cleanups (cmd_chain);
+	if (cmd_error_p)
+	  current_uiout->field_string ("error_p", "*");
+	else
+	  current_uiout->field_skip ("error_p");
+      }
 
       if (!current_uiout->is_mi_like_p ())
 	printf_filtered ("\n");
     }
-
-  do_cleanups (chain);
 }
 
 static void
diff --git a/gdb/ui-out.c b/gdb/ui-out.c
index 9c27742..97207f5 100644
--- a/gdb/ui-out.c
+++ b/gdb/ui-out.c
@@ -400,22 +400,6 @@  ui_out::table_end ()
   m_table_up = nullptr;
 }
 
-static void
-do_cleanup_table_end (void *data)
-{
-  ui_out *uiout = (ui_out *) data;
-
-  uiout->table_end ();
-}
-
-struct cleanup *
-make_cleanup_ui_out_table_begin_end (ui_out *uiout, int nr_cols, int nr_rows,
-				     const char *tblid)
-{
-  uiout->table_begin (nr_cols, nr_rows, tblid);
-  return make_cleanup (do_cleanup_table_end, uiout);
-}
-
 void
 ui_out::begin (ui_out_type type, const char *id)
 {
diff --git a/gdb/ui-out.h b/gdb/ui-out.h
index 857e252..d983837 100644
--- a/gdb/ui-out.h
+++ b/gdb/ui-out.h
@@ -66,10 +66,6 @@  enum ui_out_type
     ui_out_type_list
   };
 
-extern struct cleanup *make_cleanup_ui_out_table_begin_end (struct ui_out *ui_out,
-                                                            int nr_cols,
-							    int nr_rows,
-							    const char *tblid);
 /* Compatibility wrappers.  */
 
 extern struct cleanup *make_cleanup_ui_out_list_begin_end (struct ui_out *uiout,
@@ -220,8 +216,8 @@  private:
 typedef ui_out_emit_type<ui_out_type_tuple> ui_out_emit_tuple;
 typedef ui_out_emit_type<ui_out_type_list> ui_out_emit_list;
 
-/* This is similar to make_cleanup_ui_out_table_begin_end, but written
-   as an RAII class.  */
+/* Start a new table on construction, and end the table on
+   destruction.  */
 class ui_out_emit_table
 {
 public: