[PATCH 0/2] Changes to gdb.Value.__init__

[PATCH 0/2] Changes to gdb.Value.__init__

[Andrew Burgess]

On IRC a user was asking about inheriting from gdb.Value, and how this
was not working as expected.

After looking at the code the problems seem to be caused by the way
GDB implemented gdb.Value.__new__, but not gdb.Value.__init__.

This series of commits addresses this.  If you are not trying to
inherit from gdb.Value, or to reinitialize a gdb.Value then I don't
think you should notice any changes after this series.

All feedback welcome.

Thanks,
Andrew

---

Andrew Burgess (2):
  gdb/python: replace linked list with unordered_set
  gdb/python: Use tp_init instead of tp_new to setup gdb.Value

 gdb/doc/python.texi                   |   3 +
 gdb/python/py-value.c                 | 121 ++++++++++++++------------
 gdb/testsuite/gdb.python/py-value.exp |  59 +++++++++++--
 3 files changed, 120 insertions(+), 63 deletions(-)

-- 
2.25.4

[PATCH 1/2] gdb/python: replace linked list with unordered_set

[Andrew Burgess]

The gdb.Value objects are stored in a linked list so that we can
implement gdbpy_preserve_values which needs to iterate over each
value.

This commit replaces the linked list with an unordered_set; this data
structure has the characteristics that we need, it holds all the
objects, allows us to iterate over them, but we really don't care
about the order.

The only small concern I have is that I'm removing this code:

  /* List of all values which are currently exposed to Python. It is
     maintained so that when an objfile is discarded, preserve_values
     can copy the values' types if needed.  */
  /* This variable is unnecessarily initialized to NULL in order to
     work around a linker bug on MacOS.  */
  static value_object *values_in_python = NULL;

I wonder if the bug about MacOS is relevant.  This originates from
this mailing list thread:

  https://sourceware.org/pipermail/gdb-patches/2008-December/062118.html

But I think GDB now uses static global C++ containers all over the
place, so I think, if my new:

  static std::unordered_set<value_object *> values_in_python;

Doesn't work, then surely we'll be broken all over the place?

Anyway, I think this simplifies things a little.

There should be no user visible changes after this commit.
---
 gdb/python/py-value.c | 42 +++++++++++++++---------------------------
 1 file changed, 15 insertions(+), 27 deletions(-)

diff --git a/gdb/python/py-value.c b/gdb/python/py-value.c
index c843c2c3072..fa5ab2a08e7 100644
--- a/gdb/python/py-value.c
+++ b/gdb/python/py-value.c
@@ -27,8 +27,8 @@
 #include "expression.h"
 #include "cp-abi.h"
 #include "python.h"
-
 #include "python-internal.h"
+#include <unordered_set>
 
 /* Even though Python scalar types directly map to host types, we use
    target types here to remain consistent with the values system in
@@ -55,20 +55,17 @@
 
 struct value_object {
   PyObject_HEAD
-  struct value_object *next;
-  struct value_object *prev;
   struct value *value;
   PyObject *address;
   PyObject *type;
   PyObject *dynamic_type;
 };
 
-/* List of all values which are currently exposed to Python. It is
-   maintained so that when an objfile is discarded, preserve_values
-   can copy the values' types if needed.  */
-/* This variable is unnecessarily initialized to NULL in order to
-   work around a linker bug on MacOS.  */
-static value_object *values_in_python = NULL;
+/* Set containing all value where are currently exposed in Python.  It is
+   maintained so that when an objfile is discarded, preserve_values can
+   copy the values' types if needed.  */
+
+static std::unordered_set<value_object *> values_in_python;
 
 /* Called by the Python interpreter when deallocating a value object.  */
 static void
@@ -76,23 +73,19 @@ valpy_dealloc (PyObject *obj)
 {
   value_object *self = (value_object *) obj;
 
-  /* Remove SELF from the global list.  */
-  if (self->prev)
-    self->prev->next = self->next;
-  else
-    {
-      gdb_assert (values_in_python == self);
-      values_in_python = self->next;
-    }
-  if (self->next)
-    self->next->prev = self->prev;
+  /* Remove SELF from the global set of values.  */
+  gdb_assert (values_in_python.find (self) != values_in_python.end ());
+  values_in_python.erase (self);
 
+  /* Indicate we are no longer interested in the value object.  */
   value_decref (self->value);
 
+  /* Nor in the cached Python objects.  */
   Py_XDECREF (self->address);
   Py_XDECREF (self->type);
   Py_XDECREF (self->dynamic_type);
 
+  /* Finally, free the memory.  */
   Py_TYPE (self)->tp_free (self);
 }
 
@@ -100,11 +93,8 @@ valpy_dealloc (PyObject *obj)
 static void
 note_value (value_object *value_obj)
 {
-  value_obj->next = values_in_python;
-  if (value_obj->next)
-    value_obj->next->prev = value_obj;
-  value_obj->prev = NULL;
-  values_in_python = value_obj;
+  if (values_in_python.find (value_obj) == values_in_python.end ())
+    values_in_python.insert (value_obj);
 }
 
 /* Convert a python object OBJ with type TYPE to a gdb value.  The
@@ -204,9 +194,7 @@ void
 gdbpy_preserve_values (const struct extension_language_defn *extlang,
 		       struct objfile *objfile, htab_t copied_types)
 {
-  value_object *iter;
-
-  for (iter = values_in_python; iter; iter = iter->next)
+  for (value_object *iter : values_in_python)
     preserve_one_value (iter->value, objfile, copied_types);
 }
 
-- 
2.25.4

[PATCH 2/2] gdb/python: Use tp_init instead of tp_new to setup gdb.Value

[Andrew Burgess]

The documentation suggests that we implement gdb.Value.__init__,
however, this is not currently true, we really implement
gdb.Value.__new__.  This will cause confusion if a user tries to
sub-class gdb.Value.  They might write:

  class MyVal (gdb.Value):
      def __init__ (self, val):
          gdb.Value.__init__(self, val)

  obj = MyVal(123)
  print ("Got: %s" % obj)

But, when they source this code they'll see:

  (gdb) source ~/tmp/value-test.py
  Traceback (most recent call last):
    File "/home/andrew/tmp/value-test.py", line 7, in <module>
      obj = MyVal(123)
    File "/home/andrew/tmp/value-test.py", line 5, in __init__
      gdb.Value.__init__(self, val)
  TypeError: object.__init__() takes exactly one argument (the instance to initialize)
  (gdb)

The reason for this is that, as we don't implement __init__ for
gdb.Value, Python ends up calling object.__init__ instead, which
doesn't expect any arguments.

The Python docs suggest that the reason why we might take this
approach is because we want gdb.Value to be immutable:

   https://docs.python.org/3/c-api/typeobj.html#c.PyTypeObject.tp_new

But I don't see any reason why we should require gdb.Value to be
immutable when other types defined in GDB are not.  This current
immutability can be seen in this code:

  obj = gdb.Value(1234)
  print("Got: %s" % obj)
  obj.__init__ (5678)
  print("Got: %s" % obj)

Which currently runs without error, but prints:

  Got: 1234
  Got: 1234

In this commit I propose that we switch to using __init__ to
initialize gdb.Value objects.  The only extra complexity this
introduces is that, if a value is already associated with a gdb value
object, then we need to decrement the value's refcount so that it can
be released, but otherwise, nothing really changes.

After this commit the first example above works without error, while
the second example now prints:

  Got: 1234
  Got: 5678

In order to make it easier to override the gdb.Value.__init__ method,
I have tweaked the definition of gdb.Value.__init__.  The second,
optional argument to __init__ is a gdb.Type, if this argument is not
present then GDB figures out a suitable type.

However, if we want to override the __init__ method in a sub-class,
and still support the default argument, it is easier to write:

  class MyVal (gdb.Value):
      def __init__ (self, val, type=None):
          gdb.Value.__init__(self, val, type)

Currently, passing None for the Type will result in an error:

  TypeError: type argument must be a gdb.Type.

After this commit I now allow the type argument to be None, in which
case GDB figures out a suitable type just as if the type had not been
passed at all.

Unless a user is trying to reinitialize a value, or create sub-classes
of gdb.Value, there should be no user visible changes after this
commit.
---
 gdb/doc/python.texi                   |  3 +
 gdb/python/py-value.c                 | 87 ++++++++++++++++-----------
 gdb/testsuite/gdb.python/py-value.exp | 59 ++++++++++++++++--
 3 files changed, 109 insertions(+), 40 deletions(-)

diff --git a/gdb/doc/python.texi b/gdb/doc/python.texi
index 9a768133f4c..b9b01665410 100644
--- a/gdb/doc/python.texi
+++ b/gdb/doc/python.texi
@@ -819,6 +819,9 @@
 from the Python buffer object specified by @var{val}.  The number of
 bytes in the Python buffer object must be greater than or equal to the
 size of @var{type}.
+
+If @var{type} is @code{None} then this version of @code{__init__}
+behaves as though @var{type} was not passed at all.
 @end defun
 
 @defun Value.cast (type)
diff --git a/gdb/python/py-value.c b/gdb/python/py-value.c
index fa5ab2a08e7..37d7cfb0775 100644
--- a/gdb/python/py-value.c
+++ b/gdb/python/py-value.c
@@ -67,23 +67,45 @@ struct value_object {
 
 static std::unordered_set<value_object *> values_in_python;
 
-/* Called by the Python interpreter when deallocating a value object.  */
+/* Clear out an old GDB value stored within SELF, and reset the fields to
+   nullptr.  This should be called when a gdb.Value is deallocated, and
+   also if a gdb.Value is reinitialized with a new value.  */
+
 static void
