Re: [PATCH] Fix the processing of Meta-key commands in TUI

[Pedro Alves <palves at redhat dot com>]

On 08/27/2014 07:25 PM, Patrick Palka wrote:
> On Wed, 27 Aug 2014, Pedro Alves wrote:

>> The main reason I think there's a larger problem here, is that
>> if curses is reading more than one char from stdin, then that means
>> that is must be blocking for a bit waiting for the next character,
>> which is a no-no in an async world, where we want to be processing
>> target events at the same time.  The man page says:
>>
>>  While interpreting an input escape sequence, wgetch sets a timer while waiting for the next character.  If notimeout(win, TRUE) is called, then wgetch does not
>>  set a timer.  The purpose of the timeout is to differentiate between sequences received from a function key and those typed by a user.
>>
>> Looks like there's a default timeout of 1 second.  Indeed if I set a
>> breakpoint in wgetch and another right after wgetch is called, and
>> then I press escape, I see that gdb is stuck inside wgetch for around
>> one second.  During that time, gdb's own event loop isn't being processed.
>>
>> Not sure exactly how this is usually handled.  Seems like there
>> are several knobs that might be able to turn this delay off.
>> Sounds like we should enable that (whatever the option is),
>> and handle the timeout ourselves?
> 
> I don't think the timeout is the issue here.  Even if the timeout is
> disabled via notimeout(), wgetch() will still attempt to interpret keypad
> sequences by reading multiple characters from stdin -- except that the
> read will now be a non-blocking one instead of a blocking one.
> 
> One way or another, someone must read multiple keys from stdin in order
> to semantically distinguish between keypad keys and regular key
> sequences.  And when it turns out that the input is not or cannot be a
> keypad key then that someone must place the extraneous keys into a
> buffer and notify GDB's event handler that we missed their stdin events.

Right, that's a given.  What I was talking about is fixing the
1 second block in case the input stops before a sequence is complete.

> If we handle the timeout ourselves (for instance by disabling keypad()
> and enabling notimeout()) then we'll be responsible for doing the
> lookahead, interpreting the sequences and buffering the keypresses.  I
> say let ncurses continue to handle the timeout so that we'll only be
> responsible for notifying the event handler.
>
> Though I may just be misunderstanding your proposal.

The main idea was to not let ncurses ever block, as that prevents
gdb's event loop from handling target events.  If ncurses internally
already handled the timeout by comparing the time between
wgetch calls instead of doing a blocking select/poll internally, then
it'd be a bit easier, but it looks like it doesn't (GetEscdelay always
starts with the window's configured delay, on each wgetch call?), so we'd
need to track the timeout ourselves.  Even if it did, it wouldn't be that
different though.

What we'd need is:

 #1 - set ncurses to be _always_ non-blocking/no-delay.
 #2 - on input, call wgetch, as today.
 #3 - due to no-delay, that now only looks ahead for as long as
      there are already bytes ready to be read from the input file / stdin.
 #4 - if the sequence looks like a _known_ escape sequence, but
      it isn't complete yet, then wgetch leaves already-read bytes
      in the fifo, and returns ERR.  That's the "the keys stay uninterpreted"
      comment in lib_getch.c.
 #5 - at this point, we need to wait for either:
      (a) further input, in case further input finishes the sequence.
      (b) the timeout to elapse, meaning no further input, and we should
          pass the raw chars to readline.

For #5/(a), there's nothing to do, that's already what the
stdin handler does.

For #5/(b), because we don't want to block in the stdin handler (tui_getc)
blocked waiting for the timeout, we would instead install a timer in gdb's event
loop whose callback was just be the regular TUI stdin input handler.  This time, given
enough time had elapsed with no further input, we want the raw chars.  If ncurses
internally knows that sufficient time has passed, then good, we only have to
call wgetch, and we know ncurses gives us the raw keys (the incomplete sequence).
If it doesn't (looks like it doesn't, but I may be wrong), then we just temporarily
switch off the keypad, and read one char, which returns us the raw escape char at
the head of the fifo, and leaves the rest in the fifo for subsequent reads.
As the fifo is now missing the escape char, we can go back to normal, with the
keypad enabled, and the next time we call wgetch should return us the head of
the fifo immediately, if there's anything there.

