From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp.polymtl.ca (smtp.polymtl.ca [132.207.4.11]) by sourceware.org (Postfix) with ESMTPS id A4A0C3858D1E for ; Mon, 18 Apr 2022 18:11:43 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org A4A0C3858D1E Received: from simark.ca (simark.ca [158.69.221.121]) (authenticated bits=0) by smtp.polymtl.ca (8.14.7/8.14.7) with ESMTP id 23IIBYA3009832 (version=TLSv1/SSLv3 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Mon, 18 Apr 2022 14:11:39 -0400 DKIM-Filter: OpenDKIM Filter v2.11.0 smtp.polymtl.ca 23IIBYA3009832 Received: from [10.0.0.11] (192-222-157-6.qc.cable.ebox.net [192.222.157.6]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id A4B651EDF0; Mon, 18 Apr 2022 14:11:34 -0400 (EDT) Message-ID: Date: Mon, 18 Apr 2022 14:11:34 -0400 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.7.0 Subject: Re: [PATCH 5/5] gdbsupport: add path_join function Content-Language: en-US To: Eli Zaretskii Cc: gdb-patches@sourceware.org References: <20220414200137.3479373-1-simon.marchi@polymtl.ca> <20220414200137.3479373-5-simon.marchi@polymtl.ca> <83h76u3n04.fsf@gnu.org> From: Simon Marchi In-Reply-To: <83h76u3n04.fsf@gnu.org> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Poly-FromMTA: (simark.ca [158.69.221.121]) at Mon, 18 Apr 2022 18:11:34 +0000 X-Spam-Status: No, score=-3034.3 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, NICE_REPLY_A, RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL, SPF_HELO_PASS, 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: Mon, 18 Apr 2022 18:11:45 -0000 On 2022-04-15 01:59, Eli Zaretskii wrote: >> Date: Thu, 14 Apr 2022 16:01:37 -0400 >> From: Simon Marchi via Gdb-patches >> Cc: Simon Marchi >> >> From: Simon Marchi >> >> 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