[PATCH] Ignore the first entry returned by svr4_current_sos_via_xfer_libraries for dynamically linked programs

[PATCH] Ignore the first entry returned by svr4_current_sos_via_xfer_libraries for dynamically linked programs

[Ben Cheng]

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

The first entry in the xml list returned by
svr4_current_sos_via_xfer_libraries() is the executable itself. For
dynamically linked programs, the existing code that deals with
gdbserver that doesn't support svr4_current_sos_via_xfer_libraries is
ignoring the first entry. This patch applies the same logic on the
result returned by svr4_current_sos_via_xfer_libraries().

See GDB PR/15507 for more details.

--

2013-05-28  Ben Cheng  <bccheng@android.com>

    PR gdb/15507:
    * solib-svr4.c (svr4_current_sos): Ignore the first library list
    entry returned by svr4_current_sos_via_xfer_libraries call for
    dynamically linked programs.

[-- Attachment #2: diff.txt --]
[-- Type: text/plain, Size: 656 bytes --]

Index: solib-svr4.c
===================================================================
RCS file: /cvs/src/src/gdb/solib-svr4.c,v
retrieving revision 1.178
diff -r1.178 solib-svr4.c
1285c1285,1291
<       return library_list.head ? library_list.head : svr4_default_sos ();
---
>       /* Use the same logic that determines the 'ignore_first' variable
>          below. PR - 15507 */
>       if (exec_bfd && bfd_get_section_by_name (exec_bfd, ".dynamic") == NULL)
> 	return library_list.head ? library_list.head : svr4_default_sos ();
>       else
> 	return library_list.head && library_list.head->next ?
> 	  library_list.head->next : svr4_default_sos ();

Re: [PATCH] Ignore the first entry returned by svr4_current_sos_via_xfer_libraries for dynamically linked programs

[Gary Benson]

Ben Cheng wrote:
> The first entry in the xml list returned by
> svr4_current_sos_via_xfer_libraries() is the executable itself. For
> dynamically linked programs, the existing code that deals with
> gdbserver that doesn't support svr4_current_sos_via_xfer_libraries
> is ignoring the first entry. This patch applies the same logic on
> the result returned by svr4_current_sos_via_xfer_libraries().
> 
> See GDB PR/15507 for more details.
> 
> --
> 
> 2013-05-28  Ben Cheng  <bccheng@android.com>
> 
>     PR gdb/15507:
>     * solib-svr4.c (svr4_current_sos): Ignore the first library list
>     entry returned by svr4_current_sos_via_xfer_libraries call for
>     dynamically linked programs.
> 
> Index: solib-svr4.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/solib-svr4.c,v
> retrieving revision 1.178
> diff -r1.178 solib-svr4.c
> 1285c1285,1291
> <       return library_list.head ? library_list.head : svr4_default_sos ();
> ---
> >       /* Use the same logic that determines the 'ignore_first' variable
> >          below. PR - 15507 */
> >       if (exec_bfd && bfd_get_section_by_name (exec_bfd, ".dynamic") == NULL)
> > 	return library_list.head ? library_list.head : svr4_default_sos ();
> >       else
> > 	return library_list.head && library_list.head->next ?
> > 	  library_list.head->next : svr4_default_sos ();

This seems reasonable to me, though in future please use "diff -u" to
generate patches.

Thanks,
Gary

-- 
http://gbenson.net/

Re: [PATCH] Ignore the first entry returned by svr4_current_sos_via_xfer_libraries for dynamically linked programs

[Jan Kratochvil]

Hello Ben,

the patch looks fine but I do not see GDB copyright assignment either yours or
a Google global one.  Hopefully this patch is small enough to need one.

I will check it in reformatted after possibly copyright assignment reply.


On Tue, 28 May 2013 20:31:31 +0200, Ben Cheng wrote:
> <       return library_list.head ? library_list.head : svr4_default_sos ();
> ---
> >       /* Use the same logic that determines the 'ignore_first' variable
> >          below. PR - 15507 */

There should be two spaces between . and */ so that PR could be for example in
parentheses.


> >       if (exec_bfd && bfd_get_section_by_name (exec_bfd, ".dynamic") == NULL)
> > 	return library_list.head ? library_list.head : svr4_default_sos ();
> >       else
> > 	return library_list.head && library_list.head->next ?
> > 	  library_list.head->next : svr4_default_sos ();

http://www.gnu.org/prep/standards/standards.html formatting says:
 	return (library_list.head && library_list.head->next
 	        ? library_list.head->next : svr4_default_sos ());


Thanks,
Jan

Re: [PATCH] Ignore the first entry returned by svr4_current_sos_via_xfer_libraries for dynamically linked programs

[Ben Cheng]

Hi Jan,

Hope the copyright assignment issue has been taken care now. And
thanks for taking care of the formatting changes.

Thanks,
-Ben

On Tue, Jun 4, 2013 at 1:58 AM, Jan Kratochvil
<jan.kratochvil@redhat.com> wrote:
> Hello Ben,
>
> the patch looks fine but I do not see GDB copyright assignment either yours or
> a Google global one.  Hopefully this patch is small enough to need one.
>
> I will check it in reformatted after possibly copyright assignment reply.
>
>
> On Tue, 28 May 2013 20:31:31 +0200, Ben Cheng wrote:
>> <       return library_list.head ? library_list.head : svr4_default_sos ();
>> ---
>> >       /* Use the same logic that determines the 'ignore_first' variable
>> >          below. PR - 15507 */
>
> There should be two spaces between . and */ so that PR could be for example in
> parentheses.
>
>
>> >       if (exec_bfd && bfd_get_section_by_name (exec_bfd, ".dynamic") == NULL)
>> >     return library_list.head ? library_list.head : svr4_default_sos ();
>> >       else
>> >     return library_list.head && library_list.head->next ?
>> >       library_list.head->next : svr4_default_sos ();
>
> http://www.gnu.org/prep/standards/standards.html formatting says:
>         return (library_list.head && library_list.head->next
>                 ? library_list.head->next : svr4_default_sos ());
>
>
> Thanks,
> Jan

Re: [PATCH] Ignore the first entry returned by svr4_current_sos_via_xfer_libraries for dynamically linked programs

[Jan Kratochvil]

Hi Ben,

On Tue, 28 May 2013 20:31:31 +0200, Ben Cheng wrote:
> The first entry in the xml list returned by
> svr4_current_sos_via_xfer_libraries() is the executable itself.
[...]
http://sourceware.org/bugzilla/show_bug.cgi?id=15507
> Currently Android is using gdbserver from 7.1 and gdb from 7.3.
[...]
> <library-list-svr4 version="1.0"><library name="<test_program>"
> lm="0xb6fed108" l_addr="0x0" l_ld="0x0"/><library name="/system/bin/linker"

in fact this GDB patch should be dropped.

Bug is in the Android gdbsever implementation.  FSF gdbserver 7.1 did not
support <library-list-svr4/> at all, this feature is in gdbserver 7.4+ only.

Main executable should not have an entry, it has only "main-lm" attribute:
	info '(gdb)Library List Format for SVR4 Targets'
	Additionally the single `main-lm' attribute specifies address of
	`struct link_map' used for the main executable.

GNU/Linux FSF gdbserver-HEAD list looks like:

Sending packet: $qXfer:libraries-svr4:read::0,fff#91...Packet received: l<library-list-svr4 version="1.0" main-lm="0x7ffff7ffe208"><library name="/home/jkratoch/redhat/gdb-clean/gdb/testsuite/gdb.base/gcore-relro-lib.so" lm="0x7ffff7ff97f0" l_addr="0x7ffff7bda000" l_ld="0x7ffff7ddadf8"/><library name="/lib64/libm.so.6" lm="0x7ffff7fe8000" l_addr="0x7ffff78d8000" l_ld="0x7ffff7bd8da8"/><library name="/lib64/libc.so.6" lm="0x7ffff7fe84c8" l_addr="0x7ffff7518000" l_ld="0x7ffff78d0b80"/><library name="/lib64/ld-linux-x86-64.so.2" lm="0x7ffff7ffd998" l_addr="0x7ffff7ddc000" l_ld="0x7ffff7ffcdf0"/></library-list-svr4>^M


Regards,
Jan

Re: [PATCH] Ignore the first entry returned by svr4_current_sos_via_xfer_libraries for dynamically linked programs

[Ben Cheng]

Hmm this problem only shows up after I refresh gdbserver from the
unmodified gdb 7.6 tree. As you mentioned, gdb 7.1 did not support
library-list-svr4, so when I paired gdbserver 7.1 with gdb 7.6 they
also work fine.

On Tue, Jun 4, 2013 at 11:07 AM, Jan Kratochvil
<jan.kratochvil@redhat.com> wrote:
> Hi Ben,
>
> On Tue, 28 May 2013 20:31:31 +0200, Ben Cheng wrote:
>> The first entry in the xml list returned by
>> svr4_current_sos_via_xfer_libraries() is the executable itself.
> [...]
> http://sourceware.org/bugzilla/show_bug.cgi?id=15507
>> Currently Android is using gdbserver from 7.1 and gdb from 7.3.
> [...]
>> <library-list-svr4 version="1.0"><library name="<test_program>"
>> lm="0xb6fed108" l_addr="0x0" l_ld="0x0"/><library name="/system/bin/linker"
>
> in fact this GDB patch should be dropped.
>
> Bug is in the Android gdbsever implementation.  FSF gdbserver 7.1 did not
> support <library-list-svr4/> at all, this feature is in gdbserver 7.4+ only.
>
> Main executable should not have an entry, it has only "main-lm" attribute:
>         info '(gdb)Library List Format for SVR4 Targets'
>         Additionally the single `main-lm' attribute specifies address of
>         `struct link_map' used for the main executable.
>
> GNU/Linux FSF gdbserver-HEAD list looks like:
>
> Sending packet: $qXfer:libraries-svr4:read::0,fff#91...Packet received: l<library-list-svr4 version="1.0" main-lm="0x7ffff7ffe208"><library name="/home/jkratoch/redhat/gdb-clean/gdb/testsuite/gdb.base/gcore-relro-lib.so" lm="0x7ffff7ff97f0" l_addr="0x7ffff7bda000" l_ld="0x7ffff7ddadf8"/><library name="/lib64/libm.so.6" lm="0x7ffff7fe8000" l_addr="0x7ffff78d8000" l_ld="0x7ffff7bd8da8"/><library name="/lib64/libc.so.6" lm="0x7ffff7fe84c8" l_addr="0x7ffff7518000" l_ld="0x7ffff78d0b80"/><library name="/lib64/ld-linux-x86-64.so.2" lm="0x7ffff7ffd998" l_addr="0x7ffff7ddc000" l_ld="0x7ffff7ffcdf0"/></library-list-svr4>^M
>
>
> Regards,
> Jan

Re: [PATCH] Ignore the first entry returned by svr4_current_sos_via_xfer_libraries for dynamically linked programs

[Jan Kratochvil]

On Tue, 04 Jun 2013 20:18:10 +0200, Ben Cheng wrote:
> Hmm this problem only shows up after I refresh gdbserver from the
> unmodified gdb 7.6 tree. As you mentioned, gdb 7.1 did not support
> library-list-svr4, so when I paired gdbserver 7.1 with gdb 7.6 they
> also work fine.

In such case there is a bug in FSF gdbserver as it ignores only entries with
name "".  Normal glibc uses name "" for the first entry for the executable but
Android Bionic apparently uses "<test_program>" as the first entry instead.

The bug is in gdbserver/linux-low.c linux_qxfer_libraries_svr4() which should
always ignore the first entry, even if it is not "".

The test IGNORE_FIRST does not need to be done there.  The check
  /* Assume that everything is a library if the dynamic loader was loaded
     late by a static executable.  */
  if (exec_bfd && bfd_get_section_by_name (exec_bfd, ".dynamic") == NULL)

is in fact already done by gdbserver/linux-low.c get_dynamic(): If PT_DYNAMIC
(".dynamic") is not found gdbserver will never send <library-list-svr4/> and
solib-svr4.c falls back to the memory reads where it copes with static
executables fine.


Thanks,
Jan

Re: [PATCH] Ignore the first entry returned by svr4_current_sos_via_xfer_libraries for dynamically linked programs

[Ben Cheng]

Perfect timing. :) I was playing with Android's dynamic linker and
fount out that after setting the l_name field to NULL for the first
entry gdb/gdbserver 7.6 start to work on Android.

