public inbox for libffi-discuss@sourceware.org
 help / color / mirror / Atom feed
* [PATCH] Add variadic support
@ 2011-02-22 15:41 Dr. David Alan Gilbert
  2011-02-23 12:39 ` Anthony Green
  2011-02-23 21:16 ` PowerPC failures (Was: [PATCH] Add variadic support) Anthony Green
  0 siblings, 2 replies; 22+ messages in thread
From: Dr. David Alan Gilbert @ 2011-02-22 15:41 UTC (permalink / raw)
  To: libffi-discuss; +Cc: Marcus.Shawcroft, cltang

Hi,
  As discussed a couple of months ago, I've added explicit support
for calling variadic functions to libffi.  This work is required
for ARM hard float (aka VFP) systems to call variadic functions
(by switching away from the vfp abi).

  I'm assuming this is too late for the 3.0.10 cycle but I thought
I'd get the patch out for discussion anyway.

  The patch below adds a 'ffi_prep_cif_var' function that
must be used to call variadic functions and it is passed
both the total number of arguments and the number of fixed
arguments.  Code that uses ffi_prep_cif to call variadic functions
will still work on architectures where that works at the moment (most
of them).

  Notes:
   1) This patch is against the top of git ( cbb062cc35c518004f1ab45c847f8ec4f66069ad )

   2) I've tested it on i386 (1659 expected passes, 15 unsupported),
                        armel (1654 expected passes, 5 unexpecteed passes (cls_longdouble.c), 15 unsupported)
                        armhf (1654 expected passes, 5 unexpecteed passes (cls_longdouble.c), 15 unsupported)
                        s390x (1639 expected passes, 5 unexpected failures - err_bad_abi.c)
		Those cls_longdouble unexpected passes and err_bad_abi.c
		failure are there in the current head.
	powerpc64 seems to be broken on current head, but I tested this patch
	on powerpc64 on the .0.9 release and it was OK.

   3) I've not included the changed .info file in the patch, even though it's
      in the git repo, it'll just regen from the .texi I guess.

   4) The only place it touches another architecture is CRIS which has it's own
      ffi_prep_cif implementation; I think I've fixed that up, but someone
      who can test CRIS probably should.

All comments welcome,

Dave

commit 9ac312697816b20896ca9c237afa908c9875fa3c
Author: Dr. David Alan Gilbert <david.gilbert@linaro.org>
Date:   Tue Feb 22 12:30:23 2011 +0000

    Add ffi_prep_cif_var variadic api and test it.

diff --git a/doc/libffi.texi b/doc/libffi.texi
index 5cdd667..a7c629a 100644
--- a/doc/libffi.texi
+++ b/doc/libffi.texi
@@ -133,8 +133,6 @@ This initializes @var{cif} according to the given parameters.
 you want.  @ref{Multiple ABIs} for more information.
 
 @var{nargs} is the number of arguments that this function accepts.
-@samp{libffi} does not yet handle varargs functions; see @ref{Missing
-Features} for more information.
 
 @var{rtype} is a pointer to an @code{ffi_type} structure that
 describes the return type of the function.  @xref{Types}.
@@ -150,6 +148,28 @@ objects is incorrect; or @code{FFI_BAD_ABI} if the @var{abi} parameter
 is invalid.
 @end defun
 
+If the function being called is variadic (varargs) then @code{ffi_prep_cif_var}
+must be used instead of @code{ffi_prep_cif}.
+
+@findex ffi_prep_cif_var
+@defun ffi_status ffi_prep_cif_var (ffi_cif *@var{cif}, ffi_abi @var{abi}, unsigned int @var{nfixedargs}, unsigned int @var{ntotalargs}, ffi_type *@var{rtype}, ffi_type **@var{argtypes})
+This initializes @var{cif} according to the given parameters for
+a call to a variadic function.  In general it's operation is the
+same as for @code{ffi_prep_cif} except that:
+
+@var{nfixedargs} is the number of fixed arguments, prior to any
+variadic arguments.  It must be greater than zero.
+
+@var{ntotalargs} the total number of arguments, including variadic
+and fixed arguments.
+
+Note that, different cif's must be prepped for calls to the same
+function when different numbers of arguments are passed.
+
+Also note that a call to @code{ffi_prep_cif_var} with @var{nfixedargs}=@var{nototalargs}
+is NOT equivalent to a call to @code{ffi_prep_cif}.
+
+@end defun
 
 To call a function using an initialized @code{ffi_cif}, use the
 @code{ffi_call} function:
@@ -572,9 +592,7 @@ support for these.
 
 @itemize @bullet
 @item
-There is no support for calling varargs functions.  This may work on
-some platforms, depending on how the ABI is defined, but it is not
-reliable.
+Variadic closures.
 
 @item
 There is no support for bit fields in structures.
@@ -591,6 +609,7 @@ The ``raw'' API is undocumented.
 @c anything else?
 @end itemize
 
+Note that variadic support is very new and tested on a relatively small number of platforms.
 
 @node Index
 @unnumbered Index
diff --git a/include/ffi.h.in b/include/ffi.h.in
index 96b8fd3..95f1471 100644
--- a/include/ffi.h.in
+++ b/include/ffi.h.in
@@ -198,11 +198,22 @@ typedef struct {
   ffi_type *rtype;
   unsigned bytes;
   unsigned flags;
+#ifdef FFI_TARGET_SPECIFIC_VARIADIC
+  /* 0 is used as a flag to indicate a non-variadic function */
+  unsigned int nfixedargs;
+#endif
 #ifdef FFI_EXTRA_CIF_FIELDS
   FFI_EXTRA_CIF_FIELDS;
 #endif
 } ffi_cif;
 
+/* Used internally, but overridden by some architectures */
+ffi_status ffi_prep_cif_core(ffi_cif *cif,
+			     ffi_abi abi,
+			     unsigned int nargs,
+			     ffi_type *rtype,
+			     ffi_type **atypes);
+
 /* ---- Definitions for the raw API -------------------------------------- */
 
 #ifndef FFI_SIZEOF_ARG
@@ -380,6 +391,13 @@ ffi_status ffi_prep_cif(ffi_cif *cif,
 			ffi_type *rtype,
 			ffi_type **atypes);
 
