[PATCH 00/14] Minor ui / interp cleanups

[PATCH 00/14] Minor ui / interp cleanups

[Tom Tromey]

I've been looking at the interp/ui/logging code for a project I'm
working on.  While doing this, I found a number of smaller things to
clean up.  This series is the result.

Regression tested on x86-64 Fedora 34.

Tom

[PATCH 01/14] Remove some dead code

[Tom Tromey]

This patch removes some dead code and an old FIXME.  These no longer
seem useful, even for documentation purposes.
---
 gdb/event-top.c | 13 -------------
 1 file changed, 13 deletions(-)

diff --git a/gdb/event-top.c b/gdb/event-top.c
index 02b3786320f..6f67659a36b 100644
--- a/gdb/event-top.c
+++ b/gdb/event-top.c
@@ -1378,19 +1378,6 @@ gdb_disable_readline (void)
 {
   struct ui *ui = current_ui;
 
-  /* FIXME - It is too heavyweight to delete and remake these every
-     time you run an interpreter that needs readline.  It is probably
-     better to have the interpreters cache these, which in turn means
-     that this needs to be moved into interpreter specific code.  */
-
-#if 0
-  ui_file_delete (gdb_stdout);
-  ui_file_delete (gdb_stderr);
-  gdb_stdlog = NULL;
-  gdb_stdtarg = NULL;
-  gdb_stdtargerr = NULL;
-#endif
-
   if (ui->command_editing)
     gdb_rl_callback_handler_remove ();
   ui->unregister_file_handler ();
-- 
2.34.1

[PATCH 02/14] Free ui::line_buffer

[Tom Tromey]

A ui initializes its line_buffer, but never calls buffer_free on it.
This patch fixes the oversight.  I found this by inspection.
---
 gdb/top.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/gdb/top.c b/gdb/top.c
index 453fa236db1..10983919011 100644
--- a/gdb/top.c
+++ b/gdb/top.c
@@ -352,6 +352,8 @@ ui::~ui ()
   delete m_gdb_stdin;
   delete m_gdb_stdout;
   delete m_gdb_stderr;
+
+  buffer_free (&line_buffer);
 }
 
 /* Open file named NAME for read/write, making sure not to make it the
-- 
2.34.1

[PATCH 03/14] Use ui_out_redirect_pop in more places

[Tom Tromey]

This changes ui_out_redirect_pop to also perform the redirection, and
then updates several sites to use this, rather than explicit
redirects.
---
 gdb/breakpoint.c           | 13 ++-----------
 gdb/guile/scm-breakpoint.c |  3 +--
 gdb/guile/scm-ports.c      |  3 +--
 gdb/mi/mi-interp.c         | 29 ++++++++---------------------
 gdb/python/py-breakpoint.c |  4 +---
 gdb/top.c                  |  3 +--
 gdb/ui-out.h               |  6 ++++--
 7 files changed, 18 insertions(+), 43 deletions(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index dae96d205be..0329c1a47c6 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -14049,18 +14049,9 @@ save_breakpoints (const char *filename, int from_tty,
 	{
 	  fp.puts ("  commands\n");
 
-	  current_uiout->redirect (&fp);
-	  try
-	    {
-	      print_command_lines (current_uiout, tp->commands.get (), 2);
-	    }
-	  catch (const gdb_exception &ex)
-	    {
-	    current_uiout->redirect (NULL);
-	      throw;
-	    }
+	  ui_out_redirect_pop redir (current_uiout, &fp);
+	  print_command_lines (current_uiout, tp->commands.get (), 2);
 
-	  current_uiout->redirect (NULL);
 	  fp.puts ("  end\n");
 	}
 
diff --git a/gdb/guile/scm-breakpoint.c b/gdb/guile/scm-breakpoint.c
index 5406b74a389..a7799416c50 100644
--- a/gdb/guile/scm-breakpoint.c
+++ b/gdb/guile/scm-breakpoint.c
@@ -999,10 +999,10 @@ gdbscm_breakpoint_commands (SCM self)
 
   string_file buf;
 
-  current_uiout->redirect (&buf);
   gdbscm_gdb_exception exc {};
   try
     {
+      ui_out_redirect_pop redir (current_uiout, &buf);
       print_command_lines (current_uiout, breakpoint_commands (bp), 0);
     }
   catch (const gdb_exception &except)
@@ -1010,7 +1010,6 @@ gdbscm_breakpoint_commands (SCM self)
       exc = unpack (except);
     }
 
-  current_uiout->redirect (NULL);
   GDBSCM_HANDLE_GDB_EXCEPTION (exc);
   result = gdbscm_scm_from_c_string (buf.c_str ());
 
diff --git a/gdb/guile/scm-ports.c b/gdb/guile/scm-ports.c
index 946859aa1f3..9173cb6f26e 100644
--- a/gdb/guile/scm-ports.c
+++ b/gdb/guile/scm-ports.c
@@ -607,8 +607,7 @@ ioscm_with_output_to_port_worker (SCM port, SCM thunk, enum oport oport,
       gdb_stderr = port_file.get ();
     else
       {
-	current_uiout->redirect (port_file.get ());
-	redirect_popper.emplace (current_uiout);
+	redirect_popper.emplace (current_uiout, port_file.get ());
 
 	gdb_stdout = port_file.get ();
       }
diff --git a/gdb/mi/mi-interp.c b/gdb/mi/mi-interp.c
index ae15177890c..6633a7fa0a8 100644
--- a/gdb/mi/mi-interp.c
+++ b/gdb/mi/mi-interp.c
@@ -816,16 +816,14 @@ mi_tsv_modified (const struct trace_state_variable *tsv)
       gdb_printf (mi->event_channel,
 		  "tsv-modified");
 
-      mi_uiout->redirect (mi->event_channel);
+      ui_out_redirect_pop redir (mi_uiout, mi->event_channel);
 
       mi_uiout->field_string ("name", tsv->name);
       mi_uiout->field_string ("initial",
-			   plongest (tsv->initial_value));
+			      plongest (tsv->initial_value));
       if (tsv->value_known)
 	mi_uiout->field_string ("current", plongest (tsv->value));
 
-      mi_uiout->redirect (NULL);
-
       gdb_flush (mi->event_channel);
     }
 }
@@ -844,7 +842,7 @@ mi_print_breakpoint_for_event (struct mi_interp *mi, breakpoint *bp)
      break if anything is output to mi_uiout prior to calling the
      breakpoint_created notifications.  So, we use
      ui_out_redirect.  */
