[2/2] gdb/readline: don't get stuck thinking an EOF arrived

  It was brought to my attention[1] that if a user makes use of Ctrl+d
to quit from a secondary prompt (e.g. the prompt used to enter lines
for the 'commands' command) then GDB will start displaying some
unexpected blank lines.  Here's an example:

  Reading symbols from /tmp/hello.x...
  (gdb) break main
  Breakpoint 1 at 0x401198: file hello.c, line 18.
  (gdb) commands
  Type commands for breakpoint(s) 1, one per line.
  End with a line saying just "end".
  >quit			# <----------- Use Ctrl+d to quit here.
  (gdb) show architecture
			# <----------- This blank line is unexpected.
  The target architecture is set to "auto" (currently "i386:x86-64").
  (gdb)

I've marked up where I press 'Ctrl+d', and also the unexpected blank
line.

This issue will only happen if bracketed-paste-mode is in use.  If
this has been disabled (e.g. in ~/.inputrc) then this issue will not
occur.

The blank line is not just emitted for the 'show architecture'
command.  The blank line is actually caused by an extra '\n' character
emitted by readline after it has gathered a complete line of input,
and so will occur for any command.

The problem is caused by readline getting "stuck" in a state where it
thinks that an EOF has just been delivered.  This state is set when
the 'Ctrl+d' does deliver an EOF, but then this state is never fully
reset.  As a result, every time readline completes a line, it thinks
that the line was completed due to an EOF and so adds an extra '\n'
character.

Obviously the real fix for this issue is to patch readline, and I do
have a patch for that[2], however, version 8.2 of readline has been
released, and contains this issue.  As such, if GDB is linked against
the system readline, and that system readline is 8.2, then we can
expect to see this issue.

There's a pretty simple, and cheap workaround that we can add to GDB
that will mitigate this issue.

I propose that we add this workaround to GDB.  If/when the readline
patch is accepted then I'll back-port this to our local readline copy,
but retaining the workaround will be harmless, and will make GDB play
nicer with system readline libraries (version 8.2).

[1] https://inbox.sourceware.org/gdb-patches/34ef5438-8644-44cd-8537-5068e0e5e434@redhat.com
[2] https://lists.gnu.org/archive/html/bug-readline/2024-10/msg00014.html
---
 gdb/event-top.c                               |  17 +++
 .../gdb.base/readline-commands-eof.c          |  22 +++
 .../gdb.base/readline-commands-eof.exp        | 128 ++++++++++++++++++
 gdb/testsuite/lib/gdb.exp                     |  28 ++++
 4 files changed, 195 insertions(+)
 create mode 100644 gdb/testsuite/gdb.base/readline-commands-eof.c
 create mode 100644 gdb/testsuite/gdb.base/readline-commands-eof.exp
  

On 10/30/24 8:53 AM, Andrew Burgess wrote:
> It was brought to my attention[1] that if a user makes use of Ctrl+d
> to quit from a secondary prompt (e.g. the prompt used to enter lines
> for the 'commands' command) then GDB will start displaying some
> unexpected blank lines.

Thank you for digging into that. I had a feeling this was a pre-existing
bug...

> This issue will only happen if bracketed-paste-mode is in use.  If
> this has been disabled (e.g. in ~/.inputrc) then this issue will not
> occur.

Bracketed-paste strikes again!

> There's a pretty simple, and cheap workaround that we can add to GDB
> that will mitigate this issue.
> 
> I propose that we add this workaround to GDB.  If/when the readline
> patch is accepted then I'll back-port this to our local readline copy,
> but retaining the workaround will be harmless, and will make GDB play
> nicer with system readline libraries (version 8.2).

Given the simplicity of the workaround, I certainly don't mind including
it.

LGTM with a few typos.

Reviewed-by: Keith Seitz <keiths@redhat.com>

Thank you for fixing this. I did not mean to derail your inferior
args series.

Keith

