mips-tdep.c: FP varargs fixes

mips-tdep.c: FP varargs fixes

[Maciej W. Rozycki]

Hello,

 The following changes fix the following test case failure:

print find_max_double(5,1.0,17.0,2.0,3.0,4.0)
find_max(5, -0.000000, 0.000000, 0.000000, 0.000000, 0.000000) returns 0.000000
$5 = 5.3101701311997171e-315
(gdb) FAIL: gdb.base/varargs.exp: print find_max_double(5,1.0,17.0,2.0,3.0,4.0)

This change has been tested natively for mips-unknown-linux-gnu and 
remotely for mipsisa32-sde-elf, using mips-sim-sde32/-EB and 
mips-sim-sde32/-EL as the targets, removing the failure for all.

2007-03-23  Nigel Stephens  <nigel@mips.com>
            Maciej W. Rozycki  <macro@mips.com>

	* mips-tdep.c (mips_o32_push_dummy_call): Take account of
	argument alignment requirements when calculating stack space
	required.  When aligning an arg register to eight bytes
	boundary, align stack_offset too.  Write floating-point
	arguments to the appropriate integer register, if we're not
	passing in a floating point register.

 OK to apply?

  Maciej

12100-0.diff
Index: binutils-quilt/src/gdb/mips-tdep.c
===================================================================
--- binutils-quilt.orig/src/gdb/mips-tdep.c	2007-03-23 14:21:18.000000000 +0000
+++ binutils-quilt/src/gdb/mips-tdep.c	2007-03-23 14:25:57.000000000 +0000
@@ -3071,8 +3071,16 @@
 
   /* Now make space on the stack for the args.  */
   for (argnum = 0; argnum < nargs; argnum++)
-    len += align_up (TYPE_LENGTH (value_type (args[argnum])),
-		     mips_stack_argsize (gdbarch));
+    {
+      struct type *arg_type = check_typedef (value_type (args[argnum]));
+      int arglen = TYPE_LENGTH (arg_type);
+      /* Align to double-word if necessary.  */
+      if (mips_abi_regsize (gdbarch) < 8
+	  && mips_type_needs_double_align (arg_type))
+	len = align_up (len, mips_stack_argsize (gdbarch) * 2);
+      /* Allocate space on the stack.  */
+      len += align_up (arglen, mips_stack_argsize (gdbarch));
+    }
   sp -= align_up (len, 16);
 
   if (mips_debug)
@@ -3208,7 +3216,10 @@
 	      && mips_type_needs_double_align (arg_type))
 	    {
 	      if ((argreg & 1))
-		argreg++;
+		{
+		  argreg++;
+		  stack_offset += mips_abi_regsize (gdbarch);
+		}
 	    }
 	  /* Note: Floating-point values that didn't fit into an FP
 	     register are only written to memory.  */
