bug#18592: FFI should have portable access to ‘errno’

[Mark H Weaver <mhw@netris.org>]

Hi Nala,

Thanks for working on this :)
Please see below for comments.

Nala Ginrut <nalaginrut@gmail.com> writes:

> From 88a99af4b5db9096c3cde51c72eb371b6be76754 Mon Sep 17 00:00:00 2001
> From: Nala Ginrut <nalaginrut@gmail.com>
> Date: Thu, 31 Dec 2015 20:27:59 +0800
> Subject: [PATCH 1/2] Add option to pointer->procedure to return errno if
>  necessary
>
> ---
>  libguile/foreign.c |   33 ++++++++++++++++++++++++---------
>  libguile/foreign.h |    2 +-
>  2 files changed, 25 insertions(+), 10 deletions(-)
>
> diff --git a/libguile/foreign.c b/libguile/foreign.c
> index 29cfc73..6909023 100644
> --- a/libguile/foreign.c
> +++ b/libguile/foreign.c
> @@ -1,4 +1,4 @@
> -/* Copyright (C) 2010-2015  Free Software Foundation, Inc.
> +/* Copyright (C) 2010-2016  Free Software Foundation, Inc.
>   *
>   * This library is free software; you can redistribute it and/or
>   * modify it under the terms of the GNU Lesser General Public License
> @@ -85,7 +85,7 @@ null_pointer_error (const char *func_name)
>  }
>  
>  \f
> -static SCM cif_to_procedure (SCM cif, SCM func_ptr);
> +static SCM cif_to_procedure (SCM cif, SCM func_ptr, SCM return_errno);
>  
>  
>  static SCM pointer_weak_refs = SCM_BOOL_F;
> @@ -753,24 +753,29 @@ make_cif (SCM return_type, SCM arg_types, const char *caller)
>  }
>  #undef FUNC_NAME
>  
> -SCM_DEFINE (scm_pointer_to_procedure, "pointer->procedure", 3, 0, 0,
> -            (SCM return_type, SCM func_ptr, SCM arg_types),
> +SCM_DEFINE (scm_pointer_to_procedure, "pointer->procedure", 3, 1, 0,
> +            (SCM return_type, SCM func_ptr, SCM arg_types, SCM return_errno),

Is this patch intended for 2.0.x?  Unfortunately, we cannot change the
signature of a C API function in 2.0.x, because it would change the ABI
and thus break any C code compiled against guile-2.0.11 that calls
scm_pointer_to_procedure.

Also, the proposal that we agreed upon was to add a #:return-errno?
keyword argument, not an optional positional argument.

We didn't discuss what the C API should look like.  We cannot change the
signature of 'scm_pointer_to_procedure' in 2.0.x, and I guess it would
be better to leave it unchanged in 2.2+ as well, to avoid
incompatibilities between C code written for 2.0 vs 2.2.

One possibility is to keep 'scm_pointer_to_procedure' as it is, and add
a new C API function 'scm_pointer_to_procedure_with_errno' that accepts
the same arguments but implies #:return-errno? #t.

If we adopt this approach, then this C function defined with SCM_DEFINE,
which accepts keyword arguments and corresponds to 'pointer->procedure',
will not be part of the public C API and thus not declared in foreign.h.

Thoughts?

I suggest that you look at commit 3ace9a8e4e, which added keyword
arguments to 'open-file' while keeping the API of 'scm_open_file'
unchanged.  It shows the preferred approach for handling keyword
arguments from C.

>              "Make a foreign function.\n\n"
>              "Given the foreign void pointer @var{func_ptr}, its argument and\n"
>              "return types @var{arg_types} and @var{return_type}, return a\n"
>              "procedure that will pass arguments to the foreign function\n"
>              "and return appropriate values.\n\n"
>              "@var{arg_types} should be a list of foreign types.\n"
> -            "@code{return_type} should be a foreign type.")
> +            "@code{return_type} should be a foreign type.\n"
> +	    "@var{return_errno} is @code{#f} in default, if set to #t, then\n"
> +	    "the @var{errno} will be returned as the second value.")

"If @var{return_errno} is true, then @code{errno} will be returned as a
second return value."

Also, please use spaces instead of tabs to indent here, for consistency
with the existing code.

>  #define FUNC_NAME s_scm_pointer_to_procedure
>  {
>    ffi_cif *cif;
>  
>    SCM_VALIDATE_POINTER (2, func_ptr);
>  
> +  if (SCM_UNLIKELY (SCM_UNBNDP (return_errno)))
> +    return_errno = SCM_BOOL_F;
> +

Please remove SCM_UNLIKELY here.  SCM_UNBNDP (return_errno) is not
unlikely.

>    cif = make_cif (return_type, arg_types, FUNC_NAME);
>  
> -  return cif_to_procedure (scm_from_pointer (cif, NULL), func_ptr);
> +  return cif_to_procedure (scm_from_pointer (cif, NULL), func_ptr, return_errno);

Please keep lines within 80 columns.

>  }
>  #undef FUNC_NAME
>  
> @@ -940,7 +945,7 @@ get_objcode_trampoline (unsigned int nargs)
>  }
>  
>  static SCM
> -cif_to_procedure (SCM cif, SCM func_ptr)
> +cif_to_procedure (SCM cif, SCM func_ptr, SCM return_errno)
>  {
>    ffi_cif *c_cif;
>    SCM objcode, table, ret;
> @@ -949,7 +954,8 @@ cif_to_procedure (SCM cif, SCM func_ptr)
>    objcode = get_objcode_trampoline (c_cif->nargs);
>    
>    table = scm_c_make_vector (2, SCM_UNDEFINED);
> -  SCM_SIMPLE_VECTOR_SET (table, 0, scm_cons (cif, func_ptr));
> +  SCM_SIMPLE_VECTOR_SET (table, 0,
> +			 scm_cons (cif, scm_cons (func_ptr, return_errno)));

You could use 'scm_cons2' here: scm_cons2 (cif, func_ptr, return_errno)
is equivalent to the code above.

Also, before storing 'return_errno', it would be good to convert it to a
simple boolean.  For example, if a user does this:

  (pointer->procedure ... #:return-errno? (assoc 'errno options))

then 'return_errno' will be a pair, and in general it could be a large
data structure which is simply meant to be interpreted as #t, but it
would be wasteful to retain a reference to that object and prevent it
from being garbage collected.  So, how about putting something like the
following code near the beginning of the function?

  /* Convert 'return_errno' to a simple boolean, to avoid retaining
     references to non-boolean objects. */
  return_errno = scm_from_bool (scm_is_true (return_errno));