-  mi_uiout->redirect (mi->event_channel);
+  ui_out_redirect_pop redir (mi_uiout, mi->event_channel);
 
   try
     {
@@ -857,8 +855,6 @@ mi_print_breakpoint_for_event (struct mi_interp *mi, breakpoint *bp)
     {
       exception_print (gdb_stderr, ex);
     }
-
-  mi_uiout->redirect (NULL);
 }
 
 /* Emit notification about a created breakpoint.  */
@@ -1090,12 +1086,10 @@ mi_solib_loaded (struct so_list *solib)
 
       gdb_printf (mi->event_channel, "library-loaded");
 
-      uiout->redirect (mi->event_channel);
+      ui_out_redirect_pop redir (uiout, mi->event_channel);
 
       mi_output_solib_attribs (uiout, solib);
 
-      uiout->redirect (NULL);
-
       gdb_flush (mi->event_channel);
     }
 }
@@ -1118,7 +1112,7 @@ mi_solib_unloaded (struct so_list *solib)
 
       gdb_printf (mi->event_channel, "library-unloaded");
 
-      uiout->redirect (mi->event_channel);
+      ui_out_redirect_pop redir (uiout, mi->event_channel);
 
       uiout->field_string ("id", solib->so_original_name);
       uiout->field_string ("target-name", solib->so_original_name);
@@ -1128,8 +1122,6 @@ mi_solib_unloaded (struct so_list *solib)
 	  uiout->field_fmt ("thread-group", "i%d", current_inferior ()->num);
 	}
 
-      uiout->redirect (NULL);
-
       gdb_flush (mi->event_channel);
     }
 }
@@ -1157,13 +1149,11 @@ mi_command_param_changed (const char *param, const char *value)
 
       gdb_printf (mi->event_channel, "cmd-param-changed");
 
-      mi_uiout->redirect (mi->event_channel);
+      ui_out_redirect_pop redir (mi_uiout, mi->event_channel);
 
       mi_uiout->field_string ("param", param);
       mi_uiout->field_string ("value", value);
 
-      mi_uiout->redirect (NULL);
-
       gdb_flush (mi->event_channel);
     }
 }
@@ -1193,7 +1183,7 @@ mi_memory_changed (struct inferior *inferior, CORE_ADDR memaddr,
 
       gdb_printf (mi->event_channel, "memory-changed");
 
-      mi_uiout->redirect (mi->event_channel);
+      ui_out_redirect_pop redir (mi_uiout, mi->event_channel);
 
       mi_uiout->field_fmt ("thread-group", "i%d", inferior->num);
       mi_uiout->field_core_addr ("addr", target_gdbarch (), memaddr);
@@ -1210,8 +1200,6 @@ mi_memory_changed (struct inferior *inferior, CORE_ADDR memaddr,
 	    mi_uiout->field_string ("type", "code");
 	}
 
-      mi_uiout->redirect (NULL);
-
       gdb_flush (mi->event_channel);
     }
 }
@@ -1243,8 +1231,7 @@ mi_user_selected_context_changed (user_selected_what selection)
 
       mi_uiout = top_level_interpreter ()->interp_ui_out ();
 
-      mi_uiout->redirect (mi->event_channel);
-      ui_out_redirect_pop redirect_popper (mi_uiout);
+      ui_out_redirect_pop redirect_popper (mi_uiout, mi->event_channel);
 
       target_terminal::scoped_restore_terminal_state term_state;
       target_terminal::ours_for_output ();
diff --git a/gdb/python/py-breakpoint.c b/gdb/python/py-breakpoint.c
index 48ec86cebfe..dd4519a1b05 100644
--- a/gdb/python/py-breakpoint.c
+++ b/gdb/python/py-breakpoint.c
@@ -548,19 +548,17 @@ bppy_get_commands (PyObject *self, void *closure)
 
   string_file stb;
 