@@ -3272,7 +3283,7 @@
 	      /* Write this portion of the argument to a general
 	         purpose register.  */
 	      if (argreg <= MIPS_LAST_ARG_REGNUM
-		  && !fp_register_arg_p (typecode, arg_type))
+		  /*&& !fp_register_arg_p (typecode, arg_type)*/)
 		{
 		  LONGEST regval = extract_signed_integer (val, partial_len);
 		  /* Value may need to be sign extended, because

Re: mips-tdep.c: FP varargs fixes

[Mark Kettenis]

> Date: Fri, 23 Mar 2007 14:41:16 +0000 (GMT)
> From: "Maciej W. Rozycki" <macro@mips.com>

> @@ -3272,7 +3283,7 @@
>  	      /* Write this portion of the argument to a general
>  	         purpose register.  */
>  	      if (argreg <= MIPS_LAST_ARG_REGNUM
> -		  && !fp_register_arg_p (typecode, arg_type))
> +		  /*&& !fp_register_arg_p (typecode, arg_type)*/)
>  		{
>  		  LONGEST regval = extract_signed_integer (val, partial_len);
>  		  /* Value may need to be sign extended, because
> 

What's up with this bit?  Your ChangeLog message says this:

> 	boundary, align stack_offset too.  Write floating-point
> 	arguments to the appropriate integer register, if we're not
> 	passing in a floating point register.

But that matches the old code, not the new code.

In any case please don't comment out code like that.  The code is
either right or wring.  In the latter case it should be removed.  If
there is something non-obvious going on, it needs a proper comment.

Thanks,

Mark

Re: mips-tdep.c: FP varargs fixes

[Maciej W. Rozycki]

On Fri, 23 Mar 2007, Mark Kettenis wrote:

> > @@ -3272,7 +3283,7 @@
> >  	      /* Write this portion of the argument to a general
> >  	         purpose register.  */
> >  	      if (argreg <= MIPS_LAST_ARG_REGNUM
> > -		  && !fp_register_arg_p (typecode, arg_type))
> > +		  /*&& !fp_register_arg_p (typecode, arg_type)*/)
> >  		{
> >  		  LONGEST regval = extract_signed_integer (val, partial_len);
> >  		  /* Value may need to be sign extended, because
> > 
> 
> What's up with this bit?  Your ChangeLog message says this:
> 
> > 	boundary, align stack_offset too.  Write floating-point
> > 	arguments to the appropriate integer register, if we're not
> > 	passing in a floating point register.
> 
> But that matches the old code, not the new code.

 That's unfortunate wording, but I took this chance as an opportunity to 
further review the patch and here is the result.  The current code is 
incorrect as is, because for the O32 ABI once two FP arguments have been 
passed in floating point registers or any non-FP arguments have been 
passed in general registers, but the amount of data passed so far has not 
reached four 32-bit words (which may be the case if single floats are 
involved) further FP arguments are passed in general registers.  Only once 
four 32-bit words have been used, further arguments are passed on the 
stack.

 I have now adjusted mips_o64_push_dummy_call() accordingly and rephrased 
the text in the ChangeLog entry for clarity.

 This new patch has been tested natively for mips-unknown-linux-gnu and 
remotely for mipsisa32-sde-elf, using mips-sim-sde32/-EB and 
mips-sim-sde32/-EL as the targets.

2007-03-26  Maciej W. Rozycki  <macro@mips.com>
            Nigel Stephens  <nigel@mips.com>

	* mips-tdep.c (mips_o32_push_dummy_call): Take account of
	argument alignment requirements when calculating stack space
	required.  When aligning an arg register to eight bytes
	boundary, align stack_offset too.  Write floating-point
	arguments to the appropriate integer register if need go there.
	(mips_o64_push_dummy_call): Likewise.

 It looks like the calls to mips_abi_regsize() within are now redundant in 
these functions, because the result is implied by the very fact of calling 
either of these functions, but this is functionally independent from this 
change so I think it should be dealt with separately.

 OK to apply?

  Maciej

12100-0.diff
Index: binutils-quilt/src/gdb/mips-tdep.c
===================================================================
--- binutils-quilt.orig/src/gdb/mips-tdep.c	2007-03-26 14:16:14.000000000 +0100
+++ binutils-quilt/src/gdb/mips-tdep.c	2007-03-26 15:46:29.000000000 +0100
@@ -3071,8 +3071,17 @@
 
   /* Now make space on the stack for the args.  */
   for (argnum = 0; argnum < nargs; argnum++)
-    len += align_up (TYPE_LENGTH (value_type (args[argnum])),
-		     mips_stack_argsize (gdbarch));
+    {
+      struct type *arg_type = check_typedef (value_type (args[argnum]));
+      int arglen = TYPE_LENGTH (arg_type);
+
+      /* Align to double-word if necessary.  */
+      if (mips_abi_regsize (gdbarch) < 8
+	  && mips_type_needs_double_align (arg_type))
+	len = align_up (len, mips_stack_argsize (gdbarch) * 2);
+      /* Allocate space on the stack.  */
+      len += align_up (arglen, mips_stack_argsize (gdbarch));
+    }
   sp -= align_up (len, 16);
 
   if (mips_debug)
@@ -3208,10 +3217,11 @@
 	      && mips_type_needs_double_align (arg_type))
 	    {
 	      if ((argreg & 1))
-		argreg++;
+		{
+		  argreg++;
+		  stack_offset += mips_abi_regsize (gdbarch);
+		}
 	    }
-	  /* Note: Floating-point values that didn't fit into an FP
-	     register are only written to memory.  */
 	  while (len > 0)
 	    {
 	      /* Remember if the argument was written to the stack.  */
@@ -3225,8 +3235,7 @@
 
 	      /* Write this portion of the argument to the stack.  */
 	      if (argreg > MIPS_LAST_ARG_REGNUM
-		  || odd_sized_struct
-		  || fp_register_arg_p (typecode, arg_type))
+		  || odd_sized_struct)
 		{
 		  /* Should shorter than int integer values be
 		     promoted to int before being stored? */
@@ -3267,12 +3276,10 @@
 		}
 
 	      /* Note!!! This is NOT an else clause.  Odd sized
-	         structs may go thru BOTH paths.  Floating point
-	         arguments will not.  */
+	         structs may go thru BOTH paths.  */
 	      /* Write this portion of the argument to a general
 	         purpose register.  */
-	      if (argreg <= MIPS_LAST_ARG_REGNUM
-		  && !fp_register_arg_p (typecode, arg_type))
+	      if (argreg <= MIPS_LAST_ARG_REGNUM)
 		{
 		  LONGEST regval = extract_signed_integer (val, partial_len);
 		  /* Value may need to be sign extended, because
@@ -3525,8 +3532,17 @@
 
   /* Now make space on the stack for the args.  */
   for (argnum = 0; argnum < nargs; argnum++)
-    len += align_up (TYPE_LENGTH (value_type (args[argnum])),
-		     mips_stack_argsize (gdbarch));
+    {
+      struct type *arg_type = check_typedef (value_type (args[argnum]));
+      int arglen = TYPE_LENGTH (arg_type);
+
+      /* Align to double-word if necessary.  */
+      if (mips_abi_regsize (gdbarch) < 8
+	  && mips_type_needs_double_align (arg_type))
+	len = align_up (len, mips_stack_argsize (gdbarch) * 2);
+      /* Allocate space on the stack.  */
+      len += align_up (arglen, mips_stack_argsize (gdbarch));
+    }
   sp -= align_up (len, 16);
 
   if (mips_debug)
@@ -3662,10 +3678,11 @@
 	      && mips_type_needs_double_align (arg_type))
 	    {
 	      if ((argreg & 1))
-		argreg++;
+		{
+		  argreg++;
+		  stack_offset += mips_abi_regsize (gdbarch);
+		}
 	    }
-	  /* Note: Floating-point values that didn't fit into an FP
-	     register are only written to memory.  */
 	  while (len > 0)
 	    {
 	      /* Remember if the argument was written to the stack.  */
@@ -3679,8 +3696,7 @@
 
 	      /* Write this portion of the argument to the stack.  */
 	      if (argreg > MIPS_LAST_ARG_REGNUM
-		  || odd_sized_struct
-		  || fp_register_arg_p (typecode, arg_type))
+		  || odd_sized_struct)
 		{
 		  /* Should shorter than int integer values be
 		     promoted to int before being stored? */
@@ -3721,12 +3737,10 @@
 		}
 
 	      /* Note!!! This is NOT an else clause.  Odd sized
-	         structs may go thru BOTH paths.  Floating point
-	         arguments will not.  */
+	         structs may go thru BOTH paths.  */
 	      /* Write this portion of the argument to a general
 	         purpose register.  */
