Re: [RFC] rl78-tdep.c: Make PC a pseudo-register

[Kevin Buettner <kevinb at redhat dot com>]

Hi Joel,

On Wed, 14 Aug 2013 07:04:46 -0700
Joel Brobecker <brobecker@adacore.com> wrote:

> 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...

Thanks for looking it over.  I'll probably end up committing
it after formatting issues that you noticed.

> > @@ -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?

Before, RL78_SP_REGNUM was the first pseudo-register.  Now,
RL78_PC_REGNUM will be the first pseudo-register and SP will be the
second.  I did it this way to preserve the register order in "info
registers".  There is less user visible change this way.

> >        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...

I'll adjust this before committing.

> > +  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).

I'll fix that too.

> Other than that, I didn't see anything obviously wrong with the patch.

Thanks again for looking it over.  Much appreciated.

Kevin