Reporting Lisp errors in dynamic modules

Reporting Lisp errors in dynamic modules

[Eli Zaretskii]

With the latest emacs-25 branch, load modules/mod-test/mod-test.so,
then evaluate '(mod-test-sum "1" 2)'.  The backtrace that pops up is
this:

  Debugger entered--Lisp error: (wrong-type-argument integerp "1")
    #<subr module-call>(#<save-value <pointer 00fdab90> <unused> <unused> <unused>> ("1" 2))
    mod-test-sum("1" 2)
    eval((mod-test-sum "1" 2) nil)
    elisp--eval-last-sexp(nil)
    eval-last-sexp(nil)
    funcall-interactively(eval-last-sexp nil)
    call-interactively(eval-last-sexp nil nil)
    command-execute(eval-last-sexp)

I believe the line with "<subr module-call>" is suboptimal, in that it
looks alien and includes all kinds of unneeded and weirdly formatted
data.  AFAIU, the reason is that we deliberately unintern module-call.
Should we perhaps reconsider that decision, so that the backtrace is
in more familiar form?  What exactly are the dangers of having
module-call exposed as any other primitive?

Re: Reporting Lisp errors in dynamic modules

[Aurélien Aptel]

On Fri, Nov 27, 2015 at 12:07 PM, Eli Zaretskii <eliz@gnu.org> wrote:
> I believe the line with "<subr module-call>" is suboptimal, in that it
> looks alien and includes all kinds of unneeded and weirdly formatted
> data.  AFAIU, the reason is that we deliberately unintern module-call.
> Should we perhaps reconsider that decision, so that the backtrace is
> in more familiar form?  What exactly are the dangers of having
> module-call exposed as any other primitive?

It's an implementation detail and cannot be used safely by Lisp users.
The danger of using it is it can make core Emacs crash. We also don't
want the risk of someone overriding it.

Maybe we could try to detect it and print something more meaningful in
the backtrace as a compromise?

Also, unrelated but can I be added to the savannah emacs group? My
savannah account is aaptel.

Re: Reporting Lisp errors in dynamic modules

[Eli Zaretskii]

> Date: Fri, 27 Nov 2015 12:20:03 +0100
> From: Aurélien Aptel <aurelien.aptel+emacs@gmail.com>
> Cc: Emacs development discussions <emacs-devel@gnu.org>, Ted Zlatanov <tzz@lifelogs.com>, 
> 	Daniel Colascione <dancol@dancol.org>, Philipp Stephani <p.stephani2@gmail.com>
> 
> On Fri, Nov 27, 2015 at 12:07 PM, Eli Zaretskii <eliz@gnu.org> wrote:
> > I believe the line with "<subr module-call>" is suboptimal, in that it
> > looks alien and includes all kinds of unneeded and weirdly formatted
> > data.  AFAIU, the reason is that we deliberately unintern module-call.
> > Should we perhaps reconsider that decision, so that the backtrace is
> > in more familiar form?  What exactly are the dangers of having
> > module-call exposed as any other primitive?
> 
> It's an implementation detail and cannot be used safely by Lisp users.
> The danger of using it is it can make core Emacs crash.

How can it cause Emacs to crash?  Can you show a specific scenario?
We should be able to prevent that.

> We also don't want the risk of someone overriding it.

How is that different from any other primitive?

> Also, unrelated but can I be added to the savannah emacs group? My
> savannah account is aaptel.

Please apply, and please explain your interests.

Re: Reporting Lisp errors in dynamic modules

[Aurélien Aptel]

On Fri, Nov 27, 2015 at 12:46 PM, Eli Zaretskii <eliz@gnu.org> wrote:
>> It's an implementation detail and cannot be used safely by Lisp users.
>> The danger of using it is it can make core Emacs crash.
>
> How can it cause Emacs to crash?  Can you show a specific scenario?
> We should be able to prevent that.

You cannot allocate a valid struct module_fun_env embedded in a
save_value from Lisp (first expected argument of 'module-call').
Therefore any call to 'module-call' from Lisp is invalid. It's an
implementation detail and shouldn't be used directly by users.

>> We also don't want the risk of someone overriding it.
> How is that different from any other primitive?

It's the same.

>> Also, unrelated but can I be added to the savannah emacs group? My
>> savannah account is aaptel.
> Please apply, and please explain your interests.

