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

[Elena Zannoni <ezannoni@redhat.com>]

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.