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

[PATCH v2 1/4] 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 | 17 +++++++++++++++--
 2 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 5d5f72b817..38d740e440 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,8 @@
+2019-01-28  John Baldwin  <jhb@FreeBSD.org>
+
+	* symfile.c (find_separate_debug_file): Look for separate debug
+	files in debug directories under the sysroot.
+
 2019-01-28  John Baldwin  <jhb@FreeBSD.org>
 
 	* aarch64-fbsd-tdep.c (aarch64_fbsd_gregmap)
diff --git a/gdb/symfile.c b/gdb/symfile.c
index e5ff38462c..b5802e20ad 100644
--- a/gdb/symfile.c
+++ b/gdb/symfile.c
@@ -1452,22 +1452,35 @@ 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
-	 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)]))
 	{
+	  /* If the file is in the sysroot, try using its base path in
+	     the global debugfile directory.  */
 	  debugfile = target_prefix ? "target:" : "";
 	  debugfile += debugdir.get ();
 	  debugfile += (canon_dir + strlen (gdb_sysroot));
 	  debugfile += "/";
 	  debugfile += debuglink;
 
+	  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.  */
+	  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

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

[John Baldwin]

Relative to the first series:

1) I combined the duplicate checks for "are we in a sysroot" in the
   first patch as as suggested by Simon.

2) I dropped the second patch (trim trailing '/' from sysroot).

3) Patches 2 and 3 are a different take on solving the issue when the
   sysroot ends in '/'.  Patch 2 adds a 'child_path' function to
   determine if a child path is a child of a parent (requiring the child
   to have at least one component "below" the parent).  It also returns
   a pointer to the first component below the parent (but after the
   directory separator).  Patch 3 uses child_path in
   find_separate_debug_file which fixes it in the case that the sysroot
   ends in a /.

4) The 4th patch is a new patch for a different issue I ran into while
   testing this some more today.  The paths to object files are always
   canonical paths with symlinks resolved.  If the sysroot entered by
   the user is a path containing symlinks, the filename_ncmp will
   never match.  To handle sysroot paths that traverse symlinks,
   use gdb_realpath to generate a canonical sysroot path and use that
   instead of gdb_sysroot with child_path.

As an aside, it's not clear to me when one should use gdb_realpath
instead of lrealpath.  gdb_realpath seems more widespread and also
returns an RAII-friendly type, so I used that.

John Baldwin (4):
  Look for separate debug files in debug directories under a sysroot.
  Add a new function child_path.
  Use child_path to determine if an object file is under a sysroot.
  Try to use the canonical version of a sysroot for debug file links.

 gdb/ChangeLog                        | 23 ++++++++++
 gdb/Makefile.in                      |  1 +
 gdb/common/pathstuff.c               | 52 ++++++++++++++++++++++
 gdb/common/pathstuff.h               |  6 +++
 gdb/symfile.c                        | 36 +++++++++++++---
 gdb/unittests/child-path-selftests.c | 64 ++++++++++++++++++++++++++++
 6 files changed, 175 insertions(+), 7 deletions(-)
 create mode 100644 gdb/unittests/child-path-selftests.c

-- 
2.19.2

[PATCH v2 3/4] Use child_path to determine if an object file is under a sysroot.

[John Baldwin]

This fixes the case where the sysroot happens to end in a trailing
'/'.  Note that the path returned from child_path always skips over
the directory separator at the start of the base path, so a separator
must always be explicitly added before the base path.

gdb/ChangeLog:

	* symfile.c (find_separate_debug_file): Use child_path to
	determine if an object file is under a sysroot.
---
 gdb/ChangeLog |  5 +++++
 gdb/symfile.c | 12 +++++++-----
 2 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 93a2cebe1c..e65182d84d 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,8 @@
+2019-01-28  John Baldwin  <jhb@FreeBSD.org>
+
+	* symfile.c (find_separate_debug_file): Use child_path to
+	determine if an object file is under a sysroot.
+
 2019-01-28  John Baldwin  <jhb@FreeBSD.org>
 
 	* Makefile.in (SUBDIR_UNITTESTS_SRCS): Add
