[PATCH 0/4] Minor 'struct ui' cleanups

[PATCH 0/4] Minor 'struct ui' cleanups

[Tom Tromey]

While looking into gdb 'struct ui' handling, I found a few spots that
could be cleaned up.

Regression tested on x86-64 Fedora 34.

Tom

[PATCH 1/4] Remove ui_register_input_event_handler

[Tom Tromey]

This patch removes ui_register_input_event_handler and
ui_unregister_input_event_handler, replacing them with methods on
'ui'.  It also changes gdb to use these methods everywhere, rather
than sometimes reaching in to the ui to manage the file descriptor
directly.
---
 gdb/event-top.c | 20 ++++++++++----------
 gdb/infcall.c   |  6 +++---
 gdb/infrun.c    |  2 +-
 gdb/top.h       | 12 ++++++------
 gdb/utils.c     |  4 ++--
 5 files changed, 22 insertions(+), 22 deletions(-)

diff --git a/gdb/event-top.c b/gdb/event-top.c
index 74960c8ed3c..2863a8aff69 100644
--- a/gdb/event-top.c
+++ b/gdb/event-top.c
@@ -491,7 +491,7 @@ stdin_event_handler (int error, gdb_client_data client_data)
       /* Switch to the main UI, so diagnostics always go there.  */
       current_ui = main_ui;
 
-      delete_file_handler (ui->input_fd);
+      ui->unregister_file_handler ();
       if (main_ui == ui)
 	{
 	  /* If stdin died, we may as well kill gdb.  */
@@ -531,18 +531,18 @@ stdin_event_handler (int error, gdb_client_data client_data)
 /* See top.h.  */
 
 void
-ui_register_input_event_handler (struct ui *ui)
+ui::register_file_handler ()
 {
-  add_file_handler (ui->input_fd, stdin_event_handler, ui,
-		    string_printf ("ui-%d", ui->num), true);
+  add_file_handler (input_fd, stdin_event_handler, this,
+		    string_printf ("ui-%d", num), true);
 }
 
 /* See top.h.  */
 
 void
-ui_unregister_input_event_handler (struct ui *ui)
+ui::unregister_file_handler ()
 {
-  delete_file_handler (ui->input_fd);
+  delete_file_handler (input_fd);
 }
 
 /* Re-enable stdin after the end of an execution command in
@@ -557,7 +557,7 @@ async_enable_stdin (void)
   if (ui->prompt_state == PROMPT_BLOCKED)
     {
       target_terminal::ours ();
-      ui_register_input_event_handler (ui);
+      ui->register_file_handler ();
       ui->prompt_state = PROMPT_NEEDED;
     }
 }
@@ -571,7 +571,7 @@ async_disable_stdin (void)
   struct ui *ui = current_ui;
 
   ui->prompt_state = PROMPT_BLOCKED;
-  delete_file_handler (ui->input_fd);
+  ui->unregister_file_handler ();
 }
 \f
 
@@ -1366,7 +1366,7 @@ gdb_setup_readline (int editing)
      Another source is going to be the target program (inferior), but
      that must be registered only when it actually exists (I.e. after
      we say 'run' or after we connect to a remote target.  */
-  ui_register_input_event_handler (ui);
+  ui->register_file_handler ();
 }
 
 /* Disable command input through the standard CLI channels.  Used in
@@ -1393,7 +1393,7 @@ gdb_disable_readline (void)
 
   if (ui->command_editing)
     gdb_rl_callback_handler_remove ();
-  delete_file_handler (ui->input_fd);
+  ui->unregister_file_handler ();
 }
 
 scoped_segv_handler_restore::scoped_segv_handler_restore (segv_handler_t new_handler)
diff --git a/gdb/infcall.c b/gdb/infcall.c
index 9334648ac0e..2acceed4b07 100644
--- a/gdb/infcall.c
+++ b/gdb/infcall.c
@@ -581,7 +581,7 @@ run_inferior_call (std::unique_ptr<call_thread_fsm> sm,
   ptid_t call_thread_ptid = call_thread->ptid;
   int was_running = call_thread->state == THREAD_RUNNING;
 
-  delete_file_handler (current_ui->input_fd);
+  current_ui->unregister_file_handler ();
 
   scoped_restore restore_in_infcall
     = make_scoped_restore (&call_thread->control.in_infcall, 1);
@@ -624,9 +624,9 @@ run_inferior_call (std::unique_ptr<call_thread_fsm> sm,
      state again here.  In other cases, stdin will be re-enabled by
      inferior_event_handler, when an exception is thrown.  */
   if (current_ui->prompt_state == PROMPT_BLOCKED)