>    SCM_SIMPLE_VECTOR_SET (table, 1, SCM_BOOL_F); /* name */
>    ret = scm_make_program (objcode, table, SCM_BOOL_F);
>    
> @@ -1116,9 +1122,11 @@ scm_i_foreign_call (SCM foreign, const SCM *argv)
>    unsigned i;
>    size_t arg_size;
>    scm_t_ptrdiff off;
> +  SCM return_errno;
>  
>    cif = SCM_POINTER_VALUE (SCM_CAR (foreign));
> -  func = SCM_POINTER_VALUE (SCM_CDR (foreign));
> +  func = SCM_POINTER_VALUE (SCM_CADR (foreign));
> +  return_errno = SCM_CDDR (foreign);
>  
>    /* Argument pointers.  */
>    args = alloca (sizeof (void *) * cif->nargs);
> @@ -1153,9 +1161,16 @@ scm_i_foreign_call (SCM foreign, const SCM *argv)
>    rvalue = (void *) ROUND_UP ((scm_t_uintptr) data + off,
>  			      max (sizeof (void *), cif->rtype->alignment));
>  
> +  errno = 0;

Please do not touch 'errno' unless scm_is_true (return_errno).

To avoid testing scm_is_true (return_errno) twice, I would recommend
calling 'ffi_call' within both branches of the 'if' statement.

