[RFA 0/6] Remove some uses of is_mi_like_p

[RFA 0/6] Remove some uses of is_mi_like_p

[Tom Tromey]

I think in the long run it would be good to remove
ui_out::is_mi_like_p.  This series removes some unnecessary uses of
it.  This is a mild improvement because it means there will be less
temptation for people to copy bad examples from the existing code.


Relatedly, I had an idea for how to change cli_out so that most
(perhaps not all -- there are some tricky cases) uses of is_mi_like_p
could be removed, and also provide new features like user control over
display and colorizing.

The basic idea is first to realize that table names are fixed by the
MI spec, and so can be relied on by cli_out.  Then, whereever there is
a sequence involving textual output mixed with ui_out field output,
the whole sequence could be replaced with a format string that
references the field names.


Regression tested by the buildbot.

Tom

[RFA 3/6] Remove some uses of is_mi_like_p from spu-tdep.c

[Tom Tromey]

There were a few spots in spu-tdep.c where a use of is_mi_like_p was
not needed.

ChangeLog
2018-04-29  Tom Tromey  <tom@tromey.com>

	* spu-tdep.c (info_spu_mailbox_list, info_spu_dma_cmdlist)
	(info_spu_event_command): Remove some uses of is_mi_like_p.
---
 gdb/ChangeLog  |  5 +++++
 gdb/spu-tdep.c | 22 ++++++----------------
 2 files changed, 11 insertions(+), 16 deletions(-)

diff --git a/gdb/spu-tdep.c b/gdb/spu-tdep.c
index 783ef5b247..772ec7339d 100644
--- a/gdb/spu-tdep.c
+++ b/gdb/spu-tdep.c
@@ -2096,18 +2096,10 @@ info_spu_event_command (const char *args, int from_tty)
  
   ui_out_emit_tuple tuple_emitter (current_uiout, "SPUInfoEvent");
 
-  if (current_uiout->is_mi_like_p ())
-    {
-      current_uiout->field_fmt ("event_status",
-				"0x%s", phex_nz (event_status, 4));
-      current_uiout->field_fmt ("event_mask",
-				"0x%s", phex_nz (event_mask, 4));
-    }
-  else
-    {
-      printf_filtered (_("Event Status 0x%s\n"), phex (event_status, 4));
-      printf_filtered (_("Event Mask   0x%s\n"), phex (event_mask, 4));
-    }
+  current_uiout->text (_("Event Status "));
+  current_uiout->field_fmt ("event_status", "0x%s", phex_nz (event_status, 4));
+  current_uiout->text (_("\nEvent Mask   "));
+  current_uiout->field_fmt ("event_mask", "0x%s", phex_nz (event_mask, 4));
 }
 
 static void
@@ -2226,8 +2218,7 @@ info_spu_mailbox_list (gdb_byte *buf, int nr, enum bfd_endian byte_order,
 	current_uiout->field_fmt (field, "0x%s", phex (val, 4));
       }
 
-      if (!current_uiout->is_mi_like_p ())
-	printf_filtered ("\n");
+      current_uiout->text ("\n");
     }
 }
 
@@ -2454,8 +2445,7 @@ info_spu_dma_cmdlist (gdb_byte *buf, int nr, enum bfd_endian byte_order)
 	  current_uiout->field_skip ("error_p");
       }
 
-      if (!current_uiout->is_mi_like_p ())
-	printf_filtered ("\n");
+      current_uiout->text ("\n");
     }
 }
 
-- 
2.13.6

[RFA 2/6] Remove some uses of is_mi_like_p from py-framefilter.c

[Tom Tromey]

Some uses of is_mi_like_p in py-framefilter.c were not needed.  In
general a call to ui_out::text, ui_out::message, or ui_out::spaces
does not need to be guarded -- these are already ignored by MI.

ChangeLog
2018-04-29  Tom Tromey  <tom@tromey.com>

	* python/py-framefilter.c (py_print_single_arg)
	(enumerate_locals, py_print_args, py_print_frame): Remove some
	uses of is_mi_like_p.
---
 gdb/ChangeLog               |  6 ++++++
 gdb/python/py-framefilter.c | 23 +++++++----------------
 2 files changed, 13 insertions(+), 16 deletions(-)

