From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca (simark.ca [158.69.221.121]) by sourceware.org (Postfix) with ESMTPS id 86A9F3858D1E for ; Tue, 13 Sep 2022 14:15:55 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 86A9F3858D1E Received: from [10.0.0.11] (unknown [217.28.27.60]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id 4793E1E0D5; Tue, 13 Sep 2022 10:15:55 -0400 (EDT) Message-ID: <9c5a599c-faf3-940b-745b-3528cd72fcdb@simark.ca> Date: Tue, 13 Sep 2022 10:15:54 -0400 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.2.0 Subject: Re: [RFC][gdb] Handle pending ^C after rl_callback_read_char Content-Language: en-US To: Tom de Vries , gdb-patches@sourceware.org References: <20220913125619.GA15500@delia.home> From: Simon Marchi In-Reply-To: <20220913125619.GA15500@delia.home> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-11.7 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, NICE_REPLY_A, SPF_HELO_PASS, 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:15:57 -0000 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