[RFC] mips-tdep.c: pseudo-register -> raw register sign extension

[RFC] mips-tdep.c: pseudo-register -> raw register sign extension

[Kevin Buettner]

Below is another patch that fixes another problem arising from the
simulator catching UNPREDICTABLE behavior.  Again, this affects mips64
targets that are being run in 32-bit mode.

For the mipsisa64-elf target, using target_board set to
mips-sim-idt64/-EB/-mips32, it fixes 198 failures in
gdb.base/store.exp and 1 failure in gdb.cp/koenig.exp A large number
of the gdb.base/store.exp failures are cascade failures though.

The sequence which triggers this first failure in gdb.base/store.exp
is as follows:

Temporary breakpoint 4, wack_int (u=-1, v=-2) at /ironwood1/sourceware-mips/mipsisa64-elf/../src/gdb/testsuite/gdb.base/store.c:85
85	  register int l = u, r = v;
(gdb) PASS: gdb.base/store.exp: continue to wack_int
next
86	  l = add_int (l, r);
(gdb) PASS: gdb.base/store.exp: var int l; next int
print l
$9 = -1
(gdb) PASS: gdb.base/store.exp: var int l; print old l, expecting -1
print r
$10 = -2
(gdb) PASS: gdb.base/store.exp: var int l; print old r, expecting -2
set variable l = 4
(gdb) PASS: gdb.base/store.exp: var int l; setting l to 4
print l
$11 = 4
(gdb) PASS: gdb.base/store.exp: var int l; print new l, expecting 4
next
UNPREDICTABLE: PC = 0x800205bc
Quit
(gdb) FAIL: gdb.base/store.exp: var int l; next over add call

What is happening here is that `l' resides in register s0 and
the following instruction is the first of line 86:

    0x800205bc <wack_int+40>:    move    a0,s0

Note from the log above that `l' (register s0) starts out as -1, but
is changed by the testing machinery to 4.  In the course of changing
this value, mips_pseudo_register_write() is called in order to
transfer the 32-bit pseudo register for s0 to the corresponding 64-bit
raw register.  Without the patch below, only 32 bits are being
transferred, thus leaving in place the (high 32-bit) sign extension of
-1.  When the sim attempts to execute the instruction noted above, it
first checks to make sure that the sign extension for the register
being transferred is sane.  It is not, and therefore quits printing
the UNPREDICTABLE message.  This ends up causing the first failure as
well as the resulting cascade since GDB prints the following message
for many of the later commands that it tries to execute:  "Cannot
execute this command while the selected thread is running. 

Comments?

Kevin


	* mips-tdep.c (mips_pseudo_register_write): Sign extend 32-bit
	cooked values that are being transferred to 64-bit raw registers.

