GNU bug report logs -
#75593
31.0.50; Faulty macro kills Emacs
Previous Next
Full log
View this message in rfc822 format
> Date: Thu, 16 Jan 2025 10:38:21 +0000
> From: Pip Cet <pipcet <at> protonmail.com>
> Cc: monnier <at> iro.umontreal.ca, mattiase <at> acm.org, ahprae <at> protonmail.com, 75593 <at> debbugs.gnu.org
>
> "Eli Zaretskii" <eliz <at> gnu.org> writes:
>
> >> Date: Wed, 15 Jan 2025 22:20:42 +0000
> >> From: Pip Cet <pipcet <at> protonmail.com>
> >> Cc: Eli Zaretskii <eliz <at> gnu.org>, Alexander Prähauser <ahprae <at> protonmail.com>, 75593 <at> debbugs.gnu.org
> >>
> >> --- a/src/eval.c
> >> +++ b/src/eval.c
> >> @@ -2446,8 +2446,11 @@ DEFUN ("eval", Feval, Seval, 1, 2, 0,
> >> (Lisp_Object form, Lisp_Object lexical)
> >> {
> >> specpdl_ref count = SPECPDL_INDEX ();
> >> - specbind (Qinternal_interpreter_environment,
> >> - CONSP (lexical) || NILP (lexical) ? lexical : list_of_t);
> >> + if (CONSP (lexical) || NILP (lexical))
> >> + Flength (lexical);
> >> + else
> >> + lexical = list_of_t;
> >> + specbind (Qinternal_interpreter_environment, lexical);
> >> return unbind_to (count, eval_sub (form));
> >> }
> >
> > Isn't there a cheaper way of verifying LEXICAL is a list, not a cons
> > cell?
>
> I don't think there is, no. As I explained, it's unlikely to be a
> significant expense: lexical environments are not very large, usually,
> and all other users of Vinternal_interpreter_environment are optimized
> for small lists which are scanned until we find the right entry. If we
> wanted to optimize for large lexical environments, we might be able to,
> but we've decided not to do that.
OK, so can you then benchmark this? E.g., by byte-compiling all the
*.el files in the tree with and without the change? Or maybe there's
something appropriate in the benchmark suite? Or both?
> > How about calling Flength only "#ifdef ENABLE_CHECKING", and otherwise
> > just verifying that LEXICAL is a list?
>
> ENABLE_CHECKING shouldn't affect code behavior in such ways, no.
?? It already does. We discover problems and bugs earlier all the
time when running a build with --enable-checking, because it aborts
before it runs into a potentially fatal problem.
> I
> don't think a crash is acceptable here, but if people end up disabling
> ENABLE_CHECKING and using XCAR on a non-cons, a crash is the *best*
> possible outcome: more likely, we'd just be creating nonsensical
> Lisp_Objects which may cause memory corruption.
I think the best outcome is to signal an error, not to crash, if that
is feasible.
> Making behavior depend on ENABLE_CHECKING in such ways will cause bugs
> in !ENABLE_CHECKING builds that cannot be debugged because the
> corresponding ENABLE_CHECKING build will never reach the problematic
> code.
I wrote that under the assumption that we could test for a proper list
without calling Flength. If that is not possible/feasible, that point
is moot, and IMO we should only see if the run-time price is
acceptable.
> (Also, this is not primarily about circular lists, it's about non-lists
> in general).
That should be easy to detect up front, no? I thought the issue was
about a cons cell vs a proper list.
> > IOW, can we prevent crashes in a way that is cheap in production
> > builds, since 'eval' is quite a hotspot?
>
> If there's any evidence that lexenvs are large enough that scanning them
> linearly in this one place is a significant expense
I didn't say that; it remains to be established. Thus my suggestion
above to benchmark. But if there is a less expensive way, we could
take it regardless: one doesn't have to prove that a simple test is
less expensive than scanning the entire list structure.
This bug report was last modified 207 days ago.
Previous Next
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.