diff --git a/linker/linker.cpp b/linker/linker.cpp
index c97b712..ddc16d8 100644
--- a/linker/linker.cpp
+++ b/linker/linker.cpp
@@ -1644,7 +1644,8 @@ static Elf32_Addr __linker_init_post_relocation(KernelArgu
     link_map_t* map = &(si->link_map);

     map->l_addr = 0;
-    map->l_name = args.argv[0];
+    //map->l_name = args.argv[0];
+    map->l_name = NULL;
     map->l_prev = NULL;
     map->l_next = NULL;

Thanks,
-Ben

On Tue, Jun 4, 2013 at 1:23 PM, Jan Kratochvil
<jan.kratochvil@redhat.com> wrote:
> On Tue, 04 Jun 2013 20:18:10 +0200, Ben Cheng wrote:
>> Hmm this problem only shows up after I refresh gdbserver from the
>> unmodified gdb 7.6 tree. As you mentioned, gdb 7.1 did not support
>> library-list-svr4, so when I paired gdbserver 7.1 with gdb 7.6 they
>> also work fine.
>
> In such case there is a bug in FSF gdbserver as it ignores only entries with
> name "".  Normal glibc uses name "" for the first entry for the executable but
> Android Bionic apparently uses "<test_program>" as the first entry instead.
>
> The bug is in gdbserver/linux-low.c linux_qxfer_libraries_svr4() which should
> always ignore the first entry, even if it is not "".
>
> The test IGNORE_FIRST does not need to be done there.  The check
>   /* Assume that everything is a library if the dynamic loader was loaded
>      late by a static executable.  */
>   if (exec_bfd && bfd_get_section_by_name (exec_bfd, ".dynamic") == NULL)
>
> is in fact already done by gdbserver/linux-low.c get_dynamic(): If PT_DYNAMIC
> (".dynamic") is not found gdbserver will never send <library-list-svr4/> and
> solib-svr4.c falls back to the memory reads where it copes with static
> executables fine.
>
>
> Thanks,
> Jan

Re: [PATCH] Ignore the first entry returned by svr4_current_sos_via_xfer_libraries for dynamically linked programs

[Ben Cheng]

And it looks like reversing the order of checks for lm_prev and
libname in gdbserver/linux_qxfer_libraries_svr4:linux_qxfer_libraries_svr4()
is a possible fix for this problem.

On Tue, Jun 4, 2013 at 1:36 PM, Ben Cheng <bccheng@google.com> wrote:
> Perfect timing. :) I was playing with Android's dynamic linker and
> fount out that after setting the l_name field to NULL for the first
> entry gdb/gdbserver 7.6 start to work on Android.
>
> diff --git a/linker/linker.cpp b/linker/linker.cpp
> index c97b712..ddc16d8 100644
> --- a/linker/linker.cpp
> +++ b/linker/linker.cpp
> @@ -1644,7 +1644,8 @@ static Elf32_Addr __linker_init_post_relocation(KernelArgu
>      link_map_t* map = &(si->link_map);
>
>      map->l_addr = 0;
> -    map->l_name = args.argv[0];
> +    //map->l_name = args.argv[0];
> +    map->l_name = NULL;
>      map->l_prev = NULL;
>      map->l_next = NULL;
>
> Thanks,
> -Ben
>
> On Tue, Jun 4, 2013 at 1:23 PM, Jan Kratochvil
> <jan.kratochvil@redhat.com> wrote:
>> On Tue, 04 Jun 2013 20:18:10 +0200, Ben Cheng wrote:
>>> Hmm this problem only shows up after I refresh gdbserver from the
>>> unmodified gdb 7.6 tree. As you mentioned, gdb 7.1 did not support
>>> library-list-svr4, so when I paired gdbserver 7.1 with gdb 7.6 they
>>> also work fine.
>>
>> In such case there is a bug in FSF gdbserver as it ignores only entries with
>> name "".  Normal glibc uses name "" for the first entry for the executable but
>> Android Bionic apparently uses "<test_program>" as the first entry instead.
>>
>> The bug is in gdbserver/linux-low.c linux_qxfer_libraries_svr4() which should
>> always ignore the first entry, even if it is not "".
>>
>> The test IGNORE_FIRST does not need to be done there.  The check
>>   /* Assume that everything is a library if the dynamic loader was loaded
>>      late by a static executable.  */
>>   if (exec_bfd && bfd_get_section_by_name (exec_bfd, ".dynamic") == NULL)
>>
>> is in fact already done by gdbserver/linux-low.c get_dynamic(): If PT_DYNAMIC
>> (".dynamic") is not found gdbserver will never send <library-list-svr4/> and
>> solib-svr4.c falls back to the memory reads where it copes with static
>> executables fine.
>>
>>
>> Thanks,
>> Jan

Re: [PATCH] Ignore the first entry returned by svr4_current_sos_via_xfer_libraries for dynamically linked programs

[Jan Kratochvil]

On Tue, 04 Jun 2013 22:45:22 +0200, Ben Cheng wrote:
> And it looks like reversing the order of checks for lm_prev and
> libname in gdbserver/linux_qxfer_libraries_svr4:linux_qxfer_libraries_svr4()
> is a possible fix for this problem.

I agree.


Jan

Re: [PATCH] Ignore the first entry returned by svr4_current_sos_via_xfer_libraries for dynamically linked programs

[Ben Cheng]

Hi Jan,

If you don't mind I'll let you take care of it. :)

