Subject: Re: bug#78898: Make read/readevalloop reentrant

From: Lynn Winebarger <[email-masked]>
Date: Sat, 28 Jun 2025 22:38:09 -0400

On Sat, Jun 28, 2025, 12:23 PM Stefan Monnier <[email-masked]>
wrote:

> > I want to use readevalloop to process command-line arguments, and my
> > initial attempts failed to handle '"(load "loadup.el") (princ
> > "Done")'.  So I took a stab at making all the entry points synchronize
> > with an explicit stack of continuation frames shared between read0 and
> > readevalloop.
>
> I'm afraid with the `Subject:` and the above paragraph, I'm still not
> able to figure out what you're trying to achieve (and the patch is
> quite large).
>
> Also `read` and `readevalloop` are quite different.  Do you want both to
> be re-entrant?  Is it in both cases for the same reason?
> AFAIK `readevalloop` is also re-entrant in that you can call it
> recursively from its "eval" part.  What do you mean by "process
> command-line arguments"?
>

It's true that read and readevalloop are distinct, but, ignoring the
readfun parameter of readevalloop, any static variables read0 is dependent
on, readevalloop is also dependent on.  It's also true that not every entry
to read0 is an entry to readevalloop, but practically every entry to
readevalloop will enter read0.  And they both depend on the state accessed
by readchar and unreadchar.  So, readevalloop can never really be reentrant
as long as read0/readchar/unreadchar are not.  My initial (current)
approach has been to identify all the private state in lread.c accessed
(directly or indirectly) and create a single data structure representing
that state as a collective stack frame for essentially all the procedures
of lread.c in the transitive closure of readevalloop 's call graph.  It's
true that this data structure is bigger than necessary, but readevalloop
only has to allocate one frame that will be used by each invocation of
read0 it makes in that activation frame of readevalloop.

Initially, I had tried implementing a simple read-eval loop for processing
sequences of expressions in a string, similar to "load" for files or
"eval-buffer".  I had implemented a new temacs mode, eval-args, which skips
loadup and all other command line options processing, to evaluate programs
given as command line arguments. I made read_internal_start reentrant with
respect to the obvious string state variables using unwind-protect and
specbind techniques.  However, it wasn't enough, because while it worked
for simple cases like "(princ comp-abi-hash) (terpri)" or even a simple
repl on stdin, the program '(load "loadup.el") (print "Finished")' would
complete loading load up, but immediately exit without printing
"Finished".  Or, if I typed '(load "loadup.el")' into that simple repl, it
would immediately exit after completing the load.

I considered that even if I determined the proximate cause of the issue, it
would be difficult to tell whether any particular solution would be TRT or
just a band-aid. So, I suppose the most immediate advantage for me is
improving my ability to debug recursive invocations of read0/readevalloop
by looking at the explicit stack of frames.  In lread.c's current state,
you can't tell just from looking at the static variables how or whether
they are related to one another, and in what order, at least not without
travelling along the backtrace and digging through the specpdl entries.

So, for example, I've implemented an input stream "reader-input-stream"
that can be used arbitrarily by code being evaluated for characters from
the "current" input stream, which is pretty much impossible in the current
implementation.  It's main use is for "readfun" in readevalloop.
Implementing "eval-stream", which evaluates all input streams as programs
uniformly, becomes trivial.

Another consideration is just the incremental removal of some barriers to
concurrency .  Really, the "current_frame" variable should be thread local,
the way specpdl variables are.  It just seemed like too much for my first
cut at this.  Obviously, it's just a drop in the bucket, but with the
progress of the igc branch, effective concurrency is looking more feasible.

That said, the patch was derived by dropping the lread.c from that other
project into a recent master, and hacking on it a bit to make it function
on a stand-alone basis.  So I can cut out some of the elements of the patch
that aren't really needed for re-entrancy.

Some of the changes (like moving a lot of definitions/declarations to the
top of the file) are just because I had to identify the functions that were
being changed to take the single "frame" parameter and keep track of them
somehow.  There was a lot of trial and error in figuring out what
information belonged to the "input" based on the type of stream it
represented, versus the fields that were properly part of the general logic
in read0/readevalloop/etc.  I'm sure there's room for improvement, but my
brain is a bit fried from combing over the code.

I also made a minor effort to differentiate between the case where
readevalloop initiates and eval and is responsible for handling errors,
versus the case where an intervening evaluation began due to an interrupt
(i.e. while processing input events or a timer in a maybe_quit()
somewhere).  It's half-baked because I only implemented it for calls to
maybe_quit() that appear in lread.c, just to see if I understood how I
might make it work.  In particular, at least for signals processed during
those particular maybe_quit calls, An unhandled end-of-file error generated
during one of those intervening evaluations will not be intercepted by
readevalloop and mistaken for ending the current reader stream.  Instead,
all such spurious signals are passed down to the entry point of that
activation of readevalloop (or read) and then.rethrown appropriately.  That
piece isn't really necessary, as any real solution will require overhauling
the way interrupts are handled.

At any rate, I'm working on cutting down the patch.  Pip asked that it be
split into smaller chunks.  I can do that, but if I don't think I'd want to
guarantee that the project would necessarily even compile after applying a
subset of the patches, even when applied in order.  Is that acceptable?

Lynn