Patchwork [1/2] Look for separate debug files in debug directories under a sysroot.

login
register
mail settings
Submitter Simon Marchi
Date Jan. 26, 2019, 5:12 p.m.
Message ID <96b4fb01-d703-ad1b-5ea5-d7e5366f5f6b@polymtl.ca>
Download mbox | patch
Permalink /patch/31218/
State New
Headers show

Comments

Simon Marchi - Jan. 26, 2019, 5:12 p.m.
On 2019-01-26 12:16 a.m., Simon Marchi wrote:
> As discussed on IRC, we should probably add the same behavior for 
> build-id-based separate debug files.

I gave a shot to this, here's what I have:

From 377ef615ad6e2c57966c8853c61cbce95ea6c2b3 Mon Sep 17 00:00:00 2001
From: Simon Marchi <simon.marchi@polymtl.ca>
Date: Sat, 26 Jan 2019 11:34:45 -0500
Subject: [PATCH] Look for build-id-based separate debug files under the
 sysroot

When looking for a separate debug file that matches a given build-id,
GDB only looks in the host's debug dir (typically /usr/lib/debug).  This
patch makes it look in the sysroot as well.  This is to match the
behavior of GDB when using debuglink-based separate debug files.

In the following example, my sysroot is "/tmp/sysroot" and I am trying
to load symbols for
/tmp/sysroot/usr/lib/arm-linux-gnueabihf/gconv/EBCDIC-AT-DE.so.  This is
the current behavior:

    (gdb) file /tmp/sysroot/usr/lib/arm-linux-gnueabihf/gconv/EBCDIC-AT-DE.so
    Reading symbols from /tmp/sysroot/usr/lib/arm-linux-gnueabihf/gconv/EBCDIC-AT-DE.so...

    Looking for separate debug info (build-id) for /tmp/sysroot/usr/lib/arm-linux-gnueabihf/gconv/EBCDIC-AT-DE.so
      Trying /usr/lib/debug/.build-id/f3/d6594d2600e985812cd4ba2ad083ac2aceae22.debug... no, unable to compute real path

    <snip>
    (No debugging symbols found in /tmp/sysroot/usr/lib/arm-linux-gnueabihf/gconv/EBCDIC-AT-DE.so)

With this patch:

    (gdb) file /tmp/sysroot/usr/lib/arm-linux-gnueabihf/gconv/EBCDIC-AT-DE.so
    Reading symbols from /tmp/sysroot/usr/lib/arm-linux-gnueabihf/gconv/EBCDIC-AT-DE.so...

    Looking for separate debug info (build-id) for /tmp/sysroot/usr/lib/arm-linux-gnueabihf/gconv/EBCDIC-AT-DE.so
      Trying /usr/lib/debug/.build-id/f3/d6594d2600e985812cd4ba2ad083ac2aceae22.debug... no, unable to compute real path
      Trying /tmp/sysroot/usr/lib/debug/.build-id/f3/d6594d2600e985812cd4ba2ad083ac2aceae22.debug... yes!
    Reading symbols from /tmp/sysroot/usr/lib/debug/.build-id/f3/d6594d2600e985812cd4ba2ad083ac2aceae22.debug...

In the original code, there is a suspicious "abfd.release ()" in
build_id_to_debug_bfd, that I don't understand.  If a file with the
right name exists but its build-id note doesn't match, we release (leak)
our reference, meaning the file will stay open?  I removed it in the new
code, so that the reference is dropped if we end up not using that file.
I tested briefly by corrupting a separate debug file to trigger this
code, nothing exploded.

gdb/ChangeLog:

	* build-id.c (build_id_to_debug_bfd_1): New function.
	(build_id_to_debug_bfd): Look for separate debug file in
	sysroot.
---
 gdb/build-id.c | 115 +++++++++++++++++++++++++++++++------------------
 1 file changed, 72 insertions(+), 43 deletions(-)
