From 2988a36005f2821cee6744473ad8a3ba7638c212 Mon Sep 17 00:00:00 2001 From: Andrew Burgess Date: Wed, 17 Nov 2021 11:55:37 +0000 Subject: [PATCH] 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 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. --- gdb/doc/python.texi | 3 + gdb/python/py-value.c | 116 +++++++++++++++----------- gdb/testsuite/gdb.python/py-value.exp | 59 +++++++++++-- 3 files changed, 126 insertions(+), 52 deletions(-) diff --git a/gdb/doc/python.texi b/gdb/doc/python.texi index 4a66c11c19d..568aabc4885 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 c843c2c3072..255a30875be 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 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.30.2