From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 15529 invoked by alias); 26 Oct 2004 02:27:07 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 15354 invoked from network); 26 Oct 2004 02:27:00 -0000 Received: from unknown (HELO dair.pair.com) (209.68.1.49) by sourceware.org with SMTP; 26 Oct 2004 02:27:00 -0000 Received: (qmail 6845 invoked by uid 20157); 26 Oct 2004 02:27:00 -0000 Date: Tue, 26 Oct 2004 03:43:00 -0000 From: Hans-Peter Nilsson X-X-Sender: hp@dair.pair.com To: Simon Posnjak cc: GCC Patches Subject: Re: [PATCH] libffi support for CRIS In-Reply-To: <1098703798.10993.29.camel@matilda.kranj.cetrtapot.si> Message-ID: References: <1098703798.10993.29.camel@matilda.kranj.cetrtapot.si> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII X-SW-Source: 2004-10/txt/msg02153.txt.bz2 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