Re: [RFC][gdb] Handle ^D when terminal is not prepped

[Andrew Burgess via Gdb-patches <gdb-patches@sourceware.org>]

Tom de Vries via Gdb-patches <gdb-patches@sourceware.org> 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 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...

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...

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, after your patch the EOF will be acted on once we get back to a
prompt.  Is that a desirable change?

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?

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 $" {
 	}
     }