Package: emacs;
Reported by: Lynn Winebarger <owinebar <at> gmail.com>
Date: Wed, 25 Jun 2025 22:01:05 UTC
Severity: normal
Message #50 received at 78898 <at> debbugs.gnu.org (full text, mbox):
From: Lynn Winebarger <owinebar <at> gmail.com> To: Stefan Monnier <monnier <at> iro.umontreal.ca> Cc: 78898 <at> debbugs.gnu.org Subject: Re: bug#78898: Make read/readevalloop reentrant Date: Sun, 29 Jun 2025 22:28:34 -0400
On Sun, Jun 29, 2025 at 1:03 PM Stefan Monnier <monnier <at> iro.umontreal.ca> wrote:
>
> > (a) My personal immediate problem is evaluating arbitrary programs
> > supplied a user of a bare temacs as a command line argument, but I
> > don't think that is in itself very motivating for emacs maintainers
>
> That's still very vague:
>
> temacs .../foo.el
>
> satisfies my understanding of "evaluating arbitrary programs
> supplied [by] a user of a bare temacs as a command line argument" and
> I can't see why that would require a reentrant `read`.
>
> If you mean
>
> temacs ... '(progn (blabla))'
>
> I still can't see why you'd need a reentrant `read` for that.
Basically the latter, because I was looking at using it Makefiles and
didn't want to deal with generating little files to load. If I was
willing to limit the strings to single expressions, you'd be right,
but why should the form a program is input (string, file, buffer,
whatever) determine what's allowed?
I did try to get by with just save/restore on the string variables,
but it failed when evaluating '(load "loadup.el") (print "Finished")".
The failure certainly seems like an issue caused by some subtlety in
the way recursive use of readevalloop is implemented, but I can't
bring myself to try and figure out a workaround for the specific
brokenness when I could just rework it to be sure its reentrant, then
see if that fixes the issue. Then, even if I lost the bet that
re-entrancy was the issue, I"ve at least gotten something from the
effort, whereas if I just hack away looking for some magic fix I'll
never feel like I actually solved the issue.
Or maybe the problem I'm solving is my ignorance of how the emacs
runtime works in practice? A lack of amusing pasttimes? A stubborn
refusal to accept things that do not make sense to me?
>
> Any hope you can give us an actual concrete scenario?
>
I keep giving you one, you just consider it unnecessary.
> >> How/when&why do you expect/want `read` to be called recursively?
> > It can definitely be entered multiple times if a stream being read
> > blocks and the command loop executes another function that calls read.
> > That can clearly happen when reading from user-supplied procedures or
> > a raw stdio stream (i.e. from "load").
>
> A concrete scenario would help, here as well.
Ok, there are calls to maybe quit in readbyte_from_stdio where the
file returns EOF due to an interrupt. So, if one function is reading
a lisp expression when this occurs, then the user type "M-: (read
<something>)", you will have a recursive re-entry to read. Or any
other source of lisp evaluation that might happen while processing
events - who knows, it really has nothing to do with the read that is
stuck waiting for input (but not blocked).
>
> In any case, what I gather from this exchange is that your main
> motivation is to stop `read` relying so much on global variables just
> because It's Right (the benefits you describe don't seem to be ones you
> personally need).
I mean, I guess. I just don't like spending a lot of time
understanding the finer details of how a fragile system fails instead
of just simplifying it into a form where the fragility can be reduced
if not eliminated?
> I can definitely go along with that. The reason why I care so much
> about the motivation behind the change is that it affects the tradeoffs
> (e.g. impact on performance or code maintenance). Have you made any
> measurements about the impact on the speed for `read`?
> Emacs startup time is significantly affected by the time to `read`.
Not yet. I wanted to make sure it actually functioned and preserved
the intended behavior first. Although I personally haven't noticed
any real impact, and I expected to since I'm running -O0. Maybe it
would be more noticeable with optimization enabled - I don't know if
using the register keyword for the frame actually achieved my hope
that performance wouldn't be much worse than having to access local
variables from the stack. But I'm sure you know Knuth's maxim.
>
> Given the patch size, I barely started looking at it, but see some
> comments below.
>
Thanks!
> > -#define lread_fd_p (fd >= 0)
> > -#define lread_close emacs_close
> > -#define lread_fstat sys_fstat
> > -#define lread_read_quit emacs_read_quit
> > -#define lread_lseek lseek
> > -
> > -#define file_stream FILE *
> > -#define file_seek fseek
> > -#define file_stream_valid_p(p) (p)
> > -#define file_stream_close emacs_fclose
> > -#define file_stream_invalid NULL
> > -#define file_get_char getc
> > +#define lread_fd_p (fd >= 0)
> > +#define lread_close emacs_close
> > +#define lread_fstat sys_fstat
> > +#define lread_read_quit emacs_read_quit
> > +#define lread_lseek lseek
> > +
> > +#define file_stream FILE *
> > +#define file_seek fseek
> > +#define file_stream_valid_p(p) (p)
> > +#define file_stream_close emacs_fclose
> > +#define file_stream_invalid NULL
> > +#define file_get_char getc
>
> Please avoid changing whitespace in parts of the code you don't
> otherwise touch.
Yeah, I goofed on that.
> > +struct inbuffer
> > +{
> > + Lisp_Object buffer;
> > +};
> > +#define INIT_INBUFFER() \
> > + ((struct inbuffer) \
> > + { \
> > + .buffer = Qnil \
> > + } )
>
> I think INIT_INBUFFER can just as well be a static function. And more
> importantly, it would benefit from taking the buffer as argument so you
> don't have this intermediate state where the struct has been created but
> not yet really initialized (other than with those dummy values).
>
> Same for the other INIT_*
It's not really intended to be a procedure, just a static initializer.
I haven't regularly programmed in C since the decade I learned the
language from the recently published K&R 2nd edition, so my style
discipline is pretty low. I probably got bit by an identifier used
for member access getting expanded as an object-like macro and
started making all my macros function-like for a while.
As for the static initializers themselves, I'm happy to take some
guidance on style preferences. I'm just going by the emacs code base
and the material on cppreference.com. I kept on getting complaints
from gcc about the form of the initializers, so I ended up just
writing everything out explicitly just to be done with it. It took me
forever to work out a "missing braces" message was referring to a
specpdl_ref initializer in the INIT_LREAD_FRAME.
>
> > +typedef union lread_input
> > +{
> > + unsigned char uninit[LREAD_INPUT_SIZE];
> > + struct infile f;
> > + struct instring s;
> > + struct inbuffer b;
> > + struct inmarker m;
> > + struct inproc p;
> > + struct inindirect frame;
> > + struct innull n;
> > +} lread_input;
>
> I'd put the `lread_input_type` right alongside it inside a `struct`, so
> we know they always come in pairs.
The details of those data representations definitely need some
attention. I will probably fine-tune them to follow the approach used
by the pseudovector types.
>
> Also, why do you need the "uninit" case?
That's my unfamiliarity with C99 and zero-initializing a union and
being certain it zeros the entire thing and not just the first
alternative. It seemed like { 0 } wasn't zero initializing members of
structures that were also structures, but I could have been mistaken -
that wasn't the interesting thing I was working out, just an annoying
hoop to jump through at the time. I may just be waving a dead chicken
here.
Lynn
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.