[PATCH 00/21] ui / interp cleansup

[PATCH 00/21] ui / interp cleansup

[Simon Marchi]

When trying to untangle things in the ui and interp area (notably,
trying to understand what happens when setting and restoring
interpreters, in the context of interpreter-exec), I came up with a few
cleanups.  Here is a first round.

One goal is to make some interp operations be methods of struct ui.
Interpreters are children of UIs, so I think it makes sense if the
process of looking up / creating interpreters is implemented in methods
of struct ui.  The concept of top level interpreter is also per UI, so I
made getting and setting the top level interpreter methods of struct ui.
I plan to do the same with the current and the command interpreter
later, but it wasn't as obvious.

Another goal is to try to reduce the number of references to current_ui
in the code that manages interpreters.  Such references are often behind
macros, which in my opinion makes it difficult to understand how things
work.  To this end, there is one patch that adds a backlink from
interpreters to their parent UI, so that interpreters know which UI they
belong to (and they should never need to touch current_ui).

Simon Marchi (21):
  gdb: use intrusive_list for struct ui linked list
  gdb: make interp_lookup_existing a method of struct ui
  gdb: make interp_add a method of struct ui
  gdb: make interp_lookup a method of struct ui
  gdb: remove ui:::add_interp and ui::lookup_existing_interp
  gdb: uncover some current_ui uses in interp_set
  gdb: add backlink to ui in interp
  gdb: pass ui down to gdb_setup_readline and gdb_disable_readline
  gdb/python: use m_ui instead of current_ui in dap_interp::init
  gdb/mi: use m_ui instead of current_ui in mi_interp::init
  gdb/cli: use m_ui instead of current_ui in cli_interp::resume
  gdb/tui: use m_ui instead of current_ui in tui_interp::resume
  gdb/mi: use m_ui instead of current_ui in mi_interp::resume
  gdb/cli: use m_ui instead of current_ui in cli_interp::suspend
  gdb/tui: use m_ui instead of current_ui in tui_interp::suspend
  gdb/mi: use m_ui instead of current_ui in mi_interp::suspend
  gdb: pass current_ui down to interp_set
  gdb: make interp_set a method of struct ui
  gdb: pass down current_ui to set_top_level_interpreter
  gdb: make set_top_level_interpreter a method of struct ui
  gdb: make top_level_interpreter a method of struct ui

 gdb/cli/cli-interp.c |  25 +++----
 gdb/cli/cli-interp.h |   2 +-
 gdb/cli/cli-script.c |   2 +-
 gdb/event-top.c      |  10 +--
 gdb/event-top.h      |   4 +-
 gdb/infrun.c         |   8 +--
 gdb/interps.c        | 159 ++++---------------------------------------
 gdb/interps.h        |  40 ++++++-----
 gdb/linespec.c       |   3 +-
 gdb/main.c           |   6 +-
 gdb/mi/mi-interp.c   |  23 +++----
 gdb/mi/mi-interp.h   |   4 +-
 gdb/python/py-dap.c  |  12 ++--
 gdb/python/python.c  |   2 +-
 gdb/solib.c          |   4 +-
 gdb/top.c            |   6 +-
 gdb/tui/tui-interp.c |  19 +++---
 gdb/tui/tui.c        |   2 +-
 gdb/ui.c             | 124 +++++++++++++++++++++++++--------
 gdb/ui.h             |  64 ++++++++++++-----
 gdb/utils.c          |   5 +-
 21 files changed, 243 insertions(+), 281 deletions(-)


base-commit: 15db2284f2f8259e46635ca6df3efc772d951fac
-- 
2.42.0

[PATCH 01/21] gdb: use intrusive_list for struct ui linked list

[Simon Marchi]

Replace the hand-made list with intrusive_list.

Change-Id: If003cafda2f680794b9b6ae63236f19dd40908af
---
 gdb/infrun.c |  4 ++--
 gdb/top.c    |  4 ++--
 gdb/ui.c     | 29 ++++-------------------------
 gdb/ui.h     | 29 ++++++++++++-----------------
 4 files changed, 20 insertions(+), 46 deletions(-)

