From patchwork Mon Apr 11 09:01:29 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: "Metzger, Markus T" X-Patchwork-Id: 11689 Received: (qmail 122656 invoked by alias); 11 Apr 2016 09:01:57 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 122599 invoked by uid 89); 11 Apr 2016 09:01:56 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.4 required=5.0 tests=AWL, BAYES_00, RP_MATCHES_RCVD, SPF_PASS autolearn=ham version=3.3.2 spammy=U*dje, dje@google.com, djegooglecom, daniel.gutson@tallertechnologies.com X-HELO: mga14.intel.com Received: from mga14.intel.com (HELO mga14.intel.com) (192.55.52.115) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 11 Apr 2016 09:01:46 +0000 Received: from fmsmga004.fm.intel.com ([10.253.24.48]) by fmsmga103.fm.intel.com with ESMTP; 11 Apr 2016 02:01:34 -0700 X-ExtLoop1: 1 Received: from irsmsx109.ger.corp.intel.com ([163.33.3.23]) by fmsmga004.fm.intel.com with ESMTP; 11 Apr 2016 02:01:31 -0700 Received: from irsmsx104.ger.corp.intel.com ([169.254.5.87]) by IRSMSX109.ger.corp.intel.com ([169.254.13.57]) with mapi id 14.03.0248.002; Mon, 11 Apr 2016 10:01:30 +0100 From: "Metzger, Markus T" To: Leonardo Boquillon , "gdb-patches@sourceware.org" , "daniel.gutson@tallertechnologies.com" , "dje@google.com" Subject: RE: [PATCH v3] Fix alignment of disassemble /r Date: Mon, 11 Apr 2016 09:01:29 +0000 Message-ID: References: <1460066322-21732-1-git-send-email-leonardo.boquillon@tallertechnologies.com> In-Reply-To: <1460066322-21732-1-git-send-email-leonardo.boquillon@tallertechnologies.com> MIME-Version: 1.0 X-IsSubscribed: yes > -----Original Message----- > From: gdb-patches-owner@sourceware.org [mailto:gdb-patches- > owner@sourceware.org] On Behalf Of Leonardo Boquillon > Sent: Thursday, April 7, 2016 11:59 PM > To: gdb-patches@sourceware.org; daniel.gutson@tallertechnologies.com; > dje@google.com; Metzger, Markus T > Subject: [PATCH v3] Fix alignment of disassemble /r > > When disassembling in raw mode (/r) in a variable-length insn architecture (i.e. > x86), > the output can be messed since no alignment takes place. > > The first version of this was sent on this mail thread > https://sourceware.org/ml/gdb-patches/2014-04/msg00226.html and this patch > is > the same placed previously here > https://sourceware.org/bugzilla/show_bug.cgi?id=19768 > > This patch performs the two passes: the first is at get_insn_set_longest_opcode > at line 136 in the patch, and the second is the while loop that follows like was > agreed here https://sourceware.org/ml/gdb-patches/2014-04/msg00427.html > > In this version have been added the capability to compute longest opcode for > btrace. Thanks. I tested the btrace part with this: You might want to add such a test to your patch, as well. > diff --git a/gdb/record-btrace.c b/gdb/record-btrace.c > index 77b5180..84d9d57 100644 > --- a/gdb/record-btrace.c > +++ b/gdb/record-btrace.c > @@ -660,6 +660,29 @@ btrace_print_lines (struct btrace_line_range lines, struct > ui_out *uiout, > } > } > > +static unsigned int > +btrace_get_longest_opcode(struct gdbarch *gdbarch, > + const struct btrace_insn_iterator *begin, > + const struct btrace_insn_iterator *end) Please add a short comment for the function. > +{ > + unsigned int longest_opcode = 0; > + struct btrace_insn_iterator it; > + > + for (it = *begin; btrace_insn_cmp (&it, end) != 0; btrace_insn_next (&it, 1)) > + { > + const struct btrace_insn *insn = btrace_insn_get (&it); > + > + if (insn != NULL) > + { > + const unsigned int current_length = > + gdb_insn_length(gdbarch, insn->pc); Note that gdb_insn_length returns (signed) int. I'd use int throughout for the opcode size. There's a space between function name and '('. > + longest_opcode = > + (current_length > longest_opcode) ? current_length : longest_opcode; > + } > + } > + > + return longest_opcode; > +} > /* Disassemble a section of the recorded instruction trace. */ > > static void > @@ -674,6 +697,7 @@ btrace_insn_history (struct ui_out *uiout, > struct gdbarch *gdbarch; > struct btrace_insn_iterator it; > struct btrace_line_range last_lines; > + unsigned int longest_opcode; > > DEBUG ("itrace (0x%x): [%u; %u)", flags, btrace_insn_number (begin), > btrace_insn_number (end)); > @@ -692,6 +716,7 @@ btrace_insn_history (struct ui_out *uiout, > instructions corresponding to that line. */ > ui_item_chain = NULL; > > + longest_opcode = btrace_get_longest_opcode(gdbarch, begin, end); There's a space between function name and '('. > for (it = *begin; btrace_insn_cmp (&it, end) != 0; btrace_insn_next (&it, 1)) > { > const struct btrace_insn *insn; > @@ -745,7 +770,8 @@ btrace_insn_history (struct ui_out *uiout, > if ((insn->flags & BTRACE_INSN_FLAG_SPECULATIVE) != 0) > dinsn.is_speculative = 1; > > - gdb_pretty_print_insn (gdbarch, uiout, &di, &dinsn, flags, stb); > + gdb_pretty_print_insn (gdbarch, uiout, &di, &dinsn, flags, stb, > + longest_opcode); > } > } The btrace hunks are OK otherwise. The signed vs. unsigned int and the formatting comments apply to the other changes, as well. Regards, Markus. Intel Deutschland GmbH Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany Tel: +49 89 99 8853-0, www.intel.de Managing Directors: Christin Eisenschmid, Christian Lamprechter Chairperson of the Supervisory Board: Nicole Lau Registered Office: Munich Commercial Register: Amtsgericht Muenchen HRB 186928 diff --git a/gdb/testsuite/gdb.btrace/instruction_history.exp b/gdb/testsuite/gdb.btrace/instruction_h index 58ae770..b361e32 100644 --- a/gdb/testsuite/gdb.btrace/instruction_history.exp +++ b/gdb/testsuite/gdb.btrace/instruction_history.exp @@ -97,6 +97,18 @@ gdb_test "record instruction-history /pf 3,7" [multi_line \ "7\t 0x\[0-9a-f\]+ <\\+\[0-9\]+>:\tje 0x\[0-9a-f\]+ \r" \ ] +# We don't know the exact encoding that is used but we know that the raw +# opcode bytes should be padded so they should all be the same length. +# +# To simplify things let's assume that the longest opcode is 3B. +gdb_test "record instruction-history /r 3,7" [multi_line \ + "3\t 0x\[0-9a-f\]+ :\t\[0-9a-f \]{8} je 0x\[0-9a-f\]+ " \ + "4\t 0x\[0-9a-f\]+ :\t\[0-9a-f \]{8} dec %eax" \ + "5\t 0x\[0-9a-f\]+ :\t\[0-9a-f \]{8} jmp 0x\[0-9a-f\]+ " \ + "6\t 0x\[0-9a-f\]+ :\t\[0-9a-f \]{8} cmp \\\$0x0,%eax" \ + "7\t 0x\[0-9a-f\]+ :\t\[0-9a-f \]{8} je 0x\[0-9a-f\]+ \r" + ] + gdb_test "record instruction-history 3,3" "3\t 0x\[0-9a-f\]+ :\tje 0x\[0-9a-f\ # the following tests are checking the iterators