[PATCH 1/5] gdb: call gdb_tilde_expand instead of gdb_tilde_expand_up in source_script_with_search

[PATCH 1/5] gdb: call gdb_tilde_expand instead of gdb_tilde_expand_up in source_script_with_search

[Simon Marchi]

This removes a use of gdb_tilde_expand_up, which is removed later in
this series.

Change-Id: I5887d526cea987103e4ca24514a982b0a28e992a
---
 gdb/cli/cli-cmds.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/gdb/cli/cli-cmds.c b/gdb/cli/cli-cmds.c
index 424a8740706c..2b4becc97b22 100644
--- a/gdb/cli/cli-cmds.c
+++ b/gdb/cli/cli-cmds.c
@@ -749,13 +749,13 @@ source_script_with_search (const char *file, int from_tty, int search_path)
      this if we (may have) used search_path, as printing the full path in
      errors for the non-search case can be more noise than signal.  */
   const char *file_to_open;
-  gdb::unique_xmalloc_ptr<char> tilde_expanded_file;
+  std::string tilde_expanded_file;
   if (search_path)
     file_to_open = opened->full_path.get ();
   else
     {
-      tilde_expanded_file = gdb_tilde_expand_up (file);
-      file_to_open = tilde_expanded_file.get ();
+      tilde_expanded_file = gdb_tilde_expand (file);
+      file_to_open = tilde_expanded_file.c_str ();
     }
   source_script_from_stream (opened->stream.get (), file, file_to_open);
 }

base-commit: 8e347faf8f1556a0f1afc33bd53099ec5f2f8efe
-- 
2.35.2

[PATCH 2/5] gdbsupport: make gdb_abspath return an std::string

[Simon Marchi]

From: Simon Marchi <simon.marchi@efficios.com>

I'm trying to switch these functions to use std::string instead of char
arrays, as much as possible.  Some callers benefit from it (can avoid
doing a copy of the result), while others suffer (have to make one more
copy).

Change-Id: Iced49b8ee2f189744c5072a3b217aab5af17a993
---
 gdb/compile/compile.c    |  4 ++--
 gdb/corelow.c            |  2 +-
 gdb/dwarf2/index-cache.c |  4 +---
 gdb/main.c               |  7 +------
 gdb/objfiles.c           |  4 ++--
 gdb/source.c             | 19 +++++++++++--------
 gdb/top.c                | 10 ++--------
 gdb/tracefile-tfile.c    |  2 +-
 gdbserver/server.cc      | 20 ++++++++++----------
 gdbsupport/pathstuff.cc  | 41 ++++++++++++++++++++--------------------
 gdbsupport/pathstuff.h   |  2 +-
 11 files changed, 52 insertions(+), 63 deletions(-)

diff --git a/gdb/compile/compile.c b/gdb/compile/compile.c
index 2a1f0f1bd6b9..5cbb341b383b 100644
--- a/gdb/compile/compile.c
+++ b/gdb/compile/compile.c
@@ -297,8 +297,8 @@ compile_file_command (const char *args, int from_tty)
     error (_("You must provide a filename for this command."));
 
   args = skip_spaces (args);
-  gdb::unique_xmalloc_ptr<char> abspath = gdb_abspath (args);
-  std::string buffer = string_printf ("#include \"%s\"\n", abspath.get ());
+  std::string abspath = gdb_abspath (args);
+  std::string buffer = string_printf ("#include \"%s\"\n", abspath.c_str ());
   eval_compile_command (NULL, buffer.c_str (), scope, NULL);
 }
 
diff --git a/gdb/corelow.c b/gdb/corelow.c
index a23dc81c5af9..8c33fb7ebb2f 100644
--- a/gdb/corelow.c
+++ b/gdb/corelow.c
@@ -469,7 +469,7 @@ core_target_open (const char *arg, int from_tty)
   gdb::unique_xmalloc_ptr<char> filename (tilde_expand (arg));
   if (strlen (filename.get ()) != 0
       && !IS_ABSOLUTE_PATH (filename.get ()))
-    filename = gdb_abspath (filename.get ());
+    filename = make_unique_xstrdup (gdb_abspath (filename.get ()).c_str ());
 
   flags = O_BINARY | O_LARGEFILE;
   if (write_files)
diff --git a/gdb/dwarf2/index-cache.c b/gdb/dwarf2/index-cache.c
index 18e60cbd78cd..fb827e04e59b 100644
--- a/gdb/dwarf2/index-cache.c
+++ b/gdb/dwarf2/index-cache.c
@@ -298,9 +298,7 @@ set_index_cache_directory_command (const char *arg, int from_tty,
 				   cmd_list_element *element)
 {
   /* Make sure the index cache directory is absolute and tilde-expanded.  */
-  gdb::unique_xmalloc_ptr<char> abs
-    = gdb_abspath (index_cache_directory.c_str ());
-  index_cache_directory = abs.get ();
+  index_cache_directory = gdb_abspath (index_cache_directory.c_str ());
   global_index_cache.set_directory (index_cache_directory);
 }
 
diff --git a/gdb/main.c b/gdb/main.c
index 8c0807ff83b2..ec2b7b017524 100644
--- a/gdb/main.c
+++ b/gdb/main.c
@@ -133,12 +133,7 @@ set_gdb_data_directory (const char *new_datadir)
      "../foo" and "../foo" doesn't exist then we'll record $(pwd)/../foo which
      isn't canonical, but that's ok.  */
   if (!IS_ABSOLUTE_PATH (gdb_datadir.c_str ()))
-    {
-      gdb::unique_xmalloc_ptr<char> abs_datadir
-	= gdb_abspath (gdb_datadir.c_str ());
-
-      gdb_datadir = abs_datadir.get ();
-    }
+    gdb_datadir = gdb_abspath (gdb_datadir.c_str ());
 }
 
 /* Relocate a file or directory.  PROGNAME is the name by which gdb
diff --git a/gdb/objfiles.c b/gdb/objfiles.c
index 0c71e0bd6a9f..849c6d73cab8 100644
--- a/gdb/objfiles.c
+++ b/gdb/objfiles.c
@@ -331,7 +331,7 @@ objfile::objfile (bfd *abfd, const char *name, objfile_flags flags_)
 
   objfile_alloc_data (this);
 
-  gdb::unique_xmalloc_ptr<char> name_holder;
+  std::string name_holder;
   if (name == NULL)
     {
       gdb_assert (abfd == NULL);
@@ -344,7 +344,7 @@ objfile::objfile (bfd *abfd, const char *name, objfile_flags flags_)
   else
     {
       name_holder = gdb_abspath (name);
-      expanded_name = name_holder.get ();
+      expanded_name = name_holder.c_str ();
     }
   original_name = obstack_strdup (&objfile_obstack, expanded_name);
 
diff --git a/gdb/source.c b/gdb/source.c
index e9016573333c..9d9ff4bbc3e9 100644
--- a/gdb/source.c
+++ b/gdb/source.c
@@ -537,15 +537,15 @@ add_path (const char *dirname, char **which_path, int parse_separators)
 
   for (const gdb::unique_xmalloc_ptr<char> &name_up : dir_vec)
     {
-      char *name = name_up.get ();
+      const char *name = name_up.get ();
       char *p;
       struct stat st;
-      gdb::unique_xmalloc_ptr<char> new_name_holder;
+      std::string new_name_holder;
 
       /* Spaces and tabs will have been removed by buildargv().
 	 NAME is the start of the directory.
 	 P is the '\0' following the end.  */
-      p = name + strlen (name);
+      p = name_up.get () + strlen (name);
 
       while (!(IS_DIR_SEPARATOR (*name) && p <= name + 1)	/* "/" */
 #ifdef HAVE_DOS_BASED_FILE_SYSTEM
@@ -589,16 +589,18 @@ add_path (const char *dirname, char **which_path, int parse_separators)
       if (name[0] == '\0')
         goto skip_dup;
       if (name[0] == '~')
-	new_name_holder.reset (tilde_expand (name));
+	new_name_holder
+	  = gdb::unique_xmalloc_ptr<char[]> (tilde_expand (name)).get ();
 #ifdef HAVE_DOS_BASED_FILE_SYSTEM
       else if (IS_ABSOLUTE_PATH (name) && p == name + 2) /* "d:" => "d:." */
-	new_name_holder.reset (concat (name, ".", (char *) NULL));
+	new_name_holder = std::string (name) + ".";
 #endif
       else if (!IS_ABSOLUTE_PATH (name) && name[0] != '$')
 	new_name_holder = gdb_abspath (name);
       else
-	new_name_holder.reset (savestring (name, p - name));
-      name = new_name_holder.get ();
+	new_name_holder = std::string (name, p - name);
+
+      name = new_name_holder.c_str ();
 
       /* Unless it's a variable, check existence.  */
       if (name[0] != '$')
@@ -950,7 +952,8 @@ openp (const char *path, openp_flags opts, const char *string,
       else if ((opts & OPF_RETURN_REALPATH) != 0)
 	*filename_opened = gdb_realpath (filename);
       else
-	*filename_opened = gdb_abspath (filename);
+	*filename_opened
+	  = make_unique_xstrdup (gdb_abspath (filename).c_str ());
     }
 
   errno = last_errno;
diff --git a/gdb/top.c b/gdb/top.c
index e776ac2d70e1..1cfffbeee7ee 100644
--- a/gdb/top.c
+++ b/gdb/top.c
@@ -2167,12 +2167,7 @@ set_history_filename (const char *args,
      that was read.  */
   if (!history_filename.empty ()
       && !IS_ABSOLUTE_PATH (history_filename.c_str ()))
-    {
-      gdb::unique_xmalloc_ptr<char> temp
-	(gdb_abspath (history_filename.c_str ()));
-
-      history_filename = temp.get ();
-    }
+    history_filename = gdb_abspath (history_filename.c_str ());
 }
 
 /* Whether we're in quiet startup mode.  */
@@ -2444,7 +2439,6 @@ _initialize_top ()
       const char *fname = ".gdb_history";
 #endif
 
-      gdb::unique_xmalloc_ptr<char> temp (gdb_abspath (fname));
-      history_filename = temp.get ();
+      history_filename = gdb_abspath (fname);
     }
 }
diff --git a/gdb/tracefile-tfile.c b/gdb/tracefile-tfile.c
index 107236330d1c..f84ad2f4a3fd 100644
--- a/gdb/tracefile-tfile.c
+++ b/gdb/tracefile-tfile.c
@@ -471,7 +471,7 @@ tfile_target_open (const char *arg, int from_tty)
 
   gdb::unique_xmalloc_ptr<char> filename (tilde_expand (arg));
   if (!IS_ABSOLUTE_PATH (filename.get ()))
-    filename = gdb_abspath (filename.get ());
+    filename = make_unique_xstrdup (gdb_abspath (filename.get ()).c_str ());
 
   flags = O_BINARY | O_LARGEFILE;
   flags |= O_RDONLY;
diff --git a/gdbserver/server.cc b/gdbserver/server.cc
index 24925cbbb5f5..33c42714e726 100644
--- a/gdbserver/server.cc
+++ b/gdbserver/server.cc
@@ -90,31 +90,31 @@ bool non_stop;
 static struct {
   /* Set the PROGRAM_PATH.  Here we adjust the path of the provided
      binary if needed.  */
-  void set (gdb::unique_xmalloc_ptr<char> &&path)
+  void set (const char *path)
   {
-    m_path = std::move (path);
+    m_path = path;
 
     /* Make sure we're using the absolute path of the inferior when
        creating it.  */
-    if (!contains_dir_separator (m_path.get ()))
+    if (!contains_dir_separator (m_path.c_str ()))
       {
 	int reg_file_errno;
 
 	/* Check if the file is in our CWD.  If it is, then we prefix
 	   its name with CURRENT_DIRECTORY.  Otherwise, we leave the
 	   name as-is because we'll try searching for it in $PATH.  */
-	if (is_regular_file (m_path.get (), &reg_file_errno))
-	  m_path = gdb_abspath (m_path.get ());
+	if (is_regular_file (m_path.c_str (), &reg_file_errno))
+	  m_path = gdb_abspath (m_path.c_str ());
       }
   }
 
   /* Return the PROGRAM_PATH.  */
-  char *get ()
-  { return m_path.get (); }
+  const char *get ()
+  { return m_path.empty () ? nullptr : m_path.c_str (); }
 
 private:
   /* The program name, adjusted if needed.  */
-  gdb::unique_xmalloc_ptr<char> m_path;
+  std::string m_path;
 } program_path;
 static std::vector<char *> program_args;
 static std::string wrapper_argv;
