* [RFC][gdb] Handle pending ^C after rl_callback_read_char
@ 2022-09-13 12:56 Tom de Vries
2022-09-13 14:15 ` Simon Marchi
0 siblings, 1 reply; 5+ messages in thread
From: Tom de Vries @ 2022-09-13 12:56 UTC (permalink / raw)
To: gdb-patches
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) ();
}
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [RFC][gdb] Handle pending ^C after rl_callback_read_char
2022-09-13 12:56 [RFC][gdb] Handle pending ^C after rl_callback_read_char Tom de Vries
@ 2022-09-13 14:15 ` Simon Marchi
2022-09-13 14:32 ` Tom de Vries
0 siblings, 1 reply; 5+ messages in thread
From: Simon Marchi @ 2022-09-13 14:15 UTC (permalink / raw)
To: Tom de Vries, gdb-patches
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [RFC][gdb] Handle pending ^C after rl_callback_read_char
2022-09-13 14:15 ` Simon Marchi
@ 2022-09-13 14:32 ` Tom de Vries
2022-09-16 14:28 ` Tom de Vries
0 siblings, 1 reply; 5+ messages in thread
From: Tom de Vries @ 2022-09-13 14:32 UTC (permalink / raw)
To: Simon Marchi, gdb-patches
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [RFC][gdb] Handle pending ^C after rl_callback_read_char
2022-09-13 14:32 ` Tom de Vries
@ 2022-09-16 14:28 ` Tom de Vries
2022-09-16 14:50 ` Simon Marchi
0 siblings, 1 reply; 5+ messages in thread
From: Tom de Vries @ 2022-09-16 14:28 UTC (permalink / raw)
To: Simon Marchi, gdb-patches
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [RFC][gdb] Handle pending ^C after rl_callback_read_char
2022-09-16 14:28 ` Tom de Vries
@ 2022-09-16 14:50 ` Simon Marchi
0 siblings, 0 replies; 5+ messages in thread
From: Simon Marchi @ 2022-09-16 14:50 UTC (permalink / raw)
To: Tom de Vries, gdb-patches
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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-09-16 14:50 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-09-13 12:56 [RFC][gdb] Handle pending ^C after rl_callback_read_char Tom de Vries
2022-09-13 14:15 ` Simon Marchi
2022-09-13 14:32 ` Tom de Vries
2022-09-16 14:28 ` Tom de Vries
2022-09-16 14:50 ` Simon Marchi
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).