This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH] libffi support for CRIS
(CC as per
<URL:http://gcc.gnu.org/ml/gcc-patches/2004-10/msg02173.html>.
This is a reply to
<URL:http://gcc.gnu.org/ml/gcc-patches/2004-10/msg02170.html>
which didn't CC java-patches.)
Again, thanks for your work!
If you're able to do all the requested fixups I'll be thankful.
I'm ok with your interpretation of what should be in copyright
headers. (If libffi maintainers think otherwise, they'll
override.) I'll handle shepherding of the port past libffi
maintainers after your reply (and hopefully last patch
iteration?)
On Tue, 26 Oct 2004, Simon Posnjak wrote:
> V tor, 26.10.2004 ob 04:27 je Hans-Peter Nilsson napisal(a):
> > Over to the actual patch: Please use the "-p" diff option ("diff
> > -up", not just "diff -u"). Sending the patch as an attachment
> > (rather than in-body) made it harder to comment on each piece
> > (to the crowd: comments on my choice of mailer to /dev/null).
> OK. I will do it like that.
(Except this too was in an attachment. Maybe next time? ;-)
>
> > Same comment applies as to boehm-gc, CRIS32 is a bit confusing.
> > I'd rather just use the predefined __CRIS__ and __linux__ (eh,
> > make that __gnu_linux__ ;-) and not define anything in
> > configury. Or maybe define CRIS_LINUX.
> I choose CRIS_LINUX.
>
> > I can't approve this as-is, as it touches target-independent
> > libffi bits (which I anyway think should be handled with less
> > #ifdefs).
> I moved the stuff to target-depended part of libffi.
Progress, but a bit more needed, IMO.
> > +++ gcc_cris/libffi/src/cris/ffi.c 2004-10-25 12:58:24.103724264 +0200
> > > +/* -----------------------------------------------------------------------
> > > + ffi.c - Copyright (c) 1998 Cygnus Solutions
> > > + Copyright (c) 2004 Simon Posnjak
> >
> > I doubt Cygnus Solutions should be credited here.
> Yes, they should be be, because the file came from arm/ffi.c and was
> adopted for cris. (The copyright headers are correct on all files)
In the GCC repo, the arm/ffi.c header is updated to say
"Red Hat Inc". But I see what you mean.
(Hm, I hope your patch applies to the GCC repo.)
> > > +++ gcc_cris/libffi/src/cris/ffitarget.h 2004-10-25 12:58:24.104724112 +0200
> > > @@ -0,0 +1,47 @@
> > > +/* -----------------------------------------------------------------*-C-*-
> > > + ffitarget.h - Copyright (c) 1996-2003 Red Hat, Inc.
> >
> > Why is Red Hat mentioned here? Copy-paste?
> > Please resend with *at least* this fixed.
> The file is exatcly the same as in arm/ffitarget.h. (s/ARM/CRIS/g)
Hm, I see your point.
There are many targets for which this file is the same. This
*should* be a generic header used as a default, but that's
supposedly a fallacy of the libffi framework, which shouldn't be
altered in this patch.
> > Hm, if the area ecif.rvalue points to is *always* at least 64
> > bits long, there's no need for different epilogues. You could
> > remove all the above rval type compares and branches and just do
> > this part:
> It is not always 64 bits long. Only if return value is long long or double, all
> others are 32 bit. Or am I missing something?
A thinko on my part.
Over to your updated patch:
> --- gcc/libffi/include/ffi_common.h 2004-08-30 17:43:02.000000000 +0200
> +#ifdef __cris__
The preferred macro is __CRIS__, but...
> +ffi_status ffi_prep_cif_machdep (ffi_cif * cif,
> + ffi_abi abi,
> + unsigned int nargs,
> + ffi_type * rtype, ffi_type ** atypes);
> +#else
> ffi_status ffi_prep_cif_machdep(ffi_cif *cif);
> +#endif
...this should be general, not target-dependent -- *if* it had
been a good change. But... both arguments are moot, see further
below.
> +++ 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.
> +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?
> diff -uNrp gcc/libffi/src/cris/sysv.S gcc_cris/libffi/src/cris/sysv.S
> +ffi_call_SYSV:
> + ;; Save the regs to the stack.
> + push $srp
> + ;; Used for stack pointer saving.
> + push $r6
> + ;; Used for function address pointer.
> + push $r7
> + ;; Used for stack pointer saving.
> + push $r8
Still suboptimal register allocation, wrt. my earlier movem comment.
> + ;; Detect rval type.
> + cmpq FFI_TYPE_VOID,$r13
> + beq epilogue
> +
> + cmpq FFI_TYPE_STRUCT,$r13
> + beq epilogue
> +
> + cmpq FFI_TYPE_DOUBLE,$r13
> + beq return_double_or_longlong
> +
> + cmpq FFI_TYPE_UINT64,$r13
> + beq return_double_or_longlong
> +
> + cmpq FFI_TYPE_SINT64,$r13
> + beq return_double_or_longlong
> + nop
Maybe I should have added "...but please add (a) comment that the
delay-slot-filling by the next cmpq is deliberate". (A few other
delay-slots are also fillable, FWIW.)
> +++ gcc_cris/libffi/src/prep_cif.c 2004-10-26 12:04:38.000000000 +0200
> @@ -25,7 +25,6 @@
> #include <ffi_common.h>
> #include <stdlib.h>
>
> -
> /* Round up to FFI_SIZEOF_ARG. */
Gratuitous whitespace fix. I didn't see many, but try to avoid them.
(I'd hope they are welcome as a general *separate* patch though.)
> @@ -33,7 +32,7 @@
> /* Perform machine independent initialization of aggregate type
> specifications. */
>
> -static ffi_status initialize_aggregate(/*@out@*/ ffi_type *arg)
> +ffi_status initialize_aggregate(/*@out@*/ ffi_type *arg)
*If* you'd needed it, I think you should add a initialize_aggregate
declaration in a header somewhere... right, in ffi_common.h.
On the other hand, I *don't* think you don't need it. See comment at
callsite above.
> @@ -89,6 +88,9 @@ ffi_status ffi_prep_cif(/*@out@*/ /*@par
> /*@dependent@*/ /*@out@*/ /*@partial@*/ ffi_type *rtype,
> /*@dependent@*/ ffi_type **atypes)
> {
> +#ifdef __cris__
> + return ffi_prep_cif_machdep (cif, abi, nargs, rtype, atypes);
> +#else
> unsigned bytes = 0;
> unsigned int i;
> ffi_type **ptr;
> @@ -147,7 +149,7 @@ ffi_status ffi_prep_cif(/*@out@*/ /*@par
> /* Add any padding if necessary */
> if (((*ptr)->alignment - 1) & bytes)
> bytes = ALIGN(bytes, (*ptr)->alignment);
> -
> +
(The only other spurious whitespace fixup I noticed.)
> bytes += STACK_ARG_SIZE((*ptr)->size);
> }
> #endif
> @@ -157,4 +159,5 @@ ffi_status ffi_prep_cif(/*@out@*/ /*@par
>
> /* Perform machine dependent cif processing */
> return ffi_prep_cif_machdep(cif);
> +#endif
Nah, me not really happy with this solution. (For example, with
the prototype changed to include all the new parameters, you
could just #ifdef out all except the return statement in the
function; presumably better than replacing it.)
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.)
IMHO this is another fallacy of the libffi framework. Here, it
seems oblivious of the possibility of an ABI that doesn't pad
structures to some alignment. But fixing that should be a
separate patch, IMHO.
brgds, H-P
PS. It boggles the mind that the main reason for the CRIS ABI having
structures "packed" was once upon a time compatibility with
other compilers!