diff --git a/gdb/infrun.c b/gdb/infrun.c
index 4730d2904423..aaa7b2de14cd 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -5454,9 +5454,9 @@ handle_no_resumed (struct execution_control_state *ecs)
     {
       bool any_sync = false;
 
-      for (ui *ui : all_uis ())
+      for (ui &ui : all_uis ())
 	{
-	  if (ui->prompt_state == PROMPT_BLOCKED)
+	  if (ui.prompt_state == PROMPT_BLOCKED)
 	    {
 	      any_sync = true;
 	      break;
diff --git a/gdb/top.c b/gdb/top.c
index 2322e55f1db4..483e201140e7 100644
--- a/gdb/top.c
+++ b/gdb/top.c
@@ -1783,9 +1783,9 @@ quit_force (int *exit_arg, int from_tty)
 
 	  /* History is currently shared between all UIs.  If there's
 	     any UI with a terminal, save history.  */
-	  for (ui *ui : all_uis ())
+	  for (ui &ui : all_uis ())
 	    {
-	      if (ui->input_interactive_p ())
+	      if (ui.input_interactive_p ())
 		{
 		  save = 1;
 		  break;
diff --git a/gdb/ui.c b/gdb/ui.c
index 38ec61ea6731..f3dd14bfd3e0 100644
--- a/gdb/ui.c
+++ b/gdb/ui.c
@@ -33,7 +33,7 @@
 
 struct ui *main_ui;
 struct ui *current_ui;
-struct ui *ui_list;
+intrusive_list<ui> ui_list;
 
 /* The highest UI number ever assigned.  */
 
@@ -56,34 +56,13 @@ ui::ui (FILE *instream_, FILE *outstream_, FILE *errstream_)
 {
   unbuffer_stream (instream_);
 
-  if (ui_list == NULL)
-    ui_list = this;
-  else
-    {
-      struct ui *last;
-
-      for (last = ui_list; last->next != NULL; last = last->next)
-	;
-      last->next = this;
-    }
+  ui_list.push_back (*this);
 }
 
 ui::~ui ()
 {
-  struct ui *ui, *uiprev;
-
-  uiprev = NULL;
-
-  for (ui = ui_list; ui != NULL; uiprev = ui, ui = ui->next)
-    if (ui == this)
-      break;
-
-  gdb_assert (ui != NULL);
-
-  if (uiprev != NULL)
-    uiprev->next = next;
-  else
-    ui_list = next;
+  gdb_assert (this->is_linked ());
+  ui_list.erase (ui_list.iterator_to (*this));
 
   delete m_gdb_stdin;
   delete m_gdb_stdout;
diff --git a/gdb/ui.h b/gdb/ui.h
index ed75e041e5f2..c5e65c0393d8 100644
--- a/gdb/ui.h
+++ b/gdb/ui.h
@@ -51,7 +51,7 @@ enum prompt_state
    example, to create a separate MI channel on its own stdio
    streams.  */
 
-struct ui
+struct ui : public intrusive_list_node<ui>
 {
   /* Create a new UI.  */
   ui (FILE *instream, FILE *outstream, FILE *errstream);
@@ -59,9 +59,6 @@ struct ui
 
   DISABLE_COPY_AND_ASSIGN (ui);
 
-  /* Pointer to next in singly-linked list.  */
-  struct ui *next = nullptr;
-
   /* Convenient handle (UI number).  Unique across all UIs.  */
   int num;
 
@@ -172,16 +169,17 @@ extern struct ui *main_ui;
 extern struct ui *current_ui;
 
 /* The list of all UIs.  */
-extern struct ui *ui_list;
+extern intrusive_list<ui> ui_list;
 
 /* State for SWITCH_THRU_ALL_UIS.  */
 class switch_thru_all_uis
 {
 public:
 
-  switch_thru_all_uis () : m_iter (ui_list), m_save_ui (&current_ui)
+  switch_thru_all_uis () : m_iter (ui_list.begin ()), m_save_ui (&current_ui)
   {
-    current_ui = ui_list;
+    if (m_iter != ui_list.end ())
+      current_ui = &*m_iter;
   }
 
   DISABLE_COPY_AND_ASSIGN (switch_thru_all_uis);
@@ -189,22 +187,21 @@ class switch_thru_all_uis
   /* If done iterating, return true; otherwise return false.  */
   bool done () const
   {
-    return m_iter == NULL;
+    return m_iter == ui_list.end ();
   }
 
   /* Move to the next UI, setting current_ui if iteration is not yet
      complete.  */
   void next ()
   {
-    m_iter = m_iter->next;
-    if (m_iter != NULL)
-      current_ui = m_iter;
+    ++m_iter;
+    if (m_iter != ui_list.end ())
+      current_ui = &*m_iter;
   }
 
  private:
-
   /* Used to iterate through the UIs.  */
-  struct ui *m_iter;
+  intrusive_list<ui>::iterator m_iter;
 
   /* Save and restore current_ui.  */
   scoped_restore_tmpl<struct ui *> m_save_ui;
@@ -215,13 +212,11 @@ class switch_thru_all_uis
 #define SWITCH_THRU_ALL_UIS()		\
   for (switch_thru_all_uis stau_state; !stau_state.done (); stau_state.next ())
 
-using ui_range = next_range<ui>;
-
 /* An adapter that can be used to traverse over all UIs.  */
 static inline
-ui_range all_uis ()
+intrusive_list<ui> &all_uis ()
 {
-  return ui_range (ui_list);
+  return ui_list;
 }
 
 #endif /* UI_H */
-- 
2.42.0

[PATCH 02/21] gdb: make interp_lookup_existing a method of struct ui

[Simon Marchi]

No behavior changes expected.

Change-Id: I2bd15b70425326a3b499b9346217b93f76175999
---
 gdb/interps.c | 22 ++--------------------
 gdb/ui.c      | 12 ++++++++++++
 gdb/ui.h      |  4 ++++
 3 files changed, 18 insertions(+), 20 deletions(-)

diff --git a/gdb/interps.c b/gdb/interps.c
index adac98125239..3ddcfe9566fe 100644
--- a/gdb/interps.c
+++ b/gdb/interps.c
@@ -41,11 +41,6 @@
 #include "gdbsupport/buildargv.h"
 #include "gdbsupport/scope-exit.h"
 
-/* The magic initialization routine for this module.  */
-
-static struct interp *interp_lookup_existing (struct ui *ui,
-					      const char *name);
-
 interp::interp (const char *name)
   : m_name (name)
 {
@@ -93,7 +88,7 @@ interp_factory_register (const char *name, interp_factory_func func)
 static void
 interp_add (struct ui *ui, struct interp *interp)
 {
-  gdb_assert (interp_lookup_existing (ui, interp->name ()) == NULL);
+  gdb_assert (ui->lookup_existing_interp (interp->name ()) == nullptr);
 
   ui->interp_list.push_back (*interp);
 }
@@ -158,19 +153,6 @@ interp_set (struct interp *interp, bool top_level)
 	       "to a newer version of MI."));
 }
 
-/* Look up the interpreter for NAME.  If no such interpreter exists,
-   return NULL, otherwise return a pointer to the interpreter.  */
-
-static struct interp *
-interp_lookup_existing (struct ui *ui, const char *name)
-{
-  for (interp &interp : ui->interp_list)
-    if (strcmp (interp.name (), name) == 0)
-      return &interp;
-
-  return nullptr;
-}
-
 /* See interps.h.  */
 
 struct interp *
@@ -180,7 +162,7 @@ interp_lookup (struct ui *ui, const char *name)
     return NULL;
 
   /* Only create each interpreter once per top level.  */
-  struct interp *interp = interp_lookup_existing (ui, name);
+  interp *interp = ui->lookup_existing_interp (name);
   if (interp != NULL)
     return interp;
 
diff --git a/gdb/ui.c b/gdb/ui.c
index f3dd14bfd3e0..ae87dcda2453 100644
--- a/gdb/ui.c
+++ b/gdb/ui.c
@@ -147,6 +147,18 @@ ui::register_file_handler ()
 		      string_printf ("ui-%d", num), true);
 }
 
+/* See ui.h.  */
+
+interp *
+ui::lookup_existing_interp (const char *name) const
+{
+  for (interp &interp : this->interp_list)
+    if (strcmp (interp.name (), name) == 0)
+      return &interp;
+
+  return nullptr;
+}
+
 /* See top.h.  */
 
 void
diff --git a/gdb/ui.h b/gdb/ui.h
index c5e65c0393d8..23c6b2e1a3d9 100644
--- a/gdb/ui.h
+++ b/gdb/ui.h
@@ -158,6 +158,10 @@ struct ui : public intrusive_list_node<ui>
 
   /* Return true if this UI's input fd is a tty.  */
   bool input_interactive_p () const;
+
+  /* Look up the interpreter for NAME.  If no such interpreter exists,
+     return nullptr, otherwise return a pointer to the interpreter.  */
+  interp *lookup_existing_interp (const char *name) const;
 };
 
 /* The main UI.  This is the UI that is bound to stdin/stdout/stderr.
-- 
2.42.0

[PATCH 03/21] gdb: make interp_add a method of struct ui

[Simon Marchi]

No behavior changes expected.

Change-Id: Ice8958068fd4025e23bff87bcdee45a5f64a41c4
---
 gdb/interps.c | 12 +-----------
 gdb/ui.c      | 10 ++++++++++
 gdb/ui.h      |  4 ++++
 3 files changed, 15 insertions(+), 11 deletions(-)

diff --git a/gdb/interps.c b/gdb/interps.c
index 3ddcfe9566fe..b05a6c4eb875 100644
--- a/gdb/interps.c
+++ b/gdb/interps.c
@@ -83,16 +83,6 @@ interp_factory_register (const char *name, interp_factory_func func)
   interpreter_factories.emplace_back (name, func);
 }
 
-/* Add interpreter INTERP to the gdb interpreter list.  The
-   interpreter must not have previously been added.  */
-static void
-interp_add (struct ui *ui, struct interp *interp)
-{
-  gdb_assert (ui->lookup_existing_interp (interp->name ()) == nullptr);
-
-  ui->interp_list.push_back (*interp);
-}
-
 /* This sets the current interpreter to be INTERP.  If INTERP has not
    been initialized, then this will also run the init method.
 
@@ -170,7 +160,7 @@ interp_lookup (struct ui *ui, const char *name)
     if (strcmp (factory.name, name) == 0)
       {
 	interp = factory.func (factory.name);
-	interp_add (ui, interp);
+	ui->add_interp (interp);
 	return interp;
       }
 
diff --git a/gdb/ui.c b/gdb/ui.c
index ae87dcda2453..624187ac73c3 100644
--- a/gdb/ui.c
+++ b/gdb/ui.c
@@ -159,6 +159,16 @@ ui::lookup_existing_interp (const char *name) const
   return nullptr;
 }
 
+/* See ui.h.  */
+
+void
+ui::add_interp (interp *interp)
+{
+  gdb_assert (this->lookup_existing_interp (interp->name ()) == nullptr);
+
+  this->interp_list.push_back (*interp);
+}
+
 /* See top.h.  */
 
 void
diff --git a/gdb/ui.h b/gdb/ui.h
index 23c6b2e1a3d9..48cad96b3fb8 100644
--- a/gdb/ui.h
+++ b/gdb/ui.h
@@ -162,6 +162,10 @@ struct ui : public intrusive_list_node<ui>
   /* Look up the interpreter for NAME.  If no such interpreter exists,
      return nullptr, otherwise return a pointer to the interpreter.  */
   interp *lookup_existing_interp (const char *name) const;
+
+  /* Add interpreter INTERP to this UI's interpreter list.  The
+     interpreter must not have previously been added.  */
+  void add_interp (interp *interp);
 };
 
 /* The main UI.  This is the UI that is bound to stdin/stdout/stderr.
-- 
2.42.0

[PATCH 04/21] gdb: make interp_lookup a method of struct ui

[Simon Marchi]

This requires exposing some things about interpreter factories in
interps.h, for ui.c to use it.  No behavior changes expected.

Change-Id: I7a9e93621c0588e367b5356916d4dad90757bb3d
---
 gdb/cli/cli-script.c |  2 +-
 gdb/interps.c        | 58 ++++++++++++--------------------------------
 gdb/interps.h        | 25 ++++++++++++++-----
 gdb/mi/mi-interp.c   |  4 +--
 gdb/python/python.c  |  2 +-
 gdb/ui.c             | 25 ++++++++++++++++++-
 gdb/ui.h             |  6 +++++
 7 files changed, 68 insertions(+), 54 deletions(-)

diff --git a/gdb/cli/cli-script.c b/gdb/cli/cli-script.c
index 8ec5689ebcfd..49ba854c2a09 100644
--- a/gdb/cli/cli-script.c
+++ b/gdb/cli/cli-script.c
@@ -702,7 +702,7 @@ execute_control_command (struct command_line *cmd, int from_tty)
 
   /* Make sure we use the console uiout.  It's possible that we are executing
      breakpoint commands while running the MI interpreter.  */
-  interp *console = interp_lookup (current_ui, INTERP_CONSOLE);
+  interp *console = current_ui->lookup_interp (INTERP_CONSOLE);
   scoped_restore save_uiout
     = make_scoped_restore (&current_uiout, console->interp_ui_out ());
 
diff --git a/gdb/interps.c b/gdb/interps.c
index b05a6c4eb875..bd95c3175adb 100644
--- a/gdb/interps.c
+++ b/gdb/interps.c
@@ -48,22 +48,6 @@ interp::interp (const char *name)
 
 interp::~interp () = default;
 
-/* An interpreter factory.  Maps an interpreter name to the factory
-   function that instantiates an interpreter by that name.  */
-
-struct interp_factory
-{
-  interp_factory (const char *name_, interp_factory_func func_)
-  : name (name_), func (func_)
-  {}
-
-  /* This is the name in "-i=INTERP" and "interpreter-exec INTERP".  */
-  const char *name;
-
-  /* The function that creates the interpreter.  */
-  interp_factory_func func;
-};
-
 /* The registered interpreter factories.  */
 static std::vector<interp_factory> interpreter_factories;
 
@@ -83,6 +67,18 @@ interp_factory_register (const char *name, interp_factory_func func)
   interpreter_factories.emplace_back (name, func);
 }
 
+/* See interps.h.  */
+
+const interp_factory *
+find_interp_factory (const char *name)
+{
+  for (const interp_factory &factory : interpreter_factories)
+    if (strcmp (factory.name, name) == 0)
+      return &factory;
+
+  return nullptr;
+}
+
 /* This sets the current interpreter to be INTERP.  If INTERP has not
    been initialized, then this will also run the init method.
 
@@ -145,35 +141,11 @@ interp_set (struct interp *interp, bool top_level)
 
 /* See interps.h.  */
 
-struct interp *
-interp_lookup (struct ui *ui, const char *name)
-{
-  if (name == NULL || strlen (name) == 0)
-    return NULL;
-
-  /* Only create each interpreter once per top level.  */
-  interp *interp = ui->lookup_existing_interp (name);
-  if (interp != NULL)
-    return interp;
-
-  for (const interp_factory &factory : interpreter_factories)
-    if (strcmp (factory.name, name) == 0)
-      {
-	interp = factory.func (factory.name);
-	ui->add_interp (interp);
-	return interp;
-      }
-
-  return NULL;
-}
-
-/* See interps.h.  */
-
 void
 set_top_level_interpreter (const char *name)
 {
   /* Find it.  */
-  struct interp *interp = interp_lookup (current_ui, name);
+  struct interp *interp = current_ui->lookup_interp (name);
 
   if (interp == NULL)
     error (_("Interpreter `%s' unrecognized"), name);
@@ -194,7 +166,7 @@ current_interp_set_logging (ui_file_up logfile, bool logging_redirect,
 struct interp *
 scoped_restore_interp::set_interp (const char *name)
 {
-  struct interp *interp = interp_lookup (current_ui, name);
+  struct interp *interp = current_ui->lookup_interp (name);
   struct interp *old_interp = current_ui->current_interpreter;
 
   if (interp)
@@ -290,7 +262,7 @@ interpreter_exec_cmd (const char *args, int from_tty)
 
   interp *old_interp = current_ui->current_interpreter;
 
-  interp_to_use = interp_lookup (current_ui, prules[0]);
+  interp_to_use = current_ui->lookup_interp (prules[0]);
   if (interp_to_use == NULL)
     error (_("Could not find interpreter \"%s\"."), prules[0]);
 
diff --git a/gdb/interps.h b/gdb/interps.h
index 287df2c8c810..433d92439eba 100644
--- a/gdb/interps.h
+++ b/gdb/interps.h
@@ -36,6 +36,22 @@ struct trace_state_variable;
 
 typedef struct interp *(*interp_factory_func) (const char *name);
 
+/* An interpreter factory.  Maps an interpreter name to the factory
+   function that instantiates an interpreter by that name.  */
+
+struct interp_factory
+{
+  interp_factory (const char *name_, interp_factory_func func_)
+  : name (name_), func (func_)
+  {}
+
+  /* This is the name in "-i=INTERP" and "interpreter-exec INTERP".  */
+  const char *name;
+
+  /* The function that creates the interpreter.  */
+  interp_factory_func func;
+};
+
 /* Each interpreter kind (CLI, MI, etc.) registers itself with a call
    to this function, passing along its name, and a pointer to a
    function that creates a new instance of an interpreter with that
@@ -45,6 +61,9 @@ typedef struct interp *(*interp_factory_func) (const char *name);
 extern void interp_factory_register (const char *name,
 				     interp_factory_func func);
 
+/* Return the interpreter factory for NAME.  Return nullptr if not found.  */
+extern const interp_factory *find_interp_factory (const char *name);
+
 extern void interp_exec (struct interp *interp, const char *command);
 
 class interp : public intrusive_list_node<interp>
@@ -193,12 +212,6 @@ class interp : public intrusive_list_node<interp>
   bool inited = false;
 };
 
-/* Look up the interpreter for NAME, creating one if none exists yet.
-   If NAME is not a interpreter type previously registered with
-   interp_factory_register, return NULL; otherwise return a pointer to
-   the interpreter.  */
-extern struct interp *interp_lookup (struct ui *ui, const char *name);
-
 /* Set the current UI's top level interpreter to the interpreter named
    NAME.  Throws an error if NAME is not a known interpreter or the
    interpreter fails to initialize.  */
diff --git a/gdb/mi/mi-interp.c b/gdb/mi/mi-interp.c
index b3b0f5bb1f51..946fed5867c4 100644
--- a/gdb/mi/mi-interp.c
+++ b/gdb/mi/mi-interp.c
@@ -162,7 +162,7 @@ mi_cmd_interpreter_exec (const char *command, const char *const *argv,
     error (_("-interpreter-exec: "
 	     "Usage: -interpreter-exec interp command"));
 
-  interp_to_use = interp_lookup (current_ui, argv[0]);
+  interp_to_use = current_ui->lookup_interp (argv[0]);
   if (interp_to_use == NULL)
     error (_("-interpreter-exec: could not find interpreter \"%s\""),
 	   argv[0]);
@@ -416,7 +416,7 @@ mi_interp::on_normal_stop (struct bpstat *bs, int print_frame)
 	  mi_uiout->field_string ("reason", async_reason_lookup (reason));
 	}
 
-      interp *console_interp = interp_lookup (current_ui, INTERP_CONSOLE);
+      interp *console_interp = current_ui->lookup_interp (INTERP_CONSOLE);
 
       /* We only want to print the displays once, and we want it to
 	 look just how it would on the console, so we use this to
diff --git a/gdb/python/python.c b/gdb/python/python.c
index 6a978d632e9a..a1aaa29d0ce5 100644
--- a/gdb/python/python.c
+++ b/gdb/python/python.c
@@ -683,7 +683,7 @@ execute_gdb_command (PyObject *self, PyObject *args, PyObject *kw)
 
 	/* Use the console interpreter uiout to have the same print format
 	   for console or MI.  */
-	interp = interp_lookup (current_ui, "console");
+	interp = current_ui->lookup_interp ("console");
 	current_uiout = interp->interp_ui_out ();
 
 	if (to_string)
diff --git a/gdb/ui.c b/gdb/ui.c
index 624187ac73c3..a7b81c077e9a 100644
--- a/gdb/ui.c
+++ b/gdb/ui.c
@@ -169,7 +169,30 @@ ui::add_interp (interp *interp)
   this->interp_list.push_back (*interp);
 }
 
-/* See top.h.  */
+/* See interps.h.  */
+
+interp *
+ui::lookup_interp (const char *name)
+{
+  if (name == nullptr || strlen (name) == 0)
+    return nullptr;
+
+  /* Only create each interpreter once per UI.  */
+  interp *interp = this->lookup_existing_interp (name);
+  if (interp != nullptr)
+    return interp;
+
+  const interp_factory *factory = find_interp_factory (name);
+  if (factory == nullptr)
+    return nullptr;
+
+  interp = factory->func (factory->name);
+  this->add_interp (interp);
+
+  return interp;
+}
+
+/* See ui.h.  */
 
 void
 ui::unregister_file_handler ()
diff --git a/gdb/ui.h b/gdb/ui.h
index 48cad96b3fb8..aeb26c68823a 100644
--- a/gdb/ui.h
+++ b/gdb/ui.h
@@ -163,6 +163,12 @@ struct ui : public intrusive_list_node<ui>
      return nullptr, otherwise return a pointer to the interpreter.  */
   interp *lookup_existing_interp (const char *name) const;
 
+  /* Look up the interpreter for NAME, creating one if none exists yet.
+     If NAME is not a interpreter type previously registered with
+     interp_factory_register, return nullptr; otherwise return a pointer to
+     the interpreter.  */
+  interp *lookup_interp (const char *name);
+
   /* Add interpreter INTERP to this UI's interpreter list.  The
      interpreter must not have previously been added.  */
   void add_interp (interp *interp);
-- 
2.42.0

Re: [PATCH 04/21] gdb: make interp_lookup a method of struct ui

[Andrew Burgess]

Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:

> This requires exposing some things about interpreter factories in
> interps.h, for ui.c to use it.  No behavior changes expected.
>
> Change-Id: I7a9e93621c0588e367b5356916d4dad90757bb3d
> ---
>  gdb/cli/cli-script.c |  2 +-
>  gdb/interps.c        | 58 ++++++++++++--------------------------------
>  gdb/interps.h        | 25 ++++++++++++++-----
>  gdb/mi/mi-interp.c   |  4 +--
>  gdb/python/python.c  |  2 +-
>  gdb/ui.c             | 25 ++++++++++++++++++-
>  gdb/ui.h             |  6 +++++
>  7 files changed, 68 insertions(+), 54 deletions(-)
>
> diff --git a/gdb/cli/cli-script.c b/gdb/cli/cli-script.c
> index 8ec5689ebcfd..49ba854c2a09 100644
> --- a/gdb/cli/cli-script.c
> +++ b/gdb/cli/cli-script.c
> @@ -702,7 +702,7 @@ execute_control_command (struct command_line *cmd, int from_tty)
>  
>    /* Make sure we use the console uiout.  It's possible that we are executing
>       breakpoint commands while running the MI interpreter.  */
> -  interp *console = interp_lookup (current_ui, INTERP_CONSOLE);
> +  interp *console = current_ui->lookup_interp (INTERP_CONSOLE);
>    scoped_restore save_uiout
>      = make_scoped_restore (&current_uiout, console->interp_ui_out ());
>  
> diff --git a/gdb/interps.c b/gdb/interps.c
> index b05a6c4eb875..bd95c3175adb 100644
> --- a/gdb/interps.c
> +++ b/gdb/interps.c
> @@ -48,22 +48,6 @@ interp::interp (const char *name)
>  
>  interp::~interp () = default;
>  
> -/* An interpreter factory.  Maps an interpreter name to the factory
> -   function that instantiates an interpreter by that name.  */
> -
> -struct interp_factory
> -{
> -  interp_factory (const char *name_, interp_factory_func func_)
> -  : name (name_), func (func_)
> -  {}
> -
> -  /* This is the name in "-i=INTERP" and "interpreter-exec INTERP".  */
> -  const char *name;
> -
> -  /* The function that creates the interpreter.  */
> -  interp_factory_func func;
> -};
> -
>  /* The registered interpreter factories.  */
>  static std::vector<interp_factory> interpreter_factories;
>  
> @@ -83,6 +67,18 @@ interp_factory_register (const char *name, interp_factory_func func)
>    interpreter_factories.emplace_back (name, func);
>  }
>  
> +/* See interps.h.  */
> +
> +const interp_factory *
> +find_interp_factory (const char *name)
> +{
> +  for (const interp_factory &factory : interpreter_factories)
> +    if (strcmp (factory.name, name) == 0)
> +      return &factory;
> +
> +  return nullptr;
> +}
> +
>  /* This sets the current interpreter to be INTERP.  If INTERP has not
>     been initialized, then this will also run the init method.
>  
> @@ -145,35 +141,11 @@ interp_set (struct interp *interp, bool top_level)
>  
>  /* See interps.h.  */
>  
> -struct interp *
> -interp_lookup (struct ui *ui, const char *name)
> -{
> -  if (name == NULL || strlen (name) == 0)
> -    return NULL;
> -
> -  /* Only create each interpreter once per top level.  */
> -  interp *interp = ui->lookup_existing_interp (name);
> -  if (interp != NULL)
> -    return interp;
> -
> -  for (const interp_factory &factory : interpreter_factories)
> -    if (strcmp (factory.name, name) == 0)
> -      {
> -	interp = factory.func (factory.name);
> -	ui->add_interp (interp);
> -	return interp;
> -      }
> -
> -  return NULL;
> -}
> -
> -/* See interps.h.  */
> -
>  void
>  set_top_level_interpreter (const char *name)
>  {
>    /* Find it.  */
> -  struct interp *interp = interp_lookup (current_ui, name);
> +  struct interp *interp = current_ui->lookup_interp (name);
>  
>    if (interp == NULL)
>      error (_("Interpreter `%s' unrecognized"), name);
> @@ -194,7 +166,7 @@ current_interp_set_logging (ui_file_up logfile, bool logging_redirect,
>  struct interp *
>  scoped_restore_interp::set_interp (const char *name)
>  {
> -  struct interp *interp = interp_lookup (current_ui, name);
> +  struct interp *interp = current_ui->lookup_interp (name);
>    struct interp *old_interp = current_ui->current_interpreter;
>  
>    if (interp)
> @@ -290,7 +262,7 @@ interpreter_exec_cmd (const char *args, int from_tty)
>  
>    interp *old_interp = current_ui->current_interpreter;
>  
> -  interp_to_use = interp_lookup (current_ui, prules[0]);
> +  interp_to_use = current_ui->lookup_interp (prules[0]);
>    if (interp_to_use == NULL)
>      error (_("Could not find interpreter \"%s\"."), prules[0]);
>  
> diff --git a/gdb/interps.h b/gdb/interps.h
> index 287df2c8c810..433d92439eba 100644
> --- a/gdb/interps.h
> +++ b/gdb/interps.h
> @@ -36,6 +36,22 @@ struct trace_state_variable;
>  
>  typedef struct interp *(*interp_factory_func) (const char *name);
>  
> +/* An interpreter factory.  Maps an interpreter name to the factory
> +   function that instantiates an interpreter by that name.  */
> +
> +struct interp_factory
> +{
> +  interp_factory (const char *name_, interp_factory_func func_)
> +  : name (name_), func (func_)
> +  {}
> +
> +  /* This is the name in "-i=INTERP" and "interpreter-exec INTERP".  */
> +  const char *name;
> +
> +  /* The function that creates the interpreter.  */
> +  interp_factory_func func;
> +};
> +
>  /* Each interpreter kind (CLI, MI, etc.) registers itself with a call
>     to this function, passing along its name, and a pointer to a
>     function that creates a new instance of an interpreter with that
> @@ -45,6 +61,9 @@ typedef struct interp *(*interp_factory_func) (const char *name);
>  extern void interp_factory_register (const char *name,
>  				     interp_factory_func func);
>  
> +/* Return the interpreter factory for NAME.  Return nullptr if not found.  */
> +extern const interp_factory *find_interp_factory (const char *name);
> +
>  extern void interp_exec (struct interp *interp, const char *command);
>  
>  class interp : public intrusive_list_node<interp>
> @@ -193,12 +212,6 @@ class interp : public intrusive_list_node<interp>
>    bool inited = false;
>  };
>  
> -/* Look up the interpreter for NAME, creating one if none exists yet.
> -   If NAME is not a interpreter type previously registered with
> -   interp_factory_register, return NULL; otherwise return a pointer to
> -   the interpreter.  */
> -extern struct interp *interp_lookup (struct ui *ui, const char *name);
> -
>  /* Set the current UI's top level interpreter to the interpreter named
>     NAME.  Throws an error if NAME is not a known interpreter or the
>     interpreter fails to initialize.  */
> diff --git a/gdb/mi/mi-interp.c b/gdb/mi/mi-interp.c
> index b3b0f5bb1f51..946fed5867c4 100644
> --- a/gdb/mi/mi-interp.c
> +++ b/gdb/mi/mi-interp.c
> @@ -162,7 +162,7 @@ mi_cmd_interpreter_exec (const char *command, const char *const *argv,
>      error (_("-interpreter-exec: "
>  	     "Usage: -interpreter-exec interp command"));
>  
> -  interp_to_use = interp_lookup (current_ui, argv[0]);
> +  interp_to_use = current_ui->lookup_interp (argv[0]);
>    if (interp_to_use == NULL)
>      error (_("-interpreter-exec: could not find interpreter \"%s\""),
>  	   argv[0]);
> @@ -416,7 +416,7 @@ mi_interp::on_normal_stop (struct bpstat *bs, int print_frame)
>  	  mi_uiout->field_string ("reason", async_reason_lookup (reason));
>  	}
>  
> -      interp *console_interp = interp_lookup (current_ui, INTERP_CONSOLE);
> +      interp *console_interp = current_ui->lookup_interp (INTERP_CONSOLE);
>  
>        /* We only want to print the displays once, and we want it to
>  	 look just how it would on the console, so we use this to
> diff --git a/gdb/python/python.c b/gdb/python/python.c
> index 6a978d632e9a..a1aaa29d0ce5 100644
> --- a/gdb/python/python.c
> +++ b/gdb/python/python.c
> @@ -683,7 +683,7 @@ execute_gdb_command (PyObject *self, PyObject *args, PyObject *kw)
>  
>  	/* Use the console interpreter uiout to have the same print format
>  	   for console or MI.  */
> -	interp = interp_lookup (current_ui, "console");
> +	interp = current_ui->lookup_interp ("console");

Might as well take this opportunity to replace "console" with
INTERP_CONSOLE which is what we use everywhere else.

>  	current_uiout = interp->interp_ui_out ();
>  
>  	if (to_string)
> diff --git a/gdb/ui.c b/gdb/ui.c
> index 624187ac73c3..a7b81c077e9a 100644
> --- a/gdb/ui.c
> +++ b/gdb/ui.c
> @@ -169,7 +169,30 @@ ui::add_interp (interp *interp)
>    this->interp_list.push_back (*interp);
>  }
>  
> -/* See top.h.  */
> +/* See interps.h.  */
> +
> +interp *
> +ui::lookup_interp (const char *name)
> +{
> +  if (name == nullptr || strlen (name) == 0)
> +    return nullptr;
> +
> +  /* Only create each interpreter once per UI.  */
> +  interp *interp = this->lookup_existing_interp (name);
> +  if (interp != nullptr)
> +    return interp;
> +
> +  const interp_factory *factory = find_interp_factory (name);
> +  if (factory == nullptr)
> +    return nullptr;
> +
> +  interp = factory->func (factory->name);
> +  this->add_interp (interp);
> +
> +  return interp;
> +}

Rather than exposing the interpreter factor stuff outside of interp.c, I
think a better approach would be either move ui::lookup_interp into
interp.c -- personally, I'm happy to see different parts of an object in
different .c files when that makes sense.

But if you prefer all to keep ui implementation in ui.c, rather than
calling find_interp_factory and then factory->func, we could just have a
new function `create_interp (name)` which creates an interpreter, or
returns nullptr, then we can write:

  interp = create_interp (name);
  if (interp != nullptr)
    this->add_interp (interp);
  return interp;

And all the factory stuff can remain private to interp.c.

Thoughts?

Thanks,
Andrew

> +
> +/* See ui.h.  */
>  
>  void
>  ui::unregister_file_handler ()
> diff --git a/gdb/ui.h b/gdb/ui.h
> index 48cad96b3fb8..aeb26c68823a 100644
> --- a/gdb/ui.h
> +++ b/gdb/ui.h
> @@ -163,6 +163,12 @@ struct ui : public intrusive_list_node<ui>
>       return nullptr, otherwise return a pointer to the interpreter.  */
>    interp *lookup_existing_interp (const char *name) const;
>  
> +  /* Look up the interpreter for NAME, creating one if none exists yet.
> +     If NAME is not a interpreter type previously registered with
> +     interp_factory_register, return nullptr; otherwise return a pointer to
> +     the interpreter.  */
> +  interp *lookup_interp (const char *name);
> +
>    /* Add interpreter INTERP to this UI's interpreter list.  The
>       interpreter must not have previously been added.  */
>    void add_interp (interp *interp);
> -- 
> 2.42.0

Re: [PATCH 04/21] gdb: make interp_lookup a method of struct ui

[Simon Marchi]

>> diff --git a/gdb/python/python.c b/gdb/python/python.c
>> index 6a978d632e9a..a1aaa29d0ce5 100644
>> --- a/gdb/python/python.c
>> +++ b/gdb/python/python.c
>> @@ -683,7 +683,7 @@ execute_gdb_command (PyObject *self, PyObject *args, PyObject *kw)
>>  
>>  	/* Use the console interpreter uiout to have the same print format
>>  	   for console or MI.  */
>> -	interp = interp_lookup (current_ui, "console");
>> +	interp = current_ui->lookup_interp ("console");
> 
> Might as well take this opportunity to replace "console" with
> INTERP_CONSOLE which is what we use everywhere else.
Good find, I'll add a preparatory patch that fixes it first.

> 
>>  	current_uiout = interp->interp_ui_out ();
>>  
>>  	if (to_string)
>> diff --git a/gdb/ui.c b/gdb/ui.c
>> index 624187ac73c3..a7b81c077e9a 100644
>> --- a/gdb/ui.c
>> +++ b/gdb/ui.c
>> @@ -169,7 +169,30 @@ ui::add_interp (interp *interp)
>>    this->interp_list.push_back (*interp);
>>  }
>>  
>> -/* See top.h.  */
>> +/* See interps.h.  */
>> +
>> +interp *
>> +ui::lookup_interp (const char *name)
>> +{
>> +  if (name == nullptr || strlen (name) == 0)
>> +    return nullptr;
>> +
>> +  /* Only create each interpreter once per UI.  */
>> +  interp *interp = this->lookup_existing_interp (name);
>> +  if (interp != nullptr)
>> +    return interp;
>> +
>> +  const interp_factory *factory = find_interp_factory (name);
>> +  if (factory == nullptr)
>> +    return nullptr;
>> +
>> +  interp = factory->func (factory->name);
>> +  this->add_interp (interp);
>> +
>> +  return interp;
>> +}
> 
> Rather than exposing the interpreter factor stuff outside of interp.c, I
> think a better approach would be either move ui::lookup_interp into
> interp.c -- personally, I'm happy to see different parts of an object in
> different .c files when that makes sense.

I don't know, I really like having declarations and definitions in
matching .h and .c files.

> But if you prefer all to keep ui implementation in ui.c, rather than
> calling find_interp_factory and then factory->func, we could just have a
> new function `create_interp (name)` which creates an interpreter, or
> returns nullptr, then we can write:
> 
>   interp = create_interp (name);
>   if (interp != nullptr)
>     this->add_interp (interp);
>   return interp;
> 
> And all the factory stuff can remain private to interp.c.

Yes, that makes sense, I'll do that.

Simon

[PATCH 05/21] gdb: remove ui:::add_interp and ui::lookup_existing_interp

[Simon Marchi]

These two methods are only used internally, in ui::lookup_interp.  I
first thought of making them private, but really I think we can just
inline their code in ui::lookup_interp, it's nothing fancy.

Change-Id: Ib65c7d49b698a6a57722c148a228bb18888c4e42
---
 gdb/ui.c | 32 +++++---------------------------
 gdb/ui.h |  8 --------
 2 files changed, 5 insertions(+), 35 deletions(-)

diff --git a/gdb/ui.c b/gdb/ui.c
index a7b81c077e9a..4bb63ed63030 100644
--- a/gdb/ui.c
+++ b/gdb/ui.c
@@ -147,28 +147,6 @@ ui::register_file_handler ()
 		      string_printf ("ui-%d", num), true);
 }
 