> diff --git a/gdb/event-top.c b/gdb/event-top.c
> index 0d46dec1774..adc17508c64 100644
> --- a/gdb/event-top.c
> +++ b/gdb/event-top.c
> @@ -387,6 +387,23 @@ gdb_rl_callback_handler_install (const char *prompt)
>        therefore loses input.  */
>     gdb_assert (!callback_handler_installed);
>   
> +#ifdef RL_STATE_EOF
> +  /* Some versions of readline contain a but where the rl_eof_found flag
> +     would not be reset back to 0 in rl_initialize, despite the
> +     RL_STATE_EOF flag being cleared in this function.

"contain a bug" (I presume)

> +
> +     The consequence of this mistake is that readline will appear to get
> +     stuck in the EOF state, and will emit an extra '\n' character each
> +     time an input line is completed.
> +
> +     Work around this by clearing the EOF state now ourselves.  */
> +  if (RL_ISSTATE (RL_STATE_EOF))
> +    {
> +      RL_UNSETSTATE (RL_STATE_EOF);
> +      rl_eof_found = 0;
> +    }
> +#endif /* RL_STATE_EOF */
> +
>     rl_callback_handler_install (prompt, gdb_rl_callback_handler);
>     callback_handler_installed = true;
>   }
> diff --git a/gdb/testsuite/gdb.base/readline-commands-eof.exp b/gdb/testsuite/gdb.base/readline-commands-eof.exp
> new file mode 100644
> index 00000000000..c1efbbfe123
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/readline-commands-eof.exp
> @@ -0,0 +1,128 @@
> +# Copyright 2024 Free Software Foundation, Inc.
> +#
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +
> +# If a user uses 'Ctrl+d' to exit from a secondary prompt, then
> +# readline can get stuck thinking that an EOF has arrived.  As a
> +# consequence of this readline will output an extra newline everytime

"every time"

> +# that it exits bracketed-paste-mode (which is done after every line
> +# of input).  The result is the user will see some unexpected blank
> +# lines in the output.
> +
> +standard_testfile
> +
> +if { [prepare_for_testing "failed to prepare" $testfile $srcfile] } {
> +    return -1
> +}
> +
> +# The fix for this issue relies on GDB being able to adjust the EOF
> +# flag state within readline.  Access to this state was added for
> +# readline 8.2, but was also backported to out internal readline.  If
> +# this feature is not available then this test might not pass.
> +if { ![readline_supports_eof_flag] } {
> +    unsupported "readline is not eof flag aware"
> +    return -1
> +}
> +
> +# Create a breakpoint then issue the 'commands' commands.  When the
> +# secondary prompt is displayed, use Ctrl+d to send EOF to readline
> +# and cancel the input.
> +#
> +# Then check that readline is not stuck thinking that an EOF has
> +# arrived.  If it is then GDB will start displaying extra blank lines
> +# after each line of input.
> +proc run_test {} {
> +    clean_restart $::binfile
> +
> +    gdb_breakpoint main
> +
> +    # Issue the 'commands' command, and wait for the secondary prompt
> +    # to be displayed.
> +    gdb_test_multiple "commands" "start b/p commands" {
> +	-re "Type commands for breakpoint\\(s\\) 1, one per line\\.\r\n" {
> +	    exp_continue
> +	}
> +	-re "^End with a line saying just \"end\"\\.\r\n" {
> +	    exp_continue
> +	}
> +	-re "^\[^\r\n\]*>$" {
> +	    pass $gdb_test_name
> +	}
> +    }
> +
> +    # Send Ctrl+d to GDB and wait for the 'quit' message, and then for
> +    # the GDB prompt to be displayed.
> +    #
> +    # As this test runs (sometimes) with bracketed-paste-mode on then
> +    # we need to accept a control sequence before the prompt.  This
> +    # control sequence can contain '\r', which is why we only check
> +    # for '\n' here, which is different than what we do in the rest of
> +    # the testsuite, where we usually check for '\r\n' together.
> +    send_gdb "\004"
> +    gdb_test_multiple "" "quit b/p commands" {
> +	-re "^quit\r\n\[^\n\]*$::gdb_prompt $" {
> +	    pass $gdb_test_name
> +	}
> +    }
> +
> +    # Now issue any other command.  If readline is stuck in EOF mode
> +    # (thinking that an EOF has just arrived), then we'll see an extra
> +    # blank line afer the command, and before any command output.

"after"

> +    #
> +    # As described above we scan for '\n' only in some patterns here
> +    # as we're allowing for a control sequence that might include
> +    # '\r'.
> +    gdb_test_multiple "show architecture" "check for excessive blank lines" {
> +	-re "^show architecture\r\n" {
> +	    exp_continue
> +	}
> +	-re "^\[^\n\]*The target architecture is set to \[^\r\n\]+\\.\r\n\[^\n\]*$::gdb_prompt $" {
> +	    pass $gdb_test_name
> +	}
> +	-re "^\[^\n\]*\nThe target architecture is set to \[^\r\n\]+\\.\r\n\[^\n\]*$::gdb_prompt" {
> +	    fail $gdb_test_name
> +	}
> +    }
> +}
> +
> +# Run the test in various different terminal modes.
> +with_test_prefix "default" {
> +    run_test
> +}
> +
> +save_vars { env(TERM) } {
> +    setenv TERM ansi
> +
> +    with_test_prefix "with non-dump terminal" {
> +	run_test
> +
> +	save_vars { env(INPUTRC) } {
> +
> +	    # Create an inputrc file that turns bracketed paste mode
> +	    # on.  This is usually turned off (see lib/gdb.exp), but
> +	    # for the next test we want to see what happens with this
> +	    # on.
> +	    set inputrc [standard_output_file inputrc]
> +	    set fd [open "$inputrc" w]
> +	    puts $fd "set enable-bracketed-paste on"
> +	    close $fd
> +
> +	    setenv INPUTRC "$inputrc"
> +	    with_test_prefix "with bracketed-paste-mode on" {
> +		run_test
> +	    }
> +	}
> +    }
> +}
> +
  

