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

[Joel Brobecker <brobecker@adacore.com>]

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