[PATCH 0/4] Remove some uses of iterate_over_inferiors

[PATCH 0/4] Remove some uses of iterate_over_inferiors

[Simon Marchi]

Today, it's very easy to iterate over inferiors using a range-based for
loop combined with all_inferiors.  I think it gives simpler and easier
to understand code than iterate_over_inferiors, which uses a void
pointer to pass data from the caller.  I noticed that there were very
few uses of iterate_over_inferiors left, so I think we could convert
them to range-based for loop and get rid of it.

This patch series removes the uses that are in the files I can build
easily, I can take care of the rest later.

Simon Marchi (4):
  gdb: remove use of iterate_over_inferiors in py-inferior.c
  gdb: remove use of iterate_over_inferiors in mi/mi-interp.c
  gdb: remove uses of iterate_over_inferiors in mi/mi-main.c
  gdb: remove uses of iterate_over_inferiors in top.c

 gdb/mi/mi-interp.c       | 40 +++++++++-------------
 gdb/mi/mi-main.c         | 73 +++++++++++++++-------------------------
 gdb/python/py-inferior.c | 24 ++++++-------
 gdb/top.c                | 51 +++++++++++-----------------
 4 files changed, 72 insertions(+), 116 deletions(-)

-- 
2.25.0

[PATCH 2/4] gdb: remove use of iterate_over_inferiors in mi/mi-interp.c

[Simon Marchi]

Replace it with a range-based for.  I figured that in the hypothetical case
where there are multiple inferiors, we only need to configure the terminal and
flush the output once, so I have moved these out of the loop.

gdb/ChangeLog:

	* mi/mi-interp.c (report_initial_inferior): Remove.
	(mi_interp::init): Use range-based for to iterate over inferiors.
---
 gdb/mi/mi-interp.c | 40 ++++++++++++++++------------------------
 1 file changed, 16 insertions(+), 24 deletions(-)

diff --git a/gdb/mi/mi-interp.c b/gdb/mi/mi-interp.c
index 2ac4c119961c..6f4f56849611 100644
--- a/gdb/mi/mi-interp.c
+++ b/gdb/mi/mi-interp.c
@@ -91,8 +91,6 @@ static void mi_memory_changed (struct inferior *inf, CORE_ADDR memaddr,
 			       ssize_t len, const bfd_byte *myaddr);
 static void mi_on_sync_execution_done (void);
 
-static int report_initial_inferior (struct inferior *inf, void *closure);
-
 /* Display the MI prompt.  */
 
 static void
@@ -140,8 +138,22 @@ mi_interp::init (bool top_level)
       /* The initial inferior is created before this function is
 	 called, so we need to report it explicitly.  Use iteration in
 	 case future version of GDB creates more than one inferior
-	 up-front.  */
-      iterate_over_inferiors (report_initial_inferior, mi);
+	 up-front.
+
+         This function is called from mi_interpreter_init, and since
+	 mi_inferior_added assumes that inferior is fully initialized
+	 and top_level_interpreter_data is set, we cannot call
+	 it here.  */
+
+      target_terminal::scoped_restore_terminal_state term_state;
+      target_terminal::ours_for_output ();
+
+      for (inferior *inf : all_inferiors ())
+	fprintf_unfiltered (mi->event_channel,
+			    "thread-group-added,id=\"i%d\"",
+			    inf->num);
+
+      gdb_flush (mi->event_channel);
     }
 }
 
@@ -1253,26 +1265,6 @@ mi_user_selected_context_changed (user_selected_what selection)
     }
 }
 
-static int
-report_initial_inferior (struct inferior *inf, void *closure)
-{
-  /* This function is called from mi_interpreter_init, and since
-     mi_inferior_added assumes that inferior is fully initialized
-     and top_level_interpreter_data is set, we cannot call
-     it here.  */
-  struct mi_interp *mi = (struct mi_interp *) closure;
-
-  target_terminal::scoped_restore_terminal_state term_state;
-  target_terminal::ours_for_output ();
-
-  fprintf_unfiltered (mi->event_channel,
-		      "thread-group-added,id=\"i%d\"",
-		      inf->num);
-  gdb_flush (mi->event_channel);
-
-  return 0;
-}
-
 ui_out *
 mi_interp::interp_ui_out ()
 {
-- 
2.25.0

[PATCH 4/4] gdb: remove uses of iterate_over_inferiors in top.c

[Simon Marchi]

Replace with range-based for loops.

gdb/ChangeLog:

        * top.c (struct qt_args): Remove.
        (kill_or_detach): Change return type to void, replace `void *`
	parameter with a proper one.
        (print_inferior_quit_action):  Likewise.
        (quit_confirm): Use range-based for loop to iterate over inferiors.
        (quit_force): Likewise.
---
 gdb/top.c | 51 +++++++++++++++++++--------------------------------
 1 file changed, 19 insertions(+), 32 deletions(-)

diff --git a/gdb/top.c b/gdb/top.c
index a266bfa59256..f702af9acd34 100644
--- a/gdb/top.c
+++ b/gdb/top.c
@@ -1607,21 +1607,14 @@ set_prompt (const char *s)
 }
 \f
 
