From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!not-for-mail From: Philipp Stephani Newsgroups: gmane.emacs.devel Subject: Re: Dynamic loading progress Date: Thu, 19 Nov 2015 22:41:09 +0000 Message-ID: References: <83k2ptq5t3.fsf@gnu.org> <87h9kxx60e.fsf@lifelogs.com> <877flswse5.fsf@lifelogs.com> <8737wgw7kf.fsf@lifelogs.com> <87io5bv1it.fsf@lifelogs.com> <87egfzuwca.fsf@lifelogs.com> <876118u6f2.fsf@lifelogs.com> <8737w3qero.fsf@lifelogs.com> <831tbn9g9j.fsf@gnu.org> <878u5upw7o.fsf@lifelogs.com> <83ziya8xph.fsf@gnu.org> <83y4du80xo.fsf@gnu.org> NNTP-Posting-Host: plane.gmane.org Mime-Version: 1.0 Content-Type: multipart/alternative; boundary=001a114450c81561190524ec75f0 X-Trace: ger.gmane.org 1447972910 16890 80.91.229.3 (19 Nov 2015 22:41:50 GMT) X-Complaints-To: usenet@ger.gmane.org NNTP-Posting-Date: Thu, 19 Nov 2015 22:41:50 +0000 (UTC) Cc: emacs-devel@gnu.org To: Eli Zaretskii , Ted Zlatanov , =?UTF-8?Q?Aur=C3=A9lien_Aptel?= Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Thu Nov 19 23:41:36 2015 Return-path: Envelope-to: ged-emacs-devel@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 1ZzXtW-00016k-6c for ged-emacs-devel@m.gmane.org; Thu, 19 Nov 2015 23:41:34 +0100 Original-Received: from localhost ([::1]:44357 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZzXtV-0003G8-GC for ged-emacs-devel@m.gmane.org; Thu, 19 Nov 2015 17:41:33 -0500 Original-Received: from eggs.gnu.org ([2001:4830:134:3::10]:53910) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZzXtP-0003G0-CD for emacs-devel@gnu.org; Thu, 19 Nov 2015 17:41:30 -0500 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZzXtL-0002Ca-US for emacs-devel@gnu.org; Thu, 19 Nov 2015 17:41:27 -0500 Original-Received: from mail-wm0-x22b.google.com ([2a00:1450:400c:c09::22b]:34733) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZzXtH-00023A-Cz; Thu, 19 Nov 2015 17:41:19 -0500 Original-Received: by wmvv187 with SMTP id v187so47930775wmv.1; Thu, 19 Nov 2015 14:41:18 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc:content-type; bh=uRcNAO/zppDDwT/Wghss0CY1VGSUVJYt8NCtVkFaGDo=; b=dFxLdfq6mRqOWIFS4ydTQC9EJNFEQwojUjZIssOc7CqfVs/91eQRT9eX3cdOKEeRac TFSi38jsTyEdOIZwgOwCQ82xql9gZtz7i0y7AmVdum61Ct2qEO9jPklO4uBqtl1RCzhH 4tauCmeugD7TrJXqZN1q0WfIB7Rkqp8o5oXkaaeacPS0tVI5LAHNyh64kq3ydtQdTP19 lCuxd+2M0ZvcpIWsRg720dL1Wb4aFB/xYhP3N0slv4qIIOmBhTvnySlwuzWKxTt/B90M 9qrEU379pNJ1e21Vg0bvZFQMKSjwc7uXSqL4RCm/bHraIX99PUDhdYZbqw+QJB1j3NeR I0EQ== X-Received: by 10.28.7.8 with SMTP id 8mr192395wmh.45.1447972878768; Thu, 19 Nov 2015 14:41:18 -0800 (PST) In-Reply-To: <83y4du80xo.fsf@gnu.org> X-detected-operating-system: by eggs.gnu.org: Error: Malformed IPv6 address (bad octet value). X-Received-From: 2a00:1450:400c:c09::22b X-BeenThere: emacs-devel@gnu.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: "Emacs development discussions." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Original-Sender: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Xref: news.gmane.org gmane.emacs.devel:194826 Archived-At: --001a114450c81561190524ec75f0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Thanks for the thorough review; I'll prepare patches for all of these issues. Eli Zaretskii schrieb am Do., 19. Nov. 2015 um 16:28 Uhr: > Thanks for making this possible. > > Please allow me a few questions/comments about the dynamic modules > code, based on some initial reading: > > Why module.c, but confusingly emacs_module.h? Are there any reasons > not to use the same base name, like we do for other C sources? > > emacs_module.h is intended to be included by module authors. Therefore its name needs to be globally unique, which in practice means it needs to start with 'emacs_'. module.c could be renamed accordingly, if you prefer that. However, Aur=C3=A9lien picked the simple name because it's not required to be global= ly unique. > This comment in module.c: > > /* If checking is enabled, abort if the current thread is not the > Emacs main thread. */ > static void check_main_thread (void); > > confused me, because a later comment says: > > void module_init (void) > { > /* It is not guaranteed that dynamic initializers run in the main > thread, > therefore we detect the main thread here. */ > > If dynamic initializers might run in a thread different from the Emacs > main thread, then the code in module_init will record that other > thread, not the Emacs main thread, right? > No, because module_init is guaranteed to be called from the main thread because main calls it explicitly. > > Also, another comment: > > /* On Windows, we store both a handle to the main thread and the > thread ID because the latter can be reused when a thread > terminates. */ > > seems to imply that 'main_thread' here is not the Emacs's main thread, > because that thread never terminates as long as the Emacs session is > alive. > > So what's the deal here? what does this thread checking supposed to > detect? > This guards against the Emacs main thread having exited while module code in some other thread is still running and attempting to call Emacs functions. This is undefined behavior, but we included an explicit check if checking is enabled because that case is somewhat subtle. > > In this code from module.c: > > Lisp_Object value =3D HASH_VALUE (h, i); > eassert (NATNUMP (value)); > const EMACS_UINT refcount =3D XFASTINT (value); > if (refcount >=3D MOST_POSITIVE_FIXNUM) > { > module_non_local_exit_signal_1 (env, Qoverflow_error, Qnil); > return NULL; > } > > how can the 'if' clause ever be true? refcount is an Emacs integer, > as you have just verified, no? And if this somehow can happen, then > why isn't there a similar check in the other functions? > refcount can be MOST_POSITIVE_FIXNUM because that's an inclusive bound. It's important to check that case because later refcount is incremented by one, and if it's equal to MOST_POSITIVE_FIXNUM it would be outside the allowed range afterwards. No other function increments numbers, thus no other functions need this check. > > Re this fragment from module.c: > > Lisp_Object ret =3D list4 (Qlambda, > list2 (Qand_rest, Qargs), > documentation ? build_string (documentation) : > Qnil, > list3 (module_call_func, > envobj, > Qargs)); > > Thou shalt not use build_string, except when you _know_ the argument > will always be a pure-ASCII string. Practically, this means the > argument must be a constant ASCII string. See these messages (and the > preceding discussion, if you are interested) for the gory details: > > http://lists.gnu.org/archive/html/bug-gnu-emacs/2013-10/msg00955.html > http://lists.gnu.org/archive/html/bug-gnu-emacs/2013-10/msg00976.html > http://lists.gnu.org/archive/html/bug-gnu-emacs/2013-10/msg00979.html > > The above should call make_multibyte_string instead. > We had a discussion about encodings in https://github.com/aaptel/emacs-dynamic-module/issues/37. Sorry that this didn't get resolved earlier; it's an important point. My suggestion would be to always mandate specifying an encoding whenever a char* is passed, and limit that to two or three functions dealing with creating strings and accessing string contents. Would that address your concerns? > > Again in module.c: > > /* > * Emacs internal encoding is more-or-less UTF8, let's assume utf8 > * encoded emacs string are the same byte size. > */ > > if (!buffer || length =3D=3D 0 || *length-1 < raw_size) > { > *length =3D raw_size + 1; > return false; > } > > I don't understand why you need this assumption. You are going to > encode the string in a moment, so why not test 'length' against the > size you actually obtain there? (The above test will misfire when the > original string includes characters above the Unicode codespace, which > require 5 bytes internally, but their encoding maps them to Unicode > codepoints which cannot take more than 4 bytes. So you might reject > perfectly valid calls.) > > In module_make_string you have: > > /* Assume STR is utf8 encoded */ > return lisp_to_value (env, make_string (str, length)); > > The discussion I pointed to above concluded that make_string is > a bug. So please use make_multibyte_string here instead. > See above; my suggestion would be to change the string handling code by limiting encoding and decoding to a small set of functions where the encoding would have to be specified explicitly. > > It looks like XUSER_PTR is used both as an lvalue and an rvalue. This > is different from any other object, where we have separate Xfoo and > XSETfoo macros. Suggest to follow suit. > Agreed. > > static void module_set_user_finalizer (emacs_env *env, > emacs_value uptr, > emacs_finalizer_function fin= ) > { > check_main_thread (); > eassert (module_non_local_exit_check (env) =3D=3D > emacs_funcall_exit_return); > const Lisp_Object lisp =3D value_to_lisp (uptr); > if (! USER_PTRP (lisp)) module_wrong_type (env, Quser_ptr, lisp); > XUSER_PTR (lisp)->finalizer =3D fin; <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<= < > } > > No validity checks on 'fin'? > How should it be validated? In C an arbitrary (invalid) pointer could be passed. I think we just have to accept that this is UB. > > In module_vec_get: > > /* Prevent error-prone comparison between types of different signedness= . > */ > const size_t size =3D ASIZE (lvec); > eassert (size >=3D 0); > > How can the assertion be ever violated? > Yeah, that's a bug. I probably meant size to be declared as ptrdiff_t, which is what ASIZE returns. > > In module-load: > > CHECK_STRING (file); > handle =3D dynlib_open (SDATA (file)); > > Every Lisp primitive that accepts file arguments _must_ call > expand-file-name on the file name, before using it. Otherwise, > relative file names will produce subtle and hard-to-debug problems > when the Lisp program calling them involves changing the current > directory of the Emacs process. > > The other mandatory thing absent from the above is ENCODE_FILE. You > cannot pass unencoded file names to C runtime functions. > OK, will send a patch. > > struct { > struct emacs_runtime pub; > struct emacs_runtime_private priv; > } runtime =3D { > .pub =3D { > .size =3D sizeof runtime.pub, > .get_environment =3D module_get_environment, > .private_members =3D &runtime.priv > } > }; > > Is this portable enough? > According to http://en.cppreference.com/w/c/language/struct_initialization and https://gcc.gnu.org/onlinedocs/gcc/Designated-Inits.html designated initializers are part of C99, which is required for building Emacs. > > int r =3D module_init (&runtime.pub); > > I think calling this function "module_init" is sub-optimal: you have a > void function by the same name in this source file. How about > renaming it to something else? > Agree > > Also, it seems that once a module is loaded, it cannot be unloaded > (i.e., no unload-feature equivalent is provided). Is that on purpose? > An Emacs process can live for a very long time, so keeping all of the > modules open in it at all times is not necessarily TRT. E.g., how do > I update to a newer version of a module? > Unloading is important ( https://github.com/aaptel/emacs-dynamic-module/issues/36), but for now we decided to delay it to a later version because we expect much discussion about the precise semantics. > > Shouldn't module-init call dynlib_close before it returns? Otherwise > we are leaking descriptors here, no? > My impression from reading dlclose(3) is that modules shouldn't be unloaded while they are still used. > > In module-call: > > const EMACS_INT len =3D XINT (Flength (arglist)); > eassert (len >=3D 0); > if (len > MOST_POSITIVE_FIXNUM) > xsignal0 (Qoverflow_error); > > How can the 'if' clause ever be true? XINT by definition cannot > produce anything but a valid EMACS_INT, can it? > True. Not sure what I was thinking. (Could be replaced by an eassert, to document the assumption.) > > if (len > INT_MAX || len < envptr->min_arity || (envptr->max_arity >=3D= 0 > && len > envptr->max_arity)) > xsignal2 (Qwrong_number_of_arguments, module_format_fun_env (envptr), > make_number (len)); > > Why the test against INT_MAX? EMACS_INT can legitimately be a 64-bit > data type with values far exceeding INT_MAX. Why impose this > limitation here? > Because the nargs argument in the module interface is an int. If you think functions with more than INT_MAX arguments should be supported, the type for nargs should be changed to int64. > > allocate_emacs_value calls malloc; shouldn't it call xmalloc instead, > or at least conform to the XMALLOC_BLOCK_INPUT_CHECK protocol? > If xmalloc is called, then we need to make sure that no signals (longjmps) can escape to module code. If appropriate setjmps are in place that should be doable, but I need to check whether there are edge cases. > > In module_format_fun_env, you produce a unibyte string, and then use > that in calls to functions like xsignal1, which expect Lisp strings in > their internal multibyte representation. You should instead decode > the unibyte string (using UTF-8) before you return it. > OK, will send a patch. > > Btw, I wonder whether we should provide a more friendly capabilities > for retrieving the function name and its module. dladdr is not > portable enough, and we have all the information at our fingertips > in the module's init function, we just need to keep it instead of > throwing it away. I envision debugging module-related problems will > not be a very rare situation, so we need any help we can get. WDYT? > Hmm, I don't know whether we have access to the function name without using dladdr. The user just passes a pointer to the module, not its name. > > In syms_of_module: > > /* Unintern `module-environments' because it is only used > internally. */ > Funintern (Qmodule_environments, Qnil); > > What if some Lisp defines a (interned) symbol by that name? Won't > they clash? > I followed the lead of internal-interpreter-environment in eval.c, which uses the same pattern. > > The Windows-specific parts of dynlib.c need work, e.g. you cannot pass > UTF-8 encoded file names to Windows APIs. And there are some other > issues. I'll take care of that. > I can also do that, I should be able to set up a build environment in a VM. > > About the tests: The Makefile in mod-test is Unix-specific: it uses a > literal .so extension. I also think the Python script should be > rewritten in Emacs Lisp, so that Python installation is not required. > Finally, all of the module tests and associated files should be moved > into test/, preferably even test/automated/ and made part of the "make > check" run. > > Yes, tracked in https://github.com/aaptel/emacs-dynamic-module/issues/34 --001a114450c81561190524ec75f0 Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable
Thanks for the thorough review; I'll prepare patches f= or all of these issues.

= Eli Zaretskii <eliz@gnu.org> schr= ieb am Do., 19. Nov. 2015 um 16:28=C2=A0Uhr:
Thanks for making this possible.

Please allow me a few questions/comments about the dynamic modules
code, based on some initial reading:

Why module.c, but confusingly emacs_module.h?=C2=A0 Are there any reasons not to use the same base name, like we do for other C sources?


emacs_module.h is intended to be inclu= ded by module authors. Therefore its name needs to be globally unique, whic= h in practice means it needs to start with 'emacs_'.
modu= le.c could be renamed accordingly, if you prefer that. However, Aur=C3=A9li= en picked the simple name because it's not required to be globally uniq= ue.
=C2=A0
This comment in module.c:

=C2=A0 /* If checking is enabled, abort if the current thread is not the =C2=A0 =C2=A0 =C2=A0Emacs main thread. */
=C2=A0 static void check_main_thread (void);

confused me, because a later comment says:

=C2=A0 void module_init (void)
=C2=A0 {
=C2=A0 =C2=A0 /* It is not guaranteed that dynamic initializers run in the = main thread,
=C2=A0 =C2=A0 =C2=A0 =C2=A0therefore we detect the main thread here. */

If dynamic initializers might run in a thread different from the Emacs
main thread, then the code in module_init will record that other
thread, not the Emacs main thread, right?

No, because module_init is guaranteed to be called from the main thread = because main calls it explicitly.
=C2=A0

Also, another comment:

=C2=A0 /* On Windows, we store both a handle to the main thread and the
=C2=A0 =C2=A0 =C2=A0thread ID because the latter can be reused when a threa= d
=C2=A0 =C2=A0 =C2=A0terminates. */

seems to imply that 'main_thread' here is not the Emacs's main = thread,
because that thread never terminates as long as the Emacs session is
alive.

So what's the deal here? what does this thread checking supposed to
detect?

This guards against the Emacs m= ain thread having exited while module code in some other thread is still ru= nning and attempting to call Emacs functions. This is undefined behavior, b= ut we included an explicit check if checking is enabled because that case i= s somewhat subtle.
=C2=A0

In this code from module.c:

=C2=A0 =C2=A0 =C2=A0 Lisp_Object value =3D HASH_VALUE (h, i);
=C2=A0 =C2=A0 =C2=A0 eassert (NATNUMP (value));
=C2=A0 =C2=A0 =C2=A0 const EMACS_UINT refcount =3D XFASTINT (value);
=C2=A0 =C2=A0 =C2=A0 if (refcount >=3D MOST_POSITIVE_FIXNUM)
=C2=A0 =C2=A0 =C2=A0 =C2=A0 {
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 module_non_local_exit_signal_1 (env, Qov= erflow_error, Qnil);
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return NULL;
=C2=A0 =C2=A0 =C2=A0 =C2=A0 }

how can the 'if' clause ever be true?=C2=A0 refcount is an Emacs in= teger,
as you have just verified, no?=C2=A0 And if this somehow can happen, then why isn't there a similar check in the other functions?

refcount can be MOST_POSITIVE_FIXNUM because that'= s an inclusive bound. It's important to check that case because later r= efcount is incremented by one, and if it's equal to MOST_POSITIVE_FIXNU= M it would be outside the allowed range afterwards. No other function incre= ments numbers, thus no other functions need this check.
=C2=A0

Re this fragment from module.c:

=C2=A0 Lisp_Object ret =3D list4 (Qlambda,
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0list2 (Qand_rest, Qargs),
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0documentation ? build_string (documentation) : Qnil= ,
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0list3 (module_call_func,
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 envobj,
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 Qargs));

Thou shalt not use build_string, except when you _know_ the argument
will always be a pure-ASCII string.=C2=A0 Practically, this means the
argument must be a constant ASCII string.=C2=A0 See these messages (and the=
preceding discussion, if you are interested) for the gory details:

=C2=A0 http://lists.gnu.org/arc= hive/html/bug-gnu-emacs/2013-10/msg00955.html
=C2=A0 http://lists.gnu.org/arc= hive/html/bug-gnu-emacs/2013-10/msg00976.html
=C2=A0 http://lists.gnu.org/arc= hive/html/bug-gnu-emacs/2013-10/msg00979.html

The above should call make_multibyte_string instead.
<= br>
We had a discussion about encodings in=C2=A0https://github.com/aapt= el/emacs-dynamic-module/issues/37. Sorry that this didn't get resol= ved earlier; it's an important point. My suggestion would be to always = mandate specifying an encoding whenever a char* is passed, and limit that t= o two or three functions dealing with creating strings and accessing string= contents. Would that address your concerns?
=C2=A0

Again in module.c:

=C2=A0 /*
=C2=A0 =C2=A0* Emacs internal encoding is more-or-less UTF8, let's assu= me utf8
=C2=A0 =C2=A0* encoded emacs string are the same byte size.
=C2=A0 =C2=A0*/

=C2=A0 if (!buffer || length =3D=3D 0 || *length-1 < raw_size)
=C2=A0 =C2=A0 {
=C2=A0 =C2=A0 =C2=A0 *length =3D raw_size + 1;
=C2=A0 =C2=A0 =C2=A0 return false;
=C2=A0 =C2=A0 }

I don't understand why you need this assumption.=C2=A0 You are going to=
encode the string in a moment, so why not test 'length' against the=
size you actually obtain there?=C2=A0 (The above test will misfire when the=
original string includes characters above the Unicode codespace, which
require 5 bytes internally, but their encoding maps them to Unicode
codepoints which cannot take more than 4 bytes.=C2=A0 So you might reject perfectly valid calls.)

In module_make_string you have:

=C2=A0 /* Assume STR is utf8 encoded */
=C2=A0 return lisp_to_value (env, make_string (str, length));

The discussion I pointed to above concluded that <quote>make_string i= s
a bug</quote>.=C2=A0 So please use make_multibyte_string here instead= .

See above; my suggestion would be to = change the string handling code by limiting encoding and decoding to a smal= l set of functions where the encoding would have to be specified explicitly= .
=C2=A0

It looks like XUSER_PTR is used both as an lvalue and an rvalue.=C2=A0 This=
is different from any other object, where we have separate Xfoo and
XSETfoo macros.=C2=A0 Suggest to follow suit.

Agreed.
=C2=A0

=C2=A0 static void module_set_user_finalizer (emacs_env *env,
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0emacs_value uptr,
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0emacs_finalizer_function fin)
=C2=A0 {
=C2=A0 =C2=A0 check_main_thread ();
=C2=A0 =C2=A0 eassert (module_non_local_exit_check (env) =3D=3D emacs_funca= ll_exit_return);
=C2=A0 =C2=A0 const Lisp_Object lisp =3D value_to_lisp (uptr);
=C2=A0 =C2=A0 if (! USER_PTRP (lisp)) module_wrong_type (env, Quser_ptr, li= sp);
=C2=A0 =C2=A0 XUSER_PTR (lisp)->finalizer =3D fin; <<<<<&= lt;<<<<<<<<<<<<<<<<<<= <<<<<<<<<
=C2=A0 }

No validity checks on 'fin'?

Ho= w should it be validated? In C an arbitrary (invalid) pointer could be pass= ed. I think we just have to accept that this is UB.
=C2=A0
<= blockquote class=3D"gmail_quote" style=3D"margin:0 0 0 .8ex;border-left:1px= #ccc solid;padding-left:1ex">
In module_vec_get:

=C2=A0 /* Prevent error-prone comparison between types of different signedn= ess. */
=C2=A0 const size_t size =3D ASIZE (lvec);
=C2=A0 eassert (size >=3D 0);

How can the assertion be ever violated?

Yeah, that's a bug. I probably meant size to be declared as ptrdiff_t,= which is what ASIZE returns.
=C2=A0

In module-load:

=C2=A0 CHECK_STRING (file);
=C2=A0 handle =3D dynlib_open (SDATA (file));

Every Lisp primitive that accepts file arguments _must_ call
expand-file-name on the file name, before using it.=C2=A0 Otherwise,
relative file names will produce subtle and hard-to-debug problems
when the Lisp program calling them involves changing the current
directory of the Emacs process.

The other mandatory thing absent from the above is ENCODE_FILE.=C2=A0 You cannot pass unencoded file names to C runtime functions.

OK, will send a patch.
=C2=A0

=C2=A0 struct {
=C2=A0 =C2=A0 struct emacs_runtime pub;
=C2=A0 =C2=A0 struct emacs_runtime_private priv;
=C2=A0 } runtime =3D {
=C2=A0 =C2=A0 .pub =3D {
=C2=A0 =C2=A0 =C2=A0 .size =3D sizeof runtime.pub,
=C2=A0 =C2=A0 =C2=A0 .get_environment =3D module_get_environment,
=C2=A0 =C2=A0 =C2=A0 .private_members =3D &runtime.priv
=C2=A0 =C2=A0 }
=C2=A0 };

Is this portable enough?

According to= =C2=A0http://en.cppreference.com/w/c/language/struct_initialization=C2= =A0and=C2=A0https://gcc.gnu.org/onlinedocs/gcc/Designated-Inits.html=C2=A0des= ignated initializers are part of C99, which is required for building Emacs.=
=C2=A0

=C2=A0 int r =3D module_init (&runtime.pub);

I think calling this function "module_init" is sub-optimal: you h= ave a
void function by the same name in this source file.=C2=A0 How about
renaming it to something else?

Agree
=C2=A0

Also, it seems that once a module is loaded, it cannot be unloaded
(i.e., no unload-feature equivalent is provided).=C2=A0 Is that on purpose?=
An Emacs process can live for a very long time, so keeping all of the
modules open in it at all times is not necessarily TRT.=C2=A0 E.g., how do<= br> I update to a newer version of a module?

Unloading is important (https://github.com/aaptel/emacs-dynamic-module/issues/3= 6), but for now we decided to delay it to a later version because we ex= pect much discussion about the precise semantics.
=C2=A0

Shouldn't module-init call dynlib_close before it returns?=C2=A0 Otherw= ise
we are leaking descriptors here, no?

My= impression from reading dlclose(3) is that modules shouldn't be unload= ed while they are still used.
=C2=A0

In module-call:

=C2=A0 const EMACS_INT len =3D XINT (Flength (arglist));
=C2=A0 eassert (len >=3D 0);
=C2=A0 if (len > MOST_POSITIVE_FIXNUM)
=C2=A0 =C2=A0 xsignal0 (Qoverflow_error);

How can the 'if' clause ever be true?=C2=A0 XINT by definition cann= ot
produce anything but a valid EMACS_INT, can it?

True. Not sure what I was thinking. (Could be replaced by an easse= rt, to document the assumption.)
=C2=A0

=C2=A0 if (len > INT_MAX || len < envptr->min_arity || (envptr->= ;max_arity >=3D 0 && len > envptr->max_arity))
=C2=A0 =C2=A0 xsignal2 (Qwrong_number_of_arguments, module_format_fun_env (= envptr), make_number (len));

Why the test against INT_MAX?=C2=A0 EMACS_INT can legitimately be a 64-bit<= br> data type with values far exceeding INT_MAX.=C2=A0 Why impose this
limitation here?

Because the nargs argu= ment in the module interface is an int.
If you think functions wi= th more than INT_MAX arguments should be supported, the type for nargs shou= ld be changed to int64.
=C2=A0

In module_format_fun_env, you produce a unibyte string, and then use
that in calls to functions like xsignal1, which expect Lisp strings in
their internal multibyte representation.=C2=A0 You should instead decode the unibyte string (using UTF-8) before you return it.

OK, will send a patch.
=C2=A0

Btw, I wonder whether we should provide a more friendly capabilities
for retrieving the function name and its module.=C2=A0 dladdr is not
portable enough, and we have all the information at our fingertips
in the module's init function, we just need to keep it instead of
throwing it away.=C2=A0 I envision debugging module-related problems will not be a very rare situation, so we need any help we can get.=C2=A0 WDYT?

Hmm, I don't know whether we have ac= cess to the function name without using dladdr. The user just passes a poin= ter to the module, not its name.
=C2=A0

In syms_of_module:

=C2=A0 /* Unintern `module-environments' because it is only used
=C2=A0 =C2=A0 =C2=A0internally. */
=C2=A0 Funintern (Qmodule_environments, Qnil);

What if some Lisp defines a (interned) symbol by that name?=C2=A0 Won't=
they clash?

I followed the lead of inte= rnal-interpreter-environment in eval.c, which uses the same pattern.
<= div>=C2=A0

The Windows-specific parts of dynlib.c need work, e.g. you cannot pass
UTF-8 encoded file names to Windows APIs.=C2=A0 And there are some other issues.=C2=A0 I'll take care of that.

I can also do that, I should be able to set up a build environment in a = VM.
=C2=A0

About the tests: The Makefile in mod-test is Unix-specific: it uses a
literal .so extension.=C2=A0 I also think the Python script should be
rewritten in Emacs Lisp, so that Python installation is not required.
Finally, all of the module tests and associated files should be moved
into test/, preferably even test/automated/ and made part of the "make=
check" run.


--001a114450c81561190524ec75f0--