Re: [PATCH 3/3] gdb/python: add Corefile.mapped_files method

Tom Tromey
2025-09-16T17:54:40Z

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

Andrew> Add a new Corefile.mapped_files method which returns a list of
Andrew> gdb.CorefileMappedFile objects.

Andrew> Each gdb.CorefileMappedFile object represents a file that was mapped
Andrew> into the process when the core file was created.

Thanks.

Andrew> + /* A List of gdb.CorefileMappedFile objects. This list is only created
Andrew> + the first time the user calls gdb.Corefile.mapped_files(), the result
Andrew> + is cached here. If this pointer is not NULL then this is an owning
Andrew> + pointer (i.e. this owns a reference to the List). */
Andrew> + PyObject *mapped_files;

A Python list is a mutable object, so I think user code could mess
around with this data.

IMO it would be better to make this a tuple instead.

Andrew> + /* A List of gdb.CorefileMappedFileRegion objects. */
Andrew> + PyObject *regions;

Same here. Though I wonder if that's overly paranoid of me.

Andrew> + object->mapped_files = nullptr;

Perhaps this has to be hoisted if the initialization of object->inferior
does (see earlier review).

Andrew> + gdbpy_ref<corefile_mapped_file_region_object> region_obj
Andrew> + ((corefile_mapped_file_region_object *) PyObject_New

I think the cast of the return value of PyObject_New shouldn't be
needed. PyObject_New is a macro that supplies the cast.

There were a few cases of this.

Andrew> + gdbpy_ref<> start (gdb_py_object_from_ulongest (obj->start));
Andrew> + if (start == nullptr)
Andrew> + return nullptr;
Andrew> +
Andrew> + return start.release ();

The null check isn't really needed since the release does the same.
I think it's normal to just write

return gdb_py_object_from_ulongest (obj->start).release ();

There are a few of these.
Though if you prefer the wordier approach that is fine too I suppose.

Tom