[PATCH 0/5] Fix using an exec file with target: prefix

[PATCH 0/5] Fix using an exec file with target: prefix

[Andrew Burgess]

I spotted that if a user specifies an executable with a target:
prefix, e.g.:

  (gdb) file target:/path/to/exec

Then GDB _almost_ does what we'd want.  This is definitely suppsosed
to work as there's some code in place to support this .... but there's
also a bug.  Patch #5 fixes this issue.

Patches #1 to #4 are various bits of cleanup and refactoring that fell
out while I was working on patch #5.

---

Andrew Burgess (5):
  gdb: some additional filename styling
  gdb: use archive name in warning when appropriate
  gdb: remove use of a static buffer for building error strings
  gdb: remove print_sys_errmsg
  gdb: fix reread_symbols when an objfile has target: prefix

 gdb/inflow.c                                  |   3 +-
 gdb/main.c                                    |   7 +-
 gdb/procfs.c                                  |  32 ++--
 gdb/source.c                                  |  18 +-
 gdb/symfile.c                                 |  30 ++-
 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 ++++++++++++++++++
 gdb/utils.c                                   |   9 +-
 gdb/utils.h                                   |   8 +-
 gdb/windows-nat.c                             |   2 +-
 12 files changed, 313 insertions(+), 46 deletions(-)
 create mode 100644 gdb/testsuite/gdb.server/target-exec-file.c
 create mode 100644 gdb/testsuite/gdb.server/target-exec-file.exp


base-commit: f586e3409b752748bf213520c2dbb0b44e0005d8
-- 
2.25.4

[PATCH 1/5] gdb: some additional filename styling

[Andrew Burgess]

Fix up another couple of places where we can apply filename styling.
---
 gdb/source.c  | 3 ++-
 gdb/symfile.c | 5 +++--
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/gdb/source.c b/gdb/source.c
index 9c701e866a6..5bdd729be8b 100644
--- a/gdb/source.c
+++ b/gdb/source.c
@@ -594,7 +594,8 @@ add_path (const char *dirname, char **which_path, int parse_separators)
 	      print_sys_errmsg (name, save_errno);
 	    }
 	  else if ((st.st_mode & S_IFMT) != S_IFDIR)
-	    warning (_("%s is not a directory."), name);
+	    warning (_("%ps is not a directory."),
+		     styled_string (file_name_style.style (), name));
 	}
 
     append:
diff --git a/gdb/symfile.c b/gdb/symfile.c
index 76b5e1b8fe7..19cf9c911f9 100644
--- a/gdb/symfile.c
+++ b/gdb/symfile.c
@@ -2482,8 +2482,9 @@ reread_symbols (int from_tty)
       if (res != 0)
 	{
 	  /* FIXME, should use print_sys_errmsg but it's not filtered.  */
-	  gdb_printf (_("`%s' has disappeared; keeping its symbols.\n"),
-		      objfile_name (objfile));
+	  gdb_printf (_("`%ps' has disappeared; keeping its symbols.\n"),
+		      styled_string (file_name_style.style (),
+				     objfile_name (objfile)));
 	  continue;
 	}
       new_modtime = new_statbuf.st_mtime;
-- 
2.25.4

[PATCH 2/5] gdb: use archive name in warning when appropriate

[Andrew Burgess]

While working on some other patch I noticed that in reread_symbols
there is a diagnostic message that can be printed, and in some cases
we might use the wrong filename in the message.

The code in question is checking to see if an objfile has changed on
disk, we do this by stat-ing the on disk file and checking the mtime.
If this file has been removed from disk then we print a message that
the file has been removed, however, if the objfile is within an
archive then we stat the archive itself, but then warn that the
component within the archive has disappeared.  I think it makes more
sense to say that the archive has disappeared.

The last related commit is this one:

  commit 02aeec7bde8ec8a04d14a5637e75f1c6ab899e23
  Date:   Tue Apr 27 21:01:30 2010 +0000

      Check library name rather than member name when rereading symbols.

Though this just makes the code to stat the archive unconditional, the
code in question existed before this commit.

However, the above commit doesn't include any tests, and seems to
indicate that the problem being addressed was seen on Darwin.  I'm not
sure how to setup a test where GDB is using an objfile from within an
archive, and so there's no tests for this commit...

... but if someone can let me know how I can setup a suitable test,
please let me know and I'll try to get something working.
---
 gdb/symfile.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/gdb/symfile.c b/gdb/symfile.c
index 19cf9c911f9..145621f3c67 100644
--- a/gdb/symfile.c
+++ b/gdb/symfile.c
@@ -2459,7 +2459,6 @@ reread_symbols (int from_tty)
 {
   long new_modtime;
   struct stat new_statbuf;
-  int res;
   std::vector<struct objfile *> new_objfiles;
 
   for (objfile *objfile : current_program_space->objfiles ())
@@ -2475,16 +2474,18 @@ reread_symbols (int from_tty)
 	 `ar', often called a `static library' on most systems, though
 	 a `shared library' on AIX is also an archive), then you should
 	 stat on the archive name, not member name.  */
+      const char *filename;
       if (objfile->obfd->my_archive)
-	res = stat (bfd_get_filename (objfile->obfd->my_archive), &new_statbuf);
+	filename = bfd_get_filename (objfile->obfd->my_archive);
       else
-	res = stat (objfile_name (objfile), &new_statbuf);
+	filename = objfile_name (objfile);
+
+      int res = stat (filename, &new_statbuf);
       if (res != 0)
 	{
 	  /* FIXME, should use print_sys_errmsg but it's not filtered.  */
 	  gdb_printf (_("`%ps' has disappeared; keeping its symbols.\n"),
-		      styled_string (file_name_style.style (),
-				     objfile_name (objfile)));
+		      styled_string (file_name_style.style (), filename));
 	  continue;
 	}
       new_modtime = new_statbuf.st_mtime;
-- 
2.25.4

[PATCH 3/5] gdb: remove use of a static buffer for building error strings

[Andrew Burgess]

I noticed in procfs.c that we use a static buffer for building error
strings when we could easily use std::string and string_printf to
achieve the same result, this commit does that.

I ran into this while performing a further refactor/cleanup that will
be presented in a later patch in this series, and thought this was
worth splitting out into its own patch.

As far as I can tell, only Solaris uses procfs.c, so I did a test
build on a Solaris machine, and I don't believe that I've broken
anything.

There should be no user visible changes after this commit.
---
 gdb/procfs.c | 27 ++++++++++++++-------------
 1 file changed, 14 insertions(+), 13 deletions(-)

diff --git a/gdb/procfs.c b/gdb/procfs.c
index 9443b074483..706ccf0965c 100644
--- a/gdb/procfs.c
+++ b/gdb/procfs.c
@@ -255,8 +255,6 @@ typedef struct procinfo {
   int threads_valid: 1;
 } procinfo;
 
-static char errmsg[128];	/* shared error msg buffer */
-
 /* Function prototypes for procinfo module: */
 
 static procinfo *find_procinfo_or_die (int pid, int tid);
