[RFC 0/9] Attempt to unify Python object's lifecycle

[RFC 0/9] Attempt to unify Python object's lifecycle

[Jan Vrany]

Hi,

this RFC is an attempt to systematically address Andrew's
comment on a patch I submitted earlier [1]. In particular,
this part:

> I don't really like the approach taken by this function.  Each time the
> function is called we get a new gdb.Compunit object created and chained
> onto the objfile.

This is in fact true for other objects - gdb.Symtab, gdb.Symbol,
gdb.Type. Moreover each of these objects have their own copy
of (largely same) housekeeping code. The gdb.Block used to be
the same but not long ago it was changed so gdb.Blocks are now
interned (memoized). From the user point of view, I found it bit
counter-intuitive.

My idea was to refactor this housekeeping code into a common class.
This RFC is result of several iterations. Tested on x86_64-linux.

First four commits expand existing code to intern (memoize)
gdb.Symtab, gdb.Symbol and gdb.Type. The rest then introduces
template classes that implement necessary housekeeping and
converts gdb.Symtab, gdb.Symbol and gdb.Type to use it.
It could go further (one can convert gdb.Block too and
gdb.Value and gdb.Value.type and dynamic_type could be
further simplified too) but I decided to stop here.

The main reason is that it turned not to be as simple as
I thought - there seem to be few differences here and there
(see [2]). This complicated the housekeeping classes
(gdbpy_registry and associated "storage"). My C++ is
is pretty basic so perhaps there's better and simpler way
of doing it. Another reason is that I was hoping for some code
reduction in terms of size but looking at the result,
there's hardly any. On the other hand, the lifecycle management
is more unified across different Python objects.

All in all, I'm not sure this is the best approach and worth
it. By this RFC, I'd like to solicit feedback from experienced
GDB developers on how to move on.

Basically I see following options:

1) Do not change anything in this area (I'm perfectly
   happy with that).
2) Intern (memoize) Python objects (where it makes sense)
   but keep the current approach. Basically first four
   commits of this RFC.
3) Continue working on this.

Thanks,

Jan

[1]: https://inbox.sourceware.org/gdb-patches/87o70ar34z.fsf@redhat.com/
[2]: https://inbox.sourceware.org/gdb/6c73f834d3f7b545188e1999125e7ae63ae83eab.camel@vrany.io/T/#u

---

Jan Vrany (9):
  gdb/python: preserve identity for gdb.Symtab objects
  gdb/python: preserve identity for gdb.Symbol objects
  gdb/python: do not hold on gdb.Symtab object from gdb.Symtab_and_line
  gdb/python: preserve identity for gdb.Type objects
  gdb/python: introduce gdbpy_registry
  gdb/python: convert gdb.Symbol to use gdbpy_registry
  gdb/python: convert gdb.Type to use gdbpy_registry
  gdb/python: convert gdb.Symtab to use gdbpy_registry
  gdb/python: convert gdb.Symtab_and_line to use gdbpy_registry

 gdb/python/py-symbol.c                 |  75 +++-------
 gdb/python/py-symtab.c                 | 182 ++++++----------------
 gdb/python/py-type.c                   |  95 +++++-------
 gdb/python/python-internal.h           | 200 +++++++++++++++++++++++++
 gdb/testsuite/gdb.python/py-arch.exp   |   5 +
 gdb/testsuite/gdb.python/py-symtab.exp |  28 ++++
 gdb/testsuite/gdb.python/py-type.exp   |  15 ++
 7 files changed, 347 insertions(+), 253 deletions(-)

-- 
2.45.2

[RFC 1/9] gdb/python: preserve identity for gdb.Symtab objects

[Jan Vrany]

This commit changes symtab_to_symtab_object() so that each it is called
with a particular struct symtab * it returns the very same gdb.Symtab
object.

This is done by searching per-objfile linked list of instances and - if
found - return it rather than creating new gdb.Symtab.
---
 gdb/python/py-symtab.c                 | 18 ++++++++++++++++-
 gdb/testsuite/gdb.python/py-symtab.exp | 28 ++++++++++++++++++++++++++
 2 files changed, 45 insertions(+), 1 deletion(-)

