[1/2] Look for separate debug files in debug directories under a sysroot.
Commit Message
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(-)
Comments
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?
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
@@ -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. */