Going back to step #4, in case the sequence is _unknown_ or the timeout
has expired:

 #4.2 - if the sequence in the fifo is definitely an _unknown_ escape
    sequence, wgetch returns the first char in the fifo, leaving the
    remainder of the bytes in the fifo.  TBC, in this case, we _don't_
    get back ERR.  As there are more bytes in the fifo, then we need
    to compensate for the missed stdin events, like in your patch.  (*)

(*) - so it sounds your patch would be necessary anyway.

Oddly, even when doing:

  nodelay (w, TRUE);
  notimeout (w, TRUE);

in tui_getc, I _still_ get that a one second block within wgetch...
Looks like related to mouse event handling, even though mouse
events were not enabled...:

(top-gdb) bt
#0  0x000000373bcea9c0 in __poll_nocancel () at ../sysdeps/unix/syscall-template.S:81
#1  0x00007f62bc49d43d in _nc_timed_wait (sp=0x18f7730, mode=3, milliseconds=1000, timeleft=0x0) at ../../ncurses/tty/lib_twait.c:265
#2  0x00007f62bc6bf484 in check_mouse_activity (sp=0x18f7730, delay=1000) at ../../ncurses/base/lib_getch.c:145
#3  0x00007f62bc6c00c8 in kgetch (sp=0x18f7730) at ../../ncurses/base/lib_getch.c:734
#4  0x00007f62bc6bfbec in _nc_wgetch (win=0x193e6a0, result=0x7fffdf59f2c8, use_meta=1) at ../../ncurses/base/lib_getch.c:513
#5  0x00007f62bc6bfe6e in wgetch (win=0x193e6a0) at ../../ncurses/base/lib_getch.c:643
#6  0x0000000000516d88 in tui_getc (fp=0x373bfb9640 <_IO_2_1_stdin_>) at /home/pedro/gdb/mygit/src/gdb/tui/tui-io.c:662
#7  0x000000000078385f in rl_read_key () at /home/pedro/gdb/mygit/src/readline/input.c:448
#8  0x000000000076b5a8 in _rl_subseq_getchar (key=27) at /home/pedro/gdb/mygit/src/readline/readline.c:658
#9  0x000000000076b5fb in _rl_dispatch_callback (cxt=0x1661190) at /home/pedro/gdb/mygit/src/readline/readline.c:680
#10 0x0000000000783d73 in rl_callback_read_char () at /home/pedro/gdb/mygit/src/readline/callback.c:170
#11 0x0000000000618ba9 in rl_callback_read_char_wrapper (client_data=0x0) at /home/pedro/gdb/mygit/src/gdb/event-top.c:167
#12 0x0000000000618f7f in stdin_event_handler (error=0, client_data=0x0) at /home/pedro/gdb/mygit/src/gdb/event-top.c:373
#13 0x0000000000617b72 in handle_file_event (data=...) at /home/pedro/gdb/mygit/src/gdb/event-loop.c:763
#14 0x0000000000617059 in process_event () at /home/pedro/gdb/mygit/src/gdb/event-loop.c:340
#15 0x0000000000617120 in gdb_do_one_event () at /home/pedro/gdb/mygit/src/gdb/event-loop.c:404
#16 0x0000000000617170 in start_event_loop () at /home/pedro/gdb/mygit/src/gdb/event-loop.c:429

If such delays/blocks can't be eliminated due to buggy ncurses, or
something missing in the APIs, then it looks like the only way
to fix this would be to move the wgetch call to a separate thread,
like, we'd create a pipe, and put one end in the event loop as
stdin source instead of the real stdin, and then the separate thread
would push the results of wgetch into this pipe...

Thanks,
Pedro Alves