From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from lndn.lancelotsix.com (vps-42846194.vps.ovh.net [IPv6:2001:41d0:801:2000::2400]) by sourceware.org (Postfix) with ESMTPS id AD7DF3858C2C for ; Fri, 15 Apr 2022 16:55:23 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org AD7DF3858C2C Received: from ubuntu.lan (unknown [IPv6:2a02:390:9086::635]) by lndn.lancelotsix.com (Postfix) with ESMTPSA id D45A4818C2; Fri, 15 Apr 2022 16:55:22 +0000 (UTC) Date: Fri, 15 Apr 2022 16:55:18 +0000 From: Lancelot SIX To: Simon Marchi , Simon Marchi , gdb-patches@sourceware.org Subject: Re: [PATCH 5/5] gdbsupport: add path_join function Message-ID: <20220415164959.w2vfbvkrolhj36bk@ubuntu.lan> References: <20220414200137.3479373-1-simon.marchi@polymtl.ca> <20220414200137.3479373-5-simon.marchi@polymtl.ca> <20220415143827.t2nlcfhmh2pondev@ubuntu.lan> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20220415143827.t2nlcfhmh2pondev@ubuntu.lan> X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.5.11 (lndn.lancelotsix.com [0.0.0.0]); Fri, 15 Apr 2022 16:55:23 +0000 (UTC) X-Spam-Status: No, score=-11.6 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, KAM_SHORT, SPF_HELO_NONE, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 15 Apr 2022 16:55:26 -0000 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 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 *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 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 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 *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)) { @@ -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 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 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 . */ + +#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 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 +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