-  current_uiout->redirect (&stb);
   try
     {
+      ui_out_redirect_pop redir (current_uiout, &stb);
       print_command_lines (current_uiout, breakpoint_commands (bp), 0);
     }
   catch (const gdb_exception &except)
     {
-      current_uiout->redirect (NULL);
       gdbpy_convert_exception (except);
       return NULL;
     }
 
-  current_uiout->redirect (NULL);
   return host_string_to_python_string (stb.c_str ()).release ();
 }
 
diff --git a/gdb/top.c b/gdb/top.c
index 10983919011..655bcd77598 100644
--- a/gdb/top.c
+++ b/gdb/top.c
@@ -735,8 +735,7 @@ execute_fn_to_ui_file (struct ui_file *file, std::function<void(void)> fn)
   scoped_restore save_async = make_scoped_restore (&current_ui->async, 0);
 
   {
-    current_uiout->redirect (file);
-    ui_out_redirect_pop redirect_popper (current_uiout);
+    ui_out_redirect_pop redirect_popper (current_uiout, file);
 
     scoped_restore save_stdout
       = make_scoped_restore (&gdb_stdout, file);
diff --git a/gdb/ui-out.h b/gdb/ui-out.h
index 9e6ff9a29bf..9c455d3ed67 100644
--- a/gdb/ui-out.h
+++ b/gdb/ui-out.h
@@ -427,15 +427,17 @@ class ui_out_emit_table
   struct ui_out *m_uiout;
 };
 
