[ Note: not with system readline. ] When running test-case gdb.base/eof-exit.exp, I get: ... Running /home/vries/gdb_versions/devel/src/gdb/testsuite/gdb.base/eof-exit.exp ... === gdb Summary === # of expected passes 2 # of known failures 1 ... But when using taskset -c 0: ... FAIL: gdb.base/eof-exit.exp: with non-dump terminal: close GDB with eof (timeout) FAIL: gdb.base/eof-exit.exp: with non-dump terminal: with bracketed-paste-mode on: close GDB with eof (timeout) ... First one in more detail: ... (gdb) dir^M Reinitialize source path to empty? (y or n) y^M Source directories searched: $cdir:$cwd^M (gdb) dir /home/vries/gdb_versions/devel/src/gdb/testsuite/gdb.base^M Source directories searched: /home/vries/gdb_versions/devel/src/gdb/testsuite/gdb.base:$cdir:$cwd^M (gdb) ^M (gdb) FAIL: gdb.base/eof-exit.exp: default: close GDB with eof (timeout) ... The observed behaviour matches with ^D getting dropped. Indeed, a retry at the point of the timeout fixes the fail: ... diff --git a/gdb/testsuite/gdb.base/eof-exit.exp b/gdb/testsuite/gdb.base/eof-exit.exp index 2d9530ccebe..0b310ee4b0b 100644 --- a/gdb/testsuite/gdb.base/eof-exit.exp +++ b/gdb/testsuite/gdb.base/eof-exit.exp @@ -55,6 +55,10 @@ proc run_test {} { eof { fail "$gdb_test_name (missed the prompt)" } + timeout { + send_gdb "\004" + exp_continue + } } } ... So, is ^D allowed to get dropped?
(In reply to Tom de Vries from comment #0) > [ Note: not with system readline. ] I tried now with system readline ( /lib64/libreadline.so.7 ), and one of the FAILs disappears, so we've only got: ... FAIL: gdb.base/eof-exit.exp: default: close GDB with eof (timeout) ...
Also reproduces with gdb 11.2. I did the following experiment: print out the chars in a debug file as they are read from the input stream by readline (iow the result of the read in rl_getc). In the passing case, readline indeed sees the \004 char. But in the failing case, it gets \000 instead. My current theory is that this is caused by the terminal being switched forth and back between prepped and unprepped mode, and that what we're seeing is the result of the 'send_gdb "\004"' arriving at the terminal while being unprepped. Reading here ( https://www.gnu.org/software/libc/manual/html_node/Canonical-or-Not.html ) I read: ... In canonical input processing mode, terminal input is processed in lines terminated by newline ('\n'), EOF, or EOL characters ... So, that sounds plausible. If the EOF is interpreted by the terminal as end of input, and there's no other input, the result could well be an empty string, that is, a string consisting of a single '\0'. I tried to test this theory by extending the period that the terminal is unprepped, like so: ... diff --git a/readline/readline/rltty.c b/readline/readline/rltty.c index d0cd572713a..6b81bd804ce 100644 --- a/readline/readline/rltty.c +++ b/readline/readline/rltty.c @@ -711,6 +711,7 @@ rl_deprep_terminal (void) RL_UNSETSTATE(RL_STATE_TERMPREPPED); _rl_release_sigint (); + usleep (100 * 1000); } #endif /* !NO_TTY_DRIVER */ ... and indeed this makes the problem reproducible without taskset.
This make the test-case pass reliably: ... diff --git a/gdb/inflow.c b/gdb/inflow.c index 9c7e2907625..c268ca9e4dc 100644 --- a/gdb/inflow.c +++ b/gdb/inflow.c @@ -145,6 +145,11 @@ set_initial_gdb_ttystate (void) /* Note we can't do any of this in _initialize_inflow because at that point stdin_serial has not been created yet. */ + struct termios termios; + tcgetattr (0, &termios); + termios.c_lflag &= ~(ICANON); + tcsetattr (0, TCSANOW, &termios); + initial_gdb_ttystate = serial_get_tty_state (stdin_serial); if (initial_gdb_ttystate != NULL) ... It changes the initial terminal state (the one that unprepped returns to) to non-canonical.
(In reply to Tom de Vries from comment #3) > This make the test-case pass reliably: But, runs into trouble in the test-suite: ... (gdb) ^M 100-gdb-set height 0^J100^done^M (gdb) ^M WARNING: Couldn't set the height to 0 101-gdb-set width 0^J101^done^M (gdb) ^M WARNING: Couldn't set the width to 0. ... The change doesn't sit well with terminals used for mi input. Now trying out moving the code to a place where it has less scope: ... diff --git a/gdb/event-top.c b/gdb/event-top.c index b628f0397cb..6fa0d492a2d 100644 --- a/gdb/event-top.c +++ b/gdb/event-top.c @@ -43,6 +43,7 @@ #include "async-event.h" #include "bt-utils.h" #include "pager.h" +#include "termios.h" /* readline include files. */ #include "readline/readline.h" @@ -1312,6 +1313,11 @@ gdb_setup_readline (int editing) one instance of readline. */ if (ISATTY (ui->instream) && editing && ui == main_ui) { + struct termios termios; + tcgetattr (0, &termios); + termios.c_lflag &= ~(ICANON); + tcsetattr (0, TCSANOW, &termios); + /* Tell gdb that we will be using the readline library. This could be overwritten by a command in .gdbinit like 'set editing on' or 'off'. */ ...
Hmm, the setting is sticky, and needs to be undone: ... gdb-subshell$ PASS: gdb.base/batch-preserve-term-settings.exp: cli exit: quit gdb stty || echo "not found"^M speed 38400 baud; line = 0;^M min = 1; time = 0;^M -icanon^M gdb-subshell$ PASS: gdb.base/batch-preserve-term-settings.exp: cli exit: stty after FAIL: gdb.base/batch-preserve-term-settings.exp: cli exit: terminal settings preserved ...
OK, let's try again: ... diff --git a/gdb/event-top.c b/gdb/event-top.c index b628f0397cb..8ff4846b405 100644 --- a/gdb/event-top.c +++ b/gdb/event-top.c @@ -43,6 +43,7 @@ #include "async-event.h" #include "bt-utils.h" #include "pager.h" +#include "termios.h" /* readline include files. */ #include "readline/readline.h" @@ -1312,6 +1313,12 @@ gdb_setup_readline (int editing) one instance of readline. */ if (ISATTY (ui->instream) && editing && ui == main_ui) { + struct termios termios; + tcgetattr (fileno (ui->instream), &termios); + termios.c_lflag &= ~(ICANON); + tcsetattr (fileno (ui->instream), TCSANOW, &termios); + //fprintf (stderr, "Clearing ICANON on fileno %d\n", fileno (ui->instream)); m + /* Tell gdb that we will be using the readline library. This could be overwritten by a command in .gdbinit like 'set editing on' or 'off'. */ diff --git a/gdb/top.c b/gdb/top.c index e776ac2d70e..f796d12a14e 100644 --- a/gdb/top.c +++ b/gdb/top.c @@ -57,6 +57,7 @@ #include "gdbsupport/pathstuff.h" #include "cli/cli-style.h" #include "pager.h" +#include "termios.h" /* readline include files. */ #include "readline/readline.h" @@ -1774,6 +1775,15 @@ undo_terminal_modifications_before_exit (void) #endif gdb_disable_readline (); + if (ISATTY (current_ui->instream)) + { + struct termios termios; + tcgetattr (fileno (current_ui->instream), &termios); + termios.c_lflag |= (ICANON); + tcsetattr (fileno (current_ui->instream), TCSANOW, &termios); + //fprintf (stderr, "Restoring ICANON on fileno %d\n", fileno (current_ui->ins tream)); + } + current_ui = saved_top_level; } ... This at least passes: - gdb.base/eof-exit.exp - gdb.ada/mi_catch_assert.exp - gdb.base/batch-preserve-term-settings.exp
Created attachment 14052 [details] Tentative patch passes at least (FAILs in previous versions of patch): - gdb.base/eof-exit.exp - gdb.base/batch-preserve-term-settings.exp - gdb.ada/mi_catch_assert.exp - gdb.base/page.exp - gdb.mi/mi-cmd-param-changed.exp
This also feels like an issue with readline's callback api. I wonder if we should (in parallel) be reporting this issue upstream? Pretty much untested, this patch seems to "fix" readline: diff --git a/readline/readline/readline.c b/readline/readline/readline.c index e61d188bbe9..1306ac7873d 100644 --- a/readline/readline/readline.c +++ b/readline/readline/readline.c @@ -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); But I understand that we might want a solution in GDB (as well) that doesn't tie us to a particular readline release. I happy to take this to the readline list, but I'd value your thoughts before I did.
(In reply to Tom de Vries from comment #7) > Created attachment 14052 [details] > Tentative patch > > passes at least (FAILs in previous versions of patch): > - gdb.base/eof-exit.exp > - gdb.base/batch-preserve-term-settings.exp > - gdb.ada/mi_catch_assert.exp > - gdb.base/page.exp > - gdb.mi/mi-cmd-param-changed.exp Testsuite run with and without taskset -c 0 went ok.
(In reply to Andrew Burgess from comment #8) > This also feels like an issue with readline's callback api. I wonder if we > should (in parallel) be reporting this issue upstream? > > Pretty much untested, this patch seems to "fix" readline: > > diff --git a/readline/readline/readline.c b/readline/readline/readline.c > index e61d188bbe9..1306ac7873d 100644 > --- a/readline/readline/readline.c > +++ b/readline/readline/readline.c > @@ -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); > > > But I understand that we might want a solution in GDB (as well) that doesn't > tie us to a particular readline release. I happy to take this to the > readline list, but I'd value your thoughts before I did. Thanks for looking into this. First off, my understanding of terminals/readline and how gdb uses those is well, work in progress. I'd be happy if there could be a fix (or workaround) in readline. The reason I ended up trying to write a fix in gdb, is because my understanding is that the observed problem falls outside the responsibilities of readline. That is, AFAIU readline responsibilities are limited to: - what happens while readline () is called, or - what happens while a readline handler is installed. And my theory here is that the problem that happens, happens while readline handler is not installed (and gdb doesn't call readline directly, I think). So, AFAIU the patch you're posted above is a workaround, not a fix. Either way, I'd like to understand what you think is the specific "issue with readline's callback api". That said, I think that reaching out to the readline list is a good idea, perhaps the patch you posted is already acceptable, perhaps they can suggest a different solution. Anyway, I still got open questions about my patch (f.i. why only disable that one flag...), so I will try to write up an RFC and send it to gdb ml.
(In reply to Andrew Burgess from comment #8) > Pretty much untested, this patch seems to "fix" readline: > > diff --git a/readline/readline/readline.c b/readline/readline/readline.c > index e61d188bbe9..1306ac7873d 100644 > --- a/readline/readline/readline.c > +++ b/readline/readline/readline.c > @@ -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); > FWIW, I've tested this, and ran into: ... $ egrep '(UNRESOLVED|ERROR):' gdb.sum ERROR: : spawn id exp49 not open UNRESOLVED: gdb.tui/basic.exp: scroll up ERROR: : spawn id exp49 not open UNRESOLVED: gdb.tui/basic.exp: scroll right ERROR: : spawn id exp49 not open UNRESOLVED: gdb.tui/basic.exp: scroll down ERROR: : spawn id exp49 not open UNRESOLVED: gdb.tui/basic.exp: asm window shows main ERROR: : spawn id exp49 not open UNRESOLVED: gdb.tui/basic.exp: split layout contents ERROR: : spawn id exp49 not open UNRESOLVED: gdb.tui/tui-layout-asm.exp: scroll to end of assembler (scroll failed) ... Specifically: ... PASS: gdb.tui/basic.exp: list -q main ^[[14;13H^[[2;16H^[[36mMERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the^[[3;13H GNU General Public License for more details.^[[4;13H^[[39m^[[0;10m^[[67X^[[5;16H^[[36mYou should have received a copy of the GNU General Public Licens^[[6;13H along with this program. If not, see ^[[34m<http://www.gnu.org/licens^[[7;13H^[[39m^[[0;10m^[[25X^[[B^[[32mint^[[39m^[[0;10m ^[[0;10;1m^[[30mmain^[[39m^[[0;10m ^[[31m()^[[39m^[[0;10m ^[[31m{^[[39m^[[0;10m ^[[0;10;1m^[[34mreturn^[[39m^[[0;10m ^[[35m0^[[31m;^[[39m^[[0;10m ^[[31m}^[[14;13H^[[39m^[[0;10m^[[2;10H2^[[B^[[D3^[[B^[[D4^[[B^[[D5^[[B^[[D6^[[B^[[D7^[[B^[[D8^[[9;9H19^[[B^[[D0^[[B^[[D1^[[B^[[D2^[[B^[[D3^[[B^[[D4 ^M^[[18d^[[Jquit^M^[[B^[[24;1H^MERROR: : spawn id exp9 not open ...
RFC posted: https://sourceware.org/pipermail/gdb-patches/2022-April/187670.html
The master branch has been updated by Andrew Burgess <aburgess@sourceware.org>: https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=205d0542821c91e04e0ed174d8862f486a076950 commit 205d0542821c91e04e0ed174d8862f486a076950 Author: Andrew Burgess <aburgess@redhat.com> Date: Wed Apr 20 14:08:49 2022 +0100 gdb: fix for gdb.base/eof-exit.exp test failures This fix relates to PR gdb/29032, this makes the test more stable by ensuring that the Ctrl-D is only sent once the prompt has been displayed. This issue was also discussed on the mailing list here: https://sourceware.org/pipermail/gdb-patches/2022-April/187670.html The problem identified in the bug report is that sometimes the Ctrl-D (that the test sends to GDB) arrives while GDB is processing a command. When this happens the Ctrl-D is handled differently than if the Ctrl-D is sent while GDB is waiting for input at a prompt. The original intent of the test was that the Ctrl-D be sent while GDB was waiting at a prompt, and that is the case the occurs most often, but, when the Ctrl-D arrives during command processing, then GDB will ignore the Ctrl-D, and the test will fail. This commit ensures the Ctrl-D is always sent while GDB is waiting at a prompt, which makes this test stable. But, that still leaves an open question, what should happen when the Ctrl-D arrives while GDB is processing a command? This commit doesn't attempt to answer that question, which is while bug PR gdb/29032 will not be closed once this commit is merged. Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=29032
I've updated the summary to reflect the current state of affairs, not referring to a testsuite failure anymore.