From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 983 invoked by alias); 14 Aug 2013 14:04:52 -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 944 invoked by uid 89); 14 Aug 2013 14:04:51 -0000 X-Spam-SWARE-Status: No, score=-2.0 required=5.0 tests=AWL,BAYES_00,RCVD_IN_HOSTKARMA_NO autolearn=ham version=3.3.2 Received: from rock.gnat.com (HELO rock.gnat.com) (205.232.38.15) by sourceware.org (qpsmtpd/0.84/v0.84-167-ge50287c) with ESMTP; Wed, 14 Aug 2013 14:04:50 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id 2D7EE11667A; Wed, 14 Aug 2013 10:04:52 -0400 (EDT) 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 EUnrGQN1Y2ka; Wed, 14 Aug 2013 10:04:52 -0400 (EDT) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id D1FE6116675; Wed, 14 Aug 2013 10:04:51 -0400 (EDT) Received: by joel.gnat.com (Postfix, from userid 1000) id 3CF66C25A3; Wed, 14 Aug 2013 07:04:46 -0700 (PDT) Date: Wed, 14 Aug 2013 14:04:00 -0000 From: Joel Brobecker To: Kevin Buettner Cc: gdb-patches@sourceware.org Subject: Re: [RFC] rl78-tdep.c: Make PC a pseudo-register Message-ID: <20130814140446.GC11128@adacore.com> References: <20130808205554.070f72ce@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20130808205554.070f72ce@redhat.com> User-Agent: Mutt/1.5.21 (2010-09-15) X-SW-Source: 2013-08/txt/msg00381.txt.bz2 Hi Kevin, > I'm not tremendously fond of my patch below because I feel that there > should be a simpler way of doing things. This patch changes PC to be > a pseudo register in which three bytes are read and written to/from > the corresponding raw register. > > Comments? Can anyone think of a better way to do this? > > * rl78-tdep.c (RL78_RAW_PC_REGNUM): New enum. > (RL78_PC_REGNUM): Move to list of pseudo-register enums. > (rl78_register_type, rl78_register_name, rl78_register_reggroup_p): > Update to account for fact that PC is now a pseudo-register. > (rl78_pseudo_register_write, rl78_pseudo_register_read): Add > cases for RL78_PC_REGNUM. I am not an expert by any means, but I thought that this type of situation is usually handled the way you just did with this patch. For instance, 32bit mode with 64bit registers... > > Index: gdb/rl78-tdep.c > =================================================================== > RCS file: /cvs/src/src/gdb/rl78-tdep.c,v > retrieving revision 1.9 > diff -u -p -r1.9 rl78-tdep.c > --- gdb/rl78-tdep.c 4 May 2013 06:14:53 -0000 1.9 > +++ gdb/rl78-tdep.c 8 Aug 2013 00:02:28 -0000 > @@ -94,7 +94,7 @@ enum > RL78_PSW_REGNUM, /* 8 bits */ > RL78_ES_REGNUM, /* 8 bits */ > RL78_CS_REGNUM, /* 8 bits */ > - RL78_PC_REGNUM, /* 20 bits; we'll use 32 bits for it. */ > + RL78_RAW_PC_REGNUM, /* 20 bits; we'll use 32 bits for it. */ > > /* Fixed address SFRs (some of those above are SFRs too.) */ > RL78_SPL_REGNUM, /* 8 bits; lower half of SP */ > @@ -105,7 +105,8 @@ enum > RL78_NUM_REGS, > > /* Pseudo registers. */ > - RL78_SP_REGNUM = RL78_NUM_REGS, > + RL78_PC_REGNUM = RL78_NUM_REGS, > + RL78_SP_REGNUM, Out of curiosity, why not include RL78_SP_REGNUM in RL78_NUM_REGS? > if ((regnum < RL78_NUM_REGS > && regnum != RL78_SPL_REGNUM > - && regnum != RL78_SPH_REGNUM) > - || regnum == RL78_SP_REGNUM) > + && regnum != RL78_SPH_REGNUM > + && regnum != RL78_RAW_PC_REGNUM) > + || regnum == RL78_SP_REGNUM > + || regnum == RL78_PC_REGNUM) FYI, there is an inconsistent use of tabs vs spaces that made the review of this change a little harder... > + else if (reg == RL78_PC_REGNUM) > + { > + gdb_byte rawbuf[4]; > + status = regcache_raw_read (regcache, RL78_RAW_PC_REGNUM, rawbuf); > + memcpy (buffer, rawbuf, 3); > + } > else if (RL78_X_REGNUM <= reg && reg <= RL78_H_REGNUM) > { > ULONGEST psw; > @@ -527,6 +540,13 @@ rl78_pseudo_register_write (struct gdbar > regcache_raw_write (regcache, RL78_SPL_REGNUM, buffer); > regcache_raw_write (regcache, RL78_SPH_REGNUM, buffer + 1); > } > + else if (reg == RL78_PC_REGNUM) > + { > + gdb_byte rawbuf[4]; > + memcpy (rawbuf, buffer, 3); > + rawbuf[3] = 0; > + regcache_raw_write (regcache, RL78_RAW_PC_REGNUM, rawbuf); > + } In both hunks, you're missing an empty line after the rawbuf variable declaration (one of the many coding rules of the GDB project). Other than that, I didn't see anything obviously wrong with the patch. -- Joel