-valpy_dealloc (PyObject *obj)
+valpy_clear_value (value_object *self)
 {
-  value_object *self = (value_object *) obj;
-
-  /* Remove SELF from the global set of values.  */
-  gdb_assert (values_in_python.find (self) != values_in_python.end ());
-  values_in_python.erase (self);
-
   /* Indicate we are no longer interested in the value object.  */
   value_decref (self->value);
+  self->value = nullptr;
 
   /* Nor in the cached Python objects.  */
   Py_XDECREF (self->address);
+  self->address = nullptr;
+
   Py_XDECREF (self->type);
+  self->type = nullptr;
+
   Py_XDECREF (self->dynamic_type);
+  self->dynamic_type = nullptr;
+}
+
+/* Called by the Python interpreter when deallocating a value object.  */
+static void
+valpy_dealloc (PyObject *obj)
+{
+  value_object *self = (value_object *) obj;
+
+  /* If SELF failed to initialize correctly then it may not have a value
+     contained within it.  */
+  if (self->value != nullptr)
+    {
+      /* Remove SELF from the global set of values.  */
+      gdb_assert (values_in_python.find (self) != values_in_python.end ());
+      values_in_python.erase (self);
+
+      /* Release the value object and any cached Python objects.  */
+      valpy_clear_value (self);
+    }
 
   /* Finally, free the memory.  */
   Py_TYPE (self)->tp_free (self);
@@ -132,60 +154,55 @@ convert_buffer_and_type_to_value (PyObject *obj, struct type *type)
   return value_from_contents (type, (const gdb_byte *) py_buf.buf);
 }
 
