[1/2] Make probe_ops::get_probes fill an std::vector
Commit Message
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.
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(-)
Comments
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 *>"?
> struct dtrace_dof_hdr *dof,
> struct dtrace_dof_probe *probe,
> struct dtrace_dof_provider *provider,
> @@ -448,7 +449,7 @@ dtrace_process_dof_probe (struct objfile *objfile,
> ret->enablers = VEC_copy (dtrace_probe_enabler_s, enablers);
>
> /* Successfully created probe. */
> - VEC_safe_push (probe_p, *probesp, (struct probe *) ret);
> + probesp->push_back ((struct probe *) ret);
> }
>
> do_cleanups (cleanup);
> @@ -461,7 +462,7 @@ dtrace_process_dof_probe (struct objfile *objfile,
>
> static void
> dtrace_process_dof (asection *sect, struct objfile *objfile,
> - VEC (probe_p) **probesp, struct dtrace_dof_hdr *dof)
> + std::vector<probe *> *probesp, struct dtrace_dof_hdr *dof)
> {
> struct gdbarch *gdbarch = get_objfile_arch (objfile);
> struct dtrace_dof_sect *section;
> @@ -620,7 +621,7 @@ dtrace_get_arg (struct dtrace_probe *probe, unsigned n,
> /* Implementation of the get_probes method. */
>
> static void
> -dtrace_get_probes (VEC (probe_p) **probesp, struct objfile *objfile)
> +dtrace_get_probes (std::vector<probe *> *probesp, struct objfile *objfile)
> {
> bfd *abfd = objfile->obfd;
> asection *sect = NULL;
> diff --git a/gdb/elfread.c b/gdb/elfread.c
> index f3d4641..8a64865 100644
> --- a/gdb/elfread.c
> +++ b/gdb/elfread.c
> @@ -1309,35 +1309,30 @@ elf_symfile_init (struct objfile *objfile)
>
> /* Implementation of `sym_get_probes', as documented in symfile.h. */
>
> -static VEC (probe_p) *
> +static const std::vector<probe *> &
> elf_get_probes (struct objfile *objfile)
> {
> - VEC (probe_p) *probes_per_bfd;
> + std::vector<probe *> *probes_per_bfd;
>
> /* Have we parsed this objfile's probes already? */
> - probes_per_bfd = (VEC (probe_p) *) bfd_data (objfile->obfd, probe_key);
> + probes_per_bfd = (std::vector<probe *> *) bfd_data (objfile->obfd, probe_key);
>
> - if (!probes_per_bfd)
> + if (probes_per_bfd == NULL)
> {
> int ix;
> const struct probe_ops *probe_ops;
> + probes_per_bfd = new std::vector<probe *>;
>
> /* Here we try to gather information about all types of probes from the
> objfile. */
> for (ix = 0; VEC_iterate (probe_ops_cp, all_probe_ops, ix, probe_ops);
> ix++)
> - probe_ops->get_probes (&probes_per_bfd, objfile);
> -
> - if (probes_per_bfd == NULL)
> - {
> - VEC_reserve (probe_p, probes_per_bfd, 1);
> - gdb_assert (probes_per_bfd != NULL);
> - }
> + probe_ops->get_probes (probes_per_bfd, objfile);
>
> set_bfd_data (objfile->obfd, probe_key, probes_per_bfd);
> }
>
> - return probes_per_bfd;
> + return *probes_per_bfd;
> }
>
> /* Helper function used to free the space allocated for storing SystemTap
> @@ -1346,14 +1341,12 @@ elf_get_probes (struct objfile *objfile)
> static void
> probe_key_free (bfd *abfd, void *d)
> {
> - int ix;
> - VEC (probe_p) *probes = (VEC (probe_p) *) d;
> - struct probe *probe;
> + std::vector<probe *> *probes = (std::vector<probe *> *) d;
>
> - for (ix = 0; VEC_iterate (probe_p, probes, ix, probe); ix++)
> + for (struct probe *probe : *probes)
> probe->pops->destroy (probe);
>
> - VEC_free (probe_p, probes);
> + delete probes;
> }
>
>
> diff --git a/gdb/probe.c b/gdb/probe.c
> index 686e90e..2d68437 100644
> --- a/gdb/probe.c
> +++ b/gdb/probe.c
> @@ -58,10 +58,6 @@ parse_probes_in_pspace (const struct probe_ops *probe_ops,
>
> ALL_PSPACE_OBJFILES (search_pspace, objfile)
> {
> - VEC (probe_p) *probes;
> - struct probe *probe;
> - int ix;
> -
> if (!objfile->sf || !objfile->sf->sym_probe_fns)
> continue;
>
> @@ -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?
> {
> - VEC (probe_p) *probes, *result = NULL;
> - int ix;
> - struct probe *probe;
> + VEC (probe_p) *result = NULL;
>
> if (!objfile->sf || !objfile->sf->sym_probe_fns)
> return NULL;
>
> - probes = objfile->sf->sym_probe_fns->sym_get_probes (objfile);
> - for (ix = 0; VEC_iterate (probe_p, probes, ix, probe); ix++)
> + const std::vector<probe *> &probes
> + = objfile->sf->sym_probe_fns->sym_get_probes (objfile);
> + for (struct probe *probe : probes)
> {
> if (strcmp (probe->provider, provider) != 0)
> continue;
> @@ -246,17 +242,14 @@ find_probe_by_pc (CORE_ADDR pc)
>
> ALL_OBJFILES (objfile)
> {
> - VEC (probe_p) *probes;
> - int ix;
> - struct probe *probe;
> -
> if (!objfile->sf || !objfile->sf->sym_probe_fns
> || objfile->sect_index_text == -1)
> continue;
>
> /* If this proves too inefficient, we can replace with a hash. */
> - probes = objfile->sf->sym_probe_fns->sym_get_probes (objfile);
> - for (ix = 0; VEC_iterate (probe_p, probes, ix, probe); ix++)
> + const std::vector<probe *> &probes
> + = objfile->sf->sym_probe_fns->sym_get_probes (objfile);
> + for (struct probe *probe : probes)
> if (get_probe_address (probe, objfile) == pc)
> {
> result.objfile = objfile;
> @@ -294,10 +287,6 @@ collect_probes (char *objname, char *provider, char *probe_name,
>
> ALL_OBJFILES (objfile)
> {
> - VEC (probe_p) *probes;
> - struct probe *probe;
> - int ix;
> -
> if (! objfile->sf || ! objfile->sf->sym_probe_fns)
> continue;
>
> @@ -307,9 +296,10 @@ collect_probes (char *objname, char *provider, char *probe_name,
> 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)
> {
> struct bound_probe bound;
>
> @@ -907,7 +897,7 @@ probe_any_is_linespec (const char **linespecp)
> /* Dummy method used for `probe_ops_any'. */
>
> static void
> -probe_any_get_probes (VEC (probe_p) **probesp, struct objfile *objfile)
> +probe_any_get_probes (std::vector<probe *> *probesp, struct objfile *objfile)
> {
> /* No probes can be provided by this dummy backend. */
> }
> diff --git a/gdb/probe.h b/gdb/probe.h
> index db7f1d1..61e3031 100644
> --- a/gdb/probe.h
> +++ b/gdb/probe.h
> @@ -64,7 +64,7 @@ struct probe_ops
>
> /* Function that should fill PROBES with known probes from OBJFILE. */
>
> - void (*get_probes) (VEC (probe_p) **probes, struct objfile *objfile);
> + void (*get_probes) (std::vector<probe *> *probes, struct objfile *objfile);
>
> /* Compute the probe's relocated address. OBJFILE is the objfile
> in which the probe originated. */
> diff --git a/gdb/stap-probe.c b/gdb/stap-probe.c
> index c0cc662..032f796 100644
> --- a/gdb/stap-probe.c
> +++ b/gdb/stap-probe.c
> @@ -1472,7 +1472,7 @@ stap_clear_semaphore (struct probe *probe_generic, struct objfile *objfile,
>
> static void
> handle_stap_probe (struct objfile *objfile, struct sdt_note *el,
> - VEC (probe_p) **probesp, CORE_ADDR base)
> + std::vector<probe *> *probesp, CORE_ADDR base)
> {
> bfd *abfd = objfile->obfd;
> int size = bfd_get_arch_size (abfd) / 8;
> @@ -1543,7 +1543,7 @@ handle_stap_probe (struct objfile *objfile, struct sdt_note *el,
> ret->args_u.text = probe_args;
>
> /* Successfully created probe. */
> - VEC_safe_push (probe_p, *probesp, (struct probe *) ret);
> + probesp->push_back ((struct probe *) ret);
> }
>
> /* Helper function which tries to find the base address of the SystemTap
> @@ -1588,7 +1588,7 @@ get_stap_base_address (bfd *obfd, bfd_vma *base)
> SystemTap probes from OBJFILE. */
>
> static void
> -stap_get_probes (VEC (probe_p) **probesp, struct objfile *objfile)
> +stap_get_probes (std::vector<probe *> *probesp, struct objfile *objfile)
> {
> /* If we are here, then this is the first time we are parsing the
> SystemTap probe's information. We basically have to count how many
> @@ -1597,7 +1597,7 @@ stap_get_probes (VEC (probe_p) **probesp, struct objfile *objfile)
> bfd *obfd = objfile->obfd;
> bfd_vma base;
> struct sdt_note *iter;
> - unsigned save_probesp_len = VEC_length (probe_p, *probesp);
> + unsigned save_probesp_len = probesp->size ();
>
> if (objfile->separate_debug_objfile_backlink != NULL)
> {
> @@ -1628,7 +1628,7 @@ stap_get_probes (VEC (probe_p) **probesp, struct objfile *objfile)
> handle_stap_probe (objfile, iter, probesp, base);
> }
>
> - if (save_probesp_len == VEC_length (probe_p, *probesp))
> + if (save_probesp_len == probesp->size ())
> {
> /* If we are here, it means we have failed to parse every known
> probe. */
> diff --git a/gdb/symfile-debug.c b/gdb/symfile-debug.c
> index e7890c9..32dafa8 100644
> --- a/gdb/symfile-debug.c
> +++ b/gdb/symfile-debug.c
> @@ -384,20 +384,20 @@ static const struct quick_symbol_functions debug_sym_quick_functions =
>
> /* Debugging version of struct sym_probe_fns. */
>
> -static VEC (probe_p) *
> +static const std::vector<probe *> &
> debug_sym_get_probes (struct objfile *objfile)
> {
> const struct debug_sym_fns_data *debug_data
> = ((const struct debug_sym_fns_data *)
> objfile_data (objfile, symfile_debug_objfile_data_key));
> - VEC (probe_p) *retval;
>
> - retval = debug_data->real_sf->sym_probe_fns->sym_get_probes (objfile);
> + const std::vector<probe *> &retval
> + = debug_data->real_sf->sym_probe_fns->sym_get_probes (objfile);
>
> fprintf_filtered (gdb_stdlog,
> "probes->sym_get_probes (%s) = %s\n",
> objfile_debug_name (objfile),
> - host_address_to_string (retval));
> + host_address_to_string (retval.data ()));
>
> return retval;
> }
> diff --git a/gdb/symfile.h b/gdb/symfile.h
> index bb47fdf..1f4460c 100644
> --- a/gdb/symfile.h
> +++ b/gdb/symfile.h
> @@ -314,11 +314,8 @@ struct quick_symbol_functions
>
> struct sym_probe_fns
> {
> - /* If non-NULL, return an array of probe objects.
> -
> - The returned value does not have to be freed and it has lifetime of the
> - OBJFILE. */
> - VEC (probe_p) *(*sym_get_probes) (struct objfile *);
> + /* If non-NULL, return a reference to vector of probe objects. */
> + const std::vector<probe *> &(*sym_get_probes) (struct objfile *);
> };
>
> /* Structure to keep track of symbol reading functions for various
> --
> 2.7.4
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 :-).
Thanks!
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
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.
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
@@ -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,
struct dtrace_dof_hdr *dof,
struct dtrace_dof_probe *probe,
struct dtrace_dof_provider *provider,
@@ -448,7 +449,7 @@ dtrace_process_dof_probe (struct objfile *objfile,
ret->enablers = VEC_copy (dtrace_probe_enabler_s, enablers);
/* Successfully created probe. */
- VEC_safe_push (probe_p, *probesp, (struct probe *) ret);
+ probesp->push_back ((struct probe *) ret);
}
do_cleanups (cleanup);
@@ -461,7 +462,7 @@ dtrace_process_dof_probe (struct objfile *objfile,
static void
dtrace_process_dof (asection *sect, struct objfile *objfile,
- VEC (probe_p) **probesp, struct dtrace_dof_hdr *dof)
+ std::vector<probe *> *probesp, struct dtrace_dof_hdr *dof)
{
struct gdbarch *gdbarch = get_objfile_arch (objfile);
struct dtrace_dof_sect *section;
@@ -620,7 +621,7 @@ dtrace_get_arg (struct dtrace_probe *probe, unsigned n,
/* Implementation of the get_probes method. */
static void
-dtrace_get_probes (VEC (probe_p) **probesp, struct objfile *objfile)
+dtrace_get_probes (std::vector<probe *> *probesp, struct objfile *objfile)
{
bfd *abfd = objfile->obfd;
asection *sect = NULL;
@@ -1309,35 +1309,30 @@ elf_symfile_init (struct objfile *objfile)
/* Implementation of `sym_get_probes', as documented in symfile.h. */
-static VEC (probe_p) *
+static const std::vector<probe *> &
elf_get_probes (struct objfile *objfile)
{
- VEC (probe_p) *probes_per_bfd;
+ std::vector<probe *> *probes_per_bfd;
/* Have we parsed this objfile's probes already? */
- probes_per_bfd = (VEC (probe_p) *) bfd_data (objfile->obfd, probe_key);
+ probes_per_bfd = (std::vector<probe *> *) bfd_data (objfile->obfd, probe_key);
- if (!probes_per_bfd)
+ if (probes_per_bfd == NULL)
{
int ix;
const struct probe_ops *probe_ops;
+ probes_per_bfd = new std::vector<probe *>;
/* Here we try to gather information about all types of probes from the
objfile. */
for (ix = 0; VEC_iterate (probe_ops_cp, all_probe_ops, ix, probe_ops);
ix++)
- probe_ops->get_probes (&probes_per_bfd, objfile);
-
- if (probes_per_bfd == NULL)
- {
- VEC_reserve (probe_p, probes_per_bfd, 1);
- gdb_assert (probes_per_bfd != NULL);
- }
+ probe_ops->get_probes (probes_per_bfd, objfile);
set_bfd_data (objfile->obfd, probe_key, probes_per_bfd);
}
- return probes_per_bfd;
+ return *probes_per_bfd;
}
/* Helper function used to free the space allocated for storing SystemTap
@@ -1346,14 +1341,12 @@ elf_get_probes (struct objfile *objfile)
static void
probe_key_free (bfd *abfd, void *d)
{
- int ix;
- VEC (probe_p) *probes = (VEC (probe_p) *) d;
- struct probe *probe;
+ std::vector<probe *> *probes = (std::vector<probe *> *) d;
- for (ix = 0; VEC_iterate (probe_p, probes, ix, probe); ix++)
+ for (struct probe *probe : *probes)
probe->pops->destroy (probe);
- VEC_free (probe_p, probes);
+ delete probes;
}
@@ -58,10 +58,6 @@ parse_probes_in_pspace (const struct probe_ops *probe_ops,
ALL_PSPACE_OBJFILES (search_pspace, objfile)
{
- VEC (probe_p) *probes;
- struct probe *probe;
- int ix;
-
if (!objfile->sf || !objfile->sf->sym_probe_fns)
continue;
@@ -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)
{
- VEC (probe_p) *probes, *result = NULL;
- int ix;
- struct probe *probe;
+ VEC (probe_p) *result = NULL;
if (!objfile->sf || !objfile->sf->sym_probe_fns)
return NULL;
- probes = objfile->sf->sym_probe_fns->sym_get_probes (objfile);
- for (ix = 0; VEC_iterate (probe_p, probes, ix, probe); ix++)
+ const std::vector<probe *> &probes
+ = objfile->sf->sym_probe_fns->sym_get_probes (objfile);
+ for (struct probe *probe : probes)
{
if (strcmp (probe->provider, provider) != 0)
continue;
@@ -246,17 +242,14 @@ find_probe_by_pc (CORE_ADDR pc)
ALL_OBJFILES (objfile)
{
- VEC (probe_p) *probes;
- int ix;
- struct probe *probe;
-
if (!objfile->sf || !objfile->sf->sym_probe_fns
|| objfile->sect_index_text == -1)
continue;
/* If this proves too inefficient, we can replace with a hash. */
- probes = objfile->sf->sym_probe_fns->sym_get_probes (objfile);
- for (ix = 0; VEC_iterate (probe_p, probes, ix, probe); ix++)
+ const std::vector<probe *> &probes
+ = objfile->sf->sym_probe_fns->sym_get_probes (objfile);
+ for (struct probe *probe : probes)
if (get_probe_address (probe, objfile) == pc)
{
result.objfile = objfile;
@@ -294,10 +287,6 @@ collect_probes (char *objname, char *provider, char *probe_name,
ALL_OBJFILES (objfile)
{
- VEC (probe_p) *probes;
- struct probe *probe;
- int ix;
-
if (! objfile->sf || ! objfile->sf->sym_probe_fns)
continue;
@@ -307,9 +296,10 @@ collect_probes (char *objname, char *provider, char *probe_name,
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)
{
struct bound_probe bound;
@@ -907,7 +897,7 @@ probe_any_is_linespec (const char **linespecp)
/* Dummy method used for `probe_ops_any'. */
static void
-probe_any_get_probes (VEC (probe_p) **probesp, struct objfile *objfile)
+probe_any_get_probes (std::vector<probe *> *probesp, struct objfile *objfile)
{
/* No probes can be provided by this dummy backend. */
}
@@ -64,7 +64,7 @@ struct probe_ops
/* Function that should fill PROBES with known probes from OBJFILE. */
- void (*get_probes) (VEC (probe_p) **probes, struct objfile *objfile);
+ void (*get_probes) (std::vector<probe *> *probes, struct objfile *objfile);
/* Compute the probe's relocated address. OBJFILE is the objfile
in which the probe originated. */
@@ -1472,7 +1472,7 @@ stap_clear_semaphore (struct probe *probe_generic, struct objfile *objfile,
static void
handle_stap_probe (struct objfile *objfile, struct sdt_note *el,
- VEC (probe_p) **probesp, CORE_ADDR base)
+ std::vector<probe *> *probesp, CORE_ADDR base)
{
bfd *abfd = objfile->obfd;
int size = bfd_get_arch_size (abfd) / 8;
@@ -1543,7 +1543,7 @@ handle_stap_probe (struct objfile *objfile, struct sdt_note *el,
ret->args_u.text = probe_args;
/* Successfully created probe. */
- VEC_safe_push (probe_p, *probesp, (struct probe *) ret);
+ probesp->push_back ((struct probe *) ret);
}
/* Helper function which tries to find the base address of the SystemTap
@@ -1588,7 +1588,7 @@ get_stap_base_address (bfd *obfd, bfd_vma *base)
SystemTap probes from OBJFILE. */
static void
-stap_get_probes (VEC (probe_p) **probesp, struct objfile *objfile)
+stap_get_probes (std::vector<probe *> *probesp, struct objfile *objfile)
{
/* If we are here, then this is the first time we are parsing the
SystemTap probe's information. We basically have to count how many
@@ -1597,7 +1597,7 @@ stap_get_probes (VEC (probe_p) **probesp, struct objfile *objfile)
bfd *obfd = objfile->obfd;
bfd_vma base;
struct sdt_note *iter;
- unsigned save_probesp_len = VEC_length (probe_p, *probesp);
+ unsigned save_probesp_len = probesp->size ();
if (objfile->separate_debug_objfile_backlink != NULL)
{
@@ -1628,7 +1628,7 @@ stap_get_probes (VEC (probe_p) **probesp, struct objfile *objfile)
handle_stap_probe (objfile, iter, probesp, base);
}
- if (save_probesp_len == VEC_length (probe_p, *probesp))
+ if (save_probesp_len == probesp->size ())
{
/* If we are here, it means we have failed to parse every known
probe. */
@@ -384,20 +384,20 @@ static const struct quick_symbol_functions debug_sym_quick_functions =
/* Debugging version of struct sym_probe_fns. */
-static VEC (probe_p) *
+static const std::vector<probe *> &
debug_sym_get_probes (struct objfile *objfile)
{
const struct debug_sym_fns_data *debug_data
= ((const struct debug_sym_fns_data *)
objfile_data (objfile, symfile_debug_objfile_data_key));
- VEC (probe_p) *retval;
- retval = debug_data->real_sf->sym_probe_fns->sym_get_probes (objfile);
+ const std::vector<probe *> &retval
+ = debug_data->real_sf->sym_probe_fns->sym_get_probes (objfile);
fprintf_filtered (gdb_stdlog,
"probes->sym_get_probes (%s) = %s\n",
objfile_debug_name (objfile),
- host_address_to_string (retval));
+ host_address_to_string (retval.data ()));
return retval;
}
@@ -314,11 +314,8 @@ struct quick_symbol_functions
struct sym_probe_fns
{
- /* If non-NULL, return an array of probe objects.
-
- The returned value does not have to be freed and it has lifetime of the
- OBJFILE. */
- VEC (probe_p) *(*sym_get_probes) (struct objfile *);
+ /* If non-NULL, return a reference to vector of probe objects. */
+ const std::vector<probe *> &(*sym_get_probes) (struct objfile *);
};
/* Structure to keep track of symbol reading functions for various