-    delete_file_handler (current_ui->input_fd);
+    current_ui->unregister_file_handler ();
   else
-    ui_register_input_event_handler (current_ui);
+    current_ui->register_file_handler ();
 
   /* If the infcall does NOT succeed, normal_stop will have already
      finished the thread states.  However, on success, normal_stop
diff --git a/gdb/infrun.c b/gdb/infrun.c
index 02c98b50c8c..a6694230d29 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -4096,7 +4096,7 @@ check_curr_ui_sync_execution_done (void)
     {
       target_terminal::ours ();
       gdb::observers::sync_execution_done.notify ();
-      ui_register_input_event_handler (ui);
+      ui->register_file_handler ();
     }
 }
 
diff --git a/gdb/top.h b/gdb/top.h
index b26209e8c8d..18c49cc5513 100644
--- a/gdb/top.h
+++ b/gdb/top.h
@@ -148,6 +148,12 @@ struct ui
 
   /* The current ui_out.  */
   struct ui_out *m_current_uiout;
+
+  /* Register the UI's input file descriptor in the event loop.  */
+  void register_file_handler ();
+
+  /* Unregister the UI's input file descriptor from the event loop.  */
+  void unregister_file_handler ();
 };
 
 /* The main UI.  This is the UI that is bound to stdin/stdout/stderr.
@@ -211,12 +217,6 @@ ui_range all_uis ()
   return ui_range (ui_list);
 }
 
-/* Register the UI's input file descriptor in the event loop.  */
-extern void ui_register_input_event_handler (struct ui *ui);
-
-/* Unregister the UI's input file descriptor from the event loop.  */
-extern void ui_unregister_input_event_handler (struct ui *ui);
-
 /* From top.c.  */
 extern bool confirm;
 extern int inhibit_gdbinit;
diff --git a/gdb/utils.c b/gdb/utils.c
index 413a4f4d53b..5503acfd6bc 100644
--- a/gdb/utils.c
+++ b/gdb/utils.c
@@ -799,7 +799,7 @@ class scoped_input_handler
       m_ui (NULL)
   {
     target_terminal::ours ();
-    ui_register_input_event_handler (current_ui);
+    current_ui->register_file_handler ();
     if (current_ui->prompt_state == PROMPT_BLOCKED)
       m_ui = current_ui;
   }
@@ -807,7 +807,7 @@ class scoped_input_handler
   ~scoped_input_handler ()
   {
     if (m_ui != NULL)
-      ui_unregister_input_event_handler (m_ui);
+      m_ui->unregister_file_handler ();
   }
 
   DISABLE_COPY_AND_ASSIGN (scoped_input_handler);
-- 
2.34.1

[PATCH 2/4] Replace input_interactive_p with a method

[Tom Tromey]

This replaces the global input_interactive_p function with a new
method ui::input_interactive_p.
---
 gdb/cli/cli-script.c |  4 ++--
 gdb/defs.h           |  2 --
 gdb/event-top.c      |  4 ++--
 gdb/top.c            | 18 +++++++++---------
 gdb/top.h            |  5 ++++-
 gdb/utils.c          |  2 +-
 6 files changed, 18 insertions(+), 17 deletions(-)

