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

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

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?

>> @@ -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.

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?

> 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 for reviewing.

Simon