This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]