From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 29088 invoked by alias); 27 Feb 2009 19:56:17 -0000 Received: (qmail 29078 invoked by uid 22791); 27 Feb 2009 19:56:17 -0000 X-SWARE-Spam-Status: No, hits=-2.4 required=5.0 tests=AWL,BAYES_00 X-Spam-Check-By: sourceware.org Received: from rock.gnat.com (HELO rock.gnat.com) (205.232.38.15) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Fri, 27 Feb 2009 19:56:11 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id 50BE52BABE0; Fri, 27 Feb 2009 14:56:12 -0500 (EST) Received: from rock.gnat.com ([127.0.0.1]) by localhost (rock.gnat.com [127.0.0.1]) (amavisd-new, port 10024) with LMTP id 0K-WkG9rDPvE; Fri, 27 Feb 2009 14:56:12 -0500 (EST) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id C39C42BABAA; Fri, 27 Feb 2009 14:56:11 -0500 (EST) Received: by joel.gnat.com (Postfix, from userid 1000) id 205FAE7ACD; Fri, 27 Feb 2009 11:56:07 -0800 (PST) Date: Fri, 27 Feb 2009 20:36:00 -0000 From: Joel Brobecker To: Aleksandar Ristovski Cc: gdb-patches@sources.redhat.com Subject: Re: [patch] mips-tdep: info registers Message-ID: <20090227195607.GJ26056@adacore.com> References: <20090223020820.GC26056@adacore.com> <20090223025230.GA11699@caradoc.them.org> <200902230718.n1N7IoBD028396@brahms.sibelius.xs4all.nl> <20090223161756.GA19411@caradoc.them.org> <20090223162602.GH26056@adacore.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.4.2.2i 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: 2009-02/txt/msg00497.txt.bz2 Aleksandar, >Joel, the attached removes numeric value syntax, but allows it for mips; >mips will explicitly map regno to raw regno (which is currently 1-1). > >I will let you take it from here. I am afraid that I won't have time to do much more than review your change. Just reviewing changes right now is a struggle for me, so do you think you could submit them officially? I think there are two parts to this patch: > Index: gdb/infcmd.c > =================================================================== > RCS file: /cvs/src/src/gdb/infcmd.c,v > retrieving revision 1.231 > diff -u -p -r1.231 infcmd.c > --- gdb/infcmd.c 25 Jan 2009 23:35:51 -0000 1.231 > +++ gdb/infcmd.c 23 Feb 2009 18:21:54 -0000 > @@ -1948,21 +1948,6 @@ registers_info (char *addr_exp, int fpre > } > } > > - /* A register number? (how portable is this one?). */ > - { > - char *endptr; > - int regnum = strtol (start, &endptr, 0); > - if (endptr == end > - && regnum >= 0 > - && regnum < gdbarch_num_regs (gdbarch) > - + gdbarch_num_pseudo_regs (gdbarch)) > - { > - gdbarch_print_registers_info (gdbarch, gdb_stdout, > - frame, regnum, fpregs); > - continue; > - } > - } > - > /* A register group? */ > { > struct reggroup *group; This patch is fine, and approved. Please just run it through the testsuite before checking in. (can you remember to post a ChangeLog for it as well?) The mips-tdep part can be treated independently. Although I don't see any problem with it, and you updated it the same way I would have changed it, I'm not sure about going against the comment: > /* Map [gdbarch_num_regs .. 2*gdbarch_num_regs) onto the raw registers, > but then don't make the raw register names visible. */ It looks fine to return "0" ... "31" as the names of the raw registers, but I'd like someone with more experience with the mips target to confirm it. > Index: gdb/mips-tdep.c > =================================================================== > RCS file: /cvs/src/src/gdb/mips-tdep.c,v > retrieving revision 1.490 > diff -u -p -r1.490 mips-tdep.c > --- gdb/mips-tdep.c 22 Feb 2009 01:02:17 -0000 1.490 > +++ gdb/mips-tdep.c 23 Feb 2009 18:21:57 -0000 > @@ -440,13 +440,25 @@ mips_register_name (struct gdbarch *gdba > "t8", "t9", "k0", "k1", "gp", "sp", "s8", "ra" > }; > > + static char *mips_gpr_numeric_names[] = { A little short comment explaining what this is about might be useful. > + "0", "1", "2", "3", "4", "5", "6", "7", > + "8", "9", "10", "11", "12", "13", "14", "15", > + "16", "17", "18", "19", "20", "21", "22", "23", > + "24", "25", "26", "27", "28", "29", "30", "31" > + }; > + > enum mips_abi abi = mips_abi (gdbarch); > > /* Map [gdbarch_num_regs .. 2*gdbarch_num_regs) onto the raw registers, > but then don't make the raw register names visible. */ This comment needs to be removed or updated. If the comment next to mips_gpr_numeric_names is clear enough, I think it can simply be removed. > int rawnum = regno % gdbarch_num_regs (gdbarch); > if (regno < gdbarch_num_regs (gdbarch)) > - return ""; > + { > + if (regno >= 0) > + return mips_gpr_numeric_names [regno]; > + else > + return ""; > + } > > /* The MIPS integer registers are always mapped from 0 to 31. The > names of the registers (which reflects the conventions regarding > @@ -4601,7 +4613,9 @@ mips_print_registers_info (struct gdbarc > { > if (regnum != -1) /* do one specified register */ > { > - gdb_assert (regnum >= gdbarch_num_regs (gdbarch)); > + if (regnum < gdbarch_num_regs (gdbarch) > + && regnum >= 0) > + regnum += gdbarch_num_regs (gdbarch); /* Print pseudo register. */ > if (*(gdbarch_register_name (gdbarch, regnum)) == '\0') > error (_("Not a valid register for the current processor type")); > -- Joel