Stefan Monnier wrote:
> it seems that XCDR and XCAR here are
> safe because before calling those functions, eval_sub happens to call
> Flength on the args, and that triggers an error if the form is not
> a proper list, so `XCDR (args)` will indeed be a cons once we get
> to Fif.

That's true only if the S-expression does not mutate as it is being evaluated. 
If the S-expression modifies itself after Flength and before XCDR, Emacs can 
crash. So Alex's first patch is the opposite of what it should be: instead of 
changing the Fcar to an XCAR, we should change an XCDR to an Fcdr in the next 
line. I looked for nearby instances of this problem, and fixed them by 
installing the attached patch. This should address both problems that Alex 
mentioned.

> Arguably Fif could be called from elsewhere than eval_sub, and arguably
> eval_sub's implementation could be changed in such a way that it doesn't
> catch this error, so the safety of using XCDR is debatable.

Even before the attached patch was installed, several UNEVALLED functions 
assumed that their arguments were proper lists. So, when writing the 
abovementioned patch, I found it simpler to make this assumption everywhere. The 
argument lists might become improper if they are mutated, so UNEVALLED functions 
that can invoke arbitrary lisp code still must check the lists as they go, though.

> Fif should not be performance sensitive

Yes, in this part of eval.c using XCDR instead of Fcdr is helpful mostly as a 
hint to the reader that the object is a cons; it's not a significant performance 
improvement.