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

[Simon Marchi <simon.marchi@polymtl.ca>]

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