From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp-out1.suse.de (smtp-out1.suse.de [195.135.220.28]) by sourceware.org (Postfix) with ESMTPS id 875143858418 for ; Sun, 8 May 2022 07:51:52 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 875143858418 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-out1.suse.de (Postfix) with ESMTPS id 03012219C3; Sun, 8 May 2022 07:51:39 +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 C89E113A97; Sun, 8 May 2022 07:51:38 +0000 (UTC) Received: from dovecot-director2.suse.de ([192.168.254.65]) by imap2.suse-dmz.suse.de with ESMTPSA id l5prL4p2d2JkeQAAMHmgww (envelope-from ); Sun, 08 May 2022 07:51:38 +0000 Message-ID: <0e439ffb-5ba2-8b7b-0a83-55fbba1a21cc@suse.de> Date: Sun, 8 May 2022 09:51:38 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.8.0 Subject: Re: [RFC][gdb] Handle ^D when terminal is not prepped Content-Language: en-US To: Andrew Burgess , gdb-patches@sourceware.org Cc: Andrew Burgess , Pedro Alves , Tom Tromey References: <20220408153656.GA32258@delia> <87y200q836.fsf@redhat.com> From: Tom de Vries In-Reply-To: <87y200q836.fsf@redhat.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-13.3 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, KAM_SHORT, NICE_REPLY_A, SPF_HELO_NONE, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) 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: Sun, 08 May 2022 07:51:57 -0000 On 4/20/22 13:59, Andrew Burgess wrote: > Tom de Vries via Gdb-patches writes: > >> Hi, >> >> When running test-case gdb.base/eof-exit.exp, I get: >> ... >> (gdb) ^M >> (gdb) quit^M >> PASS: gdb.base/eof-exit.exp: with non-dump terminal: close GDB with eof >> ... >> but when run in combination with taskset -c 0, I get instead: >> ... >> (gdb) ^M >> (gdb) FAIL: gdb.base/eof-exit.exp: default: close GDB with eof (timeout) >> ... >> >> The test-case is a bit unusual, in the sense that most test-cases wait for >> gdb to present the prompt '(gdb) ' before sending input. Instead, this >> test-case first sends a newline to the prompt: >> ... >> send_gdb "\n" >> ... >> to generate a new prompt, but rather than waiting for the new prompt, it just >> waits for the resulting newline: >> ... >> gdb_test_multiple "" "discard newline" { >> -re "^\r\n" { >> } >> } >> ... >> and then sends the End-of-Transmission character (ascii code 4, caret notation >> '^D'): >> ... >> send_gdb "\004" >> ... >> >> The purpose of this is to verify that the result is: >> ... >> (gdb) quit >> ... >> instead of: >> ... >> quit) >> ... >> >> Putting this together with the failure log above, it seems like the ^D has no >> observable effect. >> >> After adding some debugging code in readline, to verify what chars are read >> from stdin (call to read in rl_getc), we find that in the passing case we get >> '^D', but in the failing case '^@' (ascii code 0, '\0') instead. >> >> Readline treats the '^D' as EOF, and calls gdb's installed handler with NULL >> (meaning EOF), which then issues the quit. >> >> But readline does not invoke gdb's installed handler for the '^@', AFAIU >> because as per default keymap it treats it as the 'set mark' function. >> >> So, why does ^D end up as ^@? >> >> My theory is that this is due to "canonical mode" ( >> "https://man7.org/linux/man-pages/man3/tcflow.3.html" ): >> ... >> Input is made available line by line. An input line is >> available when one of the line delimiters is typed (NL, EOL, >> EOL2; or EOF at the start of line). Except in the case of EOF, >> the line delimiter is included in the buffer returned by read(2). >> ... >> >> So, if the line is empty to start out with, and then ^D is issued and >> interpreted by the terminal as EOF, it'll make available an empty line, in >> other words a pointer to char ^@. >> >> The canonical mode seems to be on by default, but is switched off when >> readline puts the terminal in "prepped" mode. >> >> Gdb switches forth and back between having the readline handler installed and >> not (which makes readline switch back and forth between "prepped" and >> "deprepped" mode), and even readline itself seems to switch back and forth >> internally, in rl_callback_read_char. So, apparantly the '^D' arrives at a >> moment when the terminal happens to be unprepped. >> >> Indeed, adding a 'usleep (100 * 1000)' at the end of rl_deprep_terminal, makes >> it more likely to catch the terminal in unprepped mode, and triggers the FAIL >> without taskset. >> >> At this point, it's good to point out that I have no idea whether this >> behaviour is as expected, or should be considered a bug, and if so whether the >> bug is in gdb, readline, or the terminal emulator. >> >> Either way, I suppose it would be nice if we treat the ^D the same regardless. >> >> This patch has a way of achieving this, by setting the terminal to >> non-canonical mode before initializing readline, such that both the prepped >> and deprepped mode keep the terminal in non-canonical mode. >> >> But terminal settings are sticky so they also need to be undone. >> >> The patch adds functions termios_enable_readline and termios_disable_readline, >> which are called at points found necessary using trial-and-error. >> >> Tested on x86_64-linux, both with and without taskset -c 0. >> >> I have an open question whether it would be necessary or a good idea to change >> more that just the canonical mode. >> >> Andrew also noted that a potential readline fix / workaround could be: >> ... >> @@ -630,7 +630,7 @@ readline_internal_charloop (void) >> previous character is interpreted as EOF. This doesn't work when >> READLINE_CALLBACKS is defined, so hitting a series of ^Ds will >> erase all the chars on the line and then return EOF. */ >> - if (((c == _rl_eof_char && lastc != c) || c == EOF) && rl_end == 0) >> + if (((c == _rl_eof_char && lastc != c) || c == EOF || c == 0) && rl_end == 0) >> { >> #if defined (READLINE_CALLBACKS) >> RL_SETSTATE(RL_STATE_DONE); >> ... >> which indeed fixes the test-case, but broader testing runs into trouble in >> gdb.tui, so that might need more work, but could of course be trivial >> to fix. > > I looked at this a little more. If you check out tui_getc_1 (tui-io.c) > you'll see that we sometimes return 0 to indicate "ignore this > character", so clearly my idea above for handling 0 is not a good one. > I see that indeed, I do wonder though if that in itself is correct. I managed to manually feed ^@ (char with ascii code 0) to a tui session in a meaningful way: - gdb -tui - c-x o to put focus in command window - type "word1" + space - type c-2 to get ^@, to set mark at start of "word2" - type "word2" + space + "word3" - move cursor to start of "word3" - type c-x c-x to swap point and mark - watch point move from start of "word3" to start of "word2" So, doesn't returning 0 in tui_getc_1 set the mark, in accordance with the active key bindings? > I wonder if we should, instead be handling EOF earlier, e.g. in > readline/input.c, maybe in here we should spot that we read '\0' and > convert this to EOF? But then what if the user (for some reason) > legitimately wanted to send \0 to GDB... > Agreed, and I think the scenario mentioned above is an example. > So then I start wondering if this is an artefact of switching between > canonical and non-canonical mode? The EOF arrives in canonical mode, > which doesn't add \004 to the terminal buffer, then we switch to > non-canonical mode to read, and by this point its too late. I do wonder > if the kernel should actually be adding the \004 character to the > pending characters buffer when we switch between modes... > Apropos, the mode change is done using tcsetattr and it takes an argument that decides what to do with pending i/o, but that seems more output oriented, so AFAIU the only input thingy we have available there is TCSAFLUSH which would discard pending input. But regardless, I suspect the problem will be same if the ^D arrives after switching to non-canonical mode. I think the add-usleep scenario exposes that. > Anyway, having thought about that for a while I did wonder if we really > need to fix this at all. I mean, right now, the behaviour of GDB is > that EOF sent to GDB while we're _not_ at a prompt will basically be > ignored Well, my theory is that it's not ignored, but sets the mark, which mostly looks like it's ignored, unless you do c-x c-x after which you may find point somewhere in the prompt, I'm guessing. >, after your patch the EOF will be acted on once we get back to a > prompt. Is that a desirable change? > I'd say this is roughly how I expect interactive programs to behave, yes. I type keystrokes that are processed, if it's busy it'll buffer those keystrokes and get back to processing them when it stops being busy. I don't expect the program to suddenly start to process the keystrokes differently, just because it's busy for a short while. So I guess my argument is that the change makes the behaviour more consistent for the user. > So, I wondered if there was a way we could just "fix" the test, that is, > ensure the Ctrl-D is only sent once the prompt has been displayed and > readline has put the terminal back into non-canonical mode. Turns out > that's pretty easy (see the patch below). > > With your suggested usleep, I see the eof-exit.exp test fail reliably > without the patch below, and pass reliably with the patch below. What > are are your thoughts? > I think that fixing the test-case is a good idea, since we now know about this issue (and a PR is filed to keep track of it), and there's no reason to keep this occasional fail around. If we want to change gdb behaviour, we can just add a test-case. As mentioned before, I'm not confident that this patch is a good solution, or even that I understand the problem entirely (or that this is a problem rather than a feature). Anyway, test-case patch LGTM. Thanks, - Tom > Thanks, > Andrew > > --- > > diff --git a/gdb/testsuite/gdb.base/eof-exit.exp b/gdb/testsuite/gdb.base/eof-exit.exp > index 2d9530ccebe..c88aced9f35 100644 > --- a/gdb/testsuite/gdb.base/eof-exit.exp > +++ b/gdb/testsuite/gdb.base/eof-exit.exp > @@ -25,9 +25,27 @@ proc run_test {} { > # > # Send a newline character, which will cause GDB to redisplay the > # prompt. > + # > + # We then consume the newline characters, and then make use of > + # expect's -notransfer option to ensure that the prompt has been > + # displayed, but to leave the prompt in expect's internal buffer. > + # This is important as the following test wants to check how GDB > + # displays the 'quit' message relative to the prompt, this is much > + # easier to do if the prompt is still in expect's buffers. > + # > + # The other special thing we do here is avoid printing a 'PASS' > + # result. The reason for this is so that the GDB output in the > + # log file will match what a user should see, this makes it much > + # easier to debug issues. Obviously we could print a 'PASS' here > + # as the text printed by expect is not considered part of GDB's > + # output, so the pattern matching will work just fine... but, the > + # log file becomes much harder to read. > send_gdb "\n" > gdb_test_multiple "" "discard newline" { > -re "^\r\n" { > + exp_continue > + } > + -notransfer -re "^\[^\n\]*$::gdb_prompt $" { > } > } > >