[v5] Add connect.exp

  This test starts a communication with gdbsever setting
the remotelog file. Then, it modifies the remotelog with
update_log proc, injects an error message instead of
the expected replay to the vMustReplyEmpty packet in order
to test GDB reacts to the error response properly. After
the remotelog modification, this test restarts GDB and starts
communication with gdbreply instead of the gdbserver using
the remotelog.

Add a lib/gdbreplay-support.exp. update_log proc matches lines
from GDB to gdbserver in a remotelogfile. Once a match is found then
NEWLINE is used to build a replacement line to send from gdbserver to
GDB.
---
 gdb/testsuite/gdb.replay/connect.c      |  22 ++++
 gdb/testsuite/gdb.replay/connect.exp    |  99 ++++++++++++++++
 gdb/testsuite/lib/gdbreplay-support.exp | 144 ++++++++++++++++++++++++
 gdbserver/gdbreplay.cc                  |   6 +-
 4 files changed, 268 insertions(+), 3 deletions(-)
 create mode 100644 gdb/testsuite/gdb.replay/connect.c
 create mode 100644 gdb/testsuite/gdb.replay/connect.exp
 create mode 100644 gdb/testsuite/lib/gdbreplay-support.exp
  

Ping

On Thu, Dec 5, 2024 at 10:33 AM Alexandra Hájková <ahajkova@redhat.com>
wrote:

