From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!not-for-mail From: Mark H Weaver Newsgroups: gmane.lisp.guile.bugs Subject: bug#18592: FFI should have portable access to =?UTF-8?Q?=E2=80=98errno=E2=80=99?= Date: Mon, 04 Jan 2016 11:12:27 -0500 Message-ID: <8760z9gw7o.fsf@netris.org> References: <87fvf8oocf.fsf@ft.bewatermyfriend.org> <87h9vmy0zw.fsf@gnu.org> <87twzgeh3c.fsf@yeeloong.lan> <87r3uko4c9.fsf@gnu.org> <1451565229.3594.59.camel@Renee-desktop.suse> <1451909046.3594.135.camel@Renee-desktop.suse> NNTP-Posting-Host: plane.gmane.org Mime-Version: 1.0 Content-Type: text/plain X-Trace: ger.gmane.org 1451924002 11457 80.91.229.3 (4 Jan 2016 16:13:22 GMT) X-Complaints-To: usenet@ger.gmane.org NNTP-Posting-Date: Mon, 4 Jan 2016 16:13:22 +0000 (UTC) Cc: 18592@debbugs.gnu.org, Ludovic =?UTF-8?Q?Court=C3=A8s?= , Chaos Eternal To: Nala Ginrut Original-X-From: bug-guile-bounces+guile-bugs=m.gmane.org@gnu.org Mon Jan 04 17:13:12 2016 Return-path: Envelope-to: guile-bugs@m.gmane.org Original-Received: from lists.gnu.org ([208.118.235.17]) by plane.gmane.org with esmtp (Exim 4.69) (envelope-from ) id 1aG7ku-0003kh-74 for guile-bugs@m.gmane.org; Mon, 04 Jan 2016 17:13:12 +0100 Original-Received: from localhost ([::1]:45655 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aG7kt-0004cy-Ld for guile-bugs@m.gmane.org; Mon, 04 Jan 2016 11:13:11 -0500 Original-Received: from eggs.gnu.org ([2001:4830:134:3::10]:37212) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aG7kp-0004ch-EO for bug-guile@gnu.org; Mon, 04 Jan 2016 11:13:09 -0500 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aG7kk-0005Az-9z for bug-guile@gnu.org; Mon, 04 Jan 2016 11:13:07 -0500 Original-Received: from debbugs.gnu.org ([208.118.235.43]:49536) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aG7kk-0005Au-5Y for bug-guile@gnu.org; Mon, 04 Jan 2016 11:13:02 -0500 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84) (envelope-from ) id 1aG7kj-0006Pj-T5 for bug-guile@gnu.org; Mon, 04 Jan 2016 11:13:01 -0500 X-Loop: help-debbugs@gnu.org Resent-From: Mark H Weaver Original-Sender: "Debbugs-submit" Resent-CC: bug-guile@gnu.org Resent-Date: Mon, 04 Jan 2016 16:13:01 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 18592 X-GNU-PR-Package: guile X-GNU-PR-Keywords: Original-Received: via spool by 18592-submit@debbugs.gnu.org id=B18592.145192396124628 (code B ref 18592); Mon, 04 Jan 2016 16:13:01 +0000 Original-Received: (at 18592) by debbugs.gnu.org; 4 Jan 2016 16:12:41 +0000 Original-Received: from localhost ([127.0.0.1]:37756 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84) (envelope-from ) id 1aG7kP-0006P9-74 for submit@debbugs.gnu.org; Mon, 04 Jan 2016 11:12:41 -0500 Original-Received: from world.peace.net ([50.252.239.5]:52609 ident=hope7) by debbugs.gnu.org with esmtp (Exim 4.84) (envelope-from ) id 1aG7kN-0006P0-Lm for 18592@debbugs.gnu.org; Mon, 04 Jan 2016 11:12:40 -0500 Original-Received: from [10.1.10.78] (helo=jojen) by world.peace.net with esmtpsa (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.72) (envelope-from ) id 1aG7kC-0002Ii-O2; Mon, 04 Jan 2016 11:12:28 -0500 In-Reply-To: <1451909046.3594.135.camel@Renee-desktop.suse> (Nala Ginrut's message of "Mon, 04 Jan 2016 20:04:06 +0800") User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] X-Received-From: 208.118.235.43 X-BeenThere: bug-guile@gnu.org List-Id: "Bug reports for GUILE, GNU's Ubiquitous Extension Language" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: bug-guile-bounces+guile-bugs=m.gmane.org@gnu.org Original-Sender: bug-guile-bounces+guile-bugs=m.gmane.org@gnu.org Xref: news.gmane.org gmane.lisp.guile.bugs:7921 Archived-At: Hi Nala, Thanks for working on this :) Please see below for comments. Nala Ginrut writes: > From 88a99af4b5db9096c3cde51c72eb371b6be76754 Mon Sep 17 00:00:00 2001 > From: Nala Ginrut > 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 > 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