John Baldwin - Jan. 28, 2019, 6:53 p.m.
On 1/26/19 9:12 AM, Simon Marchi wrote:
> On 2019-01-26 12:16 a.m., Simon Marchi wrote:
>> As discussed on IRC, we should probably add the same behavior for 
>> build-id-based separate debug files.
> 
> I gave a shot to this, here's what I have:
> 
> From 377ef615ad6e2c57966c8853c61cbce95ea6c2b3 Mon Sep 17 00:00:00 2001
> From: Simon Marchi <simon.marchi@polymtl.ca>
> Date: Sat, 26 Jan 2019 11:34:45 -0500
> Subject: [PATCH] Look for build-id-based separate debug files under the
>  sysroot
> 
> When looking for a separate debug file that matches a given build-id,
> GDB only looks in the host's debug dir (typically /usr/lib/debug).  This
> patch makes it look in the sysroot as well.  This is to match the
> behavior of GDB when using debuglink-based separate debug files.
> 
> In the following example, my sysroot is "/tmp/sysroot" and I am trying
> to load symbols for
> /tmp/sysroot/usr/lib/arm-linux-gnueabihf/gconv/EBCDIC-AT-DE.so.  This is
> the current behavior:
> 
>     (gdb) file /tmp/sysroot/usr/lib/arm-linux-gnueabihf/gconv/EBCDIC-AT-DE.so
>     Reading symbols from /tmp/sysroot/usr/lib/arm-linux-gnueabihf/gconv/EBCDIC-AT-DE.so...
> 
>     Looking for separate debug info (build-id) for /tmp/sysroot/usr/lib/arm-linux-gnueabihf/gconv/EBCDIC-AT-DE.so
>       Trying /usr/lib/debug/.build-id/f3/d6594d2600e985812cd4ba2ad083ac2aceae22.debug... no, unable to compute real path
> 
>     <snip>
>     (No debugging symbols found in /tmp/sysroot/usr/lib/arm-linux-gnueabihf/gconv/EBCDIC-AT-DE.so)
> 
> With this patch:
> 
>     (gdb) file /tmp/sysroot/usr/lib/arm-linux-gnueabihf/gconv/EBCDIC-AT-DE.so
>     Reading symbols from /tmp/sysroot/usr/lib/arm-linux-gnueabihf/gconv/EBCDIC-AT-DE.so...
> 
>     Looking for separate debug info (build-id) for /tmp/sysroot/usr/lib/arm-linux-gnueabihf/gconv/EBCDIC-AT-DE.so
>       Trying /usr/lib/debug/.build-id/f3/d6594d2600e985812cd4ba2ad083ac2aceae22.debug... no, unable to compute real path
>       Trying /tmp/sysroot/usr/lib/debug/.build-id/f3/d6594d2600e985812cd4ba2ad083ac2aceae22.debug... yes!
>     Reading symbols from /tmp/sysroot/usr/lib/debug/.build-id/f3/d6594d2600e985812cd4ba2ad083ac2aceae22.debug...
> 
> In the original code, there is a suspicious "abfd.release ()" in
> build_id_to_debug_bfd, that I don't understand.  If a file with the
> right name exists but its build-id note doesn't match, we release (leak)
> our reference, meaning the file will stay open?  I removed it in the new
> code, so that the reference is dropped if we end up not using that file.
> I tested briefly by corrupting a separate debug file to trigger this
> code, nothing exploded.

I think this looks good to me.  I think the .release () should have been
abfd.reset (nullptr) instead as you noted.  I think this isn't the first
time we've seen release () used when reset (nullptr) should have been used
instead unfortunately.  The name of that method is a bit ambiguous
unfortunately.  Do you want me to pull this into my series or just push it
after I push the final version of mine?
Simon Marchi - Feb. 22, 2019, 8:51 p.m.
On 2019-01-28 1:53 p.m., John Baldwin wrote:
> On 1/26/19 9:12 AM, Simon Marchi wrote:

>> On 2019-01-26 12:16 a.m., Simon Marchi wrote:

>>> As discussed on IRC, we should probably add the same behavior for 

>>> build-id-based separate debug files.

>>

>> I gave a shot to this, here's what I have:

>>

>> From 377ef615ad6e2c57966c8853c61cbce95ea6c2b3 Mon Sep 17 00:00:00 2001

>> From: Simon Marchi <simon.marchi@polymtl.ca>

>> Date: Sat, 26 Jan 2019 11:34:45 -0500

>> Subject: [PATCH] Look for build-id-based separate debug files under the

>>  sysroot

>>

>> When looking for a separate debug file that matches a given build-id,

