Re: [PATCH 1/3] gdb/python: introduce gdb.Corefile API

Tom Tromey
2025-09-16T17:26:10Z

>>>>> "Andrew" == Andrew Burgess <aburgess@redhat.com> writes:

Andrew> This commit starts adding some core file related features to the
Andrew> Python API.

Andrew> In this initial commit I've tried to keep the changes as small as
Andrew> possible for easy review.

Thanks for doing this.

Andrew> +++ b/gdb/python/py-corefile.c
Andrew> @@ -0,0 +1,293 @@
Andrew> +/* Python interface to core files.
Andrew> +
Andrew> + Copyright (C) 2010-2025 Free Software Foundation, Inc.

Date seems like copy-paste error.

Andrew> + if (result == nullptr)
Andrew> + {
Andrew> + gdbpy_ref<corefile_object> object
Andrew> + ((corefile_object *) PyObject_New (corefile_object,
Andrew> + &corefile_object_type));
Andrew> + if (object == nullptr)
Andrew> + return nullptr;
Andrew> +
Andrew> + object->dict = PyDict_New ();
Andrew> + if (object->dict == nullptr)
Andrew> + return nullptr;
Andrew> + object->inferior = inf;

I don't recall how Python objects are initialized, so I wonder if we
could end up in a situation where PyDict_New fails, and then
object->inferior is uninitialized.

cfpy_dealloc checks corefile->inferior so I guess it could matter.
So if Python doesn't memset the object to zero, I guess object->inferior
should be initialized before the dict.

Andrew> +/* Callback from gdb::observers::core_file_changed. The core file in
Andrew> + PSPACE has been changed. */
Andrew> +
Andrew> +static void
Andrew> +cfpy_corefile_changed (inferior *inf)
Andrew> +{
Andrew> + if (!gdb_python_initialized)
Andrew> + return;
Andrew> +
Andrew> + gdbpy_enter enter_py;
Andrew> +
Andrew> + /* Get any existing corefile_object for PSPACE. */
Andrew> + corefile_object *object = cfpy_inferior_corefile_data_key.get (inf);
Andrew> +
Andrew> + /* If we have an object, then... */
Andrew> + if (object != nullptr)
Andrew> + {
Andrew> + /* Clearing the inferior pointer marks the gdb.Corefile as invalid. */
Andrew> + object->inferior = nullptr;
Andrew> +
Andrew> + /* We're discarding our reference to the gdb.Corefile. */

I wouldn't mind seeing this comment moved to
inferior_corefile_deleter::operator().

Andrew> + Py_XDECREF ((PyObject *) object);
Andrew> + cfpy_inferior_corefile_data_key.set (inf, nullptr);

I think the entire body of this function could be replaced with:

cfpy_inferior_corefile_data_key.clear (inf);

... and this would remove duplicate code.

Andrew> +static int CPYCHECKER_NEGATIVE_RESULT_SETS_EXCEPTION
Andrew> +gdbpy_initialize_corefile (void)

No more (void)

I don't think the checker works for us any more -- at least, IIRC it
didn't work for C++ stuff very well (I guess we could ask David Malcolm)
and I haven't run it in years. So maybe those annotations aren't
worthwhile any more.

Andrew> + corefile_getset, /* tp_getset */
Andrew> + 0, /* tp_base */
Andrew> + 0, /* tp_dict */
Andrew> + 0, /* tp_descr_get */
Andrew> + 0, /* tp_descr_set */
Andrew> + offsetof (corefile_object, dict), /* tp_dictoffset */

I think if you want __dict__ to work you have to implement it by hand,
see gdb_py_generic_dict and its various users. I.e., I think the getset
object needs another entry.

Tom