[PATCH 0/2] find debug by debuglink minor fixes

[PATCH 0/2] find debug by debuglink minor fixes

[Andrew Burgess]

This started out as me realising the sysroot-debug-lookup.exp test has
a KPASS on the native-extended-remote board.  So I was going to change
that to a PASS.  The I realised that for that board I could actuall
change another KFAIL into a PASS if I fixed the expected output.

And while I was thinking about that, I realised that with a few minor
fixes to GDB I could resolve the KFAIL for the unix board too, so I've
also done that.

---

Andrew Burgess (2):
  gdb: avoid '//' in filenames when searching for debuginfo
  gdb: fix a target: prefix issue in find_separate_debug_file

 gdb/symfile.c                                 | 87 ++++++++++++-------
 .../gdb.base/sysroot-debug-lookup.exp         | 15 ----
 2 files changed, 57 insertions(+), 45 deletions(-)


base-commit: 6dfd07222c02edc792447049ba94518ae982f362
-- 
2.25.4

[PATCH 1/2] gdb: avoid '//' in filenames when searching for debuginfo

[Andrew Burgess]

I spotted that the gdb.base/sysroot-debug-lookup.exp test that I added
recently actually had a KPASS when run with the
native-extended-gdbserver board.  This was an oversight when adding
the test.

The failures in this test, when using the 'unix' board, are logged as
bug PR gdb/31804.  The problem appears to be caused by the use of the
child_path function in find_separate_debug_file.

What happens on the 'unix' board is that the file is specified to GDB
with a target: prefix, however GDB spots that the target filesystem is
local to GDB and so opens the file without a target: prefix.  When we
call into find_separate_debug_file the DIR and CANON_DIR arguments,
which are computed from the objfile_name() no longer have a target:
prefix.

However, in this test if the file was opened with a target: prefix,
then the sysroot also has a target: prefix.  When child_path is called
it looks for a common prefix between CANON_DIR (from the objfile_name)
and the sysroot.  However, the sysroot still has the target: prefix,
which means the child_path() call fails and returns nullptr.

What happens in the native-extended-gdbserver case is that GDB doesn't
see the target filesystem as local.  Now the filename retains the
target: prefix, which means that in the child_path() call both the
sysroot and the CANON_DIR have a target: prefix, and so the
child_path() call succeeds.  This allows GDB to progress, try some
additional paths, and then find the debug information.

So, this commit changes gdb.base/sysroot-debug-lookup.exp to expect
the test to succeed when using the native-extended-gdbserver protocol.

This leaves one KFAIL when using the native-extended-gdbserver board,
we find the debug information but (apparently) find it in the wrong
file.  What's happening is that when GDB builds the path to the debug
information we end up with a '//' string as a directory separator, the
test regexp only expects a single separator.

Instead of just fixing the test regexp, I've added a new function
combine_paths which I then use for building the debug info paths.
This function takes care of ensuring only a single '/' is added when
joining paths together.  With this done I now have no KFAIL when using
the native-extended-gdbserver board.

The new combine_paths function is similar to the existing path_join
function (see gdbsupport/pathstuff.h), but unlike path_join the new
function doesn't require that the first path be absolute.  For now
I've left the new function in symfile.c, it can always be moved if
this proves to be more widely useful.

After this commit we still have 2 KFAIL when not using the
native-extended-gdbserver board.

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31804
---
 gdb/symfile.c                                 | 80 ++++++++++++-------
 .../gdb.base/sysroot-debug-lookup.exp         |  6 +-
 2 files changed, 56 insertions(+), 30 deletions(-)