>> GDB only looks in the host's debug dir (typically /usr/lib/debug).  This

>> patch makes it look in the sysroot as well.  This is to match the

>> behavior of GDB when using debuglink-based separate debug files.

>>

>> In the following example, my sysroot is "/tmp/sysroot" and I am trying

>> to load symbols for

>> /tmp/sysroot/usr/lib/arm-linux-gnueabihf/gconv/EBCDIC-AT-DE.so.  This is

>> the current behavior:

>>

>>     (gdb) file /tmp/sysroot/usr/lib/arm-linux-gnueabihf/gconv/EBCDIC-AT-DE.so

>>     Reading symbols from /tmp/sysroot/usr/lib/arm-linux-gnueabihf/gconv/EBCDIC-AT-DE.so...

>>

>>     Looking for separate debug info (build-id) for /tmp/sysroot/usr/lib/arm-linux-gnueabihf/gconv/EBCDIC-AT-DE.so

>>       Trying /usr/lib/debug/.build-id/f3/d6594d2600e985812cd4ba2ad083ac2aceae22.debug... no, unable to compute real path

>>

>>     <snip>

>>     (No debugging symbols found in /tmp/sysroot/usr/lib/arm-linux-gnueabihf/gconv/EBCDIC-AT-DE.so)

>>

>> With this patch:

>>

>>     (gdb) file /tmp/sysroot/usr/lib/arm-linux-gnueabihf/gconv/EBCDIC-AT-DE.so

>>     Reading symbols from /tmp/sysroot/usr/lib/arm-linux-gnueabihf/gconv/EBCDIC-AT-DE.so...

>>

>>     Looking for separate debug info (build-id) for /tmp/sysroot/usr/lib/arm-linux-gnueabihf/gconv/EBCDIC-AT-DE.so

>>       Trying /usr/lib/debug/.build-id/f3/d6594d2600e985812cd4ba2ad083ac2aceae22.debug... no, unable to compute real path

>>       Trying /tmp/sysroot/usr/lib/debug/.build-id/f3/d6594d2600e985812cd4ba2ad083ac2aceae22.debug... yes!

>>     Reading symbols from /tmp/sysroot/usr/lib/debug/.build-id/f3/d6594d2600e985812cd4ba2ad083ac2aceae22.debug...

>>

>> In the original code, there is a suspicious "abfd.release ()" in

>> build_id_to_debug_bfd, that I don't understand.  If a file with the

>> right name exists but its build-id note doesn't match, we release (leak)

>> our reference, meaning the file will stay open?  I removed it in the new

>> code, so that the reference is dropped if we end up not using that file.

>> I tested briefly by corrupting a separate debug file to trigger this

>> code, nothing exploded.

> 

> I think this looks good to me.  I think the .release () should have been

> abfd.reset (nullptr) instead as you noted.  I think this isn't the first

> time we've seen release () used when reset (nullptr) should have been used

> instead unfortunately.  The name of that method is a bit ambiguous

> unfortunately.  Do you want me to pull this into my series or just push it

> after I push the final version of mine?


Thanks, this is now pushed.

Simon

Patch

diff --git a/gdb/build-id.c b/gdb/build-id.c
index e0c35579cd4a..27f29cd04423 100644
--- a/gdb/build-id.c
+++ b/gdb/build-id.c
@@ -65,13 +65,63 @@  build_id_verify (bfd *abfd, size_t check_len, const bfd_byte *check)
   return retval;
 }

