[PATCH 02/10] clean up allocation of bfd filenames

[PATCH 02/10] clean up allocation of bfd filenames

[Tom Tromey]

BFD requires the user to allocate the file name for a BFD.
GDB does this inconsistently: sometimes the file name is malloc'd,
sometimes not.  Sometimes it is freed, sometimes not.

This patch adds a new function that reallocated the BFD's filename
using bfd_alloc.  This ties the lifetime to the BFD and removes the
need to free the filename when closing the BFD.

	* symfile.c (symfile_bfd_open): Don't copy name.  Call
	gdb_bfd_stash_filename.
	(load_command): Open the new BFD before freeing the old.
	(bfd_open_maybe_remote): Call gdb_bfd_stash_filename.
	* symfile-mem.c (symbol_file_add_from_memory): Don't copy name.
	Call gdb_bfd_stash_filename.
	* spu-linux-nat.c (spu_bfd_open): Don't copy name.
	* solib-spu.c (spu_bfd_fopen): Don't copy name.  Call
	gdb_bfd_stash_filename.
	* solib-darwin.c (darwin_solib_get_all_image_info_addr_at_init):
	Free found_pathname.
	* rs6000-nat.c (add_vmap): Don't copy filename.  Call
	gdb_bfd_stash_filename.
	* remote.c (remote_bfd_open): Call gdb_bfd_stash_filename.
	* machoread.c (macho_add_oso_symfile): Call
	gdb_bfd_stash_filename.
	(macho_symfile_read_all_oso): Arrange to free archive_name.  Call
	gdb_bfd_stash_filename.
	(macho_check_dsym): Don't copy filename.  Call
	gdb_bfd_stash_filename.
	* jit.c (bfd_open_from_target_memory): Don't copy the filename.
	* gdb_bfd.c (gdb_bfd_stash_filename): New function.
	* gdb_bfd.h (gdb_bfd_stash_filename): Declare.
	* gcore.c (create_gcore_bfd): Call gdb_bfd_stash_filename.
	* exec.c (exec_close): Don't free the BFD's filename.
	(exec_file_attach): Don't copy the filename.  Call
	gdb_bfd_stash_filename.
	* corelow.c (core_close): Don't free the BFD's filename.
	(core_open): Call gdb_bfd_stash_filename.
	* corefile.c (reopen_exec_file): Remove #if 0 code.
	* solib.c (solib_bfd_fopen): Call gdb_bfd_stash_filename.  Free
	pathname.
	* dwarf2read.c (try_open_dwo_file): Call gdb_bfd_stash_filename.
---
 gdb/corelow.c       |    6 +++---
 gdb/dwarf2read.c    |    3 ++-
 gdb/exec.c          |   14 +++++---------
 gdb/gcore.c         |    1 +
 gdb/gdb_bfd.c       |   16 ++++++++++++++++
 gdb/gdb_bfd.h       |    6 ++++++
 gdb/jit.c           |    3 +--
 gdb/machoread.c     |   21 ++++++++++++---------
 gdb/remote.c        |   15 ++++++++++-----
 gdb/rs6000-nat.c    |   16 ++++++++--------
 gdb/solib-darwin.c  |   11 ++---------
 gdb/solib-spu.c     |    3 ++-
 gdb/solib.c         |   16 +++++++++-------
 gdb/spu-linux-nat.c |    2 +-
 gdb/symfile-mem.c   |    8 ++++++--
 gdb/symfile.c       |   38 +++++++++++++++++++++++---------------
 16 files changed, 107 insertions(+), 72 deletions(-)

diff --git a/gdb/corelow.c b/gdb/corelow.c
index 3dfa2f4..1fd60e4 100644
--- a/gdb/corelow.c
+++ b/gdb/corelow.c
@@ -216,9 +216,7 @@ core_close (int quitting)
 	  core_data = NULL;
 	}
 
-      name = bfd_get_filename (core_bfd);
       gdb_bfd_unref (core_bfd);
-      xfree (name);
       core_bfd = NULL;
     }
   core_vec = NULL;
@@ -326,6 +324,8 @@ core_open (char *filename, int from_tty)
   if (temp_bfd == NULL)
     perror_with_name (filename);
 
+  gdb_bfd_stash_filename (temp_bfd);
+
   if (!bfd_check_format (temp_bfd, bfd_core)
       && !gdb_check_format (temp_bfd))
     {
@@ -341,7 +341,7 @@ core_open (char *filename, int from_tty)
   /* Looks semi-reasonable.  Toss the old core file and work on the
      new.  */
 
-  discard_cleanups (old_chain);	/* Don't free filename any more */
+  do_cleanups (old_chain);
   unpush_target (&core_ops);
   core_bfd = temp_bfd;
   old_chain = make_cleanup (core_close_cleanup, 0 /*ignore*/);
diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index 4c976f3..6667c05 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -8121,11 +8121,12 @@ try_open_dwo_file (const char *file_name)
       xfree (absolute_name);
       return NULL;
     }
+  gdb_bfd_stash_filename (sym_bfd);
+  xfree (absolute_name);
   bfd_set_cacheable (sym_bfd, 1);
 
   if (!bfd_check_format (sym_bfd, bfd_object))
     {
-      xfree (absolute_name);
       gdb_bfd_unref (sym_bfd); /* This also closes desc.  */
       return NULL;
     }
diff --git a/gdb/exec.c b/gdb/exec.c
index 3d26bcc..540c271 100644
--- a/gdb/exec.c
+++ b/gdb/exec.c
@@ -99,10 +99,8 @@ exec_close (void)
   if (exec_bfd)
     {
       bfd *abfd = exec_bfd;
-      char *name = bfd_get_filename (abfd);
 
       gdb_bfd_unref (abfd);
-      xfree (name);
 
       /* Removing target sections may close the exec_ops target.
 	 Clear exec_bfd before doing so to prevent recursion.  */
@@ -230,6 +228,9 @@ exec_file_attach (char *filename, int from_tty)
 	     &scratch_pathname);
 	}
 #endif
+
+      cleanups = make_cleanup (xfree, scratch_pathname);
+
       if (scratch_chan < 0)
 	perror_with_name (filename);
       exec_bfd = gdb_bfd_ref (bfd_fopen (scratch_pathname, gnutarget,
@@ -242,13 +243,6 @@ exec_file_attach (char *filename, int from_tty)
 		 scratch_pathname, bfd_errmsg (bfd_get_error ()));
 	}
 
