[PATCH 0/2] Some fixes for debug files and sysroots

[PATCH 0/2] Some fixes for debug files and sysroots

[John Baldwin]

When working on non-x86 architectures, I often install a base system
to a sysroot directory that I then use to generate a filesystem for
use with qemu using a tool like makefs.  Since this sysroot is a full
system install, it also contains a nested global debugfile directory
for base system libraries and binaries (e.g.
/sysroot/usr/lib/debug/bin/ls.debug for /sysroot/bin/ls).  GDB's
current logic for handling separate debug files does not look for
debug files in debug directories under a sysroot.  These two patches
seek to make this work seamlessly (my current workaround is to go
create a symlink from /usr/lib/debug/sysroot to /sysroot/usr/lib/debug
on the host and these patches remove the need for that).

The first patch augments the existing logic to add another path check
for <sysroot>/<debug_dir>/<debugfile>.  The second patch is a
convenience to deal with tab completion of directories passed to 'set
sysroot' usually adding a trailing / that then breaks the logic for
separate debug files.  Rather than trying to cope with a possible
trailing separator in find_separate_debug_file, I instead chose to
"fix" the sysroot to when it is set to remove the trailing /.  One
downside of this is that it doesn't fix a static sysroot set at build
time with --sysroot.

John Baldwin (2):
  Look for separate debug files in debug directories under a sysroot.
  Trim trailing directory separators from new sysroots.

 gdb/ChangeLog | 10 ++++++++++
 gdb/solib.c   |  8 ++++++++
 gdb/symfile.c | 19 +++++++++++++++++++
 3 files changed, 37 insertions(+)

-- 
2.19.2

[PATCH 2/2] Trim trailing directory separators from new sysroots.

[John Baldwin]

The separate debugfile logic assumes that the sysroot does not include
a trailing separator (all of the sysroot logic in
find_separate_debug_file requires the next character after the sysroot
in a object file path to be a directory separator).  However, normal
filename completion will result in setting a sysroot with a trailing
directory separator.  If the directory portion of a new sysroot ends
with a directory separator and is not specifying the root directory,
trim the trailing separator.  This permits the sysroot logic to work
the same if a sysroot of "/myroot/" is specified instead of "/myroot".

Note that the sysroot displayed by 'show sysroot' will reflect the
trimmed sysroot.

gdb/ChangeLog:

	* solib.c (gdb_sysroot_changed): Trim trailing directory separator
	from new sysroot.
---
 gdb/ChangeLog | 5 +++++
 gdb/solib.c   | 8 ++++++++
 2 files changed, 13 insertions(+)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index a401cfc784..c9f2d049bc 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,8 @@
+2019-01-22  John Baldwin  <jhb@FreeBSD.org>
+
+	* solib.c (gdb_sysroot_changed): Trim trailing directory separator
+	from new sysroot.
+
 2019-01-22  John Baldwin  <jhb@FreeBSD.org>
 
 	* symfile.c (find_separate_debug_file): Look for separate debug
diff --git a/gdb/solib.c b/gdb/solib.c
index 3a6db5e12d..a837f27c73 100644
--- a/gdb/solib.c
+++ b/gdb/solib.c
@@ -1433,6 +1433,14 @@ gdb_sysroot_changed (const char *ignored, int from_tty,
 	}
     }
 
+  /* Trim trailing directory separator.  */
+  char *sysroot_dir = gdb_sysroot;
+  if (startswith (gdb_sysroot, TARGET_SYSROOT_PREFIX))
+    sysroot_dir += strlen (TARGET_SYSROOT_PREFIX);
+  size_t len = strlen (sysroot_dir);
+  if (len > 1 && IS_DIR_SEPARATOR (sysroot_dir[len - 1]))
+      sysroot_dir[len - 1] = '\0';
+
   reload_shared_libraries (ignored, from_tty, e);
 }
 
-- 
2.19.2

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

[John Baldwin]

When an object file is present in a system root, GDB currently looks
for separate debug files under the global debugfile directories.  For
example, if the sysroot is set to "/myroot" and hte global debugfile
directory is set to "/usr/lib/debug", GDB will look for a separate
debug file for "/myroot/lib/libc.so.7" in the following paths:

  /myroot/lib/libc.so.7.debug
  /myroot/lib/.debug/libc.so.7.debug
  /usr/lib/debug//myroot/lib/libc.so.7.debug
  /usr/lib/debug/lib/libc.so.7.debug

