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

[Elena Zannoni <ezannoni at redhat dot com>]

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.