diff --git a/gdb/python/py-framefilter.c b/gdb/python/py-framefilter.c
index 698fd87f4b..4edefd48f9 100644
--- a/gdb/python/py-framefilter.c
+++ b/gdb/python/py-framefilter.c
@@ -368,8 +368,7 @@ py_print_single_arg (struct ui_out *out,
 
   annotate_arg_name_end ();
 
-  if (! out->is_mi_like_p ())
-    out->text ("=");
+  out->text ("=");
 
   if (print_args_field)
     out->field_int ("arg", 1);
@@ -602,16 +601,11 @@ enumerate_locals (PyObject *iter,
 	  if (print_args_field || args_type != NO_VALUES)
 	    tuple.emplace (out, nullptr);
 	}
-      if (! out->is_mi_like_p ())
-	{
-	  /* If the output is not MI we indent locals.  */
-	  out->spaces (local_indent);
-	}
 
+      /* If the output is not MI we indent locals.  */
+      out->spaces (local_indent);
       out->field_string ("name", sym_name.get ());
-
-      if (! out->is_mi_like_p ())
-	out->text (" = ");
+      out->text (" = ");
 
       if (args_type == MI_PRINT_SIMPLE_VALUES)
 	py_print_type (out, val);
@@ -717,16 +711,14 @@ py_print_args (PyObject *filter,
 
   out->wrap_hint ("   ");
   annotate_frame_args ();
-  if (! out->is_mi_like_p ())
-    out->text (" (");
+  out->text (" (");
 
   if (args_iter != Py_None
       && (enumerate_args (args_iter.get (), out, args_type, 0, frame)
 	  == EXT_LANG_BT_ERROR))
     return EXT_LANG_BT_ERROR;
 
-  if (! out->is_mi_like_p ())
-    out->text (")");
+  out->text (")");
 
   return EXT_LANG_BT_OK;
 }
@@ -996,8 +988,7 @@ py_print_frame (PyObject *filter, frame_filter_flags flags,
 
 	  ui_out_emit_list inner_list_emiter (out, "children");
 
-	  if (! out->is_mi_like_p ())
-	    indent++;
+	  indent++;
 
 	  while ((item = PyIter_Next (elided.get ())))
 	    {
-- 
2.13.6

[RFA 1/6] Make do_is_mi_like_p const.

[Tom Tromey]

This changes ui_out to make is_mi_like_p and do_is_mi_like_p "const".

ChangeLog
2018-04-29  Tom Tromey  <tom@tromey.com>

	* ui-out.c: Update.
	* cli-out.h (cli_ui_out::do_is_mi_like_p): Update.
	* ui-out.h (ui_out::is_mi_like_p): Now const.
	(ui_out::do_is_mi_like_p): Now const.
	* mi/mi-out.h (mi_ui_out::do_is_mi_like_p): Update.
---
 gdb/ChangeLog   | 8 ++++++++
 gdb/mi/mi-out.h | 2 +-
 gdb/ui-out.c    | 2 +-
 gdb/ui-out.h    | 4 ++--
 4 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/gdb/mi/mi-out.h b/gdb/mi/mi-out.h
index 7389071dcf..89ff88cd32 100644
--- a/gdb/mi/mi-out.h
+++ b/gdb/mi/mi-out.h
@@ -69,7 +69,7 @@ protected:
   virtual void do_flush () override;
   virtual void do_redirect (struct ui_file *outstream) override;
 
-  virtual bool do_is_mi_like_p () override
+  virtual bool do_is_mi_like_p () const override
   { return true; }
 
 private:
diff --git a/gdb/ui-out.c b/gdb/ui-out.c
index 3648815090..ebe0285b49 100644
--- a/gdb/ui-out.c
+++ b/gdb/ui-out.c
@@ -581,7 +581,7 @@ ui_out::test_flags (ui_out_flags mask)
 }
 
 bool
-ui_out::is_mi_like_p ()
+ui_out::is_mi_like_p () const
 {
   return do_is_mi_like_p ();
 }
diff --git a/gdb/ui-out.h b/gdb/ui-out.h
index a415100d7e..81e2e0b20d 100644
--- a/gdb/ui-out.h
+++ b/gdb/ui-out.h
@@ -119,7 +119,7 @@ class ui_out
      a hack to encapsulate that test.  Once GDB manages to separate the
      CLI/MI from the core of GDB the problem should just go away ....  */
 
-  bool is_mi_like_p ();
+  bool is_mi_like_p () const;
 
   bool query_table_field (int colno, int *width, int *alignment,
 			  const char **col_name);
@@ -157,7 +157,7 @@ class ui_out
   /* Set as not MI-like by default.  It is overridden in subclasses if
      necessary.  */
 
-  virtual bool do_is_mi_like_p ()
+  virtual bool do_is_mi_like_p () const
   { return false; }
 
  private:
-- 
2.13.6

[RFA 4/6] Remove a use of is_mi_like_p from tracepoint.c

[Tom Tromey]

This removes a use of is_mi_like_p and changes a printf_filtered into
a call to ui_out::text.

2018-04-29  Tom Tromey  <tom@tromey.com>

	* tracepoint.c (tvariables_info_1): Remove use of is_mi_like_p.
---
 gdb/ChangeLog    |  4 +++
 gdb/tracepoint.c | 75 ++++++++++++++++++++++++++++----------------------------
 2 files changed, 41 insertions(+), 38 deletions(-)

diff --git a/gdb/tracepoint.c b/gdb/tracepoint.c
index 7e173ce75d..f40fb048d8 100644
--- a/gdb/tracepoint.c
+++ b/gdb/tracepoint.c
@@ -420,51 +420,50 @@ tvariables_info_1 (void)
 {
   struct ui_out *uiout = current_uiout;
 
-  if (tvariables.empty () && !uiout->is_mi_like_p ())
-    {
-      printf_filtered (_("No trace state variables.\n"));
-      return;
-    }
-
   /* Try to acquire values from the target.  */
   for (trace_state_variable &tsv : tvariables)
     tsv.value_known
       = target_get_trace_state_variable_value (tsv.number, &tsv.value);
 
-  ui_out_emit_table table_emitter (uiout, 3, tvariables.size (),
-				   "trace-variables");
-  uiout->table_header (15, ui_left, "name", "Name");
-  uiout->table_header (11, ui_left, "initial", "Initial");
-  uiout->table_header (11, ui_left, "current", "Current");
+  {
+    ui_out_emit_table table_emitter (uiout, 3, tvariables.size (),
+				     "trace-variables");
+    uiout->table_header (15, ui_left, "name", "Name");
+    uiout->table_header (11, ui_left, "initial", "Initial");
+    uiout->table_header (11, ui_left, "current", "Current");
 
-  uiout->table_body ();
+    uiout->table_body ();
 
-  for (const trace_state_variable &tsv : tvariables)
-    {
-      const char *c;
-
-      ui_out_emit_tuple tuple_emitter (uiout, "variable");
-
-      uiout->field_string ("name", std::string ("$") + tsv.name);
-      uiout->field_string ("initial", plongest (tsv.initial_value));
-
-      if (tsv.value_known)
-        c = plongest (tsv.value);
-      else if (uiout->is_mi_like_p ())
-        /* For MI, we prefer not to use magic string constants, but rather
-           omit the field completely.  The difference between unknown and
-           undefined does not seem important enough to represent.  */
-        c = NULL;
-      else if (current_trace_status ()->running || traceframe_number >= 0)
-	/* The value is/was defined, but we don't have it.  */
-        c = "<unknown>";
-      else
-	/* It is not meaningful to ask about the value.  */
-        c = "<undefined>";
-      if (c)
-        uiout->field_string ("current", c);
-      uiout->text ("\n");
-    }
+    for (const trace_state_variable &tsv : tvariables)
+      {
+	const char *c;
+
+	ui_out_emit_tuple tuple_emitter (uiout, "variable");
+
+	uiout->field_string ("name", std::string ("$") + tsv.name);
+	uiout->field_string ("initial", plongest (tsv.initial_value));
+
+	if (tsv.value_known)
+	  c = plongest (tsv.value);
+	else if (uiout->is_mi_like_p ())
+	  /* For MI, we prefer not to use magic string constants, but rather
+	     omit the field completely.  The difference between unknown and
+	     undefined does not seem important enough to represent.  */
+	  c = NULL;
+	else if (current_trace_status ()->running || traceframe_number >= 0)
+	  /* The value is/was defined, but we don't have it.  */
+	  c = "<unknown>";
+	else
+	  /* It is not meaningful to ask about the value.  */
+	  c = "<undefined>";
+	if (c)
+	  uiout->field_string ("current", c);
+	uiout->text ("\n");
+      }
+  }
+
+  if (tvariables.empty ())
+    uiout->text (_("No trace state variables.\n"));
 }
 
 /* List all the trace state variables.  */
-- 
2.13.6

[RFA 5/6] Remove some is_mi_like_p from breakpoint code

[Tom Tromey]

This removes some uses of is_mi_like_p from the breakpoint code.  The
break-catch-throw.c change brings it into line with what other
breakpoint classes do.  The other changes simply replace printf calls
with ui_out::text or ui_out::message calls.

2018-04-29  Tom Tromey  <tom@tromey.com>

	* breakpoint.c (mention): Remove use of is_mi_like_p.
	(print_mention_ranged_breakpoint): Likewise.
	* break-catch-throw.c (print_it_exception_catchpoint): Remove use
	of is_mi_like_p.
---
 gdb/ChangeLog           |  7 +++++++
 gdb/break-catch-throw.c |  4 +---
 gdb/breakpoint.c        | 13 ++++---------
 3 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/gdb/break-catch-throw.c b/gdb/break-catch-throw.c
index a911ddd9b5..bdb552c863 100644
--- a/gdb/break-catch-throw.c
+++ b/gdb/break-catch-throw.c
@@ -238,8 +238,7 @@ print_it_exception_catchpoint (bpstat bs)
   bp_temp = b->disposition == disp_del;
   uiout->text (bp_temp ? "Temporary catchpoint "
 		       : "Catchpoint ");
-  if (!uiout->is_mi_like_p ())
-    uiout->field_int ("bkptno", b->number);
+  uiout->field_int ("bkptno", b->number);
   uiout->text ((kind == EX_EVENT_THROW ? " (exception thrown), "
 		: (kind == EX_EVENT_CATCH ? " (exception caught), "
 		   : " (exception rethrown), ")));
@@ -248,7 +247,6 @@ print_it_exception_catchpoint (bpstat bs)
       uiout->field_string ("reason",
 			   async_reason_lookup (EXEC_ASYNC_BREAKPOINT_HIT));
       uiout->field_string ("disp", bpdisp_text (b->disposition));
-      uiout->field_int ("bkptno", b->number);
     }
   return PRINT_SRC_AND_LOC;
 }
diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index b019610130..ae6f161ec9 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -8596,9 +8596,7 @@ static void
 mention (struct breakpoint *b)
 {
   b->ops->print_mention (b);
-  if (current_uiout->is_mi_like_p ())
-    return;
-  printf_filtered ("\n");
+  current_uiout->text ("\n");
 }
 \f
 
@@ -9795,12 +9793,9 @@ print_mention_ranged_breakpoint (struct breakpoint *b)
   gdb_assert (bl);
   gdb_assert (b->type == bp_hardware_breakpoint);
 
-  if (uiout->is_mi_like_p ())
-    return;
-
-  printf_filtered (_("Hardware assisted ranged breakpoint %d from %s to %s."),
-		   b->number, paddress (bl->gdbarch, bl->address),
-		   paddress (bl->gdbarch, bl->address + bl->length - 1));
+  uiout->message (_("Hardware assisted ranged breakpoint %d from %s to %s."),
+		  b->number, paddress (bl->gdbarch, bl->address),
+		  paddress (bl->gdbarch, bl->address + bl->length - 1));
 }
 
 /* Implement the "print_recreate" breakpoint_ops method for
-- 
2.13.6

[RFA 6/6] Remove a use of is_mi_like_p from darwin-nat-info.c

[Tom Tromey]

This removes a use of is_mi_like_p from darwin-nat-info.c.
This is not needed because MI already ignores ui_out::text.

2018-04-29  Tom Tromey  <tom@tromey.com>

	* darwin-nat-info.c (darwin_debug_regions_recurse): Remove use of
	is_mi_like_p.
---
 gdb/ChangeLog         | 5 +++++
 gdb/darwin-nat-info.c | 3 +--
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/gdb/darwin-nat-info.c b/gdb/darwin-nat-info.c
index 31086b17dd..e68637fc0d 100644
--- a/gdb/darwin-nat-info.c
+++ b/gdb/darwin-nat-info.c
@@ -676,8 +676,7 @@ darwin_debug_regions_recurse (task_t task)
 	  uiout->field_int ("tag", r_info.user_tag);
       }
 
-      if (!uiout->is_mi_like_p ())
-	uiout->text ("\n");
+      uiout->text ("\n");
 
       if (r_info.is_submap)
 	r_depth++;
-- 
2.13.6

Re: [RFA 0/6] Remove some uses of is_mi_like_p

[Pedro Alves]

On 04/30/2018 03:07 PM, Tom Tromey wrote:
> I think in the long run it would be good to remove
> ui_out::is_mi_like_p.  This series removes some unnecessary uses of
> it.  This is a mild improvement because it means there will be less
> temptation for people to copy bad examples from the existing code.

The series looks fine to me.

> Relatedly, I had an idea for how to change cli_out so that most
> (perhaps not all -- there are some tricky cases) uses of is_mi_like_p
> could be removed, and also provide new features like user control over
> display and colorizing.
> 
> The basic idea is first to realize that table names are fixed by the
> MI spec, and so can be relied on by cli_out.  Then, whereever there is
> a sequence involving textual output mixed with ui_out field output,
> the whole sequence could be replaced with a format string that
> references the field names.

I'm not sure what you mean exactly, but I've wished before that
we could write ui-out field output mixed in with plain text with
something like:

  ui_out->field_fmt (_("Field 1 is 0x%pF, at 0x%pF\n"),
                     ui_field_int ("field1", var1),
                     ui_field_func ("func_name", var2));

That's allow proper i18n, and would allow things like colorizing.

I guess that this is something like what you mean, only you
probably have it more thought through.

Thanks,
Pedro Alves

Re: [RFA 0/6] Remove some uses of is_mi_like_p

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

Pedro> I'm not sure what you mean exactly, but I've wished before that
Pedro> we could write ui-out field output mixed in with plain text with
Pedro> something like:

Pedro> ui_out-> field_fmt (_("Field 1 is 0x%pF, at 0x%pF\n"),
Pedro>                      ui_field_int ("field1", var1),
Pedro>                      ui_field_func ("func_name", var2));

Pedro> That's allow proper i18n, and would allow things like colorizing.

Pedro> I guess that this is something like what you mean, only you
Pedro> probably have it more thought through.

Haha, I wouldn't say that.

What I was thinking, though, was a combination of:

* teaching gdb to know when an escape sequence doesn't contribute to the
  column count (I have a patch for this);
* moving all such formatting to a special formatting language;
* and finally, letting users set the formats.


So for the above example, the emission code would just look something
like:

   ui_out->field_int ("field1", var1);
   ui_out->field_core_addr ("func_name", var2);

Elsewhere the CLI would set up the default format for this table (or
tuple or list):

   cli_out_register ("field-info-entry",
                     "Field 1 is %{field1:x}, at %{field2:x}\n");

Colorizing could be done by just changing the format.  Users could
rearrange the columns or do other simple-ish customization as well.  If
we were feeling really ambitious we could replace annotations this way.

I was also thinking that the column widths could be removed from the
producers and put into these format strings.

However, there are various tricky cases here, due to the ways that
is_mi_like_p has been abused so far, and also the complicated ways that
messages are actually constructed.  I have a (partial) list on my other
machine, if you really want to go into the weeds.


Another idea I had was to do all this, only from Python.  I have some
code in this direction as well.  However it seemed to me that, although
Python would allow more flexibility (advanced users could program it),
on the other hand it's better for the gdb core to be cleaned up if
possible.

Tom