[PATCH] gdb: remove uses of iterate_over_inferiors in remote-sim.c

[PATCH] gdb: remove uses of iterate_over_inferiors in remote-sim.c

[Simon Marchi]

This removes the two uses of iterate_over_inferiors, in favor of
range-based loops.

gdb/ChangeLog:

	* remote-sim.c (check_for_duplicate_sim_descriptor): Remove.
	(get_sim_inferior_data): Remove use of iterate_over_inferiors,
	replace with range-based for.
	(gdbsim_interrupt_inferior): Change return type to void, remove
	unused parameter.
	(gdbsim_target::interrupt): Replace iterate_over_inferiors use
	with a range-based for.
---
 gdb/remote-sim.c | 69 +++++++++++++++++++-----------------------------
 1 file changed, 27 insertions(+), 42 deletions(-)

diff --git a/gdb/remote-sim.c b/gdb/remote-sim.c
index caa94464e1eb..69fdabcb8bc5 100644
--- a/gdb/remote-sim.c
+++ b/gdb/remote-sim.c
@@ -189,21 +189,6 @@ static char **sim_argv = NULL;
 static host_callback gdb_callback;
 static int callbacks_initialized = 0;
 
-/* Callback for iterate_over_inferiors.  It checks to see if the sim
-   descriptor passed via ARG is the same as that for the inferior
-   designated by INF.  Return true if so; false otherwise.  */
-
-static int
-check_for_duplicate_sim_descriptor (struct inferior *inf, void *arg)
-{
-  struct sim_inferior_data *sim_data;
-  SIM_DESC new_sim_desc = (SIM_DESC) arg;
-
-  sim_data = sim_inferior_data_key.get (inf);
-
-  return (sim_data != NULL && sim_data->gdbsim_desc == new_sim_desc);
-}
-
 /* Flags indicating whether or not a sim instance is needed.  One of these
    flags should be passed to get_sim_inferior_data().  */
 
@@ -225,27 +210,33 @@ get_sim_inferior_data (struct inferior *inf, int sim_instance_needed)
   if (sim_instance_needed == SIM_INSTANCE_NEEDED
       && (sim_data == NULL || sim_data->gdbsim_desc == NULL))
     {
-      struct inferior *idup;
       sim_desc = sim_open (SIM_OPEN_DEBUG, &gdb_callback, exec_bfd, sim_argv);
       if (sim_desc == NULL)
 	error (_("Unable to create simulator instance for inferior %d."),
 	       inf->num);
 
-      idup = iterate_over_inferiors (check_for_duplicate_sim_descriptor,
-				     sim_desc);
-      if (idup != NULL)
+      /* Check if the sim descriptor is the same as that of another
+	 inferior.  */
+      for (inferior *other_inf : all_inferiors ())
 	{
-	  /* We don't close the descriptor due to the fact that it's
-	     shared with some other inferior.  If we were to close it,
-	     that might needlessly muck up the other inferior.  Of
-	     course, it's possible that the damage has already been
-	     done...  Note that it *will* ultimately be closed during
-	     cleanup of the other inferior.  */
-	  sim_desc = NULL;
-	  error (
- _("Inferior %d and inferior %d would have identical simulator state.\n"
-   "(This simulator does not support the running of more than one inferior.)"),
-		 inf->num, idup->num);
+	  sim_inferior_data *other_sim_data
+	    = sim_inferior_data_key.get (other_inf);
+
+	  if (other_sim_data != NULL
+	      && other_sim_data->gdbsim_desc == sim_desc)
+	    {
+	      /* We don't close the descriptor due to the fact that it's
+		 shared with some other inferior.  If we were to close it,
+		 that might needlessly muck up the other inferior.  Of
+		 course, it's possible that the damage has already been
+		 done...  Note that it *will* ultimately be closed during
+		 cleanup of the other inferior.  */
+	      sim_desc = NULL;
+	      error (
+_("Inferior %d and inferior %d would have identical simulator state.\n"
+ "(This simulator does not support the running of more than one inferior.)"),
+		     inf->num, other_inf->num);
+	    }
 	}
     }
 
@@ -896,30 +887,24 @@ gdbsim_target::resume (ptid_t ptid, int step, enum gdb_signal siggnal)
 
    For simulators that do not support this operation, just abort.  */
 
