Subject: bug#78898: Make read/readevalloop reentrant

From: Pip Cet <[email-masked]>
Date: Sat, 28 Jun 2025 14:57:53 +0000

"Lynn Winebarger" <[email-masked]> writes:

> On Fri, Jun 27, 2025 at 5:42 AM Pip Cet <[email-masked]> wrote:
>>
>> "Lynn Winebarger" <[email-masked]> writes:
>>
>> Thanks for providing an updated patch!  It applies and, with the changes
>> below, it seems to work for me.
>>
>> I haven't really looked at the code yet, just tried to get it to work.
>> I did notice that, at least with debug CFLAGS, Emacs startup is much
>> slower than it was before.
>
> That's interesting - I use the debug CFLAGS, and I was surprised that
> I didn't notice any slowdown at startup.

It's entirely possible it's just the nativecomp abi hash changing, I
didn't think of that!  In any case, it's too early to benchmark these
changes, IMHO.

In particular, I tried running "make check", and while I've reduced the
number of local test failures a little (see below), there are still some
mysterious ones.

> I replaced a bunch of static variables with indirect references to a
> stack-allocated "frame" data structure, so I expected some
> pessimization.

That doesn't sound like it would be noticeable, at least when
optimization is on.

> I did use "register" for the frame parameter, in the hope that the
> compiler would at least generate code that wouldn't be much worse than
> for ordinary stack-allocated variables.

I think "register" provides no benefit (it makes non-optimizing GCC put
things into registers, sometimes, which is undesirable for debugging)
and harms readability, but others disagree.

> There are other optimization opportunities, I just got bitten by
> Knuth's "root of all evil" enough that I gave up on optimizing before
> getting the logic right.

Sounds like the right approach to me.

> Are you using native compilation in your build?  The comp-abi-hash
> changes since I added some subrs. Could you tell me the configuration?
> My build/test is on x86_64 Linux.

Same here.

> If I isolate the changes to just the re-entrancy, I could try making
> read0 a monolithic procedure so the read stack is a local variable,
> and making inline versions of the read char procedures in that
> procedure, and use the tricks in bytecode.c for using computed goto in
> read0 to use local variables uniformly in read0.  That should take
> care of some of the pessimization, at least.

I'd suggest attempting that only if it turns out to be absolutely
necessary!  That would hurt readability of this code a lot, and we need
more people to understand it.

>> Is there any possibility of splitting this into several smaller patches?
>> I get the impression it would be a lot of work, but maybe I'm wrong.
>
> Making read0/readevalloop re-entrant requires a global control-flow
> transformation, so most of it is necessary. Two pieces that aren't
> strictly necessary, but are in it because this was actually the last
> step in my initial development of a "noneditor" mode for running
> command-line type emacs-lisp programs with non-standard dump-files.

Even if it isn't possible to split the changeset into functional
patches, splitting it into separate patches may make the diffs more
readable purely for syntactic reasons.  The first patch could merely add
unused macro arguments to READCHAR etc; that would help diff (the
program), in subsequent patches, to anchor the diff in the right
place.  (For the same reason, I'd refrain from whitespace changes and
drive-by fixes).

> FIrst, the load_source_file step, because I want to try building a
> minimal command-line compiler for boot strapping and async compiling
> and think it ought to be able to handle all the source files in the
> emacs lisp distribution without having to load every character set and
> coding system available into the dump file.

> Second, I modified lisp_file_lexical_cookie to allow the
> lexical-binding cookie on any line in a leading comment block, rather
> than just the first line.  Now that the cookie is being warned about,
> I would prefer it if the system didn't force users to violate their
> style requirements by making the first line obscenely long.  However,
> it makes use of the "lread_rewind_input" to get around possible
> limitations on ungetc.
>
> If I remove those two pieces, I think the rest will probably work.

I think removing them for now would be a good idea.  The first change
sounds like a great idea, but does it really depend on a reentrant
reader?

>> > I tried explicitly forcing emacs-internal as the coding-system for
>> > load_source_file, but without any noticeable difference.
>>
>> Hmm.  I think the problem there is that you specbind
>> Qcoding_system_for_read to Qemacs_internal while calling
>> dump-emacs-portable.  The pdumper doesn't unwind specbind bindings, so
>> the Vcoding_system_for_read variable is dumped with that value, and
>> restored with that value from the dump, and then things go wrong because
>> other code assumes Vcoding_system_for_read is nil.
>
> I'm sure that is a big, but it didn't fix the problem for me (ignoring
> the the 2 non-fixes).

