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

Elena Zannoni
2003-10-10T16:28:08Z

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.