Message ID | CAF5HaEUBbK4d2reSxUpUQwp7Zt_KjcpNC_nbUrMHRb0yNo9wtw@mail.gmail.com |
---|---|
State | Changes Requested, archived |
Headers |
Return-Path: <x14314964@homiemail-mx21.g.dreamhost.com> X-Original-To: siddhesh@wilcox.dreamhost.com Delivered-To: siddhesh@wilcox.dreamhost.com Received: from homiemail-mx21.g.dreamhost.com (peon2454.g.dreamhost.com [208.113.200.127]) by wilcox.dreamhost.com (Postfix) with ESMTP id 6797536007D for <siddhesh@wilcox.dreamhost.com>; Fri, 11 Apr 2014 15:25:00 -0700 (PDT) Received: by homiemail-mx21.g.dreamhost.com (Postfix, from userid 14314964) id 0477B1283F36; Fri, 11 Apr 2014 15:24:59 -0700 (PDT) X-Original-To: gdb@patchwork.siddhesh.in Delivered-To: x14314964@homiemail-mx21.g.dreamhost.com Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by homiemail-mx21.g.dreamhost.com (Postfix) with ESMTPS id CE1AB1283F2E for <gdb@patchwork.siddhesh.in>; Fri, 11 Apr 2014 15:24:59 -0700 (PDT) DomainKey-Signature: a=rsa-sha1; c=nofws; d=sourceware.org; h=list-id :list-unsubscribe:list-subscribe:list-archive:list-post :list-help:sender:mime-version:date:message-id:subject:from:to :content-type; q=dns; s=default; b=HwLbkGQKnBy1a9XFnEM3fvL1ggF5y BdXhuFqSMXx3D4LJ+xW5dFpIx1DArWCxTzoMImftviPLYPlKR60EIH50xKwK0P2a jjH6eQgjIDnZav96P07AjUm2NYDeMJdKXqInyjVHlzpfFTFUVu2TX8Gxg9CppFcF 0xHrjwHc/0k5e4= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=sourceware.org; h=list-id :list-unsubscribe:list-subscribe:list-archive:list-post :list-help:sender:mime-version:date:message-id:subject:from:to :content-type; s=default; bh=aaR0QpYIYGWmsNXSp+HljvRvs/I=; b=Jjp X+HqX0FdApjpUULWCtk6Sp0egEpR/DyQRwl9kbhA59ovGWTk4iKoqRJg7tbFBKZB 0Bb/PaPJtKuLnU6QjLtyY6ot4Bjjgi1EPpF7Wnkzzv2q0O56dPa8YFaFpMDBHU4U yunOGpNBh6JxUOtJ/ha/sRdDvZu4WpJbX/2G3H0s= Received: (qmail 14068 invoked by alias); 11 Apr 2014 22:24:57 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <gdb-patches.sourceware.org> List-Unsubscribe: <mailto:gdb-patches-unsubscribe-gdb=patchwork.siddhesh.in@sourceware.org> List-Subscribe: <mailto:gdb-patches-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/gdb-patches/> List-Post: <mailto:gdb-patches@sourceware.org> List-Help: <mailto:gdb-patches-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 14058 invoked by uid 89); 11 Apr 2014 22:24:56 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.4 required=5.0 tests=AWL, BAYES_00, RCVD_IN_DNSWL_LOW, SPF_SOFTFAIL autolearn=no version=3.3.2 X-HELO: mail-la0-f42.google.com Received: from mail-la0-f42.google.com (HELO mail-la0-f42.google.com) (209.85.215.42) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-SHA encrypted) ESMTPS; Fri, 11 Apr 2014 22:24:56 +0000 Received: by mail-la0-f42.google.com with SMTP id ec20so4060855lab.1 for <gdb-patches@sourceware.org>; Fri, 11 Apr 2014 15:24:51 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:date:message-id:subject:from:to :content-type; bh=6QE/Ll3Cd77cGdOJFDDKuZww3pp/3Wplt4xpp7E3wKc=; b=EiXvKL1Zkf/9yX6D+bRca0WhW9PXPuV5FL3YhKa/YX98v4PfluKD2l2XUj5X/2ZkCZ RDjsM8R4lFHvNrt5oQtKzxXrpYdN899vZlWnKsC1MoN0fg9Ydh5bdXBoX7qPyzlkBDpy zrkksLhb6UhUTzRvm4e19S/y+Qqu/uH9pLnVuUx5XUPo4SYOnmXpamwdIV1z+u5pfN6Y W4DDfnWZ+7CiYfx3KPGu0a37bACFPdxunf5DQfdO1QVC795G5p+fwVwg7MOCIITL61+s mFYaNVRhtog963600bbb0kXBGs8IdR/LX/lxubtazGEQKDHmMA5EfWJRbzCCGlVBy8Fc GXYQ== X-Gm-Message-State: ALoCoQmNyg1WxqJB/x+pOFYLK77bVasa3OShF9lOpBuvaoDNU65Qjz8f/U2Xs/d8/PH57R5AE5ph MIME-Version: 1.0 X-Received: by 10.112.142.68 with SMTP id ru4mr2918308lbb.49.1397255090974; Fri, 11 Apr 2014 15:24:50 -0700 (PDT) Received: by 10.112.9.40 with HTTP; Fri, 11 Apr 2014 15:24:50 -0700 (PDT) Date: Fri, 11 Apr 2014 19:24:50 -0300 Message-ID: <CAF5HaEUBbK4d2reSxUpUQwp7Zt_KjcpNC_nbUrMHRb0yNo9wtw@mail.gmail.com> Subject: [PATCH] Fix alignment of disassemble /r From: Daniel Gutson <daniel.gutson@tallertechnologies.com> To: gdb-patches <gdb-patches@sourceware.org> Content-Type: multipart/mixed; boundary=001a11c36dfc5bd7c404f6cbcd42 X-IsSubscribed: yes X-DH-Original-To: gdb@patchwork.siddhesh.in |
Commit Message
Daniel Gutson
April 11, 2014, 10:24 p.m. UTC
Hi, when disassembling in raw mode (/r) in a variable-length insn architecture (i.e. x86), the output can be completely messed since no alignment takes place. I am aware of the uiout->table stuff, but it seems an overkill since I should change the current_uiout when disassembling in this mode (and I didn't find any actual use of this machinery at least for x86). Therefore, I added a hack in the dump_insns when the /r flag is specified. This clearly isn't the cutiest thing in the world, and I specified a hardcoded maximum number of opcode bytes to align (currently 8) though it is easily changeable. Please let me know if this approach is OK or I should do something else. Maybe consider whether current arch is insn-len variable? If this happens to be OK, please commit it for me since I don't have write access. Thanks, Daniel. 2014-04-11 Daniel Gutson <daniel.gutson@tallertechnologies.com> * disasm.c (dump_insns): Added right alignment when showing opcodes.
Comments
Ping for maintainer feedback. Thanks, Daniel. On Fri, Apr 11, 2014 at 7:24 PM, Daniel Gutson <daniel.gutson@tallertechnologies.com> wrote: > Hi, > > when disassembling in raw mode (/r) in a variable-length insn > architecture (i.e. x86), > the output can be completely messed since no alignment takes place. > > I am aware of the uiout->table stuff, but it seems an overkill since I > should change > the current_uiout when disassembling in this mode (and I didn't find > any actual use of this > machinery at least for x86). > Therefore, I added a hack in the dump_insns when the /r flag is specified. > This clearly isn't the cutiest thing in the world, and I specified a > hardcoded maximum number > of opcode bytes to align (currently 8) though it is easily changeable. > > Please let me know if this approach is OK or I should do something > else. Maybe consider whether > current arch is insn-len variable? > > If this happens to be OK, please commit it for me since I don't have > write access. > > Thanks, > > Daniel. > > > 2014-04-11 Daniel Gutson <daniel.gutson@tallertechnologies.com> > > * disasm.c (dump_insns): Added right alignment when showing opcodes.
On 04/12/2014 06:24 AM, Daniel Gutson wrote: > when disassembling in raw mode (/r) in a variable-length insn > architecture (i.e. x86), > the output can be completely messed since no alignment takes place. The /r output is messed, but not completely :). > > I am aware of the uiout->table stuff, but it seems an overkill since I > should change > the current_uiout when disassembling in this mode (and I didn't find > any actual use of this > machinery at least for x86). > Therefore, I added a hack in the dump_insns when the /r flag is specified. > This clearly isn't the cutiest thing in the world, and I specified a > hardcoded maximum number > of opcode bytes to align (currently 8) though it is easily changeable. Hard-coded opcode length will affect other targets. For example, without your patch, the disassembly for c6x is like, (gdb) disassemble /r main Dump of assembler code for function main: 0x00000860 <+0>: c2 1b be 07 subah .D2 b15,16,b15 0x00000864 <+4>: f6 02 3d 07 stw .D2T2 b14,*+b15(32) with your patch applied, it becomes (gdb) disassemble /r main Dump of assembler code for function main: 0x00000860 <+0>: c2 1b be 07 subah .D2 b15,16,b15 0x00000864 <+4>: f6 02 3d 07 stw .D2T2 b14,*+b15(32) gdbarch_max_insn_length can tell us the max instruction length, but if we align the instruction to the max length (it is 16 on x86), the text instruction will be far behind the hex code, which is a little ugly.
On Wed, Apr 16, 2014 at 10:07 PM, Yao Qi <yao@codesourcery.com> wrote: > On 04/12/2014 06:24 AM, Daniel Gutson wrote: >> when disassembling in raw mode (/r) in a variable-length insn >> architecture (i.e. x86), >> the output can be completely messed since no alignment takes place. > > The /r output is messed, but not completely :). > >> >> I am aware of the uiout->table stuff, but it seems an overkill since I >> should change >> the current_uiout when disassembling in this mode (and I didn't find >> any actual use of this >> machinery at least for x86). >> Therefore, I added a hack in the dump_insns when the /r flag is specified. >> This clearly isn't the cutiest thing in the world, and I specified a >> hardcoded maximum number >> of opcode bytes to align (currently 8) though it is easily changeable. > > Hard-coded opcode length will affect other targets. For example, without > your patch, the disassembly for c6x is like, > (gdb) disassemble /r main > Dump of assembler code for function main: > 0x00000860 <+0>: c2 1b be 07 subah .D2 b15,16,b15 > 0x00000864 <+4>: f6 02 3d 07 stw .D2T2 b14,*+b15(32) > > with your patch applied, it becomes > > (gdb) disassemble /r main > Dump of assembler code for function main: > 0x00000860 <+0>: c2 1b be 07 subah .D2 b15,16,b15 > 0x00000864 <+4>: f6 02 3d 07 stw .D2T2 b14,*+b15(32) > > gdbarch_max_insn_length can tell us the max instruction length, but if we > align the instruction to the max length (it is 16 on x86), the text > instruction will be far behind the hex code, which is a little ugly. Hi Yao, thanks for answering. So what if I add a new configuration variable, such as set disassemble-raw-alignment with "off" as default, and if set to on, pad to gdbarch_max_insn_length ? > > -- > Yao (齐尧)
On Thu, Apr 17, 2014 at 7:37 AM, Daniel Gutson <daniel.gutson@tallertechnologies.com> wrote: > So what if I add a new configuration variable, such as > set disassemble-raw-alignment > with "off" as default, and if set to on, pad to gdbarch_max_insn_length ? Presumably some frontends will do their own alignment. If we went with disassemble-raw-alignment, a boolean value won't help x86 much, it's either no alignment or (in general) too much whitespace. An improvement would be a value from min-insn-length to max-insn-length, but that would be problematic in a multi-arch debugging scenario. If we could agree on some minimum alignment for each variable-length ISA (5 would be fine for me for x86) then maybe a boolean value could be useful ("off" = no alignment, "on" = employ arch-specific minimum). OTOH, what if we made two passes over the instructions, with the first pass computing the maximum instruction length that is present? [And maybe only doing this for CLI.]
On Thu, Apr 17, 2014 at 1:27 PM, Doug Evans <dje@google.com> wrote: > On Thu, Apr 17, 2014 at 7:37 AM, Daniel Gutson > <daniel.gutson@tallertechnologies.com> wrote: >> So what if I add a new configuration variable, such as >> set disassemble-raw-alignment >> with "off" as default, and if set to on, pad to gdbarch_max_insn_length ? > > Presumably some frontends will do their own alignment. > > If we went with disassemble-raw-alignment, a boolean value won't help > x86 much, it's either no alignment or (in general) too much > whitespace. > An improvement would be a value from min-insn-length to > max-insn-length, but that would be problematic in a multi-arch > debugging scenario. > > If we could agree on some minimum alignment for each variable-length > ISA (5 would be fine for me for x86) then maybe a boolean value could > be useful ("off" = no alignment, "on" = employ arch-specific minimum). > > OTOH, what if we made two passes over the instructions, with the first > pass computing the maximum instruction length that is present? I already considered this, but thought that it would be going to be rejected due to be too much non-performant. Wouldn't each pass translate in a lot of MI messaging in a case of a remote server? And, what about screen paginig? I shouldn't iterate over all the range, but the screen height range only. I can go for any of the proposed solutions. 5 insn-length was fine for me. On a side note, I did this since I was debugging some self-modifying-code where the mis-alignment was driving me crazy. The two-passes solution is the one I dislike more IMVHO. Yet another option is to set the disassembly raw alignment width, something like set disassemble-raw-width 5 (5 is the amount of insn; 0 means no padding equal to the current behavior and that would be the default value). Daniel. > [And maybe only doing this for CLI.]
On Thu, Apr 17, 2014 at 10:27 AM, Daniel Gutson <daniel.gutson@tallertechnologies.com> wrote: > On Thu, Apr 17, 2014 at 1:27 PM, Doug Evans <dje@google.com> wrote: >> On Thu, Apr 17, 2014 at 7:37 AM, Daniel Gutson >> <daniel.gutson@tallertechnologies.com> wrote: >>> So what if I add a new configuration variable, such as >>> set disassemble-raw-alignment >>> with "off" as default, and if set to on, pad to gdbarch_max_insn_length ? >> >> Presumably some frontends will do their own alignment. >> >> If we went with disassemble-raw-alignment, a boolean value won't help >> x86 much, it's either no alignment or (in general) too much >> whitespace. >> An improvement would be a value from min-insn-length to >> max-insn-length, but that would be problematic in a multi-arch >> debugging scenario. >> >> If we could agree on some minimum alignment for each variable-length >> ISA (5 would be fine for me for x86) then maybe a boolean value could >> be useful ("off" = no alignment, "on" = employ arch-specific minimum). >> >> OTOH, what if we made two passes over the instructions, with the first >> pass computing the maximum instruction length that is present? > > I already considered this, but thought that it would be going to be > rejected due to be too much non-performant. Wouldn't each pass > translate in a lot of MI messaging in a case of a remote server? [nit: If by "remote server" you mean things like gdbserver, then MI isn't involved.] Yes, it *could* involve extra traffic between gdb and gdbserver (or whatever stub is being used) but we have caching now that should mitigate that. Still, it's a valid concern. > And, > what about screen paginig? I shouldn't iterate over all the range, but > the screen height range only. I guess one could go that route but IMO it's not necessary. OTOH, One *could* also do the max-length computation in bunches that took advantage of the cache size thus guaranteeing no additional gdb/gdbserver traffic. Not my first choice, just a possibility. [gdb has a lot of knobs, just want to make sure no stone is left unturned before we add this one :-)] > I can go for any of the proposed solutions. 5 insn-length was fine for > me. On a side note, I did this since I was debugging some > self-modifying-code where the mis-alignment was driving me crazy. > The two-passes solution is the one I dislike more IMVHO. > > Yet another option is to set the disassembly raw alignment width, something like > set disassemble-raw-width 5 > (5 is the amount of insn; 0 means no padding equal to the current > behavior and that would be the default value). The problem with this is that the setting is arch-specific. Not that one can do this today (or maybe one can in certain situations, e.g. Cell), but if I'm debugging two different programs from two different architectures it's possible any one value of disassemble-raw-width is going to be painful. One *could* have prefix commands for each architecture, and then one could have set arch-$arch disassemble-raw-width N or set arch $arch disassemble-raw-width N [again, just a possibility, not sure I prefer it] Sorry for not having a definitive suggestion. Maybe others will express an opinion and we can go from there.
On 04/18/2014 01:27 AM, Daniel Gutson wrote: > I already considered this, but thought that it would be going to be > rejected due to be too much non-performant. Wouldn't each pass > translate in a lot of MI messaging in a case of a remote server? And, If you meant "rsp packets" rather than "MI messaging", we don't worry about the performance much here. disassemble uses code cache (target_read_code) to read instructions from remote server and the following read to the same area will hit the cache. > what about screen paginig? I shouldn't iterate over all the range, but > the screen height range only. What is the reason do you think we shouldn't iterator over all the range? IMO, screen height and alignment are orthogonal.
On Sun, Apr 20, 2014 at 9:54 PM, Yao Qi <yao@codesourcery.com> wrote: > On 04/18/2014 01:27 AM, Daniel Gutson wrote: >> I already considered this, but thought that it would be going to be >> rejected due to be too much non-performant. Wouldn't each pass >> translate in a lot of MI messaging in a case of a remote server? And, > > If you meant "rsp packets" rather than "MI messaging", Yes, my bad, sorry. we don't worry > about the performance much here. disassemble uses code cache > (target_read_code) to read instructions from remote server and the > following read to the same area will hit the cache. > >> what about screen paginig? I shouldn't iterate over all the range, but >> the screen height range only. > > What is the reason do you think we shouldn't iterator over all the > range? IMO, screen height and alignment are orthogonal. Suppose your function takes two screens of disassembly. Then suppose that the instructions that fit in the first screen take N bytes at most, and those of the second takes N+k. If I ignore screen paging, I would be aligning the first screen to N+k too, whereas those k are just useless space. It's a minor and cosmetic concern, but I'd like to get it right and pretty in one shot :) Daniel. > > -- > Yao (齐尧)
OK timeout, I wil just KISS and implement the 2-passes solution without considering paging. I'll be back with the new version of the patch. Thanks for your comments! Daniel. On Mon, Apr 21, 2014 at 1:00 PM, Daniel Gutson <daniel.gutson@tallertechnologies.com> wrote: > On Sun, Apr 20, 2014 at 9:54 PM, Yao Qi <yao@codesourcery.com> wrote: >> On 04/18/2014 01:27 AM, Daniel Gutson wrote: >>> I already considered this, but thought that it would be going to be >>> rejected due to be too much non-performant. Wouldn't each pass >>> translate in a lot of MI messaging in a case of a remote server? And, >> >> If you meant "rsp packets" rather than "MI messaging", > > Yes, my bad, sorry. > > we don't worry >> about the performance much here. disassemble uses code cache >> (target_read_code) to read instructions from remote server and the >> following read to the same area will hit the cache. >> >>> what about screen paginig? I shouldn't iterate over all the range, but >>> the screen height range only. >> >> What is the reason do you think we shouldn't iterator over all the >> range? IMO, screen height and alignment are orthogonal. > > Suppose your function takes two screens of disassembly. > Then suppose that the instructions that fit in the first screen take > N bytes at most, > and those of the second takes N+k. If I ignore screen paging, I would > be aligning > the first screen to N+k too, whereas those k are just useless space. > It's a minor and cosmetic concern, but I'd like to get it right and > pretty in one shot :) > > Daniel. > >> >> -- >> Yao (齐尧) > > > > -- > > Daniel F. Gutson > Chief Engineering Officer, SPD > > > San Lorenzo 47, 3rd Floor, Office 5 > > Córdoba, Argentina > > > Phone: +54 351 4217888 / +54 351 4218211 > > Skype: dgutson
FWIW: https://sourceware.org/bugzilla/show_bug.cgi?id=19768 On Tue, Apr 22, 2014 at 9:13 AM, Daniel Gutson <daniel.gutson@tallertechnologies.com> wrote: > OK timeout, I wil just KISS and implement the 2-passes solution > without considering paging. > > I'll be back with the new version of the patch. > > Thanks for your comments! > > Daniel. > > On Mon, Apr 21, 2014 at 1:00 PM, Daniel Gutson > <daniel.gutson@tallertechnologies.com> wrote: >> On Sun, Apr 20, 2014 at 9:54 PM, Yao Qi <yao@codesourcery.com> wrote: >>> On 04/18/2014 01:27 AM, Daniel Gutson wrote: >>>> I already considered this, but thought that it would be going to be >>>> rejected due to be too much non-performant. Wouldn't each pass >>>> translate in a lot of MI messaging in a case of a remote server? And, >>> >>> If you meant "rsp packets" rather than "MI messaging", >> >> Yes, my bad, sorry. >> >> we don't worry >>> about the performance much here. disassemble uses code cache >>> (target_read_code) to read instructions from remote server and the >>> following read to the same area will hit the cache. >>> >>>> what about screen paginig? I shouldn't iterate over all the range, but >>>> the screen height range only. >>> >>> What is the reason do you think we shouldn't iterator over all the >>> range? IMO, screen height and alignment are orthogonal. >> >> Suppose your function takes two screens of disassembly. >> Then suppose that the instructions that fit in the first screen take >> N bytes at most, >> and those of the second takes N+k. If I ignore screen paging, I would >> be aligning >> the first screen to N+k too, whereas those k are just useless space. >> It's a minor and cosmetic concern, but I'd like to get it right and >> pretty in one shot :) >> >> Daniel. >> >>> >>> -- >>> Yao (齐尧) >> >> >> >> -- >> >> Daniel F. Gutson >> Chief Engineering Officer, SPD >> >> >> San Lorenzo 47, 3rd Floor, Office 5 >> >> Córdoba, Argentina >> >> >> Phone: +54 351 4217888 / +54 351 4218211 >> >> Skype: dgutson > > > > -- > > Daniel F. Gutson > Chief Engineering Officer, SPD > > > San Lorenzo 47, 3rd Floor, Office 5 > > Córdoba, Argentina > > > Phone: +54 351 4217888 / +54 351 4218211 > > Skype: dgutson
diff --git a/gdb/disasm.c b/gdb/disasm.c index d94225b..0fd5aa1 100644 --- a/gdb/disasm.c +++ b/gdb/disasm.c @@ -107,6 +107,13 @@ dump_insns (struct gdbarch *gdbarch, struct ui_out *uiout, int offset; int line; struct cleanup *ui_out_chain; + /* This array holds enough space for 8 bytes of opcodes; + since the format is 02x plus the space, the length + the array is (2chars + 1space) * 8 bytes + 1zero = 25 chars. */ + char right_align[8 * 3 + 1]; + + memset(right_align, ' ', sizeof(right_align) - 2); + right_align[sizeof(right_align) - 1] = 0; for (pc = low; pc < high;) { @@ -154,6 +161,7 @@ dump_insns (struct gdbarch *gdbarch, struct ui_out *uiout, bfd_byte data; int status; const char *spacer = ""; + unsigned int alignment_pos = 0; /* Build the opcodes using a temporary stream so we can write them out in a single go for the MI. */ @@ -170,8 +178,11 @@ dump_insns (struct gdbarch *gdbarch, struct ui_out *uiout, fprintf_filtered (opcode_stream, "%s%02x", spacer, (unsigned) data); spacer = " "; + alignment_pos += 3; } ui_out_field_stream (uiout, "opcodes", opcode_stream); + if (alignment_pos < sizeof(right_align)) + ui_out_text (uiout, &right_align[alignment_pos]); ui_out_text (uiout, "\t"); do_cleanups (cleanups);