[RFA] sh-tdep.c: Fix little endian problem with doubles

[RFA] sh-tdep.c: Fix little endian problem with doubles

[Corinna Vinschen]

Hi,

I missed to get little endian mode right when it comes to passing
doubles in registers on FPU CPUs.  The below patch fixes that.
It's against the two patches I sent on Saturday.

Corinna

	* sh-tdep.c (sh_push_dummy_call_fpu): Accomodate double passing
	in little endian mode.
	(sh3e_sh4_extract_return_value): Ditto.

--- sh-tdep.c.INTERIM	2003-10-04 13:22:01.000000000 +0200
+++ sh-tdep.c	2003-10-07 18:42:13.000000000 +0200
@@ -846,6 +846,17 @@ sh_push_dummy_call_fpu (struct gdbarch *
 	      /* Argument goes in a float argument register.  */
 	      reg_size = register_size (gdbarch, flt_argreg);
 	      regval = extract_unsigned_integer (val, reg_size);
+	      /* A float type taking two registers must be handled
+	         differently in LE mode.  */
+	      if (TARGET_BYTE_ORDER == BFD_ENDIAN_LITTLE
+	          && len == 2 * reg_size)
+	        {
+		  regcache_cooked_write_unsigned (regcache, flt_argreg + 1,
+						  regval);
+		  val += reg_size;
+		  len -= reg_size;
+		  regval = extract_unsigned_integer (val, reg_size);
+		}
 	      regcache_cooked_write_unsigned (regcache, flt_argreg++, regval);
 	    }
 	  else if (!treat_as_flt && argreg <= ARGLAST_REGNUM)
@@ -978,7 +989,10 @@ sh3e_sh4_extract_return_value (struct ty
       int len = TYPE_LENGTH (type);
       int i, regnum = FP0_REGNUM;
       for (i = 0; i < len; i += 4)
-	regcache_raw_read (regcache, regnum++, (char *) valbuf + i);
+	if (TARGET_BYTE_ORDER == BFD_ENDIAN_LITTLE)
+	  regcache_raw_read (regcache, regnum++, (char *) valbuf + len - 4 - i);
+	else
+	  regcache_raw_read (regcache, regnum++, (char *) valbuf + i);
     }
   else
     sh_default_extract_return_value (type, regcache, valbuf);

-- 
Corinna Vinschen
Cygwin Developer
Red Hat, Inc.

Re: [RFA] sh-tdep.c: Fix little endian problem with doubles

[Michael Snyder]

Corinna Vinschen wrote:
> Hi,
> 
> I missed to get little endian mode right when it comes to passing
> doubles in registers on FPU CPUs.  The below patch fixes that.
> It's against the two patches I sent on Saturday.
> 
> Corinna

Giving an independent non-maintainer nod to this code...
I encountered the same issue earlier, and I think what
you're doing is right.  In fact, I thought I had done it.
Memory fails me...

Michael

> 
> 	* sh-tdep.c (sh_push_dummy_call_fpu): Accomodate double passing
> 	in little endian mode.
> 	(sh3e_sh4_extract_return_value): Ditto.
> 
> --- sh-tdep.c.INTERIM	2003-10-04 13:22:01.000000000 +0200
> +++ sh-tdep.c	2003-10-07 18:42:13.000000000 +0200
> @@ -846,6 +846,17 @@ sh_push_dummy_call_fpu (struct gdbarch *
>  	      /* Argument goes in a float argument register.  */
>  	      reg_size = register_size (gdbarch, flt_argreg);
>  	      regval = extract_unsigned_integer (val, reg_size);
> +	      /* A float type taking two registers must be handled
> +	         differently in LE mode.  */
> +	      if (TARGET_BYTE_ORDER == BFD_ENDIAN_LITTLE
> +	          && len == 2 * reg_size)
> +	        {
> +		  regcache_cooked_write_unsigned (regcache, flt_argreg + 1,
> +						  regval);
> +		  val += reg_size;
> +		  len -= reg_size;
> +		  regval = extract_unsigned_integer (val, reg_size);
> +		}
>  	      regcache_cooked_write_unsigned (regcache, flt_argreg++, regval);
>  	    }
>  	  else if (!treat_as_flt && argreg <= ARGLAST_REGNUM)
> @@ -978,7 +989,10 @@ sh3e_sh4_extract_return_value (struct ty
>        int len = TYPE_LENGTH (type);
>        int i, regnum = FP0_REGNUM;
>        for (i = 0; i < len; i += 4)
> -	regcache_raw_read (regcache, regnum++, (char *) valbuf + i);
> +	if (TARGET_BYTE_ORDER == BFD_ENDIAN_LITTLE)
> +	  regcache_raw_read (regcache, regnum++, (char *) valbuf + len - 4 - i);
> +	else
> +	  regcache_raw_read (regcache, regnum++, (char *) valbuf + i);
>      }
>    else
>      sh_default_extract_return_value (type, regcache, valbuf);
> 

