[RFA] sh-tdep.c (sh_use_struct_convention): Restructure and fix

[RFA] sh-tdep.c (sh_use_struct_convention): Restructure and fix

[Corinna Vinschen]

Hi,

the below patch straightens out sh_use_struct_convention() so that it
allows a far better readability than before, especially by allowing
a bunch of comments spread out through the code.

Additionally it fixes one bug:  A struct of lenght 4 bytes, which
consists of only a bitfield, is returned in register r0, not on the
stack using the struct convention.  So far, GDB got that wrong.

Corinna

	* sh-tdep.c (sh_use_struct_convention): Clean up to have a
	more readable code.  Accomodate 4 byte structs with 4 byte sized
	first field (e.g. bitfields).

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
@@ -565,8 +565,25 @@ sh_use_struct_convention (int gcc_p, str
 {
   int len = TYPE_LENGTH (type);
   int nelem = TYPE_NFIELDS (type);
-  return ((len != 1 && len != 2 && len != 4 && len != 8) || nelem != 1) &&
-    (len != 8 || TYPE_LENGTH (TYPE_FIELD_TYPE (type, 0)) != 4);
+
+  /* Non-power of 2 length types and types bigger than 8 bytes (which don't
+     fit in two registers anyway) use struct convention.  */
+  if (len != 1 && len != 2 && len != 4 && len != 8)
+    return 1;
+  /* Structs with more than 1 fields use struct convention, if...  */
+  if (nelem != 1)
+    {
+      /* ... they are 1 or 2 bytes in size (e.g. struct of two chars)... */
+      if (len != 4 && len != 8)
+	return 1;
+      /* ... or, if the struct is 4 or 8 bytes and the first field is
+	 not of size 4 bytes.  Note that this also covers structs with
+	 bitfields. */
+      if (TYPE_LENGTH (TYPE_FIELD_TYPE (type, 0)) != 4)
+	return 1;
+    }
+  /* Otherwise return in registers.  */
+  return 0;
 }
 
 /* Extract from an array REGBUF containing the (raw) register state

-- 
Corinna Vinschen
Cygwin Developer
Red Hat, Inc.

Re: [RFA] sh-tdep.c (sh_use_struct_convention): Restructure and fix

[Andrew Cagney]

> 	* sh-tdep.c (sh_use_struct_convention): Clean up to have a
> 	more readable code.  Accomodate 4 byte structs with 4 byte sized
> 	first field (e.g. bitfields).

Corinna,

See: http://sources.redhat.com/ml/gdb-patches/2003-10/msg00033.html.
The ppc64_sysv_return_value code in ppc-sysv-tdep.c, has been written in 
a way that allows a quick update to this new iterface.

Andrew

Re: [RFA] sh-tdep.c (sh_use_struct_convention): Restructure and fix

[Kevin Buettner]

On Oct 4, 11:54am, Andrew Cagney wrote:

> See: http://sources.redhat.com/ml/gdb-patches/2003-10/msg00033.html.
> The ppc64_sysv_return_value code in ppc-sysv-tdep.c, has been written in 
> a way that allows a quick update to this new iterface.

Andrew,

There are pros and cons to the approach that you used in
ppc64_sysv_abi_return_value().

On the pro side - and this is definitely a good thing - you keep the
struct convention information together with the implementation of how
to return a value.

But this is also a con because you've spread the definition of
"use_struct_convention" out over a much larger number of lines.  It
isn't (IMO) as easy to comprehend when arranged in this way.

The jury is still out (at least as far as I'm concerned) as to
which approach is better.  I do happen to think that your approach
is better for ppc64 (and ppc too), but this may not necessarily be the
case for other architectures.

With regard to Corinna's patch, she's fixed some bugs and has improved
readability.  If Corinna looks at your approach and finds it
compelling enough to redo her patch, that's fine.  But I don't think
there should be an (implied) requirement that she do so.

Kevin

Re: [RFA] sh-tdep.c (sh_use_struct_convention): Restructure and fix

[Andrew Cagney]

> On Oct 4, 11:54am, Andrew Cagney wrote:
> 
> 
>> See: http://sources.redhat.com/ml/gdb-patches/2003-10/msg00033.html.
>> The ppc64_sysv_return_value code in ppc-sysv-tdep.c, has been written in 
>> a way that allows a quick update to this new iterface.
> 
> 
> Andrew,
> 
> There are pros and cons to the approach that you used in
> ppc64_sysv_abi_return_value().
> 
> On the pro side - and this is definitely a good thing - you keep the
> struct convention information together with the implementation of how
> to return a value.

It is stronger than that, it moves all of the ABIs struct convention 
logic to one place.  At present key parts of the logic are scattered 
inconsistently across core parts of GDB

The result is a longer but more correct function.

> But this is also a con because you've spread the definition of
> "use_struct_convention" out over a much larger number of lines.  It
> isn't (IMO) as easy to comprehend when arranged in this way.

As my spec for the interface points out, if this were an OO language it 
would have a different interface.

> The jury is still out (at least as far as I'm concerned) as to
> which approach is better.  I do happen to think that your approach
> is better for ppc64 (and ppc too), but this may not necessarily be the
> case for other architectures.

This is a technical problem.  The return_value patch fixes the problem 
of GDB not being able to handle all cases of:

	return return-value-in-register

correctly.  If Corinna instead implements use_struct_convention, then 
that bug won't be fixed.

Andrew

Re: [RFA] sh-tdep.c (sh_use_struct_convention): Restructure and fix

[Kevin Buettner]

On Oct 4,  1:35pm, Andrew Cagney wrote:

> >> See: http://sources.redhat.com/ml/gdb-patches/2003-10/msg00033.html.
> >> The ppc64_sysv_return_value code in ppc-sysv-tdep.c, has been written in 
> >> a way that allows a quick update to this new iterface.
> > 
> > Andrew,
> > 
> > There are pros and cons to the approach that you used in
> > ppc64_sysv_abi_return_value().
> > 
> > On the pro side - and this is definitely a good thing - you keep the
> > struct convention information together with the implementation of how
> > to return a value.
> 
> It is stronger than that, it moves all of the ABIs struct convention 
> logic to one place.

Huh?  What other parts are there?  (I fail to see why it's stronger
than what I stated.)

> At present key parts of the logic are scattered 
> inconsistently across core parts of GDB

Please explain.  How did the (traditional) ppc struct return
mechanisms get scattered across core parts of GDB?

> The result is a longer but more correct function.

More correct?  There is absolutely nothing which prevents a
traditional "use_struct_convention" function from being correct. 
What's "more correct" than "correct"?

I'll grant you that it may be easier to verify correctness by having
the struct convention code placed side by side with the value
(extract/store) return code.

And then again, it might not.  I can certainly envision an
architecture which has a very simple struct return convention, but for
which it's immensely complicated to implement the mechanics of that
convention.  (E.g, maybe a lot of twiddling is extract/store parts of
the code to get all the bits in the correct places.) In such a case,
having an easy to read "use_struct_convention" function would make it
easier to verify with respect to an ABI doc.

> > But this is also a con because you've spread the definition of
> > "use_struct_convention" out over a much larger number of lines.  It
> > isn't (IMO) as easy to comprehend when arranged in this way.
> 
> As my spec for the interface points out, if this were an OO language it 
> would have a different interface.
> 
> > The jury is still out (at least as far as I'm concerned) as to
> > which approach is better.  I do happen to think that your approach
> > is better for ppc64 (and ppc too), but this may not necessarily be the
> > case for other architectures.
> 
> This is a technical problem.  The return_value patch fixes the problem 
> of GDB not being able to handle all cases of:
> 
> 	return return-value-in-register
> 
> correctly.  If Corinna instead implements use_struct_convention, then 
> that bug won't be fixed.

You're referring to your WIP patch, right?  (Or did I miss a commit?)
You could certainly arrange to have you're WIP patch use the current
interfaces, couldn't you?  If you did, then that bug would be fixed
even with the traditional use_struct_convention code.

Kevin

Re: [RFA] sh-tdep.c (sh_use_struct_convention): Restructure and fix

[Andrew Cagney]

> On Oct 4,  1:35pm, Andrew Cagney wrote:
> 
> 
>> >> See: http://sources.redhat.com/ml/gdb-patches/2003-10/msg00033.html.
>> >> The ppc64_sysv_return_value code in ppc-sysv-tdep.c, has been written in 
>> >> a way that allows a quick update to this new iterface.
> 
>> > 
>> > Andrew,
>> > 
>> > There are pros and cons to the approach that you used in
>> > ppc64_sysv_abi_return_value().
>> > 
>> > On the pro side - and this is definitely a good thing - you keep the
>> > struct convention information together with the implementation of how
>> > to return a value.
> 
>> 
>> It is stronger than that, it moves all of the ABIs struct convention 
>> logic to one place.
> 
> 
> Huh?  What other parts are there?  (I fail to see why it's stronger
> than what I stated.)

See my reply to corinna.  The core functions set_return_value and 
using_struct_return both make assumptions about the ABIs return-value 
conventioins.  In particular, set_return_value doesn't allow the return 
of any structs, even when the ABI allows it.  value_being_returned was 
similar, until I restructured it.

>> At present key parts of the logic are scattered 
>> inconsistently across core parts of GDB
> 
> 
> Please explain.  How did the (traditional) ppc struct return
> mechanisms get scattered across core parts of GDB?

See above.

>> The result is a longer but more correct function.
> 
> 
> More correct?  There is absolutely nothing which prevents a
> traditional "use_struct_convention" function from being correct. 
> What's "more correct" than "correct"?

See above.

Of the existing architectures only sparc and m68hc11 thought to 
implement RETURN_VALUE_ON_STACK.  This is worrying since I don't believe 
it to be possible to implement a typical modern ABI without either:

- splitting the logic between RETURN_VALUE_ON_STACK and 
USE_STRUCT_CONVENTION (or making RETURN_VALUE_ON_STACK return 1).

- implementing the logic once in return_value

Ex: The PPC64's char array, and float/complex conventions.  I'd expect 
the SH ABI (assuming that someone can find it?) to have similar edge cases.

> I'll grant you that it may be easier to verify correctness by having
> the struct convention code placed side by side with the value
> (extract/store) return code.
> 
> And then again, it might not.  I can certainly envision an
> architecture which has a very simple struct return convention, but for
> which it's immensely complicated to implement the mechanics of that
> convention.  (E.g, maybe a lot of twiddling is extract/store parts of
> the code to get all the bits in the correct places.) In such a case,
> having an easy to read "use_struct_convention" function would make it
> easier to verify with respect to an ABI doc.

Per my comment below, if this were an OO language it would have been 
implemented differently.  "return_value" would likely return an object 
that contained return-value read and write methods (While I'm still 
tempted to do this, I think doing it would be seriously overengineering 
the interface).  Also, for the case you describe, it could easily 
written as:

	if (value in register)
	  if (inval)
	    extract_return_value ()
	  if (outval)
	    store_return_value ()
	  return RETURN_VALUE_REGISTER_CONVENTION;
	else
	  return RETURN_VALUE_STRUCT_CONVENTION;

>> > But this is also a con because you've spread the definition of
>> > "use_struct_convention" out over a much larger number of lines.  It
>> > isn't (IMO) as easy to comprehend when arranged in this way.
> 
>> 
>> As my spec for the interface points out, if this were an OO language it 
>> would have a different interface.
>> 
> 
>> > The jury is still out (at least as far as I'm concerned) as to
>> > which approach is better.  I do happen to think that your approach
>> > is better for ppc64 (and ppc too), but this may not necessarily be the
>> > case for other architectures.
> 
>> 
>> This is a technical problem.  The return_value patch fixes the problem 
>> of GDB not being able to handle all cases of:
>> 
>> 	return return-value-in-register
>> 
>> correctly.  If Corinna instead implements use_struct_convention, then 
>> that bug won't be fixed.
> 
> 
> You're referring to your WIP patch, right?  (Or did I miss a commit?)
> You could certainly arrange to have you're WIP patch use the current
> interfaces, couldn't you?  If you did, then that bug would be fixed
> even with the traditional use_struct_convention code.

No.  That isn't possible.  See:

> This also finally makes it possible to fix:
> http://sources.redhat.com/cgi-bin/gnatsweb.pl?cmd=view%20audit-trail&database=gdb&pr=659
> along with many many other cases where GDB was incorrectly claiming that it wasn't able to find a return value.

the current store_return_value interface is _not_ required to handle the 
storing of small struct conventions in registers.  The new interface is.

Andrew

Re: [RFA] sh-tdep.c (sh_use_struct_convention): Restructure and fix

[Corinna Vinschen]

On Sat, Oct 04, 2003 at 11:54:09AM -0400, Andrew Cagney wrote:
> >	* sh-tdep.c (sh_use_struct_convention): Clean up to have a
> >	more readable code.  Accomodate 4 byte structs with 4 byte sized
> >	first field (e.g. bitfields).
> 
> Corinna,
> 
> See: http://sources.redhat.com/ml/gdb-patches/2003-10/msg00033.html.
> The ppc64_sysv_return_value code in ppc-sysv-tdep.c, has been written in 
> a way that allows a quick update to this new iterface.

I think my patch is fine.  I'm fixing non-deprecated functions to
work better than before.  There's nothing wrong with this and I
don't see any gain to use an entirely new technique (2 days old!)
to get probably new errors which I don't have using this method.

Fixing the bugs is step 1.  Converting them to a new technique is
another, later step.

Corinna

-- 
Corinna Vinschen
Cygwin Developer
Red Hat, Inc.

Re: [RFA] sh-tdep.c (sh_use_struct_convention): Restructure and fix

[Andrew Cagney]

> On Sat, Oct 04, 2003 at 11:54:09AM -0400, Andrew Cagney wrote:
> 
>> >	* sh-tdep.c (sh_use_struct_convention): Clean up to have a
>> >	more readable code.  Accomodate 4 byte structs with 4 byte sized
>> >	first field (e.g. bitfields).
> 
>> 
>> Corinna,
>> 
>> See: http://sources.redhat.com/ml/gdb-patches/2003-10/msg00033.html.
>> The ppc64_sysv_return_value code in ppc-sysv-tdep.c, has been written in 
>> a way that allows a quick update to this new iterface.
> 
> 
> I think my patch is fine.  I'm fixing non-deprecated functions to
> work better than before.  There's nothing wrong with this and I
> don't see any gain to use an entirely new technique (2 days old!)
> to get probably new errors which I don't have using this method.

Corinna, here are some URLs that pretty much spell out the fact that 
these changes are comming.  They are even prompted by problems you, 
yourself, identifed.

2003-09-29: [wip] return value architecture method
http://sources.redhat.com/ml/gdb-patches/2003-09/msg00616.html
which was me giving a very clear heads up that the method was comming.
2003-09-28: value to function?
http://sources.redhat.com/ml/gdb/2003-09/msg00345.html
2003-09-20: [rfa:ppc64] Fix return value
http://sources.redhat.com/ml/gdb-patches/2003-09/msg00435.html
For the origin of that version.
2003-08-??: [rfc] Supporting alternative ABIs
http://sources.redhat.com/ml/gdb/2003-08/msg00252.html
For for a potential future need to add "struct value *function".
2003-09-30: Phone discussion where I alerted you to these pending changes.

> Fixing the bugs is step 1.  Converting them to a new technique is
> another, later step.

It stops the existing test cases failing.  Unfortunatly the testsuite is 
deficient in this area, lacking proper coverage of this feature (would 
you like to help me improve the testsuite?).

Unless the new _return_value method is used, GDB refuses to handle:

	(gdb) list
	struct { float f; } s;
	(gdb) return s;

In fact, your SH changes to store_return_value:
http://sources.redhat.com/ml/gdb-patches/2003-10/msg00077.html
that try to handle this specific case are never executed!  See 
set_return_value.

So?  There's a judgment call here - will GDB be better off using the old 
or new technique during this transition period?  Here, I think it is 
questionably the case that it is in everyones best interest to make the 
switch.  This is because things will be in a much better position for 
the change to add a "struct value *function" method to the return-value 
code (I assume that that you're still working towards that).  The old 
code would have required mods to four architecture methods 
(RETURN_VALUE_ON_STACK was missed) while the new code will involve mods 
to only one.

I should also note that what I'm suggesting here isn't exactly rocket 
science.  Just grab extract_return_value, or store_return_value, and 
massage it into return_value.  I just did it for ppc32 and it took an 
hour, at max in the case of ppc64 it was a day.

Andrew

Re: [RFA] sh-tdep.c (sh_use_struct_convention): Restructure and fix

[Corinna Vinschen]

Andrew,

On Mon, Oct 06, 2003 at 11:52:10AM -0400, Andrew Cagney wrote:
> >On Sat, Oct 04, 2003 at 11:54:09AM -0400, Andrew Cagney wrote:
> >>>	* sh-tdep.c (sh_use_struct_convention): Clean up to have a
> >>>	more readable code.  Accomodate 4 byte structs with 4 byte sized
> >>>	first field (e.g. bitfields).
> >
> Corinna, here are some URLs that pretty much spell out the fact that 
> these changes are comming.  They are even prompted by problems you, 
> yourself, identifed.
> 
> 2003-09-29: [wip] return value architecture method
> http://sources.redhat.com/ml/gdb-patches/2003-09/msg00616.html
> which was me giving a very clear heads up that the method was comming.

so the premonition time was a whole week.  I'm impressed.  I'm sorry that
I missed this due to being busy enough to concentrate on my target.

> 2003-09-20: [rfa:ppc64] Fix return value
> http://sources.redhat.com/ml/gdb-patches/2003-09/msg00435.html
> For the origin of that version.

Again sorry, I don't read postings about targets I have no relation to.

> 2003-09-30: Phone discussion where I alerted you to these pending changes.

I don't think that this belongs to gdb-patches.

> Unless the new _return_value method is used, GDB refuses to handle:
> 
> 	(gdb) list
> 	struct { float f; } s;
> 	(gdb) return s;
> 
> In fact, your SH changes to store_return_value:
> http://sources.redhat.com/ml/gdb-patches/2003-10/msg00077.html
> that try to handle this specific case are never executed!  See 
> set_return_value.

Which is arguably a bug in set_return_value(), not in my code.

> So?  There's a judgment call here - will GDB be better off using the old 
> or new technique during this transition period?  Here, I think it is 
> questionably the case that it is in everyones best interest to make the 
> switch.  This is because things will be in a much better position for 
> the change to add a "struct value *function" method to the return-value 
> code (I assume that that you're still working towards that).  The old 
> code would have required mods to four architecture methods 
> (RETURN_VALUE_ON_STACK was missed) while the new code will involve mods 
> to only one.

I don't think so since from my point of view your new functionality
has some interesting flaws or, at least, points which have to be
discussed first.  See my other posting replying to your wip posting.

Really, holding off a good working patch because another not yet
discussed functionality is glancing over the horizon for just 8 days
isn't quite useful, IMHO.

Corinna

-- 
Corinna Vinschen
Cygwin Developer
Red Hat, Inc.

Re: [RFA] sh-tdep.c (sh_use_struct_convention): Restructure and fix

[Andrew Cagney]

>> So?  There's a judgment call here - will GDB be better off using the old 
>> or new technique during this transition period?  Here, I think it is 
>> questionably the case that it is in everyones best interest to make the 
>> switch.  This is because things will be in a much better position for 
>> the change to add a "struct value *function" method to the return-value 
>> code

 >> (I assume that that you're still working towards that).

... are you still working towards this?

Andrew

Re: [RFA] sh-tdep.c (sh_use_struct_convention): Restructure and fix

[Elena Zannoni]

Corinna Vinschen writes:
 > On Sat, Oct 04, 2003 at 11:54:09AM -0400, Andrew Cagney wrote:
 > > >	* sh-tdep.c (sh_use_struct_convention): Clean up to have a
 > > >	more readable code.  Accomodate 4 byte structs with 4 byte sized
 > > >	first field (e.g. bitfields).
 > > 
 > > Corinna,
 > > 
 > > See: http://sources.redhat.com/ml/gdb-patches/2003-10/msg00033.html.
 > > The ppc64_sysv_return_value code in ppc-sysv-tdep.c, has been written in 
 > > a way that allows a quick update to this new iterface.
 > 
 > I think my patch is fine.  I'm fixing non-deprecated functions to
 > work better than before.  There's nothing wrong with this and I
 > don't see any gain to use an entirely new technique (2 days old!)
 > to get probably new errors which I don't have using this method.
 > 
 > Fixing the bugs is step 1.  Converting them to a new technique is
 > another, later step.

Hmm, I think you guys are all getting worked up for nothing, really.
Rereading the thread, I think Andrew was referring to a different way
to implement this set of ABI related functions, so that it would
become easier to do something else that Corinna had posted earlier,
namely supporting the Renesas ABI based on the current function,
i.e. here:
http://sources.redhat.com/ml/gdb-patches/2003-09/msg00524.html

To do what needed, we would have to pass the function address as
parameter to these sh-tdep.c functions. If we tighten the lot into
fewer methods, then there are less things to change.  So at this
point, what is still unresolved is whether or not somebody is going to
do the work, i.e. add an automatic mechanism to detect the abi.

If not, what Andrew is suggesting is not a priority.
If yes, it will need to be done.
And, really, this discussion belongs to the other thread.

As far as the change posted in this thread, I'll reply separately.

elena

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

Re: [RFA] sh-tdep.c (sh_use_struct_convention): Restructure and fix

[Andrew Cagney]

> Hmm, I think you guys are all getting worked up for nothing, really.
> Rereading the thread, I think Andrew was referring to a different way
> to implement this set of ABI related functions, so that it would
> become easier to do something else that Corinna had posted earlier,
> namely supporting the Renesas ABI based on the current function,
> i.e. here:
> http://sources.redhat.com/ml/gdb-patches/2003-09/msg00524.html
> 
> To do what needed, we would have to pass the function address as
> parameter to these sh-tdep.c functions. If we tighten the lot into
> fewer methods, then there are less things to change.  So at this
> point, what is still unresolved is whether or not somebody is going to
> do the work, i.e. add an automatic mechanism to detect the abi.
> 
> If not, what Andrew is suggesting is not a priority.
> If yes, it will need to be done.

Yes.

Andrew

Re: [RFA] sh-tdep.c (sh_use_struct_convention): Restructure and fix

[Elena Zannoni]

Corinna Vinschen writes:
 > Hi,
 > 
 > the below patch straightens out sh_use_struct_convention() so that it
 > allows a far better readability than before, especially by allowing
 > a bunch of comments spread out through the code.
 > 

I just added a detailed comment. Does that match what you implemented?
I'd prefer the use of 'aggregate' instead of 'struct' in your comments.

 > Additionally it fixes one bug:  A struct of lenght 4 bytes, which
 > consists of only a bitfield, is returned in register r0, not on the
 > stack using the struct convention.  So far, GDB got that wrong.
 > 

Is there a test case that was failing? If not, it should be added. 

 > Corinna
 > 
 > 	* sh-tdep.c (sh_use_struct_convention): Clean up to have a
 > 	more readable code.  Accomodate 4 byte structs with 4 byte sized
 > 	first field (e.g. bitfields).
 > 
 > 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
 > @@ -565,8 +565,25 @@ sh_use_struct_convention (int gcc_p, str
 >  {
 >    int len = TYPE_LENGTH (type);
 >    int nelem = TYPE_NFIELDS (type);
 > -  return ((len != 1 && len != 2 && len != 4 && len != 8) || nelem != 1) &&
 > -    (len != 8 || TYPE_LENGTH (TYPE_FIELD_TYPE (type, 0)) != 4);
 > +
 > +  /* Non-power of 2 length types and types bigger than 8 bytes (which don't
 > +     fit in two registers anyway) use struct convention.  */
 > +  if (len != 1 && len != 2 && len != 4 && len != 8)
 > +    return 1;
 > +  /* Structs with more than 1 fields use struct convention, if...  */
 > +  if (nelem != 1)
 > +    {
 > +      /* ... they are 1 or 2 bytes in size (e.g. struct of two chars)... */
 > +      if (len != 4 && len != 8)

Can you just say len == 1 or len == 2 so that it matches your comment?
Wait, this contradicts what the comments I just added say:

For example, a 2-byte aligned structure with size 2 bytes has the
same size and alignment as a short int, and will be returned in R0.

Which is correct?


 > +	return 1;

 > +      /* ... or, if the struct is 4 or 8 bytes and the first field is
 > +	 not of size 4 bytes.  Note that this also covers structs with
 > +	 bitfields. */
 > +      if (TYPE_LENGTH (TYPE_FIELD_TYPE (type, 0)) != 4)

I am not sure I understand this one, that's why asked a pointer to a
test case. It seems to contradict the following, i.e. it should still
be in registers, or maybe I don't understand the language:

When an aggregate type is returned in R0 and R1, R0 contains the first
four bytes of the aggregate, and R1 contains the remainder. If the size
of the aggregate type is not a multiple of 4 bytes, the aggregate is
tail-padded up to a multiple of 4 bytes. The value of the padding is
undefined.


elena


 > +	return 1;
 > +    }
 > +  /* Otherwise return in registers.  */
 > +  return 0;
 >  }
 >  
 >  /* Extract from an array REGBUF containing the (raw) register state
 > 
 > -- 
 > Corinna Vinschen
 > Cygwin Developer
 > Red Hat, Inc.

Re: [RFA] sh-tdep.c (sh_use_struct_convention): Restructure and fix

[Corinna Vinschen]

On Thu, Oct 09, 2003 at 05:14:53PM -0400, Elena Zannoni wrote:
> Corinna Vinschen writes:
>  > Hi,
>  > 
>  > the below patch straightens out sh_use_struct_convention() so that it
>  > allows a far better readability than before, especially by allowing
>  > a bunch of comments spread out through the code.
>  > 
> 
> I just added a detailed comment. Does that match what you implemented?
> I'd prefer the use of 'aggregate' instead of 'struct' in your comments.

Yes, thanks, I saw the comment.  It's enlightening.  However, the
first sentence seems to be a copy/paste hangover:

    /* Should call_function allocate stack space for a struct return?

And even though I have to admit, that I'm not 100% sure (perhaps
I miss a case) I think the implementation should match at least 99%
of the description. 

The difference between the old and the new code is given by allowing
4 byte structs (erm, aggregates) with more than one element, but a size
of 4 byte for the first element.  This sounds somewhat weird, but that's
exactly the case if the 4 byte agregate is a bitfield or contains a
bitfield.  So the change in this patch solves exactly these bitfields
as return type problem.

>  > Additionally it fixes one bug:  A struct of lenght 4 bytes, which
>  > consists of only a bitfield, is returned in register r0, not on the
>  > stack using the struct convention.  So far, GDB got that wrong.
> 
> Is there a test case that was failing? If not, it should be added. 

Yes, testcases which uncover that problem exist in call-ar-st and
call-rt-st.

Actually the whole change was a result of these testcases.  I saw the
bitfield problem but I found the former one-expression evaluation
very unreadable.  So, first I straightened out the expression, then
I added the bitfield case.

>  > +  if (len != 1 && len != 2 && len != 4 && len != 8)
>  > +    return 1;
>  > +  /* Structs with more than 1 fields use struct convention, if...  */
>  > +  if (nelem != 1)
>  > +    {
>  > +      /* ... they are 1 or 2 bytes in size (e.g. struct of two chars)... */
>  > +      if (len != 4 && len != 8)
> 
> Can you just say len == 1 or len == 2 so that it matches your comment?

Sure.  No problem to change this.

> Wait, this contradicts what the comments I just added say:
> 
> For example, a 2-byte aligned structure with size 2 bytes has the
> same size and alignment as a short int, and will be returned in R0.
> 
> Which is correct?

Both.  An aggregate of size 1 or 2 byte with more than 1 element is not
correctly alligned so it will not be returned in R0.

>  > +      /* ... or, if the struct is 4 or 8 bytes and the first field is
>  > +	 not of size 4 bytes.  Note that this also covers structs with
>  > +	 bitfields. */
>  > +      if (TYPE_LENGTH (TYPE_FIELD_TYPE (type, 0)) != 4)
> 
> I am not sure I understand this one, that's why asked a pointer to a
> test case. It seems to contradict the following, i.e. it should still
> be in registers, or maybe I don't understand the language:
> 
> When an aggregate type is returned in R0 and R1, R0 contains the first
> four bytes of the aggregate, and R1 contains the remainder. If the size
> of the aggregate type is not a multiple of 4 bytes, the aggregate is
> tail-padded up to a multiple of 4 bytes. The value of the padding is
> undefined.

Is my above description better?  The code is identical to the former
implementation except for the 4 byte bitfield case.  That one is now
covered here.

I have not attached a new patch, but I've noted to change "struct" to
"aggregate" in the comments and the
  if (len != 4 && len != 8)
to
  if (len == 1 || len == 2)

Corinna

-- 
Corinna Vinschen
Cygwin Developer
Red Hat, Inc.

Re: [RFA] sh-tdep.c (sh_use_struct_convention): Restructure and fix

[Corinna Vinschen]

On Fri, Oct 10, 2003 at 09:29:29AM +0200, Corinna Vinschen wrote:
> And even though I have to admit, that I'm not 100% sure (perhaps
> I miss a case) I think the implementation should match at least 99%
> of the description. 
> 
> The difference between the old and the new code is given by allowing
> 4 byte structs (erm, aggregates) with more than one element, but a size
> of 4 byte for the first element.  This sounds somewhat weird, but that's
> exactly the case if the 4 byte agregate is a bitfield or contains a
> bitfield.  So the change in this patch solves exactly these bitfields
> as return type problem.

