Message ID | 20240303164756.870514-1-lancelot.six@amd.com |
---|---|
State | New |
Headers |
Return-Path: <gdb-patches-bounces+patchwork=sourceware.org@sourceware.org> X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id AAE5F3858C33 for <patchwork@sourceware.org>; Sun, 3 Mar 2024 16:48:42 +0000 (GMT) X-Original-To: gdb-patches@sourceware.org Delivered-To: gdb-patches@sourceware.org Received: from NAM10-BN7-obe.outbound.protection.outlook.com (mail-bn7nam10on2059.outbound.protection.outlook.com [40.107.92.59]) by sourceware.org (Postfix) with ESMTPS id 8D68A3858C55 for <gdb-patches@sourceware.org>; Sun, 3 Mar 2024 16:48:20 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 8D68A3858C55 Authentication-Results: sourceware.org; dmarc=fail (p=quarantine dis=none) header.from=amd.com Authentication-Results: sourceware.org; spf=fail smtp.mailfrom=amd.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 8D68A3858C55 Authentication-Results: server2.sourceware.org; arc=pass smtp.remote-ip=40.107.92.59 ARC-Seal: i=2; a=rsa-sha256; d=sourceware.org; s=key; t=1709484503; cv=pass; b=NKP51K1HtO/qyXkNG3Sh77PoHUfFADTbzo///N86FqZ6klTU9MYB1Vcj0aS2bTq+2NYKYq0wbhg/+8h/IEAEJj/AuBwdFNqdNkTwe5GjTNgW21u8khWrndg0ogkJEYojh72h+hVI24JXhKQRAxPzITOpEEPVj9DtSwjKO9NuB38= ARC-Message-Signature: i=2; a=rsa-sha256; d=sourceware.org; s=key; t=1709484503; c=relaxed/simple; bh=9FebN6QvslpoALks5K9LKSHmyui3KN4omUqAb12KrpY=; h=DKIM-Signature:From:To:Subject:Date:Message-ID:MIME-Version; b=CH2L98NUcHejROB+Qt1BDEVwczjKxANh98sT5KufOPPmVA2BOPFqkGI/twbGo+u+Luz5WKC984qgs+p1ezJJws3uyJ2gomspLdA1QYvicShFw8/F4A3qxgvkB9qZMbSQ/4Dxl/Q04BBv8sekTnwJUv4vZJgdlTmZ1eHOHJqAJG4= ARC-Authentication-Results: i=2; server2.sourceware.org ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=FH1hvQx7HfGx8GaLdRjINig0cWXQI7XmvR/4rExX89l8d/yfjt82gLfspAwR5w8Ic+LLLLUO5xNc4isHnKF+8I0+dRsSBitB8Wg4qPEAGm6mIcTRhzVD1lysJANvt2eE0d1nbPqpsAk7InZzp9JViZzvPvuow7k56ButiC8v0Y4uMGWxt08FklwTkAkgZYfYqRRzXRUe6kMZRcUDPs2mfaaemkoeuqrCw6GngHtXnk9bN83jmQe2C1j2BkpPb8rg8i4XP9m3mcrdwG2vk03D1dAE4Si+6SluqlcRing0qak5aQcVzaVVFBVFSDL04Dj7q41/jZssZQ3HVpcAvsZN6g== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=djP4X3zGqauzRsjONyMd5OmVBlRQcwRrxUL0BUAAL8E=; b=aCrFs3wqr+rvtdGUXXggfi8p7vZwfozekVnTf5cJjaNwssRiT/j/kW/GtTg7JzVtf32lT+GvSms7RzmR/IoPIHtYsj91QGecsGQvJWqcdb3mEixxvhtjyQGoTxCbiVcA/Gne/O9wNzT9J8YK1KY4j4kcY0fAtqHCzg92qyhpGSq6O8kNjK7lHRVHZPmqG1SMvLGZyvh29e6lZJrzDRZf6KFZs3+eqN5fchzma+vECjrY7AHYLCKpDwZY5F9RNyiuNbI6fmb95VpSDC//YtDxvzvuH5Nudwl/uEVn8xoQ5WGsYY2ZNQrHXlVKXkL0ICuBxPAzZywCd/67MF7fDIx8Tw== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass (sender ip is 165.204.84.17) smtp.rcpttodomain=sourceware.org smtp.mailfrom=amd.com; dmarc=pass (p=quarantine sp=quarantine pct=100) action=none header.from=amd.com; dkim=none (message not signed); arc=none (0) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=amd.com; s=selector1; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=djP4X3zGqauzRsjONyMd5OmVBlRQcwRrxUL0BUAAL8E=; b=d9j6kZXcP8Qu/U6pxh7vqF83TU6WjIHByL876wuTSzYFTwnSCwglA3a7Q2cQXjYt8HrY85scEmjOu/+Rv3wajH3U5IZzzbcBXUQJ1JYF8D9nfGiMG+Pf0V5xhkD7h+AVewrMSY+lu4ZSGvc4KX6P19iKZq92UTd4X8FGQqfSBNc= Received: from DM6PR01CA0007.prod.exchangelabs.com (2603:10b6:5:296::12) by PH7PR12MB8014.namprd12.prod.outlook.com (2603:10b6:510:27c::19) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.7339.38; Sun, 3 Mar 2024 16:48:16 +0000 Received: from DS2PEPF0000343B.namprd02.prod.outlook.com (2603:10b6:5:296:cafe::a0) by DM6PR01CA0007.outlook.office365.com (2603:10b6:5:296::12) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.7339.38 via Frontend Transport; Sun, 3 Mar 2024 16:48:15 +0000 X-MS-Exchange-Authentication-Results: spf=pass (sender IP is 165.204.84.17) smtp.mailfrom=amd.com; dkim=none (message not signed) header.d=none;dmarc=pass action=none header.from=amd.com; Received-SPF: Pass (protection.outlook.com: domain of amd.com designates 165.204.84.17 as permitted sender) receiver=protection.outlook.com; client-ip=165.204.84.17; helo=SATLEXMB04.amd.com; pr=C Received: from SATLEXMB04.amd.com (165.204.84.17) by DS2PEPF0000343B.mail.protection.outlook.com (10.167.18.38) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.20.7362.11 via Frontend Transport; Sun, 3 Mar 2024 16:48:15 +0000 Received: from khazad-dum.amd.com (10.180.168.240) by SATLEXMB04.amd.com (10.181.40.145) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2507.35; Sun, 3 Mar 2024 10:48:14 -0600 From: Lancelot SIX <lancelot.six@amd.com> To: <gdb-patches@sourceware.org> CC: <lsix@lancelotsix.com>, Lancelot SIX <lancelot.six@amd.com> Subject: [PATCH] gdb/compile: Use std::filesystem::remove_all in cleanup Date: Sun, 3 Mar 2024 16:47:56 +0000 Message-ID: <20240303164756.870514-1-lancelot.six@amd.com> X-Mailer: git-send-email 2.34.1 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Content-Type: text/plain X-Originating-IP: [10.180.168.240] X-ClientProxiedBy: SATLEXMB03.amd.com (10.181.40.144) To SATLEXMB04.amd.com (10.181.40.145) X-EOPAttributedMessage: 0 X-MS-PublicTrafficType: Email X-MS-TrafficTypeDiagnostic: DS2PEPF0000343B:EE_|PH7PR12MB8014:EE_ X-MS-Office365-Filtering-Correlation-Id: 08e2570d-961d-45ed-d4ec-08dc3ba1b8c3 X-MS-Exchange-SenderADCheck: 1 X-MS-Exchange-AntiSpam-Relay: 0 X-Microsoft-Antispam: BCL:0; X-Microsoft-Antispam-Message-Info: TRuwhtBLNqydLXtfAT8StubAj7LwE6HfUn6mltzVhnoqF+8zKGIfM+AvkhvYlV4/rzJFSl2chZr0YD9AXQgeUOdqZk0TRGVBWB8sE6ilyWwv7bLY1VLefhE6Py+qOBFyp9aW9OnxikOPmWxH/yr/TFQATlaqSo5Wlj1deK+EAb//xo5BaEJ04HjivNifV+iz4J4d0KbmimP92roH8oNIJXAKCprXJC26IEz1UrWl88LywAXgDnWlF4tSShj9U59l8STzyBv6nFmJ3HJr0+QKB5Jy/i9y+Iv+uaIQRZaDUDahA7UaVZPoZyr/Ynwfc1g4abl6XiflKEMzmwf0tKS/wYiNQPJBPaZO1FfeMQgIazLpmwHh92H/MiLuk9BxG1m6igYASMlV+8JDPxPXDM9XTieJLdEIDTh+l0xgQcwoG6fNXRiXbzUfJCrHFPmH5QJpPZsvm6KRcvBXC+jupsJtzMtO8rnHLZJ0PFi36fFhvz7g/H9jkn8EU56sBZykFAGR1io9hTMBmEwnsp0coWJq7c+X99DtSy2F+uGDLgXgozsTFR/3yhLa71g8YaG8naLlkRlO96KIcmJL8a6gj/n8L7J9P7BarVSwshHq9IAkfPsj1KRJmM1cHZwmvGH0Ec6/Uiy5rNv4xuZZJBRxKP2qI5oZvWZO3Ku77S6IW/zq24F3qtSR0eYzDSRW72X6XdzfyO3RsT+u6cDzS6XXDo/1yw== X-Forefront-Antispam-Report: CIP:165.204.84.17; CTRY:US; LANG:en; SCL:1; SRV:; IPV:CAL; SFV:NSPM; H:SATLEXMB04.amd.com; PTR:InfoDomainNonexistent; CAT:NONE; SFS:(13230031)(376005)(36860700004)(82310400014); DIR:OUT; SFP:1101; X-OriginatorOrg: amd.com X-MS-Exchange-CrossTenant-OriginalArrivalTime: 03 Mar 2024 16:48:15.4601 (UTC) X-MS-Exchange-CrossTenant-Network-Message-Id: 08e2570d-961d-45ed-d4ec-08dc3ba1b8c3 X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d X-MS-Exchange-CrossTenant-OriginalAttributedTenantConnectingIp: TenantId=3dd8961f-e488-4e60-8e11-a82d994e183d; Ip=[165.204.84.17]; Helo=[SATLEXMB04.amd.com] X-MS-Exchange-CrossTenant-AuthSource: DS2PEPF0000343B.namprd02.prod.outlook.com X-MS-Exchange-CrossTenant-AuthAs: Anonymous X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH7PR12MB8014 X-Spam-Status: No, score=-11.1 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FORGED_SPF_HELO, GIT_PATCH_0, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H2, SPF_HELO_PASS, SPF_NONE, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.30 Precedence: list List-Id: Gdb-patches mailing list <gdb-patches.sourceware.org> List-Unsubscribe: <https://sourceware.org/mailman/options/gdb-patches>, <mailto:gdb-patches-request@sourceware.org?subject=unsubscribe> List-Archive: <https://sourceware.org/pipermail/gdb-patches/> List-Post: <mailto:gdb-patches@sourceware.org> List-Help: <mailto:gdb-patches-request@sourceware.org?subject=help> List-Subscribe: <https://sourceware.org/mailman/listinfo/gdb-patches>, <mailto:gdb-patches-request@sourceware.org?subject=subscribe> Errors-To: gdb-patches-bounces+patchwork=sourceware.org@sourceware.org |
Series |
gdb/compile: Use std::filesystem::remove_all in cleanup
|
|
Checks
Context | Check | Description |
---|---|---|
linaro-tcwg-bot/tcwg_gdb_build--master-aarch64 | success | Testing passed |
linaro-tcwg-bot/tcwg_gdb_build--master-arm | success | Testing passed |
linaro-tcwg-bot/tcwg_gdb_check--master-arm | success | Testing passed |
linaro-tcwg-bot/tcwg_gdb_check--master-aarch64 | success | Testing passed |
Commit Message
Lancelot SIX
March 3, 2024, 4:47 p.m. UTC
In a previous review, I noticed that some code in gdb/compile/compile.c could use c++17's `std::filesystem::remove_all` instead of using some `system ("rm -rf ...");`. This patch implements this. Note that I use the noexcept overload of std::filesystem::remove_all and explicitly check for an error code. This means that this code called during the cleanup procedure cannot throw, and does not risk preventing other cleanup functions to be called. Tested on x86_64-linux. Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31420 Change-Id: If5668bf3e15e66c020e5c3b4fa999f861690e4cf --- gdb/compile/compile.c | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) base-commit: 90f8d97c8efa75f7f019b868eca9c626bc35203d
Comments
>>>>> "Lancelot" == Lancelot SIX <lancelot.six@amd.com> writes:
Lancelot> In a previous review, I noticed that some code in gdb/compile/compile.c
Lancelot> could use c++17's `std::filesystem::remove_all` instead of using some
Lancelot> `system ("rm -rf ...");`.
Lancelot> This patch implements this.
Lancelot> Note that I use the noexcept overload of std::filesystem::remove_all and
Lancelot> explicitly check for an error code. This means that this code called
Lancelot> during the cleanup procedure cannot throw, and does not risk preventing
Lancelot> other cleanup functions to be called.
Lancelot> Tested on x86_64-linux.
Lancelot> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31420
Lancelot> Change-Id: If5668bf3e15e66c020e5c3b4fa999f861690e4cf
Thank you. I think this is ok.
There might be some risk that there's a compiler that implements C++17
but not std::filesystem. If this happens I suppose we can either
consider simply rejecting that compiler, or alternatively, revert this
patch.
Approved-By: Tom Tromey <tom@tromey.com>
Tom
> Thank you. I think this is ok. > > There might be some risk that there's a compiler that implements C++17 > but not std::filesystem. If this happens I suppose we can either > consider simply rejecting that compiler, or alternatively, revert this > patch. > > Approved-By: Tom Tromey <tom@tromey.com> > > Tom I have just pushed this patch. If it turns out that this causes any issue for some configuration, I'll revert the patch. Best, Lancelot.
On 3/8/24 17:27, Tom Tromey wrote: >>>>>> "Lancelot" == Lancelot SIX <lancelot.six@amd.com> writes: > > Lancelot> In a previous review, I noticed that some code in gdb/compile/compile.c > Lancelot> could use c++17's `std::filesystem::remove_all` instead of using some > Lancelot> `system ("rm -rf ...");`. > > Lancelot> This patch implements this. > > Lancelot> Note that I use the noexcept overload of std::filesystem::remove_all and > Lancelot> explicitly check for an error code. This means that this code called > Lancelot> during the cleanup procedure cannot throw, and does not risk preventing > Lancelot> other cleanup functions to be called. > > Lancelot> Tested on x86_64-linux. > > Lancelot> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31420 > Lancelot> Change-Id: If5668bf3e15e66c020e5c3b4fa999f861690e4cf > > Thank you. I think this is ok. > > There might be some risk that there's a compiler that implements C++17 > but not std::filesystem. If this happens I suppose we can either > consider simply rejecting that compiler, or alternatively, revert this > patch. This broke the build for me with gcc 7.5.0, which apparently falls into the category of "mostly implements C++17 but not std::filesystem". As we can see here ( https://gcc.gnu.org/gcc-8/changes.html#cxx ) support was added in gcc 8. Thanks, - Tom
> > This broke the build for me with gcc 7.5.0, which apparently falls into > the category of "mostly implements C++17 but not std::filesystem". > Sorry about that, I have sent https://sourceware.org/pipermail/gdb-patches/2024-April/207760.html to revert that change. At least, we know it is best to stay away from std::filesystem for now. Best, Lancelot.
On 4/3/24 16:47, Lancelot SIX wrote: >> >> This broke the build for me with gcc 7.5.0, which apparently falls into >> the category of "mostly implements C++17 but not std::filesystem". >> > > Sorry about that, I have sent > https://sourceware.org/pipermail/gdb-patches/2024-April/207760.html to > revert that change. At least, we know it is best to stay away from > std::filesystem for now. > I wonder if there is a case for conditionally enabling this. That would ensure that the work is not lost, as well as used when using gcc >=8. Thanks, - Tom
On Wed, Apr 3, 2024 at 7:51 AM Tom de Vries <tdevries@suse.de> wrote: > > On 4/3/24 16:47, Lancelot SIX wrote: > >> > >> This broke the build for me with gcc 7.5.0, which apparently falls into > >> the category of "mostly implements C++17 but not std::filesystem". > >> > > > > Sorry about that, I have sent > > https://sourceware.org/pipermail/gdb-patches/2024-April/207760.html to > > revert that change. At least, we know it is best to stay away from > > std::filesystem for now. > > > > I wonder if there is a case for conditionally enabling this. That would > ensure that the work is not lost, as well as used when using gcc >=8. Let me add even though support was added for GCC 8, the functions are in included in libstdc++fs.a rather than the standard libstdc++.so/libstdc++.a. It was added to libstdc++ in GCC 9. So if you add this conditionally, you either need to do a feature test that does a link too or have one that adds libstdc++fs.a if not found in the standard library. Thanks, Andrew > > Thanks, > - Tom > >
>> I wonder if there is a case for conditionally enabling this. That would >> ensure that the work is not lost, as well as used when using gcc >=8. > > Let me add even though support was added for GCC 8, the functions are > in included in libstdc++fs.a rather than the standard > libstdc++.so/libstdc++.a. > It was added to libstdc++ in GCC 9. So if you add this conditionally, > you either need to do a feature test that does a link too or have one > that adds libstdc++fs.a if not found in the standard library. > > Thanks, > Andrew > Thanks for the input. We did discuss briefly using "-libstdc++fs" on IRC, thanks for the precision regarding where it is required. As gcc-7.5 seems to still be used to build gdb, it looks like we will stay away from std::filesystem for now. Thanks, Lancelot.
diff --git a/gdb/compile/compile.c b/gdb/compile/compile.c index 27cff2553ee..45452f19abb 100644 --- a/gdb/compile/compile.c +++ b/gdb/compile/compile.c @@ -40,7 +40,9 @@ #include "osabi.h" #include "gdbsupport/gdb_wait.h" #include "valprint.h" +#include <filesystem> #include <optional> +#include <system_error> #include "gdbsupport/gdb_unlinker.h" #include "gdbsupport/pathstuff.h" #include "gdbsupport/scoped_ignore_signal.h" @@ -450,15 +452,11 @@ get_compile_file_tempdir (void) tempdir_name = xstrdup (tempdir_name); add_final_cleanup ([] () { - char *zap; - int wstat; - - gdb_assert (startswith (tempdir_name, TMP_PREFIX)); - zap = concat ("rm -rf ", tempdir_name, (char *) NULL); - wstat = system (zap); - if (wstat == -1 || !WIFEXITED (wstat) || WEXITSTATUS (wstat) != 0) - warning (_("Could not remove temporary directory %s"), tempdir_name); - XDELETEVEC (zap); + std::error_code error; + if (std::filesystem::remove_all (tempdir_name, error) + == static_cast<std::uintmax_t> (-1)) + warning (_("Could not remove temporary directory %s (%s)"), + tempdir_name, error.message ().c_str ()); }); return tempdir_name; }