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