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

[Sergio Durigan Junior <sergiodj at redhat dot com>]

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
http://sergiodj.net/