[RFA] sh-tdep.c: Fix float handling

[RFA] sh-tdep.c: Fix float handling

[Corinna Vinschen]

Hi,

the below patch solves a problem which was unfortunately hidden by using
the sh-hms-sim.exp baseboard file.  Removing the following line from
dejagnu/baseboards/sh-hms-sim.exp:

  set_board_info gdb,cannot_call_functions 1

uncovered that the current implementation doesn't get the ABI of floating
point CPUs (sh2e, sh3e, sh4) right.

First, in contrast to non-FPU CPUs, arguments are never split between
registers and stack.  If an argument doesn't fit in the remaining registers
it's always pushed entirely on the stack.  My testing led me to the wrong
assumption that just types > 16 bytes are pushed entirely on the stack.

Second, the FPU ABIs have a special way how to treat types as float types.
Structures with exactly one member, which is of type float or double, are
treated exactly as the base types float or double:

  struct sf {
    float f;
  };

  struct sd {
    double d;
  };

are handled the same way as just

  float f;

  double d;

As a result, arguments of these struct types are pushed into floating point
registers exactly as floats or doubles, using the same decision algorithm.

The same is valid if these types are used as function return types.  The
above structs are returned in fr0 resp. fr0,fr1 instead of in r0, r0,r1
or even using struct convention as it is for other structs.

The below patch fixes this by adding a helper function sh_treat_as_flt(),
which evaluates if a type is treated as float type.  The result of this
function is then used in sh_push_dummy_call_fpu(),
sh3e_sh4_extract_return_value() and sh3e_sh4_store_return_value() to
push resp. return these structs correctly.

Over all, including the sh_use_struct_convention patch send 20 minutes ago
the FAIL count is lowered by 1 FAIL on non-FPU ABIs and 5 FAILs on FPU
ABIs, but that's only visible when tweaking sh-hms-sim.exp appropriately.


Corinna


	* sh-tdep.c (sh_treat_as_flt): New function to recognize float
	types correctly.
	(sh_push_dummy_call_fpu): Fix argument passing rules.
	(sh3e_sh4_extract_return_value): Call sh_treat_as_flt to recognize
	float types.
	(sh3e_sh4_store_return_value): Ditto.

Index: sh-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/sh-tdep.c,v
retrieving revision 1.145
diff -u -p -r1.145 sh-tdep.c
--- sh-tdep.c	3 Oct 2003 08:13:37 -0000	1.145
+++ sh-tdep.c	4 Oct 2003 11:32:00 -0000
@@ -732,6 +749,33 @@ sh_next_flt_argreg (int len)
   return FLOAT_ARG0_REGNUM + argreg;
 }
 
