[PATCH 3/8] gdb: warn of slow remote file reading only after a successful open

[Andrew Burgess <aburgess@redhat.com>]

While working on a later patch in this series, I noticed that GDB
would print the message:

  Reading /path/to/file from remote target...

Even when /path/to/file doesn't exist on the remote target.

GDB does indeed try to open /path/to/file, but I'm not sure we really
need to tell the user unless we actually manage to open the file, and
plan to read content from it.

If we consider how GDB probes for separate debug files, we can attempt
to open multiple possible files, most of them will not exist.  When we
are native debugging we don't bother telling the user about each file
we're checking for, we just announce any file we finally use.

I think it makes sense to do a similar thing for remote files.

So, in remote_target::remote_hostio_open(), I'd like to move the block
of code that prints the above message to after the open call has been
made, and we should only print the message if the open succeeds.

Now GDB only tells the user about files that we actually open and read
from the remote.
---
 gdb/remote.c                                  |  36 +++---
 gdb/testsuite/gdb.server/remote-read-msgs.c   |  22 ++++
 gdb/testsuite/gdb.server/remote-read-msgs.exp | 119 ++++++++++++++++++
 3 files changed, 160 insertions(+), 17 deletions(-)
 create mode 100644 gdb/testsuite/gdb.server/remote-read-msgs.c
 create mode 100644 gdb/testsuite/gdb.server/remote-read-msgs.exp

diff --git a/gdb/remote.c b/gdb/remote.c
index 241803ab2b3..4742cdf2606 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -12751,7 +12751,24 @@ remote_target::remote_hostio_open (inferior *inf, const char *filename,
   char *p = rs->buf.data ();
   int left = get_remote_packet_size () - 1;
 
-  if (warn_if_slow)
+  if (remote_hostio_set_filesystem (inf, remote_errno) != 0)
+    return -1;
+
+  remote_buffer_add_string (&p, &left, "vFile:open:");
+
+  remote_buffer_add_bytes (&p, &left, (const gdb_byte *) filename,
+			   strlen (filename));
+  remote_buffer_add_string (&p, &left, ",");
+
+  remote_buffer_add_int (&p, &left, flags);
+  remote_buffer_add_string (&p, &left, ",");
+
+  remote_buffer_add_int (&p, &left, mode);
+
+  int res = remote_hostio_send_command (p - rs->buf.data (), PACKET_vFile_open,
+					remote_errno, nullptr, nullptr);
+
+  if (warn_if_slow && res != -1)
     {
       static int warning_issued = 0;
 
@@ -12767,22 +12784,7 @@ remote_target::remote_hostio_open (inferior *inf, const char *filename,
 	}
     }
 
-  if (remote_hostio_set_filesystem (inf, remote_errno) != 0)
-    return -1;
-
-  remote_buffer_add_string (&p, &left, "vFile:open:");
-
-  remote_buffer_add_bytes (&p, &left, (const gdb_byte *) filename,
-			   strlen (filename));
-  remote_buffer_add_string (&p, &left, ",");
-
-  remote_buffer_add_int (&p, &left, flags);
-  remote_buffer_add_string (&p, &left, ",");
-
-  remote_buffer_add_int (&p, &left, mode);
-
-  return remote_hostio_send_command (p - rs->buf.data (), PACKET_vFile_open,
-				     remote_errno, NULL, NULL);
+  return res;
 }
 
 int