-struct qt_args
-{
-  int from_tty;
-};
-
-/* Callback for iterate_over_inferiors.  Kills or detaches the given
-   inferior, depending on how we originally gained control of it.  */
+/* Kills or detaches the given inferior, depending on how we originally
+   gained control of it.  */
 
-static int
-kill_or_detach (struct inferior *inf, void *args)
+static void
+kill_or_detach (inferior *inf, int from_tty)
 {
-  struct qt_args *qt = (struct qt_args *) args;
-
   if (inf->pid == 0)
-    return 0;
+    return;
 
   thread_info *thread = any_thread_of_inferior (inf);
   if (thread != NULL)
@@ -1632,37 +1625,30 @@ kill_or_detach (struct inferior *inf, void *args)
       if (target_has_execution)
 	{
 	  if (inf->attach_flag)
-	    target_detach (inf, qt->from_tty);
+	    target_detach (inf, from_tty);
 	  else
 	    target_kill ();
 	}
     }
-
-  return 0;
 }
 
-/* Callback for iterate_over_inferiors.  Prints info about what GDB
-   will do to each inferior on a "quit".  ARG points to a struct
-   ui_out where output is to be collected.  */
+/* Prints info about what GDB will do to inferior INF on a "quit".  OUT is
+   where to collect the output.  */
 
