[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
bug#18592: FFI should have portable access to ‘errno’
From: |
Mark H Weaver |
Subject: |
bug#18592: FFI should have portable access to ‘errno’ |
Date: |
Mon, 04 Jan 2016 11:12:27 -0500 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) |
Hi Nala,
Thanks for working on this :)
Please see below for comments.
Nala Ginrut <address@hidden> writes:
> From 88a99af4b5db9096c3cde51c72eb371b6be76754 Mon Sep 17 00:00:00 2001
> From: Nala Ginrut <address@hidden>
> 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)
> }
>
>
> -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 <address@hidden>
> 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.
>
> address@hidden {Scheme Procedure} pointer->procedure return_type func_ptr
> arg_types
> address@hidden {C Procedure} scm_pointer_to_procedure (return_type, func_ptr,
> arg_types)
> address@hidden {Scheme Procedure} pointer->procedure return_type func_ptr
> arg_types [return_errno=#f]
> address@hidden {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.
> address@hidden 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
- bug#18592: FFI should have portable access to ‘errno’, Nala Ginrut, 2016/01/04
- bug#18592: FFI should have portable access to ‘errno’,
Mark H Weaver <=
- bug#18592: FFI should have portable access to ‘errno’, Nala Ginrut, 2016/01/04
- bug#18592: FFI should have portable access to ‘errno’, Chaos Eternal, 2016/01/04
- bug#18592: FFI should have portable access to ‘errno’, tomas, 2016/01/05
- bug#18592: FFI should have portable access to ‘errno’, Nala Ginrut, 2016/01/05
- bug#18592: FFI should have portable access to ‘errno’, Mark H Weaver, 2016/01/05
- bug#18592: FFI should have portable access to ‘errno’, Nala Ginrut, 2016/01/05
- bug#18592: FFI should have portable access to ‘errno’, Mark H Weaver, 2016/01/05
bug#18592: FFI should have portable access to ‘errno’, Mark H Weaver, 2016/01/04