diff --git a/gdb/symfile.c b/gdb/symfile.c
index b5802e20ad..ffcba1a090 100644
--- a/gdb/symfile.c
+++ b/gdb/symfile.c
@@ -57,6 +57,7 @@
 #include "gdb_bfd.h"
 #include "cli/cli-utils.h"
 #include "common/byte-vector.h"
+#include "common/pathstuff.h"
 #include "common/selftest.h"
 #include "cli/cli-style.h"
 #include "common/forward-scope-exit.h"
@@ -1452,16 +1453,16 @@ find_separate_debug_file (const char *dir,
       if (separate_debug_file_exists (debugfile, crc32, objfile))
 	return debugfile;
 
+      const char *base_path;
       if (canon_dir != NULL
-	  && filename_ncmp (canon_dir, gdb_sysroot,
-			    strlen (gdb_sysroot)) == 0
-	  && IS_DIR_SEPARATOR (canon_dir[strlen (gdb_sysroot)]))
+	  && (base_path = child_path (gdb_sysroot, canon_dir)) != NULL)
 	{
 	  /* If the file is in the sysroot, try using its base path in
 	     the global debugfile directory.  */
 	  debugfile = target_prefix ? "target:" : "";
 	  debugfile += debugdir.get ();
-	  debugfile += (canon_dir + strlen (gdb_sysroot));
+	  debugfile += "/";
+	  debugfile += base_path;
 	  debugfile += "/";
 	  debugfile += debuglink;
 
@@ -1473,7 +1474,8 @@ find_separate_debug_file (const char *dir,
 	  debugfile = target_prefix ? "target:" : "";
 	  debugfile += gdb_sysroot;
 	  debugfile += debugdir.get ();
-	  debugfile += (canon_dir + strlen (gdb_sysroot));
+	  debugfile += "/";
+	  debugfile += base_path;
 	  debugfile += "/";
 	  debugfile += debuglink;
 
-- 
2.19.2

[PATCH v2 2/4] Add a new function child_path.

[John Baldwin]

child_path returns a pointer to the first component in a child path
that comes after a parent path.  This does not depend on trying to
stat() the paths since they may describe remote paths but instead
relies on filename parsing.  The function requires that the child path
describe a filename that contains at least one component below the
parent path and returns a pointer to the first component.

gdb/ChangeLog:

	* Makefile.in (SUBDIR_UNITTESTS_SRCS): Add
	unittests/child-path-selftests.c.
	* common/pathstuff.c (child_path): New function.
	* common/pathstuff.h (child_path): New prototype.
	* unittests/child-path-selftests.c: New file.
---
 gdb/ChangeLog                        |  8 ++++
 gdb/Makefile.in                      |  1 +
 gdb/common/pathstuff.c               | 52 ++++++++++++++++++++++
 gdb/common/pathstuff.h               |  6 +++
 gdb/unittests/child-path-selftests.c | 64 ++++++++++++++++++++++++++++
 5 files changed, 131 insertions(+)
 create mode 100644 gdb/unittests/child-path-selftests.c

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 38d740e440..93a2cebe1c 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,11 @@
+2019-01-28  John Baldwin  <jhb@FreeBSD.org>
+
+	* Makefile.in (SUBDIR_UNITTESTS_SRCS): Add
+	unittests/child-path-selftests.c.
+	* common/pathstuff.c (child_path): New function.
+	* common/pathstuff.h (child_path): New prototype.
+	* unittests/child-path-selftests.c: New file.
+
 2019-01-28  John Baldwin  <jhb@FreeBSD.org>
 
 	* symfile.c (find_separate_debug_file): Look for separate debug
diff --git a/gdb/Makefile.in b/gdb/Makefile.in
index 72ca855eb0..cec7ad32a4 100644
--- a/gdb/Makefile.in
+++ b/gdb/Makefile.in
@@ -411,6 +411,7 @@ SUBDIR_PYTHON_CFLAGS =
 
 SUBDIR_UNITTESTS_SRCS = \
 	unittests/array-view-selftests.c \
+	unittests/child-path-selftests.c \
 	unittests/cli-utils-selftests.c \
 	unittests/common-utils-selftests.c \
 	unittests/copy_bitwise-selftests.c \
diff --git a/gdb/common/pathstuff.c b/gdb/common/pathstuff.c
index 11675303b3..d6beb8faf1 100644
--- a/gdb/common/pathstuff.c
+++ b/gdb/common/pathstuff.c
@@ -147,6 +147,58 @@ gdb_abspath (const char *path)
 
 /* See common/pathstuff.h.  */
 
+const char *
+child_path (const char *parent, const char *child)
+{
+  /* The child path must start with the parent path.  */
+  size_t parent_len = strlen (parent);
+  if (filename_ncmp (parent, child, parent_len) != 0)
+    return NULL;
+
+  /* The parent path must be a directory and the child must contain at
+     least one component underneath the parent.  */
+  const char *child_component;
+  if (IS_DIR_SEPARATOR (parent[parent_len - 1]))
+    {
+      /* The parent path ends in a directory separator, so it is a
+	 directory.  The first child component starts after the common
+	 prefix.  */
+      child_component = child + parent_len;
+    }
+  else
+    {
+      /* The parent path does not end in a directory separator.  The
+	 first character in the child after the common prefix must be
+	 a directory separator.
+
+	 Note that CHILD must hold at least parent_len characters for
+	 filename_ncmp to return zero.  If the character at parent_len
+	 is nul due to CHILD containing the same path as PARENT, the
+	 IS_DIR_SEPARATOR check will fail here.  */
+      if (!IS_DIR_SEPARATOR (child[parent_len]))
+	return NULL;
+
+      /* The first child component starts after the separator after the
+	 common prefix.  */
+      child_component = child + parent_len + 1;
+    }
+
+  /* The child must contain at least one non-separator character after
+     the parent.  */
+  while (*child_component != '\0')
+    {
+      if (IS_DIR_SEPARATOR (*child_component))
+	{
+	  child_component++;
+	  continue;
+	}
+      return child_component;
+    }
+  return NULL;
+}
+
+/* See common/pathstuff.h.  */
+
 bool
 contains_dir_separator (const char *path)
 {
diff --git a/gdb/common/pathstuff.h b/gdb/common/pathstuff.h
index d43f337550..20a7bdda26 100644
--- a/gdb/common/pathstuff.h
+++ b/gdb/common/pathstuff.h
@@ -48,6 +48,12 @@ extern gdb::unique_xmalloc_ptr<char>
 
 extern gdb::unique_xmalloc_ptr<char> gdb_abspath (const char *path);
 
+/* If the path in CHILD is a child of the path in PARENT, return a
+   pointer to the first component in the CHILD's pathname below the
+   PARENT.  Otherwise, return NULL.  */
+
+extern const char *child_path (const char *parent, const char *child);
+
 /* Return whether PATH contains a directory separator character.  */
 
 extern bool contains_dir_separator (const char *path);
diff --git a/gdb/unittests/child-path-selftests.c b/gdb/unittests/child-path-selftests.c
new file mode 100644
index 0000000000..2621eecef6
--- /dev/null
+++ b/gdb/unittests/child-path-selftests.c
@@ -0,0 +1,64 @@
+/* Self tests for child_path for GDB, the GNU debugger.
+
+   Copyright (C) 2019 Free Software Foundation, Inc.
+
+   This file is part of GDB.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#include "defs.h"
+#include "common/pathstuff.h"
+#include "common/selftest.h"
+
+namespace selftests {
+namespace child_path {
+
+/* Verify the result of a single child_path test.  */
+
+static bool
+child_path_check (const char *parent, const char *child, const char *expected)
+{
+  const char *result = ::child_path (parent, child);
+  if (result == NULL || expected == NULL)
+    return result == expected;
+  return strcmp (result, expected) == 0;
+}
+
+/* Test child_path.  */
+
+static void
+test ()
+{
+  SELF_CHECK (child_path_check ("/one", "/two", NULL));
+  SELF_CHECK (child_path_check ("/one", "/one", NULL));
+  SELF_CHECK (child_path_check ("/one", "/one/", NULL));
+  SELF_CHECK (child_path_check ("/one", "/one//", NULL));
+  SELF_CHECK (child_path_check ("/one", "/one/two", "two"));
+  SELF_CHECK (child_path_check ("/one/", "/two", NULL));
+  SELF_CHECK (child_path_check ("/one/", "/one", NULL));
+  SELF_CHECK (child_path_check ("/one/", "/one/", NULL));
+  SELF_CHECK (child_path_check ("/one/", "/one//", NULL));
+  SELF_CHECK (child_path_check ("/one/", "/one/two", "two"));
+}
+
+}
+}
+
+void
+_initialize_child_path_selftests ()
+{
+  selftests::register_test ("child_path",
+			    selftests::child_path::test);
+}
+
-- 
2.19.2

[PATCH v2 4/4] Try to use the canonical version of a sysroot for debug file links.

[John Baldwin]

Object file paths passed to find_separate_debug_file are always
canonical paths with symbolic links resolved.  If a sysroot path
traverses a symbolic link, it will not match the object file paths.
Generate a canonical version of the sysroot directory.  If it is
valid, use it instead of gdb_sysroot with child_path to dtermine if an
object file is under a system root.

gdb/ChangeLog:

	* symfile.c (find_separate_debug_file): Use canonical path of
	sysroot while child_path instead of gdb_sysroot if it is valid.
---
 gdb/ChangeLog |  5 +++++
 gdb/symfile.c | 13 ++++++++++---
 2 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index e65182d84d..f581c63198 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,8 @@
+2019-01-28  John Baldwin  <jhb@FreeBSD.org>
+
+	* symfile.c (find_separate_debug_file): Use canonical path of
+	sysroot while child_path instead of gdb_sysroot if it is valid.
+
 2019-01-28  John Baldwin  <jhb@FreeBSD.org>
 
 	* symfile.c (find_separate_debug_file): Use child_path to
diff --git a/gdb/symfile.c b/gdb/symfile.c
index ffcba1a090..61483824f6 100644
--- a/gdb/symfile.c
+++ b/gdb/symfile.c
@@ -1441,6 +1441,7 @@ find_separate_debug_file (const char *dir,
   const char *dir_notarget = target_prefix ? dir + strlen ("target:") : dir;
   std::vector<gdb::unique_xmalloc_ptr<char>> debugdir_vec
     = dirnames_to_char_ptr_vec (debug_file_directory);
+  gdb::unique_xmalloc_ptr<char> canon_sysroot = gdb_realpath (gdb_sysroot);
 
   for (const gdb::unique_xmalloc_ptr<char> &debugdir : debugdir_vec)
     {
@@ -1453,9 +1454,15 @@ find_separate_debug_file (const char *dir,
       if (separate_debug_file_exists (debugfile, crc32, objfile))
 	return debugfile;
 
-      const char *base_path;
-      if (canon_dir != NULL
-	  && (base_path = child_path (gdb_sysroot, canon_dir)) != NULL)
+      const char *base_path = NULL;
+      if (canon_dir != NULL)
+	{
+	  if (canon_sysroot.get () != NULL)
+	    base_path = child_path (canon_sysroot.get (), canon_dir);
+	  else
+	    base_path = child_path (gdb_sysroot, canon_dir);
+	}
+      if (base_path != NULL)
 	{
 	  /* If the file is in the sysroot, try using its base path in
 	     the global debugfile directory.  */
-- 
2.19.2

Re: [PING] [PATCH v2 0/4] Some fixes for debug files and sysroots

[John Baldwin]

On 1/28/19 12:47 PM, John Baldwin wrote:
> Relative to the first series:
> 
> 1) I combined the duplicate checks for "are we in a sysroot" in the
>    first patch as as suggested by Simon.
> 
> 2) I dropped the second patch (trim trailing '/' from sysroot).
> 
> 3) Patches 2 and 3 are a different take on solving the issue when the
>    sysroot ends in '/'.  Patch 2 adds a 'child_path' function to
>    determine if a child path is a child of a parent (requiring the child
>    to have at least one component "below" the parent).  It also returns
>    a pointer to the first component below the parent (but after the
>    directory separator).  Patch 3 uses child_path in
>    find_separate_debug_file which fixes it in the case that the sysroot
>    ends in a /.
> 
> 4) The 4th patch is a new patch for a different issue I ran into while
>    testing this some more today.  The paths to object files are always
>    canonical paths with symlinks resolved.  If the sysroot entered by
>    the user is a path containing symlinks, the filename_ncmp will
>    never match.  To handle sysroot paths that traverse symlinks,
>    use gdb_realpath to generate a canonical sysroot path and use that
>    instead of gdb_sysroot with child_path.
> 
> As an aside, it's not clear to me when one should use gdb_realpath
> instead of lrealpath.  gdb_realpath seems more widespread and also
> returns an RAII-friendly type, so I used that.
> 
> John Baldwin (4):
>   Look for separate debug files in debug directories under a sysroot.
>   Add a new function child_path.
>   Use child_path to determine if an object file is under a sysroot.
>   Try to use the canonical version of a sysroot for debug file links.

Just a ping.

-- 
John Baldwin

                                                                            

Re: [PATCH v2 2/4] Add a new function child_path.

[Simon Marchi]

On 2019-01-28 3:47 p.m., John Baldwin wrote:
> child_path returns a pointer to the first component in a child path
> that comes after a parent path.  This does not depend on trying to
> stat() the paths since they may describe remote paths but instead
> relies on filename parsing.  The function requires that the child path
> describe a filename that contains at least one component below the
> parent path and returns a pointer to the first component.
> 
> gdb/ChangeLog:
> 
> 	* Makefile.in (SUBDIR_UNITTESTS_SRCS): Add
> 	unittests/child-path-selftests.c.
> 	* common/pathstuff.c (child_path): New function.
> 	* common/pathstuff.h (child_path): New prototype.
> 	* unittests/child-path-selftests.c: New file.

Thanks, this LGTM.  Just minor comments below.

> ---
>  gdb/ChangeLog                        |  8 ++++
>  gdb/Makefile.in                      |  1 +
>  gdb/common/pathstuff.c               | 52 ++++++++++++++++++++++
>  gdb/common/pathstuff.h               |  6 +++
>  gdb/unittests/child-path-selftests.c | 64 ++++++++++++++++++++++++++++
>  5 files changed, 131 insertions(+)
>  create mode 100644 gdb/unittests/child-path-selftests.c
> 
> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
> index 38d740e440..93a2cebe1c 100644
> --- a/gdb/ChangeLog
> +++ b/gdb/ChangeLog
> @@ -1,3 +1,11 @@
> +2019-01-28  John Baldwin  <jhb@FreeBSD.org>
> +
> +	* Makefile.in (SUBDIR_UNITTESTS_SRCS): Add
> +	unittests/child-path-selftests.c.
> +	* common/pathstuff.c (child_path): New function.
> +	* common/pathstuff.h (child_path): New prototype.
> +	* unittests/child-path-selftests.c: New file.
> +
>  2019-01-28  John Baldwin  <jhb@FreeBSD.org>
>  
>  	* symfile.c (find_separate_debug_file): Look for separate debug
> diff --git a/gdb/Makefile.in b/gdb/Makefile.in
> index 72ca855eb0..cec7ad32a4 100644
> --- a/gdb/Makefile.in
> +++ b/gdb/Makefile.in
> @@ -411,6 +411,7 @@ SUBDIR_PYTHON_CFLAGS =
>  
>  SUBDIR_UNITTESTS_SRCS = \
>  	unittests/array-view-selftests.c \
> +	unittests/child-path-selftests.c \
>  	unittests/cli-utils-selftests.c \
>  	unittests/common-utils-selftests.c \
>  	unittests/copy_bitwise-selftests.c \
> diff --git a/gdb/common/pathstuff.c b/gdb/common/pathstuff.c
> index 11675303b3..d6beb8faf1 100644
> --- a/gdb/common/pathstuff.c
> +++ b/gdb/common/pathstuff.c
> @@ -147,6 +147,58 @@ gdb_abspath (const char *path)
>  
>  /* See common/pathstuff.h.  */
>  
> +const char *
> +child_path (const char *parent, const char *child)
> +{
> +  /* The child path must start with the parent path.  */
> +  size_t parent_len = strlen (parent);
> +  if (filename_ncmp (parent, child, parent_len) != 0)
> +    return NULL;
> +
> +  /* The parent path must be a directory and the child must contain at
> +     least one component underneath the parent.  */
> +  const char *child_component;
> +  if (IS_DIR_SEPARATOR (parent[parent_len - 1]))
> +    {
> +      /* The parent path ends in a directory separator, so it is a
> +	 directory.  The first child component starts after the common
> +	 prefix.  */
> +      child_component = child + parent_len;
> +    }
> +  else
> +    {
> +      /* The parent path does not end in a directory separator.  The
> +	 first character in the child after the common prefix must be
> +	 a directory separator.
> +
> +	 Note that CHILD must hold at least parent_len characters for
> +	 filename_ncmp to return zero.  If the character at parent_len
> +	 is nul due to CHILD containing the same path as PARENT, the
> +	 IS_DIR_SEPARATOR check will fail here.  */
> +      if (!IS_DIR_SEPARATOR (child[parent_len]))
> +	return NULL;
> +
> +      /* The first child component starts after the separator after the
> +	 common prefix.  */
> +      child_component = child + parent_len + 1;
> +    }
> +
> +  /* The child must contain at least one non-separator character after
> +     the parent.  */
> +  while (*child_component != '\0')
> +    {
> +      if (IS_DIR_SEPARATOR (*child_component))
> +	{
> +	  child_component++;
> +	  continue;
> +	}
> +      return child_component;

This might be simplified (avoid the continue) by reversing the logic, using
this as the loop body:

{
  if (!IS_DIR_SEPARATOR (*child_component))
    return child_component;

  child_component++;
}

> +    }
> +  return NULL;
> +}
> +
> +/* See common/pathstuff.h.  */
> +
>  bool
>  contains_dir_separator (const char *path)
>  {
> diff --git a/gdb/common/pathstuff.h b/gdb/common/pathstuff.h
> index d43f337550..20a7bdda26 100644
> --- a/gdb/common/pathstuff.h
> +++ b/gdb/common/pathstuff.h
> @@ -48,6 +48,12 @@ extern gdb::unique_xmalloc_ptr<char>
>  
>  extern gdb::unique_xmalloc_ptr<char> gdb_abspath (const char *path);
>  
> +/* If the path in CHILD is a child of the path in PARENT, return a
> +   pointer to the first component in the CHILD's pathname below the
> +   PARENT.  Otherwise, return NULL.  */
> +
> +extern const char *child_path (const char *parent, const char *child);
> +
>  /* Return whether PATH contains a directory separator character.  */
>  
>  extern bool contains_dir_separator (const char *path);
> diff --git a/gdb/unittests/child-path-selftests.c b/gdb/unittests/child-path-selftests.c
> new file mode 100644
> index 0000000000..2621eecef6
> --- /dev/null
> +++ b/gdb/unittests/child-path-selftests.c
> @@ -0,0 +1,64 @@
> +/* Self tests for child_path for GDB, the GNU debugger.
> +
> +   Copyright (C) 2019 Free Software Foundation, Inc.
> +
> +   This file is part of GDB.
> +
> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; either version 3 of the License, or
> +   (at your option) any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
> +
> +#include "defs.h"
> +#include "common/pathstuff.h"
> +#include "common/selftest.h"
> +
> +namespace selftests {
> +namespace child_path {
> +
> +/* Verify the result of a single child_path test.  */
> +
> +static bool
> +child_path_check (const char *parent, const char *child, const char *expected)
> +{
> +  const char *result = ::child_path (parent, child);
> +  if (result == NULL || expected == NULL)
> +    return result == expected;
> +  return strcmp (result, expected) == 0;
> +}
> +
> +/* Test child_path.  */
> +
> +static void
> +test ()
> +{
> +  SELF_CHECK (child_path_check ("/one", "/two", NULL));
> +  SELF_CHECK (child_path_check ("/one", "/one", NULL));
> +  SELF_CHECK (child_path_check ("/one", "/one/", NULL));
> +  SELF_CHECK (child_path_check ("/one", "/one//", NULL));
> +  SELF_CHECK (child_path_check ("/one", "/one/two", "two"));
> +  SELF_CHECK (child_path_check ("/one/", "/two", NULL));
> +  SELF_CHECK (child_path_check ("/one/", "/one", NULL));
> +  SELF_CHECK (child_path_check ("/one/", "/one/", NULL));
> +  SELF_CHECK (child_path_check ("/one/", "/one//", NULL));
> +  SELF_CHECK (child_path_check ("/one/", "/one/two", "two"));
> +}

Here are a few more edge cases I could think of:

  SELF_CHECK (child_path_check ("/one/", "/one//two", "two"));
  SELF_CHECK (child_path_check ("/one/", "/one//two/", "two/"));
  SELF_CHECK (child_path_check ("/one", "/onetwo", NULL));
  SELF_CHECK (child_path_check ("/one", "/onetwo/three", NULL));

> +
> +}
> +}
> +
> +void
> +_initialize_child_path_selftests ()
> +{
> +  selftests::register_test ("child_path",
> +			    selftests::child_path::test);
> +}
> +
> 