> This test starts a communication with gdbsever setting
> the remotelog file. Then, it modifies the remotelog with
> update_log proc, injects an error message instead of
> the expected replay to the vMustReplyEmpty packet in order
> to test GDB reacts to the error response properly. After
> the remotelog modification, this test restarts GDB and starts
> communication with gdbreply instead of the gdbserver using
> the remotelog.
>
> Add a lib/gdbreplay-support.exp. update_log proc matches lines
> from GDB to gdbserver in a remotelogfile. Once a match is found then
> NEWLINE is used to build a replacement line to send from gdbserver to
> GDB.
> ---
>  gdb/testsuite/gdb.replay/connect.c      |  22 ++++
>  gdb/testsuite/gdb.replay/connect.exp    |  99 ++++++++++++++++
>  gdb/testsuite/lib/gdbreplay-support.exp | 144 ++++++++++++++++++++++++
>  gdbserver/gdbreplay.cc                  |   6 +-
>  4 files changed, 268 insertions(+), 3 deletions(-)
>  create mode 100644 gdb/testsuite/gdb.replay/connect.c
>  create mode 100644 gdb/testsuite/gdb.replay/connect.exp
>  create mode 100644 gdb/testsuite/lib/gdbreplay-support.exp
>
> diff --git a/gdb/testsuite/gdb.replay/connect.c
> b/gdb/testsuite/gdb.replay/connect.c
> new file mode 100644
> index 00000000000..d96cd4b5eb5
> --- /dev/null
> +++ b/gdb/testsuite/gdb.replay/connect.c
> @@ -0,0 +1,22 @@
> +/* This testcase is part of GDB, the GNU debugger.
> +
> +   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/>.
> */
> +
> +int
> +main (int argc, char **argv)
> +{
> +  return 0;
> +}
> diff --git a/gdb/testsuite/gdb.replay/connect.exp
> b/gdb/testsuite/gdb.replay/connect.exp
> new file mode 100644
> index 00000000000..e2b85fd0d17
> --- /dev/null
> +++ b/gdb/testsuite/gdb.replay/connect.exp
> @@ -0,0 +1,99 @@
> +# 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/>.
> */
> +#
> +# Starts a communication with gdbsever setting the remotelog file.
> +# Modifies the remotelog with update_log proc, injects an error message
> +# instead of the expected replay to the vMustReplyEmpty packet in order
> +# to test GDB reacts to the error response properly. After the remotelog
> +# modification, this test restarts GDB and starts communication with
> gdbreply
> +# instead of the gdbserver using the remotelog.
> +
> +load_lib gdbserver-support.exp
> +load_lib gdbreplay-support.exp
> +
> +require allow_gdbserver_tests
> +require has_gdbreplay
> +
> +standard_testfile
> +
> +if { [prepare_for_testing "failed to prepare" $testfile $srcfile] } {
> +      return -1
> +  }
> +
> +# Make sure we're disconnected, in case we're testing with an
> +# extended-remote board, therefore already connected.
> +gdb_test "disconnect" ".*"
> +
> +gdb_test_no_output "set sysroot" \
> +    "setting sysroot before starting gdbreplay"
> +
> +# Start gdbserver like:
> +#   gdbserver :PORT ....
> +set res [gdbserver_start "" $binfile]
> +set gdbserver_protocol [lindex $res 0]
> +set gdbserver_gdbport [lindex $res 1]
> +
> +# The replay log is placed in 'replay.log'.
> +set remotelog [standard_output_file replay.log]
> +
> +gdb_test_no_output "set remotelogfile $remotelog" \
> +    "setup remotelogfile"
> +
> +# Connect to gdbserver, making sure GDB reads in the binary correctly.
> +if {![gdb_target_cmd $gdbserver_protocol $gdbserver_gdbport] == 0} {
> +    unsupported "$testfile (couldn't start gdbserver)"
> +    return
> +}
> +
> +# If we're connecting as 'remote' then we can't use 'runto'.
> +gdb_breakpoint main
> +gdb_continue_to_breakpoint "continuing to main"
> +
> +clean_restart $binfile
> +# Make sure we're disconnected, in case we're testing with an
> +# extended-remote board, therefore already connected.
> +gdb_test "disconnect2" ".*"
> +
> +gdb_test_no_output "set sysroot" "setting sysroot"
> +
> +set res [gdbreplay_start $remotelog]
> +set gdbserver_protocol [lindex $res 0]
> +set gdbserver_gdbport [lindex $res 1]
> +
> +gdb_test "target $gdbserver_protocol $gdbserver_gdbport" ".*" \
> +    "set target remote to gdbserver"
> +gdb_breakpoint main
> +gdb_continue_to_breakpoint "continue to main"
> +
> +set newline E.errtext
> +set output_file [standard_output_file ${testfile}_out.log]
> +
> +# modify the log file by changing the *response* to
> +# the vMustReplayEmty packet to an error
> +update_log $remotelog $output_file "vMustReplyEmpty" $newline
> +
> +clean_restart $binfile
> +# Make sure we're disconnected, in case we're testing with an
> +# extended-remote board, therefore already connected.
> +gdb_test "disconnect3" ".*"
> +
> +gdb_test_no_output "set sysroot"
> +
> +set res [gdbreplay_start $output_file]
> +set gdbserver_protocol [lindex $res 0]
> +set gdbserver_gdbport [lindex $res 1]
> +
> +gdb_test "target $gdbserver_protocol $gdbserver_gdbport" ".*Remote
> replied unexpectedly.*"\
> +    "set target remote to gdbreplay"
> diff --git a/gdb/testsuite/lib/gdbreplay-support.exp
> b/gdb/testsuite/lib/gdbreplay-support.exp
> new file mode 100644
> index 00000000000..8a6194ccd32
> --- /dev/null
> +++ b/gdb/testsuite/lib/gdbreplay-support.exp
> @@ -0,0 +1,144 @@
> +# 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/>.
> +
> +# We're going to reuse some helper function from the gdbserver library.
> +load_lib gdbserver-support.exp
> +
> +if {![info exists GDBREPLAY]} {
> +    set GDBREPLAY [findfile $base_dir/../../gdbserver/gdbreplay]
> +} else {
> +    set GDBREPLAY ""
> +}
> +
> +global GDBREPLAY
> +verbose "using GDBREPLAY = $GDBREPLAY" 2
> +
> +proc has_gdbreplay {} {
> +    global GDBREPLAY
> +    if {$GDBREPLAY == ""} {
> +       return false
> +    }
> +
> +    # We currently rely on running gdbreplay on the same machine as
> +    # GDB.
> +    if {[is_remote target]} {
> +       return false
> +    }
> +
> +    return true
> +}
> +
> +# Write the command line used to invocate gdbserver to the cmd file.
> +
> +proc gdbreplay_write_cmd_file { cmdline } {
> +    set logfile [standard_output_file_with_gdb_instance gdbreplay.cmd]
> +    set cmd_file [open $logfile w]
> +    puts $cmd_file $cmdline
> +    catch "close $cmd_file"
> +}
> +
> +# Start gdbreplay using REMOTELOG as the log file.  Return a list of
> +# two elements, the protocol and the hostname:port string.  This
> +# result list has the same format as gdbserver_start.
> +
> +proc gdbreplay_start { remotelog } {
> +    # Port id -- either specified in baseboard file, or managed here.
> +    set portnum [get_portnum]
> +
> +    # Extract the protocol
> +    if [target_info exists gdb_protocol] {
> +       set protocol [target_info gdb_protocol]
> +    } else {
> +       set protocol "remote"
> +    }
> +
> +    # Loop till we find a free port.
> +    while 1 {
> +       # Fire off the debug agent.
> +       set gdbreplay_command "$::GDBREPLAY $remotelog localhost:$portnum"
> +
> +       gdbreplay_write_cmd_file $gdbreplay_command
> +
> +       global gdbreplay_spawn_id
> +       set gdbreplay_spawn_id [remote_spawn target $gdbreplay_command]
> +
> +       # Wait for the server to open its TCP socket, so that GDB can
> connect.
> +       expect {
> +           -i $gdbreplay_spawn_id
> +           -timeout 120
> +           -notransfer
> +           -re "Replay logfile using" { }
> +           -re "Can't (bind address|listen on socket): Address already in
> use\\.\r\n" {
> +               verbose -log "Port $portnum is already in use."
> +               set other_portnum [get_portnum]
> +               if { $other_portnum != $portnum } {
> +                   # Bump the port number to avoid the conflict.
> +                   wait -i $expect_out(spawn_id)
> +                   set portnum $other_portnum
> +                   continue
> +               }
> +           }
> +           -re ".*: cannot resolve name: .*\r\n" {
> +               error "gdbserver cannot resolve name."
> +           }
> +           -re "Can't open socket: Address family not supported by
> protocol." {
> +               return {}
> +           }
> +           timeout {
> +               error "Timeout waiting for gdbreplay response."
> +           }
> +       }
> +       break
> +    }
> +
> +    return [list $protocol "localhost:$portnum"]
> +}
> +
> +# MATCH_REGEXP matches lines from GDB to gdbserver.  Once a match is
> +# found then NEWLINE is used to build a replacement line to send from
> +# gdbserver to GDB.
> +#
> +# Examples of MATCH_REGEXP:  "vMustReplyEmpty"
> +#
> +# Example usage:
> +#
> +# update_log $logname "${logname}.updated" "vMustReplyEmpty" "E.failed"
> +
> +proc update_log { filename_in filename_out match_regexp newline } {
> +    set fh_in [open $filename_in r]
> +    set fh_out [open $filename_out w]
> +
> +    while { [gets $fh_in line] >= 0 } {
> +       # Print the line to the file.
> +       puts $fh_out $line
> +       if { [regexp $match_regexp $line] } {
> +           # print out NEWLINE.
> +           puts $fh_out "r +\$${newline}"
> +
> +           # Don't truncate the file, otherwise gdbreplay will
> +           # close the connection early and this might impact
> +           # what GDB does.  We want GDB to get a chance to
> +           # process the error.
> +           puts $fh_out "c q"
> +           puts $fh_out "w \$qTStatus#49"
> +           puts $fh_out "End of log"
> +
> +           break
> +       }
> +    }
> +
> +    close $fh_out
> +    close $fh_in
> +}
> diff --git a/gdbserver/gdbreplay.cc b/gdbserver/gdbreplay.cc
> index a9a55924ca9..ec4dcd36a9f 100644
> --- a/gdbserver/gdbreplay.cc
> +++ b/gdbserver/gdbreplay.cc
> @@ -205,6 +205,9 @@ remote_open (const char *name)
>    if (bind (tmp_desc, p->ai_addr, p->ai_addrlen) != 0)
>      perror_with_name ("Can't bind address");
>
> +  fprintf (stderr, "Replay logfile using %s\n", name);
> +  fflush (stderr);
> +
>    if (p->ai_socktype == SOCK_DGRAM)
>      remote_desc_in = tmp_desc;
>    else
> @@ -258,9 +261,6 @@ remote_open (const char *name)
>    fcntl (remote_desc_in, F_SETFL, FASYNC);
>  #endif
>    remote_desc_out = remote_desc_in;
> -
> -  fprintf (stderr, "Replay logfile using %s\n", name);
> -  fflush (stderr);
>  }
>
>  static int
> --
> 2.47.0
>
>
  

