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

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

[Patrick Palka]

This patch fixes the annoying bug where key sequences such as Alt_F or
Alt_B (go forward or backwards by a word) do not behave promptly in TUI.
You have to press a third key in order for the key sequence to register.

This is mostly ncurses' fault.  Calling wgetch() normally causes ncurses
to read only a single key from stdin.  However if the key read is the
start-sequence key (^[ a.k.a. ESC) then wgetch() reads TWO keys from
stdin, storing the 2nd key into an internal FIFO buffer and returning
the start-sequence key.  The extraneous read of the 2nd key makes us
miss its corresponding stdin event, so the event loop blocks until a
third key is pressed.  This explains why such key sequences do not
behave promptly in TUI.

To fix this issue, we must somehow compensate for the missed stdin event
corresponding to the 2nd byte of a key sequence.  This patch achieves
this by hacking  up the stdin event handler to conditionally execute the
readline callback multiple multiple times in a row.  This is done via a
new global variable, call_stdin_event_handler_again_p, which is set from
tui_getc() when we receive a start-sequence key and notice extra pending
input in the ncurses buffer.

Tested on x86_64-unknown-linux-gnu.
---
 gdb/event-top.c  | 13 ++++++++++++-
 gdb/event-top.h  |  1 +
 gdb/tui/tui-io.c | 22 +++++++++++++++++++++-
 3 files changed, 34 insertions(+), 2 deletions(-)

diff --git a/gdb/event-top.c b/gdb/event-top.c
index 833f49d..df1351d 100644
--- a/gdb/event-top.c
+++ b/gdb/event-top.c
@@ -120,6 +120,11 @@ int exec_done_display_p = 0;
    read commands from.  */
 int input_fd;
 
+/* Used by the stdin event handler to compensate for missed stdin events.
+   Setting this value inside an stdin callback makes the callback run
+   again.  */
+int call_stdin_event_handler_again_p;
+
 /* Signal handling variables.  */
 /* Each of these is a pointer to a function that the event loop will
    invoke if the corresponding signal has received.  The real signal
@@ -370,7 +375,13 @@ stdin_event_handler (int error, gdb_client_data client_data)
       quit_command ((char *) 0, stdin == instream);
     }
   else
-    (*call_readline) (client_data);
+    {
+      do
+	{
+	  call_stdin_event_handler_again_p = 0;
+	  (*call_readline) (client_data);
+	} while (call_stdin_event_handler_again_p != 0);
+    }
 }
 
 /* Re-enable stdin after the end of an execution command in
diff --git a/gdb/event-top.h b/gdb/event-top.h
index 2d05d45..c7b1224 100644
--- a/gdb/event-top.h
+++ b/gdb/event-top.h
@@ -61,6 +61,7 @@ extern void (*call_readline) (void *);
 extern void (*input_handler) (char *);
 extern int input_fd;
 extern void (*after_char_processing_hook) (void);
+extern int call_stdin_event_handler_again_p;
 
 extern void cli_command_loop (void *);
 
diff --git a/gdb/tui/tui-io.c b/gdb/tui/tui-io.c
index a890678..32cc96e 100644
--- a/gdb/tui/tui-io.c
+++ b/gdb/tui/tui-io.c
@@ -695,7 +695,27 @@ tui_getc (FILE *fp)
     TUI_CMD_WIN->detail.command_info.curch = 0;
   if (ch == KEY_BACKSPACE)
     return '\b';
-  
+
+  if (async_command_editing_p && key_is_start_sequence (ch))
+    {
+      int ch_pending;
+
+      nodelay (w, TRUE);
+      ch_pending = wgetch (w);
+      nodelay (w, FALSE);
+
+      /* If we have pending input following a start sequence, call the event
+	 handler again.  ncurses may have already read and stored the input into
+	 its internal buffer, meaning that we won't get an stdin event for it.  If we
+	 don't compensate for this missed stdin event, key sequences as Alt_F (^[f)
+	 will not behave promptly.  */
+      if (ch_pending != ERR)
+	{
+	  ungetch (ch_pending);
+	  call_stdin_event_handler_again_p = 1;
+	}
+    }
+
   return ch;
 }
 
-- 
2.1.0

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

[Pedro Alves]

Hey there,

Thanks for looking at this!

