[PATCH 0/5] Restore DAP 'quit' request

[PATCH 0/5] Restore DAP 'quit' request

[Tom Tromey]

This series started as an attempt to make DAP cause gdb to exit after
a terminate request is handled.

However, as noted in earlier discussion, this can sometimes cause
crashes.

This series fixes the problem by rewriting final cleanups and then
explicitly shutting down Python after these have been run.  A new
final cleanup for runnables is added -- this is needed because the
request to quit may actually race with other reasons to quit (IIUC).

Regression tested on x86-64 Fedora 38.

---
Tom Tromey (5):
      Rewrite final cleanups
      Add extension_language_ops::shutdown
      Change finalize_values into a final cleanup
      Add final cleanup for runnables
      Explicitly quit gdb from DAP server thread

 gdb/compile/compile.c             |  30 ++++------
 gdb/debuginfod-support.c          |  14 ++---
 gdb/extension-priv.h              |   4 ++
 gdb/extension.c                   |  12 ++++
 gdb/extension.h                   |   3 +
 gdb/guile/guile.c                 |   1 +
 gdb/python/lib/gdb/dap/server.py  |   1 +
 gdb/python/python.c               |   6 +-
 gdb/run-on-main-thread.c          |  11 ++++
 gdb/testsuite/lib/dap-support.exp |   9 +++
 gdb/top.c                         |   8 +--
 gdb/value.c                       |  15 ++---
 gdb/value.h                       |   4 --
 gdbsupport/cleanups.cc            | 122 ++++----------------------------------
 gdbsupport/cleanups.h             |  17 ++----
 15 files changed, 85 insertions(+), 172 deletions(-)
---
base-commit: bf8ab2ae8d33e46bb6612408c75e75a6de137ccc
change-id: 20240223-final-cleanups-75c9e5640f5c

Best regards,
-- 
Tom Tromey <tromey@adacore.com>

[PATCH 1/5] Rewrite final cleanups

[Tom Tromey]

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);
+    });
   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 ();
 }
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

Re: [PATCH 1/5] Rewrite final cleanups

[Lancelot SIX]

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
> 

Re: [PATCH 1/5] Rewrite final cleanups

[Tom Tromey]

>>>>> "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

Re: [PATCH 1/5] Rewrite final cleanups

[Lancelot SIX]

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

Re: [PATCH 1/5] Rewrite final cleanups

[Tom Tromey]

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

Re: [PATCH 1/5] Rewrite final cleanups

[Tom Tromey]

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

Re: [PATCH 1/5] Rewrite final cleanups

[Lancelot SIX]

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.

[PATCH 2/5] Add extension_language_ops::shutdown

[Tom Tromey]

Right now, Python is shut down via a final cleanup.  However, it seems
to me that it is better for extension languages to be shut down
explicitly, after all the ordinary final cleanups are run.  The main
reason for this is that a subsequent patch adds another case like
finalize_values; and rather than add a series of workarounds for
Python shutdown, it seemed better to let these be done via final
cleanups, and then have Python shutdown itself be the special case.
---
 gdb/extension-priv.h |  4 ++++
 gdb/extension.c      | 12 ++++++++++++
 gdb/extension.h      |  3 +++
 gdb/guile/guile.c    |  1 +
 gdb/python/python.c  |  6 +++---
 gdb/top.c            |  2 ++
 6 files changed, 25 insertions(+), 3 deletions(-)

diff --git a/gdb/extension-priv.h b/gdb/extension-priv.h
index b709494927d..cb00cb6ff7b 100644
--- a/gdb/extension-priv.h
+++ b/gdb/extension-priv.h
@@ -119,6 +119,10 @@ struct extension_language_ops
      This method is required.  */
   int (*initialized) (const struct extension_language_defn *);
 