-static int
-print_inferior_quit_action (struct inferior *inf, void *arg)
+static void
+print_inferior_quit_action (inferior *inf, ui_file *out)
 {
-  struct ui_file *stb = (struct ui_file *) arg;
-
   if (inf->pid == 0)
-    return 0;
+    return;
 
   if (inf->attach_flag)
-    fprintf_filtered (stb,
+    fprintf_filtered (out,
 		      _("\tInferior %d [%s] will be detached.\n"), inf->num,
 		      target_pid_to_str (ptid_t (inf->pid)).c_str ());
   else
-    fprintf_filtered (stb,
+    fprintf_filtered (out,
 		      _("\tInferior %d [%s] will be killed.\n"), inf->num,
 		      target_pid_to_str (ptid_t (inf->pid)).c_str ());
-
-  return 0;
 }
 
 /* If necessary, make the user confirm that we should quit.  Return
@@ -1679,7 +1665,10 @@ quit_confirm (void)
   string_file stb;
 
   stb.puts (_("A debugging session is active.\n\n"));
-  iterate_over_inferiors (print_inferior_quit_action, &stb);
+
+  for (inferior *inf : all_inferiors ())
+    print_inferior_quit_action (inf, &stb);
+
   stb.puts (_("\nQuit anyway? "));
 
   return query ("%s", stb.c_str ());
@@ -1712,7 +1701,6 @@ void
 quit_force (int *exit_arg, int from_tty)
 {
   int exit_code = 0;
-  struct qt_args qt;
 
   undo_terminal_modifications_before_exit ();
 
@@ -1723,15 +1711,14 @@ quit_force (int *exit_arg, int from_tty)
   else if (return_child_result)
     exit_code = return_child_result_value;
 
-  qt.from_tty = from_tty;
-
   /* We want to handle any quit errors and exit regardless.  */
 
   /* Get out of tfind mode, and kill or detach all inferiors.  */
   try
     {
       disconnect_tracing ();
-      iterate_over_inferiors (kill_or_detach, &qt);
+      for (inferior *inf : all_inferiors ())
+	kill_or_detach (inf, from_tty);
     }
   catch (const gdb_exception &ex)
     {
-- 
2.25.0

[PATCH 1/4] gdb: remove use of iterate_over_inferiors in py-inferior.c

[Simon Marchi]

Use range-based for instead of iterate_over_inferiors in one spot in the Python
code.

gdb/ChangeLog:

	* python/py-inferior.c (build_inferior_list): Remove.
	(gdbpy_ref): Use range-based for loop to iterate over inferiors.
---
 gdb/python/py-inferior.c | 24 ++++++++++--------------
 1 file changed, 10 insertions(+), 14 deletions(-)

diff --git a/gdb/python/py-inferior.c b/gdb/python/py-inferior.c
index 4adc5d6f9988..fd7d8a8aa709 100644
--- a/gdb/python/py-inferior.c
+++ b/gdb/python/py-inferior.c
@@ -462,18 +462,6 @@ infpy_get_progspace (PyObject *self, void *closure)
   return pspace_to_pspace_object (pspace).release ();
 }
 
-static int
-build_inferior_list (struct inferior *inf, void *arg)
-{
-  PyObject *list = (PyObject *) arg;
-  gdbpy_ref<inferior_object> inferior = inferior_to_inferior_object (inf);
-
-  if (inferior == NULL)
-    return 0;
-
-  return PyList_Append (list, (PyObject *) inferior.get ()) ? 1 : 0;
-}
-
 /* Implementation of gdb.inferiors () -> (gdb.Inferior, ...).
    Returns a tuple of all inferiors.  */
 PyObject *
@@ -483,8 +471,16 @@ gdbpy_inferiors (PyObject *unused, PyObject *unused2)
   if (list == NULL)
     return NULL;
 
-  if (iterate_over_inferiors (build_inferior_list, list.get ()))
-    return NULL;
+  for (inferior *inf : all_inferiors ())
+    {
+      gdbpy_ref<inferior_object> inferior = inferior_to_inferior_object (inf);
+
+      if (inferior == NULL)
+	continue;
+
+      if (PyList_Append (list.get (), (PyObject *) inferior.get ()) != 0)
+	return NULL;
+    }
 
   return PyList_AsTuple (list.get ());
 }
-- 
2.25.0

[PATCH 3/4] gdb: remove uses of iterate_over_inferiors in mi/mi-main.c

[Simon Marchi]

Replace with range-based loops.

gdb/ChangeLog:

	* mi/mi-main.c (run_one_inferior): Change return type to void, replace
	`void *` parameter with proper parameters.
	(mi_cmd_exec_run): Use range-based loop to iterate over inferiors.
	(print_one_inferior): Change return type to void, replace `void *`
	parameter with proper parameters.
	(mi_cmd_list_thread_groups): Use range-based loop to iterate over
	inferiors.
	(get_other_inferior): Remove.
	(mi_cmd_remove_inferior): Use range-based loop to iterate over
	inferiors.
---
 gdb/mi/mi-main.c | 73 ++++++++++++++++++------------------------------
 1 file changed, 27 insertions(+), 46 deletions(-)

diff --git a/gdb/mi/mi-main.c b/gdb/mi/mi-main.c
index 014feaf64937..d0a3b2887440 100644
--- a/gdb/mi/mi-main.c
+++ b/gdb/mi/mi-main.c
@@ -390,17 +390,14 @@ mi_cmd_exec_interrupt (const char *command, char **argv, int argc)
     }
 }
 
-/* Callback for iterate_over_inferiors which starts the execution
-   of the given inferior.
+/* Start the execution of the given inferior.
 
-   ARG is a pointer to an integer whose value, if non-zero, indicates
-   that the program should be stopped when reaching the main subprogram
-   (similar to what the CLI "start" command does).  */
+   START_P indicates whether the program should be stopped when reaching the
+   main subprogram (similar to what the CLI "start" command does).  */
 
-static int
-run_one_inferior (struct inferior *inf, void *arg)
+static void
+run_one_inferior (inferior *inf, bool start_p)
 {
-  int start_p = *(int *) arg;
   const char *run_cmd = start_p ? "start" : "run";
   struct target_ops *run_target = find_run_target ();
   int async_p = mi_async && run_target->can_async_p ();
@@ -417,7 +414,6 @@ run_one_inferior (struct inferior *inf, void *arg)
     switch_to_inferior_no_thread (inf);
   mi_execute_cli_command (run_cmd, async_p,
 			  async_p ? "&" : NULL);
-  return 0;
 }
 
 void
@@ -462,7 +458,8 @@ mi_cmd_exec_run (const char *command, char **argv, int argc)
     {
       scoped_restore_current_pspace_and_thread restore_pspace_thread;
 
-      iterate_over_inferiors (run_one_inferior, &start_p);
+      for (inferior *inf : all_inferiors ())
+	run_one_inferior (inf, start_p);
     }
   else
     {
@@ -633,16 +630,13 @@ struct print_one_inferior_data
   const std::set<int> *inferiors;
 };
 