On 08/22/2014 09:44 PM, Patrick Palka wrote:
> This patch fixes the annoying bug where key sequences such as Alt_F or
> Alt_B (go forward or backwards by a word) do not behave promptly in TUI.
> You have to press a third key in order for the key sequence to register.
> 
> This is mostly ncurses' fault.  Calling wgetch() normally causes ncurses
> to read only a single key from stdin.  However if the key read is the
> start-sequence key (^[ a.k.a. ESC) then wgetch() reads TWO keys from
> stdin, storing the 2nd key into an internal FIFO buffer and returning
> the start-sequence key.  The extraneous read of the 2nd key makes us
> miss its corresponding stdin event, so the event loop blocks until a
> third key is pressed.  This explains why such key sequences do not
> behave promptly in TUI.
> 
> To fix this issue, we must somehow compensate for the missed stdin event
> corresponding to the 2nd byte of a key sequence.  This patch achieves
> this by hacking  up the stdin event handler to conditionally execute the
> readline callback multiple multiple times in a row.  This is done via a
> new global variable, call_stdin_event_handler_again_p, which is set from
> tui_getc() when we receive a start-sequence key and notice extra pending
> input in the ncurses buffer.

Hmm, I've been staring at this for a while trying to make sense of the
whole thing.  I think there's a larger issue here.

This happens because we enable the "keypad" behavior.  From the ncurses manual:

 The  keypad  option enables the keypad of the user's terminal.  If enabled (bf is TRUE), the user can press a function key (such as an arrow key) and wgetch re‐
 turns a single value representing the function key, as in KEY_LEFT.  If disabled (bf is FALSE), curses does not treat function keys specially  and  the  program
 has to interpret the escape sequences itself.  If the keypad in the terminal can be turned on (made to transmit) and off (made to work locally), turning on this
 option causes the terminal keypad to be turned on when wgetch is called.  The default value for keypad is false.

readline must already be processing escape sequences itself, so
then why we do enable this in the first place?  The answer is that
we want to intercept things like up/down -- when the TUI is active,
those should scroll the source window instead of navigating readline's
history.

This is done in tui_getc:

  if (key_is_command_char (ch))
    {				/* Handle prev/next/up/down here.  */
      ch = tui_dispatch_ctrl_char (ch);
    }

Hacking away keypad, like:

 gdb/tui/tui.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gdb/tui/tui.c b/gdb/tui/tui.c
index c30b76c..6174c0f 100644
--- a/gdb/tui/tui.c
+++ b/gdb/tui/tui.c
@@ -398,7 +398,7 @@ tui_enable (void)
       tui_show_frame_info (0);
       tui_set_layout (SRC_COMMAND, TUI_UNDEFINED_REGS);
       tui_set_win_focus_to (TUI_SRC_WIN);
-      keypad (TUI_CMD_WIN->generic.handle, TRUE);
+      // keypad (TUI_CMD_WIN->generic.handle, TRUE);
       wrefresh (TUI_CMD_WIN->generic.handle);
       tui_finish_init = 0;
     }

indeed makes Alt_F, etc. work.

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?

Thanks,
Pedro Alves

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

[Patrick Palka]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 5484 bytes --]

On Wed, 27 Aug 2014, Pedro Alves wrote:

