From ffaebc199ed7b1f7e539c938e8b234d2a4ad9df9 Mon Sep 17 00:00:00 2001 From: Simon Marchi Date: Wed, 20 Apr 2022 17:03:25 -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). I initially wanted to make it possible to join two absolute paths, to support the use case of prepending a sysroot path to a target file path, or the prepending the debug-file-directory to a target file path. But the code in solib_find_1 shows that it is more complex than this anyway (for example, when the right hand side is a Windows path with a drive letter). So I don't think we need to support that case in path_join. That also keeps the implementation simpler. Change a few spots to use path_join to show how it can be used. I believe that all the spots I changed are guarded by some checks that ensure the right hand side operand is not an absolute path. Regression-tested on Ubuntu 18.04. Built-tested on Windows, and I also ran the new unit-test there. [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 | 73 +++++++++++++++++++++++++++++ gdbsupport/pathstuff.cc | 45 ++++++++++++------ gdbsupport/pathstuff.h | 23 +++++++++ 8 files changed, 153 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 927074b7669..586c0920468 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 bb72e0e4791..8586463f550 100644 --- a/gdb/dwarf2/read.c +++ b/gdb/dwarf2/read.c @@ -1280,7 +1280,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 *name_holder); + std::string &name_holder); static htab_up allocate_signatured_type_table (); @@ -2780,9 +2780,9 @@ dw2_get_file_names_reader (const struct die_reader_specs *reader, { for (const auto &entry : lh->file_names ()) { - gdb::unique_xmalloc_ptr 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); @@ -11098,14 +11098,13 @@ open_dwo_file (dwarf2_per_objfile *per_objfile, if (comp_dir != NULL) { - gdb::unique_xmalloc_ptr 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; } @@ -19697,14 +19696,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 *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 hold_compare; + std::string hold_compare; if (!IS_ABSOLUTE_PATH (include_name) && (dir_name != nullptr || cu_info.get_comp_dir () != nullptr)) { @@ -19731,27 +19730,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 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) @@ -20486,7 +20482,7 @@ static void dwarf2_start_subfile (struct dwarf2_cu *cu, const char *filename, const char *dirname) { - gdb::unique_xmalloc_ptr copy; + std::string copy; /* In order not to lose the line information directory, we concatenate it to the filename when it makes sense. @@ -20497,8 +20493,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..d074cda3276 --- /dev/null +++ b/gdb/unittests/path-join-selftests.c @@ -0,0 +1,73 @@ +/* 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 . */ + +#include "defs.h" +#include "gdbsupport/pathstuff.h" +#include "gdbsupport/selftest.h" + +namespace selftests { +namespace path_join { + +template +static void +test_one (const char *expected, Args... paths) +{ + std::string actual = ::path_join (paths...); + + SELF_CHECK (actual == expected); +} + +/* Test path_join. */ + +static void +test () +{ + test_one ("/foo/bar", "/foo", "bar"); + test_one ("/bar", "/", "bar"); + test_one ("foo/bar/", "foo", "bar", ""); + test_one ("foo", "", "foo"); + test_one ("foo/bar", "foo", "", "bar"); + test_one ("foo/", "foo", ""); + test_one ("foo/", "foo/", ""); + + test_one ("D:/foo/bar", "D:/foo", "bar"); + test_one ("D:/foo/bar", "D:/foo/", "bar"); + +#if defined(_WIN32) + /* The current implementation doesn't recognize backslashes as directory + separators on Unix-like systems, so only run these tests on Windows. If + we ever switch our implementation to use std::filesystem::path, they + should work anywhere, in theory. */ + test_one ("D:\\foo/bar", "D:\\foo", "bar"); + test_one ("D:\\foo\\bar", "D:\\foo\\", "bar"); + test_one ("\\\\server\\dir\\file", "\\\\server\\dir\\", "file"); + test_one ("\\\\server\\dir/file", "\\\\server\\dir", "file"); +#endif /* _WIN32 */ +} + +} +} + +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..5b5a8eea904 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 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,29 @@ child_path (const char *parent, const char *child) /* See gdbsupport/pathstuff.h. */ +std::string +path_join (gdb::array_view paths) +{ + std::string ret; + + for (int i = 0; i < paths.size (); ++i) + { + const gdb::string_view path = paths[i]; + + if (i > 0) + gdb_assert (!IS_ABSOLUTE_PATH (path)); + + if (!ret.empty () && !IS_DIR_SEPARATOR (ret.back ())) + ret += '/'; + + ret.append (path.begin (), path.end ()); + } + + return ret; +} + +/* See gdbsupport/pathstuff.h. */ + bool contains_dir_separator (const char *path) { @@ -227,7 +242,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 +251,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 +260,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 +309,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 +318,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..c8592793886 100644 --- a/gdbsupport/pathstuff.h +++ b/gdbsupport/pathstuff.h @@ -21,6 +21,7 @@ #define COMMON_PATHSTUFF_H #include "gdbsupport/byte-vector.h" +#include "gdbsupport/array-view.h" #include #include @@ -60,6 +61,28 @@ extern std::string gdb_abspath (const char *path); extern const char *child_path (const char *parent, const char *child); +/* Join elements in PATHS into a single path. + + The first element can be absolute or relative. All the others must be + relative. */ + +extern std::string path_join (gdb::array_view paths); + +/* Same as the above, but accept paths as distinct parameters. */ + +template +std::string +path_join (Args... paths) +{ + /* It doesn't make sense to join less than two paths. */ + gdb_static_assert (sizeof... (Args) >= 2); + + std::array views + { gdb::string_view (paths)... }; + + return path_join (gdb::array_view (views)); +} + /* Return whether PATH contains a directory separator character. */ extern bool contains_dir_separator (const char *path); -- 2.30.2