I would suggest changing the subject line to:

  gdb: add first gdbreplay test, connect.exp

The 'gdb:' prefix because we share a repository with binutils, so
tagging gdb commits helps make things clear.

Mentioning that this is a 'gdbreplay' test will help people understand
what the patch is about without having to read the full description.

Alexandra Hájková <ahajkova@redhat.com> writes:

> This test starts a communication with gdbsever setting
> the remotelog file. Then, it modifies the remotelog with
> update_log proc, injects an error message instead of
> the expected replay to the vMustReplyEmpty packet in order
> to test GDB reacts to the error response properly. After
> the remotelog modification, this test restarts GDB and starts
> communication with gdbreply instead of the gdbserver using
> the remotelog.

I feel like you're diving straight into the detail here, and skipping
over the headline change: this is adding a test that makes use of
gdbreply to test gdb/gdbserver error cases.  Maybe rewrite the commit
message, start with a couple of sentences explaining the high level
objective and why it's helpful to GDB.

Then dive into the detailed description of the test that you have
above.

>
> Add a lib/gdbreplay-support.exp. update_log proc matches lines
> from GDB to gdbserver in a remotelogfile. Once a match is found then
> NEWLINE is used to build a replacement line to send from gdbserver to

Remember NEWLINE doesn't mean anything at this point.  Ideally, the
commit message should make sense without having to consult the code.

