From: Simon Marchi <simon.marchi@polymtl.ca>
To: gdb-patches@sourceware.org
Cc: Simon Marchi <simon.marchi@efficios.com>
Subject: [PATCH 5/5] gdbsupport: add path_join function
Date: Thu, 14 Apr 2022 16:01:37 -0400 [thread overview]
Message-ID: <20220414200137.3479373-5-simon.marchi@polymtl.ca> (raw)
In-Reply-To: <20220414200137.3479373-1-simon.marchi@polymtl.ca>
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 +-
| 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
--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
next prev parent reply other threads:[~2022-04-14 20:01 UTC|newest]
Thread overview: 28+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-04-14 20:01 [PATCH 1/5] gdb: call gdb_tilde_expand instead of gdb_tilde_expand_up in source_script_with_search Simon Marchi
2022-04-14 20:01 ` [PATCH 2/5] gdbsupport: make gdb_abspath return an std::string Simon Marchi
2022-04-18 19:41 ` Tom Tromey
2022-04-18 20:09 ` Pedro Alves
2022-04-18 20:11 ` Simon Marchi
2022-04-14 20:01 ` [PATCH 3/5] gdbsupport: make gdb_realpath_keepfile " Simon Marchi
2022-04-18 19:44 ` Tom Tromey
2022-04-14 20:01 ` [PATCH 4/5] gdb: use gdb_tilde_expand instead of gdb_tilde_expand_up in source_script_with_search Simon Marchi
2022-04-18 19:44 ` Tom Tromey
2022-04-18 20:12 ` Simon Marchi
2022-04-14 20:01 ` Simon Marchi [this message]
2022-04-15 5:59 ` [PATCH 5/5] gdbsupport: add path_join function Eli Zaretskii
2022-04-18 18:11 ` Simon Marchi
2022-04-18 18:30 ` Eli Zaretskii
2022-04-18 19:24 ` Pedro Alves
2022-04-15 14:38 ` Lancelot SIX
2022-04-15 16:55 ` Lancelot SIX
2022-04-18 18:43 ` Simon Marchi
2022-04-18 19:09 ` Pedro Alves
2022-04-18 19:12 ` Simon Marchi
2022-04-18 20:55 ` Simon Marchi
2022-04-18 21:07 ` Pedro Alves
2022-04-19 0:19 ` Simon Marchi
2022-04-18 19:22 ` Pedro Alves
2022-04-18 20:01 ` Tom Tromey
2022-04-18 23:11 ` Lancelot SIX
2022-04-20 0:22 ` Simon Marchi
2022-04-18 19:36 ` [PATCH 1/5] gdb: call gdb_tilde_expand instead of gdb_tilde_expand_up in source_script_with_search Tom Tromey
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20220414200137.3479373-5-simon.marchi@polymtl.ca \
--to=simon.marchi@polymtl.ca \
--cc=gdb-patches@sourceware.org \
--cc=simon.marchi@efficios.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).