Re: [PATCH] Use std::vector for displaced_step_inferior_states

[Simon Marchi <simon dot marchi at polymtl dot ca>]

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