diff mbox

Use std::forward_list for displaced_step_inferior_states

Message ID CAENS6EufMHiop5hJ8+wavYLZVmJyE6vQoApwhwG01P4QXLcYKg@mail.gmail.com
State New
Headers show

Commit Message

David Blaikie Nov. 19, 2018, 11:21 p.m. UTC 
Why forward list of pointers rather than forward list of values?
Forward list of pointers would make two allocations per node, rather
than one, I think?

Ah, I'd replied on the other thread about this with a patch, but my
email got bounced due to rich text (Google Inbox).

I've attached my patch for this - though it uses list instead of
forward_list - good catch on that!

On Mon, Nov 12, 2018 at 11:01 AM Simon Marchi <simon.marchi@ericsson.com> wrote:
>
> Use std::forward_list instead of manually implemented list.  This
> simplifies a bit the code, especially around removal.
>
> Regtested on the buildbot.  There are some failures as always, but I
> think they are unrelated.
>
> gdb/ChangeLog:
>
>         * infrun.c (displaced_step_inferior_states): Change type to
>         std::forward_list.
>         (get_displaced_stepping_state): Adjust.
>         (displaced_step_in_progress_any_inferior): Adjust.
>         (add_displaced_stepping_state): Adjust.
>         (remove_displaced_stepping_state): Adjust.
> ---
>  gdb/infrun.c | 80 +++++++++++++++++++++++-----------------------------
>  1 file changed, 35 insertions(+), 45 deletions(-)
>
> diff --git a/gdb/infrun.c b/gdb/infrun.c
> index 9473d1f20f6..1c48740404e 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -1516,39 +1516,36 @@ struct displaced_step_inferior_state
>
>  /* The list of states of processes involved in displaced stepping
>     presently.  */
> -static struct displaced_step_inferior_state *displaced_step_inferior_states;
> +static std::forward_list<displaced_step_inferior_state *>
> +  displaced_step_inferior_states;
>
>  /* Get the displaced stepping state of process PID.  */
>
> -static struct displaced_step_inferior_state *
> +static displaced_step_inferior_state *
>  get_displaced_stepping_state (inferior *inf)
>  {
> -  struct displaced_step_inferior_state *state;
> -
> -  for (state = displaced_step_inferior_states;
> -       state != NULL;
> -       state = state->next)
> -    if (state->inf == inf)
> -      return state;
> +  for (auto *state : displaced_step_inferior_states)
> +    {
> +      if (state->inf == inf)
> +       return state;
> +    }
>
> -  return NULL;
> +  return nullptr;
>  }
>
>  /* Returns true if any inferior has a thread doing a displaced
>     step.  */
>
> -static int
> -displaced_step_in_progress_any_inferior (void)
> +static bool
> +displaced_step_in_progress_any_inferior ()
>  {
> -  struct displaced_step_inferior_state *state;
> -
> -  for (state = displaced_step_inferior_states;
> -       state != NULL;
> -       state = state->next)
> -    if (state->step_thread != nullptr)
> -      return 1;
> +  for (auto *state : displaced_step_inferior_states)
> +    {
> +      if (state->step_thread != nullptr)
> +       return true;
> +    }
>
> -  return 0;
> +  return false;
>  }
>
>  /* Return true if thread represented by PTID is doing a displaced
> @@ -1584,21 +1581,19 @@ displaced_step_in_progress (inferior *inf)
>     stepping state list, or return a pointer to an already existing
>     entry, if it already exists.  Never returns NULL.  */
>
> -static struct displaced_step_inferior_state *
> +static displaced_step_inferior_state *
>  add_displaced_stepping_state (inferior *inf)
>  {
> -  struct displaced_step_inferior_state *state;
> +  displaced_step_inferior_state *state
> +    = get_displaced_stepping_state (inf);
>
> -  for (state = displaced_step_inferior_states;
> -       state != NULL;
> -       state = state->next)
> -    if (state->inf == inf)
> -      return state;
> +  if (state != nullptr)
> +    return state;
>
>    state = XCNEW (struct displaced_step_inferior_state);
>    state->inf = inf;
> -  state->next = displaced_step_inferior_states;
> -  displaced_step_inferior_states = state;
> +
> +  displaced_step_inferior_states.push_front (state);
>
>    return state;
>  }
> @@ -1627,24 +1622,19 @@ get_displaced_step_closure_by_addr (CORE_ADDR addr)
>  static void
>  remove_displaced_stepping_state (inferior *inf)
>  {
> -  struct displaced_step_inferior_state *it, **prev_next_p;
> -
>    gdb_assert (inf != nullptr);
>
> -  it = displaced_step_inferior_states;
> -  prev_next_p = &displaced_step_inferior_states;
> -  while (it)
> -    {
> -      if (it->inf == inf)
> -       {
> -         *prev_next_p = it->next;
> -         xfree (it);
> -         return;
> -       }
> -
> -      prev_next_p = &it->next;
> -      it = *prev_next_p;
> -    }
> +  displaced_step_inferior_states.remove_if
> +    ([inf] (displaced_step_inferior_state *state)
> +      {
> +       if (state->inf == inf)
> +         {
> +           xfree (state);
> +           return true;
> +         }
> +       else
> +         return false;
> +      });
>  }
>
>  static void
> --
> 2.19.1
>

Comments

Simon Marchi Nov. 20, 2018, 4:03 a.m. UTC | #1 
On 2018-11-19 18:21, David Blaikie wrote:
> Why forward list of pointers rather than forward list of values?
> Forward list of pointers would make two allocations per node, rather
> than one, I think?

You are right, there's no good reason (except that maybe it was a 
smaller step).

