Re: Dynamic modules: MODULE_HANDLE_SIGNALS etc.

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

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

Philipp Stephani <p.stephani2@gmail.com> schrieb am Sa., 28. Nov. 2015 um
11:58 Uhr:

> 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.
>

Here's another patch that replaces some of the custom error handling with
signals. The signals will be immediately caught by the prologue, but some
complexity and duplication is removed.

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

[-- Attachment #2: 0001-Use-standard-checks-whenever-possible.patch --]
[-- Type: application/octet-stream, Size: 9553 bytes --]

From 4d4b6516001163eb72cbcfec480ba878829f668d Mon Sep 17 00:00:00 2001
From: Philipp Stephani <phst@google.com>
Date: Sat, 19 Dec 2015 22:00:51 +0100
Subject: [PATCH] Use standard checks whenever possible.

This is possible in all functions where we catch signals anyway.

* emacs-module.c (module_make_global_ref, module_funcall)
(module_copy_string_contents, module_make_string): Use xsignal0
and CHECK macros for argument checks.
---
 src/emacs-module.c | 123 +++++++++--------------------------------------------
 1 file changed, 21 insertions(+), 102 deletions(-)

diff --git a/src/emacs-module.c b/src/emacs-module.c
index ee97644..28b73a5 100644
--- a/src/emacs-module.c
+++ b/src/emacs-module.c
@@ -102,14 +102,12 @@ static enum emacs_funcall_exit module_non_local_exit_check (emacs_env *);
 static void check_main_thread (void);
 static void finalize_environment (struct emacs_env_private *);
 static void initialize_environment (emacs_env *, struct emacs_env_private *priv);
-static void module_args_out_of_range (emacs_env *, Lisp_Object, Lisp_Object);
 static void module_handle_signal (emacs_env *, Lisp_Object);
 static void module_handle_throw (emacs_env *, Lisp_Object);
 static void module_non_local_exit_signal_1 (emacs_env *, Lisp_Object, Lisp_Object);
 static void module_non_local_exit_throw_1 (emacs_env *, Lisp_Object, Lisp_Object);
 static void module_out_of_memory (emacs_env *);
 static void module_reset_handlerlist (const int *);
-static void module_wrong_type (emacs_env *, Lisp_Object, Lisp_Object);
 
 /* We used to return NULL when emacs_value was a different type from
    Lisp_Object, but nowadays we just use Qnil instead.  Although they
@@ -261,12 +259,9 @@ module_make_global_ref (emacs_env *env, emacs_value ref)
   if (i >= 0)
     {
       Lisp_Object value = HASH_VALUE (h, i);
+      verify (EMACS_INT_MAX > MOST_POSITIVE_FIXNUM);
       EMACS_INT refcount = XFASTINT (value) + 1;
-      if (refcount > MOST_POSITIVE_FIXNUM)
-        {
-          module_non_local_exit_signal_1 (env, Qoverflow_error, Qnil);
-          return module_nil;
-        }
+      if (FIXNUM_OVERFLOW_P (refcount)) xsignal0 (Qoverflow_error);
       value = make_natnum (refcount);
       set_hash_value_slot (h, i, value);
     }
@@ -405,6 +400,7 @@ module_funcall (emacs_env *env, emacs_value fun, ptrdiff_t nargs,
      first arg, because that's what Ffuncall takes.  */
   Lisp_Object *newargs;
   USE_SAFE_ALLOCA;
+  if (nargs == PTRDIFF_MAX) xsignal0 (Qoverflow_error);
   SAFE_ALLOCA_LISP (newargs, nargs + 1);
   newargs[0] = value_to_lisp (fun);
   for (ptrdiff_t i = 0; i < nargs; i++)
@@ -451,11 +447,7 @@ module_extract_integer (emacs_env *env, emacs_value n)
 {
   MODULE_FUNCTION_BEGIN (0);
   Lisp_Object l = value_to_lisp (n);
-  if (! INTEGERP (l))
-    {
-      module_wrong_type (env, Qintegerp, l);
-      return 0;
-    }
+  CHECK_NUMBER (l);
   return XINT (l);
 }
 
@@ -463,11 +455,7 @@ static emacs_value
 module_make_integer (emacs_env *env, intmax_t n)
 {
   MODULE_FUNCTION_BEGIN (module_nil);
-  if (! (MOST_NEGATIVE_FIXNUM <= n && n <= MOST_POSITIVE_FIXNUM))
-    {
-      module_non_local_exit_signal_1 (env, Qoverflow_error, Qnil);
-      return module_nil;
-    }
+  if (FIXNUM_OVERFLOW_P (n)) xsignal0 (Qoverflow_error);
   return lisp_to_value (make_number (n));
 }
 
@@ -476,11 +464,7 @@ module_extract_float (emacs_env *env, emacs_value f)
 {
   MODULE_FUNCTION_BEGIN (0);
   Lisp_Object lisp = value_to_lisp (f);
-  if (! FLOATP (lisp))
-    {
-      module_wrong_type (env, Qfloatp, lisp);
-      return 0;
-    }
+  CHECK_TYPE (FLOATP (lisp), Qfloatp, lisp);
   return XFLOAT_DATA (lisp);
 }
 
@@ -497,19 +481,11 @@ module_copy_string_contents (emacs_env *env, emacs_value value, char *buffer,
 {
   MODULE_FUNCTION_BEGIN (false);
   Lisp_Object lisp_str = value_to_lisp (value);
-  if (! STRINGP (lisp_str))
-    {
-      module_wrong_type (env, Qstringp, lisp_str);
-      return false;
-    }
+  CHECK_STRING (lisp_str);
 
   Lisp_Object lisp_str_utf8 = ENCODE_UTF_8 (lisp_str);
   ptrdiff_t raw_size = SBYTES (lisp_str_utf8);
-  if (raw_size == PTRDIFF_MAX)
-    {
-      module_non_local_exit_signal_1 (env, Qoverflow_error, Qnil);
-      return false;
-    }
+  if (raw_size == PTRDIFF_MAX) xsignal0 (Qoverflow_error);
   ptrdiff_t required_buf_size = raw_size + 1;
 
   eassert (length != NULL);
@@ -525,8 +501,7 @@ module_copy_string_contents (emacs_env *env, emacs_value value, char *buffer,
   if (*length < required_buf_size)
     {
       *length = required_buf_size;
-      module_non_local_exit_signal_1 (env, Qargs_out_of_range, Qnil);
-      return false;
+      xsignal0 (Qargs_out_of_range);
     }
 
   *length = required_buf_size;
@@ -539,11 +514,7 @@ static emacs_value
 module_make_string (emacs_env *env, const char *str, ptrdiff_t length)
 {
   MODULE_FUNCTION_BEGIN (module_nil);
-  if (length > STRING_BYTES_BOUND)
-    {
-      module_non_local_exit_signal_1 (env, Qoverflow_error, Qnil);
-      return module_nil;
-    }
+  if (length > STRING_BYTES_BOUND) xsignal0 (Qoverflow_error);
   Lisp_Object lstr = make_unibyte_string (str, length);
   return lisp_to_value (code_convert_string_norecord (lstr, Qutf_8, false));
 }
@@ -560,11 +531,7 @@ module_get_user_ptr (emacs_env *env, emacs_value uptr)
 {
   MODULE_FUNCTION_BEGIN (NULL);
   Lisp_Object lisp = value_to_lisp (uptr);
-  if (! USER_PTRP (lisp))
-    {
-      module_wrong_type (env, Quser_ptr, lisp);
-      return NULL;
-    }
+  CHECK_TYPE (USER_PTRP (lisp), Quser_ptrp, lisp);
   return XUSER_PTR (lisp)->p;
 }
 
@@ -573,12 +540,8 @@ 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;
   Lisp_Object lisp = value_to_lisp (uptr);
-  if (! USER_PTRP (lisp))
-    module_wrong_type (env, Quser_ptr, lisp);
+  CHECK_TYPE (USER_PTRP (lisp), Quser_ptrp, lisp);
   XUSER_PTR (lisp)->p = ptr;
 }
 
@@ -587,11 +550,7 @@ module_get_user_finalizer (emacs_env *env, emacs_value uptr)
 {
   MODULE_FUNCTION_BEGIN (NULL);
   Lisp_Object lisp = value_to_lisp (uptr);
-  if (! USER_PTRP (lisp))
-    {
-      module_wrong_type (env, Quser_ptr, lisp);
-      return NULL;
-    }
+  CHECK_TYPE (USER_PTRP (lisp), Quser_ptrp, lisp);
   return XUSER_PTR (lisp)->finalizer;
 }
 
@@ -602,8 +561,7 @@ module_set_user_finalizer (emacs_env *env, emacs_value uptr,
   /* 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);
+  CHECK_TYPE (USER_PTRP (lisp), Quser_ptrp, lisp);
   XUSER_PTR (lisp)->finalizer = fin;
 }
 
@@ -613,19 +571,9 @@ module_vec_set (emacs_env *env, emacs_value vec, ptrdiff_t i, emacs_value val)
   /* FIXME: This function should return bool because it can fail.  */
   MODULE_FUNCTION_BEGIN ();
   Lisp_Object lvec = value_to_lisp (vec);
-  if (! VECTORP (lvec))
-    {
-      module_wrong_type (env, Qvectorp, lvec);
-      return;
-    }
-  if (! (0 <= i && i < ASIZE (lvec)))
-    {
-      if (MOST_NEGATIVE_FIXNUM <= i && i <= MOST_POSITIVE_FIXNUM)
-	module_args_out_of_range (env, lvec, make_number (i));
-      else
-	module_non_local_exit_signal_1 (env, Qoverflow_error, Qnil);
-      return;
-    }
+  CHECK_VECTOR (lvec);
+  if (FIXNUM_OVERFLOW_P (i)) xsignal0 (Qoverflow_error);
+  CHECK_RANGED_INTEGER (make_number (i), 0, ASIZE (lvec) - 1);
   ASET (lvec, i, value_to_lisp (val));
 }
 
@@ -634,19 +582,9 @@ module_vec_get (emacs_env *env, emacs_value vec, ptrdiff_t i)
 {
   MODULE_FUNCTION_BEGIN (module_nil);
   Lisp_Object lvec = value_to_lisp (vec);
-  if (! VECTORP (lvec))
-    {
-      module_wrong_type (env, Qvectorp, lvec);
-      return module_nil;
-    }
-  if (! (0 <= i && i < ASIZE (lvec)))
-    {
-      if (MOST_NEGATIVE_FIXNUM <= i && i <= MOST_POSITIVE_FIXNUM)
-	module_args_out_of_range (env, lvec, make_number (i));
-      else
-	module_non_local_exit_signal_1 (env, Qoverflow_error, Qnil);
-      return module_nil;
-    }
+  CHECK_VECTOR (lvec);
+  if (FIXNUM_OVERFLOW_P (i)) xsignal0 (Qoverflow_error);
+  CHECK_RANGED_INTEGER (make_number (i), 0, ASIZE (lvec) - 1);
   return lisp_to_value (AREF (lvec, i));
 }
 
@@ -656,11 +594,7 @@ module_vec_size (emacs_env *env, emacs_value vec)
   /* FIXME: Return a sentinel value (e.g., -1) on error.  */
   MODULE_FUNCTION_BEGIN (0);
   Lisp_Object lvec = value_to_lisp (vec);
-  if (! VECTORP (lvec))
-    {
-      module_wrong_type (env, Qvectorp, lvec);
-      return 0;
-    }
+  CHECK_VECTOR (lvec);
   return ASIZE (lvec);
 }
 
@@ -819,14 +753,6 @@ module_non_local_exit_throw_1 (emacs_env *env, Lisp_Object tag,
     }
 }
 
-/* Module version of `wrong_type_argument'.  */
-static void
-module_wrong_type (emacs_env *env, Lisp_Object predicate, Lisp_Object value)
-{
-  module_non_local_exit_signal_1 (env, Qwrong_type_argument,
-				  list2 (predicate, value));
-}
-
 /* Signal an out-of-memory condition to the caller.  */
 static void
 module_out_of_memory (emacs_env *env)
@@ -837,13 +763,6 @@ module_out_of_memory (emacs_env *env)
 				  XCDR (Vmemory_signal_data));
 }
 
-/* Signal arguments are out of range.  */
-static void
-module_args_out_of_range (emacs_env *env, Lisp_Object a1, Lisp_Object a2)
-{
-  module_non_local_exit_signal_1 (env, Qargs_out_of_range, list2 (a1, a2));
-}
-
 \f
 /* Value conversion.  */
 
-- 
2.6.3