Keith Seitz <keiths@redhat.com> writes:

> On 10/30/24 8:53 AM, Andrew Burgess wrote:
>> It was brought to my attention[1] that if a user makes use of Ctrl+d
>> to quit from a secondary prompt (e.g. the prompt used to enter lines
>> for the 'commands' command) then GDB will start displaying some
>> unexpected blank lines.
>
> Thank you for digging into that. I had a feeling this was a pre-existing
> bug...
>
>> This issue will only happen if bracketed-paste-mode is in use.  If
>> this has been disabled (e.g. in ~/.inputrc) then this issue will not
>> occur.
>
> Bracketed-paste strikes again!
>
>> There's a pretty simple, and cheap workaround that we can add to GDB
>> that will mitigate this issue.
>> 
>> I propose that we add this workaround to GDB.  If/when the readline
>> patch is accepted then I'll back-port this to our local readline copy,
>> but retaining the workaround will be harmless, and will make GDB play
>> nicer with system readline libraries (version 8.2).
>
> Given the simplicity of the workaround, I certainly don't mind including
> it.
>
> LGTM with a few typos.

I've fixed all these typos locally for now.

>
> Reviewed-by: Keith Seitz <keiths@redhat.com>
>
> Thank you for fixing this. I did not mean to derail your inferior
> args series.

Really not a problem.

Thanks for the review.

Thanks,
Andrew