-/* On destruction, pop the last redirection by calling the uiout's
+/* On construction, redirect a uiout to a given stream.  On
+   destruction, pop the last redirection by calling the uiout's
    redirect method with a NULL parameter.  */
 class ui_out_redirect_pop
 {
 public:
 
-  ui_out_redirect_pop (ui_out *uiout)
+  ui_out_redirect_pop (ui_out *uiout, ui_file *stream)
     : m_uiout (uiout)
   {
+    m_uiout->redirect (stream);
   }
 
   ~ui_out_redirect_pop ()
-- 
2.34.1

[PATCH 04/14] Remove the "for moment" comments

[Tom Tromey]

A few spots setting some gdb output stream variables have a "for
moment" comment.  These comments aren't useful and I think the moment
has passed -- these are permanent now.
---
 gdb/event-top.c  | 4 ++--
 gdb/main.c       | 4 ++--
 gdb/tui/tui-io.c | 8 ++++----
 3 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/gdb/event-top.c b/gdb/event-top.c
index 6f67659a36b..3d7bf4ad560 100644
--- a/gdb/event-top.c
+++ b/gdb/event-top.c
@@ -1336,8 +1336,8 @@ gdb_setup_readline (int editing)
     gdb_stdout = new pager_file (new stdio_file (ui->outstream));
   gdb_stderr = new stderr_file (ui->errstream);
   gdb_stdlog = new timestamped_file (gdb_stderr);
-  gdb_stdtarg = gdb_stderr; /* for moment */
-  gdb_stdtargerr = gdb_stderr; /* for moment */
+  gdb_stdtarg = gdb_stderr;
+  gdb_stdtargerr = gdb_stderr;
 
   /* If the input stream is connected to a terminal, turn on editing.
      However, that is only allowed on the main UI, as we can only have
diff --git a/gdb/main.c b/gdb/main.c
index 8c97987956b..91fa1966738 100644
--- a/gdb/main.c
+++ b/gdb/main.c
@@ -676,8 +676,8 @@ captured_main_1 (struct captured_main_args *context)
   main_ui = new ui (stdin, stdout, stderr);
   current_ui = main_ui;
 
-  gdb_stdtargerr = gdb_stderr;	/* for moment */
-  gdb_stdtargin = gdb_stdin;	/* for moment */
+  gdb_stdtargerr = gdb_stderr;
+  gdb_stdtargin = gdb_stdin;
 
   if (bfd_init () != BFD_INIT_MAGIC)
     error (_("fatal error: libbfd ABI mismatch"));
diff --git a/gdb/tui/tui-io.c b/gdb/tui/tui-io.c
index deea9b90afc..51f08168a90 100644
--- a/gdb/tui/tui-io.c
+++ b/gdb/tui/tui-io.c
@@ -839,8 +839,8 @@ tui_setup_io (int mode)
       gdb_stdout = tui_stdout;
       gdb_stderr = tui_stderr;
       gdb_stdlog = tui_stdlog;
-      gdb_stdtarg = gdb_stderr;	/* for moment */
-      gdb_stdtargerr = gdb_stderr;	/* for moment */
+      gdb_stdtarg = gdb_stderr;
+      gdb_stdtargerr = gdb_stderr;
       current_uiout = tui_out;
 
       /* Save tty for SIGCONT.  */
@@ -852,8 +852,8 @@ tui_setup_io (int mode)
       gdb_stdout = tui_old_stdout;
       gdb_stderr = tui_old_stderr;
       gdb_stdlog = tui_old_stdlog;
-      gdb_stdtarg = gdb_stderr;	/* for moment */
-      gdb_stdtargerr = gdb_stderr;	/* for moment */
+      gdb_stdtarg = gdb_stderr;
+      gdb_stdtargerr = gdb_stderr;
       current_uiout = tui_old_uiout;
 
       /* Restore readline.  */
-- 
2.34.1

[PATCH 05/14] Remove obsolete filtering comment

[Tom Tromey]

top.h has an obsolete comment about the use of _unfiltered.
---
 gdb/top.h | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/gdb/top.h b/gdb/top.h
index 5c1db84b2ce..cb3abb80d84 100644
--- a/gdb/top.h
+++ b/gdb/top.h
@@ -141,9 +141,7 @@ struct ui
   /* Serious error notifications */
   struct ui_file *m_gdb_stderr;
   /* Log/debug/trace messages that should bypass normal stdout/stderr
-     filtering.  For moment, always call this stream using
-     *_unfiltered.  In the very near future that restriction shall be
-     removed - either call shall be unfiltered.  (cagney 1999-06-13).  */
+     filtering.  */
   struct ui_file *m_gdb_stdlog;
 
   /* The current ui_out.  */
-- 
2.34.1

[PATCH 06/14] Remove two unused members from mi_interp

[Tom Tromey]

These members of mi_interp aren't used and can be removed.
---
 gdb/mi/mi-interp.c | 1 -
 gdb/mi/mi-interp.h | 8 ++------
 2 files changed, 2 insertions(+), 7 deletions(-)

diff --git a/gdb/mi/mi-interp.c b/gdb/mi/mi-interp.c
index 6633a7fa0a8..ee270c7c9d2 100644
--- a/gdb/mi/mi-interp.c
+++ b/gdb/mi/mi-interp.c
@@ -1297,7 +1297,6 @@ mi_interp::set_logging (ui_file_up logfile, bool logging_redirect,
 	}
 
       mi->raw_stdout = logging_redirect ? logfile_p : tee;
-      mi->raw_stdlog = debug_redirect ? logfile_p : tee;
     }
   else
     {
diff --git a/gdb/mi/mi-interp.h b/gdb/mi/mi-interp.h
index adf6eaffef4..d89439f54c5 100644
--- a/gdb/mi/mi-interp.h
+++ b/gdb/mi/mi-interp.h
@@ -52,14 +52,10 @@ class mi_interp final : public interp
   /* Raw console output.  */
   struct ui_file *raw_stdout;
 
-  /* Raw logfile output.  */
-  struct ui_file *raw_stdlog;
-
-  /* Save the original value of raw_stdout and raw_stdlog here when logging, and
-     the file which we need to delete, so we can restore correctly when
+  /* Save the original value of raw_stdout here when logging, and the
+     file which we need to delete, so we can restore correctly when
      done.  */
   struct ui_file *saved_raw_stdout;
-  struct ui_file *saved_raw_stdlog;
   struct ui_file *saved_raw_file_to_delete;
 
 
-- 
2.34.1

[PATCH 07/14] Use member initialization in 'struct ui'

[Tom Tromey]

This changes 'struct ui' to use member initialization.  This is
simpler to understand.
---
 gdb/top.c | 13 ++-----------
 gdb/top.h | 18 +++++++++---------
 2 files changed, 11 insertions(+), 20 deletions(-)

diff --git a/gdb/top.c b/gdb/top.c
index 655bcd77598..b63804b14d1 100644
--- a/gdb/top.c
+++ b/gdb/top.c
@@ -295,26 +295,17 @@ unbuffer_stream (FILE *stream)
 /* See top.h.  */
 
 ui::ui (FILE *instream_, FILE *outstream_, FILE *errstream_)
-  : next (nullptr),
-    num (++highest_ui_num),
-    call_readline (nullptr),
-    input_handler (nullptr),
-    command_editing (0),
-    interp_info (nullptr),
-    async (0),
-    secondary_prompt_depth (0),
+  : num (++highest_ui_num),
     stdin_stream (instream_),
     instream (instream_),
     outstream (outstream_),
     errstream (errstream_),
     input_fd (fileno (instream)),
     m_input_interactive_p (ISATTY (instream)),
-    prompt_state (PROMPT_NEEDED),
     m_gdb_stdout (new pager_file (new stdio_file (outstream))),
     m_gdb_stdin (new stdio_file (instream)),
     m_gdb_stderr (new stderr_file (errstream)),
-    m_gdb_stdlog (m_gdb_stderr),
-    m_current_uiout (nullptr)
+    m_gdb_stdlog (m_gdb_stderr)
 {
   buffer_init (&line_buffer);
 
diff --git a/gdb/top.h b/gdb/top.h
index cb3abb80d84..6634738c1a0 100644
--- a/gdb/top.h
+++ b/gdb/top.h
@@ -61,7 +61,7 @@ struct ui
   DISABLE_COPY_AND_ASSIGN (ui);
 
   /* Pointer to next in singly-linked list.  */
-  struct ui *next;
+  struct ui *next = nullptr;
 
   /* Convenient handle (UI number).  Unique across all UIs.  */
   int num;
@@ -76,19 +76,19 @@ struct ui
      point of invocation.  In the special case in which the character
      read is newline, the function invokes the INPUT_HANDLER callback
      (see below).  */
-  void (*call_readline) (gdb_client_data);
+  void (*call_readline) (gdb_client_data) = nullptr;
 
   /* The function to invoke when a complete line of input is ready for
      processing.  */
-  void (*input_handler) (gdb::unique_xmalloc_ptr<char> &&);
+  void (*input_handler) (gdb::unique_xmalloc_ptr<char> &&) = nullptr;
 
   /* True if this UI is using the readline library for command
      editing; false if using GDB's own simple readline emulation, with
      no editing support.  */
-  int command_editing;
+  int command_editing = 0;
 
   /* Each UI has its own independent set of interpreters.  */
-  struct ui_interp_info *interp_info;
+  struct ui_interp_info *interp_info = nullptr;
 
   /* True if the UI is in async mode, false if in sync mode.  If in
      sync mode, a synchronous execution command (e.g, "next") does not
@@ -98,11 +98,11 @@ struct ui
      the top event loop.  For the main UI, this starts out disabled,
      until all the explicit command line arguments (e.g., `gdb -ex
      "start" -ex "next"') are processed.  */
-  int async;
+  int async = 0;
 
   /* The number of nested readline secondary prompts that are
      currently active.  */
-  int secondary_prompt_depth;
+  int secondary_prompt_depth = 0;
 
   /* The UI's stdin.  Set to stdin for the main UI.  */
   FILE *stdin_stream;
@@ -127,7 +127,7 @@ struct ui
   bool m_input_interactive_p;
 
   /* See enum prompt_state's description.  */
-  enum prompt_state prompt_state;
+  enum prompt_state prompt_state = PROMPT_NEEDED;
 
   /* The fields below that start with "m_" are "private".  They're
      meant to be accessed through wrapper macros that make them look
@@ -145,7 +145,7 @@ struct ui
   struct ui_file *m_gdb_stdlog;
 
   /* The current ui_out.  */
-  struct ui_out *m_current_uiout;
+  struct ui_out *m_current_uiout = nullptr;
 
   /* Register the UI's input file descriptor in the event loop.  */
   void register_file_handler ();
-- 
2.34.1

[PATCH 08/14] Use scoped_restore in safe_parse_type

[Tom Tromey]

This changes safe_parse_type to use scoped_restore rather than
explicit assignments.
---
 gdb/gdbtypes.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/gdb/gdbtypes.c b/gdb/gdbtypes.c
index 8c2558dd7ac..c458b204157 100644
--- a/gdb/gdbtypes.c
+++ b/gdb/gdbtypes.c
@@ -3168,12 +3168,11 @@ check_typedef (struct type *type)
 static struct type *
 safe_parse_type (struct gdbarch *gdbarch, const char *p, int length)
 {
-  struct ui_file *saved_gdb_stderr;
   struct type *type = NULL; /* Initialize to keep gcc happy.  */
 
   /* Suppress error messages.  */
-  saved_gdb_stderr = gdb_stderr;
-  gdb_stderr = &null_stream;
+  scoped_restore saved_gdb_stderr = make_scoped_restore (&gdb_stderr,
+							 &null_stream);
 
   /* Call parse_and_eval_type() without fear of longjmp()s.  */
   try
@@ -3185,9 +3184,6 @@ safe_parse_type (struct gdbarch *gdbarch, const char *p, int length)
       type = builtin_type (gdbarch)->builtin_void;
     }
 
-  /* Stop suppressing error messages.  */
-  gdb_stderr = saved_gdb_stderr;
-
   return type;
 }
 
-- 
2.34.1

[PATCH 09/14] Remove tui_out_new

[Tom Tromey]

tui_out_new is just a simple wrapper for 'new' and can be removed,
simplifying gdb a tiny bit.
---
 gdb/tui/tui-io.c  | 2 +-
 gdb/tui/tui-out.c | 6 ------
 gdb/tui/tui-out.h | 2 --
 3 files changed, 1 insertion(+), 9 deletions(-)

diff --git a/gdb/tui/tui-io.c b/gdb/tui/tui-io.c
index 51f08168a90..9f27f8bcc01 100644
--- a/gdb/tui/tui-io.c
+++ b/gdb/tui/tui-io.c
@@ -907,7 +907,7 @@ tui_initialize_io (void)
   tui_stdout = new pager_file (new tui_file (stdout));
   tui_stderr = new tui_file (stderr);
   tui_stdlog = new timestamped_file (tui_stderr);
-  tui_out = tui_out_new (tui_stdout);
+  tui_out = new tui_ui_out (tui_stdout);
 
   /* Create the default UI.  */
   tui_old_uiout = new cli_ui_out (gdb_stdout);
diff --git a/gdb/tui/tui-out.c b/gdb/tui/tui-out.c
index ae50b65a3c6..fdbd9c9fcc3 100644
--- a/gdb/tui/tui-out.c
+++ b/gdb/tui/tui-out.c
@@ -109,9 +109,3 @@ tui_ui_out::tui_ui_out (ui_file *stream)
   : cli_ui_out (stream, 0)
 {
 }
-
-tui_ui_out *
-tui_out_new (struct ui_file *stream)
-{
-  return new tui_ui_out (stream);
-}
diff --git a/gdb/tui/tui-out.h b/gdb/tui/tui-out.h
index b71d308cf14..967900f0307 100644
--- a/gdb/tui/tui-out.h
+++ b/gdb/tui/tui-out.h
@@ -61,6 +61,4 @@ class tui_ui_out : public cli_ui_out
   int m_start_of_line = 0;
 };
 
