[PATCH] Use std::vector for displaced_step_inferior_states

[Simon Marchi]

Commit

39a36629f68e ("Use std::forward_list for displaced_step_inferior_states")

changed a hand-made linked list to use std::forward_list of pointers.
As suggested by David Blaikie, we might as well use values instead of
pointers. And instead of a list, we might as well use a vector. The
size of this list will always be at most the number of inferiors,
typically very small. And in any case the operation we do in the
hottest path (doing a displaced step) is iterate, and iterating on a
vector is always faster than a linked list.

A consequence of using a vector is that objects can be moved, when the
vector is resized. I don't think this is a problem, because we don't
save the address of the objects. In displaced_step_prepare_throw, we
save a pointer to the step_saved_copy field in a cleanup, but it is ran
or discarded immediately after.

gdb/ChangeLog:

* infrun.c (struct displaced_step_inferior_state): Add
constructor, initialize fields.
<failed_before>: Change type to bool.
(displaced_step_inferior_states): Change type to vector.
(get_displaced_stepping_state): Adjust.
(displaced_step_in_progress_any_inferior): Adjust.
(add_displaced_stepping_state): Adjust.
(remove_displaced_stepping_state): Adjust.
(displaced_step_prepare): Change 1 to true.
---
gdb/infrun.c | 57 ++++++++++++++++++++++++++--------------------------
1 file changed, 28 insertions(+), 29 deletions(-)

diff --git a/gdb/infrun.c b/gdb/infrun.c
index 3e9acb45aa9..6cdcd1a1f93 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -1484,36 +1484,40 @@ displaced_step_closure::~displaced_step_closure () = default;
/* Per-inferior displaced stepping state. */
struct displaced_step_inferior_state
{
+ displaced_step_inferior_state (inferior *inf)
+ : inf (inf)
+ {}
+
/* The process this displaced step state refers to. */
inferior *inf;

/* True if preparing a displaced step ever failed. If so, we won't
try displaced stepping for this inferior again. */
- int failed_before;
+ bool failed_before = false;

/* If this is not nullptr, this is the thread carrying out a
displaced single-step in process PID. This thread's state will
require fixing up once it has completed its step. */
- thread_info *step_thread;
+ thread_info *step_thread = nullptr;

/* The architecture the thread had when we stepped it. */
- struct gdbarch *step_gdbarch;
+ gdbarch *step_gdbarch = nullptr;

/* The closure provided gdbarch_displaced_step_copy_insn, to be used
for post-step cleanup. */
- struct displaced_step_closure *step_closure;
+ displaced_step_closure *step_closure = nullptr;

/* The address of the original instruction, and the copy we
made. */
- CORE_ADDR step_original, step_copy;
+ CORE_ADDR step_original = 0, step_copy = 0;

/* Saved contents of copy area. */
- gdb_byte *step_saved_copy;
+ gdb_byte *step_saved_copy = nullptr;
};

/* The list of states of processes involved in displaced stepping
presently. */
-static std::forward_list<displaced_step_inferior_state *>
+static std::vector<displaced_step_inferior_state>
displaced_step_inferior_states;