...and I just found out what this patch does *not* cover.  It does
not cover the case of bitfields of size 1 or 2 bytes :-(

I rewrote the implementation of sh_use_struct_convention and tested it
again.  Below is the entire implementation instead of the patch.  I think
this is easier to read.  I also tried to match the comments even better
to the actual code.  Is that ok to check in?

Btw., this time, there *are* tests missing.  The testsuite doesn't check
for returning bitfield types of size 1 and 2 bytes.  I'm going to add
two tests to call-rt-st.exp which I hope to submit at least Monday.

Corinna

========== SNIP ===========
static int
sh_use_struct_convention (int gcc_p, struct type *type)
{
  int len = TYPE_LENGTH (type);
  int nelem = TYPE_NFIELDS (type);

  /* Non-power of 2 length types and types bigger than 8 bytes (which don't
     fit in two registers anyway) use struct convention.  */
  if (len != 1 && len != 2 && len != 4 && len != 8)
    return 1;

  /* Scalar types and aggregate types with exactly one field are aligned
     by definition.  They are returned in registers.  */
  if (nelem <= 1)
    return 0;

  /* If the first field in the aggregate has the same length as the entire
     aggregate type, the type is returned in registers.  */
  if (TYPE_LENGTH (TYPE_FIELD_TYPE (type, 0)) == len)
    return 0;

  /* If the size of the aggregate is 8 bytes and the first field is
     of size 4 bytes its alignment is equal to long long's alignment,
     so it's returned in registers.  */
  if (len == 8 && TYPE_LENGTH (TYPE_FIELD_TYPE (type, 0)) == 4)
    return 0;

  /* Otherwise use struct convention.  */
  return 1;
}
========== SNAP ===========


-- 
Corinna Vinschen
Cygwin Developer
Red Hat, Inc.

Re: [RFA] sh-tdep.c (sh_use_struct_convention): Restructure and fix

[Elena Zannoni]

Corinna Vinschen writes:
 > On Fri, Oct 10, 2003 at 09:29:29AM +0200, Corinna Vinschen wrote:
 > > And even though I have to admit, that I'm not 100% sure (perhaps
 > > I miss a case) I think the implementation should match at least 99%
 > > of the description. 
 > > 
 > > The difference between the old and the new code is given by allowing
 > > 4 byte structs (erm, aggregates) with more than one element, but a size
 > > of 4 byte for the first element.  This sounds somewhat weird, but that's
 > > exactly the case if the 4 byte agregate is a bitfield or contains a
 > > bitfield.  So the change in this patch solves exactly these bitfields
 > > as return type problem.
 > 
 > ...and I just found out what this patch does *not* cover.  It does
 > not cover the case of bitfields of size 1 or 2 bytes :-(
 > 

Argh, I just noticed this mail. 

 > I rewrote the implementation of sh_use_struct_convention and tested it
 > again.  Below is the entire implementation instead of the patch.  I think
 > this is easier to read.  I also tried to match the comments even better
 > to the actual code.  Is that ok to check in?
 > 
 > Btw., this time, there *are* tests missing.  The testsuite doesn't check
 > for returning bitfield types of size 1 and 2 bytes.  I'm going to add
 > two tests to call-rt-st.exp which I hope to submit at least Monday.
 > 
 > Corinna
 > 
 > ========== SNIP ===========
 > static int
 > sh_use_struct_convention (int gcc_p, struct type *type)
 > {
 >   int len = TYPE_LENGTH (type);
 >   int nelem = TYPE_NFIELDS (type);
 > 
 >   /* Non-power of 2 length types and types bigger than 8 bytes (which don't
 >      fit in two registers anyway) use struct convention.  */
 >   if (len != 1 && len != 2 && len != 4 && len != 8)
 >     return 1;
 > 
 >   /* Scalar types and aggregate types with exactly one field are aligned
 >      by definition.  They are returned in registers.  */
 >   if (nelem <= 1)
 >     return 0;
 > 
 >   /* If the first field in the aggregate has the same length as the entire
 >      aggregate type, the type is returned in registers.  */
 >   if (TYPE_LENGTH (TYPE_FIELD_TYPE (type, 0)) == len)
 >     return 0;
 > 
 >   /* If the size of the aggregate is 8 bytes and the first field is
 >      of size 4 bytes its alignment is equal to long long's alignment,
 >      so it's returned in registers.  */
 >   if (len == 8 && TYPE_LENGTH (TYPE_FIELD_TYPE (type, 0)) == 4)
 >     return 0;
 > 
 >   /* Otherwise use struct convention.  */
 >   return 1;
 > }


Much better, however, I would still like to know what the behavior is
for a struct of 2 chars.  Probably this needs another test case.

elena


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

Re: [RFA] sh-tdep.c (sh_use_struct_convention): Restructure and fix

[Corinna Vinschen]

On Fri, Oct 10, 2003 at 12:43:27PM -0400, Elena Zannoni wrote:
> Corinna Vinschen writes:
>  > static int
>  > sh_use_struct_convention (int gcc_p, struct type *type)
>  > {
>  >   int len = TYPE_LENGTH (type);
>  >   int nelem = TYPE_NFIELDS (type);
>  > 
>  >   /* Non-power of 2 length types and types bigger than 8 bytes (which don't
>  >      fit in two registers anyway) use struct convention.  */
>  >   if (len != 1 && len != 2 && len != 4 && len != 8)
>  >     return 1;
>  > 
>  >   /* Scalar types and aggregate types with exactly one field are aligned
>  >      by definition.  They are returned in registers.  */
>  >   if (nelem <= 1)
>  >     return 0;
>  > 
>  >   /* If the first field in the aggregate has the same length as the entire
>  >      aggregate type, the type is returned in registers.  */
>  >   if (TYPE_LENGTH (TYPE_FIELD_TYPE (type, 0)) == len)
>  >     return 0;
>  > 
>  >   /* If the size of the aggregate is 8 bytes and the first field is
>  >      of size 4 bytes its alignment is equal to long long's alignment,
>  >      so it's returned in registers.  */
>  >   if (len == 8 && TYPE_LENGTH (TYPE_FIELD_TYPE (type, 0)) == 4)
>  >     return 0;
>  > 
>  >   /* Otherwise use struct convention.  */
>  >   return 1;
>  > }
> 
> Much better, however, I would still like to know what the behavior is
> for a struct of 2 chars.  Probably this needs another test case.

A struct of two chars is returned in memory.  That's correct.

Corinna

-- 
Corinna Vinschen
Cygwin Developer
Red Hat, Inc.

Re: [RFA] sh-tdep.c (sh_use_struct_convention): Restructure and fix

[Elena Zannoni]

Corinna Vinschen writes:
 > On Fri, Oct 10, 2003 at 12:43:27PM -0400, Elena Zannoni wrote:
 > > Corinna Vinschen writes:
 > >  > static int
 > >  > sh_use_struct_convention (int gcc_p, struct type *type)
 > >  > {
 > >  >   int len = TYPE_LENGTH (type);
 > >  >   int nelem = TYPE_NFIELDS (type);
 > >  > 
 > >  >   /* Non-power of 2 length types and types bigger than 8 bytes (which don't
 > >  >      fit in two registers anyway) use struct convention.  */
 > >  >   if (len != 1 && len != 2 && len != 4 && len != 8)
 > >  >     return 1;
 > >  > 
 > >  >   /* Scalar types and aggregate types with exactly one field are aligned
 > >  >      by definition.  They are returned in registers.  */
 > >  >   if (nelem <= 1)
 > >  >     return 0;
 > >  > 
 > >  >   /* If the first field in the aggregate has the same length as the entire
 > >  >      aggregate type, the type is returned in registers.  */
 > >  >   if (TYPE_LENGTH (TYPE_FIELD_TYPE (type, 0)) == len)
 > >  >     return 0;
 > >  > 
 > >  >   /* If the size of the aggregate is 8 bytes and the first field is
 > >  >      of size 4 bytes its alignment is equal to long long's alignment,
 > >  >      so it's returned in registers.  */
 > >  >   if (len == 8 && TYPE_LENGTH (TYPE_FIELD_TYPE (type, 0)) == 4)
 > >  >     return 0;
 > >  > 
 > >  >   /* Otherwise use struct convention.  */
 > >  >   return 1;
 > >  > }
 > > 
 > > Much better, however, I would still like to know what the behavior is
 > > for a struct of 2 chars.  Probably this needs another test case.
 > 
 > A struct of two chars is returned in memory.  That's correct.

OK, after careful re-re-re-parsing of the description, I think so too.

I'll add another comment to explain that case better.

This can go in.

elena


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

Re: [RFA] sh-tdep.c (sh_use_struct_convention): Restructure and fix

[Corinna Vinschen]

On Fri, Oct 10, 2003 at 02:07:45PM -0400, Elena Zannoni wrote:
> This can go in.

Thanks, applied.

Corinna

-- 
Corinna Vinschen
Cygwin Developer
Red Hat, Inc.

Re: [RFA] sh-tdep.c (sh_use_struct_convention): Restructure and fix

[Elena Zannoni]

Corinna Vinschen writes:
 > On Thu, Oct 09, 2003 at 05:14:53PM -0400, Elena Zannoni wrote:
 > > Corinna Vinschen writes:
 > >  > Hi,
 > >  > 
 > >  > the below patch straightens out sh_use_struct_convention() so that it
 > >  > allows a far better readability than before, especially by allowing
 > >  > a bunch of comments spread out through the code.
 > >  > 
 > > 
 > > I just added a detailed comment. Does that match what you implemented?
 > > I'd prefer the use of 'aggregate' instead of 'struct' in your comments.
 > 
 > Yes, thanks, I saw the comment.  It's enlightening.  However, the
 > first sentence seems to be a copy/paste hangover:
 > 
 >     /* Should call_function allocate stack space for a struct return?
 > 
 > And even though I have to admit, that I'm not 100% sure (perhaps
 > I miss a case) I think the implementation should match at least 99%
 > of the description. 
 > 
 > The difference between the old and the new code is given by allowing
 > 4 byte structs (erm, aggregates) with more than one element, but a size
 > of 4 byte for the first element.  This sounds somewhat weird, but that's
 > exactly the case if the 4 byte agregate is a bitfield or contains a
 > bitfield.  So the change in this patch solves exactly these bitfields
 > as return type problem.
 > 
 > >  > Additionally it fixes one bug:  A struct of lenght 4 bytes, which
 > >  > consists of only a bitfield, is returned in register r0, not on the
 > >  > stack using the struct convention.  So far, GDB got that wrong.
 > > 
 > > Is there a test case that was failing? If not, it should be added. 
 > 
 > Yes, testcases which uncover that problem exist in call-ar-st and
 > call-rt-st.
 > 
 > Actually the whole change was a result of these testcases.  I saw the
 > bitfield problem but I found the former one-expression evaluation
 > very unreadable.  So, first I straightened out the expression, then
 > I added the bitfield case.
 > 
 > >  > +  if (len != 1 && len != 2 && len != 4 && len != 8)
 > >  > +    return 1;
 > >  > +  /* Structs with more than 1 fields use struct convention, if...  */
 > >  > +  if (nelem != 1)
 > >  > +    {
 > >  > +      /* ... they are 1 or 2 bytes in size (e.g. struct of two chars)... */
 > >  > +      if (len != 4 && len != 8)
 > > 
 > > Can you just say len == 1 or len == 2 so that it matches your comment?
 > 
 > Sure.  No problem to change this.
 > 
 > > Wait, this contradicts what the comments I just added say:
 > > 
 > > For example, a 2-byte aligned structure with size 2 bytes has the
 > > same size and alignment as a short int, and will be returned in R0.
 > > 
 > > Which is correct?
 > 
 > Both.  An aggregate of size 1 or 2 byte with more than 1 element is not
 > correctly alligned so it will not be returned in R0.
 > 

How does this get returned?
struct {char a; char b;} Shouldn't this be in R0 with some padding?
You code would return it in memory.

elena


 > >  > +      /* ... or, if the struct is 4 or 8 bytes and the first field is
 > >  > +	 not of size 4 bytes.  Note that this also covers structs with
 > >  > +	 bitfields. */
 > >  > +      if (TYPE_LENGTH (TYPE_FIELD_TYPE (type, 0)) != 4)
 > > 
 > > I am not sure I understand this one, that's why asked a pointer to a
 > > test case. It seems to contradict the following, i.e. it should still
 > > be in registers, or maybe I don't understand the language:
 > > 
 > > When an aggregate type is returned in R0 and R1, R0 contains the first
 > > four bytes of the aggregate, and R1 contains the remainder. If the size
 > > of the aggregate type is not a multiple of 4 bytes, the aggregate is
 > > tail-padded up to a multiple of 4 bytes. The value of the padding is
 > > undefined.
 > 
 > Is my above description better?  The code is identical to the former
 > implementation except for the 4 byte bitfield case.  That one is now
 > covered here.
 > 
 > I have not attached a new patch, but I've noted to change "struct" to
 > "aggregate" in the comments and the
 >   if (len != 4 && len != 8)
 > to
 >   if (len == 1 || len == 2)
 > 



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