From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 47654 invoked by alias); 12 Sep 2017 00:01:22 -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 35912 invoked by uid 89); 12 Sep 2017 00:01:05 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,RP_MATCHES_RCVD,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy= X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 12 Sep 2017 00:01:03 +0000 Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 9D5C492E0D; Tue, 12 Sep 2017 00:00:54 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 9D5C492E0D Authentication-Results: ext-mx10.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx10.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=sergiodj@redhat.com Received: from localhost (unused-10-15-17-193.yyz.redhat.com [10.15.17.193]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 7455E61353; Tue, 12 Sep 2017 00:00:54 +0000 (UTC) From: Sergio Durigan Junior To: Simon Marchi Cc: Simon Marchi , gdb-patches@sourceware.org Subject: Re: [PATCH 1/2] Make probe_ops::get_probes fill an std::vector References: <1505053377-10061-1-git-send-email-simon.marchi@ericsson.com> <87efreo3jf.fsf@redhat.com> <0e57a6bff39936ce166f029a0a584530@polymtl.ca> Date: Tue, 12 Sep 2017 00:01:00 -0000 In-Reply-To: <0e57a6bff39936ce166f029a0a584530@polymtl.ca> (Simon Marchi's message of "Sun, 10 Sep 2017 19:59:18 +0200") Message-ID: <87y3pkiy4q.fsf@redhat.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/25.2 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-IsSubscribed: yes X-SW-Source: 2017-09/txt/msg00319.txt.bz2 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. -- Sergio GPG key ID: 237A 54B1 0287 28BF 00EF 31F4 D0EB 7628 65FC 5E36 Please send encrypted e-mail if possible http://sergiodj.net/