/* Get the displaced stepping state of process PID. */
@@ -1521,10 +1525,10 @@ static std::forward_list<displaced_step_inferior_state *>
static displaced_step_inferior_state *
get_displaced_stepping_state (inferior *inf)
{
- for (auto *state : displaced_step_inferior_states)
+ for (auto &state : displaced_step_inferior_states)
{
- if (state->inf == inf)
- return state;
+ if (state.inf == inf)
+ return &state;
}

return nullptr;
@@ -1536,9 +1540,9 @@ get_displaced_stepping_state (inferior *inf)
static bool
displaced_step_in_progress_any_inferior ()
{
- for (auto *state : displaced_step_inferior_states)
+ for (const auto &state : displaced_step_inferior_states)
{
- if (state->step_thread != nullptr)
+ if (state.step_thread != nullptr)
return true;
}

@@ -1587,12 +1591,9 @@ add_displaced_stepping_state (inferior *inf)
if (state != nullptr)
return state;

- state = XCNEW (struct displaced_step_inferior_state);
- state->inf = inf;
+ displaced_step_inferior_states.emplace_back (inf);

- displaced_step_inferior_states.push_front (state);
-
- return state;
+ return &displaced_step_inferior_states.back ();
}

/* If inferior is in displaced stepping, and ADDR equals to starting address
@@ -1621,17 +1622,15 @@ remove_displaced_stepping_state (inferior *inf)
{
gdb_assert (inf != nullptr);

- displaced_step_inferior_states.remove_if
- ([inf] (displaced_step_inferior_state *state)
- {
- if (state->inf == inf)
- {
- xfree (state);
- return true;
- }
- else
- return false;
- });
+ auto it = std::find_if (displaced_step_inferior_states.begin (),
+ displaced_step_inferior_states.end (),
+ [inf] (const displaced_step_inferior_state &s)
+ {
+ return s.inf == inf;
+ });
+
+ if (it != displaced_step_inferior_states.end ())
+ displaced_step_inferior_states.erase (it);
}

static void
@@ -1910,7 +1909,7 @@ displaced_step_prepare (thread_info *thread)
/* Disable further displaced stepping attempts. */
displaced_state
= get_displaced_stepping_state (thread->inf);
- displaced_state->failed_before = 1;
+ displaced_state->failed_before = true;
}
END_CATCH

[Pedro Alves]

Post by Simon Marchi
Commit
39a36629f68e ("Use std::forward_list for displaced_step_inferior_states")
changed a hand-made linked list to use std::forward_list of pointers.
As suggested by David Blaikie, we might as well use values instead of
pointers. And instead of a list, we might as well use a vector. The
size of this list will always be at most the number of inferiors,
typically very small. And in any case the operation we do in the
hottest path (doing a displaced step) is iterate, and iterating on a
vector is always faster than a linked list.
A consequence of using a vector is that objects can be moved, when the
vector is resized. I don't think this is a problem, because we don't
save the address of the objects. In displaced_step_prepare_throw, we
save a pointer to the step_saved_copy field in a cleanup, but it is ran
or discarded immediately after.

Another alternative would be to put the displaced_step_inferior_state
object in struct inferior directly instead of keeping the objects
on the side. In practice, on x86 GNU/Linux at least, you end
up with an object per inferior anyway, assuming we actually
run the inferiors, which sounds like a good assumption. It didn't
use to be the case originally, since back then displaced stepping
was a new thing that wasn't on by default.

