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

Sergio Durigan Junior
2017-09-12T00:01:22Z

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.

--
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF 31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible