From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 9143 invoked by alias); 10 Sep 2017 17:59:28 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 8339 invoked by uid 89); 10 Sep 2017 17:59:27 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-24.8 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,RP_MATCHES_RCVD,SPF_HELO_PASS,SPF_PASS autolearn=ham version=3.3.2 spammy= X-HELO: smtp.polymtl.ca Received: from smtp.polymtl.ca (HELO smtp.polymtl.ca) (132.207.4.11) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Sun, 10 Sep 2017 17:59:26 +0000 Received: from simark.ca (simark.ca [158.69.221.121]) (authenticated bits=0) by smtp.polymtl.ca (8.14.7/8.14.7) with ESMTP id v8AHxJrC020741 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT) for ; Sun, 10 Sep 2017 13:59:24 -0400 Received: by simark.ca (Postfix, from userid 112) id 71BA21EAAC; Sun, 10 Sep 2017 13:59:19 -0400 (EDT) Received: from simark.ca (localhost [127.0.0.1]) by simark.ca (Postfix) with ESMTP id 61AE31EA17; Sun, 10 Sep 2017 13:59:18 -0400 (EDT) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII; format=flowed Content-Transfer-Encoding: 7bit Date: Sun, 10 Sep 2017 17:59:00 -0000 From: Simon Marchi To: Sergio Durigan Junior Cc: Simon Marchi , gdb-patches@sourceware.org Subject: Re: [PATCH 1/2] Make probe_ops::get_probes fill an std::vector In-Reply-To: <87efreo3jf.fsf@redhat.com> References: <1505053377-10061-1-git-send-email-simon.marchi@ericsson.com> <87efreo3jf.fsf@redhat.com> Message-ID: <0e57a6bff39936ce166f029a0a584530@polymtl.ca> X-Sender: simon.marchi@polymtl.ca User-Agent: Roundcube Webmail/1.3.0 X-Poly-FromMTA: (simark.ca [158.69.221.121]) at Sun, 10 Sep 2017 17:59:19 +0000 X-IsSubscribed: yes X-SW-Source: 2017-09/txt/msg00275.txt.bz2 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) : 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) 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 *probesp, > > Since you're doing this, what do you think about const-fying these > occurrences of "std::vector"? 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 &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