Re: Dynamic modules: MODULE_HANDLE_SIGNALS etc.

[Philipp Stephani <p.stephani2@gmail.com>]

[-- Attachment #1.1: Type: text/plain, Size: 2026 bytes --]

Philipp Stephani <p.stephani2@gmail.com> schrieb am Fr., 27. Nov. 2015 um
20:19 Uhr:

> Eli Zaretskii <eliz@gnu.org> schrieb am Fr., 27. Nov. 2015 um 08:36 Uhr:
>
>> > Cc: emacs-devel@gnu.org
>> > From: Paul Eggert <eggert@cs.ucla.edu>
>> > Date: Thu, 26 Nov 2015 13:29:49 -0800
>> >
>> > Eli Zaretskii wrote:
>> > > it would be a maintenance burden to have to
>> > >      analyze upon each such change whether emacs-module.c needs some
>> > >      augmentation.
>> >
>> > While that's true in general, I think some exceptions are OK.  E.g.,
>> it's OK if
>> > emacs-module.c assumes that ASIZE is a simple access function or macro
>> that
>> > doesn't throw signals.  If we actually changed ASIZE to throw signals,
>> there's a
>> > boatload of other code we'd need to change as well, and changing
>> emacs-module.c
>> > wouldn't add much more to the maintenance burden.
>>
>> So what are the rules here, exactly?  I'd like to write them down in
>> the commentary to emacs-module.c, so that any future changes there
>> will have lower probability of breaking things.
>>
>> E.g., can make_number signal an error?  What about make_float or
>> make_string?  And what about accessors like XFLOAT_DATA or AREF?
>>
>>
> Are there any established rules? If not we should probably be conservative
> and  assume that everything signals. If we figure out that this introduces
> an unacceptably high overhead in some situations we can reconsider later.
> I would propose three exceptions: free_global_ref, is_not_nil, eq.
> free_global_ref cannot fail in Daniel's design, and implementing it that
> way would be consistent with other resource deallocation functions such as
> free(3). is_not_nil and eq seem so fundamental that I cannot imagine a
> situation where they could ever fail. Documenting that these three cannot
> fail would free module authors from the need to check for errors after
> calling these functions.
>

For now I've attached a patch to replace the initial setup of most
environment functions with a single macro.

[-- Attachment #1.2: Type: text/html, Size: 2863 bytes --]

[-- Attachment #2: 0001-Create-and-use-a-macro-MODULE_FUNCTION_BEGIN.patch --]
[-- Type: application/octet-stream, Size: 13021 bytes --]

From 1c30db558708575635905c48ae2c6fc5649f8ea8 Mon Sep 17 00:00:00 2001
From: Philipp Stephani <phst@google.com>
Date: Sat, 28 Nov 2015 11:55:02 +0100
Subject: [PATCH] Create and use a macro MODULE_FUNCTION_BEGIN.

This replaces the initial boilerplate for most environment functions.

* emacs-module.c (module_make_global_ref)
(module_free_global_ref, module_make_function, module_funcall)
(module_intern, module_type_of, module_extract_integer)
(module_make_integer, module_extract_float, module_make_float)
(module_copy_string_contents, module_make_string)
(module_make_user_ptr, module_get_user_ptr, module_set_user_ptr)
(module_get_user_finalizer, module_set_user_finalizer)
(module_vec_set, module_vec_get, module_vec_size): Use new helper
macro MODULE_FUNCTION_BEGIN.
---
 src/emacs-module.c | 132 +++++++++++++++++++----------------------------------
 1 file changed, 46 insertions(+), 86 deletions(-)

diff --git a/src/emacs-module.c b/src/emacs-module.c
index 3686470..fc77765 100644
--- a/src/emacs-module.c
+++ b/src/emacs-module.c
@@ -156,24 +156,14 @@ static void module_wrong_type (emacs_env *, Lisp_Object, Lisp_Object);
    passing information to the handler functions.  */
 
 /* Place this macro at the beginning of a function returning a number
-   or a pointer to handle signals.  The function must have an ENV
-   parameter.  The function will return 0 (or NULL) if a signal is
-   caught.  */
-#define MODULE_HANDLE_SIGNALS MODULE_HANDLE_SIGNALS_RETURN (0)
-
-/* Place this macro at the beginning of a function returning void to
-   handle signals.  The function must have an ENV parameter.  */
-#define MODULE_HANDLE_SIGNALS_VOID MODULE_HANDLE_SIGNALS_RETURN ()
-
-#define MODULE_HANDLE_SIGNALS_RETURN(retval)                                   \
-  MODULE_SETJMP (CONDITION_CASE, module_handle_signal, retval)
-
-/* Place this macro at the beginning of a function returning a pointer
-   to handle non-local exits via `throw'.  The function must have an
-   ENV parameter.  The function will return NULL if a `throw' is
-   caught.  */
-#define MODULE_HANDLE_THROW                                                    \
-  MODULE_SETJMP (CATCHER_ALL, module_handle_throw, NULL)
+   or a pointer to handle non-local exits.  The function must have an
+   ENV parameter.  The function will return the specified value if a
+   signal or throw is caught.  */
+// TODO: Have Fsignal check for CATCHER_ALL so we only have to install
+// one handler.
+#define MODULE_HANDLE_NONLOCAL_EXIT(retval)                     \
+  MODULE_SETJMP (CONDITION_CASE, module_handle_signal, retval); \
+  MODULE_SETJMP (CATCHER_ALL, module_handle_throw, retval)
 
 #define MODULE_SETJMP(handlertype, handlerfunc, retval)			       \
   MODULE_SETJMP_1 (handlertype, handlerfunc, retval,			       \
@@ -190,6 +180,8 @@ static void module_wrong_type (emacs_env *, Lisp_Object, Lisp_Object);
    code after the macro may longjmp back into the macro, which means
    its local variable C must stay live in later code.  */
 
+// TODO: Make backtraces work if this macros is used.
+
 #define MODULE_SETJMP_1(handlertype, handlerfunc, retval, c, dummy)	\
   if (module_non_local_exit_check (env) != emacs_funcall_exit_return)	\
     return retval;							\
@@ -252,8 +244,8 @@ static Lisp_Object module_call_func;
    4. Any function that needs to call Emacs facilities, such as
       encoding or decoding functions, or 'intern', or 'make_string',
       should protect itself from signals and 'throw' in the called
-      Emacs functions, by placing the macros MODULE_HANDLE_SIGNALS
-      and/or MODULE_HANDLE_THROW right after the above 2 tests.
+      Emacs functions, by placing the macro
+      MODULE_HANDLE_NONLOCAL_EXIT right after the above 2 tests.
 
    5. Do NOT use 'eassert' for checking validity of user code in the
       module.  Instead, make those checks part of the code, and if the
@@ -263,6 +255,16 @@ static Lisp_Object module_call_func;
       instead of reporting the error back to Lisp, and also because
       'eassert' is compiled to nothing in the release version.  */
 
+/* Use MODULE_FUNCTION_BEGIN to implement steps 2 through 4 for most
+   environment functions.  On error it will return its argument, which
+   should be a sentinel value.  */
+
+#define MODULE_FUNCTION_BEGIN(error_retval)                             \
+  check_main_thread ();                                                 \
+  if (module_non_local_exit_check (env) != emacs_funcall_exit_return)   \
+    return error_retval;                                                \
+  MODULE_HANDLE_NONLOCAL_EXIT (error_retval)
+
 /* Catch signals and throws only if the code can actually signal or
    throw.  If checking is enabled, abort if the current thread is not
    the Emacs main thread.  */
@@ -280,10 +282,7 @@ module_get_environment (struct emacs_runtime *ert)
 static emacs_value
 module_make_global_ref (emacs_env *env, emacs_value ref)
 {
-  check_main_thread ();
-  if (module_non_local_exit_check (env) != emacs_funcall_exit_return)
-    return NULL;
-  MODULE_HANDLE_SIGNALS;
+  MODULE_FUNCTION_BEGIN (NULL);
   struct Lisp_Hash_Table *h = XHASH_TABLE (Vmodule_refs_hash);
   Lisp_Object new_obj = value_to_lisp (ref);
   EMACS_UINT hashcode;
@@ -312,13 +311,10 @@ module_make_global_ref (emacs_env *env, emacs_value ref)
 static void
 module_free_global_ref (emacs_env *env, emacs_value ref)
 {
-  check_main_thread ();
-  if (module_non_local_exit_check (env) != emacs_funcall_exit_return)
-    return;
   /* TODO: This probably never signals.  */
   /* FIXME: Wait a minute.  Shouldn't this function report an error if
      the hash lookup fails?  */
-  MODULE_HANDLE_SIGNALS_VOID;
+  MODULE_FUNCTION_BEGIN ();
   struct Lisp_Hash_Table *h = XHASH_TABLE (Vmodule_refs_hash);
   Lisp_Object obj = value_to_lisp (ref);
   EMACS_UINT hashcode;
@@ -396,10 +392,7 @@ module_make_function (emacs_env *env, ptrdiff_t min_arity, ptrdiff_t max_arity,
 		      emacs_subr subr, const char *documentation,
 		      void *data)
 {
-  check_main_thread ();
-  if (module_non_local_exit_check (env) != emacs_funcall_exit_return)
-    return NULL;
-  MODULE_HANDLE_SIGNALS;
+  MODULE_FUNCTION_BEGIN (NULL);
 
   if (! (0 <= min_arity
 	 && (max_arity < 0
@@ -434,11 +427,7 @@ static emacs_value
 module_funcall (emacs_env *env, emacs_value fun, ptrdiff_t nargs,
 		emacs_value args[])
 {
-  check_main_thread ();
-  if (module_non_local_exit_check (env) != emacs_funcall_exit_return)
-    return NULL;
-  MODULE_HANDLE_SIGNALS;
-  MODULE_HANDLE_THROW;
+  MODULE_FUNCTION_BEGIN (NULL);
 
   /* Make a new Lisp_Object array starting with the function as the
      first arg, because that's what Ffuncall takes.  */
@@ -456,19 +445,14 @@ module_funcall (emacs_env *env, emacs_value fun, ptrdiff_t nargs,
 static emacs_value
 module_intern (emacs_env *env, const char *name)
 {
-  check_main_thread ();
-  if (module_non_local_exit_check (env) != emacs_funcall_exit_return)
-    return NULL;
-  MODULE_HANDLE_SIGNALS;
+  MODULE_FUNCTION_BEGIN (NULL);
   return lisp_to_value (env, intern (name));
 }
 
 static emacs_value
 module_type_of (emacs_env *env, emacs_value value)
 {
-  check_main_thread ();
-  if (module_non_local_exit_check (env) != emacs_funcall_exit_return)
-    return NULL;
+  MODULE_FUNCTION_BEGIN (NULL);
   return lisp_to_value (env, Ftype_of (value_to_lisp (value)));
 }
 
@@ -493,9 +477,7 @@ module_eq (emacs_env *env, emacs_value a, emacs_value b)
 static intmax_t
 module_extract_integer (emacs_env *env, emacs_value n)
 {
-  check_main_thread ();
-  if (module_non_local_exit_check (env) != emacs_funcall_exit_return)
-    return 0;
+  MODULE_FUNCTION_BEGIN (0);
   Lisp_Object l = value_to_lisp (n);
   if (! INTEGERP (l))
     {
@@ -508,9 +490,7 @@ module_extract_integer (emacs_env *env, emacs_value n)
 static emacs_value
 module_make_integer (emacs_env *env, intmax_t n)
 {
-  check_main_thread ();
-  if (module_non_local_exit_check (env) != emacs_funcall_exit_return)
-    return NULL;
+  MODULE_FUNCTION_BEGIN (NULL);
   if (! (MOST_NEGATIVE_FIXNUM <= n && n <= MOST_POSITIVE_FIXNUM))
     {
       module_non_local_exit_signal_1 (env, Qoverflow_error, Qnil);
@@ -522,9 +502,7 @@ module_make_integer (emacs_env *env, intmax_t n)
 static double
 module_extract_float (emacs_env *env, emacs_value f)
 {
-  check_main_thread ();
-  if (module_non_local_exit_check (env) != emacs_funcall_exit_return)
-    return 0;
+  MODULE_FUNCTION_BEGIN (0);
   Lisp_Object lisp = value_to_lisp (f);
   if (! FLOATP (lisp))
     {
@@ -537,10 +515,7 @@ module_extract_float (emacs_env *env, emacs_value f)
 static emacs_value
 module_make_float (emacs_env *env, double d)
 {
-  check_main_thread ();
-  if (module_non_local_exit_check (env) != emacs_funcall_exit_return)
-    return NULL;
-  MODULE_HANDLE_SIGNALS;
+  MODULE_FUNCTION_BEGIN (NULL);
   return lisp_to_value (env, make_float (d));
 }
 
@@ -548,10 +523,7 @@ static bool
 module_copy_string_contents (emacs_env *env, emacs_value value, char *buffer,
 			     ptrdiff_t *length)
 {
-  check_main_thread ();
-  if (module_non_local_exit_check (env) != emacs_funcall_exit_return)
-    return false;
-  MODULE_HANDLE_SIGNALS;
+  MODULE_FUNCTION_BEGIN (false);
   Lisp_Object lisp_str = value_to_lisp (value);
   if (! STRINGP (lisp_str))
     {
@@ -594,10 +566,7 @@ module_copy_string_contents (emacs_env *env, emacs_value value, char *buffer,
 static emacs_value
 module_make_string (emacs_env *env, const char *str, ptrdiff_t length)
 {
-  check_main_thread ();
-  if (module_non_local_exit_check (env) != emacs_funcall_exit_return)
-    return NULL;
-  MODULE_HANDLE_SIGNALS;
+  MODULE_FUNCTION_BEGIN (NULL);
   if (length > STRING_BYTES_BOUND)
     {
       module_non_local_exit_signal_1 (env, Qoverflow_error, Qnil);
@@ -611,18 +580,14 @@ module_make_string (emacs_env *env, const char *str, ptrdiff_t length)
 static emacs_value
 module_make_user_ptr (emacs_env *env, emacs_finalizer_function fin, void *ptr)
 {
-  check_main_thread ();
-  if (module_non_local_exit_check (env) != emacs_funcall_exit_return)
-    return NULL;
+  MODULE_FUNCTION_BEGIN (NULL);
   return lisp_to_value (env, make_user_ptr (fin, ptr));
 }
 
 static void *
 module_get_user_ptr (emacs_env *env, emacs_value uptr)
 {
-  check_main_thread ();
-  if (module_non_local_exit_check (env) != emacs_funcall_exit_return)
-    return NULL;
+  MODULE_FUNCTION_BEGIN (NULL);
   Lisp_Object lisp = value_to_lisp (uptr);
   if (! USER_PTRP (lisp))
     {
@@ -635,6 +600,8 @@ module_get_user_ptr (emacs_env *env, emacs_value uptr)
 static void
 module_set_user_ptr (emacs_env *env, emacs_value uptr, void *ptr)
 {
+  // FIXME: This function should return bool because it can fail.
+  MODULE_FUNCTION_BEGIN ();
   check_main_thread ();
   if (module_non_local_exit_check (env) != emacs_funcall_exit_return)
     return;
@@ -647,9 +614,7 @@ module_set_user_ptr (emacs_env *env, emacs_value uptr, void *ptr)
 static emacs_finalizer_function
 module_get_user_finalizer (emacs_env *env, emacs_value uptr)
 {
-  check_main_thread ();
-  if (module_non_local_exit_check (env) != emacs_funcall_exit_return)
-    return NULL;
+  MODULE_FUNCTION_BEGIN (NULL);
   Lisp_Object lisp = value_to_lisp (uptr);
   if (! USER_PTRP (lisp))
     {
@@ -663,9 +628,8 @@ static void
 module_set_user_finalizer (emacs_env *env, emacs_value uptr,
 			   emacs_finalizer_function fin)
 {
-  check_main_thread ();
-  if (module_non_local_exit_check (env) != emacs_funcall_exit_return)
-    return;
+  // FIXME: This function should return bool because it can fail.
+  MODULE_FUNCTION_BEGIN ();
   Lisp_Object lisp = value_to_lisp (uptr);
   if (! USER_PTRP (lisp))
     module_wrong_type (env, Quser_ptr, lisp);
@@ -675,9 +639,8 @@ module_set_user_finalizer (emacs_env *env, emacs_value uptr,
 static void
 module_vec_set (emacs_env *env, emacs_value vec, ptrdiff_t i, emacs_value val)
 {
-  check_main_thread ();
-  if (module_non_local_exit_check (env) != emacs_funcall_exit_return)
-    return;
+  // FIXME: This function should return bool because it can fail.
+  MODULE_FUNCTION_BEGIN ();
   Lisp_Object lvec = value_to_lisp (vec);
   if (! VECTORP (lvec))
     {
@@ -698,9 +661,7 @@ module_vec_set (emacs_env *env, emacs_value vec, ptrdiff_t i, emacs_value val)
 static emacs_value
 module_vec_get (emacs_env *env, emacs_value vec, ptrdiff_t i)
 {
-  check_main_thread ();
-  if (module_non_local_exit_check (env) != emacs_funcall_exit_return)
-    return NULL;
+  MODULE_FUNCTION_BEGIN (NULL);
   Lisp_Object lvec = value_to_lisp (vec);
   if (! VECTORP (lvec))
     {
@@ -721,9 +682,8 @@ module_vec_get (emacs_env *env, emacs_value vec, ptrdiff_t i)
 static ptrdiff_t
 module_vec_size (emacs_env *env, emacs_value vec)
 {
-  check_main_thread ();
-  if (module_non_local_exit_check (env) != emacs_funcall_exit_return)
-    return 0;
+  // FIXME: Return a sentinel value (e.g., -1) on error.
+  MODULE_FUNCTION_BEGIN (0);
   Lisp_Object lvec = value_to_lisp (vec);
   if (! VECTORP (lvec))
     {
-- 
2.6.3