Subject: Re: bug#75593: 31.0.50; Faulty macro kills Emacs

From: Eli Zaretskii <[email-masked]>
Date: Thu, 16 Jan 2025 13:36:08 +0200

> Date: Thu, 16 Jan 2025 10:38:21 +0000
> From: Pip Cet <[email-masked]>
> Cc: [email-masked], [email-masked], [email-masked], [email-masked]
> 
> "Eli Zaretskii" <[email-masked]> writes:
> 
> >> Date: Wed, 15 Jan 2025 22:20:42 +0000
> >> From: Pip Cet <[email-masked]>
> >> Cc: Eli Zaretskii <[email-masked]>, Alexander Prähauser <[email-masked]>, [email-masked]
> >>
> >> --- 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.