+ffi_status ffi_prep_cif_var(ffi_cif *cif,
+			    ffi_abi abi,
+			    unsigned int nfixedargs,
+			    unsigned int ntotalargs,
+			    ffi_type *rtype,
+			    ffi_type **atypes);
+
 void ffi_call(ffi_cif *cif,
 	      void (*fn)(void),
 	      void *rvalue,
diff --git a/man/Makefile.am b/man/Makefile.am
index 2519277..afcbfb6 100644
--- a/man/Makefile.am
+++ b/man/Makefile.am
@@ -2,7 +2,7 @@
 
 AUTOMAKE_OPTIONS=foreign
 
-EXTRA_DIST = ffi.3 ffi_call.3 ffi_prep_cif.3
+EXTRA_DIST = ffi.3 ffi_call.3 ffi_prep_cif.3 ffi_prep_cif_var.3
 
-man_MANS = ffi.3 ffi_call.3 ffi_prep_cif.3
+man_MANS = ffi.3 ffi_call.3 ffi_prep_cif.3 ffi_prep_cif_var.3
 
diff --git a/man/ffi.3 b/man/ffi.3
index 18b5d5d..1f1d303 100644
--- a/man/ffi.3
+++ b/man/ffi.3
@@ -16,6 +16,15 @@ libffi, -lffi
 .Fa "ffi_type **atypes"
 .Fc
 .Ft void
+.Fo ffi_prep_cif_var
+.Fa "ffi_cif *cif"
+.Fa "ffi_abi abi"
+.Fa "unsigned int nfixedargs"
+.Fa "unsigned int ntotalargs"
+.Fa "ffi_type *rtype"
+.Fa "ffi_type **atypes"
+.Fc
+.Ft void
 .Fo ffi_call
 .Fa "ffi_cif *cif"
 .Fa "void (*fn)(void)"
@@ -28,4 +37,5 @@ generate a call to another function at runtime without requiring knowledge of
 the called function's interface at compile time.
 .Sh SEE ALSO
 .Xr ffi_prep_cif 3 ,
+.Xr ffi_prep_cif_var 3 ,
 .Xr ffi_call 3
diff --git a/man/ffi_prep_cif.3 b/man/ffi_prep_cif.3
index 9436b31..fe66452 100644
--- a/man/ffi_prep_cif.3
+++ b/man/ffi_prep_cif.3
@@ -37,7 +37,9 @@ structs that describe the data type, size and alignment of each argument.
 points to an
 .Nm ffi_type
 that describes the data type, size and alignment of the
-return value.
+return value. Note that to call a variadic function
+.Nm ffi_prep_cif_var
+must be used instead.
 .Sh RETURN VALUES
 Upon successful completion,
 .Nm ffi_prep_cif
@@ -63,4 +65,5 @@ defined in
 .
 .Sh SEE ALSO
 .Xr ffi 3 ,
-.Xr ffi_call 3 
+.Xr ffi_call 3 ,
+.Xr ffi_prep_cif_var 3
diff --git a/man/ffi_prep_cif_var.3 b/man/ffi_prep_cif_var.3
new file mode 100644
index 0000000..1b39c03
--- /dev/null
+++ b/man/ffi_prep_cif_var.3
@@ -0,0 +1,73 @@
+.Dd January 25, 2011
+.Dt ffi_prep_cif_var 3
+.Sh NAME
+.Nm ffi_prep_cif_var
+.Nd Prepare a
+.Nm ffi_cif
+structure for use with
+.Nm ffi_call 
+for variadic functions.
+.Sh SYNOPSIS
+.In ffi.h
+.Ft ffi_status
+.Fo ffi_prep_cif_var
+.Fa "ffi_cif *cif"
+.Fa "ffi_abi abi"
+.Fa "unsigned int nfixedargs"
+.Fa "unsigned int ntotalargs"
+.Fa "ffi_type *rtype"
+.Fa "ffi_type **atypes"
+.Fc
+.Sh DESCRIPTION
+The
+.Nm ffi_prep_cif_var
+function prepares a
+.Nm ffi_cif
+structure for use with 
+.Nm ffi_call
+for variadic functions.
+.Fa abi
+specifies a set of calling conventions to use.
+.Fa atypes
+is an array of
+.Fa ntotalargs
+pointers to
+.Nm ffi_type
+structs that describe the data type, size and alignment of each argument.
+.Fa rtype
+points to an
+.Nm ffi_type
+that describes the data type, size and alignment of the
+return value. 
+.Fa nfixedargs
+must contain the number of fixed (non-variadic) arguments.
+Note that to call a non-variadic function
+.Nm ffi_prep_cif
+must be used.
+.Sh RETURN VALUES
+Upon successful completion,
+.Nm ffi_prep_cif_var
+returns
+.Nm FFI_OK .
+It will return
+.Nm FFI_BAD_TYPEDEF
+if
+.Fa cif
+is
+.Nm NULL
+or
+.Fa atypes
+or
+.Fa rtype
+is malformed. If
+.Fa abi
+does not refer to a valid ABI,
+.Nm FFI_BAD_ABI
+will be returned. Available ABIs are
+defined in
+.Nm <ffitarget.h>
+.
+.Sh SEE ALSO
+.Xr ffi 3 ,
+.Xr ffi_call 3 ,
+.Xr ffi_prep_cif 3
diff --git a/src/arm/ffi.c b/src/arm/ffi.c
index 885a9cb..b1bf5a8 100644
--- a/src/arm/ffi.c
+++ b/src/arm/ffi.c
@@ -143,6 +143,12 @@ ffi_status ffi_prep_cif_machdep(ffi_cif *cif)
      when it isn't needed.  */
   cif->bytes = (cif->bytes + 7) & ~7;
 
+#ifdef __ARM_PCS_VFP
+  /* VFP uses the standard ABI for variadic functions */
+  if ((cif->abi == FFI_VFP) && (cif->nfixedargs != 0))
+    cif->abi = FFI_SYSV;
+#endif
+
   /* Set the return type flag */
   switch (cif->rtype->type)
     {
diff --git a/src/arm/ffitarget.h b/src/arm/ffitarget.h
index ce25b23..3f70acb 100644
--- a/src/arm/ffitarget.h
+++ b/src/arm/ffitarget.h
@@ -55,6 +55,10 @@ typedef enum ffi_abi {
 #define FFI_TYPE_STRUCT_VFP_FLOAT  (FFI_TYPE_LAST + 1)
 #define FFI_TYPE_STRUCT_VFP_DOUBLE (FFI_TYPE_LAST + 2)
 
+#ifdef __ARM_PCS_VFP
+#define FFI_TARGET_SPECIFIC_VARIADIC
+#endif
+
 /* ---- Definitions for closures ----------------------------------------- */
 
 #define FFI_CLOSURES 1
@@ -62,4 +66,3 @@ typedef enum ffi_abi {
 #define FFI_NATIVE_RAW_API 0
 
 #endif
-
diff --git a/src/cris/ffi.c b/src/cris/ffi.c
index f25d7b4..a114c0b 100644
--- a/src/cris/ffi.c
+++ b/src/cris/ffi.c
@@ -153,10 +153,10 @@ ffi_prep_args (char *stack, extended_cif * ecif)
   return (struct_count);
 }
 
-ffi_status
-ffi_prep_cif (ffi_cif * cif,
-	      ffi_abi abi, unsigned int nargs,
-	      ffi_type * rtype, ffi_type ** atypes)
+ffi_status FFI_HIDDEN
+ffi_prep_cif_core (ffi_cif * cif,
+	           ffi_abi abi, unsigned int nargs,
+	           ffi_type * rtype, ffi_type ** atypes)
 {
   unsigned bytes = 0;
   unsigned int i;
diff --git a/src/prep_cif.c b/src/prep_cif.c
index 8548cfd..b1ce954 100644
--- a/src/prep_cif.c
+++ b/src/prep_cif.c
@@ -90,8 +90,8 @@ static ffi_status initialize_aggregate(ffi_type *arg)
 /* Perform machine independent ffi_cif preparation, then call
    machine dependent routine. */
 
-ffi_status ffi_prep_cif(ffi_cif *cif, ffi_abi abi, unsigned int nargs,
-			ffi_type *rtype, ffi_type **atypes)
+ffi_status FFI_HIDDEN ffi_prep_cif_core(ffi_cif *cif, ffi_abi abi, unsigned int nargs,
+			     ffi_type *rtype, ffi_type **atypes)
 {
   unsigned bytes = 0;
   unsigned int i;
@@ -163,6 +163,33 @@ ffi_status ffi_prep_cif(ffi_cif *cif, ffi_abi abi, unsigned int nargs,
 }
 #endif /* not __CRIS__ */
 
+ffi_status ffi_prep_cif(ffi_cif *cif, ffi_abi abi, unsigned int nargs,
+			     ffi_type *rtype, ffi_type **atypes)
+{
+#ifdef FFI_TARGET_SPECIFIC_VARIADIC
+  cif->nfixedargs = 0;
+#endif
+
+  return ffi_prep_cif_core(cif, abi, nargs, rtype, atypes);
+}
+
+ffi_status ffi_prep_cif_var(ffi_cif *cif,
+                            ffi_abi abi,
+                            unsigned int nfixedargs,
+                            unsigned int ntotalargs,
+                            ffi_type *rtype,
+                            ffi_type **atypes)
+{
+  FFI_ASSERT(cif != NULL);
+  /* There should always be at least one fixed arg */
+  FFI_ASSERT(nfixedargs >= 1);
+  FFI_ASSERT(nfixedargs <= ntotalargs);
+#ifdef FFI_TARGET_SPECIFIC_VARIADIC
+  cif->nfixedargs = nfixedargs;
+#endif
+  return ffi_prep_cif_core(cif, abi, ntotalargs, rtype, atypes);
+}
+
 #if FFI_CLOSURES
 
 ffi_status
diff --git a/testsuite/libffi.call/cls_double_va.c b/testsuite/libffi.call/cls_double_va.c
index 62bebbd..1a2706e 100644
--- a/testsuite/libffi.call/cls_double_va.c
+++ b/testsuite/libffi.call/cls_double_va.c
@@ -6,7 +6,6 @@
 
 /* { dg-do run { xfail strongarm*-*-* xscale*-*-* } } */
 /* { dg-output "" { xfail avr32*-*-* } } */
-/* { dg-skip-if "" arm*-*-* { "-mfloat-abi=hard" } { "" } } */
 
 #include "ffitest.h"
 
@@ -36,7 +35,8 @@ int main (void)
 	arg_types[1] = &ffi_type_double;
 	arg_types[2] = NULL;
 
-	CHECK(ffi_prep_cif(&cif, FFI_DEFAULT_ABI, 2, &ffi_type_sint,
+	/* This printf call is variadic */
+	CHECK(ffi_prep_cif_var(&cif, FFI_DEFAULT_ABI, 1, 2, &ffi_type_sint,
 		arg_types) == FFI_OK);
 
 	args[0] = &format;
@@ -48,6 +48,10 @@ int main (void)
 	printf("res: %d\n", (int) res);
 	// { dg-output "\nres: 4" }
 
+	/* The call to cls_double_va_fn is static, so have to use a normal prep_cif */
+	CHECK(ffi_prep_cif(&cif, FFI_DEFAULT_ABI, 2, &ffi_type_sint,
+		arg_types) == FFI_OK);
+
 	CHECK(ffi_prep_closure_loc(pcl, &cif, cls_double_va_fn, NULL, code) == FFI_OK);
 
 	res	= ((int(*)(char*, double))(code))(format, doubleArg);
diff --git a/testsuite/libffi.call/cls_longdouble_va.c b/testsuite/libffi.call/cls_longdouble_va.c
index b33b2b7..95f9ff4 100644
--- a/testsuite/libffi.call/cls_longdouble_va.c
+++ b/testsuite/libffi.call/cls_longdouble_va.c
@@ -6,7 +6,6 @@
 
 /* { dg-do run { xfail strongarm*-*-* xscale*-*-* } } */
 /* { dg-output "" { xfail avr32*-*-* x86_64-*-mingw* } } */
-/* { dg-skip-if "" arm*-*-* { "-mfloat-abi=hard" } { "" } } */
 
 #include "ffitest.h"
 
@@ -36,7 +35,8 @@ int main (void)
 	arg_types[1] = &ffi_type_longdouble;
 	arg_types[2] = NULL;
 
-	CHECK(ffi_prep_cif(&cif, FFI_DEFAULT_ABI, 2, &ffi_type_sint,
+	/* This printf call is variadic */
+	CHECK(ffi_prep_cif_var(&cif, FFI_DEFAULT_ABI, 1, 2, &ffi_type_sint,
 		arg_types) == FFI_OK);
 
 	args[0] = &format;
@@ -48,6 +48,10 @@ int main (void)
 	printf("res: %d\n", (int) res);
 	// { dg-output "\nres: 4" }
 
+	/* The call to cls_longdouble_va_fn is static, so have to use a normal prep_cif */
+	CHECK(ffi_prep_cif(&cif, FFI_DEFAULT_ABI, 2, &ffi_type_sint,
+		arg_types) == FFI_OK);
+
 	CHECK(ffi_prep_closure_loc(pcl, &cif, cls_longdouble_va_fn, NULL, code) == FFI_OK);
 
 	res	= ((int(*)(char*, long double))(code))(format, ldArg);
diff --git a/testsuite/libffi.call/float_va.c b/testsuite/libffi.call/float_va.c
new file mode 100644
index 0000000..4611969
--- /dev/null
+++ b/testsuite/libffi.call/float_va.c
@@ -0,0 +1,107 @@
+/* Area:        fp and variadics
+   Purpose:     check fp inputs and returns work on variadics, even the fixed params
+   Limitations: None
+   PR:          none
+   Originator:  <david.gilbert@linaro.org> 2011-01-25
+
+   Intended to stress the difference in ABI on ARM vfp
+*/
+
+/* { dg-do run } */
+
+#include <stdarg.h>
+
+#include "ffitest.h"
+
+/* prints out all the parameters, and returns the sum of them all.
+ * 'x' is the number of variadic parameters all of which are double in this test
+ */
+double float_va_fn(unsigned int x, double y,...)
+{
+  double total=0.0;
+  va_list ap;
+  unsigned int i;
+
+  total+=(double)x;
+  total+=y;
+
+  printf("%u: %.1lf :", x, y);
+
+  va_start(ap, y);
+  for(i=0;i<x;i++)
+  {
+    double arg=va_arg(ap, double);
+    total+=arg;
+    printf(" %d:%.1lf ", i, arg);
+  }
+  va_end(ap);
+  
+  printf(" total: %.1lf\n", total);
+
+  return total;
+}
+
+int main (void)
+{
+  ffi_cif    cif;
+
+  ffi_type    *arg_types[5];
+  void        *values[5];
+  double        doubles[5];
+  unsigned int firstarg;
+  double        resfp;
+
+  /* First test, pass float_va_fn(0,2.0) - note there are no actual
+   * variadic parameters, but it's declared variadic so the ABI may be
+   * different. */
+  /* Call it statically and then via ffi */
+  resfp=float_va_fn(0,2.0);
+  // { dg-output "0: 2.0 : total: 2.0" }
+  printf("compiled: %.1lf\n", resfp);
+  // { dg-output "\ncompiled: 2.0" }
+
+  arg_types[0] = &ffi_type_uint;
+  arg_types[1] = &ffi_type_double;
+  arg_types[2] = NULL;
+  CHECK(ffi_prep_cif_var(&cif, FFI_DEFAULT_ABI, 2, 2,
+        &ffi_type_double, arg_types) == FFI_OK);
+
+  firstarg = 0;
+  doubles[0] = 2.0;
+  values[0] = &firstarg;
+  values[1] = &doubles[0];
+  ffi_call(&cif, FFI_FN(float_va_fn), &resfp, values); 
+  // { dg-output "\n0: 2.0 : total: 2.0" }
+  printf("ffi: %.1lf\n", resfp);
+  // { dg-output "\nffi: 2.0" }
+
+  /* Second test, float_va_fn(2,2.0,3.0,4.0), now with variadic params */
+  /* Call it statically and then via ffi */
+  resfp=float_va_fn(2,2.0,3.0,4.0);
+  // { dg-output "\n2: 2.0 : 0:3.0  1:4.0  total: 11.0" }
+  printf("compiled: %.1lf\n", resfp);
+  // { dg-output "\ncompiled: 11.0" }
+
+  arg_types[0] = &ffi_type_uint;
+  arg_types[1] = &ffi_type_double;
+  arg_types[2] = &ffi_type_double;
+  arg_types[3] = &ffi_type_double;
+  arg_types[4] = NULL;
+  CHECK(ffi_prep_cif_var(&cif, FFI_DEFAULT_ABI, 2, 4,
+        &ffi_type_double, arg_types) == FFI_OK);
+
+  firstarg = 2;
+  doubles[0] = 2.0;
+  doubles[1] = 3.0;
+  doubles[2] = 4.0;
+  values[0] = &firstarg;
+  values[1] = &doubles[0];
+  values[2] = &doubles[1];
+  values[3] = &doubles[2];
+  ffi_call(&cif, FFI_FN(float_va_fn), &resfp, values); 
+  // { dg-output "\n2: 2.0 : 0:3.0  1:4.0  total: 11.0" }
+  printf("ffi: %.1lf\n", resfp);
+  // { dg-output "\nffi: 11.0" }
+
+  exit(0);
+}

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH] Add variadic support
  2011-02-22 15:41 [PATCH] Add variadic support Dr. David Alan Gilbert
@ 2011-02-23 12:39 ` Anthony Green
  2011-02-23 13:12   ` David Gilbert
  2011-02-23 21:16 ` PowerPC failures (Was: [PATCH] Add variadic support) Anthony Green
  1 sibling, 1 reply; 22+ messages in thread
From: Anthony Green @ 2011-02-23 12:39 UTC (permalink / raw)
  To: Dr. David Alan Gilbert; +Cc: libffi-discuss, Marcus.Shawcroft, cltang

"Dr. David Alan Gilbert" <david.gilbert@linaro.org> writes:

> Hi,
>   As discussed a couple of months ago, I've added explicit support
> for calling variadic functions to libffi.  This work is required
> for ARM hard float (aka VFP) systems to call variadic functions
> (by switching away from the vfp abi).
>
>   I'm assuming this is too late for the 3.0.10 cycle but I thought
> I'd get the patch out for discussion anyway.

Thanks David.  This sounds promising, but it looks like the patch is not
complete.  Where is ffi_prep_cif_var?

Thanks,

AG

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH] Add variadic support
  2011-02-23 12:39 ` Anthony Green
@ 2011-02-23 13:12   ` David Gilbert
  2011-02-23 13:26     ` Chung-Lin Tang
  0 siblings, 1 reply; 22+ messages in thread
From: David Gilbert @ 2011-02-23 13:12 UTC (permalink / raw)
  To: Anthony Green; +Cc: libffi-discuss, Marcus.Shawcroft, cltang

On 23 February 2011 12:40, Anthony Green <green@redhat.com> wrote:
> "Dr. David Alan Gilbert" <david.gilbert@linaro.org> writes:
>
>> Hi,
>>   As discussed a couple of months ago, I've added explicit support
>> for calling variadic functions to libffi.  This work is required
>> for ARM hard float (aka VFP) systems to call variadic functions
>> (by switching away from the vfp abi).
>>
>>   I'm assuming this is too late for the 3.0.10 cycle but I thought
>> I'd get the patch out for discussion anyway.
>
> Thanks David.  This sounds promising, but it looks like the patch is not
> complete.  Where is ffi_prep_cif_var?

It's only small and it is there (I checked the mail list archive!) ;
it's in the prep_cif.c diff:

+ffi_status ffi_prep_cif_var(ffi_cif *cif,
+                            ffi_abi abi,
+                            unsigned int nfixedargs,
+                            unsigned int ntotalargs,
+                            ffi_type *rtype,
+                            ffi_type **atypes)
+{
+  FFI_ASSERT(cif != NULL);
+  /* There should always be at least one fixed arg */
+  FFI_ASSERT(nfixedargs >= 1);
+  FFI_ASSERT(nfixedargs <= ntotalargs);
+#ifdef FFI_TARGET_SPECIFIC_VARIADIC
+  cif->nfixedargs = nfixedargs;
+#endif
+  return ffi_prep_cif_core(cif, abi, ntotalargs, rtype, atypes);
+}
+

What I'm doing is that I've renamed ffi_prep_cif to ffi_prep_cif_core and then
ffi_prep_cif and ffi_prep_cif_var are wrappers around _core that set
cif->nfixedargs
which acts as a flag as to whether it's a variadic function that the
backend code can
decide what to do.

The result is that there are no changes to any of the backends that
don't care about it.

In ARM's ffi_prep_cif_machdep I have:

+#ifdef __ARM_PCS_VFP
+  /* VFP uses the standard ABI for variadic functions */
+  if ((cif->abi == FFI_VFP) && (cif->nfixedargs != 0))
+    cif->abi = FFI_SYSV;
+#endif

and that's the only active change that's needed.

Dave

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH] Add variadic support
  2011-02-23 13:12   ` David Gilbert
@ 2011-02-23 13:26     ` Chung-Lin Tang
  2011-02-23 16:20       ` David Gilbert
  0 siblings, 1 reply; 22+ messages in thread
From: Chung-Lin Tang @ 2011-02-23 13:26 UTC (permalink / raw)
  To: David Gilbert; +Cc: Anthony Green, libffi-discuss, Marcus.Shawcroft

On 2011/2/23 09:11 PM, David Gilbert wrote:
> In ARM's ffi_prep_cif_machdep I have:
> 
> +#ifdef __ARM_PCS_VFP
> +  /* VFP uses the standard ABI for variadic functions */
> +  if ((cif->abi == FFI_VFP) && (cif->nfixedargs != 0))
> +    cif->abi = FFI_SYSV;
> +#endif
> 
> and that's the only active change that's needed.

I suggest removing the #ifdef __ARM_PCS_VFP #ifdef.  We always have
support for both FFI_SYSV and FFI_VFP compiled in, so it doesn't make
sense to leave this bit of logic out when we are under __ARM_PCS.

Chung-Lin

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH] Add variadic support
  2011-02-23 13:26     ` Chung-Lin Tang
@ 2011-02-23 16:20       ` David Gilbert
  2011-02-23 16:56         ` Chung-Lin Tang
  0 siblings, 1 reply; 22+ messages in thread
From: David Gilbert @ 2011-02-23 16:20 UTC (permalink / raw)
  To: Chung-Lin Tang; +Cc: Anthony Green, libffi-discuss, Marcus.Shawcroft

On 23 February 2011 13:25, Chung-Lin Tang <cltang@linaro.org> wrote:
> On 2011/2/23 09:11 PM, David Gilbert wrote:
>> In ARM's ffi_prep_cif_machdep I have:
>>
>> +#ifdef __ARM_PCS_VFP
>> +  /* VFP uses the standard ABI for variadic functions */
>> +  if ((cif->abi == FFI_VFP) && (cif->nfixedargs != 0))
>> +    cif->abi = FFI_SYSV;
>> +#endif
>>
>> and that's the only active change that's needed.
>
> I suggest removing the #ifdef __ARM_PCS_VFP #ifdef.  We always have
> support for both FFI_SYSV and FFI_VFP compiled in, so it doesn't make
> sense to leave this bit of logic out when we are under __ARM_PCS.

I could do, although cif->nfixedargs is also not present except in the
VFP case, so
it would still need an ifdef.

I did that since I thought it best not to change the layout of the CIF
structure for
existing ARM builds.

Dave

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH] Add variadic support
  2011-02-23 16:20       ` David Gilbert
@ 2011-02-23 16:56         ` Chung-Lin Tang
  2011-02-23 17:21           ` David Gilbert
  0 siblings, 1 reply; 22+ messages in thread
From: Chung-Lin Tang @ 2011-02-23 16:56 UTC (permalink / raw)
  To: David Gilbert; +Cc: Anthony Green, libffi-discuss, Marcus.Shawcroft

On 2011/2/24 12:20 AM, David Gilbert wrote:
> On 23 February 2011 13:25, Chung-Lin Tang <cltang@linaro.org> wrote:
>> On 2011/2/23 09:11 PM, David Gilbert wrote:
>>> In ARM's ffi_prep_cif_machdep I have:
>>>
>>> +#ifdef __ARM_PCS_VFP
>>> +  /* VFP uses the standard ABI for variadic functions */
>>> +  if ((cif->abi == FFI_VFP) && (cif->nfixedargs != 0))
>>> +    cif->abi = FFI_SYSV;
>>> +#endif
>>>
>>> and that's the only active change that's needed.
>>
>> I suggest removing the #ifdef __ARM_PCS_VFP #ifdef.  We always have
>> support for both FFI_SYSV and FFI_VFP compiled in, so it doesn't make
>> sense to leave this bit of logic out when we are under __ARM_PCS.
> 
> I could do, although cif->nfixedargs is also not present except in the
> VFP case, so
> it would still need an ifdef.
> 
> I did that since I thought it best not to change the layout of the CIF
> structure for
> existing ARM builds.

Sigh...you're right about the layout issue :(

Still I think it's best we keep the full FFI_SYSV/VFP capabilities in,
or we'll have the weird case of "FFI_VFP with buggy variadics" under softfp.

I suggest moving the nfixedargs field into the ARM FFI_EXTRA_CIF_FIELDS,
placed after the current VFP fields; this means doing away with the
FFI_TARGET_SPECIFIC_VARIADIC symbol, and probably having a
machine-specific ffi_prep_cif_var_machdep() function, analogous to the
current ffi_prep_cif_machdep().

ffi_prep_cif_var_machdep() can then save things like 'number of fixed
arguments' from the ffi_prep_cif_var() arguments if it is necessary for
that architecture.

Chung-Lin

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH] Add variadic support
  2011-02-23 16:56         ` Chung-Lin Tang
@ 2011-02-23 17:21           ` David Gilbert
  2011-02-23 17:39             ` Chung-Lin Tang
  0 siblings, 1 reply; 22+ messages in thread
From: David Gilbert @ 2011-02-23 17:21 UTC (permalink / raw)
  To: Chung-Lin Tang; +Cc: Anthony Green, libffi-discuss, Marcus.Shawcroft

On 23 February 2011 16:56, Chung-Lin Tang <cltang@linaro.org> wrote:
> On 2011/2/24 12:20 AM, David Gilbert wrote:
>> On 23 February 2011 13:25, Chung-Lin Tang <cltang@linaro.org> wrote:
>>> On 2011/2/23 09:11 PM, David Gilbert wrote:
>>>> In ARM's ffi_prep_cif_machdep I have:
>>>>
>>>> +#ifdef __ARM_PCS_VFP
>>>> +  /* VFP uses the standard ABI for variadic functions */
>>>> +  if ((cif->abi == FFI_VFP) && (cif->nfixedargs != 0))
>>>> +    cif->abi = FFI_SYSV;
>>>> +#endif
>>>>
>>>> and that's the only active change that's needed.
>>>
>>> I suggest removing the #ifdef __ARM_PCS_VFP #ifdef.  We always have
>>> support for both FFI_SYSV and FFI_VFP compiled in, so it doesn't make
>>> sense to leave this bit of logic out when we are under __ARM_PCS.
>>
>> I could do, although cif->nfixedargs is also not present except in the
>> VFP case, so
>> it would still need an ifdef.
>>
>> I did that since I thought it best not to change the layout of the CIF
>> structure for
>> existing ARM builds.
>
> Sigh...you're right about the layout issue :(
>
> Still I think it's best we keep the full FFI_SYSV/VFP capabilities in,
> or we'll have the weird case of "FFI_VFP with buggy variadics" under softfp.

Wouldn't it make more sense to ban FFI_VFP under softfp?

> I suggest moving the nfixedargs field into the ARM FFI_EXTRA_CIF_FIELDS,
> placed after the current VFP fields; this means doing away with the
> FFI_TARGET_SPECIFIC_VARIADIC symbol, and probably having a
> machine-specific ffi_prep_cif_var_machdep() function, analogous to the
> current ffi_prep_cif_machdep().

That would mean changing every back end to add that function or again
ifdefing for it, and the belief is that storing the nfixedargs value
is likely to
be useful to some backends who have to do different calling conventions for
variadics anyway.

> ffi_prep_cif_var_machdep() can then save things like 'number of fixed
> arguments' from the ffi_prep_cif_var() arguments if it is necessary for
> that architecture.
>
> Chung-Lin
>

Dave

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH] Add variadic support
  2011-02-23 17:21           ` David Gilbert
@ 2011-02-23 17:39             ` Chung-Lin Tang
  2011-02-24  6:37               ` Chung-Lin Tang
  2011-02-25 12:56               ` David Gilbert
  0 siblings, 2 replies; 22+ messages in thread
From: Chung-Lin Tang @ 2011-02-23 17:39 UTC (permalink / raw)
  To: David Gilbert; +Cc: Anthony Green, libffi-discuss, Marcus.Shawcroft

On 2011/2/24 01:20 AM, David Gilbert wrote:
> On 23 February 2011 16:56, Chung-Lin Tang <cltang@linaro.org> wrote:
>> On 2011/2/24 12:20 AM, David Gilbert wrote:
>>> On 23 February 2011 13:25, Chung-Lin Tang <cltang@linaro.org> wrote:
>>>> On 2011/2/23 09:11 PM, David Gilbert wrote:
>>>>> In ARM's ffi_prep_cif_machdep I have:
>>>>>
>>>>> +#ifdef __ARM_PCS_VFP
>>>>> +  /* VFP uses the standard ABI for variadic functions */
>>>>> +  if ((cif->abi == FFI_VFP) && (cif->nfixedargs != 0))
>>>>> +    cif->abi = FFI_SYSV;
>>>>> +#endif
>>>>>
>>>>> and that's the only active change that's needed.
>>>>
>>>> I suggest removing the #ifdef __ARM_PCS_VFP #ifdef.  We always have
>>>> support for both FFI_SYSV and FFI_VFP compiled in, so it doesn't make
>>>> sense to leave this bit of logic out when we are under __ARM_PCS.
>>>
>>> I could do, although cif->nfixedargs is also not present except in the
>>> VFP case, so
>>> it would still need an ifdef.
>>>
>>> I did that since I thought it best not to change the layout of the CIF
>>> structure for
>>> existing ARM builds.
>>
>> Sigh...you're right about the layout issue :(
>>
>> Still I think it's best we keep the full FFI_SYSV/VFP capabilities in,
>> or we'll have the weird case of "FFI_VFP with buggy variadics" under softfp.
> 
> Wouldn't it make more sense to ban FFI_VFP under softfp?

Well, that would be simple :)

The current code paths already have support for both ABIs;  since this
is a "foreign function interface", I thought it made sense to support
all ABIs on that platform, for the most flexibility, so I wrote it that way.

If we were to make softfp/hardfp a libffi build-time configuration,
we're already wasting some runtime cost now. In fact, we would have no
need for 'FFI_VFP' at all; FFI_SYSV would just mean "the ABI".

>> I suggest moving the nfixedargs field into the ARM FFI_EXTRA_CIF_FIELDS,
>> placed after the current VFP fields; this means doing away with the
>> FFI_TARGET_SPECIFIC_VARIADIC symbol, and probably having a
>> machine-specific ffi_prep_cif_var_machdep() function, analogous to the
>> current ffi_prep_cif_machdep().
> 
> That would mean changing every back end to add that function or again

Okay, then we should keep the preprocessor symbol then; we can then use
the basic ffi_prep_cif() as the default ffi_prep_cif_var() implementation.

> ifdefing for it, and the belief is that storing the nfixedargs value
> is likely to
> be useful to some backends who have to do different calling conventions for
> variadics anyway.

The keyword is "some" :)
Some means it is not universally needed for supporting variadics, thus
probably should be in FFI_EXTRA_CIF_FIELDS.

Chung-Lin

^ permalink raw reply	[flat|nested] 22+ messages in thread

* PowerPC failures (Was: [PATCH] Add variadic support)
  2011-02-22 15:41 [PATCH] Add variadic support Dr. David Alan Gilbert
  2011-02-23 12:39 ` Anthony Green
@ 2011-02-23 21:16 ` Anthony Green
  2011-02-24 10:43   ` David Gilbert
  1 sibling, 1 reply; 22+ messages in thread
From: Anthony Green @ 2011-02-23 21:16 UTC (permalink / raw)
  To: Dr. David Alan Gilbert; +Cc: libffi-discuss, Marcus.Shawcroft, cltang

"Dr. David Alan Gilbert" <david.gilbert@linaro.org> writes:

>    2) I've tested it on i386 (1659 expected passes, 15 unsupported),
>                         armel (1654 expected passes, 5 unexpecteed passes (cls_longdouble.c), 15 unsupported)
>                         armhf (1654 expected passes, 5 unexpecteed passes (cls_longdouble.c), 15 unsupported)
>                         s390x (1639 expected passes, 5 unexpected failures - err_bad_abi.c)
> 		Those cls_longdouble unexpected passes and err_bad_abi.c
> 		failure are there in the current head.
> 	powerpc64 seems to be broken on current head, but I tested this patch
> 	on powerpc64 on the .0.9 release and it was OK.

I just tested the head on powerpc64-unknown-linux-gnu and had no
problems.  You appear to be testing powerpc64-unknown-linux-gnu like me,
but with a different gcc and binutils version.

You wrote on the wiki...
> Failure is src/powerpc/ffi.c:961 where cif->abi is expected to be
> FFI_GCC_SYSV or FFI_SYSV but is getting FFI_LINUX

Line 961 is only run when POWERPC64 isn't defined.

POWERPC64 is defined in src/powerpc/ffitarget.h.   Could you please see
why it's not being defined in your build environment?

Thanks!

AG

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH] Add variadic support
  2011-02-23 17:39             ` Chung-Lin Tang
@ 2011-02-24  6:37               ` Chung-Lin Tang
  2011-02-25 12:56               ` David Gilbert
  1 sibling, 0 replies; 22+ messages in thread
From: Chung-Lin Tang @ 2011-02-24  6:37 UTC (permalink / raw)
  To: David Gilbert; +Cc: Anthony Green, libffi-discuss, Marcus.Shawcroft

On 2011/2/24, Chung-Lin Tang wrote:
>>> I suggest moving the nfixedargs field into the ARM FFI_EXTRA_CIF_FIELDS,
>>> >> placed after the current VFP fields; this means doing away with the
>>> >> FFI_TARGET_SPECIFIC_VARIADIC symbol, and probably having a
>>> >> machine-specific ffi_prep_cif_var_machdep() function, analogous to the
>>> >> current ffi_prep_cif_machdep().
>> > 
>> > That would mean changing every back end to add that function or again
> Okay, then we should keep the preprocessor symbol then; we can then use
> the basic ffi_prep_cif() as the default ffi_prep_cif_var() implementation.
> 

I just wanted to add that, instead of a new ffi_prep_cif_var_machdep()
function for all backends to add, thus needing a preprocessor symbol to
set a 'default' implementation (to avoid changing every port), we can do
this:  add a flag value, passed by cif->flags, into
ffi_prep_cif_machdep() to indicate variadic function processing. We'll
just need a new flag value CPP symbol, but avoid an ugly #ifndef in the
machine-independent code.

I'm suggesting this because I see cif->flags being overwritten anyways
by the backend ffi_prep_cif_machdep() implementations I see, so it
should be valid to pass in values for this use.

Chung-Lin

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: PowerPC failures (Was: [PATCH] Add variadic support)
  2011-02-23 21:16 ` PowerPC failures (Was: [PATCH] Add variadic support) Anthony Green
@ 2011-02-24 10:43   ` David Gilbert
  2011-02-24 22:37     ` PowerPC failures Anthony Green
  0 siblings, 1 reply; 22+ messages in thread
From: David Gilbert @ 2011-02-24 10:43 UTC (permalink / raw)
  To: Anthony Green; +Cc: libffi-discuss, Marcus.Shawcroft, cltang

On 23 February 2011 21:17, Anthony Green <green@redhat.com> wrote:
> "Dr. David Alan Gilbert" <david.gilbert@linaro.org> writes:
>
>>    2) I've tested it on i386 (1659 expected passes, 15 unsupported),
>>                         armel (1654 expected passes, 5 unexpecteed passes (cls_longdouble.c), 15 unsupported)
>>                         armhf (1654 expected passes, 5 unexpecteed passes (cls_longdouble.c), 15 unsupported)
>>                         s390x (1639 expected passes, 5 unexpected failures - err_bad_abi.c)
>>               Those cls_longdouble unexpected passes and err_bad_abi.c
>>               failure are there in the current head.
>>       powerpc64 seems to be broken on current head, but I tested this patch
>>       on powerpc64 on the .0.9 release and it was OK.
>
> I just tested the head on powerpc64-unknown-linux-gnu and had no
> problems.  You appear to be testing powerpc64-unknown-linux-gnu like me,
> but with a different gcc and binutils version.
>
> You wrote on the wiki...
>> Failure is src/powerpc/ffi.c:961 where cif->abi is expected to be
>> FFI_GCC_SYSV or FFI_SYSV but is getting FFI_LINUX
>
> Line 961 is only run when POWERPC64 isn't defined.
>
> POWERPC64 is defined in src/powerpc/ffitarget.h.   Could you please see
> why it's not being defined in your build environment?

It seems to be a 32 v 64 issue. It's got a 64bit kernel but by default
gcc is producing 32bit
binaries, the __powerpc64__ flag is only set if -m64 is passed.
However, config.guess is saying
it's a powerpc64-unknown-linux-gnu - and hence that's the directory
that was created.

So I guess it's actually a failure on powerpc32 not  64.


$ uname -a
Linux daves-g5 2.6.26-2-powerpc64 #1 SMP Thu Nov 25 15:01:22 UTC 2010
ppc64 GNU/Linux
$ cat /etc/issue
Debian GNU/Linux 5.0 \n \l
$ gcc -v
Using built-in specs.
Target: powerpc-linux-gnu
Configured with: ../src/configure -v --with-pkgversion='Debian
4.3.2-1.1' --with-bugurl=file:///usr/share/doc/gcc-4.3/README.Bugs
--enable-languages=c,c++,fortran,objc,obj-c++ --prefix=/usr
--enable-shared --with-system-zlib --libexecdir=/usr/lib
--without-included-gettext --enable-threads=posix --enable-nls
--with-gxx-include-dir=/usr/include/c++/4.3 --program-suffix=-4.3
--enable-clocale=gnu --enable-libstdcxx-debug --enable-objc-gc
--enable-mpfr --disable-softfloat --enable-secureplt
--enable-targets=powerpc-linux,powerpc64-linux --with-cpu=default32
--with-long-double-128 --enable-checking=release
--build=powerpc-linux-gnu --host=powerpc-linux-gnu
--target=powerpc-linux-gnu
Thread model: posix
gcc version 4.3.2 (Debian 4.3.2-1.1)

There is the opposite problem that if you pass CFLAGS=-m64 and/or run
with linux64 the make check fails because it doesn't build the tests
64bit:

Executing on host: gcc
/home/dg/libffi-3.0.10rc5/testsuite/libffi.call/closure_fn0.c  -O0 -W
-Wall  -I/home/dg/libffi-3.0.10rc5/powerpc64-unknown-linux-gnu/testsuite/../include
-I/home/dg/libffi-3.0.10rc5/testsuite/../include
-I/home/dg/libffi-3.0.10rc5/powerpc64-unknown-linux-gnu/testsuite/../include/..
-L/home/dg/libffi-3.0.10rc5/powerpc64-unknown-linux-gnu/testsuite/../.libs
 -lffi -lm   -o ./closure_fn0.exe    (timeout = 300)
/usr/bin/ld: skipping incompatible
/home/dg/libffi-3.0.10rc5/powerpc64-unknown-linux-gnu/testsuite/../.libs/libffi.so
when searching for -lffi

(here that libffi.so is 64bi)

Dave

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: PowerPC failures
  2011-02-24 10:43   ` David Gilbert
@ 2011-02-24 22:37     ` Anthony Green
  2011-02-24 22:41       ` Matthias Klose
  0 siblings, 1 reply; 22+ messages in thread
From: Anthony Green @ 2011-02-24 22:37 UTC (permalink / raw)
  To: David Gilbert; +Cc: libffi-discuss, Marcus.Shawcroft, cltang

David Gilbert <david.gilbert@linaro.org> writes:

> It seems to be a 32 v 64 issue. It's got a 64bit kernel but by default
> gcc is producing 32bit
> binaries, the __powerpc64__ flag is only set if -m64 is passed.
> However, config.guess is saying
> it's a powerpc64-unknown-linux-gnu - and hence that's the directory
> that was created.

I just built and tested on powerpc64-unknown-linux-gnu with both -m32
and -m64 and both worked fine (and I made sure that it was actually
generating 32- and 64-bit binaries respectively).  Something funky is
definitely going on here.  I'll look into it more tomorrow.

> There is the opposite problem that if you pass CFLAGS=-m64 and/or run
> with linux64 the make check fails because it doesn't build the tests
> 64bit:

I used "CC='gcc -m64'" and it worked.  I'll look into the CFLAGS issue
as well.

Thanks David.

AG

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: PowerPC failures
  2011-02-24 22:37     ` PowerPC failures Anthony Green
@ 2011-02-24 22:41       ` Matthias Klose
  2011-02-25 19:18         ` Anthony Green
  0 siblings, 1 reply; 22+ messages in thread
From: Matthias Klose @ 2011-02-24 22:41 UTC (permalink / raw)
  To: Anthony Green; +Cc: David Gilbert, libffi-discuss, Marcus.Shawcroft, cltang

On 24.02.2011 23:37, Anthony Green wrote:
> David Gilbert<david.gilbert@linaro.org>  writes:
>
>> It seems to be a 32 v 64 issue. It's got a 64bit kernel but by default
>> gcc is producing 32bit
>> binaries, the __powerpc64__ flag is only set if -m64 is passed.
>> However, config.guess is saying
>> it's a powerpc64-unknown-linux-gnu - and hence that's the directory
>> that was created.
>
> I just built and tested on powerpc64-unknown-linux-gnu with both -m32
> and -m64 and both worked fine (and I made sure that it was actually
> generating 32- and 64-bit binaries respectively).  Something funky is
> definitely going on here.  I'll look into it more tomorrow.

for me the easiest way to test is to enter the chroot with `linux32', or prefix 
the configure and make calls. then I make sure that the userland is detected as 
32bit while running a 64bit kernel.

   Matthias

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH] Add variadic support
  2011-02-23 17:39             ` Chung-Lin Tang
  2011-02-24  6:37               ` Chung-Lin Tang
@ 2011-02-25 12:56               ` David Gilbert
  2011-02-25 13:23                 ` Chung-Lin Tang
  1 sibling, 1 reply; 22+ messages in thread
From: David Gilbert @ 2011-02-25 12:56 UTC (permalink / raw)
  To: Chung-Lin Tang; +Cc: Anthony Green, libffi-discuss, Marcus.Shawcroft

On 23 February 2011 17:39, Chung-Lin Tang <cltang@linaro.org> wrote:
> On 2011/2/24 01:20 AM, David Gilbert wrote:

>> Wouldn't it make more sense to ban FFI_VFP under softfp?
>
> Well, that would be simple :)
>
> The current code paths already have support for both ABIs;  since this
> is a "foreign function interface", I thought it made sense to support
> all ABIs on that platform, for the most flexibility, so I wrote it that way.
>
> If we were to make softfp/hardfp a libffi build-time configuration,
> we're already wasting some runtime cost now. In fact, we would have no
> need for 'FFI_VFP' at all; FFI_SYSV would just mean "the ABI".

Yes, you're right - I hadn't thought of calling VFP code from a SYSV program.

>>> I suggest moving the nfixedargs field into the ARM FFI_EXTRA_CIF_FIELDS,
>>> placed after the current VFP fields; this means doing away with the
>>> FFI_TARGET_SPECIFIC_VARIADIC symbol, and probably having a
>>> machine-specific ffi_prep_cif_var_machdep() function, analogous to the
>>> current ffi_prep_cif_machdep().
>>
>> That would mean changing every back end to add that function or again
>
> Okay, then we should keep the preprocessor symbol then; we can then use
> the basic ffi_prep_cif() as the default ffi_prep_cif_var() implementation.
>
>> ifdefing for it, and the belief is that storing the nfixedargs value
>> is likely to
>> be useful to some backends who have to do different calling conventions for
>> variadics anyway.
>
> The keyword is "some" :)
> Some means it is not universally needed for supporting variadics, thus
> probably should be in FFI_EXTRA_CIF_FIELDS.