Thanks,
-Ben

On Tue, Jun 4, 2013 at 1:57 PM, Jan Kratochvil
<jan.kratochvil@redhat.com> wrote:
> On Tue, 04 Jun 2013 22:45:22 +0200, Ben Cheng wrote:
>> And it looks like reversing the order of checks for lm_prev and
>> libname in gdbserver/linux_qxfer_libraries_svr4:linux_qxfer_libraries_svr4()
>> is a possible fix for this problem.
>
> I agree.
>
>
> Jan

[patchv2] Ignore the first entry returned by svr4_current_sos_via_xfer_libraries for dynamically linked programs

[Jan Kratochvil]

On Tue, 04 Jun 2013 23:21:36 +0200, Ben Cheng wrote:
> If you don't mind I'll let you take care of it. :)

I will check it in in some time.


Regards,
Jan


gdb/gdbserver/
2013-06-05  Jan Kratochvil  <jan.kratochvil@redhat.com>

	Fix compatibility with Android Bionic.
	* linux-low.c (linux_qxfer_libraries_svr4): Ignore first entry even if
	it is not empty.

diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
index 03ac469..0f3e0f0 100644
--- a/gdb/gdbserver/linux-low.c
+++ b/gdb/gdbserver/linux-low.c
@@ -5829,45 +5829,54 @@ linux_qxfer_libraries_svr4 (const char *annex, unsigned char *readbuf,
 	  break;
 	}
 
