From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id 6JTNALC/22VPlCwAWB0awg (envelope-from ) for ; Sun, 25 Feb 2024 17:31:12 -0500 Authentication-Results: simark.ca; dkim=pass (2048-bit key; secure) header.d=lancelotsix.com header.i=@lancelotsix.com header.a=rsa-sha256 header.s=2021 header.b=nAo9cIjH; dkim-atps=neutral Received: by simark.ca (Postfix, from userid 112) id F2ECE1E0D2; Sun, 25 Feb 2024 17:31:11 -0500 (EST) Received: from server2.sourceware.org (server2.sourceware.org [8.43.85.97]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (prime256v1) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPS id 432AC1E030 for ; Sun, 25 Feb 2024 17:31:09 -0500 (EST) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id ACB343858C33 for ; Sun, 25 Feb 2024 22:31:08 +0000 (GMT) Received: from lndn.lancelotsix.com (lndn.lancelotsix.com [51.195.220.111]) by sourceware.org (Postfix) with ESMTPS id 7BDBF3858D32 for ; Sun, 25 Feb 2024 22:30:42 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 7BDBF3858D32 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 7BDBF3858D32 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=1708900245; cv=none; b=xDpIfqOP7cpFRJS3ANZA8jWkRqqd9vmYPadp2ggilt8OR9dVOvLhp+f4StdSeq4WkunaR9yuHw5K+yP1hePPH8lS/I3/iZyhR9KM372UTyW0i0EBLV/dElTb62es+of1praBDbPOH+8novPmdnN/TRrgiM5iYC2dRLCxLY2XUPA= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1708900245; c=relaxed/simple; bh=6NkFVrH4GzsA4f78pe8mlgSzUgGNA5pANNXRiC7Px6I=; h=DKIM-Signature:Date:From:To:Subject:Message-ID:MIME-Version; b=eMhF0dRS8P2p8D1iOEcLsNbbTs0sEY/QiA0Bseit/5xVs5j9TXiPte2yLV28W00xIeY9k/xvRd1fWKwVwOr+ihhr89kBbnE9ithY5+j/icKSI840O+mNNo5DisuqLMsGhfh9sNihI5KE1qNQJLSIZO+gojpjd4B+jG6attR9WzM= 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 460898469D; Sun, 25 Feb 2024 22:30:41 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=lancelotsix.com; s=2021; t=1708900241; bh=6NkFVrH4GzsA4f78pe8mlgSzUgGNA5pANNXRiC7Px6I=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=nAo9cIjHF1nc6b2zAWKJj9Yc3+CHooss+VUZKEDSy4aUH9CuKOGa8OAJ0YrzkX+66 9i+xJ0Pr1RoIATUCORxgTA0cn88J5FL/DPdxfb53IeFAmbCp7PWkkqJRuvPviNo6dT TOegKSs8+BrXPWuV06CdBY90Is464aPYAWQ9I1PyNovl29L2vdFgeMqcif+75X1oaX aVlqq1NgmlrebPmWLw5VOS1m4O8xj80GjERwQ4vc2aall7IjLXljB7gZ+H/kgs1U06 STcBPV752JG2ONIMVTpBUFYPnjBFA+TbiZ1DL7KsULtokaWXTTgFRdigR7eg3C68ZG X7zCQqsdYZAXw== Date: Sun, 25 Feb 2024 22:30:34 +0000 From: Lancelot SIX To: Tom Tromey Cc: gdb-patches@sourceware.org Subject: Re: [PATCH 1/5] Rewrite final cleanups Message-ID: <20240225223034.l6gnzbamvivyzuyu@octopus> References: <20240223-final-cleanups-v1-0-84d5271e9979@adacore.com> <20240223-final-cleanups-v1-1-84d5271e9979@adacore.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20240223-final-cleanups-v1-1-84d5271e9979@adacore.com> X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.6.2 (lndn.lancelotsix.com [0.0.0.0]); Sun, 25 Feb 2024 22:30:41 +0000 (UTC) X-Spam-Status: No, score=-11.9 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, 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 X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.30 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: gdb-patches-bounces+public-inbox=simark.ca@sourceware.org Hi Tom, On Fri, Feb 23, 2024 at 02:11:37PM -0700, Tom Tromey wrote: > This patch rewrites final cleanups to use std::function and otherwise > be more C++-ish. > --- > gdb/compile/compile.c | 30 +++++------- > gdb/debuginfod-support.c | 14 ++---- > gdb/python/python.c | 4 +- > gdbsupport/cleanups.cc | 122 +++++------------------------------------------ > gdbsupport/cleanups.h | 17 ++----- > 5 files changed, 33 insertions(+), 154 deletions(-) > > diff --git a/gdb/compile/compile.c b/gdb/compile/compile.c > index 8cb2e8ac7f1..27cff2553ee 100644 > --- a/gdb/compile/compile.c > +++ b/gdb/compile/compile.c > @@ -427,23 +427,6 @@ compile_print_command (const char *arg, int from_tty) > } > } > > -/* A cleanup function to remove a directory and all its contents. */ > - > -static void > -do_rmdir (void *arg) > -{ > - const char *dir = (const char *) arg; > - char *zap; > - int wstat; > - > - gdb_assert (startswith (dir, TMP_PREFIX)); > - zap = concat ("rm -rf ", dir, (char *) NULL); > - wstat = system (zap); > - if (wstat == -1 || !WIFEXITED (wstat) || WEXITSTATUS (wstat) != 0) > - warning (_("Could not remove temporary directory %s"), dir); > - XDELETEVEC (zap); > -} > - > /* Return the name of the temporary directory to use for .o files, and > arrange for the directory to be removed at shutdown. */ > > @@ -465,7 +448,18 @@ get_compile_file_tempdir (void) > perror_with_name (_("Could not make temporary directory")); > > 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); I am aware that this is orthogonal to this patch and can be address by another patch, but in the way to c++ification, this could be replaced with: std::filesystem::remove_all (tempdir_name); > + }); > return tempdir_name; > } > > diff --git a/gdb/debuginfod-support.c b/gdb/debuginfod-support.c > index 7d8ada39e96..9bb3748c8c3 100644 > --- a/gdb/debuginfod-support.c > +++ b/gdb/debuginfod-support.c > @@ -188,15 +188,6 @@ progressfn (debuginfod_client *c, long cur, long total) > return 0; > } > > -/* Cleanup ARG, which is a debuginfod_client pointer. */ > - > -static void > -cleanup_debuginfod_client (void *arg) > -{ > - debuginfod_client *client = static_cast (arg); > - debuginfod_end (client); > -} > - > /* Return a pointer to the single global debuginfod_client, initialising it > first if needed. */ > > @@ -221,7 +212,10 @@ get_debuginfod_client () > handlers, which is too late. > > So instead, we make use of GDB's final cleanup mechanism. */ > - make_final_cleanup (cleanup_debuginfod_client, global_client); > + add_final_cleanup ([] () > + { > + debuginfod_end (global_client); > + }); > debuginfod_set_progressfn (global_client, progressfn); > } > } > diff --git a/gdb/python/python.c b/gdb/python/python.c > index 7b0997c8d52..971fc850dbb 100644 > --- a/gdb/python/python.c > +++ b/gdb/python/python.c > @@ -2070,7 +2070,7 @@ static struct cmd_list_element *user_show_python_list; > interpreter. This lets Python's 'atexit' work. */ > > static void > -finalize_python (void *ignore) > +finalize_python () > { > struct active_ext_lang_state *previous_active; > > @@ -2310,7 +2310,7 @@ do_start_initialization () > /* Release the GIL while gdb runs. */ > PyEval_SaveThread (); > > - make_final_cleanup (finalize_python, NULL); > + add_final_cleanup (finalize_python); > > /* Only set this when initialization has succeeded. */ > gdb_python_initialized = 1; > diff --git a/gdbsupport/cleanups.cc b/gdbsupport/cleanups.cc > index 619db023063..cc14523b2d1 100644 > --- a/gdbsupport/cleanups.cc > +++ b/gdbsupport/cleanups.cc > @@ -19,126 +19,26 @@ > > #include "common-defs.h" > #include "cleanups.h" > +#include > > -/* The cleanup list records things that have to be undone > - if an error happens (descriptors to be closed, memory to be freed, etc.) > - Each link in the chain records a function to call and an > - argument to give it. > +/* All the cleanup functions. */ > > - Use make_cleanup to add an element to the cleanup chain. > - Use do_cleanups to do all cleanup actions back to a given > - point in the chain. Use discard_cleanups to remove cleanups > - from the chain back to a given point, not doing them. > +static std::vector> all_cleanups; > > - If the argument is pointer to allocated memory, then you need > - to additionally set the 'free_arg' member to a function that will > - free that memory. This function will be called both when the cleanup > - is executed and when it's discarded. */ > +/* See cleanups.h. */ > > -struct cleanup > -{ > - struct cleanup *next; > - void (*function) (void *); > - void (*free_arg) (void *); > - void *arg; > -}; > - > -/* Used to mark the end of a cleanup chain. > - The value is chosen so that it: > - - is non-NULL so that make_cleanup never returns NULL, > - - causes a segv if dereferenced > - [though this won't catch errors that a value of, say, > - ((struct cleanup *) -1) will] > - - displays as something useful when printed in gdb. > - This is const for a bit of extra robustness. > - It is initialized to coax gcc into putting it into .rodata. > - All fields are initialized to survive -Wextra. */ > -static const struct cleanup sentinel_cleanup = { 0, 0, 0, 0 }; > - > -/* Handy macro to use when referring to sentinel_cleanup. */ > -#define SENTINEL_CLEANUP ((struct cleanup *) &sentinel_cleanup) > - > -/* Chain of cleanup actions established with make_final_cleanup, > - to be executed when gdb exits. */ > -static struct cleanup *final_cleanup_chain = SENTINEL_CLEANUP; > - > -/* Main worker routine to create a cleanup. > - PMY_CHAIN is a pointer to either cleanup_chain or final_cleanup_chain. > - FUNCTION is the function to call to perform the cleanup. > - ARG is passed to FUNCTION when called. > - FREE_ARG, if non-NULL, is called after the cleanup is performed. > - > - The result is a pointer to the previous chain pointer > - to be passed later to do_cleanups or discard_cleanups. */ > - > -static struct cleanup * > -make_my_cleanup2 (struct cleanup **pmy_chain, make_cleanup_ftype *function, > - void *arg, void (*free_arg) (void *)) > -{ > - struct cleanup *newobj = XNEW (struct cleanup); > - struct cleanup *old_chain = *pmy_chain; > - > - newobj->next = *pmy_chain; > - newobj->function = function; > - newobj->free_arg = free_arg; > - newobj->arg = arg; > - *pmy_chain = newobj; > - > - gdb_assert (old_chain != NULL); > - return old_chain; > -} > - > -/* Worker routine to create a cleanup without a destructor. > - PMY_CHAIN is a pointer to either cleanup_chain or final_cleanup_chain. > - FUNCTION is the function to call to perform the cleanup. > - ARG is passed to FUNCTION when called. > - > - The result is a pointer to the previous chain pointer > - to be passed later to do_cleanups or discard_cleanups. */ > - > -static struct cleanup * > -make_my_cleanup (struct cleanup **pmy_chain, make_cleanup_ftype *function, > - void *arg) > -{ > - return make_my_cleanup2 (pmy_chain, function, arg, NULL); > -} > - > -/* Add a new cleanup to the final cleanup_chain, > - and return the previous chain pointer > - to be passed later to do_cleanups or discard_cleanups. > - Args are FUNCTION to clean up with, and ARG to pass to it. */ > - > -struct cleanup * > -make_final_cleanup (make_cleanup_ftype *function, void *arg) > -{ > - return make_my_cleanup (&final_cleanup_chain, function, arg); > -} > - > -/* Worker routine to perform cleanups. > - PMY_CHAIN is a pointer to either cleanup_chain or final_cleanup_chain. > - OLD_CHAIN is the result of a "make" cleanup routine. > - Cleanups are performed until we get back to the old end of the chain. */ > - > -static void > -do_my_cleanups (struct cleanup **pmy_chain, > - struct cleanup *old_chain) > +void > +add_final_cleanup (std::function &&func) > { > - struct cleanup *ptr; > - > - while ((ptr = *pmy_chain) != old_chain) > - { > - *pmy_chain = ptr->next; /* Do this first in case of recursion. */ > - (*ptr->function) (ptr->arg); > - if (ptr->free_arg) > - (*ptr->free_arg) (ptr->arg); > - xfree (ptr); > - } > + all_cleanups.emplace_back (std::move (func)); > } > > -/* Discard final cleanups and do the actions they describe. */ > +/* See cleanups.h. */ > > void > do_final_cleanups () > { > - do_my_cleanups (&final_cleanup_chain, SENTINEL_CLEANUP); > + for (auto &func : all_cleanups) > + func (); > + all_cleanups.clear (); I am wondering if we want special handling if one of the cleanup function ever throws. It is probably acceptable to expect callbacks to never throw (unfortunately, we can’t have use std::function to have this in the type system). If we accept that callbacks can throw, is it OK to skip pending cleanup functions? Best, Lancelot. > } > diff --git a/gdbsupport/cleanups.h b/gdbsupport/cleanups.h > index 3e64f7d1684..985cf81ff7d 100644 > --- a/gdbsupport/cleanups.h > +++ b/gdbsupport/cleanups.h > @@ -19,21 +19,12 @@ > #ifndef COMMON_CLEANUPS_H > #define COMMON_CLEANUPS_H > > -/* Outside of cleanups.c, this is an opaque type. */ > -struct cleanup; > +#include > > -/* NOTE: cagney/2000-03-04: This typedef is strictly for the > - make_cleanup function declarations below. Do not use this typedef > - as a cast when passing functions into the make_cleanup() code. > - Instead either use a bounce function or add a wrapper function. > - Calling a f(char*) function with f(void*) is non-portable. */ > -typedef void (make_cleanup_ftype) (void *); > - > -/* Function type for the dtor in make_cleanup_dtor. */ > -typedef void (make_cleanup_dtor_ftype) (void *); > - > -extern struct cleanup *make_final_cleanup (make_cleanup_ftype *, void *); > +/* Register a function that will be called on exit. */ > +extern void add_final_cleanup (std::function &&func); > > +/* Run all the registered functions. */ > extern void do_final_cleanups (); > > #endif /* COMMON_CLEANUPS_H */ > > -- > 2.43.0 >