This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH] Use std::vector for displaced_step_inferior_states
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