-static int
-print_one_inferior (struct inferior *inferior, void *xdata)
+static void
+print_one_inferior (struct inferior *inferior, bool recurse,
+		    const std::set<int> &ids)
 {
-  struct print_one_inferior_data *top_data
-    = (struct print_one_inferior_data *) xdata;
   struct ui_out *uiout = current_uiout;
 
-  if (top_data->inferiors->empty ()
-      || (top_data->inferiors->find (inferior->pid)
-	  != top_data->inferiors->end ()))
+  if (ids.empty () || (ids.find (inferior->pid) != ids.end ()))
     {
       struct collect_cores_data data;
       ui_out_emit_tuple tuple_emitter (uiout, NULL);
@@ -675,11 +669,9 @@ print_one_inferior (struct inferior *inferior, void *xdata)
 	    uiout->field_signed (NULL, b);
 	}
 
-      if (top_data->recurse)
+      if (recurse)
 	print_thread_info (uiout, NULL, inferior->pid);
     }
-
-  return 0;
 }
 
 /* Output a field named 'cores' with a list as the value.  The
@@ -853,18 +845,14 @@ mi_cmd_list_thread_groups (const char *command, char **argv, int argc)
     }
   else
     {
-      struct print_one_inferior_data data;
-
-      data.recurse = recurse;
-      data.inferiors = &ids;
-
       /* Local thread groups.  Either no explicit ids -- and we
 	 print everything, or several explicit ids.  In both cases,
 	 we print more than one group, and have to use 'groups'
 	 as the top-level element.  */
       ui_out_emit_list list_emitter (uiout, "groups");
       update_thread_list ();
-      iterate_over_inferiors (print_one_inferior, &data);
+      for (inferior *inf : all_inferiors ())
+	print_one_inferior (inf, recurse, ids);
     }
 }
 
@@ -1719,23 +1707,11 @@ mi_cmd_add_inferior (const char *command, char **argv, int argc)
   current_uiout->field_fmt ("inferior", "i%d", inf->num);
 }
 
-/* Callback used to find the first inferior other than the current
-   one.  */
-
-static int
-get_other_inferior (struct inferior *inf, void *arg)
-{
-  if (inf == current_inferior ())
-    return 0;
-
-  return 1;
-}
-
 void
 mi_cmd_remove_inferior (const char *command, char **argv, int argc)
 {
   int id;
-  struct inferior *inf;
+  struct inferior *inf_to_remove;
 
   if (argc != 1)
     error (_("-remove-inferior should be passed a single argument"));
@@ -1743,18 +1719,23 @@ mi_cmd_remove_inferior (const char *command, char **argv, int argc)
   if (sscanf (argv[0], "i%d", &id) != 1)
     error (_("the thread group id is syntactically invalid"));
 
-  inf = find_inferior_id (id);
-  if (!inf)
+  inf_to_remove = find_inferior_id (id);
+  if (inf_to_remove == NULL)
     error (_("the specified thread group does not exist"));
 
-  if (inf->pid != 0)
+  if (inf_to_remove->pid != 0)
     error (_("cannot remove an active inferior"));
 
-  if (inf == current_inferior ())
+  if (inf_to_remove == current_inferior ())
     {
       struct thread_info *tp = 0;
-      struct inferior *new_inferior
-	= iterate_over_inferiors (get_other_inferior, NULL);
+      struct inferior *new_inferior = NULL;
+
+      for (inferior *inf : all_inferiors ())
+	{
+	  if (inf != inf_to_remove)
+	    new_inferior = inf;
+	}
 
       if (new_inferior == NULL)
 	error (_("Cannot remove last inferior"));
@@ -1769,7 +1750,7 @@ mi_cmd_remove_inferior (const char *command, char **argv, int argc)
       set_current_program_space (new_inferior->pspace);
     }
 
-  delete_inferior (inf);
+  delete_inferior (inf_to_remove);
 }
 
 \f
-- 
2.25.0

RE: [PATCH 2/4] gdb: remove use of iterate_over_inferiors in mi/mi-interp.c

[Aktemur, Tankut Baris]