+  /* Called just before GDB exits.  This shuts down the extension
+     language.  This can be NULL.  */
+  void (*shutdown) (const struct extension_language_defn *);
+
   /* Process a sequence of commands embedded in GDB's own scripting language.
      E.g.,
      python
diff --git a/gdb/extension.c b/gdb/extension.c
index 42e05199d2c..9f403500497 100644
--- a/gdb/extension.c
+++ b/gdb/extension.c
@@ -341,6 +341,18 @@ ext_lang_initialization (void)
     }
 }
 
+/* See extension.h.  */
+
+void
+ext_lang_shutdown ()
+{
+  for (const struct extension_language_defn *extlang : extension_languages)
+    {
+      if (extlang->ops != nullptr && extlang->ops->shutdown != nullptr)
+	extlang->ops->shutdown (extlang);
+    }
+}
+
 /* Invoke the appropriate extension_language_ops.eval_from_control_command
    method to perform CMD, which is a list of commands in an extension language.
 
diff --git a/gdb/extension.h b/gdb/extension.h
index 0514d7930a2..5260bcbde00 100644
--- a/gdb/extension.h
+++ b/gdb/extension.h
@@ -282,6 +282,9 @@ extern bool ext_lang_auto_load_enabled (const struct extension_language_defn *);
 
 extern void ext_lang_initialization (void);
 
+/* Shut down all extension languages.  */
+extern void ext_lang_shutdown ();
+
 extern void eval_ext_lang_from_control_command (struct command_line *cmd);
 
 extern void auto_load_ext_lang_scripts_for_objfile (struct objfile *);
diff --git a/gdb/guile/guile.c b/gdb/guile/guile.c
index 30d1c72fceb..f0db709e4fe 100644
--- a/gdb/guile/guile.c
+++ b/gdb/guile/guile.c
@@ -115,6 +115,7 @@ static const struct extension_language_ops guile_extension_ops =
 {
   gdbscm_initialize,
   gdbscm_initialized,
+  nullptr,
 
   gdbscm_eval_from_control_command,
 
diff --git a/gdb/python/python.c b/gdb/python/python.c
index 971fc850dbb..dde8afc3c65 100644
--- a/gdb/python/python.c
+++ b/gdb/python/python.c
@@ -110,6 +110,7 @@ static objfile_script_sourcer_func gdbpy_source_objfile_script;
 static objfile_script_executor_func gdbpy_execute_objfile_script;
 static void gdbpy_initialize (const struct extension_language_defn *);
 static int gdbpy_initialized (const struct extension_language_defn *);
+static void finalize_python (const struct extension_language_defn *);
 static void gdbpy_eval_from_control_command
   (const struct extension_language_defn *, struct command_line *cmd);
 static void gdbpy_start_type_printers (const struct extension_language_defn *,
@@ -147,6 +148,7 @@ static const struct extension_language_ops python_extension_ops =
 {
   gdbpy_initialize,
   gdbpy_initialized,
+  finalize_python,
 
   gdbpy_eval_from_control_command,
 
@@ -2070,7 +2072,7 @@ static struct cmd_list_element *user_show_python_list;
    interpreter.  This lets Python's 'atexit' work.  */
 
 static void
-finalize_python ()
+finalize_python (const struct extension_language_defn *ignore)
 {
   struct active_ext_lang_state *previous_active;
 
@@ -2310,8 +2312,6 @@ do_start_initialization ()
   /* Release the GIL while gdb runs.  */
   PyEval_SaveThread ();
 
-  add_final_cleanup (finalize_python);
-
   /* Only set this when initialization has succeeded.  */
   gdb_python_initialized = 1;
   return true;
diff --git a/gdb/top.c b/gdb/top.c
index 5114713baa4..67d6670cd9c 100644
--- a/gdb/top.c
+++ b/gdb/top.c
@@ -1819,6 +1819,8 @@ quit_force (int *exit_arg, int from_tty)
       exception_print (gdb_stderr, ex);
     }
 
+  ext_lang_shutdown ();
+
   exit (exit_code);
 }
 

-- 
2.43.0

[PATCH 3/5] Change finalize_values into a final cleanup

[Tom Tromey]

This removes finalize_values in favor of adding a new final cleanup.
This is safe now that extension languages are explicitly shut down.
---
 gdb/top.c   |  6 ------
 gdb/value.c | 15 ++++++++-------
 gdb/value.h |  4 ----
 3 files changed, 8 insertions(+), 17 deletions(-)

diff --git a/gdb/top.c b/gdb/top.c
index 67d6670cd9c..cf7d3a913ba 100644
--- a/gdb/top.c
+++ b/gdb/top.c
@@ -1803,12 +1803,6 @@ quit_force (int *exit_arg, int from_tty)
       exception_print (gdb_stderr, ex);
     }
 
-  /* Destroy any values currently allocated now instead of leaving it
-     to global destructors, because that may be too late.  For
-     example, the destructors of xmethod values call into the Python
-     runtime, which is finalized via a final cleanup.  */
-  finalize_values ();
-
   /* Do any final cleanups before exiting.  */
   try
     {
diff --git a/gdb/value.c b/gdb/value.c
index c7e940badb9..a2b2721d183 100644
--- a/gdb/value.c
+++ b/gdb/value.c
@@ -4499,12 +4499,13 @@ and exceeds this limit will cause an error."),
 			    selftests::test_insert_into_bit_range_vector);
   selftests::register_test ("value_copy", selftests::test_value_copy);
 #endif
-}
-
-/* See value.h.  */
 
