[1/5] Rewrite final cleanups

  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(-)
  

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<debuginfod_client *> (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 <vector>
>  
> -/* 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<std::function<void ()>> 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<void ()> &&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<void ()
noexcept> 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 <functional>
>  
> -/* 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<void ()> &&func);
>  
> +/* Run all the registered functions.  */
>  extern void do_final_cleanups ();
>  
>  #endif /* COMMON_CLEANUPS_H */
> 
> -- 
> 2.43.0
>
  

>>>>> "Lancelot" == Lancelot SIX <lsix@lancelotsix.com> 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<void ()
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.

Tom
  

On Mon, Feb 26, 2024 at 11:53:35AM -0700, Tom Tromey wrote:
> >>>>> "Lancelot" == Lancelot SIX <lsix@lancelotsix.com> 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<void ()
> 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
  

Lancelot> My train of thought was
Lancelot> 1) This `system ("rm -rf ...")` could be replaced with
Lancelot> std::filesystem::remove_all
Lancelot> 2) At least one overload of remove_all can throw
Lancelot> 3) What would happen if a cleanup throws (either an existing one, or one
Lancelot> to change/introduce in the future).

Makes sense.

I filed a bug for this:

https://sourceware.org/bugzilla/show_bug.cgi?id=31420

... and also a C++17 meta-bug, and various C++17 to-do items I've
thought of in the past.

Lancelot> FWIW, I skimmed through the rest of the series, and looks reasonable to
Lancelot> me.

Thanks.  Normally I would not rush this, but the current state of the
tree is preventing us from doing a merge here at AdaCore, as DAP exiting
is currently broken.

So, I am going to check this in.

Tom
  

Tom> ... and also a C++17 meta-bug, and various C++17 to-do items I've
Tom> thought of in the past.

... wanted to add on to this.

I think Simon had some other ideas in this space, though I don't recall
them offhand & couldn't easily find them.

My notes say that I thought we could use CTAD somewhere, but I couldn't
find the spot I was thinking of.  Also we could use the [[maybe_unused]]
or [[nodiscard]] attributes.  I didn't file bugs for these but I suppose
I could... somehow they seemed less well-defined as tasks to me, like
when would we know they were done.

Finally, I also looked at using std::invoke in expop.h, but it doesn't
seem like it would really simplify the code that much.

Tom
  

On 27/02/2024 17:27, Tom Tromey wrote:
> Lancelot> My train of thought was
> Lancelot> 1) This `system ("rm -rf ...")` could be replaced with
> Lancelot> std::filesystem::remove_all
> Lancelot> 2) At least one overload of remove_all can throw
> Lancelot> 3) What would happen if a cleanup throws (either an existing one, or one
> Lancelot> to change/introduce in the future).
>
> Makes sense.
>
> I filed a bug for this:
>
> https://sourceware.org/bugzilla/show_bug.cgi?id=31420
>
> ... and also a C++17 meta-bug, and various C++17 to-do items I've
> thought of in the past.

Thanks for creating the tickets to track this!

I have submitted 
https://sourceware.org/pipermail/gdb-patches/2024-March/206982.html to 
handle this ticket.

Best,
Lancelot.