diff --git a/gdb/python/py-symtab.c b/gdb/python/py-symtab.c
index 99a5094ba60..7b51e211daf 100644
--- a/gdb/python/py-symtab.c
+++ b/gdb/python/py-symtab.c
@@ -449,7 +449,7 @@ set_symtab (symtab_object *obj, struct symtab *symtab)
 {
   obj->symtab = symtab;
   obj->prev = NULL;
-  if (symtab)
+  if (symtab != nullptr)
     {
       obj->next = stpy_objfile_data_key.get (symtab->compunit ()->objfile ());
       if (obj->next)
@@ -467,6 +467,22 @@ symtab_to_symtab_object (struct symtab *symtab)
 {
   symtab_object *symtab_obj;
 
+  /* Look if there's already a gdb.Symtab object for given SYMTAB
+     and if so, return it.  */
+  if (symtab != nullptr)
+    {
+      symtab_obj = stpy_objfile_data_key.get (symtab->compunit ()->objfile ());
+      while (symtab_obj != nullptr)
+	{
+	  if (symtab_obj->symtab == symtab)
+	    {
+	      Py_INCREF (symtab_obj);
+	      return (PyObject*)symtab_obj;
+	    }
+	  symtab_obj = symtab_obj->next;
+	}
+    }
+
   symtab_obj = PyObject_New (symtab_object, &symtab_object_type);
   if (symtab_obj)
     set_symtab (symtab_obj, symtab);
diff --git a/gdb/testsuite/gdb.python/py-symtab.exp b/gdb/testsuite/gdb.python/py-symtab.exp
index 4765ef5cb2f..18d77a0296f 100644
--- a/gdb/testsuite/gdb.python/py-symtab.exp
+++ b/gdb/testsuite/gdb.python/py-symtab.exp
@@ -90,6 +90,34 @@ gdb_test_multiple "python print (\"simple_struct\" in static_symbols)" \
 	}
     }
 
+# Test symtab identity
+gdb_test "python print (symtab is symtab)"\
+    "True" \
+    "test symtab identity 1"
+gdb_test "python print (symtab is gdb.selected_frame().find_sal().symtab)"\
+    "True" \
+    "test symtab identity 2"
+gdb_test "python print (sal.symtab is gdb.selected_frame().find_sal().symtab)"\
+    "True" \
+    "test symtab identity 3"
+gdb_test "python print (symtab is not \"xxx\")"\
+    "True" \
+    "test symtab non-identity with non-symtab"
+
+# Test symtab equality
+gdb_test "python print (symtab == symtab)"\
+    "True" \
+    "test symtab equality 1"
+gdb_test "python print (symtab == gdb.selected_frame().find_sal().symtab)"\
+    "True" \
+    "test symtab equality 2"
+gdb_test "python print (sal.symtab == gdb.selected_frame().find_sal().symtab)"\
+    "True" \
+    "test symtab equality 3"
+gdb_test "python print (symtab != \"xxx\")"\
+    "True" \
+    "test symtab non-equality with non-symtab"
+
 # Test is_valid when the objfile is unloaded.  This must be the last
 # test as it unloads the object file in GDB.
 gdb_unload
-- 
2.45.2

[RFC 2/9] gdb/python: preserve identity for gdb.Symbol objects

[Jan Vrany]

This commit changes symbol_to_symbol_object() so that each it is called
with a particular struct symbol * it returns the very same gdb.Symbol
object.

This is done in the same way as for gdb.Symtab objects in earlier commit
("gdb/python: preserve identity for gdb.Symtab objects") except that
symbols may be either objfile-owned or arch-owned.

Prior this commit, arch-owned objects we not put into any list (like
objfile-owned ones) so they could not be easily looked up. This commit
changes the code so arch-owned list are put into per-architecture list
which is then used (solely) for looking up arch-owned gdb.Symbol.
---
 gdb/python/py-symbol.c | 64 ++++++++++++++++++++++++++++++++++--------
 1 file changed, 52 insertions(+), 12 deletions(-)

diff --git a/gdb/python/py-symbol.c b/gdb/python/py-symbol.c
index f1ba0ba00e0..2abac6553a0 100644
--- a/gdb/python/py-symbol.c
+++ b/gdb/python/py-symbol.c
@@ -70,6 +70,8 @@ struct symbol_object_deleter
 
 static const registry<objfile>::key<symbol_object, symbol_object_deleter>
      sympy_objfile_data_key;
+static const registry<gdbarch>::key<symbol_object, symbol_object_deleter>
+     sympy_gdbarch_data_key;
 
 static PyObject *
 sympy_str (PyObject *self)
@@ -334,19 +336,29 @@ static void
 set_symbol (symbol_object *obj, struct symbol *symbol)
 {
   obj->symbol = symbol;
-  obj->prev = NULL;
-  if (symbol->is_objfile_owned ()
-      && symbol->symtab () != NULL)
+  obj->prev = nullptr;
+  if (symbol->is_objfile_owned ())
     {
-      struct objfile *objfile = symbol->objfile ();
+      /* Can it really happen that symbol->symtab () is NULL?  */
+      if (symbol->symtab () != nullptr)
+	{
+	  struct objfile *objfile = symbol->objfile ();
+
+	  obj->next = sympy_objfile_data_key.get (objfile);
+	  if (obj->next)
+	    obj->next->prev = obj;
+	  sympy_objfile_data_key.set (objfile, obj);
+	}
+    }
+  else
+    {
+      struct gdbarch *arch = symbol->arch ();
 
-      obj->next = sympy_objfile_data_key.get (objfile);
+      obj->next = sympy_gdbarch_data_key.get (arch);
       if (obj->next)
 	obj->next->prev = obj;
-      sympy_objfile_data_key.set (objfile, obj);
+      sympy_gdbarch_data_key.set (arch, obj);
     }
-  else
-    obj->next = NULL;
 }
 
 /* Create a new symbol object (gdb.Symbol) that encapsulates the struct
@@ -356,6 +368,23 @@ symbol_to_symbol_object (struct symbol *sym)
 {
   symbol_object *sym_obj;
 
+  /* Look if there's already a gdb.Symtab object for given SYMTAB
+     and if so, return it.  */
+  if (sym->is_objfile_owned ())
+    sym_obj = sympy_objfile_data_key.get (sym->objfile ());
+  else
+    sym_obj = sympy_gdbarch_data_key.get (sym->arch ());
+
+  while (sym_obj != nullptr)
+    {
+      if (sym_obj->symbol == sym)
+	{
+	  Py_INCREF (sym_obj);
+	  return (PyObject*)sym_obj;
+	}
+      sym_obj = sym_obj->next;
+    }
+
   sym_obj = PyObject_New (symbol_object, &symbol_object_type);
   if (sym_obj)
     set_symbol (sym_obj, sym);
@@ -379,10 +408,21 @@ sympy_dealloc (PyObject *obj)
 
   if (sym_obj->prev)
     sym_obj->prev->next = sym_obj->next;
-  else if (sym_obj->symbol != NULL
-	   && sym_obj->symbol->is_objfile_owned ()
-	   && sym_obj->symbol->symtab () != NULL)
-    sympy_objfile_data_key.set (sym_obj->symbol->objfile (), sym_obj->next);
+  else if (sym_obj->symbol != nullptr)
+    {
+      if (sym_obj->symbol->is_objfile_owned ())
+	{
+	  /* Can it really happen that symbol->symtab () is NULL?  */
+	  if (sym_obj->symbol->symtab () != nullptr)
+	    sympy_objfile_data_key.set (sym_obj->symbol->objfile (),
+					sym_obj->next);
+	}
+      else
+	{
+	  sympy_gdbarch_data_key.set (sym_obj->symbol->arch (),
+				      sym_obj->next);
+	}
+    }
   if (sym_obj->next)
     sym_obj->next->prev = sym_obj->prev;
   sym_obj->symbol = NULL;
-- 
2.45.2

[RFC 3/9] gdb/python: do not hold on gdb.Symtab object from gdb.Symtab_and_line

[Jan Vrany]

Previous commit changed symtab_to_symtab_object() so each time it is
called with particula struct symtab* it returns the same object.

Therefore there's no longer need to hold on symtab object (gdb.Symtab)
from struct sal_object in order to preserve identity of Symtab object
held in gdb.Symtab_and_line.symtab property. This in turn allowed for
some simplification in various functions.

While at it I changed a couple of NULLs to nullptrs.
---
 gdb/python/py-symbol.c |  2 +-
 gdb/python/py-symtab.c | 73 ++++++++++++------------------------------
 2 files changed, 22 insertions(+), 53 deletions(-)

diff --git a/gdb/python/py-symbol.c b/gdb/python/py-symbol.c
index 2abac6553a0..c157bd7f5d7 100644
--- a/gdb/python/py-symbol.c
+++ b/gdb/python/py-symbol.c
@@ -368,7 +368,7 @@ symbol_to_symbol_object (struct symbol *sym)
 {
   symbol_object *sym_obj;
 
-  /* Look if there's already a gdb.Symtab object for given SYMTAB
+  /* Look if there's already a gdb.Symbol object for given SYMBOL
      and if so, return it.  */
   if (sym->is_objfile_owned ())
     sym_obj = sympy_objfile_data_key.get (sym->objfile ());
diff --git a/gdb/python/py-symtab.c b/gdb/python/py-symtab.c
index 7b51e211daf..177028ecf01 100644
--- a/gdb/python/py-symtab.c
+++ b/gdb/python/py-symtab.c
@@ -77,8 +77,6 @@ static const registry<objfile>::key<symtab_object, stpy_deleter>
 
 struct sal_object {
   PyObject_HEAD
-  /* The GDB Symbol table structure.  */
-  PyObject *symtab;
   /* The GDB Symbol table and line structure.  */
   struct symtab_and_line *sal;
   /* A Symtab and line object is associated with an objfile, so keep
@@ -104,14 +102,10 @@ struct salpy_deleter
       {
 	sal_object *next = obj->next;
 
-	gdbpy_ref<> tmp (obj->symtab);
-	obj->symtab = Py_None;
-	Py_INCREF (Py_None);
-
-	obj->next = NULL;
-	obj->prev = NULL;
+	obj->next = nullptr;
+	obj->prev = nullptr;
 	xfree (obj->sal);
-	obj->sal = NULL;
+	obj->sal = nullptr;
 
 	obj = next;
       }
@@ -272,18 +266,15 @@ salpy_str (PyObject *self)
 {
   const char *filename;
   sal_object *sal_obj;
-  struct symtab_and_line *sal = NULL;
+  struct symtab_and_line *sal = nullptr;
 
   SALPY_REQUIRE_VALID (self, sal);
 
   sal_obj = (sal_object *) self;
-  if (sal_obj->symtab == Py_None)
+  if (sal_obj->sal->symtab == nullptr)
     filename = "<unknown>";
   else
-    {
-      symtab *symtab = symtab_object_to_symtab (sal_obj->symtab);
-      filename = symtab_to_filename_for_display (symtab);
-    }
+    filename = symtab_to_filename_for_display (sal_obj->sal->symtab);
 
   return PyUnicode_FromFormat ("symbol and line for %s, line %d", filename,
 			       sal->line);
@@ -346,13 +337,13 @@ static PyObject *
 salpy_get_symtab (PyObject *self, void *closure)
 {
   struct symtab_and_line *sal;
-  sal_object *self_sal = (sal_object *) self;
 
   SALPY_REQUIRE_VALID (self, sal);
 
-  Py_INCREF (self_sal->symtab);
-
-  return (PyObject *) self_sal->symtab;
+  if (sal->symtab == nullptr)
+    Py_RETURN_NONE;
+  else
+    return symtab_to_symtab_object (sal->symtab);
 }
 
 /* Implementation of gdb.Symtab_and_line.is_valid (self) -> Boolean.
@@ -377,15 +368,14 @@ salpy_dealloc (PyObject *self)
 
   if (self_sal->prev)
     self_sal->prev->next = self_sal->next;
-  else if (self_sal->symtab != Py_None)
+  else if (self_sal->sal != nullptr && self_sal->sal->symtab != nullptr)
     salpy_objfile_data_key.set
-      (symtab_object_to_symtab (self_sal->symtab)->compunit ()->objfile (),
+      (self_sal->sal->symtab->compunit ()->objfile (),
        self_sal->next);
 
   if (self_sal->next)
     self_sal->next->prev = self_sal->prev;
 
-  Py_DECREF (self_sal->symtab);
   xfree (self_sal->sal);
   Py_TYPE (self)->tp_free (self);
 }
@@ -395,37 +385,19 @@ salpy_dealloc (PyObject *self)
    Also, register the sal_object life-cycle with the life-cycle of the
    object file associated with this sal, if needed.  If a failure
    occurs during the sal population, this function will return -1.  */
-static int CPYCHECKER_NEGATIVE_RESULT_SETS_EXCEPTION
+static void
 set_sal (sal_object *sal_obj, struct symtab_and_line sal)
 {
-  PyObject *symtab_obj;
-
-  if (sal.symtab)
-    {
-      symtab_obj = symtab_to_symtab_object  (sal.symtab);
-      /* If a symtab existed in the sal, but it cannot be duplicated,
-	 we exit.  */
-      if (symtab_obj == NULL)
-	return -1;
-    }
-  else
-    {
-      symtab_obj = Py_None;
-      Py_INCREF (Py_None);
-    }
-
   sal_obj->sal = ((struct symtab_and_line *)
 		  xmemdup (&sal, sizeof (struct symtab_and_line),
 			   sizeof (struct symtab_and_line)));
-  sal_obj->symtab = symtab_obj;
   sal_obj->prev = NULL;
 
   /* If the SAL does not have a symtab, we do not add it to the
      objfile cleanup observer linked list.  */
-  if (sal_obj->symtab != Py_None)
+  symtab *symtab = sal_obj->sal->symtab;
+  if (symtab != nullptr)
     {
-      symtab *symtab = symtab_object_to_symtab (sal_obj->symtab);
-
       sal_obj->next
 	= salpy_objfile_data_key.get (symtab->compunit ()->objfile ());
       if (sal_obj->next)
@@ -435,8 +407,6 @@ set_sal (sal_object *sal_obj, struct symtab_and_line sal)
     }
   else
     sal_obj->next = NULL;
-
-  return 0;
 }
 
 /* Given a symtab, and a symtab_object that has previously been
@@ -495,14 +465,13 @@ symtab_to_symtab_object (struct symtab *symtab)
 PyObject *
 symtab_and_line_to_sal_object (struct symtab_and_line sal)
 {
-  gdbpy_ref<sal_object> sal_obj (PyObject_New (sal_object, &sal_object_type));
-  if (sal_obj != NULL)
-    {
-      if (set_sal (sal_obj.get (), sal) < 0)
-	return NULL;
-    }
+  sal_object *sal_obj;
+
+  sal_obj = PyObject_New (sal_object, &sal_object_type);
+  if (sal_obj != nullptr)
+    set_sal (sal_obj, sal);
 
-  return (PyObject *) sal_obj.release ();
+  return (PyObject *) sal_obj;
 }
 
 /* Return struct symtab_and_line reference that is wrapped by this
-- 
2.45.2

[RFC 4/9] gdb/python: preserve identity for gdb.Type objects

[Jan Vrany]

This commit changes type_to_type_object() so that each it is called
with a particular struct type * it returns the very same gdb.Type
object.

This is done in the same way as for gdb.Symtab objects in earlier commit
("gdb/python: preserve identity for gdb.Symtab objects") except that
types may be either objfile-owned or arch-owned.

Prior this commit, arch-owned objects we not put into any list (like
objfile-owned ones) so they could not be easily looked up. This commit
changes the code so arch-owned list are put into per-architecture list
which is then used (solely) for looking up arch-owned gdb.Type.

Another complication comes from the fact that when objfile is about to
be freed, associated gdb.Type instances are not merely invalidated
(like it is done with gdb.Symtab or gdb.Symbol objects) but instead the
type is copied and the copy is arch-owned. So we need two different
"deleters", one for objfile-owned types that copies the type (as before)
and then insert the object to per-architecture list and another one
for arch-owned types.
---
 gdb/python/py-type.c                 | 104 ++++++++++++++++++++++-----
 gdb/testsuite/gdb.python/py-arch.exp |   5 ++
 gdb/testsuite/gdb.python/py-type.exp |  15 ++++
 3 files changed, 105 insertions(+), 19 deletions(-)

diff --git a/gdb/python/py-type.c b/gdb/python/py-type.c
index 11a96d52c2e..c9fa6a37250 100644
--- a/gdb/python/py-type.c
+++ b/gdb/python/py-type.c
@@ -1162,8 +1162,12 @@ typy_richcompare (PyObject *self, PyObject *other, int op)
 
 \f
 
+/* Forward declaration, see below.  */
+static void
+set_type (type_object *obj, struct type *type);
+
 /* Deleter that saves types when an objfile is being destroyed.  */
-struct typy_deleter
+struct typy_deleter_for_objfile_owned
 {
   void operator() (type_object *obj)
   {
@@ -1181,35 +1185,74 @@ struct typy_deleter
 	type_object *next = obj->next;
 
 	copied_types.clear ();
-	obj->type = copy_type_recursive (obj->type, copied_types);
 
-	obj->next = NULL;
-	obj->prev = NULL;
+	/* Set a copied (now arch-owned) type.  As a side-effect this
+	   adds OBJ to per-arch list.  We do not need to remove it from
+	   per-objfile list since the objfile is going to go completely
+	   anyway.  */
+	set_type (obj, copy_type_recursive (obj->type, copied_types));
 
 	obj = next;
       }
   }
 };
 
-static const registry<objfile>::key<type_object, typy_deleter>
+/* Deleter that is used when an arch is is about to be freed.  */
+struct typy_deleter_for_arch_owned
+{
+  void operator() (type_object *obj)
+  {
+    while (obj)
+      {
+	type_object *next = obj->next;
+
+	obj->type = nullptr;
+
+	obj->next = nullptr;
+	obj->prev = nullptr;
+
+	obj = next;
+      }
+  }
+};
+
+
+
+static const registry<objfile>::key<type_object, typy_deleter_for_objfile_owned>
      typy_objfile_data_key;
+static const registry<gdbarch>::key<type_object, typy_deleter_for_arch_owned>
+     typy_gdbarch_data_key;
 
 static void
 set_type (type_object *obj, struct type *type)
 {
   obj->type = type;
-  obj->prev = NULL;
-  if (type != nullptr && type->objfile_owner () != nullptr)
+  obj->prev = nullptr;
+
+  /* Can it really happen that type is NULL?  */
+  if (type != nullptr)
     {
-      struct objfile *objfile = type->objfile_owner ();
+      if (type->objfile_owner () != nullptr)
+	{
+	  struct objfile *objfile = type->objfile_owner ();
+
+	  obj->next = typy_objfile_data_key.get (objfile);
+	  if (obj->next)
+	    obj->next->prev = obj;
+	  typy_objfile_data_key.set (objfile, obj);
+	}
+      else
+	{
+	  struct gdbarch *arch = type->arch_owner ();
 
-      obj->next = typy_objfile_data_key.get (objfile);
-      if (obj->next)
-	obj->next->prev = obj;
-      typy_objfile_data_key.set (objfile, obj);
+	  obj->next = typy_gdbarch_data_key.get (arch);
+	  if (obj->next)
+	    obj->next->prev = obj;
+	  typy_gdbarch_data_key.set (arch, obj);
+	}
     }
   else
-    obj->next = NULL;
+    obj->next = nullptr;
 }
 
 static void
@@ -1219,13 +1262,19 @@ typy_dealloc (PyObject *obj)
 
   if (type->prev)
     type->prev->next = type->next;
-  else if (type->type != nullptr && type->type->objfile_owner () != nullptr)
+  else if (type->type != nullptr)
     {
-      /* Must reset head of list.  */
-      struct objfile *objfile = type->type->objfile_owner ();
-
-      if (objfile)
-	typy_objfile_data_key.set (objfile, type->next);
+      if (type->type->is_objfile_owned ())
+	{
+	  /* Must reset head of list.  */
+	  struct objfile *objfile = type->type->objfile_owner ();
+	  typy_objfile_data_key.set (objfile, type->next);
+	}
+      else
+	{
+	  struct gdbarch *arch = type->type->arch_owner ();
+	  typy_gdbarch_data_key.set (arch, type->next);
+	}
     }
   if (type->next)
     type->next->prev = type->prev;
@@ -1473,6 +1522,23 @@ type_to_type_object (struct type *type)
       return gdbpy_handle_gdb_exception (nullptr, except);
     }
 
+  /* Look if there's already a gdb.Type object for given TYPE
+     and if so, return it.  */
+  if (type->is_objfile_owned ())
+    type_obj = typy_objfile_data_key.get (type->objfile_owner ());
+  else
+    type_obj = typy_gdbarch_data_key.get (type->arch_owner ());
+
+  while (type_obj != nullptr)
+    {
+      if (type_obj->type == type)
+	{
+	  Py_INCREF (type_obj);
+	  return (PyObject*)type_obj;
+	}
+      type_obj = type_obj->next;
+    }
+
   type_obj = PyObject_New (type_object, &type_object_type);
   if (type_obj)
     set_type (type_obj, type);
diff --git a/gdb/testsuite/gdb.python/py-arch.exp b/gdb/testsuite/gdb.python/py-arch.exp
index 14802ec80a3..aefb9e90eac 100644
--- a/gdb/testsuite/gdb.python/py-arch.exp
+++ b/gdb/testsuite/gdb.python/py-arch.exp
@@ -104,6 +104,11 @@ foreach_with_prefix test_data { {None None} \
 	"check 'signed' argument can handle non-bool type $bad_type"
 }
 
+# Test type identity
+gdb_test "python print(arch.integer_type(32) is arch.integer_type(32))" \
+    "True" \
+    "arch.integer_type(32) always return the same Python object"
+
 # Test for gdb.architecture_names().  First we're going to grab the
 # complete list of architecture names using the 'complete' command.
 set arch_names []
diff --git a/gdb/testsuite/gdb.python/py-type.exp b/gdb/testsuite/gdb.python/py-type.exp
index 7e469c93c35..c9d4353e488 100644
--- a/gdb/testsuite/gdb.python/py-type.exp
+++ b/gdb/testsuite/gdb.python/py-type.exp
@@ -324,6 +324,19 @@ proc test_type_equality {} {
     }
 }
 
