More over-engineering

More over-engineering

[Stefan Monnier]

What's up with allocate_emacs_value in emacs-modules.c?

Are we really allocating a structure for every Lisp_Object value we pass
through the modules API?  Why do that?

The GC will find all Lisp_Object values that are stored in other Lisp
object or on the stack, so there are rather few remaining cases where
a Lisp_Object value has to be protected from GC.  For those remaining
cases, we do want to provide a way for the C code to "pin" the object
(we call it "gc-protect" them in Emacs's C code), but looking at the 
emacs-modules.c is looks like we don't even do that because those
emacs_value objects are auto-reclaimed when upon return, so they only
"protect" data whose lifetime doesn't escape the current module call
(and most of those cases correspond to having a Lisp_Object on the
stack, so there's no need for GC protection anyway).


        Stefan

Re: More over-engineering

[Aurélien Aptel]

On Fri, Nov 27, 2015 at 6:00 PM, Stefan Monnier
<monnier@iro.umontreal.ca> wrote:
> Are we really allocating a structure for every Lisp_Object value we pass
> through the modules API?  Why do that?

Look at the old thread for Philip's arguments. As far as I'm
concerned, we need an indirect pointer to support wide-int
Lisp_Objects on 32bit platforms. Global reference can still be made
with the current code. We have a reference counting hash-table to keep
track of them. Non-global refs are marked explicitely in case the GC
misses them.

Re: More over-engineering

[David Kastrup]

Aurélien Aptel <aurelien.aptel+emacs@gmail.com> writes:

> On Fri, Nov 27, 2015 at 6:00 PM, Stefan Monnier
> <monnier@iro.umontreal.ca> wrote:
>> Are we really allocating a structure for every Lisp_Object value we pass
>> through the modules API?  Why do that?
>
> Look at the old thread for Philip's arguments. As far as I'm
> concerned, we need an indirect pointer to support wide-int
> Lisp_Objects on 32bit platforms. Global reference can still be made
> with the current code. We have a reference counting hash-table to keep
> track of them. Non-global refs are marked explicitely in case the GC
> misses them.

If the same modules cannot be used at the same time for wide-int and
normal int Emacs compilations (can they?), then it would seem that
module code could become significantly simpler when declaring wide-int
and module support mutually exclusive.

-- 
David Kastrup

Re: More over-engineering

[Stefan Monnier]

>> Are we really allocating a structure for every Lisp_Object value we pass
>> through the modules API?  Why do that?
> Look at the old thread for Philip's arguments.  As far as I'm
> concerned, we need an indirect pointer to support wide-int
> Lisp_Objects on 32bit platforms.

We don't need it in core so we don't need it in the modules code either.

The only advantage in this respect is that the representation of
Lisp_Object is encapsulated inside Emacs core, so the same module can be
used with a "emacs-32bit-narrow-int" and on "emacs-32bit-wide-int".
The cost for that minor advantage seems way out of proportion.

> Global reference can still be made with the current code. We have
> a reference counting hash-table to keep track of them.

Oh, I see the "make_global_ref" now, thanks.

> Non-global refs are marked explicitely in case the GC misses them.

Why would the GC miss them?


        Stefan


PS: To give an idea of the cost of this, the patch below changes
emacs_value to be synonym of Lisp_Object (well, it doesn't do it right:
it's a quick&dirty patch for now).


diff --git a/modules/mod-test/mod-test.c b/modules/mod-test/mod-test.c
index b0c535c..79f0ae7 100644
--- a/modules/mod-test/mod-test.c
+++ b/modules/mod-test/mod-test.c
@@ -61,7 +61,7 @@ Fmod_test_signal (emacs_env *env, ptrdiff_t nargs, emacs_value args[],
   assert (env->non_local_exit_check (env) == emacs_funcall_exit_return);
   env->non_local_exit_signal (env, env->intern (env, "error"),
 			      env->make_integer (env, 56));
-  return NULL;
+  return 0/* FIXME!  NULL */;
 }
 
 
@@ -73,7 +73,7 @@ Fmod_test_throw (emacs_env *env, ptrdiff_t nargs, emacs_value args[],
   assert (env->non_local_exit_check (env) == emacs_funcall_exit_return);
   env->non_local_exit_throw (env, env->intern (env, "tag"),
 			     env->make_integer (env, 65));
-  return NULL;
+  return 0/* FIXME!  NULL */;
 }
 
 
diff --git a/src/emacs-module.c b/src/emacs-module.c
index c75ddeb..bf5178e 100644
--- a/src/emacs-module.c
+++ b/src/emacs-module.c
@@ -59,10 +59,6 @@ static DWORD main_thread;
 \f
 /* Memory management.  */
 
-/* An `emacs_value' is just a pointer to a structure holding an
-   internal Lisp object.  */
-struct emacs_value_tag { Lisp_Object v; };
-
 /* Local value objects use a simple fixed-sized block allocation
    scheme without explicit deallocation.  All local values are
    deallocated when the lifetime of their environment ends.  Keep
@@ -71,28 +67,6 @@ struct emacs_value_tag { Lisp_Object v; };
 
 enum { value_frame_size = 512 };
 
-/* A block from which `emacs_value' object can be allocated.  */
-struct emacs_value_frame
-{
-  /* Storage for values.  */
-  struct emacs_value_tag objects[value_frame_size];
-
-  /* Index of the next free value in `objects'.  */
-  int offset;
-
-  /* Pointer to next frame, if any.  */
-  struct emacs_value_frame *next;
-};
-
-/* A structure that holds an initial frame (so that the first local
-   values require no dynamic allocation) and keeps track of the
-   current frame.  */
-static struct emacs_value_storage
-{
-  struct emacs_value_frame initial;
-  struct emacs_value_frame *current;
-} global_storage;
-
 \f
 /* Private runtime and environment members.  */
 
@@ -106,9 +80,7 @@ struct emacs_env_private
   /* Dedicated storage for non-local exit symbol and data so that
      storage is always available for them, even in an out-of-memory
      situation.  */
-  struct emacs_value_tag non_local_exit_symbol, non_local_exit_data;
-
-  struct emacs_value_storage storage;
+  Lisp_Object non_local_exit_symbol, non_local_exit_data;
 };
 
 /* Combine public and private parts in one structure.  This structure
@@ -134,8 +106,7 @@ struct module_fun_env;
 
 static Lisp_Object module_format_fun_env (const struct module_fun_env *);
 static Lisp_Object value_to_lisp (emacs_value);
-static emacs_value allocate_emacs_value (emacs_env *, struct emacs_value_storage *, Lisp_Object);
-static emacs_value lisp_to_value (emacs_env *, Lisp_Object);
+static emacs_value lisp_to_value (Lisp_Object);
 static enum emacs_funcall_exit module_non_local_exit_check (emacs_env *);
 static void check_main_thread (void);
 static void finalize_environment (struct env_storage *);
@@ -149,6 +120,7 @@ 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);
 
+#define mod_NULL (lisp_to_value (Qnil))
 \f
 /* Convenience macros for non-local exit handling.  */
 
@@ -180,7 +152,7 @@ static void module_wrong_type (emacs_env *, Lisp_Object, Lisp_Object);
    ENV parameter.  The function will return NULL if a `throw' is
    caught.  */
 #define MODULE_HANDLE_THROW                                                    \
-  MODULE_SETJMP (CATCHER_ALL, module_handle_throw, NULL)
+  MODULE_SETJMP (CATCHER_ALL, module_handle_throw, mod_NULL)
 
 #define MODULE_SETJMP(handlertype, handlerfunc, retval)			       \
   MODULE_SETJMP_1 (handlertype, handlerfunc, retval,			       \
@@ -261,6 +233,8 @@ static Lisp_Object module_call_func;
       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.
+   FIXME: Why is it always needed?  What happens if we don't?
+          Why is only one of the two needed rather than always both?
 
    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
@@ -289,7 +263,7 @@ 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;
+    return mod_NULL;
   MODULE_HANDLE_SIGNALS;
   struct Lisp_Hash_Table *h = XHASH_TABLE (Vmodule_refs_hash);
   Lisp_Object new_obj = value_to_lisp (ref);
@@ -303,7 +277,7 @@ module_make_global_ref (emacs_env *env, emacs_value ref)
       if (refcount > MOST_POSITIVE_FIXNUM)
         {
           module_non_local_exit_signal_1 (env, Qoverflow_error, Qnil);
-          return NULL;
+          return mod_NULL;
         }
       value = make_natnum (refcount);
       set_hash_value_slot (h, i, value);
@@ -313,7 +287,7 @@ module_make_global_ref (emacs_env *env, emacs_value ref)
       hash_put (h, new_obj, make_natnum (1), hashcode);
     }
 
-  return allocate_emacs_value (env, &global_storage, new_obj);
+  return lisp_to_value (new_obj);
 }
 
 static void
@@ -366,8 +340,8 @@ module_non_local_exit_get (emacs_env *env, emacs_value *sym, emacs_value *data)
   struct emacs_env_private *p = env->private_members;
   if (p->pending_non_local_exit != emacs_funcall_exit_return)
     {
-      *sym = &p->non_local_exit_symbol;
-      *data = &p->non_local_exit_data;
+      *sym = lisp_to_value (p->non_local_exit_symbol);
+      *data = lisp_to_value (p->non_local_exit_data);
     }
   return p->pending_non_local_exit;
 }
@@ -404,9 +378,7 @@ module_make_function (emacs_env *env, ptrdiff_t min_arity, ptrdiff_t max_arity,
 		      void *data)
 {
   check_main_thread ();
-  if (module_non_local_exit_check (env) != emacs_funcall_exit_return)
-    return NULL;
-  MODULE_HANDLE_SIGNALS;
+  MODULE_HANDLE_SIGNALS;  /* FIXME: Why?  */
 
   if (! (0 <= min_arity
 	 && (max_arity < 0
@@ -434,7 +406,7 @@ module_make_function (emacs_env *env, ptrdiff_t min_arity, ptrdiff_t max_arity,
                                   envobj,
                                   Qargs));
 
-  return lisp_to_value (env, ret);
+  return lisp_to_value (ret);
 }
 
 static emacs_value
@@ -443,7 +415,7 @@ module_funcall (emacs_env *env, emacs_value fun, ptrdiff_t nargs,
 {
   check_main_thread ();
   if (module_non_local_exit_check (env) != emacs_funcall_exit_return)
-    return NULL;
+    return mod_NULL;
   MODULE_HANDLE_SIGNALS;
   MODULE_HANDLE_THROW;
 
@@ -455,7 +427,7 @@ module_funcall (emacs_env *env, emacs_value fun, ptrdiff_t nargs,
   newargs[0] = value_to_lisp (fun);
   for (ptrdiff_t i = 0; i < nargs; i++)
     newargs[1 + i] = value_to_lisp (args[i]);
-  emacs_value result = lisp_to_value (env, Ffuncall (nargs + 1, newargs));
+  emacs_value result = lisp_to_value (Ffuncall (nargs + 1, newargs));
   SAFE_FREE ();
   return result;
 }
@@ -465,9 +437,9 @@ module_intern (emacs_env *env, const char *name)
 {
   check_main_thread ();
   if (module_non_local_exit_check (env) != emacs_funcall_exit_return)
-    return NULL;
+    return mod_NULL;
   MODULE_HANDLE_SIGNALS;
-  return lisp_to_value (env, intern (name));
+  return lisp_to_value (intern (name));
 }
 
 static emacs_value
@@ -475,8 +447,8 @@ 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;
-  return lisp_to_value (env, Ftype_of (value_to_lisp (value)));
+    return mod_NULL;
+  return lisp_to_value (Ftype_of (value_to_lisp (value)));
 }
 
 static bool
@@ -517,13 +489,13 @@ 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;
+    return mod_NULL;
   if (! (MOST_NEGATIVE_FIXNUM <= n && n <= MOST_POSITIVE_FIXNUM))
     {
       module_non_local_exit_signal_1 (env, Qoverflow_error, Qnil);
-      return NULL;
+      return mod_NULL;
     }
-  return lisp_to_value (env, make_number (n));
+  return lisp_to_value (make_number (n));
 }
 
 static double
@@ -546,9 +518,9 @@ module_make_float (emacs_env *env, double d)
 {
   check_main_thread ();
   if (module_non_local_exit_check (env) != emacs_funcall_exit_return)
-    return NULL;
+    return mod_NULL;
   MODULE_HANDLE_SIGNALS;
-  return lisp_to_value (env, make_float (d));
+  return lisp_to_value (make_float (d));
 }
 
 static bool
@@ -603,16 +575,15 @@ 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;
+    return mod_NULL;
   MODULE_HANDLE_SIGNALS;
   if (length > STRING_BYTES_BOUND)
     {
       module_non_local_exit_signal_1 (env, Qoverflow_error, Qnil);
-      return NULL;
+      return mod_NULL;
     }
   Lisp_Object lstr = make_unibyte_string (str, length);
-  return lisp_to_value (env,
-			code_convert_string_norecord (lstr, Qutf_8, false));
+  return lisp_to_value (code_convert_string_norecord (lstr, Qutf_8, false));
 }
 
 static emacs_value
@@ -620,8 +591,8 @@ 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;
-  return lisp_to_value (env, make_user_ptr (fin, ptr));
+    return mod_NULL;
+  return lisp_to_value (make_user_ptr (fin, ptr));
 }
 
 static void *
@@ -707,12 +678,12 @@ 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;
+    return mod_NULL;
   Lisp_Object lvec = value_to_lisp (vec);
   if (! VECTORP (lvec))
     {
       module_wrong_type (env, Qvectorp, lvec);
-      return NULL;
+      return mod_NULL;
     }
   if (! (0 <= i && i < ASIZE (lvec)))
     {
@@ -720,9 +691,9 @@ module_vec_get (emacs_env *env, emacs_value vec, ptrdiff_t i)
 	module_args_out_of_range (env, lvec, make_number (i));
       else
 	module_non_local_exit_signal_1 (env, Qoverflow_error, Qnil);
-      return NULL;
+      return mod_NULL;
     }
-  return lisp_to_value (env, AREF (lvec, i));
+  return lisp_to_value (AREF (lvec, i));
 }
 
 static ptrdiff_t
@@ -785,14 +756,15 @@ DEFUN ("module-load", Fmodule_load, Smodule_load, 1, 1, 0,
   return Qt;
 }
 
-DEFUN ("module-call", Fmodule_call, Smodule_call, 2, 2, 0,
+DEFUN ("module-call", Fmodule_call, Smodule_call, 1, MANY, 0,
        doc: /* Internal function to call a module function.
 ENVOBJ is a save pointer to a module_fun_env structure.
 ARGLIST is a list of arguments passed to SUBRPTR.  */)
-  (Lisp_Object envobj, Lisp_Object arglist)
+  (ptrdiff_t nargs, Lisp_Object *args)
 {
+  Lisp_Object envobj = args[0];
   struct module_fun_env *envptr = XSAVE_POINTER (envobj, 0);
-  EMACS_INT len = XFASTINT (Flength (arglist));
+  EMACS_INT len = nargs - 1;
   eassume (0 <= envptr->min_arity);
   if (! (envptr->min_arity <= len
 	 && len <= (envptr->max_arity < 0 ? PTRDIFF_MAX : envptr->max_arity)))
@@ -802,37 +774,28 @@ ARGLIST is a list of arguments passed to SUBRPTR.  */)
   struct env_storage env;
   initialize_environment (&env);
 
-  emacs_value *args = xnmalloc (len, sizeof *args);
-
-  for (ptrdiff_t i = 0; i < len; i++)
-    {
-      args[i] = lisp_to_value (&env.pub, XCAR (arglist));
-      if (! args[i])
-	memory_full (sizeof *args[i]);
-      arglist = XCDR (arglist);
-    }
-
-  emacs_value ret = envptr->subr (&env.pub, len, args, envptr->data);
-  xfree (args);
+  emacs_value ret = envptr->subr (&env.pub, len,
+                                  /* BEWARE!  Here, we assume that Lisp_Object
+                                     and emacs_value have the exact same
+                                     representation.  */
+                                  (emacs_value*) args+1, envptr->data);
 
   switch (env.priv.pending_non_local_exit)
     {
     case emacs_funcall_exit_return:
       finalize_environment (&env);
-      if (ret == NULL)
-	xsignal1 (Qinvalid_module_call, module_format_fun_env (envptr));
       return value_to_lisp (ret);
     case emacs_funcall_exit_signal:
       {
-        Lisp_Object symbol = value_to_lisp (&env.priv.non_local_exit_symbol);
-        Lisp_Object data = value_to_lisp (&env.priv.non_local_exit_data);
+        Lisp_Object symbol = env.priv.non_local_exit_symbol;
+        Lisp_Object data = env.priv.non_local_exit_data;
         finalize_environment (&env);
         xsignal (symbol, data);
       }
     case emacs_funcall_exit_throw:
       {
-        Lisp_Object tag = value_to_lisp (&env.priv.non_local_exit_symbol);
-        Lisp_Object value = value_to_lisp (&env.priv.non_local_exit_data);
+        Lisp_Object tag = env.priv.non_local_exit_symbol;
+        Lisp_Object value = env.priv.non_local_exit_data;
         finalize_environment (&env);
         Fthrow (tag, value);
       }
@@ -864,8 +827,8 @@ module_non_local_exit_signal_1 (emacs_env *env, Lisp_Object sym,
   if (p->pending_non_local_exit == emacs_funcall_exit_return)
     {
       p->pending_non_local_exit = emacs_funcall_exit_signal;
-      p->non_local_exit_symbol.v = sym;
-      p->non_local_exit_data.v = data;
+      p->non_local_exit_symbol = sym;
+      p->non_local_exit_data = data;
     }
 }
 
@@ -877,8 +840,8 @@ module_non_local_exit_throw_1 (emacs_env *env, Lisp_Object tag,
   if (p->pending_non_local_exit == emacs_funcall_exit_return)
     {
       p->pending_non_local_exit = emacs_funcall_exit_throw;
-      p->non_local_exit_symbol.v = tag;
-      p->non_local_exit_data.v = value;
+      p->non_local_exit_symbol = tag;
+      p->non_local_exit_data = value;
     }
 }
 
@@ -915,94 +878,25 @@ module_args_out_of_range (emacs_env *env, Lisp_Object a1, Lisp_Object a2)
 static Lisp_Object
 value_to_lisp (emacs_value v)
 {
-  return v->v;
+  return XIL (v);
 }
 
 /* Convert an internal object to an `emacs_value'.  Allocate storage
    from the environment; return NULL if allocation fails.  */
 static emacs_value
-lisp_to_value (emacs_env *env, Lisp_Object o)
+lisp_to_value (Lisp_Object o)
 {
-  struct emacs_env_private *p = env->private_members;
-  if (p->pending_non_local_exit != emacs_funcall_exit_return)
-    return NULL;
-  return allocate_emacs_value (env, &p->storage, o);
+  return XLI (o);
 }
 
 \f
 /* Memory management.  */
 
-/* Must be called for each frame before it can be used for allocation.  */
-static void
-initialize_frame (struct emacs_value_frame *frame)
-{
-  frame->offset = 0;
-  frame->next = NULL;
-}
-
-/* Must be called for any storage object before it can be used for
-   allocation.  */
-static void
-initialize_storage (struct emacs_value_storage *storage)
-{
-  initialize_frame (&storage->initial);
-  storage->current = &storage->initial;
-}
-
-/* Must be called for any initialized storage object before its
-   lifetime ends.  Free all dynamically-allocated frames.  */
-static void
-finalize_storage (struct emacs_value_storage *storage)
-{
-  struct emacs_value_frame *next = storage->initial.next;
-  while (next != NULL)
-    {
-      struct emacs_value_frame *current = next;
-      next = current->next;
-      free (current);
-    }
-}
-
-/* Allocate a new value from STORAGE and stores OBJ in it.  Return
-   NULL if allocation fails and use ENV for non local exit reporting.  */
-static emacs_value
-allocate_emacs_value (emacs_env *env, struct emacs_value_storage *storage,
-		      Lisp_Object obj)
-{
-  eassert (storage->current);
-  eassert (storage->current->offset < value_frame_size);
-  eassert (! storage->current->next);
-  if (storage->current->offset == value_frame_size - 1)
-    {
-      storage->current->next = malloc (sizeof *storage->current->next);
-      if (! storage->current->next)
-        {
-          module_out_of_memory (env);
-          return NULL;
-        }
-      initialize_frame (storage->current->next);
-      storage->current = storage->current->next;
-    }
-  emacs_value value = storage->current->objects + storage->current->offset;
-  value->v = obj;
-  ++storage->current->offset;
-  return value;
-}
-
 /* Mark all objects allocated from local environments so that they
    don't get garbage-collected.  */
 void
 mark_modules (void)
 {
-  for (Lisp_Object tem = Vmodule_environments; CONSP (tem); tem = XCDR (tem))
-    {
-      struct env_storage *env = XSAVE_POINTER (tem, 0);
-      for (struct emacs_value_frame *frame = &env->priv.storage.initial;
-	   frame != NULL;
-	   frame = frame->next)
-        for (int i = 0; i < frame->offset; ++i)
-          mark_object (frame->objects[i].v);
-    }
 }
 
 \f
@@ -1013,7 +907,6 @@ static void
 initialize_environment (struct env_storage *env)
 {
   env->priv.pending_non_local_exit = emacs_funcall_exit_return;
-  initialize_storage (&env->priv.storage);
   env->pub.size = sizeof env->pub;
   env->pub.private_members = &env->priv;
   env->pub.make_global_ref = module_make_global_ref;
@@ -1051,7 +944,6 @@ initialize_environment (struct env_storage *env)
 static void
 finalize_environment (struct env_storage *env)
 {
-  finalize_storage (&env->priv.storage);
   Vmodule_environments = XCDR (Vmodule_environments);
 }
 
@@ -1152,8 +1044,6 @@ syms_of_module (void)
   Fput (Qinvalid_arity, Qerror_message,
         build_pure_c_string ("Invalid function arity"));
 
-  initialize_storage (&global_storage);
-
   /* Unintern `module-refs-hash' because it is internal-only and Lisp
      code or modules should not access it.  */
   Funintern (Qmodule_refs_hash, Qnil);
diff --git a/src/emacs-module.h b/src/emacs-module.h
index ea5de76..f6e3141 100644
--- a/src/emacs-module.h
+++ b/src/emacs-module.h
@@ -38,7 +38,7 @@ extern "C" {
 typedef struct emacs_env_25 emacs_env;
 
 /* Opaque structure pointer representing an Emacs Lisp value.  */
-typedef struct emacs_value_tag *emacs_value;
+typedef long emacs_value;
 
 enum emacs_arity { emacs_variadic_function = -2 };
 

Re: More over-engineering

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Date: Fri, 27 Nov 2015 13:02:56 -0500
> Cc: Emacs development discussions <emacs-devel@gnu.org>
> 
>        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.
> +   FIXME: Why is it always needed?  What happens if we don't?
> +          Why is only one of the two needed rather than always both?

I indeed think we should have only one macro that does the job of all
three of them.  It's on my todo to make that change, if no one beats
me to it.

Re: More over-engineering

[Stefan Monnier]

>> 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.
>> +   FIXME: Why is it always needed?  What happens if we don't?
>> +          Why is only one of the two needed rather than always both?

> I indeed think we should have only one macro that does the job of all
> three of them.  It's on my todo to make that change, if no one beats
> me to it.

How 'bout the other question: what happens if we don't use that macro?


        Stefan

Re: More over-engineering

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: aurelien.aptel+emacs@gmail.com,  emacs-devel@gnu.org
> Date: Fri, 27 Nov 2015 13:53:22 -0500
> 
> >> 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.
> >> +   FIXME: Why is it always needed?  What happens if we don't?
> >> +          Why is only one of the two needed rather than always both?
> 
> > I indeed think we should have only one macro that does the job of all
> > three of them.  It's on my todo to make that change, if no one beats
> > me to it.
> 
> How 'bout the other question: what happens if we don't use that macro?

I assumed that question was rhetorical.

Re: More over-engineering

[Stefan Monnier]

>> How 'bout the other question: what happens if we don't use that macro?
> I assumed that question was rhetorical.

No, I'm really curious why all functions need to do that.


        Stefan

Re: More over-engineering

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: aurelien.aptel+emacs@gmail.com,  emacs-devel@gnu.org
> Date: Fri, 27 Nov 2015 16:55:56 -0500
> 
> >> How 'bout the other question: what happens if we don't use that macro?
> > I assumed that question was rhetorical.
> 
> No, I'm really curious why all functions need to do that.

Because they want to catch all non-local exits.  Those macros are the
implementation of what was discussed in the other thread all over.

Re: More over-engineering

[Stefan Monnier]

>> >> How 'bout the other question: what happens if we don't use that macro?
>> > I assumed that question was rhetorical.
>> No, I'm really curious why all functions need to do that.
> Because they want to catch all non-local exits.  Those macros are the
> implementation of what was discussed in the other thread all over.

So it is not protecting something inside those functions, but rather
just trying to hide the non-local exits from the caller?

I can understand having a safe_funcall since that can be very useful
when you don't know what the funcall will do (e.g. a hook), but for
"primitives" where the set of possible non-local exits are well known,
catching all non-local exits unconditionally seems hard to justify.

It really seems like it would be saner to export the error catching
functionality as a separate function and then let every module use it
when it needs it.


        Stefan

Re: More over-engineering

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: aurelien.aptel+emacs@gmail.com,  emacs-devel@gnu.org
> Date: Sat, 28 Nov 2015 11:22:01 -0500
> 
> >> >> How 'bout the other question: what happens if we don't use that macro?
> >> > I assumed that question was rhetorical.
> >> No, I'm really curious why all functions need to do that.
> > Because they want to catch all non-local exits.  Those macros are the
> > implementation of what was discussed in the other thread all over.
> 
> So it is not protecting something inside those functions, but rather
> just trying to hide the non-local exits from the caller?

Not hide, intercept them and prevent them.

Re: More over-engineering

[Paul Eggert]

Stefan Monnier wrote:
> The only advantage in this respect is that the representation of
> Lisp_Object is encapsulated inside Emacs core, so the same module can be
> used with a "emacs-32bit-narrow-int" and on "emacs-32bit-wide-int".
> The cost for that minor advantage seems way out of proportion.

I agree.  Although I'm a fan of --with-wide-int, I don't think it should force 
this extra complication in modules.  People who want Emacs with wide ints can 
build modules to suit.  This is the same as 32- vs 64-bit off_t, in traditional 
C libraries.

Re: More over-engineering

[Stefan Monnier]

> I agree.  Although I'm a fan of --with-wide-int, I don't think it should
> force this extra complication in modules.

I managed to make it work for --with-wide-int without having to
re-compile the modules (those rare 64bit values which can't be crammed
into 32bit go through an extra proxy object).  That imposes an extra
cost in the module-API for the wide-int case, but this cost should still
be smaller than what we had before (since in 99.9% of the cases, the
conversion 64<=>32 is just a matter of shuffling bits).


        Stefan

Re: More over-engineering

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Date: Mon, 30 Nov 2015 14:40:02 -0500
> Cc: Aurélien Aptel <aurelien.aptel+emacs@gmail.com>,
> 	Emacs development discussions <emacs-devel@gnu.org>
> 
> > I agree.  Although I'm a fan of --with-wide-int, I don't think it should
> > force this extra complication in modules.
> 
> I managed to make it work for --with-wide-int without having to
> re-compile the modules (those rare 64bit values which can't be crammed
> into 32bit go through an extra proxy object).

I get compilation warnings with those changes:

  emacs-module.c: In function 'value_to_lisp':
  emacs-module.c:853:19: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
     EMACS_INT tmp = (EMACS_INT)v;
		     ^
  emacs-module.c:859:7: warning: passing argument 1 of 'make_lisp_ptr' makes pointer from integer without a cast [enabled by default]
	 o = make_lisp_ptr ((tmp - tag) >> GCTYPEBITS, tag); break;
	 ^
  In file included from emacs-module.c:30:0:
  lisp.h:1095:1: note: expected 'void *' but argument is of type 'EMACS_INT'
   make_lisp_ptr (void *ptr, enum Lisp_Type type)
   ^
  emacs-module.c:861:26: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
	 o = make_lisp_ptr ((void*)(tmp - tag), tag);
			    ^
  emacs-module.c: In function 'lisp_to_value':
  emacs-module.c:895:32: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
	   if (val == (EMACS_UINT)(emacs_value)val)
				  ^
  emacs-module.c:895:20: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
	   if (val == (EMACS_UINT)(emacs_value)val)
		      ^
  emacs-module.c:897:29: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
	       emacs_value v = (emacs_value) ((val << GCTYPEBITS) | tag);
			       ^
  emacs-module.c:907:14: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
	   if (((EMACS_UINT)ptr) & ((1 << GCTYPEBITS) - 1))
		^
  emacs-module.c:913:40: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
	   emacs_value v = (emacs_value)(((EMACS_UINT) ptr) | tag);
					  ^
  emacs-module.c:913:25: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
	   emacs_value v = (emacs_value)(((EMACS_UINT) ptr) | tag);
			   ^

Re: More over-engineering

[Stefan Monnier]

> I get compilation warnings with those changes:

Oh, yes!  It's ugly bit-twiddling with pointer values, especially in
the --with-wide-int case.
I hope someone can figure out how to do it without triggering all
those warnings.


        Stefan

Re: More over-engineering

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: eggert@cs.ucla.edu,  aurelien.aptel+emacs@gmail.com,  emacs-devel@gnu.org
> Date: Mon, 30 Nov 2015 15:39:10 -0500
> 
> > I get compilation warnings with those changes:
> 
> Oh, yes!  It's ugly bit-twiddling with pointer values, especially in
> the --with-wide-int case.
> I hope someone can figure out how to do it without triggering all
> those warnings.

I think I just did, but I'd love you and Paul to take a look, please.

Re: More over-engineering

[Paul Eggert]

Eli Zaretskii wrote:
> I think I just did, but I'd love you and Paul to take a look, please.

I installed something that should fix the emacs-module.c warnings in a better way.