-/* Called when a new gdb.Value object needs to be allocated.  Returns NULL on
-   error, with a python exception set.  */
-static PyObject *
-valpy_new (PyTypeObject *subtype, PyObject *args, PyObject *kwargs)
+/* Implement gdb.Value.__init__.  */
+
+static int
+valpy_init (PyObject *self, PyObject *args, PyObject *kwds)
 {
   static const char *keywords[] = { "val", "type", NULL };
   PyObject *val_obj = nullptr;
   PyObject *type_obj = nullptr;
 
-  if (!gdb_PyArg_ParseTupleAndKeywords (args, kwargs, "O|O", keywords,
+  if (!gdb_PyArg_ParseTupleAndKeywords (args, kwds, "O|O", keywords,
 					&val_obj, &type_obj))
-    return nullptr;
+    return -1;
 
   struct type *type = nullptr;
-
-  if (type_obj != nullptr)
+  if (type_obj != nullptr && type_obj != Py_None)
     {
       type = type_object_to_type (type_obj);
       if (type == nullptr)
 	{
 	  PyErr_SetString (PyExc_TypeError,
 			   _("type argument must be a gdb.Type."));
-	  return nullptr;
+	  return -1;
 	}
     }
 
-  value_object *value_obj = (value_object *) subtype->tp_alloc (subtype, 1);
-  if (value_obj == NULL)
-    {
-      PyErr_SetString (PyExc_MemoryError, _("Could not allocate memory to "
-					    "create Value object."));
-      return NULL;
-    }
-
   struct value *value;
-
   if (type == nullptr)
     value = convert_value_from_python (val_obj);
   else
     value = convert_buffer_and_type_to_value (val_obj, type);
-
   if (value == nullptr)
     {
-      subtype->tp_free (value_obj);
-      return NULL;
+      gdb_assert (PyErr_Occurred ());
+      return -1;
     }
 
+  /* There might be a previous value here.  */
+  value_object *value_obj = (value_object *) self;
+  if (value_obj->value != nullptr)
+    valpy_clear_value (value_obj);
+
+  /* Store the value into this Python object.  */
   value_obj->value = release_value (value).release ();
-  value_obj->address = NULL;
-  value_obj->type = NULL;
-  value_obj->dynamic_type = NULL;
+
+  /* Ensure that this gdb.Value is in the set of all gdb.Value objects.  If
+     we are already in the set then this is call does nothing.  */
   note_value (value_obj);
 
-  return (PyObject *) value_obj;
+  return 0;
 }
 
 /* Iterate over all the Value objects, calling preserve_one_value on
@@ -2220,7 +2237,7 @@ PyTypeObject value_object_type = {
   0,				  /* tp_descr_get */
   0,				  /* tp_descr_set */
   0,				  /* tp_dictoffset */
-  0,				  /* tp_init */
+  valpy_init,			  /* tp_init */
   0,				  /* tp_alloc */
-  valpy_new			  /* tp_new */
+  PyType_GenericNew,		  /* tp_new */
 };
diff --git a/gdb/testsuite/gdb.python/py-value.exp b/gdb/testsuite/gdb.python/py-value.exp
index f4b7c23fb7d..297b128da47 100644
--- a/gdb/testsuite/gdb.python/py-value.exp
+++ b/gdb/testsuite/gdb.python/py-value.exp
@@ -51,6 +51,7 @@ proc test_value_creation {} {
 
   gdb_py_test_silent_cmd "python i = gdb.Value (True)" "create boolean value" 1
   gdb_py_test_silent_cmd "python i = gdb.Value (5)" "create integer value" 1
+  gdb_py_test_silent_cmd "python i = gdb.Value (3,None)" "create integer value, with None type" 1
   if { $gdb_py_is_py3k == 0 } {
     gdb_py_test_silent_cmd "python i = gdb.Value (5L)" "create long value" 1
   }
@@ -77,6 +78,18 @@ proc test_value_creation {} {
   gdb_test "python print ('result = %s' % i.address)" "= None" "test address attribute in non-addressable value"
 }
 
+# Check that we can call gdb.Value.__init__ to change a value.
+proc test_value_reinit {} {
+    gdb_py_test_silent_cmd "python v = gdb.Value (3)" \
+	"create initial integer value" 1
+    gdb_test "python print(v)" "3" \
+	"check initial value contents"
+    gdb_py_test_silent_cmd "python v.__init__(5)" \
+	"call gdb.Value.__init__ manually" 1
+    gdb_test "python print(v)" "5" \
+	"check new value contents"
+}
+
 proc test_value_numeric_ops {} {
   global gdb_prompt
   global gdb_py_is_py3k
@@ -531,10 +544,14 @@ proc test_float_conversion {} {
     gdb_test "python print(float(gdb.Value(0)))" "0\\.0"
 }
 
-proc test_value_from_buffer {} {
-  global gdb_prompt
-  global gdb_py_is_py3k
-
+# Setup some Python variables:
+#   tp      : a gdb.Type for 'int',
+#   size_a  : the size of array 'a' from the inferior,
+#   size_a0 : the size of array element 'a[0] from the inferior,
+#   addr    : the address of 'a[0]' from the inferior,
+#   b       : a buffer containing the full contents of array 'a' from the
+#             inferior.
+proc prepare_type_and_buffer {} {
   gdb_py_test_silent_cmd "python tp=gdb.lookup_type('int')" "look up int type" 0
   gdb_py_test_silent_cmd "python size_a=gdb.parse_and_eval('sizeof(a)')" \
                          "find size of a" 0
@@ -543,7 +560,14 @@ proc test_value_from_buffer {} {
   gdb_py_test_silent_cmd "python addr=gdb.parse_and_eval('&a')" \
                          "find address of a" 0
   gdb_py_test_silent_cmd "python b=gdb.selected_inferior().read_memory(addr,size_a)" \
-                         "read buffer from memory" 1
+                         "read buffer from memory" 0
+}
+
+proc test_value_from_buffer {} {
+  global gdb_prompt
+  global gdb_py_is_py3k
+
+  prepare_type_and_buffer
   gdb_test "python v=gdb.Value(b,tp); print(v)" "1" \
             "construct value from buffer"
   gdb_test "python v=gdb.Value(b\[size_a0:\],tp); print(v)" "2" \
@@ -600,6 +624,29 @@ proc test_add_to_history {} {
 	"TypeError: Could not convert Python object: .*"
 }
 
+# Check we can create sub-classes of gdb.Value.
+proc test_value_sub_classes {} {
+    prepare_type_and_buffer
+
+    gdb_test_multiline "Create sub-class of gdb.Value" \
+	"python" "" \
+	"class MyValue(gdb.Value):" "" \
+	"  def __init__(self,val,type=None):" "" \
+	"    gdb.Value.__init__(self,val,type)" "" \
+	"    print(\"In MyValue.__init__\")" "" \
+	"end"
+
+    gdb_test "python obj = MyValue (123)" "In MyValue.__init__" \
+	"create instance of MyValue"
+    gdb_test "python print(obj)" "123" \
+	"check printing of MyValue"
+
+    gdb_test "python obj = MyValue(b\[size_a0:\],tp)" "In MyValue.__init__" \
+	"convert 2nd elem of buffer to a MyValue"
+    gdb_test "python print(obj)" "2" \
+	"check printing of MyValue when initiaized with a type"
+}
+
 # Build C version of executable.  C++ is built later.
 if { [build_inferior "${binfile}" "c"] < 0 } {
     return -1
@@ -612,6 +659,7 @@ clean_restart ${binfile}
 if { [skip_python_tests] } { continue }
 
 test_value_creation
+test_value_reinit
 test_value_numeric_ops
 test_value_boolean
 test_value_compare
@@ -629,6 +677,7 @@ if ![runto_main] then {
 
 test_value_in_inferior
 test_value_from_buffer
+test_value_sub_classes
 test_inferior_function_call
 test_value_after_death
 
-- 
2.25.4

Re: [PATCH 1/2] gdb/python: replace linked list with unordered_set

[Lancelot SIX]

On Wed, Nov 17, 2021 at 02:04:25PM +0000, Andrew Burgess via Gdb-patches wrote:
> The gdb.Value objects are stored in a linked list so that we can
> implement gdbpy_preserve_values which needs to iterate over each
> value.
> 
> This commit replaces the linked list with an unordered_set; this data
> structure has the characteristics that we need, it holds all the
> objects, allows us to iterate over them, but we really don't care
> about the order.
> 
> The only small concern I have is that I'm removing this code:
> 
>   /* List of all values which are currently exposed to Python. It is
>      maintained so that when an objfile is discarded, preserve_values
>      can copy the values' types if needed.  */
>   /* This variable is unnecessarily initialized to NULL in order to
>      work around a linker bug on MacOS.  */
>   static value_object *values_in_python = NULL;
> 
> I wonder if the bug about MacOS is relevant.  This originates from
> this mailing list thread:
> 
>   https://sourceware.org/pipermail/gdb-patches/2008-December/062118.html
> 
> But I think GDB now uses static global C++ containers all over the
> place, so I think, if my new:
> 
>   static std::unordered_set<value_object *> values_in_python;
> 
> Doesn't work, then surely we'll be broken all over the place?
> 
> Anyway, I think this simplifies things a little.
> 
> There should be no user visible changes after this commit.
> ---
>  gdb/python/py-value.c | 42 +++++++++++++++---------------------------
>  1 file changed, 15 insertions(+), 27 deletions(-)
> 
> diff --git a/gdb/python/py-value.c b/gdb/python/py-value.c
> index c843c2c3072..fa5ab2a08e7 100644
> --- a/gdb/python/py-value.c
> +++ b/gdb/python/py-value.c
> @@ -27,8 +27,8 @@
>  #include "expression.h"
>  #include "cp-abi.h"
>  #include "python.h"
> -
>  #include "python-internal.h"
> +#include <unordered_set>
>  
>  /* Even though Python scalar types directly map to host types, we use
>     target types here to remain consistent with the values system in
> @@ -55,20 +55,17 @@
>  
>  struct value_object {
>    PyObject_HEAD
> -  struct value_object *next;
> -  struct value_object *prev;
>    struct value *value;
>    PyObject *address;
>    PyObject *type;
>    PyObject *dynamic_type;
>  };
>  
> -/* List of all values which are currently exposed to Python. It is
> -   maintained so that when an objfile is discarded, preserve_values
> -   can copy the values' types if needed.  */
> -/* This variable is unnecessarily initialized to NULL in order to
> -   work around a linker bug on MacOS.  */
> -static value_object *values_in_python = NULL;
> +/* Set containing all value where are currently exposed in Python.  It is
                               ^^^^^
Hi,

Looks like a typo slipped in when you re-arranged this comment.  Did you
mean 'which' instead of 'where'?

Best,
Lancelot.

> +   maintained so that when an objfile is discarded, preserve_values can
> +   copy the values' types if needed.  */
> +
> +static std::unordered_set<value_object *> values_in_python;
>  
>  /* Called by the Python interpreter when deallocating a value object.  */
>  static void
> @@ -76,23 +73,19 @@ valpy_dealloc (PyObject *obj)
>  {
>    value_object *self = (value_object *) obj;
>  
> -  /* Remove SELF from the global list.  */
> -  if (self->prev)
> -    self->prev->next = self->next;
> -  else
> -    {
> -      gdb_assert (values_in_python == self);
> -      values_in_python = self->next;
> -    }
> -  if (self->next)
> -    self->next->prev = self->prev;
> +  /* Remove SELF from the global set of values.  */
> +  gdb_assert (values_in_python.find (self) != values_in_python.end ());
> +  values_in_python.erase (self);
>  
> +  /* Indicate we are no longer interested in the value object.  */
>    value_decref (self->value);
>  
> +  /* Nor in the cached Python objects.  */
>    Py_XDECREF (self->address);
>    Py_XDECREF (self->type);
>    Py_XDECREF (self->dynamic_type);
>  
> +  /* Finally, free the memory.  */
>    Py_TYPE (self)->tp_free (self);
>  }
>  
> @@ -100,11 +93,8 @@ valpy_dealloc (PyObject *obj)
>  static void
>  note_value (value_object *value_obj)
>  {
> -  value_obj->next = values_in_python;
> -  if (value_obj->next)
> -    value_obj->next->prev = value_obj;
> -  value_obj->prev = NULL;
> -  values_in_python = value_obj;
> +  if (values_in_python.find (value_obj) == values_in_python.end ())
> +    values_in_python.insert (value_obj);
>  }
>  
>  /* Convert a python object OBJ with type TYPE to a gdb value.  The
> @@ -204,9 +194,7 @@ void
>  gdbpy_preserve_values (const struct extension_language_defn *extlang,
>  		       struct objfile *objfile, htab_t copied_types)
>  {
> -  value_object *iter;
> -
> -  for (iter = values_in_python; iter; iter = iter->next)
> +  for (value_object *iter : values_in_python)
>      preserve_one_value (iter->value, objfile, copied_types);
>  }
>  
> -- 
> 2.25.4
> 

Re: [PATCH 1/2] gdb/python: replace linked list with unordered_set

[Simon Marchi]

On 2021-11-17 9:04 a.m., Andrew Burgess via Gdb-patches wrote:
> The gdb.Value objects are stored in a linked list so that we can
> implement gdbpy_preserve_values which needs to iterate over each
> value.
> 
> This commit replaces the linked list with an unordered_set; this data
> structure has the characteristics that we need, it holds all the
> objects, allows us to iterate over them, but we really don't care
> about the order.
> 
> The only small concern I have is that I'm removing this code:
> 
>   /* List of all values which are currently exposed to Python. It is
>      maintained so that when an objfile is discarded, preserve_values
>      can copy the values' types if needed.  */
>   /* This variable is unnecessarily initialized to NULL in order to
>      work around a linker bug on MacOS.  */
>   static value_object *values_in_python = NULL;
> 
> I wonder if the bug about MacOS is relevant.  This originates from
> this mailing list thread:
> 
>   https://sourceware.org/pipermail/gdb-patches/2008-December/062118.html
> 
> But I think GDB now uses static global C++ containers all over the
> place, so I think, if my new:
> 
>   static std::unordered_set<value_object *> values_in_python;
> 
> Doesn't work, then surely we'll be broken all over the place?
> 
> Anyway, I think this simplifies things a little.
> 
> There should be no user visible changes after this commit.

An alternative with less overhead than an unordered_set but still fits
the requirements (I think) would be to make the list an intrusive_list.
I keeps the existing data structure, but makes it more friendly to use.

Given that value_object is allocated by Python, and I think we want to
keep PyObject_HEAD as the first member of value_object, then we would
use a member list node (as opposed to a base list node).  So
value_object would become:

struct value_object
{
  PyObject_HEAD
  intrusive_list_node<value_object> node;
  struct value *value;
  PyObject *address;
  PyObject *type;
  PyObject *dynamic_type;
};

values_in_python would be declared as:

using value_object_list_node
  = intrusive_member_node<value_object, &value_object::node>;
using value_object_list = intrusive_list<value_object, value_object_list_node>;
static value_object_list values_in_python;

The uses of values_in_python should be mostly trivial to update (and simpler).

Simon

Re: [PATCH 1/2] gdb/python: replace linked list with unordered_set

[Andrew Burgess]

* Simon Marchi <simark@simark.ca> [2021-11-17 12:05:21 -0500]:

> On 2021-11-17 9:04 a.m., Andrew Burgess via Gdb-patches wrote:
> > The gdb.Value objects are stored in a linked list so that we can
> > implement gdbpy_preserve_values which needs to iterate over each
> > value.
> > 
> > This commit replaces the linked list with an unordered_set; this data
> > structure has the characteristics that we need, it holds all the
> > objects, allows us to iterate over them, but we really don't care
> > about the order.
> > 
> > The only small concern I have is that I'm removing this code:
> > 
> >   /* List of all values which are currently exposed to Python. It is
> >      maintained so that when an objfile is discarded, preserve_values
> >      can copy the values' types if needed.  */
> >   /* This variable is unnecessarily initialized to NULL in order to
> >      work around a linker bug on MacOS.  */
> >   static value_object *values_in_python = NULL;
> > 
> > I wonder if the bug about MacOS is relevant.  This originates from
> > this mailing list thread:
> > 
> >   https://sourceware.org/pipermail/gdb-patches/2008-December/062118.html
> > 
> > But I think GDB now uses static global C++ containers all over the
> > place, so I think, if my new:
> > 
> >   static std::unordered_set<value_object *> values_in_python;
> > 
> > Doesn't work, then surely we'll be broken all over the place?
> > 
> > Anyway, I think this simplifies things a little.
> > 
> > There should be no user visible changes after this commit.
> 
> An alternative with less overhead than an unordered_set but still fits
> the requirements (I think) would be to make the list an intrusive_list.
> I keeps the existing data structure, but makes it more friendly to
> use.

I'll rewrite this patch as you suggest, OOI, is there one aspect of
the overhead that concerns you the most?  I might be able to collect
some performance data...

Thanks,
Andrew


> 
> Given that value_object is allocated by Python, and I think we want to
> keep PyObject_HEAD as the first member of value_object, then we would
> use a member list node (as opposed to a base list node).  So
> value_object would become:
> 
> struct value_object
> {
>   PyObject_HEAD
>   intrusive_list_node<value_object> node;
>   struct value *value;
>   PyObject *address;
>   PyObject *type;
>   PyObject *dynamic_type;
> };
> 
> values_in_python would be declared as:
> 
> using value_object_list_node
>   = intrusive_member_node<value_object, &value_object::node>;
> using value_object_list = intrusive_list<value_object, value_object_list_node>;
> static value_object_list values_in_python;
> 
> The uses of values_in_python should be mostly trivial to update (and simpler).
> 
> Simon

Re: [PATCH 1/2] gdb/python: replace linked list with unordered_set

[Tom Tromey]

Andrew> The only small concern I have is that I'm removing this code:
...
Andrew> But I think GDB now uses static global C++ containers all over the
Andrew> place, so I think, if my new:
Andrew>   static std::unordered_set<value_object *> values_in_python;
Andrew> Doesn't work, then surely we'll be broken all over the place?

Yeah.  I wouldn't worry about this.

Tom

Re: [PATCH 2/2] gdb/python: Use tp_init instead of tp_new to setup gdb.Value

[Tom Tromey]

>>>>> "Andrew" == Andrew Burgess via Gdb-patches <gdb-patches@sourceware.org> writes:

Andrew> The documentation suggests that we implement gdb.Value.__init__,
Andrew> however, this is not currently true, we really implement
Andrew> gdb.Value.__new__.  This will cause confusion if a user tries to
Andrew> sub-class gdb.Value.

This all makes sense to me FWIW.

Andrew>    /* Nor in the cached Python objects.  */
Andrew>    Py_XDECREF (self->address);
Andrew> +  self->address = nullptr;
Andrew> +
Andrew>    Py_XDECREF (self->type);
Andrew> +  self->type = nullptr;
Andrew> +
Andrew>    Py_XDECREF (self->dynamic_type);
Andrew> +  self->dynamic_type = nullptr;

I don't think we use it much in gdb, but the Py_XDECREF + assign NULL
combo is Py_CLEAR.

https://docs.python.org/3/c-api/refcounting.html#c.Py_CLEAR

Tom

[PATCHv2] Changes to gdb.Value.__init__

[Andrew Burgess]

Simon,

Thanks for your feedback on the v1 series.

I tried to take your suggestion, but I don't think it can "just
work".  The intrusive_list_node definition looks like this:

  template<typename T>
  struct intrusive_list_node
  {
    bool is_linked () const
    {
      return next != INTRUSIVE_LIST_UNLINKED_VALUE;
    }

    T *next = INTRUSIVE_LIST_UNLINKED_VALUE;
    T *prev = INTRUSIVE_LIST_UNLINKED_VALUE;
  };

When embedded into a Python type the default values will not be
honoured during object creation, in some cases these fields will be
zero'd and in other cases they are left uninitialised.  We'd have to
manually set these fields ourselves, which, to me, feels worse than
just not using intrusive_list at all - a user seeing that we use
intrusive_list might make assumptions about the behaviour which are
not correct, by using some other data structure these assumptions will
be avoided.

Given your concerns about the overhead of using std::unordered_set
I've just dropped the old first patch.  I did briefly consider
std::list, but finding items for deleting becomes expensive, so I
discarded that.

Thoughts welcome,

Thanks,
Andrew

---

Changes since v1:

  - Dropped the old patch #1, updated patch #2 to stand on its own.

---

commit 249bfca4a5de410e89515abd22a7ae8aba7ba640
Author: Andrew Burgess <aburgess@redhat.com>
Date:   Wed Nov 17 11:55:37 2021 +0000

    gdb/python: Use tp_init instead of tp_new to setup gdb.Value
    
    The documentation suggests that we implement gdb.Value.__init__,
    however, this is not currently true, we really implement
    gdb.Value.__new__.  This will cause confusion if a user tries to
    sub-class gdb.Value.  They might write:
    
      class MyVal (gdb.Value):
          def __init__ (self, val):
              gdb.Value.__init__(self, val)
    
      obj = MyVal(123)
      print ("Got: %s" % obj)
    
    But, when they source this code they'll see:
    
      (gdb) source ~/tmp/value-test.py
      Traceback (most recent call last):
        File "/home/andrew/tmp/value-test.py", line 7, in <module>
          obj = MyVal(123)
        File "/home/andrew/tmp/value-test.py", line 5, in __init__
          gdb.Value.__init__(self, val)
      TypeError: object.__init__() takes exactly one argument (the instance to initialize)
      (gdb)
    
    The reason for this is that, as we don't implement __init__ for
    gdb.Value, Python ends up calling object.__init__ instead, which
    doesn't expect any arguments.
    
    The Python docs suggest that the reason why we might take this
    approach is because we want gdb.Value to be immutable:
    
       https://docs.python.org/3/c-api/typeobj.html#c.PyTypeObject.tp_new
    
    But I don't see any reason why we should require gdb.Value to be
    immutable when other types defined in GDB are not.  This current
    immutability can be seen in this code:
    
      obj = gdb.Value(1234)
      print("Got: %s" % obj)
      obj.__init__ (5678)
      print("Got: %s" % obj)
    
    Which currently runs without error, but prints:
    
      Got: 1234
      Got: 1234
    
    In this commit I propose that we switch to using __init__ to
    initialize gdb.Value objects.
    
    This does introduce some additional complexity, during the __init__
    call a gdb.Value might already be associated with a gdb value object,
    in which case we need to cleanly break that association before
    installing the new gdb value object.  However, the cost of doing this
    is not great, and the benefit - being able to easily sub-class
    gdb.Value seems worth it.
    
    After this commit the first example above works without error, while
    the second example now prints:
    
      Got: 1234
      Got: 5678
    
    In order to make it easier to override the gdb.Value.__init__ method,
    I have tweaked the definition of gdb.Value.__init__.  The second,
    optional argument to __init__ is a gdb.Type, if this argument is not
    present then GDB figures out a suitable type.
    
    However, if we want to override the __init__ method in a sub-class,
    and still support the default argument, it is easier to write:
    
      class MyVal (gdb.Value):
          def __init__ (self, val, type=None):
              gdb.Value.__init__(self, val, type)
    
    Currently, passing None for the Type will result in an error:
    
      TypeError: type argument must be a gdb.Type.
    
    After this commit I now allow the type argument to be None, in which
    case GDB figures out a suitable type just as if the type had not been
    passed at all.
    
    Unless a user is trying to reinitialize a value, or create sub-classes
    of gdb.Value, there should be no user visible changes after this
    commit.

diff --git a/gdb/doc/python.texi b/gdb/doc/python.texi
index 9a768133f4c..b9b01665410 100644
--- a/gdb/doc/python.texi
+++ b/gdb/doc/python.texi
@@ -819,6 +819,9 @@
 from the Python buffer object specified by @var{val}.  The number of
 bytes in the Python buffer object must be greater than or equal to the
 size of @var{type}.
+
+If @var{type} is @code{None} then this version of @code{__init__}
+behaves as though @var{type} was not passed at all.
 @end defun
 
 @defun Value.cast (type)
diff --git a/gdb/python/py-value.c b/gdb/python/py-value.c
index c843c2c3072..a5c43389a80 100644
--- a/gdb/python/py-value.c
+++ b/gdb/python/py-value.c
@@ -70,41 +70,69 @@ struct value_object {
    work around a linker bug on MacOS.  */
 static value_object *values_in_python = NULL;
 
+/* Clear out an old GDB value stored within SELF, and reset the fields to
+   nullptr.  This should be called when a gdb.Value is deallocated, and
+   also if a gdb.Value is reinitialized with a new value.  */
+
+static void
+valpy_clear_value (value_object *self)
+{
+  /* Indicate we are no longer interested in the value object.  */
+  value_decref (self->value);
+  self->value = nullptr;
+
+  Py_XDECREF (self->address);
+  self->address = nullptr;
+
+  Py_XDECREF (self->type);
+  self->type = nullptr;
+
+  Py_XDECREF (self->dynamic_type);
+  self->dynamic_type = nullptr;
+}
+
 /* Called by the Python interpreter when deallocating a value object.  */
 static void
 valpy_dealloc (PyObject *obj)
 {
   value_object *self = (value_object *) obj;
 
-  /* Remove SELF from the global list.  */
-  if (self->prev)
-    self->prev->next = self->next;
-  else
+  /* If SELF failed to initialize correctly then it may not have a value
+     contained within it.  */
+  if (self->value != nullptr)
     {
-      gdb_assert (values_in_python == self);
-      values_in_python = self->next;
-    }
-  if (self->next)
-    self->next->prev = self->prev;
-
-  value_decref (self->value);
+      /* Remove SELF from the global list of values.  */
+      if (self->prev != nullptr)
+	self->prev->next = self->next;
+      else
+	{
+	  gdb_assert (values_in_python == self);
+	  values_in_python = self->next;
+	}
+      if (self->next != nullptr)
+	self->next->prev = self->prev;
 
-  Py_XDECREF (self->address);
-  Py_XDECREF (self->type);
-  Py_XDECREF (self->dynamic_type);
+      /* Release the value object and any cached Python objects.  */
+      valpy_clear_value (self);
+    }
 
   Py_TYPE (self)->tp_free (self);
 }
 
-/* Helper to push a Value object on the global list.  */
+/* Helper to push a gdb.Value object on to the global list of values.  If
+   VALUE_OBJ is already on the lit then this does nothing.  */
+
 static void
 note_value (value_object *value_obj)
 {
-  value_obj->next = values_in_python;
-  if (value_obj->next)
-    value_obj->next->prev = value_obj;
-  value_obj->prev = NULL;
-  values_in_python = value_obj;
+  if (value_obj->next == nullptr)
+    {
+      gdb_assert (value_obj->prev == nullptr);
+      value_obj->next = values_in_python;
+      if (value_obj->next != nullptr)
+	value_obj->next->prev = value_obj;
+      values_in_python = value_obj;
+    }
 }
 
 /* Convert a python object OBJ with type TYPE to a gdb value.  The
@@ -142,60 +170,55 @@ convert_buffer_and_type_to_value (PyObject *obj, struct type *type)
   return value_from_contents (type, (const gdb_byte *) py_buf.buf);
 }
 
-/* Called when a new gdb.Value object needs to be allocated.  Returns NULL on
-   error, with a python exception set.  */
-static PyObject *
-valpy_new (PyTypeObject *subtype, PyObject *args, PyObject *kwargs)
+/* Implement gdb.Value.__init__.  */
+
+static int
+valpy_init (PyObject *self, PyObject *args, PyObject *kwds)
 {
   static const char *keywords[] = { "val", "type", NULL };
   PyObject *val_obj = nullptr;
   PyObject *type_obj = nullptr;
 
-  if (!gdb_PyArg_ParseTupleAndKeywords (args, kwargs, "O|O", keywords,
+  if (!gdb_PyArg_ParseTupleAndKeywords (args, kwds, "O|O", keywords,
 					&val_obj, &type_obj))
-    return nullptr;
+    return -1;
 
   struct type *type = nullptr;
-
-  if (type_obj != nullptr)
+  if (type_obj != nullptr && type_obj != Py_None)
     {
       type = type_object_to_type (type_obj);
       if (type == nullptr)
 	{
 	  PyErr_SetString (PyExc_TypeError,
 			   _("type argument must be a gdb.Type."));
-	  return nullptr;
+	  return -1;
 	}
     }
 
-  value_object *value_obj = (value_object *) subtype->tp_alloc (subtype, 1);
-  if (value_obj == NULL)
-    {
-      PyErr_SetString (PyExc_MemoryError, _("Could not allocate memory to "
-					    "create Value object."));
-      return NULL;
-    }
-
   struct value *value;
-
   if (type == nullptr)
     value = convert_value_from_python (val_obj);
   else
     value = convert_buffer_and_type_to_value (val_obj, type);
-
   if (value == nullptr)
     {
-      subtype->tp_free (value_obj);
-      return NULL;
+      gdb_assert (PyErr_Occurred ());
+      return -1;
     }
 
+  /* There might be a previous value here.  */
+  value_object *value_obj = (value_object *) self;
+  if (value_obj->value != nullptr)
+    valpy_clear_value (value_obj);
+
+  /* Store the value into this Python object.  */
   value_obj->value = release_value (value).release ();
-  value_obj->address = NULL;
-  value_obj->type = NULL;
-  value_obj->dynamic_type = NULL;
+
+  /* Ensure that this gdb.Value is in the set of all gdb.Value objects.  If
+     we are already in the set then this is call does nothing.  */
   note_value (value_obj);
 
-  return (PyObject *) value_obj;
+  return 0;
 }
 
 /* Iterate over all the Value objects, calling preserve_one_value on
@@ -1784,6 +1807,8 @@ value_to_value_object (struct value *val)
   if (val_obj != NULL)
     {
       val_obj->value = release_value (val).release ();
+      val_obj->next = nullptr;
+      val_obj->prev = nullptr;
       val_obj->address = NULL;
       val_obj->type = NULL;
       val_obj->dynamic_type = NULL;
@@ -1805,6 +1830,8 @@ value_to_value_object_no_release (struct value *val)
     {
       value_incref (val);
       val_obj->value = val;
+      val_obj->next = nullptr;
+      val_obj->prev = nullptr;
       val_obj->address = NULL;
       val_obj->type = NULL;
       val_obj->dynamic_type = NULL;
@@ -2232,7 +2259,7 @@ PyTypeObject value_object_type = {
   0,				  /* tp_descr_get */
   0,				  /* tp_descr_set */
   0,				  /* tp_dictoffset */
-  0,				  /* tp_init */
+  valpy_init,			  /* tp_init */
   0,				  /* tp_alloc */
-  valpy_new			  /* tp_new */
+  PyType_GenericNew,		  /* tp_new */
 };
diff --git a/gdb/testsuite/gdb.python/py-value.exp b/gdb/testsuite/gdb.python/py-value.exp
index f4b7c23fb7d..297b128da47 100644
--- a/gdb/testsuite/gdb.python/py-value.exp
+++ b/gdb/testsuite/gdb.python/py-value.exp
@@ -51,6 +51,7 @@ proc test_value_creation {} {
 
   gdb_py_test_silent_cmd "python i = gdb.Value (True)" "create boolean value" 1
   gdb_py_test_silent_cmd "python i = gdb.Value (5)" "create integer value" 1
+  gdb_py_test_silent_cmd "python i = gdb.Value (3,None)" "create integer value, with None type" 1
   if { $gdb_py_is_py3k == 0 } {
     gdb_py_test_silent_cmd "python i = gdb.Value (5L)" "create long value" 1
   }
@@ -77,6 +78,18 @@ proc test_value_creation {} {
   gdb_test "python print ('result = %s' % i.address)" "= None" "test address attribute in non-addressable value"
 }
 
+# Check that we can call gdb.Value.__init__ to change a value.
+proc test_value_reinit {} {
+    gdb_py_test_silent_cmd "python v = gdb.Value (3)" \
+	"create initial integer value" 1
+    gdb_test "python print(v)" "3" \
+	"check initial value contents"
+    gdb_py_test_silent_cmd "python v.__init__(5)" \
+	"call gdb.Value.__init__ manually" 1
+    gdb_test "python print(v)" "5" \
+	"check new value contents"
+}
+
 proc test_value_numeric_ops {} {
   global gdb_prompt
   global gdb_py_is_py3k
@@ -531,10 +544,14 @@ proc test_float_conversion {} {
     gdb_test "python print(float(gdb.Value(0)))" "0\\.0"
 }
 
-proc test_value_from_buffer {} {
-  global gdb_prompt
-  global gdb_py_is_py3k
-
+# Setup some Python variables:
+#   tp      : a gdb.Type for 'int',
+#   size_a  : the size of array 'a' from the inferior,
+#   size_a0 : the size of array element 'a[0] from the inferior,
+#   addr    : the address of 'a[0]' from the inferior,
+#   b       : a buffer containing the full contents of array 'a' from the
+#             inferior.
+proc prepare_type_and_buffer {} {
   gdb_py_test_silent_cmd "python tp=gdb.lookup_type('int')" "look up int type" 0
   gdb_py_test_silent_cmd "python size_a=gdb.parse_and_eval('sizeof(a)')" \
                          "find size of a" 0
@@ -543,7 +560,14 @@ proc test_value_from_buffer {} {
   gdb_py_test_silent_cmd "python addr=gdb.parse_and_eval('&a')" \
                          "find address of a" 0
   gdb_py_test_silent_cmd "python b=gdb.selected_inferior().read_memory(addr,size_a)" \
-                         "read buffer from memory" 1
+                         "read buffer from memory" 0
+}
+
+proc test_value_from_buffer {} {
+  global gdb_prompt
+  global gdb_py_is_py3k
+
+  prepare_type_and_buffer
   gdb_test "python v=gdb.Value(b,tp); print(v)" "1" \
             "construct value from buffer"
   gdb_test "python v=gdb.Value(b\[size_a0:\],tp); print(v)" "2" \
@@ -600,6 +624,29 @@ proc test_add_to_history {} {
 	"TypeError: Could not convert Python object: .*"
 }
 
+# Check we can create sub-classes of gdb.Value.
+proc test_value_sub_classes {} {
+    prepare_type_and_buffer
+
+    gdb_test_multiline "Create sub-class of gdb.Value" \
+	"python" "" \
+	"class MyValue(gdb.Value):" "" \
+	"  def __init__(self,val,type=None):" "" \
+	"    gdb.Value.__init__(self,val,type)" "" \
+	"    print(\"In MyValue.__init__\")" "" \
+	"end"
+
+    gdb_test "python obj = MyValue (123)" "In MyValue.__init__" \
+	"create instance of MyValue"
+    gdb_test "python print(obj)" "123" \
+	"check printing of MyValue"
+
+    gdb_test "python obj = MyValue(b\[size_a0:\],tp)" "In MyValue.__init__" \
+	"convert 2nd elem of buffer to a MyValue"
+    gdb_test "python print(obj)" "2" \
+	"check printing of MyValue when initiaized with a type"
+}
+
 # Build C version of executable.  C++ is built later.
 if { [build_inferior "${binfile}" "c"] < 0 } {
     return -1
@@ -612,6 +659,7 @@ clean_restart ${binfile}
 if { [skip_python_tests] } { continue }
 
 test_value_creation
+test_value_reinit
 test_value_numeric_ops
 test_value_boolean
 test_value_compare
@@ -629,6 +677,7 @@ if ![runto_main] then {
 
 test_value_in_inferior
 test_value_from_buffer
+test_value_sub_classes
 test_inferior_function_call
 test_value_after_death
 

Re: [PATCH 1/2] gdb/python: replace linked list with unordered_set

[Simon Marchi]

>> An alternative with less overhead than an unordered_set but still fits
>> the requirements (I think) would be to make the list an intrusive_list.
>> I keeps the existing data structure, but makes it more friendly to
>> use.
>
> I'll rewrite this patch as you suggest, OOI, is there one aspect of
> the overhead that concerns you the most?  I might be able to collect
> some performance data...

No, nothing specific.  It's just that the linked list solution seems
better and more lightweight from a theoritical point of view.  If I look
at the operations we need to do:

 - assert that a value is in values_in_python
 - remove a value from values_in_python
 - insert a value in values_in_python
 - iterate over all values in values_in_python

All these operations are as quick or faster with the linked list than
with the unordered set, in addition to requiring less memory
allocation and processing, so it just seems like a no-brainer to me.

I used to hate dealing with linked lists, but only because we had all
these separate ugly implementations that fiddled with pointers.  I find
that intrusive_list makes using them much easier and less error-prone.

Simon

Re: [PATCHv2] Changes to gdb.Value.__init__

[Simon Marchi]

On 2021-11-18 10:20 a.m., Andrew Burgess wrote:
> Simon,
>
> Thanks for your feedback on the v1 series.
>
> I tried to take your suggestion, but I don't think it can "just
> work".  The intrusive_list_node definition looks like this:
>
>   template<typename T>
>   struct intrusive_list_node
>   {
>     bool is_linked () const
>     {
>       return next != INTRUSIVE_LIST_UNLINKED_VALUE;
>     }
>
>     T *next = INTRUSIVE_LIST_UNLINKED_VALUE;
>     T *prev = INTRUSIVE_LIST_UNLINKED_VALUE;
>   };
>
> When embedded into a Python type the default values will not be
> honoured during object creation, in some cases these fields will be
> zero'd and in other cases they are left uninitialised.  We'd have to
> manually set these fields ourselves, which, to me, feels worse than
> just not using intrusive_list at all - a user seeing that we use
> intrusive_list might make assumptions about the behaviour which are
> not correct, by using some other data structure these assumptions will
> be avoided.

Ah shoot, I forgot that unlinked value was -1, I thought it was 0.  I
don't even remember why it's -1 and not 0.

> Given your concerns about the overhead of using std::unordered_set
> I've just dropped the old first patch.  I did briefly consider
> std::list, but finding items for deleting becomes expensive, so I
> discarded that.

In that case, I wouldn't mind using unordered_set if it makes some
things easier.  As I said, I didn't have any practical concerns with
using unordered_set, it was just that using intrusive_list seemed easy.

Simon

Re: [PATCHv2] Changes to gdb.Value.__init__

[Andrew Burgess]

Simon Marchi <simark@simark.ca> writes:

> On 2021-11-18 10:20 a.m., Andrew Burgess wrote:
>> Simon,
>>
>> Thanks for your feedback on the v1 series.
>>
>> I tried to take your suggestion, but I don't think it can "just
>> work".  The intrusive_list_node definition looks like this:
>>
>>   template<typename T>
>>   struct intrusive_list_node
>>   {
>>     bool is_linked () const
>>     {
>>       return next != INTRUSIVE_LIST_UNLINKED_VALUE;
>>     }
>>
>>     T *next = INTRUSIVE_LIST_UNLINKED_VALUE;
>>     T *prev = INTRUSIVE_LIST_UNLINKED_VALUE;
>>   };
>>
>> When embedded into a Python type the default values will not be
>> honoured during object creation, in some cases these fields will be
>> zero'd and in other cases they are left uninitialised.  We'd have to
>> manually set these fields ourselves, which, to me, feels worse than
>> just not using intrusive_list at all - a user seeing that we use
>> intrusive_list might make assumptions about the behaviour which are
>> not correct, by using some other data structure these assumptions will
>> be avoided.
>
> Ah shoot, I forgot that unlinked value was -1, I thought it was 0.  I
> don't even remember why it's -1 and not 0.
>
>> Given your concerns about the overhead of using std::unordered_set
>> I've just dropped the old first patch.  I did briefly consider
>> std::list, but finding items for deleting becomes expensive, so I
>> discarded that.
>
> In that case, I wouldn't mind using unordered_set if it makes some
> things easier.  As I said, I didn't have any practical concerns with
> using unordered_set, it was just that using intrusive_list seemed easy.

It doesn't really make things easier.  It (originall) just seemed like
an easy cleanup, but given its not considered an improvement by all then
I'm not really concerned about merging the change.

The __init__ change however .... that's something I would like to see
merged.

Thanks,
Andrew

Ping: [PATCHv2] Changes to gdb.Value.__init__

[Andrew Burgess]

Ping!

Thanks,
Andrew

* Andrew Burgess via Gdb-patches <gdb-patches@sourceware.org> [2021-11-18 15:20:23 +0000]:

> Simon,
> 
> Thanks for your feedback on the v1 series.
> 
> I tried to take your suggestion, but I don't think it can "just
> work".  The intrusive_list_node definition looks like this:
> 
>   template<typename T>
>   struct intrusive_list_node
>   {
>     bool is_linked () const
>     {
>       return next != INTRUSIVE_LIST_UNLINKED_VALUE;
>     }
> 
>     T *next = INTRUSIVE_LIST_UNLINKED_VALUE;
>     T *prev = INTRUSIVE_LIST_UNLINKED_VALUE;
>   };
> 
> When embedded into a Python type the default values will not be
> honoured during object creation, in some cases these fields will be
> zero'd and in other cases they are left uninitialised.  We'd have to
> manually set these fields ourselves, which, to me, feels worse than
> just not using intrusive_list at all - a user seeing that we use
> intrusive_list might make assumptions about the behaviour which are
> not correct, by using some other data structure these assumptions will
> be avoided.
> 
> Given your concerns about the overhead of using std::unordered_set
> I've just dropped the old first patch.  I did briefly consider
> std::list, but finding items for deleting becomes expensive, so I
> discarded that.
> 
> Thoughts welcome,
> 
> Thanks,
> Andrew
> 
> ---
> 
> Changes since v1:
> 
>   - Dropped the old patch #1, updated patch #2 to stand on its own.
> 
> ---
> 
> commit 249bfca4a5de410e89515abd22a7ae8aba7ba640
> Author: Andrew Burgess <aburgess@redhat.com>
> Date:   Wed Nov 17 11:55:37 2021 +0000
> 
>     gdb/python: Use tp_init instead of tp_new to setup gdb.Value
>     
>     The documentation suggests that we implement gdb.Value.__init__,
>     however, this is not currently true, we really implement
>     gdb.Value.__new__.  This will cause confusion if a user tries to
>     sub-class gdb.Value.  They might write:
>     
>       class MyVal (gdb.Value):
>           def __init__ (self, val):
>               gdb.Value.__init__(self, val)
>     
>       obj = MyVal(123)
>       print ("Got: %s" % obj)
>     
>     But, when they source this code they'll see:
>     
>       (gdb) source ~/tmp/value-test.py
>       Traceback (most recent call last):
>         File "/home/andrew/tmp/value-test.py", line 7, in <module>
>           obj = MyVal(123)
>         File "/home/andrew/tmp/value-test.py", line 5, in __init__
>           gdb.Value.__init__(self, val)
>       TypeError: object.__init__() takes exactly one argument (the instance to initialize)
>       (gdb)
>     
>     The reason for this is that, as we don't implement __init__ for
>     gdb.Value, Python ends up calling object.__init__ instead, which
>     doesn't expect any arguments.
>     
>     The Python docs suggest that the reason why we might take this
>     approach is because we want gdb.Value to be immutable:
>     
>        https://docs.python.org/3/c-api/typeobj.html#c.PyTypeObject.tp_new
>     
>     But I don't see any reason why we should require gdb.Value to be
>     immutable when other types defined in GDB are not.  This current
>     immutability can be seen in this code:
>     
>       obj = gdb.Value(1234)
>       print("Got: %s" % obj)
>       obj.__init__ (5678)
>       print("Got: %s" % obj)
>     
>     Which currently runs without error, but prints:
>     
>       Got: 1234
>       Got: 1234
>     
>     In this commit I propose that we switch to using __init__ to
>     initialize gdb.Value objects.
>     
>     This does introduce some additional complexity, during the __init__
>     call a gdb.Value might already be associated with a gdb value object,
>     in which case we need to cleanly break that association before
>     installing the new gdb value object.  However, the cost of doing this
>     is not great, and the benefit - being able to easily sub-class
>     gdb.Value seems worth it.
>     
>     After this commit the first example above works without error, while
>     the second example now prints:
>     
>       Got: 1234
>       Got: 5678
>     
>     In order to make it easier to override the gdb.Value.__init__ method,
>     I have tweaked the definition of gdb.Value.__init__.  The second,
>     optional argument to __init__ is a gdb.Type, if this argument is not
>     present then GDB figures out a suitable type.
>     
>     However, if we want to override the __init__ method in a sub-class,
>     and still support the default argument, it is easier to write:
>     
>       class MyVal (gdb.Value):
>           def __init__ (self, val, type=None):
>               gdb.Value.__init__(self, val, type)
>     
>     Currently, passing None for the Type will result in an error:
>     
>       TypeError: type argument must be a gdb.Type.
>     
>     After this commit I now allow the type argument to be None, in which
>     case GDB figures out a suitable type just as if the type had not been
>     passed at all.
>     
>     Unless a user is trying to reinitialize a value, or create sub-classes
>     of gdb.Value, there should be no user visible changes after this
>     commit.
> 
> diff --git a/gdb/doc/python.texi b/gdb/doc/python.texi
> index 9a768133f4c..b9b01665410 100644
> --- a/gdb/doc/python.texi
> +++ b/gdb/doc/python.texi
> @@ -819,6 +819,9 @@
>  from the Python buffer object specified by @var{val}.  The number of
>  bytes in the Python buffer object must be greater than or equal to the
>  size of @var{type}.
> +
> +If @var{type} is @code{None} then this version of @code{__init__}
> +behaves as though @var{type} was not passed at all.
>  @end defun
>  
>  @defun Value.cast (type)
> diff --git a/gdb/python/py-value.c b/gdb/python/py-value.c
> index c843c2c3072..a5c43389a80 100644
> --- a/gdb/python/py-value.c
> +++ b/gdb/python/py-value.c
> @@ -70,41 +70,69 @@ struct value_object {
>     work around a linker bug on MacOS.  */
>  static value_object *values_in_python = NULL;
>  
> +/* Clear out an old GDB value stored within SELF, and reset the fields to
> +   nullptr.  This should be called when a gdb.Value is deallocated, and
> +   also if a gdb.Value is reinitialized with a new value.  */
> +
> +static void
> +valpy_clear_value (value_object *self)
> +{
> +  /* Indicate we are no longer interested in the value object.  */
> +  value_decref (self->value);
> +  self->value = nullptr;
> +
> +  Py_XDECREF (self->address);
> +  self->address = nullptr;
> +
> +  Py_XDECREF (self->type);
> +  self->type = nullptr;
> +
> +  Py_XDECREF (self->dynamic_type);
> +  self->dynamic_type = nullptr;
> +}
> +
>  /* Called by the Python interpreter when deallocating a value object.  */
>  static void
>  valpy_dealloc (PyObject *obj)
>  {
>    value_object *self = (value_object *) obj;
>  
> -  /* Remove SELF from the global list.  */
> -  if (self->prev)
> -    self->prev->next = self->next;
> -  else
> +  /* If SELF failed to initialize correctly then it may not have a value
> +     contained within it.  */
> +  if (self->value != nullptr)
>      {
> -      gdb_assert (values_in_python == self);
> -      values_in_python = self->next;
> -    }
> -  if (self->next)
> -    self->next->prev = self->prev;
> -
> -  value_decref (self->value);
> +      /* Remove SELF from the global list of values.  */
> +      if (self->prev != nullptr)
> +	self->prev->next = self->next;
> +      else
> +	{
> +	  gdb_assert (values_in_python == self);
> +	  values_in_python = self->next;
> +	}
> +      if (self->next != nullptr)
> +	self->next->prev = self->prev;
>  
> -  Py_XDECREF (self->address);
> -  Py_XDECREF (self->type);
> -  Py_XDECREF (self->dynamic_type);
> +      /* Release the value object and any cached Python objects.  */
> +      valpy_clear_value (self);
> +    }
>  
>    Py_TYPE (self)->tp_free (self);
>  }
>  
> -/* Helper to push a Value object on the global list.  */
> +/* Helper to push a gdb.Value object on to the global list of values.  If
> +   VALUE_OBJ is already on the lit then this does nothing.  */
> +
>  static void
>  note_value (value_object *value_obj)
>  {
> -  value_obj->next = values_in_python;
> -  if (value_obj->next)
> -    value_obj->next->prev = value_obj;
> -  value_obj->prev = NULL;
> -  values_in_python = value_obj;
> +  if (value_obj->next == nullptr)
> +    {
> +      gdb_assert (value_obj->prev == nullptr);
> +      value_obj->next = values_in_python;
> +      if (value_obj->next != nullptr)
> +	value_obj->next->prev = value_obj;
> +      values_in_python = value_obj;
> +    }
>  }
>  
>  /* Convert a python object OBJ with type TYPE to a gdb value.  The
> @@ -142,60 +170,55 @@ convert_buffer_and_type_to_value (PyObject *obj, struct type *type)
>    return value_from_contents (type, (const gdb_byte *) py_buf.buf);
>  }
>  
> -/* Called when a new gdb.Value object needs to be allocated.  Returns NULL on
> -   error, with a python exception set.  */
> -static PyObject *
> -valpy_new (PyTypeObject *subtype, PyObject *args, PyObject *kwargs)
> +/* Implement gdb.Value.__init__.  */
> +
> +static int
> +valpy_init (PyObject *self, PyObject *args, PyObject *kwds)
>  {
>    static const char *keywords[] = { "val", "type", NULL };
>    PyObject *val_obj = nullptr;
>    PyObject *type_obj = nullptr;
>  
> -  if (!gdb_PyArg_ParseTupleAndKeywords (args, kwargs, "O|O", keywords,
> +  if (!gdb_PyArg_ParseTupleAndKeywords (args, kwds, "O|O", keywords,
>  					&val_obj, &type_obj))
> -    return nullptr;
> +    return -1;
>  
>    struct type *type = nullptr;
> -
> -  if (type_obj != nullptr)
> +  if (type_obj != nullptr && type_obj != Py_None)
>      {
>        type = type_object_to_type (type_obj);
>        if (type == nullptr)
>  	{
>  	  PyErr_SetString (PyExc_TypeError,
>  			   _("type argument must be a gdb.Type."));
> -	  return nullptr;
> +	  return -1;
>  	}
>      }
>  
> -  value_object *value_obj = (value_object *) subtype->tp_alloc (subtype, 1);
> -  if (value_obj == NULL)
> -    {
> -      PyErr_SetString (PyExc_MemoryError, _("Could not allocate memory to "
> -					    "create Value object."));
> -      return NULL;
> -    }
> -
>    struct value *value;
> -
>    if (type == nullptr)
>      value = convert_value_from_python (val_obj);
>    else
>      value = convert_buffer_and_type_to_value (val_obj, type);
> -
>    if (value == nullptr)
>      {
> -      subtype->tp_free (value_obj);
> -      return NULL;
> +      gdb_assert (PyErr_Occurred ());
> +      return -1;
>      }
>  
> +  /* There might be a previous value here.  */
> +  value_object *value_obj = (value_object *) self;
> +  if (value_obj->value != nullptr)
> +    valpy_clear_value (value_obj);
> +
> +  /* Store the value into this Python object.  */
>    value_obj->value = release_value (value).release ();
> -  value_obj->address = NULL;
> -  value_obj->type = NULL;
> -  value_obj->dynamic_type = NULL;
> +
> +  /* Ensure that this gdb.Value is in the set of all gdb.Value objects.  If
> +     we are already in the set then this is call does nothing.  */
>    note_value (value_obj);
>  
> -  return (PyObject *) value_obj;
> +  return 0;
>  }
>  
>  /* Iterate over all the Value objects, calling preserve_one_value on
> @@ -1784,6 +1807,8 @@ value_to_value_object (struct value *val)
>    if (val_obj != NULL)
>      {
>        val_obj->value = release_value (val).release ();
> +      val_obj->next = nullptr;
> +      val_obj->prev = nullptr;
>        val_obj->address = NULL;
>        val_obj->type = NULL;
>        val_obj->dynamic_type = NULL;
> @@ -1805,6 +1830,8 @@ value_to_value_object_no_release (struct value *val)
>      {
>        value_incref (val);
>        val_obj->value = val;
> +      val_obj->next = nullptr;
> +      val_obj->prev = nullptr;
>        val_obj->address = NULL;
>        val_obj->type = NULL;
>        val_obj->dynamic_type = NULL;
> @@ -2232,7 +2259,7 @@ PyTypeObject value_object_type = {
>    0,				  /* tp_descr_get */
>    0,				  /* tp_descr_set */
>    0,				  /* tp_dictoffset */
> -  0,				  /* tp_init */
> +  valpy_init,			  /* tp_init */
>    0,				  /* tp_alloc */
> -  valpy_new			  /* tp_new */
> +  PyType_GenericNew,		  /* tp_new */
>  };
> diff --git a/gdb/testsuite/gdb.python/py-value.exp b/gdb/testsuite/gdb.python/py-value.exp
> index f4b7c23fb7d..297b128da47 100644
> --- a/gdb/testsuite/gdb.python/py-value.exp
> +++ b/gdb/testsuite/gdb.python/py-value.exp
> @@ -51,6 +51,7 @@ proc test_value_creation {} {
>  
>    gdb_py_test_silent_cmd "python i = gdb.Value (True)" "create boolean value" 1
>    gdb_py_test_silent_cmd "python i = gdb.Value (5)" "create integer value" 1
> +  gdb_py_test_silent_cmd "python i = gdb.Value (3,None)" "create integer value, with None type" 1
>    if { $gdb_py_is_py3k == 0 } {
>      gdb_py_test_silent_cmd "python i = gdb.Value (5L)" "create long value" 1
>    }
> @@ -77,6 +78,18 @@ proc test_value_creation {} {
>    gdb_test "python print ('result = %s' % i.address)" "= None" "test address attribute in non-addressable value"
>  }
>  
> +# Check that we can call gdb.Value.__init__ to change a value.
> +proc test_value_reinit {} {
> +    gdb_py_test_silent_cmd "python v = gdb.Value (3)" \
> +	"create initial integer value" 1
> +    gdb_test "python print(v)" "3" \
> +	"check initial value contents"
> +    gdb_py_test_silent_cmd "python v.__init__(5)" \
> +	"call gdb.Value.__init__ manually" 1
> +    gdb_test "python print(v)" "5" \
> +	"check new value contents"
> +}
> +
>  proc test_value_numeric_ops {} {
>    global gdb_prompt
>    global gdb_py_is_py3k
> @@ -531,10 +544,14 @@ proc test_float_conversion {} {
>      gdb_test "python print(float(gdb.Value(0)))" "0\\.0"
>  }
>  
> -proc test_value_from_buffer {} {
> -  global gdb_prompt
> -  global gdb_py_is_py3k
> -
> +# Setup some Python variables:
> +#   tp      : a gdb.Type for 'int',
> +#   size_a  : the size of array 'a' from the inferior,
> +#   size_a0 : the size of array element 'a[0] from the inferior,
> +#   addr    : the address of 'a[0]' from the inferior,
> +#   b       : a buffer containing the full contents of array 'a' from the
> +#             inferior.
> +proc prepare_type_and_buffer {} {
>    gdb_py_test_silent_cmd "python tp=gdb.lookup_type('int')" "look up int type" 0
>    gdb_py_test_silent_cmd "python size_a=gdb.parse_and_eval('sizeof(a)')" \
>                           "find size of a" 0
> @@ -543,7 +560,14 @@ proc test_value_from_buffer {} {
>    gdb_py_test_silent_cmd "python addr=gdb.parse_and_eval('&a')" \
>                           "find address of a" 0
>    gdb_py_test_silent_cmd "python b=gdb.selected_inferior().read_memory(addr,size_a)" \
> -                         "read buffer from memory" 1
> +                         "read buffer from memory" 0
> +}
> +
> +proc test_value_from_buffer {} {
> +  global gdb_prompt
> +  global gdb_py_is_py3k
> +
> +  prepare_type_and_buffer
>    gdb_test "python v=gdb.Value(b,tp); print(v)" "1" \
>              "construct value from buffer"
>    gdb_test "python v=gdb.Value(b\[size_a0:\],tp); print(v)" "2" \
> @@ -600,6 +624,29 @@ proc test_add_to_history {} {
>  	"TypeError: Could not convert Python object: .*"
>  }
>  
> +# Check we can create sub-classes of gdb.Value.
> +proc test_value_sub_classes {} {
> +    prepare_type_and_buffer
> +
> +    gdb_test_multiline "Create sub-class of gdb.Value" \
> +	"python" "" \
> +	"class MyValue(gdb.Value):" "" \
> +	"  def __init__(self,val,type=None):" "" \
> +	"    gdb.Value.__init__(self,val,type)" "" \
> +	"    print(\"In MyValue.__init__\")" "" \
> +	"end"
> +
> +    gdb_test "python obj = MyValue (123)" "In MyValue.__init__" \
> +	"create instance of MyValue"
> +    gdb_test "python print(obj)" "123" \
> +	"check printing of MyValue"
> +
> +    gdb_test "python obj = MyValue(b\[size_a0:\],tp)" "In MyValue.__init__" \
> +	"convert 2nd elem of buffer to a MyValue"
> +    gdb_test "python print(obj)" "2" \
> +	"check printing of MyValue when initiaized with a type"
> +}
> +
>  # Build C version of executable.  C++ is built later.
>  if { [build_inferior "${binfile}" "c"] < 0 } {
>      return -1
> @@ -612,6 +659,7 @@ clean_restart ${binfile}
>  if { [skip_python_tests] } { continue }
>  
>  test_value_creation
> +test_value_reinit
>  test_value_numeric_ops
>  test_value_boolean
>  test_value_compare
> @@ -629,6 +677,7 @@ if ![runto_main] then {
>  
>  test_value_in_inferior
>  test_value_from_buffer
> +test_value_sub_classes
>  test_inferior_function_call
>  test_value_after_death
>  
> 

Re: Ping: [PATCHv2] Changes to gdb.Value.__init__

[Tom Tromey]

>>>>> "Andrew" == Andrew Burgess via Gdb-patches <gdb-patches@sourceware.org> writes:

Andrew> Ping!
Andrew> Thanks,

FWIW this looks good to me.

Tom

Re: Ping: [PATCHv2] Changes to gdb.Value.__init__

[Andrew Burgess]

* Tom Tromey <tom@tromey.com> [2021-12-07 12:20:19 -0700]:

> >>>>> "Andrew" == Andrew Burgess via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
> Andrew> Ping!
> Andrew> Thanks,
> 
> FWIW this looks good to me.

Thanks, I've now pushed this.

Andrew