-/* See ui.h.  */
-
-interp *
-ui::lookup_existing_interp (const char *name) const
-{
-  for (interp &interp : this->interp_list)
-    if (strcmp (interp.name (), name) == 0)
-      return &interp;
-
-  return nullptr;
-}
-
-/* See ui.h.  */
-
-void
-ui::add_interp (interp *interp)
-{
-  gdb_assert (this->lookup_existing_interp (interp->name ()) == nullptr);
-
-  this->interp_list.push_back (*interp);
-}
-
 /* See interps.h.  */
 
 interp *
@@ -178,16 +156,16 @@ ui::lookup_interp (const char *name)
     return nullptr;
 
   /* Only create each interpreter once per UI.  */
-  interp *interp = this->lookup_existing_interp (name);
-  if (interp != nullptr)
-    return interp;
+  for (auto &candidate : this->interp_list)
+    if (strcmp (candidate.name (), name) == 0)
+      return &candidate;
 
   const interp_factory *factory = find_interp_factory (name);
   if (factory == nullptr)
     return nullptr;
 
-  interp = factory->func (factory->name);
-  this->add_interp (interp);
+  interp *interp = factory->func (factory->name);
+  this->interp_list.push_back (*interp);
 
   return interp;
 }
diff --git a/gdb/ui.h b/gdb/ui.h
index aeb26c68823a..b9ca9c0c80a0 100644
--- a/gdb/ui.h
+++ b/gdb/ui.h
@@ -159,19 +159,11 @@ struct ui : public intrusive_list_node<ui>
   /* Return true if this UI's input fd is a tty.  */
   bool input_interactive_p () const;
 
-  /* Look up the interpreter for NAME.  If no such interpreter exists,
-     return nullptr, otherwise return a pointer to the interpreter.  */
-  interp *lookup_existing_interp (const char *name) const;
-
   /* Look up the interpreter for NAME, creating one if none exists yet.
      If NAME is not a interpreter type previously registered with
      interp_factory_register, return nullptr; otherwise return a pointer to
      the interpreter.  */
   interp *lookup_interp (const char *name);
-
-  /* Add interpreter INTERP to this UI's interpreter list.  The
-     interpreter must not have previously been added.  */
-  void add_interp (interp *interp);
 };
 
 /* The main UI.  This is the UI that is bound to stdin/stdout/stderr.
-- 
2.42.0

[PATCH 06/21] gdb: uncover some current_ui uses in interp_set

[Simon Marchi]

A following patch will make interp_set a method of struct ui.  Before
doing this, I'd like to uncover all the hidden uses of current_ui, as
they will be replaced with a passed-in `ui` parameter, and then `this`.
So, replace uses of the current_uiout macro with the long form.

Change-Id: I8de2e6a451de11ff9040322bd0869021171dcf7f
---
 gdb/interps.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/gdb/interps.c b/gdb/interps.c
index bd95c3175adb..9d27d6a2bd2c 100644
--- a/gdb/interps.c
+++ b/gdb/interps.c
@@ -102,7 +102,7 @@ interp_set (struct interp *interp, bool top_level)
 
   if (old_interp != NULL)
     {
-      current_uiout->flush ();
+      current_ui->m_current_uiout->flush ();
       old_interp->suspend ();
     }
 
@@ -126,7 +126,7 @@ interp_set (struct interp *interp, bool top_level)
     }
 
   /* Do this only after the interpreter is initialized.  */
-  current_uiout = interp->interp_ui_out ();
+  current_ui->m_current_uiout = interp->interp_ui_out ();
 
   /* Clear out any installed interpreter hooks/event handlers.  */
   clear_interpreter_hooks ();
-- 
2.42.0

[PATCH 07/21] gdb: add backlink to ui in interp

[Simon Marchi]

Interps are bound to a single parent UI.  Pass this UI down when
constructing interps, so that they'll be able to use it internally,
instead of using current_ui.

Change-Id: Ife900d501ae26d1a7a1d06b1d5f4013917235d7a
---
 gdb/cli/cli-interp.c | 14 +++++++-------
 gdb/cli/cli-interp.h |  2 +-
 gdb/interps.c        |  5 +++--
 gdb/interps.h        |  7 +++++--
 gdb/mi/mi-interp.c   |  4 ++--
 gdb/mi/mi-interp.h   |  4 ++--
 gdb/python/py-dap.c  |  8 ++++----
 gdb/tui/tui-interp.c |  8 ++++----
 gdb/ui.c             |  2 +-
 9 files changed, 29 insertions(+), 25 deletions(-)

diff --git a/gdb/cli/cli-interp.c b/gdb/cli/cli-interp.c
index 6a175f7baa13..374d379ec7a3 100644
--- a/gdb/cli/cli-interp.c
+++ b/gdb/cli/cli-interp.c
@@ -31,8 +31,8 @@
 #include "thread-fsm.h"
 #include "inferior.h"
 
-cli_interp_base::cli_interp_base (const char *name)
-  : interp (name)
+cli_interp_base::cli_interp_base (ui *ui, const char *name)
+  : interp (ui, name)
 {}
 
 cli_interp_base::~cli_interp_base ()
@@ -43,7 +43,7 @@ cli_interp_base::~cli_interp_base ()
 class cli_interp final : public cli_interp_base
 {
  public:
-  explicit cli_interp (const char *name);
+  explicit cli_interp (ui *ui, const char *name);
   ~cli_interp () = default;
 
   void init (bool top_level) override;
@@ -58,8 +58,8 @@ class cli_interp final : public cli_interp_base
   std::unique_ptr<cli_ui_out> m_cli_uiout;
 };
 
-cli_interp::cli_interp (const char *name)
-  : cli_interp_base (name),
+cli_interp::cli_interp (ui *ui, const char *name)
+  : cli_interp_base (ui, name),
     m_cli_uiout (new cli_ui_out (gdb_stdout))
 {
 }