-      /* Not checking for error because reading may stop before
-	 we've got PATH_MAX worth of characters.  */
-      libname[0] = '\0';
-      linux_read_memory (l_name, libname, sizeof (libname) - 1);
-      libname[sizeof (libname) - 1] = '\0';
-      if (libname[0] != '\0')
+      /* Ignore the first entry even if it has valid name as the first entry
+	 corresponds to the main executable.  The first entry should not be
+	 skipped if the dynamic loader was loaded late by a static executable
+	 (see solib-svr4.c parameter ignore_first).  But in such case the main
+	 executable does not have PT_DYNAMIC present and this function already
+	 exited above due to failed get_r_debug.  */
+      if (lm_prev == 0)
 	{
-	  /* 6x the size for xml_escape_text below.  */
-	  size_t len = 6 * strlen ((char *) libname);
-	  char *name;
-
-	  if (!header_done)
+	  sprintf (p, " main-lm=\"0x%lx\"", (unsigned long) lm_addr);
+	  p = p + strlen (p);
+	}
+      else
+	{
+	  /* Not checking for error because reading may stop before
+	     we've got PATH_MAX worth of characters.  */
+	  libname[0] = '\0';
+	  linux_read_memory (l_name, libname, sizeof (libname) - 1);
+	  libname[sizeof (libname) - 1] = '\0';
+	  if (libname[0] != '\0')
 	    {
-	      /* Terminate `<library-list-svr4'.  */
-	      *p++ = '>';
-	      header_done = 1;
-	    }
+	      /* 6x the size for xml_escape_text below.  */
+	      size_t len = 6 * strlen ((char *) libname);
+	      char *name;
 
-	  while (allocated < p - document + len + 200)
-	    {
-	      /* Expand to guarantee sufficient storage.  */
-	      uintptr_t document_len = p - document;
+	      if (!header_done)
+		{
+		  /* Terminate `<library-list-svr4'.  */
+		  *p++ = '>';
+		  header_done = 1;
+		}
 
-	      document = xrealloc (document, 2 * allocated);
-	      allocated *= 2;
-	      p = document + document_len;
-	    }
+	      while (allocated < p - document + len + 200)
+		{
+		  /* Expand to guarantee sufficient storage.  */
+		  uintptr_t document_len = p - document;
 
-	  name = xml_escape_text ((char *) libname);
-	  p += sprintf (p, "<library name=\"%s\" lm=\"0x%lx\" "
-			"l_addr=\"0x%lx\" l_ld=\"0x%lx\"/>",
-			name, (unsigned long) lm_addr,
-			(unsigned long) l_addr, (unsigned long) l_ld);
-	  free (name);
-	}
-      else if (lm_prev == 0)
-	{
-	  sprintf (p, " main-lm=\"0x%lx\"", (unsigned long) lm_addr);
-	  p = p + strlen (p);
+		  document = xrealloc (document, 2 * allocated);
+		  allocated *= 2;
+		  p = document + document_len;
+		}
+
+	      name = xml_escape_text ((char *) libname);
+	      p += sprintf (p, "<library name=\"%s\" lm=\"0x%lx\" "
+			    "l_addr=\"0x%lx\" l_ld=\"0x%lx\"/>",
+			    name, (unsigned long) lm_addr,
+			    (unsigned long) l_addr, (unsigned long) l_ld);
+	      free (name);
+	    }
 	}
 
       lm_prev = lm_addr;