diff --git a/gdb/testsuite/gdb.server/remote-read-msgs.c b/gdb/testsuite/gdb.server/remote-read-msgs.c
new file mode 100644
index 00000000000..bbcfb01316e
--- /dev/null
+++ b/gdb/testsuite/gdb.server/remote-read-msgs.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 ()
+{
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.server/remote-read-msgs.exp b/gdb/testsuite/gdb.server/remote-read-msgs.exp
new file mode 100644
index 00000000000..d2d659aa365
--- /dev/null
+++ b/gdb/testsuite/gdb.server/remote-read-msgs.exp
@@ -0,0 +1,119 @@
+# 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/>.
+
+# Setup a number of directories in the debug-file-directory, start gdbserver
+# and connect GDB.  The debug information is downloaded from the last
+# directory in the debug-file-directory.
+#
+# We are checking that GDB reports 'Reading <filename> from remote target'
+# only for the paths that are actually being read, and not for paths that
+# GDB is simply probing for existence.
+
+load_lib gdbserver-support.exp
+
+require allow_gdbserver_tests
+require {!is_remote host}
+
+standard_testfile
+
+if {[build_executable "failed to prepare" $testfile $srcfile] == -1} {
+    return -1
+}
+
+# Split out BINFILE.debug.  Remove debug from BINFILE.
+if {[gdb_gnu_strip_debug $binfile] != 0} {
+    return -1
+}
+
+# Get the '.build-id/xx/xxx...xxx' part of the filename.
+set build_id_filename [build_id_debug_filename_get $binfile]
+
+set hidden_binfile [standard_output_file "hidden_$testfile"]
+set hidden_debuginfo [standard_output_file "hidden_$testfile.debug"]
+
+# Hide (rename) BINFILE and associated debug information, this should ensure
+# GDB can't find it directly.
+remote_exec build "mv $binfile $hidden_binfile"
+remote_exec build "mv ${binfile}.debug $hidden_debuginfo"
+
+# Helper called from gdb_finish when the 'target' is remote.  Ensure the
+# debug directory we create is deleted.
+proc cleanup_remote_target {} {
+    remote_exec target "rm -fr debug/"
+}
+
+if { ![is_remote target] } {
+    set gdbserver_dir [standard_output_file "gdbserver-dir"]/
+} else {
+    lappend gdb_finish_hooks cleanup_remote_target
+    set gdbserver_dir ""
+}
+
+# Copy files to the target (if needed).
+set target_binfile [gdb_remote_download target $hidden_binfile]
+set target_debuginfo [gdb_remote_download target $hidden_debuginfo]
+
+# Setup the debug information on the target.
+remote_exec target \
+    "mkdir -p ${gdbserver_dir}debug/[file dirname $build_id_filename]"
+remote_exec target \
+    "ln -sf $target_debuginfo ${gdbserver_dir}debug/$build_id_filename"
+
+# Reading debug info from the remote target can take a bit of time, so
+# increase the timeout.
+with_timeout_factor 5 {
+    # Restart GDB.
+    clean_restart
+
+    # Add some dummy entries to the debug-file-directory search list.
+    gdb_test_no_output "set debug-file-directory xxx:yyy:debug"
+
+    # Set the sysroot.
+    gdb_test_no_output "set sysroot target:"
+
+    # Make sure we're disconnected, in case we're testing with an
+    # extended-remote board, therefore already connected.
+    gdb_test "disconnect" ".*"
+
+    # Start gdbserver.  This needs to be done after starting GDB.  When
+    # gdbserver is running local to GDB, start gdbserver in a sub-directory,
+    # this prevents GDB from finding the debug information itself.
+    if { ![is_remote target] } {
+	with_cwd $gdbserver_dir {
+	    set res [gdbserver_start "" $target_binfile]
+	}
+    } else {
+	set res [gdbserver_start "" $target_binfile]
+    }
+    set gdbserver_protocol [lindex $res 0]
+    set gdbserver_gdbport [lindex $res 1]
+
+    # Connect to gdbserver.  The output will be placed into the global
+    # GDB_TARGET_REMOTE_CMD_MSG, and we'll match against this below.
+    gdb_assert {[gdb_target_cmd $gdbserver_protocol $gdbserver_gdbport] == 0} \
+	"connect to gdbserver"
+
+    gdb_assert { ![regexp "Reading xxx/\[^\r\n\]+ from remote target\\.\\.\\.\r\n" \
+		       $gdb_target_remote_cmd_msg] \
+		     && ![regexp "Reading yyy/\[^\r\n\]+ from remote target\\.\\.\\.\r\n" \
+			      $gdb_target_remote_cmd_msg] } \
+	"check xxx/ and yyy/ are not mentioned"
+
+    gdb_assert { [regexp "Reading debug/[string_to_regexp $build_id_filename] from remote target\\.\\.\\.\r\n" \
+		      $gdb_target_remote_cmd_msg] } \
+	"check debug information is found"
+}
-- 
2.25.4