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

[Corinna Vinschen <vinschen at redhat dot com>]

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.