> GDB.
> ---
>  gdb/testsuite/gdb.replay/connect.c      |  22 ++++
>  gdb/testsuite/gdb.replay/connect.exp    |  99 ++++++++++++++++
>  gdb/testsuite/lib/gdbreplay-support.exp | 144 ++++++++++++++++++++++++
>  gdbserver/gdbreplay.cc                  |   6 +-
>  4 files changed, 268 insertions(+), 3 deletions(-)
>  create mode 100644 gdb/testsuite/gdb.replay/connect.c
>  create mode 100644 gdb/testsuite/gdb.replay/connect.exp
>  create mode 100644 gdb/testsuite/lib/gdbreplay-support.exp
>
> diff --git a/gdb/testsuite/gdb.replay/connect.c b/gdb/testsuite/gdb.replay/connect.c
> new file mode 100644
> index 00000000000..d96cd4b5eb5
> --- /dev/null
> +++ b/gdb/testsuite/gdb.replay/connect.c
> @@ -0,0 +1,22 @@
> +/* This testcase is part of GDB, the GNU debugger.
> +
> +   Copyright 2024 Free Software Foundation, Inc.

When you update/merge, remember to change the date here to '2024-2025'.

> +
> +   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/>.  */
> +
> +int
> +main (int argc, char **argv)
> +{
> +  return 0;
> +}
> diff --git a/gdb/testsuite/gdb.replay/connect.exp b/gdb/testsuite/gdb.replay/connect.exp
> new file mode 100644
> index 00000000000..e2b85fd0d17
> --- /dev/null
> +++ b/gdb/testsuite/gdb.replay/connect.exp
> @@ -0,0 +1,99 @@
> +# Copyright 2024 Free Software Foundation, Inc.

