GNU bug report logs - #78898
Make read/readevalloop reentrant

Previous Next

Full log

View this message in rfc822 format

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Lynn Winebarger <owinebar <at> gmail.com>
Cc: 78898 <at> debbugs.gnu.org
Subject: bug#78898: Make read/readevalloop reentrant
Date: Sun, 29 Jun 2025 13:03:38 -0400

> (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.

Any hope you can give us an actual concrete scenario?

>> 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.

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 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`.

Given the patch size, I barely started looking at it, but see some
comments below.


        Stefan


> -#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.

> +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_*

> +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.

Also, why do you need the "uninit" case?


        Stefan

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.