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

[Patrick Palka <patrick at parcs dot ath dot cx>]

On Thu, Aug 28, 2014 at 7:22 AM, Pedro Alves <palves@redhat.com> wrote:
> 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
>