-      /* At this point, scratch_pathname and exec_bfd->name both point to the
-         same malloc'd string.  However exec_close() will attempt to free it
-         via the exec_bfd->name pointer, so we need to make another copy and
-         leave exec_bfd as the new owner of the original copy.  */
-      scratch_pathname = xstrdup (scratch_pathname);
-      cleanups = make_cleanup (xfree, scratch_pathname);
-
       if (!bfd_check_format_matches (exec_bfd, bfd_object, &matching))
 	{
 	  /* Make sure to close exec_bfd, or else "run" might try to use
@@ -259,6 +253,8 @@ exec_file_attach (char *filename, int from_tty)
 		 gdb_bfd_errmsg (bfd_get_error (), matching));
 	}
 
+      gdb_bfd_stash_filename (exec_bfd);
+
       /* FIXME - This should only be run for RS6000, but the ifdef is a poor
          way to accomplish.  */
 #ifdef DEPRECATED_IBM6000_TARGET
diff --git a/gdb/gcore.c b/gdb/gcore.c
index 486ea5f..f9a1389 100644
--- a/gdb/gcore.c
+++ b/gdb/gcore.c
@@ -55,6 +55,7 @@ create_gcore_bfd (char *filename)
 
   if (!obfd)
     error (_("Failed to open '%s' for output."), filename);
+  gdb_bfd_stash_filename (obfd);
   bfd_set_format (obfd, bfd_core);
   bfd_set_arch_mach (obfd, default_gcore_arch (), default_gcore_mach ());
   return obfd;
diff --git a/gdb/gdb_bfd.c b/gdb/gdb_bfd.c
index 160e9e6..8f40d74 100644
--- a/gdb/gdb_bfd.c
+++ b/gdb/gdb_bfd.c
@@ -21,6 +21,22 @@
 #include "defs.h"
 #include "gdb_bfd.h"
 #include "gdb_assert.h"
+#include "gdb_string.h"
+
+/* See gdb_bfd.h.  */
+
+void
+gdb_bfd_stash_filename (struct bfd *abfd)
+{
+  char *name = bfd_get_filename (abfd);
+  char *data;
+
+  data = bfd_alloc (abfd, strlen (name) + 1);
+  strcpy (data, name);
+
+  /* Unwarranted chumminess with BFD.  */
+  abfd->filename = data;
+}
 
 /* Close ABFD, and warn if that fails.  */
 
diff --git a/gdb/gdb_bfd.h b/gdb/gdb_bfd.h
index c6d94a0..a1d5b03 100644
--- a/gdb/gdb_bfd.h
+++ b/gdb/gdb_bfd.h
@@ -21,6 +21,12 @@
 #ifndef GDB_BFD_H
 #define GDB_BFD_H
 
+/* Make a copy ABFD's filename using bfd_alloc, and reassign it to the
+   BFD.  This ensures that the BFD's filename has the same lifetime as
+   the BFD itself.  */
+
+void gdb_bfd_stash_filename (struct bfd *abfd);
+
 /* Acquire a new reference to ABFD.  Returns ABFD for convenience.
    It is fine for ABFD to be NULL; in this case the function does
    nothing and returns NULL.  */
diff --git a/gdb/jit.c b/gdb/jit.c
index 6a1425c..6478397 100644
--- a/gdb/jit.c
+++ b/gdb/jit.c
@@ -133,12 +133,11 @@ mem_bfd_iovec_stat (struct bfd *abfd, void *stream, struct stat *sb)
 static struct bfd *
 bfd_open_from_target_memory (CORE_ADDR addr, ULONGEST size, char *target)
 {
-  const char *filename = xstrdup ("<in-memory>");
   struct target_buffer *buffer = xmalloc (sizeof (struct target_buffer));
 
   buffer->base = addr;
   buffer->size = size;
-  return bfd_openr_iovec (filename, target,
+  return bfd_openr_iovec ("<in-memory>", target,
                           mem_bfd_iovec_open,
                           buffer,
                           mem_bfd_iovec_pread,
diff --git a/gdb/machoread.c b/gdb/machoread.c
index eb56f14..6d309bb 100644
--- a/gdb/machoread.c
+++ b/gdb/machoread.c
@@ -629,10 +629,10 @@ macho_add_oso_symfile (oso_el *oso, bfd *abfd,
 
   bfd_hash_table_free (&table);
 
-  /* Make sure that the filename was malloc'ed.  The current filename comes
-     either from an OSO symbol name or from an archive name.  Memory for both
-     is not managed by gdb.  */
-  abfd->filename = xstrdup (abfd->filename);
+  /* Make sure that the filename has the correct lifetime.  The
+     current filename comes either from an OSO symbol name or from an
+     archive name.  Memory for both is not managed by gdb.  */
+  gdb_bfd_stash_filename (abfd);
 
   /* We need to clear SYMFILE_MAINLINE to avoid interractive question
      from symfile.c:symbol_file_add_with_addrs_or_offsets.  */
@@ -651,6 +651,7 @@ macho_symfile_read_all_oso (struct objfile *main_objfile, int symfile_flags)
   int ix;
   VEC (oso_el) *vec;
   oso_el *oso;
+  struct cleanup *cleanup = make_cleanup (null_cleanup, NULL);
 
   vec = oso_vector;
   oso_vector = NULL;
@@ -677,6 +678,8 @@ macho_symfile_read_all_oso (struct objfile *main_objfile, int symfile_flags)
 	  memcpy (archive_name, oso->name, pfx_len);
 	  archive_name[pfx_len] = '\0';
 
+	  make_cleanup (xfree, archive_name);
+
           /* Compute number of oso for this archive.  */
           for (last_ix = ix;
                VEC_iterate (oso_el, vec, last_ix, oso2); last_ix++)
@@ -702,6 +705,9 @@ macho_symfile_read_all_oso (struct objfile *main_objfile, int symfile_flags)
               ix = last_ix;
 	      continue;
 	    }
+
+	  gdb_bfd_stash_filename (archive_bfd);
+
 	  member_bfd = gdb_bfd_ref (bfd_openr_next_archived_file (archive_bfd,
 								  NULL));
 
@@ -773,6 +779,7 @@ macho_symfile_read_all_oso (struct objfile *main_objfile, int symfile_flags)
     }
 
   VEC_free (oso_el, vec);
+  do_cleanups (cleanup);
 }
 
 /* DSYM (debug symbols) files contain the debug info of an executable.
@@ -810,20 +817,18 @@ macho_check_dsym (struct objfile *objfile)
       warning (_("can't find UUID in %s"), objfile->name);
       return NULL;
     }
-  dsym_filename = xstrdup (dsym_filename);
   dsym_bfd = gdb_bfd_ref (bfd_openr (dsym_filename, gnutarget));
   if (dsym_bfd == NULL)
     {
       warning (_("can't open dsym file %s"), dsym_filename);
-      xfree (dsym_filename);
       return NULL;
     }
+  gdb_bfd_stash_filename (dsym_filename);
 
   if (!bfd_check_format (dsym_bfd, bfd_object))
     {
       gdb_bfd_unref (dsym_bfd);
       warning (_("bad dsym file format: %s"), bfd_errmsg (bfd_get_error ()));
-      xfree (dsym_filename);
       return NULL;
     }
 
@@ -832,7 +837,6 @@ macho_check_dsym (struct objfile *objfile)
     {
       warning (_("can't find UUID in %s"), dsym_filename);
       gdb_bfd_unref (dsym_bfd);
-      xfree (dsym_filename);
       return NULL;
     }
   if (memcmp (dsym_uuid->command.uuid.uuid, main_uuid->command.uuid.uuid,
@@ -840,7 +844,6 @@ macho_check_dsym (struct objfile *objfile)
     {
       warning (_("dsym file UUID doesn't match the one in %s"), objfile->name);
       gdb_bfd_unref (dsym_bfd);
-      xfree (dsym_filename);
       return NULL;
     }
   return dsym_bfd;
diff --git a/gdb/remote.c b/gdb/remote.c
index 1c9367d..6ccab54 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -42,6 +42,7 @@
 #include "cli/cli-decode.h"
 #include "cli/cli-setshow.h"
 #include "target-descriptions.h"
+#include "gdb_bfd.h"
 
 #include <ctype.h>
 #include <sys/time.h>
@@ -9823,11 +9824,15 @@ remote_filename_p (const char *filename)
 bfd *
 remote_bfd_open (const char *remote_file, const char *target)
 {
-  return bfd_openr_iovec (remote_file, target,
-			  remote_bfd_iovec_open, NULL,
-			  remote_bfd_iovec_pread,
-			  remote_bfd_iovec_close,
-			  remote_bfd_iovec_stat);
+  bfd *abfd = bfd_openr_iovec (remote_file, target,
+			       remote_bfd_iovec_open, NULL,
+			       remote_bfd_iovec_pread,
+			       remote_bfd_iovec_close,
+			       remote_bfd_iovec_stat);
+
+  if (abfd != NULL)
+    gdb_bfd_stash_filename (abfd);
+  return abfd;
 }
 
 void
diff --git a/gdb/rs6000-nat.c b/gdb/rs6000-nat.c
index 1aa4a17..017e997 100644
--- a/gdb/rs6000-nat.c
+++ b/gdb/rs6000-nat.c
@@ -730,7 +730,7 @@ static struct vmap *
 add_vmap (LdInfo *ldi)
 {
   bfd *abfd, *last;
-  char *mem, *objname, *filename;
+  char *mem, *filename;
   struct objfile *obj;
   struct vmap *vp;
   int fd;
@@ -743,22 +743,22 @@ add_vmap (LdInfo *ldi)
   filename = LDI_FILENAME (ldi, arch64);
   mem = filename + strlen (filename) + 1;
   mem = xstrdup (mem);
-  objname = xstrdup (filename);
 
   fd = LDI_FD (ldi, arch64);
   if (fd < 0)
     /* Note that this opens it once for every member; a possible
        enhancement would be to only open it once for every object.  */
-    abfd = bfd_openr (objname, gnutarget);
+    abfd = bfd_openr (filename, gnutarget);
   else
-    abfd = bfd_fdopenr (objname, gnutarget, fd);
+    abfd = bfd_fdopenr (filename, gnutarget, fd);
   abfd = gdb_bfd_ref (abfd);
   if (!abfd)
     {
       warning (_("Could not open `%s' as an executable file: %s"),
-	       objname, bfd_errmsg (bfd_get_error ()));
+	       filename, bfd_errmsg (bfd_get_error ()));
       return NULL;
     }
+  gdb_bfd_stash_filename (abfd);
 
   /* Make sure we have an object file.  */
 
@@ -775,7 +775,7 @@ add_vmap (LdInfo *ldi)
 
       if (!last)
 	{
-	  warning (_("\"%s\": member \"%s\" missing."), objname, mem);
+	  warning (_("\"%s\": member \"%s\" missing."), filename, mem);
 	  gdb_bfd_unref (abfd);
 	  return NULL;
 	}
@@ -783,7 +783,7 @@ add_vmap (LdInfo *ldi)
       if (!bfd_check_format (last, bfd_object))
 	{
 	  warning (_("\"%s\": member \"%s\" not in executable format: %s."),
-		   objname, mem, bfd_errmsg (bfd_get_error ()));
+		   filename, mem, bfd_errmsg (bfd_get_error ()));
 	  gdb_bfd_unref (last);
 	  gdb_bfd_unref (abfd);
 	  return NULL;
@@ -794,7 +794,7 @@ add_vmap (LdInfo *ldi)
   else
     {
       warning (_("\"%s\": not in executable format: %s."),
-	       objname, bfd_errmsg (bfd_get_error ()));
+	       filename, bfd_errmsg (bfd_get_error ()));
       gdb_bfd_unref (abfd);
       return NULL;
     }
