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

Tom de Vries via Gdb-patches
2022-09-13T14:32:40Z

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