[PATCH] Use std::forward_list for displaced_step_inferior_states

[Simon Marchi]

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

[Kevin Buettner]

On Mon, 12 Nov 2018 19:00:03 +0000

Post by Simon Marchi
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.
* 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.

LGTM.

Kevin

[Simon Marchi]

Post by Kevin Buettner
On Mon, 12 Nov 2018 19:00:03 +0000

Post by Simon Marchi
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.
* 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.

LGTM.
Kevin

Thanks, I pushed it.

Simon

[David Blaikie]

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!

Post by Simon Marchi
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.
* 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

[Simon Marchi]

Post by David Blaikie
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).

Post by David Blaikie
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]

Post by Simon Marchi

Post by David Blaikie
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).

Post by David Blaikie
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

Post by Simon Marchi
Simon