Re: [RFA] sh-tdep.c: Fix little endian problem with doubles

[Elena Zannoni]

Corinna Vinschen writes:
 > Hi,
 > 
 > I missed to get little endian mode right when it comes to passing
 > doubles in registers on FPU CPUs.  The below patch fixes that.
 > It's against the two patches I sent on Saturday.
 > 
 > Corinna
 > 
 > 	* sh-tdep.c (sh_push_dummy_call_fpu): Accomodate double passing
 > 	in little endian mode.
 > 	(sh3e_sh4_extract_return_value): Ditto.
 > 
 > --- sh-tdep.c.INTERIM	2003-10-04 13:22:01.000000000 +0200
 > +++ sh-tdep.c	2003-10-07 18:42:13.000000000 +0200
 > @@ -846,6 +846,17 @@ sh_push_dummy_call_fpu (struct gdbarch *
 >  	      /* Argument goes in a float argument register.  */
 >  	      reg_size = register_size (gdbarch, flt_argreg);
 >  	      regval = extract_unsigned_integer (val, reg_size);
 > +	      /* A float type taking two registers must be handled
 > +	         differently in LE mode.  */
 > +	      if (TARGET_BYTE_ORDER == BFD_ENDIAN_LITTLE
 > +	          && len == 2 * reg_size)
 > +	        {
 > +		  regcache_cooked_write_unsigned (regcache, flt_argreg + 1,
 > +						  regval);
 > +		  val += reg_size;
 > +		  len -= reg_size;
 > +		  regval = extract_unsigned_integer (val, reg_size);
 > +		}

I'd prefer if there is an 'else if' clause just for the
doubles. I.e. don't use len in the test, but TYPE_LENGTH(type). This is
too confusing.

 >  	      regcache_cooked_write_unsigned (regcache, flt_argreg++, regval);
 >  	    }
 >  	  else if (!treat_as_flt && argreg <= ARGLAST_REGNUM)
 > @@ -978,7 +989,10 @@ sh3e_sh4_extract_return_value (struct ty
 >        int len = TYPE_LENGTH (type);
 >        int i, regnum = FP0_REGNUM;
 >        for (i = 0; i < len; i += 4)
 > -	regcache_raw_read (regcache, regnum++, (char *) valbuf + i);
 > +	if (TARGET_BYTE_ORDER == BFD_ENDIAN_LITTLE)
 > +	  regcache_raw_read (regcache, regnum++, (char *) valbuf + len - 4 - i);
 > +	else
 > +	  regcache_raw_read (regcache, regnum++, (char *) valbuf + i);
 >      }
 >    else
 >      sh_default_extract_return_value (type, regcache, valbuf);
 > 

this one is fine.

elena


 > -- 
 > Corinna Vinschen
 > Cygwin Developer
 > Red Hat, Inc.

Re: [RFA] sh-tdep.c: Fix little endian problem with doubles

[Corinna Vinschen]