OK, I can see we can do this if I switch over to making a
ffi_prep_cif_var_machdep,
which isn't too bad.

> On 2011/2/24, Chung-Lin Tang wrote:
>>>> I suggest moving the nfixedargs field into the ARM FFI_EXTRA_CIF_FIELDS,
>>>> >> placed after the current VFP fields; this means doing away with the
>>>> >> FFI_TARGET_SPECIFIC_VARIADIC symbol, and probably having a
>>>> >> machine-specific ffi_prep_cif_var_machdep() function, analogous to the
>>>> >> current ffi_prep_cif_machdep().
>>> >
>>> > That would mean changing every back end to add that function or again
>> Okay, then we should keep the preprocessor symbol then; we can then use
>> the basic ffi_prep_cif() as the default ffi_prep_cif_var() implementation.
>>
>
> I just wanted to add that, instead of a new ffi_prep_cif_var_machdep()
> function for all backends to add, thus needing a preprocessor symbol to
> set a 'default' implementation (to avoid changing every port), we can do
> this:  add a flag value, passed by cif->flags, into
> ffi_prep_cif_machdep() to indicate variadic function processing. We'll
> just need a new flag value CPP symbol, but avoid an ugly #ifndef in the
> machine-independent code.
>
> I'm suggesting this because I see cif->flags being overwritten anyways
> by the backend ffi_prep_cif_machdep() implementations I see, so it
> should be valid to pass in values for this use.

