Re: [PATCH 1/2] Make probe_ops::get_probes fill an std::vector

Simon Marchi
2017-09-12T11:57:42Z

On 2017-09-12 02:00, Sergio Durigan Junior wrote:
> On Sunday, September 10 2017, Simon Marchi wrote:
>
>> On 2017-09-10 19:40, Sergio Durigan Junior wrote:
>>> On Sunday, September 10 2017, Simon Marchi wrote:
>>>
>>>> This patch changes one usage of VEC to std::vector. It is a
>>>> relatively
>>>> straightforward 1:1 change. The implementations of
>>>> sym_probe_fns::sym_get_probes return a borrowed reference to their
>>>> probe
>>>> vectors, meaning that the caller should not free it. In the new
>>>> code, I
>>>> made them return a const reference to the vector.
>>>>
>>>> This patch and the following one were tested by the buildbot. I
>>>> didn't
>>>> see any failures that looked related to this one.
>>>
>>> Thanks for the patch, Simon. A few comments below.
>>>
>>>> gdb/ChangeLog:
>>>>
>>>> * probe.h (struct probe_ops) <get_probes>: Change parameter from
>>>> vec to std::vector.
>>>> * probe.c (parse_probes_in_pspace): Update.
>>>> (find_probes_in_objfile): Update.
>>>> (find_probe_by_pc): Update.
>>>> (collect_probes): Update.
>>>> (probe_any_get_probes): Update.
>>>> * symfile.h (struct sym_probe_fns) <sym_get_probes> Change
>>>> return type to reference to std::vector.
>>>> * dtrace-probe.c (dtrace_process_dof_probe): Change parameter to
>>>> std::vector and update.
>>>> (dtrace_process_dof): Likewise.
>>>> (dtrace_get_probes): Likewise.
>>>> * elfread.c (elf_get_probes): Change return type to std::vector,
>>>> store an std::vector in bfd_data.
>>>> (probe_key_free): Update to std::vector.
>>>> * stap-probe.c (handle_stap_probe): Change parameter to
>>>> std::vector and update.
>>>> (stap_get_probes): Likewise.
>>>> * symfile-debug.c (debug_sym_get_probes): Change return type to
>>>> std::vector and update.
>>>> ---
>>>> gdb/dtrace-probe.c | 9 +++++----
>>>> gdb/elfread.c | 27 ++++++++++-----------------
>>>> gdb/probe.c | 38 ++++++++++++++------------------------
>>>> gdb/probe.h | 2 +-
>>>> gdb/stap-probe.c | 10 +++++-----
>>>> gdb/symfile-debug.c | 8 ++++----
>>>> gdb/symfile.h | 7 ++-----
>>>> 7 files changed, 41 insertions(+), 60 deletions(-)
>>>>
>>>> diff --git a/gdb/dtrace-probe.c b/gdb/dtrace-probe.c
>>>> index c1a8cb0..f9209ec 100644
>>>> --- a/gdb/dtrace-probe.c
>>>> +++ b/gdb/dtrace-probe.c
>>>> @@ -313,7 +313,8 @@ struct dtrace_dof_probe
>>>>
>>>> static void
>>>> dtrace_process_dof_probe (struct objfile *objfile,
>>>> - struct gdbarch *gdbarch, VEC (probe_p) **probesp,
>>>> + struct gdbarch *gdbarch,
>>>> + std::vector<probe *> *probesp,
>>>
>>> Since you're doing this, what do you think about const-fying these
>>> occurrences of "std::vector<probe *>"?
>>
>> The job of this function is actually to modify (append to) the vector,
>> so I don't think it would make sense to make the vector const. Or did
>> I misunderstand what you meant?
>
> What I was proposing was to make the pointer to the vector constant.
> But you're right, I've read more about it and it makes sense to leave
> it
> as is. Thanks.
>
>>>> @@ -71,9 +67,10 @@ parse_probes_in_pspace (const struct probe_ops
>>>> *probe_ops,
>>>> objfile_namestr) != 0)
>>>> continue;
>>>>
>>>> - probes = objfile->sf->sym_probe_fns->sym_get_probes
>>>> (objfile);
>>>> + const std::vector<probe *> &probes
>>>> + = objfile->sf->sym_probe_fns->sym_get_probes (objfile);
>>>>
>>>> - for (ix = 0; VEC_iterate (probe_p, probes, ix, probe); ix++)
>>>> + for (struct probe *probe : probes)
>>>> {
>>>> if (probe_ops != &probe_ops_any && probe->pops != probe_ops)
>>>> continue;
>>>> @@ -211,15 +208,14 @@ VEC (probe_p) *
>>>> find_probes_in_objfile (struct objfile *objfile, const char
>>>> *provider,
>>>> const char *name)
>>>
>>> Any reason for not converting this function's return as well?
>>
>> Yes: the return value of this function ends up assigned to, for
>> example, breakpoint_objfile_data::longjmp_probes. It would make sense
>> to convert that longjmp_probes to the same std::vector type. However,
>> we then have to look at how breakpoint_objfile_data is allocated.
>> It's currently allocated with XOBNEW on the objfile obstack, so I'm
>> not too sure how to handle this (I haven't really looked into it).
>> Since it was starting to be a bit too far-reaching, I preferred to
>> take it as a separate step.
>
> Hm, I see.
>
>> But now that we're talking about it, do you know what's the advantage
>> of using obstacks? It looks like we can just change that XOBNEW with
>> a new, and put the corresponding delete in free_breakpoint_probes
>> (which could probably get renamed to free_breakpoint_objfile_data).
>> Is there something I'm missing here?
>
> It doesn't seem there is any clear/direct advantage. The commit that
> added this specific code was discussed here:
>
> https://sourceware.org/ml/gdb-patches/2011-02/msg00054.html
>
> But there isn't any mention about the use of obstacks. I'd say you can
> go ahead and replace it new/delete.
>
>>> I know this patch is supposed to touch only probe_ops::get_probes,
>>> but
>>> I'd love to see the whole VEC (probe_p) replaced (a few places on
>>> breakpoint.c are also using it). Well, maybe on another patch :-).
>>
>> Indeed, I'll look into it as a follow-up.
>
> Thanks, this is fine by me then.

Thanks, both patches are now pushed.

Simon