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

[Andrew Burgess <aburgess@redhat.com>]

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