The flag doesn't provide the space for the generic code to pass the
extra information (the number of fixed args) to ffi_prep_cif_machdep_var
or the equivalent code in ffi_prep_cif_machdep.

I think the way to move it in the direction of what you're suggesting is:
   1) as per my code have an ffi_prep_cif and ffi_prep_cif_var that
both call ffi_prep_cif_core
   2) Explicitly pass ffi_prep_cif_core the extra parameter rather
than storing it in cif.
   3) Still keep an ifdef to say if the backend has explicit variadic
support, if that ifdef is set
       then ffi_prep_cif_core will call ffi_prep_cif_machdep_var when
appropriate.
   4) ffi_prep_cif_machdep_var will get the argument count passed as
explicit parameters,
       then it can process them as it feels fit including storing them
in an EXTRA_CIF_FIELD
       if the backend wants to.

Thoughts?

Dave

>
> Chung-Lin
>

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH] Add variadic support
  2011-02-25 12:56               ` David Gilbert
@ 2011-02-25 13:23                 ` Chung-Lin Tang
  2011-02-25 19:02                   ` Anthony Green
  2011-03-07 18:19                   ` David Gilbert
  0 siblings, 2 replies; 22+ messages in thread
From: Chung-Lin Tang @ 2011-02-25 13:23 UTC (permalink / raw)
  To: David Gilbert; +Cc: Anthony Green, libffi-discuss, Marcus.Shawcroft

On 2011/2/25 08:56 PM, David Gilbert wrote:
>> I'm suggesting this because I see cif->flags being overwritten anyways
>> > by the backend ffi_prep_cif_machdep() implementations I see, so it
>> > should be valid to pass in values for this use.
> The flag doesn't provide the space for the generic code to pass the
> extra information (the number of fixed args) to ffi_prep_cif_machdep_var
> or the equivalent code in ffi_prep_cif_machdep.

Yeah, I forgot that...

This might sound a bit hackish, but considering the usual realistic
number of fixed args, we should be able to splice the cif->flags word:
magic value in upper 16-bits, while lower half contains the passed-in
nfixedargs.

> I think the way to move it in the direction of what you're suggesting is:
>    1) as per my code have an ffi_prep_cif and ffi_prep_cif_var that
> both call ffi_prep_cif_core
>    2) Explicitly pass ffi_prep_cif_core the extra parameter rather
> than storing it in cif.
>    3) Still keep an ifdef to say if the backend has explicit variadic
> support, if that ifdef is set
>        then ffi_prep_cif_core will call ffi_prep_cif_machdep_var when
> appropriate.
>    4) ffi_prep_cif_machdep_var will get the argument count passed as
> explicit parameters,
>        then it can process them as it feels fit including storing them
> in an EXTRA_CIF_FIELD
>        if the backend wants to.

Yeah, I think that's fine. My attempt was to avoid the extra #ifdefs and
extra backend machine-dependent function; if that's unavoidable, then I
think this is fine.

Anthony, as maintainer, do you have any ideas?

Thanks,
Chung-Lin

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH] Add variadic support
  2011-02-25 13:23                 ` Chung-Lin Tang
@ 2011-02-25 19:02                   ` Anthony Green
  2011-02-28  9:08                     ` David Gilbert
  2011-03-07 18:19                   ` David Gilbert
  1 sibling, 1 reply; 22+ messages in thread
From: Anthony Green @ 2011-02-25 19:02 UTC (permalink / raw)
  To: Chung-Lin Tang; +Cc: David Gilbert, libffi-discuss, Marcus.Shawcroft

Chung-Lin Tang <cltang@linaro.org> writes:

> This might sound a bit hackish, but considering the usual realistic
> number of fixed args, we should be able to splice the cif->flags word:
> magic value in upper 16-bits, while lower half contains the passed-in
> nfixedargs.
>
>> I think the way to move it in the direction of what you're suggesting is:
>>    1) as per my code have an ffi_prep_cif and ffi_prep_cif_var that
>> both call ffi_prep_cif_core
>>    2) Explicitly pass ffi_prep_cif_core the extra parameter rather
>> than storing it in cif.
>>    3) Still keep an ifdef to say if the backend has explicit variadic
>> support, if that ifdef is set
>>        then ffi_prep_cif_core will call ffi_prep_cif_machdep_var when
>> appropriate.
>>    4) ffi_prep_cif_machdep_var will get the argument count passed as
>> explicit parameters,
>>        then it can process them as it feels fit including storing them
>> in an EXTRA_CIF_FIELD
>>        if the backend wants to.
>
> Yeah, I think that's fine. My attempt was to avoid the extra #ifdefs and
> extra backend machine-dependent function; if that's unavoidable, then I
> think this is fine.
>
> Anthony, as maintainer, do you have any ideas?
>

I always envisioned it working this way:

  - add a new FFI type, say FFI_TYPE_VARIADIC
  - users would just append this to the array of types when they
    create a cif for a variadic function

No new API is required in this case.  Wouldn't this work?

AG
  

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: PowerPC failures
  2011-02-24 22:41       ` Matthias Klose
@ 2011-02-25 19:18         ` Anthony Green
  0 siblings, 0 replies; 22+ messages in thread
From: Anthony Green @ 2011-02-25 19:18 UTC (permalink / raw)
  To: Matthias Klose; +Cc: David Gilbert, libffi-discuss, Marcus.Shawcroft, cltang

Matthias Klose <doko@ubuntu.com> writes:

> for me the easiest way to test is to enter the chroot with `linux32',
> or prefix the configure and make calls. then I make sure that the
> userland is detected as 32bit while running a 64bit kernel.

This was a great tip - thanks!   I see all of tests failing in this
32-bit chroot now.

AG

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH] Add variadic support
  2011-02-25 19:02                   ` Anthony Green
