diff mbox series

[v2] Hold the GIL while clearing gdbpy_reggroup_object_map

Message ID 20260324172109.570014-1-tromey@adacore.com
State New
Headers
Series [v2] Hold the GIL while clearing gdbpy_reggroup_object_map |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_gdb_build--master-aarch64 success Build passed
linaro-tcwg-bot/tcwg_gdb_build--master-arm success Build passed
linaro-tcwg-bot/tcwg_gdb_check--master-arm success Test passed
linaro-tcwg-bot/tcwg_gdb_check--master-aarch64 success Test passed

Commit Message

Tom Tromey March 24, 2026, 5:21 p.m. UTC 
  Tom de Vries pointed out that a certain Python test left a core file.
Looking into this, I think the problem is that the GIL is not held
when gdbpy_reggroup_object_map is destroyed.

This patch changes the code to explicitly clear the global while
holding the GIL.  This fixed the crash for me.

This also adds a comment to gdbpy_initialize_file to explicitly
mention that the GIL is held while the finalizers are run.

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=34013
---
 gdb/python/py-registers.c    | 23 ++++++++++++++++-------
 gdb/python/python-internal.h |  4 +++-
 2 files changed, 19 insertions(+), 8 deletions(-)


base-commit: 7e0417f49792ea5865faa47735be638989fe5229

Comments

Tom de Vries March 25, 2026, 3:28 p.m. UTC | #1 
On 3/24/26 6:21 PM, Tom Tromey wrote:
> Tom de Vries pointed out that a certain Python test left a core file.
> Looking into this, I think the problem is that the GIL is not held
> when gdbpy_reggroup_object_map is destroyed.
> 
> This patch changes the code to explicitly clear the global while
> holding the GIL.  This fixed the crash for me.
> 
> This also adds a comment to gdbpy_initialize_file to explicitly
> mention that the GIL is held while the finalizers are run.
> 

Hi,

LGTM, thanks for fixing this.

Reviewed-By: Tom de Vries <tdevries@suse.de>

Thanks,
- Tom

> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=34013
> ---
>   gdb/python/py-registers.c    | 23 ++++++++++++++++-------
>   gdb/python/python-internal.h |  4 +++-
>   2 files changed, 19 insertions(+), 8 deletions(-)
> 
> diff --git a/gdb/python/py-registers.c b/gdb/python/py-registers.c
> index 44bc53e6a9d..c6e0748e935 100644
> --- a/gdb/python/py-registers.c
> +++ b/gdb/python/py-registers.c
> @@ -80,6 +80,13 @@ struct reggroup_object : public PyObject
>   
>   extern PyTypeObject reggroup_object_type;
>   
> +/* Map from GDB's internal reggroup objects to the Python
> +   representation.  GDB's reggroups are global, and are never deleted,
> +   so using a map like this is safe.  Note it is explicitly cleaned at
> +   Python shutdown, see gdbpy_finalize_registers.  */
> +static gdb::unordered_map<const struct reggroup *,gdbpy_ref<>>
> +    gdbpy_reggroup_object_map;
> +
>   /* Return a gdb.RegisterGroup object wrapping REGGROUP.  The register
>      group objects are cached, and the same Python object will always be
>      returned for the same REGGROUP pointer.  */
> @@ -87,12 +94,6 @@ extern PyTypeObject reggroup_object_type;
>   static gdbpy_ref<>
>   gdbpy_get_reggroup (const reggroup *reggroup)
>   {
> -  /* Map from GDB's internal reggroup objects to the Python representation.
> -     GDB's reggroups are global, and are never deleted, so using a map like
> -     this is safe.  */
> -  static gdb::unordered_map<const struct reggroup *,gdbpy_ref<>>
> -    gdbpy_reggroup_object_map;
> -
>     /* If there is not already a suitable Python object in the map then
>        create a new one, and add it to the map.  */
>     if (gdbpy_reggroup_object_map[reggroup] == nullptr)
> @@ -433,7 +434,15 @@ gdbpy_initialize_registers ()
>     return 0;
>   }
>   
> -GDBPY_INITIALIZE_FILE (gdbpy_initialize_registers);
> +static void
> +gdbpy_finalize_registers ()
> +{
> +  /* Have to hold the GIL while clearing the global map; this is
> +     guaranteed for finalizers.  */
> +  gdbpy_reggroup_object_map.clear ();
> +}
> +
> +GDBPY_INITIALIZE_FILE (gdbpy_initialize_registers, gdbpy_finalize_registers);
>   
>   
>   
> diff --git a/gdb/python/python-internal.h b/gdb/python/python-internal.h
> index bdc960ed208..b24b9314994 100644
> --- a/gdb/python/python-internal.h
> +++ b/gdb/python/python-internal.h
> @@ -616,7 +616,9 @@ class gdbpy_initialize_file
>   
>        There is no error return in this case.  This function is only called
>        when GDB is already shutting down.  The function should make a best
> -     effort to clean up, and then return.  */
> +     effort to clean up, and then return.
> +
> +     The GIL will be held while calling the finalizers.  */
>   
>     using gdbpy_finalize_file_ftype = void (*) (void);
>   
> 
> base-commit: 7e0417f49792ea5865faa47735be638989fe5229
diff mbox series

