* [PATCH 0/2] Some fixes for debug files and sysroots @ 2019-01-23 1:04 John Baldwin 2019-01-23 1:04 ` [PATCH 1/2] Look for separate debug files in debug directories under a sysroot John Baldwin 2019-01-23 1:04 ` [PATCH 2/2] Trim trailing directory separators from new sysroots John Baldwin 0 siblings, 2 replies; 9+ messages in thread From: John Baldwin @ 2019-01-23 1:04 UTC (permalink / raw) To: gdb-patches 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 1/2] Look for separate debug files in debug directories under a sysroot. 2019-01-23 1:04 [PATCH 0/2] Some fixes for debug files and sysroots John Baldwin @ 2019-01-23 1:04 ` John Baldwin 2019-01-26 5:17 ` Simon Marchi 2019-01-23 1:04 ` [PATCH 2/2] Trim trailing directory separators from new sysroots John Baldwin 1 sibling, 1 reply; 9+ messages in thread From: John Baldwin @ 2019-01-23 1:04 UTC (permalink / raw) To: gdb-patches 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] Look for separate debug files in debug directories under a sysroot. 2019-01-23 1:04 ` [PATCH 1/2] Look for separate debug files in debug directories under a sysroot John Baldwin @ 2019-01-26 5:17 ` Simon Marchi 2019-01-26 17:12 ` Simon Marchi 0 siblings, 1 reply; 9+ messages in thread From: Simon Marchi @ 2019-01-26 5:17 UTC (permalink / raw) To: John Baldwin; +Cc: gdb-patches 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] Look for separate debug files in debug directories under a sysroot. 2019-01-26 5:17 ` Simon Marchi @ 2019-01-26 17:12 ` Simon Marchi 2019-01-28 18:53 ` John Baldwin 0 siblings, 1 reply; 9+ messages in thread From: Simon Marchi @ 2019-01-26 17:12 UTC (permalink / raw) To: John Baldwin; +Cc: gdb-patches 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] Look for separate debug files in debug directories under a sysroot. 2019-01-26 17:12 ` Simon Marchi @ 2019-01-28 18:53 ` John Baldwin 2019-02-22 20:51 ` Simon Marchi 0 siblings, 1 reply; 9+ messages in thread From: John Baldwin @ 2019-01-28 18:53 UTC (permalink / raw) To: Simon Marchi; +Cc: gdb-patches 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                                                                             ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] Look for separate debug files in debug directories under a sysroot. 2019-01-28 18:53 ` John Baldwin @ 2019-02-22 20:51 ` Simon Marchi 0 siblings, 0 replies; 9+ messages in thread From: Simon Marchi @ 2019-02-22 20:51 UTC (permalink / raw) To: John Baldwin, Simon Marchi; +Cc: gdb-patches 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 2/2] Trim trailing directory separators from new sysroots. 2019-01-23 1:04 [PATCH 0/2] Some fixes for debug files and sysroots John Baldwin 2019-01-23 1:04 ` [PATCH 1/2] Look for separate debug files in debug directories under a sysroot John Baldwin @ 2019-01-23 1:04 ` John Baldwin 2019-01-26 19:10 ` Simon Marchi 1 sibling, 1 reply; 9+ messages in thread From: John Baldwin @ 2019-01-23 1:04 UTC (permalink / raw) To: gdb-patches 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] Trim trailing directory separators from new sysroots. 2019-01-23 1:04 ` [PATCH 2/2] Trim trailing directory separators from new sysroots John Baldwin @ 2019-01-26 19:10 ` Simon Marchi 2019-01-28 20:41 ` John Baldwin 0 siblings, 1 reply; 9+ messages in thread From: Simon Marchi @ 2019-01-26 19:10 UTC (permalink / raw) To: John Baldwin, gdb-patches 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] Trim trailing directory separators from new sysroots. 2019-01-26 19:10 ` Simon Marchi @ 2019-01-28 20:41 ` John Baldwin 0 siblings, 0 replies; 9+ messages in thread From: John Baldwin @ 2019-01-28 20:41 UTC (permalink / raw) To: Simon Marchi, gdb-patches 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                                                                             ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2019-02-22 20:51 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-01-23 1:04 [PATCH 0/2] Some fixes for debug files and sysroots John Baldwin 2019-01-23 1:04 ` [PATCH 1/2] Look for separate debug files in debug directories under a sysroot John Baldwin 2019-01-26 5:17 ` Simon Marchi 2019-01-26 17:12 ` Simon Marchi 2019-01-28 18:53 ` John Baldwin 2019-02-22 20:51 ` Simon Marchi 2019-01-23 1:04 ` [PATCH 2/2] Trim trailing directory separators from new sysroots John Baldwin 2019-01-26 19:10 ` Simon Marchi 2019-01-28 20:41 ` John Baldwin
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox