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

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

[Tom de Vries]

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.

Tested on x86_64-linux.

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=27813

Any comments?

Thanks,
- Tom

[gdb] Handle pending ^C after rl_callback_read_char

---
 gdb/event-top.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/gdb/event-top.c b/gdb/event-top.c
index 290c3d87744..00adbe8fb4f 100644
--- a/gdb/event-top.c
+++ b/gdb/event-top.c
@@ -186,6 +186,22 @@ gdb_rl_callback_read_char_wrapper_noexcept () noexcept
   TRY_SJLJ
     {
       rl_callback_read_char ();
+#if RL_VERSION_MAJOR >= 8
+      /* It can happen that readline (while in rl_callback_read_char)
+	 received a signal, but didn't handle it yet.  Make sure it's handled
+	 now.  If we don't do that we run into two related problems:
+	 - we have to wait for another event triggering
+	   rl_callback_read_char before the signal is handled
+	 - there's no guarantee that the signal will be processed before the
+	   event.  */
+      while (rl_pending_signal () != 0)
+	/* Do this in a while loop, in case rl_check_signals also leaves a
+	   pending signal.  I'm not sure if that's possible, but it seems
+	   better to handle the scenario than to assert.  */
+	rl_check_signals ();
+#else
+      /* Unfortunately, rl_check_signals is not available.  */
+#endif
       if (after_char_processing_hook)
 	(*after_char_processing_hook) ();
     }

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

[Simon Marchi]

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

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

[Tom de Vries]

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

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

[Tom de Vries]

On 9/13/22 16:32, Tom de Vries via Gdb-patches wrote:
> 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.

I've committed, and send an email to bug-readline.  I don't see it yet 
in the archives.

Thanks,
- Tom

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

[Simon Marchi]

On 9/16/22 10:28, Tom de Vries via Gdb-patches wrote:
> On 9/13/22 16:32, Tom de Vries via Gdb-patches wrote:
>> 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.
> 
> I've committed, and send an email to bug-readline.  I don't see it yet in the archives.
> 
> Thanks,
> - Tom

Here it is:

https://lists.gnu.org/archive/html/bug-readline/2022-09/msg00001.html

Thanks for following up.

Simon