Simon

Re: [PATCH v2 2/4] Add a new function child_path.

[Simon Marchi]

On 2019-02-11 9:43 p.m., Simon Marchi wrote:
> On 2019-01-28 3:47 p.m., John Baldwin wrote:
>> child_path returns a pointer to the first component in a child path
>> that comes after a parent path.  This does not depend on trying to
>> stat() the paths since they may describe remote paths but instead
>> relies on filename parsing.  The function requires that the child path
>> describe a filename that contains at least one component below the
>> parent path and returns a pointer to the first component.
>>
>> gdb/ChangeLog:
>>
>> 	* Makefile.in (SUBDIR_UNITTESTS_SRCS): Add
>> 	unittests/child-path-selftests.c.
>> 	* common/pathstuff.c (child_path): New function.
>> 	* common/pathstuff.h (child_path): New prototype.
>> 	* unittests/child-path-selftests.c: New file.
> 
> Thanks, this LGTM.  Just minor comments below.

Oh, and maybe name the function is_child_path or child_path_p?

Simon

Re: [PING] [PATCH v2 0/4] Some fixes for debug files and sysroots

[Simon Marchi]

On 2019-02-11 12:54 p.m., John Baldwin wrote:
> On 1/28/19 12:47 PM, John Baldwin wrote:
>> Relative to the first series:
>>
>> 1) I combined the duplicate checks for "are we in a sysroot" in the
>>    first patch as as suggested by Simon.
>>
>> 2) I dropped the second patch (trim trailing '/' from sysroot).
>>
>> 3) Patches 2 and 3 are a different take on solving the issue when the
>>    sysroot ends in '/'.  Patch 2 adds a 'child_path' function to
>>    determine if a child path is a child of a parent (requiring the child
>>    to have at least one component "below" the parent).  It also returns
>>    a pointer to the first component below the parent (but after the
>>    directory separator).  Patch 3 uses child_path in
>>    find_separate_debug_file which fixes it in the case that the sysroot
>>    ends in a /.
>>
>> 4) The 4th patch is a new patch for a different issue I ran into while
>>    testing this some more today.  The paths to object files are always
>>    canonical paths with symlinks resolved.  If the sysroot entered by
>>    the user is a path containing symlinks, the filename_ncmp will
>>    never match.  To handle sysroot paths that traverse symlinks,
>>    use gdb_realpath to generate a canonical sysroot path and use that
>>    instead of gdb_sysroot with child_path.
>>
>> As an aside, it's not clear to me when one should use gdb_realpath
>> instead of lrealpath.  gdb_realpath seems more widespread and also
>> returns an RAII-friendly type, so I used that.
>>
>> John Baldwin (4):
>>   Look for separate debug files in debug directories under a sysroot.
>>   Add a new function child_path.
>>   Use child_path to determine if an object file is under a sysroot.
>>   Try to use the canonical version of a sysroot for debug file links.
> 
> Just a ping.
> 

