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

[Tom Tromey <tom@tromey.com>]

>>>>> "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.

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.

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.

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> +
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.

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?

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

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.

Matthieu> +#if PY_VERSION_HEX < 0x030a0000
Matthieu> +static inline PyObject*
Matthieu> +Py_NewRef (PyObject *obj)

The return type is missing a space before the "*"

Tom