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

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

[Kevin Buettner]

I investigated a segfault for the rl78 target recently in which the
program counter was being set incorrectly when executing a return
command.  The return address is stored in a 32-bit container on the
stack, but when storing the return address, the simulator only writes
three of the four bytes due to the fact that the rl78 PC is only 20
bits wide.  The other byte is not touched and will have whatever value
was there either at initialization or whatever other value was last
written to the location as the stack grows and shrinks during program
execution.  To the best of my knowledge, actual hardware behaves the
same way.

When DWARF unwinding is used, the entire 32-bit container is read
including the high byte which may in fact contain random garbage.  I
have spent some time examining the unwinding code, but have not been
able to locate a point at which it makes sense to add a call to
gdbarch_addr_bits_remove().

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.

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,
 
   RL78_X_REGNUM,
   RL78_A_REGNUM,
@@ -243,6 +244,8 @@ rl78_register_type (struct gdbarch *gdba
 
   if (reg_nr == RL78_PC_REGNUM)
     return tdep->rl78_code_pointer;
+  else if (reg_nr == RL78_RAW_PC_REGNUM)
+    return tdep->rl78_uint32;
   else if (reg_nr <= RL78_MEM_REGNUM
            || (RL78_X_REGNUM <= reg_nr && reg_nr <= RL78_H_REGNUM)
 	   || (RL78_BANK0_R0_REGNUM <= reg_nr
@@ -298,13 +301,14 @@ rl78_register_name (struct gdbarch *gdba
     "psw",
     "es",
     "cs",
-    "pc",
+    "",
 
     "",		/* spl */
     "",		/* sph */
     "pmc",
     "mem",
 
+    "pc",
     "sp",
 
     "x",
@@ -395,8 +399,10 @@ rl78_register_reggroup_p (struct gdbarch
     {
       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)
 	return 1;
       else
 	return 0;
@@ -409,6 +415,7 @@ rl78_register_reggroup_p (struct gdbarch
       || regnum == RL78_SPH_REGNUM
       || regnum == RL78_PMC_REGNUM
       || regnum == RL78_MEM_REGNUM
+      || regnum == RL78_RAW_PC_REGNUM
       || (RL78_BANK0_RP0_REGNUM <= regnum && regnum <= RL78_BANK3_RP3_REGNUM))
     return group == system_reggroup;
 
@@ -464,6 +471,12 @@ rl78_pseudo_register_read (struct gdbarc
       if (status == REG_VALID)
 	status = regcache_raw_read (regcache, RL78_SPH_REGNUM, buffer + 1);
     }
+  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);
+    }
   else if (RL78_X_REGNUM <= reg && reg <= RL78_H_REGNUM)
     {
       ULONGEST psw;

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

[Joel Brobecker]

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

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

[Kevin Buettner]

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

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

[Kevin Buettner]

On Thu, 8 Aug 2013 20:55:54 -0700
Kevin Buettner <kevinb@redhat.com> wrote:

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

Committed (with Joel's suggested changes).

Thanks again to Joel for looking it over.

Kevin