@@ -317,9 +317,9 @@ cli_interp_base::set_logging (ui_file_up logfile, bool logging_redirect,
 /* Factory for CLI interpreters.  */
 
 static struct interp *
-cli_interp_factory (const char *name)
+cli_interp_factory (ui *ui, const char *name)
 {
-  return new cli_interp (name);
+  return new cli_interp (ui, name);
 }
 
 /* Standard gdb initialization hook.  */
diff --git a/gdb/cli/cli-interp.h b/gdb/cli/cli-interp.h
index a1a20b678942..b92a49b3fc40 100644
--- a/gdb/cli/cli-interp.h
+++ b/gdb/cli/cli-interp.h
@@ -25,7 +25,7 @@
 class cli_interp_base : public interp
 {
 public:
-  explicit cli_interp_base (const char *name);
+  explicit cli_interp_base (ui *ui, const char *name);
   virtual ~cli_interp_base () = 0;
 
   void set_logging (ui_file_up logfile, bool logging_redirect,
diff --git a/gdb/interps.c b/gdb/interps.c
index 9d27d6a2bd2c..0575128b8124 100644
--- a/gdb/interps.c
+++ b/gdb/interps.c
@@ -41,8 +41,9 @@
 #include "gdbsupport/buildargv.h"
 #include "gdbsupport/scope-exit.h"
 
-interp::interp (const char *name)
-  : m_name (name)
+interp::interp (ui *ui, const char *name)
+  : m_ui (ui),
+    m_name (name)
 {
 }
 
diff --git a/gdb/interps.h b/gdb/interps.h
index 433d92439eba..278ee5aff9a5 100644
--- a/gdb/interps.h
+++ b/gdb/interps.h
@@ -34,7 +34,7 @@ struct inferior;
 struct so_list;
 struct trace_state_variable;
 
-typedef struct interp *(*interp_factory_func) (const char *name);
+using interp_factory_func = interp *(*) (ui *ui, const char *name);
 
 /* An interpreter factory.  Maps an interpreter name to the factory
    function that instantiates an interpreter by that name.  */
@@ -69,7 +69,7 @@ extern void interp_exec (struct interp *interp, const char *command);
 class interp : public intrusive_list_node<interp>
 {
 public:
-  explicit interp (const char *name);
+  explicit interp (ui *ui, const char *name);
   virtual ~interp () = 0;
 
   virtual void init (bool top_level)
@@ -202,6 +202,9 @@ class interp : public intrusive_list_node<interp>
   /* Notify the interpreter that inferior INF's memory was changed.  */
   virtual void on_memory_changed (inferior *inf, CORE_ADDR addr, ssize_t len,
 				  const bfd_byte *data) {}
+protected:
+  /* Backlink to the UI owning this interpreter.  */
+  ui *m_ui;
 
 private:
   /* The memory for this is static, it comes from literal strings (e.g. "cli").  */
diff --git a/gdb/mi/mi-interp.c b/gdb/mi/mi-interp.c
index 946fed5867c4..be9dacaf9304 100644
--- a/gdb/mi/mi-interp.c
+++ b/gdb/mi/mi-interp.c
@@ -925,9 +925,9 @@ mi_interp::set_logging (ui_file_up logfile, bool logging_redirect,
 /* Factory for MI interpreters.  */
 
 static struct interp *
-mi_interp_factory (const char *name)
+mi_interp_factory (ui *ui, const char *name)
 {
-  return new mi_interp (name);
+  return new mi_interp (ui, name);
 }
 
 void _initialize_mi_interp ();
diff --git a/gdb/mi/mi-interp.h b/gdb/mi/mi-interp.h
index f9af61f0a571..54ba70bdab88 100644
--- a/gdb/mi/mi-interp.h
+++ b/gdb/mi/mi-interp.h
@@ -29,8 +29,8 @@ struct mi_console_file;
 class mi_interp final : public interp
 {
 public:
-  mi_interp (const char *name)
-    : interp (name)
+  mi_interp (ui *ui, const char *name)
+    : interp (ui, name)
   {}
 
   void init (bool top_level) override;
diff --git a/gdb/python/py-dap.c b/gdb/python/py-dap.c
index 3444eccb6fb6..724a4925938c 100644
--- a/gdb/python/py-dap.c
+++ b/gdb/python/py-dap.c
@@ -27,8 +27,8 @@ class dap_interp final : public interp
 {
 public:
 
-  explicit dap_interp (const char *name)
-    : interp (name),
+  explicit dap_interp (ui *ui, const char *name)
+    : interp (ui, name),
       m_ui_out (new cli_ui_out (gdb_stdout))
   {
   }
@@ -93,9 +93,9 @@ _initialize_py_interp ()
 {
   /* The dap code uses module typing, available starting python 3.5.  */
 #if PY_VERSION_HEX >= 0x03050000
-  interp_factory_register ("dap", [] (const char *name) -> interp *
+  interp_factory_register ("dap", [] (ui *ui, const char *name) -> interp *
     {
-      return new dap_interp (name);
+      return new dap_interp (ui, name);
     });
 #endif
 }
diff --git a/gdb/tui/tui-interp.c b/gdb/tui/tui-interp.c
index 299cc4caea09..227b37f374dc 100644
--- a/gdb/tui/tui-interp.c
+++ b/gdb/tui/tui-interp.c
@@ -42,8 +42,8 @@ static bool tui_start_enabled = false;
 class tui_interp final : public cli_interp_base
 {
 public:
-  explicit tui_interp (const char *name)
-    : cli_interp_base (name)
+  explicit tui_interp (ui *ui, const char *name)
+    : cli_interp_base (ui, name)
   {}
 
   void init (bool top_level) override;
@@ -159,9 +159,9 @@ tui_interp::exec (const char *command_str)
 /* Factory for TUI interpreters.  */
 
 static struct interp *
-tui_interp_factory (const char *name)
+tui_interp_factory (ui *ui, const char *name)
 {
-  return new tui_interp (name);
+  return new tui_interp (ui, name);
 }
 
 void _initialize_tui_interp ();
diff --git a/gdb/ui.c b/gdb/ui.c
index 4bb63ed63030..8c04dc92b89e 100644
--- a/gdb/ui.c
+++ b/gdb/ui.c
@@ -164,7 +164,7 @@ ui::lookup_interp (const char *name)
   if (factory == nullptr)
     return nullptr;
 
-  interp *interp = factory->func (factory->name);
+  interp *interp = factory->func (this, factory->name);
   this->interp_list.push_back (*interp);
 
   return interp;
-- 
2.42.0

[PATCH 08/21] gdb: pass ui down to gdb_setup_readline and gdb_disable_readline

[Simon Marchi]

This is useful for the upcoming patches that make interps use m_ui
instead of current_ui.

Change-Id: I27a3e586a7e1063050b84714fe408e57953ec108
---
 gdb/cli/cli-interp.c | 4 ++--
 gdb/event-top.c      | 8 ++------
 gdb/event-top.h      | 4 ++--
 gdb/mi/mi-interp.c   | 4 ++--
 gdb/top.c            | 2 +-
 gdb/tui/tui-interp.c | 4 ++--
 6 files changed, 11 insertions(+), 15 deletions(-)

diff --git a/gdb/cli/cli-interp.c b/gdb/cli/cli-interp.c
index 374d379ec7a3..f0fa26919e76 100644
--- a/gdb/cli/cli-interp.c
+++ b/gdb/cli/cli-interp.c
@@ -206,7 +206,7 @@ cli_interp::resume ()
       stream = NULL;
     }
 
-  gdb_setup_readline (1);
+  gdb_setup_readline (ui, 1);
 
   ui->input_handler = command_line_handler;
 
@@ -217,7 +217,7 @@ cli_interp::resume ()
 void
 cli_interp::suspend ()
 {
-  gdb_disable_readline ();
+  gdb_disable_readline (current_ui);
 }
 
 void
diff --git a/gdb/event-top.c b/gdb/event-top.c
index d1be23bcbe9b..54f199d2e393 100644
--- a/gdb/event-top.c
+++ b/gdb/event-top.c
@@ -1275,10 +1275,8 @@ async_sigtstp_handler (gdb_client_data arg)
    loop.  */
 
 void
-gdb_setup_readline (int editing)
+gdb_setup_readline (ui *ui, int editing)
 {
-  struct ui *ui = current_ui;
-
   /* 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.  Also, INSTREAM might be nullptr when
@@ -1316,10 +1314,8 @@ gdb_setup_readline (int editing)
    interface, like the cli & the mi.  */
 
 void
-gdb_disable_readline (void)
+gdb_disable_readline (ui *ui)
 {
-  struct ui *ui = current_ui;
-
   if (ui->command_editing)
     gdb_rl_callback_handler_remove ();
   ui->unregister_file_handler ();
diff --git a/gdb/event-top.h b/gdb/event-top.h
index f7247f5c4f23..ab78f95a9f62 100644
--- a/gdb/event-top.h
+++ b/gdb/event-top.h
@@ -30,8 +30,8 @@ struct cmd_list_element;
    FIXME: these should really go into top.h.  */
 
 extern void display_gdb_prompt (const char *new_prompt);
-extern void gdb_setup_readline (int);
-extern void gdb_disable_readline (void);
+extern void gdb_setup_readline (ui *ui, int editing);
+extern void gdb_disable_readline (ui *ui);
 extern void gdb_init_signals (void);
 extern void change_line_handler (int);
 
diff --git a/gdb/mi/mi-interp.c b/gdb/mi/mi-interp.c
index be9dacaf9304..3095301b2f11 100644
--- a/gdb/mi/mi-interp.c
+++ b/gdb/mi/mi-interp.c
@@ -122,7 +122,7 @@ mi_interp::resume ()
 
   /* As per hack note in mi_interpreter_init, swap in the output
      channels... */
-  gdb_setup_readline (0);
+  gdb_setup_readline (ui, 0);
 
   ui->call_readline = gdb_readline_no_editing_callback;
   ui->input_handler = mi_execute_command_input_handler;
@@ -142,7 +142,7 @@ mi_interp::resume ()
 void
 mi_interp::suspend ()
 {
-  gdb_disable_readline ();
+  gdb_disable_readline (current_ui);
 }
 
 void
diff --git a/gdb/top.c b/gdb/top.c
index 483e201140e7..a1ef328242c8 100644
--- a/gdb/top.c
+++ b/gdb/top.c
@@ -1714,7 +1714,7 @@ undo_terminal_modifications_before_exit (void)
 #if defined(TUI)
   tui_disable ();
 #endif
-  gdb_disable_readline ();
+  gdb_disable_readline (current_ui);
 
   current_ui = saved_top_level;
 }
diff --git a/gdb/tui/tui-interp.c b/gdb/tui/tui-interp.c
index 227b37f374dc..af99501ac69f 100644
--- a/gdb/tui/tui-interp.c
+++ b/gdb/tui/tui-interp.c
@@ -121,7 +121,7 @@ tui_interp::resume ()
       stream = NULL;
     }
 
-  gdb_setup_readline (1);
+  gdb_setup_readline (ui, 1);
 
   ui->input_handler = tui_command_line_handler;
 
@@ -135,7 +135,7 @@ tui_interp::resume ()
 void
 tui_interp::suspend ()
 {
-  gdb_disable_readline ();
+  gdb_disable_readline (current_ui);
   tui_start_enabled = tui_active;
   tui_disable ();
 }
-- 
2.42.0

[PATCH 09/21] gdb/python: use m_ui instead of current_ui in dap_interp::init

[Simon Marchi]

Change-Id: I95dd19952f22bb92eafad46cb1c6f22b63e09783
---
 gdb/python/py-dap.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/gdb/python/py-dap.c b/gdb/python/py-dap.c
index 724a4925938c..652ce9cb0eda 100644
--- a/gdb/python/py-dap.c
+++ b/gdb/python/py-dap.c
@@ -83,8 +83,8 @@ dap_interp::init (bool top_level)
   if (result_obj == nullptr)
     gdbpy_handle_exception ();
 
-  current_ui->input_fd = -1;
-  current_ui->m_input_interactive_p = false;
+  m_ui->input_fd = -1;
+  m_ui->m_input_interactive_p = false;
 }
 
 void _initialize_py_interp ();
-- 
2.42.0

[PATCH 10/21] gdb/mi: use m_ui instead of current_ui in mi_interp::init

[Simon Marchi]

gdb_stdout accesses current_ui under the hood.  Use m_ui to get the
stdout file instead.  Add an stdout method to ui to help with that (it
would feel wrong to access ui::m_gdb_stdout directly).

Change-Id: I03739df387c8de01bc692d7ddddbb66544555630
---
 gdb/mi/mi-interp.c | 2 +-
 gdb/ui.h           | 3 +++
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/gdb/mi/mi-interp.c b/gdb/mi/mi-interp.c
index 3095301b2f11..f260d98837cf 100644
--- a/gdb/mi/mi-interp.c
+++ b/gdb/mi/mi-interp.c
@@ -86,7 +86,7 @@ mi_interp::init (bool top_level)
   /* Store the current output channel, so that we can create a console
      channel that encapsulates and prefixes all gdb_output-type bits
      coming from the rest of the debugger.  */
-  mi->raw_stdout = gdb_stdout;
+  mi->raw_stdout = m_ui->stdout ();
 
   /* Create MI console channels, each with a different prefix so they
      can be distinguished.  */
diff --git a/gdb/ui.h b/gdb/ui.h
index b9ca9c0c80a0..be89ab3d6848 100644
--- a/gdb/ui.h
+++ b/gdb/ui.h
@@ -150,6 +150,9 @@ struct ui : public intrusive_list_node<ui>
   /* The current ui_out.  */
   struct ui_out *m_current_uiout = nullptr;
 
+  ui_file *stdout ()
+  { return m_gdb_stdout; }
+
   /* Register the UI's input file descriptor in the event loop.  */
   void register_file_handler ();
 
-- 
2.42.0

[PATCH 11/21] gdb/cli: use m_ui instead of current_ui in cli_interp::resume

[Simon Marchi]

No behavior changes expected.

Change-Id: I7fd944c99d249b3080d74d949186fe92795568eb
---
 gdb/cli/cli-interp.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/gdb/cli/cli-interp.c b/gdb/cli/cli-interp.c
index f0fa26919e76..8f1dbef56a6e 100644
--- a/gdb/cli/cli-interp.c
+++ b/gdb/cli/cli-interp.c
@@ -190,25 +190,22 @@ cli_interp::init (bool top_level)
 void
 cli_interp::resume ()
 {
-  struct ui *ui = current_ui;
-  struct ui_file *stream;
-
   /*sync_execution = 1; */
 
   /* gdb_setup_readline will change gdb_stdout.  If the CLI was
      previously writing to gdb_stdout, then set it to the new
      gdb_stdout afterwards.  */
 
-  stream = m_cli_uiout->set_stream (gdb_stdout);
+  ui_file *stream = m_cli_uiout->set_stream (gdb_stdout);
   if (stream != gdb_stdout)
     {
       m_cli_uiout->set_stream (stream);
       stream = NULL;
     }
 
-  gdb_setup_readline (ui, 1);
+  gdb_setup_readline (m_ui, 1);
 
-  ui->input_handler = command_line_handler;
+  m_ui->input_handler = command_line_handler;
 
   if (stream != NULL)
     m_cli_uiout->set_stream (gdb_stdout);
-- 
2.42.0

Re: [PATCH 11/21] gdb/cli: use m_ui instead of current_ui in cli_interp::resume

[Andrew Burgess]

Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:

> No behavior changes expected.
>
> Change-Id: I7fd944c99d249b3080d74d949186fe92795568eb
> ---
>  gdb/cli/cli-interp.c | 9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)
>
> diff --git a/gdb/cli/cli-interp.c b/gdb/cli/cli-interp.c
> index f0fa26919e76..8f1dbef56a6e 100644
> --- a/gdb/cli/cli-interp.c
> +++ b/gdb/cli/cli-interp.c
> @@ -190,25 +190,22 @@ cli_interp::init (bool top_level)
>  void
>  cli_interp::resume ()
>  {
> -  struct ui *ui = current_ui;
> -  struct ui_file *stream;
> -
>    /*sync_execution = 1; */
>  
>    /* gdb_setup_readline will change gdb_stdout.  If the CLI was
>       previously writing to gdb_stdout, then set it to the new
>       gdb_stdout afterwards.  */
>  
> -  stream = m_cli_uiout->set_stream (gdb_stdout);
> +  ui_file *stream = m_cli_uiout->set_stream (gdb_stdout);

In the previous patch you changed an explicit reference to gdb_stdout to
m_ui->stdout ().  It's not clear why you haven't done that here, and in
other places in this function.

Thanks,
Andrew

>    if (stream != gdb_stdout)
>      {
>        m_cli_uiout->set_stream (stream);
>        stream = NULL;
>      }
>  
> -  gdb_setup_readline (ui, 1);
> +  gdb_setup_readline (m_ui, 1);
>  
> -  ui->input_handler = command_line_handler;
> +  m_ui->input_handler = command_line_handler;
>  
>    if (stream != NULL)
>      m_cli_uiout->set_stream (gdb_stdout);
> -- 
> 2.42.0

Re: [PATCH 11/21] gdb/cli: use m_ui instead of current_ui in cli_interp::resume

[Simon Marchi]

On 9/12/23 06:40, Andrew Burgess via Gdb-patches wrote:
> Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
>> No behavior changes expected.
>>
>> Change-Id: I7fd944c99d249b3080d74d949186fe92795568eb
>> ---
>>  gdb/cli/cli-interp.c | 9 +++------
>>  1 file changed, 3 insertions(+), 6 deletions(-)
>>
>> diff --git a/gdb/cli/cli-interp.c b/gdb/cli/cli-interp.c
>> index f0fa26919e76..8f1dbef56a6e 100644
>> --- a/gdb/cli/cli-interp.c
>> +++ b/gdb/cli/cli-interp.c
>> @@ -190,25 +190,22 @@ cli_interp::init (bool top_level)
>>  void
>>  cli_interp::resume ()
>>  {
>> -  struct ui *ui = current_ui;
>> -  struct ui_file *stream;
>> -
>>    /*sync_execution = 1; */
>>  
>>    /* gdb_setup_readline will change gdb_stdout.  If the CLI was
>>       previously writing to gdb_stdout, then set it to the new
>>       gdb_stdout afterwards.  */
>>  
>> -  stream = m_cli_uiout->set_stream (gdb_stdout);
>> +  ui_file *stream = m_cli_uiout->set_stream (gdb_stdout);
> 
> In the previous patch you changed an explicit reference to gdb_stdout to
> m_ui->stdout ().  It's not clear why you haven't done that here, and in
> other places in this function.

gdb_stdout uses current_ui under the hood, so I think I should change it
here as well (as well as in the other patches, as you have noted).

For v2, I will add a preparatory patch that adds all the necessary
getters and setters in struct ui for stdout, stdin, etc, and I will go
over these patches to uncover uses of current_ui hidden behind these
macros.

Simon

[PATCH 12/21] gdb/tui: use m_ui instead of current_ui in tui_interp::resume

[Simon Marchi]

No behavior changes expected.

Change-Id: I607a50a7ed83d9afd536d41d6318fc4cd24d8192
---
 gdb/tui/tui-interp.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/gdb/tui/tui-interp.c b/gdb/tui/tui-interp.c
index af99501ac69f..87c079949f4b 100644
--- a/gdb/tui/tui-interp.c
+++ b/gdb/tui/tui-interp.c
@@ -107,23 +107,20 @@ tui_command_line_handler (gdb::unique_xmalloc_ptr<char> &&rl)
 void
 tui_interp::resume ()
 {
-  struct ui *ui = current_ui;
-  struct ui_file *stream;
-
   /* gdb_setup_readline will change gdb_stdout.  If the TUI was
      previously writing to gdb_stdout, then set it to the new
      gdb_stdout afterwards.  */
 
-  stream = tui_old_uiout->set_stream (gdb_stdout);
+  ui_file *stream = tui_old_uiout->set_stream (gdb_stdout);
   if (stream != gdb_stdout)
     {
       tui_old_uiout->set_stream (stream);
       stream = NULL;
     }
 
-  gdb_setup_readline (ui, 1);
+  gdb_setup_readline (m_ui, 1);
 
-  ui->input_handler = tui_command_line_handler;
+  m_ui->input_handler = tui_command_line_handler;
 
   if (stream != NULL)
     tui_old_uiout->set_stream (gdb_stdout);
-- 
2.42.0

Re: [PATCH 12/21] gdb/tui: use m_ui instead of current_ui in tui_interp::resume

[Andrew Burgess]

Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:

> No behavior changes expected.
>
> Change-Id: I607a50a7ed83d9afd536d41d6318fc4cd24d8192
> ---
>  gdb/tui/tui-interp.c | 9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)
>
> diff --git a/gdb/tui/tui-interp.c b/gdb/tui/tui-interp.c
> index af99501ac69f..87c079949f4b 100644
> --- a/gdb/tui/tui-interp.c
> +++ b/gdb/tui/tui-interp.c
> @@ -107,23 +107,20 @@ tui_command_line_handler (gdb::unique_xmalloc_ptr<char> &&rl)
>  void
>  tui_interp::resume ()
>  {
> -  struct ui *ui = current_ui;
> -  struct ui_file *stream;
> -
>    /* gdb_setup_readline will change gdb_stdout.  If the TUI was
>       previously writing to gdb_stdout, then set it to the new
>       gdb_stdout afterwards.  */
>  
> -  stream = tui_old_uiout->set_stream (gdb_stdout);
> +  ui_file *stream = tui_old_uiout->set_stream (gdb_stdout);

Same stdout question as for the previous patch.

Thanks,
Andrew

>    if (stream != gdb_stdout)
>      {
>        tui_old_uiout->set_stream (stream);
>        stream = NULL;
>      }
>  
> -  gdb_setup_readline (ui, 1);
> +  gdb_setup_readline (m_ui, 1);
>  
> -  ui->input_handler = tui_command_line_handler;
> +  m_ui->input_handler = tui_command_line_handler;
>  
>    if (stream != NULL)
>      tui_old_uiout->set_stream (gdb_stdout);
> -- 
> 2.42.0

[PATCH 13/21] gdb/mi: use m_ui instead of current_ui in mi_interp::resume

[Simon Marchi]

No behavior changes expected.

Change-Id: Ic379a18d6a6d83a55cd4a6d1239d4daa98966cc8
---
 gdb/mi/mi-interp.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/gdb/mi/mi-interp.c b/gdb/mi/mi-interp.c
index f260d98837cf..b676e22f3550 100644
--- a/gdb/mi/mi-interp.c
+++ b/gdb/mi/mi-interp.c
@@ -118,14 +118,13 @@ void
 mi_interp::resume ()
 {
   struct mi_interp *mi = this;
-  struct ui *ui = current_ui;
 
   /* As per hack note in mi_interpreter_init, swap in the output
      channels... */
-  gdb_setup_readline (ui, 0);
+  gdb_setup_readline (m_ui, 0);
 
-  ui->call_readline = gdb_readline_no_editing_callback;
-  ui->input_handler = mi_execute_command_input_handler;
+  m_ui->call_readline = gdb_readline_no_editing_callback;
+  m_ui->input_handler = mi_execute_command_input_handler;
 
   gdb_stdout = mi->out;
   /* Route error and log output through the MI.  */
-- 
2.42.0

Re: [PATCH 13/21] gdb/mi: use m_ui instead of current_ui in mi_interp::resume

[Andrew Burgess]

Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:

> No behavior changes expected.
>
> Change-Id: Ic379a18d6a6d83a55cd4a6d1239d4daa98966cc8
> ---
>  gdb/mi/mi-interp.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/gdb/mi/mi-interp.c b/gdb/mi/mi-interp.c
> index f260d98837cf..b676e22f3550 100644
> --- a/gdb/mi/mi-interp.c
> +++ b/gdb/mi/mi-interp.c
> @@ -118,14 +118,13 @@ void
>  mi_interp::resume ()
>  {
>    struct mi_interp *mi = this;
> -  struct ui *ui = current_ui;
>  
>    /* As per hack note in mi_interpreter_init, swap in the output
>       channels... */
> -  gdb_setup_readline (ui, 0);
> +  gdb_setup_readline (m_ui, 0);
>  
> -  ui->call_readline = gdb_readline_no_editing_callback;
> -  ui->input_handler = mi_execute_command_input_handler;
> +  m_ui->call_readline = gdb_readline_no_editing_callback;
> +  m_ui->input_handler = mi_execute_command_input_handler;
>  
>    gdb_stdout = mi->out;

Same stdout question as for the previous two patches.

I'm wondering if we're going to have to live in some halfway house for a
while, in which case maybe we should be adding an assert like:

  gdb_assert (current_ui == m_ui);

Or maybe I've jut not understood something here (or maybe a later patch
fixes all this suff, and I'm just making noise).

Thanks,
Andrew

>    /* Route error and log output through the MI.  */
> -- 
> 2.42.0

Re: [PATCH 13/21] gdb/mi: use m_ui instead of current_ui in mi_interp::resume

[Simon Marchi]

On 9/12/23 06:44, Andrew Burgess via Gdb-patches wrote:
> Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
>> No behavior changes expected.
>>
>> Change-Id: Ic379a18d6a6d83a55cd4a6d1239d4daa98966cc8
>> ---
>>  gdb/mi/mi-interp.c | 7 +++----
>>  1 file changed, 3 insertions(+), 4 deletions(-)
>>
>> diff --git a/gdb/mi/mi-interp.c b/gdb/mi/mi-interp.c
>> index f260d98837cf..b676e22f3550 100644
>> --- a/gdb/mi/mi-interp.c
>> +++ b/gdb/mi/mi-interp.c
>> @@ -118,14 +118,13 @@ void
>>  mi_interp::resume ()
>>  {
>>    struct mi_interp *mi = this;
>> -  struct ui *ui = current_ui;
>>  
>>    /* As per hack note in mi_interpreter_init, swap in the output
>>       channels... */
>> -  gdb_setup_readline (ui, 0);
>> +  gdb_setup_readline (m_ui, 0);
>>  
>> -  ui->call_readline = gdb_readline_no_editing_callback;
>> -  ui->input_handler = mi_execute_command_input_handler;
>> +  m_ui->call_readline = gdb_readline_no_editing_callback;
>> +  m_ui->input_handler = mi_execute_command_input_handler;
>>  
>>    gdb_stdout = mi->out;
> 
> Same stdout question as for the previous two patches.
> 
> I'm wondering if we're going to have to live in some halfway house for a
> while, in which case maybe we should be adding an assert like:
> 
>   gdb_assert (current_ui == m_ui);
> 
> Or maybe I've jut not understood something here (or maybe a later patch
> fixes all this suff, and I'm just making noise).

If I change those uses of gdb_stdout and friends, I think these methods
(init, resume, suspend) are pretty-much independent of global state.

Simon

[PATCH 14/21] gdb/cli: use m_ui instead of current_ui in cli_interp::suspend

[Simon Marchi]

No behavior changes expected.

Change-Id: Ice542e98b7add70f40c32e8d477bec4605d3ca3f
---
 gdb/cli/cli-interp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gdb/cli/cli-interp.c b/gdb/cli/cli-interp.c
index 8f1dbef56a6e..2980c87479cc 100644
--- a/gdb/cli/cli-interp.c
+++ b/gdb/cli/cli-interp.c
@@ -214,7 +214,7 @@ cli_interp::resume ()
 void
 cli_interp::suspend ()
 {
-  gdb_disable_readline (current_ui);
+  gdb_disable_readline (m_ui);
 }
 
 void
-- 
2.42.0

[PATCH 15/21] gdb/tui: use m_ui instead of current_ui in tui_interp::suspend

[Simon Marchi]

No behavior changes expected.

Change-Id: I61d7f849909a333403c6fb978bfe8efcc39d58fa
---
 gdb/tui/tui-interp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gdb/tui/tui-interp.c b/gdb/tui/tui-interp.c
index 87c079949f4b..2520b7158d45 100644
--- a/gdb/tui/tui-interp.c
+++ b/gdb/tui/tui-interp.c
@@ -132,7 +132,7 @@ tui_interp::resume ()
 void
 tui_interp::suspend ()
 {
-  gdb_disable_readline (current_ui);
+  gdb_disable_readline (m_ui);
   tui_start_enabled = tui_active;
   tui_disable ();
 }
-- 
2.42.0

[PATCH 16/21] gdb/mi: use m_ui instead of current_ui in mi_interp::suspend

[Simon Marchi]

No behavior changes expected.

Change-Id: I17d11fb5ed38a51bc0ea9708ec5e290bb54d41c1
---
 gdb/mi/mi-interp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gdb/mi/mi-interp.c b/gdb/mi/mi-interp.c
index b676e22f3550..60a2ed0bcc4c 100644
--- a/gdb/mi/mi-interp.c
+++ b/gdb/mi/mi-interp.c
@@ -141,7 +141,7 @@ mi_interp::resume ()
 void
 mi_interp::suspend ()
 {
-  gdb_disable_readline (current_ui);
+  gdb_disable_readline (m_ui);
 }
 
 void
-- 
2.42.0

[PATCH 17/21] gdb: pass current_ui down to interp_set

[Simon Marchi]

In preparation for making interp_set a method of struct ui.  No behavior
changes expected.

Change-Id: I507c7701438967502d714ecda99401d785806cab
---
 gdb/interps.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/gdb/interps.c b/gdb/interps.c
index 0575128b8124..18758f1f7af6 100644
--- a/gdb/interps.c
+++ b/gdb/interps.c
@@ -92,24 +92,24 @@ find_interp_factory (const char *name)
    are caused by CLI commands.  */
 
 static void
-interp_set (struct interp *interp, bool top_level)
+interp_set (ui *ui, struct interp *interp, bool top_level)
 {
-  struct interp *old_interp = current_ui->current_interpreter;
+  struct interp *old_interp = ui->current_interpreter;
 
   /* If we already have an interpreter, then trying to
      set top level interpreter is kinda pointless.  */
-  gdb_assert (!top_level || !current_ui->current_interpreter);
-  gdb_assert (!top_level || !current_ui->top_level_interpreter);
+  gdb_assert (!top_level || !ui->current_interpreter);
+  gdb_assert (!top_level || !ui->top_level_interpreter);
 
   if (old_interp != NULL)
     {
-      current_ui->m_current_uiout->flush ();
+      ui->m_current_uiout->flush ();
       old_interp->suspend ();
     }
 
-  current_ui->current_interpreter = interp;
+  ui->current_interpreter = interp;
   if (top_level)
-    current_ui->top_level_interpreter = interp;
+    ui->top_level_interpreter = interp;
 
   if (interpreter_p != interp->name ())
     interpreter_p = interp->name ();
@@ -127,7 +127,7 @@ interp_set (struct interp *interp, bool top_level)
     }
 
   /* Do this only after the interpreter is initialized.  */
-  current_ui->m_current_uiout = interp->interp_ui_out ();
+  ui->m_current_uiout = interp->interp_ui_out ();
 
   /* Clear out any installed interpreter hooks/event handlers.  */
   clear_interpreter_hooks ();
@@ -151,7 +151,7 @@ set_top_level_interpreter (const char *name)
   if (interp == NULL)
     error (_("Interpreter `%s' unrecognized"), name);
   /* Install it.  */
-  interp_set (interp, true);
+  interp_set (current_ui, interp, true);
 }
 
 void
@@ -267,10 +267,10 @@ interpreter_exec_cmd (const char *args, int from_tty)
   if (interp_to_use == NULL)
     error (_("Could not find interpreter \"%s\"."), prules[0]);
 
-  interp_set (interp_to_use, false);
+  interp_set (current_ui, interp_to_use, false);
   SCOPE_EXIT
     {
-      interp_set (old_interp, false);
+      interp_set (current_ui, old_interp, false);
     };
 
   for (i = 1; i < nrules; i++)
-- 
2.42.0

Re: [PATCH 17/21] gdb: pass current_ui down to interp_set

[Andrew Burgess]

Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:

> In preparation for making interp_set a method of struct ui.  No behavior
> changes expected.
>
> Change-Id: I507c7701438967502d714ecda99401d785806cab
> ---
>  gdb/interps.c | 22 +++++++++++-----------
>  1 file changed, 11 insertions(+), 11 deletions(-)
>
> diff --git a/gdb/interps.c b/gdb/interps.c
> index 0575128b8124..18758f1f7af6 100644
> --- a/gdb/interps.c
> +++ b/gdb/interps.c
> @@ -92,24 +92,24 @@ find_interp_factory (const char *name)
>     are caused by CLI commands.  */
>  
>  static void
> -interp_set (struct interp *interp, bool top_level)
> +interp_set (ui *ui, struct interp *interp, bool top_level)
>  {

Given where you're going, I'm fine with this.  If we planned to stop
here then I'd say, lets use interp->m_ui instead of passing in a
separate ui parameter, but I see why this helps.

Thanks,
Andrew

> -  struct interp *old_interp = current_ui->current_interpreter;
> +  struct interp *old_interp = ui->current_interpreter;
>  
>    /* If we already have an interpreter, then trying to
>       set top level interpreter is kinda pointless.  */
> -  gdb_assert (!top_level || !current_ui->current_interpreter);
> -  gdb_assert (!top_level || !current_ui->top_level_interpreter);
> +  gdb_assert (!top_level || !ui->current_interpreter);
> +  gdb_assert (!top_level || !ui->top_level_interpreter);
>  
>    if (old_interp != NULL)
>      {
> -      current_ui->m_current_uiout->flush ();
> +      ui->m_current_uiout->flush ();
>        old_interp->suspend ();
>      }
>  
> -  current_ui->current_interpreter = interp;
> +  ui->current_interpreter = interp;
>    if (top_level)
> -    current_ui->top_level_interpreter = interp;
> +    ui->top_level_interpreter = interp;
>  
>    if (interpreter_p != interp->name ())
>      interpreter_p = interp->name ();
> @@ -127,7 +127,7 @@ interp_set (struct interp *interp, bool top_level)
>      }
>  
>    /* Do this only after the interpreter is initialized.  */
> -  current_ui->m_current_uiout = interp->interp_ui_out ();
> +  ui->m_current_uiout = interp->interp_ui_out ();
>  
>    /* Clear out any installed interpreter hooks/event handlers.  */
>    clear_interpreter_hooks ();
> @@ -151,7 +151,7 @@ set_top_level_interpreter (const char *name)
>    if (interp == NULL)
>      error (_("Interpreter `%s' unrecognized"), name);
>    /* Install it.  */
> -  interp_set (interp, true);
> +  interp_set (current_ui, interp, true);
>  }
>  
>  void
> @@ -267,10 +267,10 @@ interpreter_exec_cmd (const char *args, int from_tty)
>    if (interp_to_use == NULL)
>      error (_("Could not find interpreter \"%s\"."), prules[0]);
>  
> -  interp_set (interp_to_use, false);
> +  interp_set (current_ui, interp_to_use, false);
>    SCOPE_EXIT
>      {
> -      interp_set (old_interp, false);
> +      interp_set (current_ui, old_interp, false);
>      };
>  
>    for (i = 1; i < nrules; i++)
> -- 
> 2.42.0

Re: [PATCH 17/21] gdb: pass current_ui down to interp_set

[Simon Marchi]

On 9/12/23 06:54, Andrew Burgess wrote:
> Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
>> In preparation for making interp_set a method of struct ui.  No behavior
>> changes expected.
>>
>> Change-Id: I507c7701438967502d714ecda99401d785806cab
>> ---
>>  gdb/interps.c | 22 +++++++++++-----------
>>  1 file changed, 11 insertions(+), 11 deletions(-)
>>
>> diff --git a/gdb/interps.c b/gdb/interps.c
>> index 0575128b8124..18758f1f7af6 100644
>> --- a/gdb/interps.c
>> +++ b/gdb/interps.c
>> @@ -92,24 +92,24 @@ find_interp_factory (const char *name)
>>     are caused by CLI commands.  */
>>  
>>  static void
>> -interp_set (struct interp *interp, bool top_level)
>> +interp_set (ui *ui, struct interp *interp, bool top_level)
>>  {
> 
> Given where you're going, I'm fine with this.  If we planned to stop
> here then I'd say, lets use interp->m_ui instead of passing in a
> separate ui parameter, but I see why this helps.

Ack.  This patch basically adds the equivalent of "this", in preparation
to making this a method, so it indeed disappears in the next patch.

However, this makes me realize it's possible to do:

  ui->set_current_interpreter (interp)

with an INTERP that doesn't belong to UI, and that would be a mistake
(that's nothing new, it could happen with interp_set today).  Perhaps I
can add an assert in set_current_interpreter to ensure we don't try to
add set an interp on a UI that belongs to another UI.

Simon

[PATCH 18/21] gdb: make interp_set a method of struct ui

[Simon Marchi]

Move interp_set to ui::set_current_interpreter.

Change-Id: I7e83ca7732bb6229a1e61dc0e3832f54c20c3f52
---
 gdb/interps.c | 66 +++------------------------------------------------
 gdb/ui.c      | 51 +++++++++++++++++++++++++++++++++++++++
 gdb/ui.h      | 12 ++++++++++
 3 files changed, 66 insertions(+), 63 deletions(-)

diff --git a/gdb/interps.c b/gdb/interps.c
index 18758f1f7af6..465c174ea794 100644
--- a/gdb/interps.c
+++ b/gdb/interps.c
@@ -80,66 +80,6 @@ find_interp_factory (const char *name)
   return nullptr;
 }
 
-/* This sets the current interpreter to be INTERP.  If INTERP has not
-   been initialized, then this will also run the init method.
-
-   The TOP_LEVEL parameter tells if this new interpreter is
-   the top-level one.  The top-level is what is requested
-   on the command line, and is responsible for reporting general
-   notification about target state changes.  For example, if
-   MI is the top-level interpreter, then it will always report
-   events such as target stops and new thread creation, even if they
-   are caused by CLI commands.  */
-
-static void
-interp_set (ui *ui, struct interp *interp, bool top_level)
-{
-  struct interp *old_interp = ui->current_interpreter;
-
-  /* If we already have an interpreter, then trying to
-     set top level interpreter is kinda pointless.  */
-  gdb_assert (!top_level || !ui->current_interpreter);
-  gdb_assert (!top_level || !ui->top_level_interpreter);
-
-  if (old_interp != NULL)
-    {
-      ui->m_current_uiout->flush ();
-      old_interp->suspend ();
-    }
-
-  ui->current_interpreter = interp;
-  if (top_level)
-    ui->top_level_interpreter = interp;
-
-  if (interpreter_p != interp->name ())
-    interpreter_p = interp->name ();
-
-  bool warn_about_mi1 = false;
-
-  /* Run the init proc.  */
-  if (!interp->inited)
-    {
-      interp->init (top_level);
-      interp->inited = true;
-
-      if (streq (interp->name (), "mi1"))
-	warn_about_mi1 = true;
-    }
-
-  /* Do this only after the interpreter is initialized.  */
-  ui->m_current_uiout = interp->interp_ui_out ();
-
-  /* Clear out any installed interpreter hooks/event handlers.  */
-  clear_interpreter_hooks ();
-
-  interp->resume ();
-
-  if (warn_about_mi1)
-    warning (_("MI version 1 is deprecated in GDB 13 and "
-	       "will be removed in GDB 14.  Please upgrade "
-	       "to a newer version of MI."));
-}
-
 /* See interps.h.  */
 
 void
@@ -151,7 +91,7 @@ set_top_level_interpreter (const char *name)
   if (interp == NULL)
     error (_("Interpreter `%s' unrecognized"), name);
   /* Install it.  */
-  interp_set (current_ui, interp, true);
+  current_ui->set_current_interpreter (interp, true);
 }
 
 void
@@ -267,10 +207,10 @@ interpreter_exec_cmd (const char *args, int from_tty)
   if (interp_to_use == NULL)
     error (_("Could not find interpreter \"%s\"."), prules[0]);
 
-  interp_set (current_ui, interp_to_use, false);
+  current_ui->set_current_interpreter (interp_to_use, false);
   SCOPE_EXIT
     {
-      interp_set (current_ui, old_interp, false);
+      current_ui->set_current_interpreter (old_interp, false);
     };
 
   for (i = 1; i < nrules; i++)
diff --git a/gdb/ui.c b/gdb/ui.c
index 8c04dc92b89e..2db899eb9c31 100644
--- a/gdb/ui.c
+++ b/gdb/ui.c
@@ -172,6 +172,57 @@ ui::lookup_interp (const char *name)
 
 /* See ui.h.  */
 
+void
+ui::set_current_interpreter (interp *interp, bool top_level)
+{
+  struct interp *old_interp = this->current_interpreter;
+
+  /* If we already have an interpreter, then trying to
+     set top level interpreter is kinda pointless.  */
+  gdb_assert (!top_level || !this->current_interpreter);
+  gdb_assert (!top_level || !this->top_level_interpreter);
+
+  if (old_interp != NULL)
+    {
+      m_current_uiout->flush ();
+      old_interp->suspend ();
+    }
+
+  this->current_interpreter = interp;
+  if (top_level)
+    this->top_level_interpreter = interp;
+
+  if (interpreter_p != interp->name ())
+    interpreter_p = interp->name ();
+
+  bool warn_about_mi1 = false;
+
+  /* Run the init proc.  */
+  if (!interp->inited)
+    {
+      interp->init (top_level);
+      interp->inited = true;
+
+      if (streq (interp->name (), "mi1"))
+	warn_about_mi1 = true;
+    }
+
+  /* Do this only after the interpreter is initialized.  */
+  m_current_uiout = interp->interp_ui_out ();
+
+  /* Clear out any installed interpreter hooks/event handlers.  */
+  clear_interpreter_hooks ();
+
+  interp->resume ();
+
+  if (warn_about_mi1)
+    warning (_("MI version 1 is deprecated in GDB 13 and "
+	       "will be removed in GDB 14.  Please upgrade "
+	       "to a newer version of MI."));
+}
+
+/* See ui.h.  */
+
 void
 ui::unregister_file_handler ()
 {
diff --git a/gdb/ui.h b/gdb/ui.h
index be89ab3d6848..4f6a32991d6d 100644
--- a/gdb/ui.h
+++ b/gdb/ui.h
@@ -167,6 +167,18 @@ struct ui : public intrusive_list_node<ui>
      interp_factory_register, return nullptr; otherwise return a pointer to
      the interpreter.  */
   interp *lookup_interp (const char *name);
+
+  /* This sets the current interpreter of this UI to be INTERP.  If INTERP has
+     not been initialized, then this will also run the init method.
+
+     The TOP_LEVEL parameter tells if this new interpreter is
+     the top-level one.  The top-level is what is requested
+     on the command line, and is responsible for reporting general
+     notification about target state changes.  For example, if
+     MI is the top-level interpreter, then it will always report
+     events such as target stops and new thread creation, even if they
+     are caused by CLI commands.  */
+  void set_current_interpreter (interp *interp, bool top_level);
 };
 
 /* The main UI.  This is the UI that is bound to stdin/stdout/stderr.
-- 
2.42.0

Re: [PATCH 18/21] gdb: make interp_set a method of struct ui

[Andrew Burgess]

Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:

> Move interp_set to ui::set_current_interpreter.
>
> Change-Id: I7e83ca7732bb6229a1e61dc0e3832f54c20c3f52
> ---
>  gdb/interps.c | 66 +++------------------------------------------------
>  gdb/ui.c      | 51 +++++++++++++++++++++++++++++++++++++++
>  gdb/ui.h      | 12 ++++++++++
>  3 files changed, 66 insertions(+), 63 deletions(-)
>
> diff --git a/gdb/interps.c b/gdb/interps.c
> index 18758f1f7af6..465c174ea794 100644
> --- a/gdb/interps.c
> +++ b/gdb/interps.c
> @@ -80,66 +80,6 @@ find_interp_factory (const char *name)
>    return nullptr;
>  }
>  
> -/* This sets the current interpreter to be INTERP.  If INTERP has not
> -   been initialized, then this will also run the init method.
> -
> -   The TOP_LEVEL parameter tells if this new interpreter is
> -   the top-level one.  The top-level is what is requested
> -   on the command line, and is responsible for reporting general
> -   notification about target state changes.  For example, if
> -   MI is the top-level interpreter, then it will always report
> -   events such as target stops and new thread creation, even if they
> -   are caused by CLI commands.  */
> -
> -static void
> -interp_set (ui *ui, struct interp *interp, bool top_level)
> -{
> -  struct interp *old_interp = ui->current_interpreter;
> -
> -  /* If we already have an interpreter, then trying to
> -     set top level interpreter is kinda pointless.  */
> -  gdb_assert (!top_level || !ui->current_interpreter);
> -  gdb_assert (!top_level || !ui->top_level_interpreter);
> -
> -  if (old_interp != NULL)
> -    {
> -      ui->m_current_uiout->flush ();
> -      old_interp->suspend ();
> -    }
> -
> -  ui->current_interpreter = interp;
> -  if (top_level)
> -    ui->top_level_interpreter = interp;
> -
> -  if (interpreter_p != interp->name ())
> -    interpreter_p = interp->name ();
> -
> -  bool warn_about_mi1 = false;
> -
> -  /* Run the init proc.  */
> -  if (!interp->inited)
> -    {
> -      interp->init (top_level);
> -      interp->inited = true;
> -
> -      if (streq (interp->name (), "mi1"))
> -	warn_about_mi1 = true;
> -    }
> -
> -  /* Do this only after the interpreter is initialized.  */
> -  ui->m_current_uiout = interp->interp_ui_out ();
> -
> -  /* Clear out any installed interpreter hooks/event handlers.  */
> -  clear_interpreter_hooks ();
> -
> -  interp->resume ();
> -
> -  if (warn_about_mi1)
> -    warning (_("MI version 1 is deprecated in GDB 13 and "
> -	       "will be removed in GDB 14.  Please upgrade "
> -	       "to a newer version of MI."));
> -}
> -
>  /* See interps.h.  */
>  
>  void
> @@ -151,7 +91,7 @@ set_top_level_interpreter (const char *name)
>    if (interp == NULL)
>      error (_("Interpreter `%s' unrecognized"), name);
>    /* Install it.  */
> -  interp_set (current_ui, interp, true);
> +  current_ui->set_current_interpreter (interp, true);
>  }
>  
>  void
> @@ -267,10 +207,10 @@ interpreter_exec_cmd (const char *args, int from_tty)
>    if (interp_to_use == NULL)
>      error (_("Could not find interpreter \"%s\"."), prules[0]);
>  
> -  interp_set (current_ui, interp_to_use, false);
> +  current_ui->set_current_interpreter (interp_to_use, false);
>    SCOPE_EXIT
>      {
> -      interp_set (current_ui, old_interp, false);
> +      current_ui->set_current_interpreter (old_interp, false);
>      };
>  
>    for (i = 1; i < nrules; i++)
> diff --git a/gdb/ui.c b/gdb/ui.c
> index 8c04dc92b89e..2db899eb9c31 100644
> --- a/gdb/ui.c
> +++ b/gdb/ui.c
> @@ -172,6 +172,57 @@ ui::lookup_interp (const char *name)
>  
>  /* See ui.h.  */
>  
> +void
> +ui::set_current_interpreter (interp *interp, bool top_level)
> +{
> +  struct interp *old_interp = this->current_interpreter;

Is there any way that we can assert something like:

  gdb_assert (this == interp->m_ui);

we maybe need an interp::mi() accessor function.

> +
> +  /* If we already have an interpreter, then trying to
> +     set top level interpreter is kinda pointless.  */
> +  gdb_assert (!top_level || !this->current_interpreter);
> +  gdb_assert (!top_level || !this->top_level_interpreter);
> +
> +  if (old_interp != NULL)
> +    {
> +      m_current_uiout->flush ();
> +      old_interp->suspend ();
> +    }
> +
> +  this->current_interpreter = interp;
> +  if (top_level)
> +    this->top_level_interpreter = interp;
> +
> +  if (interpreter_p != interp->name ())
> +    interpreter_p = interp->name ();
> +
> +  bool warn_about_mi1 = false;
> +
> +  /* Run the init proc.  */
> +  if (!interp->inited)
> +    {
> +      interp->init (top_level);
> +      interp->inited = true;
> +
> +      if (streq (interp->name (), "mi1"))
> +	warn_about_mi1 = true;
> +    }
> +
> +  /* Do this only after the interpreter is initialized.  */
> +  m_current_uiout = interp->interp_ui_out ();
> +
> +  /* Clear out any installed interpreter hooks/event handlers.  */
> +  clear_interpreter_hooks ();
> +
> +  interp->resume ();
> +
> +  if (warn_about_mi1)
> +    warning (_("MI version 1 is deprecated in GDB 13 and "
> +	       "will be removed in GDB 14.  Please upgrade "
> +	       "to a newer version of MI."));
> +}
> +
> +/* See ui.h.  */
> +
>  void
>  ui::unregister_file_handler ()
>  {
> diff --git a/gdb/ui.h b/gdb/ui.h
> index be89ab3d6848..4f6a32991d6d 100644
> --- a/gdb/ui.h
> +++ b/gdb/ui.h
> @@ -167,6 +167,18 @@ struct ui : public intrusive_list_node<ui>
>       interp_factory_register, return nullptr; otherwise return a pointer to
>       the interpreter.  */
>    interp *lookup_interp (const char *name);
> +
> +  /* This sets the current interpreter of this UI to be INTERP.  If INTERP has
> +     not been initialized, then this will also run the init method.
> +
> +     The TOP_LEVEL parameter tells if this new interpreter is

I know these aren't your words, but could we take this chance to update
this text to:

  The TOP_LEVEL parameter is true if this new interpreter is

which reads much nicer.

Thanks,
Andrew

> +     the top-level one.  The top-level is what is requested
> +     on the command line, and is responsible for reporting general
> +     notification about target state changes.  For example, if
> +     MI is the top-level interpreter, then it will always report
> +     events such as target stops and new thread creation, even if they
> +     are caused by CLI commands.  */
> +  void set_current_interpreter (interp *interp, bool top_level);
>  };
>  
>  /* The main UI.  This is the UI that is bound to stdin/stdout/stderr.
> -- 
> 2.42.0

Re: [PATCH 18/21] gdb: make interp_set a method of struct ui

[Simon Marchi]

>> diff --git a/gdb/ui.c b/gdb/ui.c
>> index 8c04dc92b89e..2db899eb9c31 100644
>> --- a/gdb/ui.c
>> +++ b/gdb/ui.c
>> @@ -172,6 +172,57 @@ ui::lookup_interp (const char *name)
>>  
>>  /* See ui.h.  */
>>  
>> +void
>> +ui::set_current_interpreter (interp *interp, bool top_level)
>> +{
>> +  struct interp *old_interp = this->current_interpreter;
> 
> Is there any way that we can assert something like:
> 
>   gdb_assert (this == interp->m_ui);
> 
> we maybe need an interp::mi() accessor function.
Ah lol, we had the same idea.  I'll add a patch on top of this one to
make this change, since I'd like to keep this one "just move stuff" as
much as possible.

>> diff --git a/gdb/ui.h b/gdb/ui.h
>> index be89ab3d6848..4f6a32991d6d 100644
>> --- a/gdb/ui.h
>> +++ b/gdb/ui.h
>> @@ -167,6 +167,18 @@ struct ui : public intrusive_list_node<ui>
>>       interp_factory_register, return nullptr; otherwise return a pointer to
>>       the interpreter.  */
>>    interp *lookup_interp (const char *name);
>> +
>> +  /* This sets the current interpreter of this UI to be INTERP.  If INTERP has
>> +     not been initialized, then this will also run the init method.
>> +
>> +     The TOP_LEVEL parameter tells if this new interpreter is
> 
> I know these aren't your words, but could we take this chance to update
> this text to:
> 
>   The TOP_LEVEL parameter is true if this new interpreter is
> 
> which reads much nicer.

I'll add a patch on top of this one to do this change.

Simon

Re: [PATCH 18/21] gdb: make interp_set a method of struct ui

[Tom Tromey]

>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:

Simon> +  if (warn_about_mi1)
Simon> +    warning (_("MI version 1 is deprecated in GDB 13 and "
Simon> +	       "will be removed in GDB 14.  Please upgrade "
Simon> +	       "to a newer version of MI."));

Supposedly we did this already... I guess I missed a spot.

Tom

Re: [PATCH 18/21] gdb: make interp_set a method of struct ui

[Simon Marchi]

On 9/12/23 09:41, Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
> Simon> +  if (warn_about_mi1)
> Simon> +    warning (_("MI version 1 is deprecated in GDB 13 and "
> Simon> +	       "will be removed in GDB 14.  Please upgrade "
> Simon> +	       "to a newer version of MI."));
> 
> Supposedly we did this already... I guess I missed a spot.
> 
> Tom

I'll send a separate patch to remove this warning.

Simon

[PATCH 19/21] gdb: pass down current_ui to set_top_level_interpreter

[Simon Marchi]

In preparation for making set_top_level_interpreter a method of struct
ui.

Change-Id: I9d1a67f01f49743ef72c002bd7876e6d2f97b77a
---
 gdb/interps.c | 6 +++---
 gdb/interps.h | 9 +++++----
 2 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/gdb/interps.c b/gdb/interps.c
index 465c174ea794..f954d503538c 100644
--- a/gdb/interps.c
+++ b/gdb/interps.c
@@ -83,15 +83,15 @@ find_interp_factory (const char *name)
 /* See interps.h.  */
 
 void
-set_top_level_interpreter (const char *name)
+set_top_level_interpreter (ui *ui, const char *name)
 {
   /* Find it.  */
-  struct interp *interp = current_ui->lookup_interp (name);
+  struct interp *interp = ui->lookup_interp (name);
 
   if (interp == NULL)
     error (_("Interpreter `%s' unrecognized"), name);
   /* Install it.  */
-  current_ui->set_current_interpreter (interp, true);
+  ui->set_current_interpreter (interp, true);
 }
 
 void
diff --git a/gdb/interps.h b/gdb/interps.h
index 278ee5aff9a5..4c094bf33e32 100644
--- a/gdb/interps.h
+++ b/gdb/interps.h
@@ -215,10 +215,11 @@ class interp : public intrusive_list_node<interp>
   bool inited = false;
 };
 
-/* Set the current UI's top level interpreter to the interpreter named
-   NAME.  Throws an error if NAME is not a known interpreter or the
-   interpreter fails to initialize.  */
-extern void set_top_level_interpreter (const char *name);
+/* Set UI's top level interpreter to the interpreter named NAME.
+
+   Throws an error if NAME is not a known interpreter or the interpreter fails
+   to initialize.  */
+extern void set_top_level_interpreter (ui *ui, const char *name);
 
 /* Temporarily set the current interpreter, and reset it on
    destruction.  */
-- 
2.42.0

Re: [PATCH 19/21] gdb: pass down current_ui to set_top_level_interpreter

[Simon Marchi]

On 9/8/23 14:23, Simon Marchi via Gdb-patches wrote:
> In preparation for making set_top_level_interpreter a method of struct
> ui.
> 
> Change-Id: I9d1a67f01f49743ef72c002bd7876e6d2f97b77a
> ---
>  gdb/interps.c | 6 +++---
>  gdb/interps.h | 9 +++++----
>  2 files changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/gdb/interps.c b/gdb/interps.c
> index 465c174ea794..f954d503538c 100644
> --- a/gdb/interps.c
> +++ b/gdb/interps.c
> @@ -83,15 +83,15 @@ find_interp_factory (const char *name)
>  /* See interps.h.  */
>  
>  void
> -set_top_level_interpreter (const char *name)
> +set_top_level_interpreter (ui *ui, const char *name)
>  {
>    /* Find it.  */
> -  struct interp *interp = current_ui->lookup_interp (name);
> +  struct interp *interp = ui->lookup_interp (name);
>  
>    if (interp == NULL)
>      error (_("Interpreter `%s' unrecognized"), name);
>    /* Install it.  */
> -  current_ui->set_current_interpreter (interp, true);
> +  ui->set_current_interpreter (interp, true);
>  }
>  
>  void
> diff --git a/gdb/interps.h b/gdb/interps.h
> index 278ee5aff9a5..4c094bf33e32 100644
> --- a/gdb/interps.h
> +++ b/gdb/interps.h
> @@ -215,10 +215,11 @@ class interp : public intrusive_list_node<interp>
>    bool inited = false;
>  };
>  
> -/* Set the current UI's top level interpreter to the interpreter named
> -   NAME.  Throws an error if NAME is not a known interpreter or the
> -   interpreter fails to initialize.  */
> -extern void set_top_level_interpreter (const char *name);
> +/* Set UI's top level interpreter to the interpreter named NAME.
> +
> +   Throws an error if NAME is not a known interpreter or the interpreter fails
> +   to initialize.  */
> +extern void set_top_level_interpreter (ui *ui, const char *name);
>  
>  /* Temporarily set the current interpreter, and reset it on
>     destruction.  */
> -- 
> 2.42.0
> 

The Linaro CI helpfully built this patch series and found that this
patch misses updating the set_top_level_interpreter calls, so doesn't
build (the next patch fixes things up).  I have updated this patch
locally to pass current_ui to the places that call
set_top_level_interpreter.

Simon

[PATCH 20/21] gdb: make set_top_level_interpreter a method of struct ui

[Simon Marchi]

No behavior changes expected.

Change-Id: Ie3ab2a2f2191a9df769c51ea81d564724789c6f6
---
 gdb/interps.c | 14 --------------
 gdb/interps.h |  6 ------
 gdb/main.c    |  2 +-
 gdb/ui.c      | 17 ++++++++++++++++-
 gdb/ui.h      |  6 ++++++
 5 files changed, 23 insertions(+), 22 deletions(-)

diff --git a/gdb/interps.c b/gdb/interps.c
index f954d503538c..f30357405877 100644
--- a/gdb/interps.c
+++ b/gdb/interps.c
@@ -80,20 +80,6 @@ find_interp_factory (const char *name)
   return nullptr;
 }
 
-/* See interps.h.  */
-
-void
-set_top_level_interpreter (ui *ui, const char *name)
-{
-  /* Find it.  */
-  struct interp *interp = ui->lookup_interp (name);
-
-  if (interp == NULL)
-    error (_("Interpreter `%s' unrecognized"), name);
-  /* Install it.  */
-  ui->set_current_interpreter (interp, true);
-}
-
 void
 current_interp_set_logging (ui_file_up logfile, bool logging_redirect,
 			    bool debug_redirect)
diff --git a/gdb/interps.h b/gdb/interps.h
index 4c094bf33e32..8416d657fb9a 100644
--- a/gdb/interps.h
+++ b/gdb/interps.h
@@ -215,12 +215,6 @@ class interp : public intrusive_list_node<interp>
   bool inited = false;
 };
 
-/* Set UI's top level interpreter to the interpreter named NAME.
-
-   Throws an error if NAME is not a known interpreter or the interpreter fails
-   to initialize.  */
-extern void set_top_level_interpreter (ui *ui, const char *name);
-
 /* Temporarily set the current interpreter, and reset it on
    destruction.  */
 class scoped_restore_interp
diff --git a/gdb/main.c b/gdb/main.c
index cf46f6acb208..58a79c518999 100644
--- a/gdb/main.c
+++ b/gdb/main.c
@@ -1136,7 +1136,7 @@ captured_main_1 (struct captured_main_args *context)
 
   /* Install the default UI.  All the interpreters should have had a
      look at things by now.  Initialize the default interpreter.  */
-  set_top_level_interpreter (interpreter_p.c_str ());
+  current_ui->set_top_level_interpreter (interpreter_p.c_str ());
 
   if (!quiet)
     {
diff --git a/gdb/ui.c b/gdb/ui.c
index 2db899eb9c31..ec74cc91cd21 100644
--- a/gdb/ui.c
+++ b/gdb/ui.c
@@ -223,6 +223,21 @@ ui::set_current_interpreter (interp *interp, bool top_level)
 
 /* See ui.h.  */
 
+void
+ui::set_top_level_interpreter (const char *name)
+{
+  /* Find it.  */
+  struct interp *interp = this->lookup_interp (name);
+
+  if (interp == NULL)
+    error (_("Interpreter `%s' unrecognized"), name);
+
+  /* Install it.  */
+  this->set_current_interpreter (interp, true);
+}
+
+/* See ui.h.  */
+
 void
 ui::unregister_file_handler ()
 {
@@ -278,7 +293,7 @@ new_ui_command (const char *args, int from_tty)
 
     current_ui = ui.get ();
 
-    set_top_level_interpreter (interpreter_name);
+    current_ui->set_top_level_interpreter (interpreter_name);
 
     top_level_interpreter ()->pre_command_loop ();
 
diff --git a/gdb/ui.h b/gdb/ui.h
index 4f6a32991d6d..cebf112f971c 100644
--- a/gdb/ui.h
+++ b/gdb/ui.h
@@ -179,6 +179,12 @@ struct ui : public intrusive_list_node<ui>
      events such as target stops and new thread creation, even if they
      are caused by CLI commands.  */
   void set_current_interpreter (interp *interp, bool top_level);
+
+  /* Set this UI's top level interpreter to the interpreter named NAME.
+
+     Throws an error if NAME is not a known interpreter or the interpreter fails
+     to initialize.  */
+  void set_top_level_interpreter (const char *name);
 };
 
 /* The main UI.  This is the UI that is bound to stdin/stdout/stderr.
-- 
2.42.0

Re: [PATCH 20/21] gdb: make set_top_level_interpreter a method of struct ui

[Andrew Burgess]

Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:

> No behavior changes expected.
>
> Change-Id: Ie3ab2a2f2191a9df769c51ea81d564724789c6f6
> ---
>  gdb/interps.c | 14 --------------
>  gdb/interps.h |  6 ------
>  gdb/main.c    |  2 +-
>  gdb/ui.c      | 17 ++++++++++++++++-
>  gdb/ui.h      |  6 ++++++
>  5 files changed, 23 insertions(+), 22 deletions(-)
>
> diff --git a/gdb/interps.c b/gdb/interps.c
> index f954d503538c..f30357405877 100644
> --- a/gdb/interps.c
> +++ b/gdb/interps.c
> @@ -80,20 +80,6 @@ find_interp_factory (const char *name)
>    return nullptr;
>  }
>  
> -/* See interps.h.  */
> -
> -void
> -set_top_level_interpreter (ui *ui, const char *name)
> -{
> -  /* Find it.  */
> -  struct interp *interp = ui->lookup_interp (name);
> -
> -  if (interp == NULL)
> -    error (_("Interpreter `%s' unrecognized"), name);
> -  /* Install it.  */
> -  ui->set_current_interpreter (interp, true);
> -}
> -
>  void
>  current_interp_set_logging (ui_file_up logfile, bool logging_redirect,
>  			    bool debug_redirect)
> diff --git a/gdb/interps.h b/gdb/interps.h
> index 4c094bf33e32..8416d657fb9a 100644
> --- a/gdb/interps.h
> +++ b/gdb/interps.h
> @@ -215,12 +215,6 @@ class interp : public intrusive_list_node<interp>
>    bool inited = false;
>  };
>  
> -/* Set UI's top level interpreter to the interpreter named NAME.
> -
> -   Throws an error if NAME is not a known interpreter or the interpreter fails
> -   to initialize.  */
> -extern void set_top_level_interpreter (ui *ui, const char *name);
> -
>  /* Temporarily set the current interpreter, and reset it on
>     destruction.  */
>  class scoped_restore_interp
> diff --git a/gdb/main.c b/gdb/main.c
> index cf46f6acb208..58a79c518999 100644
> --- a/gdb/main.c
> +++ b/gdb/main.c
> @@ -1136,7 +1136,7 @@ captured_main_1 (struct captured_main_args *context)
>  
>    /* Install the default UI.  All the interpreters should have had a
>       look at things by now.  Initialize the default interpreter.  */
> -  set_top_level_interpreter (interpreter_p.c_str ());
> +  current_ui->set_top_level_interpreter (interpreter_p.c_str ());
>  
>    if (!quiet)
>      {
> diff --git a/gdb/ui.c b/gdb/ui.c
> index 2db899eb9c31..ec74cc91cd21 100644
> --- a/gdb/ui.c
> +++ b/gdb/ui.c
> @@ -223,6 +223,21 @@ ui::set_current_interpreter (interp *interp, bool top_level)
>  
>  /* See ui.h.  */
>  
> +void
> +ui::set_top_level_interpreter (const char *name)
> +{
> +  /* Find it.  */
> +  struct interp *interp = this->lookup_interp (name);
> +
> +  if (interp == NULL)

s/NULL/nullptr/

Thanks,
Andrew

> +    error (_("Interpreter `%s' unrecognized"), name);
> +
> +  /* Install it.  */
> +  this->set_current_interpreter (interp, true);
> +}
> +
> +/* See ui.h.  */
> +
>  void
>  ui::unregister_file_handler ()
>  {
> @@ -278,7 +293,7 @@ new_ui_command (const char *args, int from_tty)
>  
>      current_ui = ui.get ();
>  
> -    set_top_level_interpreter (interpreter_name);
> +    current_ui->set_top_level_interpreter (interpreter_name);
>  
>      top_level_interpreter ()->pre_command_loop ();
>  
> diff --git a/gdb/ui.h b/gdb/ui.h
> index 4f6a32991d6d..cebf112f971c 100644
> --- a/gdb/ui.h
> +++ b/gdb/ui.h
> @@ -179,6 +179,12 @@ struct ui : public intrusive_list_node<ui>
>       events such as target stops and new thread creation, even if they
>       are caused by CLI commands.  */
>    void set_current_interpreter (interp *interp, bool top_level);
> +
> +  /* Set this UI's top level interpreter to the interpreter named NAME.
> +
> +     Throws an error if NAME is not a known interpreter or the interpreter fails
> +     to initialize.  */
> +  void set_top_level_interpreter (const char *name);
>  };
>  
>  /* The main UI.  This is the UI that is bound to stdin/stdout/stderr.
> -- 
> 2.42.0

Re: [PATCH 20/21] gdb: make set_top_level_interpreter a method of struct ui

[Simon Marchi]

>> diff --git a/gdb/ui.c b/gdb/ui.c
>> index 2db899eb9c31..ec74cc91cd21 100644
>> --- a/gdb/ui.c
>> +++ b/gdb/ui.c
>> @@ -223,6 +223,21 @@ ui::set_current_interpreter (interp *interp, bool top_level)
>>  
>>  /* See ui.h.  */
>>  
>> +void
>> +ui::set_top_level_interpreter (const char *name)
>> +{
>> +  /* Find it.  */
>> +  struct interp *interp = this->lookup_interp (name);
>> +
>> +  if (interp == NULL)
> 
> s/NULL/nullptr/
This is code that is being moved, but since this is a harmless and
non-behavioral change, I'll include it here.

Thanks,

Simon

[PATCH 21/21] gdb: make top_level_interpreter a method of struct ui

[Simon Marchi]

Rename the field to m_top_level_interpreter and make it private.  Adjust
all callers to use `current_ui->top_level_interpreter ()`.

Change-Id: I6b5461d47d566af5fffc835454c08dd9a1dc9570
---
 gdb/event-top.c    | 2 +-
 gdb/infrun.c       | 4 ++--
 gdb/interps.c      | 8 +-------
 gdb/interps.h      | 3 ---
 gdb/linespec.c     | 3 ++-
 gdb/main.c         | 4 ++--
 gdb/mi/mi-interp.c | 4 ++--
 gdb/solib.c        | 4 +++-
 gdb/tui/tui.c      | 2 +-
 gdb/ui.c           | 6 +++---
 gdb/ui.h           | 8 +++++++-
 gdb/utils.c        | 5 +++--
 12 files changed, 27 insertions(+), 26 deletions(-)

diff --git a/gdb/event-top.c b/gdb/event-top.c
index 54f199d2e393..0dd85cb24d49 100644
--- a/gdb/event-top.c
+++ b/gdb/event-top.c
@@ -299,7 +299,7 @@ change_line_handler (int editing)
 
   /* Don't try enabling editing if the interpreter doesn't support it
      (e.g., MI).  */
-  if (!top_level_interpreter ()->supports_command_editing ()
+  if (!current_ui->top_level_interpreter ()->supports_command_editing ()
       || !command_interp ()->supports_command_editing ())
     return;
 
diff --git a/gdb/infrun.c b/gdb/infrun.c
index aaa7b2de14cd..9129abba8524 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -2916,7 +2916,7 @@ clear_proceed_status_thread (struct thread_info *tp)
 static void
 notify_about_to_proceed ()
 {
-  top_level_interpreter ()->on_about_to_proceed ();
+  current_ui->top_level_interpreter ()->on_about_to_proceed ();
   gdb::observers::about_to_proceed.notify ();
 }
 
@@ -4282,7 +4282,7 @@ check_curr_ui_sync_execution_done (void)
       && !gdb_in_secondary_prompt_p (ui))
     {
       target_terminal::ours ();
-      top_level_interpreter ()->on_sync_execution_done ();
+      current_ui->top_level_interpreter ()->on_sync_execution_done ();
       ui->register_file_handler ();
     }
 }
diff --git a/gdb/interps.c b/gdb/interps.c
index f30357405877..0c5afa970651 100644
--- a/gdb/interps.c
+++ b/gdb/interps.c
@@ -222,12 +222,6 @@ interpreter_completer (struct cmd_list_element *ignore,
     }
 }
 
-struct interp *
-top_level_interpreter (void)
-{
-  return current_ui->top_level_interpreter;
-}
-
 /* See interps.h.  */
 
 struct interp *
@@ -245,7 +239,7 @@ interps_notify (void (interp::*method) (Args...), Args... args)
 {
   SWITCH_THRU_ALL_UIS ()
     {
-      interp *tli = top_level_interpreter ();
+      interp *tli = current_ui->top_level_interpreter ();
       if (tli != nullptr)
 	(tli->*method) (args...);
     }
diff --git a/gdb/interps.h b/gdb/interps.h
index 8416d657fb9a..1073d0516cfe 100644
--- a/gdb/interps.h
+++ b/gdb/interps.h
@@ -258,9 +258,6 @@ extern void current_interp_set_logging (ui_file_up logfile,
 					bool logging_redirect,
 					bool debug_redirect);
 
-/* Returns the top-level interpreter.  */
-extern struct interp *top_level_interpreter (void);
-
 /* Return the current UI's current interpreter.  */
 extern struct interp *current_interpreter (void);
 
diff --git a/gdb/linespec.c b/gdb/linespec.c
index fa733d880e38..bfee951bef2a 100644
--- a/gdb/linespec.c
+++ b/gdb/linespec.c
@@ -48,6 +48,7 @@
 #include "gdbsupport/def-vector.h"
 #include <algorithm>
 #include "inferior.h"
+#include "ui.h"
 
 /* An enumeration of the various things a user might attempt to
    complete for a linespec location.  */
@@ -3168,7 +3169,7 @@ decode_line_full (struct location_spec *locspec, int flags,
 
   if (select_mode == NULL)
     {
-      if (top_level_interpreter ()->interp_ui_out ()->is_mi_like_p ())
+      if (current_ui->top_level_interpreter ()->interp_ui_out ()->is_mi_like_p ())
 	select_mode = multiple_symbols_all;
       else
 	select_mode = multiple_symbols_select_mode ();
diff --git a/gdb/main.c b/gdb/main.c
index 58a79c518999..c31e28a79a74 100644
--- a/gdb/main.c
+++ b/gdb/main.c
@@ -430,7 +430,7 @@ start_event_loop ()
 	     get around to resetting the prompt, which leaves readline
 	     in a messed-up state.  Reset it here.  */
 	  current_ui->prompt_state = PROMPT_NEEDED;
-	  top_level_interpreter ()->on_command_error ();
+	  current_ui->top_level_interpreter ()->on_command_error ();
 	  /* This call looks bizarre, but it is required.  If the user
 	     entered a command that caused an error,
 	     after_char_processing_hook won't be called from
@@ -470,7 +470,7 @@ captured_command_loop ()
 
   /* Give the interpreter a chance to print a prompt, if necessary  */
   if (ui->prompt_state != PROMPT_BLOCKED)
-    top_level_interpreter ()->pre_command_loop ();
+    current_ui->top_level_interpreter ()->pre_command_loop ();
 
   /* Now it's time to start the event loop.  */
   start_event_loop ();
diff --git a/gdb/mi/mi-interp.c b/gdb/mi/mi-interp.c
index 60a2ed0bcc4c..30ba7a347b48 100644
--- a/gdb/mi/mi-interp.c
+++ b/gdb/mi/mi-interp.c
@@ -235,7 +235,7 @@ mi_interp::on_sync_execution_done ()
 static void
 mi_execute_command_input_handler (gdb::unique_xmalloc_ptr<char> &&cmd)
 {
-  struct mi_interp *mi = as_mi_interp (top_level_interpreter ());
+  mi_interp *mi = as_mi_interp (current_ui->top_level_interpreter ());
   struct ui *ui = current_ui;
 
   ui->prompt_state = PROMPT_NEEDED;
@@ -630,7 +630,7 @@ mi_output_running (struct thread_info *thread)
 {
   SWITCH_THRU_ALL_UIS ()
     {
-      struct mi_interp *mi = as_mi_interp (top_level_interpreter ());
+      mi_interp *mi = as_mi_interp (current_ui->top_level_interpreter ());
 
       if (mi == NULL)
 	continue;
diff --git a/gdb/solib.c b/gdb/solib.c
index 4f980e9365c4..e434c7145e4b 100644
--- a/gdb/solib.c
+++ b/gdb/solib.c
@@ -54,6 +54,7 @@
 #include "source.h"
 #include "cli/cli-style.h"
 #include "solib-target.h"
+#include "ui.h"
 
 /* See solib.h.  */
 
@@ -1146,7 +1147,8 @@ info_sharedlibrary_command (const char *pattern, int from_tty)
 	    uiout->field_skip ("to");
 	  }
 
-	if (! top_level_interpreter ()->interp_ui_out ()->is_mi_like_p ()
+	if (!(current_ui->top_level_interpreter ()->interp_ui_out ()
+	      ->is_mi_like_p ())
 	    && so->symbols_loaded
 	    && !objfile_has_symbols (so->objfile))
 	  {
diff --git a/gdb/tui/tui.c b/gdb/tui/tui.c
index 941c65c970f3..818f5f8a1382 100644
--- a/gdb/tui/tui.c
+++ b/gdb/tui/tui.c
@@ -399,7 +399,7 @@ tui_enable (void)
 
       /* If the top level interpreter is not the console/tui (e.g.,
 	 MI), enabling curses will certainly lose.  */
-      interp = top_level_interpreter ()->name ();
+      interp = current_ui->top_level_interpreter ()->name ();
       if (strcmp (interp, INTERP_TUI) != 0)
 	error (_("Cannot enable the TUI when the interpreter is '%s'"), interp);
 
diff --git a/gdb/ui.c b/gdb/ui.c
index ec74cc91cd21..e3d4a4765b80 100644
--- a/gdb/ui.c
+++ b/gdb/ui.c
@@ -180,7 +180,7 @@ ui::set_current_interpreter (interp *interp, bool top_level)
   /* If we already have an interpreter, then trying to
      set top level interpreter is kinda pointless.  */
   gdb_assert (!top_level || !this->current_interpreter);
-  gdb_assert (!top_level || !this->top_level_interpreter);
+  gdb_assert (!top_level || this->m_top_level_interpreter == nullptr);
 
   if (old_interp != NULL)
     {
@@ -190,7 +190,7 @@ ui::set_current_interpreter (interp *interp, bool top_level)
 
   this->current_interpreter = interp;
   if (top_level)
-    this->top_level_interpreter = interp;
+    this->m_top_level_interpreter = interp;
 
   if (interpreter_p != interp->name ())
     interpreter_p = interp->name ();
@@ -295,7 +295,7 @@ new_ui_command (const char *args, int from_tty)
 
     current_ui->set_top_level_interpreter (interpreter_name);
 
-    top_level_interpreter ()->pre_command_loop ();
+    current_ui->top_level_interpreter ()->pre_command_loop ();
 
     /* Make sure the file is not closed.  */
     stream.release ();
diff --git a/gdb/ui.h b/gdb/ui.h
index cebf112f971c..1ba45760c186 100644
--- a/gdb/ui.h
+++ b/gdb/ui.h
@@ -86,7 +86,6 @@ struct ui : public intrusive_list_node<ui>
   /* Each UI has its own independent set of interpreters.  */
   intrusive_list<interp> interp_list;
   interp *current_interpreter = nullptr;
-  interp *top_level_interpreter = nullptr;
 
   /* The interpreter that is active while `interp_exec' is active, NULL
      at all other times.  */
@@ -185,6 +184,13 @@ struct ui : public intrusive_list_node<ui>
      Throws an error if NAME is not a known interpreter or the interpreter fails
      to initialize.  */
   void set_top_level_interpreter (const char *name);
+
+  /* Return this UI's top level interpreter.  */
+  interp *top_level_interpreter ()
+  { return m_top_level_interpreter; }
+
+private:
+  interp *m_top_level_interpreter = nullptr;
 };
 
 /* 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 cacd6cbd23e6..ef46b6bc292d 100644
--- a/gdb/utils.c
+++ b/gdb/utils.c
@@ -1606,8 +1606,9 @@ pager_file::puts (const char *linebuffer)
   /* Don't do any filtering or wrapping if both are disabled.  */
   if (batch_flag
       || (lines_per_page == UINT_MAX && chars_per_line == UINT_MAX)
-      || top_level_interpreter () == NULL
-      || top_level_interpreter ()->interp_ui_out ()->is_mi_like_p ())
+      || current_ui->top_level_interpreter () == NULL
+      || (current_ui->top_level_interpreter ()->interp_ui_out ()
+	  ->is_mi_like_p ()))
     {
       flush_wrap_buffer ();
       m_stream->puts (linebuffer);
-- 
2.42.0

Re: [PATCH 21/21] gdb: make top_level_interpreter a method of struct ui

[Andrew Burgess]

Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:

> Rename the field to m_top_level_interpreter and make it private.  Adjust
> all callers to use `current_ui->top_level_interpreter ()`.
>
> Change-Id: I6b5461d47d566af5fffc835454c08dd9a1dc9570
> ---
>  gdb/event-top.c    | 2 +-
>  gdb/infrun.c       | 4 ++--
>  gdb/interps.c      | 8 +-------
>  gdb/interps.h      | 3 ---
>  gdb/linespec.c     | 3 ++-
>  gdb/main.c         | 4 ++--
>  gdb/mi/mi-interp.c | 4 ++--
>  gdb/solib.c        | 4 +++-
>  gdb/tui/tui.c      | 2 +-
>  gdb/ui.c           | 6 +++---
>  gdb/ui.h           | 8 +++++++-
>  gdb/utils.c        | 5 +++--
>  12 files changed, 27 insertions(+), 26 deletions(-)
>
> diff --git a/gdb/event-top.c b/gdb/event-top.c
> index 54f199d2e393..0dd85cb24d49 100644
> --- a/gdb/event-top.c
> +++ b/gdb/event-top.c
> @@ -299,7 +299,7 @@ change_line_handler (int editing)
>  
>    /* Don't try enabling editing if the interpreter doesn't support it
>       (e.g., MI).  */
> -  if (!top_level_interpreter ()->supports_command_editing ()
> +  if (!current_ui->top_level_interpreter ()->supports_command_editing ()
>        || !command_interp ()->supports_command_editing ())
>      return;
>  
> diff --git a/gdb/infrun.c b/gdb/infrun.c
> index aaa7b2de14cd..9129abba8524 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -2916,7 +2916,7 @@ clear_proceed_status_thread (struct thread_info *tp)
>  static void
>  notify_about_to_proceed ()
>  {
> -  top_level_interpreter ()->on_about_to_proceed ();
> +  current_ui->top_level_interpreter ()->on_about_to_proceed ();
>    gdb::observers::about_to_proceed.notify ();
>  }
>  
> @@ -4282,7 +4282,7 @@ check_curr_ui_sync_execution_done (void)
>        && !gdb_in_secondary_prompt_p (ui))
>      {
>        target_terminal::ours ();
> -      top_level_interpreter ()->on_sync_execution_done ();
> +      current_ui->top_level_interpreter ()->on_sync_execution_done ();
>        ui->register_file_handler ();
>      }
>  }
> diff --git a/gdb/interps.c b/gdb/interps.c
> index f30357405877..0c5afa970651 100644
> --- a/gdb/interps.c
> +++ b/gdb/interps.c
> @@ -222,12 +222,6 @@ interpreter_completer (struct cmd_list_element *ignore,
>      }
>  }
>  
> -struct interp *
> -top_level_interpreter (void)
> -{
> -  return current_ui->top_level_interpreter;
> -}
> -
>  /* See interps.h.  */
>  
>  struct interp *
> @@ -245,7 +239,7 @@ interps_notify (void (interp::*method) (Args...), Args... args)
>  {
>    SWITCH_THRU_ALL_UIS ()
>      {
> -      interp *tli = top_level_interpreter ();
> +      interp *tli = current_ui->top_level_interpreter ();
>        if (tli != nullptr)
>  	(tli->*method) (args...);
>      }
> diff --git a/gdb/interps.h b/gdb/interps.h
> index 8416d657fb9a..1073d0516cfe 100644
> --- a/gdb/interps.h
> +++ b/gdb/interps.h
> @@ -258,9 +258,6 @@ extern void current_interp_set_logging (ui_file_up logfile,
>  					bool logging_redirect,
>  					bool debug_redirect);
>  
> -/* Returns the top-level interpreter.  */
> -extern struct interp *top_level_interpreter (void);
> -
>  /* Return the current UI's current interpreter.  */
>  extern struct interp *current_interpreter (void);
>  
> diff --git a/gdb/linespec.c b/gdb/linespec.c
> index fa733d880e38..bfee951bef2a 100644
> --- a/gdb/linespec.c
> +++ b/gdb/linespec.c
> @@ -48,6 +48,7 @@
>  #include "gdbsupport/def-vector.h"
>  #include <algorithm>
>  #include "inferior.h"
> +#include "ui.h"
>  
>  /* An enumeration of the various things a user might attempt to
>     complete for a linespec location.  */
> @@ -3168,7 +3169,7 @@ decode_line_full (struct location_spec *locspec, int flags,
>  
>    if (select_mode == NULL)
>      {
> -      if (top_level_interpreter ()->interp_ui_out ()->is_mi_like_p ())
> +      if (current_ui->top_level_interpreter ()->interp_ui_out ()->is_mi_like_p ())
>  	select_mode = multiple_symbols_all;
>        else
>  	select_mode = multiple_symbols_select_mode ();
> diff --git a/gdb/main.c b/gdb/main.c
> index 58a79c518999..c31e28a79a74 100644
> --- a/gdb/main.c
> +++ b/gdb/main.c
> @@ -430,7 +430,7 @@ start_event_loop ()
>  	     get around to resetting the prompt, which leaves readline
>  	     in a messed-up state.  Reset it here.  */
>  	  current_ui->prompt_state = PROMPT_NEEDED;
> -	  top_level_interpreter ()->on_command_error ();
> +	  current_ui->top_level_interpreter ()->on_command_error ();
>  	  /* This call looks bizarre, but it is required.  If the user
>  	     entered a command that caused an error,
>  	     after_char_processing_hook won't be called from
> @@ -470,7 +470,7 @@ captured_command_loop ()
>  
>    /* Give the interpreter a chance to print a prompt, if necessary  */
>    if (ui->prompt_state != PROMPT_BLOCKED)
> -    top_level_interpreter ()->pre_command_loop ();
> +    current_ui->top_level_interpreter ()->pre_command_loop ();
>  
>    /* Now it's time to start the event loop.  */
>    start_event_loop ();
> diff --git a/gdb/mi/mi-interp.c b/gdb/mi/mi-interp.c
> index 60a2ed0bcc4c..30ba7a347b48 100644
> --- a/gdb/mi/mi-interp.c
> +++ b/gdb/mi/mi-interp.c
> @@ -235,7 +235,7 @@ mi_interp::on_sync_execution_done ()
>  static void
>  mi_execute_command_input_handler (gdb::unique_xmalloc_ptr<char> &&cmd)
>  {
> -  struct mi_interp *mi = as_mi_interp (top_level_interpreter ());
> +  mi_interp *mi = as_mi_interp (current_ui->top_level_interpreter ());
>    struct ui *ui = current_ui;
>  
>    ui->prompt_state = PROMPT_NEEDED;
> @@ -630,7 +630,7 @@ mi_output_running (struct thread_info *thread)
>  {
>    SWITCH_THRU_ALL_UIS ()
>      {
> -      struct mi_interp *mi = as_mi_interp (top_level_interpreter ());
> +      mi_interp *mi = as_mi_interp (current_ui->top_level_interpreter ());
>  
>        if (mi == NULL)
>  	continue;
> diff --git a/gdb/solib.c b/gdb/solib.c
> index 4f980e9365c4..e434c7145e4b 100644
> --- a/gdb/solib.c
> +++ b/gdb/solib.c
> @@ -54,6 +54,7 @@
>  #include "source.h"
>  #include "cli/cli-style.h"
>  #include "solib-target.h"
> +#include "ui.h"
>  
>  /* See solib.h.  */
>  
> @@ -1146,7 +1147,8 @@ info_sharedlibrary_command (const char *pattern, int from_tty)
>  	    uiout->field_skip ("to");
>  	  }
>  
> -	if (! top_level_interpreter ()->interp_ui_out ()->is_mi_like_p ()
> +	if (!(current_ui->top_level_interpreter ()->interp_ui_out ()
> +	      ->is_mi_like_p ())
>  	    && so->symbols_loaded
>  	    && !objfile_has_symbols (so->objfile))
>  	  {
> diff --git a/gdb/tui/tui.c b/gdb/tui/tui.c
> index 941c65c970f3..818f5f8a1382 100644
> --- a/gdb/tui/tui.c
> +++ b/gdb/tui/tui.c
> @@ -399,7 +399,7 @@ tui_enable (void)
>  
>        /* If the top level interpreter is not the console/tui (e.g.,
>  	 MI), enabling curses will certainly lose.  */
> -      interp = top_level_interpreter ()->name ();
> +      interp = current_ui->top_level_interpreter ()->name ();
>        if (strcmp (interp, INTERP_TUI) != 0)
>  	error (_("Cannot enable the TUI when the interpreter is '%s'"), interp);
>  
> diff --git a/gdb/ui.c b/gdb/ui.c
> index ec74cc91cd21..e3d4a4765b80 100644
> --- a/gdb/ui.c
> +++ b/gdb/ui.c
> @@ -180,7 +180,7 @@ ui::set_current_interpreter (interp *interp, bool top_level)
>    /* If we already have an interpreter, then trying to
>       set top level interpreter is kinda pointless.  */
>    gdb_assert (!top_level || !this->current_interpreter);
> -  gdb_assert (!top_level || !this->top_level_interpreter);
> +  gdb_assert (!top_level || this->m_top_level_interpreter == nullptr);
>  
>    if (old_interp != NULL)
>      {
> @@ -190,7 +190,7 @@ ui::set_current_interpreter (interp *interp, bool top_level)
>  
>    this->current_interpreter = interp;
>    if (top_level)
> -    this->top_level_interpreter = interp;
> +    this->m_top_level_interpreter = interp;
>  
>    if (interpreter_p != interp->name ())
>      interpreter_p = interp->name ();
> @@ -295,7 +295,7 @@ new_ui_command (const char *args, int from_tty)
>  
>      current_ui->set_top_level_interpreter (interpreter_name);
>  
> -    top_level_interpreter ()->pre_command_loop ();
> +    current_ui->top_level_interpreter ()->pre_command_loop ();
>  
>      /* Make sure the file is not closed.  */
>      stream.release ();
> diff --git a/gdb/ui.h b/gdb/ui.h
> index cebf112f971c..1ba45760c186 100644
> --- a/gdb/ui.h
> +++ b/gdb/ui.h
> @@ -86,7 +86,6 @@ struct ui : public intrusive_list_node<ui>
>    /* Each UI has its own independent set of interpreters.  */
>    intrusive_list<interp> interp_list;
>    interp *current_interpreter = nullptr;
> -  interp *top_level_interpreter = nullptr;
>  
>    /* The interpreter that is active while `interp_exec' is active, NULL
>       at all other times.  */
> @@ -185,6 +184,13 @@ struct ui : public intrusive_list_node<ui>
>       Throws an error if NAME is not a known interpreter or the interpreter fails
>       to initialize.  */
>    void set_top_level_interpreter (const char *name);
> +
> +  /* Return this UI's top level interpreter.  */
> +  interp *top_level_interpreter ()
> +  { return m_top_level_interpreter; }

I think this function should be marked const.

Thanks,
Andrew

> +
> +private:
> +  interp *m_top_level_interpreter = nullptr;
>  };
>  
>  /* 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 cacd6cbd23e6..ef46b6bc292d 100644
> --- a/gdb/utils.c
> +++ b/gdb/utils.c
> @@ -1606,8 +1606,9 @@ pager_file::puts (const char *linebuffer)
>    /* Don't do any filtering or wrapping if both are disabled.  */
>    if (batch_flag
>        || (lines_per_page == UINT_MAX && chars_per_line == UINT_MAX)
> -      || top_level_interpreter () == NULL
> -      || top_level_interpreter ()->interp_ui_out ()->is_mi_like_p ())
> +      || current_ui->top_level_interpreter () == NULL
> +      || (current_ui->top_level_interpreter ()->interp_ui_out ()
> +	  ->is_mi_like_p ()))
>      {
>        flush_wrap_buffer ();
>        m_stream->puts (linebuffer);
> -- 
> 2.42.0

Re: [PATCH 21/21] gdb: make top_level_interpreter a method of struct ui

[Simon Marchi]

>> @@ -185,6 +184,13 @@ struct ui : public intrusive_list_node<ui>
>>       Throws an error if NAME is not a known interpreter or the interpreter fails
>>       to initialize.  */
>>    void set_top_level_interpreter (const char *name);
>> +
>> +  /* Return this UI's top level interpreter.  */
>> +  interp *top_level_interpreter ()
>> +  { return m_top_level_interpreter; }
> 
> I think this function should be marked const.
If we want things to be const-correct, then I guess a const version of
this method should return a const interp.  And that would be a bunch
more work (if even possible).  But I guess that marking the method const
and returning a non-const interp doesn't have downsides versus what I
have above, so let's add const.

Simon

Re: [PATCH 00/21] ui / interp cleansup

[Andrew Burgess]

Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:

> When trying to untangle things in the ui and interp area (notably,
> trying to understand what happens when setting and restoring
> interpreters, in the context of interpreter-exec), I came up with a few
> cleanups.  Here is a first round.
>
> One goal is to make some interp operations be methods of struct ui.
> Interpreters are children of UIs, so I think it makes sense if the
> process of looking up / creating interpreters is implemented in methods
> of struct ui.  The concept of top level interpreter is also per UI, so I
> made getting and setting the top level interpreter methods of struct ui.
> I plan to do the same with the current and the command interpreter
> later, but it wasn't as obvious.
>
> Another goal is to try to reduce the number of references to current_ui
> in the code that manages interpreters.  Such references are often behind
> macros, which in my opinion makes it difficult to understand how things
> work.  To this end, there is one patch that adds a backlink from
> interpreters to their parent UI, so that interpreters know which UI they
> belong to (and they should never need to touch current_ui).

Anything we can do to make GDB's I/O & interpreter sub-system simpler --
or at least easier to understand -- is a good thing, I've always found
this part of GDB painful to work with.

I've gone through all the patches and left a few comments, but mostly I
think this looks great.

Reviewed-By: Andrew Burgess <aburgess@redhat.com>

Thanks,
Andrew

>
> Simon Marchi (21):
>   gdb: use intrusive_list for struct ui linked list
>   gdb: make interp_lookup_existing a method of struct ui
>   gdb: make interp_add a method of struct ui
>   gdb: make interp_lookup a method of struct ui
>   gdb: remove ui:::add_interp and ui::lookup_existing_interp
>   gdb: uncover some current_ui uses in interp_set
>   gdb: add backlink to ui in interp
>   gdb: pass ui down to gdb_setup_readline and gdb_disable_readline
>   gdb/python: use m_ui instead of current_ui in dap_interp::init
>   gdb/mi: use m_ui instead of current_ui in mi_interp::init
>   gdb/cli: use m_ui instead of current_ui in cli_interp::resume
>   gdb/tui: use m_ui instead of current_ui in tui_interp::resume
>   gdb/mi: use m_ui instead of current_ui in mi_interp::resume
>   gdb/cli: use m_ui instead of current_ui in cli_interp::suspend
>   gdb/tui: use m_ui instead of current_ui in tui_interp::suspend
>   gdb/mi: use m_ui instead of current_ui in mi_interp::suspend
>   gdb: pass current_ui down to interp_set
>   gdb: make interp_set a method of struct ui
>   gdb: pass down current_ui to set_top_level_interpreter
>   gdb: make set_top_level_interpreter a method of struct ui
>   gdb: make top_level_interpreter a method of struct ui
>
>  gdb/cli/cli-interp.c |  25 +++----
>  gdb/cli/cli-interp.h |   2 +-
>  gdb/cli/cli-script.c |   2 +-
>  gdb/event-top.c      |  10 +--
>  gdb/event-top.h      |   4 +-
>  gdb/infrun.c         |   8 +--
>  gdb/interps.c        | 159 ++++---------------------------------------
>  gdb/interps.h        |  40 ++++++-----
>  gdb/linespec.c       |   3 +-
>  gdb/main.c           |   6 +-
>  gdb/mi/mi-interp.c   |  23 +++----
>  gdb/mi/mi-interp.h   |   4 +-
>  gdb/python/py-dap.c  |  12 ++--
>  gdb/python/python.c  |   2 +-
>  gdb/solib.c          |   4 +-
>  gdb/top.c            |   6 +-
>  gdb/tui/tui-interp.c |  19 +++---
>  gdb/tui/tui.c        |   2 +-
>  gdb/ui.c             | 124 +++++++++++++++++++++++++--------
>  gdb/ui.h             |  64 ++++++++++++-----
>  gdb/utils.c          |   5 +-
>  21 files changed, 243 insertions(+), 281 deletions(-)
>
>
> base-commit: 15db2284f2f8259e46635ca6df3efc772d951fac
> -- 
> 2.42.0

Re: [PATCH 00/21] ui / interp cleansup

[Simon Marchi]

> Anything we can do to make GDB's I/O & interpreter sub-system simpler --
> or at least easier to understand -- is a good thing, I've always found
> this part of GDB painful to work with.
> 
> I've gone through all the patches and left a few comments, but mostly I
> think this looks great.
> 
> Reviewed-By: Andrew Burgess <aburgess@redhat.com>
Thanks, I'll send a v2 though.  I'll note the changes so you don't need
to go through everything again.

Simon

Re: [PATCH 00/21] ui / interp cleansup

[Tom Tromey]

>>>>> "Andrew" == Andrew Burgess via Gdb-patches <gdb-patches@sourceware.org> writes:

Andrew> Anything we can do to make GDB's I/O & interpreter sub-system simpler --
Andrew> or at least easier to understand -- is a good thing, I've always found
Andrew> this part of GDB painful to work with.

Yes, me too.  It's very confusing.

I have an unfinished series to try to clean up the I/O redirect code.
It's unfinished because this area is so hard to deal with.  And due to
MI stuff it seems like we're locked into a bad design :(

Tom