However, some system roots include a full system installation
including a nested global debugfile directory under the sysroot.  This
patch adds an additional check to support such systems.  In the
example above the additional path searched is:

  /myroot/usr/lib/debug/lib/libc.so.7.debug

To try to preserve existing behavior as much as possible, this new
path is searched last for each global debugfile directory.

gdb/ChangeLog:

	* symfile.c (find_separate_debug_file): Look for separate debug
	files in debug directories under the sysroot.
---
 gdb/ChangeLog |  5 +++++
 gdb/symfile.c | 19 +++++++++++++++++++
 2 files changed, 24 insertions(+)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 938fa83ca8..a401cfc784 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,8 @@
+2019-01-22  John Baldwin  <jhb@FreeBSD.org>
+
+	* symfile.c (find_separate_debug_file): Look for separate debug
+	files in debug directories under the sysroot.
+
 2019-01-22  John Baldwin  <jhb@FreeBSD.org>
 
 	* ppc-fbsd-tdep.c (ppcfbsd_get_thread_local_address): New.
diff --git a/gdb/symfile.c b/gdb/symfile.c
index 7f800add8c..c6d2c7c537 100644
--- a/gdb/symfile.c
+++ b/gdb/symfile.c
@@ -1465,6 +1465,25 @@ find_separate_debug_file (const char *dir,
 	  if (separate_debug_file_exists (debugfile, crc32, objfile))
 	    return debugfile;
 	}
+
+      /* If the file is in the sysroot, try using its base path in the
+	 sysroot's global debugfile directory.  */
+      if (canon_dir != NULL
+	  && filename_ncmp (canon_dir, gdb_sysroot,
+			    strlen (gdb_sysroot)) == 0
+	  && IS_DIR_SEPARATOR (canon_dir[strlen (gdb_sysroot)]))
+	{
+	  debugfile = target_prefix ? "target:" : "";
+	  debugfile += gdb_sysroot;
+	  debugfile += debugdir.get ();
+	  debugfile += (canon_dir + strlen (gdb_sysroot));
+	  debugfile += "/";
+	  debugfile += debuglink;
+
+	  if (separate_debug_file_exists (debugfile, crc32, objfile))
+	    return debugfile;
+	}
+
     }
 
   return std::string ();
-- 
2.19.2

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

[Simon Marchi]

On 2019-01-22 20:03, John Baldwin wrote:
> When an object file is present in a system root, GDB currently looks
> for separate debug files under the global debugfile directories.  For
> example, if the sysroot is set to "/myroot" and hte global debugfile
> directory is set to "/usr/lib/debug", GDB will look for a separate
> debug file for "/myroot/lib/libc.so.7" in the following paths:
> 
>   /myroot/lib/libc.so.7.debug
>   /myroot/lib/.debug/libc.so.7.debug
>   /usr/lib/debug//myroot/lib/libc.so.7.debug
>   /usr/lib/debug/lib/libc.so.7.debug
> 
> However, some system roots include a full system installation
> including a nested global debugfile directory under the sysroot.  This
> patch adds an additional check to support such systems.  In the
> example above the additional path searched is:
> 
>   /myroot/usr/lib/debug/lib/libc.so.7.debug
> 
> To try to preserve existing behavior as much as possible, this new
> path is searched last for each global debugfile directory.

I played with this a bit using a Raspbian sysroot.  I think the behavior 
you propose makes sense, perhaps more than the current behavior (I would 
be curious to see a real-word case for that one).  But anyway, keeping 
the old behavior and adding a new one is fine.  It's rather cheap to 
check many possible locations, and since there's a crc check, there's 
very little chance of loading a wrong separate debug file.

As discussed on IRC, we should probably add the same behavior for 
build-id-based separate debug files.

> diff --git a/gdb/symfile.c b/gdb/symfile.c
> index 7f800add8c..c6d2c7c537 100644
> --- a/gdb/symfile.c
> +++ b/gdb/symfile.c
> @@ -1465,6 +1465,25 @@ find_separate_debug_file (const char *dir,
>  	  if (separate_debug_file_exists (debugfile, crc32, objfile))
>  	    return debugfile;
>  	}
> +
> +      /* If the file is in the sysroot, try using its base path in the
> +	 sysroot's global debugfile directory.  */
> +      if (canon_dir != NULL
> +	  && filename_ncmp (canon_dir, gdb_sysroot,
> +			    strlen (gdb_sysroot)) == 0
> +	  && IS_DIR_SEPARATOR (canon_dir[strlen (gdb_sysroot)]))
> +	{

Is there any reason to duplicate the if above?  It looks to be the same 
as the previous check, so the new code could go in the existing if.

> +	  debugfile = target_prefix ? "target:" : "";
> +	  debugfile += gdb_sysroot;
> +	  debugfile += debugdir.get ();
> +	  debugfile += (canon_dir + strlen (gdb_sysroot));
> +	  debugfile += "/";
> +	  debugfile += debuglink;
> +
> +	  if (separate_debug_file_exists (debugfile, crc32, objfile))
> +	    return debugfile;
> +	}
> +