On Fri, Oct 10, 2003 at 05:47:20PM -0400, Elena Zannoni wrote:
> Corinna Vinschen writes:
>  > --- sh-tdep.c.INTERIM	2003-10-04 13:22:01.000000000 +0200
>  > +++ sh-tdep.c	2003-10-07 18:42:13.000000000 +0200
>  > @@ -846,6 +846,17 @@ sh_push_dummy_call_fpu (struct gdbarch *
>  >  	      /* Argument goes in a float argument register.  */
>  >  	      reg_size = register_size (gdbarch, flt_argreg);
>  >  	      regval = extract_unsigned_integer (val, reg_size);
>  > +	      /* A float type taking two registers must be handled
>  > +	         differently in LE mode.  */
>  > +	      if (TARGET_BYTE_ORDER == BFD_ENDIAN_LITTLE
>  > +	          && len == 2 * reg_size)
>  > +	        {
>  > +		  regcache_cooked_write_unsigned (regcache, flt_argreg + 1,
>  > +						  regval);
>  > +		  val += reg_size;
>  > +		  len -= reg_size;
>  > +		  regval = extract_unsigned_integer (val, reg_size);
>  > +		}
> 
> I'd prefer if there is an 'else if' clause just for the
> doubles. I.e. don't use len in the test, but TYPE_LENGTH(type). This is
> too confusing.

Erm... sorry, I don't quite understand.  An `else if' in conjuction
with what `if'?  Actually, the double case is handled normally in
BE mode, it's only slightly different in LE mode in that the registers
are swapped.  The above code just makes the swap so I really don't see
what the problem is.

I see, however, that a TYPE_LENGTH(type) might be more readable than the
`len' and even more correct, since len is modified in the loop.  Yes,
that makes sense.

Back to the `else if'.  Wouldn't it be better just to pump up the comment
to explain what happens?  Instead of

  A float type taking two registers must be handled differently in LE mode.

better something along the lines

  In little endian mode, float types taking two registers (doubles on sh4,
  long doubles on sh2e, sh3e and sh4) must be stored swapped in the argument
  registers.  The below code first writes the first 32 bits in the next but
  one register, increments the val and len values accordingly and then
  proceeds as normal by writing the second 32 bits in the next register.

?

Corinna

-- 
Corinna Vinschen
Cygwin Developer
Red Hat, Inc.

Re: [RFA] sh-tdep.c: Fix little endian problem with doubles

[Elena Zannoni]

Corinna Vinschen writes:
 > On Fri, Oct 10, 2003 at 05:47:20PM -0400, Elena Zannoni wrote:
 > > Corinna Vinschen writes:
 > >  > --- sh-tdep.c.INTERIM	2003-10-04 13:22:01.000000000 +0200
 > >  > +++ sh-tdep.c	2003-10-07 18:42:13.000000000 +0200
 > >  > @@ -846,6 +846,17 @@ sh_push_dummy_call_fpu (struct gdbarch *
 > >  >  	      /* Argument goes in a float argument register.  */
 > >  >  	      reg_size = register_size (gdbarch, flt_argreg);
 > >  >  	      regval = extract_unsigned_integer (val, reg_size);
 > >  > +	      /* A float type taking two registers must be handled
 > >  > +	         differently in LE mode.  */
 > >  > +	      if (TARGET_BYTE_ORDER == BFD_ENDIAN_LITTLE
 > >  > +	          && len == 2 * reg_size)
 > >  > +	        {
 > >  > +		  regcache_cooked_write_unsigned (regcache, flt_argreg + 1,
 > >  > +						  regval);
 > >  > +		  val += reg_size;
 > >  > +		  len -= reg_size;
 > >  > +		  regval = extract_unsigned_integer (val, reg_size);
 > >  > +		}
 > > 
 > > I'd prefer if there is an 'else if' clause just for the
 > > doubles. I.e. don't use len in the test, but TYPE_LENGTH(type). This is
 > > too confusing.
 > 
 > Erm... sorry, I don't quite understand.  An `else if' in conjuction
 > with what `if'?  Actually, the double case is handled normally in
 > BE mode, it's only slightly different in LE mode in that the registers
 > are swapped.  The above code just makes the swap so I really don't see
 > what the problem is.

I mean: the function is structured so that there is pretty much a
clause for each possible type. Just add another one. I don't care if
there is a bit of code duplication. Something like that, or similar.

	  else if (TYPE_CODE (type) == TYPE_CODE_FLT
		   && flt_argreg <= FLOAT_ARGLAST_REGNUM
		   && TYPE_LENGTH(type) == reg_size)
		   { do old stuff}

	  else if (TYPE_CODE (type) == TYPE_CODE_FLT
		   &&  TYPE_LENGTH(type) == 2 * reg_size
		   && flt_argreg <= FLOAT_ARGLAST_REGNUM)
		   {
                     if (TARGET_BYTE_ORDER == BFD_ENDIAN_LITTLE)
		     {do something}
                     else
                     {do something else}
                   }

          else if (blah)

Actually, the test that (flt_argreg <= FLOAT_ARGLAST_REGNUM) may not
be sufficient anymore, because you are going to be using 2 registers,
and you could end up beyond FLOAT_ARGLAST_REGNUM. Or is it fine to
have the argument using the last float register, and the stack? Hmm,
is sh_next_flt_argreg taking care that doesn't happen? Seems so.


 > I see, however, that a TYPE_LENGTH(type) might be more readable than the
 > `len' and even more correct, since len is modified in the loop.  Yes,
 > that makes sense.
 > 
 > Back to the `else if'.  Wouldn't it be better just to pump up the comment
 > to explain what happens?  Instead of
 > 
 >   A float type taking two registers must be handled differently in LE mode.
 > 
 > better something along the lines
 > 
 >   In little endian mode, float types taking two registers (doubles on sh4,
 >   long doubles on sh2e, sh3e and sh4) must be stored swapped in the argument
 >   registers.  The below code first writes the first 32 bits in the next but
 >   one register, increments the val and len values accordingly and then
 >   proceeds as normal by writing the second 32 bits in the next register.

This would be good to add anyway.

elena

 > 
 > ?
 > 
 > Corinna
 > 
 > -- 
 > Corinna Vinschen
 > Cygwin Developer
 > Red Hat, Inc.

Re: [RFA] sh-tdep.c: Fix little endian problem with doubles

[Corinna Vinschen]

On Mon, Oct 13, 2003 at 10:18:46AM -0400, Elena Zannoni wrote:
> Corinna Vinschen writes:
>  > Erm... sorry, I don't quite understand.  An `else if' in conjuction
>  > with what `if'?  Actually, the double case is handled normally in
>  > BE mode, it's only slightly different in LE mode in that the registers
>  > are swapped.  The above code just makes the swap so I really don't see
>  > what the problem is.
> 
> I mean: the function is structured so that there is pretty much a
> clause for each possible type. Just add another one. I don't care if
> there is a bit of code duplication. Something like that, or similar.
> 
> 	  else if (TYPE_CODE (type) == TYPE_CODE_FLT
> 		   && flt_argreg <= FLOAT_ARGLAST_REGNUM
> 		   && TYPE_LENGTH(type) == reg_size)
> 		   { do old stuff}
> 
> 	  else if (TYPE_CODE (type) == TYPE_CODE_FLT
> 		   &&  TYPE_LENGTH(type) == 2 * reg_size
> 		   && flt_argreg <= FLOAT_ARGLAST_REGNUM)
> 		   {
>                      if (TARGET_BYTE_ORDER == BFD_ENDIAN_LITTLE)
> 		     {do something}
>                      else
>                      {do something else}
>                    }
> 
>           else if (blah)

Hmm, but you see that this messes up the code more than it cleans it up,
don't you?  The reg_size variable depends already on the decision, that
we know if the argument goes in a general or a floating point register.
So the outer if/else if's do have to be kept as they are, to be able to
evaluate the register size correctly, and only then the code can react
on the information that it's a regsize or 2*regsize argument.  Except we
drop the reg_size variable again, since we know that registers are always
4 byte anyway.

> Actually, the test that (flt_argreg <= FLOAT_ARGLAST_REGNUM) may not
> be sufficient anymore, because you are going to be using 2 registers,
> and you could end up beyond FLOAT_ARGLAST_REGNUM. Or is it fine to
> have the argument using the last float register, and the stack? Hmm,

No.  See the code right before the loop.

> is sh_next_flt_argreg taking care that doesn't happen? Seems so.

Yes.  Consider that doubles are always passed in even register pairs.

Corinna

-- 
Corinna Vinschen
Cygwin Developer
Red Hat, Inc.

Re: [RFA] sh-tdep.c: Fix little endian problem with doubles

[Elena Zannoni]

Corinna Vinschen writes:
 > On Mon, Oct 13, 2003 at 10:18:46AM -0400, Elena Zannoni wrote:
 > > Corinna Vinschen writes:
 > >  > Erm... sorry, I don't quite understand.  An `else if' in conjuction
 > >  > with what `if'?  Actually, the double case is handled normally in
 > >  > BE mode, it's only slightly different in LE mode in that the registers
 > >  > are swapped.  The above code just makes the swap so I really don't see
 > >  > what the problem is.
 > > 

Corinna, whatever. I'll clean it up myself. I am tired of arguing. Please
check it in.

elena


 > > I mean: the function is structured so that there is pretty much a
 > > clause for each possible type. Just add another one. I don't care if
 > > there is a bit of code duplication. Something like that, or similar.
 > > 
 > > 	  else if (TYPE_CODE (type) == TYPE_CODE_FLT
 > > 		   && flt_argreg <= FLOAT_ARGLAST_REGNUM
 > > 		   && TYPE_LENGTH(type) == reg_size)
 > > 		   { do old stuff}
 > > 
 > > 	  else if (TYPE_CODE (type) == TYPE_CODE_FLT
 > > 		   &&  TYPE_LENGTH(type) == 2 * reg_size
 > > 		   && flt_argreg <= FLOAT_ARGLAST_REGNUM)
 > > 		   {
 > >                      if (TARGET_BYTE_ORDER == BFD_ENDIAN_LITTLE)
 > > 		     {do something}
 > >                      else
 > >                      {do something else}
 > >                    }
 > > 
 > >           else if (blah)
 > 
 > Hmm, but you see that this messes up the code more than it cleans it up,
 > don't you?  The reg_size variable depends already on the decision, that
 > we know if the argument goes in a general or a floating point register.
 > So the outer if/else if's do have to be kept as they are, to be able to
 > evaluate the register size correctly, and only then the code can react
 > on the information that it's a regsize or 2*regsize argument.  Except we
 > drop the reg_size variable again, since we know that registers are always
 > 4 byte anyway.
 > 
 > > Actually, the test that (flt_argreg <= FLOAT_ARGLAST_REGNUM) may not
 > > be sufficient anymore, because you are going to be using 2 registers,
 > > and you could end up beyond FLOAT_ARGLAST_REGNUM. Or is it fine to
 > > have the argument using the last float register, and the stack? Hmm,
 > 
 > No.  See the code right before the loop.
 > 
 > > is sh_next_flt_argreg taking care that doesn't happen? Seems so.
 > 
 > Yes.  Consider that doubles are always passed in even register pairs.
 > 
 > Corinna
 > 
 > -- 
 > Corinna Vinschen
 > Cygwin Developer
 > Red Hat, Inc.

Re: [RFA] sh-tdep.c: Fix little endian problem with doubles

[Corinna Vinschen]

On Tue, Oct 14, 2003 at 10:47:41AM -0400, Elena Zannoni wrote:
> Corinna Vinschen writes:
>  > On Mon, Oct 13, 2003 at 10:18:46AM -0400, Elena Zannoni wrote:
>  > > Corinna Vinschen writes:
>  > >  > Erm... sorry, I don't quite understand.  An `else if' in conjuction
>  > >  > with what `if'?  Actually, the double case is handled normally in
>  > >  > BE mode, it's only slightly different in LE mode in that the registers
>  > >  > are swapped.  The above code just makes the swap so I really don't see
>  > >  > what the problem is.
>  > > 
> 
> Corinna, whatever. I'll clean it up myself. I am tired of arguing. Please
> check it in.

Ok, applied with the other discussed changes.  I'm curious what you
consider a clean up, though.

Corinna

-- 
Corinna Vinschen
Cygwin Developer
Red Hat, Inc.