@ 2011-02-28  9:08                     ` David Gilbert
  0 siblings, 0 replies; 22+ messages in thread
From: David Gilbert @ 2011-02-28  9:08 UTC (permalink / raw)
  To: Anthony Green; +Cc: Chung-Lin Tang, libffi-discuss, Marcus.Shawcroft

On 25 February 2011 19:03, Anthony Green <green@redhat.com> wrote:
> Chung-Lin Tang <cltang@linaro.org> writes:


>> Anthony, as maintainer, do you have any ideas?
>>
>
> I always envisioned it working this way:
>
>  - add a new FFI type, say FFI_TYPE_VARIADIC
>  - users would just append this to the array of types when they
>    create a cif for a variadic function
>
> No new API is required in this case.  Wouldn't this work?

I think my main reason against that was it would require every backend
to be modified
to skip it (and skip a hole in the values array).

Dave

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH] Add variadic support
  2011-02-25 13:23                 ` Chung-Lin Tang
  2011-02-25 19:02                   ` Anthony Green
@ 2011-03-07 18:19                   ` David Gilbert
  2011-03-16  9:52                     ` Chung-Lin Tang
  1 sibling, 1 reply; 22+ messages in thread
From: David Gilbert @ 2011-03-07 18:19 UTC (permalink / raw)
  To: libffi-discuss; +Cc: Anthony Green, Chung-Lin Tang, Marcus.Shawcroft, markos

Hi,
  Here is a rework of the previous patch, based on the previous discussion and
suggestions from Chung-Lin.

(This is relative libffi git rev cbb062cc35c518004f1ab45c847f8ec4f66069ad)

Dave

diff --git a/doc/libffi.texi b/doc/libffi.texi
index 5cdd667..a7c629a 100644
--- a/doc/libffi.texi
+++ b/doc/libffi.texi
@@ -133,8 +133,6 @@ This initializes @var{cif} according to the given
parameters.
 you want.  @ref{Multiple ABIs} for more information.

 @var{nargs} is the number of arguments that this function accepts.
-@samp{libffi} does not yet handle varargs functions; see @ref{Missing
-Features} for more information.

 @var{rtype} is a pointer to an @code{ffi_type} structure that
 describes the return type of the function.  @xref{Types}.
@@ -150,6 +148,28 @@ objects is incorrect; or @code{FFI_BAD_ABI} if
the @var{abi} parameter
 is invalid.
 @end defun

+If the function being called is variadic (varargs) then @code{ffi_prep_cif_var}
+must be used instead of @code{ffi_prep_cif}.
+
+@findex ffi_prep_cif_var
+@defun ffi_status ffi_prep_cif_var (ffi_cif *@var{cif}, ffi_abi
@var{abi}, unsigned int @var{nfixedargs}, unsigned int
@var{ntotalargs}, ffi_type *@var{rtype}, ffi_type **@var{argtypes})
+This initializes @var{cif} according to the given parameters for
+a call to a variadic function.  In general it's operation is the
+same as for @code{ffi_prep_cif} except that:
+
+@var{nfixedargs} is the number of fixed arguments, prior to any
+variadic arguments.  It must be greater than zero.
+
+@var{ntotalargs} the total number of arguments, including variadic
+and fixed arguments.
+
+Note that, different cif's must be prepped for calls to the same
+function when different numbers of arguments are passed.
+
+Also note that a call to @code{ffi_prep_cif_var} with
@var{nfixedargs}=@var{nototalargs}
+is NOT equivalent to a call to @code{ffi_prep_cif}.
+
+@end defun

 To call a function using an initialized @code{ffi_cif}, use the
 @code{ffi_call} function:
@@ -572,9 +592,7 @@ support for these.

 @itemize @bullet
 @item
-There is no support for calling varargs functions.  This may work on
-some platforms, depending on how the ABI is defined, but it is not
-reliable.
+Variadic closures.

 @item
 There is no support for bit fields in structures.
@@ -591,6 +609,7 @@ The ``raw'' API is undocumented.
 @c anything else?
 @end itemize

+Note that variadic support is very new and tested on a relatively
small number of platforms.

 @node Index
 @unnumbered Index
diff --git a/include/ffi.h.in b/include/ffi.h.in
index 96b8fd3..f27f4f3 100644
--- a/include/ffi.h.in
+++ b/include/ffi.h.in
@@ -203,6 +203,15 @@ typedef struct {
 #endif
 } ffi_cif;

+/* Used internally, but overridden by some architectures */
+ffi_status ffi_prep_cif_core(ffi_cif *cif,
+			     ffi_abi abi,
+			     unsigned int isvariadic,
+			     unsigned int nfixedargs,
+			     unsigned int ntotalargs,
+			     ffi_type *rtype,
+			     ffi_type **atypes);
+
 /* ---- Definitions for the raw API -------------------------------------- */

 #ifndef FFI_SIZEOF_ARG
@@ -380,6 +389,13 @@ ffi_status ffi_prep_cif(ffi_cif *cif,
 			ffi_type *rtype,
 			ffi_type **atypes);

+ffi_status ffi_prep_cif_var(ffi_cif *cif,
+			    ffi_abi abi,
+			    unsigned int nfixedargs,
+			    unsigned int ntotalargs,
+			    ffi_type *rtype,
+			    ffi_type **atypes);
+
 void ffi_call(ffi_cif *cif,
 	      void (*fn)(void),
 	      void *rvalue,
diff --git a/include/ffi_common.h b/include/ffi_common.h
index d953762..919eec2 100644
--- a/include/ffi_common.h
+++ b/include/ffi_common.h
@@ -75,6 +75,8 @@ void ffi_type_test(ffi_type *a, char *file, int line);

 /* Perform machine dependent cif processing */
 ffi_status ffi_prep_cif_machdep(ffi_cif *cif);
+ffi_status ffi_prep_cif_machdep_var(ffi_cif *cif,
+	 unsigned int nfixedargs, unsigned int ntotalargs);

 /* Extended cif, used in callback from assembly routine */
 typedef struct
diff --git a/man/Makefile.am b/man/Makefile.am
index 2519277..afcbfb6 100644
--- a/man/Makefile.am
+++ b/man/Makefile.am
@@ -2,7 +2,7 @@

 AUTOMAKE_OPTIONS=foreign

-EXTRA_DIST = ffi.3 ffi_call.3 ffi_prep_cif.3
+EXTRA_DIST = ffi.3 ffi_call.3 ffi_prep_cif.3 ffi_prep_cif_var.3

-man_MANS = ffi.3 ffi_call.3 ffi_prep_cif.3
+man_MANS = ffi.3 ffi_call.3 ffi_prep_cif.3 ffi_prep_cif_var.3

diff --git a/man/ffi.3 b/man/ffi.3
index 18b5d5d..1f1d303 100644
--- a/man/ffi.3
+++ b/man/ffi.3
@@ -16,6 +16,15 @@ libffi, -lffi
 .Fa "ffi_type **atypes"
 .Fc
 .Ft void
+.Fo ffi_prep_cif_var
+.Fa "ffi_cif *cif"
+.Fa "ffi_abi abi"
+.Fa "unsigned int nfixedargs"
+.Fa "unsigned int ntotalargs"
+.Fa "ffi_type *rtype"
+.Fa "ffi_type **atypes"
+.Fc
+.Ft void
 .Fo ffi_call
 .Fa "ffi_cif *cif"
 .Fa "void (*fn)(void)"
@@ -28,4 +37,5 @@ generate a call to another function at runtime
without requiring knowledge of
 the called function's interface at compile time.
 .Sh SEE ALSO
 .Xr ffi_prep_cif 3 ,
+.Xr ffi_prep_cif_var 3 ,
 .Xr ffi_call 3
diff --git a/man/ffi_prep_cif.3 b/man/ffi_prep_cif.3
index 9436b31..fe66452 100644
--- a/man/ffi_prep_cif.3
+++ b/man/ffi_prep_cif.3
@@ -37,7 +37,9 @@ structs that describe the data type, size and
alignment of each argument.
 points to an
 .Nm ffi_type
 that describes the data type, size and alignment of the
-return value.
+return value. Note that to call a variadic function
+.Nm ffi_prep_cif_var
+must be used instead.
 .Sh RETURN VALUES
 Upon successful completion,
 .Nm ffi_prep_cif
@@ -63,4 +65,5 @@ defined in
 .
 .Sh SEE ALSO
 .Xr ffi 3 ,
-.Xr ffi_call 3
+.Xr ffi_call 3 ,
+.Xr ffi_prep_cif_var 3
diff --git a/man/ffi_prep_cif_var.3 b/man/ffi_prep_cif_var.3
new file mode 100644
index 0000000..1b39c03
--- /dev/null
+++ b/man/ffi_prep_cif_var.3
@@ -0,0 +1,73 @@
+.Dd January 25, 2011
+.Dt ffi_prep_cif_var 3
+.Sh NAME
+.Nm ffi_prep_cif_var
+.Nd Prepare a
+.Nm ffi_cif
+structure for use with
+.Nm ffi_call
+for variadic functions.
+.Sh SYNOPSIS
+.In ffi.h
+.Ft ffi_status
+.Fo ffi_prep_cif_var
+.Fa "ffi_cif *cif"
+.Fa "ffi_abi abi"
+.Fa "unsigned int nfixedargs"
+.Fa "unsigned int ntotalargs"
+.Fa "ffi_type *rtype"
+.Fa "ffi_type **atypes"
+.Fc
+.Sh DESCRIPTION
+The
+.Nm ffi_prep_cif_var
+function prepares a
+.Nm ffi_cif
+structure for use with
+.Nm ffi_call
+for variadic functions.
+.Fa abi
+specifies a set of calling conventions to use.
+.Fa atypes
+is an array of
+.Fa ntotalargs
+pointers to
+.Nm ffi_type
+structs that describe the data type, size and alignment of each argument.
+.Fa rtype
+points to an
+.Nm ffi_type
+that describes the data type, size and alignment of the
+return value.
+.Fa nfixedargs
+must contain the number of fixed (non-variadic) arguments.
+Note that to call a non-variadic function
+.Nm ffi_prep_cif
+must be used.
+.Sh RETURN VALUES
+Upon successful completion,
+.Nm ffi_prep_cif_var
+returns
+.Nm FFI_OK .
+It will return
+.Nm FFI_BAD_TYPEDEF
+if
+.Fa cif
+is
+.Nm NULL
+or
+.Fa atypes
+or
+.Fa rtype
+is malformed. If
+.Fa abi
+does not refer to a valid ABI,
+.Nm FFI_BAD_ABI
+will be returned. Available ABIs are
+defined in
+.Nm <ffitarget.h>
+.
+.Sh SEE ALSO
+.Xr ffi 3 ,
+.Xr ffi_call 3 ,
+.Xr ffi_prep_cif 3
diff --git a/src/arm/ffi.c b/src/arm/ffi.c
index 885a9cb..c226dbd 100644
--- a/src/arm/ffi.c
+++ b/src/arm/ffi.c
@@ -189,6 +189,18 @@ ffi_status ffi_prep_cif_machdep(ffi_cif *cif)
   return FFI_OK;
 }

+/* Perform machine dependent cif processing for variadic calls */
+ffi_status ffi_prep_cif_machdep_var(ffi_cif *cif,
+				    unsigned int nfixedargs,
+				    unsigned int ntotalargs)
+{
+  /* VFP variadic calls actually use the SYSV ABI */
+  if (cif->abi == FFI_VFP)
+	cif->abi = FFI_SYSV;
+
+  return ffi_prep_cif_machdep(cif);
+}
+
 /* Prototypes for assembly functions, in sysv.S */
 extern void ffi_call_SYSV (void (*fn)(void), extended_cif *,
unsigned, unsigned, unsigned *);
 extern void ffi_call_VFP (void (*fn)(void), extended_cif *, unsigned,
unsigned, unsigned *);
diff --git a/src/arm/ffitarget.h b/src/arm/ffitarget.h
index ce25b23..b9a0428 100644
--- a/src/arm/ffitarget.h
+++ b/src/arm/ffitarget.h
@@ -55,6 +55,8 @@ typedef enum ffi_abi {
 #define FFI_TYPE_STRUCT_VFP_FLOAT  (FFI_TYPE_LAST + 1)
 #define FFI_TYPE_STRUCT_VFP_DOUBLE (FFI_TYPE_LAST + 2)

+#define FFI_TARGET_SPECIFIC_VARIADIC
+
 /* ---- Definitions for closures ----------------------------------------- */

 #define FFI_CLOSURES 1
@@ -62,4 +64,3 @@ typedef enum ffi_abi {
 #define FFI_NATIVE_RAW_API 0

 #endif
-
diff --git a/src/cris/ffi.c b/src/cris/ffi.c
index f25d7b4..aaca5b1 100644
--- a/src/cris/ffi.c
+++ b/src/cris/ffi.c
@@ -153,21 +153,24 @@ ffi_prep_args (char *stack, extended_cif * ecif)
   return (struct_count);
 }

-ffi_status
-ffi_prep_cif (ffi_cif * cif,
-	      ffi_abi abi, unsigned int nargs,
-	      ffi_type * rtype, ffi_type ** atypes)
+ffi_status FFI_HIDDEN
+ffi_prep_cif_core (ffi_cif * cif,
+	           ffi_abi abi, unsigned int isvariadic,
+		   unsigned int nfixedargs, unsigned int ntotalargs,
+	           ffi_type * rtype, ffi_type ** atypes)
 {
   unsigned bytes = 0;
   unsigned int i;
   ffi_type **ptr;

   FFI_ASSERT (cif != NULL);
+  FFI_ASSERT((!isvariadic) || (nfixedargs >= 1));
+  FFI_ASSERT(nfixedargs <= ntotalargs);
   FFI_ASSERT (abi > FFI_FIRST_ABI && abi < FFI_LAST_ABI);

   cif->abi = abi;
   cif->arg_types = atypes;
-  cif->nargs = nargs;
+  cif->nargs = ntotalargs;
   cif->rtype = rtype;

   cif->flags = 0;
diff --git a/src/prep_cif.c b/src/prep_cif.c
index 8548cfd..0e2e116 100644
--- a/src/prep_cif.c
+++ b/src/prep_cif.c
@@ -90,20 +90,33 @@ static ffi_status initialize_aggregate(ffi_type *arg)
 /* Perform machine independent ffi_cif preparation, then call
    machine dependent routine. */

-ffi_status ffi_prep_cif(ffi_cif *cif, ffi_abi abi, unsigned int nargs,
-			ffi_type *rtype, ffi_type **atypes)
+/* For non variadic functions isvariadic should be 0 and
+   nfixedargs==ntotalargs.
+
+   For variadic calls, isvariadic should be 1 and nfixedargs
+   and ntotalargs set as appropriate. nfixedargs must always be >=1 */
+
+
+ffi_status FFI_HIDDEN ffi_prep_cif_core(ffi_cif *cif, ffi_abi abi,
+			     unsigned int isvariadic,
+                             unsigned int nfixedargs,
+                             unsigned int ntotalargs,
+			     ffi_type *rtype, ffi_type **atypes)
 {
   unsigned bytes = 0;
   unsigned int i;
   ffi_type **ptr;

   FFI_ASSERT(cif != NULL);
+  FFI_ASSERT((!isvariadic) || (nfixedargs >= 1));
+  FFI_ASSERT(nfixedargs <= ntotalargs);
+
   if (! (abi > FFI_FIRST_ABI && abi < FFI_LAST_ABI))
     return FFI_BAD_ABI;

   cif->abi = abi;
   cif->arg_types = atypes;
-  cif->nargs = nargs;
+  cif->nargs = ntotalargs;
   cif->rtype = rtype;

   cif->flags = 0;
@@ -159,10 +172,31 @@ ffi_status ffi_prep_cif(ffi_cif *cif, ffi_abi
abi, unsigned int nargs,
   cif->bytes = bytes;

   /* Perform machine dependent cif processing */
+#ifdef FFI_TARGET_SPECIFIC_VARIADIC
+  if (isvariadic)
+	return ffi_prep_cif_machdep_var(cif, nfixedargs, ntotalargs);
+#endif
+
   return ffi_prep_cif_machdep(cif);
 }
 #endif /* not __CRIS__ */

+ffi_status ffi_prep_cif(ffi_cif *cif, ffi_abi abi, unsigned int nargs,
+			     ffi_type *rtype, ffi_type **atypes)
+{
+  return ffi_prep_cif_core(cif, abi, 0, nargs, nargs, rtype, atypes);
+}
+
+ffi_status ffi_prep_cif_var(ffi_cif *cif,
+                            ffi_abi abi,
+                            unsigned int nfixedargs,
+                            unsigned int ntotalargs,
+                            ffi_type *rtype,
+                            ffi_type **atypes)
+{
+  return ffi_prep_cif_core(cif, abi, 1, nfixedargs, ntotalargs, rtype, atypes);
+}
+
 #if FFI_CLOSURES

 ffi_status
diff --git a/testsuite/libffi.call/cls_double_va.c
b/testsuite/libffi.call/cls_double_va.c
index 62bebbd..1a2706e 100644
--- a/testsuite/libffi.call/cls_double_va.c
+++ b/testsuite/libffi.call/cls_double_va.c
@@ -6,7 +6,6 @@

 /* { dg-do run { xfail strongarm*-*-* xscale*-*-* } } */
 /* { dg-output "" { xfail avr32*-*-* } } */
-/* { dg-skip-if "" arm*-*-* { "-mfloat-abi=hard" } { "" } } */

 #include "ffitest.h"

@@ -36,7 +35,8 @@ int main (void)
 	arg_types[1] = &ffi_type_double;
 	arg_types[2] = NULL;

-	CHECK(ffi_prep_cif(&cif, FFI_DEFAULT_ABI, 2, &ffi_type_sint,
+	/* This printf call is variadic */
+	CHECK(ffi_prep_cif_var(&cif, FFI_DEFAULT_ABI, 1, 2, &ffi_type_sint,
 		arg_types) == FFI_OK);

 	args[0] = &format;
@@ -48,6 +48,10 @@ int main (void)
 	printf("res: %d\n", (int) res);
 	// { dg-output "\nres: 4" }

+	/* The call to cls_double_va_fn is static, so have to use a normal prep_cif */
+	CHECK(ffi_prep_cif(&cif, FFI_DEFAULT_ABI, 2, &ffi_type_sint,
+		arg_types) == FFI_OK);
+
 	CHECK(ffi_prep_closure_loc(pcl, &cif, cls_double_va_fn, NULL, code)
== FFI_OK);

 	res	= ((int(*)(char*, double))(code))(format, doubleArg);
diff --git a/testsuite/libffi.call/cls_longdouble_va.c
b/testsuite/libffi.call/cls_longdouble_va.c
index b33b2b7..95f9ff4 100644
--- a/testsuite/libffi.call/cls_longdouble_va.c
+++ b/testsuite/libffi.call/cls_longdouble_va.c
@@ -6,7 +6,6 @@

 /* { dg-do run { xfail strongarm*-*-* xscale*-*-* } } */
 /* { dg-output "" { xfail avr32*-*-* x86_64-*-mingw* } } */
-/* { dg-skip-if "" arm*-*-* { "-mfloat-abi=hard" } { "" } } */

 #include "ffitest.h"

@@ -36,7 +35,8 @@ int main (void)
 	arg_types[1] = &ffi_type_longdouble;
 	arg_types[2] = NULL;

-	CHECK(ffi_prep_cif(&cif, FFI_DEFAULT_ABI, 2, &ffi_type_sint,
+	/* This printf call is variadic */
+	CHECK(ffi_prep_cif_var(&cif, FFI_DEFAULT_ABI, 1, 2, &ffi_type_sint,
 		arg_types) == FFI_OK);

 	args[0] = &format;
@@ -48,6 +48,10 @@ int main (void)
 	printf("res: %d\n", (int) res);
 	// { dg-output "\nres: 4" }

+	/* The call to cls_longdouble_va_fn is static, so have to use a
normal prep_cif */
+	CHECK(ffi_prep_cif(&cif, FFI_DEFAULT_ABI, 2, &ffi_type_sint,
+		arg_types) == FFI_OK);
+
 	CHECK(ffi_prep_closure_loc(pcl, &cif, cls_longdouble_va_fn, NULL,
code) == FFI_OK);

 	res	= ((int(*)(char*, long double))(code))(format, ldArg);
diff --git a/testsuite/libffi.call/float_va.c b/testsuite/libffi.call/float_va.c
new file mode 100644
index 0000000..4611969
--- /dev/null
+++ b/testsuite/libffi.call/float_va.c
@@ -0,0 +1,107 @@
+/* Area:        fp and variadics
+   Purpose:     check fp inputs and returns work on variadics, even
the fixed params
+   Limitations: None
+   PR:          none
+   Originator:  <david.gilbert@linaro.org> 2011-01-25
+
+   Intended to stress the difference in ABI on ARM vfp
+*/
+
+/* { dg-do run } */
+
+#include <stdarg.h>
+
+#include "ffitest.h"
+
+/* prints out all the parameters, and returns the sum of them all.
+ * 'x' is the number of variadic parameters all of which are double
in this test
+ */
+double float_va_fn(unsigned int x, double y,...)
+{
+  double total=0.0;
+  va_list ap;
+  unsigned int i;
+
+  total+=(double)x;
+  total+=y;
+
+  printf("%u: %.1lf :", x, y);
+
+  va_start(ap, y);
+  for(i=0;i<x;i++)
+  {
+    double arg=va_arg(ap, double);
+    total+=arg;
+    printf(" %d:%.1lf ", i, arg);
+  }
+  va_end(ap);
+
+  printf(" total: %.1lf\n", total);
+
+  return total;
+}
+
+int main (void)
+{
+  ffi_cif    cif;
+
+  ffi_type    *arg_types[5];
+  void        *values[5];
+  double        doubles[5];
+  unsigned int firstarg;
+  double        resfp;
+
+  /* First test, pass float_va_fn(0,2.0) - note there are no actual
+   * variadic parameters, but it's declared variadic so the ABI may be
+   * different. */
+  /* Call it statically and then via ffi */
+  resfp=float_va_fn(0,2.0);
+  // { dg-output "0: 2.0 : total: 2.0" }
+  printf("compiled: %.1lf\n", resfp);
+  // { dg-output "\ncompiled: 2.0" }
+
+  arg_types[0] = &ffi_type_uint;
+  arg_types[1] = &ffi_type_double;
+  arg_types[2] = NULL;
+  CHECK(ffi_prep_cif_var(&cif, FFI_DEFAULT_ABI, 2, 2,
+        &ffi_type_double, arg_types) == FFI_OK);
+
+  firstarg = 0;
+  doubles[0] = 2.0;
+  values[0] = &firstarg;
+  values[1] = &doubles[0];
+  ffi_call(&cif, FFI_FN(float_va_fn), &resfp, values);
+  // { dg-output "\n0: 2.0 : total: 2.0" }
+  printf("ffi: %.1lf\n", resfp);
+  // { dg-output "\nffi: 2.0" }
+
+  /* Second test, float_va_fn(2,2.0,3.0,4.0), now with variadic params */
+  /* Call it statically and then via ffi */
+  resfp=float_va_fn(2,2.0,3.0,4.0);
+  // { dg-output "\n2: 2.0 : 0:3.0  1:4.0  total: 11.0" }
+  printf("compiled: %.1lf\n", resfp);
+  // { dg-output "\ncompiled: 11.0" }
+
+  arg_types[0] = &ffi_type_uint;
+  arg_types[1] = &ffi_type_double;
+  arg_types[2] = &ffi_type_double;
+  arg_types[3] = &ffi_type_double;
+  arg_types[4] = NULL;
+  CHECK(ffi_prep_cif_var(&cif, FFI_DEFAULT_ABI, 2, 4,
+        &ffi_type_double, arg_types) == FFI_OK);
+
+  firstarg = 2;
+  doubles[0] = 2.0;
+  doubles[1] = 3.0;
+  doubles[2] = 4.0;
+  values[0] = &firstarg;
+  values[1] = &doubles[0];
+  values[2] = &doubles[1];
+  values[3] = &doubles[2];
+  ffi_call(&cif, FFI_FN(float_va_fn), &resfp, values);
+  // { dg-output "\n2: 2.0 : 0:3.0  1:4.0  total: 11.0" }
+  printf("ffi: %.1lf\n", resfp);
+  // { dg-output "\nffi: 11.0" }
+
+  exit(0);
+}

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH] Add variadic support
  2011-03-07 18:19                   ` David Gilbert
@ 2011-03-16  9:52                     ` Chung-Lin Tang
  2011-03-16 14:12                       ` Anthony Green
  0 siblings, 1 reply; 22+ messages in thread
From: Chung-Lin Tang @ 2011-03-16  9:52 UTC (permalink / raw)
  To: David Gilbert; +Cc: libffi-discuss, Anthony Green, Marcus.Shawcroft, markos

I think this looks fine.
Anthony, does the patch look okay to you?

Thanks,
Chung-Lin


