Use std::forward_list for displaced_step_inferior_states
Commit Message
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
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
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
@@ -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