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

Andrew Burgess
2025-09-23T13:51:57Z

Tom Tromey <tom@tromey.com> writes:

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

Fixed.

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

You are correct to worry. PyObject_New only initialises the Python
object header, not any of the other fields, i.e. 'inferior' will indeed
be uninitialised.

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

This is fixed in V2. I claim we need to first initialise the 'inferior'
field to NULL. if the PyDict_New call fails then we'll end up in
cfpy_dealloc as you say, and that only expects to see invalidated
(inferior == NULL) core file objects.

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

Done.

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

Fixed.

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

I dropped the annotation.

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

I hadn't even realised this was a thing. I've added this in now. The
code as it was already supported doing things like:

core = gdb.selected_inferior().corefile
core._my_attribute = "blah blah blah"

which is how I've always used this feature. But users can now:

core.__dict__['_my_attribute'] = "blah blah blah"

too. It doesn't hurt to be consistent with other objects that implement
this feature.

I posted a V2 series with these fixes included.

Thanks,
Andrew