Re: Argument passed in register and stack may be loaded from wrong address

[Alexandre Oliva <aoliva at redhat dot com>]

On Mar 11, 2001, Alexandre Oliva <aoliva@redhat.com> wrote:

> In a port I'm working on, stack arguments are aligned to addresses
> multiple of 8 bytes.  In certain conditions, arguments of type `float'
> are passed both in a register and on the stack.  However, since the
> size of a float is 4, it has to be padded.  Unfortunately, GCC stores
> the float value in the stack slot + 4, but loads it into the register
> from the stack slot address.  Oops.

> This patch fixes the problem, by arranging for non-BLKmode arguments
> to have their actual stack address kept in arg->value, so that
> load_register_parameters() doesn't have to take padding into account.
> The original code, that copied arg->stack_slot to arg->value, is now
> only used only in the BLKmode case.  It is no longer used in case
> arg-> value == arg->stack from the beginning.  I'm not sure this is
> right.  It hasn't introduced any regressions in this new port, and I'm
> currently bootstrapping it on alpha-unknown-linux-gnu.  Can anybody
> recommend any other architectures in which it would be relevant to
> test this patch?  Or is it ok to install as is?

This patch was also for SH5 compact mode, and the failing testcase is:

float f(int i1, int i2, int i3, int i4, int i5, int i6, int i7, int i8,
	float v1)
{
  return v1;
}

int main() {
  if (f(1,2,3,4,5,6,7,8,1.0f) != 1.0f)
    abort ();
  exit (0);
}

The 1.0f constant is stored at $sp+4 and $fs0 loaded from $sp.  Then,
when compile with -O0, the called function stores $fs0, that contains
the incorrect value, over the correct value passed on the stack, so it
ends up lost.  The problem is in the caller, and is fixed with the
following patch, updated for mainline.  Ok to install this time?

Index: gcc/ChangeLog
from  Alexandre Oliva  <aoliva@redhat.com>
	* calls.c (store_one_arg): In the non-BLKmode non-partial case,
	take padding into account when computing the argument value.

Index: gcc/calls.c
===================================================================
RCS file: /cvs/gcc/egcs/gcc/calls.c,v
retrieving revision 1.220
diff -u -p -r1.220 calls.c
--- gcc/calls.c 2002/02/09 03:08:04 1.220
+++ gcc/calls.c 2002/02/09 04:38:00
@@ -4465,6 +4465,11 @@ store_one_arg (arg, argblock, flags, var
 		      partial, reg, used - size, argblock,
 		      ARGS_SIZE_RTX (arg->offset), reg_parm_stack_space,
 		      ARGS_SIZE_RTX (arg->alignment_pad));
+
+      /* Unless this is a partially-in-register argument, the argument is now
+	 in the stack.  */
+      if (partial == 0)
+	arg->value = arg->stack;
     }
   else
     {
@@ -4564,16 +4569,18 @@ store_one_arg (arg, argblock, flags, var
 		      argblock, ARGS_SIZE_RTX (arg->offset),
 		      reg_parm_stack_space,
 		      ARGS_SIZE_RTX (arg->alignment_pad));
-    }
 
-  /* Unless this is a partially-in-register argument, the argument is now
-     in the stack.
+      /* Unless this is a partially-in-register argument, the argument is now
+	 in the stack.
 
-     ??? Note that this can change arg->value from arg->stack to
-     arg->stack_slot and it matters when they are not the same.
-     It isn't totally clear that this is correct in all cases.  */
-  if (partial == 0)
-    arg->value = arg->stack_slot;
+	 ??? Unlike the case above, in which we want the actual
+	 address of the data, so that we can load it directly into a
+	 register, here we want the address of the stack slot, so that
+	 it's properly aligned for word-by-word copying or something
+	 like that.  It's not clear that this is always correct.  */
+      if (partial == 0)
+	arg->value = arg->stack_slot;
+    }
 
   /* Once we have pushed something, pops can't safely
      be deferred during the rest of the arguments.  */

-- 
Alexandre Oliva   Enjoy Guarana', see http://www.ic.unicamp.br/~oliva/
Red Hat GCC Developer                  aoliva@{cygnus.com, redhat.com}
CS PhD student at IC-Unicamp        oliva@{lsd.ic.unicamp.br, gnu.org}
Free Software Evangelist                Professional serial bug killer