Re: [Emacs-diffs] master 5b5f441: read_key_sequence: correct the handling of raw_keybuf in recursive calls

[Alan Mackenzie <acm@muc.de>]

Hello, Stefan.

On Mon, Nov 20, 2017 at 16:43:52 -0500, Stefan Monnier wrote:
> >> But this `read_char` is within read_key_sequence: this read_key_sequence
> >> may have been called from anywhere, ....
> > read_key_sequence is static in keyboard.c, and is called from precisely
> > three places: command_loop_1, read_key_sequence_vs, and
> > read_menu_command.

> Right: the middle one corresponds to `read-key-sequence` which can be
> called from "anywhere" (i.e. Elisp).

That is surely not the problem.  The problem would be if something
aborted a read_key_sequence, and somehow started another one without
going via command_loop_1.

In the current formulation read_key_sequence_vs could have set the
pertinent pointer variables to the static ones as part of its
initialisation.  I didn't do that because it seemed unneeded.  But it
would be harmless, since there surely cannot be a recursive call to
read_key_sequence_vs.

> >> .... so after we exit it (non-locally), we may end up running
> >> arbitrary Elisp code before we return to command_loop_1, can't we?
> > Is it possible to exit non-locally from read_char (or one of its called
> > subroutines)?

> `read_char` will run timers and process filters.  See for example
> `read-key` which calls `read-key-sequence` with a timer that makes it
> exit by throwing `read-key` as soon as one key is detected.

I've had a closer look.  read_char does indeed call out to lisp.  I
can't yet see any place where a non-local exit could cause a re-entry
into read_key_sequence without re-initialising the static variables, but
it might well be there.

> > You have a point, here.  Perhaps it would be better to get storage from
> > the Emacs heap rather than using the stack.

> I like using the stack, here, actually.

I thought about that.  It would mean passing this buffer as a parameter
through lots of functions to get from where it's defined to where it
gets filled.  Here "lots" means a stack of, perhaps, 7 or 8 function
calls.  It would certainly be safer, though.

> Maybe another option is to make raw_keybuf local to read_key_sequence,
> and to *copy* it into the global raw_keybuf_buffer just before exiting.

That's quite close to what I've done.  The difficulty is in getting a
pointer to that local buffer for use by the menu handling code.

>         Stefan


> PS: Of course, even better would be to provide another way to get what
> `this-single-command-raw-keys` returns, so we don't need to use a global
> variable for it.  E.g. have `read-key-sequence` return both the key
> sequence and the raw key sequence.  But we'd still have to support
> `this-single-command-raw-keys` for the foreseeable future anyway.

I don't think that idea would fly.  Most uses of `read-key-sequence'
aren't going to be interested in the raw events.

-- 
Alan Mackenzie (Nuremberg, Germany).