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

[Tom de Vries <tdevries@suse.de>]

On 9/13/22 16:15, Simon Marchi wrote:
> 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) ();
> 

Nice.

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

I don't know the answer to that.  For me the fix you suggest looks 
reasonable though.  And I actually prefer that one, because it might be 
possible to backport it to older system readlines.

I suppose we'll have to find out by asking the readline maintainer.

> In any case, I'm fine with your fix
> going in even if it's a temporary measure.
> 

Ack, I'll wait a bit for more comments, and then commit.  Then we can 
refer to current gdb source code when posing the question about readline.

Thanks,
- Tom