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

[Joel Brobecker <brobecker at adacore dot com>]

> 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

>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