-extern tui_ui_out *tui_out_new (struct ui_file *stream);
-
 #endif /* TUI_TUI_OUT_H */
-- 
2.34.1

[PATCH 10/14] Remove a ui-related memory leak

[Tom Tromey]

gdb_setup_readline makes new streams and assigns to the various stream
members of struct ui.  However, these assignments cause the previous
values to leak.  As far as I can, this code is simply unnecessary and
can be removed -- with the exception of the assignment to gdb_stdtarg,
which is not initialized anywhere else.
---
 gdb/event-top.c | 11 -----------
 gdb/main.c      |  1 +
 2 files changed, 1 insertion(+), 11 deletions(-)

diff --git a/gdb/event-top.c b/gdb/event-top.c
index 3d7bf4ad560..151849c01ea 100644
--- a/gdb/event-top.c
+++ b/gdb/event-top.c
@@ -1328,17 +1328,6 @@ gdb_setup_readline (int editing)
 {
   struct ui *ui = current_ui;
 
-  /* This function is a noop for the sync case.  The assumption is
-     that the sync setup is ALL done in gdb_init, and we would only
-     mess it up here.  The sync stuff should really go away over
-     time.  */
-  if (!batch_silent)
-    gdb_stdout = new pager_file (new stdio_file (ui->outstream));
-  gdb_stderr = new stderr_file (ui->errstream);
-  gdb_stdlog = new timestamped_file (gdb_stderr);
-  gdb_stdtarg = gdb_stderr;
-  gdb_stdtargerr = gdb_stderr;
-
   /* If the input stream is connected to a terminal, turn on editing.
      However, that is only allowed on the main UI, as we can only have
      one instance of readline.  */
diff --git a/gdb/main.c b/gdb/main.c
index 91fa1966738..8d2666ecc95 100644
--- a/gdb/main.c
+++ b/gdb/main.c
@@ -676,6 +676,7 @@ captured_main_1 (struct captured_main_args *context)
   main_ui = new ui (stdin, stdout, stderr);
   current_ui = main_ui;
 
+  gdb_stdtarg = gdb_stderr;
   gdb_stdtargerr = gdb_stderr;
   gdb_stdtargin = gdb_stdin;
 
-- 
2.34.1

[PATCH 11/14] TUI stdout buffering cleanup

[Tom Tromey]

The TUI checks against gdb_stdout to decide when to buffer.  It seems
much cleaner to me to simply record this as an attribute of the stream
itself.
---
 gdb/tui/tui-file.c | 23 +++--------------------
 gdb/tui/tui-file.h | 10 +++++++++-
 gdb/tui/tui-io.c   |  4 ++--
 3 files changed, 14 insertions(+), 23 deletions(-)

diff --git a/gdb/tui/tui-file.c b/gdb/tui/tui-file.c
index 49dafce112c..3cf3dbc171a 100644
--- a/gdb/tui/tui-file.c
+++ b/gdb/tui/tui-file.c
@@ -22,24 +22,11 @@
 #include "tui/tui-command.h"
 #include "tui.h"
 
-tui_file::tui_file (FILE *stream)
-  : stdio_file (stream)
-{}
-
-/* All TUI I/O sent to the *_filtered and *_unfiltered functions
-   eventually ends up here.  The fputs_unfiltered_hook is primarily
-   used by GUIs to collect all output and send it to the GUI, instead
-   of the controlling terminal.  Only output to gdb_stdout and
-   gdb_stderr are sent to the hook.  Everything else is sent on to
-   fputs to allow file I/O to be handled appropriately.  */
-
 void
 tui_file::puts (const char *linebuffer)
 {
   tui_puts (linebuffer);
-  /* gdb_stdout is buffered, and the caller must gdb_flush it at
-     appropriate times.  Other streams are not so buffered.  */
-  if (this != gdb_stdout)
+  if (!m_buffered)
     tui_refresh_cmd_win ();
 }
 
@@ -47,18 +34,14 @@ void
 tui_file::write (const char *buf, long length_buf)
 {
   tui_write (buf, length_buf);
-  /* gdb_stdout is buffered, and the caller must gdb_flush it at
-     appropriate times.  Other streams are not so buffered.  */
-  if (this != gdb_stdout)
+  if (!m_buffered)
     tui_refresh_cmd_win ();
 }
 
 void
 tui_file::flush ()
 {
-  /* gdb_stdout is buffered.  Other files are always flushed on
-     every write.  */
-  if (this == gdb_stdout)
+  if (m_buffered)
     tui_refresh_cmd_win ();
   stdio_file::flush ();
 }
diff --git a/gdb/tui/tui-file.h b/gdb/tui/tui-file.h
index 1b28780c51b..ff60ded71c1 100644
--- a/gdb/tui/tui-file.h
+++ b/gdb/tui/tui-file.h
@@ -26,11 +26,19 @@
 class tui_file : public stdio_file
 {
 public:
-  explicit tui_file (FILE *stream);
+  tui_file (FILE *stream, bool buffered)
+    : stdio_file (stream),
+      m_buffered (buffered)
+  {}
 
   void write (const char *buf, long length_buf) override;
   void puts (const char *) override;
   void flush () override;
+
+private:
+
+  /* True if this stream is buffered.  */
+  bool m_buffered;
 };
 
 #endif /* TUI_TUI_FILE_H */
diff --git a/gdb/tui/tui-io.c b/gdb/tui/tui-io.c
index 9f27f8bcc01..0efaf69335c 100644
--- a/gdb/tui/tui-io.c
+++ b/gdb/tui/tui-io.c
@@ -904,8 +904,8 @@ tui_initialize_io (void)
 #endif
 
   /* Create tui output streams.  */
-  tui_stdout = new pager_file (new tui_file (stdout));
-  tui_stderr = new tui_file (stderr);
+  tui_stdout = new pager_file (new tui_file (stdout, true));
+  tui_stderr = new tui_file (stderr, false);
   tui_stdlog = new timestamped_file (tui_stderr);
   tui_out = new tui_ui_out (tui_stdout);
 
-- 
2.34.1

[PATCH 12/14] Remove a call to clear_interpreter_hooks

[Tom Tromey]

mi_interp::resume does not need to call clear_interpreter_hooks,
because this is already done by interp_set.
---
 gdb/mi/mi-interp.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/gdb/mi/mi-interp.c b/gdb/mi/mi-interp.c
index ee270c7c9d2..0a4d6713947 100644
--- a/gdb/mi/mi-interp.c
+++ b/gdb/mi/mi-interp.c
@@ -190,10 +190,6 @@ mi_interp::resume ()
   /* Route target error through the MI as well.  */
   gdb_stdtargerr = mi->targ;
 
-  /* Replace all the hooks that we know about.  There really needs to
-     be a better way of doing this... */
-  clear_interpreter_hooks ();
-
   deprecated_show_load_progress = mi_load_progress;
 }
 
