From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 31523 invoked by alias); 30 Nov 2016 23:34:16 -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 31350 invoked by uid 89); 30 Nov 2016 23:33:53 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-4.8 required=5.0 tests=BAYES_00,RCVD_IN_DNSWL_NONE,RP_MATCHES_RCVD,SPF_PASS autolearn=ham version=3.3.2 spammy=H*M:sk:94eb2c1, H*MI:sk:94eb2c1, watched, gap X-HELO: mail-pg0-f74.google.com Received: from mail-pg0-f74.google.com (HELO mail-pg0-f74.google.com) (74.125.83.74) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 30 Nov 2016 23:33:43 +0000 Received: by mail-pg0-f74.google.com with SMTP id p66so18451193pga.0 for ; Wed, 30 Nov 2016 15:33:43 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:message-id:date:subject:from:to:cc; bh=+Qzw8c3khGvmFVdzLO6uErO4suy8ST5ABghpxBea2fU=; b=g6GJX3/Oomi3jc9PFFcJKSZzX3suS2Wmp1de4+VKPFEWYU8M9YEUChmxxii7ruxUa7 BqUoewg8JKFo2FVvySHT+UnxdHQ/d8b+7B9xc2RFy3owuQOtxi8pzMbnGucu6QqMniKj b5G8a5LIe6P7EzRGvuajaz36jNfqDn3CUzZUx4t0L9Wjy8ntA7Z3jyPlyd8ZiSKl1QHl Yrv/xOWoCHyUzJLonypex5lhRnXfqtl0BmJp8MMoviXoFsLlypcDeP9tMFDH4aL4Ko+i l7EA/s8mG2xyIWfNu7u1HSf0TtpXEIEGcGRyMw7Jt2MSnfyv2KzXCkhM8W+lkUKX6G7y 4sNA== X-Gm-Message-State: AKaTC01b13Cgwp/ycxcF3MmyegjF1+ZT64TDrQ063hK3Mh1Q7QnV/LI/j6W8jDA8TmC/JH+5nA== MIME-Version: 1.0 X-Received: by 10.98.130.68 with SMTP id w65mr1914322pfd.39.1480548822002; Wed, 30 Nov 2016 15:33:42 -0800 (PST) Message-ID: <94eb2c1423329c0de305428d22aa@google.com> Date: Wed, 30 Nov 2016 23:34:00 -0000 Subject: Re: [PATCH v3 1/8] btrace: Count gaps as one instruction explicitly. From: Doug Evans To: Tim Wiederhake Cc: gdb-patches@sourceware.org, palves@redhat.com, markus.t.metzger@intel.com Content-Type: text/plain; charset=UTF-8; format=flowed; delsp=yes X-IsSubscribed: yes X-SW-Source: 2016-11/txt/msg01034.txt.bz2 Tim Wiederhake writes: > This gives all instructions, including gaps, a unique number. Add a function > to retrieve the error code if a btrace instruction iterator points to an > invalid instruction. > > 2016-11-21 Tim Wiederhake > > gdb/ChangeLog: > > * btrace.c (ftrace_call_num_insn, btrace_insn_get_error): New function. > (ftrace_new_function, btrace_insn_number, btrace_insn_cmp, > btrace_find_insn_by_number): Remove special case for gaps. > * btrace.h (btrace_insn_get_error): New export. > (btrace_insn_number, btrace_find_insn_by_number): Adjust comment. > * record-btrace.c (btrace_insn_history): Print number for gaps. > (record_btrace_info, record_btrace_goto): Handle gaps. > > > --- > gdb/btrace.c | 83 +++++++++++++++++------------------------------------ > gdb/btrace.h | 9 ++++-- > gdb/record-btrace.c | 34 ++++++++-------------- > 3 files changed, 45 insertions(+), 81 deletions(-) > > diff --git a/gdb/btrace.c b/gdb/btrace.c > index 39d537c..7df1b00 100644 > --- a/gdb/btrace.c > +++ b/gdb/btrace.c > @@ -141,6 +141,20 @@ ftrace_debug (const struct btrace_function *bfun, const char *prefix) > prefix, fun, file, level, ibegin, iend); > } > > +/* Return the number of instructions in a given function call segment. */ > + > +static unsigned int > +ftrace_call_num_insn (const struct btrace_function* bfun) > +{ > + if (bfun == NULL) > + return 0; > + > + if (bfun->errcode != 0) > + return 1; Hi. It's not immediately clear why this test is here and why we return 1. I'm sure there's a reason, and perhaps I just missed the place in the code that makes it clear why, but if such a comment doesn't already exist can you add a comment here explaining why this test is here and why we return 1? [It'll help at least this reader more quickly understand the why of things.] > + > + return VEC_length (btrace_insn_s, bfun->insn); > +} > + > /* Return non-zero if BFUN does not match MFUN and FUN, > return zero otherwise. */ > > @@ -216,8 +230,7 @@ ftrace_new_function (struct btrace_function *prev, > prev->flow.next = bfun; > > bfun->number = prev->number + 1; > - bfun->insn_offset = (prev->insn_offset > - + VEC_length (btrace_insn_s, prev->insn)); > + bfun->insn_offset = prev->insn_offset + ftrace_call_num_insn (prev); > bfun->level = prev->level; > } > > @@ -2150,18 +2163,18 @@ btrace_insn_get (const struct btrace_insn_iterator *it) > > /* See btrace.h. */ > > -unsigned int > -btrace_insn_number (const struct btrace_insn_iterator *it) > +int > +btrace_insn_get_error (const struct btrace_insn_iterator *it) > { > - const struct btrace_function *bfun; > - > - bfun = it->function; > + return it->function->errcode; > +} > > - /* Return zero if the iterator points to a gap in the trace. */ > - if (bfun->errcode != 0) > - return 0; > +/* See btrace.h. */ > > - return bfun->insn_offset + it->index; > +unsigned int > +btrace_insn_number (const struct btrace_insn_iterator *it) > +{ > + return it->function->insn_offset + it->index; > } Previously btrace_insn_number watched for bfun->errcode. Now it does not, which is ok in and of itself. But I don't see anything in the function comment that says something like "Do not call this if errcode is non-zero." or some such. Or maybe in places where this will be called errcode is irrelevant or must be zero (if the latter then an assert here would be nice)? > > /* See btrace.h. */ > @@ -2356,37 +2369,6 @@ btrace_insn_cmp (const struct btrace_insn_iterator *lhs, > lnum = btrace_insn_number (lhs); > rnum = btrace_insn_number (rhs); > > - /* A gap has an instruction number of zero. Things are getting more > - complicated if gaps are involved. > - > - We take the instruction number offset from the iterator's function. > - This is the number of the first instruction after the gap. > - > - This is OK as long as both lhs and rhs point to gaps. If only one of > - them does, we need to adjust the number based on the other's regular > - instruction number. Otherwise, a gap might compare equal to an > - instruction. */ > - > - if (lnum == 0 && rnum == 0) > - { > - lnum = lhs->function->insn_offset; > - rnum = rhs->function->insn_offset; > - } > - else if (lnum == 0) > - { > - lnum = lhs->function->insn_offset; > - > - if (lnum == rnum) > - lnum -= 1; > - } > - else if (rnum == 0) > - { > - rnum = rhs->function->insn_offset; > - > - if (rnum == lnum) > - rnum -= 1; > - } > - > return (int) (lnum - rnum); > } > > @@ -2398,26 +2380,15 @@ btrace_find_insn_by_number (struct btrace_insn_iterator *it, > unsigned int number) > { > const struct btrace_function *bfun; > - unsigned int end, length; > > for (bfun = btinfo->end; bfun != NULL; bfun = bfun->flow.prev) > - { > - /* Skip gaps. */ > - if (bfun->errcode != 0) > - continue; > - > - if (bfun->insn_offset <= number) > - break; > - } > + if (bfun->insn_offset <= number) > + break; > > if (bfun == NULL) > return 0; > > - length = VEC_length (btrace_insn_s, bfun->insn); > - gdb_assert (length > 0); > - > - end = bfun->insn_offset + length; > - if (end <= number) > + if (bfun->insn_offset + ftrace_call_num_insn (bfun) <= number) > return 0; > > it->function = bfun; > diff --git a/gdb/btrace.h b/gdb/btrace.h > index 202b986..114242d 100644 > --- a/gdb/btrace.h > +++ b/gdb/btrace.h > @@ -405,9 +405,12 @@ extern void parse_xml_btrace_conf (struct btrace_config *conf, const char *xml); > extern const struct btrace_insn * > btrace_insn_get (const struct btrace_insn_iterator *); > > +/* Return the error code for a branch trace instruction iterator. Returns zero > + if there is no error, i.e. the instruction is valid. */ > +extern int btrace_insn_get_error (const struct btrace_insn_iterator *); > + > /* Return the instruction number for a branch trace iterator. > - Returns one past the maximum instruction number for the end iterator. > - Returns zero if the iterator does not point to a valid instruction. */ > + Returns one past the maximum instruction number for the end iterator. */ > extern unsigned int btrace_insn_number (const struct btrace_insn_iterator *); > > /* Initialize a branch trace instruction iterator to point to the begin/end of > @@ -433,7 +436,7 @@ extern unsigned int btrace_insn_prev (struct btrace_insn_iterator *, > extern int btrace_insn_cmp (const struct btrace_insn_iterator *lhs, > const struct btrace_insn_iterator *rhs); > > -/* Find an instruction in the function branch trace by its number. > +/* Find an instruction or gap in the function branch trace by its number. > If the instruction is found, initialize the branch trace instruction > iterator to point to this instruction and return non-zero. > Return zero otherwise. */ > diff --git a/gdb/record-btrace.c b/gdb/record-btrace.c > index 7c0e39f..6c50bab 100644 > --- a/gdb/record-btrace.c > +++ b/gdb/record-btrace.c > @@ -439,28 +439,12 @@ record_btrace_info (struct target_ops *self) > calls = btrace_call_number (&call); > > btrace_insn_end (&insn, btinfo); > - > insns = btrace_insn_number (&insn); > - if (insns != 0) > - { > - /* The last instruction does not really belong to the trace. */ > - insns -= 1; > - } > - else > - { > - unsigned int steps; > > - /* Skip gaps at the end. */ > - do > - { > - steps = btrace_insn_prev (&insn, 1); > - if (steps == 0) > - break; > - > - insns = btrace_insn_number (&insn); > - } > - while (insns == 0); > - } > + /* If the last instruction is not a gap, it is the current instruction > + that is not actually part of the record. */ > + if (btrace_insn_get (&insn) != NULL) > + insns -= 1; > > gaps = btinfo->ngaps; > } > @@ -736,7 +720,11 @@ btrace_insn_history (struct ui_out *uiout, > /* We have trace so we must have a configuration. */ > gdb_assert (conf != NULL); > > - btrace_ui_out_decode_error (uiout, it.function->errcode, > + ui_out_field_fmt (uiout, "insn-number", "%u", > + btrace_insn_number (&it)); > + ui_out_text (uiout, "\t"); > + > + btrace_ui_out_decode_error (uiout, btrace_insn_get_error (&it), > conf->format); > } > else > @@ -2827,7 +2815,9 @@ record_btrace_goto (struct target_ops *self, ULONGEST insn) > tp = require_btrace_thread (); > > found = btrace_find_insn_by_number (&it, &tp->btrace, number); > - if (found == 0) > + > + /* Check if the instruction could not be found or is a gap. */ > + if (found == 0 || btrace_insn_get (&it) == NULL) > error (_("No such instruction.")); > > record_btrace_set_replay (tp, &it); > -- > 2.7.4 >