I'm not sure what "the problem" is here; IIRC, the pdumper change fixes
some of the differences in the generated .el files, while the non-fixes
can be dropped if we use coding = Qutf_8 instead of coding =
Qno_conversion in Fload.  (But I'm not sure that's the right fix).

> It looks like the real issue is the bootstrap generation of charset
> and coding system files by elisp programs run by bootstrap-emacs.
> Some of those generated files are basically compiled elisp, so should
> be loaded like elc files.  I'm not sure why they aren't generated as
> elc files in the first place.

I'm not sure how .elc files should be loaded, TBH.  Your current code
sets coding = Qno_conversion, but some of the .elc files (like
warnings.elc) contain unescaped UTF-8.  Setting coding = Qutf_8 seems to
work a bit better.

> I'm addressing that issue by building our a primitive set-auto-code
> function that detects coding cookies in the header, and modifying the
> lisp file generation code to create coding cookies instead of in the
> local variable section.  Then, check if the coding system is actually
> defined and use it if so, or no-conversion if not.  Then, I am only
> adding definitions for utf-8 and utf-8-emacs if the system is not
> dumping (but isn't initialized from a dump file either).  That should
> restore the previous behavior.  Maybe it will also remove some of the
> unidata-related fragility currently required in loadup.el, I'm not
> sure.

That sounds more complicated than what we do now.  I don't understand
what changed to make such additional complexities necessary, and I don't
see the connection between coding systems and making lread.c reentrant.

> Thanks for the additional notes, they have really helped!

Feel free to ignore this diff, it serves mostly to show up the places
where I changed something in an attempt to reduce test case failures
(some offsets expected as extracted from a larger diff).  The changes
themselves should be treated as highly questionable :-)

diff --git a/src/lread.c b/src/lread.c
index cb8559f455c..b9cb57c7a75 100644
--- a/src/lread.c
+++ b/src/lread.c
@@ -1217,7 +1218,7 @@ #define READCHAR_BEGIN(frame) \
   while (0)
 
 #define READCHAR_FROM_FILE_BEGIN(frame) \
-  unsigned char buf[4]; \
+  unsigned char buf[MAX_MULTIBYTE_LENGTH]; \
   READCHAR_BEGIN(frame); \
   c = readbyte_from_file (frame); \
   if (c < 0) \
@@ -3000,7 +3001,7 @@ DEFUN ("load", Fload, Sload, 1, 5, 0,
     }
   /* we may or may not need this */
   lread_frame this_frame = INIT_LREAD_FRAME ();
-  Lisp_Object coding = Qno_conversion;
+  Lisp_Object coding = Qutf_8;
   this_frame.loop_dynamic_frame = SPECPDL_INDEX();
   if (is_module || is_native_elisp)
     {
@@ -3891,7 +3892,8 @@ nonstandard_read (Lisp_Object frameptr)
 {
   register lread_frame *frame = xmint_pointer (frameptr);
   Lisp_Object val;
-  if (!NILP (frame->readfun))
+  if (!NILP (frame->readfun)
+      && !NILP (Fequal (frame->start, frame->end)))
     {
       val = calln (frame->readfun, Qreader_input_stream);
 
@@ -4124,7 +4126,7 @@ enter_buffer_readevalloop (Lisp_Object buffer,
     tem = printflag;
   if (NILP (filename))
     filename = BVAR (XBUFFER (buf), filename);
-  if (NILP (Ffile_name_absolute_p (filename)))
+  if (!NILP (filename) && NILP (Ffile_name_absolute_p (filename)))
     full_name = Fexpand_file_name (filename, Qnil);
   else
     full_name = filename;
@@ -7540,6 +7535,7 @@ activate_lread_frame (lread_frame *new_frame, lread_frame_type new_type)
   switch (frame->intype)
     {
     case LREAD_INPUT_FILE:
+    case LREAD_INPUT_FILE_MULE:
       if (frame->input.f.stream == stdin)
 	{
 	  frame->unread_char = stdin_infile.unread_char;

Pip