> Ah, I'd replied on the other thread about this with a patch, but my
> email got bounced due to rich text (Google Inbox).
> 
> I've attached my patch for this - though it uses list instead of
> forward_list - good catch on that!

Actually, I would use an std::vector.  There's a single object per 
inferior, so that list is likely to be very small.  A vector should be 
faster for pretty much every situation.  From what I can see, it doesn't 
matter if objects are moved (we don't save a pointer to them anywhere).  
Does that sound good to you (I can take care of writing the patch)?

Simon
David Blaikie Nov. 21, 2018, 5:27 p.m. UTC | #2 
On Mon, Nov 19, 2018 at 8:03 PM Simon Marchi <simon.marchi@polymtl.ca> wrote:
>
> On 2018-11-19 18:21, David Blaikie wrote:
> > Why forward list of pointers rather than forward list of values?
> > Forward list of pointers would make two allocations per node, rather
> > than one, I think?
>
> You are right, there's no good reason (except that maybe it was a
> smaller step).
>
> > Ah, I'd replied on the other thread about this with a patch, but my
> > email got bounced due to rich text (Google Inbox).
> >
> > I've attached my patch for this - though it uses list instead of
> > forward_list - good catch on that!
>
> Actually, I would use an std::vector.  There's a single object per
> inferior, so that list is likely to be very small.  A vector should be
> faster for pretty much every situation.  From what I can see, it doesn't
> matter if objects are moved (we don't save a pointer to them anywhere).
> Does that sound good to you (I can take care of writing the patch)?

Yeah, for sure! Thanks!

- Dave

>
> Simon
diff mbox

Patch

diff --git gdb/infrun.c gdb/infrun.c
index 9473d1f20f..87c20a7982 100644
--- gdb/infrun.c
+++ gdb/infrun.c
@@ -21,6 +21,7 @@ 
 #include "defs.h"
 #include "infrun.h"
 #include <ctype.h>
+#include <list>
 #include "symtab.h"
 #include "frame.h"
 #include "inferior.h"
@@ -1484,9 +1485,6 @@  displaced_step_closure::~displaced_step_closure () = default;
 /* Per-inferior displaced stepping state.  */
 struct displaced_step_inferior_state
 {
-  /* Pointer to next in linked list.  */
-  struct displaced_step_inferior_state *next;
-
   /* The process this displaced step state refers to.  */
   inferior *inf;
 
@@ -1516,22 +1514,18 @@  struct displaced_step_inferior_state
 
 /* The list of states of processes involved in displaced stepping
    presently.  */
-static struct displaced_step_inferior_state *displaced_step_inferior_states;
+static std::list<displaced_step_inferior_state> displaced_step_inferior_states;
 
 /* Get the displaced stepping state of process PID.  */
 
 static struct displaced_step_inferior_state *
 get_displaced_stepping_state (inferior *inf)
 {
-  struct displaced_step_inferior_state *state;
-
-  for (state = displaced_step_inferior_states;
-       state != NULL;
-       state = state->next)
-    if (state->inf == inf)
-      return state;
+  for (displaced_step_inferior_state &state : displaced_step_inferior_states)
+    if (state.inf == inf)
+      return &state;
 
-  return NULL;
+  return nullptr;
 }
 
 /* Returns true if any inferior has a thread doing a displaced
@@ -1540,12 +1534,8 @@  get_displaced_stepping_state (inferior *inf)
 static int
 displaced_step_in_progress_any_inferior (void)
 {
-  struct displaced_step_inferior_state *state;
-
-  for (state = displaced_step_inferior_states;
-       state != NULL;
-       state = state->next)
-    if (state->step_thread != nullptr)
+  for (displaced_step_inferior_state &state : displaced_step_inferior_states)
+    if (state.step_thread != nullptr)
       return 1;
 
   return 0;
@@ -1587,20 +1577,13 @@  displaced_step_in_progress (inferior *inf)
 static struct displaced_step_inferior_state *
 add_displaced_stepping_state (inferior *inf)
 {
-  struct displaced_step_inferior_state *state;
+  for (displaced_step_inferior_state &state : displaced_step_inferior_states)
+    if (state.inf == inf)
+      return &state;
 
-  for (state = displaced_step_inferior_states;
-       state != NULL;
-       state = state->next)
-    if (state->inf == inf)
-      return state;
+  displaced_step_inferior_states.push_front({inf});
 
-  state = XCNEW (struct displaced_step_inferior_state);
-  state->inf = inf;
-  state->next = displaced_step_inferior_states;
-  displaced_step_inferior_states = state;
-
-  return state;
+  return &displaced_step_inferior_states.front();
 }
 
 /* If inferior is in displaced stepping, and ADDR equals to starting address
@@ -1627,24 +1610,14 @@  get_displaced_step_closure_by_addr (CORE_ADDR addr)
 static void
 remove_displaced_stepping_state (inferior *inf)
 {
-  struct displaced_step_inferior_state *it, **prev_next_p;
-
   gdb_assert (inf != nullptr);
 
-  it = displaced_step_inferior_states;
-  prev_next_p = &displaced_step_inferior_states;
-  while (it)
-    {
-      if (it->inf == inf)
-	{
-	  *prev_next_p = it->next;
-	  xfree (it);
-	  return;
-	}
-
-      prev_next_p = &it->next;
-      it = *prev_next_p;
-    }
+  for (auto I = displaced_step_inferior_states.begin(), E = displaced_step_inferior_states.end(); I != E; ) {
+    if (I->inf == inf)
+      I = displaced_step_inferior_states.erase(I);
+    else
+      ++I;
+  }
 }
 
 static void