Hi John,

Note the comments on patch #2, otherwise this LGTM.

Thanks!

Simon

Re: [PATCH v2 2/4] Add a new function child_path.

[John Baldwin]

On 2/11/19 6:46 PM, Simon Marchi wrote:
> On 2019-02-11 9:43 p.m., Simon Marchi wrote:
>> On 2019-01-28 3:47 p.m., John Baldwin wrote:
>>> child_path returns a pointer to the first component in a child path
>>> that comes after a parent path.  This does not depend on trying to
>>> stat() the paths since they may describe remote paths but instead
>>> relies on filename parsing.  The function requires that the child path
>>> describe a filename that contains at least one component below the
>>> parent path and returns a pointer to the first component.
>>>
>>> gdb/ChangeLog:
>>>
>>> 	* Makefile.in (SUBDIR_UNITTESTS_SRCS): Add
>>> 	unittests/child-path-selftests.c.
>>> 	* common/pathstuff.c (child_path): New function.
>>> 	* common/pathstuff.h (child_path): New prototype.
>>> 	* unittests/child-path-selftests.c: New file.
>>
>> Thanks, this LGTM.  Just minor comments below.
> 
> Oh, and maybe name the function is_child_path or child_path_p?

I started with that name in an earlier version when it returned a boolean,
but renamed it when I found that I needed it to return the trailing portion
of the child pathname to avoid duplicating logic in the caller.  Maybe
"get_child_path" would be better?

-- 
John Baldwin

                                                                            

Re: [PATCH v2 2/4] Add a new function child_path.

[Simon Marchi]

On 2019-02-12 11:52, John Baldwin wrote:
> On 2/11/19 6:46 PM, Simon Marchi wrote:
>> On 2019-02-11 9:43 p.m., Simon Marchi wrote:
>>> On 2019-01-28 3:47 p.m., John Baldwin wrote:
>>>> child_path returns a pointer to the first component in a child path
>>>> that comes after a parent path.  This does not depend on trying to
>>>> stat() the paths since they may describe remote paths but instead
>>>> relies on filename parsing.  The function requires that the child 
>>>> path
>>>> describe a filename that contains at least one component below the
>>>> parent path and returns a pointer to the first component.
>>>> 
>>>> gdb/ChangeLog:
>>>> 
>>>> 	* Makefile.in (SUBDIR_UNITTESTS_SRCS): Add
>>>> 	unittests/child-path-selftests.c.
>>>> 	* common/pathstuff.c (child_path): New function.
>>>> 	* common/pathstuff.h (child_path): New prototype.
>>>> 	* unittests/child-path-selftests.c: New file.
>>> 
>>> Thanks, this LGTM.  Just minor comments below.
>> 
>> Oh, and maybe name the function is_child_path or child_path_p?
> 
> I started with that name in an earlier version when it returned a 
> boolean,
> but renamed it when I found that I needed it to return the trailing 
> portion
> of the child pathname to avoid duplicating logic in the caller.  Maybe
> "get_child_path" would be better?

