[RFC] mips-tdep.c: Sign extend values placed into registers for inferior function calls

[RFC] mips-tdep.c: Sign extend values placed into registers for inferior function calls

[Kevin Buettner]

The patch below fixes a significant number of failures for mips64
targets running in 32-bit mode.  In recent testing of mipsisa64-elf
with the target_board set to mips-sim-idt64/-EB/-mips32, I saw 529
fewer failures, though, admittedly, many of these are cascade
failures.

Here is an example, taken from the log file, of one failure fixed by
this patch:

    p t_short_values(10,-23)
    UNPREDICTABLE: PC = 0x800209d4
    Quit
    (gdb) FAIL: gdb.base/callfuncs.exp: p t_short_values(10,-23)

In creating the inferior function call, gdb is placing a negative
value, -23, sign extended only to 32-bits, into one of the 64-bit
argument registers.  When the simulator executes a move instruction
in the course of executing t_short_values(), it first checks to make
sure that the register value is correctly sign extended.  I.e. it looks
at bits 32-63 and makes sure that they all match the value in bit 31. 
If it's not correctly sign extended, it outputs the "UNPREDICTABLE"
message.  It then aborts back to GDB, often causing further problems
for later tests.

The following tests are all affected by this bug.  (There are many
other failures too, but most are due to GDB attempting to perform the
requested operation while it still thinks the target is running.)  Note
that some of these tests are listed as PASS.  There's still a problem
though.  Subsequent tests fail due to the abort caused due to the
incorrect sign extension.

FAIL: gdb.base/call-ar-st.exp: print sum_array_print(10, *list1, *list2, *list3, *list4)
FAIL: gdb.base/call-rt-st.exp: print print_struct_rep(*struct1)
FAIL: gdb.base/callfuncs.exp: p t_short_values(10,-23)
PASS: gdb.base/structs.exp: call Fun<n>(foo<n>); call 5 structs-tc
PASS: gdb.base/structs.exp: call Fun<n>(foo<n>); call 3 structs-ts
PASS: gdb.base/structs.exp: call Fun<n>(foo<n>); call 2 structs-ti
PASS: gdb.base/structs.exp: call Fun<n>(foo<n>); call 2 structs-tl
PASS: gdb.base/structs.exp: call Fun<n>(foo<n>); call 2 structs-tll
PASS: gdb.base/structs.exp: call Fun<n>(foo<n>); call 2 structs-tf
PASS: gdb.base/structs.exp: call Fun<n>(foo<n>); call 2 structs-td
PASS: gdb.base/structs.exp: call Fun<n>(foo<n>); call 2 structs-tld
PASS: gdb.base/structs.exp: call Fun<n>(foo<n>); call 4 structs-ts-tc
PASS: gdb.base/structs.exp: call Fun<n>(foo<n>); call 2 structs-ti-tc
PASS: gdb.base/structs.exp: call Fun<n>(foo<n>); call 2 structs-tl-tc
PASS: gdb.base/structs.exp: call Fun<n>(foo<n>); call 2 structs-tll-tc
PASS: gdb.base/structs.exp: call Fun<n>(foo<n>); call 2 structs-tf-tc
PASS: gdb.base/structs.exp: call Fun<n>(foo<n>); call 2 structs-td-tc
PASS: gdb.base/structs.exp: call Fun<n>(foo<n>); call 2 structs-tld-tc
PASS: gdb.base/structs.exp: call Fun<n>(foo<n>); call 3 structs-tc-ts
PASS: gdb.base/structs.exp: call Fun<n>(foo<n>); call 2 structs-tc-ti
PASS: gdb.base/structs.exp: call Fun<n>(foo<n>); call 2 structs-tc-tl
PASS: gdb.base/structs.exp: call Fun<n>(foo<n>); call 2 structs-tc-tll
PASS: gdb.base/structs.exp: call Fun<n>(foo<n>); call 2 structs-tc-tf
PASS: gdb.base/structs.exp: call Fun<n>(foo<n>); call 2 structs-tc-td
PASS: gdb.base/structs.exp: call Fun<n>(foo<n>); call 2 structs-tc-tld
PASS: gdb.base/structs.exp: call Fun<n>(foo<n>); call 2 structs-td-tf
PASS: gdb.base/structs.exp: call Fun<n>(foo<n>); call 2 structs-tf-td
FAIL: gdb.cp/classes.exp: call class_param.Aval_a (g_A)
FAIL: gdb.cp/virtfunc.exp: print pDe->vg()

I think the patch below is almost obvious.  We all know by now that
mips values usually need to be sign extended.

I should note too that there are several other instances in
mips-tdep.c where unsigned operations are still performed.  I'll
address some of those problems in other patches soon to be posted, but
only for those cases where I can demonstrate that a failure is fixed
as a result.

Comments?