+/* Helper for build_id_to_debug_bfd.  LINK is a path to a potential
+   build-id-based separate debug file, potentially a symlink to the real file.
+   If the file exists and matches BUILD_ID, return a BFD reference to it.  */
+
+static gdb_bfd_ref_ptr
+build_id_to_debug_bfd_1 (const std::string &link, size_t build_id_len,
+			 const bfd_byte *build_id)
+{
+  if (separate_debug_file_debug)
+    {
+      printf_unfiltered (_("  Trying %s..."), link.c_str ());
+      gdb_flush (gdb_stdout);
+    }
+
+  /* lrealpath() is expensive even for the usually non-existent files.  */
+  gdb::unique_xmalloc_ptr<char> filename;
+  if (access (link.c_str (), F_OK) == 0)
+    filename.reset (lrealpath (link.c_str ()));
+
+  if (filename == NULL)
+    {
+      if (separate_debug_file_debug)
+	printf_unfiltered (_(" no, unable to compute real path\n"));
+
+      return {};
+    }
+
+  /* We expect to be silent on the non-existing files.  */
+  gdb_bfd_ref_ptr debug_bfd = gdb_bfd_open (filename.get (), gnutarget, -1);
+
+  if (debug_bfd == NULL)
+    {
+      if (separate_debug_file_debug)
+	printf_unfiltered (_(" no, unable to open.\n"));
+
+      return {};
+    }
+
+  if (!build_id_verify (debug_bfd.get(), build_id_len, build_id))
+    {
+      if (separate_debug_file_debug)
+	printf_unfiltered (_(" no, build-id does not match.\n"));
+
+      return {};
+    }
+
+  if (separate_debug_file_debug)
+    printf_unfiltered (_(" yes!\n"));
+
+  return debug_bfd;
+}
+
 /* See build-id.h.  */

 gdb_bfd_ref_ptr
 build_id_to_debug_bfd (size_t build_id_len, const bfd_byte *build_id)
 {
-  gdb_bfd_ref_ptr abfd;
-
   /* Keep backward compatibility so that DEBUG_FILE_DIRECTORY being "" will
      cause "/.build-id/..." lookups.  */

@@ -83,6 +133,10 @@  build_id_to_debug_bfd (size_t build_id_len, const bfd_byte *build_id)
       const gdb_byte *data = build_id;
       size_t size = build_id_len;

+      /* Compute where the file named after the build-id would be.
+
+	 If debugdir is "/usr/lib/debug" and the build-id is abcdef, this will
+         give "/usr/lib/debug/.build-id/ab/cdef.debug".  */
       std::string link = debugdir.get ();
       link += "/.build-id/";

@@ -97,53 +151,28 @@  build_id_to_debug_bfd (size_t build_id_len, const bfd_byte *build_id)

       link += ".debug";

-      if (separate_debug_file_debug)
-	{
-	  printf_unfiltered (_("  Trying %s..."), link.c_str ());
-	  gdb_flush (gdb_stdout);
-	}
+      gdb_bfd_ref_ptr debug_bfd
+	= build_id_to_debug_bfd_1 (link, build_id_len, build_id);
+      if (debug_bfd != NULL)
+	return debug_bfd;

-      /* lrealpath() is expensive even for the usually non-existent files.  */
-      gdb::unique_xmalloc_ptr<char> filename;
-      if (access (link.c_str (), F_OK) == 0)
-	filename.reset (lrealpath (link.c_str ()));
+      /* Try to look under the sysroot as well.  If the sysroot is
+         "/the/sysroot", it will give
+         "/the/sysroot/usr/lib/debug/.build-id/ab/cdef.debug".

-      if (filename == NULL)
+         Don't do it if the sysroot is the target system ("target:").  It
+         could work in theory, but the lrealpath in build_id_to_debug_bfd_1
+         only works with local paths.  */
+      if (strcmp (gdb_sysroot, TARGET_SYSROOT_PREFIX) != 0)
 	{
-	  if (separate_debug_file_debug)
-	    printf_unfiltered (_(" no, unable to compute real path\n"));
-
-	  continue;
+	  link = gdb_sysroot + link;
+	  debug_bfd = build_id_to_debug_bfd_1 (link, build_id_len, build_id);
+	  if (debug_bfd != NULL)
+	    return debug_bfd;
 	}
-
-      /* We expect to be silent on the non-existing files.  */
-      abfd = gdb_bfd_open (filename.get (), gnutarget, -1);
-
-      if (abfd == NULL)
-	{
-	  if (separate_debug_file_debug)
-	    printf_unfiltered (_(" no, unable to open.\n"));
-
-	  continue;
-	}
-
-      if (build_id_verify (abfd.get(), build_id_len, build_id))
-	{
-	  if (separate_debug_file_debug)
-	    printf_unfiltered (_(" yes!\n"));
-
-	  break;
-	}
-      else
-	{
-	  if (separate_debug_file_debug)
-	    printf_unfiltered (_(" no, build-id does not match.\n"));
-	}
-
-      abfd.release ();
     }

-  return abfd;
+  return {};
 }

 /* See build-id.h.  */