Dynamic modules: emacs-module.c and signaling errors

Dynamic modules: emacs-module.c and signaling errors

[Eli Zaretskii]

In "emacs -Q", load the modules/mod-test/mod-test module, then try
this:

  M-: (mod-test-sum "1" 2) RET

Result: Emacs aborts.  This happens because the eassert in
module_extract_integer aborts, when that function is called for the
2nd time:

  static intmax_t
  module_extract_integer (emacs_env *env, emacs_value n)
  {
    check_main_thread ();
    eassert (module_non_local_exit_check (env) == emacs_funcall_exit_return);
    Lisp_Object l = value_to_lisp (n);
    if (! INTEGERP (l))
      {
	module_wrong_type (env, Qintegerp, l);
	return 0;
      }

The first call to module_extract_integer correctly detects the wrong
type of argument and calls module_wrong_type.  But module_wrong_type
just records the problem in the env structure, it doesn't signal any
Lisp error, like an Emacs primitive would.  So the actual error goes
undetected, and is masked by the assertion violation (because Emacs is
built with --enable-checking).

Since this obviously works as it was designed, my question is: how
should a module be written so that this kind of errors signal a normal
Lisp error we are accustomed with Emacs primitives?

TIA

Re: Dynamic modules: emacs-module.c and signaling errors

[Eli Zaretskii]

> Date: Tue, 24 Nov 2015 21:41:12 +0200
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: emacs-devel@gnu.org
> 
> The first call to module_extract_integer correctly detects the wrong
> type of argument and calls module_wrong_type.  But module_wrong_type
> just records the problem in the env structure, it doesn't signal any
> Lisp error, like an Emacs primitive would.  So the actual error goes
> undetected, and is masked by the assertion violation (because Emacs is
  ^^^^^^^^^^
I meant to say "unreported", of course.

Re: Dynamic modules: emacs-module.c and signaling errors

[Tom Tromey]

>>>>> "Eli" == Eli Zaretskii <eliz@gnu.org> writes:

Eli> Since this obviously works as it was designed, my question is: how
Eli> should a module be written so that this kind of errors signal a normal
Eli> Lisp error we are accustomed with Emacs primitives?

What I did in my module is do an error check after every call in to
Emacs, returning early when one is seen.  Then eventually control
returns to Fmodule_call, which handles translating this to a Lisp
signal.

The module in question isn't doing this check after calling
extract_integer, but it should.

Tom

Re: Dynamic modules: emacs-module.c and signaling errors

[Eli Zaretskii]

> From: Tom Tromey <tom@tromey.com>
> Cc: Philipp Stephani <p.stephani2@gmail.com>,  aurelien.aptel+emacs@gmail.com,  tzz@lifelogs.com,  dancol@dancol.org,  emacs-devel@gnu.org
> Date: Tue, 24 Nov 2015 13:12:30 -0700
> 
> What I did in my module is do an error check after every call in to
> Emacs, returning early when one is seen.  Then eventually control
> returns to Fmodule_call, which handles translating this to a Lisp
> signal.

Thanks.  However, this is highly non-obvious, and IMO quite error
prone.  The consequences of failing to conform to this paradigm are
dire: the error goes unreported, and instead an unrelated assertion
blows up.

Perhaps we should remove these assertions, or at least make them
conditional on something that isn't ENABLE_CHECKING, since it seems
they were added for debugging emacs-module.c, not for catching errors
in modules.

Btw, using this method the module code becomes very tedious, something
like

  intmax_t a = env->extract_integer (env, args[0]);
  if (env->non_local_exit_check (env) != emacs_funcall_exit_return)
    return NULL;
  intmax_t b = env->extract_integer (env, args[1]);
  if (env->non_local_exit_check (env) != emacs_funcall_exit_return)
    return NULL;

etc.

Re: Dynamic modules: emacs-module.c and signaling errors

[Paul Eggert]

On 11/24/2015 12:49 PM, Eli Zaretskii wrote:
> Btw, using this method the module code becomes very tedious

It's so tedious I can't imagine using it that way. Module authors 
shouldn't have to call env->non_local_exit_check (env) after every 
function call, to see whether the function actually worked. Functions 
should be able to signal an error, just as they can in the Emacs core, 
and the error should propagate through calls in a natural way.

Re: Dynamic modules: emacs-module.c and signaling errors

[Daniel Colascione]

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

On 11/24/2015 01:34 PM, Paul Eggert wrote:
> On 11/24/2015 12:49 PM, Eli Zaretskii wrote:
>> Btw, using this method the module code becomes very tedious
> 
> It's so tedious I can't imagine using it that way. Module authors
> shouldn't have to call env->non_local_exit_check (env) after every
> function call, to see whether the function actually worked. Functions
> should be able to signal an error, just as they can in the Emacs core,
> and the error should propagate through calls in a natural way.

And in a C++ wrapper interface, that can happen naturally and safely. If
you want to write C, you check for errors the C way.


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: Dynamic modules: emacs-module.c and signaling errors

[Paul Eggert]

Daniel Colascione wrote:
> And in a C++ wrapper interface, that can happen naturally and safely. If
> you want to write C, you check for errors the C way.

C provides multiple ways to check. We're not obliged to implement a tedious and 
confusing API, if better alternatives are available.

Re: Dynamic modules: emacs-module.c and signaling errors

[Daniel Colascione]

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

On 11/24/2015 10:52 PM, Paul Eggert wrote:
> Daniel Colascione wrote:
>> And in a C++ wrapper interface, that can happen naturally and safely. If
>> you want to write C, you check for errors the C way.
> 
> C provides multiple ways to check. We're not obliged to implement a
> tedious and confusing API, if better alternatives are available.

C provides many error checking alternatives. Longjmping outside Emacs is
not one of them.


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: Dynamic modules: emacs-module.c and signaling errors

[Paul Eggert]

Daniel Colascione wrote:
> C provides many error checking alternatives. Longjmping outside Emacs is
> not one of them.

Surely there is a reasonably natural way to address the problem without 
requiring module authors to call longjmp, or to have a complex and error-prone 
check after every call.

Re: Dynamic modules: emacs-module.c and signaling errors

[Daniel Colascione]

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

On 11/24/2015 11:14 PM, Paul Eggert wrote:
> Daniel Colascione wrote:
>> C provides many error checking alternatives. Longjmping outside Emacs is
>> not one of them.
> 
> Surely there is a reasonably natural way to address the problem without
> requiring module authors to call longjmp, or to have a complex and
> error-prone check after every call.

How is it any worse than checking whether open(2) returns -1? I'd be
fine with just making future calls fail until a module clears the
pending condition or returns to Emacs.


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: Dynamic modules: emacs-module.c and signaling errors

[Paul Eggert]

Daniel Colascione wrote:
> How is it any worse than checking whether open(2) returns -1?

'open' probes the outside world, and programs should be prepared for such probes 
to fail. In contrast, 'cons' is basic memory allocation, just as calling a 
function is basic memory allocation. We don't require programs to check each 
function call to see whether the call failed because the stack was exhausted. 
Similarly we don't require programs to check each call to 'cons' to see whether 
'cons' failed because the heap was exhausted.

Re: Dynamic modules: emacs-module.c and signaling errors

[Daniel Colascione]

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

On 11/24/2015 11:23 PM, Paul Eggert wrote:
> Daniel Colascione wrote:
>> How is it any worse than checking whether open(2) returns -1?
> 
> 'open' probes the outside world, and programs should be prepared for
> such probes to fail. In contrast, 'cons' is basic memory allocation,
> just as calling a function is basic memory allocation. We don't require
> programs to check each function call to see whether the call failed
> because the stack was exhausted. Similarly we don't require programs to
> check each call to 'cons' to see whether 'cons' failed because the heap
> was exhausted.

I don't believe in treating memory specially. It's just another
resource. We can exhaust it just like anything else. In elisp, we have
conditions. In a module API, we have to rely on traditional error checking.


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: Dynamic modules: emacs-module.c and signaling errors

[Paul Eggert]

Daniel Colascione wrote:
> In a module API, we have to rely on traditional error checking.

No we don't.  We certainly do not rely on it for checking stack space 
exhaustion.  And we don't have to rely on it for heap space exhaustion either.

Re: Dynamic modules: emacs-module.c and signaling errors

[Daniel Colascione]

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

On 11/24/2015 11:49 PM, Paul Eggert wrote:
> Daniel Colascione wrote:
>> In a module API, we have to rely on traditional error checking.
> 
> No we don't.  We certainly do not rely on it for checking stack space
> exhaustion.  And we don't have to rely on it for heap space exhaustion
> either.

What's your alternative? Assuming memory allocation never fails? That's
unacceptable in robust programming. I disagree that it's tedious to
check for allocation failure. We can QUIT almost anywhere too: do you
think we should use some kind of special reporting mechanism for that too?

I don't think having to check for errors is a particularly onerous
requirement when writing C code.


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: Dynamic modules: emacs-module.c and signaling errors

[Paul Eggert]

Daniel Colascione wrote:
> What's your alternative?

I don't have one right now. But surely a better API can be devised.

I agree that checking errors in return values is reasonable for things like 
'open'. But memory allocation is so basic. Plus, the method for checking errors 
in the current API is awkward and confusing. It'd be a real turnoff to have to 
use it after each little step in one's program.

Re: Dynamic modules: emacs-module.c and signaling errors

[Daniel Colascione]

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

On 11/24/2015 11:58 PM, Paul Eggert wrote:
> Daniel Colascione wrote:
>> What's your alternative?
> 
> I don't have one right now. But surely a better API can be devised.
> 
> I agree that checking errors in return values is reasonable for things
> like 'open'. But memory allocation is so basic.

You could argue that file descriptors are basic. They're just handles to
bits of kernel memory, right?

Memory is a resource like anything else, and like all resources, can be
exhausted. (Overcommit is no answer: you can't overcommit address
space.) We have to do _something_ on memory exhaustion, and the only
reasonable choices are communicating the failure up the call stack and
aborting.

Aborting is brittle, so I'd rather not do it. In the Emacs core, we can
activate the low-memory code and let users save work, but if we abort
because a module asks us to allocate, we'll lose data.

> Plus, the method for
> checking errors in the current API is awkward and confusing. It'd be a
> real turnoff to have to use it after each little step in one's program.

Well, there's the Xlib method, where calls just silently fail until you
bother checking for error. I think that's even more cumbersome, since it
makes isolating the source of an error difficult unless your program
runs in synchronous mode.

Lots of extension APIs require error checks. Python's in particular
comes to mind. Using these APIs doesn't feel particularly onerous.

That said, I agree that the specific API for error checking is
cumbersome. The function name is long, and comparing the value it
returns to a specific sentinel feels awkward. IMHO, we could simplify it
to if(env->error_p(env)) { return NULL; } (The name "error" here isn't
strictly accurate, but I think it's close enough, and it's short.


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: Dynamic modules: emacs-module.c and signaling errors

[Paul Eggert]

Daniel Colascione wrote:
> You could argue that file descriptors are basic. They're just handles to
> bits of kernel memory, right?

I'm not making that argument. I am arguing that memory allocation is basic, 
though. It really is. It happens *all the time* in the C code inside Emacs, and 
it's almost surely going to happen all the time in module code as well. It 
should be easy, not a pain.

Re: Dynamic modules: emacs-module.c and signaling errors

[Daniel Colascione]

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

On 11/25/2015 12:24 AM, Paul Eggert wrote:
> Daniel Colascione wrote:
>> You could argue that file descriptors are basic. They're just handles to
>> bits of kernel memory, right?
> 
> I'm not making that argument. I am arguing that memory allocation is
> basic, though. It really is. It happens *all the time* in the C code
> inside Emacs, and it's almost surely going to happen all the time in
> module code as well. It should be easy, not a pain.

Modules in C already have to handle checking for failures of their
internal allocations. What makes checking for failures of Emacs-side
allocations so much worse?

There are ways of making working with Lisp easier while preserving
robustness. I've previously proposed providing a utility function that
lets callers write expressions like this:

emacs_value v =
  env->eval_fmt(env, "`(foo (xyz . bar) ,%v ,%s ,(something %d))",
my_emacs_value, "blah", 42);
if(!v) { return NULL; } // Equivalently, if (env->error_p(env)) { return
NULL; }
do_something_with(v);

It's possible to implement eval_fmt mostly in Lisp, and it would let C
callers naturally construct arbitrarily complicated Lisp data structures
while only checking for errors in one place. Sure, it's not the fastest
thing the world, but in hot spots, modules can always drop down to the
very exact, but very verbose, API we've been complaining about here.

It should also be possible to write a function that pulls values out of
existing data structures just like destructuring-bind ---

char* key;
int value;
emacs_value some_plist = mumble;

while (env->is_not_nil(some_plist) && env->extract_fmt(env, some_plist,
"(%s %d . %v)", &key, &value, &some_plist)) {
  do_something(key, value);
  env->emacs_free(key); // extract_fmt allocated memory for the contents
of the string key using the internal emacs allocator
}
if (env->error_p(env)) {
  return NULL;
}

This way, it's possible to do fairly involved things while not checking
for errors at every single step.

It's also possible to implement eval_fmt and extract_fmt as library
functions written in terms of the current module API, but it wouldn't
hurt to provide them directly for the convenience of C callers.


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: Dynamic modules: emacs-module.c and signaling errors

[Daniel Colascione]

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

On 11/25/2015 12:50 AM, Daniel Colascione wrote:
> On 11/25/2015 12:24 AM, Paul Eggert wrote:
>> Daniel Colascione wrote:
>>> You could argue that file descriptors are basic. They're just handles to
>>> bits of kernel memory, right?
>>
>> I'm not making that argument. I am arguing that memory allocation is
>> basic, though. It really is. It happens *all the time* in the C code
>> inside Emacs, and it's almost surely going to happen all the time in
>> module code as well. It should be easy, not a pain.
> 
> Modules in C already have to handle checking for failures of their
> internal allocations. What makes checking for failures of Emacs-side
> allocations so much worse?
> 
> There are ways of making working with Lisp easier while preserving
> robustness.

Another option for making the Emacs API more ergonomic might be to
define most functions to do nothing if we have non-local control flow
pending, even on otherwise-invalid inputs. (That's opposed to making
emacs_env calls with pending non-local control into a programming
error.) That way, you'd be able to write something like this...

int64_t vplus1 = env->extract_integer(env, env->eval_fmt(env, "(1+ blah
%v)", v));
if (env->error_p()) {
  return NULL;
}

... and preserve whatever went wrong inside eval_fmt, not abort or raise
some generic "invalid call to extract_integer" error.

I'm not sure that this scheme is a good idea, but it does make for
shorter code while still allowing us to propagate errors from any spot
inside Emacs.


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: Dynamic modules: emacs-module.c and signaling errors

[Eli Zaretskii]

> Cc: aurelien.aptel+emacs@gmail.com, p.stephani2@gmail.com, tzz@lifelogs.com,
>  emacs-devel@gnu.org
> From: Daniel Colascione <dancol@dancol.org>
> Date: Wed, 25 Nov 2015 01:11:16 -0800
> 
> Another option for making the Emacs API more ergonomic might be to
> define most functions to do nothing if we have non-local control flow
> pending, even on otherwise-invalid inputs.

That's what AFAIU Tom suggested, and I tried to implement in a patch
sent a few minutes ago.  Please take a look.

> I'm not sure that this scheme is a good idea

Why not?

Re: Dynamic modules: emacs-module.c and signaling errors

[Philipp Stephani]

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

Daniel Colascione <dancol@dancol.org> schrieb am Mi., 25. Nov. 2015 um
10:11 Uhr:

> On 11/25/2015 12:50 AM, Daniel Colascione wrote:
> > On 11/25/2015 12:24 AM, Paul Eggert wrote:
> >> Daniel Colascione wrote:
> >>> You could argue that file descriptors are basic. They're just handles
> to
> >>> bits of kernel memory, right?
> >>
> >> I'm not making that argument. I am arguing that memory allocation is
> >> basic, though. It really is. It happens *all the time* in the C code
> >> inside Emacs, and it's almost surely going to happen all the time in
> >> module code as well. It should be easy, not a pain.
> >
> > Modules in C already have to handle checking for failures of their
> > internal allocations. What makes checking for failures of Emacs-side
> > allocations so much worse?
> >
> > There are ways of making working with Lisp easier while preserving
> > robustness.
>
> Another option for making the Emacs API more ergonomic might be to
> define most functions to do nothing if we have non-local control flow
> pending, even on otherwise-invalid inputs. (That's opposed to making
> emacs_env calls with pending non-local control into a programming
> error.) That way, you'd be able to write something like this...
>
> int64_t vplus1 = env->extract_integer(env, env->eval_fmt(env, "(1+ blah
> %v)", v));
> if (env->error_p()) {
>   return NULL;
> }
>
> ... and preserve whatever went wrong inside eval_fmt, not abort or raise
> some generic "invalid call to extract_integer" error.
>
> I'm not sure that this scheme is a good idea, but it does make for
> shorter code while still allowing us to propagate errors from any spot
> inside Emacs.
>
>
I'm not a fan of this idea. It is unlike all other C APIs I'm aware of and
I think the behavior would be quite surprising to the module author. After
all, it causes code to silently do nothing if doing something was requested.

I see a couple of alternatives:
- If an unhandled error is detected, crash unconditionally, even if
checking is disabled. This would be consistent with the behavior of
structured exceptions in popular programming languages, and would make bugs
very obvious.
- Ignore unhandled errors, proceed as if no error happened. This would be
consistent with popular C APIs such a the C standard library itself and
POSIX.
- The current approach: crash if checking is enabled, ignore otherwise.

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

Re: Dynamic modules: emacs-module.c and signaling errors

[Eli Zaretskii]

> From: Philipp Stephani <p.stephani2@gmail.com>
> Date: Wed, 25 Nov 2015 18:26:31 +0000
> Cc: aurelien.aptel+emacs@gmail.com, tzz@lifelogs.com, emacs-devel@gnu.org
> 
> I see a couple of alternatives:
> - If an unhandled error is detected, crash unconditionally, even if checking is
> disabled.

What do you mean by "crash"?  Abort?  That's unacceptable: Emacs
should not abort because some module misbehaved.  A long-lived Emacs
session holds gobs of precious data and state, utterly unrelated to
what a module tried to do, so discarding all that because of some
programming error in some module would be unthinkable.

We should cause such errors signal an error (eventually), like any
Lisp code does.

> - Ignore unhandled errors, proceed as if no error happened. This would be
> consistent with popular C APIs such a the C standard library itself and POSIX.
> - The current approach: crash if checking is enabled, ignore otherwise.

The simple patch I sent a few minutes ago, according to Tom's
suggestion, should cause such errors to signal a Lisp error from
module-call.  I think this is what we should do.

Re: Dynamic modules: emacs-module.c and signaling errors

[Philipp Stephani]

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

Eli Zaretskii <eliz@gnu.org> schrieb am Mi., 25. Nov. 2015 um 20:19 Uhr:

> > - Ignore unhandled errors, proceed as if no error happened. This would be
> > consistent with popular C APIs such a the C standard library itself and
> POSIX.
> > - The current approach: crash if checking is enabled, ignore otherwise.
>
> The simple patch I sent a few minutes ago, according to Tom's
> suggestion, should cause such errors to signal a Lisp error from
> module-call.  I think this is what we should do.
>

Yes, I think that's better than crashing, longjmping, and also the current
approach. Ignoring unhandled errors would indeed have the potential to
swallow errors, unless we saturate the error state (i.e. never set it to
successful except in clear_error). So now I think that your patch indeed is
the best solution.

Maybe we should further reduce the amount of undefined behavior. Currently
e.g. passing null pointers is undefined behavior that will typically crash
Emacs. We could make this more robust by signaling an error instead; the
overhead should be minimal.

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

Re: Dynamic modules: emacs-module.c and signaling errors

[Paul Eggert]

On 11/25/2015 12:50 AM, Daniel Colascione wrote:
> Modules in C already have to handle checking for failures of their
> internal allocations. What makes checking for failures of Emacs-side
> allocations so much worse?
Allocation is more-common in Lisp-like programs than in C-like programs, 
so it needs to be catered to more. Traditionally, C dynamic allocation 
was often done on the stack, where the programmer doesn't need to 
manually insert code to check whether it succeeds. Lisp and module code 
should be as convenient as that, for 'cons'.

This raises another issue, though. Suppose Emacs detects C stack 
overflow and signals an exception when in the middle of a module. How 
does that work? We need to solve the stack-exhaustion problem 
independently of the details of the module API for memory allocation. 
And whatever method we come up with, should make it easier for us to 
provide convenient heap allocation too, because stack and heap 
exhaustion can be handled by similar mechanisms.

Re: Dynamic modules: emacs-module.c and signaling errors

[Tom Tromey]

Paul> It's so tedious I can't imagine using it that way. Module authors
Paul> shouldn't have to call env->non_local_exit_check (env) after every
Paul> function call, to see whether the function actually worked. Functions
Paul> should be able to signal an error, just as they can in the Emacs core,
Paul> and the error should propagate through calls in a natural way.

Yeah, it's tedious.

The alternative is also error-prone though.  It's very easy to forget
that some cleanup is needed; and the Emacs facilities here aren't really
all that easy to use either.  gdb uses a similar internal system, and it
is better than what Emacs provides, but still error-prone in practice.

Crashing may be the wrong thing to do.  Perhaps module APIs should just
fail when the environment is already in the error state.

Tom

Re: Dynamic modules: emacs-module.c and signaling errors

[Paul Eggert]

Tom Tromey wrote:
> The alternative is also error-prone though.  It's very easy to forget
> that some cleanup is needed; and the Emacs facilities here aren't really
> all that easy to use either.

We're talking about memory allocation here.  If Emacs allocates the memory, the 
caller shouldn't need to clean up, as the Emacs garbage collector can do that. 
This should be less error-prone.

Re: Dynamic modules: emacs-module.c and signaling errors

[Eli Zaretskii]

> Cc: Eli Zaretskii <eliz@gnu.org>, aurelien.aptel+emacs@gmail.com,
>  p.stephani2@gmail.com, dancol@dancol.org, tzz@lifelogs.com,
>  emacs-devel@gnu.org
> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Tue, 24 Nov 2015 22:55:14 -0800
> 
> Tom Tromey wrote:
> > The alternative is also error-prone though.  It's very easy to forget
> > that some cleanup is needed; and the Emacs facilities here aren't really
> > all that easy to use either.
> 
> We're talking about memory allocation here.  If Emacs allocates the memory, the 
> caller shouldn't need to clean up, as the Emacs garbage collector can do that. 
> This should be less error-prone.

Maybe I misunderstand the issue, but AFAICS any Lisp data that is
allocated by emacs-module.c on behalf of modules already goes through
xmalloc, so what exactly are the remaining problems?

Re: Dynamic modules: emacs-module.c and signaling errors

[Paul Eggert]

On 11/25/2015 09:30 AM, Eli Zaretskii wrote:
> Maybe I misunderstand the issue, but AFAICS any Lisp data that is
> allocated by emacs-module.c on behalf of modules already goes through
> xmalloc, so what exactly are the remaining problems?

I think Daniel is worried that module code will call xmalloc, and then 
do more work that signals an Elisp error or throws a C++ exception, and 
the module will then leak the xmalloced memory. There may be other 
concerns like that too, but I'm not sure what they are.

Re: Dynamic modules: emacs-module.c and signaling errors

[Stefan Monnier]

> I think Daniel is worried that module code will call xmalloc, and then do
> more work that signals an Elisp error or throws a C++ exception, and the
> module will then leak the xmalloced memory. There may be other concerns like
> that too, but I'm not sure what they are.

I find it hard to believe that we'd follow Daniel's recommendation
over mine.  As far as I know, he didn't give any more evidence of his
assertions of goodness of his approach than I did (OTOH, it's obvious at
the technical level that Daniel's approach can be layered on top of
mine, whereas the reverse is not true, which I think should a strong
argument in favor of following the simpler approach I advocate).


        Stefan

Re: Dynamic modules: emacs-module.c and signaling errors

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Date: Wed, 25 Nov 2015 16:23:24 -0500
> 
> > I think Daniel is worried that module code will call xmalloc, and then do
> > more work that signals an Elisp error or throws a C++ exception, and the
> > module will then leak the xmalloced memory. There may be other concerns like
> > that too, but I'm not sure what they are.
> 
> I find it hard to believe that we'd follow Daniel's recommendation
> over mine.  As far as I know, he didn't give any more evidence of his
> assertions of goodness of his approach than I did (OTOH, it's obvious at
> the technical level that Daniel's approach can be layered on top of
> mine, whereas the reverse is not true, which I think should a strong
> argument in favor of following the simpler approach I advocate).

Are you talking about signaling and throwing in case of errors?  Or
are you talking about something else?

I think we should be pragmatic here.  If there's a way of having an
interface that is safe in the face of the likes of C++ exceptions,
without unduly punishing module authors, we should take it.  It sounds
like a win-win to me.

I believe we've already found a reasonably good way to handle
signaling errors and throwing.  (I will soon install the changes I
posted yesterday.)  With that in place, module code that wants to
signal an error will look very much like what we do in core in similar
situations.

Paul wants to extend these conveniences to memory allocation, and that
is what he wrote about above.  I agree it would be nice to have that.
I'm not sure I fully understand the concerns (Paul says he doesn't
understand them fully, either), but with my perhaps limited
understanding, I fail to see why the same approach -- save the
information about the Lisp error in the environment object, then
return instead of jumping, and let module-call signal -- cannot be
used to provide a convenient memory allocator very similar to xmalloc.
At worst, we will just need to teach emacs-module.c about the
"memory-full" error, that's all.

Am I missing something?

Re: Dynamic modules: emacs-module.c and signaling errors

[Stefan Monnier]

> Are you talking about signaling and throwing in case of errors?

Yes, that's the only issue at stake.

> I think we should be pragmatic here.  If there's a way of having an
> interface that is safe in the face of the likes of C++ exceptions,
> without unduly punishing module authors, we should take it.

The interface we expose is a C interface.
We could add a C++ interface on top of it, of course, but I think we
should first focus on interfacing with C code.

> I believe we've already found a reasonably good way to handle
> signaling errors and throwing.  (I will soon install the changes I
> posted yesterday.)  With that in place, module code that wants to
> signal an error will look very much like what we do in core in similar
> situations.

The issue at stake is when module code calls an Emacs function that
signals an error.  In the basic API this signal should not be caught by
the module API (i.e. Emacs's Fsignal code will happily longjmp out of
the modules code when applicable), unless explicitly requested via
something like env->safe_funcall.


        Stefan

Re: Dynamic modules: emacs-module.c and signaling errors

[Eli Zaretskii]

> From: Stefan Monnier <monnier@IRO.UMontreal.CA>
> Cc: emacs-devel@gnu.org
> Date: Thu, 26 Nov 2015 11:36:00 -0500
> 
> The issue at stake is when module code calls an Emacs function that
> signals an error.  In the basic API this signal should not be caught by
> the module API (i.e. Emacs's Fsignal code will happily longjmp out of
> the modules code when applicable), unless explicitly requested via
> something like env->safe_funcall.

What happens now is that modules can call Lisp only via env->funcall,
which _will_ catch the signal.  When the signal is caught, its data is
recorded.  All the other API calls made by the module function on
whose watch the signal happened will do nothing, and eventually the
module function will return to module-call, which will re-throw the
signal with exactly the same data.

Why is that a problem?

Re: Dynamic modules: emacs-module.c and signaling errors

[Stefan Monnier]

> What happens now is that modules can call Lisp only via env->funcall,
> which _will_ catch the signal.  When the signal is caught, its data is
> recorded.  All the other API calls made by the module function on
> whose watch the signal happened will do nothing, and eventually the
> module function will return to module-call, which will re-throw the
> signal with exactly the same data.
> Why is that a problem?

Because this loses the connection between the signal and its origin.
Because it imposes a cost which we may not want to pay.

The current "funcall" should be renamed to "safe_funcall", and a new
"funcall" should be provided which does not catch signals.


        Stefan

Re: Dynamic modules: emacs-module.c and signaling errors

[Eli Zaretskii]

> From: Stefan Monnier <monnier@IRO.UMontreal.CA>
> Cc: emacs-devel@gnu.org
> Date: Thu, 26 Nov 2015 14:28:34 -0500
> 
> > What happens now is that modules can call Lisp only via env->funcall,
> > which _will_ catch the signal.  When the signal is caught, its data is
> > recorded.  All the other API calls made by the module function on
> > whose watch the signal happened will do nothing, and eventually the
> > module function will return to module-call, which will re-throw the
> > signal with exactly the same data.
> > Why is that a problem?
> 
> Because this loses the connection between the signal and its origin.

Not really, it doesn't.  You get the same Lisp error you'd get in
Emacs proper.

> Because it imposes a cost which we may not want to pay.

What cost is that?

> The current "funcall" should be renamed to "safe_funcall", and a new
> "funcall" should be provided which does not catch signals.

I still think we should be pragmatic about that, not radical.  If the
cake can be had and eaten, too, why not?

Re: Dynamic modules: emacs-module.c and signaling errors

[Stefan Monnier]

>> Because this loses the connection between the signal and its origin.
> Not really, it doesn't.  You get the same Lisp error you'd get in
> Emacs proper.

That's only the signal, not its origin.  Its origin is lost.

>> Because it imposes a cost which we may not want to pay.
> What cost is that?

Catch and then rethrow.  Which is pure cost with no benefit, if you ask me.

>> The current "funcall" should be renamed to "safe_funcall", and a new
>> "funcall" should be provided which does not catch signals.
> I still think we should be pragmatic about that, not radical.

That's exactly what I am.  The straightforward code doesn't
catch&rethrow, so rather than let people do funcall_without_catch by
testing if the not_so_plain_funcall has caught a signal and then rethrow
it, it makes a boat load more sense to do it the other way around and
let those who need the signals to be caught to request explicitly the
not_so_plain_funcall.

> If the cake can be had and eaten, too, why not?

I'm fine with having both funcalls available.  I just want to have the
"raw" funcall advertized at least as much as the other, and
the other one built on top rather than the other way around.

I actually find it difficult to believe I have to argue this point.
It's so blindingly obvious from a simple engineering perspective, not to
mention the decades of experience with using a "raw non-catching
funcall" from Emacs's C code.


        Stefan

Re: Dynamic modules: emacs-module.c and signaling errors

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: emacs-devel@gnu.org
> Date: Thu, 26 Nov 2015 15:11:25 -0500
> 
> >> Because this loses the connection between the signal and its origin.
> > Not really, it doesn't.  You get the same Lisp error you'd get in
> > Emacs proper.
> 
> That's only the signal, not its origin.  Its origin is lost.

Not sure what you mean by that.  The origin is clearly stated in the
error message.

> >> Because it imposes a cost which we may not want to pay.
> > What cost is that?
> 
> Catch and then rethrow.  Which is pure cost with no benefit, if you ask me.

A cost very close to zero.  Let's keep the proper perspective.

> The straightforward code doesn't
> catch&rethrow, so rather than let people do funcall_without_catch by
> testing if the not_so_plain_funcall has caught a signal and then rethrow
> it, it makes a boat load more sense to do it the other way around and
> let those who need the signals to be caught to request explicitly the
> not_so_plain_funcall.

No one needs to do anything, the code is already written, and module
authors don't even need to know it exists, or what exactly does it do.

> > If the cake can be had and eaten, too, why not?
> 
> I'm fine with having both funcalls available.  I just want to have the
> "raw" funcall advertized at least as much as the other, and
> the other one built on top rather than the other way around.

Feel free to add it, then.

> I actually find it difficult to believe I have to argue this point.
> It's so blindingly obvious from a simple engineering perspective, not to
> mention the decades of experience with using a "raw non-catching
> funcall" from Emacs's C code.

It's not obvious to me, sorry.  The costs of the current code are
minimal, and the advantages to have the "raw" functionality in
addition to what we have aren't clear-cut.

Re: Dynamic modules: emacs-module.c and signaling errors

[Paul Eggert]

Eli Zaretskii wrote:
> It's not obvious to me, sorry.  The costs of the current code are
> minimal, and the advantages to have the "raw" functionality in
> addition to what we have aren't clear-cut.

My admittedly vague impression is that Stefan's approach would be friendlier to 
module authors who want to write code that feels like Emacs's current C code. 
This should be a plus, no?

I don't fully understand why the current emacs-module.c is as complicated as it 
is, to be honest.  I know it has something to do with C++ exception handling, 
but the details still escape me.  But really, we should support C modules well 
too, as C is still the lingua franca for low-level software integration.  And 
"well" does not mean "every time you call a function you then have to call some 
other function to see whether the first function worked".

Re: Dynamic modules: emacs-module.c and signaling errors

[Philipp Stephani]

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

Paul Eggert <eggert@cs.ucla.edu> schrieb am Do., 26. Nov. 2015 um 22:21 Uhr:

> Eli Zaretskii wrote:
> > It's not obvious to me, sorry.  The costs of the current code are
> > minimal, and the advantages to have the "raw" functionality in
> > addition to what we have aren't clear-cut.
>
> My admittedly vague impression is that Stefan's approach would be
> friendlier to
> module authors who want to write code that feels like Emacs's current C
> code.
> This should be a plus, no?
>
> I don't fully understand why the current emacs-module.c is as complicated
> as it
> is, to be honest.  I know it has something to do with C++ exception
> handling,
> but the details still escape me.  But really, we should support C modules
> well
> too, as C is still the lingua franca for low-level software integration.
> And
> "well" does not mean "every time you call a function you then have to call
> some
> other function to see whether the first function worked".
>
>
Please see Daniel's original design and the follow-up discussions:
https://lists.gnu.org/archive/html/emacs-devel/2015-02/msg00960.html

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

Re: Dynamic modules: emacs-module.c and signaling errors

[Philipp Stephani]

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

Philipp Stephani <p.stephani2@gmail.com> schrieb am Do., 26. Nov. 2015 um
23:29 Uhr:

> Paul Eggert <eggert@cs.ucla.edu> schrieb am Do., 26. Nov. 2015 um
> 22:21 Uhr:
>
>> Eli Zaretskii wrote:
>> > It's not obvious to me, sorry.  The costs of the current code are
>> > minimal, and the advantages to have the "raw" functionality in
>> > addition to what we have aren't clear-cut.
>>
>> My admittedly vague impression is that Stefan's approach would be
>> friendlier to
>> module authors who want to write code that feels like Emacs's current C
>> code.
>> This should be a plus, no?
>>
>> I don't fully understand why the current emacs-module.c is as complicated
>> as it
>> is, to be honest.  I know it has something to do with C++ exception
>> handling,
>> but the details still escape me.  But really, we should support C modules
>> well
>> too, as C is still the lingua franca for low-level software integration.
>> And
>> "well" does not mean "every time you call a function you then have to
>> call some
>> other function to see whether the first function worked".
>>
>>
> Please see Daniel's original design and the follow-up discussions:
> https://lists.gnu.org/archive/html/emacs-devel/2015-02/msg00960.html
>


Also this subthread with even more discussion:
https://lists.gnu.org/archive/html/emacs-devel/2015-09/msg00540.html

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

Re: Dynamic modules: emacs-module.c and signaling errors

[Stefan Monnier]

> Please see Daniel's original design and the follow-up discussions:

The general design is good, but forcing this signal-handling is just
brain-dead from where I stand.  I stated this many times.  And I'm
shocked that we'd follow Daniel's design on this detail just because he
advocates it.


        Stefan

Re: Dynamic modules: emacs-module.c and signaling errors

[Daniel Colascione]

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

On 11/26/2015 05:06 PM, Stefan Monnier wrote:
>> Please see Daniel's original design and the follow-up discussions:
> 
> The general design is good, but forcing this signal-handling is just
> brain-dead from where I stand.  I stated this many times.  And I'm
> shocked that we'd follow Daniel's design on this detail just because he
> advocates it.

So we should follow your design because _you_ advocate it?

And it's not "just" because I advocate it --- there's a good reason that
the vast majority of general-purpose C libraries don't use non-local
control flow for general-purpose error reporting.


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: Dynamic modules: emacs-module.c and signaling errors

[Stefan Monnier]

>> The general design is good, but forcing this signal-handling is just
>> brain-dead from where I stand.  I stated this many times.  And I'm
>> shocked that we'd follow Daniel's design on this detail just because he
>> advocates it.
> So we should follow your design because _you_ advocate it?

Well, yes.  All things being equal (i.e. just hot air on either side),
I'd assume that years-of-service and megabytes-of-contributions should
tilt the balance in my favor.

> And it's not "just" because I advocate it --- there's a good reason that
> the vast majority of general-purpose C libraries don't use non-local
> control flow for general-purpose error reporting.

Emacs is not a general purpose C library, so I don't see why that'd
be relevant.


        Stefan

Re: Dynamic modules: emacs-module.c and signaling errors

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Date: Thu, 26 Nov 2015 20:06:39 -0500
> Cc: Eli Zaretskii <eliz@gnu.org>, Paul Eggert <eggert@cs.ucla.edu>,
> 	Daniel Colascione <dancol@dancol.org>,
> 	Emacs developers <emacs-devel@gnu.org>
> 
> > Please see Daniel's original design and the follow-up discussions:
> 
> The general design is good, but forcing this signal-handling is just
> brain-dead from where I stand.  I stated this many times.  And I'm
> shocked that we'd follow Daniel's design on this detail just because he
> advocates it.

I think this is extreme and uncalled-for.  You may disagree with the
design, but it's here and it works.  It brings some additional
benefits, albeit not free of cost.  After looking at the code for
quite some time, my conclusion is that the price is not insignificant,
but not painful enough to be an issue.  It certainly doesn't deserve
to be called "brain-dead".  Definitely not when the work has already
been done and endured the initial shock of being integrated with
Emacs.

So if you still disagree, let's please leave it at that, and move on.

Re: Dynamic modules: emacs-module.c and signaling errors

[Eli Zaretskii]

> Cc: emacs-devel@gnu.org
> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Thu, 26 Nov 2015 13:20:46 -0800
> 
> Eli Zaretskii wrote:
> > It's not obvious to me, sorry.  The costs of the current code are
> > minimal, and the advantages to have the "raw" functionality in
> > addition to what we have aren't clear-cut.
> 
> My admittedly vague impression is that Stefan's approach would be friendlier to 
> module authors who want to write code that feels like Emacs's current C code. 
> This should be a plus, no?

Yes.  I already posted a summary of the differences, and how IMO we
should go about keeping these differences to a minimum.  No one
replied.

> I don't fully understand why the current emacs-module.c is as complicated as it 
> is, to be honest.  I know it has something to do with C++ exception handling, 
> but the details still escape me.  But really, we should support C modules well 
> too, as C is still the lingua franca for low-level software integration.  And 
> "well" does not mean "every time you call a function you then have to call some 
> other function to see whether the first function worked".

This is concealed in emacs-module.c.  Module authors don't need to
know that, or be bothered.  They can simply write code "as usual".

As to emacs-module.c, it's more complicated due to this, I agree.  But
not too complicated, IMO.  And the rules for writing functions in
emacs-module.c are really quite simple, they almost boil down to
having some pretty boilerplate code at the beginning of every
function.  Maybe we even could come up with a single macro that
injects all that code.

Re: Dynamic modules: emacs-module.c and signaling errors

[Aurélien Aptel]

As tedious as it sounds, checking for errors after every API call is
the safest way to go. We have to remind ourselves we're extending an
editor in C. Crashing/aborting can make someone loose work.

As ignorant as it will sound, working on the dynamic modules feature
made me realize how Emacs core might leak memory everytime a call
signals in the middle a function if you're not careful. I find this
very error-prone.

Re: Dynamic modules: emacs-module.c and signaling errors

[Eli Zaretskii]

> Date: Fri, 27 Nov 2015 13:25:26 +0100
> From: Aurélien Aptel <aurelien.aptel+emacs@gmail.com>
> Cc: Paul Eggert <eggert@cs.ucla.edu>, Stefan Monnier <monnier@iro.umontreal.ca>, 
> 	Emacs development discussions <emacs-devel@gnu.org>
> 
> As tedious as it sounds, checking for errors after every API call is
> the safest way to go.

With the latest emacs-25 branch, we no longer do.  Which is a Good
Thing, IMO.  Writing an Emacs module isn't supposed to be such an
unpleasant exercise.

> We have to remind ourselves we're extending an editor in
> C. Crashing/aborting can make someone loose work.

Who said anything about crashing?  The current code doesn't crash, but
doesn't require error checking in the module code after each call to
an Emacs function.  Please take a look.

Btw, if we are talking about crashing, I think the massive use of
eassert in emacs-module.c is a much more probable cause of losing work
than anything else.  eassert is for things that "should never happen",
not for catching programmer's mistakes.  An assertion that is violated
_really_ crashes Emacs and risks losing valuable work.  Doing that
because some module has a bug is simply unacceptable.

> As ignorant as it will sound, working on the dynamic modules feature
> made me realize how Emacs core might leak memory everytime a call
> signals in the middle a function if you're not careful. I find this
> very error-prone.

You will have to explain that to us, because we do this in Emacs all
the time, and there are no leaks caused by that.

Re: Dynamic modules: emacs-module.c and signaling errors

[Paul Eggert]

Aurélien Aptel wrote:
> working on the dynamic modules feature
> made me realize how Emacs core might leak memory everytime a call
> signals in the middle a function if you're not careful

If modules use the Emacs memory allocator we should be able to arrange for any 
such leaks to be garbage collected, so this shouldn't be a problem.

Re: Dynamic modules: emacs-module.c and signaling errors

[Stefan Monnier]

>> >> Because this loses the connection between the signal and its origin.
>> > Not really, it doesn't.  You get the same Lisp error you'd get in
>> > Emacs proper.
>> That's only the signal, not its origin.  Its origin is lost.
> Not sure what you mean by that.  The origin is clearly stated in the
> error message.

You mean debug-on-error will properly show me the right stack state and
let me look up and change the corresponding variables?
Hmm... didn't think so.

>> >> Because it imposes a cost which we may not want to pay.
>> > What cost is that?
>> Catch and then rethrow.  Which is pure cost with no benefit, if you ask me.
> A cost very close to zero.

Who cares.  Still pure cost with no benefit.
The core provides naturally a plain raw non-catching funcall, but with
the current design a module author who wants this behavior can't have it
because the module API hides it behind some "idiot-proofed layer", so
the best the module author can do is add yet another layer to try and
reproduce as best it can the underling non-catching funcall behavior.

This is completely dumb.

> No one needs to do anything, the code is already written, and module
> authors don't even need to know it exists, or what exactly does it do.

The criteria to accept bad design shouldn't be "does the code
already exist?".

> It's not obvious to me, sorry.  The costs of the current code are
> minimal, and the advantages to have the "raw" functionality in
> addition to what we have aren't clear-cut.

As I said, we have plenty of experience in Emacs's own source code:
look in Emacs's source code and compare the number of uses of
"safe_call" and friends to the number of uses of "callN" and friends.
The catching version is useful at times, but it's the exception rather
than the rule.  XEmacs's experience is the same, apparently.


        Stefan

Re: Dynamic modules: emacs-module.c and signaling errors

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: emacs-devel@gnu.org
> Date: Thu, 26 Nov 2015 20:21:26 -0500
> 
> >> >> Because this loses the connection between the signal and its origin.
> >> > Not really, it doesn't.  You get the same Lisp error you'd get in
> >> > Emacs proper.
> >> That's only the signal, not its origin.  Its origin is lost.
> > Not sure what you mean by that.  The origin is clearly stated in the
> > error message.
> 
> You mean debug-on-error will properly show me the right stack state and
> let me look up and change the corresponding variables?

Up to the C level, yes.  You cannot do that once you get to the C
level, not with the Lisp debugger we have.

> Hmm... didn't think so.

Can you show some example of where some data is lost?  Because I think
we may be miscommunicating: I don't see that significant lost of data
that causes such strenuous objections.  Maybe you can explain that to
me.  Maybe something is indeed missing.

> >> >> Because it imposes a cost which we may not want to pay.
> >> > What cost is that?
> >> Catch and then rethrow.  Which is pure cost with no benefit, if you ask me.
> > A cost very close to zero.
> 
> Who cares.  Still pure cost with no benefit.

It's not without benefits.

> The core provides naturally a plain raw non-catching funcall, but with
> the current design a module author who wants this behavior can't have it

Why would a module author want a non-catching funcall?  Are the
reasons for having that strong enough to override the dangers?
Because there are other things a module author currently cannot do, as
the API offers only limited ways into Emacs.  Why would having a
non-catching funcall be so much more important than, say, being able
to add watchers to the Emacs event queue?

> This is completely dumb.

I'd appreciate if you refrained from name-calling.  It doesn't help
keep the discussion technical and constructive.

> > No one needs to do anything, the code is already written, and module
> > authors don't even need to know it exists, or what exactly does it do.
> 
> The criteria to accept bad design shouldn't be "does the code
> already exist?".

And whether it works, yes.  This is Free Software: whoever does the
job gets to choose the tools and the methods.  You know that.

> > It's not obvious to me, sorry.  The costs of the current code are
> > minimal, and the advantages to have the "raw" functionality in
> > addition to what we have aren't clear-cut.
> 
> As I said, we have plenty of experience in Emacs's own source code:
> look in Emacs's source code and compare the number of uses of
> "safe_call" and friends to the number of uses of "callN" and friends.
> The catching version is useful at times, but it's the exception rather
> than the rule.  XEmacs's experience is the same, apparently.

Modules aren't core Emacs, and weren't supposed to be it.  Not
everything that's possible in core _must_ be available to modules.
Modules aren't a way to extend Emacs in unlimited ways.  So just
saying this is possible in core is not a useful argument in this
discussion.  The important question here is: why is it important that
modules have this functionality at their disposal.

Re: Dynamic modules: emacs-module.c and signaling errors

[Stefan Monnier]

>> You mean debug-on-error will properly show me the right stack state and
>> let me look up and change the corresponding variables?
> Up to the C level, yes.  You cannot do that once you get to the C
> level, not with the Lisp debugger we have.

So you're saying that if I call a command which internally calls
a C module function which turn does "env->funcall" on some Elisp code
which then signals an error, I'll be throw into the *Backtrace* buffer
(after setting debug-on-error and not debug-on-signal) right at the
point where the error is really signaled (i.e. before returning to the
module's C code), and not later on when it is rethrown by the module
code?

>> The core provides naturally a plain raw non-catching funcall, but with
>> the current design a module author who wants this behavior can't have it
> Why would a module author want a non-catching funcall?

Why not?  Most funcalls from Emacs's C to Elisp in Emacs do not want to
catch errors and I see no reason why module code should be so
fundamentally different.

> Are the reasons for having that strong enough to override the dangers?

What danger?

> Because there are other things a module author currently cannot do, as
> the API offers only limited ways into Emacs.

Because it's less work than what the current code does.  It's just
giving access to some internal entry point.  Why hide this entry point?

>> The criteria to accept bad design shouldn't be "does the code
>> already exist?".
> And whether it works, yes.

No.  These are necessary criteria but not sufficient.


        Stefan

Re: Dynamic modules: emacs-module.c and signaling errors

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: emacs-devel@gnu.org
> Date: Fri, 27 Nov 2015 10:15:49 -0500
> 
> >> You mean debug-on-error will properly show me the right stack state and
> >> let me look up and change the corresponding variables?
> > Up to the C level, yes.  You cannot do that once you get to the C
> > level, not with the Lisp debugger we have.
> 
> So you're saying that if I call a command which internally calls
> a C module function which turn does "env->funcall" on some Elisp code
> which then signals an error, I'll be throw into the *Backtrace* buffer
> (after setting debug-on-error and not debug-on-signal) right at the
> point where the error is really signaled (i.e. before returning to the
> module's C code), and not later on when it is rethrown by the module
> code?

Yes:

  emacs -Q
  M-x load-file RET modules/mod-test/mod-test.dll
  M-: (mod-test-string-a-to-b (string-match "a" nil) "b") RET
   =>
     Debugger entered--Lisp error: (wrong-type-argument stringp nil)
       string-match("a" nil)
       (mod-test-string-a-to-b (string-match "a" nil) "b")
       eval((mod-test-string-a-to-b (string-match "a" nil) "b") 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)

> >> The core provides naturally a plain raw non-catching funcall, but with
> >> the current design a module author who wants this behavior can't have it
> > Why would a module author want a non-catching funcall?
> 
> Why not?

Sorry, "why not" is not an answer.  If there's no real need for this
functionality, then there's no harm in not providing it.

Re: Dynamic modules: emacs-module.c and signaling errors

[Stefan Monnier]

>> So you're saying that if I call a command which internally calls
>> a C module function which turn does "env->funcall" on some Elisp code
>> which then signals an error, I'll be throw into the *Backtrace* buffer
>> (after setting debug-on-error and not debug-on-signal) right at the
>> point where the error is really signaled (i.e. before returning to the
>> module's C code), and not later on when it is rethrown by the module
>> code?

> Yes:

>   emacs -Q
>   M-x load-file RET modules/mod-test/mod-test.dll
>   M-: (mod-test-string-a-to-b (string-match "a" nil) "b") RET
>    =>
>      Debugger entered--Lisp error: (wrong-type-argument stringp nil)
>        string-match("a" nil)
>        (mod-test-string-a-to-b (string-match "a" nil) "b")
>        eval((mod-test-string-a-to-b (string-match "a" nil) "b") 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)

Hmm... that's not the case I'm talking about: here the string-match is
called before even entering the module's code, so the module code is not
involved at all.

>> >> The core provides naturally a plain raw non-catching funcall, but with
>> >> the current design a module author who wants this behavior can't have it
>> > Why would a module author want a non-catching funcall?
>> Why not?
> Sorry, "why not" is not an answer.

Given then 271-vs-15 rather of non-catching funcalls vs catching
funcalls in Emacs's C code, I think it's pretty clear that it can be
very useful.  So really the question is the other way around: what makes
you think that the modules's code will be so vastly different that it
won't want a non-catching funcall, contrary to the experience so far?


        Stefan

Re: Dynamic modules: emacs-module.c and signaling errors

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: emacs-devel@gnu.org
> Date: Fri, 27 Nov 2015 11:29:31 -0500
> 
> >   emacs -Q
> >   M-x load-file RET modules/mod-test/mod-test.dll
> >   M-: (mod-test-string-a-to-b (string-match "a" nil) "b") RET
> >    =>
> >      Debugger entered--Lisp error: (wrong-type-argument stringp nil)
> >        string-match("a" nil)
> >        (mod-test-string-a-to-b (string-match "a" nil) "b")
> >        eval((mod-test-string-a-to-b (string-match "a" nil) "b") 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)
> 
> Hmm... that's not the case I'm talking about: here the string-match is
> called before even entering the module's code, so the module code is not
> involved at all.

Then maybe you could show the case you were talking about.

> >> >> The core provides naturally a plain raw non-catching funcall, but with
> >> >> the current design a module author who wants this behavior can't have it
> >> > Why would a module author want a non-catching funcall?
> >> Why not?
> > Sorry, "why not" is not an answer.
> 
> Given then 271-vs-15 rather of non-catching funcalls vs catching
> funcalls in Emacs's C code, I think it's pretty clear that it can be
> very useful.

The count should be based on modules, not on what we have in core.

> So really the question is the other way around: what makes
> you think that the modules's code will be so vastly different that it
> won't want a non-catching funcall, contrary to the experience so far?

I think module authors won't (and shouldn't) care.

Re: Dynamic modules: emacs-module.c and signaling errors

[Stefan Monnier]

> Then maybe you could show the case you were talking about.

M-: (setq debug-on-error t) RET
M-: (mod-test-non-local-exit-funcall (lambda () (+ "" 2)))


        Stefan

Re: Dynamic modules: emacs-module.c and signaling errors

[Tom Tromey]

>>>>> "Paul" == Paul Eggert <eggert@cs.ucla.edu> writes:

Paul> Tom Tromey wrote:
>> The alternative is also error-prone though.  It's very easy to forget
>> that some cleanup is needed; and the Emacs facilities here aren't really
>> all that easy to use either.

Paul> We're talking about memory allocation here.  If Emacs allocates the
Paul> memory, the caller shouldn't need to clean up, as the Emacs garbage
Paul> collector can do that. This should be less error-prone.

The particular problem Eli mentioned was not memory allocation, it was a
type-checking error.  In particular see how module_extract_integer calls
module_wrong_type.

Tom

Re: Dynamic modules: emacs-module.c and signaling errors

[Eli Zaretskii]

> From: Tom Tromey <tom@tromey.com>
> Cc: Eli Zaretskii <eliz@gnu.org>,  Tom Tromey <tom@tromey.com>,  aurelien.aptel+emacs@gmail.com,  p.stephani2@gmail.com,  dancol@dancol.org,  tzz@lifelogs.com,  emacs-devel@gnu.org
> Date: Tue, 24 Nov 2015 15:21:20 -0700
> 
> Perhaps module APIs should just fail when the environment is already
> in the error state.

Thanks, I think this is a very simple measure that goes a long way
towards resolving the issue.

Any objections to installing the patch below?  With it, I can get the
expected Lisp error when invoking the unchanged mod-test-sum with
non-integer arguments.

diff --git a/src/emacs-module.c b/src/emacs-module.c
index 1388e53..c75ddeb 100644
--- a/src/emacs-module.c
+++ b/src/emacs-module.c
@@ -198,7 +198,8 @@ static void module_wrong_type (emacs_env *, Lisp_Object, Lisp_Object);
    its local variable C must stay live in later code.  */
 
 #define MODULE_SETJMP_1(handlertype, handlerfunc, retval, c, dummy)	\
-  eassert (module_non_local_exit_check (env) == emacs_funcall_exit_return); \
+  if (module_non_local_exit_check (env) != emacs_funcall_exit_return)	\
+    return retval;							\
   struct handler *c = push_handler_nosignal (Qt, handlertype);		\
   if (!c)								\
     {									\
@@ -256,7 +288,8 @@ static emacs_value
 module_make_global_ref (emacs_env *env, emacs_value ref)
 {
   check_main_thread ();
-  eassert (module_non_local_exit_check (env) == emacs_funcall_exit_return);
+  if (module_non_local_exit_check (env) != emacs_funcall_exit_return)
+    return NULL;
   MODULE_HANDLE_SIGNALS;
   struct Lisp_Hash_Table *h = XHASH_TABLE (Vmodule_refs_hash);
   Lisp_Object new_obj = value_to_lisp (ref);
@@ -287,7 +320,8 @@ static void
 module_free_global_ref (emacs_env *env, emacs_value ref)
 {
   check_main_thread ();
-  eassert (module_non_local_exit_check (env) == emacs_funcall_exit_return);
+  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?  */
@@ -343,18 +377,18 @@ static void
 module_non_local_exit_signal (emacs_env *env, emacs_value sym, emacs_value data)
 {
   check_main_thread ();
-  eassert (module_non_local_exit_check (env) == emacs_funcall_exit_return);
-  module_non_local_exit_signal_1 (env, value_to_lisp (sym),
-				  value_to_lisp (data));
+  if (module_non_local_exit_check (env) == emacs_funcall_exit_return)
+    module_non_local_exit_signal_1 (env, value_to_lisp (sym),
+				    value_to_lisp (data));
 }
 
 static void
 module_non_local_exit_throw (emacs_env *env, emacs_value tag, emacs_value value)
 {
   check_main_thread ();
-  eassert (module_non_local_exit_check (env) == emacs_funcall_exit_return);
-  module_non_local_exit_throw_1 (env, value_to_lisp (tag),
-				 value_to_lisp (value));
+  if (module_non_local_exit_check (env) == emacs_funcall_exit_return)
+    module_non_local_exit_throw_1 (env, value_to_lisp (tag),
+				   value_to_lisp (value));
 }
 
 /* A module function is lambda function that calls `module-call',
@@ -370,7 +404,8 @@ module_make_function (emacs_env *env, ptrdiff_t min_arity, ptrdiff_t max_arity,
 		      void *data)
 {
   check_main_thread ();
-  eassert (module_non_local_exit_check (env) == emacs_funcall_exit_return);
+  if (module_non_local_exit_check (env) != emacs_funcall_exit_return)
+    return NULL;
   MODULE_HANDLE_SIGNALS;
 
   if (! (0 <= min_arity
@@ -407,7 +442,8 @@ module_funcall (emacs_env *env, emacs_value fun, ptrdiff_t nargs,
 		emacs_value args[])
 {
   check_main_thread ();
-  eassert (module_non_local_exit_check (env) == emacs_funcall_exit_return);
+  if (module_non_local_exit_check (env) != emacs_funcall_exit_return)
+    return NULL;
   MODULE_HANDLE_SIGNALS;
   MODULE_HANDLE_THROW;
 
@@ -428,7 +464,8 @@ static emacs_value
 module_intern (emacs_env *env, const char *name)
 {
   check_main_thread ();
-  eassert (module_non_local_exit_check (env) == emacs_funcall_exit_return);
+  if (module_non_local_exit_check (env) != emacs_funcall_exit_return)
+    return NULL;
   MODULE_HANDLE_SIGNALS;
   return lisp_to_value (env, intern (name));
 }
@@ -437,7 +474,8 @@ static emacs_value
 module_type_of (emacs_env *env, emacs_value value)
 {
   check_main_thread ();
-  eassert (module_non_local_exit_check (env) == emacs_funcall_exit_return);
+  if (module_non_local_exit_check (env) != emacs_funcall_exit_return)
+    return NULL;
   return lisp_to_value (env, Ftype_of (value_to_lisp (value)));
 }
 
@@ -445,7 +483,8 @@ static bool
 module_is_not_nil (emacs_env *env, emacs_value value)
 {
   check_main_thread ();
-  eassert (module_non_local_exit_check (env) == emacs_funcall_exit_return);
+  if (module_non_local_exit_check (env) != emacs_funcall_exit_return)
+    return false;
   return ! NILP (value_to_lisp (value));
 }
 
@@ -453,7 +492,8 @@ static bool
 module_eq (emacs_env *env, emacs_value a, emacs_value b)
 {
   check_main_thread ();
-  eassert (module_non_local_exit_check (env) == emacs_funcall_exit_return);
+  if (module_non_local_exit_check (env) != emacs_funcall_exit_return)
+    return false;
   return EQ (value_to_lisp (a), value_to_lisp (b));
 }
 
@@ -461,7 +501,8 @@ static intmax_t
 module_extract_integer (emacs_env *env, emacs_value n)
 {
   check_main_thread ();
-  eassert (module_non_local_exit_check (env) == emacs_funcall_exit_return);
+  if (module_non_local_exit_check (env) != emacs_funcall_exit_return)
+    return 0;
   Lisp_Object l = value_to_lisp (n);
   if (! INTEGERP (l))
     {
@@ -475,7 +516,8 @@ static emacs_value
 module_make_integer (emacs_env *env, intmax_t n)
 {
   check_main_thread ();
-  eassert (module_non_local_exit_check (env) == emacs_funcall_exit_return);
+  if (module_non_local_exit_check (env) != emacs_funcall_exit_return)
+    return NULL;
   if (! (MOST_NEGATIVE_FIXNUM <= n && n <= MOST_POSITIVE_FIXNUM))
     {
       module_non_local_exit_signal_1 (env, Qoverflow_error, Qnil);
@@ -488,7 +530,8 @@ static double
 module_extract_float (emacs_env *env, emacs_value f)
 {
   check_main_thread ();
-  eassert (module_non_local_exit_check (env) == emacs_funcall_exit_return);
+  if (module_non_local_exit_check (env) != emacs_funcall_exit_return)
+    return 0;
   Lisp_Object lisp = value_to_lisp (f);
   if (! FLOATP (lisp))
     {
@@ -502,7 +545,8 @@ static emacs_value
 module_make_float (emacs_env *env, double d)
 {
   check_main_thread ();
-  eassert (module_non_local_exit_check (env) == emacs_funcall_exit_return);
+  if (module_non_local_exit_check (env) != emacs_funcall_exit_return)
+    return NULL;
   MODULE_HANDLE_SIGNALS;
   return lisp_to_value (env, make_float (d));
 }
@@ -512,7 +556,8 @@ module_copy_string_contents (emacs_env *env, emacs_value value, char *buffer,
 			     ptrdiff_t *length)
 {
   check_main_thread ();
-  eassert (module_non_local_exit_check (env) == emacs_funcall_exit_return);
+  if (module_non_local_exit_check (env) != emacs_funcall_exit_return)
+    return false;
   MODULE_HANDLE_SIGNALS;
   Lisp_Object lisp_str = value_to_lisp (value);
   if (! STRINGP (lisp_str))
@@ -557,7 +602,8 @@ static emacs_value
 module_make_string (emacs_env *env, const char *str, ptrdiff_t length)
 {
   check_main_thread ();
-  eassert (module_non_local_exit_check (env) == emacs_funcall_exit_return);
+  if (module_non_local_exit_check (env) != emacs_funcall_exit_return)
+    return NULL;
   MODULE_HANDLE_SIGNALS;
   if (length > STRING_BYTES_BOUND)
     {
@@ -573,6 +619,8 @@ 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;
   return lisp_to_value (env, make_user_ptr (fin, ptr));
 }
 
@@ -580,7 +628,8 @@ static void *
 module_get_user_ptr (emacs_env *env, emacs_value uptr)
 {
   check_main_thread ();
-  eassert (module_non_local_exit_check (env) == emacs_funcall_exit_return);
+  if (module_non_local_exit_check (env) != emacs_funcall_exit_return)
+    return NULL;
   Lisp_Object lisp = value_to_lisp (uptr);
   if (! USER_PTRP (lisp))
     {
@@ -594,7 +643,8 @@ static void
 module_set_user_ptr (emacs_env *env, emacs_value uptr, void *ptr)
 {
   check_main_thread ();
-  eassert (module_non_local_exit_check (env) == emacs_funcall_exit_return);
+  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);
@@ -605,7 +655,8 @@ static emacs_finalizer_function
 module_get_user_finalizer (emacs_env *env, emacs_value uptr)
 {
   check_main_thread ();
-  eassert (module_non_local_exit_check (env) == emacs_funcall_exit_return);
+  if (module_non_local_exit_check (env) != emacs_funcall_exit_return)
+    return NULL;
   Lisp_Object lisp = value_to_lisp (uptr);
   if (! USER_PTRP (lisp))
     {
@@ -620,7 +671,8 @@ module_set_user_finalizer (emacs_env *env, emacs_value uptr,
 			   emacs_finalizer_function fin)
 {
   check_main_thread ();
-  eassert (module_non_local_exit_check (env) == emacs_funcall_exit_return);
+  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);
@@ -631,7 +683,8 @@ static void
 module_vec_set (emacs_env *env, emacs_value vec, ptrdiff_t i, emacs_value val)
 {
   check_main_thread ();
-  eassert (module_non_local_exit_check (env) == emacs_funcall_exit_return);
+  if (module_non_local_exit_check (env) != emacs_funcall_exit_return)
+    return;
   Lisp_Object lvec = value_to_lisp (vec);
   if (! VECTORP (lvec))
     {
@@ -653,7 +706,8 @@ static emacs_value
 module_vec_get (emacs_env *env, emacs_value vec, ptrdiff_t i)
 {
   check_main_thread ();
-  eassert (module_non_local_exit_check (env) == emacs_funcall_exit_return);
+  if (module_non_local_exit_check (env) != emacs_funcall_exit_return)
+    return NULL;
   Lisp_Object lvec = value_to_lisp (vec);
   if (! VECTORP (lvec))
     {
@@ -675,7 +729,8 @@ static ptrdiff_t
 module_vec_size (emacs_env *env, emacs_value vec)
 {
   check_main_thread ();
-  eassert (module_non_local_exit_check (env) == emacs_funcall_exit_return);
+  if (module_non_local_exit_check (env) != emacs_funcall_exit_return)
+    return 0;
   Lisp_Object lvec = value_to_lisp (vec);
   if (! VECTORP (lvec))
     {
@@ -806,10 +861,12 @@ module_non_local_exit_signal_1 (emacs_env *env, Lisp_Object sym,
 				Lisp_Object data)
 {
   struct emacs_env_private *p = env->private_members;
-  eassert (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;
+  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;
+    }
 }
 
 static void
@@ -817,10 +874,12 @@ module_non_local_exit_throw_1 (emacs_env *env, Lisp_Object tag,
 			       Lisp_Object value)
 {
   struct emacs_env_private *p = env->private_members;
-  eassert (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;
+  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;
+    }
 }
 
 /* Module version of `wrong_type_argument'.  */

Re: Dynamic modules: emacs-module.c and signaling errors

[Philipp Stephani]

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

Eli Zaretskii <eliz@gnu.org> schrieb am Di., 24. Nov. 2015 um 20:41 Uhr:

> In "emacs -Q", load the modules/mod-test/mod-test module, then try
> this:
>
>   M-: (mod-test-sum "1" 2) RET
>
> Result: Emacs aborts.  This happens because the eassert in
> module_extract_integer aborts, when that function is called for the
> 2nd time:
>
>   static intmax_t
>   module_extract_integer (emacs_env *env, emacs_value n)
>   {
>     check_main_thread ();
>     eassert (module_non_local_exit_check (env) ==
> emacs_funcall_exit_return);
>     Lisp_Object l = value_to_lisp (n);
>     if (! INTEGERP (l))
>       {
>         module_wrong_type (env, Qintegerp, l);
>         return 0;
>       }
>
> The first call to module_extract_integer correctly detects the wrong
> type of argument and calls module_wrong_type.  But module_wrong_type
> just records the problem in the env structure, it doesn't signal any
> Lisp error, like an Emacs primitive would.  So the actual error goes
> undetected, and is masked by the assertion violation (because Emacs is
> built with --enable-checking).
>
> Since this obviously works as it was designed, my question is: how
> should a module be written so that this kind of errors signal a normal
> Lisp error we are accustomed with Emacs primitives?
>
>
I still need to fix most of the example module (unfortunately that was
written before the error handling got implemented), but the gist is that
functions returning emacs_value should usually be called like this:

emacs_value ret = env->foo(env, ...);
if (ret == NULL) return NULL;

This will cause errors to be reported back to Emacs, where they are thrown
as normal signals.

I have thought about how we can prevent some accidental misuse. I propose
that all environment functions should have a signature like this:

__attribute__((warn_unused_result))
bool (*func)(emacs_env *env, ..., result_type* result);

instead of returning result directly. This way users get a warning if they
forget to check for errors. The above example would then turn into:

emacs_value ret;
if (!env->foo(env, ..., &ret)) return false;

which isn't worse, but avoids sentinel values and makes the API harder to
misuse. Thoughts?

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

Re: Dynamic modules: emacs-module.c and signaling errors

[Stefan Monnier]

I'll just mention that having to learn a whole new way to run Elisp
functions when writing module code instead of core code just
sucks rocks!

It presumes Emacs modules are mainly written by non-Emacs developers.
And it makes it harder for module writers to graduate to
Emacs contributors.
Not to mention that it makes the interface between Emacs modules and
Emacs core less efficient.

> The alternative is also error-prone though.  It's very easy to forget
> that some cleanup is needed;

The alternative is what we use inside Emacs core.  Maybe it's not
perfect but it worked fine for the last 30 years, with interfaces to
fairly varied libraries, like libX11, libxml2, libgnutls, ...
I think it has a pretty good track record.


        Stefan

Re: Dynamic modules: emacs-module.c and signaling errors

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Date: Wed, 25 Nov 2015 16:19:37 -0500
> 
> I'll just mention that having to learn a whole new way to run Elisp
> functions when writing module code instead of core code just
> sucks rocks!
> 
> It presumes Emacs modules are mainly written by non-Emacs developers.
> And it makes it harder for module writers to graduate to
> Emacs contributors.

I agree that having an entirely different paradigm would be
sub-optimal, to say the least.  But I don't quite see it happening in
this case.

What are the main differences between writing core C code and module C
code?  Here's what I see:

  . The need to call Emacs functions via a pointer.

    IOW, instead of calling, say, 'intern', you need to call
    'env->intern' instead.

    This is not a terrible pain, IMO, and we could make it even less
    so, with 2 measures:

    - Make all the functions in the API have the same names as their
      Emacs siblings.

      Most of them already are; for the rest, we could change the
      names, so that, e.g., vec_size is called ASIZE and vec_set is
      called ASET.

    - Have emacs-module.h define macros that would eliminate the
      'env->' part altogether, so that you can just call 'intern',
      'ASET', etc.

  . Lisp objects are declared as 'emacs_value' instead of
    'Lisp_Object'.

    Once again, we could rename the 'emacs_value' typedef to, say,
    'module_Lisp_Object' or some such.  (I don't recommend using
    'Lisp_Object', because IMO we should make the distinction
    explicit.)

  . You cannot freely assign Lisp objects.

    IOW, instead of this:

     Lisp_Object foo = Vsome_variable;

    you need to do this:

     emacs_value foo = env->intern (env, "some-variable");

    and similarly with function calls:

     emacs_value bar = env->funcall (env, env->intern (env, "list"), 3, args);

    But again, with some minimal syntactic sugar, like

     emacs_value Qlist = env->intern (env, "list");

    you can almost have the core syntax back.  And uses of Vfoo in the
    core is possible because we do

     Vfoo = intern ("foo");

    in the initialization code; modules could do the same in their
    init function, and then they'll have this sugar as well.

  . A different (and somewhat awkward) way of doing the equivalent of
    DEFUN and 'provide'.

    I think we should have macros MODULE_DEFUN and MODULE_PROVIDE for
    this.  They are straightforward to write.  I don't think it's
    reasonable to ask each module author to invent the code one can
    see near the end of mod-test.c, it's fairly boilerplate, and will
    most probably be present in any module hence.

  . Signaling an error and 'throw'

    Once again, we should wrap them into convenient macros that look
    similar enough to a call to xsignal or Fthrow.

Are there any other problems?

Maybe you are thinking about emacs-module.c itself.  It does include
some fairly arcane code that is meant to support what was deemed as a
safer interface.  But emacs-module.c is a small file, so I think we
can sustain its somewhat unusual techniques, especially if we add
there a commentary with detailed rules for how each function there
should be written.

> Not to mention that it makes the interface between Emacs modules and
> Emacs core less efficient.

Maybe I'm missing something, but I don't see it that way.  The
interface between the core and a module is the normal Lisp
interpreter; once the module's functions and global variables are
registered, they are just normal Lisp functions and variables.  Is
this incorrect?

The module implementation itself is indeed slightly less efficient
than if we'd code the same functionality directly in core.  But so
what? it's still very close to raw C efficiency, and paying a small
price for the extensibility is well worth it, I think.

> > The alternative is also error-prone though.  It's very easy to forget
> > that some cleanup is needed;
> 
> The alternative is what we use inside Emacs core.  Maybe it's not
> perfect but it worked fine for the last 30 years, with interfaces to
> fairly varied libraries, like libX11, libxml2, libgnutls, ...
> I think it has a pretty good track record.

That's not really an alternative, as doing that means you'd need to
recompile Emacs after adding code to core.

The real question is: what is the alternative _interface_ between
Emacs and the modules that would look like what we do with GnuTLS
etc.?  Do you mean we should have exported all the internal Emacs
functions, and let modules call them directly?  Or do you mean
something else?  IOW, you'd have to present a fairly complete
alternative design, before we could be able to discuss an alternative.

And all those libraries you've mentioned don't need any cleanup when
application code longjmp's.  So this, too, will have to be part of any
alternative design.