From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 39345 invoked by alias); 10 May 2017 05:10:07 -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 26390 invoked by uid 89); 10 May 2017 05:08:40 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-24.0 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,SPF_HELO_PASS,SPF_SOFTFAIL autolearn=ham version=3.3.2 spammy=held X-HELO: simark.ca Received: from simark.ca (HELO simark.ca) (158.69.221.121) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 10 May 2017 05:08:38 +0000 Received: by simark.ca (Postfix, from userid 33) id 2514C1E4A4; Wed, 10 May 2017 01:08:39 -0400 (EDT) To: Tim Wiederhake Subject: Re: [PATCH v3 12/12] btrace: Store function segments as objects. X-PHP-Originating-Script: 33:rcube.php MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII; format=flowed Content-Transfer-Encoding: 7bit Date: Wed, 10 May 2017 05:10:00 -0000 From: Simon Marchi Cc: gdb-patches@sourceware.org, markus.t.metzger@intel.com In-Reply-To: <1494312929-22749-13-git-send-email-tim.wiederhake@intel.com> References: <1494312929-22749-1-git-send-email-tim.wiederhake@intel.com> <1494312929-22749-13-git-send-email-tim.wiederhake@intel.com> Message-ID: <03bcd302ae8b132f5d2209c3e40cf494@polymtl.ca> X-Sender: simon.marchi@polymtl.ca User-Agent: Roundcube Webmail/1.2.5 X-IsSubscribed: yes X-SW-Source: 2017-05/txt/msg00242.txt.bz2 On 2017-05-09 02:55, Tim Wiederhake wrote: > 2017-05-09 Tim Wiederhake > > gdb/ChangeLog: > * btrace.c: > * btrace.h: > * record-btrace.c: IWBN if you could be more explicit :). > > --- > gdb/btrace.c | 94 > ++++++++++++++++++++++++++--------------------------- > gdb/btrace.h | 7 ++-- > gdb/record-btrace.c | 10 +++--- > 3 files changed, 56 insertions(+), 55 deletions(-) > > diff --git a/gdb/btrace.c b/gdb/btrace.c > index 7b82000..4c7020d 100644 > --- a/gdb/btrace.c > +++ b/gdb/btrace.c > @@ -155,14 +155,27 @@ ftrace_call_num_insn (const struct > btrace_function* bfun) > /* Return the function segment with the given NUMBER or NULL if no > such segment > exists. BTINFO is the branch trace information for the current > thread. */ > > -static struct btrace_function * > +const static struct btrace_function * It would make more sense to put the "static" first. > ftrace_find_call_by_number (const struct btrace_thread_info *btinfo, > unsigned int number) > { > if (number == 0 || number > btinfo->functions.size ()) > return NULL; > > - return btinfo->functions[number - 1]; > + return &btinfo->functions[number - 1]; > +} > + > +/* Return the function segment with the given NUMBER or NULL if no > such segment > + exists. BTINFO is the branch trace information for the current > thread. */ It took me a surprisingly high amount of seconds to understand that this was a const version of the function below. To avoid reapeating the comment and to make it clear it's the same thing, you can replace the comment of the const version to something like: /* A const version of the function above. */ > + > +static struct btrace_function * > +ftrace_find_call_by_number (struct btrace_thread_info *btinfo, > + unsigned int number) > +{ > + if (number == 0 || number > btinfo->functions.size ()) > + return NULL; > + > + return &btinfo->functions[number - 1]; > } > > /* Return non-zero if BFUN does not match MFUN and FUN, > @@ -214,37 +227,33 @@ ftrace_function_switched (const struct > btrace_function *bfun, > /* Allocate and initialize a new branch trace function segment at the > end of > the trace. > BTINFO is the branch trace information for the current thread. > - MFUN and FUN are the symbol information we have for this function. > */ > + MFUN and FUN are the symbol information we have for this function. > + This invalidates all struct btrace_function pointer currently held. > */ > > static struct btrace_function * > ftrace_new_function (struct btrace_thread_info *btinfo, > struct minimal_symbol *mfun, > struct symbol *fun) > { > - struct btrace_function *bfun; > - > - bfun = XCNEW (struct btrace_function); > - > - bfun->msym = mfun; > - bfun->sym = fun; > + struct btrace_function bfun {mfun, fun, 0, 0, 0, NULL, 0, 0, 0, 0, > 0}; I think it would be much better to add a simple constructor to btrace_function. For the fields that should simply be zero'ed, you can initialize fields directly, like we do in many other places (e.g. class inferior). > > if (btinfo->functions.empty ()) > { > /* Start counting at one. */ > - bfun->number = 1; > - bfun->insn_offset = 1; > + bfun.number = 1; > + bfun.insn_offset = 1; > } > else > { > - struct btrace_function *prev = btinfo->functions.back (); > + struct btrace_function *prev = &btinfo->functions.back (); > > - bfun->number = prev->number + 1; > - bfun->insn_offset = prev->insn_offset + ftrace_call_num_insn > (prev); > - bfun->level = prev->level; > + bfun.number = prev->number + 1; > + bfun.insn_offset = prev->insn_offset + ftrace_call_num_insn > (prev); > + bfun.level = prev->level; > } > > - btinfo->functions.push_back (bfun); > - return bfun; > + btinfo->functions.push_back (std::move (bfun)); > + return &btinfo->functions.back (); I could be mistaken, but I don't think the move is very useful here, since all fields of btrace_function are trivial (?). You could use emplace_back instead: btinfo->functions.emplace_back (mfun, fun); btrace_function &bfun = btinfo->functions.back (); ... return &bfun; or unsigned int number, insn_offset; unsigned int insn_offset = prev->insn_offset + ftrace_call_num_insn (prev); int level = prev->level; if (btinfo->functions.empty ()) { /* Start counting at one. */ number = 1; insn_offset = 1; level = 0; } else { struct btrace_function *prev = &btinfo->functions.back (); number = prev->number + 1; insn_offset = prev->insn_offset + ftrace_call_num_insn (prev); level = prev->level; } btinfo->functions.emplace_back (mfun, fun, number, insn_offset, level); return &btinfo->functions.back (); Thanks, Simon