And the date here.

> +
> +# 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/>.  */
> +#
> +# Starts a communication with gdbsever setting the remotelog file.
> +# Modifies the remotelog with update_log proc, injects an error message
> +# instead of the expected replay to the vMustReplyEmpty packet in order
> +# to test GDB reacts to the error response properly. After the remotelog
> +# modification, this test restarts GDB and starts communication with gdbreply
> +# instead of the gdbserver using the remotelog.
> +
> +load_lib gdbserver-support.exp
> +load_lib gdbreplay-support.exp
> +
> +require allow_gdbserver_tests
> +require has_gdbreplay
> +
> +standard_testfile
> +
> +if { [prepare_for_testing "failed to prepare" $testfile $srcfile] } {
> +      return -1
> +  }

Weird indentation of the trailing '}' here.

> +
> +# Make sure we're disconnected, in case we're testing with an
> +# extended-remote board, therefore already connected.
> +gdb_test "disconnect" ".*"
> +
> +gdb_test_no_output "set sysroot" \
> +    "setting sysroot before starting gdbreplay"

s/gdbreplay/gdbserver/ here I think.

> +
> +# Start gdbserver like:
> +#   gdbserver :PORT ....
> +set res [gdbserver_start "" $binfile]
> +set gdbserver_protocol [lindex $res 0]
> +set gdbserver_gdbport [lindex $res 1]
> +
> +# The replay log is placed in 'replay.log'.
> +set remotelog [standard_output_file replay.log]
> +
> +gdb_test_no_output "set remotelogfile $remotelog" \
> +    "setup remotelogfile"
> +
> +# Connect to gdbserver, making sure GDB reads in the binary correctly.

I don't think the 'making sure GDB reads in the binary correctly.' part
is correct, I think gdb_target_cmd just connects and waits for the prompt.

> +if {![gdb_target_cmd $gdbserver_protocol $gdbserver_gdbport] == 0} {
> +    unsupported "$testfile (couldn't start gdbserver)"
> +    return
> +}
> +
> +# If we're connecting as 'remote' then we can't use 'runto'.
> +gdb_breakpoint main
> +gdb_continue_to_breakpoint "continuing to main"
> +
> +clean_restart $binfile
> +# Make sure we're disconnected, in case we're testing with an
> +# extended-remote board, therefore already connected.
> +gdb_test "disconnect2" ".*"

The 'disconnect2' here is not a command.  I guess the intention was to
stop DejaGNU complaining about duplicate test names?  I think what you
should do is consider restructuring this script so that each part of the
test is done as a separate proc.  As in:

  # ... build executable here ...

  proc_with_prefix record_initial_logfile {} {
    ...
  }

  proc_with_prefix replay_without_error {} {
    ...
  }

  proc_with_prefix replay_with_mustreplyempty_error {} {
    ...
  }

  record_initial_logfile
  replay_without_error
  replay_with_mustreplyempty_error

An alternative to this would be to just wrap the different stages of the
test in 'with_test_prefix "string" { ... }', or, my least favourite,
just give each test a unique name as in:

  gdb_test "disconnect" ".*" "disconnect before replay without error"

but you often find you end up using the same prefix string all over the
place, which is why I think the other choices are better.

> +
> +gdb_test_no_output "set sysroot" "setting sysroot"
> +
> +set res [gdbreplay_start $remotelog]
> +set gdbserver_protocol [lindex $res 0]
> +set gdbserver_gdbport [lindex $res 1]
> +
> +gdb_test "target $gdbserver_protocol $gdbserver_gdbport" ".*" \
> +    "set target remote to gdbserver"

I think you could use gdb_target_cmd again here.  Seemed to work when I
tried.  At a minimum, the test name is wrong here: s/gdbserver/gdbreplay/.

> +gdb_breakpoint main
> +gdb_continue_to_breakpoint "continue to main"
> +
> +set newline E.errtext
> +set output_file [standard_output_file ${testfile}_out.log]
> +
> +# modify the log file by changing the *response* to
> +# the vMustReplayEmty packet to an error

