From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 24137 invoked by alias); 12 Sep 2017 11:57:42 -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 24127 invoked by uid 89); 12 Sep 2017 11:57:42 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-24.9 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; Tue, 12 Sep 2017 11:57:40 +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 v8CBvXhY014408 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT) for ; Tue, 12 Sep 2017 07:57:38 -0400 Received: by simark.ca (Postfix, from userid 112) id 8E15F1ECDF; Tue, 12 Sep 2017 07:57:33 -0400 (EDT) Received: from simark.ca (localhost [127.0.0.1]) by simark.ca (Postfix) with ESMTP id C73A51E988; Tue, 12 Sep 2017 07:57:12 -0400 (EDT) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII; format=flowed Content-Transfer-Encoding: 7bit Date: Tue, 12 Sep 2017 11:57: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: <87y3pkiy4q.fsf@redhat.com> References: <1505053377-10061-1-git-send-email-simon.marchi@ericsson.com> <87efreo3jf.fsf@redhat.com> <0e57a6bff39936ce166f029a0a584530@polymtl.ca> <87y3pkiy4q.fsf@redhat.com> Message-ID: <7a04e6812f52be66018e278ae202347e@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 Tue, 12 Sep 2017 11:57:33 +0000 X-IsSubscribed: yes X-SW-Source: 2017-09/txt/msg00333.txt.bz2 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) : 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? > > 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 &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