diff --git a/gdb/cli/cli-script.c b/gdb/cli/cli-script.c
index aa73d5307b3..5f81db418bc 100644
--- a/gdb/cli/cli-script.c
+++ b/gdb/cli/cli-script.c
@@ -1176,7 +1176,7 @@ counted_command_line
 read_command_lines (const char *prompt_arg, int from_tty, int parse_commands,
 		    gdb::function_view<void (const char *)> validator)
 {
-  if (from_tty && input_interactive_p (current_ui))
+  if (from_tty && current_ui->input_interactive_p ())
     {
       if (deprecated_readline_begin_hook)
 	{
@@ -1203,7 +1203,7 @@ read_command_lines (const char *prompt_arg, int from_tty, int parse_commands,
 				   validator);
     }
 
-  if (from_tty && input_interactive_p (current_ui)
+  if (from_tty && current_ui->input_interactive_p ()
       && deprecated_readline_end_hook)
     {
       (*deprecated_readline_end_hook) ();
diff --git a/gdb/defs.h b/gdb/defs.h
index 99bfdd526ff..6ee804765af 100644
--- a/gdb/defs.h
+++ b/gdb/defs.h
@@ -319,8 +319,6 @@ extern void print_prompt (void);
 
 struct ui;
 
-extern int input_interactive_p (struct ui *);
-
 extern bool info_verbose;
 
 /* From printcmd.c */
diff --git a/gdb/event-top.c b/gdb/event-top.c
index 2863a8aff69..02b3786320f 100644
--- a/gdb/event-top.c
+++ b/gdb/event-top.c
@@ -687,7 +687,7 @@ handle_line_of_input (struct buffer *cmd_line_buffer,
     }
 
   /* Do history expansion if that is wished.  */
-  if (history_expansion_p && from_tty && input_interactive_p (current_ui))
+  if (history_expansion_p && from_tty && current_ui->input_interactive_p ())
     {
       char *cmd_expansion;
       int expanded;
@@ -729,7 +729,7 @@ handle_line_of_input (struct buffer *cmd_line_buffer,
      and then later fetch it from the value history and remove the
      '#'.  The kill ring is probably better, but some people are in
      the habit of commenting things out.  */
-  if (*cmd != '\0' && from_tty && input_interactive_p (current_ui))
+  if (*cmd != '\0' && from_tty && current_ui->input_interactive_p ())
     gdb_add_history (cmd);
 
   /* Save into global buffer if appropriate.  */
diff --git a/gdb/top.c b/gdb/top.c
index 86c9971fa6d..60835acd5e5 100644
--- a/gdb/top.c
+++ b/gdb/top.c
@@ -308,7 +308,7 @@ ui::ui (FILE *instream_, FILE *outstream_, FILE *errstream_)
     outstream (outstream_),
     errstream (errstream_),
     input_fd (fileno (instream)),
-    input_interactive_p (ISATTY (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)),
@@ -1405,13 +1405,13 @@ command_line_input (const char *prompt_arg, const char *annotation_suffix)
       /* Don't use fancy stuff if not talking to stdin.  */
       if (deprecated_readline_hook
 	  && from_tty
-	  && input_interactive_p (current_ui))
+	  && current_ui->input_interactive_p ())
 	{
 	  rl.reset ((*deprecated_readline_hook) (prompt));
 	}
       else if (command_editing_p
 	       && from_tty
-	       && input_interactive_p (current_ui))
+	       && current_ui->input_interactive_p ())
 	{
 	  rl.reset (gdb_readline_wrapper (prompt));
 	}
@@ -1875,7 +1875,7 @@ quit_force (int *exit_arg, int from_tty)
 	     any UI with a terminal, save history.  */
 	  for (ui *ui : all_uis ())
 	    {
-	      if (input_interactive_p (ui))
+	      if (ui->input_interactive_p ())
 		{
 		  save = 1;
 		  break;
@@ -1923,23 +1923,23 @@ show_interactive_mode (struct ui_file *file, int from_tty,
   if (interactive_mode == AUTO_BOOLEAN_AUTO)
     gdb_printf (file, "Debugger's interactive mode "
 		"is %s (currently %s).\n",
-		value, input_interactive_p (current_ui) ? "on" : "off");
+		value, current_ui->input_interactive_p () ? "on" : "off");
   else
     gdb_printf (file, "Debugger's interactive mode is %s.\n", value);
 }
 
 /* Returns whether GDB is running on an interactive terminal.  */
 
-int
-input_interactive_p (struct ui *ui)
+bool
+ui::input_interactive_p () const
 {
   if (batch_flag)
-    return 0;
+    return false;
 
   if (interactive_mode != AUTO_BOOLEAN_AUTO)
     return interactive_mode == AUTO_BOOLEAN_TRUE;
 
-  return ui->input_interactive_p;
+  return m_input_interactive_p;
 }
 \f
 static void
diff --git a/gdb/top.h b/gdb/top.h
index 18c49cc5513..5c1db84b2ce 100644
--- a/gdb/top.h
+++ b/gdb/top.h
@@ -124,7 +124,7 @@ struct ui
   /* Whether ISATTY returns true on input_fd.  Cached here because
      quit_force needs to know this _after_ input_fd might be
      closed.  */
-  int input_interactive_p;
+  bool m_input_interactive_p;
 
   /* See enum prompt_state's description.  */
   enum prompt_state prompt_state;
@@ -154,6 +154,9 @@ struct ui
 
   /* Unregister the UI's input file descriptor from the event loop.  */
   void unregister_file_handler ();
+
+  /* Return true if this UI's input fd is a tty.  */
+  bool input_interactive_p () const;
 };
 
 /* The main UI.  This is the UI that is bound to stdin/stdout/stderr.
diff --git a/gdb/utils.c b/gdb/utils.c
index 5503acfd6bc..b0841e1fe5e 100644
--- a/gdb/utils.c
+++ b/gdb/utils.c
@@ -880,7 +880,7 @@ defaulted_query (const char *ctlstr, const char defchar, va_list args)
      way, important error messages don't get lost when talking to GDB
      over a pipe.  */
   if (current_ui->instream != current_ui->stdin_stream
-      || !input_interactive_p (current_ui)
+      || !current_ui->input_interactive_p ()
       /* Restrict queries to the main UI.  */
       || current_ui != main_ui)
     {
-- 
2.34.1

[PATCH 3/4] Remove cli_out_new

[Tom Tromey]

cli_out_new is just a small wrapper around 'new'.  This patch removes
it, replacing it with uses of 'new' instead.
---
 gdb/cli-out.c        | 8 --------
 gdb/cli-out.h        | 4 +---
 gdb/cli/cli-interp.c | 2 +-
 gdb/mi/mi-interp.c   | 2 +-
 gdb/tui/tui-io.c     | 2 +-
 5 files changed, 4 insertions(+), 14 deletions(-)

diff --git a/gdb/cli-out.c b/gdb/cli-out.c
index e0802df352b..fdbed6f5e91 100644
--- a/gdb/cli-out.c
+++ b/gdb/cli-out.c
@@ -386,14 +386,6 @@ cli_ui_out::~cli_ui_out ()
 {
 }
 
-/* Initialize private members at startup.  */
-
-cli_ui_out *
-cli_out_new (struct ui_file *stream)
-{
-  return new cli_ui_out (stream, ui_source_list);
-}
-
 ui_file *
 cli_ui_out::set_stream (struct ui_file *stream)
 {
diff --git a/gdb/cli-out.h b/gdb/cli-out.h
index 3fc794b61a4..3f01fe0db6d 100644
--- a/gdb/cli-out.h
+++ b/gdb/cli-out.h
@@ -27,7 +27,7 @@ class cli_ui_out : public ui_out
 {
 public:
 
-  explicit cli_ui_out (ui_file *stream, ui_out_flags flags);
+  explicit cli_ui_out (ui_file *stream, ui_out_flags flags = ui_source_list);
   virtual ~cli_ui_out ();
 
   ui_file *set_stream (ui_file *stream);
@@ -113,8 +113,6 @@ class cli_ui_out : public ui_out
   std::vector<cli_progress_info> m_meters;
 };
 
-extern cli_ui_out *cli_out_new (struct ui_file *stream);
-
 extern void cli_display_match_list (char **matches, int len, int max);
 
 #endif
diff --git a/gdb/cli/cli-interp.c b/gdb/cli/cli-interp.c
index 036bc723b24..c26b6a75227 100644
--- a/gdb/cli/cli-interp.c
+++ b/gdb/cli/cli-interp.c
@@ -59,7 +59,7 @@ cli_interp::cli_interp (const char *name)
   : cli_interp_base (name)
 {
   /* Create a default uiout builder for the CLI.  */
-  this->cli_uiout = cli_out_new (gdb_stdout);
+  this->cli_uiout = new cli_ui_out (gdb_stdout);
 }
 
 cli_interp::~cli_interp ()
diff --git a/gdb/mi/mi-interp.c b/gdb/mi/mi-interp.c
index 8d6e0334a90..a14903a0718 100644
--- a/gdb/mi/mi-interp.c
+++ b/gdb/mi/mi-interp.c
@@ -141,7 +141,7 @@ mi_interp::init (bool top_level)
   mi->event_channel = new mi_console_file (mi->raw_stdout, "=", 0);
   mi->mi_uiout = mi_out_new (name ());
   gdb_assert (mi->mi_uiout != nullptr);
-  mi->cli_uiout = cli_out_new (mi->out);
+  mi->cli_uiout = new cli_ui_out (mi->out);
 
   if (top_level)
     {
diff --git a/gdb/tui/tui-io.c b/gdb/tui/tui-io.c
index 22c234a0dc2..deea9b90afc 100644
--- a/gdb/tui/tui-io.c
+++ b/gdb/tui/tui-io.c
@@ -910,7 +910,7 @@ tui_initialize_io (void)
   tui_out = tui_out_new (tui_stdout);
 
   /* Create the default UI.  */
-  tui_old_uiout = cli_out_new (gdb_stdout);
+  tui_old_uiout = new cli_ui_out (gdb_stdout);
 
 #ifdef TUI_USE_PIPE_FOR_READLINE
   /* Temporary solution for readline writing to stdout: redirect
-- 
2.34.1

[PATCH 4/4] Remove manual lifetime management from cli_interp

[Tom Tromey]

cli_interp manually manages its cli_out object.  This patch changes it
to use a unique_ptr, and also changes cli_uiout to be a private
member.
---
 gdb/cli/cli-interp.c | 36 ++++++++++++++----------------------
 1 file changed, 14 insertions(+), 22 deletions(-)

diff --git a/gdb/cli/cli-interp.c b/gdb/cli/cli-interp.c
index c26b6a75227..ca3a1abcaae 100644
--- a/gdb/cli/cli-interp.c
+++ b/gdb/cli/cli-interp.c
@@ -43,7 +43,7 @@ class cli_interp final : public cli_interp_base
 {
  public:
   explicit cli_interp (const char *name);
-  ~cli_interp ();
+  ~cli_interp () = default;
 
   void init (bool top_level) override;
   void resume () override;
@@ -51,20 +51,16 @@ class cli_interp final : public cli_interp_base
   gdb_exception exec (const char *command_str) override;
   ui_out *interp_ui_out () override;
 
+private:
+
   /* The ui_out for the console interpreter.  */
-  cli_ui_out *cli_uiout;
+  std::unique_ptr<cli_ui_out> m_cli_uiout;
 };
 
 cli_interp::cli_interp (const char *name)
-  : cli_interp_base (name)
+  : cli_interp_base (name),
+    m_cli_uiout (new cli_ui_out (gdb_stdout))
 {
-  /* Create a default uiout builder for the CLI.  */
-  this->cli_uiout = new cli_ui_out (gdb_stdout);
-}
-
-cli_interp::~cli_interp ()
-{
-  delete cli_uiout;
 }
 
 /* Suppress notification struct.  */
@@ -289,7 +285,6 @@ void
 cli_interp::resume ()
 {
   struct ui *ui = current_ui;
-  struct cli_interp *cli = this;
   struct ui_file *stream;
 
   /*sync_execution = 1; */
@@ -298,10 +293,10 @@ cli_interp::resume ()
      previously writing to gdb_stdout, then set it to the new
      gdb_stdout afterwards.  */
 
-  stream = cli->cli_uiout->set_stream (gdb_stdout);
+  stream = m_cli_uiout->set_stream (gdb_stdout);
   if (stream != gdb_stdout)
     {
-      cli->cli_uiout->set_stream (stream);
+      m_cli_uiout->set_stream (stream);
       stream = NULL;
     }
 
@@ -310,7 +305,7 @@ cli_interp::resume ()
   ui->input_handler = command_line_handler;
 
   if (stream != NULL)
-    cli->cli_uiout->set_stream (gdb_stdout);
+    m_cli_uiout->set_stream (gdb_stdout);
 }
 
 void
@@ -322,20 +317,19 @@ cli_interp::suspend ()
 gdb_exception
 cli_interp::exec (const char *command_str)
 {
-  struct cli_interp *cli = this;
   struct ui_file *old_stream;
   struct gdb_exception result;
 
-  /* gdb_stdout could change between the time cli_uiout was
+  /* gdb_stdout could change between the time m_cli_uiout was
      initialized and now.  Since we're probably using a different
      interpreter which has a new ui_file for gdb_stdout, use that one
      instead of the default.
 
      It is important that it gets reset everytime, since the user
      could set gdb to use a different interpreter.  */
-  old_stream = cli->cli_uiout->set_stream (gdb_stdout);
-  result = safe_execute_command (cli->cli_uiout, command_str, 1);
-  cli->cli_uiout->set_stream (old_stream);
+  old_stream = m_cli_uiout->set_stream (gdb_stdout);
+  result = safe_execute_command (m_cli_uiout.get (), command_str, 1);
+  m_cli_uiout->set_stream (old_stream);
   return result;
 }
 
@@ -373,9 +367,7 @@ safe_execute_command (struct ui_out *command_uiout, const char *command,
 ui_out *
 cli_interp::interp_ui_out ()
 {
-  struct cli_interp *cli = (struct cli_interp *) this;
-
-  return cli->cli_uiout;
+  return m_cli_uiout.get ();
 }
 
 /* These hold the pushed copies of the gdb output files.
-- 
2.34.1

Re: [PATCH 0/4] Minor 'struct ui' cleanups

[Tom Tromey]

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

> While looking into gdb 'struct ui' handling, I found a few spots that
> could be cleaned up.

> Regression tested on x86-64 Fedora 34.

I'm checking these in.

Tom