> Hey there,
>
> Thanks for looking at this!
>
> On 08/22/2014 09:44 PM, Patrick Palka wrote:
>> This patch fixes the annoying bug where key sequences such as Alt_F or
>> Alt_B (go forward or backwards by a word) do not behave promptly in TUI.
>> You have to press a third key in order for the key sequence to register.
>>
>> This is mostly ncurses' fault.  Calling wgetch() normally causes ncurses
>> to read only a single key from stdin.  However if the key read is the
>> start-sequence key (^[ a.k.a. ESC) then wgetch() reads TWO keys from
>> stdin, storing the 2nd key into an internal FIFO buffer and returning
>> the start-sequence key.  The extraneous read of the 2nd key makes us
>> miss its corresponding stdin event, so the event loop blocks until a
>> third key is pressed.  This explains why such key sequences do not
>> behave promptly in TUI.
>>
>> To fix this issue, we must somehow compensate for the missed stdin event
>> corresponding to the 2nd byte of a key sequence.  This patch achieves
>> this by hacking  up the stdin event handler to conditionally execute the
>> readline callback multiple multiple times in a row.  This is done via a
>> new global variable, call_stdin_event_handler_again_p, which is set from
>> tui_getc() when we receive a start-sequence key and notice extra pending
>> input in the ncurses buffer.
>
> Hmm, I've been staring at this for a while trying to make sense of the
> whole thing.  I think there's a larger issue here.
>
> This happens because we enable the "keypad" behavior.  From the ncurses manual:
>
> The  keypad  option enables the keypad of the user's terminal.  If enabled (bf is TRUE), the user can press a function key (such as an arrow key) and wgetch re‐
> turns a single value representing the function key, as in KEY_LEFT.  If disabled (bf is FALSE), curses does not treat function keys specially  and  the  program
> has to interpret the escape sequences itself.  If the keypad in the terminal can be turned on (made to transmit) and off (made to work locally), turning on this
> option causes the terminal keypad to be turned on when wgetch is called.  The default value for keypad is false.
>
> readline must already be processing escape sequences itself, so
> then why we do enable this in the first place?  The answer is that
> we want to intercept things like up/down -- when the TUI is active,
> those should scroll the source window instead of navigating readline's
> history.
>
> This is done in tui_getc:
>
>  if (key_is_command_char (ch))
>    {				/* Handle prev/next/up/down here.  */
>      ch = tui_dispatch_ctrl_char (ch);
>    }
>
> Hacking away keypad, like:
>
> gdb/tui/tui.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/gdb/tui/tui.c b/gdb/tui/tui.c
> index c30b76c..6174c0f 100644
> --- a/gdb/tui/tui.c
> +++ b/gdb/tui/tui.c
> @@ -398,7 +398,7 @@ tui_enable (void)
>       tui_show_frame_info (0);
>       tui_set_layout (SRC_COMMAND, TUI_UNDEFINED_REGS);
>       tui_set_win_focus_to (TUI_SRC_WIN);
> -      keypad (TUI_CMD_WIN->generic.handle, TRUE);
> +      // keypad (TUI_CMD_WIN->generic.handle, TRUE);
>       wrefresh (TUI_CMD_WIN->generic.handle);
>       tui_finish_init = 0;
>     }
>
> indeed makes Alt_F, etc. work.
>
> 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.

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.

Patrick

>
> Thanks,
> Pedro Alves
>

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

[Pedro Alves]

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

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

[Pedro Alves]

On 08/22/2014 09:44 PM, Patrick Palka wrote:
> +
> +  if (async_command_editing_p && key_is_start_sequence (ch))

I think the key_is_start_sequence check means that we'll
fail to compensate in case the sequence if longer than
2 bytes?  That is, we'll compensate for the second char,
but fail to compensate for the third, because by then,
ch will not be a start sequence key.

> +    {
> +      int ch_pending;
> +
> +      nodelay (w, TRUE);
> +      ch_pending = wgetch (w);
> +      nodelay (w, FALSE);
> +

Thanks,
Pedro Alves

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

[Patrick Palka]

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
>

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

[Patrick Palka]

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.

Is the internal timeout a big deal though?  The handling of the target
event just gets temporarily delayed, not missed entirely, right?  And
isn't this timeout only experienced when one presses ESC (which has no
use in TUI) and/or attempts to manually type a function key sequence?
I'm not sure why anyone would do that.

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

Not sure it's possible to make wgetch() behave this way.  From what I
can tell, wgetch() will always return the key from its fifo if there's
one available -- it won't check whether the fifo contents + a new key
from stdin will make a complete sequence.  It won't even read from
stdin if there's a key in the fifo.

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

Yeah same here.  I can't seem to find the magical invocation that
actually disables this timeout.

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

I am not sure that it's possible to eliminate the internal timeout
completely so it seems to me that  your thread-based solution may be
necessary to fix this.  But is it worth the complexity to fix this
seemingly obscure issue?

>
> Thanks,
> Pedro Alves
>

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

[Patrick Palka]

On Thu, Aug 28, 2014 at 7:31 AM, Pedro Alves <palves@redhat.com> wrote:
> On 08/22/2014 09:44 PM, Patrick Palka wrote:
>> +
>> +  if (async_command_editing_p && key_is_start_sequence (ch))
>
> I think the key_is_start_sequence check means that we'll
> fail to compensate in case the sequence if longer than
> 2 bytes?  That is, we'll compensate for the second char,
> but fail to compensate for the third, because by then,
> ch will not be a start sequence key.

Yes I think so.  I only took into account common 2-byte sequences such
as Alt_F.  I suppose that the check could be removed to account for
3+-byte key sequences too, but I haven't thought out the consequences
of such change.  And I'm not sure that readline uses any of such
sequences.

