[PATCH 5/5] gdb: fix reread_symbols when an objfile has target: prefix

[Andrew Burgess <aburgess@redhat.com>]

When using a remote target, it is possible to tell GDB that the
executable to be debugged is located on the remote machine, like this:

  (gdb) target extended-remote :54321
  ... snip ...
  (gdb) file target:/tmp/hello.x
  Reading /tmp/hello.x from remote target...
  warning: File transfers from remote targets can be slow. Use "set sysroot" to access files locally instead.
  Reading /tmp/hello.x from remote target...
  Reading symbols from target:/tmp/hello.x...
  (gdb)

So far so good.  However, when we try to start the inferior we run
into a small problem:

  (gdb) set remote exec-file /tmp/hello.x
  (gdb) start
  `target:/tmp/hello.x' has disappeared; keeping its symbols.
  Temporary breakpoint 1 at 0x401198: file /tmp/hello.c, line 18.
  Starting program: target:/tmp/hello.x
  ... snip ...

  Temporary breakpoint 1, main () at /tmp/hello.c:18
  18	  printf ("Hello World\n");
  (gdb)

Notice this line:

  `target:/tmp/hello.x' has disappeared; keeping its symbols.

That's wrong, the executable hasn't been removed, GDB just doesn't
know how to check if the remote file has changed, and so falls back to
assuming that the file has been removed.

In this commit I add support to reread_symbols for stat-ing remote
files.  With this in place GDB no longer complains when using a remote
executable file.
---
 gdb/symfile.c                                 |  19 +-
 gdb/target.c                                  |  15 ++
 gdb/target.h                                  |  39 ++++
 gdb/testsuite/gdb.server/target-exec-file.c   |  22 +++
 gdb/testsuite/gdb.server/target-exec-file.exp | 174 ++++++++++++++++++
 5 files changed, 268 insertions(+), 1 deletion(-)
 create mode 100644 gdb/testsuite/gdb.server/target-exec-file.c
 create mode 100644 gdb/testsuite/gdb.server/target-exec-file.exp

diff --git a/gdb/symfile.c b/gdb/symfile.c
index 10074e281bd..b21c8ead225 100644
--- a/gdb/symfile.c
+++ b/gdb/symfile.c
@@ -2480,7 +2480,24 @@ reread_symbols (int from_tty)
       else
 	filename = objfile_name (objfile);
 