-static int
-gdbsim_interrupt_inferior (struct inferior *inf, void *arg)
+static void
+gdbsim_interrupt_inferior (struct inferior *inf)
 {
   struct sim_inferior_data *sim_data
     = get_sim_inferior_data (inf, SIM_INSTANCE_NEEDED);
 
-  if (sim_data)
+  if (sim_data != nullptr)
     {
       if (!sim_stop (sim_data->gdbsim_desc))
-	{
-	  quit ();
-	}
+	quit ();
     }
-
-  /* When called from iterate_over_inferiors, a zero return causes the
-     iteration process to proceed until there are no more inferiors to
-     consider.  */
-  return 0;
 }
 
 void
 gdbsim_target::interrupt ()
 {
-  iterate_over_inferiors (gdbsim_interrupt_inferior, NULL);
+  for (inferior *inf : all_inferiors ())
+    gdbsim_interrupt_inferior (inf);
 }
 
 /* GDB version of os_poll_quit callback.
-- 
2.25.0

Re: [PATCH] gdb: remove uses of iterate_over_inferiors in remote-sim.c

[Tom Tromey]

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

Simon> This removes the two uses of iterate_over_inferiors, in favor of
Simon> range-based loops.

Hah, I was writing the same patch :)

Simon> +static void
Simon> +gdbsim_interrupt_inferior (struct inferior *inf)
Simon>  {
Simon>    struct sim_inferior_data *sim_data
Simon>      = get_sim_inferior_data (inf, SIM_INSTANCE_NEEDED);
 
Simon> -  if (sim_data)
Simon> +  if (sim_data != nullptr)
Simon>      {
Simon>        if (!sim_stop (sim_data->gdbsim_desc))

This could be shortened to using an "&&" instead of a nested if.  And, I
think it would be clearer still to remove gdbsim_interrupt_inferior and
just inline the body into gdbsim_target::interrupt.

Tom

Re: [PATCH] gdb: remove uses of iterate_over_inferiors in remote-sim.c

[Simon Marchi]

On 2020-01-21 2:57 p.m., Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi <simon.marchi@efficios.com> writes:
> 
> Simon> This removes the two uses of iterate_over_inferiors, in favor of
> Simon> range-based loops.
> 
> Hah, I was writing the same patch :)
> 
> Simon> +static void
> Simon> +gdbsim_interrupt_inferior (struct inferior *inf)
> Simon>  {
> Simon>    struct sim_inferior_data *sim_data
> Simon>      = get_sim_inferior_data (inf, SIM_INSTANCE_NEEDED);
>  
> Simon> -  if (sim_data)
> Simon> +  if (sim_data != nullptr)
> Simon>      {
> Simon>        if (!sim_stop (sim_data->gdbsim_desc))
> 
> This could be shortened to using an "&&" instead of a nested if.  And, I
> think it would be clearer still to remove gdbsim_interrupt_inferior and
> just inline the body into gdbsim_target::interrupt.
> 
> Tom
> 

Agreed.  So, something like this?


From abcd58f8987f4af07218b3037e416482fd274edc Mon Sep 17 00:00:00 2001
From: Simon Marchi <simon.marchi@efficios.com>
Date: Mon, 20 Jan 2020 20:52:57 -0500
Subject: [PATCH] gdb: remove uses of iterate_over_inferiors in remote-sim.c

This removes the two uses of iterate_over_inferiors, in favor of
range-based loops.

gdb/ChangeLog:

	* remote-sim.c (check_for_duplicate_sim_descriptor): Remove.
	(get_sim_inferior_data): Remove use of iterate_over_inferiors,
	replace with range-based for.
	(gdbsim_interrupt_inferior): Remove.
	(gdbsim_target::interrupt): Replace iterate_over_inferiors use
	with a range-based for.  Inline code from
	gdbsim_interrupt_inferior.
---
 gdb/remote-sim.c | 78 +++++++++++++++++-------------------------------
 1 file changed, 28 insertions(+), 50 deletions(-)

diff --git a/gdb/remote-sim.c b/gdb/remote-sim.c
index caa94464e1eb..281232cc4e5b 100644
--- a/gdb/remote-sim.c
+++ b/gdb/remote-sim.c
@@ -189,21 +189,6 @@ static char **sim_argv = NULL;
 static host_callback gdb_callback;
 static int callbacks_initialized = 0;

-/* Callback for iterate_over_inferiors.  It checks to see if the sim
-   descriptor passed via ARG is the same as that for the inferior
-   designated by INF.  Return true if so; false otherwise.  */
-
-static int
-check_for_duplicate_sim_descriptor (struct inferior *inf, void *arg)
-{
-  struct sim_inferior_data *sim_data;
-  SIM_DESC new_sim_desc = (SIM_DESC) arg;
-
-  sim_data = sim_inferior_data_key.get (inf);
-
-  return (sim_data != NULL && sim_data->gdbsim_desc == new_sim_desc);
-}
-
 /* Flags indicating whether or not a sim instance is needed.  One of these
    flags should be passed to get_sim_inferior_data().  */