-- 
2.34.1

[PATCH 13/14] Fix "source" with interpreter-exec

[Tom Tromey]

PR mi/15811 points out that "source"ing a file that uses
interpreter-exec will put gdb in a weird state, where the CLI stops
working.  The bug is that tui_interp::suspend does not unregister the
event file descriptor.

The test case is from Andrew Burgess.

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=15811
---
 gdb/testsuite/gdb.base/interpreter-exec.gdb | 20 ++++++++++++++++++++
 gdb/testsuite/gdb.base/source.exp           |  6 ++++++
 gdb/tui/tui-interp.c                        |  1 +
 3 files changed, 27 insertions(+)
 create mode 100644 gdb/testsuite/gdb.base/interpreter-exec.gdb

diff --git a/gdb/testsuite/gdb.base/interpreter-exec.gdb b/gdb/testsuite/gdb.base/interpreter-exec.gdb
new file mode 100644
index 00000000000..6afff23af0f
--- /dev/null
+++ b/gdb/testsuite/gdb.base/interpreter-exec.gdb
@@ -0,0 +1,20 @@
+# This testcase is part of GDB, the GNU debugger.
+
+# Copyright 2013 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# Test GDB's "source" command for scripts containing interpreter-exec.
+
+interpreter-exec mi "-gdb-show verbose"
diff --git a/gdb/testsuite/gdb.base/source.exp b/gdb/testsuite/gdb.base/source.exp
index 3d72b504832..9884a720420 100644
--- a/gdb/testsuite/gdb.base/source.exp
+++ b/gdb/testsuite/gdb.base/source.exp
@@ -73,3 +73,9 @@ gdb_test "source ${srcdir}/${subdir}/source-error.gdb" \
 		"source-error-1.gdb:21: Error in sourced command file:" \
 		"Cannot access memory at address 0x0" ] \
     "script contains error"