>
> Keith
>
>> diff --git a/gdb/event-top.c b/gdb/event-top.c
>> index 0d46dec1774..adc17508c64 100644
>> --- a/gdb/event-top.c
>> +++ b/gdb/event-top.c
>> @@ -387,6 +387,23 @@ gdb_rl_callback_handler_install (const char *prompt)
>>        therefore loses input.  */
>>     gdb_assert (!callback_handler_installed);
>>   
>> +#ifdef RL_STATE_EOF
>> +  /* Some versions of readline contain a but where the rl_eof_found flag
>> +     would not be reset back to 0 in rl_initialize, despite the
>> +     RL_STATE_EOF flag being cleared in this function.
>
> "contain a bug" (I presume)
>
>> +
>> +     The consequence of this mistake is that readline will appear to get
>> +     stuck in the EOF state, and will emit an extra '\n' character each
>> +     time an input line is completed.
>> +
>> +     Work around this by clearing the EOF state now ourselves.  */
>> +  if (RL_ISSTATE (RL_STATE_EOF))
>> +    {
>> +      RL_UNSETSTATE (RL_STATE_EOF);
>> +      rl_eof_found = 0;
>> +    }
>> +#endif /* RL_STATE_EOF */
>> +
>>     rl_callback_handler_install (prompt, gdb_rl_callback_handler);
>>     callback_handler_installed = true;
>>   }
>> diff --git a/gdb/testsuite/gdb.base/readline-commands-eof.exp b/gdb/testsuite/gdb.base/readline-commands-eof.exp
>> new file mode 100644
>> index 00000000000..c1efbbfe123
>> --- /dev/null
>> +++ b/gdb/testsuite/gdb.base/readline-commands-eof.exp
>> @@ -0,0 +1,128 @@
>> +# Copyright 2024 Free Software Foundation, Inc.
>> +#
>> +# This program is free software; you can redistribute it and/or modify
>> +# it under the terms of the GNU General Public License as published by
>> +# the Free Software Foundation; either version 3 of the License, or
>> +# (at your option) any later version.
>> +#
>> +# This program is distributed in the hope that it will be useful,
>> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
>> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> +# GNU General Public License for more details.
>> +#
>> +# You should have received a copy of the GNU General Public License
>> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
>> +
>> +# If a user uses 'Ctrl+d' to exit from a secondary prompt, then
>> +# readline can get stuck thinking that an EOF has arrived.  As a
>> +# consequence of this readline will output an extra newline everytime
>
> "every time"
>
>> +# that it exits bracketed-paste-mode (which is done after every line
>> +# of input).  The result is the user will see some unexpected blank
>> +# lines in the output.
>> +
>> +standard_testfile
>> +
>> +if { [prepare_for_testing "failed to prepare" $testfile $srcfile] } {
>> +    return -1
>> +}
>> +
>> +# The fix for this issue relies on GDB being able to adjust the EOF
>> +# flag state within readline.  Access to this state was added for
>> +# readline 8.2, but was also backported to out internal readline.  If
>> +# this feature is not available then this test might not pass.
>> +if { ![readline_supports_eof_flag] } {
>> +    unsupported "readline is not eof flag aware"
>> +    return -1
>> +}
>> +
>> +# Create a breakpoint then issue the 'commands' commands.  When the
>> +# secondary prompt is displayed, use Ctrl+d to send EOF to readline
>> +# and cancel the input.
>> +#
>> +# Then check that readline is not stuck thinking that an EOF has
>> +# arrived.  If it is then GDB will start displaying extra blank lines
>> +# after each line of input.
>> +proc run_test {} {
>> +    clean_restart $::binfile
>> +
>> +    gdb_breakpoint main
>> +
>> +    # Issue the 'commands' command, and wait for the secondary prompt
>> +    # to be displayed.
>> +    gdb_test_multiple "commands" "start b/p commands" {
>> +	-re "Type commands for breakpoint\\(s\\) 1, one per line\\.\r\n" {
>> +	    exp_continue
>> +	}
>> +	-re "^End with a line saying just \"end\"\\.\r\n" {
>> +	    exp_continue
>> +	}
>> +	-re "^\[^\r\n\]*>$" {
>> +	    pass $gdb_test_name
>> +	}
>> +    }
>> +
>> +    # Send Ctrl+d to GDB and wait for the 'quit' message, and then for
>> +    # the GDB prompt to be displayed.
>> +    #
>> +    # As this test runs (sometimes) with bracketed-paste-mode on then
>> +    # we need to accept a control sequence before the prompt.  This
>> +    # control sequence can contain '\r', which is why we only check
>> +    # for '\n' here, which is different than what we do in the rest of
>> +    # the testsuite, where we usually check for '\r\n' together.
>> +    send_gdb "\004"
>> +    gdb_test_multiple "" "quit b/p commands" {
>> +	-re "^quit\r\n\[^\n\]*$::gdb_prompt $" {
>> +	    pass $gdb_test_name
>> +	}
>> +    }
>> +
>> +    # Now issue any other command.  If readline is stuck in EOF mode
>> +    # (thinking that an EOF has just arrived), then we'll see an extra
>> +    # blank line afer the command, and before any command output.
>
> "after"
>
>> +    #
>> +    # As described above we scan for '\n' only in some patterns here
>> +    # as we're allowing for a control sequence that might include
>> +    # '\r'.
>> +    gdb_test_multiple "show architecture" "check for excessive blank lines" {
>> +	-re "^show architecture\r\n" {
>> +	    exp_continue
>> +	}
>> +	-re "^\[^\n\]*The target architecture is set to \[^\r\n\]+\\.\r\n\[^\n\]*$::gdb_prompt $" {
>> +	    pass $gdb_test_name
>> +	}
>> +	-re "^\[^\n\]*\nThe target architecture is set to \[^\r\n\]+\\.\r\n\[^\n\]*$::gdb_prompt" {
>> +	    fail $gdb_test_name
>> +	}
>> +    }
>> +}
>> +
>> +# Run the test in various different terminal modes.
>> +with_test_prefix "default" {
>> +    run_test
>> +}
>> +
>> +save_vars { env(TERM) } {
>> +    setenv TERM ansi
>> +
>> +    with_test_prefix "with non-dump terminal" {
>> +	run_test
>> +
>> +	save_vars { env(INPUTRC) } {
>> +
>> +	    # Create an inputrc file that turns bracketed paste mode
>> +	    # on.  This is usually turned off (see lib/gdb.exp), but
>> +	    # for the next test we want to see what happens with this
>> +	    # on.
>> +	    set inputrc [standard_output_file inputrc]
>> +	    set fd [open "$inputrc" w]
>> +	    puts $fd "set enable-bracketed-paste on"
>> +	    close $fd
>> +
>> +	    setenv INPUTRC "$inputrc"
>> +	    with_test_prefix "with bracketed-paste-mode on" {
>> +		run_test
>> +	    }
>> +	}
>> +    }
>> +}
>> +