> Am 29.01.2025 um 18:52 schrieb Xinyang Chen : > > This is still a problem. > > Using a user buffer will require gc-protecting it and thus a major overhaul, so I think it's not a good idea. Yeah, I figured out that approach is a dead end meanwhile. > > IMO what we should do is: if we fail to allocate, we discard the original signal and replace it with an OOM signal (pointing to constants so requiring no allocation). Yeah, that's a good idea, thanks for bringing it up. I've attached a patch to that effect. > Perhaps we should make a new field in emacs_funcall_exit for OOM, or we can just use emacs_funcall_exit_signal. My patch does the latter: Adding a new enum value risks UB if callers don't have a default case in their switch statements, behavior in OOM situations is best-effort anyway, and very careful callers can still compare the returned error symbol against the (documented) OOM symbol. > > Alternatively, make a copy_emacs_value function that allows the user to copy the signal out, returning NULL to let the caller know that an allocation failure occurred. I also considered that, but it puts too much onus on the module authors to deal with a situation that effectively never happens. > > > > On Thu, Sep 7, 2023 at 5:24 AM Philipp Stephani wrote: > On Thu, 7 Sept 2023 at 09:07, Eli Zaretskii wrote: > > > > > From: Xinyang Chen > > > 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).