Re: [PATCH] libffi support for CRIS

[Hans-Peter Nilsson <hp at bitrange dot com>]

I'll fix up all issues below and would like to start negotiation
with libffi approvers.  I'd like to hear from them about this
patch (for reference,
<URL:http://gcc.gnu.org/ml/gcc-patches/2004-10/msg02477.html>).

I ask for approval on the target-independent changes there,
minus the globalization of initialize_aggregate plus comments
at the #ifdef additions to the effect of
/* The CRIS ABI specifies structure elements to the effect of
   byte alignment only, so it completely overrides this
   functions, which assumes "natural" alignment and padding.  */

On Thu, 28 Oct 2004, Simon Posnjak wrote:

Huh, again sent as an attachment, not in-text.  It takes a
little longer to review it this way.  Though that's not the
main reason for this delay, sorry.

> V Ä?et, 28.10.2004 ob 05:19 je Hans-Peter Nilsson napisal(a):
> > > +++ gcc_cris/libffi/src/cris/ffi.c	2004-10-26 12:30:15.674498944 +0200
> >
> > Not including the copyright header, I didn't see a *single
> > comment* in this file.  I had to look at the other targets.

I still don't see *any* comments in cris/ffi.c and a few "{
statement; }" where no braces should be as per the coding
standards.

> > > +ffi_status
> > > +ffi_prep_cif_machdep (ffi_cif * cif,
> >
> > > +  if ((cif->rtype->size == 0)
> > > +      && (initialize_aggregate (cif->rtype) != FFI_OK))
> >
> > Don't you mean initialize_aggregate_packed_struct here?
> No, thats correct - this one works on aligned parameters, not packed.

What do you mean by that?  The return value doesn't have any
different layout than parameters; it's "packed" too.  I see no
reason to not call initialize_aggregate_packed_struct there.

> > > +	;; Used for stack pointer saving.
> > > +	push $r8
> >
> > Still suboptimal register allocation, wrt. my earlier movem comment.
> This is still open. I do not have the time (and my knowledge of CRIS asm
> is not that good) to fix it right now. (I tried a quick fix but I am
> getting just segfaults).

If you care to revisit some time, look at output from an -O2
compilation.

> > On the other hand, you should wrap the *whole* function in #ifdef __CRIS__
> > (you *could* also do that for initialize_aggregate) and rename the CRIS
> > ffi_prep_cif_machdep to ffi_prep_cif, completely replacing it.
> > (That's what I refer to at the top.)
> Done.

Much better.  A comment at the #ifdef would help followers,
though.

brgds, H-P