Strip trailing newlines from input string

  A co-worker noticed a strange situation where "target remote" would
fail due to a trailing newline in the address part of the command.
Eventually he tracked this down to the fact that he was pasting the
command into the terminal, and due to bracketed paste mode, the
newline was being preserved by readline.

It seems to me that we basically never want a trailing newline on a
gdb command, so this patch removes it when handling the readline
result.

Co-Authored-By: Kévin Le Gouguec <legouguec@adacore.com>
---
 gdb/event-top.c                          |  8 +++++
 gdb/testsuite/gdb.base/paste-newline.exp | 45 ++++++++++++++++++++++++
 2 files changed, 53 insertions(+)
 create mode 100644 gdb/testsuite/gdb.base/paste-newline.exp
  

On 4/8/24 18:18, Tom Tromey wrote:
> A co-worker noticed a strange situation where "target remote" would
> fail due to a trailing newline in the address part of the command.
> Eventually he tracked this down to the fact that he was pasting the
> command into the terminal, and due to bracketed paste mode, the
> newline was being preserved by readline.
> 
> It seems to me that we basically never want a trailing newline on a
> gdb command, so this patch removes it when handling the readline
> result.
> 
> Co-Authored-By: Kévin Le Gouguec <legouguec@adacore.com>
> ---
>  gdb/event-top.c                          |  8 +++++
>  gdb/testsuite/gdb.base/paste-newline.exp | 45 ++++++++++++++++++++++++
>  2 files changed, 53 insertions(+)
>  create mode 100644 gdb/testsuite/gdb.base/paste-newline.exp
> 
> diff --git a/gdb/event-top.c b/gdb/event-top.c
> index 9a02ac6df27..f0c07ba7f64 100644
> --- a/gdb/event-top.c
> +++ b/gdb/event-top.c
> @@ -251,6 +251,14 @@ gdb_rl_callback_handler (char *rl) noexcept
>    static struct gdb_exception gdb_rl_expt;
>    struct ui *ui = current_ui;
>  
> +  /* In bracketed paste mode, pasting a complete line can result in a
> +     literal newline appearing at the end of LINE.  However, we never
> +     want this in gdb.  */
> +  size_t len = strlen (rl);
> +  while (len > 0 && (rl[len - 1] == '\r' || rl[len - 1] == '\n'))
> +    --len;
> +  rl[len] = '\0';
> +
>    try
>      {
>        /* Ensure the exception is reset on each call.  */
> diff --git a/gdb/testsuite/gdb.base/paste-newline.exp b/gdb/testsuite/gdb.base/paste-newline.exp
> new file mode 100644
> index 00000000000..b886fae6871
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/paste-newline.exp
> @@ -0,0 +1,45 @@
> +# Copyright (C) 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/>.
> +
> +# This test script checks that a trailing newline is stripped from a
> +# bracketed paste.
> +
> +save_vars { env(TERM) env(INPUTRC) } {
> +    setenv TERM ansi
> +
> +    # Create an inputrc file that enables bracketed paste mode.
> +    set inputrc [standard_output_file inputrc]
> +    set fd [open "$inputrc" w]
> +    puts $fd "set enable-bracketed-paste on"
> +    close $fd
> +
> +    setenv INPUTRC "$inputrc"
> +
> +    clean_restart
> +
> +    send_gdb "\033\[200~echo hello\n\033\[201~\n"
> +
> +    gdb_test_multiple "" "newline removed from paste" {
> +	-re "hello\[^\n\]*$gdb_prompt $" {
> +	    # Some escape sequences are expected between echo's output
> +	    # and the prompt (e.g. the paste-bracketing toggle
> +	    # sequences) but _newlines_ are not.
> +	    pass $gdb_test_name
> +	}
> +	-re "hello.*\r\n.*$gdb_prompt $" {
> +	    fail $gdb_test_name
> +	}
> +    }
> +}

Looks good to me.

Approved-By: Luis Machado <luis.machado@arm.com>
Tested-By: Luis Machado <luis.machado@arm.com>
  

Luis> Looks good to me.

Luis> Approved-By: Luis Machado <luis.machado@arm.com>
Luis> Tested-By: Luis Machado <luis.machado@arm.com>

Thanks.

Unfortunately the CI didn't like it, but I have no idea why.
In the gdb.log the echo output doesn't appear at all.

^[[?2004h(gdb) ^[[7mecho hello^[[m^M
^M^[[A(gdb) FAIL: gdb.base/paste-newline.exp: newline removed from paste

I don't know why this would happen or how to debug it.

Tom
  

On 4/11/24 14:45, Tom Tromey wrote:
> Luis> Looks good to me.
> 
> Luis> Approved-By: Luis Machado <luis.machado@arm.com>
> Luis> Tested-By: Luis Machado <luis.machado@arm.com>
> 
> Thanks.
> 
> Unfortunately the CI didn't like it, but I have no idea why.
> In the gdb.log the echo output doesn't appear at all.
> 
> ^[[?2004h(gdb) ^[[7mecho hello^[[m^M
> ^M^[[A(gdb) FAIL: gdb.base/paste-newline.exp: newline removed from paste
> 
> I don't know why this would happen or how to debug it.
> 
> Tom

What particular builder was this? I ran a few checks on my end and the test passed OK.

Maybe a different terminal setup or timing issue?
  

Luis Machado <luis.machado@arm.com> writes:

> On 4/11/24 14:45, Tom Tromey wrote:
>> Luis> Looks good to me.
>>
>> Luis> Approved-By: Luis Machado <luis.machado@arm.com>
>> Luis> Tested-By: Luis Machado <luis.machado@arm.com>
>>
>> Thanks.
>>
>> Unfortunately the CI didn't like it, but I have no idea why.
>> In the gdb.log the echo output doesn't appear at all.
>>
>> ^[[?2004h(gdb) ^[[7mecho hello^[[m^M
>> ^M^[[A(gdb) FAIL: gdb.base/paste-newline.exp: newline removed from paste
>>
>> I don't know why this would happen or how to debug it.
>>
>> Tom
>
> What particular builder was this? I ran a few checks on my end and the test passed OK.

This is with the Linaro CI:

https://patchwork.sourceware.org/project/gdb/patch/20240408171818.1856529-1-tromey@adacore.com/

> Maybe a different terminal setup or timing issue?

The Linaro CI uses the read1 tool which makes Expect read the GDB output
one byte at a time. We do that to make the testsuite results more
deterministic since otherwise Expect will read variable amounts of
output from GDB, which can trigger flaky failures in testcases that
aren't strict enough in their match patterns.

I can reproduce the problem when I do the same:

$ make -C gdb check-read1 TESTS=gdb.base/paste-newline.exp

It looks like reading output one byte at a time breaks ANSI sequences.

--
Thiago
  

Thiago> The Linaro CI uses the read1 tool which makes Expect read the GDB output
Thiago> one byte at a time.

Aha, thanks.  I'd forgotten about this.

I think the problem is that the gdb_test_multiple isn't matching the
command itself, so the 'echo hello' confuses it into failing.

I'm going to check this in with a small update to work with read1.

Tom
  

On 4/8/24 1:18 PM, Tom Tromey wrote:
> A co-worker noticed a strange situation where "target remote" would
> fail due to a trailing newline in the address part of the command.
> Eventually he tracked this down to the fact that he was pasting the
> command into the terminal, and due to bracketed paste mode, the
> newline was being preserved by readline.
> 
> It seems to me that we basically never want a trailing newline on a
> gdb command, so this patch removes it when handling the readline
> result.
> 
> Co-Authored-By: Kévin Le Gouguec <legouguec@adacore.com>

This caused a regression when quitting gdb with ctrl-D:

$ ./gdb -nx -q --data-directory=data-directory
(gdb) quit   <--- hit ctrl-D
/home/smarchi/src/binutils-gdb/gdb/event-top.c:257:23: runtime error: null pointer passed as argument 1, which is declared to never be null

Here, I have UBSan (I think?) that prints me a nice message, but
otherwise it segfaults.

I think this is caught by gdb.base/eof-exit.exp, I now see this test
failing on my CI.

Simon
  

>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:

Simon> On 4/8/24 1:18 PM, Tom Tromey wrote:
>> A co-worker noticed a strange situation where "target remote" would
>> fail due to a trailing newline in the address part of the command.
>> Eventually he tracked this down to the fact that he was pasting the
>> command into the terminal, and due to bracketed paste mode, the
>> newline was being preserved by readline.
>> 
>> It seems to me that we basically never want a trailing newline on a
>> gdb command, so this patch removes it when handling the readline
>> result.
>> 
>> Co-Authored-By: Kévin Le Gouguec <legouguec@adacore.com>

Simon> This caused a regression when quitting gdb with ctrl-D:

Simon> $ ./gdb -nx -q --data-directory=data-directory
Simon> (gdb) quit   <--- hit ctrl-D
Simon> /home/smarchi/src/binutils-gdb/gdb/event-top.c:257:23: runtime error: null pointer passed as argument 1, which is declared to never be null

Simon> Here, I have UBSan (I think?) that prints me a nice message, but
Simon> otherwise it segfaults.

Simon> I think this is caught by gdb.base/eof-exit.exp, I now see this test
Simon> failing on my CI.

Huh, I see it too.

I'll fix this, sorry about the breakage.

Tom