On 2011/3/8 03:18 AM, David Gilbert wrote:
> Hi,
>   Here is a rework of the previous patch, based on the previous discussion and
> suggestions from Chung-Lin.
> 
> (This is relative libffi git rev cbb062cc35c518004f1ab45c847f8ec4f66069ad)
> 
> Dave
> 
> diff --git a/doc/libffi.texi b/doc/libffi.texi
> index 5cdd667..a7c629a 100644
> --- a/doc/libffi.texi
> +++ b/doc/libffi.texi
> @@ -133,8 +133,6 @@ This initializes @var{cif} according to the given
> parameters.
>  you want.  @ref{Multiple ABIs} for more information.
> 
>  @var{nargs} is the number of arguments that this function accepts.
> -@samp{libffi} does not yet handle varargs functions; see @ref{Missing
> -Features} for more information.
> 
>  @var{rtype} is a pointer to an @code{ffi_type} structure that
>  describes the return type of the function.  @xref{Types}.
> @@ -150,6 +148,28 @@ objects is incorrect; or @code{FFI_BAD_ABI} if
> the @var{abi} parameter
>  is invalid.
>  @end defun
> 
> +If the function being called is variadic (varargs) then @code{ffi_prep_cif_var}
> +must be used instead of @code{ffi_prep_cif}.
> +
> +@findex ffi_prep_cif_var
> +@defun ffi_status ffi_prep_cif_var (ffi_cif *@var{cif}, ffi_abi
> @var{abi}, unsigned int @var{nfixedargs}, unsigned int
> @var{ntotalargs}, ffi_type *@var{rtype}, ffi_type **@var{argtypes})
> +This initializes @var{cif} according to the given parameters for
> +a call to a variadic function.  In general it's operation is the
> +same as for @code{ffi_prep_cif} except that:
> +
> +@var{nfixedargs} is the number of fixed arguments, prior to any
> +variadic arguments.  It must be greater than zero.
> +
> +@var{ntotalargs} the total number of arguments, including variadic
> +and fixed arguments.
> +
> +Note that, different cif's must be prepped for calls to the same
> +function when different numbers of arguments are passed.
> +
> +Also note that a call to @code{ffi_prep_cif_var} with
> @var{nfixedargs}=@var{nototalargs}
> +is NOT equivalent to a call to @code{ffi_prep_cif}.
> +
> +@end defun
> 
>  To call a function using an initialized @code{ffi_cif}, use the
>  @code{ffi_call} function:
> @@ -572,9 +592,7 @@ support for these.
> 
>  @itemize @bullet
>  @item
> -There is no support for calling varargs functions.  This may work on
> -some platforms, depending on how the ABI is defined, but it is not
> -reliable.
> +Variadic closures.
> 
>  @item
>  There is no support for bit fields in structures.
> @@ -591,6 +609,7 @@ The ``raw'' API is undocumented.
>  @c anything else?
>  @end itemize
> 
> +Note that variadic support is very new and tested on a relatively
> small number of platforms.
> 
>  @node Index
>  @unnumbered Index
> diff --git a/include/ffi.h.in b/include/ffi.h.in
> index 96b8fd3..f27f4f3 100644
> --- a/include/ffi.h.in
> +++ b/include/ffi.h.in
> @@ -203,6 +203,15 @@ typedef struct {
>  #endif
>  } ffi_cif;
> 
> +/* Used internally, but overridden by some architectures */
> +ffi_status ffi_prep_cif_core(ffi_cif *cif,
> +			     ffi_abi abi,
> +			     unsigned int isvariadic,
> +			     unsigned int nfixedargs,
> +			     unsigned int ntotalargs,
> +			     ffi_type *rtype,
> +			     ffi_type **atypes);
> +
>  /* ---- Definitions for the raw API -------------------------------------- */
> 
>  #ifndef FFI_SIZEOF_ARG
> @@ -380,6 +389,13 @@ ffi_status ffi_prep_cif(ffi_cif *cif,
>  			ffi_type *rtype,
>  			ffi_type **atypes);
> 
> +ffi_status ffi_prep_cif_var(ffi_cif *cif,
> +			    ffi_abi abi,
> +			    unsigned int nfixedargs,
> +			    unsigned int ntotalargs,
> +			    ffi_type *rtype,
> +			    ffi_type **atypes);
> +
>  void ffi_call(ffi_cif *cif,
>  	      void (*fn)(void),
>  	      void *rvalue,
> diff --git a/include/ffi_common.h b/include/ffi_common.h
> index d953762..919eec2 100644
> --- a/include/ffi_common.h
> +++ b/include/ffi_common.h
> @@ -75,6 +75,8 @@ void ffi_type_test(ffi_type *a, char *file, int line);
> 
>  /* Perform machine dependent cif processing */
>  ffi_status ffi_prep_cif_machdep(ffi_cif *cif);
> +ffi_status ffi_prep_cif_machdep_var(ffi_cif *cif,
> +	 unsigned int nfixedargs, unsigned int ntotalargs);
> 
>  /* Extended cif, used in callback from assembly routine */
>  typedef struct
> diff --git a/man/Makefile.am b/man/Makefile.am
> index 2519277..afcbfb6 100644
> --- a/man/Makefile.am
> +++ b/man/Makefile.am
> @@ -2,7 +2,7 @@
> 
>  AUTOMAKE_OPTIONS=foreign
> 
> -EXTRA_DIST = ffi.3 ffi_call.3 ffi_prep_cif.3
> +EXTRA_DIST = ffi.3 ffi_call.3 ffi_prep_cif.3 ffi_prep_cif_var.3
> 
> -man_MANS = ffi.3 ffi_call.3 ffi_prep_cif.3
> +man_MANS = ffi.3 ffi_call.3 ffi_prep_cif.3 ffi_prep_cif_var.3
> 
> diff --git a/man/ffi.3 b/man/ffi.3
> index 18b5d5d..1f1d303 100644
> --- a/man/ffi.3
> +++ b/man/ffi.3
> @@ -16,6 +16,15 @@ libffi, -lffi
>  .Fa "ffi_type **atypes"
>  .Fc
>  .Ft void
> +.Fo ffi_prep_cif_var
> +.Fa "ffi_cif *cif"
> +.Fa "ffi_abi abi"
> +.Fa "unsigned int nfixedargs"
> +.Fa "unsigned int ntotalargs"
> +.Fa "ffi_type *rtype"
> +.Fa "ffi_type **atypes"
> +.Fc
> +.Ft void
>  .Fo ffi_call
>  .Fa "ffi_cif *cif"
>  .Fa "void (*fn)(void)"
> @@ -28,4 +37,5 @@ generate a call to another function at runtime
> without requiring knowledge of
>  the called function's interface at compile time.
>  .Sh SEE ALSO
>  .Xr ffi_prep_cif 3 ,
> +.Xr ffi_prep_cif_var 3 ,
>  .Xr ffi_call 3
> diff --git a/man/ffi_prep_cif.3 b/man/ffi_prep_cif.3
> index 9436b31..fe66452 100644
> --- a/man/ffi_prep_cif.3
> +++ b/man/ffi_prep_cif.3
> @@ -37,7 +37,9 @@ structs that describe the data type, size and
> alignment of each argument.
>  points to an
>  .Nm ffi_type
>  that describes the data type, size and alignment of the
> -return value.
> +return value. Note that to call a variadic function
> +.Nm ffi_prep_cif_var
> +must be used instead.
>  .Sh RETURN VALUES
>  Upon successful completion,
>  .Nm ffi_prep_cif
> @@ -63,4 +65,5 @@ defined in
>  .
>  .Sh SEE ALSO
>  .Xr ffi 3 ,
> -.Xr ffi_call 3
> +.Xr ffi_call 3 ,
> +.Xr ffi_prep_cif_var 3
> diff --git a/man/ffi_prep_cif_var.3 b/man/ffi_prep_cif_var.3
> new file mode 100644
> index 0000000..1b39c03
> --- /dev/null
> +++ b/man/ffi_prep_cif_var.3
> @@ -0,0 +1,73 @@
> +.Dd January 25, 2011
> +.Dt ffi_prep_cif_var 3
> +.Sh NAME
> +.Nm ffi_prep_cif_var
> +.Nd Prepare a
> +.Nm ffi_cif
> +structure for use with
> +.Nm ffi_call
> +for variadic functions.
> +.Sh SYNOPSIS
> +.In ffi.h
> +.Ft ffi_status
> +.Fo ffi_prep_cif_var
> +.Fa "ffi_cif *cif"
> +.Fa "ffi_abi abi"
> +.Fa "unsigned int nfixedargs"
> +.Fa "unsigned int ntotalargs"
> +.Fa "ffi_type *rtype"
> +.Fa "ffi_type **atypes"
> +.Fc
> +.Sh DESCRIPTION
> +The
> +.Nm ffi_prep_cif_var
> +function prepares a
> +.Nm ffi_cif
> +structure for use with
> +.Nm ffi_call
> +for variadic functions.
> +.Fa abi
> +specifies a set of calling conventions to use.
> +.Fa atypes
> +is an array of
> +.Fa ntotalargs
> +pointers to
> +.Nm ffi_type
> +structs that describe the data type, size and alignment of each argument.
> +.Fa rtype
> +points to an
> +.Nm ffi_type
> +that describes the data type, size and alignment of the
> +return value.
> +.Fa nfixedargs
> +must contain the number of fixed (non-variadic) arguments.
> +Note that to call a non-variadic function
> +.Nm ffi_prep_cif
> +must be used.
> +.Sh RETURN VALUES
> +Upon successful completion,
> +.Nm ffi_prep_cif_var
> +returns
> +.Nm FFI_OK .
> +It will return
> +.Nm FFI_BAD_TYPEDEF
> +if
> +.Fa cif
> +is
> +.Nm NULL
> +or
> +.Fa atypes
> +or
> +.Fa rtype
> +is malformed. If
> +.Fa abi
> +does not refer to a valid ABI,
> +.Nm FFI_BAD_ABI
> +will be returned. Available ABIs are
> +defined in
> +.Nm <ffitarget.h>
> +.
> +.Sh SEE ALSO
> +.Xr ffi 3 ,
> +.Xr ffi_call 3 ,
> +.Xr ffi_prep_cif 3
> diff --git a/src/arm/ffi.c b/src/arm/ffi.c
> index 885a9cb..c226dbd 100644
> --- a/src/arm/ffi.c
> +++ b/src/arm/ffi.c
> @@ -189,6 +189,18 @@ ffi_status ffi_prep_cif_machdep(ffi_cif *cif)
>    return FFI_OK;
>  }
> 
> +/* Perform machine dependent cif processing for variadic calls */
> +ffi_status ffi_prep_cif_machdep_var(ffi_cif *cif,
> +				    unsigned int nfixedargs,
> +				    unsigned int ntotalargs)
> +{
> +  /* VFP variadic calls actually use the SYSV ABI */
> +  if (cif->abi == FFI_VFP)
> +	cif->abi = FFI_SYSV;
> +
> +  return ffi_prep_cif_machdep(cif);
> +}
> +
>  /* Prototypes for assembly functions, in sysv.S */
>  extern void ffi_call_SYSV (void (*fn)(void), extended_cif *,
> unsigned, unsigned, unsigned *);
>  extern void ffi_call_VFP (void (*fn)(void), extended_cif *, unsigned,
> unsigned, unsigned *);
> diff --git a/src/arm/ffitarget.h b/src/arm/ffitarget.h
> index ce25b23..b9a0428 100644
> --- a/src/arm/ffitarget.h
> +++ b/src/arm/ffitarget.h
> @@ -55,6 +55,8 @@ typedef enum ffi_abi {
>  #define FFI_TYPE_STRUCT_VFP_FLOAT  (FFI_TYPE_LAST + 1)
>  #define FFI_TYPE_STRUCT_VFP_DOUBLE (FFI_TYPE_LAST + 2)
> 
> +#define FFI_TARGET_SPECIFIC_VARIADIC
> +
>  /* ---- Definitions for closures ----------------------------------------- */
> 
>  #define FFI_CLOSURES 1
> @@ -62,4 +64,3 @@ typedef enum ffi_abi {
>  #define FFI_NATIVE_RAW_API 0
> 
>  #endif
> -
> diff --git a/src/cris/ffi.c b/src/cris/ffi.c
> index f25d7b4..aaca5b1 100644
> --- a/src/cris/ffi.c
> +++ b/src/cris/ffi.c
> @@ -153,21 +153,24 @@ ffi_prep_args (char *stack, extended_cif * ecif)
>    return (struct_count);
>  }
> 
> -ffi_status
> -ffi_prep_cif (ffi_cif * cif,
> -	      ffi_abi abi, unsigned int nargs,
> -	      ffi_type * rtype, ffi_type ** atypes)
> +ffi_status FFI_HIDDEN
> +ffi_prep_cif_core (ffi_cif * cif,
> +	           ffi_abi abi, unsigned int isvariadic,
> +		   unsigned int nfixedargs, unsigned int ntotalargs,
> +	           ffi_type * rtype, ffi_type ** atypes)
>  {
>    unsigned bytes = 0;
>    unsigned int i;
>    ffi_type **ptr;
> 
>    FFI_ASSERT (cif != NULL);
> +  FFI_ASSERT((!isvariadic) || (nfixedargs >= 1));
> +  FFI_ASSERT(nfixedargs <= ntotalargs);
>    FFI_ASSERT (abi > FFI_FIRST_ABI && abi < FFI_LAST_ABI);
> 
>    cif->abi = abi;
>    cif->arg_types = atypes;
> -  cif->nargs = nargs;
> +  cif->nargs = ntotalargs;
>    cif->rtype = rtype;
> 
>    cif->flags = 0;
> diff --git a/src/prep_cif.c b/src/prep_cif.c
> index 8548cfd..0e2e116 100644
> --- a/src/prep_cif.c
> +++ b/src/prep_cif.c
> @@ -90,20 +90,33 @@ static ffi_status initialize_aggregate(ffi_type *arg)
>  /* Perform machine independent ffi_cif preparation, then call
>     machine dependent routine. */
> 
> -ffi_status ffi_prep_cif(ffi_cif *cif, ffi_abi abi, unsigned int nargs,
> -			ffi_type *rtype, ffi_type **atypes)
> +/* For non variadic functions isvariadic should be 0 and
> +   nfixedargs==ntotalargs.
> +
> +   For variadic calls, isvariadic should be 1 and nfixedargs
> +   and ntotalargs set as appropriate. nfixedargs must always be >=1 */
> +
> +
> +ffi_status FFI_HIDDEN ffi_prep_cif_core(ffi_cif *cif, ffi_abi abi,
> +			     unsigned int isvariadic,
> +                             unsigned int nfixedargs,
> +                             unsigned int ntotalargs,
> +			     ffi_type *rtype, ffi_type **atypes)
>  {
>    unsigned bytes = 0;
>    unsigned int i;
>    ffi_type **ptr;
> 
>    FFI_ASSERT(cif != NULL);
> +  FFI_ASSERT((!isvariadic) || (nfixedargs >= 1));
> +  FFI_ASSERT(nfixedargs <= ntotalargs);
> +
>    if (! (abi > FFI_FIRST_ABI && abi < FFI_LAST_ABI))
>      return FFI_BAD_ABI;
> 
>    cif->abi = abi;
>    cif->arg_types = atypes;
> -  cif->nargs = nargs;
> +  cif->nargs = ntotalargs;
>    cif->rtype = rtype;
> 
>    cif->flags = 0;
> @@ -159,10 +172,31 @@ ffi_status ffi_prep_cif(ffi_cif *cif, ffi_abi
> abi, unsigned int nargs,
>    cif->bytes = bytes;
> 
>    /* Perform machine dependent cif processing */
> +#ifdef FFI_TARGET_SPECIFIC_VARIADIC
> +  if (isvariadic)
> +	return ffi_prep_cif_machdep_var(cif, nfixedargs, ntotalargs);
> +#endif
> +
>    return ffi_prep_cif_machdep(cif);
>  }
>  #endif /* not __CRIS__ */
> 
> +ffi_status ffi_prep_cif(ffi_cif *cif, ffi_abi abi, unsigned int nargs,
> +			     ffi_type *rtype, ffi_type **atypes)
> +{
> +  return ffi_prep_cif_core(cif, abi, 0, nargs, nargs, rtype, atypes);
> +}
> +
> +ffi_status ffi_prep_cif_var(ffi_cif *cif,
> +                            ffi_abi abi,
> +                            unsigned int nfixedargs,
> +                            unsigned int ntotalargs,
> +                            ffi_type *rtype,
> +                            ffi_type **atypes)
> +{
> +  return ffi_prep_cif_core(cif, abi, 1, nfixedargs, ntotalargs, rtype, atypes);
> +}
> +
>  #if FFI_CLOSURES
> 
>  ffi_status
> diff --git a/testsuite/libffi.call/cls_double_va.c
> b/testsuite/libffi.call/cls_double_va.c
> index 62bebbd..1a2706e 100644
> --- a/testsuite/libffi.call/cls_double_va.c
> +++ b/testsuite/libffi.call/cls_double_va.c
> @@ -6,7 +6,6 @@
> 
>  /* { dg-do run { xfail strongarm*-*-* xscale*-*-* } } */
>  /* { dg-output "" { xfail avr32*-*-* } } */
> -/* { dg-skip-if "" arm*-*-* { "-mfloat-abi=hard" } { "" } } */
> 
>  #include "ffitest.h"
> 
> @@ -36,7 +35,8 @@ int main (void)
>  	arg_types[1] = &ffi_type_double;
>  	arg_types[2] = NULL;
> 
> -	CHECK(ffi_prep_cif(&cif, FFI_DEFAULT_ABI, 2, &ffi_type_sint,
> +	/* This printf call is variadic */
> +	CHECK(ffi_prep_cif_var(&cif, FFI_DEFAULT_ABI, 1, 2, &ffi_type_sint,
>  		arg_types) == FFI_OK);
> 
>  	args[0] = &format;
> @@ -48,6 +48,10 @@ int main (void)
>  	printf("res: %d\n", (int) res);
>  	// { dg-output "\nres: 4" }
> 
> +	/* The call to cls_double_va_fn is static, so have to use a normal prep_cif */
> +	CHECK(ffi_prep_cif(&cif, FFI_DEFAULT_ABI, 2, &ffi_type_sint,
> +		arg_types) == FFI_OK);
> +
>  	CHECK(ffi_prep_closure_loc(pcl, &cif, cls_double_va_fn, NULL, code)
> == FFI_OK);
> 
>  	res	= ((int(*)(char*, double))(code))(format, doubleArg);
> diff --git a/testsuite/libffi.call/cls_longdouble_va.c
> b/testsuite/libffi.call/cls_longdouble_va.c
> index b33b2b7..95f9ff4 100644
> --- a/testsuite/libffi.call/cls_longdouble_va.c
> +++ b/testsuite/libffi.call/cls_longdouble_va.c
> @@ -6,7 +6,6 @@
> 
>  /* { dg-do run { xfail strongarm*-*-* xscale*-*-* } } */
>  /* { dg-output "" { xfail avr32*-*-* x86_64-*-mingw* } } */
> -/* { dg-skip-if "" arm*-*-* { "-mfloat-abi=hard" } { "" } } */
> 
>  #include "ffitest.h"
> 
> @@ -36,7 +35,8 @@ int main (void)
>  	arg_types[1] = &ffi_type_longdouble;
>  	arg_types[2] = NULL;
> 
> -	CHECK(ffi_prep_cif(&cif, FFI_DEFAULT_ABI, 2, &ffi_type_sint,
> +	/* This printf call is variadic */
> +	CHECK(ffi_prep_cif_var(&cif, FFI_DEFAULT_ABI, 1, 2, &ffi_type_sint,
>  		arg_types) == FFI_OK);
> 
>  	args[0] = &format;
> @@ -48,6 +48,10 @@ int main (void)
>  	printf("res: %d\n", (int) res);
>  	// { dg-output "\nres: 4" }
> 
> +	/* The call to cls_longdouble_va_fn is static, so have to use a
> normal prep_cif */
> +	CHECK(ffi_prep_cif(&cif, FFI_DEFAULT_ABI, 2, &ffi_type_sint,
> +		arg_types) == FFI_OK);
> +
>  	CHECK(ffi_prep_closure_loc(pcl, &cif, cls_longdouble_va_fn, NULL,
> code) == FFI_OK);
> 
>  	res	= ((int(*)(char*, long double))(code))(format, ldArg);
> diff --git a/testsuite/libffi.call/float_va.c b/testsuite/libffi.call/float_va.c
> new file mode 100644
> index 0000000..4611969
> --- /dev/null
> +++ b/testsuite/libffi.call/float_va.c
> @@ -0,0 +1,107 @@
> +/* Area:        fp and variadics
> +   Purpose:     check fp inputs and returns work on variadics, even
> the fixed params
> +   Limitations: None
> +   PR:          none
> +   Originator:  <david.gilbert@linaro.org> 2011-01-25
> +
> +   Intended to stress the difference in ABI on ARM vfp
> +*/
> +
> +/* { dg-do run } */
> +
> +#include <stdarg.h>
> +
> +#include "ffitest.h"
> +
> +/* prints out all the parameters, and returns the sum of them all.
> + * 'x' is the number of variadic parameters all of which are double
> in this test
> + */
> +double float_va_fn(unsigned int x, double y,...)
> +{
> +  double total=0.0;
> +  va_list ap;
> +  unsigned int i;
> +
> +  total+=(double)x;
> +  total+=y;
> +
> +  printf("%u: %.1lf :", x, y);
> +
> +  va_start(ap, y);
> +  for(i=0;i<x;i++)
> +  {
> +    double arg=va_arg(ap, double);
> +    total+=arg;
> +    printf(" %d:%.1lf ", i, arg);
> +  }
> +  va_end(ap);
> +
> +  printf(" total: %.1lf\n", total);
> +
> +  return total;
> +}
> +
> +int main (void)
> +{
> +  ffi_cif    cif;
> +
> +  ffi_type    *arg_types[5];
> +  void        *values[5];
> +  double        doubles[5];
> +  unsigned int firstarg;
> +  double        resfp;
> +
> +  /* First test, pass float_va_fn(0,2.0) - note there are no actual
> +   * variadic parameters, but it's declared variadic so the ABI may be
> +   * different. */
> +  /* Call it statically and then via ffi */
> +  resfp=float_va_fn(0,2.0);
> +  // { dg-output "0: 2.0 : total: 2.0" }
> +  printf("compiled: %.1lf\n", resfp);
> +  // { dg-output "\ncompiled: 2.0" }
> +
> +  arg_types[0] = &ffi_type_uint;
> +  arg_types[1] = &ffi_type_double;
> +  arg_types[2] = NULL;
> +  CHECK(ffi_prep_cif_var(&cif, FFI_DEFAULT_ABI, 2, 2,
> +        &ffi_type_double, arg_types) == FFI_OK);
> +
> +  firstarg = 0;
> +  doubles[0] = 2.0;
> +  values[0] = &firstarg;
> +  values[1] = &doubles[0];
> +  ffi_call(&cif, FFI_FN(float_va_fn), &resfp, values);
> +  // { dg-output "\n0: 2.0 : total: 2.0" }
> +  printf("ffi: %.1lf\n", resfp);
> +  // { dg-output "\nffi: 2.0" }
> +
> +  /* Second test, float_va_fn(2,2.0,3.0,4.0), now with variadic params */
> +  /* Call it statically and then via ffi */
> +  resfp=float_va_fn(2,2.0,3.0,4.0);
> +  // { dg-output "\n2: 2.0 : 0:3.0  1:4.0  total: 11.0" }
> +  printf("compiled: %.1lf\n", resfp);
> +  // { dg-output "\ncompiled: 11.0" }
> +
> +  arg_types[0] = &ffi_type_uint;
> +  arg_types[1] = &ffi_type_double;
> +  arg_types[2] = &ffi_type_double;
> +  arg_types[3] = &ffi_type_double;
> +  arg_types[4] = NULL;
> +  CHECK(ffi_prep_cif_var(&cif, FFI_DEFAULT_ABI, 2, 4,
> +        &ffi_type_double, arg_types) == FFI_OK);
> +
> +  firstarg = 2;
> +  doubles[0] = 2.0;
> +  doubles[1] = 3.0;
> +  doubles[2] = 4.0;
> +  values[0] = &firstarg;
> +  values[1] = &doubles[0];
> +  values[2] = &doubles[1];
> +  values[3] = &doubles[2];
> +  ffi_call(&cif, FFI_FN(float_va_fn), &resfp, values);
> +  // { dg-output "\n2: 2.0 : 0:3.0  1:4.0  total: 11.0" }
> +  printf("ffi: %.1lf\n", resfp);
> +  // { dg-output "\nffi: 11.0" }
> +
> +  exit(0);
> +}

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH] Add variadic support
  2011-03-16  9:52                     ` Chung-Lin Tang
@ 2011-03-16 14:12                       ` Anthony Green
  2011-03-16 14:25                         ` David Gilbert
  0 siblings, 1 reply; 22+ messages in thread
From: Anthony Green @ 2011-03-16 14:12 UTC (permalink / raw)
  To: Chung-Lin Tang; +Cc: David Gilbert, libffi-discuss, Marcus.Shawcroft, markos

Chung-Lin Tang <cltang@linaro.org> writes:

> I think this looks fine.
> Anthony, does the patch look okay to you?

Yes, I think it's fine.  I was against adding a new API at first, but
after talking to David about it on #libffi I was convinced that there's
no reasonable alternative.  I'm going to check it into a branch on git
until 3.0.10 is done.  The branch will be called "variadic".

Thanks for you work David!

AG


>
> Thanks,
> Chung-Lin
>
>
> On 2011/3/8 03:18 AM, David Gilbert wrote:
>> Hi,
>>   Here is a rework of the previous patch, based on the previous discussion and
>> suggestions from Chung-Lin.
>> 
>> (This is relative libffi git rev cbb062cc35c518004f1ab45c847f8ec4f66069ad)
>> 
>> Dave
>> 
>> diff --git a/doc/libffi.texi b/doc/libffi.texi
>> index 5cdd667..a7c629a 100644
>> --- a/doc/libffi.texi
>> +++ b/doc/libffi.texi
>> @@ -133,8 +133,6 @@ This initializes @var{cif} according to the given
>> parameters.
>>  you want.  @ref{Multiple ABIs} for more information.
>> 
>>  @var{nargs} is the number of arguments that this function accepts.
>> -@samp{libffi} does not yet handle varargs functions; see @ref{Missing
>> -Features} for more information.
>> 
>>  @var{rtype} is a pointer to an @code{ffi_type} structure that
>>  describes the return type of the function.  @xref{Types}.
>> @@ -150,6 +148,28 @@ objects is incorrect; or @code{FFI_BAD_ABI} if
>> the @var{abi} parameter
>>  is invalid.
>>  @end defun
>> 
>> +If the function being called is variadic (varargs) then @code{ffi_prep_cif_var}
>> +must be used instead of @code{ffi_prep_cif}.
>> +
>> +@findex ffi_prep_cif_var
>> +@defun ffi_status ffi_prep_cif_var (ffi_cif *@var{cif}, ffi_abi
>> @var{abi}, unsigned int @var{nfixedargs}, unsigned int
>> @var{ntotalargs}, ffi_type *@var{rtype}, ffi_type **@var{argtypes})
>> +This initializes @var{cif} according to the given parameters for
>> +a call to a variadic function.  In general it's operation is the
>> +same as for @code{ffi_prep_cif} except that:
>> +
>> +@var{nfixedargs} is the number of fixed arguments, prior to any
>> +variadic arguments.  It must be greater than zero.
>> +
>> +@var{ntotalargs} the total number of arguments, including variadic
>> +and fixed arguments.
>> +
>> +Note that, different cif's must be prepped for calls to the same
>> +function when different numbers of arguments are passed.
>> +
>> +Also note that a call to @code{ffi_prep_cif_var} with
>> @var{nfixedargs}=@var{nototalargs}
>> +is NOT equivalent to a call to @code{ffi_prep_cif}.
>> +
>> +@end defun
>> 
>>  To call a function using an initialized @code{ffi_cif}, use the
>>  @code{ffi_call} function:
>> @@ -572,9 +592,7 @@ support for these.
>> 
>>  @itemize @bullet
>>  @item
>> -There is no support for calling varargs functions.  This may work on
>> -some platforms, depending on how the ABI is defined, but it is not
>> -reliable.
>> +Variadic closures.
>> 
>>  @item
>>  There is no support for bit fields in structures.
>> @@ -591,6 +609,7 @@ The ``raw'' API is undocumented.
>>  @c anything else?
>>  @end itemize
>> 
>> +Note that variadic support is very new and tested on a relatively
>> small number of platforms.
>> 
>>  @node Index
>>  @unnumbered Index
>> diff --git a/include/ffi.h.in b/include/ffi.h.in
>> index 96b8fd3..f27f4f3 100644
>> --- a/include/ffi.h.in
>> +++ b/include/ffi.h.in
>> @@ -203,6 +203,15 @@ typedef struct {
>>  #endif
>>  } ffi_cif;
>> 
>> +/* Used internally, but overridden by some architectures */
>> +ffi_status ffi_prep_cif_core(ffi_cif *cif,
>> +			     ffi_abi abi,
>> +			     unsigned int isvariadic,
>> +			     unsigned int nfixedargs,
>> +			     unsigned int ntotalargs,
>> +			     ffi_type *rtype,
>> +			     ffi_type **atypes);
>> +
>>  /* ---- Definitions for the raw API -------------------------------------- */
>> 
>>  #ifndef FFI_SIZEOF_ARG
>> @@ -380,6 +389,13 @@ ffi_status ffi_prep_cif(ffi_cif *cif,
>>  			ffi_type *rtype,
>>  			ffi_type **atypes);
>> 
>> +ffi_status ffi_prep_cif_var(ffi_cif *cif,
>> +			    ffi_abi abi,
>> +			    unsigned int nfixedargs,
>> +			    unsigned int ntotalargs,
>> +			    ffi_type *rtype,
>> +			    ffi_type **atypes);
>> +
>>  void ffi_call(ffi_cif *cif,
>>  	      void (*fn)(void),
>>  	      void *rvalue,
>> diff --git a/include/ffi_common.h b/include/ffi_common.h
>> index d953762..919eec2 100644
>> --- a/include/ffi_common.h
>> +++ b/include/ffi_common.h
>> @@ -75,6 +75,8 @@ void ffi_type_test(ffi_type *a, char *file, int line);
>> 
>>  /* Perform machine dependent cif processing */
>>  ffi_status ffi_prep_cif_machdep(ffi_cif *cif);
>> +ffi_status ffi_prep_cif_machdep_var(ffi_cif *cif,
>> +	 unsigned int nfixedargs, unsigned int ntotalargs);
>> 
>>  /* Extended cif, used in callback from assembly routine */
>>  typedef struct
>> diff --git a/man/Makefile.am b/man/Makefile.am
>> index 2519277..afcbfb6 100644
>> --- a/man/Makefile.am
>> +++ b/man/Makefile.am
>> @@ -2,7 +2,7 @@
>> 
>>  AUTOMAKE_OPTIONS=foreign
>> 
>> -EXTRA_DIST = ffi.3 ffi_call.3 ffi_prep_cif.3
>> +EXTRA_DIST = ffi.3 ffi_call.3 ffi_prep_cif.3 ffi_prep_cif_var.3
>> 
>> -man_MANS = ffi.3 ffi_call.3 ffi_prep_cif.3
>> +man_MANS = ffi.3 ffi_call.3 ffi_prep_cif.3 ffi_prep_cif_var.3
>> 
>> diff --git a/man/ffi.3 b/man/ffi.3
>> index 18b5d5d..1f1d303 100644
>> --- a/man/ffi.3
>> +++ b/man/ffi.3
>> @@ -16,6 +16,15 @@ libffi, -lffi
>>  .Fa "ffi_type **atypes"
>>  .Fc
>>  .Ft void
>> +.Fo ffi_prep_cif_var
>> +.Fa "ffi_cif *cif"
>> +.Fa "ffi_abi abi"
>> +.Fa "unsigned int nfixedargs"
>> +.Fa "unsigned int ntotalargs"
>> +.Fa "ffi_type *rtype"
>> +.Fa "ffi_type **atypes"
>> +.Fc
>> +.Ft void
>>  .Fo ffi_call
>>  .Fa "ffi_cif *cif"
>>  .Fa "void (*fn)(void)"
>> @@ -28,4 +37,5 @@ generate a call to another function at runtime
>> without requiring knowledge of
>>  the called function's interface at compile time.
>>  .Sh SEE ALSO
>>  .Xr ffi_prep_cif 3 ,
>> +.Xr ffi_prep_cif_var 3 ,
>>  .Xr ffi_call 3
>> diff --git a/man/ffi_prep_cif.3 b/man/ffi_prep_cif.3
>> index 9436b31..fe66452 100644
>> --- a/man/ffi_prep_cif.3
>> +++ b/man/ffi_prep_cif.3
>> @@ -37,7 +37,9 @@ structs that describe the data type, size and
>> alignment of each argument.
>>  points to an
>>  .Nm ffi_type
>>  that describes the data type, size and alignment of the
>> -return value.
>> +return value. Note that to call a variadic function
>> +.Nm ffi_prep_cif_var
>> +must be used instead.
>>  .Sh RETURN VALUES
>>  Upon successful completion,
>>  .Nm ffi_prep_cif
>> @@ -63,4 +65,5 @@ defined in
>>  .
>>  .Sh SEE ALSO
>>  .Xr ffi 3 ,
>> -.Xr ffi_call 3
>> +.Xr ffi_call 3 ,
>> +.Xr ffi_prep_cif_var 3
>> diff --git a/man/ffi_prep_cif_var.3 b/man/ffi_prep_cif_var.3
>> new file mode 100644
>> index 0000000..1b39c03
>> --- /dev/null
>> +++ b/man/ffi_prep_cif_var.3
>> @@ -0,0 +1,73 @@
>> +.Dd January 25, 2011
>> +.Dt ffi_prep_cif_var 3
>> +.Sh NAME
>> +.Nm ffi_prep_cif_var
>> +.Nd Prepare a
>> +.Nm ffi_cif
>> +structure for use with
>> +.Nm ffi_call
>> +for variadic functions.
>> +.Sh SYNOPSIS
>> +.In ffi.h
>> +.Ft ffi_status
>> +.Fo ffi_prep_cif_var
>> +.Fa "ffi_cif *cif"
>> +.Fa "ffi_abi abi"
>> +.Fa "unsigned int nfixedargs"
>> +.Fa "unsigned int ntotalargs"
>> +.Fa "ffi_type *rtype"
>> +.Fa "ffi_type **atypes"
>> +.Fc
>> +.Sh DESCRIPTION
>> +The
>> +.Nm ffi_prep_cif_var
>> +function prepares a
>> +.Nm ffi_cif
>> +structure for use with
>> +.Nm ffi_call
>> +for variadic functions.
>> +.Fa abi
>> +specifies a set of calling conventions to use.
>> +.Fa atypes
>> +is an array of
>> +.Fa ntotalargs
>> +pointers to
>> +.Nm ffi_type
>> +structs that describe the data type, size and alignment of each argument.
>> +.Fa rtype
>> +points to an
>> +.Nm ffi_type
>> +that describes the data type, size and alignment of the
>> +return value.
>> +.Fa nfixedargs
>> +must contain the number of fixed (non-variadic) arguments.
>> +Note that to call a non-variadic function
>> +.Nm ffi_prep_cif
>> +must be used.
>> +.Sh RETURN VALUES
>> +Upon successful completion,
>> +.Nm ffi_prep_cif_var
>> +returns
>> +.Nm FFI_OK .
>> +It will return
>> +.Nm FFI_BAD_TYPEDEF
>> +if
>> +.Fa cif
>> +is
>> +.Nm NULL
>> +or
>> +.Fa atypes
>> +or
>> +.Fa rtype
>> +is malformed. If
>> +.Fa abi
>> +does not refer to a valid ABI,
>> +.Nm FFI_BAD_ABI
>> +will be returned. Available ABIs are
>> +defined in
>> +.Nm <ffitarget.h>
>> +.
>> +.Sh SEE ALSO
>> +.Xr ffi 3 ,
>> +.Xr ffi_call 3 ,
>> +.Xr ffi_prep_cif 3
>> diff --git a/src/arm/ffi.c b/src/arm/ffi.c
>> index 885a9cb..c226dbd 100644
>> --- a/src/arm/ffi.c
>> +++ b/src/arm/ffi.c
>> @@ -189,6 +189,18 @@ ffi_status ffi_prep_cif_machdep(ffi_cif *cif)
>>    return FFI_OK;
>>  }
>> 
>> +/* Perform machine dependent cif processing for variadic calls */
>> +ffi_status ffi_prep_cif_machdep_var(ffi_cif *cif,
>> +				    unsigned int nfixedargs,
>> +				    unsigned int ntotalargs)
>> +{
>> +  /* VFP variadic calls actually use the SYSV ABI */
>> +  if (cif->abi == FFI_VFP)
>> +	cif->abi = FFI_SYSV;
>> +
>> +  return ffi_prep_cif_machdep(cif);
>> +}
>> +
>>  /* Prototypes for assembly functions, in sysv.S */
>>  extern void ffi_call_SYSV (void (*fn)(void), extended_cif *,
>> unsigned, unsigned, unsigned *);
>>  extern void ffi_call_VFP (void (*fn)(void), extended_cif *, unsigned,
>> unsigned, unsigned *);
>> diff --git a/src/arm/ffitarget.h b/src/arm/ffitarget.h
>> index ce25b23..b9a0428 100644
>> --- a/src/arm/ffitarget.h
>> +++ b/src/arm/ffitarget.h
>> @@ -55,6 +55,8 @@ typedef enum ffi_abi {
>>  #define FFI_TYPE_STRUCT_VFP_FLOAT  (FFI_TYPE_LAST + 1)
>>  #define FFI_TYPE_STRUCT_VFP_DOUBLE (FFI_TYPE_LAST + 2)
>> 
>> +#define FFI_TARGET_SPECIFIC_VARIADIC
>> +
>>  /* ---- Definitions for closures ----------------------------------------- */
>> 
>>  #define FFI_CLOSURES 1
>> @@ -62,4 +64,3 @@ typedef enum ffi_abi {
>>  #define FFI_NATIVE_RAW_API 0
>> 
>>  #endif
>> -
>> diff --git a/src/cris/ffi.c b/src/cris/ffi.c
>> index f25d7b4..aaca5b1 100644
>> --- a/src/cris/ffi.c
>> +++ b/src/cris/ffi.c
>> @@ -153,21 +153,24 @@ ffi_prep_args (char *stack, extended_cif * ecif)
>>    return (struct_count);
>>  }
>> 
>> -ffi_status
>> -ffi_prep_cif (ffi_cif * cif,
>> -	      ffi_abi abi, unsigned int nargs,
>> -	      ffi_type * rtype, ffi_type ** atypes)
>> +ffi_status FFI_HIDDEN
>> +ffi_prep_cif_core (ffi_cif * cif,
>> +	           ffi_abi abi, unsigned int isvariadic,
>> +		   unsigned int nfixedargs, unsigned int ntotalargs,
>> +	           ffi_type * rtype, ffi_type ** atypes)
>>  {
>>    unsigned bytes = 0;
>>    unsigned int i;
>>    ffi_type **ptr;
>> 
>>    FFI_ASSERT (cif != NULL);
>> +  FFI_ASSERT((!isvariadic) || (nfixedargs >= 1));
>> +  FFI_ASSERT(nfixedargs <= ntotalargs);
>>    FFI_ASSERT (abi > FFI_FIRST_ABI && abi < FFI_LAST_ABI);
>> 
>>    cif->abi = abi;
>>    cif->arg_types = atypes;
>> -  cif->nargs = nargs;
>> +  cif->nargs = ntotalargs;
>>    cif->rtype = rtype;
>> 
>>    cif->flags = 0;
>> diff --git a/src/prep_cif.c b/src/prep_cif.c
>> index 8548cfd..0e2e116 100644
>> --- a/src/prep_cif.c
>> +++ b/src/prep_cif.c
>> @@ -90,20 +90,33 @@ static ffi_status initialize_aggregate(ffi_type *arg)
>>  /* Perform machine independent ffi_cif preparation, then call
>>     machine dependent routine. */
>> 
>> -ffi_status ffi_prep_cif(ffi_cif *cif, ffi_abi abi, unsigned int nargs,
>> -			ffi_type *rtype, ffi_type **atypes)
>> +/* For non variadic functions isvariadic should be 0 and
>> +   nfixedargs==ntotalargs.
>> +
>> +   For variadic calls, isvariadic should be 1 and nfixedargs
>> +   and ntotalargs set as appropriate. nfixedargs must always be >=1 */
>> +
>> +
>> +ffi_status FFI_HIDDEN ffi_prep_cif_core(ffi_cif *cif, ffi_abi abi,
>> +			     unsigned int isvariadic,
>> +                             unsigned int nfixedargs,
>> +                             unsigned int ntotalargs,
>> +			     ffi_type *rtype, ffi_type **atypes)
>>  {
>>    unsigned bytes = 0;
>>    unsigned int i;
>>    ffi_type **ptr;
>> 
>>    FFI_ASSERT(cif != NULL);
>> +  FFI_ASSERT((!isvariadic) || (nfixedargs >= 1));
>> +  FFI_ASSERT(nfixedargs <= ntotalargs);
>> +
>>    if (! (abi > FFI_FIRST_ABI && abi < FFI_LAST_ABI))
>>      return FFI_BAD_ABI;
>> 
>>    cif->abi = abi;
>>    cif->arg_types = atypes;
>> -  cif->nargs = nargs;
>> +  cif->nargs = ntotalargs;
>>    cif->rtype = rtype;
>> 
>>    cif->flags = 0;
>> @@ -159,10 +172,31 @@ ffi_status ffi_prep_cif(ffi_cif *cif, ffi_abi
>> abi, unsigned int nargs,
>>    cif->bytes = bytes;
>> 
>>    /* Perform machine dependent cif processing */
>> +#ifdef FFI_TARGET_SPECIFIC_VARIADIC
>> +  if (isvariadic)
>> +	return ffi_prep_cif_machdep_var(cif, nfixedargs, ntotalargs);
>> +#endif
>> +
>>    return ffi_prep_cif_machdep(cif);
>>  }
>>  #endif /* not __CRIS__ */
>> 
>> +ffi_status ffi_prep_cif(ffi_cif *cif, ffi_abi abi, unsigned int nargs,
>> +			     ffi_type *rtype, ffi_type **atypes)
>> +{
>> +  return ffi_prep_cif_core(cif, abi, 0, nargs, nargs, rtype, atypes);
>> +}
>> +
>> +ffi_status ffi_prep_cif_var(ffi_cif *cif,
>> +                            ffi_abi abi,
>> +                            unsigned int nfixedargs,
>> +                            unsigned int ntotalargs,
>> +                            ffi_type *rtype,
>> +                            ffi_type **atypes)
>> +{
>> +  return ffi_prep_cif_core(cif, abi, 1, nfixedargs, ntotalargs, rtype, atypes);
>> +}
>> +
>>  #if FFI_CLOSURES
>> 
>>  ffi_status
>> diff --git a/testsuite/libffi.call/cls_double_va.c
>> b/testsuite/libffi.call/cls_double_va.c
>> index 62bebbd..1a2706e 100644
>> --- a/testsuite/libffi.call/cls_double_va.c
>> +++ b/testsuite/libffi.call/cls_double_va.c
>> @@ -6,7 +6,6 @@
>> 
>>  /* { dg-do run { xfail strongarm*-*-* xscale*-*-* } } */
>>  /* { dg-output "" { xfail avr32*-*-* } } */
>> -/* { dg-skip-if "" arm*-*-* { "-mfloat-abi=hard" } { "" } } */
>> 
>>  #include "ffitest.h"
>> 
>> @@ -36,7 +35,8 @@ int main (void)
>>  	arg_types[1] = &ffi_type_double;
>>  	arg_types[2] = NULL;
>> 
>> -	CHECK(ffi_prep_cif(&cif, FFI_DEFAULT_ABI, 2, &ffi_type_sint,
>> +	/* This printf call is variadic */
>> +	CHECK(ffi_prep_cif_var(&cif, FFI_DEFAULT_ABI, 1, 2, &ffi_type_sint,
>>  		arg_types) == FFI_OK);
>> 
>>  	args[0] = &format;
>> @@ -48,6 +48,10 @@ int main (void)
>>  	printf("res: %d\n", (int) res);
>>  	// { dg-output "\nres: 4" }
>> 
>> +	/* The call to cls_double_va_fn is static, so have to use a normal prep_cif */
>> +	CHECK(ffi_prep_cif(&cif, FFI_DEFAULT_ABI, 2, &ffi_type_sint,
>> +		arg_types) == FFI_OK);
>> +
>>  	CHECK(ffi_prep_closure_loc(pcl, &cif, cls_double_va_fn, NULL, code)
>> == FFI_OK);
>> 
>>  	res	= ((int(*)(char*, double))(code))(format, doubleArg);
>> diff --git a/testsuite/libffi.call/cls_longdouble_va.c
>> b/testsuite/libffi.call/cls_longdouble_va.c
>> index b33b2b7..95f9ff4 100644
>> --- a/testsuite/libffi.call/cls_longdouble_va.c
>> +++ b/testsuite/libffi.call/cls_longdouble_va.c
>> @@ -6,7 +6,6 @@
>> 
>>  /* { dg-do run { xfail strongarm*-*-* xscale*-*-* } } */
>>  /* { dg-output "" { xfail avr32*-*-* x86_64-*-mingw* } } */
>> -/* { dg-skip-if "" arm*-*-* { "-mfloat-abi=hard" } { "" } } */
>> 
>>  #include "ffitest.h"
>> 
>> @@ -36,7 +35,8 @@ int main (void)
>>  	arg_types[1] = &ffi_type_longdouble;
>>  	arg_types[2] = NULL;
>> 
>> -	CHECK(ffi_prep_cif(&cif, FFI_DEFAULT_ABI, 2, &ffi_type_sint,
>> +	/* This printf call is variadic */
>> +	CHECK(ffi_prep_cif_var(&cif, FFI_DEFAULT_ABI, 1, 2, &ffi_type_sint,
>>  		arg_types) == FFI_OK);
>> 
>>  	args[0] = &format;
>> @@ -48,6 +48,10 @@ int main (void)
>>  	printf("res: %d\n", (int) res);
>>  	// { dg-output "\nres: 4" }
>> 
>> +	/* The call to cls_longdouble_va_fn is static, so have to use a
>> normal prep_cif */
>> +	CHECK(ffi_prep_cif(&cif, FFI_DEFAULT_ABI, 2, &ffi_type_sint,
>> +		arg_types) == FFI_OK);
>> +
>>  	CHECK(ffi_prep_closure_loc(pcl, &cif, cls_longdouble_va_fn, NULL,
>> code) == FFI_OK);
>> 
>>  	res	= ((int(*)(char*, long double))(code))(format, ldArg);
>> diff --git a/testsuite/libffi.call/float_va.c b/testsuite/libffi.call/float_va.c
>> new file mode 100644
>> index 0000000..4611969
>> --- /dev/null
>> +++ b/testsuite/libffi.call/float_va.c
>> @@ -0,0 +1,107 @@
>> +/* Area:        fp and variadics
>> +   Purpose:     check fp inputs and returns work on variadics, even
>> the fixed params
>> +   Limitations: None
>> +   PR:          none
>> +   Originator:  <david.gilbert@linaro.org> 2011-01-25
>> +
>> +   Intended to stress the difference in ABI on ARM vfp
>> +*/
>> +
>> +/* { dg-do run } */
>> +
>> +#include <stdarg.h>
>> +
>> +#include "ffitest.h"
>> +
>> +/* prints out all the parameters, and returns the sum of them all.
>> + * 'x' is the number of variadic parameters all of which are double
>> in this test
>> + */
>> +double float_va_fn(unsigned int x, double y,...)
>> +{
>> +  double total=0.0;
>> +  va_list ap;
>> +  unsigned int i;
>> +
>> +  total+=(double)x;
>> +  total+=y;
>> +
>> +  printf("%u: %.1lf :", x, y);
>> +
>> +  va_start(ap, y);
>> +  for(i=0;i<x;i++)
>> +  {
>> +    double arg=va_arg(ap, double);
>> +    total+=arg;
>> +    printf(" %d:%.1lf ", i, arg);
>> +  }
>> +  va_end(ap);
>> +
>> +  printf(" total: %.1lf\n", total);
>> +
>> +  return total;
>> +}
>> +
>> +int main (void)
>> +{
>> +  ffi_cif    cif;
>> +
>> +  ffi_type    *arg_types[5];
>> +  void        *values[5];
>> +  double        doubles[5];
>> +  unsigned int firstarg;
>> +  double        resfp;
>> +
>> +  /* First test, pass float_va_fn(0,2.0) - note there are no actual
>> +   * variadic parameters, but it's declared variadic so the ABI may be
>> +   * different. */
>> +  /* Call it statically and then via ffi */
>> +  resfp=float_va_fn(0,2.0);
>> +  // { dg-output "0: 2.0 : total: 2.0" }
>> +  printf("compiled: %.1lf\n", resfp);
>> +  // { dg-output "\ncompiled: 2.0" }
>> +
>> +  arg_types[0] = &ffi_type_uint;
>> +  arg_types[1] = &ffi_type_double;
>> +  arg_types[2] = NULL;
>> +  CHECK(ffi_prep_cif_var(&cif, FFI_DEFAULT_ABI, 2, 2,
>> +        &ffi_type_double, arg_types) == FFI_OK);
>> +
>> +  firstarg = 0;
>> +  doubles[0] = 2.0;
>> +  values[0] = &firstarg;
>> +  values[1] = &doubles[0];
>> +  ffi_call(&cif, FFI_FN(float_va_fn), &resfp, values);
>> +  // { dg-output "\n0: 2.0 : total: 2.0" }
>> +  printf("ffi: %.1lf\n", resfp);
>> +  // { dg-output "\nffi: 2.0" }
>> +
>> +  /* Second test, float_va_fn(2,2.0,3.0,4.0), now with variadic params */
>> +  /* Call it statically and then via ffi */
>> +  resfp=float_va_fn(2,2.0,3.0,4.0);
>> +  // { dg-output "\n2: 2.0 : 0:3.0  1:4.0  total: 11.0" }
>> +  printf("compiled: %.1lf\n", resfp);
>> +  // { dg-output "\ncompiled: 11.0" }
>> +
>> +  arg_types[0] = &ffi_type_uint;
>> +  arg_types[1] = &ffi_type_double;
>> +  arg_types[2] = &ffi_type_double;
>> +  arg_types[3] = &ffi_type_double;
>> +  arg_types[4] = NULL;
>> +  CHECK(ffi_prep_cif_var(&cif, FFI_DEFAULT_ABI, 2, 4,
>> +        &ffi_type_double, arg_types) == FFI_OK);
>> +
>> +  firstarg = 2;
>> +  doubles[0] = 2.0;
>> +  doubles[1] = 3.0;
>> +  doubles[2] = 4.0;
>> +  values[0] = &firstarg;
>> +  values[1] = &doubles[0];
>> +  values[2] = &doubles[1];
>> +  values[3] = &doubles[2];
>> +  ffi_call(&cif, FFI_FN(float_va_fn), &resfp, values);
>> +  // { dg-output "\n2: 2.0 : 0:3.0  1:4.0  total: 11.0" }
>> +  printf("ffi: %.1lf\n", resfp);
>> +  // { dg-output "\nffi: 11.0" }
>> +
>> +  exit(0);
>> +}

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH] Add variadic support
  2011-03-16 14:12                       ` Anthony Green
@ 2011-03-16 14:25                         ` David Gilbert
  0 siblings, 0 replies; 22+ messages in thread
From: David Gilbert @ 2011-03-16 14:25 UTC (permalink / raw)
  To: Anthony Green; +Cc: Chung-Lin Tang, libffi-discuss, Marcus.Shawcroft, markos

On 16 March 2011 14:13, Anthony Green <green@redhat.com> wrote:
> Chung-Lin Tang <cltang@linaro.org> writes:
>
>> I think this looks fine.
>> Anthony, does the patch look okay to you?
>
> Yes, I think it's fine.  I was against adding a new API at first, but
> after talking to David about it on #libffi I was convinced that there's
> no reasonable alternative.  I'm going to check it into a branch on git
> until 3.0.10 is done.  The branch will be called "variadic".

Great!

> Thanks for you work David!

No problem.

Now all we have to do is spot things that are using it and persuade
them to use the new
interface.

If anyone comes across something that needs changing to use it, please
give me a prod.

Dave

^ permalink raw reply	[flat|nested] 22+ messages in thread

end of thread, other threads:[~2011-03-16 14:25 UTC | newest]

Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-02-22 15:41 [PATCH] Add variadic support Dr. David Alan Gilbert
2011-02-23 12:39 ` Anthony Green
2011-02-23 13:12   ` David Gilbert
2011-02-23 13:26     ` Chung-Lin Tang
2011-02-23 16:20       ` David Gilbert
2011-02-23 16:56         ` Chung-Lin Tang
2011-02-23 17:21           ` David Gilbert
2011-02-23 17:39             ` Chung-Lin Tang
2011-02-24  6:37               ` Chung-Lin Tang
2011-02-25 12:56               ` David Gilbert
2011-02-25 13:23                 ` Chung-Lin Tang
2011-02-25 19:02                   ` Anthony Green
2011-02-28  9:08                     ` David Gilbert
2011-03-07 18:19                   ` David Gilbert
2011-03-16  9:52                     ` Chung-Lin Tang
2011-03-16 14:12                       ` Anthony Green
2011-03-16 14:25                         ` David Gilbert
2011-02-23 21:16 ` PowerPC failures (Was: [PATCH] Add variadic support) Anthony Green
2011-02-24 10:43   ` David Gilbert
2011-02-24 22:37     ` PowerPC failures Anthony Green
2011-02-24 22:41       ` Matthias Klose
2011-02-25 19:18         ` Anthony Green

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).