+# Test type identity
+proc test_type_identity {} {
+    gdb_test_no_output "python v1 = gdb.parse_and_eval('global_unsigned_int')"
+    gdb_test_no_output "python v2 = gdb.parse_and_eval('global_unsigned_int')"
+
+    gdb_test "python print(v1.type is v2.type)" "True"
+
+    gdb_test_no_output "python t1 = gdb.lookup_type ('char')"
+    gdb_test_no_output "python t2 = gdb.lookup_type ('char')"
+
+    gdb_test "python print(t1 is t2)" "True"
+}
+
 # Test the gdb.Type.is_scalar property.
 proc test_is_scalar { lang } {
     if {$lang == "c++"} {
@@ -376,6 +389,7 @@ if { [build_inferior "${binfile}" "c"] == 0 } {
       test_is_scalar "c"
       test_is_signed "c"
       test_type_equality
+      test_type_identity
   }
 }
 
@@ -392,6 +406,7 @@ if { [build_inferior "${binfile}-cxx" "c++"] == 0 } {
       test_is_scalar "c++"
       test_is_signed "c++"
       test_type_equality
+      test_type_identity
   }
 }
 
-- 
2.45.2

[RFC 5/9] gdb/python: introduce gdbpy_registry

[Jan Vrany]

This commit introduces new template class gdbpy_registry to simplify
Python object lifecycle management. As of now, each of the Python
object implementations contain its own (copy of) lifecycle management
code that is largely very similar. The aim of gdbpy_registry is to
factor out this code into a common (template) class in order to simplify
the code.
---
 gdb/python/python-internal.h | 200 +++++++++++++++++++++++++++++++++++
 1 file changed, 200 insertions(+)