I already signed the copyright assignment papers few years ago. I've
already contributed a few things here and there (underwave support)
and finally I initiated and spent most of a year working on this
dynamic module feature, along with Philip Stephani. Is there anything
else I should do?

Re: Reporting Lisp errors in dynamic modules

[David Kastrup]

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

>>> Also, unrelated but can I be added to the savannah emacs group? My
>>> savannah account is aaptel.
>> Please apply, and please explain your interests.
>
> I already signed the copyright assignment papers few years ago. I've
> already contributed a few things here and there (underwave support)
> and finally I initiated and spent most of a year working on this
> dynamic module feature, along with Philip Stephani. Is there anything
> else I should do?

By "please apply" Eli means using the respective "join a project"
functionality from your Savannah account.

-- 
David Kastrup

Re: Reporting Lisp errors in dynamic modules

[Eli Zaretskii]

> Date: Fri, 27 Nov 2015 13:49:39 +0100
> From: Aurélien Aptel <aurelien.aptel+emacs@gmail.com>
> Cc: Emacs development discussions <emacs-devel@gnu.org>, Ted Zlatanov <tzz@lifelogs.com>, 
> 	Daniel Colascione <dancol@dancol.org>, Philipp Stephani <p.stephani2@gmail.com>
> 
> On Fri, Nov 27, 2015 at 12:46 PM, Eli Zaretskii <eliz@gnu.org> wrote:
> >> It's an implementation detail and cannot be used safely by Lisp users.
> >> The danger of using it is it can make core Emacs crash.
> >
> > How can it cause Emacs to crash?  Can you show a specific scenario?
> > We should be able to prevent that.
> 
> You cannot allocate a valid struct module_fun_env embedded in a
> save_value from Lisp (first expected argument of 'module-call').
> Therefore any call to 'module-call' from Lisp is invalid.

We could easily detect that, and signal an error, couldn't we?

> It's an implementation detail and shouldn't be used directly by
> users.

I didn't want to let users use it.  I want users to see a backtrace as
close to what they are used to as possible.  Please help me improve
the backtrace.

> >> Also, unrelated but can I be added to the savannah emacs group? My
> >> savannah account is aaptel.
> > Please apply, and please explain your interests.
> 
> I already signed the copyright assignment papers few years ago. I've
> already contributed a few things here and there (underwave support)
> and finally I initiated and spent most of a year working on this
> dynamic module feature, along with Philip Stephani. Is there anything
> else I should do?

No, I meant apply for membership on Savannah.  Sorry if that was
unclear.

Re: Reporting Lisp errors in dynamic modules

[Philipp Stephani]

[-- Attachment #1: Type: text/plain, Size: 1745 bytes --]

Eli Zaretskii <eliz@gnu.org> schrieb am Fr., 27. Nov. 2015 um 12:07 Uhr:

> With the latest emacs-25 branch, load modules/mod-test/mod-test.so,
> then evaluate '(mod-test-sum "1" 2)'.  The backtrace that pops up is
> this:
>
>   Debugger entered--Lisp error: (wrong-type-argument integerp "1")
>     #<subr module-call>(#<save-value <pointer 00fdab90> <unused> <unused>
> <unused>> ("1" 2))
>     mod-test-sum("1" 2)
>     eval((mod-test-sum "1" 2) nil)
>     elisp--eval-last-sexp(nil)
>     eval-last-sexp(nil)
>     funcall-interactively(eval-last-sexp nil)
>     call-interactively(eval-last-sexp nil nil)
>     command-execute(eval-last-sexp)
>
> I believe the line with "<subr module-call>" is suboptimal, in that it
> looks alien and includes all kinds of unneeded and weirdly formatted
> data.  AFAIU, the reason is that we deliberately unintern module-call.
> Should we perhaps reconsider that decision, so that the backtrace is
> in more familiar form?  What exactly are the dangers of having
> module-call exposed as any other primitive?
>

- Somebody calls it with the wrong type. Right now it doesn't do any type
checking, so that would crash/be UB.
- An existing function outside of Emacs might already be called like that.

Those are really minor issues, but since module-call is unusable from Lisp
or module code anyway, I've decided to avoid them by uninterning it. If
type checking is added and the name clash issue is consider minor,
interning it is fine.