On Wednesday, January 15, 2020 8:12 PM, Simon Marchi wrote:
> Replace it with a range-based for.  I figured that in the hypothetical case
> where there are multiple inferiors, we only need to configure the terminal and
> flush the output once, so I have moved these out of the loop.
> 
> gdb/ChangeLog:
> 
> 	* mi/mi-interp.c (report_initial_inferior): Remove.
> 	(mi_interp::init): Use range-based for to iterate over inferiors.
> ---
>  gdb/mi/mi-interp.c | 40 ++++++++++++++++------------------------
>  1 file changed, 16 insertions(+), 24 deletions(-)
> 
> diff --git a/gdb/mi/mi-interp.c b/gdb/mi/mi-interp.c
> index 2ac4c119961c..6f4f56849611 100644
> --- a/gdb/mi/mi-interp.c
> +++ b/gdb/mi/mi-interp.c
> @@ -91,8 +91,6 @@ static void mi_memory_changed (struct inferior *inf, CORE_ADDR memaddr,
>  			       ssize_t len, const bfd_byte *myaddr);
>  static void mi_on_sync_execution_done (void);
> 
> -static int report_initial_inferior (struct inferior *inf, void *closure);
> -
>  /* Display the MI prompt.  */
> 
>  static void
> @@ -140,8 +138,22 @@ mi_interp::init (bool top_level)
>        /* The initial inferior is created before this function is
>  	 called, so we need to report it explicitly.  Use iteration in
>  	 case future version of GDB creates more than one inferior
> -	 up-front.  */
> -      iterate_over_inferiors (report_initial_inferior, mi);
> +	 up-front.
> +
> +         This function is called from mi_interpreter_init, and since

There is an 8-spaces/tab issue above.  And also, the comment piece "This
function is called from mi_interpreter_init, and" seems obsolete now.

Thanks,
-Baris

