[RFA] mips_push_arguments(): Make O64 ABI test explicit

[RFA] mips_push_arguments(): Make O64 ABI test explicit

[Kevin Buettner]

Now that mips-tdep.c defines another ABI such that

    mips_default_saved_regsize == 8,

it becomes necessary to revise one of the conditions in
mips_push_arguments().

The comment above the test in question says:

		     ...
		     Also don't do this adjustment on EABI and O64
		     binaries.
		     ...

I believe that the expression ``MIPS_SAVED_REGSIZE < 8'' was being used
to test to see if the O64 ABI was being used.  I've changed the test
so that it explicitly checks the tdep struct for the O64 ABI instead.

This patch fixes the following failures for the N64 ABI:

FAIL: gdb.base/call-ar-st.exp: print print_small_structs, pattern 4
FAIL: gdb.base/call-ar-st.exp: print print_small_structs from print_long_arg_list, pattern 4

Okay to commit?

	* mips-tdep.c (mips_push_arguments): Don't assume that
	``MIPS_SAVED_REGSIZE < 8'' means that the o64 abi is in use.
	Instead, test for it explicitly.

Index: mips-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/mips-tdep.c,v
retrieving revision 1.85
diff -u -p -r1.85 mips-tdep.c
--- mips-tdep.c	31 Jul 2002 20:26:49 -0000	1.85
+++ mips-tdep.c	31 Jul 2002 20:48:49 -0000
@@ -2615,7 +2615,7 @@ mips_push_arguments (int nargs,
 		     such and GDB gets tweaked accordingly.  */
 
 		  if (!MIPS_EABI
-		      && MIPS_SAVED_REGSIZE < 8
+		      && tdep->mips_abi != MIPS_ABI_O64
 		      && TARGET_BYTE_ORDER == BFD_ENDIAN_BIG
 		      && partial_len < MIPS_SAVED_REGSIZE
 		      && (typecode == TYPE_CODE_STRUCT ||

Re: [RFA] mips_push_arguments(): Make O64 ABI test explicit

[Michael Snyder]

Kevin Buettner wrote:
> 
> Now that mips-tdep.c defines another ABI such that
> 
>     mips_default_saved_regsize == 8,
> 
> it becomes necessary to revise one of the conditions in
> mips_push_arguments().
> 
> The comment above the test in question says:
> 
>                      ...
>                      Also don't do this adjustment on EABI and O64
>                      binaries.
>                      ...
> 
> I believe that the expression ``MIPS_SAVED_REGSIZE < 8'' was being used
> to test to see if the O64 ABI was being used.  I've changed the test
> so that it explicitly checks the tdep struct for the O64 ABI instead.
> 
> This patch fixes the following failures for the N64 ABI:
> 
> FAIL: gdb.base/call-ar-st.exp: print print_small_structs, pattern 4
> FAIL: gdb.base/call-ar-st.exp: print print_small_structs from print_long_arg_list, pattern 4
> 
> Okay to commit?
> 
>         * mips-tdep.c (mips_push_arguments): Don't assume that
>         ``MIPS_SAVED_REGSIZE < 8'' means that the o64 abi is in use.
>         Instead, test for it explicitly.

Kevin, 

This change makes things worse for N32.  I haven't tested
N64 yet.  I think we're gonna have to look at these lines
from the perspective of all 3 (6) abis.





> Index: mips-tdep.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/mips-tdep.c,v
> retrieving revision 1.85
> diff -u -p -r1.85 mips-tdep.c
> --- mips-tdep.c 31 Jul 2002 20:26:49 -0000      1.85
> +++ mips-tdep.c 31 Jul 2002 20:48:49 -0000
> @@ -2615,7 +2615,7 @@ mips_push_arguments (int nargs,
>                      such and GDB gets tweaked accordingly.  */
> 
>                   if (!MIPS_EABI
> -                     && MIPS_SAVED_REGSIZE < 8
> +                     && tdep->mips_abi != MIPS_ABI_O64
>                       && TARGET_BYTE_ORDER == BFD_ENDIAN_BIG
>                       && partial_len < MIPS_SAVED_REGSIZE
>                       && (typecode == TYPE_CODE_STRUCT ||

Re: [RFA] mips_push_arguments(): Make O64 ABI test explicit

[Kevin Buettner]

On Jul 31,  3:44pm, Michael Snyder wrote:

> >         * mips-tdep.c (mips_push_arguments): Don't assume that
> >         ``MIPS_SAVED_REGSIZE < 8'' means that the o64 abi is in use.
> >         Instead, test for it explicitly.
> 
> Kevin, 
> 
> This change makes things worse for N32.  I haven't tested
> N64 yet.  I think we're gonna have to look at these lines
> from the perspective of all 3 (6) abis.

Okay, this patch is withdrawn while I retest...

To save some time, can you tell me which N32 regressions you see?

Kevin

Re: [RFA] mips_push_arguments(): Make O64 ABI test explicit

[Michael Snyder]

Kevin Buettner wrote:
> 
> On Jul 31,  3:44pm, Michael Snyder wrote:
> 
> > >         * mips-tdep.c (mips_push_arguments): Don't assume that
> > >         ``MIPS_SAVED_REGSIZE < 8'' means that the o64 abi is in use.
> > >         Instead, test for it explicitly.
> >
> > Kevin,
> >
> > This change makes things worse for N32.  I haven't tested
> > N64 yet.  I think we're gonna have to look at these lines
> > from the perspective of all 3 (6) abis.
> 
> Okay, this patch is withdrawn while I retest...
> 
> To save some time, can you tell me which N32 regressions you see?

Buncha failures in structs.exp.
If you have a look at the tweak that I sent, it might be the thing.

Re: [RFA] mips_push_arguments(): Make O64 ABI test explicit

[Kevin Buettner]

On Jul 31,  4:21pm, Michael Snyder wrote:

> Kevin Buettner wrote:
> > 
> > On Jul 31,  3:44pm, Michael Snyder wrote:
> > 
> > > >         * mips-tdep.c (mips_push_arguments): Don't assume that
> > > >         ``MIPS_SAVED_REGSIZE < 8'' means that the o64 abi is in use.
> > > >         Instead, test for it explicitly.
> > >
> > > Kevin,
> > >
> > > This change makes things worse for N32.  I haven't tested
> > > N64 yet.  I think we're gonna have to look at these lines
> > > from the perspective of all 3 (6) abis.
> > 
> > Okay, this patch is withdrawn while I retest...
> > 
> > To save some time, can you tell me which N32 regressions you see?
> 
> Buncha failures in structs.exp.
> If you have a look at the tweak that I sent, it might be the thing.

Hmm.  It looks to me like SGI's cc and gcc disagree on the meaning of
the N32 ABI.

When I use the original test (or the tweak that you sent me), I see
the following failures when testing with cc, but not gcc:

    FAIL: gdb.base/structs.exp: p L1
    FAIL: gdb.base/structs.exp: p L2
    FAIL: gdb.base/structs.exp: p L3
    FAIL: gdb.base/structs.exp: p L4
    FAIL: gdb.base/structs.exp: p L5
    FAIL: gdb.base/structs.exp: p L6
    FAIL: gdb.base/structs.exp: p L7
    FAIL: gdb.base/structs.exp: p L9
    FAIL: gdb.base/structs.exp: p L10
    FAIL: gdb.base/structs.exp: p L11
    FAIL: gdb.base/structs.exp: p L12

OTOH, when I use my posted change, I don't see the above failures in cc,
but I do in gcc.

So, it seems that cc wants small structs shifted, but that gcc does not.

Time for me to dig into the ABI documents to find out which compiler's
right...

Re: [RFA] mips_push_arguments(): Make O64 ABI test explicit

[Kevin Buettner]

On Jul 31,  6:10pm, Kevin Buettner wrote:

> So, it seems that cc wants small structs shifted, but that gcc does not.
> 
> Time for me to dig into the ABI documents to find out which compiler's
> right...

It looks to me like the SGI (cc) compiler is right.  The text below was
obtained from:

http://techpubs.sgi.com/library/tpl/cgi-bin/getdoc.cgi/srch3@n64/0650/bks/SGI_Developer/books/Mpro_n32_ABI/sgi_html/ch02.html


    * Structs, unions, or other composite types are treated as a
      sequence of doublewords, and are passed in integer or floating
      point registers as though they were simple scalar parameters to
      the extent that they fit, with any excess on the stack packed
      according to the normal memory layout of the object.  More
      specifically:

	- Regardless of the struct field structure, it is treated as a
	  sequence of 64-bit chunks.  If a chunk consists solely of a
	  double float field (but not a double, which is part of a
	  union), it is passed in a floating point register.  Any
	  other chunk is passed in an integer register.

	- A union, either as the parameter itself or as a struct
	  parameter field, is treated as a sequence of integer
	  doublewords for purposes of assignment to integer parameter
	  registers.  No attempt is made to identify floating point
	  components for passing in floating point registers.

	- Array fields of structs are passed like unions.  Array
	  parameters are passed by reference (unless the relevant
	  language standard requires otherwise).

	- Right-justifying small scalar parameters in their save area
	  slots notwithstanding, struct parameters are always
	  left-justified.  This applies both to the case of a struct
	  smaller than 64 bits, and to the final chunk of a struct
	  which is not an integral multiple of 64 bits in size.  The
	  implication of this rule is that the address of the first
	  chunk's save area slot is the address of the struct, and the
	  struct is laid out in the save area memory exactly as if it
	  were allocated normally (once any part in registers has been
	  stored to the save area).  [These rules are analogous to the
	  o32-bit ABI treatment - only the chunk size and the ability
	  to pass double fields in floating point registers are
	  different.]

So, if I understand this correctly, gdb ought to be left shifting small
struct parameters for the o32, n32, and n64 ABIs.  I.e, I think my patch
was correct, though I do agree with Michael that it might be better to
list the ABIs that we want to do this struct shifting for instead of
trying to list the ones that we don't want to shift.

Kevin

Re: [RFA] mips_push_arguments(): Make O64 ABI test explicit

[Eric Christopher]

> inclined to go with SGI CC's interpretation; you should complain at
> Eric some more to get GCC fixed properly.  Lord knows I haven't had
> much luck :P
> 

Yeesh. I'll get to it, as soon as you can fetch me a document describing
what should happen... (and no, "look at irix's compiler" is not a good
answer) ;)

-eric

-- 
I don't want a pony, I want a rocket
powered jetpack!

Re: [RFA] mips_push_arguments(): Make O64 ABI test explicit

[Kevin Buettner]

On Aug 1,  4:54pm, Eric Christopher wrote:

> > inclined to go with SGI CC's interpretation; you should complain at
> > Eric some more to get GCC fixed properly.  Lord knows I haven't had
> > much luck :P
> > 
> 
> Yeesh. I'll get to it, as soon as you can fetch me a document describing
> what should happen... (and no, "look at irix's compiler" is not a good
> answer) ;)

What about

http://techpubs.sgi.com/library/tpl/cgi-bin/getdoc.cgi?coll=0650&db=bks&srch=&fname=/SGI_Developer/Mpro_n32_ABI/sgi_html/ch02.html

?

Re: [RFA] mips_push_arguments(): Make O64 ABI test explicit

[Eric Christopher]

On Thu, 2002-08-01 at 17:02, Kevin Buettner wrote:
> On Aug 1,  4:54pm, Eric Christopher wrote:
> 
> > > inclined to go with SGI CC's interpretation; you should complain at
> > > Eric some more to get GCC fixed properly.  Lord knows I haven't had
> > > much luck :P
> > > 
> > 
> > Yeesh. I'll get to it, as soon as you can fetch me a document describing
> > what should happen... (and no, "look at irix's compiler" is not a good
> > answer) ;)
> 
> What about
> 
> http://techpubs.sgi.com/library/tpl/cgi-bin/getdoc.cgi?coll=0650&db=bks&srch=&fname=/SGI_Developer/Mpro_n32_ABI/sgi_html/ch02.html
> 
> ?

That'll probably do. :) I'll take a look at it when I get a chance.

-eric

-- 
I don't want a pony, I want a rocket
powered jetpack!

Re: [RFA] mips_push_arguments(): Make O64 ABI test explicit

[Andrew Cagney]

 From mips-tdep.c:

                   /* A non-floating-point argument being passed in a
                      general register.  If a struct or union, and if
                      the remaining length is smaller than the register
                      size, we have to adjust the register value on
                      big endian targets.

                      It does not seem to be necessary to do the
                      same for integral types.

                      Also don't do this adjustment on EABI and O64
                      binaries.

                      cagney/2001-07-23: gdb/179: Also, GCC, when
                      outputting LE O32 with sizeof (struct) <
                      MIPS_SAVED_REGSIZE, generates a left shift as
                      part of storing the argument in a register a
                      register (the left shift isn't generated when
                      sizeof (struct) >= MIPS_SAVED_REGSIZE).  Since it
                      is quite possible that this is GCC contradicting
                      the LE/O32 ABI, GDB has not been adjusted to
                      accommodate this.  Either someone needs to
                      demonstrate that the LE/O32 ABI specifies such a
                      left shift OR this new ABI gets identified as
                      such and GDB gets tweaked accordingly.  */

Sounds like the comment should be updated to mention a few more ABIs and 
confirm that it is GCC that is wrong.   I'll assume this patch is withdrawn.

enjoy,
Andrew

Re: [RFA] mips_push_arguments(): Make O64 ABI test explicit

[Kevin Buettner]

On Aug 1,  5:55pm, Andrew Cagney wrote:

>                    /* A non-floating-point argument being passed in a
>                       general register.  If a struct or union, and if
>                       the remaining length is smaller than the register
>                       size, we have to adjust the register value on
>                       big endian targets.
> 
>                       It does not seem to be necessary to do the
>                       same for integral types.
> 
>                       Also don't do this adjustment on EABI and O64
>                       binaries.
> 
>                       cagney/2001-07-23: gdb/179: Also, GCC, when
>                       outputting LE O32 with sizeof (struct) <
>                       MIPS_SAVED_REGSIZE, generates a left shift as
>                       part of storing the argument in a register a
>                       register (the left shift isn't generated when
>                       sizeof (struct) >= MIPS_SAVED_REGSIZE).  Since it
>                       is quite possible that this is GCC contradicting
>                       the LE/O32 ABI, GDB has not been adjusted to
>                       accommodate this.  Either someone needs to
>                       demonstrate that the LE/O32 ABI specifies such a
>                       left shift OR this new ABI gets identified as
>                       such and GDB gets tweaked accordingly.  */
> 
> Sounds like the comment should be updated to mention a few more ABIs and 
> confirm that it is GCC that is wrong.   I'll assume this patch is withdrawn.

It is for the moment.

If you read further in the thread, you'll see that I've verified that
it's gcc which is wrong.  That being the case, more than just the comment
will need to change.  (IMO, of course.)

I'll wait a bit for further discussion before submitting a new patch.

Kevin

Re: [RFA] mips_push_arguments(): Make O64 ABI test explicit

[Andrew Cagney]

> Sounds like the comment should be updated to mention a few more ABIs and 
>> confirm that it is GCC that is wrong.   I'll assume this patch is withdrawn.
> 
> 
> It is for the moment.
> 
> If you read further in the thread, you'll see that I've verified that
> it's gcc which is wrong.

I did, that is why I suggested ``confirm that it is GCC'' :-)

> That being the case, more than just the comment
> will need to change.  (IMO, of course.)

In what way?  GDB was trying to implement things according to the ABI 
(unless someone snuck in a change or the code suffered bit rot).

As DanielJ mentioned, GCC should be fixed.

> I'll wait a bit for further discussion before submitting a new patch.

Andrew

Re: [RFA] mips_push_arguments(): Make O64 ABI test explicit

[Kevin Buettner]

On Aug 1,  6:27pm, Andrew Cagney wrote:

> > That being the case, more than just the comment
> > will need to change.  (IMO, of course.)
> 
> In what way?  GDB was trying to implement things according to the ABI 
> (unless someone snuck in a change or the code suffered bit rot).

The short answer:  I've probably managed to bit rot it with one of my
other (as of yet unapplied) patches since it changes MIPS_SAVED_REGSIZE
for the N32 ABI.  Even if it weren't for this "bit rot", I'd still need
to change the code to accomodate the N64 ABI.

Long answer:

Let's look at the condition again...

 		  if (!MIPS_EABI
		      && MIPS_SAVED_REGSIZE < 8
 		      && TARGET_BYTE_ORDER == BFD_ENDIAN_BIG
 		      && partial_len < MIPS_SAVED_REGSIZE
 		      && (typecode == TYPE_CODE_STRUCT ||
		      ...

The part that bothers me in the above is ``MIPS_SAVED_REGSIZE < 8''.
This, in conjunction with !MIPS_EABI, is supposed to specify some
set of ABI's that the shifting operations needs to be performed on
when the other conditions hold.

The problem is that one of my other patches changes MIPS_SAVED_REGSIZE
for N32.  We can discuss whether my other patch is correct or not
elsewhere.  The point is that we shouldn't be using the value of
MIPS_SAVED_REGSIZE to be an indicator of the ABI which is in use.

In my opinion, it would be better for us to explicitly list the ABI's
affected.  That way someone reading the code doesn't need to deduce
the affected ABIs or scratch their heads over whether the prefatory
comment is correct or not.  I.e, maybe something like this:

 		  if ( (tdep->mips_abi == MIPS_ABI_O32
		        || tdep->mips_abi == MIPS_ABI_N32
		        || tdep->mips_abi == MIPS_ABI_N64)
 		      && TARGET_BYTE_ORDER == BFD_ENDIAN_BIG
 		      && partial_len < MIPS_SAVED_REGSIZE
 		      && (typecode == TYPE_CODE_STRUCT ||
		      ...

> As DanielJ mentioned, GCC should be fixed.

I agree with this.

Re: [RFA] mips_push_arguments(): Make O64 ABI test explicit

[Michael Snyder]

Kevin Buettner wrote:
> 
> Now that mips-tdep.c defines another ABI such that
> 
>     mips_default_saved_regsize == 8,
> 
> it becomes necessary to revise one of the conditions in
> mips_push_arguments().
> 
> The comment above the test in question says:
> 
>                      ...
>                      Also don't do this adjustment on EABI and O64
>                      binaries.
>                      ...
> 
> I believe that the expression ``MIPS_SAVED_REGSIZE < 8'' was being used
> to test to see if the O64 ABI was being used. 

OK, with you so far...

> I've changed the test
> so that it explicitly checks the tdep struct for the O64 ABI instead.

Ah, but both N32 and N64 use 8-byte registers, and 
at least N32 does NOT want to have this adjustment.
I'm guessing N64 doesn't either (haven't tried it yet).

Your patch makes N32 fail, but we could add to it thusly:

> Okay to commit?
> 
>         * mips-tdep.c (mips_push_arguments): Don't assume that
>         ``MIPS_SAVED_REGSIZE < 8'' means that the o64 abi is in use.
>         Instead, test for each ABI which might not want this shift.
> 
> Index: mips-tdep.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/mips-tdep.c,v
> retrieving revision 1.85
> diff -u -p -r1.85 mips-tdep.c
> --- mips-tdep.c 31 Jul 2002 20:26:49 -0000      1.85
> +++ mips-tdep.c 31 Jul 2002 20:48:49 -0000
> @@ -2615,7 +2615,7 @@ mips_push_arguments (int nargs,
>                      such and GDB gets tweaked accordingly.  */
> 
>                   if (!MIPS_EABI
> -                     && MIPS_SAVED_REGSIZE < 8
  +                     && tdep->mips_abi != MIPS_ABI_O64
  +                     && tdep->mips_abi != MIPS_ABI_N32
  +                     && tdep->mips_abi != MIPS_ABI_N64
>                       && TARGET_BYTE_ORDER == BFD_ENDIAN_BIG
>                       && partial_len < MIPS_SAVED_REGSIZE
>                       && (typecode == TYPE_CODE_STRUCT ||


Of course, at this point, we might as well test for all the abis
that DO want the shift, instead of those that don't...

Michael