Post by Simon Marchi
@@ -1484,36 +1484,40 @@ displaced_step_closure::~displaced_step_closure () = default;
/* Per-inferior displaced stepping state. */
struct displaced_step_inferior_state
{
+ displaced_step_inferior_state (inferior *inf)
+ : inf (inf)
+ {}

explicit.

Post by Simon Marchi
+
+ if (it != displaced_step_inferior_states.end ())
+ displaced_step_inferior_states.erase (it);

I think this could be unordered_remove.

Thanks,
Pedro Alves

[Simon Marchi]

Post by Pedro Alves

Post by Simon Marchi
Commit
39a36629f68e ("Use std::forward_list for
displaced_step_inferior_states")
changed a hand-made linked list to use std::forward_list of pointers.
As suggested by David Blaikie, we might as well use values instead of
pointers. And instead of a list, we might as well use a vector. The
size of this list will always be at most the number of inferiors,
typically very small. And in any case the operation we do in the
hottest path (doing a displaced step) is iterate, and iterating on a
vector is always faster than a linked list.
A consequence of using a vector is that objects can be moved, when the
vector is resized. I don't think this is a problem, because we don't
save the address of the objects. In displaced_step_prepare_throw, we
save a pointer to the step_saved_copy field in a cleanup, but it is ran
or discarded immediately after.

Another alternative would be to put the displaced_step_inferior_state
object in struct inferior directly instead of keeping the objects
on the side. In practice, on x86 GNU/Linux at least, you end
up with an object per inferior anyway, assuming we actually
run the inferiors, which sounds like a good assumption. It didn't
use to be the case originally, since back then displaced stepping
was a new thing that wasn't on by default.

Ok, I was wondering about that too. I assumed that it was simply to
avoid stuffing too much random stuff in the inferior struct. I also
thought about how other files use a registry for things like this.

I did a quick test of having a pointer to displaced_step_inferior_state
in the inferior structure (the implementation of
displaced_step_inferior_state stays in infrun.c), it seems to work well.
Would you prefer that?

Post by Pedro Alves

Post by Simon Marchi
@@ -1484,36 +1484,40 @@
displaced_step_closure::~displaced_step_closure () = default;
/* Per-inferior displaced stepping state. */
struct displaced_step_inferior_state
{
+ displaced_step_inferior_state (inferior *inf)
+ : inf (inf)
+ {}

explicit.

Post by Simon Marchi
+
+ if (it != displaced_step_inferior_states.end ())
+ displaced_step_inferior_states.erase (it);

I think this could be unordered_remove.

Thanks, I'll fix those two if we end up merging this patch.

Simon

[Pedro Alves]

Post by Pedro Alves

Post by Simon Marchi
Commit
  39a36629f68e ("Use std::forward_list for displaced_step_inferior_states")
changed a hand-made linked list to use std::forward_list of pointers.
As suggested by David Blaikie, we might as well use values instead of
pointers.  And instead of a list, we might as well use a vector.  The
size of this list will always be at most the number of inferiors,
typically very small.  And in any case the operation we do in the
hottest path (doing a displaced step) is iterate, and iterating on a
vector is always faster than a linked list.
A consequence of using a vector is that objects can be moved, when the
vector is resized.  I don't think this is a problem, because we don't
save the address of the objects.  In displaced_step_prepare_throw, we
save a pointer to the step_saved_copy field in a cleanup, but it is ran
or discarded immediately after.

Another alternative would be to put the displaced_step_inferior_state
object in struct inferior directly instead of keeping the objects
on the side.  In practice, on x86 GNU/Linux at least, you end
up with an object per inferior anyway, assuming we actually
run the inferiors, which sounds like a good assumption.  It didn't
use to be the case originally, since back then displaced stepping
was a new thing that wasn't on by default.

Ok, I was wondering about that too.  I assumed that it was simply to avoid stuffing too much random stuff in the inferior struct.  I also thought about how other files use a registry for things like this.

Yeah, I think the original motivation for the registry is for when you
want dynamic registration, say because the resource in question is managed
by a source file that isn't always included in the build, like
some foocpu-tdep.c file.

For code that is always included in the build, I think that the
registry obfuscates more than it helps. E.g., it makes debugging
GDB harder. And it also doesn't have any benefit memory-wise.

I did a quick test of having a pointer to displaced_step_inferior_state in the inferior structure (the implementation of displaced_step_inferior_state stays in infrun.c), it seems to work well.  Would you prefer that?

I think that would be better, yeah. Either pointer or object (and moving the
struct to some header), both are fine with me.

Post by Pedro Alves

Post by Simon Marchi
@@ -1484,36 +1484,40 @@ displaced_step_closure::~displaced_step_closure () = default;
 /* Per-inferior displaced stepping state.  */
 struct displaced_step_inferior_state
 {
+  displaced_step_inferior_state (inferior *inf)
+    : inf (inf)
+  {}

explicit.

Post by Simon Marchi
+
+  if (it != displaced_step_inferior_states.end ())
+    displaced_step_inferior_states.erase (it);

I think this could be unordered_remove.

Thanks, I'll fix those two if we end up merging this patch.

Thanks,
Pedro Alves

[Simon Marchi]

Post by Pedro Alves

I did a quick test of having a pointer to displaced_step_inferior_state in the inferior structure (the implementation of displaced_step_inferior_state stays in infrun.c), it seems to work well.  Would you prefer that?

I think that would be better, yeah. Either pointer or object (and moving the
struct to some header), both are fine with me.

Dropping this patch in favor of:

https://sourceware.org/ml/gdb-patches/2018-11/msg0039