Kevin

	* mips-tdep.c (mips_eabi_push_dummy_call): Place signed, rather
	than unsigned, values in 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 19:52:14 -0000
@@ -2755,7 +2755,7 @@ mips_eabi_push_dummy_call (struct gdbarc
 	fprintf_unfiltered (gdb_stdlog,
 			    "mips_eabi_push_dummy_call: struct_return reg=%d %s\n",
 			    argreg, paddress (gdbarch, struct_addr));
-      regcache_cooked_write_unsigned (regcache, argreg++, struct_addr);
+      regcache_cooked_write_signed (regcache, argreg++, struct_addr);
     }
 
   /* Now load as many as possible of the first arguments into
@@ -2834,7 +2834,7 @@ mips_eabi_push_dummy_call (struct gdbarc
 	      if (mips_debug)
 		fprintf_unfiltered (gdb_stdlog, " - fpreg=%d val=%s",
 				    float_argreg, phex (regval, 4));
-	      regcache_cooked_write_unsigned (regcache, float_argreg++, regval);
+	      regcache_cooked_write_signed (regcache, float_argreg++, regval);
 
 	      /* Write the high word of the double to the odd register(s).  */
 	      regval = extract_unsigned_integer (val + 4 - low_offset,
@@ -2842,7 +2842,7 @@ mips_eabi_push_dummy_call (struct gdbarc
 	      if (mips_debug)
 		fprintf_unfiltered (gdb_stdlog, " - fpreg=%d val=%s",
 				    float_argreg, phex (regval, 4));
-	      regcache_cooked_write_unsigned (regcache, float_argreg++, regval);
+	      regcache_cooked_write_signed (regcache, float_argreg++, regval);
 	    }
 	  else
 	    {
@@ -2854,7 +2854,7 @@ mips_eabi_push_dummy_call (struct gdbarc
 	      if (mips_debug)
 		fprintf_unfiltered (gdb_stdlog, " - fpreg=%d val=%s",
 				    float_argreg, phex (regval, len));
-	      regcache_cooked_write_unsigned (regcache, float_argreg++, regval);
+	      regcache_cooked_write_signed (regcache, float_argreg++, regval);
 	    }
 	}
       else
@@ -2937,13 +2937,13 @@ mips_eabi_push_dummy_call (struct gdbarc
 		  && !fp_register_arg_p (gdbarch, typecode, arg_type))
 		{
 		  LONGEST regval =
-		    extract_unsigned_integer (val, partial_len, byte_order);
+		    extract_signed_integer (val, partial_len, byte_order);
 
 		  if (mips_debug)
 		    fprintf_filtered (gdb_stdlog, " - reg=%d val=%s",
 				      argreg,
 				      phex (regval, regsize));
-		  regcache_cooked_write_unsigned (regcache, argreg, regval);
+		  regcache_cooked_write_signed (regcache, argreg, regval);
 		  argreg++;
 		}
 

Re: [RFC] mips-tdep.c: Sign extend values placed into registers for inferior function calls

[Mark Kettenis]

> Date: Tue, 7 Dec 2010 13:28:06 -0700
> From: Kevin Buettner <kevinb@redhat.com>
> 
> Here is an example, taken from the log file, of one failure fixed by
> this patch:
> 
>     p t_short_values(10,-23)
>     UNPREDICTABLE: PC = 0x800209d4
>     Quit
>     (gdb) FAIL: gdb.base/callfuncs.exp: p t_short_values(10,-23)
> 
> In creating the inferior function call, gdb is placing a negative
> value, -23, sign extended only to 32-bits, into one of the 64-bit
> argument registers.  When the simulator executes a move instruction
> in the course of executing t_short_values(), it first checks to make
> sure that the register value is correctly sign extended.  I.e. it looks
> at bits 32-63 and makes sure that they all match the value in bit 31. 
> If it's not correctly sign extended, it outputs the "UNPREDICTABLE"
> message.  It then aborts back to GDB, often causing further problems
> for later tests.

So do I understand correctly that this is a problem with the
simulator, and that real hardware doesn't really care what is in the
unused upper half of the register?

Did you test this on real hardware?

> Comments?
> 
> Kevin
> 
> 	* mips-tdep.c (mips_eabi_push_dummy_call): Place signed, rather
> 	than unsigned, values in 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 19:52:14 -0000
> @@ -2755,7 +2755,7 @@ mips_eabi_push_dummy_call (struct gdbarc
>  	fprintf_unfiltered (gdb_stdlog,
>  			    "mips_eabi_push_dummy_call: struct_return reg=%d %s\n",
>  			    argreg, paddress (gdbarch, struct_addr));
> -      regcache_cooked_write_unsigned (regcache, argreg++, struct_addr);
> +      regcache_cooked_write_signed (regcache, argreg++, struct_addr);

This one makes sense, func_addr and bp_addr are already written signed.

> @@ -2834,7 +2834,7 @@ mips_eabi_push_dummy_call (struct gdbarc
>  	      if (mips_debug)
>  		fprintf_unfiltered (gdb_stdlog, " - fpreg=%d val=%s",
>  				    float_argreg, phex (regval, 4));
> -	      regcache_cooked_write_unsigned (regcache, float_argreg++, regval);
> +	      regcache_cooked_write_signed (regcache, float_argreg++, regval);
>  
>  	      /* Write the high word of the double to the odd register(s).  */
>  	      regval = extract_unsigned_integer (val + 4 - low_offset,
> @@ -2842,7 +2842,7 @@ mips_eabi_push_dummy_call (struct gdbarc
>  	      if (mips_debug)
>  		fprintf_unfiltered (gdb_stdlog, " - fpreg=%d val=%s",
>  				    float_argreg, phex (regval, 4));
> -	      regcache_cooked_write_unsigned (regcache, float_argreg++, regval);
> +	      regcache_cooked_write_signed (regcache, float_argreg++, regval);

These two are odd though, since the values are still read using
extract_integer_unsigned.  That's a bit odd.

>  	    }
>  	  else
>  	    {
> @@ -2854,7 +2854,7 @@ mips_eabi_push_dummy_call (struct gdbarc
>  	      if (mips_debug)
>  		fprintf_unfiltered (gdb_stdlog, " - fpreg=%d val=%s",
>  				    float_argreg, phex (regval, len));
> -	      regcache_cooked_write_unsigned (regcache, float_argreg++, regval);
> +	      regcache_cooked_write_signed (regcache, float_argreg++, regval);

same here

> @@ -2937,13 +2937,13 @@ mips_eabi_push_dummy_call (struct gdbarc
>  		  && !fp_register_arg_p (gdbarch, typecode, arg_type))
>  		{
>  		  LONGEST regval =
> -		    extract_unsigned_integer (val, partial_len, byte_order);
> +		    extract_signed_integer (val, partial_len, byte_order);
>  
>  		  if (mips_debug)
>  		    fprintf_filtered (gdb_stdlog, " - reg=%d val=%s",
>  				      argreg,
>  				      phex (regval, regsize));
> -		  regcache_cooked_write_unsigned (regcache, argreg, regval);
> +		  regcache_cooked_write_signed (regcache, argreg, regval);
>  		  argreg++;
>  		}
>  
> 
> 

Re: [RFC] mips-tdep.c: Sign extend values placed into registers for inferior function calls

[Kevin Buettner]

On Tue, 7 Dec 2010 22:45:26 +0100 (CET)
Mark Kettenis <mark.kettenis@xs4all.nl> wrote:

> > Here is an example, taken from the log file, of one failure fixed by
> > this patch:
> > 
> >     p t_short_values(10,-23)
> >     UNPREDICTABLE: PC = 0x800209d4
> >     Quit
> >     (gdb) FAIL: gdb.base/callfuncs.exp: p t_short_values(10,-23)
> > 
> > In creating the inferior function call, gdb is placing a negative
> > value, -23, sign extended only to 32-bits, into one of the 64-bit
> > argument registers.  When the simulator executes a move instruction
> > in the course of executing t_short_values(), it first checks to make
> > sure that the register value is correctly sign extended.  I.e. it looks
> > at bits 32-63 and makes sure that they all match the value in bit 31. 
> > If it's not correctly sign extended, it outputs the "UNPREDICTABLE"
> > message.  It then aborts back to GDB, often causing further problems
> > for later tests.
> 
> So do I understand correctly that this is a problem with the
> simulator, and that real hardware doesn't really care what is in the
> unused upper half of the register?

I suspect that most real hardware would not care.  Here's what a comment
in the mips simulator has to say about the matter:

   This function implements what the MIPS32 and MIPS64 ISAs define as
   "UNPREDICTABLE" behaviour.

   About UNPREDICTABLE behaviour they say: "UNPREDICTABLE results
   may vary from processor implementation to processor implementation,
   instruction to instruction, or as a function of time on the same
   implementation or instruction.  Software can never depend on results
   that are UNPREDICTABLE. ..."  (MIPS64 Architecture for Programmers
   Volume II, The MIPS64 Instruction Set.  MIPS Document MD00087 revision
   0.95, page 2.)
  
   For UNPREDICTABLE behaviour, we print a message, if possible print
   the offending instructions mips.igen instruction name (provided by
   the caller), and stop the simulator.

> Did you test this on real hardware?

I have, though not recently.  I don't recall running into this issue
on the hardware that I used for testing.

> > @@ -2834,7 +2834,7 @@ mips_eabi_push_dummy_call (struct gdbarc
> >  	      if (mips_debug)
> >  		fprintf_unfiltered (gdb_stdlog, " - fpreg=%d val=%s",
> >  				    float_argreg, phex (regval, 4));
> > -	      regcache_cooked_write_unsigned (regcache, float_argreg++, regval);
> > +	      regcache_cooked_write_signed (regcache, float_argreg++, regval);
> >  
> >  	      /* Write the high word of the double to the odd register(s).  */
> >  	      regval = extract_unsigned_integer (val + 4 - low_offset,
> > @@ -2842,7 +2842,7 @@ mips_eabi_push_dummy_call (struct gdbarc
> >  	      if (mips_debug)
> >  		fprintf_unfiltered (gdb_stdlog, " - fpreg=%d val=%s",
> >  				    float_argreg, phex (regval, 4));
> > -	      regcache_cooked_write_unsigned (regcache, float_argreg++, regval);
> > +	      regcache_cooked_write_signed (regcache, float_argreg++, regval);
> 
> These two are odd though, since the values are still read using
> extract_integer_unsigned.  That's a bit odd.

As I recall, back when I wrote this patch, my conclusion at that time
was that it didn't matter whether extract_integer_unsigned() or
extract_integer_signed() was used.  But I do agree that it looks odd. 
I've amended my patch to use extract_signed_integer instead.  See
below for the amended patch.  (I don't see any change in test results
using the revised patch.)

Thanks for looking it over...

Kevin

	* mips-tdep.c (mips_eabi_push_dummy_call): Place signed, rather
	than unsigned, values in 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 22:24:13 -0000
@@ -2826,23 +2834,23 @@ mips_eabi_push_dummy_call (struct gdbarc
 	    {
 	      int low_offset = gdbarch_byte_order (gdbarch)
 			       == BFD_ENDIAN_BIG ? 4 : 0;
-	      unsigned long regval;
+	      long regval;
 
 	      /* Write the low word of the double to the even register(s).  */
-	      regval = extract_unsigned_integer (val + low_offset,
-						 4, byte_order);
+	      regval = extract_signed_integer (val + low_offset,
+					       4, byte_order);
 	      if (mips_debug)
 		fprintf_unfiltered (gdb_stdlog, " - fpreg=%d val=%s",
 				    float_argreg, phex (regval, 4));
-	      regcache_cooked_write_unsigned (regcache, float_argreg++, regval);
+	      regcache_cooked_write_signed (regcache, float_argreg++, regval);
 
 	      /* Write the high word of the double to the odd register(s).  */
-	      regval = extract_unsigned_integer (val + 4 - low_offset,
-						 4, byte_order);
+	      regval = extract_signed_integer (val + 4 - low_offset,
+					       4, byte_order);
 	      if (mips_debug)
 		fprintf_unfiltered (gdb_stdlog, " - fpreg=%d val=%s",
 				    float_argreg, phex (regval, 4));
-	      regcache_cooked_write_unsigned (regcache, float_argreg++, regval);
+	      regcache_cooked_write_signed (regcache, float_argreg++, regval);
 	    }
 	  else
 	    {
@@ -2850,11 +2858,11 @@ mips_eabi_push_dummy_call (struct gdbarc
 	         in a single register.  */
 	      /* On 32 bit ABI's the float_argreg is further adjusted
 	         above to ensure that it is even register aligned.  */
-	      LONGEST regval = extract_unsigned_integer (val, len, byte_order);
+	      LONGEST regval = extract_signed_integer (val, len, byte_order);
 	      if (mips_debug)
 		fprintf_unfiltered (gdb_stdlog, " - fpreg=%d val=%s",
 				    float_argreg, phex (regval, len));
-	      regcache_cooked_write_unsigned (regcache, float_argreg++, regval);
+	      regcache_cooked_write_signed (regcache, float_argreg++, regval);
 	    }
 	}
       else
@@ -2937,13 +2945,13 @@ mips_eabi_push_dummy_call (struct gdbarc
 		  && !fp_register_arg_p (gdbarch, typecode, arg_type))
 		{
 		  LONGEST regval =
-		    extract_unsigned_integer (val, partial_len, byte_order);
+		    extract_signed_integer (val, partial_len, byte_order);
 
 		  if (mips_debug)
 		    fprintf_filtered (gdb_stdlog, " - reg=%d val=%s",
 				      argreg,
 				      phex (regval, regsize));
-		  regcache_cooked_write_unsigned (regcache, argreg, regval);
+		  regcache_cooked_write_signed (regcache, argreg, regval);
 		  argreg++;
 		}
 

Re: [RFC] mips-tdep.c: Sign extend values placed into registers for inferior function calls

[Kevin Buettner]

On Tue, 7 Dec 2010 15:38:19 -0700
Kevin Buettner <kevinb@redhat.com> wrote:

> 	* mips-tdep.c (mips_eabi_push_dummy_call): Place signed, rather
> 	than unsigned, values in registers.

Committed.

Kevin