diff mbox

Use std::vector for displaced_step_inferior_states

Message ID 20181122031229.15621-1-simon.marchi@ericsson.com
State New
Headers show

Commit Message

Simon Marchi Nov. 22, 2018, 3:12 a.m. UTC
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

Pedro Alves Nov. 22, 2018, 3:32 p.m. UTC | #1
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
Simon Marchi Nov. 22, 2018, 5:05 p.m. UTC | #2
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
Pedro Alves Nov. 22, 2018, 5:17 p.m. UTC | #3
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
Simon Marchi Nov. 23, 2018, 6:26 p.m. UTC | #4
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
diff mbox

Patch

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