gdb: fix reread_symbols when an objfile has target: prefix

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.

There is another issue that came up during testing that this commit
fixes.  Consider this GDB session:

  $ gdb -q
  (gdb) target extended-remote | ./gdbserver/gdbserver --multi --once -
  Remote debugging using | ./gdbserver/gdbserver --multi --once -
  Remote debugging using stdio
  (gdb) file /tmp/hello.x
  Reading symbols from /tmp/hello.x...
  (gdb) set remote exec-file /tmp/hello.x
  (gdb) start
  ... snip ...
  (gdb) load
  `system-supplied DSO at 0x7ffff7fcf000' has disappeared; keeping its symbols.
  Loading section .interp, size 0x1c lma 0x4002a8
  ... snip ...
  Start address 0x0000000000401050, load size 2004
  Transfer rate: 326 KB/sec, 87 bytes/write.

Notice this line:

  `system-supplied DSO at 0x7ffff7fcf000' has disappeared; keeping its symbols.

We actually see the same output, for the same reasons, when using a
native target, like this:

  $ gdb -q
  (gdb) file /tmp/hello.x
  Reading symbols from /tmp/hello.x...
  (gdb) start
  ... snip ...
  (gdb) load
  `system-supplied DSO at 0x7ffff7fcf000' has disappeared; keeping its symbols.
  You can't do that when your target is `native'
  (gdb)

In both cases this line appears because load_command (symfile.c) calls
reread_symbols, and reread_symbols loops over every currently loaded
objfile and tries to check if the file has changed on disk by calling
stat.

However, the `system-supplied DSO at 0x7ffff7fcf000' is an in-memory
BFD, the filename for this BFD is literally the string
'system-supplied DSO at 0x7ffff7fcf000'.

Before this commit GDB would try to use the system 'stat' call to stat
the file `system-supplied DSO at 0x7ffff7fcf000', which obviously
fails; there's no file with that name (usually).  As a consequence of
the stat failing GDB prints the ' .... has disappeared ...' line.

Initially, all this commit did was switch from using 'stat' to using
'bfd_stat'.  Calling bfd_stat on an in-memory BFD works just fine,
however, BFD just fills the 'struct stat' buffer with zeros (except
for the file size), see memory_bstat in bfd/bfdio.c.

However, there is a bit of a weirdness about in-memory BFDs.  When
they are initially created the libbfd caches an mtime within the bfd
object, this is done in bfd_from_remote_memory (elfcode.h), the cached
mtime is the time at which the in-memory BFD is created.

What this means is that when GDB creates the in-memory BFD, and we
call bfd_get_mtime(), the value returned, which GDB caches within
objfile::mtime is the creation time of the in-memory BFD.  But, when
this patch changes to use bfd_stat() we now get back 0, and so we
believe that the in-memory BFD has changed.  This is a change in
behaviour.

To avoid this change in behaviour, in this commit, I propose that we
always skip in-memory BFDs in reread_symbols.  This preserves the
behaviour from before this commit -- mostly.

As I'm not specifically checking for, and then skipping, in-memory
BFDs, we no longer see this line:

  `system-supplied DSO at 0x7ffff7fcf000' has disappeared; keeping its symbols.

Which I think is an improvement.

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

1 files changed, 22 insertions, 10 deletions

diff --git a/gdb/symfile.c b/gdb/symfile.c
index 30147b7..584a62c 100644
--- a/gdb/symfile.c
+++ b/gdb/symfile.c
@@ -2475,20 +2475,32 @@ 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);
+      /* When a in-memory BFD is initially created, it's mtime (as
+	 returned by bfd_get_mtime) is the creation time of the BFD.
+	 However, we call bfd_stat here as we want to see if the
+	 underlying file has changed, and in this case an in-memory BFD
+	 will return an st_mtime of zero, so it appears that the in-memory
+	 file has changed, which isn't what we want here -- this code is
+	 about reloading BFDs that changed on disk.
+
+	 Just skip any in-memory BFD.  */
+      if (objfile->obfd.get ()->flags & BFD_IN_MEMORY)
+	continue;
 
       struct stat new_statbuf;
-      int res = stat (filename, &new_statbuf);
+      int res = bfd_stat (objfile->obfd.get (), &new_statbuf);
       if (res != 0)
 	{
+	  /* 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);
+
 	  warning (_("`%ps' has disappeared; keeping its symbols."),
 		   styled_string (file_name_style.style (), filename));
 	  continue;