diff --git a/gdb/python/python-internal.h b/gdb/python/python-internal.h
index c48f260c15f..73df7f34a18 100644
--- a/gdb/python/python-internal.h
+++ b/gdb/python/python-internal.h
@@ -22,6 +22,10 @@
 
 #include "extension.h"
 #include "extension-priv.h"
+#include "registry.h"
+#include <unordered_set>
+#include <unordered_map>
+
 
 /* These WITH_* macros are defined by the CPython API checker that
    comes with the Python plugin for GCC.  See:
@@ -1145,4 +1149,200 @@ gdbpy_type_ready (PyTypeObject *type, PyObject *mod = nullptr)
 # define PyType_Ready POISONED_PyType_Ready
 #endif
 
+/* A class to manage lifecycle of Python objects for objects that are "owned" 
+   by an objfile or a gdbarch.  It keeps track of Python objects and when
+   the "owning" object (objfile or gdbarch) is about to be freed, ensures that
+   all Python objects "owned" by that object are properly invalidated.
+
+   The actuall tracking of "owned" Python objects is handled externally
+   by storage class.  Storage object is created for each owning object
+   on demand and it is deleted when owning object is about to be freed.
+
+   The storage class must provide two member types:
+     
+     * obj_type - the type of Python object whose lifecycle is managed. 
+     * val_type - the type of GDB structure the Python objects are 
+       representing.
+
+   It must also provide following methods.
+
+     void add (obj_type *obj);
+     void remove (obj_type *obj);
+     obj_type *lookup (val_type *val);
+
+   Finally it must invalidate all registered Python objects upon deletion.  */
+template <typename Storage>
+class gdbpy_registry
+{
+public:
+  using obj_type = typename Storage::obj_type;
+  using val_type = typename Storage::val_type;
+
+  /* Register Python object OBJ as being "owned" by OWNER.  When OWNER is
+     about to be freed, OBJ will be invalidated.  */
+  template <typename O>
+  void add (O *owner, obj_type *obj) const
+  {
+    get_storage (owner)->add (obj);
+  }
+
+  /* Unregister Python object OBJ.  OBJ will no longer be invalidated when
+     OWNER is about to be be freed.  */
+  template <typename O>
+  void remove (O *owner, obj_type *obj) const
+  {
+    get_storage (owner)->remove (obj);
+  }
+
+  /* Lookup pre-existing Python object for given VAL.  Return such object
+     if found, otherwise return NULL.  This method always returns new
+     reference.  */
+  template <typename O>
+  obj_type *lookup (O *owner, val_type *val) const
+  {
+    obj_type *obj = get_storage (owner)->lookup (val);
+    Py_XINCREF (obj);
+    return obj;
+  }
+
+private:
+
+  template<typename O>
+  using StorageKey = typename registry<O>::key<Storage>;
+
+  template<typename O>
+  Storage *get_storage (O *owner, const StorageKey<O> &key) const
+  {
+    Storage *r = key.get (owner);
+    if (r == nullptr)
+      {
+	r = new Storage();
+	key.set (owner, r);
+      }
+    return r;
+  }
+
+  Storage *get_storage(struct objfile* objf) const
+  {
+    return get_storage(objf, m_key_for_objf);
+  }
+
+  Storage *get_storage(struct gdbarch* arch) const
+  {
+    return get_storage(arch, m_key_for_arch);
+  }
+
+  const registry<objfile>::key<Storage> m_key_for_objf;
+  const registry<gdbarch>::key<Storage> m_key_for_arch;
+};
+
+/* Default invalidator for Python objects.  */
+template <typename P, typename V, V* P::*val_slot>
+struct gdbpy_default_invalidator
+{
+  void operator() (P *obj)
+  {
+    obj->*val_slot = nullptr;
+  }
+};
+
+/* A "storage" implementation suitable for temporary (on-demand) objects.  */
+template <typename P, 
+          typename V, 
+          V* P::*val_slot, 
+	  typename Invalidator = gdbpy_default_invalidator<P, V, val_slot>>
+class gdbpy_tracking_registry_storage
+{
+public:
+  using obj_type = P;
+  using val_type = V;
+
+  void add (obj_type *obj)
+  {
+    gdb_assert (obj != nullptr && obj->*val_slot != nullptr);
+
+    m_objects.insert (obj);    
+  }
+
+  void remove (obj_type *obj)
+  {
+    gdb_assert (obj != nullptr && obj->*val_slot != nullptr);
+
+    m_objects.erase (obj);    
+  }
+
+  obj_type *lookup (val_type *val) const
+  {
+    gdb_assert_not_reached ("Should not be used.");
+  }
+
+  ~gdbpy_tracking_registry_storage ()
+  {
+    Invalidator invalidate;
+    gdbpy_enter enter_py;
+
+    for (auto each : m_objects)
+      invalidate (each);
+    m_objects.clear ();
+  }
+
+private:
+  std::unordered_set<obj_type *> m_objects;
+};
+
+/* A "storage" implementation suitable for memoized (interned) Python objects.
+
+   This implementation retains a reference to Python object for a long as 
+   the owning object exists.  When the owning object is about to be freed,
+   registered Python objects are invalidated and reference dropped.  */
+template <typename P, 
+          typename V, 
+          V* P::*val_slot, 
+	  typename Invalidator = gdbpy_default_invalidator<P, V, val_slot>>
+class gdbpy_memoizing_registry_storage
+{
+public:
+  using obj_type = P;
+  using val_type = V;
+
+  void add (obj_type *obj)
+  {
+    gdb_assert (obj != nullptr && obj->*val_slot != nullptr);
+
+    m_objects[obj->*val_slot] = obj;
+    Py_INCREF (obj);
+  }
+
+  void remove (obj_type *obj)
+  {
+    gdb_assert_not_reached ("Should not be used.");
+  }
+
+  obj_type *lookup (val_type *val) const
+  {
+    auto result = m_objects.find (val);
+    if (result != m_objects.end ())
+      return result->second;
+    else
+      return nullptr;
+  }
+
+  ~gdbpy_memoizing_registry_storage ()
+  {
+    Invalidator invalidate;
+    gdbpy_enter enter_py;
+
+    for (auto each : m_objects)
+      {
+	invalidate (each.second);
+	Py_DECREF (each.second);
+      }
+
+    m_objects.clear ();
+  }
+
+private:
+  std::unordered_map<val_type *, obj_type *> m_objects;
+};
+
 #endif /* GDB_PYTHON_PYTHON_INTERNAL_H */
-- 
2.45.2

[RFC 6/9] gdb/python: convert gdb.Symbol to use gdbpy_registry

[Jan Vrany]

This commit converts gdb.Symbol to use gdbpy_registry for lifecycle
management. Since gdb.Symbol only holds on the struct symbol * (and
prev/next links) the default invalidator can be used.
---
 gdb/python/py-symbol.c | 91 +++++-------------------------------------
 1 file changed, 9 insertions(+), 82 deletions(-)

diff --git a/gdb/python/py-symbol.c b/gdb/python/py-symbol.c
index c157bd7f5d7..f891c5619d6 100644
--- a/gdb/python/py-symbol.c
+++ b/gdb/python/py-symbol.c
@@ -29,12 +29,6 @@ struct symbol_object {
   PyObject_HEAD
   /* The GDB symbol structure this object is wrapping.  */
   struct symbol *symbol;
-  /* A symbol object is associated with an objfile, so keep track with
-     doubly-linked list, rooted in the objfile.  This lets us
-     invalidate the underlying struct symbol when the objfile is
-     deleted.  */
-  symbol_object *prev;
-  symbol_object *next;
 };
 
 /* Require a valid symbol.  All access to symbol_object->symbol should be
@@ -50,28 +44,8 @@ struct symbol_object {
       }							\
   } while (0)
 
-/* A deleter that is used when an objfile is about to be freed.  */
-struct symbol_object_deleter
-{
-  void operator() (symbol_object *obj)
-  {
-    while (obj)
-      {
-	symbol_object *next = obj->next;
-
-	obj->symbol = NULL;
-	obj->next = NULL;
-	obj->prev = NULL;
-
-	obj = next;
-      }
-  }
-};
-
-static const registry<objfile>::key<symbol_object, symbol_object_deleter>
-     sympy_objfile_data_key;
-static const registry<gdbarch>::key<symbol_object, symbol_object_deleter>
-     sympy_gdbarch_data_key;
+static const gdbpy_registry<gdbpy_memoizing_registry_storage<symbol_object, symbol, &symbol_object::symbol>>
+     sympy_registry;
 
 static PyObject *
 sympy_str (PyObject *self)