diff --git a/gdb/solib-darwin.c b/gdb/solib-darwin.c
index bc2cd79..242f8cc 100644
--- a/gdb/solib-darwin.c
+++ b/gdb/solib-darwin.c
@@ -510,17 +510,10 @@ darwin_bfd_open (char *pathname)
 				gdbarch_bfd_arch_info (target_gdbarch));
   if (!res)
     {
-      gdb_bfd_unref (abfd);
-      make_cleanup (xfree, found_pathname);
+      make_cleanup_bfd_close (abfd);
       error (_("`%s': not a shared-library: %s"),
-	     found_pathname, bfd_errmsg (bfd_get_error ()));
+	     bfd_get_filename (abfd), bfd_errmsg (bfd_get_error ()));
     }
-
-  /* Make sure that the filename is malloc'ed.  The current filename
-     for fat-binaries BFDs is a name that was generated by BFD, usually
-     a static string containing the name of the architecture.  */
-  res->filename = xstrdup (pathname);
-
   return res;
 }
 
diff --git a/gdb/solib-spu.c b/gdb/solib-spu.c
index 5f03a42..5eeae62 100644
--- a/gdb/solib-spu.c
+++ b/gdb/solib-spu.c
@@ -326,7 +326,7 @@ spu_bfd_fopen (char *name, CORE_ADDR addr)
   CORE_ADDR *open_closure = xmalloc (sizeof (CORE_ADDR));
   *open_closure = addr;
 
-  nbfd = gdb_bfd_ref (bfd_openr_iovec (xstrdup (name), "elf32-spu",
+  nbfd = gdb_bfd_ref (bfd_openr_iovec (name, "elf32-spu",
 				       spu_bfd_iovec_open, open_closure,
 				       spu_bfd_iovec_pread, spu_bfd_iovec_close,
 				       spu_bfd_iovec_stat));
@@ -339,6 +339,7 @@ spu_bfd_fopen (char *name, CORE_ADDR addr)
       return NULL;
     }
 
+  gdb_bfd_stash_filename (nbfd);
   return nbfd;
 }
 
diff --git a/gdb/solib.c b/gdb/solib.c
index 7b9f473..4ddf91a 100644
--- a/gdb/solib.c
+++ b/gdb/solib.c
@@ -361,9 +361,9 @@ solib_find (char *in_pathname, int *fd)
    it is used as file handle to open the file.  Throws an error if the file
    could not be opened.  Handles both local and remote file access.
 
-   PATHNAME must be malloc'ed by the caller.  If successful, the new BFD's
-   name will point to it.  If unsuccessful, PATHNAME will be freed and the
-   FD will be closed (unless FD was -1).  */
+   PATHNAME must be malloc'ed by the caller.  It will be freed by this
+   function.  If unsuccessful, the FD will be closed (unless FD was
+   -1).  */
 
 bfd *
 solib_bfd_fopen (char *pathname, int fd)
@@ -390,6 +390,9 @@ solib_bfd_fopen (char *pathname, int fd)
 	     pathname, bfd_errmsg (bfd_get_error ()));
     }
 
+  gdb_bfd_stash_filename (abfd);
+  xfree (pathname);
+
   return gdb_bfd_ref (abfd);
 }
 
@@ -421,17 +424,16 @@ solib_bfd_open (char *pathname)
   /* Check bfd format.  */
   if (!bfd_check_format (abfd, bfd_object))
     {
-      gdb_bfd_unref (abfd);
-      make_cleanup (xfree, found_pathname);
+      make_cleanup_bfd_close (abfd);
       error (_("`%s': not in executable format: %s"),
-	     found_pathname, bfd_errmsg (bfd_get_error ()));
+	     bfd_get_filename (abfd), bfd_errmsg (bfd_get_error ()));
     }
 
   /* Check bfd arch.  */
   b = gdbarch_bfd_arch_info (target_gdbarch);
   if (!b->compatible (b, bfd_get_arch_info (abfd)))
     warning (_("`%s': Shared library architecture %s is not compatible "
-               "with target architecture %s."), found_pathname,
+               "with target architecture %s."), bfd_get_filename (abfd),
              bfd_get_arch_info (abfd)->printable_name, b->printable_name);
 
   return abfd;