Ah you're right, it doesn't make sense if the function doesn't return a 
bool.  What you have is fine with me.

Simon

Re: [PING] [PATCH v2 0/4] Some fixes for debug files and sysroots

[John Baldwin]

On 2/11/19 6:53 PM, Simon Marchi wrote:
> On 2019-02-11 12:54 p.m., John Baldwin wrote:
>> On 1/28/19 12:47 PM, John Baldwin wrote:
>>> Relative to the first series:
>>>
>>> 1) I combined the duplicate checks for "are we in a sysroot" in the
>>>    first patch as as suggested by Simon.
>>>
>>> 2) I dropped the second patch (trim trailing '/' from sysroot).
>>>
>>> 3) Patches 2 and 3 are a different take on solving the issue when the
>>>    sysroot ends in '/'.  Patch 2 adds a 'child_path' function to
>>>    determine if a child path is a child of a parent (requiring the child
>>>    to have at least one component "below" the parent).  It also returns
>>>    a pointer to the first component below the parent (but after the
>>>    directory separator).  Patch 3 uses child_path in
>>>    find_separate_debug_file which fixes it in the case that the sysroot
>>>    ends in a /.
>>>
>>> 4) The 4th patch is a new patch for a different issue I ran into while
>>>    testing this some more today.  The paths to object files are always
>>>    canonical paths with symlinks resolved.  If the sysroot entered by
>>>    the user is a path containing symlinks, the filename_ncmp will
>>>    never match.  To handle sysroot paths that traverse symlinks,
>>>    use gdb_realpath to generate a canonical sysroot path and use that
>>>    instead of gdb_sysroot with child_path.
>>>
>>> As an aside, it's not clear to me when one should use gdb_realpath
>>> instead of lrealpath.  gdb_realpath seems more widespread and also
>>> returns an RAII-friendly type, so I used that.
>>>
>>> John Baldwin (4):
>>>   Look for separate debug files in debug directories under a sysroot.
>>>   Add a new function child_path.
>>>   Use child_path to determine if an object file is under a sysroot.
>>>   Try to use the canonical version of a sysroot for debug file links.
>>
>> Just a ping.
>>
> 
> Hi John,
> 
> Note the comments on patch #2, otherwise this LGTM.
> 
> Thanks!

Thanks, I've pushed it with the fixes in #2.  Do you want to push your
build-id + sysroot fix?

-- 
John Baldwin