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.

3 files changed, 126 insertions, 52 deletions

diff --git a/gdb/doc/python.texi b/gdb/doc/python.texi
index 4a66c11..568aabc 100644
--- a/gdb/doc/python.texi
+++ b/gdb/doc/python.texi
@@ -827,6 +827,9 @@ This second form of the @code{gdb.Value} constructor returns a
 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 c843c2c..255a308 100644
--- a/gdb/python/py-value.c
+++ b/gdb/python/py-value.c
@@ -70,41 +70,64 @@ 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_CLEAR (self->address);
+  Py_CLEAR (self->type);
+  Py_CLEAR (self->dynamic_type);
+}
+
 /* 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 +165,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 +1802,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 +1825,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 +2254,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 f4b7c23..297b128 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