[commit] [patchv2] Ignore the first entry returned by svr4_current_sos_via_xfer_libraries for dynamically linked programs

[Jan Kratochvil]

On Wed, 05 Jun 2013 16:34:03 +0200, Pedro Alves wrote:
> On 06/05/2013 12:38 PM, Jan Kratochvil wrote:
> > 	Fix compatibility with Android Bionic.
> > 	* linux-low.c (linux_qxfer_libraries_svr4): Ignore first entry even if
> > 	it is not empty.
> 
> Looks right to me.

Checked in:
	http://sourceware.org/ml/gdb-cvs/2013-06/msg00065.html


Thanks,
Jan

Re: [commit] [patchv2] Ignore the first entry returned by svr4_current_sos_via_xfer_libraries for dynamically linked programs

[Ben Cheng]

Awesome thanks!

On Sun, Jun 9, 2013 at 10:09 AM, Jan Kratochvil
<jan.kratochvil@redhat.com> wrote:
> On Wed, 05 Jun 2013 16:34:03 +0200, Pedro Alves wrote:
>> On 06/05/2013 12:38 PM, Jan Kratochvil wrote:
>> >     Fix compatibility with Android Bionic.
>> >     * linux-low.c (linux_qxfer_libraries_svr4): Ignore first entry even if
>> >     it is not empty.
>>
>> Looks right to me.
>
> Checked in:
>         http://sourceware.org/ml/gdb-cvs/2013-06/msg00065.html
>
>
> Thanks,
> Jan