Remember, comments should be full sentences.  So capital letter and full
stop required.

> +update_log $remotelog $output_file "vMustReplyEmpty" $newline
> +
> +clean_restart $binfile
> +# Make sure we're disconnected, in case we're testing with an
> +# extended-remote board, therefore already connected.
> +gdb_test "disconnect3" ".*"

Unknown command: disconnect3.

> +
> +gdb_test_no_output "set sysroot"
> +
> +set res [gdbreplay_start $output_file]
> +set gdbserver_protocol [lindex $res 0]
> +set gdbserver_gdbport [lindex $res 1]
> +
> +gdb_test "target $gdbserver_protocol $gdbserver_gdbport" ".*Remote replied unexpectedly.*"\
> +    "set target remote to gdbreplay"

You can possibly make use of gdb_target_cmd again here, and just provide
an additional text pattern to match the expected error.  See
gdb_target_cmd_ext in gdbserver-support.exp for details.

Additionally, I'd expand the expected error message to match more
context, the full error is:

  Remote replied unexpectedly to 'vMustReplyEmpty': E.errtext

I'd be sure to match at least that full string.

An interesting aside here: That the 'E.' appears in the output seems
like a bug.  I thought at one point you fixed many of these so that we'd
only see 'errtext' in this case, but I guess this one got missed.  But
that could be fixed as a follow up task, certainly shouldn't be done in
this commit.

> diff --git a/gdb/testsuite/lib/gdbreplay-support.exp b/gdb/testsuite/lib/gdbreplay-support.exp
> new file mode 100644
> index 00000000000..8a6194ccd32
> --- /dev/null
> +++ b/gdb/testsuite/lib/gdbreplay-support.exp
> @@ -0,0 +1,144 @@
> +# Copyright 2024 Free Software Foundation, Inc.

Date again here.

> +
> +# 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/>.
> +
> +# We're going to reuse some helper function from the gdbserver library.
> +load_lib gdbserver-support.exp
> +
> +if {![info exists GDBREPLAY]} {
> +    set GDBREPLAY [findfile $base_dir/../../gdbserver/gdbreplay]
> +} else {
> +    set GDBREPLAY ""
> +}

I don't think the 'else' case is needed?  If GDBREPLAY doesn't exist
then we try to find the binary in the build tree.  But if it does exist
already, then shouldn't we trust the user and use that?  Right now if
they user tries to set their own GDBREPLAY value then we'll set it to
the empty string and not run the gdbreplay tests, which seems weird.