Patch

diff --git a/gdb/python/py-registers.c b/gdb/python/py-registers.c
index 44bc53e6a9d..c6e0748e935 100644
--- a/gdb/python/py-registers.c
+++ b/gdb/python/py-registers.c
@@ -80,6 +80,13 @@  struct reggroup_object : public PyObject
 
 extern PyTypeObject reggroup_object_type;
 
+/* Map from GDB's internal reggroup objects to the Python
+   representation.  GDB's reggroups are global, and are never deleted,
+   so using a map like this is safe.  Note it is explicitly cleaned at
+   Python shutdown, see gdbpy_finalize_registers.  */
+static gdb::unordered_map<const struct reggroup *,gdbpy_ref<>>
+    gdbpy_reggroup_object_map;
+
 /* Return a gdb.RegisterGroup object wrapping REGGROUP.  The register
    group objects are cached, and the same Python object will always be
    returned for the same REGGROUP pointer.  */
@@ -87,12 +94,6 @@  extern PyTypeObject reggroup_object_type;
 static gdbpy_ref<>
 gdbpy_get_reggroup (const reggroup *reggroup)
 {
-  /* Map from GDB's internal reggroup objects to the Python representation.
-     GDB's reggroups are global, and are never deleted, so using a map like
-     this is safe.  */
-  static gdb::unordered_map<const struct reggroup *,gdbpy_ref<>>
-    gdbpy_reggroup_object_map;
-
   /* If there is not already a suitable Python object in the map then
      create a new one, and add it to the map.  */
   if (gdbpy_reggroup_object_map[reggroup] == nullptr)
@@ -433,7 +434,15 @@  gdbpy_initialize_registers ()
   return 0;
 }
 
-GDBPY_INITIALIZE_FILE (gdbpy_initialize_registers);
+static void
+gdbpy_finalize_registers ()
+{
+  /* Have to hold the GIL while clearing the global map; this is
+     guaranteed for finalizers.  */
+  gdbpy_reggroup_object_map.clear ();
+}
+
+GDBPY_INITIALIZE_FILE (gdbpy_initialize_registers, gdbpy_finalize_registers);
 
 
 
diff --git a/gdb/python/python-internal.h b/gdb/python/python-internal.h
index bdc960ed208..b24b9314994 100644
--- a/gdb/python/python-internal.h
+++ b/gdb/python/python-internal.h
@@ -616,7 +616,9 @@  class gdbpy_initialize_file
 
      There is no error return in this case.  This function is only called
      when GDB is already shutting down.  The function should make a best
-     effort to clean up, and then return.  */
+     effort to clean up, and then return.
+
+     The GIL will be held while calling the finalizers.  */
 
   using gdbpy_finalize_file_ftype = void (*) (void);