-	      if (argreg <= MIPS_LAST_ARG_REGNUM
-		  && !fp_register_arg_p (typecode, arg_type))
+	      if (argreg <= MIPS_LAST_ARG_REGNUM)
 		{
 		  LONGEST regval = extract_signed_integer (val, partial_len);
 		  /* Value may need to be sign extended, because

Re: mips-tdep.c: FP varargs fixes

[Daniel Jacobowitz]

On Mon, Mar 26, 2007 at 05:00:01PM +0100, Maciej W. Rozycki wrote:
>  This new patch has been tested natively for mips-unknown-linux-gnu and 
> remotely for mipsisa32-sde-elf, using mips-sim-sde32/-EB and 
> mips-sim-sde32/-EL as the targets.

If you really want to build up some bonus points, you could extend the
argument passing testcases to trigger some of these cases :-)

> 2007-03-26  Maciej W. Rozycki  <macro@mips.com>
>             Nigel Stephens  <nigel@mips.com>
> 
> 	* mips-tdep.c (mips_o32_push_dummy_call): Take account of
> 	argument alignment requirements when calculating stack space
> 	required.  When aligning an arg register to eight bytes
> 	boundary, align stack_offset too.  Write floating-point
> 	arguments to the appropriate integer register if need go there.
> 	(mips_o64_push_dummy_call): Likewise.
> 
>  It looks like the calls to mips_abi_regsize() within are now redundant in 
> these functions, because the result is implied by the very fact of calling 
> either of these functions, but this is functionally independent from this 
> change so I think it should be dealt with separately.

I agree.

>  OK to apply?

Yes, this is OK.  I'll just trust you on the underlying ABI issues,
since you know it better than I do.  It all looks sane.

-- 
Daniel Jacobowitz
CodeSourcery

Re: mips-tdep.c: FP varargs fixes

[Maciej W. Rozycki]

On Tue, 10 Apr 2007, Daniel Jacobowitz wrote:

> If you really want to build up some bonus points, you could extend the
> argument passing testcases to trigger some of these cases :-)

 I think gdb.base/funcargs.exp and gdb.base/varargs.exp should already 
cover the interesting combinations and I would suggest that only if any 
new problem pops up, they should get updated to take it into account.

> Yes, this is OK.  I'll just trust you on the underlying ABI issues,
> since you know it better than I do.  It all looks sane.

 Well, GCC is the definite reference in this case and it seems to be 
getting things right.  Applied now.

  Maciej

Re: mips-tdep.c: FP varargs fixes

[Daniel Jacobowitz]

On Tue, Apr 17, 2007 at 03:50:06PM +0100, Maciej W. Rozycki wrote:
> On Tue, 10 Apr 2007, Daniel Jacobowitz wrote:
> 
> > If you really want to build up some bonus points, you could extend the
> > argument passing testcases to trigger some of these cases :-)
> 
>  I think gdb.base/funcargs.exp and gdb.base/varargs.exp should already 
> cover the interesting combinations and I would suggest that only if any 
> new problem pops up, they should get updated to take it into account.

But they didn't fail before your patch - at least I don't have any
failures on mips-linux.  So you must be fixing something they didn't
test for.

-- 
Daniel Jacobowitz
CodeSourcery

Re: mips-tdep.c: FP varargs fixes

[Maciej W. Rozycki]

On Tue, 17 Apr 2007, Daniel Jacobowitz wrote:

> >  I think gdb.base/funcargs.exp and gdb.base/varargs.exp should already 
> > cover the interesting combinations and I would suggest that only if any 
> > new problem pops up, they should get updated to take it into account.
> 
> But they didn't fail before your patch - at least I don't have any
> failures on mips-linux.  So you must be fixing something they didn't
> test for.

 Well, gdb.base/varargs.exp did fail for me before this change.  The 
failure was:

print find_max_double(5,1.0,17.0,2.0,3.0,4.0)
find_max(5, -0.000000, 0.000000, 0.000000, 0.000000, 0.000000) returns 0.000000
$5 = 5.3101701311997171e-315
(gdb) FAIL: gdb.base/varargs.exp: print find_max_double(5,1.0,17.0,2.0,3.0,4.0)

and I can see in the function prologue it correctly handles the first two 
arguments that are passed through registers:

00400ec4 <find_max_double>:

/* Double-float varargs, 2 declared args */

double find_max_double(int num_vals, double first_val, ...) {
  400ec4:	3c1c0002 	lui	gp,0x2
  400ec8:	279c850c 	addiu	gp,gp,-31476
  400ecc:	0399e021 	addu	gp,gp,t9
  400ed0:	27bdffc0 	addiu	sp,sp,-64
  400ed4:	afbf003c 	sw	ra,60(sp)
  400ed8:	afbe0038 	sw	s8,56(sp)
  400edc:	03a0f021 	move	s8,sp
  400ee0:	afbc0010 	sw	gp,16(sp)
  400ee4:	afc40040 	sw	a0,64(s8)
  400ee8:	afc7004c 	sw	a3,76(s8)
  400eec:	afc60048 	sw	a2,72(s8)

Without the patch, when calling this function manually, gdb would 
incorrectly put the second argument onto the stack, which would get 
overwritten in the prologue above with whatever values would happen to 
live in a2 and a3.

 Please note that according to the o32 ABI, first four 32-bit words are 
always passed in registers, either general or floating-point, with the 
usual alignment rules applying -- backing stack space is reserved and the 
registers map to their corresponding offsets there.  However only leading 
floating-point arguments and only up to two of them are passed through 
floating-point registers.

 In this case the first argument is integer (going through a0), so the 
second one, even though floating-point, has to go through general 
registers.  It is a 64-bit double, so it has to be aligned to a doubleword 
boundry and hence a1 drops out.

  Maciej

Re: mips-tdep.c: FP varargs fixes

[Daniel Jacobowitz]

On Tue, Apr 17, 2007 at 04:51:29PM +0100, Maciej W. Rozycki wrote:
>  Well, gdb.base/varargs.exp did fail for me before this change.  The 
> failure was:

So it did.  I just can't read my own test logs.  Sorry for the noise.

-- 
Daniel Jacobowitz
CodeSourcery

Re: mips-tdep.c: FP varargs fixes

[Michael Snyder]

On Tue, 2007-04-17 at 15:50 +0100, Maciej W. Rozycki wrote:
> On Tue, 10 Apr 2007, Daniel Jacobowitz wrote:
> 
> > If you really want to build up some bonus points, you could extend the
> > argument passing testcases to trigger some of these cases :-)
> 
>  I think gdb.base/funcargs.exp and gdb.base/varargs.exp should already 
> cover the interesting combinations and I would suggest that only if any 
> new problem pops up, they should get updated to take it into account.
> 
> > Yes, this is OK.  I'll just trust you on the underlying ABI issues,
> > since you know it better than I do.  It all looks sane.
> 
>  Well, GCC is the definite reference in this case ...

You would think so -- but GCC has been known to get it wrong.
Historically, when gcc differs from the MIPS spec, we go with
the MIPS spec.  Even if it means our tests fail.

Re: mips-tdep.c: FP varargs fixes

[Maciej W. Rozycki]

On Tue, 17 Apr 2007, Michael Snyder wrote:

> >  Well, GCC is the definite reference in this case ...
> 
> You would think so -- but GCC has been known to get it wrong.
> Historically, when gcc differs from the MIPS spec, we go with
> the MIPS spec.  Even if it means our tests fail.

 No question about it, but it makes sense only if accompanied with the 
respective bug report against GCC.  Then some areas of the MIPS o32 ABI 
are obscure enough or plainly wrong (= useless as is -- see e.g. the 
R_MIPS_PC16 relocation), so that using a live piece of software instead is 
reasonably enough.  And o64 does not even exist. ;-)

  Maciej

Re: mips-tdep.c: FP varargs fixes

[Andreas Schwab]

"Maciej W. Rozycki" <macro@mips.com> writes:

> @@ -3272,7 +3283,7 @@
>  	      /* Write this portion of the argument to a general
>  	         purpose register.  */
>  	      if (argreg <= MIPS_LAST_ARG_REGNUM
> -		  && !fp_register_arg_p (typecode, arg_type))
> +		  /*&& !fp_register_arg_p (typecode, arg_type)*/)

Please don't comment out code, remove it instead, and add a comment why it
was removed.

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
PGP key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."