@@ -336,28 +310,17 @@ static void
 set_symbol (symbol_object *obj, struct symbol *symbol)
 {
   obj->symbol = symbol;
-  obj->prev = nullptr;
   if (symbol->is_objfile_owned ())
     {
       /* Can it really happen that symbol->symtab () is NULL?  */
       if (symbol->symtab () != nullptr)
 	{
-	  struct objfile *objfile = symbol->objfile ();
-
-	  obj->next = sympy_objfile_data_key.get (objfile);
-	  if (obj->next)
-	    obj->next->prev = obj;
-	  sympy_objfile_data_key.set (objfile, obj);
+	  sympy_registry.add (symbol->objfile (), obj);
 	}
     }
   else
     {
-      struct gdbarch *arch = symbol->arch ();
-
-      obj->next = sympy_gdbarch_data_key.get (arch);
-      if (obj->next)
-	obj->next->prev = obj;
-      sympy_gdbarch_data_key.set (arch, obj);
+      sympy_registry.add (symbol->arch (), obj);
     }
 }
 
@@ -371,19 +334,11 @@ symbol_to_symbol_object (struct symbol *sym)
   /* Look if there's already a gdb.Symbol object for given SYMBOL
      and if so, return it.  */
   if (sym->is_objfile_owned ())
-    sym_obj = sympy_objfile_data_key.get (sym->objfile ());
+    sym_obj = sympy_registry.lookup (sym->objfile (), sym);
   else
-    sym_obj = sympy_gdbarch_data_key.get (sym->arch ());
-
-  while (sym_obj != nullptr)
-    {
-      if (sym_obj->symbol == sym)
-	{
-	  Py_INCREF (sym_obj);
-	  return (PyObject*)sym_obj;
-	}
-      sym_obj = sym_obj->next;
-    }
+    sym_obj = sympy_registry.lookup (sym->arch (), sym);
+  if (sym_obj != nullptr)
+    return (PyObject*)sym_obj;
 
   sym_obj = PyObject_New (symbol_object, &symbol_object_type);
   if (sym_obj)
@@ -401,34 +356,6 @@ symbol_object_to_symbol (PyObject *obj)
   return ((symbol_object *) obj)->symbol;
 }
 
-static void
-sympy_dealloc (PyObject *obj)
-{
-  symbol_object *sym_obj = (symbol_object *) obj;
-
-  if (sym_obj->prev)
-    sym_obj->prev->next = sym_obj->next;
-  else if (sym_obj->symbol != nullptr)
-    {
-      if (sym_obj->symbol->is_objfile_owned ())
-	{
-	  /* Can it really happen that symbol->symtab () is NULL?  */
-	  if (sym_obj->symbol->symtab () != nullptr)
-	    sympy_objfile_data_key.set (sym_obj->symbol->objfile (),
-					sym_obj->next);
-	}
-      else
-	{
-	  sympy_gdbarch_data_key.set (sym_obj->symbol->arch (),
-				      sym_obj->next);
-	}
-    }
-  if (sym_obj->next)
-    sym_obj->next->prev = sym_obj->prev;
-  sym_obj->symbol = NULL;
-  Py_TYPE (obj)->tp_free (obj);
-}
-
 /* __repr__ implementation for gdb.Symbol.  */
 
 static PyObject *
@@ -791,7 +718,7 @@ PyTypeObject symbol_object_type = {
   "gdb.Symbol",			  /*tp_name*/
   sizeof (symbol_object),	  /*tp_basicsize*/
   0,				  /*tp_itemsize*/
-  sympy_dealloc,		  /*tp_dealloc*/
+  0,				  /*tp_dealloc*/
   0,				  /*tp_print*/
   0,				  /*tp_getattr*/
   0,				  /*tp_setattr*/
-- 
2.45.2

[RFC 7/9] gdb/python: convert gdb.Type to use gdbpy_registry

[Jan Vrany]

This commit converts gdb.Type to use gdbpy_registry for lifecycle
management.
---
 gdb/python/py-type.c | 117 ++++++-------------------------------------
 1 file changed, 14 insertions(+), 103 deletions(-)

diff --git a/gdb/python/py-type.c b/gdb/python/py-type.c
index c9fa6a37250..049cdca9a18 100644
--- a/gdb/python/py-type.c
+++ b/gdb/python/py-type.c
@@ -32,12 +32,6 @@ struct type_object
 {
   PyObject_HEAD
   struct type *type;
-
-  /* If a Type object is associated with an objfile, it is kept on a
-     doubly-linked list, rooted in the objfile.  This lets us copy the
-     underlying struct type when the objfile is deleted.  */
-  struct type_object *prev;
-  struct type_object *next;
 };
 
 extern PyTypeObject type_object_type
@@ -1166,120 +1160,44 @@ typy_richcompare (PyObject *self, PyObject *other, int op)
 static void
 set_type (type_object *obj, struct type *type);
 
-/* Deleter that saves types when an objfile is being destroyed.  */
-struct typy_deleter_for_objfile_owned
+/* Invalidator that saves types when an objfile is being destroyed.  */
+struct typy_invalidator
 {
   void operator() (type_object *obj)
   {
-    if (!gdb_python_initialized)
-      return;
-
-    /* This prevents another thread from freeing the objects we're
-       operating on.  */
-    gdbpy_enter enter_py;
-
-    copied_types_hash_t copied_types;
-
-    while (obj)
+    if (obj->type->is_objfile_owned ())
       {
-	type_object *next = obj->next;
-
-	copied_types.clear ();
+	copied_types_hash_t copied_types;
 
 	/* Set a copied (now arch-owned) type.  As a side-effect this
 	   adds OBJ to per-arch list.  We do not need to remove it from
 	   per-objfile list since the objfile is going to go completely
 	   anyway.  */
 	set_type (obj, copy_type_recursive (obj->type, copied_types));
-
-	obj = next;
       }
-  }
-};
-
-/* Deleter that is used when an arch is is about to be freed.  */
-struct typy_deleter_for_arch_owned
-{
-  void operator() (type_object *obj)
-  {
-    while (obj)
+    else
       {
-	type_object *next = obj->next;
-
 	obj->type = nullptr;
-
-	obj->next = nullptr;
-	obj->prev = nullptr;
-
-	obj = next;
       }
   }
 };
 
-
-
-static const registry<objfile>::key<type_object, typy_deleter_for_objfile_owned>
-     typy_objfile_data_key;
-static const registry<gdbarch>::key<type_object, typy_deleter_for_arch_owned>
-     typy_gdbarch_data_key;
+static const gdbpy_registry<gdbpy_memoizing_registry_storage<type_object, type, &type_object::type, typy_invalidator>>
+     typy_registry;
 
 static void
 set_type (type_object *obj, struct type *type)
 {
   obj->type = type;
-  obj->prev = nullptr;
 
   /* Can it really happen that type is NULL?  */
   if (type != nullptr)
     {
       if (type->objfile_owner () != nullptr)
-	{
-	  struct objfile *objfile = type->objfile_owner ();
-
-	  obj->next = typy_objfile_data_key.get (objfile);
-	  if (obj->next)
-	    obj->next->prev = obj;
-	  typy_objfile_data_key.set (objfile, obj);
-	}
+	typy_registry.add (type->objfile_owner (), obj);
       else
-	{
-	  struct gdbarch *arch = type->arch_owner ();
-
-	  obj->next = typy_gdbarch_data_key.get (arch);
-	  if (obj->next)
-	    obj->next->prev = obj;
-	  typy_gdbarch_data_key.set (arch, obj);
-	}
-    }
-  else
-    obj->next = nullptr;
-}
-
-static void
-typy_dealloc (PyObject *obj)
-{
-  type_object *type = (type_object *) obj;
-
-  if (type->prev)
-    type->prev->next = type->next;
-  else if (type->type != nullptr)
-    {
-      if (type->type->is_objfile_owned ())
-	{
-	  /* Must reset head of list.  */
-	  struct objfile *objfile = type->type->objfile_owner ();
-	  typy_objfile_data_key.set (objfile, type->next);
-	}
-      else
-	{
-	  struct gdbarch *arch = type->type->arch_owner ();
-	  typy_gdbarch_data_key.set (arch, type->next);
-	}
+	typy_registry.add (type->arch_owner (), obj);
     }
-  if (type->next)
-    type->next->prev = type->prev;
-
-  Py_TYPE (type)->tp_free (type);
 }
 
 /* Return number of fields ("length" of the field dictionary).  */
@@ -1525,19 +1443,12 @@ type_to_type_object (struct type *type)
   /* Look if there's already a gdb.Type object for given TYPE
      and if so, return it.  */
   if (type->is_objfile_owned ())
-    type_obj = typy_objfile_data_key.get (type->objfile_owner ());
+    type_obj = typy_registry.lookup (type->objfile_owner (), type);
   else
-    type_obj = typy_gdbarch_data_key.get (type->arch_owner ());
+    type_obj = typy_registry.lookup (type->arch_owner (), type);
 
-  while (type_obj != nullptr)
-    {
-      if (type_obj->type == type)
-	{
-	  Py_INCREF (type_obj);
-	  return (PyObject*)type_obj;
-	}
-      type_obj = type_obj->next;
-    }
+  if (type_obj != nullptr)
+    return (PyObject*)type_obj;
 
   type_obj = PyObject_New (type_object, &type_object_type);
   if (type_obj)
@@ -1750,7 +1661,7 @@ PyTypeObject type_object_type =
   "gdb.Type",			  /*tp_name*/
   sizeof (type_object),		  /*tp_basicsize*/
   0,				  /*tp_itemsize*/
-  typy_dealloc,			  /*tp_dealloc*/
+  0,			          /*tp_dealloc*/
   0,				  /*tp_print*/
   0,				  /*tp_getattr*/
   0,				  /*tp_setattr*/
-- 
2.45.2

[RFC 8/9] gdb/python: convert gdb.Symtab to use gdbpy_registry

[Jan Vrany]

This commit converts gdb.Symtab to use gdbpy_registry for lifecycle
management. Since gdb.Symtab only holds on the struct symtab * (and
prev/next links) the default invalidator can be used.
---
 gdb/python/py-symtab.c | 74 +++++-------------------------------------
 1 file changed, 8 insertions(+), 66 deletions(-)

diff --git a/gdb/python/py-symtab.c b/gdb/python/py-symtab.c
index 177028ecf01..417ad68a155 100644
--- a/gdb/python/py-symtab.c
+++ b/gdb/python/py-symtab.c
@@ -28,39 +28,12 @@ struct symtab_object {
   PyObject_HEAD
   /* The GDB Symbol table structure.  */
   struct symtab *symtab;
-  /* A symtab object is associated with an objfile, so keep track with
-     a doubly-linked list, rooted in the objfile.  This allows
-     invalidation of the underlying struct symtab when the objfile is
-     deleted.  */
-  symtab_object *prev;
-  symtab_object *next;
-};
-
-/* This function is called when an objfile is about to be freed.
-   Invalidate the symbol table as further actions on the symbol table
-   would result in bad data.  All access to obj->symtab should be
-   gated by STPY_REQUIRE_VALID which will raise an exception on
-   invalid symbol tables.  */
-struct stpy_deleter
-{
-  void operator() (symtab_object *obj)
-  {
-    while (obj)
-      {
-	symtab_object *next = obj->next;
-
-	obj->symtab = NULL;
-	obj->next = NULL;
-	obj->prev = NULL;
-	obj = next;
-      }
-  }
 };
 
 extern PyTypeObject symtab_object_type
     CPYCHECKER_TYPE_OBJECT_FOR_TYPEDEF ("symtab_object");
-static const registry<objfile>::key<symtab_object, stpy_deleter>
-     stpy_objfile_data_key;
+static const gdbpy_registry<gdbpy_memoizing_registry_storage<symtab_object, symtab, &symtab_object::symtab>>
+     stpy_registry;
 
 /* Require a valid symbol table.  All access to symtab_object->symtab
    should be gated by this call.  */
@@ -280,23 +253,6 @@ salpy_str (PyObject *self)
 			       sal->line);
 }
 