-void
-finalize_values ()
-{
-  all_values.clear ();
+  /* Destroy any values currently allocated in a final cleanup instead
+     of leaving it to global destructors, because that may be too
+     late.  For example, the destructors of xmethod values call into
+     the Python runtime.  */
+  add_final_cleanup ([] ()
+    {
+      all_values.clear ();
+    });
 }
diff --git a/gdb/value.h b/gdb/value.h
index e8d3c9fd907..9d7e88d9433 100644
--- a/gdb/value.h
+++ b/gdb/value.h
@@ -1630,10 +1630,6 @@ struct value *call_internal_function (struct gdbarch *gdbarch,
 
 const char *value_internal_function_name (struct value *);
 
-/* Destroy the values currently allocated.  This is called when GDB is
-   exiting (e.g., on quit_force).  */
-extern void finalize_values ();
-
 /* Convert VALUE to a gdb_mpq.  The caller must ensure that VALUE is
    of floating-point, fixed-point, or integer type.  */
 extern gdb_mpq value_to_gdb_mpq (struct value *value);

-- 
2.43.0

[PATCH 4/5] Add final cleanup for runnables

[Tom Tromey]

This changes run-on-main-thread.c to clear 'runnables' in a final
cleanup.  This avoids an issue where a pending runnable could require
Python, but be run after the Python interpreter was finalized.

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31172
---
 gdb/run-on-main-thread.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/gdb/run-on-main-thread.c b/gdb/run-on-main-thread.c
index 2d40048de56..7b9807e0eaf 100644
--- a/gdb/run-on-main-thread.c
+++ b/gdb/run-on-main-thread.c
@@ -133,4 +133,15 @@ _initialize_run_on_main_thread ()
   runnable_event = make_serial_event ();
   add_file_handler (serial_event_fd (runnable_event), run_events, nullptr,
 		    "run-on-main-thread");
+
+  /* A runnable may refer to an extension language.  So, we want to
+     make sure any pending ones have been deleted before the extension
+     languages are shut down.  */
+  add_final_cleanup ([] ()
+    {
+#if CXX_STD_THREAD
+      std::lock_guard lock (runnable_mutex);
+#endif
+      runnables.clear ();
+    });
 }

-- 
2.43.0

[PATCH 5/5] Explicitly quit gdb from DAP server thread

[Tom Tromey]

This changes the DAP code to explicitly request that gdb exit.
Previously this could cause crashes, but with the previous cleanups,
this should no longer happen.

This also adds a tests that ensures that gdb exits with status 0.
---
 gdb/python/lib/gdb/dap/server.py  | 1 +
 gdb/testsuite/lib/dap-support.exp | 9 +++++++++
 2 files changed, 10 insertions(+)

diff --git a/gdb/python/lib/gdb/dap/server.py b/gdb/python/lib/gdb/dap/server.py
index 6757f2921bf..19840f4028d 100644
--- a/gdb/python/lib/gdb/dap/server.py
+++ b/gdb/python/lib/gdb/dap/server.py
@@ -230,6 +230,7 @@ class Server:
         # responses are flushed to the client before exiting.
         self.write_queue.put(None)
         json_writer.join()
+        send_gdb("quit")
 
     @in_dap_thread
     def send_event_later(self, event, body=None):
diff --git a/gdb/testsuite/lib/dap-support.exp b/gdb/testsuite/lib/dap-support.exp
index 72c22d00711..54795a34e39 100644
--- a/gdb/testsuite/lib/dap-support.exp
+++ b/gdb/testsuite/lib/dap-support.exp
@@ -400,6 +400,15 @@ proc dap_check_log_file_re { re } {
 proc dap_shutdown {{terminate false}} {
     dap_check_request_and_response "shutdown" disconnect \
 	[format {o terminateDebuggee [l %s]} $terminate]
+
+    # Check gdb's exit status.
+    global gdb_spawn_id
+    set result [wait -i $gdb_spawn_id]
+    gdb_assert {[lindex $result 2] == 0}
+    gdb_assert {[lindex $result 3] == 0}
+
+    clear_gdb_spawn_id
+
     dap_check_log_file
 }
 

-- 
2.43.0