@@ -3076,7 +3076,7 @@ handle_v_run (char *own_buf)
 	}
     }
   else
-    program_path.set (gdb::unique_xmalloc_ptr<char> (new_program_name));
+    program_path.set (new_program_name);
 
   /* Free the old argv and install the new one.  */
   free_vector_argv (program_args);
@@ -3933,7 +3933,7 @@ captured_main (int argc, char *argv[])
       int i, n;
 
       n = argc - (next_arg - argv);
-      program_path.set (make_unique_xstrdup (next_arg[0]));
+      program_path.set (next_arg[0]);
       for (i = 1; i < n; i++)
 	program_args.push_back (xstrdup (next_arg[i]));
 
diff --git a/gdbsupport/pathstuff.cc b/gdbsupport/pathstuff.cc
index a347123cf2b3..dd6ffa49fb42 100644
--- a/gdbsupport/pathstuff.cc
+++ b/gdbsupport/pathstuff.cc
@@ -130,23 +130,23 @@ gdb_realpath_keepfile (const char *filename)
 
 /* See gdbsupport/pathstuff.h.  */
 
-gdb::unique_xmalloc_ptr<char>
+std::string
 gdb_abspath (const char *path)
 {
   gdb_assert (path != NULL && path[0] != '\0');
 
   if (path[0] == '~')
-    return gdb_tilde_expand_up (path);
+    return gdb_tilde_expand (path);
 
   if (IS_ABSOLUTE_PATH (path) || current_directory == NULL)
-    return make_unique_xstrdup (path);
+    return path;
 
   /* Beware the // my son, the Emacs barfs, the botch that catch...  */
-  return gdb::unique_xmalloc_ptr<char>
-    (concat (current_directory,
-	     IS_DIR_SEPARATOR (current_directory[strlen (current_directory) - 1])
-	     ? "" : SLASH_STRING,
-	     path, (char *) NULL));
+  return string_printf
+    ("%s%s%s", current_directory,
+     (IS_DIR_SEPARATOR (current_directory[strlen (current_directory) - 1])
+      ? "" : SLASH_STRING),
+     path);
 }
 
 /* See gdbsupport/pathstuff.h.  */
@@ -229,8 +229,8 @@ get_standard_cache_dir ()
   if (xdg_cache_home != NULL && xdg_cache_home[0] != '\0')
     {
       /* Make sure the path is absolute and tilde-expanded.  */
-      gdb::unique_xmalloc_ptr<char> abs (gdb_abspath (xdg_cache_home));
-      return string_printf ("%s/gdb", abs.get ());
+      std::string abs = gdb_abspath (xdg_cache_home);
+      return string_printf ("%s/gdb", abs.c_str ());
     }
 #endif
 
@@ -238,8 +238,8 @@ get_standard_cache_dir ()
   if (home != NULL && home[0] != '\0')
     {
       /* Make sure the path is absolute and tilde-expanded.  */
-      gdb::unique_xmalloc_ptr<char> abs (gdb_abspath (home));
-      return string_printf ("%s/" HOME_CACHE_DIR "/gdb", abs.get ());
+      std::string abs = gdb_abspath (home);
+      return string_printf ("%s/" HOME_CACHE_DIR "/gdb", abs.c_str ());
     }
 
 #ifdef WIN32
@@ -247,8 +247,8 @@ get_standard_cache_dir ()
   if (win_home != NULL && win_home[0] != '\0')
     {
       /* Make sure the path is absolute and tilde-expanded.  */
-      gdb::unique_xmalloc_ptr<char> abs (gdb_abspath (win_home));
-      return string_printf ("%s/gdb", abs.get ());
+      std::string abs = gdb_abspath (win_home);
+      return string_printf ("%s/gdb", abs.c_str ());
     }
 #endif
 
@@ -296,8 +296,8 @@ get_standard_config_dir ()
   if (xdg_config_home != NULL && xdg_config_home[0] != '\0')
     {
       /* Make sure the path is absolute and tilde-expanded.  */
-      gdb::unique_xmalloc_ptr<char> abs (gdb_abspath (xdg_config_home));
-      return string_printf ("%s/gdb", abs.get ());
+      std::string abs = gdb_abspath (xdg_config_home);
+      return string_printf ("%s/gdb", abs.c_str ());
     }
 #endif
 
@@ -305,8 +305,8 @@ get_standard_config_dir ()
   if (home != NULL && home[0] != '\0')
     {
       /* Make sure the path is absolute and tilde-expanded.  */
-      gdb::unique_xmalloc_ptr<char> abs (gdb_abspath (home));
-      return string_printf ("%s/" HOME_CONFIG_DIR "/gdb", abs.get ());
+      std::string abs = gdb_abspath (home);
+      return string_printf ("%s/" HOME_CONFIG_DIR "/gdb", abs.c_str ());
     }
 
   return {};
@@ -347,9 +347,8 @@ find_gdb_home_config_file (const char *name, struct stat *buf)
   if (homedir != nullptr && homedir[0] != '\0')
     {
       /* Make sure the path is absolute and tilde-expanded.  */
-      gdb::unique_xmalloc_ptr<char> abs (gdb_abspath (homedir));
-      std::string path = (std::string (abs.get ()) + SLASH_STRING
-			  + std::string (name));
+      std::string abs = gdb_abspath (homedir);
+      std::string path = string_printf ("%s/%s", abs.c_str (), name);
       if (stat (path.c_str (), buf) == 0)
 	return path;
     }
diff --git a/gdbsupport/pathstuff.h b/gdbsupport/pathstuff.h
index 50e388aad047..04d9bf9029b5 100644
--- a/gdbsupport/pathstuff.h
+++ b/gdbsupport/pathstuff.h
@@ -53,7 +53,7 @@ extern gdb::unique_xmalloc_ptr<char>
    If CURRENT_DIRECTORY is NULL, this function returns a copy of
    PATH.  */
 
-extern gdb::unique_xmalloc_ptr<char> gdb_abspath (const char *path);
+extern std::string 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
-- 
2.35.2

[PATCH 3/5] gdbsupport: make gdb_realpath_keepfile return an std::string

[Simon Marchi]

From: Simon Marchi <simon.marchi@efficios.com>

I'm trying to switch these functions to use std::string instead of char
arrays, as much as possible.  Some callers benefit from it (can avoid
doing a copy of the result), while others suffer (have to make one more
copy).

Change-Id: I793aab17baaef8345488f4c40b9094e2695425bc
---
 gdb/exec.c              |  3 ++-
 gdbsupport/pathstuff.cc | 11 ++++-------
 gdbsupport/pathstuff.h  |  3 +--
 3 files changed, 7 insertions(+), 10 deletions(-)

diff --git a/gdb/exec.c b/gdb/exec.c
index 84c36473abb0..38540c0840b9 100644
--- a/gdb/exec.c
+++ b/gdb/exec.c
@@ -467,7 +467,8 @@ exec_file_attach (const char *filename, int from_tty)
 	     (bfd_get_filename (current_program_space->exec_bfd ())));
       else
 	current_program_space->exec_filename
-	  = gdb_realpath_keepfile (scratch_pathname);
+	  = make_unique_xstrdup (gdb_realpath_keepfile
+				   (scratch_pathname).c_str ());
 
       if (!bfd_check_format_matches (current_program_space->exec_bfd (),
 				     bfd_object, &matching))
diff --git a/gdbsupport/pathstuff.cc b/gdbsupport/pathstuff.cc
index dd6ffa49fb42..ac65651d4492 100644
--- a/gdbsupport/pathstuff.cc
+++ b/gdbsupport/pathstuff.cc
@@ -86,17 +86,16 @@ gdb_realpath (const char *filename)
 
 /* See gdbsupport/pathstuff.h.  */
 
-gdb::unique_xmalloc_ptr<char>
+std::string
 gdb_realpath_keepfile (const char *filename)
 {
   const char *base_name = lbasename (filename);
   char *dir_name;
-  char *result;
 
   /* Extract the basename of filename, and return immediately
      a copy of filename if it does not contain any directory prefix.  */
   if (base_name == filename)
-    return make_unique_xstrdup (filename);
+    return filename;
 
   dir_name = (char *) alloca ((size_t) (base_name - filename + 2));
   /* Allocate enough space to store the dir_name + plus one extra
@@ -121,11 +120,9 @@ gdb_realpath_keepfile (const char *filename)
   gdb::unique_xmalloc_ptr<char> path_storage = gdb_realpath (dir_name);
   const char *real_path = path_storage.get ();
   if (IS_DIR_SEPARATOR (real_path[strlen (real_path) - 1]))
-    result = concat (real_path, base_name, (char *) NULL);
+    return string_printf ("%s%s", real_path, base_name);
   else
-    result = concat (real_path, SLASH_STRING, base_name, (char *) NULL);
-
-  return gdb::unique_xmalloc_ptr<char> (result);
+    return string_printf ("%s/%s", real_path, base_name);
 }
 
 /* See gdbsupport/pathstuff.h.  */
diff --git a/gdbsupport/pathstuff.h b/gdbsupport/pathstuff.h
index 04d9bf9029b5..f6c51e9de887 100644
--- a/gdbsupport/pathstuff.h
+++ b/gdbsupport/pathstuff.h
@@ -39,8 +39,7 @@ extern gdb::unique_xmalloc_ptr<char> gdb_realpath (const char *filename);
 /* Return a copy of FILENAME, with its directory prefix canonicalized
    by gdb_realpath.  */
 
-extern gdb::unique_xmalloc_ptr<char>
-  gdb_realpath_keepfile (const char *filename);
+extern std::string gdb_realpath_keepfile (const char *filename);
 
 /* Return PATH in absolute form, performing tilde-expansion if necessary.
    PATH cannot be NULL or the empty string.
-- 
2.35.2

[PATCH 4/5] gdb: use gdb_tilde_expand instead of gdb_tilde_expand_up in source_script_with_search

[Simon Marchi]

From: Simon Marchi <simon.marchi@efficios.com>

Since this is the latest use of gdb_tilde_expand_up, remove it.

Change-Id: I964c812ce55fe087876abf91e7a3577ad79c0425
---
 gdbsupport/gdb_tilde_expand.cc | 9 ---------
 gdbsupport/gdb_tilde_expand.h  | 4 ----
 2 files changed, 13 deletions(-)

diff --git a/gdbsupport/gdb_tilde_expand.cc b/gdbsupport/gdb_tilde_expand.cc
index e3f9308a14f0..177d296a14fd 100644
--- a/gdbsupport/gdb_tilde_expand.cc
+++ b/gdbsupport/gdb_tilde_expand.cc
@@ -102,12 +102,3 @@ gdb_tilde_expand (const char *dir)
   gdb_assert (glob.pathc () == 1);
   return std::string (glob.pathv ()[0]) + remainder;
 }
-
-/* See gdbsupport/gdb_tilde_expand.h.  */
-
-gdb::unique_xmalloc_ptr<char>
-gdb_tilde_expand_up (const char *dir)
-{
-  const std::string expanded = gdb_tilde_expand (dir);
-  return make_unique_xstrdup (expanded.c_str ());
-}
diff --git a/gdbsupport/gdb_tilde_expand.h b/gdbsupport/gdb_tilde_expand.h
index c47cd6a214f7..06b96517697c 100644
--- a/gdbsupport/gdb_tilde_expand.h
+++ b/gdbsupport/gdb_tilde_expand.h
@@ -23,8 +23,4 @@
 /* Perform tilde expansion on DIR, and return the full path.  */
 extern std::string gdb_tilde_expand (const char *dir);
 
-/* Same as GDB_TILDE_EXPAND, but return the full path as a
-   gdb::unique_xmalloc_ptr<char>.  */
-extern gdb::unique_xmalloc_ptr<char> gdb_tilde_expand_up (const char *dir);
-
 #endif /* COMMON_GDB_TILDE_EXPAND_H */
-- 
2.35.2

[PATCH 5/5] gdbsupport: add path_join function

[Simon Marchi]

From: Simon Marchi <simon.marchi@efficios.com>

In this review [1], Eli pointed out that we should be careful when
concatenating file names to avoid duplicated slashes.  On Windows, a
double slash at the beginning of a file path has a special meaning.  So
naively concatenating "/"  and "foo/bar" would give "//foo/bar", which
would not give the desired results.  We already have a few spots doing:

  if (first_path ends with a slash)
    path = first_path + second_path
  else
    path = first_path + slash + second_path

In general, I think it's nice to avoid superfluous slashes in file
paths, since they might end up visible to the user and look a bit
unprofessional.

Introduce the path_join function that can be used to join multiple path
components together (along with unit tests).

Change a few spots to use path_join to show how it can be used.

[1] https://sourceware.org/pipermail/gdb-patches/2022-April/187559.html

Change-Id: I0df889f7e3f644e045f42ff429277b732eb6c752
---
 gdb/Makefile.in                     |  1 +
 gdb/buildsym.c                      |  5 +-
 gdb/dwarf2/line-header.c            |  3 +-
 gdb/dwarf2/read.c                   | 43 ++++++++---------
 gdb/macrotab.c                      |  3 +-
 gdb/unittests/path-join-selftests.c | 75 +++++++++++++++++++++++++++++
 gdbsupport/pathstuff.cc             | 62 ++++++++++++++++++------
 gdbsupport/pathstuff.h              | 11 +++++
 8 files changed, 161 insertions(+), 42 deletions(-)
 create mode 100644 gdb/unittests/path-join-selftests.c

diff --git a/gdb/Makefile.in b/gdb/Makefile.in
index c8e140b55449..ec0e55dd803c 100644
--- a/gdb/Makefile.in
+++ b/gdb/Makefile.in
@@ -465,6 +465,7 @@ SELFTESTS_SRCS = \
 	unittests/optional-selftests.c \
 	unittests/parallel-for-selftests.c \
 	unittests/parse-connection-spec-selftests.c \
+	unittests/path-join-selftests.c \
 	unittests/ptid-selftests.c \
 	unittests/main-thread-selftests.c \
 	unittests/mkdir-recursive-selftests.c \
diff --git a/gdb/buildsym.c b/gdb/buildsym.c
index 628903d674f4..a1375e84a4ac 100644
--- a/gdb/buildsym.c
+++ b/gdb/buildsym.c
@@ -20,6 +20,7 @@
 #include "buildsym-legacy.h"
 #include "bfd.h"
 #include "gdbsupport/gdb_obstack.h"
+#include "gdbsupport/pathstuff.h"
 #include "symtab.h"
 #include "symfile.h"
 #include "objfiles.h"
@@ -507,8 +508,8 @@ buildsym_compunit::start_subfile (const char *name)
 	  && !IS_ABSOLUTE_PATH (subfile->name)
 	  && !m_comp_dir.empty ())
 	{
-	  subfile_name_holder = string_printf ("%s/%s", m_comp_dir.c_str (),
-					       subfile->name.c_str ());
+	  subfile_name_holder = path_join (m_comp_dir.c_str (),
+					   subfile->name.c_str (), nullptr);
 	  subfile_name = subfile_name_holder.c_str ();
 	}
       else
