From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp-out2.suse.de (smtp-out2.suse.de [IPv6:2001:67c:2178:6::1d]) by sourceware.org (Postfix) with ESMTPS id CF04F3858D1E for ; Tue, 13 Sep 2022 14:32:17 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org CF04F3858D1E Received: from imap2.suse-dmz.suse.de (imap2.suse-dmz.suse.de [192.168.254.74]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (P-521) server-digest SHA512) (No client certificate requested) by smtp-out2.suse.de (Postfix) with ESMTPS id DA7F05C26D; Tue, 13 Sep 2022 14:32:16 +0000 (UTC) Received: from imap2.suse-dmz.suse.de (imap2.suse-dmz.suse.de [192.168.254.74]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (P-521) server-digest SHA512) (No client certificate requested) by imap2.suse-dmz.suse.de (Postfix) with ESMTPS id C6C4613AB5; Tue, 13 Sep 2022 14:32:16 +0000 (UTC) Received: from dovecot-director2.suse.de ([192.168.254.65]) by imap2.suse-dmz.suse.de with ESMTPSA id aEsyL3CUIGOVPAAAMHmgww (envelope-from ); Tue, 13 Sep 2022 14:32:16 +0000 Message-ID: Date: Tue, 13 Sep 2022 16:32:16 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.12.0 Subject: Re: [RFC][gdb] Handle pending ^C after rl_callback_read_char Content-Language: en-US To: Simon Marchi , gdb-patches@sourceware.org References: <20220913125619.GA15500@delia.home> <9c5a599c-faf3-940b-745b-3528cd72fcdb@simark.ca> From: Tom de Vries In-Reply-To: <9c5a599c-faf3-940b-745b-3528cd72fcdb@simark.ca> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-12.9 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, NICE_REPLY_A, SPF_HELO_NONE, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 13 Sep 2022 14:32:19 -0000 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