-      int res = stat (filename, &new_statbuf);
+      int res;
+      if (is_target_filename (filename))
+	{
+	  fileio_error target_errno;
+
+	  scoped_target_fileio_open fd (nullptr,
+					(filename
+					 + strlen (TARGET_SYSROOT_PREFIX)),
+					FILEIO_O_RDONLY, 0, false,
+					&target_errno);
+	  if (fd.get () == -1)
+	    res = -1;
+	  else
+	    res = target_fileio_fstat (fd.get (), &new_statbuf,
+				       &target_errno);
+	}
+      else
+	res = stat (filename, &new_statbuf);
       if (res != 0)
 	{
 	  warning (_("`%ps' has disappeared; keeping its symbols."),
diff --git a/gdb/target.c b/gdb/target.c
index 8cb4fa1736d..0d1608bb5fe 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -54,6 +54,7 @@
 #include "target-connection.h"
 #include "valprint.h"
 #include "cli/cli-decode.h"
+#include "cli/cli-style.h"
 
 static void generic_tls_error (void) ATTRIBUTE_NORETURN;
 
@@ -3577,6 +3578,20 @@ target_fileio_read_stralloc (struct inferior *inf, const char *filename)
   return gdb::unique_xmalloc_ptr<char> (bufstr);
 }
 
+/* See target.h.  */
+
+scoped_target_fileio_open::~scoped_target_fileio_open ()
+{
+  if (m_fd != -1)
+    {
+      fileio_error target_errno;
+      if (target_fileio_close (m_fd, &target_errno) != 0)
+	warning (_("failed to close %ps: %s"),
+		 styled_string (file_name_style.style (),
+				m_filename.c_str ()),
+		 safe_strerror (fileio_error_to_host (target_errno)));
+    }
+}
 
 static int
 default_region_ok_for_hw_watchpoint (struct target_ops *self,
diff --git a/gdb/target.h b/gdb/target.h
index 936ae79219c..5712adc367d 100644
--- a/gdb/target.h
+++ b/gdb/target.h
@@ -2235,6 +2235,45 @@ extern gdb::unique_xmalloc_ptr<char> target_fileio_read_stralloc
    with EIO.  */
 extern void fileio_handles_invalidate_target (target_ops *targ);
 
+/* A class that performs a target_fileio_open within a scope.  On leaving
+   the scope target_fileio_close is called.
+
+   If the close fails then a warning is issued.  */
+struct scoped_target_fileio_open
+{
+  /* Call target_fileio_open passing all the arguments through.  See
+     target_fileio_open for the meaning of all arguments.  */
+  scoped_target_fileio_open (struct inferior *inf,
+			     const char *filename, int flags,
+			     int mode, bool warn_if_slow,
+			     fileio_error *target_errno)
+    : m_filename (filename)
+  {
+    m_fd = target_fileio_open (inf, filename, flags, mode, warn_if_slow,
+			       target_errno);
+  }
+
+  /* Close the file that was opened in the constructor, issue a warning if
+     the close fails.  */
+  ~scoped_target_fileio_open ();
+
+  /* Return the file descriptor for the opened file.  This can only be used
+     with target_fileio_* calls.  This will return -1 if the open failed.  */
+  int get () const
+  {
+    return m_fd;
+  }
+
+private:
+  /* The filename that we opened.  Stored so we can give a warning if the
+     close fails for any reason.  */
+  std::string m_filename;
+
+  /* The target file descriptor for the opened file, or -1 if the open
+     failed for any reason.  */
+  int m_fd = -1;
+};
+
 /* Tracepoint-related operations.  */
 
 extern void target_trace_init ();
diff --git a/gdb/testsuite/gdb.server/target-exec-file.c b/gdb/testsuite/gdb.server/target-exec-file.c
new file mode 100644
index 00000000000..3a264f239ed
--- /dev/null
+++ b/gdb/testsuite/gdb.server/target-exec-file.c
@@ -0,0 +1,22 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2023 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/target-exec-file.exp b/gdb/testsuite/gdb.server/target-exec-file.exp
new file mode 100644
index 00000000000..9260df8b88d
--- /dev/null
+++ b/gdb/testsuite/gdb.server/target-exec-file.exp
@@ -0,0 +1,174 @@
+# This testcase is part of GDB, the GNU debugger.
+
+# Copyright 2023 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/>.
+
+# Test GDB's handling of using a file with a 'target:' prefix as the
+# executable file.  This test includes checking what happens when the
+# file on the target system changes and GDB needs to reload it.
+
+load_lib gdbserver-support.exp
+
+require allow_gdbserver_tests !use_gdb_stub
+
+standard_testfile
+
+if { [build_executable "failed to prepare" $testfile $srcfile debug] } {
+    return -1
+}
+
+clean_restart
+
+# Some boards specifically set the sysroot to the empty string to
+# avoid copying files from the target.  But for this test we do want
+# to copy files from the target, so set the sysroot back to 'target:'.
+#
+# This is fine so long as we're not using a board file that sets the
+# sysroot to something else -- but none of the standard boards do
+# this, and plenty of other tests mess with the sysroot, so I guess we
+# don't worry about that too much.
+gdb_test "set sysroot target:" ".*"
+
+# Make sure we're disconnected, in case we're testing with an
+# extended-remote board, therefore already connected.
+gdb_test "disconnect" ".*"
+
+# Ensure the executable is on the target.
+set target_exec [gdb_remote_download target $binfile]
+
+# We're going to be restarting the inferior.  Lets ask GDB not to
+# prompt us if this is the right thing to do.
+gdb_test_no_output "set confirm off"
+
+# Start gdbserver, but always in extended-remote mode, and then
+# connect to it from GDB.
+set res [gdbserver_start "--multi" $target_exec]
+set gdbserver_protocol "extended-remote"
+set gdbserver_gdbport [lindex $res 1]
+gdb_target_cmd $gdbserver_protocol $gdbserver_gdbport
+
+# Issue a 'file' command and parse the output.  We look for a couple
+# of specific things to ensure that we are correctly reading the exec
+# from the remote target.
+set saw_read_of_remote_exec false
+set saw_read_of_syms_from_exec false
+gdb_test_multiple "file target:$target_exec" "run file command" {
+    -re "^file target:\[^\r\n\]+\r\n" {
+	exp_continue
+    }
+
+    -re "^Reading (\[^\r\n\]+) from remote target\\.\\.\\.\r\n" {
+	set filename $expect_out(1,string)
+	if { $filename eq $target_exec } {
+	    set saw_read_of_remote_exec true
+	}
+	exp_continue
+    }
+
+    -re "^warning: File transfers from remote targets can be slow\[^\r\n\]+\r\n" {
+	exp_continue
+    }
+
+    -re "^Reading symbols from target:(\[^\r\n\]+)\\.\\.\\.\r\n" {
+	set filename $expect_out(1,string)
+	if { $filename eq $target_exec } {
+	    set saw_read_of_syms_from_exec true
+	}
+	exp_continue
+    }
+
+    -re "^Expanding full symbols from \[^\r\n\]+\r\n" {
+	exp_continue
+    }
+
+    -re "^$gdb_prompt $" {
+	pass $gdb_test_name
+    }
+}
+
+gdb_assert { $saw_read_of_remote_exec } \
+    "exec was read from the remote target"
+
+gdb_assert { $saw_read_of_syms_from_exec } \
+    "symbols were read from remote exec file"
+
+# Start the inferior (with the 'start' command), use TESTNAME for any
+# pass/fail calls.  EXPECT_REREAD should be true or false and
+# indicates if we expect to too a line like:
+#
+#  `FILE' has changed; re-reading symbols.
+proc start_inferior { testname expect_reread } {
+    with_test_prefix $testname {
+	if { [gdb_start_cmd] < 0 } {
+	    fail "start command"
+	    return -1
+	}
+
+	set saw_reread false
+	gdb_test_multiple "" "stopped at main" {
+	    -re "^start\\s*\r\n" {
+		exp_continue
+	    }
+	    -re "^`\[^\r\n\]+' has changed; re-reading symbols\\.\r\n" {
+		set saw_reread true
+		exp_continue
+	    }
+	    -re "^Reading \[^\r\n\]+ from remote target\\.\\.\\.\r\n" {
+		exp_continue
+	    }
+	    -re "^Expanding full symbols from \[^\r\n\]+\\.\\.\\.\r\n" {
+		exp_continue
+	    }
+	    -re "^Temporary breakpoint $::decimal at $::hex: \[^\r\n\]+\r\n" {
+		exp_continue
+	    }
+	    -re "^Starting program: \[^\r\n\]+\r\n" {
+		exp_continue
+	    }
+	    -re "^\\s*\r\n" {
+		exp_continue
+	    }
+	    -re "^Temporary breakpoint $::decimal, main \\(\\) at .*$::gdb_prompt $" {
+		pass $testname
+	    }
+	}
+
+	gdb_assert { $expect_reread == $saw_reread } \
+	    "check symbol re-read behaviour"
+    }
+}
+
+# Start the inferior for the first time.  The symbols were already
+# read from the file when the 'file' command was used, we should not
+# see the symbols re-read now.
+start_inferior "start inferior the first time" false
+
+# Re-start the inferior.  The executable is unchanged so we should not
+# see the symbol file being re-read.
+start_inferior "start inferior a second time" false
+
+# Delay for a short while so, when we touch the exec, we know the
+# timestamp will change.
+sleep 1
+set res [remote_exec target "touch $target_exec"]
+set status [lindex $res 0]
+if { $status != 0 } {
+    fail "touching executable on target"
+    return -1
+}
+
+# Start the inferior again, we expect to see the symbols being re-read
+# from the remote file.
+start_inferior "start inferior a third time" true
-- 
2.25.4