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

[Tom Tromey <tom@tromey.com>]

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