A cleaner approach would be to define module functions as separate kind of
function object. I think right now we have lists (Lisp closures and
lambdas), subrs, and bytecode objects, module functions could be a fourth
type. But that would require more thorough changes.

[-- Attachment #2: Type: text/html, Size: 2262 bytes --]

Re: Reporting Lisp errors in dynamic modules

[Eli Zaretskii]

> From: Philipp Stephani <p.stephani2@gmail.com>
> Date: Fri, 27 Nov 2015 16:40:23 +0000
> Cc: aurelien.aptel+emacs@gmail.com, tzz@lifelogs.com, dancol@dancol.org
> 
>     I believe the line with "<subr module-call>" is suboptimal, in that it
>     looks alien and includes all kinds of unneeded and weirdly formatted
>     data. AFAIU, the reason is that we deliberately unintern module-call.
>     Should we perhaps reconsider that decision, so that the backtrace is
>     in more familiar form? What exactly are the dangers of having
>     module-call exposed as any other primitive?
>     
> 
> - Somebody calls it with the wrong type. Right now it doesn't do any type
> checking, so that would crash/be UB.

We can add checks to countermand that.

> - An existing function outside of Emacs might already be called like that.

Not really a problem, IMO.

> Those are really minor issues, but since module-call is unusable from Lisp or
> module code anyway, I've decided to avoid them by uninterning it. If type
> checking is added and the name clash issue is consider minor, interning it is
> fine.

OK, I will see what I can do, thanks.

> A cleaner approach would be to define module functions as separate kind of
> function object. I think right now we have lists (Lisp closures and lambdas),
> subrs, and bytecode objects, module functions could be a fourth type. But that
> would require more thorough changes.

Yes, indeed.

Re: Reporting Lisp errors in dynamic modules

[Philipp Stephani]

[-- Attachment #1: Type: text/plain, Size: 1062 bytes --]

Eli Zaretskii <eliz@gnu.org> schrieb am Fr., 27. Nov. 2015 um 18:40 Uhr:

> > From: Philipp Stephani <p.stephani2@gmail.com>
> > Date: Fri, 27 Nov 2015 16:40:23 +0000
> > Cc: aurelien.aptel+emacs@gmail.com, tzz@lifelogs.com, dancol@dancol.org
> >
> >     I believe the line with "<subr module-call>" is suboptimal, in that
> it
> >     looks alien and includes all kinds of unneeded and weirdly formatted
> >     data. AFAIU, the reason is that we deliberately unintern module-call.
> >     Should we perhaps reconsider that decision, so that the backtrace is
> >     in more familiar form? What exactly are the dangers of having
> >     module-call exposed as any other primitive?
> >
> >
> > - Somebody calls it with the wrong type. Right now it doesn't do any type
> > checking, so that would crash/be UB.
>
> We can add checks to countermand that.
>
> > - An existing function outside of Emacs might already be called like
> that.
>
> Not really a problem, IMO.
>

We could rename the function to "internal-module-call" to make it even less
likely to clash.

[-- Attachment #2: Type: text/html, Size: 1721 bytes --]

Re: Reporting Lisp errors in dynamic modules

[Eli Zaretskii]

> From: Philipp Stephani <p.stephani2@gmail.com>
> Date: Fri, 27 Nov 2015 18:01:54 +0000
> Cc: emacs-devel@gnu.org, aurelien.aptel+emacs@gmail.com, tzz@lifelogs.com, 
> 	dancol@dancol.org
> 
> We could rename the function to "internal-module-call" to make it even less
> likely to clash. 

Yes, I think renaming it, even to internal--module-call, is a must.

Re: Reporting Lisp errors in dynamic modules

[Philipp Stephani]

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

OK, here's a quick (untested, sorry) patch.

Eli Zaretskii <eliz@gnu.org> schrieb am Fr., 27. Nov. 2015 um 19:11 Uhr:

> > From: Philipp Stephani <p.stephani2@gmail.com>
> > Date: Fri, 27 Nov 2015 18:01:54 +0000
> > Cc: emacs-devel@gnu.org, aurelien.aptel+emacs@gmail.com,
> tzz@lifelogs.com,
> >       dancol@dancol.org
> >
> > We could rename the function to "internal-module-call" to make it even
> less
> > likely to clash.
>
> Yes, I think renaming it, even to internal--module-call, is a must.
>

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

[-- Attachment #2: 0001-Intern-module-call-but-rename-it.patch --]
[-- Type: application/octet-stream, Size: 4098 bytes --]

From 16915940e818b2f1dfc0d8aa4f299901aa0cf37e Mon Sep 17 00:00:00 2001
From: Philipp Stephani <phst@google.com>
Date: Fri, 27 Nov 2015 19:39:29 +0100
Subject: [PATCH] Intern `module-call', but rename it.

---
 src/emacs-module.c | 36 +++++++++++++++++-------------------
 1 file changed, 17 insertions(+), 19 deletions(-)

diff --git a/src/emacs-module.c b/src/emacs-module.c
index c75ddeb..8a0b02c 100644
--- a/src/emacs-module.c
+++ b/src/emacs-module.c
@@ -221,8 +221,8 @@ static void module_wrong_type (emacs_env *, Lisp_Object, Lisp_Object);
 /* A function environment is an auxiliary structure used by
    `module_make_function' to store information about a module
    function.  It is stored in a save pointer and retrieved by
-   `module-call'.  Its members correspond to the arguments given to
-   `module_make_function'.  */
+   `internal--module-call'.  Its members correspond to the arguments
+   given to `module_make_function'.  */
 
 struct module_fun_env
 {
@@ -231,11 +231,6 @@ struct module_fun_env
   void *data;
 };
 
-/* The function definition of `module-call'.  `module-call' is
-   uninterned because user code couldn't meaningfully use it, so keep
-   its definition around somewhere else.  */
-static Lisp_Object module_call_func;
-
 \f
 /* Implementation of runtime and environment functions.
 
@@ -391,12 +386,12 @@ module_non_local_exit_throw (emacs_env *env, emacs_value tag, emacs_value value)
 				   value_to_lisp (value));
 }
 
-/* A module function is lambda function that calls `module-call',
-   passing the function pointer of the module function along with the
-   module emacs_env pointer as arguments.
+/* A module function is lambda function that calls
+   `internal--module-call', passing the function pointer of the module
+   function along with the module emacs_env pointer as arguments.
 
 	(function (lambda (&rest arglist)
-		    (module-call envobj arglist)))  */
+		    (internal--module-call envobj arglist)))  */
 
 static emacs_value
 module_make_function (emacs_env *env, ptrdiff_t min_arity, ptrdiff_t max_arity,
@@ -430,7 +425,7 @@ module_make_function (emacs_env *env, ptrdiff_t min_arity, ptrdiff_t max_arity,
   Lisp_Object ret = list4 (Qlambda,
                            list2 (Qand_rest, Qargs),
                            doc,
-                           list3 (module_call_func,
+                           list3 (Qinternal_module_call,
                                   envobj,
                                   Qargs));
 
@@ -785,12 +780,16 @@ DEFUN ("module-load", Fmodule_load, Smodule_load, 1, 1, 0,
   return Qt;
 }
 
-DEFUN ("module-call", Fmodule_call, Smodule_call, 2, 2, 0,
+DEFUN ("internal--module-call", Finternal_module_call, Sinternal_module_call, 2, 2, 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)
 {
+  CHECK_TYPE (SAVE_VALUEP (envobj), Qsave_value_p, envobj);
+  struct Lisp_Save_Value *save_value = XSAVE_VALUE (envobj);
+  CHECK_TYPE (save_type (save_value, 0) == SAVE_POINTER, Qsave_pointer_p, envobj);
+  CHECK_CONS (arglist);
   struct module_fun_env *envptr = XSAVE_POINTER (envobj, 0);
   EMACS_INT len = XFASTINT (Flength (arglist));
   eassume (0 <= envptr->min_arity);
@@ -1158,14 +1157,13 @@ syms_of_module (void)
      code or modules should not access it.  */
   Funintern (Qmodule_refs_hash, Qnil);
 
+  DEFSYM (Qsave_value_p, "save-value-p");
+  DEFSYM (Qsave_pointer_p, "save-pointer-p");
+
   defsubr (&Smodule_load);
 
-  /* Don't call defsubr on `module-call' because that would intern it,
-     but `module-call' is an internal function that users cannot
-     meaningfully use.  Instead, assign its definition to a private
-     variable.  */
-  XSETPVECTYPE (&Smodule_call, PVEC_SUBR);
-  XSETSUBR (module_call_func, &Smodule_call);
+  DEFSYM (Qinternal_module_call, "internal--module-call");
+  defsubr (&Sinternal_module_call);
 }
 
 /* Unlike syms_of_module, this initializer is called even from an
-- 
2.6.3

Re: Reporting Lisp errors in dynamic modules

[Eli Zaretskii]

> From: Philipp Stephani <p.stephani2@gmail.com>
> Date: Fri, 27 Nov 2015 18:40:39 +0000
> Cc: emacs-devel@gnu.org, aurelien.aptel+emacs@gmail.com, tzz@lifelogs.com, 
> 	dancol@dancol.org
> 
> OK, here's a quick (untested, sorry) patch.

Thanks, I pushed it.

It indeed broke 2 tests from test.el; I fixed that.

(Please don't forget to provide ChangeLog style log messages.)

After this change, the backtrace in my original use case is still
somewhat unusual:

  Debugger entered--Lisp error: (wrong-type-argument integerp "1")
    internal--module-call(#<save-value <pointer 00fdab30> <unused> <unused> <unused>> ("1" 2))
    mod-test-sum("1" 2)
    eval((mod-test-sum "1" 2) nil)
    elisp--eval-last-sexp(nil)
    eval-last-sexp(nil)
    funcall-interactively(eval-last-sexp nil)
    call-interactively(eval-last-sexp nil nil)
    command-execute(eval-last-sexp)

But I guess we could live with that for now.  (If someone has ideas
how to remove the "save-value" part and unwrap the list of arguments
so it looks like in the preceding line, please tell.)

Re: Reporting Lisp errors in dynamic modules

[Stefan Monnier]

> A cleaner approach would be to define module functions as separate kind of
> function object. I think right now we have lists (Lisp closures and
> lambdas), subrs, and bytecode objects, module functions could be a fourth
> type. But that would require more thorough changes.

WIBNI we could define them as Lisp_Subr?


        Stefan

Re: Reporting Lisp errors in dynamic modules

[Aurélien Aptel]

On Thu, Dec 3, 2015 at 6:00 AM, Stefan Monnier <monnier@iro.umontreal.ca> wrote:
> WIBNI we could define them as Lisp_Subr?

(these initialisms are getting ridiculous)

Yes we could try to do that, it's still an implementation detail as
far as most module programmers are concerned.

Re: Reporting Lisp errors in dynamic modules

[Ted Zlatanov]

On Thu, 3 Dec 2015 12:33:04 +0100 Aurélien Aptel <aurelien.aptel+emacs@gmail.com> wrote: 

AA> On Thu, Dec 3, 2015 at 6:00 AM, Stefan Monnier <monnier@iro.umontreal.ca> wrote:
>> WIBNI we could define them as Lisp_Subr?

AA> Yes we could try to do that, it's still an implementation detail as
AA> far as most module programmers are concerned.

Could that also provide a way to supply secrets (like passphrases or
keys) to Emacs from external modules with a restricted scope?

Ted

Re: Reporting Lisp errors in dynamic modules

[Stefan Monnier]

>> WIBNI we could define them as Lisp_Subr?
> (these initialisms are getting ridiculous)

[ When weren't they?  ]

> Yes we could try to do that, it's still an implementation detail as
> far as most module programmers are concerned.

Yes, I totally agree that it's low priority.


        Stefan

Re: Reporting Lisp errors in dynamic modules

[Philipp Stephani]

[-- Attachment #1: Type: text/plain, Size: 626 bytes --]

Aurélien Aptel <aurelien.aptel+emacs@gmail.com> schrieb am Do., 3. Dez.
2015 um 12:33 Uhr:

> On Thu, Dec 3, 2015 at 6:00 AM, Stefan Monnier <monnier@iro.umontreal.ca>
> wrote:
> > WIBNI we could define them as Lisp_Subr?
>
> (these initialisms are getting ridiculous)
>
> Yes we could try to do that, it's still an implementation detail as
> far as most module programmers are concerned.
>
>
It would suggest an interface change: defsubr also binds the function
object to a symbol. If we want make_function to use defsubr, it should
accept a symbol name parameter (which might be NULL, to undo the binding).

[-- Attachment #2: Type: text/html, Size: 994 bytes --]