>
>> +    {
>> +      int ch_pending;
>> +
>> +      nodelay (w, TRUE);
>> +      ch_pending = wgetch (w);
>> +      nodelay (w, FALSE);
>> +
>
> Thanks,
> Pedro Alves
>

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

[Pedro Alves]

On 08/28/2014 03:13 PM, Patrick Palka wrote:
> 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.
> 
> Is the internal timeout a big deal though?  The handling of the target
> event just gets temporarily delayed, not missed entirely, right?  And
> isn't this timeout only experienced when one presses ESC (which has no
> use in TUI) and/or attempts to manually type a function key sequence?
> I'm not sure why anyone would do that.

Right.  TBC, I noticed/found this while ramping up for reviewing this patch,
reading the ncurses code and trying to make sense of the whole ncurses/tui
integration and your patch.  I'm not _that_ familiar with this
code, although I'm probably one of the most familiar nowadays...

My main motivation for pointing this out was to brainstorm.  I had
no idea if there was a canned solution for this, which your or someone
might know about.  :-)  I wasn't really sure of the complexity of the
solution until I spent a few more hours this morning working through
what it would take and write out those steps list...

> Yeah same here.  I can't seem to find the magical invocation that
> actually disables this timeout.

Thanks for poking.

>> 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...
> 
> I am not sure that it's possible to eliminate the internal timeout
> completely so it seems to me that  your thread-based solution may be
> necessary to fix this.  But is it worth the complexity to fix this
> seemingly obscure issue?

Yeah, we certainly shouldn't let the perfect be the enemy of the
good.

I'd think using vi-mode keybindings would need it, but according
to PR11383 those don't work anyway.

The thread-based solution would make your patch unnecessary,
but then again, that's easy to revert if a thread-based solution
ends up written.

I think we understand the problems and the effort required
for the potential solutions enough to be able to firmly
say: yes, let's proceed with your solution.

Thanks,
Pedro Alves

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

[Pedro Alves]

On 08/28/2014 03:18 PM, Patrick Palka wrote:
> On Thu, Aug 28, 2014 at 7:31 AM, Pedro Alves <palves@redhat.com> wrote:
>> On 08/22/2014 09:44 PM, Patrick Palka wrote:
>>> +
>>> +  if (async_command_editing_p && key_is_start_sequence (ch))
>>
>> I think the key_is_start_sequence check means that we'll
>> fail to compensate in case the sequence if longer than
>> 2 bytes?  That is, we'll compensate for the second char,
>> but fail to compensate for the third, because by then,
>> ch will not be a start sequence key.
> 
> Yes I think so.  I only took into account common 2-byte sequences such
> as Alt_F.  I suppose that the check could be removed to account for
> 3+-byte key sequences too, but I haven't thought out the consequences
> of such change.  And I'm not sure that readline uses any of such
> sequences.

It's fine with me to leave this as is, if we add a comment
mentioning this issue.

Thanks,
Pedro Alves

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

[Patrick Palka]

On Thu, Aug 28, 2014 at 11:59 AM, Pedro Alves <palves@redhat.com> wrote:
> On 08/28/2014 03:13 PM, Patrick Palka wrote:
>> 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.
>>
>> Is the internal timeout a big deal though?  The handling of the target
>> event just gets temporarily delayed, not missed entirely, right?  And
>> isn't this timeout only experienced when one presses ESC (which has no
>> use in TUI) and/or attempts to manually type a function key sequence?
>> I'm not sure why anyone would do that.
>
> Right.  TBC, I noticed/found this while ramping up for reviewing this patch,
> reading the ncurses code and trying to make sense of the whole ncurses/tui
> integration and your patch.  I'm not _that_ familiar with this
> code, although I'm probably one of the most familiar nowadays...
>
> My main motivation for pointing this out was to brainstorm.  I had
> no idea if there was a canned solution for this, which your or someone
> might know about.  :-)  I wasn't really sure of the complexity of the
> solution until I spent a few more hours this morning working through
> what it would take and write out those steps list...

Indeed.. it certainly doesn't help that the ncurses source code is
pretty difficult reading.

>
>> Yeah same here.  I can't seem to find the magical invocation that
>> actually disables this timeout.
>
> Thanks for poking.
>
>>> 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...
>>
>> I am not sure that it's possible to eliminate the internal timeout
>> completely so it seems to me that  your thread-based solution may be
>> necessary to fix this.  But is it worth the complexity to fix this
>> seemingly obscure issue?
>
> Yeah, we certainly shouldn't let the perfect be the enemy of the
> good.
>
> I'd think using vi-mode keybindings would need it, but according
> to PR11383 those don't work anyway.

Interesting, I'll take a look at this aspect.