-static void
-stpy_dealloc (PyObject *obj)
-{
-  symtab_object *symtab = (symtab_object *) obj;
-
-  if (symtab->prev)
-    symtab->prev->next = symtab->next;
-  else if (symtab->symtab)
-    stpy_objfile_data_key.set (symtab->symtab->compunit ()->objfile (),
-			       symtab->next);
-  if (symtab->next)
-    symtab->next->prev = symtab->prev;
-  symtab->symtab = NULL;
-  Py_TYPE (obj)->tp_free (obj);
-}
-
-
 static PyObject *
 salpy_get_pc (PyObject *self, void *closure)
 {
@@ -418,16 +374,8 @@ static void
 set_symtab (symtab_object *obj, struct symtab *symtab)
 {
   obj->symtab = symtab;
-  obj->prev = NULL;
   if (symtab != nullptr)
-    {
-      obj->next = stpy_objfile_data_key.get (symtab->compunit ()->objfile ());
-      if (obj->next)
-	obj->next->prev = obj;
-      stpy_objfile_data_key.set (symtab->compunit ()->objfile (), obj);
-    }
-  else
-    obj->next = NULL;
+    stpy_registry.add (symtab->compunit ()->objfile (), obj);
 }
 
 /* Create a new symbol table (gdb.Symtab) object that encapsulates the
@@ -441,16 +389,10 @@ symtab_to_symtab_object (struct symtab *symtab)
      and if so, return it.  */
   if (symtab != nullptr)
     {
-      symtab_obj = stpy_objfile_data_key.get (symtab->compunit ()->objfile ());
-      while (symtab_obj != nullptr)
-	{
-	  if (symtab_obj->symtab == symtab)
-	    {
-	      Py_INCREF (symtab_obj);
-	      return (PyObject*)symtab_obj;
-	    }
-	  symtab_obj = symtab_obj->next;
-	}
+      symtab_obj = stpy_registry.lookup (symtab->compunit ()->objfile (),
+					 symtab);
+      if (symtab_obj != nullptr)
+        return (PyObject*)symtab_obj;
     }
 
   symtab_obj = PyObject_New (symtab_object, &symtab_object_type);
@@ -545,7 +487,7 @@ PyTypeObject symtab_object_type = {
   "gdb.Symtab",			  /*tp_name*/
   sizeof (symtab_object),	  /*tp_basicsize*/
   0,				  /*tp_itemsize*/
-  stpy_dealloc,			  /*tp_dealloc*/
+  0,			          /*tp_dealloc*/
   0,				  /*tp_print*/
   0,				  /*tp_getattr*/
   0,				  /*tp_setattr*/
-- 
2.45.2

[RFC 9/9] gdb/python: convert gdb.Symtab_and_line to use gdbpy_registry

[Jan Vrany]

This commit converts gdb.Symtab_and_line to use gdbpy_registry for
lifecycle management.
---
 gdb/python/py-symtab.c | 47 ++++++++++--------------------------------
 1 file changed, 11 insertions(+), 36 deletions(-)

diff --git a/gdb/python/py-symtab.c b/gdb/python/py-symtab.c
index 417ad68a155..8749380ed2c 100644
--- a/gdb/python/py-symtab.c
+++ b/gdb/python/py-symtab.c
@@ -65,30 +65,19 @@ struct sal_object {
    data.  All access to obj->sal should be gated by
    SALPY_REQUIRE_VALID which will raise an exception on invalid symbol
    table and line objects.  */
-struct salpy_deleter
+struct salpy_invalidator
 {
   void operator() (sal_object *obj)
   {
-    gdbpy_enter enter_py;
-
-    while (obj)
-      {
-	sal_object *next = obj->next;
-
-	obj->next = nullptr;
-	obj->prev = nullptr;
-	xfree (obj->sal);
-	obj->sal = nullptr;
-
-	obj = next;
-      }
+    xfree (obj->sal);
+    obj->sal = nullptr;
   }
 };
 
 extern PyTypeObject sal_object_type
     CPYCHECKER_TYPE_OBJECT_FOR_TYPEDEF ("sal_object");
-static const registry<objfile>::key<sal_object, salpy_deleter>
-     salpy_objfile_data_key;
+static const gdbpy_registry<gdbpy_tracking_registry_storage<sal_object, symtab_and_line, &sal_object::sal, salpy_invalidator>>
+     salpy_registry;
 
 /* Require a valid symbol table and line object.  All access to
    sal_object->sal should be gated by this call.  */
@@ -322,15 +311,9 @@ salpy_dealloc (PyObject *self)
 {
   sal_object *self_sal = (sal_object *) self;
 
-  if (self_sal->prev)
-    self_sal->prev->next = self_sal->next;
-  else if (self_sal->sal != nullptr && self_sal->sal->symtab != nullptr)
-    salpy_objfile_data_key.set
-      (self_sal->sal->symtab->compunit ()->objfile (),
-       self_sal->next);
-
-  if (self_sal->next)
-    self_sal->next->prev = self_sal->prev;
+  if (self_sal->sal != nullptr && self_sal->sal->symtab != nullptr)
+    salpy_registry.remove (self_sal->sal->symtab->compunit ()->objfile (),
+			   self_sal);
 
   xfree (self_sal->sal);
   Py_TYPE (self)->tp_free (self);
@@ -347,22 +330,14 @@ set_sal (sal_object *sal_obj, struct symtab_and_line sal)
   sal_obj->sal = ((struct symtab_and_line *)
 		  xmemdup (&sal, sizeof (struct symtab_and_line),
 			   sizeof (struct symtab_and_line)));
-  sal_obj->prev = NULL;
+  sal_obj->prev = nullptr;
+  sal_obj->next = nullptr;
 
   /* If the SAL does not have a symtab, we do not add it to the
      objfile cleanup observer linked list.  */
   symtab *symtab = sal_obj->sal->symtab;
   if (symtab != nullptr)
-    {
-      sal_obj->next
-	= salpy_objfile_data_key.get (symtab->compunit ()->objfile ());
-      if (sal_obj->next)
-	sal_obj->next->prev = sal_obj;
-
-      salpy_objfile_data_key.set (symtab->compunit ()->objfile (), sal_obj);
-    }
-  else
-    sal_obj->next = NULL;
+    salpy_registry.add (symtab->compunit ()->objfile (), sal_obj);
 }
 
 /* Given a symtab, and a symtab_object that has previously been
-- 
2.45.2

[PING] Re: [RFC 0/9] Attempt to unify Python object's lifecycle

[Jan Vraný]

Polite ping. 

Thanks,
Jan

On Mon, 2025-01-27 at 10:44 +0000, Jan Vrany wrote:
> Hi,
> 
> this RFC is an attempt to systematically address Andrew's
> comment on a patch I submitted earlier [1]. In particular,
> this part:
> 
> > I don't really like the approach taken by this function.  Each time
> > the
> > function is called we get a new gdb.Compunit object created and
> > chained
> > onto the objfile.
> 
> This is in fact true for other objects - gdb.Symtab, gdb.Symbol,
> gdb.Type. Moreover each of these objects have their own copy
> of (largely same) housekeeping code. The gdb.Block used to be
> the same but not long ago it was changed so gdb.Blocks are now
> interned (memoized). From the user point of view, I found it bit
> counter-intuitive.
> 
> My idea was to refactor this housekeeping code into a common class.
> This RFC is result of several iterations. Tested on x86_64-linux.
> 
> First four commits expand existing code to intern (memoize)
> gdb.Symtab, gdb.Symbol and gdb.Type. The rest then introduces
> template classes that implement necessary housekeeping and
> converts gdb.Symtab, gdb.Symbol and gdb.Type to use it.
> It could go further (one can convert gdb.Block too and
> gdb.Value and gdb.Value.type and dynamic_type could be
> further simplified too) but I decided to stop here.
> 
> The main reason is that it turned not to be as simple as
> I thought - there seem to be few differences here and there
> (see [2]). This complicated the housekeeping classes
> (gdbpy_registry and associated "storage"). My C++ is
> is pretty basic so perhaps there's better and simpler way
> of doing it. Another reason is that I was hoping for some code
> reduction in terms of size but looking at the result,
> there's hardly any. On the other hand, the lifecycle management
> is more unified across different Python objects.
> 
> All in all, I'm not sure this is the best approach and worth
> it. By this RFC, I'd like to solicit feedback from experienced
> GDB developers on how to move on.
> 
> Basically I see following options:
> 
> 1) Do not change anything in this area (I'm perfectly
>    happy with that).
> 2) Intern (memoize) Python objects (where it makes sense)
>    but keep the current approach. Basically first four
>    commits of this RFC.
> 3) Continue working on this.
> 
> Thanks,
> 
> Jan
> 
> [1]:
> https://inbox.sourceware.org/gdb-patches/87o70ar34z.fsf@redhat.com/
> [2]:
> https://inbox.sourceware.org/gdb/6c73f834d3f7b545188e1999125e7ae63ae83eab.camel@vrany.io/T/#u
> 
> ---
> 
> Jan Vrany (9):
>   gdb/python: preserve identity for gdb.Symtab objects
>   gdb/python: preserve identity for gdb.Symbol objects
>   gdb/python: do not hold on gdb.Symtab object from
> gdb.Symtab_and_line
>   gdb/python: preserve identity for gdb.Type objects
>   gdb/python: introduce gdbpy_registry
>   gdb/python: convert gdb.Symbol to use gdbpy_registry
>   gdb/python: convert gdb.Type to use gdbpy_registry
>   gdb/python: convert gdb.Symtab to use gdbpy_registry
>   gdb/python: convert gdb.Symtab_and_line to use gdbpy_registry
> 
>  gdb/python/py-symbol.c                 |  75 +++-------
>  gdb/python/py-symtab.c                 | 182 ++++++----------------
>  gdb/python/py-type.c                   |  95 +++++-------
>  gdb/python/python-internal.h           | 200
> +++++++++++++++++++++++++
>  gdb/testsuite/gdb.python/py-arch.exp   |   5 +
>  gdb/testsuite/gdb.python/py-symtab.exp |  28 ++++
>  gdb/testsuite/gdb.python/py-type.exp   |  15 ++
>  7 files changed, 347 insertions(+), 253 deletions(-)
> 

Re: [RFC 0/9] Attempt to unify Python object's lifecycle

[Simon Marchi]

On 2025-01-27 05:44, Jan Vrany wrote:
> Hi,
> 
> this RFC is an attempt to systematically address Andrew's
> comment on a patch I submitted earlier [1]. In particular,
> this part:
> 
>> I don't really like the approach taken by this function.  Each time the
>> function is called we get a new gdb.Compunit object created and chained
>> onto the objfile.
> 
> This is in fact true for other objects - gdb.Symtab, gdb.Symbol,
> gdb.Type. Moreover each of these objects have their own copy
> of (largely same) housekeeping code. The gdb.Block used to be
> the same but not long ago it was changed so gdb.Blocks are now
> interned (memoized). From the user point of view, I found it bit
> counter-intuitive.
> 
> My idea was to refactor this housekeeping code into a common class.
> This RFC is result of several iterations. Tested on x86_64-linux.
> 
> First four commits expand existing code to intern (memoize)
> gdb.Symtab, gdb.Symbol and gdb.Type. The rest then introduces
> template classes that implement necessary housekeeping and
> converts gdb.Symtab, gdb.Symbol and gdb.Type to use it.
> It could go further (one can convert gdb.Block too and
> gdb.Value and gdb.Value.type and dynamic_type could be
> further simplified too) but I decided to stop here.
> 
> The main reason is that it turned not to be as simple as
> I thought - there seem to be few differences here and there
> (see [2]). This complicated the housekeeping classes
> (gdbpy_registry and associated "storage"). My C++ is
> is pretty basic so perhaps there's better and simpler way
> of doing it. Another reason is that I was hoping for some code
> reduction in terms of size but looking at the result,
> there's hardly any. On the other hand, the lifecycle management
> is more unified across different Python objects.
> 
> All in all, I'm not sure this is the best approach and worth
> it. By this RFC, I'd like to solicit feedback from experienced
> GDB developers on how to move on.
> 
> Basically I see following options:
> 
> 1) Do not change anything in this area (I'm perfectly
>    happy with that).
> 2) Intern (memoize) Python objects (where it makes sense)
>    but keep the current approach. Basically first four
>    commits of this RFC.
> 3) Continue working on this.
> 
> Thanks,
> 
> Jan
> 
> [1]: https://inbox.sourceware.org/gdb-patches/87o70ar34z.fsf@redhat.com/
> [2]: https://inbox.sourceware.org/gdb/6c73f834d3f7b545188e1999125e7ae63ae83eab.camel@vrany.io/T/#u

I'm sorry, I don't have any answers for your questions in [2].

I scanned your patch series, and I'm not sure about one thing: once the
user is done with a Python object and releases all their reference, do
we delete the Python object, or do we keep it around for the next time
it might be needed?  I guess both would be valid approaches.

Let me summarize our choices, see if I get it right:

 1. No memoization at all: all calls to, e.g., `block.compunit`, will
 return a new `gdb.CompUnit`.  Once the user releases their references
 on that `gdb.CompUnit`, it is freed.  For some types, we need to track
 the Python objects that are alive (e.g. through the registry you made)
 in order to invalidate them if the backing object disappears.  The
 registry holds no reference to the object.  When the refcount of the
 Python object gets to 0 (the user is done with it), the object gets
 removed from the registry (through the tp_dealloc callback).

 2. Temporary memoization: a first call to `block.compunit` will return
 a new `gdb.CompUnit`, which gets saved in the per-objfile registry.  If
 the user requests the same comp unit, we can return the same Python
 object.  The registry still doesn't hold a reference to the Python
 object.  Like for the previous case, when the user is done with the
 object, we remove it from our registry (through the tp_dealloc
 callback).  If the user requests a Python object for the same comp unit
 later, a new Python object will be created.

 3. Permanent memoization: like temporary memoization, except that the
 registry hold a reference to the Python object.  When the user drops
 all their references, the registry keeps the Python alive, such that if
 they request a Python object for the same comp unit later, they will
 get the same Python object as before.  The Python objects only get
 deleted when either the owning objfile gets deleted or the owning arch
 gets deleted (for the former, it means the objects will exist until the
 end of the process, as arches don't get deleted).

With approach 1, for the objects we need to track (to invalidate
eventually) we certainly want to forget about Python objects that the
user is done with, and not hold onto them forever (otherwise, that would
be like a memory leak).  If the user decides to keep alive 50000 Python
objects wrapping the same comp unit, then we need to track them all.  If
one of those 50000 objects gets deleted (its refcount drops to 0), we
need to find it in the linked list of at least 50000 items to remove it.
That sounds bad.  So for the objects that we need to track in order to
invalidate if needed, I guess it is easy enough and makes sense to do
some memoization.

And there is value in being consistent and predictable, so if we do some
memoization for some objects, we might as well do it for all objects
(especially if it can easily be abstracted away).

Between 2 and 3, again, it is a memory vs time tradeoff.  If you are
looping over all symbols, get the CU from the symbol to check something
and then throw the CU away, do you prefer to create/delete a Python
object every time?  If not, that means keeping some Python objects
around "forever" (until the objfile gets delete, or really forever in
the case of arch-owned objects), that may never get used again.
Intuitively I'd say I'm slightly in favor of just doing temporary
memoization, because I'm hoping that allocating/initializing/deleting a
Python object is fast enough, and I don't really like the idea of
keeping unused things around, but benchmarks / profiling would help.

Overall, I would say that I think memoization is a good idea and I like
where you're going with this series.

Simon

Re: [RFC 0/9] Attempt to unify Python object's lifecycle

[Jan Vraný]

On Wed, 2025-02-19 at 16:00 -0500, Simon Marchi wrote:
> 
> 
> On 2025-01-27 05:44, Jan Vrany wrote:
> > Hi,
> > 
> > this RFC is an attempt to systematically address Andrew's
> > comment on a patch I submitted earlier [1]. In particular,
> > this part:
> > 
> > > I don't really like the approach taken by this function.  Each
> > > time the
> > > function is called we get a new gdb.Compunit object created and
> > > chained
> > > onto the objfile.
> > 
> > This is in fact true for other objects - gdb.Symtab, gdb.Symbol,
> > gdb.Type. Moreover each of these objects have their own copy
> > of (largely same) housekeeping code. The gdb.Block used to be
> > the same but not long ago it was changed so gdb.Blocks are now
> > interned (memoized). From the user point of view, I found it bit
> > counter-intuitive.
> > 
> > My idea was to refactor this housekeeping code into a common class.
> > This RFC is result of several iterations. Tested on x86_64-linux.
> > 
> > First four commits expand existing code to intern (memoize)
> > gdb.Symtab, gdb.Symbol and gdb.Type. The rest then introduces
> > template classes that implement necessary housekeeping and
> > converts gdb.Symtab, gdb.Symbol and gdb.Type to use it.
> > It could go further (one can convert gdb.Block too and
> > gdb.Value and gdb.Value.type and dynamic_type could be
> > further simplified too) but I decided to stop here.
> > 
> > The main reason is that it turned not to be as simple as
> > I thought - there seem to be few differences here and there
> > (see [2]). This complicated the housekeeping classes
> > (gdbpy_registry and associated "storage"). My C++ is
> > is pretty basic so perhaps there's better and simpler way
> > of doing it. Another reason is that I was hoping for some code
> > reduction in terms of size but looking at the result,
> > there's hardly any. On the other hand, the lifecycle management
> > is more unified across different Python objects.
> > 
> > All in all, I'm not sure this is the best approach and worth
> > it. By this RFC, I'd like to solicit feedback from experienced
> > GDB developers on how to move on.
> > 
> > Basically I see following options:
> > 
> > 1) Do not change anything in this area (I'm perfectly
> >    happy with that).
> > 2) Intern (memoize) Python objects (where it makes sense)
> >    but keep the current approach. Basically first four
> >    commits of this RFC.
> > 3) Continue working on this.
> > 
> > Thanks,
> > 
> > Jan
> > 
> > [1]:
> > https://inbox.sourceware.org/gdb-patches/87o70ar34z.fsf@redhat.com
> > [2]:
> > https://inbox.sourceware.org/gdb/6c73f834d3f7b545188e1999125e7ae63ae83eab.camel@vrany.io/T/#u
> 
> I'm sorry, I don't have any answers for your questions in [2].
> 
> I scanned your patch series, and I'm not sure about one thing: once
> the
> user is done with a Python object and releases all their reference,
> do
> we delete the Python object, or do we keep it around for the next
> time
> it might be needed?  I guess both would be valid approaches.

In the series I have posted Python objects are kept around and
destroyed only when owner is gone. So it implements choice 3
from choices below. 

> 
> Let me summarize our choices, see if I get it right:
> 
>  1. No memoization at all: all calls to, e.g., `block.compunit`, will
>  return a new `gdb.CompUnit`.  Once the user releases their
> references
>  on that `gdb.CompUnit`, it is freed.  For some types, we need to
> track
>  the Python objects that are alive (e.g. through the registry you
> made)
>  in order to invalidate them if the backing object disappears.  The
>  registry holds no reference to the object.  When the refcount of the
>  Python object gets to 0 (the user is done with it), the object gets
>  removed from the registry (through the tp_dealloc callback).
> 
>  2. Temporary memoization: a first call to `block.compunit` will
> return
>  a new `gdb.CompUnit`, which gets saved in the per-objfile registry. 
> If
>  the user requests the same comp unit, we can return the same Python
>  object.  The registry still doesn't hold a reference to the Python
>  object.  Like for the previous case, when the user is done with the
>  object, we remove it from our registry (through the tp_dealloc
>  callback).  If the user requests a Python object for the same comp
> unit
>  later, a new Python object will be created.
> 
>  3. Permanent memoization: like temporary memoization, except that
> the
>  registry hold a reference to the Python object.  When the user drops
>  all their references, the registry keeps the Python alive, such that
> if
>  they request a Python object for the same comp unit later, they will
>  get the same Python object as before.  The Python objects only get
>  deleted when either the owning objfile gets deleted or the owning
> arch
>  gets deleted (for the former, it means the objects will exist until
> the
>  end of the process, as arches don't get deleted).
> 
> With approach 1, for the objects we need to track (to invalidate
> eventually) we certainly want to forget about Python objects that the
> user is done with, and not hold onto them forever (otherwise, that
> would
> be like a memory leak).  If the user decides to keep alive 50000
> Python
> objects wrapping the same comp unit, then we need to track them all. 
> If
> one of those 50000 objects gets deleted (its refcount drops to 0), we
> need to find it in the linked list of at least 50000 items to remove
> it.
> That sounds bad.  

The way it is currently implemented, it is okay. Python objects
themselves form the doubly-linked list so one can remove it from
the list easily - there's no need to "find" it. 

> So for the objects that we need to track in order to
> invalidate if needed, I guess it is easy enough and makes sense to do
> some memoization.
> 
> And there is value in being consistent and predictable, so if we do
> some
> memoization for some objects, we might as well do it for all objects
> (especially if it can easily be abstracted away).
> 
> Between 2 and 3, again, it is a memory vs time tradeoff.  If you are
> looping over all symbols, get the CU from the symbol to check
> something
> and then throw the CU away, do you prefer to create/delete a Python
> object every time?  If not, that means keeping some Python objects
> around "forever" (until the objfile gets delete, or really forever in
> the case of arch-owned objects), that may never get used again.
> Intuitively I'd say I'm slightly in favor of just doing temporary
> memoization, because I'm hoping that allocating/initializing/deleting
> a
> Python object is fast enough, and I don't really like the idea of
> keeping unused things around, but benchmarks / profiling would help.

I have no strong preference here. gdb.Block currently implements
temporary memoization (2). My code implements permament memoization (3)
mainly because this is what Andrew suggested (if I understood  
correctly). 

I'll try to adapt the code to do temporary memoization to see how it
feels. 

> 
> Overall, I would say that I think memoization is a good idea and I
> like
> where you're going with this series.
> 

Thanks,
Jan

Re: [RFC 0/9] Attempt to unify Python object's lifecycle

[Tom Tromey]

>>>>> "Jan" == Jan Vrany <jan.vrany@labware.com> writes:

Jan> My idea was to refactor this housekeeping code into a common class.

Thanks for doing this.

Jan> It could go further (one can convert gdb.Block too and
Jan> gdb.Value and gdb.Value.type and dynamic_type could be
Jan> further simplified too) but I decided to stop here.

I think gdb.Value might be harder as it has some special requirements.
In particular there is code in there to ensure that using the gdb.Value
API doesn't cause too much memory retention -- the "temporary" strategy
that's discussed in another sub-thread.  Also the underlying "struct
value" memory management approach is odd.

Jan> All in all, I'm not sure this is the best approach and worth
Jan> it. By this RFC, I'd like to solicit feedback from experienced
Jan> GDB developers on how to move on.

Jan> Basically I see following options:

Jan> 1) Do not change anything in this area (I'm perfectly
Jan>    happy with that).
Jan> 2) Intern (memoize) Python objects (where it makes sense)
Jan>    but keep the current approach. Basically first four
Jan>    commits of this RFC.
Jan> 3) Continue working on this.


I think #3.

I'll send a couple small comments.

Tom

Re: [RFC 2/9] gdb/python: preserve identity for gdb.Symbol objects

[Tom Tromey]

>>>>> "Jan" == Jan Vrany <jan.vrany@labware.com> writes:

Jan> This commit changes symbol_to_symbol_object() so that each it is called
Jan> with a particular struct symbol * it returns the very same gdb.Symbol
Jan> object.

Jan> +      /* Can it really happen that symbol->symtab () is NULL?  */

FWIW this is a classic sort of question in gdb and perhaps no one knows
the answer.

Jan> +	  return (PyObject*)sym_obj;

Spaces.

Jan> +      else
Jan> +	{
Jan> +	  sympy_gdbarch_data_key.set (sym_obj->symbol->arch (),
Jan> +				      sym_obj->next);
Jan> +	}

Over-bracing.

Tom

Re: [RFC 5/9] gdb/python: introduce gdbpy_registry

[Tom Tromey]

>>>>> "Jan" == Jan Vrany <jan.vrany@labware.com> writes:

Jan> +#include <unordered_set>
Jan> +#include <unordered_map>

Simon recently imported a hash table so we use gdb::unordered_map and
gdb::unordered_set now.  You can probably just remove the includes and
change the names and things will keep working.

Jan> +   The actuall tracking of "owned" Python objects is handled externally

Small typo, s/actuall/actual/

Jan> +  /* Lookup pre-existing Python object for given VAL.  Return such object
Jan> +     if found, otherwise return NULL.  This method always returns new
Jan> +     reference.  */
Jan> +  template <typename O>
Jan> +  obj_type *lookup (O *owner, val_type *val) const
Jan> +  {

Returning a new reference suggests that perhaps the return type should
be gdbpy_ref<> (or gdbpy_ref<obj_type>).

Jan> +  Storage *get_storage(struct objfile* objf) const
Jan> +  {
Jan> +    return get_storage(objf, m_key_for_objf);

Spaces before "(".  There's a few of these around.

Jan> +  obj_type *lookup (val_type *val) const
Jan> +  {
Jan> +    gdb_assert_not_reached ("Should not be used.");
Jan> +  }

I wonder if these methods can just be deleted?
Like if the registry's 'lookup' template isn't instantiated, perhaps
this function won't ever be called, and so no error generated?

Same question for the assert in the 'remove' method.

Tom