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

[Andrew Cagney <ac131313@redhat.com>]

> 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