Re: [PATCH v2 4/6] gdb: new setters and getters for __dict__, and attributes

[Matthieu Longo <matthieu.longo@arm.com>]

On 27/01/2026 19:06, Tom Tromey wrote:
>>>>>> "Matthieu" == Matthieu Longo <matthieu.longo@arm.com> writes:
> 
> Matthieu> GDB is currently using the Python unlimited API. Migrating the codebase
> Matthieu> to the Python limited API would have for benefit to make a GDB build
> Matthieu> artifacts compatible with older and newer versions of Python that they
> Matthieu> were built with.
> 
> Matthieu> This patch prepares the ground for migrating the existing C extension
> Matthieu> types from static types to heap-allocated ones, by removing the
> Matthieu> dependency on tp_dictoffset, which is unavailable when using the limited
> Matthieu> API.
> 
> Thanks.
> 
> Matthieu> - gdbpy__dict__wrapper: a base class for C extension objects that own a
> Matthieu>   __dict__.
> 
> I think a double underscore in a name is reserved by the C++
> implementation, so "gdbpy__dict__wrapper" can't be used.  I think the
> same name with single underscores would be fine.
> 

Renamed.

> Matthieu> - gdb_py_generic_dict_setter: a __dict__ setter provided for completeness.
> Matthieu>   It should not be used, as __dict__ should be read-only after the object
> Matthieu>   initialization.
> 
> If it shouldn't be used, it should just not exist, I think.
> 

Removed.

> Matthieu> -using thread_map_t
> Matthieu> -  = gdb::unordered_map<thread_info *, gdbpy_ref<thread_object>>;
> Matthieu> -
> Matthieu> -struct inferior_object
> Matthieu> -{
> Matthieu> -  PyObject_HEAD
> 
> This moved to python-internal.h, but I don't understand why that move
> is needed.
> 
> Matthieu> +  /* Compute the address of the __dict__ attribute for the given PyObject.
> Matthieu> +     The CLOSURE argument is unused.  */
> Matthieu> +  static PyObject **
> Matthieu> +  compute_addr (PyObject *self, void *closure ATTRIBUTE_UNUSED)
> 
> In classes there doesn't have to be a newline before the method name.
> Also since the closure isn't used here it should probably just be
> dropped.  If it's needed in the future we can add it then, but I'm going
> to guess it won't be.
> 

Fixed.

> Matthieu> +/* Generic attribute getter function similar to PyObject_GenericGetAttr () but
> Matthieu> +   that should be used when the object has a dictionary __dict__.  */
> Matthieu> +PyObject *
> Matthieu> +gdb_py_generic_getattro (PyObject *self, PyObject *attr)
> Matthieu> +{
> Matthieu> +  PyObject *value = PyObject_GenericGetAttr (self, attr);
> Matthieu> +  if (value != nullptr)
> Matthieu> +    return value;
> Matthieu> +
> Matthieu> +  if (! PyErr_ExceptionMatches (PyExc_AttributeError))
> Matthieu> +    return nullptr;
> Matthieu> +
> Matthieu> +  /* Clear previous AttributeError set by PyObject_GenericGetAttr.  */
> Matthieu> +  PyErr_Clear();
> Matthieu> +

Also, in both gdb_py_generic_getattro() and gdb_py_generic_setattro(), I moved the look-up of __dict__ before PyErr_Clear().
If the look-up of __dict__ were to fail for some reason, we still want the exception raised by PyObject_GenericGetAttr / PyObject_GenericSetAttr to be there.

> Matthieu> +  gdbpy_ref<> dict (gdb_py_generic_dict_getter (self, nullptr));
> Matthieu> +  value = PyDict_GetItemWithError (dict.get (), attr);
> 
> I think this has to do check 'dict == nullptr' before this call.
> 

Indeed, I forgot that. Fixed.

> Matthieu> +  if (value != nullptr)
> Matthieu> +    return Py_NewRef (value);
> Matthieu> +
> Matthieu> +  PyErr_Format (PyExc_AttributeError,
> Matthieu> +		"'%s' object has no attribute '%s'",
> Matthieu> +		Py_TYPE (self)->tp_name,
> Matthieu> +		PyUnicode_AsUTF8AndSize (attr, nullptr));
> Matthieu> +  return nullptr;
> 
> PyDict_GetItemWithError sets the error, so is this just here to make a
> nicer message?  That's fine if so, though I am not sure if you have to
> clear the existing error here first?
> 

https://github.com/python/cpython/blob/08d7bd28fecca524c648dda240022add704b8f8a/Objects/dictobject.c#L2505
/* Variant of PyDict_GetItem() that doesn't suppress exceptions.
    This returns NULL *with* an exception set if an exception occurred.
    It returns NULL *without* an exception set if the key wasn't present.
*/
PyDict_GetItemWithError has a misleading name, because if the key is not found, no PyExc_AttributeError is set.
This situation results in a new exception being raised:
   Python Exception <class 'SystemError'>: error return without exception set
Hence this surprising manual setting of an exception to makes everything fit into place.

I am going to add a comment to clarify the intent, because I had even forgotten why I did this in the first place.

> Matthieu> +  gdbpy_ref<> dict (gdb_py_generic_dict_getter (self, nullptr));
> Matthieu> +  /* Delete the old value (if there is one).  */
> Matthieu> +  PyObject *old_value = PyDict_GetItem (dict.get (), attr);
> 
> dict==nullptr check
> 

Fixed.

> Matthieu> +  if (old_value != nullptr)
> Matthieu> +    Py_DECREF (old_value);
> Matthieu> +  /* Set the new value.  */
> Matthieu> +  return PyDict_SetItem (dict.get (), attr, value);
> 
> I think this ordering means self-assignment can break.  IMO it's better
> to use gdbpy_ref to manage the lifetime of the old value.
> 

It seems that I got the whole thing wrong with my assumption that I needed to decref the old value.
This is already managed in insertdict() called from PyDict_SetItem()
https://github.com/python/cpython/blob/08d7bd28fecca524c648dda240022add704b8f8a/Objects/dictobject.c#L1934
This PyDict_GetItem () is useless, I will remove it.

> Matthieu> +#if PY_VERSION_HEX < 0x030a0000
> Matthieu> +static inline PyObject*
> Matthieu> +Py_NewRef (PyObject *obj)
> 
> The return type is missing a space before the "*"
> 

Fixed.

> Tom

Since all others patches are approved with the small modifications you suggested, I am going to only post a new revision of this patch in this same thread to avoid a new round.

Matthieu