+
+# There was a case where sourcing a script containing "interpreter-exec"
+# commands would corrupt the interpreter mechanism and crash gdb.
+gdb_test "source ${srcdir}/${subdir}/interpreter-exec.gdb" \
+    "\\^done,value=\"off\"" \
+    "source interpreter-exec"
diff --git a/gdb/tui/tui-interp.c b/gdb/tui/tui-interp.c
index 1c4ffbbc3aa..e0846fe80c4 100644
--- a/gdb/tui/tui-interp.c
+++ b/gdb/tui/tui-interp.c
@@ -135,6 +135,7 @@ tui_interp::resume ()
 void
 tui_interp::suspend ()
 {
+  gdb_disable_readline ();
   tui_start_enabled = tui_active;
   tui_disable ();
 }
-- 
2.34.1

Re: [PATCH 13/14] Fix "source" with interpreter-exec

[Enze Li]

Hi Tom,

On Fri, 2022-08-12 at 18:54 -0600, Tom Tromey via Gdb-patches wrote:
> PR mi/15811 points out that "source"ing a file that uses
> interpreter-exec will put gdb in a weird state, where the CLI stops
> working.  The bug is that tui_interp::suspend does not unregister the
> event file descriptor.
> 
> The test case is from Andrew Burgess.
> 
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=15811
> ---
>  gdb/testsuite/gdb.base/interpreter-exec.gdb | 20
> ++++++++++++++++++++
>  gdb/testsuite/gdb.base/source.exp           |  6 ++++++
>  gdb/tui/tui-interp.c                        |  1 +
>  3 files changed, 27 insertions(+)
>  create mode 100644 gdb/testsuite/gdb.base/interpreter-exec.gdb
> 
> diff --git a/gdb/testsuite/gdb.base/interpreter-exec.gdb
> b/gdb/testsuite/gdb.base/interpreter-exec.gdb
> new file mode 100644
> index 00000000000..6afff23af0f
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/interpreter-exec.gdb
> @@ -0,0 +1,20 @@
> +# This testcase is part of GDB, the GNU debugger.
> +
> +# Copyright 2013 Free Software Foundation, Inc.
     ^^^^^^^^^^^^^^