+/* Helper function which figures out, if a type is treated like a float type.
+   The FPU based calling conventions (sh2e, sh3e, sh4, ...) are handling
+   structs with exactly one member, which is of type float or double the same
+   way as a simple float or double.  That determines if such data is passed
+   in float or general registers both, as argument or as return value.  */
+static int
+sh_treat_as_flt (struct type *type)
+{
+  int len = TYPE_LENGTH (type);
+
+  /* Ordinary float types are obviously treated as float.  */
+  if (TYPE_CODE (type) == TYPE_CODE_FLT)
+    return 1;
+  /* Otherwise non-struct types are not treated as float.  */
+  if (TYPE_CODE (type) != TYPE_CODE_STRUCT)
+    return 0;
+  /* Otherwise structs with more than one memeber are not treated as float.  */
+  if (TYPE_NFIELDS (type) != 1)
+    return 0;
+  /* Otherwise if the type of that member is float, the whole type is
+     treated as float.  */
+  if (TYPE_CODE (TYPE_FIELD_TYPE (type, 0)) == TYPE_CODE_FLT)
+    return 1;
+  /* Otherwise it's not treated as float.  */
+  return 0;
+}
+
 static CORE_ADDR
 sh_push_dummy_call_fpu (struct gdbarch *gdbarch,
 			CORE_ADDR func_addr,
@@ -749,7 +793,8 @@ sh_push_dummy_call_fpu (struct gdbarch *
   CORE_ADDR regval;
   char *val;
   int len, reg_size = 0;
-  int pass_on_stack;
+  int pass_on_stack = 0;
+  int treat_as_flt;
 
   /* first force sp to a 4-byte alignment */
   sp = sh_frame_align (gdbarch, sp);
@@ -776,43 +821,41 @@ sh_push_dummy_call_fpu (struct gdbarch *
       /* Some decisions have to be made how various types are handled.
          This also differs in different ABIs. */
       pass_on_stack = 0;
-      if (len > 16)
-	pass_on_stack = 1;	/* Types bigger than 16 bytes are passed on stack. */
 
       /* Find out the next register to use for a floating point value. */
-      if (TYPE_CODE (type) == TYPE_CODE_FLT)
+      treat_as_flt = sh_treat_as_flt (type);
+      if (treat_as_flt)
 	flt_argreg = sh_next_flt_argreg (len);
+      /* No data is passed partially in registers.  */
+      else if (len > ((ARGLAST_REGNUM - argreg + 1) * 4))
+	pass_on_stack = 1;
 
       while (len > 0)
 	{
-	  if ((TYPE_CODE (type) == TYPE_CODE_FLT
-	       && flt_argreg > FLOAT_ARGLAST_REGNUM)
-	      || argreg > ARGLAST_REGNUM || pass_on_stack)
+	  if ((treat_as_flt && flt_argreg > FLOAT_ARGLAST_REGNUM)
+	      || (!treat_as_flt && (argreg > ARGLAST_REGNUM
+	                            || pass_on_stack)))
 	    {
-	      /* The remainder of the data goes entirely on the stack,
-	         4-byte aligned. */
+	      /* The data goes entirely on the stack, 4-byte aligned. */
 	      reg_size = (len + 3) & ~3;
 	      write_memory (sp + stack_offset, val, reg_size);
 	      stack_offset += reg_size;
 	    }
-	  else if (TYPE_CODE (type) == TYPE_CODE_FLT
-		   && flt_argreg <= FLOAT_ARGLAST_REGNUM)
+	  else if (treat_as_flt && flt_argreg <= FLOAT_ARGLAST_REGNUM)
 	    {
 	      /* Argument goes in a float argument register.  */
 	      reg_size = register_size (gdbarch, flt_argreg);
 	      regval = extract_unsigned_integer (val, reg_size);
 	      regcache_cooked_write_unsigned (regcache, flt_argreg++, regval);
 	    }
-	  else if (argreg <= ARGLAST_REGNUM)
+	  else if (!treat_as_flt && argreg <= ARGLAST_REGNUM)
 	    {
 	      /* there's room in a register */
 	      reg_size = register_size (gdbarch, argreg);
 	      regval = extract_unsigned_integer (val, reg_size);
 	      regcache_cooked_write_unsigned (regcache, argreg++, regval);
 	    }
-	  /* Store the value reg_size bytes at a time.  This means that things
-	     larger than reg_size bytes may go partly in registers and partly
-	     on the stack.  */
+	  /* Store the value one register at a time or in one step on stack.  */
 	  len -= reg_size;
 	  val += reg_size;
 	}
@@ -930,7 +973,7 @@ static void
 sh3e_sh4_extract_return_value (struct type *type, struct regcache *regcache,
 			       void *valbuf)
 {
-  if (TYPE_CODE (type) == TYPE_CODE_FLT)
+  if (sh_treat_as_flt (type))
     {
       int len = TYPE_LENGTH (type);
       int i, regnum = FP0_REGNUM;
@@ -971,7 +1014,7 @@ static void
 sh3e_sh4_store_return_value (struct type *type, struct regcache *regcache,
 			     const void *valbuf)
 {
-  if (TYPE_CODE (type) == TYPE_CODE_FLT)
+  if (sh_treat_as_flt (type))
     {
       int len = TYPE_LENGTH (type);
       int i, regnum = FP0_REGNUM;



-- 
Corinna Vinschen
Cygwin Developer
Red Hat, Inc.

Re: [RFA] sh-tdep.c: Fix float handling

[Elena Zannoni]

Corinna Vinschen writes:
 > Hi,
 > 
 > the below patch solves a problem which was unfortunately hidden by using
 > the sh-hms-sim.exp baseboard file.  Removing the following line from
 > dejagnu/baseboards/sh-hms-sim.exp:
 > 
 >   set_board_info gdb,cannot_call_functions 1
 > 
 > uncovered that the current implementation doesn't get the ABI of floating
 > point CPUs (sh2e, sh3e, sh4) right.
 > 

Please add this expanation as a comment. I find cleared than your
shorter comments. Just add it before the helper function.

From here....

 > First, in contrast to non-FPU CPUs, arguments are never split between
 > registers and stack.  If an argument doesn't fit in the remaining registers
 > it's always pushed entirely on the stack.  My testing led me to the wrong
 > assumption that just types > 16 bytes are pushed entirely on the stack.
 > 
 > Second, the FPU ABIs have a special way how to treat types as float types.
 > Structures with exactly one member, which is of type float or double, are
 > treated exactly as the base types float or double:
 > 
 >   struct sf {
 >     float f;
 >   };
 > 
 >   struct sd {
 >     double d;
 >   };
 > 
 > are handled the same way as just
 > 
 >   float f;
 > 
 >   double d;
 > 
 > As a result, arguments of these struct types are pushed into floating point
 > registers exactly as floats or doubles, using the same decision algorithm.
 > 
 > The same is valid if these types are used as function return types.  The
 > above structs are returned in fr0 resp. fr0,fr1 instead of in r0, r0,r1
 > or even using struct convention as it is for other structs.
 > 

....to here.

 > The below patch fixes this by adding a helper function sh_treat_as_flt(),

Since this is a predicate, can you call it sh_treat_as_flt_p .

 > which evaluates if a type is treated as float type.  The result of this
 > function is then used in sh_push_dummy_call_fpu(),
 > sh3e_sh4_extract_return_value() and sh3e_sh4_store_return_value() to
 > push resp. return these structs correctly.
 > 
 > Over all, including the sh_use_struct_convention patch send 20 minutes ago
 > the FAIL count is lowered by 1 FAIL on non-FPU ABIs and 5 FAILs on FPU
 > ABIs, but that's only visible when tweaking sh-hms-sim.exp appropriately.
 > 
 
Ok otherwise.

BTW, what multilibs are you testing?

elena


 > 
 > Corinna
 > 
 > 
 > 	* sh-tdep.c (sh_treat_as_flt): New function to recognize float
 > 	types correctly.
 > 	(sh_push_dummy_call_fpu): Fix argument passing rules.
 > 	(sh3e_sh4_extract_return_value): Call sh_treat_as_flt to recognize
 > 	float types.
 > 	(sh3e_sh4_store_return_value): Ditto.
 > 
 > Index: sh-tdep.c
 > ===================================================================
 > RCS file: /cvs/src/src/gdb/sh-tdep.c,v
 > retrieving revision 1.145
 > diff -u -p -r1.145 sh-tdep.c
 > --- sh-tdep.c	3 Oct 2003 08:13:37 -0000	1.145
 > +++ sh-tdep.c	4 Oct 2003 11:32:00 -0000
 > @@ -732,6 +749,33 @@ sh_next_flt_argreg (int len)
 >    return FLOAT_ARG0_REGNUM + argreg;
 >  }
 >  
 > +/* Helper function which figures out, if a type is treated like a float type.
 > +   The FPU based calling conventions (sh2e, sh3e, sh4, ...) are handling
 > +   structs with exactly one member, which is of type float or double the same
 > +   way as a simple float or double.  That determines if such data is passed
 > +   in float or general registers both, as argument or as return value.  */
 > +static int
 > +sh_treat_as_flt (struct type *type)
 > +{
 > +  int len = TYPE_LENGTH (type);
 > +
 > +  /* Ordinary float types are obviously treated as float.  */
 > +  if (TYPE_CODE (type) == TYPE_CODE_FLT)
 > +    return 1;
 > +  /* Otherwise non-struct types are not treated as float.  */
 > +  if (TYPE_CODE (type) != TYPE_CODE_STRUCT)
 > +    return 0;
 > +  /* Otherwise structs with more than one memeber are not treated as float.  */
 > +  if (TYPE_NFIELDS (type) != 1)
 > +    return 0;
 > +  /* Otherwise if the type of that member is float, the whole type is
 > +     treated as float.  */
 > +  if (TYPE_CODE (TYPE_FIELD_TYPE (type, 0)) == TYPE_CODE_FLT)
 > +    return 1;
 > +  /* Otherwise it's not treated as float.  */
 > +  return 0;
 > +}
 > +
 >  static CORE_ADDR
 >  sh_push_dummy_call_fpu (struct gdbarch *gdbarch,
 >  			CORE_ADDR func_addr,
 > @@ -749,7 +793,8 @@ sh_push_dummy_call_fpu (struct gdbarch *
 >    CORE_ADDR regval;
 >    char *val;
 >    int len, reg_size = 0;
 > -  int pass_on_stack;
 > +  int pass_on_stack = 0;
 > +  int treat_as_flt;
 >  
 >    /* first force sp to a 4-byte alignment */
 >    sp = sh_frame_align (gdbarch, sp);
 > @@ -776,43 +821,41 @@ sh_push_dummy_call_fpu (struct gdbarch *
 >        /* Some decisions have to be made how various types are handled.
 >           This also differs in different ABIs. */
 >        pass_on_stack = 0;
 > -      if (len > 16)
 > -	pass_on_stack = 1;	/* Types bigger than 16 bytes are passed on stack. */
 >  
 >        /* Find out the next register to use for a floating point value. */
 > -      if (TYPE_CODE (type) == TYPE_CODE_FLT)
 > +      treat_as_flt = sh_treat_as_flt (type);
 > +      if (treat_as_flt)
 >  	flt_argreg = sh_next_flt_argreg (len);
 > +      /* No data is passed partially in registers.  */
 > +      else if (len > ((ARGLAST_REGNUM - argreg + 1) * 4))
 > +	pass_on_stack = 1;
 >  
 >        while (len > 0)
 >  	{
 > -	  if ((TYPE_CODE (type) == TYPE_CODE_FLT
 > -	       && flt_argreg > FLOAT_ARGLAST_REGNUM)
 > -	      || argreg > ARGLAST_REGNUM || pass_on_stack)
 > +	  if ((treat_as_flt && flt_argreg > FLOAT_ARGLAST_REGNUM)
 > +	      || (!treat_as_flt && (argreg > ARGLAST_REGNUM
 > +	                            || pass_on_stack)))
 >  	    {
 > -	      /* The remainder of the data goes entirely on the stack,
 > -	         4-byte aligned. */
 > +	      /* The data goes entirely on the stack, 4-byte aligned. */
 >  	      reg_size = (len + 3) & ~3;
 >  	      write_memory (sp + stack_offset, val, reg_size);
 >  	      stack_offset += reg_size;
 >  	    }
 > -	  else if (TYPE_CODE (type) == TYPE_CODE_FLT
 > -		   && flt_argreg <= FLOAT_ARGLAST_REGNUM)
 > +	  else if (treat_as_flt && flt_argreg <= FLOAT_ARGLAST_REGNUM)
 >  	    {
 >  	      /* Argument goes in a float argument register.  */
 >  	      reg_size = register_size (gdbarch, flt_argreg);
 >  	      regval = extract_unsigned_integer (val, reg_size);
 >  	      regcache_cooked_write_unsigned (regcache, flt_argreg++, regval);
 >  	    }
 > -	  else if (argreg <= ARGLAST_REGNUM)
 > +	  else if (!treat_as_flt && argreg <= ARGLAST_REGNUM)
 >  	    {
 >  	      /* there's room in a register */
 >  	      reg_size = register_size (gdbarch, argreg);
 >  	      regval = extract_unsigned_integer (val, reg_size);
 >  	      regcache_cooked_write_unsigned (regcache, argreg++, regval);
 >  	    }
 > -	  /* Store the value reg_size bytes at a time.  This means that things
 > -	     larger than reg_size bytes may go partly in registers and partly
 > -	     on the stack.  */
 > +	  /* Store the value one register at a time or in one step on stack.  */
 >  	  len -= reg_size;
 >  	  val += reg_size;
 >  	}
 > @@ -930,7 +973,7 @@ static void
 >  sh3e_sh4_extract_return_value (struct type *type, struct regcache *regcache,
 >  			       void *valbuf)
 >  {
 > -  if (TYPE_CODE (type) == TYPE_CODE_FLT)
 > +  if (sh_treat_as_flt (type))
 >      {
 >        int len = TYPE_LENGTH (type);
 >        int i, regnum = FP0_REGNUM;
 > @@ -971,7 +1014,7 @@ static void
 >  sh3e_sh4_store_return_value (struct type *type, struct regcache *regcache,
 >  			     const void *valbuf)
 >  {
 > -  if (TYPE_CODE (type) == TYPE_CODE_FLT)
 > +  if (sh_treat_as_flt (type))
 >      {
 >        int len = TYPE_LENGTH (type);
 >        int i, regnum = FP0_REGNUM;
 > 
 > 
 > 
 > -- 
 > Corinna Vinschen
 > Cygwin Developer
 > Red Hat, Inc.

Re: [RFA] sh-tdep.c: Fix float handling

[Corinna Vinschen]

On Fri, Oct 10, 2003 at 02:53:18PM -0400, Elena Zannoni wrote:
> Please add this expanation as a comment. I find cleared than your
> shorter comments. Just add it before the helper function.
> 
> >From here....
> 
>  > First, in contrast to non-FPU CPUs, arguments are never split between
>  > registers and stack.  If an argument doesn't fit in the remaining registers
>  > it's always pushed entirely on the stack.  My testing led me to the wrong
>  > assumption that just types > 16 bytes are pushed entirely on the stack.

I have a problem with this first part of the comment.  This one does
not fit well as a comment to the helper function.  I would suggest to
add this (carefully rephrased) to sh_push_dummy_call_fpu(), substituting
the very short

  /* No data is passed partially in registers.  */

comment instead.  Ok?

> Since this is a predicate, can you call it sh_treat_as_flt_p .

Ok.

> Ok otherwise.

Thanks, I'd just like to hear about the above comment.

> BTW, what multilibs are you testing?

Several, -m2, -m2e, m3e, m4, m4 -ml, -m4-single.

Corinna

-- 
Corinna Vinschen
Cygwin Developer
Red Hat, Inc.

Re: [RFA] sh-tdep.c: Fix float handling

[Elena Zannoni]

Corinna Vinschen writes:
 > On Fri, Oct 10, 2003 at 02:53:18PM -0400, Elena Zannoni wrote:
 > > Please add this expanation as a comment. I find cleared than your
 > > shorter comments. Just add it before the helper function.
 > > 
 > > >From here....
 > > 
 > >  > First, in contrast to non-FPU CPUs, arguments are never split between
 > >  > registers and stack.  If an argument doesn't fit in the remaining registers
 > >  > it's always pushed entirely on the stack.  My testing led me to the wrong
 > >  > assumption that just types > 16 bytes are pushed entirely on the stack.
 > 
 > I have a problem with this first part of the comment.  This one does
 > not fit well as a comment to the helper function.  I would suggest to
 > add this (carefully rephrased) to sh_push_dummy_call_fpu(), substituting
 > the very short
 > 
 >   /* No data is passed partially in registers.  */
 > 

Where is that? I think you can add it before the function
sh_push_dummy_call_fpu. Or where it makes more sense. I don't care, as
long as the whole thing is as clear as possible. It is wasy to
understand the code now, but I bet a few months from now we will all
have forgotten the details.

elena


 > comment instead.  Ok?
 > 
 > > Since this is a predicate, can you call it sh_treat_as_flt_p .
 > 
 > Ok.
 > 
 > > Ok otherwise.
 > 
 > Thanks, I'd just like to hear about the above comment.
 > 
 > > BTW, what multilibs are you testing?
 > 
 > Several, -m2, -m2e, m3e, m4, m4 -ml, -m4-single.
 > 
 > Corinna
 > 
 > -- 
 > Corinna Vinschen
 > Cygwin Developer
 > Red Hat, Inc.

Re: [RFA] sh-tdep.c: Fix float handling

[Corinna Vinschen]

On Fri, Oct 10, 2003 at 04:25:42PM -0400, Elena Zannoni wrote:
> Corinna Vinschen writes:
>  > >  > First, in contrast to non-FPU CPUs, arguments are never split between
>  > >  > registers and stack.  If an argument doesn't fit in the remaining registers
>  > >  > it's always pushed entirely on the stack.  My testing led me to the wrong
>  > >  > assumption that just types > 16 bytes are pushed entirely on the stack.
>  > 
>  > I have a problem with this first part of the comment.  This one does
>  > not fit well as a comment to the helper function.  I would suggest to
>  > add this (carefully rephrased) to sh_push_dummy_call_fpu(), substituting
>  > the very short
>  > 
>  >   /* No data is passed partially in registers.  */
>  > 
> 
> Where is that? I think you can add it before the function
> sh_push_dummy_call_fpu. Or where it makes more sense. I don't care, as
> long as the whole thing is as clear as possible. It is wasy to
> understand the code now, but I bet a few months from now we will all
> have forgotten the details.

I've applied this with the comment at the point I think it fit most well.
I agree to what you're saying, therefore I'm always trying to put some
additional comments in.

The above comment is now inline, beginnig at line 890 in
sh_push_dummy_call_fpu(), exactly above the code which cares for this
very situation.

Corinna

-- 
Corinna Vinschen
Cygwin Developer
Red Hat, Inc.