From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from lndn.lancelotsix.com (lndn.lancelotsix.com [51.195.220.111]) by sourceware.org (Postfix) with ESMTPS id 165863858C36 for ; Tue, 27 Feb 2024 14:03:37 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 165863858C36 Authentication-Results: sourceware.org; dmarc=pass (p=reject dis=none) header.from=lancelotsix.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=lancelotsix.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 165863858C36 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=51.195.220.111 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1709042625; cv=none; b=mQhW13QsmXCX8kQTveCGxP0KueZr9ZRqv5R28qfybJpl5mOLiST19AI/bplAGuf1LCt2isbCRW8MQLT/1SJ0g9jR8rNsUWDBq1+DNhL31oTX+NmZadvOEd+inF0eMmj288WUIG9Pfwts0dPNmUVdfiaXCW5M/DIB9yzVspLJoRo= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1709042625; c=relaxed/simple; bh=lgy6Nsu5JUCUsPi4/R/I1AInIpG3GSAtANxNEZcrFYQ=; h=DKIM-Signature:Date:From:To:Subject:Message-ID:MIME-Version; b=OgO5DpmACw6wPGfmGpV7zMQqQG0mm21BORTqRD6YlfDJjem73wMXisVjbVsEFEqhBaQI2eqdYFagxxjGha9HzwCfdVH4pqmUg/YmsRnj8jDjQs2k3MEUHQ5kS/20mSSV8PWUi6HuTyjfUrpmjoWrBhbbYDV/+hBpfjd0xf8IfaI= ARC-Authentication-Results: i=1; server2.sourceware.org Received: from octopus (cust120-dsl54.idnet.net [212.69.54.120]) by lndn.lancelotsix.com (Postfix) with ESMTPSA id DD3B88F00E; Tue, 27 Feb 2024 14:03:35 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=lancelotsix.com; s=2021; t=1709042615; bh=lgy6Nsu5JUCUsPi4/R/I1AInIpG3GSAtANxNEZcrFYQ=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=0Me3b5ACTetaUw1secZw4e7PF/4a67BXTsSfwaNZn0pUB/JwGk/hQypsDNDiukYL/ RW6OLVU6e3VJr1hEJYi6uuqcmO54SbVELigNyrdt0IqHMblpE74+ZqKZXL5C3KJ4+H agsRwc2pclOGX08tb0iAkk2hdWshUX9X7tWBB5zqZCIZ5tK4QqMhvU8WxAdk6wCspg YC0j2bStvwxrOyXkRnbYFFExlHkvBhRfqcKJ9zQzUFjFHkEfRAb3YYujnYacyvQKg2 yLWpcPzA52N3MM7jj91VadETBIsS3FgY2wG/j8SYFVNfjLQkvOjuJyAFsLp+Gmcd0d HmpB92RAbrfCw== Date: Tue, 27 Feb 2024 14:03:31 +0000 From: Lancelot SIX To: Tom Tromey Cc: gdb-patches@sourceware.org Subject: Re: [PATCH 1/5] Rewrite final cleanups Message-ID: <20240227140331.cavxavrv4uoz4cdr@octopus> References: <20240223-final-cleanups-v1-0-84d5271e9979@adacore.com> <20240223-final-cleanups-v1-1-84d5271e9979@adacore.com> <20240225223034.l6gnzbamvivyzuyu@octopus> <87msrnvyz4.fsf@tromey.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <87msrnvyz4.fsf@tromey.com> X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.6.2 (lndn.lancelotsix.com [0.0.0.0]); Tue, 27 Feb 2024 14:03:35 +0000 (UTC) X-Spam-Status: No, score=-5.9 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,SPF_HELO_NONE,SPF_PASS,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 List-Id: On Mon, Feb 26, 2024 at 11:53:35AM -0700, Tom Tromey wrote: > >>>>> "Lancelot" == Lancelot SIX writes: > > >> tempdir_name = xstrdup (tempdir_name); > >> - make_final_cleanup (do_rmdir, 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); > > Lancelot> I am aware that this is orthogonal to this patch and can be address by > Lancelot> another patch, but in the way to c++ification, this could be replaced > Lancelot> with: > > Lancelot> std::filesystem::remove_all (tempdir_name); > > Yeah, I think it's better to do this kind of thing as a separate > cleanup. > > Also I wonder if all the compilers we support ship std::filesystem. > (I have no idea.) > > >> void > >> do_final_cleanups () > >> { > >> - do_my_cleanups (&final_cleanup_chain, SENTINEL_CLEANUP); > >> + for (auto &func : all_cleanups) > >> + func (); > >> + all_cleanups.clear (); > > Lancelot> I am wondering if we want special handling if one of the cleanup > Lancelot> function ever throws. It is probably acceptable to expect callbacks to > Lancelot> never throw (unfortunately, we can’t have use std::function Lancelot> noexcept> to have this in the type system). If we accept that callbacks > Lancelot> can throw, is it OK to skip pending cleanup functions? > > We could catch and ignore exceptions here. > I'm not sure how important this really is. The current code has ignored > it for decades. My train of thought was 1) This `system ("rm -rf ...")` could be replaced with std::filesystem::remove_all 2) At least one overload of remove_all can throw 3) What would happen if a cleanup throws (either an existing one, or one to change/introduce in the future). I don't think this is a critical issue in any way, just something to consider in a C -> C++ transition. The options I can think of are: - let exceptions go through (some cleanup code might not get a chance to execute), - catch and ignore any exception, - catch exceptions and re-throw the first one captured after all cleanup code has had a chance to run. I can see pros and cons to each of those, and I don't think I have a strong preference either way. FWIW, I skimmed through the rest of the series, and looks reasonable to me. Best, Lancelot. > > Tom