diff --git a/gdb/dwarf2/line-header.c b/gdb/dwarf2/line-header.c
index 77e7e553b21e..9f4447023c58 100644
--- a/gdb/dwarf2/line-header.c
+++ b/gdb/dwarf2/line-header.c
@@ -24,6 +24,7 @@
 #include "dwarf2/read.h"
 #include "complaints.h"
 #include "filenames.h"
+#include "gdbsupport/pathstuff.h"
 
 void
 line_header::add_include_dir (const char *include_dir)
@@ -73,7 +74,7 @@ line_header::file_file_name (int file) const
 	{
 	  const char *dir = fe->include_dir (this);
 	  if (dir != NULL)
-	    return string_printf ("%s/%s", dir, fe->name);
+	    return path_join (dir, fe->name, nullptr);
 	}
 
       return fe->name;
diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index 6dcd446e5f46..508cb511b4a8 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -1343,7 +1343,7 @@ static const char *compute_include_file_name
      (const struct line_header *lh,
       const file_entry &fe,
       const file_and_directory &cu_info,
-      gdb::unique_xmalloc_ptr<char> *name_holder);
+      std::string &name_holder);
 
 static htab_up allocate_signatured_type_table ();
 
@@ -2809,9 +2809,9 @@ dw2_get_file_names_reader (const struct die_reader_specs *reader,
     {
       for (const auto &entry : lh->file_names ())
 	{
-	  gdb::unique_xmalloc_ptr<char> name_holder;
+	  std::string name_holder;
 	  const char *include_name =
-	    compute_include_file_name (lh.get (), entry, fnd, &name_holder);
+	    compute_include_file_name (lh.get (), entry, fnd, name_holder);
 	  if (include_name != nullptr)
 	    {
 	      include_name = per_objfile->objfile->intern (include_name);
@@ -11166,14 +11166,13 @@ open_dwo_file (dwarf2_per_objfile *per_objfile,
 
   if (comp_dir != NULL)
     {
-      gdb::unique_xmalloc_ptr<char> path_to_try
-	(concat (comp_dir, SLASH_STRING, file_name, (char *) NULL));
+      std::string path_to_try = path_join (comp_dir, file_name, nullptr);
 
       /* NOTE: If comp_dir is a relative path, this will also try the
 	 search path, which seems useful.  */
-      gdb_bfd_ref_ptr abfd (try_open_dwop_file (per_objfile, path_to_try.get (),
-						0 /*is_dwp*/,
-						1 /*search_cwd*/));
+      gdb_bfd_ref_ptr abfd (try_open_dwop_file
+	(per_objfile, path_to_try.c_str (), 0 /*is_dwp*/, 1 /*search_cwd*/));
+
       if (abfd != NULL)
 	return abfd;
     }
@@ -19751,14 +19750,14 @@ dwarf_decode_line_header (sect_offset sect_off, struct dwarf2_cu *cu)
 static const char *
 compute_include_file_name (const struct line_header *lh, const file_entry &fe,
 			   const file_and_directory &cu_info,
-			   gdb::unique_xmalloc_ptr<char> *name_holder)
+			   std::string &name_holder)
 {
   const char *include_name = fe.name;
   const char *include_name_to_compare = include_name;
 
   const char *dir_name = fe.include_dir (lh);
 
-  gdb::unique_xmalloc_ptr<char> hold_compare;
+  std::string hold_compare;
   if (!IS_ABSOLUTE_PATH (include_name)
       && (dir_name != nullptr || cu_info.get_comp_dir () != nullptr))
     {
@@ -19785,27 +19784,25 @@ compute_include_file_name (const struct line_header *lh, const file_entry &fe,
 
       if (dir_name != NULL)
 	{
-	  name_holder->reset (concat (dir_name, SLASH_STRING,
-				      include_name, (char *) NULL));
-	  include_name = name_holder->get ();
+	  name_holder = path_join (dir_name, include_name, nullptr);
+	  include_name = name_holder.c_str ();
 	  include_name_to_compare = include_name;
 	}
       if (!IS_ABSOLUTE_PATH (include_name)
 	  && cu_info.get_comp_dir () != nullptr)
 	{
-	  hold_compare.reset (concat (cu_info.get_comp_dir (), SLASH_STRING,
-				      include_name, (char *) NULL));
-	  include_name_to_compare = hold_compare.get ();
+	  hold_compare = path_join (cu_info.get_comp_dir (), include_name,
+				    nullptr);
+	  include_name_to_compare = hold_compare.c_str ();
 	}
     }
 
-  gdb::unique_xmalloc_ptr<char> copied_name;
+  std::string copied_name;
   const char *cu_filename = cu_info.get_name ();
   if (!IS_ABSOLUTE_PATH (cu_filename) && cu_info.get_comp_dir () != nullptr)
     {
-      copied_name.reset (concat (cu_info.get_comp_dir (), SLASH_STRING,
-				 cu_filename, (char *) NULL));
-      cu_filename = copied_name.get ();
+      copied_name = path_join (cu_info.get_comp_dir (), cu_filename, nullptr);
+      cu_filename = copied_name.c_str ();
     }
 
   if (FILENAME_CMP (include_name_to_compare, cu_filename) == 0)
@@ -20540,7 +20537,7 @@ static void
 dwarf2_start_subfile (struct dwarf2_cu *cu, const char *filename,
 		      const char *dirname)
 {
-  gdb::unique_xmalloc_ptr<char> copy;
+  std::string copy;
 
   /* In order not to lose the line information directory,
      we concatenate it to the filename when it makes sense.
@@ -20551,8 +20548,8 @@ dwarf2_start_subfile (struct dwarf2_cu *cu, const char *filename,
 
   if (!IS_ABSOLUTE_PATH (filename) && dirname != NULL)
     {
-      copy.reset (concat (dirname, SLASH_STRING, filename, (char *) NULL));
-      filename = copy.get ();
+      copy = path_join (dirname, filename, nullptr);
+      filename = copy.c_str ();
     }
 
   cu->get_builder ()->start_subfile (filename);
diff --git a/gdb/macrotab.c b/gdb/macrotab.c
index 7ca3f312d330..b893c7ae3c1b 100644
--- a/gdb/macrotab.c
+++ b/gdb/macrotab.c
@@ -19,6 +19,7 @@
 
 #include "defs.h"
 #include "gdbsupport/gdb_obstack.h"
+#include "gdbsupport/pathstuff.h"
 #include "splay-tree.h"
 #include "filenames.h"
 #include "symtab.h"
@@ -1071,5 +1072,5 @@ macro_source_fullname (struct macro_source_file *file)
   if (comp_dir == NULL || IS_ABSOLUTE_PATH (file->filename))
     return file->filename;
 
-  return std::string (comp_dir) + SLASH_STRING + file->filename;
+  return path_join (comp_dir, file->filename, nullptr);
 }
diff --git a/gdb/unittests/path-join-selftests.c b/gdb/unittests/path-join-selftests.c
new file mode 100644
index 000000000000..f0dfdfd40376
--- /dev/null
+++ b/gdb/unittests/path-join-selftests.c
@@ -0,0 +1,75 @@
+/* Self tests for path_join for GDB, the GNU debugger.
+
+   Copyright (C) 2022 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 "gdbsupport/pathstuff.h"
+#include "gdbsupport/selftest.h"
+
+namespace selftests {
+namespace path_join {
+
+/* Test path_join.  */
+
+static void
+test ()
+{
+  std::string s;
+
+  s = ::path_join ("/foo", "bar", nullptr);
+  SELF_CHECK (s == "/foo/bar");
+
+  s = ::path_join ("/foo", "/bar", nullptr);
+  SELF_CHECK (s == "/foo/bar");
+
+  s = ::path_join ("/", "bar", nullptr);
+  SELF_CHECK (s == "/bar");
+
+  s = ::path_join ("foo/", "/bar", nullptr);
+  SELF_CHECK (s == "foo/bar");
+
+  s = ::path_join ("foo", "bar", "", nullptr);
+  SELF_CHECK (s == "foo/bar");
+
+  s = ::path_join ("", "/foo", nullptr);
+  SELF_CHECK (s == "/foo");
+
+  s = ::path_join ("", "foo", nullptr);
+  SELF_CHECK (s == "foo");
+
+  s = ::path_join ("foo", "", "bar", nullptr);
+  SELF_CHECK (s == "foo/bar");
+
+  s = ::path_join ("foo", "", nullptr);
+  SELF_CHECK (s == "foo");
+
+  s = ::path_join ("foo/", "", nullptr);
+  SELF_CHECK (s == "foo/");
+}
+
+}
+}
+
+void _initialize_path_join_selftests ();
+void
+_initialize_path_join_selftests ()
+{
+  selftests::register_test ("path_join",
+			    selftests::path_join::test);
+}
+
diff --git a/gdbsupport/pathstuff.cc b/gdbsupport/pathstuff.cc
index ac65651d4492..2d95eaff3d16 100644
--- a/gdbsupport/pathstuff.cc
+++ b/gdbsupport/pathstuff.cc
@@ -119,10 +119,7 @@ gdb_realpath_keepfile (const char *filename)
      directory separator, avoid doubling it.  */
   gdb::unique_xmalloc_ptr<char> path_storage = gdb_realpath (dir_name);
   const char *real_path = path_storage.get ();
-  if (IS_DIR_SEPARATOR (real_path[strlen (real_path) - 1]))
-    return string_printf ("%s%s", real_path, base_name);
-  else
-    return string_printf ("%s/%s", real_path, base_name);
+  return path_join (real_path, base_name, nullptr);
 }
 
 /* See gdbsupport/pathstuff.h.  */
@@ -138,12 +135,7 @@ gdb_abspath (const char *path)
   if (IS_ABSOLUTE_PATH (path) || current_directory == NULL)
     return path;
 
-  /* Beware the // my son, the Emacs barfs, the botch that catch...  */
-  return string_printf
-    ("%s%s%s", current_directory,
-     (IS_DIR_SEPARATOR (current_directory[strlen (current_directory) - 1])
-      ? "" : SLASH_STRING),
-     path);
+  return path_join (current_directory, path, nullptr);
 }
 
 /* See gdbsupport/pathstuff.h.  */
@@ -198,6 +190,46 @@ child_path (const char *parent, const char *child)
 
 /* See gdbsupport/pathstuff.h.  */
 
+std::string
+path_join (const char *component...)
+{
+  std::string path = component;
+
+  auto skip_leading_dir_seps = [] (const char *s)
+    {
+      while (*s == '/')
+	++s;
+
+      return s;
+    };
+
+  va_list args;
+  va_start (args, component);
+
+  const char *c = va_arg (args, const char *);
+  while (c != nullptr)
+    {
+      if (!path.empty ())
+	c = skip_leading_dir_seps (c);
+
+      if (*c != '\0')
+	{
+	  if (!path.empty () && path.back () != '/')
+	    path += '/';
+
+	  path += c;
+	}
+
+      c = va_arg (args, const char *);
+    }
+
+  va_end (args);
+
+  return path;
+}
+
+/* See gdbsupport/pathstuff.h.  */
+
 bool
 contains_dir_separator (const char *path)
 {
@@ -227,7 +259,7 @@ get_standard_cache_dir ()
     {
       /* Make sure the path is absolute and tilde-expanded.  */
       std::string abs = gdb_abspath (xdg_cache_home);
-      return string_printf ("%s/gdb", abs.c_str ());
+      return path_join (abs.c_str (), "gdb", nullptr);
     }
 #endif
 
@@ -236,7 +268,7 @@ get_standard_cache_dir ()
     {
       /* Make sure the path is absolute and tilde-expanded.  */
       std::string abs = gdb_abspath (home);
-      return string_printf ("%s/" HOME_CACHE_DIR "/gdb", abs.c_str ());
+      return path_join (abs.c_str (), HOME_CACHE_DIR,  "gdb", nullptr);
     }
 
 #ifdef WIN32
@@ -245,7 +277,7 @@ get_standard_cache_dir ()
     {
       /* Make sure the path is absolute and tilde-expanded.  */
       std::string abs = gdb_abspath (win_home);
-      return string_printf ("%s/gdb", abs.c_str ());
+      return path_join (abs.c_str (), "gdb", nullptr);
     }
 #endif
 
@@ -294,7 +326,7 @@ get_standard_config_dir ()
     {
       /* Make sure the path is absolute and tilde-expanded.  */
       std::string abs = gdb_abspath (xdg_config_home);
-      return string_printf ("%s/gdb", abs.c_str ());
+      return path_join (abs.c_str (), "gdb", nullptr);
     }
 #endif
 
@@ -303,7 +335,7 @@ get_standard_config_dir ()
     {
       /* Make sure the path is absolute and tilde-expanded.  */
       std::string abs = gdb_abspath (home);
-      return string_printf ("%s/" HOME_CONFIG_DIR "/gdb", abs.c_str ());
+      return path_join (abs.c_str (), HOME_CONFIG_DIR, "gdb", nullptr);
     }
 
   return {};
diff --git a/gdbsupport/pathstuff.h b/gdbsupport/pathstuff.h
index f6c51e9de887..3e776be072e6 100644
--- a/gdbsupport/pathstuff.h
+++ b/gdbsupport/pathstuff.h
@@ -60,6 +60,17 @@ extern std::string gdb_abspath (const char *path);
 
 extern const char *child_path (const char *parent, const char *child);
 
+/* Join COMPONENT and all subsequent null-terminated string arguments into a
+   single path.
+
+   The last argument must be nullptr to denote the end of the argument list
+
+   Empty arguments or arguments consisting only of directory separators are
+   ignored.  */
+
+extern std::string path_join (const char *component, ...)
+  ATTRIBUTE_SENTINEL;
+
 /* Return whether PATH contains a directory separator character.  */
 
 extern bool contains_dir_separator (const char *path);
-- 
2.35.2

Re: [PATCH 5/5] gdbsupport: add path_join function

[Eli Zaretskii]

> Date: Thu, 14 Apr 2022 16:01:37 -0400
> From: Simon Marchi via Gdb-patches <gdb-patches@sourceware.org>
> Cc: Simon Marchi <simon.marchi@efficios.com>
> 
> From: Simon Marchi <simon.marchi@efficios.com>
> 
> In this review [1], Eli pointed out that we should be careful when
> concatenating file names to avoid duplicated slashes.  On Windows, a
> double slash at the beginning of a file path has a special meaning.  So
> naively concatenating "/"  and "foo/bar" would give "//foo/bar", which
> would not give the desired results.  We already have a few spots doing:
> 
>   if (first_path ends with a slash)
>     path = first_path + second_path
>   else
>     path = first_path + slash + second_path
> 
> In general, I think it's nice to avoid superfluous slashes in file
> paths, since they might end up visible to the user and look a bit
> unprofessional.
> 
> Introduce the path_join function that can be used to join multiple path
> components together (along with unit tests).

Thanks.

> +static void
> +test ()
> +{
> +  std::string s;
> +
> +  s = ::path_join ("/foo", "bar", nullptr);
> +  SELF_CHECK (s == "/foo/bar");
> +
> +  s = ::path_join ("/foo", "/bar", nullptr);
> +  SELF_CHECK (s == "/foo/bar");
> +
> +  s = ::path_join ("/", "bar", nullptr);
> +  SELF_CHECK (s == "/bar");
> +
> +  s = ::path_join ("foo/", "/bar", nullptr);
> +  SELF_CHECK (s == "foo/bar");
> +
> +  s = ::path_join ("foo", "bar", "", nullptr);
> +  SELF_CHECK (s == "foo/bar");
> +
> +  s = ::path_join ("", "/foo", nullptr);
> +  SELF_CHECK (s == "/foo");
> +
> +  s = ::path_join ("", "foo", nullptr);
> +  SELF_CHECK (s == "foo");
> +
> +  s = ::path_join ("foo", "", "bar", nullptr);
> +  SELF_CHECK (s == "foo/bar");
> +
> +  s = ::path_join ("foo", "", nullptr);
> +  SELF_CHECK (s == "foo");
> +
> +  s = ::path_join ("foo/", "", nullptr);
> +  SELF_CHECK (s == "foo/");

Suggest to add a couple of Windows-specific tests here: one which
starts with "d:/" instead of just "/", and another with backslashes.

> +std::string
> +path_join (const char *component...)
> +{
> +  std::string path = component;
> +
> +  auto skip_leading_dir_seps = [] (const char *s)
> +    {
> +      while (*s == '/')
> +	++s;
> +
> +      return s;
> +    };
> +
> +  va_list args;
> +  va_start (args, component);
> +
> +  const char *c = va_arg (args, const char *);
> +  while (c != nullptr)
> +    {
> +      if (!path.empty ())
> +	c = skip_leading_dir_seps (c);
> +
> +      if (*c != '\0')
> +	{
> +	  if (!path.empty () && path.back () != '/')
> +	    path += '/';
> +
> +	  path += c;
> +	}
> +
> +      c = va_arg (args, const char *);
> +    }
> +
> +  va_end (args);
> +
> +  return path;
> +}

I think this should use IS_DIR_SEPARATOR (or override the '=' and '!='
operators), to support Windows file names with backslashes.

Re: [PATCH 5/5] gdbsupport: add path_join function

[Lancelot SIX]

Hi,

> +/* Join COMPONENT and all subsequent null-terminated string arguments into a
> +   single path.
> +
> +   The last argument must be nullptr to denote the end of the argument list
> +
> +   Empty arguments or arguments consisting only of directory separators are
> +   ignored.  */
> +
> +extern std::string path_join (const char *component, ...)
> +  ATTRIBUTE_SENTINEL;

The last part of the interface seems odd to me.  Mostly because it is
quite different to the os.path.join API from python.

Most notable, in python `os.path.join (a, b)` will yield b if b is an
absolute path:

    >>> os.path.join("/home/foo", "/")
    '/'
    >>> os.path.join("/foo", "/bar")
    '/bar'

One rational for this behavior would be to try to mimic using cd on each
argument in order:

    def strange_join(*args):
        cwd = os.getcwd()
	try:
	    for comp in args:
	        os.chdir(comp)
	    return os.getcwd()
        finally:
	    os.chdir(cwd)

Of course this implementation would be wrong for multiple reasons:
- it must work for dirs which do not exist
- the ".." remain ".."
- many other...

This behaviour regarding abs path is also consistent what I have with
std::filesystem in c++17 (which we be nice if we could use, but
unfortunately for the moment we have to stick to c++11)

    auto p = std::filesystem::path {"/foo"} / "/bar";
    assert (p == "/bar");

If you compare to one of the testcase you add, the behavior is
different:

    s = ::path_join ("/foo", "/bar", nullptr);
    SELF_CHECK (s == "/foo/bar");

Is this different semantics made on purpose?

Note that there is another part of the behavior which is different in
your proposal v.s.  python and std::filesystem.  Appending an empty
component at the end ensures that the path terminates with a path
separator:

    >>> os.path.join("foo", "")
    "foo/"

If I use your patch, I see:

    s = ::path_join ("foo", "", nullptr);
    SELF_CHECK (s == "foo");

Using similar semantics to std::filesystem might allow us to replace our
implementation with the c++ standard library when we switch to c++17.

On a different note, I am not sure I am a huge fan of the last nullptr
argument which is required.

You can achieve a similar interface except for this nullptr using light
variadic templates (not too much, should be easy enough to maintain).
Is this something you considered?

One implementation could be:

    /* The real worker can be compiled in a .cc file if you wish.  */
    extern std::string path_join_1 (std::string a, std::string b);

    template <typename ...Args)
    std::string
    path_join (std::string a, std::string b, Args... comps)
    {
      return path_join (path_join (a, b), comps...);
    }

    template <>
    std::string
    path_join (std::string a, std::string b)
    {
      return path_join_1 (a, b);
    }

I do not pretend this is optimal (it is not).  In this POC I pass all
arguments as std::string and by value, this is probably not what you
want to do in a production grade implementation, but this just gives the
overall idea.

What do you think?

Best,
Lancelot.

Re: [PATCH 5/5] gdbsupport: add path_join function

[Lancelot SIX]

Hi,

Below is a modified version of your patch which has a behavior closer to
std::filesystem when joining paths.  I re-used your testcases, only
modifying them on where they differ with what std::filesystem would do.

I also checked the testcases with a std::filesystem::path based
implementation of path_join_1 to make sure I actually have identical
results:

    std::string
    path_join_1 (const char *a, const char *b)
    {
      return std::filesystem::path {a} / b;
    }

Feel free to reuse this or not, at your discretion.

Only tested on GNU/Linux.

Best,
Lancelot.

P.S. maybe we want to use std::filesystem when compiling in c++17 mode,
not quite sure.  This could imply some configure changes since some
versions of gcc require `-lstdc++fs`

---
From 71c2ff08fb89509e7485b5fc8e43cc0097a48823 Mon Sep 17 00:00:00 2001
From: Simon Marchi <simon.marchi@efficios.com>
Date: Thu, 14 Apr 2022 16:01:37 -0400
Subject: [PATCH] gdbsupport: add path_join function

In this review [1], Eli pointed out that we should be careful when
concatenating file names to avoid duplicated slashes.  On Windows, a
double slash at the beginning of a file path has a special meaning.  So
naively concatenating "/"  and "foo/bar" would give "//foo/bar", which
would not give the desired results.  We already have a few spots doing:

  if (first_path ends with a slash)
    path = first_path + second_path
  else
    path = first_path + slash + second_path

In general, I think it's nice to avoid superfluous slashes in file
paths, since they might end up visible to the user and look a bit
unprofessional.

Introduce the path_join function that can be used to join multiple path
components together (along with unit tests).

Change a few spots to use path_join to show how it can be used.

[1] https://sourceware.org/pipermail/gdb-patches/2022-April/187559.html

Change-Id: I0df889f7e3f644e045f42ff429277b732eb6c752
---
 gdb/Makefile.in                     |  1 +
 gdb/buildsym.c                      |  5 +-
 gdb/dwarf2/line-header.c            |  3 +-
 gdb/dwarf2/read.c                   | 42 ++++++++--------
 gdb/macrotab.c                      |  3 +-
 gdb/unittests/path-join-selftests.c | 75 +++++++++++++++++++++++++++++
 gdbsupport/pathstuff.cc             | 40 +++++++++------
 gdbsupport/pathstuff.h              | 21 ++++++++
 8 files changed, 148 insertions(+), 42 deletions(-)
 create mode 100644 gdb/unittests/path-join-selftests.c

diff --git a/gdb/Makefile.in b/gdb/Makefile.in
index c8e140b5544..ec0e55dd803 100644
--- a/gdb/Makefile.in
+++ b/gdb/Makefile.in
@@ -465,6 +465,7 @@ SELFTESTS_SRCS = \
 	unittests/optional-selftests.c \
 	unittests/parallel-for-selftests.c \
 	unittests/parse-connection-spec-selftests.c \
+	unittests/path-join-selftests.c \
 	unittests/ptid-selftests.c \
 	unittests/main-thread-selftests.c \
 	unittests/mkdir-recursive-selftests.c \
diff --git a/gdb/buildsym.c b/gdb/buildsym.c
index 628903d674f..b0d79440987 100644
--- a/gdb/buildsym.c
+++ b/gdb/buildsym.c
@@ -20,6 +20,7 @@
 #include "buildsym-legacy.h"
 #include "bfd.h"
 #include "gdbsupport/gdb_obstack.h"
+#include "gdbsupport/pathstuff.h"
 #include "symtab.h"
 #include "symfile.h"
 #include "objfiles.h"
@@ -507,8 +508,8 @@ buildsym_compunit::start_subfile (const char *name)
 	  && !IS_ABSOLUTE_PATH (subfile->name)
 	  && !m_comp_dir.empty ())
 	{
-	  subfile_name_holder = string_printf ("%s/%s", m_comp_dir.c_str (),
-					       subfile->name.c_str ());
+	  subfile_name_holder = path_join (m_comp_dir.c_str (),
+					   subfile->name.c_str ());
 	  subfile_name = subfile_name_holder.c_str ();
 	}
       else
diff --git a/gdb/dwarf2/line-header.c b/gdb/dwarf2/line-header.c
index 77e7e553b21..8f4eb4f124c 100644
--- a/gdb/dwarf2/line-header.c
+++ b/gdb/dwarf2/line-header.c
@@ -24,6 +24,7 @@
 #include "dwarf2/read.h"
 #include "complaints.h"
 #include "filenames.h"
+#include "gdbsupport/pathstuff.h"
 
 void
 line_header::add_include_dir (const char *include_dir)
@@ -73,7 +74,7 @@ line_header::file_file_name (int file) const
 	{
 	  const char *dir = fe->include_dir (this);
 	  if (dir != NULL)
-	    return string_printf ("%s/%s", dir, fe->name);
+	    return path_join (dir, fe->name);
 	}
 
       return fe->name;
diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index 6dcd446e5f4..1fd39e903dc 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -1343,7 +1343,7 @@ static const char *compute_include_file_name
      (const struct line_header *lh,
       const file_entry &fe,
       const file_and_directory &cu_info,
-      gdb::unique_xmalloc_ptr<char> *name_holder);
+      std::string &name_holder);
 
 static htab_up allocate_signatured_type_table ();
 
@@ -2809,9 +2809,9 @@ dw2_get_file_names_reader (const struct die_reader_specs *reader,
     {
       for (const auto &entry : lh->file_names ())
 	{
-	  gdb::unique_xmalloc_ptr<char> name_holder;
+	  std::string name_holder;
 	  const char *include_name =
-	    compute_include_file_name (lh.get (), entry, fnd, &name_holder);
+	    compute_include_file_name (lh.get (), entry, fnd, name_holder);
 	  if (include_name != nullptr)
 	    {
 	      include_name = per_objfile->objfile->intern (include_name);
@@ -11166,14 +11166,13 @@ open_dwo_file (dwarf2_per_objfile *per_objfile,
 
   if (comp_dir != NULL)
     {
-      gdb::unique_xmalloc_ptr<char> path_to_try
-	(concat (comp_dir, SLASH_STRING, file_name, (char *) NULL));
+      std::string path_to_try = path_join (comp_dir, file_name);
 
       /* NOTE: If comp_dir is a relative path, this will also try the
 	 search path, which seems useful.  */
-      gdb_bfd_ref_ptr abfd (try_open_dwop_file (per_objfile, path_to_try.get (),
-						0 /*is_dwp*/,
-						1 /*search_cwd*/));
+      gdb_bfd_ref_ptr abfd (try_open_dwop_file
+	(per_objfile, path_to_try.c_str (), 0 /*is_dwp*/, 1 /*search_cwd*/));
+
       if (abfd != NULL)
 	return abfd;
     }
@@ -19751,14 +19750,14 @@ dwarf_decode_line_header (sect_offset sect_off, struct dwarf2_cu *cu)
 static const char *
 compute_include_file_name (const struct line_header *lh, const file_entry &fe,
 			   const file_and_directory &cu_info,
-			   gdb::unique_xmalloc_ptr<char> *name_holder)
+			   std::string &name_holder)
 {
   const char *include_name = fe.name;
   const char *include_name_to_compare = include_name;
 
   const char *dir_name = fe.include_dir (lh);
 
-  gdb::unique_xmalloc_ptr<char> hold_compare;
+  std::string hold_compare;
   if (!IS_ABSOLUTE_PATH (include_name)
       && (dir_name != nullptr || cu_info.get_comp_dir () != nullptr))
     {
@@ -19785,27 +19784,24 @@ compute_include_file_name (const struct line_header *lh, const file_entry &fe,
 
       if (dir_name != NULL)
 	{
-	  name_holder->reset (concat (dir_name, SLASH_STRING,
-				      include_name, (char *) NULL));
-	  include_name = name_holder->get ();
+	  name_holder = path_join (dir_name, include_name);
+	  include_name = name_holder.c_str ();
 	  include_name_to_compare = include_name;
 	}
       if (!IS_ABSOLUTE_PATH (include_name)
 	  && cu_info.get_comp_dir () != nullptr)
 	{
-	  hold_compare.reset (concat (cu_info.get_comp_dir (), SLASH_STRING,
-				      include_name, (char *) NULL));
-	  include_name_to_compare = hold_compare.get ();
+	  hold_compare = path_join (cu_info.get_comp_dir (), include_name);
+	  include_name_to_compare = hold_compare.c_str ();
 	}
     }
 
-  gdb::unique_xmalloc_ptr<char> copied_name;
+  std::string copied_name;
   const char *cu_filename = cu_info.get_name ();
   if (!IS_ABSOLUTE_PATH (cu_filename) && cu_info.get_comp_dir () != nullptr)
     {
-      copied_name.reset (concat (cu_info.get_comp_dir (), SLASH_STRING,
-				 cu_filename, (char *) NULL));
-      cu_filename = copied_name.get ();
+      copied_name = path_join (cu_info.get_comp_dir (), cu_filename);
+      cu_filename = copied_name.c_str ();
     }
 
   if (FILENAME_CMP (include_name_to_compare, cu_filename) == 0)
@@ -20540,7 +20536,7 @@ static void
 dwarf2_start_subfile (struct dwarf2_cu *cu, const char *filename,
 		      const char *dirname)
 {
-  gdb::unique_xmalloc_ptr<char> copy;
+  std::string copy;
 
   /* In order not to lose the line information directory,
      we concatenate it to the filename when it makes sense.
@@ -20551,8 +20547,8 @@ dwarf2_start_subfile (struct dwarf2_cu *cu, const char *filename,
 
   if (!IS_ABSOLUTE_PATH (filename) && dirname != NULL)
     {
-      copy.reset (concat (dirname, SLASH_STRING, filename, (char *) NULL));
-      filename = copy.get ();
+      copy = path_join (dirname, filename);
+      filename = copy.c_str ();
     }
 
   cu->get_builder ()->start_subfile (filename);
diff --git a/gdb/macrotab.c b/gdb/macrotab.c
index 7ca3f312d33..108e6f1bbae 100644
--- a/gdb/macrotab.c
+++ b/gdb/macrotab.c
@@ -19,6 +19,7 @@
 
 #include "defs.h"
 #include "gdbsupport/gdb_obstack.h"
+#include "gdbsupport/pathstuff.h"
 #include "splay-tree.h"
 #include "filenames.h"
 #include "symtab.h"
@@ -1071,5 +1072,5 @@ macro_source_fullname (struct macro_source_file *file)
   if (comp_dir == NULL || IS_ABSOLUTE_PATH (file->filename))
     return file->filename;
 
-  return std::string (comp_dir) + SLASH_STRING + file->filename;
+  return path_join (comp_dir, file->filename);
 }
diff --git a/gdb/unittests/path-join-selftests.c b/gdb/unittests/path-join-selftests.c
new file mode 100644
index 00000000000..6896626eb6f
--- /dev/null
+++ b/gdb/unittests/path-join-selftests.c
@@ -0,0 +1,75 @@
+/* Self tests for path_join for GDB, the GNU debugger.
+
+   Copyright (C) 2022 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 "gdbsupport/pathstuff.h"
+#include "gdbsupport/selftest.h"
+
+namespace selftests {
+namespace path_join {
+
+/* Test path_join.  */
+
+static void
+test ()
+{
+  std::string s;
+
+  s = ::path_join ("/foo", "bar");
+  SELF_CHECK (s == "/foo/bar");
+
+  s = ::path_join ("/foo", "/bar");
+  SELF_CHECK (s == "/bar");
+
+  s = ::path_join ("/", "bar");
+  SELF_CHECK (s == "/bar");
+
+  s = ::path_join ("foo/", "/bar");
+  SELF_CHECK (s == "/bar");
+
+  s = ::path_join ("foo", "bar", "");
+  SELF_CHECK (s == "foo/bar/");
+
+  s = ::path_join ("", "/foo");
+  SELF_CHECK (s == "/foo");
+
+  s = ::path_join ("", "foo");
+  SELF_CHECK (s == "foo");
+
+  s = ::path_join ("foo", "", "bar");
+  SELF_CHECK (s == "foo/bar");
+
+  s = ::path_join ("foo", "");
+  SELF_CHECK (s == "foo/");
+
+  s = ::path_join ("foo/", "");
+  SELF_CHECK (s == "foo/");
+}
+
+}
+}
+
+void _initialize_path_join_selftests ();
+void
+_initialize_path_join_selftests ()
+{
+  selftests::register_test ("path_join",
+			    selftests::path_join::test);
+}
+
diff --git a/gdbsupport/pathstuff.cc b/gdbsupport/pathstuff.cc
index ac65651d449..9ef0b15ad3c 100644
--- a/gdbsupport/pathstuff.cc
+++ b/gdbsupport/pathstuff.cc
@@ -119,10 +119,7 @@ gdb_realpath_keepfile (const char *filename)
      directory separator, avoid doubling it.  */
   gdb::unique_xmalloc_ptr<char> path_storage = gdb_realpath (dir_name);
   const char *real_path = path_storage.get ();
-  if (IS_DIR_SEPARATOR (real_path[strlen (real_path) - 1]))
-    return string_printf ("%s%s", real_path, base_name);
-  else
-    return string_printf ("%s/%s", real_path, base_name);
+  return path_join (real_path, base_name);
 }
 
 /* See gdbsupport/pathstuff.h.  */
@@ -138,12 +135,7 @@ gdb_abspath (const char *path)
   if (IS_ABSOLUTE_PATH (path) || current_directory == NULL)
     return path;
 
-  /* Beware the // my son, the Emacs barfs, the botch that catch...  */
-  return string_printf
-    ("%s%s%s", current_directory,
-     (IS_DIR_SEPARATOR (current_directory[strlen (current_directory) - 1])
-      ? "" : SLASH_STRING),
-     path);
+  return path_join (current_directory, path);
 }
 
 /* See gdbsupport/pathstuff.h.  */
@@ -198,6 +190,24 @@ child_path (const char *parent, const char *child)
 
 /* See gdbsupport/pathstuff.h.  */
 
+std::string
+path_join_1 (const char *a, const char *b)
+{
+  gdb_assert (a != nullptr);
+  gdb_assert (b != nullptr);
+
+  if (strlen (a) == 0 || IS_ABSOLUTE_PATH (b))
+    return {b};
+
+  std::string path = a;
+  if (!IS_DIR_SEPARATOR (path.back ()))
+    path += '/';
+
+  return path + b;
+}
+
+/* See gdbsupport/pathstuff.h.  */
+
 bool
 contains_dir_separator (const char *path)
 {
@@ -227,7 +237,7 @@ get_standard_cache_dir ()
     {
       /* Make sure the path is absolute and tilde-expanded.  */
       std::string abs = gdb_abspath (xdg_cache_home);
-      return string_printf ("%s/gdb", abs.c_str ());
+      return path_join (abs.c_str (), "gdb");
     }
 #endif
 
@@ -236,7 +246,7 @@ get_standard_cache_dir ()
     {
       /* Make sure the path is absolute and tilde-expanded.  */
       std::string abs = gdb_abspath (home);
-      return string_printf ("%s/" HOME_CACHE_DIR "/gdb", abs.c_str ());
+      return path_join (abs.c_str (), HOME_CACHE_DIR,  "gdb");
     }
 
 #ifdef WIN32
@@ -245,7 +255,7 @@ get_standard_cache_dir ()
     {
       /* Make sure the path is absolute and tilde-expanded.  */
       std::string abs = gdb_abspath (win_home);
-      return string_printf ("%s/gdb", abs.c_str ());
+      return path_join (abs.c_str (), "gdb");
     }
 #endif
 
@@ -294,7 +304,7 @@ get_standard_config_dir ()
     {
       /* Make sure the path is absolute and tilde-expanded.  */
       std::string abs = gdb_abspath (xdg_config_home);
-      return string_printf ("%s/gdb", abs.c_str ());
+      return path_join (abs.c_str (), "gdb");
     }
 #endif
 
@@ -303,7 +313,7 @@ get_standard_config_dir ()
     {
       /* Make sure the path is absolute and tilde-expanded.  */
       std::string abs = gdb_abspath (home);
-      return string_printf ("%s/" HOME_CONFIG_DIR "/gdb", abs.c_str ());
+      return path_join (abs.c_str (), HOME_CONFIG_DIR, "gdb");
     }
 
   return {};
diff --git a/gdbsupport/pathstuff.h b/gdbsupport/pathstuff.h
index f6c51e9de88..d543eecebdc 100644
--- a/gdbsupport/pathstuff.h
+++ b/gdbsupport/pathstuff.h
@@ -60,6 +60,27 @@ extern std::string gdb_abspath (const char *path);
 
 extern const char *child_path (const char *parent, const char *child);
 
+/* Worker function for join_path_1 joining A and B into a single path.  */
+
+extern std::string path_join_1 (const char *a, const char *b);
+
+/* Join A, B and all subsequent null-terminated string arguments into a
+   single path.  */
+
+template<typename ...Args>
+inline std::string
+path_join (const char *a, const char *b, Args ...rest)
+{
+  return path_join (path_join (a, b).c_str (), rest...);
+}
+
+template<>
+inline std::string
+path_join (const char *a, const char *b)
+{
+  return path_join_1 (a, b);
+}
+
 /* Return whether PATH contains a directory separator character.  */
 
 extern bool contains_dir_separator (const char *path);
-- 
2.35.1

Re: [PATCH 5/5] gdbsupport: add path_join function

[Simon Marchi]

On 2022-04-15 01:59, Eli Zaretskii wrote:
>> Date: Thu, 14 Apr 2022 16:01:37 -0400
>> From: Simon Marchi via Gdb-patches <gdb-patches@sourceware.org>
>> Cc: Simon Marchi <simon.marchi@efficios.com>
>>
>> From: Simon Marchi <simon.marchi@efficios.com>
>>
>> In this review [1], Eli pointed out that we should be careful when
>> concatenating file names to avoid duplicated slashes.  On Windows, a
>> double slash at the beginning of a file path has a special meaning.  So
>> naively concatenating "/"  and "foo/bar" would give "//foo/bar", which
>> would not give the desired results.  We already have a few spots doing:
>>
>>   if (first_path ends with a slash)
>>     path = first_path + second_path
>>   else
>>     path = first_path + slash + second_path
>>
>> In general, I think it's nice to avoid superfluous slashes in file
>> paths, since they might end up visible to the user and look a bit
>> unprofessional.
>>
>> Introduce the path_join function that can be used to join multiple path
>> components together (along with unit tests).
> 
> Thanks.
> 
>> +static void
>> +test ()
>> +{
>> +  std::string s;
>> +
>> +  s = ::path_join ("/foo", "bar", nullptr);
>> +  SELF_CHECK (s == "/foo/bar");
>> +
>> +  s = ::path_join ("/foo", "/bar", nullptr);
>> +  SELF_CHECK (s == "/foo/bar");
>> +
>> +  s = ::path_join ("/", "bar", nullptr);
>> +  SELF_CHECK (s == "/bar");
>> +
>> +  s = ::path_join ("foo/", "/bar", nullptr);
>> +  SELF_CHECK (s == "foo/bar");
>> +
>> +  s = ::path_join ("foo", "bar", "", nullptr);
>> +  SELF_CHECK (s == "foo/bar");
>> +
>> +  s = ::path_join ("", "/foo", nullptr);
>> +  SELF_CHECK (s == "/foo");
>> +
>> +  s = ::path_join ("", "foo", nullptr);
>> +  SELF_CHECK (s == "foo");
>> +
>> +  s = ::path_join ("foo", "", "bar", nullptr);
>> +  SELF_CHECK (s == "foo/bar");
>> +
>> +  s = ::path_join ("foo", "", nullptr);
>> +  SELF_CHECK (s == "foo");
>> +
>> +  s = ::path_join ("foo/", "", nullptr);
>> +  SELF_CHECK (s == "foo/");
> 
> Suggest to add a couple of Windows-specific tests here: one which
> starts with "d:/" instead of just "/", and another with backslashes.

Should these tests only be ran on Windows hosts?  In an ideal world,
this function (and the rest of path handling in GDB) should support
cross-debugging both ways: GDB on Linux debugging a remote Windows
program, GDB on Windows debugging a remote Linux program.  That means
corner cases like GDB on Linux should not recognize C:/foo as an
absolute path when debugging a native Linux program, but it should
recognize it as an absolute path when debugging a remote Windows
program.  And it should recognize foo\bar\ as ending with a directory
separator when debugging remotely a Windows program, and not when
debugging natively.  And vice-versa for a Windows GDB / Linux program.

Existing code isn't written with that in mind though (macros like
IS_DIR_SEPARATOR are host-specific), and I'm not ready to tackle that
right now.   So I've written path_join ignoring those cross-debugging
scenarios.

So, if I add a test like:

  s = ::path_join ("foo\\", "bar", nullptr);
  SELF_CHECK (s == "foo\\bar" || s == "foo/bar");

it won't pass on Linux, as s will be "foo\\/bar".  So, as long as
path_join is host-dependent, I think we'll need to accept that some
tests will be host-dependent.  That's not ideal, but that's how things
are at the moment.

>> +std::string
>> +path_join (const char *component...)
>> +{
>> +  std::string path = component;
>> +
>> +  auto skip_leading_dir_seps = [] (const char *s)
>> +    {
>> +      while (*s == '/')
>> +	++s;
>> +
>> +      return s;
>> +    };
>> +
>> +  va_list args;
>> +  va_start (args, component);
>> +
>> +  const char *c = va_arg (args, const char *);
>> +  while (c != nullptr)
>> +    {
>> +      if (!path.empty ())
>> +	c = skip_leading_dir_seps (c);
>> +
>> +      if (*c != '\0')
>> +	{
>> +	  if (!path.empty () && path.back () != '/')
>> +	    path += '/';
>> +
>> +	  path += c;
>> +	}
>> +
>> +      c = va_arg (args, const char *);
>> +    }
>> +
>> +  va_end (args);
>> +
>> +  return path;
>> +}
> 
> I think this should use IS_DIR_SEPARATOR (or override the '=' and '!='
> operators), to support Windows file names with backslashes.

Changed both instances of '/' to use IS_DIR_SEPARATOR.

Simon

Re: [PATCH 5/5] gdbsupport: add path_join function

[Eli Zaretskii]

> Date: Mon, 18 Apr 2022 14:11:34 -0400
> Cc: gdb-patches@sourceware.org
> From: Simon Marchi <simon.marchi@polymtl.ca>
> 
> > Suggest to add a couple of Windows-specific tests here: one which
> > starts with "d:/" instead of just "/", and another with backslashes.
> 
> Should these tests only be ran on Windows hosts?

Yes, I think so.

> > I think this should use IS_DIR_SEPARATOR (or override the '=' and '!='
> > operators), to support Windows file names with backslashes.
> 
> Changed both instances of '/' to use IS_DIR_SEPARATOR.

Thanks.

Re: [PATCH 5/5] gdbsupport: add path_join function

[Simon Marchi]

On 2022-04-15 10:38, Lancelot SIX wrote:
> Hi,
> 
>> +/* Join COMPONENT and all subsequent null-terminated string arguments into a
>> +   single path.
>> +
>> +   The last argument must be nullptr to denote the end of the argument list
>> +
>> +   Empty arguments or arguments consisting only of directory separators are
>> +   ignored.  */
>> +
>> +extern std::string path_join (const char *component, ...)
>> +  ATTRIBUTE_SENTINEL;
> 
> The last part of the interface seems odd to me.  Mostly because it is
> quite different to the os.path.join API from python.
> 
> Most notable, in python `os.path.join (a, b)` will yield b if b is an
> absolute path:
> 
>     >>> os.path.join("/home/foo", "/")
>     '/'
>     >>> os.path.join("/foo", "/bar")
>     '/bar'
> 
> One rational for this behavior would be to try to mimic using cd on each
> argument in order:
> 
>     def strange_join(*args):
>         cwd = os.getcwd()
> 	try:
> 	    for comp in args:
> 	        os.chdir(comp)
> 	    return os.getcwd()
>         finally:
> 	    os.chdir(cwd)
> 
> Of course this implementation would be wrong for multiple reasons:
> - it must work for dirs which do not exist
> - the ".." remain ".."
> - many other...
> 
> This behaviour regarding abs path is also consistent what I have with
> std::filesystem in c++17 (which we be nice if we could use, but
> unfortunately for the moment we have to stick to c++11)
> 
>     auto p = std::filesystem::path {"/foo"} / "/bar";
>     assert (p == "/bar");
> 
> If you compare to one of the testcase you add, the behavior is
> different:
> 
>     s = ::path_join ("/foo", "/bar", nullptr);
>     SELF_CHECK (s == "/foo/bar");
> 
> Is this different semantics made on purpose?

I indeed noticed how os.path.join handled ("/foo", "/bar").

I went with the behavior in my patch mostly because there are cases in
GDB where we really need to append absolute paths to another path.  For
example, looking up files in sysroots.  If the sysroot is set to
"/the/sysroot/" (with or without the trailing slash) and we want to load
the target library /usr/lib/libfoo.so, it would be nice if

    path_join (sysroot_path, lib_path)

gave

    /the/sysroot/usr/lib/libfoo.so

without a double-slash.  Same when we are looking up files in the
debug-file-directory (/usr/lib/debug).

When changing those implementations that currently use concat(...) or
string_printf("%s/%s"), we would need to be careful: if the right hand
side is an absolute path, then we would have to skip the leading
directory separator before passing it to path_join, to get the desired
result.  And then we're back at having some special path handling
details everywhere.

So, IMO, the behavior I implemented makes more sense for us.  I just
don't know what it would mean on Windows.  What would this give?

  path_join ("C:/hello", "D:/hi", nullptr);

> Note that there is another part of the behavior which is different in
> your proposal v.s.  python and std::filesystem.  Appending an empty
> component at the end ensures that the path terminates with a path
> separator:
> 
>     >>> os.path.join("foo", "")
>     "foo/"
> 
> If I use your patch, I see:
> 
>     s = ::path_join ("foo", "", nullptr);
>     SELF_CHECK (s == "foo");

I also noticed that.  In my opinion, adding that last slash is not
really useful.  For consistency, we should always present directories
the same way, either with or without a trailing slash.  Assuming we have
this code:

  s = path_join ("/base/dir", either_a_subdirectory_or_empty, nullptr);

Then if either_a_subdirectory_or_empty is empty, I'd like the result to
be /base/dir, exactly as if we had done:

  if (!either_a_subdirectory_or_empty.empty ())
    s = path_join ("/base/dir", either_a_subdirectory_or_empty, nullptr);
  else
    s = "/base/dir";

So again, the reason for my choice is just that it likely leads to
simpler code at call sites, in our probable use cases.

> Using similar semantics to std::filesystem might allow us to replace our
> implementation with the c++ standard library when we switch to c++17.

I understand the idea, and considered it.  The trailing slash one isn't
very important, but I think the std::filesystem behavior with the
absolute path would be annoying.

> On a different note, I am not sure I am a huge fan of the last nullptr
> argument which is required.
> 
> You can achieve a similar interface except for this nullptr using light
> variadic templates (not too much, should be easy enough to maintain).
> Is this something you considered?
> 
> One implementation could be:
> 
>     /* The real worker can be compiled in a .cc file if you wish.  */
>     extern std::string path_join_1 (std::string a, std::string b);
> 
>     template <typename ...Args)
>     std::string
>     path_join (std::string a, std::string b, Args... comps)
>     {
>       return path_join (path_join (a, b), comps...);
>     }
> 
>     template <>
>     std::string
>     path_join (std::string a, std::string b)
>     {
>       return path_join_1 (a, b);
>     }
> 
> I do not pretend this is optimal (it is not).  In this POC I pass all
> arguments as std::string and by value, this is probably not what you
> want to do in a production grade implementation, but this just gives the
> overall idea.
> 
> What do you think?

If you can make it work using gdb::string_view or const char *, I think
it would be ok.  I'll give it a try.  But otherwise, I am personally
fine with the sentinel nullptr, given that the compiler gives you a
warning if you forget it.

Simon

Re: [PATCH 5/5] gdbsupport: add path_join function

[Pedro Alves]

On 2022-04-18 19:43, Simon Marchi via Gdb-patches wrote:
>> One implementation could be:
>>
>>     /* The real worker can be compiled in a .cc file if you wish.  */
>>     extern std::string path_join_1 (std::string a, std::string b);
>>
>>     template <typename ...Args)
>>     std::string
>>     path_join (std::string a, std::string b, Args... comps)
>>     {
>>       return path_join (path_join (a, b), comps...);
>>     }
>>
>>     template <>
>>     std::string
>>     path_join (std::string a, std::string b)
>>     {
>>       return path_join_1 (a, b);
>>     }
>>
>> I do not pretend this is optimal (it is not).  In this POC I pass all
>> arguments as std::string and by value, this is probably not what you
>> want to do in a production grade implementation, but this just gives the
>> overall idea.
>>
>> What do you think?
> If you can make it work using gdb::string_view or const char *, I think
> it would be ok.  I'll give it a try.  But otherwise, I am personally
> fine with the sentinel nullptr, given that the compiler gives you a
> warning if you forget it.

I think it would just work as is you replace std::string with const char *
in Lancelot's snippet above.  FWIW, I had the same thought as Lancelot
about using variable templates instead of varargs.

OOC, is there any place in GDB that wants to pass more than two components
to path_join?  I skimmed the patch and didn't notice one.

Re: [PATCH 5/5] gdbsupport: add path_join function

[Simon Marchi]

On 2022-04-18 15:09, Pedro Alves wrote:
> On 2022-04-18 19:43, Simon Marchi via Gdb-patches wrote:
>>> One implementation could be:
>>>
>>>     /* The real worker can be compiled in a .cc file if you wish.  */
>>>     extern std::string path_join_1 (std::string a, std::string b);
>>>
>>>     template <typename ...Args)
>>>     std::string
>>>     path_join (std::string a, std::string b, Args... comps)
>>>     {
>>>       return path_join (path_join (a, b), comps...);
>>>     }
>>>
>>>     template <>
>>>     std::string
>>>     path_join (std::string a, std::string b)
>>>     {
>>>       return path_join_1 (a, b);
>>>     }
>>>
>>> I do not pretend this is optimal (it is not).  In this POC I pass all
>>> arguments as std::string and by value, this is probably not what you
>>> want to do in a production grade implementation, but this just gives the
>>> overall idea.
>>>
>>> What do you think?
>> If you can make it work using gdb::string_view or const char *, I think
>> it would be ok.  I'll give it a try.  But otherwise, I am personally
>> fine with the sentinel nullptr, given that the compiler gives you a
>> warning if you forget it.
> 
> I think it would just work as is you replace std::string with const char *
> in Lancelot's snippet above.  FWIW, I had the same thought as Lancelot
> about using variable templates instead of varargs.
> 
> OOC, is there any place in GDB that wants to pass more than two components
> to path_join?  I skimmed the patch and didn't notice one.

I am not aware of one, I'd be happy to make path_join just have two
parameters and be done with it.

Simon

Re: [PATCH 5/5] gdbsupport: add path_join function

[Pedro Alves]

On 2022-04-18 19:43, Simon Marchi via Gdb-patches wrote:
> 
> So, IMO, the behavior I implemented makes more sense for us.  I just
> don't know what it would mean on Windows.  What would this give?
> 
>   path_join ("C:/hello", "D:/hi", nullptr);

In solib_find_1, we have_

  /* Note, we're interested in IS_TARGET_ABSOLUTE_PATH, not
     IS_ABSOLUTE_PATH.  The latter is for host paths only, while
     IN_PATHNAME is a target path.  For example, if we're supposed to
     be handling DOS-like semantics we want to consider a
     'c:/foo/bar.dll' path as an absolute path, even on a Unix box.
     With such a path, before giving up on the sysroot, we'll try:

       1st attempt, c:/foo/bar.dll ==> /sysroot/c:/foo/bar.dll
       2nd attempt, c:/foo/bar.dll ==> /sysroot/c/foo/bar.dll
       3rd attempt, c:/foo/bar.dll ==> /sysroot/foo/bar.dll
  */

...

  /* If the search in gdb_sysroot failed, and the path name has a
     drive spec (e.g, c:/foo), try stripping ':' from the drive spec,
     and retrying in the sysroot:
       c:/foo/bar.dll ==> /sysroot/c/foo/bar.dll.  */

  if (found_file < 0
      && sysroot != NULL
      && HAS_TARGET_DRIVE_SPEC (fskind, in_pathname))
    {



If you're going to convert the path concating here to use path_join,
then you'd want to just leave the D: in the path.  Or maybe this here
wouldn't be converted, not sure...  Ideally we'd have the same logic
where we prepend debug paths to source paths, I think.

Re: [PATCH 5/5] gdbsupport: add path_join function

[Pedro Alves]

On 2022-04-18 19:11, Simon Marchi via Gdb-patches wrote:

>> Suggest to add a couple of Windows-specific tests here: one which
>> starts with "d:/" instead of just "/", and another with backslashes.
> 
> Should these tests only be ran on Windows hosts?  In an ideal world,
> this function (and the rest of path handling in GDB) should support
> cross-debugging both ways: GDB on Linux debugging a remote Windows
> program, GDB on Windows debugging a remote Linux program.  That means
> corner cases like GDB on Linux should not recognize C:/foo as an
> absolute path when debugging a native Linux program, but it should
> recognize it as an absolute path when debugging a remote Windows
> program.  And it should recognize foo\bar\ as ending with a directory
> separator when debugging remotely a Windows program, and not when
> debugging natively.  And vice-versa for a Windows GDB / Linux program.
> 
> Existing code isn't written with that in mind though (macros like
> IS_DIR_SEPARATOR are host-specific), and I'm not ready to tackle that
> right now.   So I've written path_join ignoring those cross-debugging
> scenarios.
> 
> So, if I add a test like:
> 
>   s = ::path_join ("foo\\", "bar", nullptr);
>   SELF_CHECK (s == "foo\\bar" || s == "foo/bar");
> 
> it won't pass on Linux, as s will be "foo\\/bar".  So, as long as
> path_join is host-dependent, I think we'll need to accept that some
> tests will be host-dependent.  That's not ideal, but that's how things
> are at the moment.


Note we have:

(gdb) help set target-file-system-kind 
Set assumed file system kind for target reported file names.
If `unix', target file names (e.g., loaded shared library file names)
starting the forward slash (`/') character are considered absolute,
and the directory separator character is the forward slash (`/').  If
`dos-based', target file names starting with a drive letter followed
by a colon (e.g., `c:'), are also considered absolute, and the
backslash (`\') is also considered a directory separator.  Set to
`auto' (which is the default), to let GDB decide, based on its
knowledge of the target operating system.
(gdb) 

This is used in solib.c only currently.  Something like "source-file-system-kind"
might make sense to have too.  (Not suggesting we add it now.)

Re: [PATCH 1/5] gdb: call gdb_tilde_expand instead of gdb_tilde_expand_up in source_script_with_search

[Tom Tromey]

>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:

Simon> This removes a use of gdb_tilde_expand_up, which is removed later in
Simon> this series.

This looks good to me, thank you.

Tom

Re: [PATCH 2/5] gdbsupport: make gdb_abspath return an std::string

[Tom Tromey]

>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:

Simon> From: Simon Marchi <simon.marchi@efficios.com>
Simon> I'm trying to switch these functions to use std::string instead of char
Simon> arrays, as much as possible.  Some callers benefit from it (can avoid
Simon> doing a copy of the result), while others suffer (have to make one more
Simon> copy).

I hate copies but the main thing is to make sure none of them are in hot
paths.  This seems fine on that front.

Sometimes I wonder if gdb would be better off with its own string type
-- say a combination of gdb::unique_xmalloc_ptr<char> and string_view.
This would interact more nicely with legacy code without sacrificing C++
string methods.  We could stick the string length in there as well if we
thought that was worthwhile.

Tom

Re: [PATCH 3/5] gdbsupport: make gdb_realpath_keepfile return an std::string

[Tom Tromey]

>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:

Simon> From: Simon Marchi <simon.marchi@efficios.com>
Simon> I'm trying to switch these functions to use std::string instead of char
Simon> arrays, as much as possible.  Some callers benefit from it (can avoid
Simon> doing a copy of the result), while others suffer (have to make one more
Simon> copy).

Looks good.  Thank you.

Tom

Re: [PATCH 4/5] gdb: use gdb_tilde_expand instead of gdb_tilde_expand_up in source_script_with_search

[Tom Tromey]

>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:

Simon> From: Simon Marchi <simon.marchi@efficios.com>
Simon> Since this is the latest use of gdb_tilde_expand_up, remove it.

Thanks.  Seems obviously ok once the others go in.

Tom

Re: [PATCH 5/5] gdbsupport: add path_join function

[Tom Tromey]

>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:

Simon> If you can make it work using gdb::string_view or const char *, I think
Simon> it would be ok.  I'll give it a try.  But otherwise, I am personally
Simon> fine with the sentinel nullptr, given that the compiler gives you a
Simon> warning if you forget it.

I was also thinking of whether it could be done with string_view.
See the appended for an example.

The benefits of this are that the compiler won't let you pass in
something unless it is convertible to string_view, and also that any
combination of char*, string, and string_view can be passed without
fuss.

Tom

#include <string_view>
#include <array>
#include <string>

void concat (size_t n, const std::string_view *views)
{
  // ...
}

template<typename ...Args>
void
path_join (Args... args)
{
  std::array<std::string_view, sizeof... (Args)> views
    { std::string_view (args)... };
  concat (sizeof... (Args), views.data ());
}

int main () {
  path_join ("a", "b", "c");
  path_join (std::string ("a"), "b");
  return 0;
}

Re: [PATCH 2/5] gdbsupport: make gdb_abspath return an std::string

[Pedro Alves]

On 2022-04-18 20:41, Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
> Simon> From: Simon Marchi <simon.marchi@efficios.com>
> Simon> I'm trying to switch these functions to use std::string instead of char
> Simon> arrays, as much as possible.  Some callers benefit from it (can avoid
> Simon> doing a copy of the result), while others suffer (have to make one more
> Simon> copy).
> 
> I hate copies but the main thing is to make sure none of them are in hot
> paths.  This seems fine on that front.
> 
> Sometimes I wonder if gdb would be better off with its own string type
> -- say a combination of gdb::unique_xmalloc_ptr<char> and string_view.
> This would interact more nicely with legacy code without sacrificing C++
> string methods.  We could stick the string length in there as well if we
> thought that was worthwhile.

That'd be something like cstring_view, and a corresponding cstring type, I suppose:

 http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2019/p1402r0.pdf
 https://github.com/breyerml/cstring_view

I've seen other projects and public discussions on the interwebs adding or reporting
use of such types.

Re: [PATCH 2/5] gdbsupport: make gdb_abspath return an std::string

[Simon Marchi]

On 2022-04-18 15:41, Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
> Simon> From: Simon Marchi <simon.marchi@efficios.com>
> Simon> I'm trying to switch these functions to use std::string instead of char
> Simon> arrays, as much as possible.  Some callers benefit from it (can avoid
> Simon> doing a copy of the result), while others suffer (have to make one more
> Simon> copy).
> 
> I hate copies but the main thing is to make sure none of them are in hot
> paths.  This seems fine on that front.
> 
> Sometimes I wonder if gdb would be better off with its own string type
> -- say a combination of gdb::unique_xmalloc_ptr<char> and string_view.
> This would interact more nicely with legacy code without sacrificing C++
> string methods.  We could stick the string length in there as well if we
> thought that was worthwhile.

Yeah, we'll always be stuck with APIs (system or otherwise) that hand us
strings that we need free (so, use gdb::unique_xmalloc_ptr<char>).  And
as far as I know there's no way of building an std::string that would
take ownership of the malloc'ed char array.  We would have no choice but
to make a copy if we wanted to build an std::string out of it.  So
having our own string type that could do that would be a plus.  On the
other hand, the day we use an API that returns an std::string, we won't
be able to build our string type from the std::string without copying
it...

Simon

Re: [PATCH 4/5] gdb: use gdb_tilde_expand instead of gdb_tilde_expand_up in source_script_with_search

[Simon Marchi]

On 2022-04-18 15:44, Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
> Simon> From: Simon Marchi <simon.marchi@efficios.com>
> Simon> Since this is the latest use of gdb_tilde_expand_up, remove it.
> 
> Thanks.  Seems obviously ok once the others go in.
> 
> Tom

Thanks, I pushed the series up to this point.  This will make the re-spins
of the last patch easier.

Simon

Re: [PATCH 5/5] gdbsupport: add path_join function

[Simon Marchi]

> On 2022-04-18 15:09, Pedro Alves wrote:
>> On 2022-04-18 19:43, Simon Marchi via Gdb-patches wrote:
>>>> One implementation could be:
>>>>
>>>>     /* The real worker can be compiled in a .cc file if you wish.  */
>>>>     extern std::string path_join_1 (std::string a, std::string b);
>>>>
>>>>     template <typename ...Args)
>>>>     std::string
>>>>     path_join (std::string a, std::string b, Args... comps)
>>>>     {
>>>>       return path_join (path_join (a, b), comps...);
>>>>     }
>>>>
>>>>     template <>
>>>>     std::string
>>>>     path_join (std::string a, std::string b)
>>>>     {
>>>>       return path_join_1 (a, b);
>>>>     }
>>>>
>>>> I do not pretend this is optimal (it is not).  In this POC I pass all
>>>> arguments as std::string and by value, this is probably not what you
>>>> want to do in a production grade implementation, but this just gives the
>>>> overall idea.
>>>>
>>>> What do you think?
>>> If you can make it work using gdb::string_view or const char *, I think
>>> it would be ok.  I'll give it a try.  But otherwise, I am personally
>>> fine with the sentinel nullptr, given that the compiler gives you a
>>> warning if you forget it.
>>
>> I think it would just work as is you replace std::string with const char *
>> in Lancelot's snippet above.  FWIW, I had the same thought as Lancelot
>> about using variable templates instead of varargs.
>>
>> OOC, is there any place in GDB that wants to pass more than two components
>> to path_join?  I skimmed the patch and didn't notice one.
> 
> I am not aware of one, I'd be happy to make path_join just have two
> parameters and be done with it.

Actually, I have one use for it in my upcoming DWARF reader changes,
where I join the CU's compilation directory, the directory (from the
line header directory table) and the file name.

I like Tom's proposal, here:

  https://sourceware.org/pipermail/gdb-patches/2022-April/188016.html

It seems efficient (no extraneous string copies) and type-safe.  I
successfully changed my patch to use it.

Simon

Re: [PATCH 5/5] gdbsupport: add path_join function

[Pedro Alves]

On 2022-04-18 21:55, Simon Marchi wrote:
>> On 2022-04-18 15:09, Pedro Alves wrote:
>>> On 2022-04-18 19:43, Simon Marchi via Gdb-patches wrote:
>>>>> One implementation could be:
>>>>>
>>>>>     /* The real worker can be compiled in a .cc file if you wish.  */
>>>>>     extern std::string path_join_1 (std::string a, std::string b);
>>>>>
>>>>>     template <typename ...Args)
>>>>>     std::string
>>>>>     path_join (std::string a, std::string b, Args... comps)
>>>>>     {
>>>>>       return path_join (path_join (a, b), comps...);
>>>>>     }
>>>>>
>>>>>     template <>
>>>>>     std::string
>>>>>     path_join (std::string a, std::string b)
>>>>>     {
>>>>>       return path_join_1 (a, b);
>>>>>     }
>>>>>
>>>>> I do not pretend this is optimal (it is not).  In this POC I pass all
>>>>> arguments as std::string and by value, this is probably not what you
>>>>> want to do in a production grade implementation, but this just gives the
>>>>> overall idea.
>>>>>
>>>>> What do you think?
>>>> If you can make it work using gdb::string_view or const char *, I think
>>>> it would be ok.  I'll give it a try.  But otherwise, I am personally
>>>> fine with the sentinel nullptr, given that the compiler gives you a
>>>> warning if you forget it.
>>>
>>> I think it would just work as is you replace std::string with const char *
>>> in Lancelot's snippet above.  FWIW, I had the same thought as Lancelot
>>> about using variable templates instead of varargs.
>>>
>>> OOC, is there any place in GDB that wants to pass more than two components
>>> to path_join?  I skimmed the patch and didn't notice one.
>>
>> I am not aware of one, I'd be happy to make path_join just have two
>> parameters and be done with it.
> 
> Actually, I have one use for it in my upcoming DWARF reader changes,
> where I join the CU's compilation directory, the directory (from the
> line header directory table) and the file name.
> 
> I like Tom's proposal, here:
> 
>   https://sourceware.org/pipermail/gdb-patches/2022-April/188016.html
> 
> It seems efficient (no extraneous string copies) and type-safe.  I
> successfully changed my patch to use it.

Lancelot's approach seems more straightforward (trivially changed to use string_view) and
would also work, and it avoids creating the temporary array, at the expense of being
recursive when you have more than two components to join.  Either approach seems
good to me.  In Tom's approach, I think you could use gdb::array_view in concat.

Re: [PATCH 5/5] gdbsupport: add path_join function

[Lancelot SIX]

Hi,

> > Is this different semantics made on purpose?
> 
> I indeed noticed how os.path.join handled ("/foo", "/bar").
> 
> I went with the behavior in my patch mostly because there are cases in
> GDB where we really need to append absolute paths to another path.  For
> example, looking up files in sysroots.  If the sysroot is set to
> "/the/sysroot/" (with or without the trailing slash) and we want to load
> the target library /usr/lib/libfoo.so, it would be nice if
> 
>     path_join (sysroot_path, lib_path)
> 
> gave
> 
>     /the/sysroot/usr/lib/libfoo.so
> 
> without a double-slash.  Same when we are looking up files in the
> debug-file-directory (/usr/lib/debug).
> 
> When changing those implementations that currently use concat(...) or
> string_printf("%s/%s"), we would need to be careful: if the right hand
> side is an absolute path, then we would have to skip the leading
> directory separator before passing it to path_join, to get the desired
> result.  And then we're back at having some special path handling
> details everywhere.

Fair enough.  This is a good enough reason to have a special semantics
(we could have a path_join and a  sysroot_path_join, but not sure it is
worth it having 2 functions).

> If you can make it work using gdb::string_view or const char *, I think
> it would be ok.  I'll give it a try.  But otherwise, I am personally
> fine with the sentinel nullptr, given that the compiler gives you a
> warning if you forget it.
> 

Something like this avoids copying some std::string around (which my
initial implementation did), and can work with a string view (would be
the same architecture with const char*).  The compiler should optimize
almost all the function calls away easily enough.  I saw that Tom gave
another approach which would work fine as well.

    namespace impl {
    
    template<typename ...Args>
    inline void
    path_join_1 (std::string &l, gdb::string_view r, Args... comps)
    {
      path_join_1 (l, r);
      path_join_1 (l, ...comps);
    }
    
    template<>
    inline void
    path_join_1 (std::string &l, gdb::string_view r)
    {
      /* The real logic here  */
    }
    
    } /* namespace impl  */
    
    template<typename ...Args>
    inline std::string
    path_join (Args... comps)
    {
      std::string res;
      impl::path_join_1 (res, comps...);
      return res;
    }

Best,
Lancelot.

Re: [PATCH 5/5] gdbsupport: add path_join function

[Simon Marchi]

> Lancelot's approach seems more straightforward (trivially changed to use string_view) and
> would also work, and it avoids creating the temporary array, at the expense of being
> recursive when you have more than two components to join.  Either approach seems
> good to me.  In Tom's approach, I think you could use gdb::array_view in concat.

The array is stack-allocated, so it should not incur any significant
cost.  I'll stop looking for alternative ways, because at this point it
really looks like bike-shedding, and I need to make progress :).

I indeed switched to gdb::array_view in "concat", since that allowed me
to use a range for.  I also renamed concat to path_join (overloading my
existing entry point).  If some caller has paths in an array-like data
structure (I don't know of any caller that does today), they'll be able
to call the gdb::array_view version directly.

Simon

Re: [PATCH 5/5] gdbsupport: add path_join function

[Simon Marchi]

>>> Is this different semantics made on purpose?
>>
>> I indeed noticed how os.path.join handled ("/foo", "/bar").
>>
>> I went with the behavior in my patch mostly because there are cases in
>> GDB where we really need to append absolute paths to another path.  For
>> example, looking up files in sysroots.  If the sysroot is set to
>> "/the/sysroot/" (with or without the trailing slash) and we want to load
>> the target library /usr/lib/libfoo.so, it would be nice if
>>
>>     path_join (sysroot_path, lib_path)
>>
>> gave
>>
>>     /the/sysroot/usr/lib/libfoo.so
>>
>> without a double-slash.  Same when we are looking up files in the
>> debug-file-directory (/usr/lib/debug).
>>
>> When changing those implementations that currently use concat(...) or
>> string_printf("%s/%s"), we would need to be careful: if the right hand
>> side is an absolute path, then we would have to skip the leading
>> directory separator before passing it to path_join, to get the desired
>> result.  And then we're back at having some special path handling
>> details everywhere.
> 
> Fair enough.  This is a good enough reason to have a special semantics
> (we could have a path_join and a  sysroot_path_join, but not sure it is
> worth it having 2 functions).

Well, I've been thinking about it, and perhaps that appending an
absolute path to another path (either absolute or non-absolute),
sysroot-style, is indeed a distinct use case that a regular path_join is
not meant to solve.  The code that Pedro pointed to, in solib_find_1,
convinced me.  It tries different things when dealing with a Windows
paths that contains a drive letter.

And compatibility-wise, being a drop-in replacement for
std::filesystem::operator/ is interesting.

> 
>> If you can make it work using gdb::string_view or const char *, I think
>> it would be ok.  I'll give it a try.  But otherwise, I am personally
>> fine with the sentinel nullptr, given that the compiler gives you a
>> warning if you forget it.
>>
> 
> Something like this avoids copying some std::string around (which my
> initial implementation did), and can work with a string view (would be
> the same architecture with const char*).  The compiler should optimize
> almost all the function calls away easily enough.  I saw that Tom gave
> another approach which would work fine as well.
> 
>     namespace impl {
>     
>     template<typename ...Args>
>     inline void
>     path_join_1 (std::string &l, gdb::string_view r, Args... comps)
>     {
>       path_join_1 (l, r);
>       path_join_1 (l, ...comps);
>     }
>     
>     template<>
>     inline void
>     path_join_1 (std::string &l, gdb::string_view r)
>     {
>       /* The real logic here  */
>     }
>     
>     } /* namespace impl  */
>     
>     template<typename ...Args>
>     inline std::string
>     path_join (Args... comps)
>     {
>       std::string res;
>       impl::path_join_1 (res, comps...);
>       return res;
>     }

Thanks for the suggestion, I ended up using Tom's proposal.

Simon