Re: [RFC][gdb] Handle pending ^C after rl_callback_read_char

[Simon Marchi <simark@simark.ca>]

On 2022-09-13 08:56, Tom de Vries via Gdb-patches wrote:
> Hi,
> 
> In completion tests in various test-cases, we've been running into these
> "clearing input line" timeouts:
> ...
> (gdb) $cmd^GPASS: gdb.gdb/unittest.exp: tab complete "$cmd"
> FAIL: gdb.gdb/unittest.exp: tab complete "$cmd" (clearing input line) (timeout)
> ...
> where $cmd == "maintenance selftest name_that_does_not_exist".
> 
> AFAIU, the following scenario happens:
> - expect sends "$cmd\t"
> - gdb detects the stdin event, and calls rl_callback_read_char until it
>   comes to handle \t
> - readline interprets the \t as completion, tries to complete, fails to do so
>   outputs a bell (^G)
> - expect sees the bell, and proceeds to send ^C
> - readline is still in the call to rl_callback_read_char, and stores the
>   signal in _rl_caught_signal
> - readline returns from the call to rl_callback_read_char, without having
>   handled _rl_caught_signal
> - gdb goes to wait for the next event
> - expect times out waiting for "Quit", the expected reaction for ^C
> 
> Fix this by handling pending signals after each call to rl_callback_read_char.
> 
> The fix is only available for readline 8.x, if --with-system-readline provides
> an older version, then the fix is disabled due to missing function
> rl_check_signals.

Thanks a lot for looking into this.

So, readline installs its own sigint handler, which just saves the
signal number somewhere.  It then checks on it synchronously at
different points using RL_CHECK_SIGNALS.

I put a breakpoint on rl_clear_signals, which is where it restores the
old signal handlers, and I think that the window that is between the last call to
RL_CHECK_SIGNALS and rl_clear_signals (so, the window where the signal
number is just saved but the signal is not handled) is this:

  https://gitlab.com/gnutools/binutils-gdb/-/blob/e9a241e87b42f902d0408704df6bbcd8bf465a46/readline/readline/callback.c#L266-304

Putting a 50ms delay somewhere in this window, I can reproduce the
problem all the time, with your modified gdb.gdb/unittest.exp.

diff --git a/readline/readline/callback.c b/readline/readline/callback.c
index 93f23d97bc20..e5a0560fd5f9 100644
--- a/readline/readline/callback.c
+++ b/readline/readline/callback.c
@@ -264,6 +264,11 @@ rl_callback_read_char (void)
        eof = readline_internal_char ();
 
       RL_CHECK_SIGNALS ();
+
+      /* wait 50 ms */
+      for (int i = 0; i < 50; ++i)
+       usleep (1000);
+
       if (rl_done == 0 && _rl_want_redisplay)
        {
          (*rl_redisplay_function) ();

And then with your fix below apply, the test passes fine.  So, the fix
LGTM.  My only question would be, is this a readline bug?  Should
readline do:

 - block signals
 - call RL_CHECK_SIGNALS
 - restore old handlers
 - unblock signals

to avoid that window of lost signals?  Or is this (your fix) the way
readline is supposed to be used?  In any case, I'm fine with your fix
going in even if it's a temporary measure.

Simon