> +	 mi_inferior_added assumes that inferior is fully initialized
> +	 and top_level_interpreter_data is set, we cannot call
> +	 it here.  */
> +
> +      target_terminal::scoped_restore_terminal_state term_state;
> +      target_terminal::ours_for_output ();
> +
> +      for (inferior *inf : all_inferiors ())
> +	fprintf_unfiltered (mi->event_channel,
> +			    "thread-group-added,id=\"i%d\"",
> +			    inf->num);
> +
> +      gdb_flush (mi->event_channel);
>      }
>  }
> 
> @@ -1253,26 +1265,6 @@ mi_user_selected_context_changed (user_selected_what selection)
>      }
>  }
> 
> -static int
> -report_initial_inferior (struct inferior *inf, void *closure)
> -{
> -  /* This function is called from mi_interpreter_init, and since
> -     mi_inferior_added assumes that inferior is fully initialized
> -     and top_level_interpreter_data is set, we cannot call
> -     it here.  */
> -  struct mi_interp *mi = (struct mi_interp *) closure;
> -
> -  target_terminal::scoped_restore_terminal_state term_state;
> -  target_terminal::ours_for_output ();
> -
> -  fprintf_unfiltered (mi->event_channel,
> -		      "thread-group-added,id=\"i%d\"",
> -		      inf->num);
> -  gdb_flush (mi->event_channel);
> -
> -  return 0;
> -}
> -
>  ui_out *
>  mi_interp::interp_ui_out ()
>  {
> --
> 2.25.0

Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Christin Eisenschmid, Gary Kershaw
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

Re: [PATCH 2/4] gdb: remove use of iterate_over_inferiors in mi/mi-interp.c

[Simon Marchi]

On 2020-01-16 4:01 a.m., Aktemur, Tankut Baris wrote:
>> @@ -140,8 +138,22 @@ mi_interp::init (bool top_level)
>>        /* The initial inferior is created before this function is
>>  	 called, so we need to report it explicitly.  Use iteration in
>>  	 case future version of GDB creates more than one inferior
>> -	 up-front.  */
>> -      iterate_over_inferiors (report_initial_inferior, mi);
>> +	 up-front.
>> +
>> +         This function is called from mi_interpreter_init, and since
> 
> There is an 8-spaces/tab issue above.  And also, the comment piece "This
> function is called from mi_interpreter_init, and" seems obsolete now.

Thanks for spotting these.

That made me realize there are many issues with that comment.

- The part about not being able to use mi_inferior_added because top_level_interpreter_data
  is not set is stale.  That was true before the interps were made into C++ classes.  Back
  then, the mi_interpreter_init function would return the pointer to be used as interpreter
  data, which the caller (the interp_set function) would assign to interp->data.  So it was
  indeed not possible to call mi_inferior_added, because it required interp->data to be set.

- It is still not possible today to use mi_inferior_added, because it iterates over all MI UIs
  and to print the event on all of them.  So if you do:

  1. Start GDB with `-i mi`
  2. Add some inferiors
  3. Create a new MI UI, with `new-ui mi /dev/pts/X`

  When initializing the new MI UI, it will print the inferior-added events on both MI UIs.

- It is necessary to use iteration, not only in case GDB creates more than one inferior
  up front, but also because there might be multiple inferiors when initializing a secondary
  MI UI, as shown above.

I don't know if it is by design, that the =inferior-added events are sent to a new MI UI added
with new-ui, but in any case I don't want to change the behavior with this patch.

Also, when testing this, I realized it was wrong to put the gdb_flush out of the loop.  It's not
a simple flush as in "make sure the data is all sent to the underlying device", it's also what
adds the final \n!  So with the gdb_flush outside the loop, all the events were printed on the same
line.  I've put the terminal setting calls and the gdb_flush back into the loop, to minimize the
changes.

The patch would now look like this:

From cb845a4f28d1761feb463914c13b0a45c97d9974 Mon Sep 17 00:00:00 2001
From: Simon Marchi <simon.marchi@efficios.com>
Date: Wed, 15 Jan 2020 14:12:20 -0500
Subject: [PATCH] gdb: remove use of iterate_over_inferiors in mi/mi-interp.c

Replace it with a range-based for.  I've updated the comment in
mi_interp::init, which was a bit stale.

gdb/ChangeLog:

	* mi/mi-interp.c (report_initial_inferior): Remove.
	(mi_interp::init): Use range-based for to iterate over inferiors.
---
 gdb/mi/mi-interp.c | 49 ++++++++++++++++++++--------------------------
 1 file changed, 21 insertions(+), 28 deletions(-)

diff --git a/gdb/mi/mi-interp.c b/gdb/mi/mi-interp.c
index 2ac4c119961c..e77093cfa282 100644
--- a/gdb/mi/mi-interp.c
+++ b/gdb/mi/mi-interp.c
@@ -91,8 +91,6 @@ static void mi_memory_changed (struct inferior *inf, CORE_ADDR memaddr,
 			       ssize_t len, const bfd_byte *myaddr);
 static void mi_on_sync_execution_done (void);

-static int report_initial_inferior (struct inferior *inf, void *closure);
-
 /* Display the MI prompt.  */

 static void
@@ -137,12 +135,27 @@ mi_interp::init (bool top_level)

   if (top_level)
     {
-      /* The initial inferior is created before this function is
-	 called, so we need to report it explicitly.  Use iteration in
-	 case future version of GDB creates more than one inferior
-	 up-front.  */
-      iterate_over_inferiors (report_initial_inferior, mi);
-    }
+      /* The initial inferior is created before this function is called, so we
+	 need to report it explicitly when initializing the top-level MI
+	 interpreter.
+
+	 This is also called when additional MI interpreters are added (using
+	 the new-ui command), when multiple inferiors possibly exist, so we need
+	 to use iteration to report all the inferiors.  mi_inferior_added can't
+	 be used, because it would print the event on all the other MI UIs.  */
+
+      for (inferior *inf : all_inferiors ())
+	{
+	  target_terminal::scoped_restore_terminal_state term_state;
+	  target_terminal::ours_for_output ();
+
+	  fprintf_unfiltered (mi->event_channel,
+			      "thread-group-added,id=\"i%d\"",
+			      inf->num);
+
+	  gdb_flush (mi->event_channel);
+	}
+  }
 }

 void
@@ -1253,26 +1266,6 @@ mi_user_selected_context_changed (user_selected_what selection)
     }
 }

-static int
-report_initial_inferior (struct inferior *inf, void *closure)
-{
-  /* This function is called from mi_interpreter_init, and since
-     mi_inferior_added assumes that inferior is fully initialized
-     and top_level_interpreter_data is set, we cannot call
-     it here.  */
-  struct mi_interp *mi = (struct mi_interp *) closure;
-
-  target_terminal::scoped_restore_terminal_state term_state;
-  target_terminal::ours_for_output ();
-
-  fprintf_unfiltered (mi->event_channel,
-		      "thread-group-added,id=\"i%d\"",
-		      inf->num);
-  gdb_flush (mi->event_channel);
-
-  return 0;
-}
-
 ui_out *
 mi_interp::interp_ui_out ()
 {
-- 
2.24.1

Re: [PATCH 0/4] Remove some uses of iterate_over_inferiors

[Tom Tromey]

>>>>> "Simon" == Simon Marchi <simon.marchi@efficios.com> writes:

Simon> Today, it's very easy to iterate over inferiors using a range-based for
Simon> loop combined with all_inferiors.  I think it gives simpler and easier
Simon> to understand code than iterate_over_inferiors, which uses a void
Simon> pointer to pass data from the caller.  I noticed that there were very
Simon> few uses of iterate_over_inferiors left, so I think we could convert
Simon> them to range-based for loop and get rid of it.

I sent some patches like this a while back, but never got around to
finishing the series, so I never checked them in.

https://sourceware.org/ml/gdb-patches/2019-09/msg00381.html

These all look fine to me and I think you should push them.

You wrote one patch I didn't (and I think you did more in mi-main.c
too?), and I wrote one you didn't; I'll resurrect that one and push it
once yours are in:

