bug#65796: dynamic module non_local_exit_get overwrites exit signals

bug#65796: dynamic module non_local_exit_get overwrites exit signals

[Xinyang Chen]

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

Currently `module_non_local_exit_get` returns pointers to fields
in emacs_env_private:
```
  if (p->pending_non_local_exit != emacs_funcall_exit_return)
    {
      *symbol = &p->non_local_exit_symbol;
      *data = &p->non_local_exit_data;
    }
```
this means that if one tries to:
```
funcall(...);
non_local_exit_get(&s1, &d1);
funcall(...);
non_local_exit_get(&s2, &d2);
non_local_exit_signal(s1, d1);
```
you would signal the second error, instead of the first error (I expected
this to happen).
It seems to me that `module_non_local_exit_get` should
`allocate_emacs_value` instead.

-Alan

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

bug#65796: dynamic module non_local_exit_get overwrites exit signals

[Eli Zaretskii]

> From: Xinyang Chen <chenxinyang99@gmail.com>
> Date: Wed, 6 Sep 2023 18:52:14 -0400
> 
> Currently `module_non_local_exit_get` returns pointers to fields
> in emacs_env_private:
> ```
>   if (p->pending_non_local_exit != emacs_funcall_exit_return)
>     {
>       *symbol = &p->non_local_exit_symbol;
>       *data = &p->non_local_exit_data;
>     }
> ```
> this means that if one tries to:
> ```
> funcall(...);
> non_local_exit_get(&s1, &d1);
> funcall(...);
> non_local_exit_get(&s2, &d2);
> non_local_exit_signal(s1, d1);
> ```
> you would signal the second error, instead of the first error (I expected
> this to happen).
> It seems to me that `module_non_local_exit_get` should
> `allocate_emacs_value` instead.

Philipp, Daniel: any comments?

Btw, the non_local_exit_get function is currently not documented in
the ELisp manual; should it be?

bug#65796: dynamic module non_local_exit_get overwrites exit signals

[Philipp Stephani via Bug reports for GNU Emacs, the Swiss army knife of text editors]

On Thu, 7 Sept 2023 at 09:07, Eli Zaretskii <eliz@gnu.org> wrote:
>
> > From: Xinyang Chen <chenxinyang99@gmail.com>
> > Date: Wed, 6 Sep 2023 18:52:14 -0400
> >
> > Currently `module_non_local_exit_get` returns pointers to fields
> > in emacs_env_private:
> > ```
> >   if (p->pending_non_local_exit != emacs_funcall_exit_return)
> >     {
> >       *symbol = &p->non_local_exit_symbol;
> >       *data = &p->non_local_exit_data;
> >     }
> > ```
> > this means that if one tries to:
> > ```
> > funcall(...);
> > non_local_exit_get(&s1, &d1);
> > funcall(...);
> > non_local_exit_get(&s2, &d2);
> > non_local_exit_signal(s1, d1);
> > ```
> > you would signal the second error, instead of the first error (I expected
> > this to happen).
> > It seems to me that `module_non_local_exit_get` should
> > `allocate_emacs_value` instead.
>
> Philipp, Daniel: any comments?

Nice find!
We can't use allocate_emacs_value here because non_local_exit_get has
to work in OOM situations and can never fail. What we could do here is
e.g.:
- Document the current behavior, stating that the emacs_value objects
returned from non_local_exit_get are ephemeral. I'm not a huge fan of
this because it makes non_local_exit_get behave different from all
other functions.
- Provide an alternative non_local_exit_copy that copies the 2
Lisp_Objects into an opaque buffer supplied by the user (plus a way to
determine the buffer size). That way we shift the responsibility of
dealing with allocation failures to the user.
- Attempt to allocate a new emacs_value, fall back to the current
behavior if that fails. I don't really like that option either because
it doesn't solve the initial problem in all cases (so users still need
to deal with it), but makes both the interface and the implementation
more complex.
- Crash if we can't allocate memory. That has been rejected in other cases.

>
> Btw, the non_local_exit_get function is currently not documented in
> the ELisp manual; should it be?

At least in Emacs 29 I see it documented ("Module Nonlocal" node).


--
Google Germany GmbH
Erika-Mann-Straße 33
80636 München

Geschäftsführer: Paul Manicle, Liana Sebastian
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg

Diese E-Mail ist vertraulich. Falls Sie diese fälschlicherweise
erhalten haben sollten, leiten Sie diese bitte nicht an jemand anderes
weiter, löschen Sie alle Kopien und Anhänge davon und lassen Sie mich
bitte wissen, dass die E-Mail an die falsche Person gesendet wurde.

This e-mail is confidential. If you received this communication by
mistake, please don’t forward it to anyone else, please erase all
copies and attachments, and please let me know that it has gone to the
wrong person.

bug#65796: dynamic module non_local_exit_get overwrites exit signals

[Eli Zaretskii]

> From: Philipp Stephani <phst@google.com>
> Date: Thu, 7 Sep 2023 12:24:03 +0200
> Cc: Xinyang Chen <chenxinyang99@gmail.com>, Daniel Colascione <dancol@dancol.org>, 65796@debbugs.gnu.org, 
> 	p.stephani2@gmail.com
> 
> Nice find!
> We can't use allocate_emacs_value here because non_local_exit_get has
> to work in OOM situations and can never fail. What we could do here is
> e.g.:
> - Document the current behavior, stating that the emacs_value objects
> returned from non_local_exit_get are ephemeral. I'm not a huge fan of
> this because it makes non_local_exit_get behave different from all
> other functions.
> - Provide an alternative non_local_exit_copy that copies the 2
> Lisp_Objects into an opaque buffer supplied by the user (plus a way to
> determine the buffer size). That way we shift the responsibility of
> dealing with allocation failures to the user.
> - Attempt to allocate a new emacs_value, fall back to the current
> behavior if that fails. I don't really like that option either because
> it doesn't solve the initial problem in all cases (so users still need
> to deal with it), but makes both the interface and the implementation
> more complex.
> - Crash if we can't allocate memory. That has been rejected in other cases.

I guess just one alternative is acceptable?

> > Btw, the non_local_exit_get function is currently not documented in
> > the ELisp manual; should it be?
> 
> At least in Emacs 29 I see it documented ("Module Nonlocal" node).

I need new glasses.