Index: mips-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/mips-tdep.c,v
retrieving revision 1.508
diff -u -p -r1.508 mips-tdep.c
--- mips-tdep.c	28 Nov 2010 04:31:24 -0000	1.508
+++ mips-tdep.c	7 Dec 2010 21:58:58 -0000
@@ -582,11 +582,19 @@ mips_pseudo_register_write (struct gdbar
   else if (register_size (gdbarch, rawnum) >
 	   register_size (gdbarch, cookednum))
     {
-      if (gdbarch_tdep (gdbarch)->mips64_transfers_32bit_regs_p
-	  || gdbarch_byte_order (gdbarch) == BFD_ENDIAN_LITTLE)
+      if (gdbarch_tdep (gdbarch)->mips64_transfers_32bit_regs_p)
 	regcache_raw_write_part (regcache, rawnum, 0, 4, buf);
       else
-	regcache_raw_write_part (regcache, rawnum, 4, 4, buf);
+	{
+	  /* Sign extend the shortened version of the register prior
+	     to placing it in the raw register.  This is required for
+	     some mips64 parts in order to avoid unpredictable behavior.  */
+	  gdb_byte buf8[8];
+	  enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
+	  LONGEST regval = extract_signed_integer (buf, 4, byte_order);
+	  store_signed_integer (buf8, 8, byte_order, regval);
+	  regcache_raw_write (regcache, rawnum, buf8);
+	}
     }
   else
     internal_error (__FILE__, __LINE__, _("bad register size"));

Re: [RFC] mips-tdep.c: pseudo-register -> raw register sign extension

[Mark Kettenis]

> Date: Tue, 7 Dec 2010 16:47:37 -0700
> From: Kevin Buettner <kevinb@redhat.com>
> 
> Below is another patch that fixes another problem arising from the
> simulator catching UNPREDICTABLE behavior.  Again, this affects mips64
> targets that are being run in 32-bit mode.
>
> ...
>
> What is happening here is that `l' resides in register s0 and
> the following instruction is the first of line 86:
> 
>     0x800205bc <wack_int+40>:    move    a0,s0
> 
> Note from the log above that `l' (register s0) starts out as -1, but
> is changed by the testing machinery to 4.  In the course of changing
> this value, mips_pseudo_register_write() is called in order to
> transfer the 32-bit pseudo register for s0 to the corresponding 64-bit
> raw register.  Without the patch below, only 32 bits are being
> transferred, thus leaving in place the (high 32-bit) sign extension of
> -1.  When the sim attempts to execute the instruction noted above, it
> first checks to make sure that the sign extension for the register
> being transferred is sane.  It is not, and therefore quits printing
> the UNPREDICTABLE message.  This ends up causing the first failure as
> well as the resulting cascade since GDB prints the following message
> for many of the later commands that it tries to execute:  "Cannot
> execute this command while the selected thread is running. 
> 
> Comments?

Sortof destroys the symmetry between pesudo_register_read and
pseudo_register_write.

> 	* mips-tdep.c (mips_pseudo_register_write): Sign extend 32-bit
> 	cooked values that are being transferred to 64-bit raw registers.
> 
> Index: mips-tdep.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/mips-tdep.c,v
> retrieving revision 1.508
> diff -u -p -r1.508 mips-tdep.c
> --- mips-tdep.c	28 Nov 2010 04:31:24 -0000	1.508
> +++ mips-tdep.c	7 Dec 2010 21:58:58 -0000
> @@ -582,11 +582,19 @@ mips_pseudo_register_write (struct gdbar
>    else if (register_size (gdbarch, rawnum) >
>  	   register_size (gdbarch, cookednum))
>      {
> -      if (gdbarch_tdep (gdbarch)->mips64_transfers_32bit_regs_p
> -	  || gdbarch_byte_order (gdbarch) == BFD_ENDIAN_LITTLE)
> +      if (gdbarch_tdep (gdbarch)->mips64_transfers_32bit_regs_p)
>  	regcache_raw_write_part (regcache, rawnum, 0, 4, buf);
>        else
> -	regcache_raw_write_part (regcache, rawnum, 4, 4, buf);
> +	{
> +	  /* Sign extend the shortened version of the register prior
> +	     to placing it in the raw register.  This is required for
> +	     some mips64 parts in order to avoid unpredictable behavior.  */
> +	  gdb_byte buf8[8];
> +	  enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
> +	  LONGEST regval = extract_signed_integer (buf, 4, byte_order);
> +	  store_signed_integer (buf8, 8, byte_order, regval);
> +	  regcache_raw_write (regcache, rawnum, buf8);

How about using regcache_raw_write_signed() here?  I think that
simplifies the code enough to change mips_pseudo_register_read() to
use regcache_raw_read_signed() and get rid of the explicit endian-ness
check as well.

Re: [RFC] mips-tdep.c: pseudo-register -> raw register sign extension

[Kevin Buettner]

On Thu, 9 Dec 2010 12:19:00 +0100 (CET)
Mark Kettenis <mark.kettenis@xs4all.nl> wrote:

> > +	  LONGEST regval = extract_signed_integer (buf, 4, byte_order);
> > +	  store_signed_integer (buf8, 8, byte_order, regval);
> > +	  regcache_raw_write (regcache, rawnum, buf8);
> 
> How about using regcache_raw_write_signed() here?  I think that
> simplifies the code enough to change mips_pseudo_register_read() to
> use regcache_raw_read_signed() and get rid of the explicit endian-ness
> check as well.

I like this suggestion.  See below for a revised patch...

Kevin

	* mips-tdep.c (mips_pseudo_register_write): Sign extend 32-bit
	cooked values that are being transferred to 64-bit raw registers.
	(mips_pseudo_register_read): Revise to preserve symmetry with
	mips_pseudo_register_write().

Index: mips-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/mips-tdep.c,v
retrieving revision 1.508
diff -u -p -r1.508 mips-tdep.c
--- mips-tdep.c	28 Nov 2010 04:31:24 -0000	1.508
+++ mips-tdep.c	9 Dec 2010 23:03:02 -0000
@@ -559,11 +559,15 @@ mips_pseudo_register_read (struct gdbarc
   else if (register_size (gdbarch, rawnum) >
 	   register_size (gdbarch, cookednum))
     {
-      if (gdbarch_tdep (gdbarch)->mips64_transfers_32bit_regs_p
-	  || gdbarch_byte_order (gdbarch) == BFD_ENDIAN_LITTLE)
+      if (gdbarch_tdep (gdbarch)->mips64_transfers_32bit_regs_p)
 	regcache_raw_read_part (regcache, rawnum, 0, 4, buf);
       else
-	regcache_raw_read_part (regcache, rawnum, 4, 4, buf);
+	{
+	  enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
+	  LONGEST regval;
+	  regcache_raw_read_signed (regcache, rawnum, &regval);
+	  store_signed_integer (buf, 4, byte_order, regval);
+	}
     }
   else
     internal_error (__FILE__, __LINE__, _("bad register size"));
@@ -582,11 +586,17 @@ mips_pseudo_register_write (struct gdbar
   else if (register_size (gdbarch, rawnum) >
 	   register_size (gdbarch, cookednum))
     {
-      if (gdbarch_tdep (gdbarch)->mips64_transfers_32bit_regs_p
-	  || gdbarch_byte_order (gdbarch) == BFD_ENDIAN_LITTLE)
+      if (gdbarch_tdep (gdbarch)->mips64_transfers_32bit_regs_p)
 	regcache_raw_write_part (regcache, rawnum, 0, 4, buf);
       else
-	regcache_raw_write_part (regcache, rawnum, 4, 4, buf);
+	{
+	  /* Sign extend the shortened version of the register prior
+	     to placing it in the raw register.  This is required for
+	     some mips64 parts in order to avoid unpredictable behavior.  */
+	  enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
+	  LONGEST regval = extract_signed_integer (buf, 4, byte_order);
+	  regcache_raw_write_signed (regcache, rawnum, regval);
+	}
     }
   else
     internal_error (__FILE__, __LINE__, _("bad register size"));

Re: [RFC] mips-tdep.c: pseudo-register -> raw register sign extension

[Kevin Buettner]

On Thu, 9 Dec 2010 16:20:00 -0700
Kevin Buettner <kevinb@redhat.com> wrote:

> 	* mips-tdep.c (mips_pseudo_register_write): Sign extend 32-bit
> 	cooked values that are being transferred to 64-bit raw registers.
> 	(mips_pseudo_register_read): Revise to preserve symmetry with
> 	mips_pseudo_register_write().

Committed.

Kevin