diff --git a/gdb/spu-linux-nat.c b/gdb/spu-linux-nat.c
index aeb7242..12f8211 100644
--- a/gdb/spu-linux-nat.c
+++ b/gdb/spu-linux-nat.c
@@ -315,7 +315,7 @@ spu_bfd_open (ULONGEST addr)
   ULONGEST *open_closure = xmalloc (sizeof (ULONGEST));
   *open_closure = addr;
 
-  nbfd = bfd_openr_iovec (xstrdup ("<in-memory>"), "elf32-spu",
+  nbfd = bfd_openr_iovec ("<in-memory>", "elf32-spu",
 			  spu_bfd_iovec_open, open_closure,
 			  spu_bfd_iovec_pread, spu_bfd_iovec_close,
 			  spu_bfd_iovec_stat);
diff --git a/gdb/symfile-mem.c b/gdb/symfile-mem.c
index 1c306fa..0470d97 100644
--- a/gdb/symfile-mem.c
+++ b/gdb/symfile-mem.c
@@ -103,9 +103,13 @@ symbol_file_add_from_memory (struct bfd *templ, CORE_ADDR addr, char *name,
 
   gdb_bfd_ref (nbfd);
   if (name == NULL)
-    nbfd->filename = xstrdup ("shared object read from target memory");
+    nbfd->filename = "shared object read from target memory";
   else
-    nbfd->filename = name;
+    {
+      nbfd->filename = name;
+      gdb_bfd_stash_filename (nbfd);
+      xfree (name);
+    }
 
   if (!bfd_check_format (nbfd, bfd_object))
     {
diff --git a/gdb/symfile.c b/gdb/symfile.c
index 2ad72c5..99427ae 100644
--- a/gdb/symfile.c
+++ b/gdb/symfile.c
@@ -1700,7 +1700,13 @@ bfd_open_maybe_remote (const char *name)
   if (remote_filename_p (name))
     return gdb_bfd_ref (remote_bfd_open (name, gnutarget));
   else
-    return gdb_bfd_ref (bfd_openr (name, gnutarget));
+    {
+      bfd *result = gdb_bfd_ref (bfd_openr (name, gnutarget));
+
+      if (result != NULL)
+	gdb_bfd_stash_filename (result);
+      return result;
+    }
 }
 
 
@@ -1718,19 +1724,14 @@ symfile_bfd_open (char *name)
 
   if (remote_filename_p (name))
     {
-      name = xstrdup (name);
       sym_bfd = gdb_bfd_ref (remote_bfd_open (name, gnutarget));
       if (!sym_bfd)
-	{
-	  make_cleanup (xfree, name);
-	  error (_("`%s': can't open to read symbols: %s."), name,
-		 bfd_errmsg (bfd_get_error ()));
-	}
+	error (_("`%s': can't open to read symbols: %s."), name,
+	       bfd_errmsg (bfd_get_error ()));
 
       if (!bfd_check_format (sym_bfd, bfd_object))
 	{
-	  gdb_bfd_unref (sym_bfd);
-	  make_cleanup (xfree, name);
+	  make_cleanup_bfd_close (sym_bfd);
 	  error (_("`%s': can't read symbols: %s."), name,
 		 bfd_errmsg (bfd_get_error ()));
 	}
@@ -1759,10 +1760,9 @@ symfile_bfd_open (char *name)
       perror_with_name (name);
     }
 
-  /* Free 1st new malloc'd copy, but keep the 2nd malloc'd copy in
-     bfd.  It'll be freed in free_objfile().  */
   xfree (name);
   name = absolute_name;
+  make_cleanup (xfree, name);
 
   sym_bfd = gdb_bfd_ref (bfd_fopen (name, gnutarget, FOPEN_RB, desc));
   if (!sym_bfd)
@@ -1776,11 +1776,12 @@ symfile_bfd_open (char *name)
   if (!bfd_check_format (sym_bfd, bfd_object))
     {
       make_cleanup_bfd_close (sym_bfd);
-      make_cleanup (xfree, name);
       error (_("`%s': can't read symbols: %s."), name,
 	     bfd_errmsg (bfd_get_error ()));
     }
 
+  gdb_bfd_stash_filename (sym_bfd);
+
   return sym_bfd;
 }
 
@@ -2511,9 +2512,16 @@ reread_symbols (void)
 	  /* Clean up any state BFD has sitting around.  We don't need
 	     to close the descriptor but BFD lacks a way of closing the
 	     BFD without closing the descriptor.  */
-	  obfd_filename = bfd_get_filename (objfile->obfd);
-	  gdb_bfd_unref (objfile->obfd);
-	  objfile->obfd = bfd_open_maybe_remote (obfd_filename);
+	  {
+	    struct bfd *obfd = objfile->obfd;
+
+	    obfd_filename = bfd_get_filename (objfile->obfd);
+	    /* Open the new BFD before freeing the old one, so that
+	       the filename remains live.  */
+	    objfile->obfd = bfd_open_maybe_remote (obfd_filename);
+	    gdb_bfd_unref (obfd);
+	  }
+
 	  if (objfile->obfd == NULL)
 	    error (_("Can't open %s to read symbols."), objfile->name);
 	  /* bfd_openr sets cacheable to true, which is what we want.  */
-- 
1.7.7.6

Re: [PATCH 02/10] clean up allocation of bfd filenames

[Jan Kratochvil]

On Wed, 18 Jul 2012 21:34:10 +0200, Tom Tromey wrote:
> BFD requires the user to allocate the file name for a BFD.
> GDB does this inconsistently: sometimes the file name is malloc'd,
> sometimes not.  Sometimes it is freed, sometimes not.
> 
> This patch adds a new function that reallocated the BFD's filename
> using bfd_alloc.  This ties the lifetime to the BFD and removes the
> need to free the filename when closing the BFD.
[...]
> +void
> +gdb_bfd_stash_filename (struct bfd *abfd)
> +{
> +  char *name = bfd_get_filename (abfd);
> +  char *data;
> +
> +  data = bfd_alloc (abfd, strlen (name) + 1);
> +  strcpy (data, name);
> +
> +  /* Unwarranted chumminess with BFD.  */
> +  abfd->filename = data;
> +}

Would not it be worth propose this bfd filename memory storage globally in
bfd/ ?


Thanks,
Jan

Re: [PATCH 02/10] clean up allocation of bfd filenames

[Tom Tromey]

>>>>> "Jan" == Jan Kratochvil <jan.kratochvil@redhat.com> writes:

Jan> Would not it be worth propose this bfd filename memory storage
Jan> globally in bfd/ ?

I went back and forth on this.

I think it would probably be better in general, but I wasn't too
enthused about auditing a lot of code that I don't know well to make the
change globally.

I can probably make myself do it though.

Tom

Re: [PATCH 02/10] clean up allocation of bfd filenames

[Tom Tromey]

Jan> Would not it be worth propose this bfd filename memory storage
Jan> globally in bfd/ ?

Tom> I went back and forth on this.

Tom> I think it would probably be better in general, but I wasn't too
Tom> enthused about auditing a lot of code that I don't know well to make the
Tom> change globally.

I remembered the argument that made me decide to do it in gdb.

BFD made a choice, long ago, to have clients manage the memory for the
filename.  My patch just changes one client to handle it in a uniform
way.  Changing other clients, or changing the library itself, might make
sense -- but it might not, and I don't really know anything about some
of these clients, like 'ld'.  So I figured it was ok to leave things in
this state.

Tom

Re: [PATCH 02/10] clean up allocation of bfd filenames

[Jan Kratochvil]

On Fri, 20 Jul 2012 18:30:22 +0200, Tom Tromey wrote:
> I remembered the argument that made me decide to do it in gdb.
> 
> BFD made a choice, long ago, to have clients manage the memory for the
> filename.

OK, if BFD has already made this decision I sure have no more comments.


Thanks for remembering it,
Jan

RFC: solib.c:solib_map_sections so->so_name clobbering (was: [PATCH 02/10] clean up allocation of bfd filenames)

[Joel Brobecker]

Hi Tom,

> BFD requires the user to allocate the file name for a BFD.
> GDB does this inconsistently: sometimes the file name is malloc'd,
> sometimes not.  Sometimes it is freed, sometimes not.
> 
> This patch adds a new function that reallocated the BFD's filename
> using bfd_alloc.  This ties the lifetime to the BFD and removes the
> need to free the filename when closing the BFD.
[...]
> 	* solib-darwin.c (darwin_solib_get_all_image_info_addr_at_init):
> 	Free found_pathname.

This patch accidently triggered a regression in the "info sharedlibrary"
command for darwin targets. With gdb-7.5:

    (gdb) info shared
    From                To                  Syms Read   Shared Object Library
    0x00007fff8d8129eb  0x00007fff8d8131d8  Yes (*)     /usr/lib/libSystem.B.dylib
    0x00007fff8e5a4320  0x00007fff8e5a91c0  Yes (*)     /usr/lib/system/libcache.dylib
    [etc]

But with the current HEAD, the solib name becomes:

    (gdb) info shared
    From                To                  Syms Read   Shared Object Library
    0x00007fff8d8129eb  0x00007fff8d8131d8  Yes (*)     i386:x86-64
    0x00007fff8e5a4320  0x00007fff8e5a91c0  Yes (*)     i386:x86-64

That's because the solist's so_name get overwritten with the shared
object's bfd's filename in solib_map_sections:

  /* copy full path name into so_name, so that later symbol_file_add
     can find it.  */
  [...]
  strcpy (so->so_name, bfd_get_filename (abfd));

This used to work more or less by accident, thanks to the following
piece of code in solib-darwin.c:darwin_bfd_open:

   /* Make sure that the filename is malloc'ed.  The current filename
      for fat-binaries BFDs is a name that was generated by BFD, usually
      a static string containing the name of the architecture.  */
   res->filename = xstrdup (pathname);

But your patch removed (justifiably) this code, which led us to
use the filename created by the BFD layer. For Mach-O fat binaries,
bfd explicitly uses the architecture name of the extracted object.
See bfd_mach_o_fat_member_init. I don't know why, but that's not
the only architecture that produces something which is not a valid
filename (refer to our discussion re AIX archives and their shared
objects).

To me, the part that looks the most intriguing of all is why solib.c
would overwrite the so_name set by the solib backend. I tried researching
that, but found that it has been this way since the beginning of CVS.
The comment is now OBE, since symbol_file_add is no longer responsible
for creating the BFD - the solib backend is. I also verified that
so_name appears to be used for no other purposes but printing or
so_name matching (Eg: deciding if loading of symbols for shared
library should be done or not).

With that in mind, I think the overwriting of the so_name might not
be needed or make much sense anymore. Removing it should fix the
regression on Darwin, and also help in the case of my work with the
solib-aix patch [1].

What do you (or others!) think? I am happy to test and submit a proper
patch. We wouldn't be able to use a patch like this on the 7.6 branch,
so for Darwin, I'd restore the xstrdup below - it would be a memory
leak, but better to have a leak than not having the shared library name,
IMO.

Thanks!

> diff --git a/gdb/solib-darwin.c b/gdb/solib-darwin.c
> index bc2cd79..242f8cc 100644
> --- a/gdb/solib-darwin.c
> +++ b/gdb/solib-darwin.c
> @@ -510,17 +510,10 @@ darwin_bfd_open (char *pathname)
>  				gdbarch_bfd_arch_info (target_gdbarch));
>    if (!res)
>      {
> -      gdb_bfd_unref (abfd);
> -      make_cleanup (xfree, found_pathname);
> +      make_cleanup_bfd_close (abfd);
>        error (_("`%s': not a shared-library: %s"),
> -	     found_pathname, bfd_errmsg (bfd_get_error ()));
> +	     bfd_get_filename (abfd), bfd_errmsg (bfd_get_error ()));
>      }
> -
> -  /* Make sure that the filename is malloc'ed.  The current filename
> -     for fat-binaries BFDs is a name that was generated by BFD, usually
> -     a static string containing the name of the architecture.  */
> -  res->filename = xstrdup (pathname);