> +
> +global GDBREPLAY
> +verbose "using GDBREPLAY = $GDBREPLAY" 2
> +
> +proc has_gdbreplay {} {

Comment before this proc please.

Thanks,
Andrew

> +    global GDBREPLAY
> +    if {$GDBREPLAY == ""} {
> +	return false
> +    }
> +
> +    # We currently rely on running gdbreplay on the same machine as
> +    # GDB.
> +    if {[is_remote target]} {
> +	return false
> +    }
> +
> +    return true
> +}
> +
> +# Write the command line used to invocate gdbserver to the cmd file.
> +
> +proc gdbreplay_write_cmd_file { cmdline } {
> +    set logfile [standard_output_file_with_gdb_instance gdbreplay.cmd]
> +    set cmd_file [open $logfile w]
> +    puts $cmd_file $cmdline
> +    catch "close $cmd_file"
> +}
> +
> +# Start gdbreplay using REMOTELOG as the log file.  Return a list of
> +# two elements, the protocol and the hostname:port string.  This
> +# result list has the same format as gdbserver_start.
> +
> +proc gdbreplay_start { remotelog } {
> +    # Port id -- either specified in baseboard file, or managed here.
> +    set portnum [get_portnum]
> +
> +    # Extract the protocol
> +    if [target_info exists gdb_protocol] {
> +	set protocol [target_info gdb_protocol]
> +    } else {
> +	set protocol "remote"
> +    }
> +
> +    # Loop till we find a free port.
> +    while 1 {
> +	# Fire off the debug agent.
> +	set gdbreplay_command "$::GDBREPLAY $remotelog localhost:$portnum"
> +
> +	gdbreplay_write_cmd_file $gdbreplay_command
> +
> +	global gdbreplay_spawn_id
> +	set gdbreplay_spawn_id [remote_spawn target $gdbreplay_command]
> +
> +	# Wait for the server to open its TCP socket, so that GDB can connect.
> +	expect {
> +	    -i $gdbreplay_spawn_id
> +	    -timeout 120
> +	    -notransfer
> +	    -re "Replay logfile using" { }
> +	    -re "Can't (bind address|listen on socket): Address already in use\\.\r\n" {
> +		verbose -log "Port $portnum is already in use."
> +		set other_portnum [get_portnum]
> +		if { $other_portnum != $portnum } {
> +		    # Bump the port number to avoid the conflict.
> +		    wait -i $expect_out(spawn_id)
> +		    set portnum $other_portnum
> +		    continue
> +		}
> +	    }
> +	    -re ".*: cannot resolve name: .*\r\n" {
> +		error "gdbserver cannot resolve name."
> +	    }
> +	    -re "Can't open socket: Address family not supported by protocol." {
> +		return {}
> +	    }
> +	    timeout {
> +		error "Timeout waiting for gdbreplay response."
> +	    }
> +	}
> +	break
> +    }
> +
> +    return [list $protocol "localhost:$portnum"]
> +}
> +
> +# MATCH_REGEXP matches lines from GDB to gdbserver.  Once a match is
> +# found then NEWLINE is used to build a replacement line to send from
> +# gdbserver to GDB.
> +#
> +# Examples of MATCH_REGEXP:  "vMustReplyEmpty"
> +#
> +# Example usage:
> +#
> +# update_log $logname "${logname}.updated" "vMustReplyEmpty" "E.failed"
> +
> +proc update_log { filename_in filename_out match_regexp newline } {
> +    set fh_in [open $filename_in r]
> +    set fh_out [open $filename_out w]
> +
> +    while { [gets $fh_in line] >= 0 } {
> +	# Print the line to the file.
> +	puts $fh_out $line
> +	if { [regexp $match_regexp $line] } {
> +	    # print out NEWLINE.
> +	    puts $fh_out "r +\$${newline}"
> +
> +	    # Don't truncate the file, otherwise gdbreplay will
> +	    # close the connection early and this might impact
> +	    # what GDB does.  We want GDB to get a chance to
> +	    # process the error.
> +	    puts $fh_out "c q"
> +	    puts $fh_out "w \$qTStatus#49"
> +	    puts $fh_out "End of log"
> +
> +	    break
> +	}
> +    }
> +
> +    close $fh_out
> +    close $fh_in
> +}
> diff --git a/gdbserver/gdbreplay.cc b/gdbserver/gdbreplay.cc
> index a9a55924ca9..ec4dcd36a9f 100644
> --- a/gdbserver/gdbreplay.cc
> +++ b/gdbserver/gdbreplay.cc
> @@ -205,6 +205,9 @@ remote_open (const char *name)
>    if (bind (tmp_desc, p->ai_addr, p->ai_addrlen) != 0)
>      perror_with_name ("Can't bind address");
>  
> +  fprintf (stderr, "Replay logfile using %s\n", name);
> +  fflush (stderr);
> +
>    if (p->ai_socktype == SOCK_DGRAM)
>      remote_desc_in = tmp_desc;
>    else
> @@ -258,9 +261,6 @@ remote_open (const char *name)
>    fcntl (remote_desc_in, F_SETFL, FASYNC);
>  #endif
>    remote_desc_out = remote_desc_in;
> -
> -  fprintf (stderr, "Replay logfile using %s\n", name);
> -  fflush (stderr);
>  }
>  
>  static int
> -- 
> 2.47.0