https://sourceware.org/ml/gdb-patches/2019-09/msg00383.html

Tom

Re: [PATCH 0/4] Remove some uses of iterate_over_inferiors

[Simon Marchi]

On 2020-01-16 10:48 a.m., Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi <simon.marchi@efficios.com> writes:
> 
> Simon> Today, it's very easy to iterate over inferiors using a range-based for
> Simon> loop combined with all_inferiors.  I think it gives simpler and easier
> Simon> to understand code than iterate_over_inferiors, which uses a void
> Simon> pointer to pass data from the caller.  I noticed that there were very
> Simon> few uses of iterate_over_inferiors left, so I think we could convert
> Simon> them to range-based for loop and get rid of it.
> 
> I sent some patches like this a while back, but never got around to
> finishing the series, so I never checked them in.
> 
> https://sourceware.org/ml/gdb-patches/2019-09/msg00381.html
> 
> These all look fine to me and I think you should push them.
> 
> You wrote one patch I didn't (and I think you did more in mi-main.c
> too?), and I wrote one you didn't; I'll resurrect that one and push it
> once yours are in:
> 
> https://sourceware.org/ml/gdb-patches/2019-09/msg00383.html
> 
> Tom
> 

Wow, I had completely forgotten about it!

Your patch for target.c is no longer needed, that call has been removed by
Pedro's multi-target patch.

I can merge my series.  I'll just wait for a response from Baris on my
new version of patch #2.

Simon

RE: [PATCH 2/4] gdb: remove use of iterate_over_inferiors in mi/mi-interp.c

[Aktemur, Tankut Baris]

On Thursday, January 16, 2020 4:24 PM, Simon Marchi wrote:
> On 2020-01-16 4:01 a.m., Aktemur, Tankut Baris wrote:
> >> @@ -140,8 +138,22 @@ mi_interp::init (bool top_level)
> >>        /* The initial inferior is created before this function is
> >>  	 called, so we need to report it explicitly.  Use iteration in
> >>  	 case future version of GDB creates more than one inferior
> >> -	 up-front.  */
> >> -      iterate_over_inferiors (report_initial_inferior, mi);
> >> +	 up-front.
> >> +
> >> +         This function is called from mi_interpreter_init, and since
> >
> > There is an 8-spaces/tab issue above.  And also, the comment piece "This
> > function is called from mi_interpreter_init, and" seems obsolete now.
> 
> Thanks for spotting these.
> 
> That made me realize there are many issues with that comment.
> 
> - The part about not being able to use mi_inferior_added because top_level_interpreter_data
>   is not set is stale.  That was true before the interps were made into C++ classes.  Back
>   then, the mi_interpreter_init function would return the pointer to be used as interpreter
>   data, which the caller (the interp_set function) would assign to interp->data.  So it was
>   indeed not possible to call mi_inferior_added, because it required interp->data to be set.
> 
> - It is still not possible today to use mi_inferior_added, because it iterates over all MI UIs
>   and to print the event on all of them.  So if you do:
> 
>   1. Start GDB with `-i mi`
>   2. Add some inferiors
>   3. Create a new MI UI, with `new-ui mi /dev/pts/X`
> 
>   When initializing the new MI UI, it will print the inferior-added events on both MI UIs.
> 
> - It is necessary to use iteration, not only in case GDB creates more than one inferior
>   up front, but also because there might be multiple inferiors when initializing a secondary
>   MI UI, as shown above.
> 
> I don't know if it is by design, that the =inferior-added events are sent to a new MI UI added
> with new-ui, but in any case I don't want to change the behavior with this patch.
> 
> Also, when testing this, I realized it was wrong to put the gdb_flush out of the loop.  It's not
> a simple flush as in "make sure the data is all sent to the underlying device", it's also what
> adds the final \n!  So with the gdb_flush outside the loop, all the events were printed on the same
> line.  I've put the terminal setting calls and the gdb_flush back into the loop, to minimize the
> changes.
> 
> The patch would now look like this:

