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

Corinna Vinschen
2003-10-10T07:29:35Z

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.