@@ -595,17 +593,19 @@ static void proc_resume (procinfo *pi, ptid_t scope_ptid,
 static void
 proc_warn (procinfo *pi, const char *func, int line)
 {
-  xsnprintf (errmsg, sizeof (errmsg), "procfs: %s line %d, %s",
-	     func, line, pi->pathname);
-  print_sys_errmsg (errmsg, errno);
+  int saved_errno = errno;
+  std::string errmsg
+    = string_printf ("procfs: %s line %d, %s", func, line, pi->pathname);
+  print_sys_errmsg (errmsg.c_str (), saved_errno);
 }
 
 static void
 proc_error (procinfo *pi, const char *func, int line)
 {
-  xsnprintf (errmsg, sizeof (errmsg), "procfs: %s line %d, %s",
-	     func, line, pi->pathname);
-  perror_with_name (errmsg);
+  int saved_errno = errno;
+  std::string errmsg
+    = string_printf ("procfs: %s line %d, %s", func, line, pi->pathname);
+  perror_with_name (errmsg.c_str (), saved_errno);
 }
 
 /* Updates the status struct in the procinfo.  There is a 'valid'
@@ -1805,11 +1805,12 @@ do_attach (ptid_t ptid)
 
   if (!open_procinfo_files (pi, FD_CTL))
     {
-      gdb_printf (gdb_stderr, "procfs:%d -- ", __LINE__);
-      xsnprintf (errmsg, sizeof (errmsg),
-		 "do_attach: couldn't open /proc file for process %d",
-		 ptid.pid ());
-      dead_procinfo (pi, errmsg, NOKILL);
+      int saved_errno = errno;
+      std::string errmsg
+	= string_printf ("procfs:%d -- do_attach: couldn't open /proc "
+			 "file for process %d", __LINE__, ptid.pid ());
+      errno = saved_errno;
+      dead_procinfo (pi, errmsg.c_str (), NOKILL);
     }
 
   /* Stop the process (if it isn't already stopped).  */
-- 
2.25.4

[PATCH 4/5] gdb: remove print_sys_errmsg

[Andrew Burgess]

This started with me running into this comment in symfile.c:

  /* FIXME, should use print_sys_errmsg but it's not filtered.  */
  gdb_printf (_("`%ps' has disappeared; keeping its symbols.\n"),
              styled_string (file_name_style.style (), filename));

In this particular case I think I disagree with the comment; I think
the output should be a warning rather than just a message printed to
gdb_stdout, I think when the executable, or some other objfile that is
currently being debugged, disappears from disk, this is likely an
unexpected situation, and worth warning the user about.

So, in theory, I could just call print_sys_errmsg and remove the
comment, but that would mean loosing the filename styling in the
output... so in the end I remove the comment and updated the code to
call warning.

But that got me looking at print_sys_errmsg and how it's used.

Currently the function takes a string and an errno, and prints, to
stderr, the string followed by the result of calling strerror on the
errno.

In some places the string passed to print_sys_errmsg is just a
filename, and this is used when something goes wrong.  In these cases,
I think calling warning rather than gdb_printf to gdb_stderr, would be
better, and in fact, in a couple of places we manually print a
"warning" prefix, and then call print_sys_errmsg.  And so, for these
users I have added a new function warning_filename_and_errno, which
takes a filename, which is printed with styling, and an errno, which
is passed through strerror and the resulting string printed.  This new
function calls warning to print its output.  I then updated some of
the print_sys_errmsg users to use this new function.

Some other users of print_sys_errmsg are also emitting what is clearly
a warning, however, the string being passed in is more than just a
filename, so the new warning_filename_and_errno function can't be
used, it would style the whole string.  For these users I have
switched to calling warning directly, this allows me to style the
warning message correctly.

Finally, in inflow.c there is one last call to print_sys_errmsg, in
this case I just inlined the definition of print_sys_errmsg.  This is
a really weird case, as after printing this message GDB just does a
hard exit.  This is pretty old code, dating back to the initial GDB
import, I guess it should be updated to call error() maybe, but I'm
reluctant to make this change as part of this commit, just in case
there's some reason why we can't throw an error at this point.

With that done there are now no users of print_sys_errmsg, and so the
old function can be removed.

While I was doing all of the above I added some additional filename
styling in soure.c, this is in an else block where the if contained
the print_sys_errmsg call, so these felt related.

And finally, while I was updating the uses of print_sys_errmsg in
procfs.c, I noticed that we used a static errmsg buffer to format some
error strings.  As the above changes got rid of one of the users of
errmsg I also removed the other two users, and the static buffer.
---
 gdb/inflow.c      |  3 ++-
 gdb/main.c        |  7 +------
 gdb/procfs.c      | 17 ++++++++++-------
 gdb/source.c      | 15 ++++-----------
 gdb/symfile.c     |  5 ++---
 gdb/utils.c       |  9 ++++-----
 gdb/utils.h       |  8 +++++++-
 gdb/windows-nat.c |  2 +-
 8 files changed, 31 insertions(+), 35 deletions(-)

diff --git a/gdb/inflow.c b/gdb/inflow.c
index 767cfd02c48..095c5f03672 100644
--- a/gdb/inflow.c
+++ b/gdb/inflow.c
@@ -766,7 +766,8 @@ check_syscall (const char *msg, int result)
 {
   if (result < 0)
     {
-      print_sys_errmsg (msg, errno);
+      gdb_printf (gdb_stderr, "%s:%s.\n", msg,
+		  safe_strerror (errno));
       _exit (1);
     }
 }
diff --git a/gdb/main.c b/gdb/main.c
index cf46f6acb20..1ca0fdeee1b 100644
--- a/gdb/main.c
+++ b/gdb/main.c
@@ -115,12 +115,7 @@ set_gdb_data_directory (const char *new_datadir)
   struct stat st;
 
   if (stat (new_datadir, &st) < 0)
-    {
-      int save_errno = errno;
-
-      gdb_printf (gdb_stderr, "Warning: ");
-      print_sys_errmsg (new_datadir, save_errno);
-    }
+    warning_filename_and_errno (new_datadir, errno);
   else if (!S_ISDIR (st.st_mode))
     warning (_("%ps is not a directory."),
 	     styled_string (file_name_style.style (), new_datadir));
diff --git a/gdb/procfs.c b/gdb/procfs.c
index 706ccf0965c..48e9f3dd4b5 100644
--- a/gdb/procfs.c
+++ b/gdb/procfs.c
@@ -46,6 +46,7 @@
 #include "gdbsupport/scoped_fd.h"
 #include "gdbsupport/pathstuff.h"
 #include "gdbsupport/buildargv.h"
+#include "cli/cli-style.h"
 
 /* This module provides the interface between GDB and the
    /proc file system, which is used on many versions of Unix
@@ -558,7 +559,7 @@ enum { NOKILL, KILL };
 static void
 dead_procinfo (procinfo *pi, const char *msg, int kill_p)
 {
-  print_sys_errmsg (pi->pathname, errno);
+  warning_filename_and_errno (pi->pathname, errno);
   if (kill_p == KILL)
     kill (pi->pid, SIGKILL);
 
@@ -594,18 +595,20 @@ static void
 proc_warn (procinfo *pi, const char *func, int line)
 {
   int saved_errno = errno;
-  std::string errmsg
-    = string_printf ("procfs: %s line %d, %s", func, line, pi->pathname);
-  print_sys_errmsg (errmsg.c_str (), saved_errno);
+  warning ("procfs: %s line %d, %ps: %s",
+	   func, line, styled_string (file_name_style.style (),
+				      pi->pathname),
+	   safe_strerror (saved_errno));
 }
 
 static void
 proc_error (procinfo *pi, const char *func, int line)
 {
   int saved_errno = errno;
-  std::string errmsg
-    = string_printf ("procfs: %s line %d, %s", func, line, pi->pathname);
-  perror_with_name (errmsg.c_str (), saved_errno);
+  error ("procfs: %s line %d, %ps: %s",
+	 func, line, styled_string (file_name_style.style (),
+				    pi->pathname),
+	 safe_strerror (saved_errno));
 }
 
 /* Updates the status struct in the procinfo.  There is a 'valid'
diff --git a/gdb/source.c b/gdb/source.c
index 5bdd729be8b..f648adc4520 100644
--- a/gdb/source.c
+++ b/gdb/source.c
@@ -587,12 +587,7 @@ add_path (const char *dirname, char **which_path, int parse_separators)
 	     a directory/etc, then having them in the path should be
 	     harmless.  */
 	  if (stat (name, &st) < 0)
-	    {
-	      int save_errno = errno;
-
-	      gdb_printf (gdb_stderr, "Warning: ");
-	      print_sys_errmsg (name, save_errno);
-	    }
+	    warning_filename_and_errno (name, errno);
 	  else if ((st.st_mode & S_IFMT) != S_IFDIR)
 	    warning (_("%ps is not a directory."),
 		     styled_string (file_name_style.style (), name));
@@ -1341,11 +1336,9 @@ print_source_lines_base (struct symtab *s, int line, int stopline,
       if (!(flags & PRINT_SOURCE_LINES_NOERROR))
 	{
 	  const char *filename = symtab_to_filename_for_display (s);
-	  int len = strlen (filename) + 100;
-	  char *name = (char *) alloca (len);
-
-	  xsnprintf (name, len, "%d\t%s", line, filename);
-	  print_sys_errmsg (name, errcode);
+	  warning (_("%d\t%ps: %s"), line,
+		   styled_string (file_name_style.style (), filename),
+		   safe_strerror (errcode));
 	}
       else
 	{
diff --git a/gdb/symfile.c b/gdb/symfile.c
index 145621f3c67..10074e281bd 100644
--- a/gdb/symfile.c
+++ b/gdb/symfile.c
@@ -2483,9 +2483,8 @@ reread_symbols (int from_tty)
       int res = stat (filename, &new_statbuf);
       if (res != 0)
 	{
-	  /* FIXME, should use print_sys_errmsg but it's not filtered.  */
-	  gdb_printf (_("`%ps' has disappeared; keeping its symbols.\n"),
-		      styled_string (file_name_style.style (), filename));
+	  warning (_("`%ps' has disappeared; keeping its symbols."),
+		   styled_string (file_name_style.style (), filename));
 	  continue;
 	}
       new_modtime = new_statbuf.st_mtime;
diff --git a/gdb/utils.c b/gdb/utils.c
index 2f545337cd4..a191d26a007 100644
--- a/gdb/utils.c
+++ b/gdb/utils.c
@@ -619,14 +619,13 @@ perror_warning_with_name (const char *string)
   warning (_("%s"), combined.c_str ());
 }
 
-/* Print the system error message for ERRCODE, and also mention STRING
-   as the file name for which the error was encountered.  */
+/* See utils.h.  */
 
 void
-print_sys_errmsg (const char *string, int errcode)
+warning_filename_and_errno (const char *filename, int saved_errno)
 {
-  const char *err = safe_strerror (errcode);
-  gdb_printf (gdb_stderr, "%s: %s.\n", string, err);
+  warning (_("%ps: %s"), styled_string (file_name_style.style (), filename),
+	   safe_strerror (saved_errno));
 }
 
 /* Control C eventually causes this to be called, at a convenient time.  */
diff --git a/gdb/utils.h b/gdb/utils.h
index c5364fa4b35..f646b300530 100644
--- a/gdb/utils.h
+++ b/gdb/utils.h
@@ -275,7 +275,13 @@ extern void fprintf_symbol (struct ui_file *, const char *,
 
 extern void perror_warning_with_name (const char *string);
 
-extern void print_sys_errmsg (const char *, int);
+/* Issue a warning formatted as '<filename>: <explanation>', where
+   <filename> is FILENAME with filename styling applied.  As such, don't
+   pass anything more than a filename in this string.  The <explanation>
+   is a string returned from calling safe_strerror(SAVED_ERRNO).  */
+
+extern void warning_filename_and_errno (const char *filename,
+					int saved_errno);
 \f
 /* Warnings and error messages.  */
 
diff --git a/gdb/windows-nat.c b/gdb/windows-nat.c
index 5a897dbfe76..7a139c8d36f 100644
--- a/gdb/windows-nat.c
+++ b/gdb/windows-nat.c
@@ -2625,7 +2625,7 @@ windows_nat_target::create_inferior (const char *exec_file,
       tty = open (inferior_tty.c_str (), O_RDWR | O_NOCTTY);
       if (tty < 0)
 	{
-	  print_sys_errmsg (inferior_tty.c_str (), errno);
+	  warning_filename_and_errno (inferior_tty.c_str (), errno);
 	  ostdin = ostdout = ostderr = -1;
 	}
       else
-- 
2.25.4

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

[Andrew Burgess]

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

Re: [PATCH 4/5] gdb: remove print_sys_errmsg

[Andrew Burgess]

The posted version of this patch didn't include some test fixes -- I
pulled this series out of another series I had locally, and I'd
committed the test fixes into a patch that never made it into this
version by mistake.

Here's an updated patch that contains the two test fixes to take account
of the output changes from this patch.

Thanks,
Andrew

---

commit bcc3a21e682e3c109b120db0ef020b48d1933e62
Author: Andrew Burgess <aburgess@redhat.com>
Date:   Sun Sep 24 12:37:40 2023 +0100

    gdb: remove print_sys_errmsg
    
    This started with me running into this comment in symfile.c:
    
      /* FIXME, should use print_sys_errmsg but it's not filtered.  */
      gdb_printf (_("`%ps' has disappeared; keeping its symbols.\n"),
                  styled_string (file_name_style.style (), filename));
    
    In this particular case I think I disagree with the comment; I think
    the output should be a warning rather than just a message printed to
    gdb_stdout, I think when the executable, or some other objfile that is
    currently being debugged, disappears from disk, this is likely an
    unexpected situation, and worth warning the user about.
    
    So, in theory, I could just call print_sys_errmsg and remove the
    comment, but that would mean loosing the filename styling in the
    output... so in the end I remove the comment and updated the code to
    call warning.
    
    But that got me looking at print_sys_errmsg and how it's used.
    
    Currently the function takes a string and an errno, and prints, to
    stderr, the string followed by the result of calling strerror on the
    errno.
    
    In some places the string passed to print_sys_errmsg is just a
    filename, and this is used when something goes wrong.  In these cases,
    I think calling warning rather than gdb_printf to gdb_stderr, would be
    better, and in fact, in a couple of places we manually print a
    "warning" prefix, and then call print_sys_errmsg.  And so, for these
    users I have added a new function warning_filename_and_errno, which
    takes a filename, which is printed with styling, and an errno, which
    is passed through strerror and the resulting string printed.  This new
    function calls warning to print its output.  I then updated some of
    the print_sys_errmsg users to use this new function.
    
    Some other users of print_sys_errmsg are also emitting what is clearly
    a warning, however, the string being passed in is more than just a
    filename, so the new warning_filename_and_errno function can't be
    used, it would style the whole string.  For these users I have
    switched to calling warning directly, this allows me to style the
    warning message correctly.
    
    Finally, in inflow.c there is one last call to print_sys_errmsg, in
    this case I just inlined the definition of print_sys_errmsg.  This is
    a really weird case, as after printing this message GDB just does a
    hard exit.  This is pretty old code, dating back to the initial GDB
    import, I guess it should be updated to call error() maybe, but I'm
    reluctant to make this change as part of this commit, just in case
    there's some reason why we can't throw an error at this point.
    
    With that done there are now no users of print_sys_errmsg, and so the
    old function can be removed.
    
    While I was doing all of the above I added some additional filename
    styling in soure.c, this is in an else block where the if contained
    the print_sys_errmsg call, so these felt related.
    
    And finally, while I was updating the uses of print_sys_errmsg in
    procfs.c, I noticed that we used a static errmsg buffer to format some
    error strings.  As the above changes got rid of one of the users of
    errmsg I also removed the other two users, and the static buffer.
    
    There were a couple of tests that depended on the existing output
    message format that needed updating.  In one case we gained an extra
    'warning: ' prefix, and in the other 'Warning: ' becomes 'warning: ',
    I think in both cases the new output is an improvement.

diff --git a/gdb/inflow.c b/gdb/inflow.c
index 767cfd02c48..095c5f03672 100644
--- a/gdb/inflow.c
+++ b/gdb/inflow.c
@@ -766,7 +766,8 @@ check_syscall (const char *msg, int result)
 {
   if (result < 0)
     {
-      print_sys_errmsg (msg, errno);
+      gdb_printf (gdb_stderr, "%s:%s.\n", msg,
+		  safe_strerror (errno));
       _exit (1);
     }
 }
diff --git a/gdb/main.c b/gdb/main.c
index cf46f6acb20..1ca0fdeee1b 100644
--- a/gdb/main.c
+++ b/gdb/main.c
@@ -115,12 +115,7 @@ set_gdb_data_directory (const char *new_datadir)
   struct stat st;
 
   if (stat (new_datadir, &st) < 0)
-    {
-      int save_errno = errno;
-
-      gdb_printf (gdb_stderr, "Warning: ");
-      print_sys_errmsg (new_datadir, save_errno);
-    }
+    warning_filename_and_errno (new_datadir, errno);
   else if (!S_ISDIR (st.st_mode))
     warning (_("%ps is not a directory."),
 	     styled_string (file_name_style.style (), new_datadir));
diff --git a/gdb/procfs.c b/gdb/procfs.c
index 706ccf0965c..48e9f3dd4b5 100644
--- a/gdb/procfs.c
+++ b/gdb/procfs.c
@@ -46,6 +46,7 @@
 #include "gdbsupport/scoped_fd.h"
 #include "gdbsupport/pathstuff.h"
 #include "gdbsupport/buildargv.h"
+#include "cli/cli-style.h"
 
 /* This module provides the interface between GDB and the
    /proc file system, which is used on many versions of Unix
@@ -558,7 +559,7 @@ enum { NOKILL, KILL };
 static void
 dead_procinfo (procinfo *pi, const char *msg, int kill_p)
 {
-  print_sys_errmsg (pi->pathname, errno);
+  warning_filename_and_errno (pi->pathname, errno);
   if (kill_p == KILL)
     kill (pi->pid, SIGKILL);
 
@@ -594,18 +595,20 @@ static void
 proc_warn (procinfo *pi, const char *func, int line)
 {
   int saved_errno = errno;
-  std::string errmsg
-    = string_printf ("procfs: %s line %d, %s", func, line, pi->pathname);
-  print_sys_errmsg (errmsg.c_str (), saved_errno);
+  warning ("procfs: %s line %d, %ps: %s",
+	   func, line, styled_string (file_name_style.style (),
+				      pi->pathname),
+	   safe_strerror (saved_errno));
 }
 
 static void
 proc_error (procinfo *pi, const char *func, int line)
 {
   int saved_errno = errno;
-  std::string errmsg
-    = string_printf ("procfs: %s line %d, %s", func, line, pi->pathname);
-  perror_with_name (errmsg.c_str (), saved_errno);
+  error ("procfs: %s line %d, %ps: %s",
+	 func, line, styled_string (file_name_style.style (),
+				    pi->pathname),
+	 safe_strerror (saved_errno));
 }
 
 /* Updates the status struct in the procinfo.  There is a 'valid'
diff --git a/gdb/source.c b/gdb/source.c
index 5bdd729be8b..f648adc4520 100644
--- a/gdb/source.c
+++ b/gdb/source.c
@@ -587,12 +587,7 @@ add_path (const char *dirname, char **which_path, int parse_separators)
 	     a directory/etc, then having them in the path should be
 	     harmless.  */
 	  if (stat (name, &st) < 0)
-	    {
-	      int save_errno = errno;
-
-	      gdb_printf (gdb_stderr, "Warning: ");
-	      print_sys_errmsg (name, save_errno);
-	    }
+	    warning_filename_and_errno (name, errno);
 	  else if ((st.st_mode & S_IFMT) != S_IFDIR)
 	    warning (_("%ps is not a directory."),
 		     styled_string (file_name_style.style (), name));
@@ -1341,11 +1336,9 @@ print_source_lines_base (struct symtab *s, int line, int stopline,
       if (!(flags & PRINT_SOURCE_LINES_NOERROR))
 	{
 	  const char *filename = symtab_to_filename_for_display (s);
-	  int len = strlen (filename) + 100;
-	  char *name = (char *) alloca (len);
-
-	  xsnprintf (name, len, "%d\t%s", line, filename);
-	  print_sys_errmsg (name, errcode);
+	  warning (_("%d\t%ps: %s"), line,
+		   styled_string (file_name_style.style (), filename),
+		   safe_strerror (errcode));
 	}
       else
 	{
diff --git a/gdb/symfile.c b/gdb/symfile.c
index 145621f3c67..10074e281bd 100644
--- a/gdb/symfile.c
+++ b/gdb/symfile.c
@@ -2483,9 +2483,8 @@ reread_symbols (int from_tty)
       int res = stat (filename, &new_statbuf);
       if (res != 0)
 	{
-	  /* FIXME, should use print_sys_errmsg but it's not filtered.  */
-	  gdb_printf (_("`%ps' has disappeared; keeping its symbols.\n"),
-		      styled_string (file_name_style.style (), filename));
+	  warning (_("`%ps' has disappeared; keeping its symbols."),
+		   styled_string (file_name_style.style (), filename));
 	  continue;
 	}
       new_modtime = new_statbuf.st_mtime;
diff --git a/gdb/testsuite/gdb.base/catch-syscall.exp b/gdb/testsuite/gdb.base/catch-syscall.exp
index 4a9302fb9b7..0588cb35d87 100644
--- a/gdb/testsuite/gdb.base/catch-syscall.exp
+++ b/gdb/testsuite/gdb.base/catch-syscall.exp
@@ -392,7 +392,7 @@ proc test_catch_syscall_fail_nodatadir {} {
 	# Make sure GDB doesn't load the syscalls xml from the system
 	# data directory.
 	gdb_test "set data-directory /the/path/to/nowhere" \
-	    "Warning: /the/path/to/nowhere: .*"
+	    "warning: /the/path/to/nowhere: .*"
 
 	# Testing to see if we receive a warning when calling "catch
 	# syscall" without XML support (without datadir).
@@ -658,7 +658,7 @@ proc do_syscall_tests_without_xml {} {
     # Make sure GDB doesn't load the syscalls xml from the system data
     # directory.
     gdb_test "set data-directory /the/path/to/nowhere" \
-	"Warning: /the/path/to/nowhere: .*"
+	"warning: /the/path/to/nowhere: .*"
 
     # Let's test if we can catch syscalls without XML support.
     # We should succeed, but GDB is not supposed to print syscall names.
diff --git a/gdb/testsuite/gdb.dwarf2/imported-unit.exp b/gdb/testsuite/gdb.dwarf2/imported-unit.exp
index 7e28931be22..07aa6afbee7 100644
--- a/gdb/testsuite/gdb.dwarf2/imported-unit.exp
+++ b/gdb/testsuite/gdb.dwarf2/imported-unit.exp
@@ -167,7 +167,7 @@ if { $psymtabs_p } {
 }
 
 gdb_test "l imported_unit.c:1" \
-    "1\timported_unit.c: No such file or directory\."
+    "warning: 1\timported_unit.c: No such file or directory"
 
 gdb_test "info source" "\r\nCurrent source file is imported_unit.c\r\n.*" \
     "info source for imported_unit.c"
diff --git a/gdb/utils.c b/gdb/utils.c
index 2f545337cd4..a191d26a007 100644
--- a/gdb/utils.c
+++ b/gdb/utils.c
@@ -619,14 +619,13 @@ perror_warning_with_name (const char *string)
   warning (_("%s"), combined.c_str ());
 }
 
-/* Print the system error message for ERRCODE, and also mention STRING
-   as the file name for which the error was encountered.  */
+/* See utils.h.  */
 
 void
-print_sys_errmsg (const char *string, int errcode)
+warning_filename_and_errno (const char *filename, int saved_errno)
 {
-  const char *err = safe_strerror (errcode);
-  gdb_printf (gdb_stderr, "%s: %s.\n", string, err);
+  warning (_("%ps: %s"), styled_string (file_name_style.style (), filename),
+	   safe_strerror (saved_errno));
 }
 
 /* Control C eventually causes this to be called, at a convenient time.  */
diff --git a/gdb/utils.h b/gdb/utils.h
index c5364fa4b35..f646b300530 100644
--- a/gdb/utils.h
+++ b/gdb/utils.h
@@ -275,7 +275,13 @@ extern void fprintf_symbol (struct ui_file *, const char *,
 
 extern void perror_warning_with_name (const char *string);
 
-extern void print_sys_errmsg (const char *, int);
+/* Issue a warning formatted as '<filename>: <explanation>', where
+   <filename> is FILENAME with filename styling applied.  As such, don't
+   pass anything more than a filename in this string.  The <explanation>
+   is a string returned from calling safe_strerror(SAVED_ERRNO).  */
+
+extern void warning_filename_and_errno (const char *filename,
+					int saved_errno);
 \f
 /* Warnings and error messages.  */
 
diff --git a/gdb/windows-nat.c b/gdb/windows-nat.c
index 5a897dbfe76..7a139c8d36f 100644
--- a/gdb/windows-nat.c
+++ b/gdb/windows-nat.c
@@ -2625,7 +2625,7 @@ windows_nat_target::create_inferior (const char *exec_file,
       tty = open (inferior_tty.c_str (), O_RDWR | O_NOCTTY);
       if (tty < 0)
 	{
-	  print_sys_errmsg (inferior_tty.c_str (), errno);
+	  warning_filename_and_errno (inferior_tty.c_str (), errno);
 	  ostdin = ostdout = ostderr = -1;
 	}
       else

[PATCHv2 0/5] Fix using an exec file with target: prefix

[Andrew Burgess]

In V2:

  - Fixed a test regression in patch #4,

  - Patch #5 has been completely changed.  Tom Tromey pointed me to a
    previous series of his which (a) never got merged, and (b)
    included a much better solution to this problem.  I've pulled out
    just the part I need to resolve the issue in that patch.

---

Andrew Burgess (5):
  gdb: some additional filename styling
  gdb: use archive name in warning when appropriate
  gdb: remove use of a static buffer for building error strings
  gdb: remove print_sys_errmsg
  gdb: fix reread_symbols when an objfile has target: prefix

 gdb/inflow.c                                  |   3 +-
 gdb/main.c                                    |   7 +-
 gdb/procfs.c                                  |  32 ++--
 gdb/source.c                                  |  18 +-
 gdb/symfile.c                                 |  21 +--
 gdb/testsuite/gdb.base/catch-syscall.exp      |   4 +-
 gdb/testsuite/gdb.dwarf2/imported-unit.exp    |   2 +-
 gdb/testsuite/gdb.server/target-exec-file.c   |  22 +++
 gdb/testsuite/gdb.server/target-exec-file.exp | 174 ++++++++++++++++++
 gdb/utils.c                                   |   9 +-
 gdb/utils.h                                   |   8 +-
 gdb/windows-nat.c                             |   2 +-
 12 files changed, 247 insertions(+), 55 deletions(-)
 create mode 100644 gdb/testsuite/gdb.server/target-exec-file.c
 create mode 100644 gdb/testsuite/gdb.server/target-exec-file.exp


base-commit: f586e3409b752748bf213520c2dbb0b44e0005d8
-- 
2.25.4

[PATCHv2 1/5] gdb: some additional filename styling

[Andrew Burgess]

Fix up another couple of places where we can apply filename styling.
---
 gdb/source.c  | 3 ++-
 gdb/symfile.c | 5 +++--
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/gdb/source.c b/gdb/source.c
index 9c701e866a6..5bdd729be8b 100644
--- a/gdb/source.c
+++ b/gdb/source.c
@@ -594,7 +594,8 @@ add_path (const char *dirname, char **which_path, int parse_separators)
 	      print_sys_errmsg (name, save_errno);
 	    }
 	  else if ((st.st_mode & S_IFMT) != S_IFDIR)
-	    warning (_("%s is not a directory."), name);
+	    warning (_("%ps is not a directory."),
+		     styled_string (file_name_style.style (), name));
 	}
 
     append:
diff --git a/gdb/symfile.c b/gdb/symfile.c
index 76b5e1b8fe7..19cf9c911f9 100644
--- a/gdb/symfile.c
+++ b/gdb/symfile.c
@@ -2482,8 +2482,9 @@ reread_symbols (int from_tty)
       if (res != 0)
 	{
 	  /* FIXME, should use print_sys_errmsg but it's not filtered.  */
-	  gdb_printf (_("`%s' has disappeared; keeping its symbols.\n"),
-		      objfile_name (objfile));
+	  gdb_printf (_("`%ps' has disappeared; keeping its symbols.\n"),
+		      styled_string (file_name_style.style (),
+				     objfile_name (objfile)));
 	  continue;
 	}
       new_modtime = new_statbuf.st_mtime;
-- 
2.25.4

[PATCHv2 2/5] gdb: use archive name in warning when appropriate

[Andrew Burgess]

While working on some other patch I noticed that in reread_symbols
there is a diagnostic message that can be printed, and in some cases
we might use the wrong filename in the message.

The code in question is checking to see if an objfile has changed on
disk, we do this by stat-ing the on disk file and checking the mtime.
If this file has been removed from disk then we print a message that
the file has been removed, however, if the objfile is within an
archive then we stat the archive itself, but then warn that the
component within the archive has disappeared.  I think it makes more
sense to say that the archive has disappeared.

The last related commit is this one:

  commit 02aeec7bde8ec8a04d14a5637e75f1c6ab899e23
  Date:   Tue Apr 27 21:01:30 2010 +0000

      Check library name rather than member name when rereading symbols.

Though this just makes the code to stat the archive unconditional, the
code in question existed before this commit.

However, the above commit doesn't include any tests, and seems to
indicate that the problem being addressed was seen on Darwin.  I'm not
sure how to setup a test where GDB is using an objfile from within an
archive, and so there's no tests for this commit...

... but if someone can let me know how I can setup a suitable test,
please let me know and I'll try to get something working.
---
 gdb/symfile.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/gdb/symfile.c b/gdb/symfile.c
index 19cf9c911f9..145621f3c67 100644
--- a/gdb/symfile.c
+++ b/gdb/symfile.c
@@ -2459,7 +2459,6 @@ reread_symbols (int from_tty)
 {
   long new_modtime;
   struct stat new_statbuf;
-  int res;
   std::vector<struct objfile *> new_objfiles;
 
   for (objfile *objfile : current_program_space->objfiles ())
@@ -2475,16 +2474,18 @@ reread_symbols (int from_tty)
 	 `ar', often called a `static library' on most systems, though
 	 a `shared library' on AIX is also an archive), then you should
 	 stat on the archive name, not member name.  */
+      const char *filename;
       if (objfile->obfd->my_archive)
-	res = stat (bfd_get_filename (objfile->obfd->my_archive), &new_statbuf);
+	filename = bfd_get_filename (objfile->obfd->my_archive);
       else
-	res = stat (objfile_name (objfile), &new_statbuf);
+	filename = objfile_name (objfile);
+
+      int res = stat (filename, &new_statbuf);
       if (res != 0)
 	{
 	  /* FIXME, should use print_sys_errmsg but it's not filtered.  */
 	  gdb_printf (_("`%ps' has disappeared; keeping its symbols.\n"),
-		      styled_string (file_name_style.style (),
-				     objfile_name (objfile)));
+		      styled_string (file_name_style.style (), filename));
 	  continue;
 	}
       new_modtime = new_statbuf.st_mtime;
-- 
2.25.4

[PATCHv2 3/5] gdb: remove use of a static buffer for building error strings

[Andrew Burgess]

I noticed in procfs.c that we use a static buffer for building error
strings when we could easily use std::string and string_printf to
achieve the same result, this commit does that.

I ran into this while performing a further refactor/cleanup that will
be presented in a later patch in this series, and thought this was
worth splitting out into its own patch.

As far as I can tell, only Solaris uses procfs.c, so I did a test
build on a Solaris machine, and I don't believe that I've broken
anything.

There should be no user visible changes after this commit.
---
 gdb/procfs.c | 27 ++++++++++++++-------------
 1 file changed, 14 insertions(+), 13 deletions(-)

diff --git a/gdb/procfs.c b/gdb/procfs.c
index 9443b074483..706ccf0965c 100644
--- a/gdb/procfs.c
+++ b/gdb/procfs.c
@@ -255,8 +255,6 @@ typedef struct procinfo {
   int threads_valid: 1;
 } procinfo;
 
-static char errmsg[128];	/* shared error msg buffer */
-
 /* Function prototypes for procinfo module: */
 
 static procinfo *find_procinfo_or_die (int pid, int tid);
@@ -595,17 +593,19 @@ static void proc_resume (procinfo *pi, ptid_t scope_ptid,
 static void
 proc_warn (procinfo *pi, const char *func, int line)
 {
-  xsnprintf (errmsg, sizeof (errmsg), "procfs: %s line %d, %s",
-	     func, line, pi->pathname);
-  print_sys_errmsg (errmsg, errno);
+  int saved_errno = errno;
+  std::string errmsg
+    = string_printf ("procfs: %s line %d, %s", func, line, pi->pathname);
+  print_sys_errmsg (errmsg.c_str (), saved_errno);
 }
 
 static void
 proc_error (procinfo *pi, const char *func, int line)
 {
-  xsnprintf (errmsg, sizeof (errmsg), "procfs: %s line %d, %s",
-	     func, line, pi->pathname);
-  perror_with_name (errmsg);
+  int saved_errno = errno;
+  std::string errmsg
+    = string_printf ("procfs: %s line %d, %s", func, line, pi->pathname);
+  perror_with_name (errmsg.c_str (), saved_errno);
 }
 
 /* Updates the status struct in the procinfo.  There is a 'valid'
@@ -1805,11 +1805,12 @@ do_attach (ptid_t ptid)
 
   if (!open_procinfo_files (pi, FD_CTL))
     {
-      gdb_printf (gdb_stderr, "procfs:%d -- ", __LINE__);
-      xsnprintf (errmsg, sizeof (errmsg),
-		 "do_attach: couldn't open /proc file for process %d",
-		 ptid.pid ());
-      dead_procinfo (pi, errmsg, NOKILL);
+      int saved_errno = errno;
+      std::string errmsg
+	= string_printf ("procfs:%d -- do_attach: couldn't open /proc "
+			 "file for process %d", __LINE__, ptid.pid ());
+      errno = saved_errno;
+      dead_procinfo (pi, errmsg.c_str (), NOKILL);
     }
 
   /* Stop the process (if it isn't already stopped).  */
-- 
2.25.4

[PATCHv2 4/5] gdb: remove print_sys_errmsg

[Andrew Burgess]

This started with me running into this comment in symfile.c:

  /* FIXME, should use print_sys_errmsg but it's not filtered.  */
  gdb_printf (_("`%ps' has disappeared; keeping its symbols.\n"),
              styled_string (file_name_style.style (), filename));

In this particular case I think I disagree with the comment; I think
the output should be a warning rather than just a message printed to
gdb_stdout, I think when the executable, or some other objfile that is
currently being debugged, disappears from disk, this is likely an
unexpected situation, and worth warning the user about.

So, in theory, I could just call print_sys_errmsg and remove the
comment, but that would mean loosing the filename styling in the
output... so in the end I remove the comment and updated the code to
call warning.

But that got me looking at print_sys_errmsg and how it's used.

Currently the function takes a string and an errno, and prints, to
stderr, the string followed by the result of calling strerror on the
errno.

In some places the string passed to print_sys_errmsg is just a
filename, and this is used when something goes wrong.  In these cases,
I think calling warning rather than gdb_printf to gdb_stderr, would be
better, and in fact, in a couple of places we manually print a
"warning" prefix, and then call print_sys_errmsg.  And so, for these
users I have added a new function warning_filename_and_errno, which
takes a filename, which is printed with styling, and an errno, which
is passed through strerror and the resulting string printed.  This new
function calls warning to print its output.  I then updated some of
the print_sys_errmsg users to use this new function.

Some other users of print_sys_errmsg are also emitting what is clearly
a warning, however, the string being passed in is more than just a
filename, so the new warning_filename_and_errno function can't be
used, it would style the whole string.  For these users I have
switched to calling warning directly, this allows me to style the
warning message correctly.

Finally, in inflow.c there is one last call to print_sys_errmsg, in
this case I just inlined the definition of print_sys_errmsg.  This is
a really weird case, as after printing this message GDB just does a
hard exit.  This is pretty old code, dating back to the initial GDB
import, I guess it should be updated to call error() maybe, but I'm
reluctant to make this change as part of this commit, just in case
there's some reason why we can't throw an error at this point.

With that done there are now no users of print_sys_errmsg, and so the
old function can be removed.

While I was doing all of the above I added some additional filename
styling in soure.c, this is in an else block where the if contained
the print_sys_errmsg call, so these felt related.

And finally, while I was updating the uses of print_sys_errmsg in
procfs.c, I noticed that we used a static errmsg buffer to format some
error strings.  As the above changes got rid of one of the users of
errmsg I also removed the other two users, and the static buffer.

There were a couple of tests that depended on the existing output
message format that needed updating.  In one case we gained an extra
'warning: ' prefix, and in the other 'Warning: ' becomes 'warning: ',
I think in both cases the new output is an improvement.
---
 gdb/inflow.c                               |  3 ++-
 gdb/main.c                                 |  7 +------
 gdb/procfs.c                               | 17 ++++++++++-------
 gdb/source.c                               | 15 ++++-----------
 gdb/symfile.c                              |  5 ++---
 gdb/testsuite/gdb.base/catch-syscall.exp   |  4 ++--
 gdb/testsuite/gdb.dwarf2/imported-unit.exp |  2 +-
 gdb/utils.c                                |  9 ++++-----
 gdb/utils.h                                |  8 +++++++-
 gdb/windows-nat.c                          |  2 +-
 10 files changed, 34 insertions(+), 38 deletions(-)

diff --git a/gdb/inflow.c b/gdb/inflow.c
index 767cfd02c48..095c5f03672 100644
--- a/gdb/inflow.c
+++ b/gdb/inflow.c
@@ -766,7 +766,8 @@ check_syscall (const char *msg, int result)
 {
   if (result < 0)
     {
-      print_sys_errmsg (msg, errno);
+      gdb_printf (gdb_stderr, "%s:%s.\n", msg,
+		  safe_strerror (errno));
       _exit (1);
     }
 }
diff --git a/gdb/main.c b/gdb/main.c
index cf46f6acb20..1ca0fdeee1b 100644
--- a/gdb/main.c
+++ b/gdb/main.c
@@ -115,12 +115,7 @@ set_gdb_data_directory (const char *new_datadir)
   struct stat st;
 
   if (stat (new_datadir, &st) < 0)
-    {
-      int save_errno = errno;
-
-      gdb_printf (gdb_stderr, "Warning: ");
-      print_sys_errmsg (new_datadir, save_errno);
-    }
+    warning_filename_and_errno (new_datadir, errno);
   else if (!S_ISDIR (st.st_mode))
     warning (_("%ps is not a directory."),
 	     styled_string (file_name_style.style (), new_datadir));
diff --git a/gdb/procfs.c b/gdb/procfs.c
index 706ccf0965c..48e9f3dd4b5 100644
--- a/gdb/procfs.c
+++ b/gdb/procfs.c
@@ -46,6 +46,7 @@
 #include "gdbsupport/scoped_fd.h"
 #include "gdbsupport/pathstuff.h"
 #include "gdbsupport/buildargv.h"
+#include "cli/cli-style.h"
 
 /* This module provides the interface between GDB and the
    /proc file system, which is used on many versions of Unix
@@ -558,7 +559,7 @@ enum { NOKILL, KILL };
 static void
 dead_procinfo (procinfo *pi, const char *msg, int kill_p)
 {
-  print_sys_errmsg (pi->pathname, errno);
+  warning_filename_and_errno (pi->pathname, errno);
   if (kill_p == KILL)
     kill (pi->pid, SIGKILL);
 
@@ -594,18 +595,20 @@ static void
 proc_warn (procinfo *pi, const char *func, int line)
 {
   int saved_errno = errno;
-  std::string errmsg
-    = string_printf ("procfs: %s line %d, %s", func, line, pi->pathname);
-  print_sys_errmsg (errmsg.c_str (), saved_errno);
+  warning ("procfs: %s line %d, %ps: %s",
+	   func, line, styled_string (file_name_style.style (),
+				      pi->pathname),
+	   safe_strerror (saved_errno));
 }
 
 static void
 proc_error (procinfo *pi, const char *func, int line)
 {
   int saved_errno = errno;
-  std::string errmsg
-    = string_printf ("procfs: %s line %d, %s", func, line, pi->pathname);
-  perror_with_name (errmsg.c_str (), saved_errno);
+  error ("procfs: %s line %d, %ps: %s",
+	 func, line, styled_string (file_name_style.style (),
+				    pi->pathname),
+	 safe_strerror (saved_errno));
 }
 
 /* Updates the status struct in the procinfo.  There is a 'valid'
diff --git a/gdb/source.c b/gdb/source.c
index 5bdd729be8b..f648adc4520 100644
--- a/gdb/source.c
+++ b/gdb/source.c
@@ -587,12 +587,7 @@ add_path (const char *dirname, char **which_path, int parse_separators)
 	     a directory/etc, then having them in the path should be
 	     harmless.  */
 	  if (stat (name, &st) < 0)
-	    {
-	      int save_errno = errno;
-
-	      gdb_printf (gdb_stderr, "Warning: ");
-	      print_sys_errmsg (name, save_errno);
-	    }
+	    warning_filename_and_errno (name, errno);
 	  else if ((st.st_mode & S_IFMT) != S_IFDIR)
 	    warning (_("%ps is not a directory."),
 		     styled_string (file_name_style.style (), name));
@@ -1341,11 +1336,9 @@ print_source_lines_base (struct symtab *s, int line, int stopline,
       if (!(flags & PRINT_SOURCE_LINES_NOERROR))
 	{
 	  const char *filename = symtab_to_filename_for_display (s);
-	  int len = strlen (filename) + 100;
-	  char *name = (char *) alloca (len);
-
-	  xsnprintf (name, len, "%d\t%s", line, filename);
-	  print_sys_errmsg (name, errcode);
+	  warning (_("%d\t%ps: %s"), line,
+		   styled_string (file_name_style.style (), filename),
+		   safe_strerror (errcode));
 	}
       else
 	{
diff --git a/gdb/symfile.c b/gdb/symfile.c
index 145621f3c67..10074e281bd 100644
--- a/gdb/symfile.c
+++ b/gdb/symfile.c
@@ -2483,9 +2483,8 @@ reread_symbols (int from_tty)
       int res = stat (filename, &new_statbuf);
       if (res != 0)
 	{
-	  /* FIXME, should use print_sys_errmsg but it's not filtered.  */
-	  gdb_printf (_("`%ps' has disappeared; keeping its symbols.\n"),
-		      styled_string (file_name_style.style (), filename));
+	  warning (_("`%ps' has disappeared; keeping its symbols."),
+		   styled_string (file_name_style.style (), filename));
 	  continue;
 	}
       new_modtime = new_statbuf.st_mtime;
diff --git a/gdb/testsuite/gdb.base/catch-syscall.exp b/gdb/testsuite/gdb.base/catch-syscall.exp
index 4a9302fb9b7..0588cb35d87 100644
--- a/gdb/testsuite/gdb.base/catch-syscall.exp
+++ b/gdb/testsuite/gdb.base/catch-syscall.exp
@@ -392,7 +392,7 @@ proc test_catch_syscall_fail_nodatadir {} {
 	# Make sure GDB doesn't load the syscalls xml from the system
 	# data directory.
 	gdb_test "set data-directory /the/path/to/nowhere" \
-	    "Warning: /the/path/to/nowhere: .*"
+	    "warning: /the/path/to/nowhere: .*"
 
 	# Testing to see if we receive a warning when calling "catch
 	# syscall" without XML support (without datadir).
@@ -658,7 +658,7 @@ proc do_syscall_tests_without_xml {} {
     # Make sure GDB doesn't load the syscalls xml from the system data
     # directory.
     gdb_test "set data-directory /the/path/to/nowhere" \
-	"Warning: /the/path/to/nowhere: .*"
+	"warning: /the/path/to/nowhere: .*"
 
     # Let's test if we can catch syscalls without XML support.
     # We should succeed, but GDB is not supposed to print syscall names.
diff --git a/gdb/testsuite/gdb.dwarf2/imported-unit.exp b/gdb/testsuite/gdb.dwarf2/imported-unit.exp
index 7e28931be22..07aa6afbee7 100644
--- a/gdb/testsuite/gdb.dwarf2/imported-unit.exp
+++ b/gdb/testsuite/gdb.dwarf2/imported-unit.exp
@@ -167,7 +167,7 @@ if { $psymtabs_p } {
 }
 
 gdb_test "l imported_unit.c:1" \
-    "1\timported_unit.c: No such file or directory\."
+    "warning: 1\timported_unit.c: No such file or directory"
 
 gdb_test "info source" "\r\nCurrent source file is imported_unit.c\r\n.*" \
     "info source for imported_unit.c"
diff --git a/gdb/utils.c b/gdb/utils.c
index 2f545337cd4..a191d26a007 100644
--- a/gdb/utils.c
+++ b/gdb/utils.c
@@ -619,14 +619,13 @@ perror_warning_with_name (const char *string)
   warning (_("%s"), combined.c_str ());
 }
 
-/* Print the system error message for ERRCODE, and also mention STRING
-   as the file name for which the error was encountered.  */
+/* See utils.h.  */
 
 void
-print_sys_errmsg (const char *string, int errcode)
+warning_filename_and_errno (const char *filename, int saved_errno)
 {
-  const char *err = safe_strerror (errcode);
-  gdb_printf (gdb_stderr, "%s: %s.\n", string, err);
+  warning (_("%ps: %s"), styled_string (file_name_style.style (), filename),
+	   safe_strerror (saved_errno));
 }
 
 /* Control C eventually causes this to be called, at a convenient time.  */
diff --git a/gdb/utils.h b/gdb/utils.h
index c5364fa4b35..f646b300530 100644
--- a/gdb/utils.h
+++ b/gdb/utils.h
@@ -275,7 +275,13 @@ extern void fprintf_symbol (struct ui_file *, const char *,
 
 extern void perror_warning_with_name (const char *string);
 
-extern void print_sys_errmsg (const char *, int);
+/* Issue a warning formatted as '<filename>: <explanation>', where
+   <filename> is FILENAME with filename styling applied.  As such, don't
+   pass anything more than a filename in this string.  The <explanation>
+   is a string returned from calling safe_strerror(SAVED_ERRNO).  */
+
+extern void warning_filename_and_errno (const char *filename,
+					int saved_errno);
 \f
 /* Warnings and error messages.  */
 
diff --git a/gdb/windows-nat.c b/gdb/windows-nat.c
index 5a897dbfe76..7a139c8d36f 100644
--- a/gdb/windows-nat.c
+++ b/gdb/windows-nat.c
@@ -2625,7 +2625,7 @@ windows_nat_target::create_inferior (const char *exec_file,
       tty = open (inferior_tty.c_str (), O_RDWR | O_NOCTTY);
       if (tty < 0)
 	{
-	  print_sys_errmsg (inferior_tty.c_str (), errno);
+	  warning_filename_and_errno (inferior_tty.c_str (), errno);
 	  ostdin = ostdout = ostderr = -1;
 	}
       else
-- 
2.25.4

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

[Andrew Burgess]

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 update reread_symbols to use bfd_stat instead of
a direct stat call, this adds support for target: files, and fixes the
problem.

This change was proposed before in this commit:

  https://inbox.sourceware.org/gdb-patches/20200114210956.25115-3-tromey@adacore.com/

However, that patch never got merged, and seemed to get stuck
discussing issues around gnulib stat vs system stat as used by BFD.

I didn't 100% understand the issues discussed in that thread, however,
I think the problem with the previous thread related to the changes in
gdb_bfd.c, rather than to the change in symfile.c.  As such, I think
this change might be acceptable, my reasoning is:

  - the objfile::mtime field is set by a call to bfd_get_mtime (see
    objfiles.c), which calls bfd_stat under the hood.  This will end
    up using the system stat,

  - In symfile.c we currently call stat directly, which will call the
    gnulib stat, which, if I understand the above thread correctly,
    might give a different result to the system stat in some cases,

  - By switching to using bfd_stat in symfile.c we should now be
    consistently calling the system stat,

Co-Authored-By: Tom Tromey <tromey@adacore.com>
---
 gdb/symfile.c                                 |  18 +-
 gdb/testsuite/gdb.server/target-exec-file.c   |  22 +++
 gdb/testsuite/gdb.server/target-exec-file.exp | 174 ++++++++++++++++++
 3 files changed, 203 insertions(+), 11 deletions(-)
 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..87f8e0a3ea6 100644
--- a/gdb/symfile.c
+++ b/gdb/symfile.c
@@ -2470,19 +2470,15 @@ reread_symbols (int from_tty)
       if (objfile->separate_debug_objfile_backlink)
 	continue;
 
-      /* If this object is from an archive (what you usually create with
-	 `ar', often called a `static library' on most systems, though
-	 a `shared library' on AIX is also an archive), then you should
-	 stat on the archive name, not member name.  */
-      const char *filename;
-      if (objfile->obfd->my_archive)
-	filename = bfd_get_filename (objfile->obfd->my_archive);
-      else
-	filename = objfile_name (objfile);
-
-      int res = stat (filename, &new_statbuf);
+      int res = bfd_stat (objfile->obfd.get (), &new_statbuf);
       if (res != 0)
 	{
+	  const char *filename;
+	  if (objfile->obfd->my_archive)
+	    filename = bfd_get_filename (objfile->obfd->my_archive);
+	  else
+	    filename = objfile_name (objfile);
+
 	  warning (_("`%ps' has disappeared; keeping its symbols."),
 		   styled_string (file_name_style.style (), filename));
 	  continue;
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

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

[Tom Tromey]

Andrew> I didn't 100% understand the issues discussed in that thread, however,
Andrew> I think the problem with the previous thread related to the changes in
Andrew> gdb_bfd.c, rather than to the change in symfile.c.  As such, I think
Andrew> this change might be acceptable, my reasoning is:

Andrew>   - the objfile::mtime field is set by a call to bfd_get_mtime (see
Andrew>     objfiles.c), which calls bfd_stat under the hood.  This will end
Andrew>     up using the system stat,

Andrew>   - In symfile.c we currently call stat directly, which will call the
Andrew>     gnulib stat, which, if I understand the above thread correctly,
Andrew>     might give a different result to the system stat in some cases,

Andrew>   - By switching to using bfd_stat in symfile.c we should now be
Andrew>     consistently calling the system stat,

The BFD cache (in gdb_bfd.c) uses fstat, so I think there would still be
a problem here.

Tom

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

[Andrew Burgess]

Tom Tromey <tromey@adacore.com> writes:

> Andrew> I didn't 100% understand the issues discussed in that thread, however,
> Andrew> I think the problem with the previous thread related to the changes in
> Andrew> gdb_bfd.c, rather than to the change in symfile.c.  As such, I think
> Andrew> this change might be acceptable, my reasoning is:
>
> Andrew>   - the objfile::mtime field is set by a call to bfd_get_mtime (see
> Andrew>     objfiles.c), which calls bfd_stat under the hood.  This will end
> Andrew>     up using the system stat,
>
> Andrew>   - In symfile.c we currently call stat directly, which will call the
> Andrew>     gnulib stat, which, if I understand the above thread correctly,
> Andrew>     might give a different result to the system stat in some cases,
>
> Andrew>   - By switching to using bfd_stat in symfile.c we should now be
> Andrew>     consistently calling the system stat,
>
> The BFD cache (in gdb_bfd.c) uses fstat, so I think there would still be
> a problem here.

OK, but the original mtime is captured via a call to bfd_stat.

Isn't the problem when we have two mismatched calls.  Using bfd_stat in
one place (a.k.a. system stat/fstat) vs a direct call to stat/fstat from
GDB in another place (a.k.a. gnulib stat/fstat).

In this patch I'm proposing that we _consistently_ call bfd_stat.  Sure
that might disagree with system stat/fstat -- but who cares?  So long as
the time being calculated and compared to is a BFD time_t result then we
should be fine .... or am I really not understanding the problem?

Thanks,
Andrew

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

[Tom Tromey]

>>>>> "Andrew" == Andrew Burgess <aburgess@redhat.com> writes:

>> The BFD cache (in gdb_bfd.c) uses fstat, so I think there would still be
>> a problem here.

Andrew> OK, but the original mtime is captured via a call to bfd_stat.

Andrew> Isn't the problem when we have two mismatched calls.  Using bfd_stat in
Andrew> one place (a.k.a. system stat/fstat) vs a direct call to stat/fstat from
Andrew> GDB in another place (a.k.a. gnulib stat/fstat).

Andrew> In this patch I'm proposing that we _consistently_ call bfd_stat.  Sure
Andrew> that might disagree with system stat/fstat -- but who cares?  So long as
Andrew> the time being calculated and compared to is a BFD time_t result then we
Andrew> should be fine .... or am I really not understanding the problem?

I think the fstat in gdb_bfd.c means that we will always re-read the
debug info, because now gdb_bfd_open will use fstat (and this is what is
recorded in the hash table), but reread_symbols will use bfd_stat.

Tom

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

[Andrew Burgess]

Tom Tromey <tromey@adacore.com> writes:

>>>>>> "Andrew" == Andrew Burgess <aburgess@redhat.com> writes:
>
>>> The BFD cache (in gdb_bfd.c) uses fstat, so I think there would still be
>>> a problem here.
>
> Andrew> OK, but the original mtime is captured via a call to bfd_stat.
>
> Andrew> Isn't the problem when we have two mismatched calls.  Using bfd_stat in
> Andrew> one place (a.k.a. system stat/fstat) vs a direct call to stat/fstat from
> Andrew> GDB in another place (a.k.a. gnulib stat/fstat).
>
> Andrew> In this patch I'm proposing that we _consistently_ call bfd_stat.  Sure
> Andrew> that might disagree with system stat/fstat -- but who cares?  So long as
> Andrew> the time being calculated and compared to is a BFD time_t result then we
> Andrew> should be fine .... or am I really not understanding the problem?
>
> I think the fstat in gdb_bfd.c means that we will always re-read the
> debug info, because now gdb_bfd_open will use fstat (and this is what is
> recorded in the hash table), but reread_symbols will use bfd_stat.

I really don't think this is the case.  The code in reread_symbols is
this:

      int res;
      struct stat new_statbuf;
      if (objfile->obfd->my_archive)
        res = stat (bfd_get_filename (objfile->obfd->my_archive), &new_statbuf);
      else
        res = stat (objfile_name (objfile), &new_statbuf);
      if (res != 0)
        {
          /* FIXME, should use print_sys_errmsg but it's not filtered.  */
          gdb_printf (_("`%s' has disappeared; keeping its symbols.\n"),
                      objfile_name (objfile));
          continue;
        }
      time_t new_modtime = new_statbuf.st_mtime;
      if (new_modtime != objfile->mtime)
        {
          gdb_printf (_("`%ps' has changed; re-reading symbols.\n"),
                      styled_string (file_name_style.style (),
                                     objfile_name (objfile)));

          ...
        }

So we perform a system 'stat' call and compare this to objfile::mtime.

And objfile::mtime is initialised in objfiles.c with this code:

  if (obfd != nullptr)
    {
      mtime = bfd_get_mtime (obfd.get ());

      /* Build section table.  */
      build_objfile_section_table (this);
    }

Where bfd_get_mtime is going to fetch a value by calling bfd_stat, which
will either by the system stat (linked into libbfd.a), or will be GDB's
remote fileio stat call.

The fstat call you are looking at in gdb_bfd.c is I believe only used
as part of GDB's internal caching mechanism for BFDs.  I can't see
anywhere that these mtime values can escape gdb_bfd.c, nor do I see
anywhere that these values are compared to values returned by bfd_stat.

We know that the value in reread_symbols comes from the system stat.  So
what I'm missing, is the path by which objfile::mtime gets set from
anywhere other that within libbfd.a, such that it might get contaminated
by a gnulib stat result.

Thanks,
Andrew

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

[Tom Tromey]

>>>>> "Andrew" == Andrew Burgess <aburgess@redhat.com> writes:

Andrew> We know that the value in reread_symbols comes from the system stat.  So
Andrew> what I'm missing, is the path by which objfile::mtime gets set from
Andrew> anywhere other that within libbfd.a, such that it might get contaminated
Andrew> by a gnulib stat result.

I was digging some more and I found:

commit d706b69e48268ccf3e95fd29b5374ac94c3a507b
Author: Tom Tromey <tromey@adacore.com>
Date:   Tue Sep 8 10:20:44 2020 -0600

    Do not adjust mtime timezone on Windows

... where I put in a patch to disable gnulib's stat replacement.

So I think your patch is totally fine, and I don't really know why I
didn't land my other changes after this.

Tom

Re: [PATCHv2 0/5] Fix using an exec file with target: prefix

[Tom Tromey]

>>>>> "Andrew" == Andrew Burgess via Gdb-patches <gdb-patches@sourceware.org> writes:

Andrew>   - Patch #5 has been completely changed.  Tom Tromey pointed me to a
Andrew>     previous series of his which (a) never got merged, and (b)
Andrew>     included a much better solution to this problem.  I've pulled out
Andrew>     just the part I need to resolve the issue in that patch.

I looked at my series again, and it's worse than the status quo in some
ways; for instance it requires opening a BFD before checking the cache.
But, like I mentioned elsewhere, I did seem to fix the gnulib stat
problem, which means a lot of that series is obsolete.  I think you've
picked up the only remaining good bit, which is the use of bfd_stat in
reread_symbols.

So, I think this series is ok.

Approved-By: Tom Tromey <tom@tromey.com>

Tom