>
> The thread-based solution would make your patch unnecessary,
> but then again, that's easy to revert if a thread-based solution
> ends up written.
>
> I think we understand the problems and the effort required
> for the potential solutions enough to be able to firmly
> say: yes, let's proceed with your solution.

Thanks Pedro, for your insightful and detailed review.

>
> Thanks,
> Pedro Alves
>

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

[Patrick Palka]

On Thu, Aug 28, 2014 at 12:13 PM, Pedro Alves <palves@redhat.com> wrote:
> On 08/28/2014 03:18 PM, Patrick Palka wrote:
>> On Thu, Aug 28, 2014 at 7:31 AM, Pedro Alves <palves@redhat.com> wrote:
>>> On 08/22/2014 09:44 PM, Patrick Palka wrote:
>>>> +
>>>> +  if (async_command_editing_p && key_is_start_sequence (ch))
>>>
>>> I think the key_is_start_sequence check means that we'll
>>> fail to compensate in case the sequence if longer than
>>> 2 bytes?  That is, we'll compensate for the second char,
>>> but fail to compensate for the third, because by then,
>>> ch will not be a start sequence key.
>>
>> Yes I think so.  I only took into account common 2-byte sequences such
>> as Alt_F.  I suppose that the check could be removed to account for
>> 3+-byte key sequences too, but I haven't thought out the consequences
>> of such change.  And I'm not sure that readline uses any of such
>> sequences.
>
> It's fine with me to leave this as is, if we add a comment
> mentioning this issue.

I'll do that and send a revised patch later today.

>
> Thanks,
> Pedro Alves
>

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

[Patrick Palka]

