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

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

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