* [PATCH] Use std::forward_list for displaced_step_inferior_states
@ 2018-11-12 19:00 Simon Marchi
2018-11-18 18:56 ` Kevin Buettner
2018-11-19 23:21 ` David Blaikie
0 siblings, 2 replies; 6+ messages in thread
From: Simon Marchi @ 2018-11-12 19:00 UTC (permalink / raw)
To: gdb-patches; +Cc: 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
--
2.19.1
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Use std::forward_list for displaced_step_inferior_states
2018-11-12 19:00 [PATCH] Use std::forward_list for displaced_step_inferior_states Simon Marchi
@ 2018-11-18 18:56 ` Kevin Buettner
2018-11-19 16:58 ` Simon Marchi
2018-11-19 23:21 ` David Blaikie
1 sibling, 1 reply; 6+ messages in thread
From: Kevin Buettner @ 2018-11-18 18:56 UTC (permalink / raw)
To: gdb-patches; +Cc: Simon Marchi
On Mon, 12 Nov 2018 19:00:03 +0000
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.
LGTM.
Kevin
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Use std::forward_list for displaced_step_inferior_states
2018-11-18 18:56 ` Kevin Buettner
@ 2018-11-19 16:58 ` Simon Marchi
0 siblings, 0 replies; 6+ messages in thread
From: Simon Marchi @ 2018-11-19 16:58 UTC (permalink / raw)
To: Kevin Buettner; +Cc: gdb-patches, Simon Marchi
On 2018-11-18 13:56, Kevin Buettner wrote:
> On Mon, 12 Nov 2018 19:00:03 +0000
> 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.
>
> LGTM.
>
> Kevin
Thanks, I pushed it.
Simon
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Use std::forward_list for displaced_step_inferior_states
2018-11-12 19:00 [PATCH] Use std::forward_list for displaced_step_inferior_states Simon Marchi
2018-11-18 18:56 ` Kevin Buettner
@ 2018-11-19 23:21 ` David Blaikie
2018-11-20 4:03 ` Simon Marchi
1 sibling, 1 reply; 6+ messages in thread
From: David Blaikie @ 2018-11-19 23:21 UTC (permalink / raw)
To: Simon Marchi; +Cc: gdb-patches
[-- Attachment #1: Type: text/plain, Size: 4751 bytes --]
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
>
[-- Attachment #2: displaced_step_list.diff --]
[-- Type: text/plain, Size: 3584 bytes --]
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Use std::forward_list for displaced_step_inferior_states
2018-11-19 23:21 ` David Blaikie
@ 2018-11-20 4:03 ` Simon Marchi
2018-11-21 17:27 ` David Blaikie
0 siblings, 1 reply; 6+ messages in thread
From: Simon Marchi @ 2018-11-20 4:03 UTC (permalink / raw)
To: David Blaikie; +Cc: Simon Marchi, gdb-patches
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Use std::forward_list for displaced_step_inferior_states
2018-11-20 4:03 ` Simon Marchi
@ 2018-11-21 17:27 ` David Blaikie
0 siblings, 0 replies; 6+ messages in thread
From: David Blaikie @ 2018-11-21 17:27 UTC (permalink / raw)
To: Simon Marchi; +Cc: Simon Marchi, gdb-patches
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-11-21 17:27 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-11-12 19:00 [PATCH] Use std::forward_list for displaced_step_inferior_states Simon Marchi
2018-11-18 18:56 ` Kevin Buettner
2018-11-19 16:58 ` Simon Marchi
2018-11-19 23:21 ` David Blaikie
2018-11-20 4:03 ` Simon Marchi
2018-11-21 17:27 ` David Blaikie
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).