>    /* off we go! */
>    ffi_call (cif, func, rvalue, args);
>  
> +  if (SCM_LIKELY (scm_is_true (return_errno)))

Please remove SCM_LIKELY.

> +    {
> +      return scm_values (scm_list_2 (pack (cif->rtype, rvalue, 1),
> +				     scm_from_int (errno)));

Please copy 'errno' to a local variable immediately after the call to
'ffi_call'.  In the code above, 'pack' and 'scm_list_2' are called
before 'errno' is read.  'scm_list_2' does GC allocation, and thus could
trigger a garbage collection which I guess would change 'errno'.  'pack'
might also do so.

> +    }
> +
>    return pack (cif->rtype, rvalue, 1);
>  }
>  
> diff --git a/libguile/foreign.h b/libguile/foreign.h
> index 41c0b65..8541526 100644
> --- a/libguile/foreign.h
> +++ b/libguile/foreign.h

Please add 2016 to this file's copyright dates.

> @@ -93,7 +93,7 @@ SCM_INTERNAL SCM scm_pointer_to_string (SCM pointer, SCM length, SCM encoding);
>   */
>  
>  SCM_API SCM scm_pointer_to_procedure (SCM return_type, SCM func_ptr,
> -				      SCM arg_types);
> +				      SCM arg_types, SCM return_errno);

This will need to be updated to account for the C API issues discussed
above.

>  SCM_API SCM scm_procedure_to_pointer (SCM return_type, SCM func_ptr,
>  				      SCM arg_types);
>  SCM_INTERNAL SCM scm_i_foreign_call (SCM foreign, const SCM *argv);
> -- 
> 1.7.10.4
>
>
> From 71151759513f8163e45c328e5bcae8e89ebbf614 Mon Sep 17 00:00:00 2001
> From: Nala Ginrut <nalaginrut@gmail.com>
> Date: Thu, 31 Dec 2015 20:28:36 +0800
> Subject: [PATCH 2/2] updated pointer->procedure in document
>
> ---
>  doc/ref/api-foreign.texi |    7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/doc/ref/api-foreign.texi b/doc/ref/api-foreign.texi
> index c2c49ec..9fd09f5 100644
> --- a/doc/ref/api-foreign.texi
> +++ b/doc/ref/api-foreign.texi
> @@ -813,8 +813,8 @@ tightly packed structs and unions by hand. See the code for
>  Of course, the land of C is not all nouns and no verbs: there are
>  functions too, and Guile allows you to call them.
>  
> -@deffn {Scheme Procedure} pointer->procedure return_type func_ptr arg_types
> -@deffnx {C Procedure} scm_pointer_to_procedure (return_type, func_ptr, arg_types)
> +@deffn {Scheme Procedure} pointer->procedure return_type func_ptr arg_types [return_errno=#f]
> +@deffnx {C Procedure} scm_pointer_to_procedure (return_type, func_ptr, arg_types, return_errno)

This will also need to be updated to account for the C API issues
discussed above.

>  Make a foreign function.
>  
>  Given the foreign void pointer @var{func_ptr}, its argument and
> @@ -825,6 +825,9 @@ and return appropriate values.
>  @var{arg_types} should be a list of foreign types.
>  @code{return_type} should be a foreign type. @xref{Foreign Types}, for
>  more information on foreign types.
> +@var{return_errno} is @code{#f} in default, if set to @code{#t}, then @var{errno}
> +will be returned as the second value.

If @var{return_errno} is true, then @code{errno} will be returned as the
second return value.

> +
>  @end deffn
>  
>  Here is a better definition of @code{(math bessel)}:

      Thanks!
        Mark