| Message ID | 96b4fb01-d703-ad1b-5ea5-d7e5366f5f6b@polymtl.ca |
|---|---|
| State | New |
| Headers | show |
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
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. */