Use std::vector for displaced_step_inferior_states
Commit Message
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(-)
Comments
On 11/22/2018 03:12 AM, Simon Marchi wrote:
> 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.
> @@ -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.
> +
> + if (it != displaced_step_inferior_states.end ())
> + displaced_step_inferior_states.erase (it);
I think this could be unordered_remove.
Thanks,
Pedro Alves
On 2018-11-22 10:32, Pedro Alves wrote:
> On 11/22/2018 03:12 AM, Simon Marchi wrote:
>> 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?
>> @@ -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.
>
>> +
>> + 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
On 11/22/2018 05:05 PM, Simon Marchi wrote:
> On 2018-11-22 10:32, Pedro Alves wrote:
>> On 11/22/2018 03:12 AM, Simon Marchi wrote:
>>> 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.
>>> @@ -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.
>>
>>> +
>>> + 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
On 2018-11-22 12:17 p.m., Pedro Alves wrote:
>> 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/msg00390.html
Simon
@@ -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