Thank you for the detailed explanation.  I think the revised patch is very clear.

-Baris

> 
> From cb845a4f28d1761feb463914c13b0a45c97d9974 Mon Sep 17 00:00:00 2001
> From: Simon Marchi <simon.marchi@efficios.com>
> Date: Wed, 15 Jan 2020 14:12:20 -0500
> Subject: [PATCH] gdb: remove use of iterate_over_inferiors in mi/mi-interp.c
> 
> Replace it with a range-based for.  I've updated the comment in
> mi_interp::init, which was a bit stale.
> 
> gdb/ChangeLog:
> 
> 	* mi/mi-interp.c (report_initial_inferior): Remove.
> 	(mi_interp::init): Use range-based for to iterate over inferiors.
> ---
>  gdb/mi/mi-interp.c | 49 ++++++++++++++++++++--------------------------
>  1 file changed, 21 insertions(+), 28 deletions(-)
> 
> diff --git a/gdb/mi/mi-interp.c b/gdb/mi/mi-interp.c
> index 2ac4c119961c..e77093cfa282 100644
> --- a/gdb/mi/mi-interp.c
> +++ b/gdb/mi/mi-interp.c
> @@ -91,8 +91,6 @@ static void mi_memory_changed (struct inferior *inf, CORE_ADDR memaddr,
>  			       ssize_t len, const bfd_byte *myaddr);
>  static void mi_on_sync_execution_done (void);
> 
> -static int report_initial_inferior (struct inferior *inf, void *closure);
> -
>  /* Display the MI prompt.  */
> 
>  static void
> @@ -137,12 +135,27 @@ mi_interp::init (bool top_level)
> 
>    if (top_level)
>      {
> -      /* The initial inferior is created before this function is
> -	 called, so we need to report it explicitly.  Use iteration in
> -	 case future version of GDB creates more than one inferior
> -	 up-front.  */
> -      iterate_over_inferiors (report_initial_inferior, mi);
> -    }
> +      /* The initial inferior is created before this function is called, so we
> +	 need to report it explicitly when initializing the top-level MI
> +	 interpreter.
> +
> +	 This is also called when additional MI interpreters are added (using
> +	 the new-ui command), when multiple inferiors possibly exist, so we need
> +	 to use iteration to report all the inferiors.  mi_inferior_added can't
> +	 be used, because it would print the event on all the other MI UIs.  */
> +
> +      for (inferior *inf : all_inferiors ())
> +	{
> +	  target_terminal::scoped_restore_terminal_state term_state;
> +	  target_terminal::ours_for_output ();
> +
> +	  fprintf_unfiltered (mi->event_channel,
> +			      "thread-group-added,id=\"i%d\"",
> +			      inf->num);
> +
> +	  gdb_flush (mi->event_channel);
> +	}
> +  }
>  }
> 
>  void
> @@ -1253,26 +1266,6 @@ mi_user_selected_context_changed (user_selected_what selection)
>      }
>  }
> 
> -static int
> -report_initial_inferior (struct inferior *inf, void *closure)
> -{
> -  /* This function is called from mi_interpreter_init, and since
> -     mi_inferior_added assumes that inferior is fully initialized
> -     and top_level_interpreter_data is set, we cannot call
> -     it here.  */
> -  struct mi_interp *mi = (struct mi_interp *) closure;
> -
> -  target_terminal::scoped_restore_terminal_state term_state;
> -  target_terminal::ours_for_output ();
> -
> -  fprintf_unfiltered (mi->event_channel,
> -		      "thread-group-added,id=\"i%d\"",
> -		      inf->num);
> -  gdb_flush (mi->event_channel);
> -
> -  return 0;
> -}
> -
>  ui_out *
>  mi_interp::interp_ui_out ()
>  {
> --
> 2.24.1
> 

Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Christin Eisenschmid, Gary Kershaw
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

Re: [PATCH 2/4] gdb: remove use of iterate_over_inferiors in mi/mi-interp.c

[Simon Marchi]

On 2020-01-17 2:24 a.m., Aktemur, Tankut Baris wrote:
> Thank you for the detailed explanation.  I think the revised patch is very clear.
> 
> -Baris

Ok thanks for the review, I'll push the series shortly.

Simon