diff --git a/gdb/symfile.c b/gdb/symfile.c
index 5a03def91c6..9ef31824fe0 100644
--- a/gdb/symfile.c
+++ b/gdb/symfile.c
@@ -1344,6 +1344,44 @@ show_debug_file_directory (struct ui_file *file, int from_tty,
 #define DEBUG_SUBDIRECTORY ".debug"
 #endif
 
+/* This is like path_join, but does not have the requirement that the first
+   path be an absolute path.  All PATHS are joined together and a directory
+   separator is added if needed between joined elements.  */
+
+template<typename ...Args>
+static std::string
+combine_paths (Args... paths)
+{
+  /* It doesn't make sense to join less than two paths.  */
+  static_assert (sizeof... (Args) >= 2);
+
+  std::array<const char *, sizeof... (Args)> path_array
+    { paths... };
+
+  std::string ret;
+
+  for (int i = 0; i < path_array.size (); ++i)
+    {
+      const char *path = path_array[i];
+
+      if (!ret.empty ())
+	{
+	  /* If RET doesn't end in a separator then add one now.  */
+	  if (!IS_DIR_SEPARATOR (ret.back ()))
+	    ret += '/';
+
+	  /* Now that RET ends in a separator ignore any at the start of
+	     PATH.  */
+	  while (IS_DIR_SEPARATOR (path[0]))
+	    ++path;
+	}
+
+      ret.append (path);
+    }
+
+  return ret;
+}
+
 /* Find a separate debuginfo file for OBJFILE, using DIR as the directory
    where the original file resides (may not be the same as
    dirname(objfile->name) due to symlinks), and DEBUGLINK as the file we are
@@ -1370,17 +1408,13 @@ find_separate_debug_file (const char *dir,
      objfile_name (objfile));
 
   /* First try in the same directory as the original file.  */
-  std::string debugfile = dir;
-  debugfile += debuglink;
+  std::string debugfile = combine_paths (dir, debuglink);
 
   if (separate_debug_file_exists (debugfile, crc32, objfile, warnings))
     return debugfile;
 
   /* Then try in the subdirectory named DEBUG_SUBDIRECTORY.  */
-  debugfile = dir;
-  debugfile += DEBUG_SUBDIRECTORY;
-  debugfile += "/";
-  debugfile += debuglink;
+  debugfile = combine_paths (dir, DEBUG_SUBDIRECTORY, debuglink);
 
   if (separate_debug_file_exists (debugfile, crc32, objfile, warnings))
     return debugfile;
@@ -1393,6 +1427,7 @@ find_separate_debug_file (const char *dir,
   bool target_prefix = is_target_filename (dir);
   const char *dir_notarget
     = target_prefix ? dir + strlen (TARGET_SYSROOT_PREFIX) : dir;
+  const char *target_prefix_str = target_prefix ? TARGET_SYSROOT_PREFIX : "";
   std::vector<gdb::unique_xmalloc_ptr<char>> debugdir_vec
     = dirnames_to_char_ptr_vec (debug_file_directory.c_str ());
   gdb::unique_xmalloc_ptr<char> canon_sysroot
@@ -1421,12 +1456,8 @@ find_separate_debug_file (const char *dir,
 
   for (const gdb::unique_xmalloc_ptr<char> &debugdir : debugdir_vec)
     {
-      debugfile = target_prefix ? TARGET_SYSROOT_PREFIX : "";
-      debugfile += debugdir;
-      debugfile += "/";
-      debugfile += drive;
-      debugfile += dir_notarget;
-      debugfile += debuglink;
+      debugfile = combine_paths (target_prefix_str, debugdir.get (),
+				 drive.c_str (), dir_notarget, debuglink);
 
       if (separate_debug_file_exists (debugfile, crc32, objfile, warnings))
 	return debugfile;
@@ -1443,12 +1474,8 @@ find_separate_debug_file (const char *dir,
 	{
 	  /* If the file is in the sysroot, try using its base path in
 	     the global debugfile directory.  */
-	  debugfile = target_prefix ? TARGET_SYSROOT_PREFIX : "";
-	  debugfile += debugdir;
-	  debugfile += "/";
-	  debugfile += base_path;
-	  debugfile += "/";
-	  debugfile += debuglink;
+	  debugfile = combine_paths (target_prefix_str, debugdir.get (),
+				     base_path, debuglink);
 
 	  if (separate_debug_file_exists (debugfile, crc32, objfile, warnings))
 	    return debugfile;
@@ -1461,21 +1488,18 @@ find_separate_debug_file (const char *dir,
 	     same result as above.  */
 	  if (gdb_sysroot != TARGET_SYSROOT_PREFIX)
 	    {
-	      debugfile = target_prefix ? TARGET_SYSROOT_PREFIX : "";
+	      std::string root;
 	      if (is_target_filename (gdb_sysroot))
 		{
-		  std::string root
-		    = gdb_sysroot.substr (strlen (TARGET_SYSROOT_PREFIX));
+		  root = gdb_sysroot.substr (strlen (TARGET_SYSROOT_PREFIX));
 		  gdb_assert (!root.empty ());
-		  debugfile += root;
 		}
 	      else
-		debugfile += gdb_sysroot;
-	      debugfile += debugdir;
-	      debugfile += "/";
-	      debugfile += base_path;
-	      debugfile += "/";
-	      debugfile += debuglink;
+		root = gdb_sysroot;
+
+	      debugfile = combine_paths (target_prefix_str, root.c_str (),
+					 debugdir.get (), base_path,
+					 debuglink);
 
 	      if (separate_debug_file_exists (debugfile, crc32, objfile,
 					      warnings))
diff --git a/gdb/testsuite/gdb.base/sysroot-debug-lookup.exp b/gdb/testsuite/gdb.base/sysroot-debug-lookup.exp
index 5f17315c027..30d492e0d47 100644
--- a/gdb/testsuite/gdb.base/sysroot-debug-lookup.exp
+++ b/gdb/testsuite/gdb.base/sysroot-debug-lookup.exp
@@ -183,13 +183,15 @@ proc_with_prefix lookup_via_debuglink {} {
 	#
 	# Bug PR gdb/30866 seems to be the (or a) relevant bug for
 	# this problem.
-	if { $sysroot_prefix ne "" } {
+	if { $sysroot_prefix ne ""
+	     && [target_info gdb_protocol] ne "extended-remote" } {
 	    setup_kfail "*-*-*" 31804
 	}
 	gdb_assert { $::gdb_file_cmd_debug_info eq "debug" } \
 	    "ensure debug information was found"
 
-	if { $sysroot_prefix ne "" } {
+	if { $sysroot_prefix ne ""
+	     && [target_info gdb_protocol] ne "extended-remote"  } {
 	    setup_kfail "*-*-*" 31804
 	}
 	set re [string_to_regexp "Reading symbols from ${sysroot_prefix}$debug_symlink..."]
-- 
2.25.4

Re: [PATCH 1/2] gdb: avoid '//' in filenames when searching for debuginfo

[Tom Tromey]

>>>>> "Andrew" == Andrew Burgess <aburgess@redhat.com> writes:

Andrew> The new combine_paths function is similar to the existing path_join
Andrew> function (see gdbsupport/pathstuff.h), but unlike path_join the new
Andrew> function doesn't require that the first path be absolute.

From what I can see, path_join doesn't have this requirement.
The docs say "The first element can be absolute or relative" and the
code seems to agree.

Tom

Re: [PATCH 1/2] gdb: avoid '//' in filenames when searching for debuginfo

[Andrew Burgess]

Tom Tromey <tom@tromey.com> writes:

>>>>>> "Andrew" == Andrew Burgess <aburgess@redhat.com> writes:
>
> Andrew> The new combine_paths function is similar to the existing path_join
> Andrew> function (see gdbsupport/pathstuff.h), but unlike path_join the new
> Andrew> function doesn't require that the first path be absolute.
>
> From what I can see, path_join doesn't have this requirement.
> The docs say "The first element can be absolute or relative" and the
> code seems to agree.

You're absolutely right.  And I see what's happened.  Somewhere along
the line the logic has gotten reversed in my head.  The problem isn't
that the first _must_ be absolute with path_join, it's that everything
else _must_not_ be absolute, see these lines in path_join:

      if (i > 0)
	gdb_assert (strlen (path) == 0 || !IS_ABSOLUTE_PATH (path));

And for what we're doing here we, for example, join the path to the file
(which is absolute) onto the debug directory, which might, or might not,
be absolute, e.g. these lines in find_separate_debug_file:

  /* If the file is in the sysroot, try using its base path in
     the global debugfile directory.  */
  debugfile = path_join (target_prefix_str, debugdir.get (),
			 base_path, debuglink);

I've initially understood the problem, written the code, then come back
and added comments and a commit message, and gotten mixed up somewhere.
Sorry about that.

Anyway, here's an updated patch.  I've change one comment in symfile.c,
and I've also updated the commit message, other than that the patch is
unchanged.

Oh, and just because I was doubting myself, I did try using path_join,
and as predicted the test (sysroot-debug-lookup.exp) causes GDB to
trigger an assertion as we are appending an absolute path.

Thanks,
Andrew

---

commit 5fc3331c17021e997b4b7414416e477574c52669
Author: Andrew Burgess <aburgess@redhat.com>
Date:   Wed Jun 12 15:24:46 2024 +0100

    gdb: avoid '//' in filenames when searching for debuginfo
    
    I spotted that the gdb.base/sysroot-debug-lookup.exp test that I added
    recently actually had a KPASS when run with the
    native-extended-gdbserver board.  This was an oversight when adding
    the test.
    
    The failures in this test, when using the 'unix' board, are logged as
    bug PR gdb/31804.  The problem appears to be caused by the use of the
    child_path function in find_separate_debug_file.
    
    What happens on the 'unix' board is that the file is specified to GDB
    with a target: prefix, however GDB spots that the target filesystem is
    local to GDB and so opens the file without a target: prefix.  When we
    call into find_separate_debug_file the DIR and CANON_DIR arguments,
    which are computed from the objfile_name() no longer have a target:
    prefix.
    
    However, in this test if the file was opened with a target: prefix,
    then the sysroot also has a target: prefix.  When child_path is called
    it looks for a common prefix between CANON_DIR (from the objfile_name)
    and the sysroot.  However, the sysroot still has the target: prefix,
    which means the child_path() call fails and returns nullptr.
    
    What happens in the native-extended-gdbserver case is that GDB doesn't
    see the target filesystem as local.  Now the filename retains the
    target: prefix, which means that in the child_path() call both the
    sysroot and the CANON_DIR have a target: prefix, and so the
    child_path() call succeeds.  This allows GDB to progress, try some
    additional paths, and then find the debug information.
    
    So, this commit changes gdb.base/sysroot-debug-lookup.exp to expect
    the test to succeed when using the native-extended-gdbserver protocol.
    
    This leaves one KFAIL when using the native-extended-gdbserver board,
    we find the debug information but (apparently) find it in the wrong
    file.  What's happening is that when GDB builds the path to the debug
    information we end up with a '//' string as a directory separator, the
    test regexp only expects a single separator.
    
    Instead of just fixing the test regexp, I've added a new function
    combine_paths which I then use for building the debug info paths.
    This function takes care of ensuring only a single '/' is added when
    joining paths together.  With this done I now have no KFAIL when using
    the native-extended-gdbserver board.
    
    The new combine_paths function is similar to the existing path_join
    function (see gdbsupport/pathstuff.h), but unlike path_join the new
    function doesn't require every path after the first be a relative
    path.  For now I've left the new function in symfile.c, it can always
    be moved if this proves to be more widely useful.
    
    After this commit we still have 2 KFAIL when not using the
    native-extended-gdbserver board.
    
    Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31804

diff --git a/gdb/symfile.c b/gdb/symfile.c
index 5a03def91c6..0a0ed7e56d9 100644
--- a/gdb/symfile.c
+++ b/gdb/symfile.c
@@ -1344,6 +1344,47 @@ show_debug_file_directory (struct ui_file *file, int from_tty,
 #define DEBUG_SUBDIRECTORY ".debug"
 #endif
 
+/* This is like path_join, but does not have the requirement that every
+   path after the first be a relative path.  All PATHS are joined together
+   and a directory separator is added if needed between joined elements.
+
+   This allows us to do things like, e.g. append the absolute path to a
+   file onto the debug file directory.  */
+
+template<typename ...Args>
+static std::string
+combine_paths (Args... paths)
+{
+  /* It doesn't make sense to join less than two paths.  */
+  static_assert (sizeof... (Args) >= 2);
+
+  std::array<const char *, sizeof... (Args)> path_array
+    { paths... };
+
+  std::string ret;
+
+  for (int i = 0; i < path_array.size (); ++i)
+    {
+      const char *path = path_array[i];
+
+      if (!ret.empty ())
+	{
+	  /* If RET doesn't end in a separator then add one now.  */
+	  if (!IS_DIR_SEPARATOR (ret.back ()))
+	    ret += '/';
+
+	  /* Now that RET ends in a separator ignore any at the start of
+	     PATH.  */
+	  while (IS_DIR_SEPARATOR (path[0]))
+	    ++path;
+	}
+
+      ret.append (path);
+    }
+
+  return ret;
+}
+
 /* Find a separate debuginfo file for OBJFILE, using DIR as the directory
    where the original file resides (may not be the same as
    dirname(objfile->name) due to symlinks), and DEBUGLINK as the file we are
@@ -1370,17 +1411,13 @@ find_separate_debug_file (const char *dir,
      objfile_name (objfile));
 
   /* First try in the same directory as the original file.  */
-  std::string debugfile = dir;
-  debugfile += debuglink;
+  std::string debugfile = combine_paths (dir, debuglink);
 
   if (separate_debug_file_exists (debugfile, crc32, objfile, warnings))
     return debugfile;
 
   /* Then try in the subdirectory named DEBUG_SUBDIRECTORY.  */
-  debugfile = dir;
-  debugfile += DEBUG_SUBDIRECTORY;
-  debugfile += "/";
-  debugfile += debuglink;
+  debugfile = combine_paths (dir, DEBUG_SUBDIRECTORY, debuglink);
 
   if (separate_debug_file_exists (debugfile, crc32, objfile, warnings))
     return debugfile;
@@ -1393,6 +1430,7 @@ find_separate_debug_file (const char *dir,
   bool target_prefix = is_target_filename (dir);
   const char *dir_notarget
     = target_prefix ? dir + strlen (TARGET_SYSROOT_PREFIX) : dir;
+  const char *target_prefix_str = target_prefix ? TARGET_SYSROOT_PREFIX : "";
   std::vector<gdb::unique_xmalloc_ptr<char>> debugdir_vec
     = dirnames_to_char_ptr_vec (debug_file_directory.c_str ());
   gdb::unique_xmalloc_ptr<char> canon_sysroot
@@ -1421,12 +1459,8 @@ find_separate_debug_file (const char *dir,
 
   for (const gdb::unique_xmalloc_ptr<char> &debugdir : debugdir_vec)
     {
-      debugfile = target_prefix ? TARGET_SYSROOT_PREFIX : "";
-      debugfile += debugdir;
-      debugfile += "/";
-      debugfile += drive;
-      debugfile += dir_notarget;
-      debugfile += debuglink;
+      debugfile = combine_paths (target_prefix_str, debugdir.get (),
+				 drive.c_str (), dir_notarget, debuglink);
 
       if (separate_debug_file_exists (debugfile, crc32, objfile, warnings))
 	return debugfile;
@@ -1443,12 +1477,8 @@ find_separate_debug_file (const char *dir,
 	{
 	  /* If the file is in the sysroot, try using its base path in
 	     the global debugfile directory.  */
-	  debugfile = target_prefix ? TARGET_SYSROOT_PREFIX : "";
-	  debugfile += debugdir;
-	  debugfile += "/";
-	  debugfile += base_path;
-	  debugfile += "/";
-	  debugfile += debuglink;
+	  debugfile = combine_paths (target_prefix_str, debugdir.get (),
+				     base_path, debuglink);
 
 	  if (separate_debug_file_exists (debugfile, crc32, objfile, warnings))
 	    return debugfile;
@@ -1461,21 +1491,18 @@ find_separate_debug_file (const char *dir,
 	     same result as above.  */
 	  if (gdb_sysroot != TARGET_SYSROOT_PREFIX)
 	    {
-	      debugfile = target_prefix ? TARGET_SYSROOT_PREFIX : "";
+	      std::string root;
 	      if (is_target_filename (gdb_sysroot))
 		{
-		  std::string root
-		    = gdb_sysroot.substr (strlen (TARGET_SYSROOT_PREFIX));
+		  root = gdb_sysroot.substr (strlen (TARGET_SYSROOT_PREFIX));
 		  gdb_assert (!root.empty ());
-		  debugfile += root;
 		}
 	      else
-		debugfile += gdb_sysroot;
-	      debugfile += debugdir;
-	      debugfile += "/";
-	      debugfile += base_path;
-	      debugfile += "/";
-	      debugfile += debuglink;
+		root = gdb_sysroot;
+
+	      debugfile = combine_paths (target_prefix_str, root.c_str (),
+					 debugdir.get (), base_path,
+					 debuglink);
 
 	      if (separate_debug_file_exists (debugfile, crc32, objfile,
 					      warnings))
diff --git a/gdb/testsuite/gdb.base/sysroot-debug-lookup.exp b/gdb/testsuite/gdb.base/sysroot-debug-lookup.exp
index 5f17315c027..30d492e0d47 100644
--- a/gdb/testsuite/gdb.base/sysroot-debug-lookup.exp
+++ b/gdb/testsuite/gdb.base/sysroot-debug-lookup.exp
@@ -183,13 +183,15 @@ proc_with_prefix lookup_via_debuglink {} {
 	#
 	# Bug PR gdb/30866 seems to be the (or a) relevant bug for
 	# this problem.
-	if { $sysroot_prefix ne "" } {
+	if { $sysroot_prefix ne ""
+	     && [target_info gdb_protocol] ne "extended-remote" } {
 	    setup_kfail "*-*-*" 31804
 	}
 	gdb_assert { $::gdb_file_cmd_debug_info eq "debug" } \
 	    "ensure debug information was found"
 
-	if { $sysroot_prefix ne "" } {
+	if { $sysroot_prefix ne ""
+	     && [target_info gdb_protocol] ne "extended-remote"  } {
 	    setup_kfail "*-*-*" 31804
 	}
 	set re [string_to_regexp "Reading symbols from ${sysroot_prefix}$debug_symlink..."]

Re: [PATCH 1/2] gdb: avoid '//' in filenames when searching for debuginfo

[Eli Zaretskii]

> From: Andrew Burgess <aburgess@redhat.com>
> Cc: gdb-patches@sourceware.org
> Date: Fri, 14 Jun 2024 09:58:09 +0100
> 
> Tom Tromey <tom@tromey.com> writes:
> 
> >>>>>> "Andrew" == Andrew Burgess <aburgess@redhat.com> writes:
> >
> > Andrew> The new combine_paths function is similar to the existing path_join
> > Andrew> function (see gdbsupport/pathstuff.h), but unlike path_join the new
> > Andrew> function doesn't require that the first path be absolute.
> >
> > From what I can see, path_join doesn't have this requirement.
> > The docs say "The first element can be absolute or relative" and the
> > code seems to agree.
> 
> You're absolutely right.  And I see what's happened.  Somewhere along
> the line the logic has gotten reversed in my head.  The problem isn't
> that the first _must_ be absolute with path_join, it's that everything
> else _must_not_ be absolute, see these lines in path_join:
> 
>       if (i > 0)
> 	gdb_assert (strlen (path) == 0 || !IS_ABSOLUTE_PATH (path));
> 
> And for what we're doing here we, for example, join the path to the file
> (which is absolute) onto the debug directory, which might, or might not,
> be absolute, e.g. these lines in find_separate_debug_file:
> 
>   /* If the file is in the sysroot, try using its base path in
>      the global debugfile directory.  */
>   debugfile = path_join (target_prefix_str, debugdir.get (),
> 			 base_path, debuglink);
> 
> I've initially understood the problem, written the code, then come back
> and added comments and a commit message, and gotten mixed up somewhere.
> Sorry about that.
> 
> Anyway, here's an updated patch.  I've change one comment in symfile.c,
> and I've also updated the commit message, other than that the patch is
> unchanged.
> 
> Oh, and just because I was doubting myself, I did try using path_join,
> and as predicted the test (sysroot-debug-lookup.exp) causes GDB to
> trigger an assertion as we are appending an absolute path.

I suggest to test this with the first argument empty and the second
starting with two slashes: those should be left intact, at least on
MS-Windows (due to UNCs).

Also, how do we make sure none of the arguments except the first one
is an absolute file name?  Or if we cannot ensure that, how do we
handle the case where some non-first argument is an absolute file
name?

Btw, the GNU Coding Standards frown on using "path" for anything but
PATH-style directory lists.  The ones here should be called "file
name" of "file_name" or "fname" or "file" or "directory", but not
"path".

Re: [PATCH 1/2] gdb: avoid '//' in filenames when searching for debuginfo

[Andrew Burgess]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Andrew Burgess <aburgess@redhat.com>
>> Cc: gdb-patches@sourceware.org
>> Date: Fri, 14 Jun 2024 09:58:09 +0100
>> 
>> Tom Tromey <tom@tromey.com> writes:
>> 
>> >>>>>> "Andrew" == Andrew Burgess <aburgess@redhat.com> writes:
>> >
>> > Andrew> The new combine_paths function is similar to the existing path_join
>> > Andrew> function (see gdbsupport/pathstuff.h), but unlike path_join the new
>> > Andrew> function doesn't require that the first path be absolute.
>> >
>> > From what I can see, path_join doesn't have this requirement.
>> > The docs say "The first element can be absolute or relative" and the
>> > code seems to agree.
>> 
>> You're absolutely right.  And I see what's happened.  Somewhere along
>> the line the logic has gotten reversed in my head.  The problem isn't
>> that the first _must_ be absolute with path_join, it's that everything
>> else _must_not_ be absolute, see these lines in path_join:
>> 
>>       if (i > 0)
>> 	gdb_assert (strlen (path) == 0 || !IS_ABSOLUTE_PATH (path));
>> 
>> And for what we're doing here we, for example, join the path to the file
>> (which is absolute) onto the debug directory, which might, or might not,
>> be absolute, e.g. these lines in find_separate_debug_file:
>> 
>>   /* If the file is in the sysroot, try using its base path in
>>      the global debugfile directory.  */
>>   debugfile = path_join (target_prefix_str, debugdir.get (),
>> 			 base_path, debuglink);
>> 
>> I've initially understood the problem, written the code, then come back
>> and added comments and a commit message, and gotten mixed up somewhere.
>> Sorry about that.
>> 
>> Anyway, here's an updated patch.  I've change one comment in symfile.c,
>> and I've also updated the commit message, other than that the patch is
>> unchanged.
>> 
>> Oh, and just because I was doubting myself, I did try using path_join,
>> and as predicted the test (sysroot-debug-lookup.exp) causes GDB to
>> trigger an assertion as we are appending an absolute path.
>
> I suggest to test this with the first argument empty and the second
> starting with two slashes: those should be left intact, at least on
> MS-Windows (due to UNCs).

Sorry, I'm feel foggy today... what is 'this' in this paragraph.

So, I went and read up what UNCs means, I believe it's filenames like:

  \\SERVER\foo\bar\woof

And I guess, if they can start with forward slashes then maybe this is
valid too?

  //SERVER/foo/bar/woof

I'm pretty sure that's not currently going to work, at least in some
places we'll try to do:

 /debug-directory//SERVER/foo/bar/woof

which probably isn't going to help.  Right now the new combine_path is
going to mush this into '/debug-directory/SERVER/foo/bar/woof' dropping
one of the '/' characters.  I can change this if the double slash is in
fact useful in the middle of the filename, but I couldn't find anything
immediately that suggests it would be.

> Also, how do we make sure none of the arguments except the first one
> is an absolute file name?  Or if we cannot ensure that, how do we
> handle the case where some non-first argument is an absolute file
> name?

In path_join (gdbsupport/pathstuff.cc)?  We do this:

      if (i > 0)
	gdb_assert (strlen (path) == 0 || !IS_ABSOLUTE_PATH (path));

Where 'i' is the index into the array of filenames to combine, for the
first item we don't do any checks.  For the rest the filename needs to
either be empty, or not absolute.

For the new combine_paths (gdb/symfile.c)?  We don't do any checks,
that's the point.  Paths after the first one might be absolute, and we
still want to join them together.

>
> Btw, the GNU Coding Standards frown on using "path" for anything but
> PATH-style directory lists.  The ones here should be called "file
> name" of "file_name" or "fname" or "file" or "directory", but not
> "path".

I've updated some of the uses of path to filename in the patch below.
Mostly in the commit message.  Within GDB's code base the use of path to
mean filename or directory name is pretty much ubiquitous
unfortunately.  Still, I've tried to switch where I think it makes
sense.

OOI, what should we use when we don't know if something is a filename or
directory name?  e.g. a function that joins such things (whatever they
are called) together?  i.e. path_join might be used to join together two
directory names.  Or a directory name and a filename.  If we call the
arguments 'filename' then that seems confusing, a directory name is just
as valid.  And similarly, using 'directory name' seems wrong too.  This,
I think, is where 'path' starts to creep in.  Plus you have libc
functions like 'realpath' which use 'path' in the same way....

Anyway.  An updated patch is below.

Thanks,
Andrew

---

commit 74235cbf9db8f364ff8ed46898c34ccc6f38c5b9
Author: Andrew Burgess <aburgess@redhat.com>
Date:   Wed Jun 12 15:24:46 2024 +0100

    gdb: avoid '//' in filenames when searching for debuginfo
    
    I spotted that the gdb.base/sysroot-debug-lookup.exp test that I added
    recently actually had a KPASS when run with the
    native-extended-gdbserver board.  This was an oversight when adding
    the test.
    
    The failures in this test, when using the 'unix' board, are logged as
    bug PR gdb/31804.  The problem appears to be caused by the use of the
    child_path function in find_separate_debug_file.
    
    What happens on the 'unix' board is that the file is specified to GDB
    with a target: prefix, however GDB spots that the target filesystem is
    local to GDB and so opens the file without a target: prefix.  When we
    call into find_separate_debug_file the DIR and CANON_DIR arguments,
    which are computed from the objfile_name() no longer have a target:
    prefix.
    
    However, in this test if the file was opened with a target: prefix,
    then the sysroot also has a target: prefix.  When child_path is called
    it looks for a common prefix between CANON_DIR (from the objfile_name)
    and the sysroot.  However, the sysroot still has the target: prefix,
    which means the child_path() call fails and returns nullptr.
    
    What happens in the native-extended-gdbserver case is that GDB doesn't
    see the target filesystem as local.  Now the filename retains the
    target: prefix, which means that in the child_path() call both the
    sysroot and the CANON_DIR have a target: prefix, and so the
    child_path() call succeeds.  This allows GDB to progress, try some
    additional paths, and then find the debug information.
    
    So, this commit changes gdb.base/sysroot-debug-lookup.exp to expect
    the test to succeed when using the native-extended-gdbserver protocol.
    
    This leaves one KFAIL when using the native-extended-gdbserver board,
    we find the debug information but (apparently) find it in the wrong
    file.  What's happening is that when GDB builds the filename for the
    debug information we end up with a '//' string as a directory
    separator, the test regexp only expects a single separator.
    
    Instead of just fixing the test regexp, I've added a new function
    combine_paths which I then use for building the debug info filenames.
    This function takes care of ensuring only a single '/' is added when
    joining filenames together.  With this done I now have no KFAIL when
    using the native-extended-gdbserver board.
    
    The new combine_paths function is similar to the existing path_join
    function (see gdbsupport/pathstuff.h), but unlike path_join the new
    function doesn't require every filename after the first be a relative
    filename.  For now I've left the new function in symfile.c, it can
    always be moved if this proves to be more widely useful.
    
    After this commit we still have 2 KFAIL when not using the
    native-extended-gdbserver board.
    
    Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31804

diff --git a/gdb/symfile.c b/gdb/symfile.c
index 5a03def91c6..d8120aa00e0 100644
--- a/gdb/symfile.c
+++ b/gdb/symfile.c
@@ -1344,6 +1344,48 @@ show_debug_file_directory (struct ui_file *file, int from_tty,
 #define DEBUG_SUBDIRECTORY ".debug"
 #endif
 
+/* This is like path_join, but does not have the requirement that every
+   filename after the first be a relative filename.  All PATHS are joined
+   together and a directory separator is added if needed between joined
+   elements.
+
+   This allows us to do things like, e.g. append the absolute filename of a
+   file onto the debug file directory name.  */
+
+template<typename ...Args>
+static std::string
+combine_paths (Args... paths)
+{
+  /* It doesn't make sense to join less than two filenames.  */
+  static_assert (sizeof... (Args) >= 2);
+
+  std::array<const char *, sizeof... (Args)> path_array
+    { paths... };
+
+  std::string ret;
+
+  for (int i = 0; i < path_array.size (); ++i)
+    {
+      const char *path = path_array[i];
+
+      if (!ret.empty ())
+	{
+	  /* If RET doesn't end in a separator then add one now.  */
+	  if (!IS_DIR_SEPARATOR (ret.back ()))
+	    ret += '/';
+
+	  /* Now that RET ends in a separator ignore any at the start of
+	     PATH.  */
+	  while (IS_DIR_SEPARATOR (path[0]))
+	    ++path;
+	}
+
+      ret.append (path);
+    }
+
+  return ret;
+}
+
 /* Find a separate debuginfo file for OBJFILE, using DIR as the directory
    where the original file resides (may not be the same as
    dirname(objfile->name) due to symlinks), and DEBUGLINK as the file we are
@@ -1370,17 +1412,13 @@ find_separate_debug_file (const char *dir,
      objfile_name (objfile));
 
   /* First try in the same directory as the original file.  */
-  std::string debugfile = dir;
-  debugfile += debuglink;
+  std::string debugfile = combine_paths (dir, debuglink);
 
   if (separate_debug_file_exists (debugfile, crc32, objfile, warnings))
     return debugfile;
 
   /* Then try in the subdirectory named DEBUG_SUBDIRECTORY.  */
-  debugfile = dir;
-  debugfile += DEBUG_SUBDIRECTORY;
-  debugfile += "/";
-  debugfile += debuglink;
+  debugfile = combine_paths (dir, DEBUG_SUBDIRECTORY, debuglink);
 
   if (separate_debug_file_exists (debugfile, crc32, objfile, warnings))
     return debugfile;
@@ -1393,6 +1431,7 @@ find_separate_debug_file (const char *dir,
   bool target_prefix = is_target_filename (dir);
   const char *dir_notarget
     = target_prefix ? dir + strlen (TARGET_SYSROOT_PREFIX) : dir;
+  const char *target_prefix_str = target_prefix ? TARGET_SYSROOT_PREFIX : "";
   std::vector<gdb::unique_xmalloc_ptr<char>> debugdir_vec
     = dirnames_to_char_ptr_vec (debug_file_directory.c_str ());
   gdb::unique_xmalloc_ptr<char> canon_sysroot
@@ -1421,12 +1460,8 @@ find_separate_debug_file (const char *dir,
 
   for (const gdb::unique_xmalloc_ptr<char> &debugdir : debugdir_vec)
     {
-      debugfile = target_prefix ? TARGET_SYSROOT_PREFIX : "";
-      debugfile += debugdir;
-      debugfile += "/";
-      debugfile += drive;
-      debugfile += dir_notarget;
-      debugfile += debuglink;
+      debugfile = combine_paths (target_prefix_str, debugdir.get (),
+				 drive.c_str (), dir_notarget, debuglink);
 
       if (separate_debug_file_exists (debugfile, crc32, objfile, warnings))
 	return debugfile;
@@ -1443,12 +1478,8 @@ find_separate_debug_file (const char *dir,
 	{
 	  /* If the file is in the sysroot, try using its base path in
 	     the global debugfile directory.  */
-	  debugfile = target_prefix ? TARGET_SYSROOT_PREFIX : "";
-	  debugfile += debugdir;
-	  debugfile += "/";
-	  debugfile += base_path;
-	  debugfile += "/";
-	  debugfile += debuglink;
+	  debugfile = combine_paths (target_prefix_str, debugdir.get (),
+				     base_path, debuglink);
 
 	  if (separate_debug_file_exists (debugfile, crc32, objfile, warnings))
 	    return debugfile;
@@ -1461,21 +1492,18 @@ find_separate_debug_file (const char *dir,
 	     same result as above.  */
 	  if (gdb_sysroot != TARGET_SYSROOT_PREFIX)
 	    {
-	      debugfile = target_prefix ? TARGET_SYSROOT_PREFIX : "";
+	      std::string root;
 	      if (is_target_filename (gdb_sysroot))
 		{
-		  std::string root
-		    = gdb_sysroot.substr (strlen (TARGET_SYSROOT_PREFIX));
+		  root = gdb_sysroot.substr (strlen (TARGET_SYSROOT_PREFIX));
 		  gdb_assert (!root.empty ());
-		  debugfile += root;
 		}
 	      else
-		debugfile += gdb_sysroot;
-	      debugfile += debugdir;
-	      debugfile += "/";
-	      debugfile += base_path;
-	      debugfile += "/";
-	      debugfile += debuglink;
+		root = gdb_sysroot;
+
+	      debugfile = combine_paths (target_prefix_str, root.c_str (),
+					 debugdir.get (), base_path,
+					 debuglink);
 
 	      if (separate_debug_file_exists (debugfile, crc32, objfile,
 					      warnings))
diff --git a/gdb/testsuite/gdb.base/sysroot-debug-lookup.exp b/gdb/testsuite/gdb.base/sysroot-debug-lookup.exp
index 5f17315c027..30d492e0d47 100644
--- a/gdb/testsuite/gdb.base/sysroot-debug-lookup.exp
+++ b/gdb/testsuite/gdb.base/sysroot-debug-lookup.exp
@@ -183,13 +183,15 @@ proc_with_prefix lookup_via_debuglink {} {
 	#
 	# Bug PR gdb/30866 seems to be the (or a) relevant bug for
 	# this problem.
-	if { $sysroot_prefix ne "" } {
+	if { $sysroot_prefix ne ""
+	     && [target_info gdb_protocol] ne "extended-remote" } {
 	    setup_kfail "*-*-*" 31804
 	}
 	gdb_assert { $::gdb_file_cmd_debug_info eq "debug" } \
 	    "ensure debug information was found"
 
-	if { $sysroot_prefix ne "" } {
+	if { $sysroot_prefix ne ""
+	     && [target_info gdb_protocol] ne "extended-remote"  } {
 	    setup_kfail "*-*-*" 31804
 	}
 	set re [string_to_regexp "Reading symbols from ${sysroot_prefix}$debug_symlink..."]

Re: [PATCH 1/2] gdb: avoid '//' in filenames when searching for debuginfo

[Eli Zaretskii]

> From: Andrew Burgess <aburgess@redhat.com>
> Cc: tom@tromey.com, gdb-patches@sourceware.org
> Date: Fri, 14 Jun 2024 14:29:43 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > I suggest to test this with the first argument empty and the second
> > starting with two slashes: those should be left intact, at least on
> > MS-Windows (due to UNCs).
> 
> Sorry, I'm feel foggy today... what is 'this' in this paragraph.

I meant this patch.

> So, I went and read up what UNCs means, I believe it's filenames like:
> 
>   \\SERVER\foo\bar\woof
> 
> And I guess, if they can start with forward slashes then maybe this is
> valid too?
> 
>   //SERVER/foo/bar/woof

Yes, exactly.

> I'm pretty sure that's not currently going to work, at least in some
> places we'll try to do:
> 
>  /debug-directory//SERVER/foo/bar/woof
> 
> which probably isn't going to help.  Right now the new combine_path is
> going to mush this into '/debug-directory/SERVER/foo/bar/woof' dropping
> one of the '/' characters.  I can change this if the double slash is in
> fact useful in the middle of the filename, but I couldn't find anything
> immediately that suggests it would be.
> 
> > Also, how do we make sure none of the arguments except the first one
> > is an absolute file name?  Or if we cannot ensure that, how do we
> > handle the case where some non-first argument is an absolute file
> > name?
> 
> In path_join (gdbsupport/pathstuff.cc)?  We do this:
> 
>       if (i > 0)
> 	gdb_assert (strlen (path) == 0 || !IS_ABSOLUTE_PATH (path));
> 
> Where 'i' is the index into the array of filenames to combine, for the
> first item we don't do any checks.  For the rest the filename needs to
> either be empty, or not absolute.

Does IS_ABSOLUTE_PATH return non-zero for UNC file names?  If yes, we
are okay here, I think.

> For the new combine_paths (gdb/symfile.c)?  We don't do any checks,
> that's the point.  Paths after the first one might be absolute, and we
> still want to join them together.

How's that supposed to work on Windows?  Suppose the first file name
is d:/foo/bar and the second is x:/baz/quux -- what do we expect
combine_paths to produce in that case?

> > Btw, the GNU Coding Standards frown on using "path" for anything but
> > PATH-style directory lists.  The ones here should be called "file
> > name" of "file_name" or "fname" or "file" or "directory", but not
> > "path".
> 
> I've updated some of the uses of path to filename in the patch below.
> Mostly in the commit message.  Within GDB's code base the use of path to
> mean filename or directory name is pretty much ubiquitous
> unfortunately.  Still, I've tried to switch where I think it makes
> sense.

Thanks, that's a step in the right direction.

> OOI, what should we use when we don't know if something is a filename or
> directory name?  e.g. a function that joins such things (whatever they
> are called) together?  i.e. path_join might be used to join together two
> directory names.  Or a directory name and a filename.  If we call the
> arguments 'filename' then that seems confusing, a directory name is just
> as valid.  And similarly, using 'directory name' seems wrong too.  This,
> I think, is where 'path' starts to creep in.  Plus you have libc
> functions like 'realpath' which use 'path' in the same way....

A directory is also a file, so using "file name" in those cases is
okay.

Re: [PATCH 1/2] gdb: avoid '//' in filenames when searching for debuginfo

[Andrew Burgess]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Andrew Burgess <aburgess@redhat.com>
>> Cc: tom@tromey.com, gdb-patches@sourceware.org
>> Date: Fri, 14 Jun 2024 14:29:43 +0100
>> 
>> Eli Zaretskii <eliz@gnu.org> writes:
>> 
>> > I suggest to test this with the first argument empty and the second
>> > starting with two slashes: those should be left intact, at least on
>> > MS-Windows (due to UNCs).
>> 
>> Sorry, I'm feel foggy today... what is 'this' in this paragraph.
>
> I meant this patch.
>
>> So, I went and read up what UNCs means, I believe it's filenames like:
>> 
>>   \\SERVER\foo\bar\woof
>> 
>> And I guess, if they can start with forward slashes then maybe this is
>> valid too?
>> 
>>   //SERVER/foo/bar/woof
>
> Yes, exactly.
>
>> I'm pretty sure that's not currently going to work, at least in some
>> places we'll try to do:
>> 
>>  /debug-directory//SERVER/foo/bar/woof
>> 
>> which probably isn't going to help.  Right now the new combine_path is
>> going to mush this into '/debug-directory/SERVER/foo/bar/woof' dropping
>> one of the '/' characters.  I can change this if the double slash is in
>> fact useful in the middle of the filename, but I couldn't find anything
>> immediately that suggests it would be.
>> 
>> > Also, how do we make sure none of the arguments except the first one
>> > is an absolute file name?  Or if we cannot ensure that, how do we
>> > handle the case where some non-first argument is an absolute file
>> > name?
>> 
>> In path_join (gdbsupport/pathstuff.cc)?  We do this:
>> 
>>       if (i > 0)
>> 	gdb_assert (strlen (path) == 0 || !IS_ABSOLUTE_PATH (path));
>> 
>> Where 'i' is the index into the array of filenames to combine, for the
>> first item we don't do any checks.  For the rest the filename needs to
>> either be empty, or not absolute.
>
> Does IS_ABSOLUTE_PATH return non-zero for UNC file names?  If yes, we
> are okay here, I think.

IS_ABSOLUTE_PATH is from libiberty, and can be found in
include/filenames.h.  It does not handle UNC file names, but it does
check for drive specs.

However, there is an existing problem which is documented in
find_separate_debug_file (gdb/symfile.c), the libiberty IS_ABSOLUTE_PATH
function (and friends) rely on checking whether the host is DOS based or
not.  The existing code will not handle DOS based paths correctly if the
host is non-DOS based, but the debug target _is_ DOS based.

>
>> For the new combine_paths (gdb/symfile.c)?  We don't do any checks,
>> that's the point.  Paths after the first one might be absolute, and we
>> still want to join them together.
>
> How's that supposed to work on Windows?  Suppose the first file name
> is d:/foo/bar and the second is x:/baz/quux -- what do we expect
> combine_paths to produce in that case?

OK, lets try and work through an example.  I don't have a Windows
machine to test on so I'm working through this by looking at the code.
I might make mistakes.

Our inputs:

File we're debugging:	d:/foo/bar/file
Debug link name:	file.debug
DEBUG_SUBDIRECTORY:	.debug
debug-file-directory:   x:/baz/quux/
sysroot:                d:/foo/

Locations checked for debug, in this order:

  1. d:/foo/bar/file.debug
  2. d:/foo/bar/.debug/file.debug
  3. x:/baz/quux/d/foo/bar/file.debug
  4. x:/baz/quux/bar/file.debug
  5. d:/foo/x:/baz/quux/bar/file.debug

Of these, only (5) is obviously wrong I think as it didn't consider that
the debug-file-directory might be an absolute path on a DOS based host.
This code was introduced in commit:

  commit 402d2bfec425f29c5b54089d5ff98ca9a1b8ec27
  Date:   Tue Feb 12 13:56:16 2019 -0800
  
      Look for separate debug files in debug directories under a sysroot.

As far as _this_ commit is concerned the _only_ change that might impact
DOS based hosts is that, when joining absolute paths, I convert every
instance of "//" within a path to "/".  This only happens for paths
after the fist one though, so if the first path starts with
"//server/file" that will remain as it is.  But if we try to do this:

  combine_paths ("//server_a/aaa", "//server_b/bbb")

then we'd get:

  "//server_a/aaa/server_b/bbb"

which might not be what we want.

I'm happy to drop the code that squashes the multiple "/" to a single
"/", this would mean the above came out as:

  "//server_a/aaa//server_b/bbb"

But is that actually useful?

I can potentially take a crack at fixing case (5) above once this series
lands, but I'd be doing so blind as I don't have a DOS based machine to
test on ... but maybe you'd be up for testing a patch for me in that
area?

Dealing with UNC paths in general is going to require more extensive
work I suspect.  Right now we're only really handling drive letters.  I
think that work would be better done by someone with both a need and the
ability to test their work.

Let me know your thoughts,

Thanks,
Andrew

Re: [PATCH 1/2] gdb: avoid '//' in filenames when searching for debuginfo

[Eli Zaretskii]

> From: Andrew Burgess <aburgess@redhat.com>
> Cc: tom@tromey.com, gdb-patches@sourceware.org
> Date: Sat, 15 Jun 2024 11:24:13 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> Our inputs:
> 
> File we're debugging:	d:/foo/bar/file
> Debug link name:	file.debug
> DEBUG_SUBDIRECTORY:	.debug
> debug-file-directory:   x:/baz/quux/
> sysroot:                d:/foo/
> 
> Locations checked for debug, in this order:
> 
>   1. d:/foo/bar/file.debug
>   2. d:/foo/bar/.debug/file.debug
>   3. x:/baz/quux/d/foo/bar/file.debug
>   4. x:/baz/quux/bar/file.debug
>   5. d:/foo/x:/baz/quux/bar/file.debug
> 
> Of these, only (5) is obviously wrong I think as it didn't consider that
> the debug-file-directory might be an absolute path on a DOS based host.
> This code was introduced in commit:
> 
>   commit 402d2bfec425f29c5b54089d5ff98ca9a1b8ec27
>   Date:   Tue Feb 12 13:56:16 2019 -0800
>   
>       Look for separate debug files in debug directories under a sysroot.

That should be fixed, IMO.

> As far as _this_ commit is concerned the _only_ change that might impact
> DOS based hosts is that, when joining absolute paths, I convert every
> instance of "//" within a path to "/".  This only happens for paths
> after the fist one though, so if the first path starts with
> "//server/file" that will remain as it is.  But if we try to do this:
> 
>   combine_paths ("//server_a/aaa", "//server_b/bbb")
> 
> then we'd get:
> 
>   "//server_a/aaa/server_b/bbb"
> 
> which might not be what we want.

Given that we convert d:/ into d/ for the purposes of concatenation, I
think "//server_a/aaa/server_b/bbb" _is_ what we want.

> Dealing with UNC paths in general is going to require more extensive
> work I suspect.

Maybe, but right now it sounds like we are either already close or
maybe even there.

Thanks.

Re: [PATCH 1/2] gdb: avoid '//' in filenames when searching for debuginfo

[Andrew Burgess]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Andrew Burgess <aburgess@redhat.com>
>> Cc: tom@tromey.com, gdb-patches@sourceware.org
>> Date: Sat, 15 Jun 2024 11:24:13 +0100
>> 
>> Eli Zaretskii <eliz@gnu.org> writes:
>> 
>> Our inputs:
>> 
>> File we're debugging:	d:/foo/bar/file
>> Debug link name:	file.debug
>> DEBUG_SUBDIRECTORY:	.debug
>> debug-file-directory:   x:/baz/quux/
>> sysroot:                d:/foo/
>> 
>> Locations checked for debug, in this order:
>> 
>>   1. d:/foo/bar/file.debug
>>   2. d:/foo/bar/.debug/file.debug
>>   3. x:/baz/quux/d/foo/bar/file.debug
>>   4. x:/baz/quux/bar/file.debug
>>   5. d:/foo/x:/baz/quux/bar/file.debug
>> 
>> Of these, only (5) is obviously wrong I think as it didn't consider that
>> the debug-file-directory might be an absolute path on a DOS based host.
>> This code was introduced in commit:
>> 
>>   commit 402d2bfec425f29c5b54089d5ff98ca9a1b8ec27
>>   Date:   Tue Feb 12 13:56:16 2019 -0800
>>   
>>       Look for separate debug files in debug directories under a sysroot.
>
> That should be fixed, IMO.

And I'm willing to have a go, but that's a bug that exists before my
patch, and is not made better or worse by my patch... it just happens to
be in the same area, so hopefully it shouldn't block this patch moving
forward? 

>
>> As far as _this_ commit is concerned the _only_ change that might impact
>> DOS based hosts is that, when joining absolute paths, I convert every
>> instance of "//" within a path to "/".  This only happens for paths
>> after the fist one though, so if the first path starts with
>> "//server/file" that will remain as it is.  But if we try to do this:
>> 
>>   combine_paths ("//server_a/aaa", "//server_b/bbb")
>> 
>> then we'd get:
>> 
>>   "//server_a/aaa/server_b/bbb"
>> 
>> which might not be what we want.
>
> Given that we convert d:/ into d/ for the purposes of concatenation, I
> think "//server_a/aaa/server_b/bbb" _is_ what we want.

Awesome, so I think as far as this patch goes, there's nothing blocking
it, right?

I've attached below a patch which might fix the drive letter issue in
the last case I described previously.

Here's what I think you'll need to do to test this....

  (1) compile a test binary.

  (2) split the debug into a separate file, e.g.:
      objcopy --only-keep-debug binary binary.debug
      objcopy --strip-debug binary binary.tmp
      mv binary.tmp binary

  (3) Add a debug link to the test binary, e.g.:
      objcopy --add-gnu-debuglink=binary.debug binary binary.tmp
      mv binary.tmp binary

  (4) Start GDB, but don't load the test binary yet.

  (5) Setup the sysroot to something containing a drive letter, e.g.:
      (gdb) set sysroot x:/sysroot/

  (6) Setup the debug directory to something containing a drive letter,
      maybe a different drive letter? e.g.:
      (gdb) set debug-file-directory z:/debug/

  (7) Place the test binary within the sysroot, e.g.:
      x:/sysroot/blah/binary

  (8) Place the debug information into a file:
      x:/sysroot/z/debug/blah/binary.debug

  (9) Tell GDB to load the test binary, and hopefully observe the debug
      symbols being loaded:
      (gdb) file x:/sysroot/blah/binary

Even just typing that out this search strategy doesn't make much sense,
I can't imagine this every being a use case that anyone would need.  I
suspect this makes more sense if the debug-file-directory is a relative
path, with no drive letter, e.g. is "the location of debug within the
sysroot", but the sysroot itself might change location between different
machines maybe?

If the above doesn't work (as I said, I'm writing this with no test
machine availble) you can turn 'set debug separate-debug-file on',
repeat step (9) and report the results, this will show where GDB is
trying to access.

Anyway, if this does work I'm happy to make it into a real patch and
post it upstream.

Thanks,
Andrew

---

diff --git a/gdb/symfile.c b/gdb/symfile.c
index 2575c7f37c8..61daa65c765 100644
--- a/gdb/symfile.c
+++ b/gdb/symfile.c
@@ -1504,9 +1504,20 @@ find_separate_debug_file (const char *dir,
 	      else
 		root = gdb_sysroot;
 
+	      /* If DEBUGDIR has a drive letter then we're still going to
+		 search within the sysroot, convert the drive to another
+		 directory in the path.  */
+	      const char *debugdir_str = debugdir.get ();
+	      drive.clear ();
+	      if (HAS_DRIVE_SPEC (debugdir.get ())
+		{
+		  drive = debugdir.get ()[0];
+		  debugdir_str = STRIP_DRIVE_SPEC (debugdir_str);
+		}
+
 	      debugfile = combine_paths (target_prefix_str, root.c_str (),
-					 debugdir.get (), base_path,
-					 debuglink);
+					 drive.c_str (), debugdir_str,
+					 base_path, debuglink);
 
 	      if (separate_debug_file_exists (debugfile, crc32, objfile,
 					      warnings))

      

Re: [PATCH 1/2] gdb: avoid '//' in filenames when searching for debuginfo

[Andrew Burgess]

Turns out you need to save a file before you generate a diff if you want
to get the latest changes!

Andrew Burgess <aburgess@redhat.com> writes:

>
> diff --git a/gdb/symfile.c b/gdb/symfile.c
> index 2575c7f37c8..61daa65c765 100644
> --- a/gdb/symfile.c
> +++ b/gdb/symfile.c
> @@ -1504,9 +1504,20 @@ find_separate_debug_file (const char *dir,
>  	      else
>  		root = gdb_sysroot;
>  
> +	      /* If DEBUGDIR has a drive letter then we're still going to
> +		 search within the sysroot, convert the drive to another
> +		 directory in the path.  */
> +	      const char *debugdir_str = debugdir.get ();
> +	      drive.clear ();
> +	      if (HAS_DRIVE_SPEC (debugdir.get ())

There's a closing ')' missing from the end of this line!

Sorry about that,
Andrew


> +		{
> +		  drive = debugdir.get ()[0];
> +		  debugdir_str = STRIP_DRIVE_SPEC (debugdir_str);
> +		}
> +
>  	      debugfile = combine_paths (target_prefix_str, root.c_str (),
> -					 debugdir.get (), base_path,
> -					 debuglink);
> +					 drive.c_str (), debugdir_str,
> +					 base_path, debuglink);
>  
>  	      if (separate_debug_file_exists (debugfile, crc32, objfile,
>  					      warnings))
>
>       

Re: [PATCH 1/2] gdb: avoid '//' in filenames when searching for debuginfo

[Eli Zaretskii]

> From: Andrew Burgess <aburgess@redhat.com>
> Cc: tom@tromey.com, gdb-patches@sourceware.org
> Date: Sat, 15 Jun 2024 16:28:53 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >>   1. d:/foo/bar/file.debug
> >>   2. d:/foo/bar/.debug/file.debug
> >>   3. x:/baz/quux/d/foo/bar/file.debug
> >>   4. x:/baz/quux/bar/file.debug
> >>   5. d:/foo/x:/baz/quux/bar/file.debug
> >> 
> >> Of these, only (5) is obviously wrong I think as it didn't consider that
> >> the debug-file-directory might be an absolute path on a DOS based host.
> >> This code was introduced in commit:
> >> 
> >>   commit 402d2bfec425f29c5b54089d5ff98ca9a1b8ec27
> >>   Date:   Tue Feb 12 13:56:16 2019 -0800
> >>   
> >>       Look for separate debug files in debug directories under a sysroot.
> >
> > That should be fixed, IMO.
> 
> And I'm willing to have a go, but that's a bug that exists before my
> patch, and is not made better or worse by my patch... it just happens to
> be in the same area, so hopefully it shouldn't block this patch moving
> forward? 

Of course not.

> > Given that we convert d:/ into d/ for the purposes of concatenation, I
> > think "//server_a/aaa/server_b/bbb" _is_ what we want.
> 
> Awesome, so I think as far as this patch goes, there's nothing blocking
> it, right?

Not from my POV, no.

> I've attached below a patch which might fix the drive letter issue in
> the last case I described previously.
> 
> Here's what I think you'll need to do to test this....

Thanks, but I won't have time to do this any time soon, sorry.  (I
also am not sure all the separate debug info stuff will work on
MS-Windows.)  If no one else can, I suggest to install this, and
someone, perhaps even myself, will test that at some future point.

Thanks.

Re: [PATCH 1/2] gdb: avoid '//' in filenames when searching for debuginfo

[Pedro Alves]

On 2024-06-14 14:29, Andrew Burgess wrote:
> Eli Zaretskii <eliz@gnu.org> writes:
> 
>>> From: Andrew Burgess <aburgess@redhat.com>
>>> Cc: gdb-patches@sourceware.org
>>> Date: Fri, 14 Jun 2024 09:58:09 +0100
>>>
>>> Tom Tromey <tom@tromey.com> writes:
>>>
>>>>>>>>> "Andrew" == Andrew Burgess <aburgess@redhat.com> writes:
>>>>
>>>> Andrew> The new combine_paths function is similar to the existing path_join
>>>> Andrew> function (see gdbsupport/pathstuff.h), but unlike path_join the new
>>>> Andrew> function doesn't require that the first path be absolute.
>>>>
>>>> From what I can see, path_join doesn't have this requirement.
>>>> The docs say "The first element can be absolute or relative" and the
>>>> code seems to agree.
>>>
>>> You're absolutely right.  And I see what's happened.  Somewhere along
>>> the line the logic has gotten reversed in my head.  The problem isn't
>>> that the first _must_ be absolute with path_join, it's that everything
>>> else _must_not_ be absolute, see these lines in path_join:
>>>
>>>       if (i > 0)
>>> 	gdb_assert (strlen (path) == 0 || !IS_ABSOLUTE_PATH (path));
>>>
>>> And for what we're doing here we, for example, join the path to the file
>>> (which is absolute) onto the debug directory, which might, or might not,
>>> be absolute, e.g. these lines in find_separate_debug_file:
>>>
>>>   /* If the file is in the sysroot, try using its base path in
>>>      the global debugfile directory.  */
>>>   debugfile = path_join (target_prefix_str, debugdir.get (),
>>> 			 base_path, debuglink);
>>>
>>> I've initially understood the problem, written the code, then come back
>>> and added comments and a commit message, and gotten mixed up somewhere.
>>> Sorry about that.
>>>
>>> Anyway, here's an updated patch.  I've change one comment in symfile.c,
>>> and I've also updated the commit message, other than that the patch is
>>> unchanged.
>>>
>>> Oh, and just because I was doubting myself, I did try using path_join,
>>> and as predicted the test (sysroot-debug-lookup.exp) causes GDB to
>>> trigger an assertion as we are appending an absolute path.
>>
>> I suggest to test this with the first argument empty and the second
>> starting with two slashes: those should be left intact, at least on
>> MS-Windows (due to UNCs).
> 
> Sorry, I'm feel foggy today... what is 'this' in this paragraph.
> 
> So, I went and read up what UNCs means, I believe it's filenames like:
> 
>   \\SERVER\foo\bar\woof
> 
> And I guess, if they can start with forward slashes then maybe this is
> valid too?
> 
>   //SERVER/foo/bar/woof
> 
> I'm pretty sure that's not currently going to work, at least in some
> places we'll try to do:
> 
>  /debug-directory//SERVER/foo/bar/woof
> 
> which probably isn't going to help.  Right now the new combine_path is
> going to mush this into '/debug-directory/SERVER/foo/bar/woof' dropping
> one of the '/' characters.  I can change this if the double slash is in
> fact useful in the middle of the filename, but I couldn't find anything
> immediately that suggests it would be.
> 
>> Also, how do we make sure none of the arguments except the first one
>> is an absolute file name?  Or if we cannot ensure that, how do we
>> handle the case where some non-first argument is an absolute file
>> name?
> 
> In path_join (gdbsupport/pathstuff.cc)?  We do this:
> 
>       if (i > 0)
> 	gdb_assert (strlen (path) == 0 || !IS_ABSOLUTE_PATH (path));
> 
> Where 'i' is the index into the array of filenames to combine, for the
> first item we don't do any checks.  For the rest the filename needs to
> either be empty, or not absolute.
> 
> For the new combine_paths (gdb/symfile.c)?  We don't do any checks,
> that's the point.  Paths after the first one might be absolute, and we
> still want to join them together.
> 
>>
>> Btw, the GNU Coding Standards frown on using "path" for anything but
>> PATH-style directory lists.  The ones here should be called "file
>> name" of "file_name" or "fname" or "file" or "directory", but not
>> "path".
> 
> I've updated some of the uses of path to filename in the patch below.
> Mostly in the commit message.  Within GDB's code base the use of path to
> mean filename or directory name is pretty much ubiquitous
> unfortunately.  Still, I've tried to switch where I think it makes
> sense.
> 
> OOI, what should we use when we don't know if something is a filename or
> directory name?  e.g. a function that joins such things (whatever they
> are called) together?  i.e. path_join might be used to join together two
> directory names.  Or a directory name and a filename.  If we call the
> arguments 'filename' then that seems confusing, a directory name is just
> as valid.  And similarly, using 'directory name' seems wrong too.  This,
> I think, is where 'path' starts to creep in.  Plus you have libc
> functions like 'realpath' which use 'path' in the same way....
> 
> Anyway.  An updated patch is below.
> 
> Thanks,
> Andrew
> 
> ---
> 
> commit 74235cbf9db8f364ff8ed46898c34ccc6f38c5b9
> Author: Andrew Burgess <aburgess@redhat.com>
> Date:   Wed Jun 12 15:24:46 2024 +0100
> 
>     gdb: avoid '//' in filenames when searching for debuginfo
>     
>     I spotted that the gdb.base/sysroot-debug-lookup.exp test that I added
>     recently actually had a KPASS when run with the
>     native-extended-gdbserver board.  This was an oversight when adding
>     the test.
>     
>     The failures in this test, when using the 'unix' board, are logged as
>     bug PR gdb/31804.  The problem appears to be caused by the use of the
>     child_path function in find_separate_debug_file.
>     
>     What happens on the 'unix' board is that the file is specified to GDB
>     with a target: prefix, however GDB spots that the target filesystem is
>     local to GDB and so opens the file without a target: prefix.  When we
>     call into find_separate_debug_file the DIR and CANON_DIR arguments,
>     which are computed from the objfile_name() no longer have a target:
>     prefix.
>     
>     However, in this test if the file was opened with a target: prefix,
>     then the sysroot also has a target: prefix.  When child_path is called
>     it looks for a common prefix between CANON_DIR (from the objfile_name)
>     and the sysroot.  However, the sysroot still has the target: prefix,
>     which means the child_path() call fails and returns nullptr.
>     
>     What happens in the native-extended-gdbserver case is that GDB doesn't
>     see the target filesystem as local.  Now the filename retains the
>     target: prefix, which means that in the child_path() call both the
>     sysroot and the CANON_DIR have a target: prefix, and so the
>     child_path() call succeeds.  This allows GDB to progress, try some
>     additional paths, and then find the debug information.
>     
>     So, this commit changes gdb.base/sysroot-debug-lookup.exp to expect
>     the test to succeed when using the native-extended-gdbserver protocol.
>     
>     This leaves one KFAIL when using the native-extended-gdbserver board,
>     we find the debug information but (apparently) find it in the wrong
>     file.  What's happening is that when GDB builds the filename for the
>     debug information we end up with a '//' string as a directory
>     separator, the test regexp only expects a single separator.
>     
>     Instead of just fixing the test regexp, I've added a new function
>     combine_paths which I then use for building the debug info filenames.
>     This function takes care of ensuring only a single '/' is added when
>     joining filenames together.  With this done I now have no KFAIL when
>     using the native-extended-gdbserver board.
>     
>     The new combine_paths function is similar to the existing path_join
>     function (see gdbsupport/pathstuff.h), but unlike path_join the new
>     function doesn't require every filename after the first be a relative
>     filename.  For now I've left the new function in symfile.c, it can
>     always be moved if this proves to be more widely useful.
>     
>     After this commit we still have 2 KFAIL when not using the
>     native-extended-gdbserver board.
>     
>     Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31804
> 
> diff --git a/gdb/symfile.c b/gdb/symfile.c
> index 5a03def91c6..d8120aa00e0 100644
> --- a/gdb/symfile.c
> +++ b/gdb/symfile.c
> @@ -1344,6 +1344,48 @@ show_debug_file_directory (struct ui_file *file, int from_tty,
>  #define DEBUG_SUBDIRECTORY ".debug"
>  #endif
>  
> +/* This is like path_join, but does not have the requirement that every
> +   filename after the first be a relative filename.  All PATHS are joined
> +   together and a directory separator is added if needed between joined
> +   elements.
> +
> +   This allows us to do things like, e.g. append the absolute filename of a
> +   file onto the debug file directory name.  */
> +
> +template<typename ...Args>
> +static std::string
> +combine_paths (Args... paths)
> +{
> +  /* It doesn't make sense to join less than two filenames.  */
> +  static_assert (sizeof... (Args) >= 2);
> +
> +  std::array<const char *, sizeof... (Args)> path_array
> +    { paths... };
> +
> +  std::string ret;
> +
> +  for (int i = 0; i < path_array.size (); ++i)
> +    {
> +      const char *path = path_array[i];
> +
> +      if (!ret.empty ())
> +	{
> +	  /* If RET doesn't end in a separator then add one now.  */
> +	  if (!IS_DIR_SEPARATOR (ret.back ()))
> +	    ret += '/';
> +
> +	  /* Now that RET ends in a separator ignore any at the start of
> +	     PATH.  */
> +	  while (IS_DIR_SEPARATOR (path[0]))
> +	    ++path;
> +	}
> +
> +      ret.append (path);
> +    }
> +
> +  return ret;
> +}

path_join does the bulk of the work in an array_view overload, which avoids
code bloat from having multiple instances of the same code for different
arguments.  I think this would benefit from doing the same.

Re: [PATCH 1/2] gdb: avoid '//' in filenames when searching for debuginfo

[Andrew Burgess]

Pedro Alves <pedro@palves.net> writes:

> On 2024-06-14 14:29, Andrew Burgess wrote:
>> Eli Zaretskii <eliz@gnu.org> writes:
>> 
>>>> From: Andrew Burgess <aburgess@redhat.com>
>>>> Cc: gdb-patches@sourceware.org
>>>> Date: Fri, 14 Jun 2024 09:58:09 +0100
>>>>
>>>> Tom Tromey <tom@tromey.com> writes:
>>>>
>>>>>>>>>> "Andrew" == Andrew Burgess <aburgess@redhat.com> writes:
>>>>>
>>>>> Andrew> The new combine_paths function is similar to the existing path_join
>>>>> Andrew> function (see gdbsupport/pathstuff.h), but unlike path_join the new
>>>>> Andrew> function doesn't require that the first path be absolute.
>>>>>
>>>>> From what I can see, path_join doesn't have this requirement.
>>>>> The docs say "The first element can be absolute or relative" and the
>>>>> code seems to agree.
>>>>
>>>> You're absolutely right.  And I see what's happened.  Somewhere along
>>>> the line the logic has gotten reversed in my head.  The problem isn't
>>>> that the first _must_ be absolute with path_join, it's that everything
>>>> else _must_not_ be absolute, see these lines in path_join:
>>>>
>>>>       if (i > 0)
>>>> 	gdb_assert (strlen (path) == 0 || !IS_ABSOLUTE_PATH (path));
>>>>
>>>> And for what we're doing here we, for example, join the path to the file
>>>> (which is absolute) onto the debug directory, which might, or might not,
>>>> be absolute, e.g. these lines in find_separate_debug_file:
>>>>
>>>>   /* If the file is in the sysroot, try using its base path in
>>>>      the global debugfile directory.  */
>>>>   debugfile = path_join (target_prefix_str, debugdir.get (),
>>>> 			 base_path, debuglink);
>>>>
>>>> I've initially understood the problem, written the code, then come back
>>>> and added comments and a commit message, and gotten mixed up somewhere.
>>>> Sorry about that.
>>>>
>>>> Anyway, here's an updated patch.  I've change one comment in symfile.c,
>>>> and I've also updated the commit message, other than that the patch is
>>>> unchanged.
>>>>
>>>> Oh, and just because I was doubting myself, I did try using path_join,
>>>> and as predicted the test (sysroot-debug-lookup.exp) causes GDB to
>>>> trigger an assertion as we are appending an absolute path.
>>>
>>> I suggest to test this with the first argument empty and the second
>>> starting with two slashes: those should be left intact, at least on
>>> MS-Windows (due to UNCs).
>> 
>> Sorry, I'm feel foggy today... what is 'this' in this paragraph.
>> 
>> So, I went and read up what UNCs means, I believe it's filenames like:
>> 
>>   \\SERVER\foo\bar\woof
>> 
>> And I guess, if they can start with forward slashes then maybe this is
>> valid too?
>> 
>>   //SERVER/foo/bar/woof
>> 
>> I'm pretty sure that's not currently going to work, at least in some
>> places we'll try to do:
>> 
>>  /debug-directory//SERVER/foo/bar/woof
>> 
>> which probably isn't going to help.  Right now the new combine_path is
>> going to mush this into '/debug-directory/SERVER/foo/bar/woof' dropping
>> one of the '/' characters.  I can change this if the double slash is in
>> fact useful in the middle of the filename, but I couldn't find anything
>> immediately that suggests it would be.
>> 
>>> Also, how do we make sure none of the arguments except the first one
>>> is an absolute file name?  Or if we cannot ensure that, how do we
>>> handle the case where some non-first argument is an absolute file
>>> name?
>> 
>> In path_join (gdbsupport/pathstuff.cc)?  We do this:
>> 
>>       if (i > 0)
>> 	gdb_assert (strlen (path) == 0 || !IS_ABSOLUTE_PATH (path));
>> 
>> Where 'i' is the index into the array of filenames to combine, for the
>> first item we don't do any checks.  For the rest the filename needs to
>> either be empty, or not absolute.
>> 
>> For the new combine_paths (gdb/symfile.c)?  We don't do any checks,
>> that's the point.  Paths after the first one might be absolute, and we
>> still want to join them together.
>> 
>>>
>>> Btw, the GNU Coding Standards frown on using "path" for anything but
>>> PATH-style directory lists.  The ones here should be called "file
>>> name" of "file_name" or "fname" or "file" or "directory", but not
>>> "path".
>> 
>> I've updated some of the uses of path to filename in the patch below.
>> Mostly in the commit message.  Within GDB's code base the use of path to
>> mean filename or directory name is pretty much ubiquitous
>> unfortunately.  Still, I've tried to switch where I think it makes
>> sense.
>> 
>> OOI, what should we use when we don't know if something is a filename or
>> directory name?  e.g. a function that joins such things (whatever they
>> are called) together?  i.e. path_join might be used to join together two
>> directory names.  Or a directory name and a filename.  If we call the
>> arguments 'filename' then that seems confusing, a directory name is just
>> as valid.  And similarly, using 'directory name' seems wrong too.  This,
>> I think, is where 'path' starts to creep in.  Plus you have libc
>> functions like 'realpath' which use 'path' in the same way....
>> 
>> Anyway.  An updated patch is below.
>> 
>> Thanks,
>> Andrew
>> 
>> ---
>> 
>> commit 74235cbf9db8f364ff8ed46898c34ccc6f38c5b9
>> Author: Andrew Burgess <aburgess@redhat.com>
>> Date:   Wed Jun 12 15:24:46 2024 +0100
>> 
>>     gdb: avoid '//' in filenames when searching for debuginfo
>>     
>>     I spotted that the gdb.base/sysroot-debug-lookup.exp test that I added
>>     recently actually had a KPASS when run with the
>>     native-extended-gdbserver board.  This was an oversight when adding
>>     the test.
>>     
>>     The failures in this test, when using the 'unix' board, are logged as
>>     bug PR gdb/31804.  The problem appears to be caused by the use of the
>>     child_path function in find_separate_debug_file.
>>     
>>     What happens on the 'unix' board is that the file is specified to GDB
>>     with a target: prefix, however GDB spots that the target filesystem is
>>     local to GDB and so opens the file without a target: prefix.  When we
>>     call into find_separate_debug_file the DIR and CANON_DIR arguments,
>>     which are computed from the objfile_name() no longer have a target:
>>     prefix.
>>     
>>     However, in this test if the file was opened with a target: prefix,
>>     then the sysroot also has a target: prefix.  When child_path is called
>>     it looks for a common prefix between CANON_DIR (from the objfile_name)
>>     and the sysroot.  However, the sysroot still has the target: prefix,
>>     which means the child_path() call fails and returns nullptr.
>>     
>>     What happens in the native-extended-gdbserver case is that GDB doesn't
>>     see the target filesystem as local.  Now the filename retains the
>>     target: prefix, which means that in the child_path() call both the
>>     sysroot and the CANON_DIR have a target: prefix, and so the
>>     child_path() call succeeds.  This allows GDB to progress, try some
>>     additional paths, and then find the debug information.
>>     
>>     So, this commit changes gdb.base/sysroot-debug-lookup.exp to expect
>>     the test to succeed when using the native-extended-gdbserver protocol.
>>     
>>     This leaves one KFAIL when using the native-extended-gdbserver board,
>>     we find the debug information but (apparently) find it in the wrong
>>     file.  What's happening is that when GDB builds the filename for the
>>     debug information we end up with a '//' string as a directory
>>     separator, the test regexp only expects a single separator.
>>     
>>     Instead of just fixing the test regexp, I've added a new function
>>     combine_paths which I then use for building the debug info filenames.
>>     This function takes care of ensuring only a single '/' is added when
>>     joining filenames together.  With this done I now have no KFAIL when
>>     using the native-extended-gdbserver board.
>>     
>>     The new combine_paths function is similar to the existing path_join
>>     function (see gdbsupport/pathstuff.h), but unlike path_join the new
>>     function doesn't require every filename after the first be a relative
>>     filename.  For now I've left the new function in symfile.c, it can
>>     always be moved if this proves to be more widely useful.
>>     
>>     After this commit we still have 2 KFAIL when not using the
>>     native-extended-gdbserver board.
>>     
>>     Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31804
>> 
>> diff --git a/gdb/symfile.c b/gdb/symfile.c
>> index 5a03def91c6..d8120aa00e0 100644
>> --- a/gdb/symfile.c
>> +++ b/gdb/symfile.c
>> @@ -1344,6 +1344,48 @@ show_debug_file_directory (struct ui_file *file, int from_tty,
>>  #define DEBUG_SUBDIRECTORY ".debug"
>>  #endif
>>  
>> +/* This is like path_join, but does not have the requirement that every
>> +   filename after the first be a relative filename.  All PATHS are joined
>> +   together and a directory separator is added if needed between joined
>> +   elements.
>> +
>> +   This allows us to do things like, e.g. append the absolute filename of a
>> +   file onto the debug file directory name.  */
>> +
>> +template<typename ...Args>
>> +static std::string
>> +combine_paths (Args... paths)
>> +{
>> +  /* It doesn't make sense to join less than two filenames.  */
>> +  static_assert (sizeof... (Args) >= 2);
>> +
>> +  std::array<const char *, sizeof... (Args)> path_array
>> +    { paths... };
>> +
>> +  std::string ret;
>> +
>> +  for (int i = 0; i < path_array.size (); ++i)
>> +    {
>> +      const char *path = path_array[i];
>> +
>> +      if (!ret.empty ())
>> +	{
>> +	  /* If RET doesn't end in a separator then add one now.  */
>> +	  if (!IS_DIR_SEPARATOR (ret.back ()))
>> +	    ret += '/';
>> +
>> +	  /* Now that RET ends in a separator ignore any at the start of
>> +	     PATH.  */
>> +	  while (IS_DIR_SEPARATOR (path[0]))
>> +	    ++path;
>> +	}
>> +
>> +      ret.append (path);
>> +    }
>> +
>> +  return ret;
>> +}
>
> path_join does the bulk of the work in an array_view overload, which avoids
> code bloat from having multiple instances of the same code for different
> arguments.  I think this would benefit from doing the same.

OK, I'll see how the discussion with Tom goes, but if this function
doesn't end up being deleted then I'll rewrite this with an array_view
worker core.

Thanks,
Andrew

Re: [PATCH 1/2] gdb: avoid '//' in filenames when searching for debuginfo

[Tom Tromey]

>>>>> "Andrew" == Andrew Burgess <aburgess@redhat.com> writes:

Andrew> You're absolutely right.  And I see what's happened.  Somewhere along
Andrew> the line the logic has gotten reversed in my head.  The problem isn't
Andrew> that the first _must_ be absolute with path_join, it's that everything
Andrew> else _must_not_ be absolute, see these lines in path_join:

Andrew>       if (i > 0)
Andrew> 	gdb_assert (strlen (path) == 0 || !IS_ABSOLUTE_PATH (path));

Andrew> And for what we're doing here we, for example, join the path to the file
Andrew> (which is absolute) onto the debug directory, which might, or might not,
Andrew> be absolute, e.g. these lines in find_separate_debug_file:

I'm sorry to go around on this again, but what if we just remove this
restriction from path_join?  It seems to me that removing an assertion
can't break any existing callers, and it's fairly normal in other code
bases for path-joining to work this way (that is, an absolute path
causes a "start over").

Tom

Re: [PATCH 1/2] gdb: avoid '//' in filenames when searching for debuginfo

[Andrew Burgess]

Tom Tromey <tom@tromey.com> writes:

>>>>>> "Andrew" == Andrew Burgess <aburgess@redhat.com> writes:
>
> Andrew> You're absolutely right.  And I see what's happened.  Somewhere along
> Andrew> the line the logic has gotten reversed in my head.  The problem isn't
> Andrew> that the first _must_ be absolute with path_join, it's that everything
> Andrew> else _must_not_ be absolute, see these lines in path_join:
>
> Andrew>       if (i > 0)
> Andrew> 	gdb_assert (strlen (path) == 0 || !IS_ABSOLUTE_PATH (path));
>
> Andrew> And for what we're doing here we, for example, join the path to the file
> Andrew> (which is absolute) onto the debug directory, which might, or might not,
> Andrew> be absolute, e.g. these lines in find_separate_debug_file:
>
> I'm sorry to go around on this again, but what if we just remove this

Not a problem, happy to try and get this right.

> restriction from path_join?  It seems to me that removing an assertion
> can't break any existing callers, and it's fairly normal in other code
> bases for path-joining to work this way (that is, an absolute path
> causes a "start over").

What do you mean by "start over" here?  Do you mean it would reset the
result to empty, e.g.:

  path_join ("/abc/", "def", "/xyz/", "ghi")

would give:

  "/xyz/ghi"

as we "start over" when we see the absolute "xyz"?  If that's what you
mean, then that's not what I want, if would want:

 "/abc/def/xyz/ghi"

Thanks,
Andrew

[PATCH 2/2] gdb: fix a target: prefix issue in find_separate_debug_file

[Andrew Burgess]

Following on from the previous commit, this commit fixes the two KFAIL
in gdb.base/sysroot-debug-lookup.exp when not using the
native-extended-gdbserver board.

The failures in this test, when using the 'unix' board, are logged as
bug PR gdb/31804.  The problem appears to be caused by the use of the
child_path function in find_separate_debug_file.

What happens on the 'unix' board is that the file is specified to GDB
with a target: prefix, however GDB spots that the target filesystem is
local to GDB and so opens the file without a target: prefix.  When we
call into find_separate_debug_file the DIR and CANON_DIR arguments,
which are computed from the objfile_name() no longer have a target:
prefix.

However, in this test if the file was opened with a target: prefix,
then the sysroot also has a target: prefix.  When child_path is called
it looks for a common prefix between CANON_DIR (from the objfile_name)
and the sysroot.  However, the sysroot still has the target: prefix,
which means the child_path() call fails and returns nullptr.

What I think we need to do is this: if the sysroot has a target:
prefix, and the target filesystem is local to GDB, then we should
strip the target: prefix from the sysroot, just as we do when opening
a file (see gdb_bfd_open in gdb_bfd.c).

Now, when we make the child_path() call neither the sysroot nor
CANON_DIR will have a target: prefix, the child_path() call will
succeed, and GDB will correctly find the debug information.

There is just one remaining issue, the last path we look in when
searching for debug information is built by starting with the sysroot,
then adding the debug directory, see this line:

  debugfile = combine_paths (target_prefix_str, root.c_str (),
                             debugdir.get (), base_path,
                             debuglink);

Now the target_prefix_str is set to target: if DIR has a target:
prefix, and DIR has a target: prefix if the file we're looking for was
opened with a target: prefix.

However, in our case the file was specified with a target: prefix, but
GDB stripped this off, so DIR does not have the target: prefix.

The sysroot however, does have the prefix.  I think in this case, as
the path we're building starts with the sysroot, then
target_prefix_str should be target: if the sysroot has the prefix, so
I've arranged for this to be the case.

With these fixes in place I now see no failures when using the 'unix'
or 'native-gdbserver' boards with this test, and the KFAILs can be
removed.

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31804
---
 gdb/symfile.c                                   |  7 +++++--
 gdb/testsuite/gdb.base/sysroot-debug-lookup.exp | 17 -----------------
 2 files changed, 5 insertions(+), 19 deletions(-)

diff --git a/gdb/symfile.c b/gdb/symfile.c
index 9ef31824fe0..54cc4f4f9d4 100644
--- a/gdb/symfile.c
+++ b/gdb/symfile.c
@@ -1430,8 +1430,10 @@ find_separate_debug_file (const char *dir,
   const char *target_prefix_str = target_prefix ? TARGET_SYSROOT_PREFIX : "";
   std::vector<gdb::unique_xmalloc_ptr<char>> debugdir_vec
     = dirnames_to_char_ptr_vec (debug_file_directory.c_str ());
-  gdb::unique_xmalloc_ptr<char> canon_sysroot
-    = gdb_realpath (gdb_sysroot.c_str ());
+  const char *sysroot_str = gdb_sysroot.c_str ();
+  if (is_target_filename (sysroot_str) && target_filesystem_is_local ())
+    sysroot_str += strlen (TARGET_SYSROOT_PREFIX);
+  gdb::unique_xmalloc_ptr<char> canon_sysroot = gdb_realpath (sysroot_str);
 
  /* MS-Windows/MS-DOS don't allow colons in file names; we must
     convert the drive letter into a one-letter directory, so that the
@@ -1493,6 +1495,7 @@ find_separate_debug_file (const char *dir,
 		{
 		  root = gdb_sysroot.substr (strlen (TARGET_SYSROOT_PREFIX));
 		  gdb_assert (!root.empty ());
+		  target_prefix_str = TARGET_SYSROOT_PREFIX;
 		}
 	      else
 		root = gdb_sysroot;
diff --git a/gdb/testsuite/gdb.base/sysroot-debug-lookup.exp b/gdb/testsuite/gdb.base/sysroot-debug-lookup.exp
index 30d492e0d47..7baf7c9c9ec 100644
--- a/gdb/testsuite/gdb.base/sysroot-debug-lookup.exp
+++ b/gdb/testsuite/gdb.base/sysroot-debug-lookup.exp
@@ -174,26 +174,9 @@ proc_with_prefix lookup_via_debuglink {} {
 	# in the sysroot.
 	gdb_file_cmd ${sysroot_prefix}$exec_in_sysroot
 
-	# Giving a sysroot a 'target:' prefix doesn't seem to work as
-	# might be expected for debug-link based lookups.  For this
-	# style of lookup GDB only seems to care if the original file
-	# itself had a 'target:' prefix.  The 'target:' prefix on the
-	# sysroot just seems to cause GDB to bail on looking up via
-	# the 'target:' prefixed sysroot.
-	#
-	# Bug PR gdb/30866 seems to be the (or a) relevant bug for
-	# this problem.
-	if { $sysroot_prefix ne ""
-	     && [target_info gdb_protocol] ne "extended-remote" } {
-	    setup_kfail "*-*-*" 31804
-	}
 	gdb_assert { $::gdb_file_cmd_debug_info eq "debug" } \
 	    "ensure debug information was found"
 
-	if { $sysroot_prefix ne ""
-	     && [target_info gdb_protocol] ne "extended-remote"  } {
-	    setup_kfail "*-*-*" 31804
-	}
 	set re [string_to_regexp "Reading symbols from ${sysroot_prefix}$debug_symlink..."]
 	gdb_assert {[regexp $re $::gdb_file_cmd_msg]} \
 	    "debug symbols read from correct file"
-- 
2.25.4