From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 27144 invoked by alias); 26 Apr 2012 13:34:40 -0000 Received: (qmail 27051 invoked by uid 22791); 26 Apr 2012 13:34:39 -0000 X-SWARE-Spam-Status: No, hits=-4.1 required=5.0 tests=AWL,BAYES_00,KHOP_RCVD_UNTRUST,KHOP_THREADED,RCVD_IN_HOSTKARMA_W,RCVD_IN_HOSTKARMA_WL X-Spam-Check-By: sourceware.org Received: from relay1.mentorg.com (HELO relay1.mentorg.com) (192.94.38.131) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Thu, 26 Apr 2012 13:34:25 +0000 Received: from svr-orw-exc-10.mgc.mentorg.com ([147.34.98.58]) by relay1.mentorg.com with esmtp id 1SNOpz-0004GV-21 from Maciej_Rozycki@mentor.com ; Thu, 26 Apr 2012 06:34:23 -0700 Received: from SVR-IES-FEM-01.mgc.mentorg.com ([137.202.0.104]) by SVR-ORW-EXC-10.mgc.mentorg.com with Microsoft SMTPSVC(6.0.3790.4675); Thu, 26 Apr 2012 06:34:21 -0700 Received: from [172.30.0.84] (137.202.0.76) by SVR-IES-FEM-01.mgc.mentorg.com (137.202.0.104) with Microsoft SMTP Server id 14.1.289.1; Thu, 26 Apr 2012 14:34:21 +0100 Date: Thu, 26 Apr 2012 13:54:00 -0000 From: "Maciej W. Rozycki" To: Eli Zaretskii CC: Subject: Re: [PATCH] microMIPS support In-Reply-To: <83k414fjmp.fsf@gnu.org> Message-ID: References: <83k414fjmp.fsf@gnu.org> User-Agent: Alpine 1.10 (DEB 962 2008-03-14) MIME-Version: 1.0 Content-Type: text/plain; charset="US-ASCII" 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 X-SW-Source: 2012-04/txt/msg00903.txt.bz2 On Wed, 25 Apr 2012, Eli Zaretskii wrote: > > +@item set mips compression @var{arg} > > +@kindex set mips compression > > +@cindex MIPS code compression > > Please use lower-case "mips" in the @cindex entry as well. It is a proper name, why? All the other places capitalise it correctly, e.g.: @kindex set mips mask-address @cindex MIPS addresses, masking and likewise for other proper names, e.g. AIX. > > +Tell @value{GDBN} which MIPS compressed ISA encoding is used by the > > +inferior. @value{GDBN} uses this for code disassembly and other > > +internal interpretation purposes. This setting is only referred to > > +when no executable has been associated with the debugging session or > > +the executable does not provide information about the encoding it uses. > > +Otherwise this setting is automatically updated from information > > +provided by the executable. > > + > > +Possible values of @var{arg} are @samp{mips16} and @samp{micromips}. > > +The default compressed ISA encoding is @samp{mips16}, as executables > > +containing MIPS16 code frequently are not identified as such. > > + > > +This setting is ``sticky''; that is, it retains its value across > > +debugging sessions until reset either explicitly with this command or > > +implicitly from an executable. > > + > > +The compiler and/or assembler typically add symbol table annotations to > > +identify functions compiled for the MIPS16 or microMIPS ISAs. If these > > +function-scope annotations are present, @value{GDBN} uses them in > > +preference to the global compressed ISA encoding setting. > > I would suggest to use either @acronym{ISA} or @sc{isa} (and the same > with "MIPS16" and "MIPS"), since they will look better in print. Try > both, produce the PDF version of the manual, and keep the one you like > best. Hmm, there is no existing place that uses any formatting whatever for "MIPS" or any derivatives (see e.g. "MIPS ABI" just above), except from @code{MIPS32} used in a single place to refer to a packet format. The same stands for other processor architectures or names of instruction sets. I am not trying to say that I disagree with your suggestion as is, only asking why you think I should be making these changes to my patch? FWIW, I can't tell the difference between text rendered with no formatting and the @sc macro; @acronym yields a little bit smaller text that I actually like, but as I say, it seems to make little sense to me just to apply it here where the rest of the manual does not use it. And if we're going to apply such a bulk change to the manual, then it shouldn't matter if this change has been previously included here or not. As it stands I'd prefer to keep the formatting consistent through the manual. > > +@subsubsection Breakpoint Kinds > > + > > +These breakpoint kinds are defined for the @samp{Z0} and @samp{Z1} packets. > > I'd prefer to have a @node here; subsections without a node are > possible, but are harder to find. > > Also, a @cindex entry here would be good; think about someone who'd > like to find this information quickly by searching the index. I copied the layout from the "Register Packet Format" subsubsection that immediately precedes; actually the whole section lacks any subnodes. I now recall a similar discussion a while ago about another patch; this change originally predates that discussion. Sorry for missing that. However in this case the whole section would have to be updated throughout, it does not make sense to have a subnode just for one of the subsubsections, and revamping the whole section does not belong to this change. And last but not least I am not even sure if splitting this particular section into subpages makes sense in the first place. Nesting too deep can be frustrating too, and here you'd have to have two intermediate pages with node lists only for just a couple lines' worth of nodes. I think index references would better be added separately too -- these would be "ARM Breakpoint Kinds," "MIPS Register Packet Format" and "MIPS Breakpoint Kinds," respectively. > OK with those changes. Well, as you can see I have troubles to agree, sorry. What I can do is pushing the change below on top of my microMIPS change. Or alternatively I can split it into two changes and apply changes to the existing pieces now and fold the microMIPS index entries into the microMIPS change -- whichever you might find more suitable. > > + add_setshow_enum_cmd ("compression", class_obscure, mips_compression_strings, > > + &mips_compression_string, _("\ > > +Set the compressed ISA encoding used."), _("\ > > +Show the compressed ISA encoding used."), _("\ > > I'd mention microMIPS in the doc strings, as in > > Set the compressed ISA encoding used by microMIPS binaries. Err, that's not the point of the setting in both aspects. First, it's not specific to microMIPS code as its very purpose is to switch between MIPS16 and microMIPS encoding that's mutually exclusive. Second, it's only used where no binary is available (usually selected with "file" or suchlike), e.g. ROM contents: (gdb) target remote foo (gdb) x/i $pc (gdb) stepi etc. I agree the description is a bit lacking, but I have also decided the verbose output from: (gdb) help set mips compression (gdb) help show mips compression (which is the same as I pasted in NEWS) is good enough. Do you think it is not? > > Index: gdb-fsf-trunk-quilt/gdb/NEWS > > =================================================================== > > --- gdb-fsf-trunk-quilt.orig/gdb/NEWS 2012-04-24 20:57:21.000000000 +0100 > > +++ gdb-fsf-trunk-quilt/gdb/NEWS 2012-04-24 20:58:23.645567943 +0100 > > @@ -3,6 +3,8 @@ > > > > *** Changes since GDB 7.4 > > > > +* GDB now supports debugging microMIPS binaries. > > + > > * GDB now supports reversible debugging on ARM, it allows you to > > debug basic ARM and THUMB instructions, and provides > > record/replay support. > > @@ -124,6 +126,14 @@ HP OpenVMS ia64 ia64-hp-openvms* > > > > * New options > > > > +set mips compression > > +show mips compression > > + Select the compressed ISA encoding used in functions that have no symbol > > + information available. The encoding can be set to either of: > > + mips16 > > + micromips > > + and is updated automatically from ELF file flags if available. > > + > > This is OK. Thanks for your review. Maciej 2012-04-26 Maciej W. Rozycki gdb/doc/ * gdb.texinfo (Architecture-Specific Protocol Details): Add index entries throughout. gdb-doc-remote-arch-index.diff Index: gdb-fsf-trunk-quilt/gdb/doc/gdb.texinfo =================================================================== --- gdb-fsf-trunk-quilt.orig/gdb/doc/gdb.texinfo 2012-04-26 01:56:46.885629533 +0100 +++ gdb-fsf-trunk-quilt/gdb/doc/gdb.texinfo 2012-04-26 01:57:07.065637214 +0100 @@ -36507,6 +36507,8 @@ details of XML target descriptions for e @subsection ARM @subsubsection Breakpoint Kinds +@cindex ARM breakpoint kinds +@cindex breakpoint kinds, ARM These breakpoint kinds are defined for the @samp{Z0} and @samp{Z1} packets. @@ -36526,6 +36528,8 @@ These breakpoint kinds are defined for t @subsection MIPS @subsubsection Register Packet Format +@cindex MIPS register packet format +@cindex register packet format, MIPS The following @code{g}/@code{G} packets have previously been defined. In the below, some thirty-two bit registers are transferred as @@ -36551,6 +36555,8 @@ as @code{MIPS32}. @end table @subsubsection Breakpoint Kinds +@cindex MIPS breakpoint kinds +@cindex breakpoint kinds, MIPS These breakpoint kinds are defined for the @samp{Z0} and @samp{Z1} packets.