Re: [PATCH] libffi support for CRIS

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

On Mon, 25 Oct 2004, Simon Posnjak wrote:

> This patch adds support for CRIS platform to libffi.

Many thanks for your work!

I hoped to hear something from Anthony Green in the past week
about status on copyright assignment of libffi to the FSF
generally, but he hasn't replied, so let's continue as-is, and
hope that we can persuade your employer to do the
copyright-assignment if that ever becomes an issue.  (For the
record, Simon has gone through the process for gdb in the past,
so I have faith it can happen. ;-)

AFAIK, port copyright assignment is not an issue for libffi
currently, and people seem to commit ports left and right with
personal copyright headers, so it can't be an issue that Simon
doesn't have any (GCC) copyright assignment papers on file,
right?

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).

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 don't claim to understand libffi details (after all, I hadn't
written a CRIS port for it ;-) so maybe I shouldn't comment on
the implementation.  But then, nobody else would.  My comments
are mostly about coding standard nits: they nits stick out like
sore thumbs when you review code, so I can't help commenting on
them.  (I know not all code in libffi follows the GCC coding
standard, but that is to be improved, not used as an excuse.)

> The added initialize_aggregate_packed_struct function in
> libffi/src/prep_cif.c is needed because CRIS uses *packed* structs.

I can't approve this as-is, as it touches target-independent
libffi bits (which I anyway think should be handled with less
#ifdefs).

Still, I could pick up the patch, help it past libffi
maintainers, fixing *all* the following nits and issues (but I
hope you fix most), myself except one: you yourself have to fix
the copyright notice for ffitarget.h as commented below.

(I know that file is within ten lines of effective code, but I
will *not* change the notice related to copyright holder in
someone elses code whatsoever.)


> +++ gcc_cris/libffi/configure.ac	2004-10-25 12:58:24.102724416 +0200
> @@ -68,6 +68,7 @@
>  rs6000-*-aix*) TARGET=POWERPC_AIX; TARGETDIR=powerpc;;
>  arm*-*-linux-*) TARGET=ARM; TARGETDIR=arm;;
>  arm*-*-netbsdelf* | arm*-*-knetbsd*-gnu) TARGET=ARM; TARGETDIR=arm;;
> +cris*-*-linux-*) TARGET=CRIS32; TARGETDIR=cris;;

Better use "cris-..." and even "cris-*-linux*".  The ARM port is
not to be followed; it has other suffixes, the CRIS port in FSF
just uses "cris".  (At present that is, "crisv32" coming, FWIW.)

+++ 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.

> +//  if (ecif->cif->rtype->type == FFI_TYPE_STRUCT ) {
> +//    *(void **) argp = ecif->rvalue;
> +//    argp += 4;
> +//  }

No commented-out code please.  (Why is it there at all for this new code?
Was arm/ffi.c copied here?)

> +      /* Align if necessary */
> +      if (((*p_arg)->alignment - 1) & (unsigned) argp)
> +	{
> +	  argp = (char *) ALIGN (argp, (*p_arg)->alignment);
> +	}

Please explain why alignment can be necessary here.  Is it part
of the libffi API?  Nothing needs more than byte-alignment in
the CRIS ABI, but perhaps libffi supports structs with members
with __attribute__ ((__align__ (N))) directives with N larger
than normal alignment?

(Also, coding standard nits: no braces for single statements.
Comments are full sentences, with "." and two spaces after the
".".  Those two nits applies to all this code; I won't repeat
those two.)

> +  /* If the return value is a struct and we don't have a return */
> +  /* value address then we need to make one                     */

Misformatted comment.  It's:
/* If the return value is a struct and we don't have a return
   value address then we need to make one.  */

> +  else
> +    ecif.rvalue = rvalue;
> +
> +

Only one empty line, please.

> +++ 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.

> +typedef enum ffi_abi {

Formatting nit.

> +++ gcc_cris/libffi/src/cris/sysv.S	2004-10-25 12:58:24.105723960 +0200
> @@ -0,0 +1,163 @@

There's a mix of TAB and spaces in this file.  Also, inconsistent
formatting: I suggest you consistently drop the space after comma in "move
X,Y".  Best to format it like the gcc output.  (Consistent with that,
"move X,[Y+Z]", not "move X, [Y + Z]".  This is arguably a matter or taste
but please be consistent within the file.)

> +ffi_call_SYSV:
> +	;; save the regs to the stack
> +	Push $srp
> +	push $r6
> +	push $r7
> +	push $r8
> +	push $r13
> +	move.d $sp,$r8
> +	move.d $sp,$r6