On Thu, Aug 28, 2014 at 10:13 AM, Patrick Palka <patrick@parcs.ath.cx> wrote:
> 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.
>
> Is the internal timeout a big deal though?  The handling of the target
> event just gets temporarily delayed, not missed entirely, right?  And
> isn't this timeout only experienced when one presses ESC (which has no
> use in TUI) and/or attempts to manually type a function key sequence?
> I'm not sure why anyone would do that.
>
>>
>> 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.
>
> Not sure it's possible to make wgetch() behave this way.  From what I
> can tell, wgetch() will always return the key from its fifo if there's
> one available -- it won't check whether the fifo contents + a new key
> from stdin will make a complete sequence.  It won't even read from
> stdin if there's a key in the fifo.
>
>>       (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...:
>
> Yeah same here.  I can't seem to find the magical invocation that
> actually disables this timeout.

So it looks like the (non-standard) ncurses variable ESCDELAY is what
controls the timeout:  wgetch() sets the timeout to ESCDELAY
milliseconds.  So do you suppose that we should set this variable

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

[Patrick Palka]

On Thu, Aug 28, 2014 at 1:41 PM, Patrick Palka <patrick@parcs.ath.cx> wrote:
> So it looks like the (non-standard) ncurses variable ESCDELAY is what
> controls the timeout:  wgetch() sets the timeout to ESCDELAY
> milliseconds.  So do you suppose that we should set this variable

(Ugh, stupid gmail.)

do you suppose that we should set this variable, perhaps inside
tui_enable()?  To a small value or to zero?  Setting it to zero might
create issues with slow terminals and slow network connections.

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

[Patrick Palka]

[-- Attachment #1: Type: text/plain, Size: 1195 bytes --]

On Thu, Aug 28, 2014 at 12:13 PM, Pedro Alves <palves@redhat.com> wrote:
> On 08/28/2014 03:18 PM, Patrick Palka wrote:
>> On Thu, Aug 28, 2014 at 7:31 AM, Pedro Alves <palves@redhat.com> wrote:
>>> On 08/22/2014 09:44 PM, Patrick Palka wrote:
>>>> +
>>>> +  if (async_command_editing_p && key_is_start_sequence (ch))
>>>
>>> I think the key_is_start_sequence check means that we'll
>>> fail to compensate in case the sequence if longer than
>>> 2 bytes?  That is, we'll compensate for the second char,
>>> but fail to compensate for the third, because by then,
>>> ch will not be a start sequence key.
>>
>> Yes I think so.  I only took into account common 2-byte sequences such
>> as Alt_F.  I suppose that the check could be removed to account for
>> 3+-byte key sequences too, but I haven't thought out the consequences
>> of such change.  And I'm not sure that readline uses any of such
>> sequences.
>
> It's fine with me to leave this as is, if we add a comment
> mentioning this issue.
>
> Thanks,
> Pedro Alves
>

Attached is the patch that Pedro approved, augmented with a comment
describing the potential problem with 3+ byte sequences.  I wonder if
someone could commit this for me?

[-- Attachment #2: 0001-Fix-the-processing-of-Meta-key-commands-in-TUI.patch --]
[-- Type: application/octet-stream, Size: 4568 bytes --]

From 0a88f9e0277442d586dc8dcb93164d02f922f071 Mon Sep 17 00:00:00 2001
From: Patrick Palka <patrick@parcs.ath.cx>
Date: Fri, 22 Aug 2014 16:26:51 -0400
Subject: [PATCH] Fix the processing of Meta-key commands in TUI

This patch fixes the annoying bug where key sequences such as Alt_F or
Alt_B (go forward or backwards by a word) do not behave promptly in TUI.
You have to press a third key in order for the key sequence to register.

This is mostly ncurses' fault.  Calling wgetch() normally causes ncurses
to read only a single key from stdin.  However if the key read is the
start-sequence key (^[ a.k.a. ESC) then wgetch() reads TWO keys from
stdin, storing the 2nd key into an internal FIFO buffer and returning
the start-sequence key.  The extraneous read of the 2nd key makes us
miss its corresponding stdin event, so the event loop blocks until a
third key is pressed.  This explains why such key sequences do not
behave promptly in TUI.

To fix this issue, we must somehow compensate for the missed stdin event
corresponding to the 2nd byte of a key sequence.  This patch achieves
this by hacking  up the stdin event handler to conditionally execute the
readline callback multiple times in a row.  This is done via a new
global variable, call_stdin_event_handler_again_p, which is set from
tui_getc() when we receive a start-sequence key and notice extra pending
input in the ncurses buffer.

Tested on x86_64-unknown-linux-gnu.

gdb/ChangeLog:

	* event-top.h (call_stdin_event_handler_again_p): Declare.
	* event-top.c (call_stdin_event_handler_again_p): Define.
	(stdin_event_handler): Use it.
	* tui/tui-io.c (tui_getc): Prepare to call the stdin event
	handler again if there is pending input following a
	start sequence.
---
 gdb/event-top.c  | 13 ++++++++++++-
 gdb/event-top.h  |  1 +
 gdb/tui/tui-io.c | 28 +++++++++++++++++++++++++++-
 3 files changed, 40 insertions(+), 2 deletions(-)

diff --git a/gdb/event-top.c b/gdb/event-top.c
index 3f9deec..9654a0f 100644
--- a/gdb/event-top.c
+++ b/gdb/event-top.c
@@ -119,6 +119,11 @@ int exec_done_display_p = 0;
    read commands from.  */
 int input_fd;
 
+/* Used by the stdin event handler to compensate for missed stdin events.
+   Setting this to a non-zero value inside an stdin callback makes the callback
+   run again.  */
+int call_stdin_event_handler_again_p;
+
 /* Signal handling variables.  */
 /* Each of these is a pointer to a function that the event loop will
    invoke if the corresponding signal has received.  The real signal
@@ -369,7 +374,13 @@ stdin_event_handler (int error, gdb_client_data client_data)
       quit_command ((char *) 0, stdin == instream);
     }
   else
-    (*call_readline) (client_data);
+    {
+      do
+	{
+	  call_stdin_event_handler_again_p = 0;
+	  (*call_readline) (client_data);
+	} while (call_stdin_event_handler_again_p != 0);
+    }
 }
 
 /* Re-enable stdin after the end of an execution command in
diff --git a/gdb/event-top.h b/gdb/event-top.h
index 2d05d45..c7b1224 100644
--- a/gdb/event-top.h
+++ b/gdb/event-top.h
@@ -61,6 +61,7 @@ extern void (*call_readline) (void *);
 extern void (*input_handler) (char *);
 extern int input_fd;
 extern void (*after_char_processing_hook) (void);
+extern int call_stdin_event_handler_again_p;
 
 extern void cli_command_loop (void *);
 
diff --git a/gdb/tui/tui-io.c b/gdb/tui/tui-io.c
index 601d278..aa1d1c7 100644
--- a/gdb/tui/tui-io.c
+++ b/gdb/tui/tui-io.c
@@ -691,7 +691,33 @@ tui_getc (FILE *fp)
     TUI_CMD_WIN->detail.command_info.curch = 0;
   if (ch == KEY_BACKSPACE)
     return '\b';
-  
+
+  if (async_command_editing_p && key_is_start_sequence (ch))
+    {
+      int ch_pending;
+
+      nodelay (w, TRUE);
+      ch_pending = wgetch (w);
+      nodelay (w, FALSE);
+
+      /* If we have pending input following a start sequence, call the stdin
+	 event handler again because ncurses may have already read and stored
+	 the input into its internal buffer, meaning that we won't get an stdin
+	 event for it.  If we don't compensate for this missed stdin event, key
+	 sequences as Alt_F (^[f) will not behave promptly.
+
+	 (We only compensates for the missed 2nd byte of a key sequence because
+	 2-byte sequences are by far the most commonly used. ncurses may have
+	 buffered a larger, 3+-byte key sequence though it remains to be seen
+	 whether it is useful to compensate for all the bytes of such
+	 sequences.)  */
+      if (ch_pending != ERR)
+	{
+	  ungetch (ch_pending);
+	  call_stdin_event_handler_again_p = 1;
+	}
+    }
+
   return ch;
 }
 
-- 
2.2.0.rc1.23.gf570943

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

[Joel Brobecker]

[-- Attachment #1: Type: text/plain, Size: 774 bytes --]

Patrick,

> Attached is the patch that Pedro approved, augmented with a comment
> describing the potential problem with 3+ byte sequences.  I wonder if
> someone could commit this for me?
[...]
> gdb/ChangeLog:
> 
> 	* event-top.h (call_stdin_event_handler_again_p): Declare.
> 	* event-top.c (call_stdin_event_handler_again_p): Define.
> 	(stdin_event_handler): Use it.
> 	* tui/tui-io.c (tui_getc): Prepare to call the stdin event
> 	handler again if there is pending input following a
> 	start sequence.

Your patch did not apply to today's sources. The conflict was trivially
resolved, but it would be good if you could check the "master" branch
out, and rebuild, to make sure your fix was correctly pushed.

Attached is what I did push.

Thanks for the patch.
-- 
Joel

[-- Attachment #2: 0001-Fix-the-processing-of-Meta-key-commands-in-TUI.patch --]
[-- Type: text/x-diff, Size: 5332 bytes --]

From d64e57faa89ba4de0ebacdc30fbee5d19310950c Mon Sep 17 00:00:00 2001
From: Patrick Palka <patrick@parcs.ath.cx>
Date: Sun, 23 Nov 2014 14:03:39 +0400
Subject: [PATCH] Fix the processing of Meta-key commands in TUI

This patch fixes the annoying bug where key sequences such as Alt_F or
Alt_B (go forward or backwards by a word) do not behave promptly in TUI.
You have to press a third key in order for the key sequence to register.

This is mostly ncurses' fault.  Calling wgetch() normally causes ncurses
to read only a single key from stdin.  However if the key read is the
start-sequence key (^[ a.k.a. ESC) then wgetch() reads TWO keys from
stdin, storing the 2nd key into an internal FIFO buffer and returning
the start-sequence key.  The extraneous read of the 2nd key makes us
miss its corresponding stdin event, so the event loop blocks until a
third key is pressed.  This explains why such key sequences do not
behave promptly in TUI.

To fix this issue, we must somehow compensate for the missed stdin event
corresponding to the 2nd byte of a key sequence.  This patch achieves
this by hacking  up the stdin event handler to conditionally execute the
readline callback multiple times in a row.  This is done via a new
global variable, call_stdin_event_handler_again_p, which is set from
tui_getc() when we receive a start-sequence key and notice extra pending
input in the ncurses buffer.

Tested on x86_64-unknown-linux-gnu.

gdb/ChangeLog:

	* event-top.h (call_stdin_event_handler_again_p): Declare.
	* event-top.c (call_stdin_event_handler_again_p): Define.
	(stdin_event_handler): Use it.
	* tui/tui-io.c (tui_getc): Prepare to call the stdin event
	handler again if there is pending input following a
	start sequence.
---
 gdb/ChangeLog    |  9 +++++++++
 gdb/event-top.c  | 13 ++++++++++++-
 gdb/event-top.h  |  1 +
 gdb/tui/tui-io.c | 28 +++++++++++++++++++++++++++-
 4 files changed, 49 insertions(+), 2 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index e16f28f..326a0d6 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,14 @@
 2014-11-23  Patrick Palka  <patrick@parcs.ath.cx>
 
+	* event-top.h (call_stdin_event_handler_again_p): Declare.
+	* event-top.c (call_stdin_event_handler_again_p): Define.
+	(stdin_event_handler): Use it.
+	* tui/tui-io.c (tui_getc): Prepare to call the stdin event
+	handler again if there is pending input following a
+	start sequence.
+
+2014-11-23  Patrick Palka  <patrick@parcs.ath.cx>
+
 	Pushed by Joel Brobecker  <brobecker@adacore.com>
 	* linux-fork.c (checkpoint_command): Print index of new
 	checkpoint in response message.
diff --git a/gdb/event-top.c b/gdb/event-top.c
index 282c0fe..cb438ac 100644
--- a/gdb/event-top.c
+++ b/gdb/event-top.c
@@ -119,6 +119,11 @@ int exec_done_display_p = 0;
    read commands from.  */
 int input_fd;
 
+/* Used by the stdin event handler to compensate for missed stdin events.
+   Setting this to a non-zero value inside an stdin callback makes the callback
+   run again.  */
+int call_stdin_event_handler_again_p;
+
 /* Signal handling variables.  */
 /* Each of these is a pointer to a function that the event loop will
    invoke if the corresponding signal has received.  The real signal
@@ -420,7 +425,13 @@ stdin_event_handler (int error, gdb_client_data client_data)
       quit_command ((char *) 0, stdin == instream);
     }
   else
-    (*call_readline) (client_data);
+    {
+      do
+	{
+	  call_stdin_event_handler_again_p = 0;
+	  (*call_readline) (client_data);
+	} while (call_stdin_event_handler_again_p != 0);
+    }
 }
 
 /* Re-enable stdin after the end of an execution command in
diff --git a/gdb/event-top.h b/gdb/event-top.h
index ac0d47b..919287e 100644
--- a/gdb/event-top.h
+++ b/gdb/event-top.h
@@ -61,6 +61,7 @@ extern void (*call_readline) (void *);
 extern void (*input_handler) (char *);
 extern int input_fd;
 extern void (*after_char_processing_hook) (void);
+extern int call_stdin_event_handler_again_p;
 
 /* Wrappers for rl_callback_handler_remove and
    rl_callback_handler_install that keep track of whether the callback
diff --git a/gdb/tui/tui-io.c b/gdb/tui/tui-io.c
index 601d278..aa1d1c7 100644
--- a/gdb/tui/tui-io.c
+++ b/gdb/tui/tui-io.c
@@ -691,7 +691,33 @@ tui_getc (FILE *fp)
     TUI_CMD_WIN->detail.command_info.curch = 0;
   if (ch == KEY_BACKSPACE)
     return '\b';
-  
+
+  if (async_command_editing_p && key_is_start_sequence (ch))
+    {
+      int ch_pending;
+
+      nodelay (w, TRUE);
+      ch_pending = wgetch (w);
+      nodelay (w, FALSE);
+
+      /* If we have pending input following a start sequence, call the stdin
+	 event handler again because ncurses may have already read and stored
+	 the input into its internal buffer, meaning that we won't get an stdin
+	 event for it.  If we don't compensate for this missed stdin event, key
+	 sequences as Alt_F (^[f) will not behave promptly.
+
+	 (We only compensates for the missed 2nd byte of a key sequence because
+	 2-byte sequences are by far the most commonly used. ncurses may have
+	 buffered a larger, 3+-byte key sequence though it remains to be seen
+	 whether it is useful to compensate for all the bytes of such
+	 sequences.)  */
+      if (ch_pending != ERR)
+	{
+	  ungetch (ch_pending);
+	  call_stdin_event_handler_again_p = 1;
+	}
+    }
+
   return ch;
 }
 
-- 
1.9.1

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

[Patrick Palka]

On Sun, Nov 23, 2014 at 5:08 AM, Joel Brobecker <brobecker@adacore.com> wrote:
> Patrick,
>
>> Attached is the patch that Pedro approved, augmented with a comment
>> describing the potential problem with 3+ byte sequences.  I wonder if
>> someone could commit this for me?
> [...]
>> gdb/ChangeLog:
>>
>>       * event-top.h (call_stdin_event_handler_again_p): Declare.
>>       * event-top.c (call_stdin_event_handler_again_p): Define.
>>       (stdin_event_handler): Use it.
>>       * tui/tui-io.c (tui_getc): Prepare to call the stdin event
>>       handler again if there is pending input following a
>>       start sequence.
>
> Your patch did not apply to today's sources. The conflict was trivially
> resolved, but it would be good if you could check the "master" branch
> out, and rebuild, to make sure your fix was correctly pushed.

Looks like the patch was correctly pushed.  Thanks Joel.

>
> Attached is what I did push.
>
> Thanks for the patch.
> --
> Joel