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

Andrew Burgess
2025-09-23T13:56:17Z

Tom Tromey <tom@tromey.com> writes:

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

No, I think this is the right amount of paranoid. I've updated both
these to Tuples, and extended the tests to check these are not
modifiable.

>
> Andrew> + object->mapped_files = nullptr;
>
> Perhaps this has to be hoisted if the initialization of object->inferior
> does (see earlier review).

Done.

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

Fixed the two added in this commit. I also fixed one from patch #1 too.

>
> 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 ();

Switched to this style. I was already using this for some of the string
creation stuff, so I'm not sure why I did it differently here.

I posted a V2 series with all these fixes.

Thanks,
Andrew