I don't understand the choice of call-saved registers here.  Better use
$R0 and up, so they can be saved and restored with a single "MOVEM".
Comments on register use would also be helpful.

> +
> +	;; move address of ffi_prep_args to r13

Comments should consist of full sentences, ending in "." in assembly too.

> +	cmp.d $sp, $r6
> +
> +	bmi go_on_no_params_on_stack
> +	nop
> +
> +        beq go_on_no_params_on_stack
> +        nop
> +
> +	ba go_on
> +	nop
> +
> +go_on_no_params_on_stack:
> +	move.d $r6, $sp
> +
> +go_on:

Those branches look a bit strange.  The "beq" is redundant (copying over
same contents), so just "bpl go_on" would have done it.  Also, because I
prefer to use insns that relate to the compare, I'd use "bhs go_on"
(branch-higher-same); "bmi" (branch-minus) is less understandable IMHO.
(The stack-adjusting is a bit hard to follow as it is.)

> +	;; discover if we need to put rval address in to $r9
> +        move.d [$r8 + 0], $r7
> +	cmpq FFI_TYPE_STRUCT, $r7
> +	bne call_now
> +	nop
> +
> +	move.d [$r8 + 20], $r9
> +
> +call_now:
> +	;; move address of the function in to r7
> +	move.d [$r8 + 24], $r7
> +
> +	;; call the function
> +	jsr $r7
> +
> +	;; reset stack
> +	move.d $r8, $sp
> +
> +	;; Load rval type in to r13
> +	pop $r13

Where was this pushed?  Ah, this is fig->flags?  The reference
to "rval type" confused me; flags is a *set* of values/bits to
me; not a single enum-like value as is seen below.  At first I
thought it was a bug and you meant to refer to ecif.rvalue.

> +
> +	;; detect rval type
> +	cmpq FFI_TYPE_VOID, $r13
> +	beq epilogue
> +	nop
> +
> +	cmpq FFI_TYPE_STRUCT, $r13
> +	beq epilogue
> +	nop
> +
> +	cmpq FFI_TYPE_DOUBLE, $r13
> +	beq return_double_or_longlong
> +	nop
> +
> +	cmpq FFI_TYPE_UINT64, $r13
> +	beq return_double_or_longlong
> +	nop
> +
> +	cmpq FFI_TYPE_SINT64, $r13
> +	beq return_double_or_longlong
> +	nop

All those delay-slots could have been filled with the following
"CMPQ" with no loss in readbility (code-wise, just deleting the
NOP).  Ok, maybe that's just me...

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:

> +return_double_or_longlong:
> +	move.d [$sp+16], $r13
> +	move.d $r10, [$r13]
> +	addq 4, $r13
> +	move.d $r11, [$r13]

> diff -uNr gcc/libffi/src/prep_cif.c gcc_cris/libffi/src/prep_cif.c

> +  while ((*ptr) != NULL)    {

Bad formatting: brace comes on next line.

> +      arg->alignment = (arg->alignment > (*ptr)->alignment) ?
> +        arg->alignment : (*ptr)->alignment;

Bad formatting.  The "?" should have been first on the continued line.
(I'm sure people will disagree on the coding standard interpretation
here. ;-)

>  static ffi_status initialize_aggregate(/*@out@*/ ffi_type *arg)

> @@ -127,8 +165,20 @@
>      {
>
> 	 /* Initialize any uninitialized aggregate type definitions */
> +#ifdef CRIS32
> +      if ((*ptr)->type == FFI_TYPE_STRUCT)
> +      {
> +        if (((*ptr)->size == 0) && (initialize_aggregate_packed_struct((*ptr)) != FFI_OK))
> +           return FFI_BAD_TYPEDEF;
> +      }
> +      else
> +      {
> +#endif
> 	 if (((*ptr)->size == 0) && (initialize_aggregate((*ptr)) != FFI_OK))
> 	  return FFI_BAD_TYPEDEF;
> +#ifdef CRIS32
> +      }
> +#endif

A bit too many ifdefs for my taste here.  I recognize there are
several in the code as-is, but continuing that would have things
worse. For this particular effect, wouldn't it just work with
#ifdef __CRIS__
#define initialize_aggregate initialize_aggregate_packed_struct
#endif
(perhaps in the initialize_aggregate body; after the header)?
Hmm, I don't quite understand the code here, which may be my
lack of libffi knowledge.  What does initialize_aggregate do
that your initialize_aggregate_packed_struct wouldn't do?

But wait, how about wrapping the whole initialize_aggregate body
after "cif->" lines up to including "cif->bytes = bytes;" in
"#ifndef __CRIS__ / #endif" and let ffi_prep_cif_machdep do the
structure handling?

brgds, H-P