-- 
Joel

[1] http://www.sourceware.org/ml/gdb-patches/2013-03/msg00818.html

Re: RFC: solib.c:solib_map_sections so->so_name clobbering

[Tom Tromey]

>>>>> "Joel" == Joel Brobecker <brobecker@adacore.com> writes:

Joel> With that in mind, I think the overwriting of the so_name might not
Joel> be needed or make much sense anymore. Removing it should fix the
Joel> regression on Darwin, and also help in the case of my work with the
Joel> solib-aix patch [1].

Joel> What do you (or others!) think? I am happy to test and submit a proper
Joel> patch.

It sounds reasonable to me.
I'm sorry about the mess here.

Joel> We wouldn't be able to use a patch like this on the 7.6 branch,
Joel> so for Darwin, I'd restore the xstrdup below - it would be a memory
Joel> leak, but better to have a leak than not having the shared library name,
Joel> IMO.

You can make it not leak by using bfd_alloc.

Tom

Re: RFC: solib.c:solib_map_sections so->so_name clobbering

[Joel Brobecker]

[-- Attachment #1: Type: text/plain, Size: 1056 bytes --]

> Joel> What do you (or others!) think? I am happy to test and submit a proper
> Joel> patch.
> 
> It sounds reasonable to me.
> I'm sorry about the mess here.

Thanks for the feedback, and really you should not feel sorry at all.
It wasn't your mess :-). And the good news is that I feel like we are
slowly making it better.

Attached is the patch I tested on x86_64-linux. I am off next week,
but I will also apply it to AdaCore's version of GDB to get some
exposure on all the targets that we support. JIC. That will give
more time for others to comment as well, if needed.

gdb/ChangeLog:

        * solib.c (solib_map_sections): Remove code overwriting
        SO->SO_NAME with the bfd's filename.

> Joel> We wouldn't be able to use a patch like this on the 7.6 branch,
> Joel> so for Darwin, I'd restore the xstrdup below - it would be a memory
> Joel> leak, but better to have a leak than not having the shared library name,
> Joel> IMO.
> 
> You can make it not leak by using bfd_alloc.

Neat! I will try that before I go.

Thanks again,
-- 
Joel

[-- Attachment #2: 0001-Do-not-overwrite-so_list-s-so_name-in-solib_map_sect.patch --]
[-- Type: text/x-diff, Size: 1094 bytes --]

From f7bd26fcfdbcde492d4da15d291115d714f5febd Mon Sep 17 00:00:00 2001
From: Joel Brobecker <brobecker@adacore.com>
Date: Thu, 28 Mar 2013 16:23:06 -0700
Subject: [PATCH] Do not overwrite so_list's so_name in solib_map_sections

gdb/ChangeLog:

        * solib.c (solib_map_sections): Remove code overwriting
        SO->SO_NAME with the bfd's filename.
---
 gdb/solib.c |    6 ------
 1 file changed, 6 deletions(-)

diff --git a/gdb/solib.c b/gdb/solib.c
index 8129c0f..6978677 100644
--- a/gdb/solib.c
+++ b/gdb/solib.c
@@ -465,12 +465,6 @@ solib_map_sections (struct so_list *so)
   /* Leave bfd open, core_xfer_memory and "info files" need it.  */
   so->abfd = abfd;
 
-  /* copy full path name into so_name, so that later symbol_file_add
-     can find it.  */
-  if (strlen (bfd_get_filename (abfd)) >= SO_NAME_MAX_PATH_SIZE)
-    error (_("Shared library file name is too long."));
-  strcpy (so->so_name, bfd_get_filename (abfd));
-
   if (build_section_table (abfd, &so->sections, &so->sections_end))
     {
       error (_("Can't find the file sections in `%s': %s"),
-- 
1.7.10.4

darwin: fix SO name in "info sharedlibrary" (was: "RFC: solib.c:solib_map_sections so->so_name clobbering")

[Joel Brobecker]

[-- Attachment #1: Type: text/plain, Size: 587 bytes --]

> > Joel> We wouldn't be able to use a patch like this on the 7.6 branch,
> > Joel> so for Darwin, I'd restore the xstrdup below - it would be a memory
> > Joel> leak, but better to have a leak than not having the shared library name,
> > Joel> IMO.
> > 
> > You can make it not leak by using bfd_alloc.
> 
> Neat! I will try that before I go.

Here is the patch.

gdb/ChangeLog:

        * solib-darwin.c (darwin_bfd_open): Set the filename of the
        returned bfd to a copy of PATHNAME.

Tested on x86_64-darwin, no regression. Does it look OK for the branch?

Thank you,
-- 
Joel

[-- Attachment #2: 0001-solib-darwin-Overwrite-filename-of-unpeeled-fat-bina.patch --]
[-- Type: text/x-diff, Size: 1099 bytes --]

From c77cdff7df21b5351f471faaf4d34dd423c7c385 Mon Sep 17 00:00:00 2001
From: Joel Brobecker <brobecker@adacore.com>
Date: Thu, 28 Mar 2013 21:48:36 -0400
Subject: [PATCH] solib-darwin: Overwrite filename of unpeeled fat-binary bfd.

gdb/ChangeLog:

        * solib-darwin.c (darwin_bfd_open): Set the filename of the
        returned bfd to a copy of PATHNAME.
---
 gdb/solib-darwin.c |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/gdb/solib-darwin.c b/gdb/solib-darwin.c
index b9a4be1..9b61de5 100644
--- a/gdb/solib-darwin.c
+++ b/gdb/solib-darwin.c
@@ -621,6 +621,16 @@ darwin_bfd_open (char *pathname)
 	     bfd_get_filename (abfd), bfd_errmsg (bfd_get_error ()));
     }
 
+  /* The current filename for fat-binary BFDs is a name generated
+     by BFD, usually a string containing the name of the architecture.
+     Reset its value to the actual filename.  */
+    {
+      char *data = bfd_alloc (res, strlen (pathname) + 1);
+
+      strcpy (data, pathname);
+      res->filename = data;
+    }
+
   gdb_bfd_unref (abfd);
   return res;
 }
-- 
1.7.0.4

Re: darwin: fix SO name in "info sharedlibrary"

[Tom Tromey]

>>>>> "Joel" == Joel Brobecker <brobecker@adacore.com> writes:

Joel> Here is the patch.

Joel> gdb/ChangeLog:

Joel>         * solib-darwin.c (darwin_bfd_open): Set the filename of the
Joel>         returned bfd to a copy of PATHNAME.

Joel> Tested on x86_64-darwin, no regression. Does it look OK for the branch?

Looks good to me.

Tom

checked in: darwin: fix SO name in "info sharedlibrary"

[Joel Brobecker]

> Joel> gdb/ChangeLog:
> 
> Joel>         * solib-darwin.c (darwin_bfd_open): Set the filename of the
> Joel>         returned bfd to a copy of PATHNAME.
> 
> Joel> Tested on x86_64-darwin, no regression. Does it look OK for the branch?
> 
> Looks good to me.

Thanks, Tom. Checked in.

-- 
Joel

checked in: Re: RFC: solib.c:solib_map_sections so->so_name clobbering

[Joel Brobecker]

> gdb/ChangeLog:
> 
>         * solib.c (solib_map_sections): Remove code overwriting
>         SO->SO_NAME with the bfd's filename.

Now checked in.

-- 
Joel

Re: checked in: Re: RFC: solib.c:solib_map_sections so->so_name clobbering

[Luis Machado]

Hi,

It looks like this commit introduced a small regression for MI and 
shared libraries, and the testsuite does not cover this case.

Suppose we have GDB running on a host and a stub/gdbserver running on a 
separate remote target. Suppose shared libraries (for symbols) on the 
target are located in a different path compared to the host, say, 
<host_path> and <target_path>.

During debugging, eventually the shared libraries will be loaded and we 
used to see a shared library load notification like the following:

=library-loaded,id="<target_path>/libhello.so",target-name="<target_path>/libhello.so",host-name="<host_path>/libhello.so",symbols-loaded="0",thread-group="i1"

After this commit, this is what we see:

=library-loaded,id="<target_path>/libhello.so",target-name="<target_path>/libhello.so",host-name="<target_path>/libhello.so",symbols-loaded="0",thread-group="i1"

So it looks like we've lost information about the shared library's path 
on the host, which may not be a big deal for CLI GDB, but may confuse 
consumers of MI output.

I gave this a quick thought, but reverting the change seemed like the 
most obvious solution.

But since this change affects darwin, maybe Joel has a different idea?

Re: checked in: Re: RFC: solib.c:solib_map_sections so->so_name clobbering

[Luis Machado]

On 10/26/2013 12:30 AM, Luis Machado wrote:
> Hi,
>
> It looks like this commit introduced a small regression for MI and
> shared libraries, and the testsuite does not cover this case.
>
> Suppose we have GDB running on a host and a stub/gdbserver running on a
> separate remote target. Suppose shared libraries (for symbols) on the
> target are located in a different path compared to the host, say,
> <host_path> and <target_path>.
>
> During debugging, eventually the shared libraries will be loaded and we
> used to see a shared library load notification like the following:
>
> =library-loaded,id="<target_path>/libhello.so",target-name="<target_path>/libhello.so",host-name="<host_path>/libhello.so",symbols-loaded="0",thread-group="i1"
>
>
> After this commit, this is what we see:
>
> =library-loaded,id="<target_path>/libhello.so",target-name="<target_path>/libhello.so",host-name="<target_path>/libhello.so",symbols-loaded="0",thread-group="i1"
>
>
> So it looks like we've lost information about the shared library's path
> on the host, which may not be a big deal for CLI GDB, but may confuse
> consumers of MI output.
>
> I gave this a quick thought, but reverting the change seemed like the
> most obvious solution.
>
> But since this change affects darwin, maybe Joel has a different idea?
>
>

This is the commit i was referring to in case my attempt to update the 
original thread failed:

https://sourceware.org/ml/gdb-cvs/2013-04/msg00105.html

Re: checked in: Re: RFC: solib.c:solib_map_sections so->so_name clobbering

[Joel Brobecker]

> >So it looks like we've lost information about the shared library's path
> >on the host, which may not be a big deal for CLI GDB, but may confuse
> >consumers of MI output.

Agreed.

Can you also check the output of "info shared" in GDB/CLI mode?
I suspect that you might have a change in behavior there as well.
Whether this change is actually a Good Thing or not, is unclear,
as the manual does not say whether the names are expected to be
host-side paths, or target-side paths.

> >I gave this a quick thought, but reverting the change seemed like the
> >most obvious solution.
> >
> >But since this change affects darwin, maybe Joel has a different idea?

This would also affect AIX.

It might probably need more thought, but at first sight, I am wondering
whether the real issue is that the solib backend created an entry
where "so_name" does not match the field description:

    /* Shared object file name, expanded to something GDB can open.  */

My suspicion is that the bfd_open callback takes care of the path
translation, so the backend was allowing itself to defer it. I am
not sure how difficult it would be to move that part to each backend.

Reverting the patch would be a real issue, because it would mean
that any given solib backend cannot set the so_name, and commands
such as "info shared" would print a bogus shared library name.
Nevertheless, if we did revert it, I think we can work around
the issue by using the same trick as the one we used for the 7.6
branch IIRC.

-- 
Joel

Re: checked in: Re: RFC: solib.c:solib_map_sections so->so_name clobbering

[Luis Machado]

On 10/26/2013 02:29 AM, Joel Brobecker wrote:
>>> So it looks like we've lost information about the shared library's path
>>> on the host, which may not be a big deal for CLI GDB, but may confuse
>>> consumers of MI output.
>
> Agreed.
>
> Can you also check the output of "info shared" in GDB/CLI mode?
> I suspect that you might have a change in behavior there as well.
> Whether this change is actually a Good Thing or not, is unclear,
> as the manual does not say whether the names are expected to be
> host-side paths, or target-side paths.
>

It does affect "info shared". It displays the target path instead of the 
host path now.

>>> I gave this a quick thought, but reverting the change seemed like the
>>> most obvious solution.
>>>
>>> But since this change affects darwin, maybe Joel has a different idea?
>
> This would also affect AIX.
>
> It might probably need more thought, but at first sight, I am wondering
> whether the real issue is that the solib backend created an entry
> where "so_name" does not match the field description:
>
>      /* Shared object file name, expanded to something GDB can open.  */
>

Yeah, by so_name i'd expect a canonical library name with no additional 
bits of path.

> My suspicion is that the bfd_open callback takes care of the path
> translation, so the backend was allowing itself to defer it. I am
> not sure how difficult it would be to move that part to each backend.
>
> Reverting the patch would be a real issue, because it would mean
> that any given solib backend cannot set the so_name, and commands
> such as "info shared" would print a bogus shared library name.
> Nevertheless, if we did revert it, I think we can work around
> the issue by using the same trick as the one we used for the 7.6
> branch IIRC.
>

I wouldn't say this is critical, just a slight change from an 
undocumented direction we've been following. :-)

Re: checked in: Re: RFC: solib.c:solib_map_sections so->so_name clobbering

[Joel Brobecker]

> >My suspicion is that the bfd_open callback takes care of the path
> >translation, so the backend was allowing itself to defer it. I am
> >not sure how difficult it would be to move that part to each backend.
> >
> >Reverting the patch would be a real issue, because it would mean
> >that any given solib backend cannot set the so_name, and commands
> >such as "info shared" would print a bogus shared library name.
> >Nevertheless, if we did revert it, I think we can work around
> >the issue by using the same trick as the one we used for the 7.6
> >branch IIRC.
> 
> I wouldn't say this is critical, just a slight change from an
> undocumented direction we've been following. :-)

I had the weekend to think about it some more. To me, the most
important aspect is that the output in GDB/MI is now incorrect,
not just confusing. So I think something should be done about it,
and sooner rather than later.

At the moment, the approach I dislike the least is to revert
my patch, and let the couple of solib backends (darwin, AIX)
fix up the BFD filename, the same way we did on the gdb-7.6
branch:
http://www.sourceware.org/ml/gdb-patches/2013-03/msg01084.html

This fixup is what we used to do in the past, except that we were
leaking memory. It's possible to do the same without the memory leak,
thanks to a suggestion from Tom.  It sounds contradictory to be
suggesting this, since I think this is clearly a step in the wrong
direction (making the semantics of that field a little iffy, since
time-sensitive), but seems like an acceptable compromise between amount
of work vs severity of the problem.

The alternative would be, I think, to make sure that the various
solib backends set the so_name properly. I'm not sure whether
that's actually possible. I would need to study the framework
a little longer, but lack the time at the moment.

Other thoughts/suggestions?

Thanks,
-- 
Joel

Re: checked in: Re: RFC: solib.c:solib_map_sections so->so_name clobbering

[Joel Brobecker]

I just realize I dropped the ball on this, apologies! And it affects
the 7.7 release as well. So I first started by adding this AI,
with my name attached to it, to the gdb-7.7 release wiki page.

I plan on going ahead with the proposal below as soon as I have
a moment. If there are other suggestion, please do not hesitate.

On Mon, Oct 28, 2013 at 03:39:39PM +0400, Joel Brobecker wrote:
> > >My suspicion is that the bfd_open callback takes care of the path
> > >translation, so the backend was allowing itself to defer it. I am
> > >not sure how difficult it would be to move that part to each backend.
> > >
> > >Reverting the patch would be a real issue, because it would mean
> > >that any given solib backend cannot set the so_name, and commands
> > >such as "info shared" would print a bogus shared library name.
> > >Nevertheless, if we did revert it, I think we can work around
> > >the issue by using the same trick as the one we used for the 7.6
> > >branch IIRC.
> > 
> > I wouldn't say this is critical, just a slight change from an
> > undocumented direction we've been following. :-)
> 
> I had the weekend to think about it some more. To me, the most
> important aspect is that the output in GDB/MI is now incorrect,
> not just confusing. So I think something should be done about it,
> and sooner rather than later.
> 
> At the moment, the approach I dislike the least is to revert
> my patch, and let the couple of solib backends (darwin, AIX)
> fix up the BFD filename, the same way we did on the gdb-7.6
> branch:
> http://www.sourceware.org/ml/gdb-patches/2013-03/msg01084.html
> 
> This fixup is what we used to do in the past, except that we were
> leaking memory. It's possible to do the same without the memory leak,
> thanks to a suggestion from Tom.  It sounds contradictory to be
> suggesting this, since I think this is clearly a step in the wrong
> direction (making the semantics of that field a little iffy, since
> time-sensitive), but seems like an acceptable compromise between amount
> of work vs severity of the problem.
> 
> The alternative would be, I think, to make sure that the various
> solib backends set the so_name properly. I'm not sure whether
> that's actually possible. I would need to study the framework
> a little longer, but lack the time at the moment.
> 
> Other thoughts/suggestions?

-- 
Joel

Re: checked in: Re: RFC: solib.c:solib_map_sections so->so_name clobbering

[Joel Brobecker]

[-- Attachment #1: Type: text/plain, Size: 3020 bytes --]

> I just realize I dropped the ball on this, apologies! And it affects
> the 7.7 release as well. So I first started by adding this AI,
> with my name attached to it, to the gdb-7.7 release wiki page.
> 
> I plan on going ahead with the proposal below as soon as I have
> a moment. If there are other suggestion, please do not hesitate.

Attached is the patch I just checked in.

gdb/ChangeLog:

        Revert the following commit:
        * solib.c (solib_map_sections): Remove code overwriting
        SO->SO_NAME with the bfd's filename.

        Make the following changes required after the revert above:
        * solib-aix.c (solib_aix_bfd_open): Set the filename of the
        returned bfd to a copy of the synthetic pathname.
        * solib-darwin.c (darwin_bfd_open): Set the filename of the
        returned bfd to a copy of PATHNAME.

Tested on x86_64-darwin, ppc-aix.  Pushed.

> 
> On Mon, Oct 28, 2013 at 03:39:39PM +0400, Joel Brobecker wrote:
> > > >My suspicion is that the bfd_open callback takes care of the path
> > > >translation, so the backend was allowing itself to defer it. I am
> > > >not sure how difficult it would be to move that part to each backend.
> > > >
> > > >Reverting the patch would be a real issue, because it would mean
> > > >that any given solib backend cannot set the so_name, and commands
> > > >such as "info shared" would print a bogus shared library name.
> > > >Nevertheless, if we did revert it, I think we can work around
> > > >the issue by using the same trick as the one we used for the 7.6
> > > >branch IIRC.
> > > 
> > > I wouldn't say this is critical, just a slight change from an
> > > undocumented direction we've been following. :-)
> > 
> > I had the weekend to think about it some more. To me, the most
> > important aspect is that the output in GDB/MI is now incorrect,
> > not just confusing. So I think something should be done about it,
> > and sooner rather than later.
> > 
> > At the moment, the approach I dislike the least is to revert
> > my patch, and let the couple of solib backends (darwin, AIX)
> > fix up the BFD filename, the same way we did on the gdb-7.6
> > branch:
> > http://www.sourceware.org/ml/gdb-patches/2013-03/msg01084.html
> > 
> > This fixup is what we used to do in the past, except that we were
> > leaking memory. It's possible to do the same without the memory leak,
> > thanks to a suggestion from Tom.  It sounds contradictory to be
> > suggesting this, since I think this is clearly a step in the wrong
> > direction (making the semantics of that field a little iffy, since
> > time-sensitive), but seems like an acceptable compromise between amount
> > of work vs severity of the problem.
> > 
> > The alternative would be, I think, to make sure that the various
> > solib backends set the so_name properly. I'm not sure whether
> > that's actually possible. I would need to study the framework
> > a little longer, but lack the time at the moment.
> > 
> > Other thoughts/suggestions?
> 
> -- 
> Joel

-- 
Joel

[-- Attachment #2: 0001-Revert-Do-not-overwrite-so_list-s-so_name-in-solib_m.patch --]
[-- Type: text/x-diff, Size: 4650 bytes --]

From b030cf11d6572eea467acf0ead3dad9474431033 Mon Sep 17 00:00:00 2001
From: Joel Brobecker <brobecker@adacore.com>
Date: Fri, 13 Dec 2013 18:21:37 +0100
Subject: [PATCH] Revert "Do not overwrite so_list's so_name in
 solib_map_sections"

This reverts commit 07293be44859c607a36c313e51bec2dcdcd3c243, as it
causes an unintended change of behavior with GDB/MI's =library-loaded
events: The host-name="<path>" part of the event is now showing the
target-side path instead of the host-side path.

This revert affects Darwin and AIX systems, however, where the BFD
is either artificial or icomplete, leading to the outputt of
"info shared" not containing the information we'd like. For instance,
on Darwin, we would see:

    (top-gdb) info shared
    From                To                  Syms Read   Shared Object Library
    0x00007fff8d060de4  0x00007fff8d09ce1f  Yes (*)     i386:x86-64
    0x00007fff8af08b10  0x00007fff8b1c6f73  Yes (*)     i386:x86-64

To compensate for that, we overwrite the filename of the associated bfd.

gdb/ChangeLog:

	Revert the following commit:
	* solib.c (solib_map_sections): Remove code overwriting
	SO->SO_NAME with the bfd's filename.

	Make the following changes required after the revert above:
	* solib-aix.c (solib_aix_bfd_open): Set the filename of the
	returned bfd to a copy of the synthetic pathname.
	* solib-darwin.c (darwin_bfd_open): Set the filename of the
	returned bfd to a copy of PATHNAME.
---
 gdb/ChangeLog      | 12 ++++++++++++
 gdb/solib-aix.c    | 11 +++++++++++
 gdb/solib-darwin.c | 10 ++++++++++
 gdb/solib.c        | 10 ++++++++++
 4 files changed, 43 insertions(+)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 2af71e3..b9b37b0 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,15 @@
+2013-12-15  Joel Brobecker  <brobecker@adacore.com>
+
+	Revert the following commit:
+	* solib.c (solib_map_sections): Remove code overwriting
+	SO->SO_NAME with the bfd's filename.
+
+	Make the following changes required after the revert above:
+	* solib-aix.c (solib_aix_bfd_open): Set the filename of the
+	returned bfd to a copy of the synthetic pathname.
+	* solib-darwin.c (darwin_bfd_open): Set the filename of the
+	returned bfd to a copy of PATHNAME.
+
 2013-12-13  Joel Brobecker  <brobecker@adacore.com>
 
 	* ada-lang.c (ada_array_bound_from_type): Move the declaration
diff --git a/gdb/solib-aix.c b/gdb/solib-aix.c
index 8fc516a..7bcb8ee 100644
--- a/gdb/solib-aix.c
+++ b/gdb/solib-aix.c
@@ -724,6 +724,17 @@ solib_aix_bfd_open (char *pathname)
       return NULL;
     }
 
+  /* Override the returned bfd's name with our synthetic name in order
+     to allow commands listing all shared libraries to display that
+     synthetic name.  Otherwise, we would only be displaying the name
+     of the archive member object.  */
+    {
+      char *data = bfd_alloc (object_bfd, path_len + 1);
+
+      strcpy (data, pathname);
+      object_bfd->filename = data;
+    }
+
   gdb_bfd_unref (archive_bfd);
   do_cleanups (cleanup);
   return object_bfd;
diff --git a/gdb/solib-darwin.c b/gdb/solib-darwin.c
index c37049a..4de8cb4 100644
--- a/gdb/solib-darwin.c
+++ b/gdb/solib-darwin.c
@@ -618,6 +618,16 @@ darwin_bfd_open (char *pathname)
 	     bfd_get_filename (abfd), bfd_errmsg (bfd_get_error ()));
     }
 
+  /* The current filename for fat-binary BFDs is a name generated
+     by BFD, usually a string containing the name of the architecture.
+     Reset its value to the actual filename.  */
+    {
+      char *data = bfd_alloc (res, strlen (pathname) + 1);
+
+      strcpy (data, pathname);
+      res->filename = data;
+    }
+
   gdb_bfd_unref (abfd);
   return res;
 }
diff --git a/gdb/solib.c b/gdb/solib.c
index 1a9215a..7956455 100644
--- a/gdb/solib.c
+++ b/gdb/solib.c
@@ -486,6 +486,16 @@ solib_map_sections (struct so_list *so)
   /* Leave bfd open, core_xfer_memory and "info files" need it.  */
   so->abfd = abfd;
 
+  /* Copy the full path name into so_name, allowing symbol_file_add
+     to find it later.  This also affects the =library-loaded GDB/MI
+     event, and in particular the part of that notification providing
+     the library's host-side path.  If we let the target dictate
+     that objfile's path, and the target is different from the host,
+     GDB/MI will not provide the correct host-side path.  */
+  if (strlen (bfd_get_filename (abfd)) >= SO_NAME_MAX_PATH_SIZE)
+    error (_("Shared library file name is too long."));
+  strcpy (so->so_name, bfd_get_filename (abfd));
+
   if (build_section_table (abfd, &so->sections, &so->sections_end))
     {
       error (_("Can't find the file sections in `%s': %s"),
-- 
1.8.1.2