Simon

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

[Simon Marchi]

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(-)

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.  */
-- 
2.20.1

Re: [PATCH 2/2] Trim trailing directory separators from new sysroots.

[Simon Marchi]

On 2019-01-22 8:03 p.m., John Baldwin wrote:
> The separate debugfile logic assumes that the sysroot does not include
> a trailing separator (all of the sysroot logic in
> find_separate_debug_file requires the next character after the sysroot
> in a object file path to be a directory separator).  However, normal
> filename completion will result in setting a sysroot with a trailing
> directory separator.  If the directory portion of a new sysroot ends
> with a directory separator and is not specifying the root directory,
> trim the trailing separator.  This permits the sysroot logic to work
> the same if a sysroot of "/myroot/" is specified instead of "/myroot".
> 
> Note that the sysroot displayed by 'show sysroot' will reflect the
> trimmed sysroot.

I am not sure about this, it seems fragile to do this one off thing.  The
point where the path is stripped is quite far from the point where it has
an impact, so it's really not obvious why we do it (though this could be
fixed by saying why we do it in the comment).

I think it would be a step in a better direction to have an "is_child_path"
function that returns whether a path is a child of another one.  It would
be responsible for handling the case of the parent potentially having a
trailing directory separator.  The implementation of such function might
get hairy, but the  advantage is that it should be pretty easy to unit
test.

Finally, I think it would make this code more obvious:

      if (canon_dir != NULL
	  && filename_ncmp (canon_dir, gdb_sysroot,
			    strlen (gdb_sysroot)) == 0
	  && IS_DIR_SEPARATOR (canon_dir[strlen (gdb_sysroot)]))

vs

      if (canon_dir != NULL && is_child_patn (gdb_sysroot, canon_dir))

Simon

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

[John Baldwin]

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?

-- 
John Baldwin

                                                                            

Re: [PATCH 2/2] Trim trailing directory separators from new sysroots.

[John Baldwin]

On 1/26/19 11:10 AM, Simon Marchi wrote:
> On 2019-01-22 8:03 p.m., John Baldwin wrote:
>> The separate debugfile logic assumes that the sysroot does not include
>> a trailing separator (all of the sysroot logic in
>> find_separate_debug_file requires the next character after the sysroot
>> in a object file path to be a directory separator).  However, normal
>> filename completion will result in setting a sysroot with a trailing
>> directory separator.  If the directory portion of a new sysroot ends
>> with a directory separator and is not specifying the root directory,
>> trim the trailing separator.  This permits the sysroot logic to work
>> the same if a sysroot of "/myroot/" is specified instead of "/myroot".
>>
>> Note that the sysroot displayed by 'show sysroot' will reflect the
>> trimmed sysroot.
> 
> I am not sure about this, it seems fragile to do this one off thing.  The
> point where the path is stripped is quite far from the point where it has
> an impact, so it's really not obvious why we do it (though this could be
> fixed by saying why we do it in the comment).
> 
> I think it would be a step in a better direction to have an "is_child_path"
> function that returns whether a path is a child of another one.  It would
> be responsible for handling the case of the parent potentially having a
> trailing directory separator.  The implementation of such function might
> get hairy, but the  advantage is that it should be pretty easy to unit
> test.
> 
> Finally, I think it would make this code more obvious:
> 
>       if (canon_dir != NULL
> 	  && filename_ncmp (canon_dir, gdb_sysroot,
> 			    strlen (gdb_sysroot)) == 0
> 	  && IS_DIR_SEPARATOR (canon_dir[strlen (gdb_sysroot)]))
> 
> vs
> 
>       if (canon_dir != NULL && is_child_patn (gdb_sysroot, canon_dir))

I have taken this approach, but it ended up being a bit more than just a
boolean check as I needed to reliably get the base path of the child
component as the existing 'canon_dir + strlen (gdb_sysroot)' assumed no
trailing separator in gdb_sysroot.  I'll post the updated version along
with another fix I encountered in some more testing today as a v2 in a
sec.

-- 
John Baldwin

                                                                            

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

[Simon Marchi]

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