Andrew created this test case in 2013, but it has not been modified
over the years.  In this case, do we need a range here?

Best Regards,
Enze

> +
> +# This program is free software; you can redistribute it and/or
> modify
> +# it under the terms of the GNU General Public License as published
> by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see
> <http://www.gnu.org/licenses/>.
> +
> +# Test GDB's "source" command for scripts containing interpreter-
> exec.
> +
> +interpreter-exec mi "-gdb-show verbose"
> diff --git a/gdb/testsuite/gdb.base/source.exp
> b/gdb/testsuite/gdb.base/source.exp
> index 3d72b504832..9884a720420 100644
> --- a/gdb/testsuite/gdb.base/source.exp
> +++ b/gdb/testsuite/gdb.base/source.exp
> @@ -73,3 +73,9 @@ gdb_test "source ${srcdir}/${subdir}/source-
> error.gdb" \
>                 "source-error-1.gdb:21: Error in sourced command
> file:" \
>                 "Cannot access memory at address 0x0" ] \
>      "script contains error"
> +
> +# There was a case where sourcing a script containing "interpreter-
> exec"
> +# commands would corrupt the interpreter mechanism and crash gdb.
> +gdb_test "source ${srcdir}/${subdir}/interpreter-exec.gdb" \
> +    "\\^done,value=\"off\"" \
> +    "source interpreter-exec"
> diff --git a/gdb/tui/tui-interp.c b/gdb/tui/tui-interp.c
> index 1c4ffbbc3aa..e0846fe80c4 100644
> --- a/gdb/tui/tui-interp.c
> +++ b/gdb/tui/tui-interp.c
> @@ -135,6 +135,7 @@ tui_interp::resume ()
>  void
>  tui_interp::suspend ()
>  {
> +  gdb_disable_readline ();
>    tui_start_enabled = tui_active;
>    tui_disable ();
>  }

Re: [PATCH 13/14] Fix "source" with interpreter-exec

[Tom Tromey]

>>>>> "Enze" == Enze Li <enze.li@hotmail.com> writes:

>> +# Copyright 2013 Free Software Foundation, Inc.
Enze>      ^^^^^^^^^^^^^^
Enze> Andrew created this test case in 2013, but it has not been modified
Enze> over the years.  In this case, do we need a range here?

Oops, I didn't even look at the date.
Yes, I think that makes sense.  I've changed it locally.

Tom

[PATCH 14/14] Fix interpreter-exec crash

[Tom Tromey]

PR mi/10347 points out that using interpreter-exec inside of a
"define" command will crash gdb.  The bug here is that
gdb_setup_readline doesn't check for the case where instream==nullptr.

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=10347
---
 gdb/event-top.c                   |  6 ++++--
 gdb/testsuite/gdb.base/interp.exp | 11 +++++++++++
 2 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/gdb/event-top.c b/gdb/event-top.c
index 151849c01ea..a131b571213 100644
--- a/gdb/event-top.c
+++ b/gdb/event-top.c
@@ -1330,8 +1330,10 @@ gdb_setup_readline (int editing)
 
   /* If the input stream is connected to a terminal, turn on editing.
      However, that is only allowed on the main UI, as we can only have
-     one instance of readline.  */
-  if (ISATTY (ui->instream) && editing && ui == main_ui)
+     one instance of readline.  Also, INSTREAM might be nullptr when
+     executing a user-defined command.  */
+  if (ui->instream != nullptr && ISATTY (ui->instream)
+      && editing && ui == main_ui)
     {
       /* Tell gdb that we will be using the readline library.  This
 	 could be overwritten by a command in .gdbinit like 'set
diff --git a/gdb/testsuite/gdb.base/interp.exp b/gdb/testsuite/gdb.base/interp.exp
index f748ffd8050..3db90f64103 100644
--- a/gdb/testsuite/gdb.base/interp.exp
+++ b/gdb/testsuite/gdb.base/interp.exp
@@ -79,6 +79,17 @@ gdb_test_multiple "interpreter-exec mi3 \"-break-insert main\"" "" {
     }
 }
 
+set test "define hello command"
+set commands "interpreter-exec mi3 \"-data-evaluate-expression 23\""
+gdb_test_multiple "define hello" "$test" {
+    -re "Type commands for definition of \"hello\".\r\nEnd with a line saying just \"end\".\r\n>$" {
+	pass "$test"
+    }
+}
+gdb_test "$commands\nend" "" "finish defining hello command"
+
+gdb_test "hello" [string_to_regexp "^done,value=\"23\""]
+
 if ![runto_main] then {
   return -1
 }
-- 
2.34.1

Re: [PATCH 00/14] Minor ui / interp cleanups

[Tom Tromey]

>>>>> "Tom" == Tom Tromey via Gdb-patches <gdb-patches@sourceware.org> writes:

Tom> I've been looking at the interp/ui/logging code for a project I'm
Tom> working on.  While doing this, I found a number of smaller things to
Tom> clean up.  This series is the result.

Tom> Regression tested on x86-64 Fedora 34.

I'm checking these in now.

Tom