@@ -225,27 +210,33 @@ get_sim_inferior_data (struct inferior *inf, int sim_instance_needed)
   if (sim_instance_needed == SIM_INSTANCE_NEEDED
       && (sim_data == NULL || sim_data->gdbsim_desc == NULL))
     {
-      struct inferior *idup;
       sim_desc = sim_open (SIM_OPEN_DEBUG, &gdb_callback, exec_bfd, sim_argv);
       if (sim_desc == NULL)
 	error (_("Unable to create simulator instance for inferior %d."),
 	       inf->num);

-      idup = iterate_over_inferiors (check_for_duplicate_sim_descriptor,
-				     sim_desc);
-      if (idup != NULL)
+      /* Check if the sim descriptor is the same as that of another
+	 inferior.  */
+      for (inferior *other_inf : all_inferiors ())
 	{
-	  /* We don't close the descriptor due to the fact that it's
-	     shared with some other inferior.  If we were to close it,
-	     that might needlessly muck up the other inferior.  Of
-	     course, it's possible that the damage has already been
-	     done...  Note that it *will* ultimately be closed during
-	     cleanup of the other inferior.  */
-	  sim_desc = NULL;
-	  error (
- _("Inferior %d and inferior %d would have identical simulator state.\n"
-   "(This simulator does not support the running of more than one inferior.)"),
-		 inf->num, idup->num);
+	  sim_inferior_data *other_sim_data
+	    = sim_inferior_data_key.get (other_inf);
+
+	  if (other_sim_data != NULL
+	      && other_sim_data->gdbsim_desc == sim_desc)
+	    {
+	      /* We don't close the descriptor due to the fact that it's
+		 shared with some other inferior.  If we were to close it,
+		 that might needlessly muck up the other inferior.  Of
+		 course, it's possible that the damage has already been
+		 done...  Note that it *will* ultimately be closed during
+		 cleanup of the other inferior.  */
+	      sim_desc = NULL;
+	      error (
+_("Inferior %d and inferior %d would have identical simulator state.\n"
+ "(This simulator does not support the running of more than one inferior.)"),
+		     inf->num, other_inf->num);
+	    }
 	}
     }

@@ -896,30 +887,17 @@ gdbsim_target::resume (ptid_t ptid, int step, enum gdb_signal siggnal)

    For simulators that do not support this operation, just abort.  */

-static int
-gdbsim_interrupt_inferior (struct inferior *inf, void *arg)
+void
+gdbsim_target::interrupt ()
 {
-  struct sim_inferior_data *sim_data
-    = get_sim_inferior_data (inf, SIM_INSTANCE_NEEDED);
-
-  if (sim_data)
+  for (inferior *inf : all_inferiors ())
     {
-      if (!sim_stop (sim_data->gdbsim_desc))
-	{
+      sim_inferior_data *sim_data
+	= get_sim_inferior_data (inf, SIM_INSTANCE_NEEDED);
+
+      if (sim_data != nullptr && !sim_stop (sim_data->gdbsim_desc))
 	  quit ();
-	}
     }
-
-  /* When called from iterate_over_inferiors, a zero return causes the
-     iteration process to proceed until there are no more inferiors to
-     consider.  */
-  return 0;
-}
-
-void
-gdbsim_target::interrupt ()
-{
-  iterate_over_inferiors (gdbsim_interrupt_inferior, NULL);
 }

 /* GDB version of os_poll_quit callback.
-- 
2.25.0

Re: [PATCH] gdb: remove uses of iterate_over_inferiors in remote-sim.c

[Tom Tromey]

>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:

Simon> Agreed.  So, something like this?

Looks great, thanks!

Tom

Re: [PATCH] gdb: remove uses of iterate_over_inferiors in remote-sim.c

[Simon Marchi]

On 2020-01-21 4:27 p.m., Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:
> 
> Simon> Agreed.  So, something like this?
> 
> Looks great, thanks!
> 
> Tom
> 

Thanks, pushed.

The only remaining use is in darwin-nat.c, but we need to un-break it
first (it doesn't build following multi-target).

Simon

Re: [PATCH] gdb: remove uses of iterate_over_inferiors in remote-sim.c

[Tom Tromey]

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

Simon> The only remaining use is in darwin-nat.c, but we need to un-break it
Simon> first (it doesn't build following multi-target).

I will try to look at this soon.

Tom

Re: [PATCH] gdb: remove uses of iterate_over_inferiors in remote-sim.c

[Simon Marchi]

On 2020-01-21 7:45 p.m., Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi <simon.marchi@efficios.com> writes:
> 
> Simon> The only remaining use is in darwin-nat.c, but we need to un-break it
> Simon> first (it doesn't build following multi-target).
> 
> I will try to look at this soon.

I have it building here:

https://github.com/simark/binutils-gdb/tree/darwin-nat-multi-target

But I haven't tested it.  I plan on finishing it and sending a patch eventually,
but if you want to do it before me, I won't object.

Simon