Re: [rfc] Options for "info mappings" etc. (Re: [PATCH] Implement new `info core mappings' command)

[Sergio Durigan Junior <sergiodj at redhat dot com>]

Hi Ulrich,

"Ulrich Weigand" <uweigand@de.ibm.com> writes:

> Sergio Durigan Junior <sergiodj@redhat.com> writes:
>> > Ok, so I would like to ask for another round of review of this patch
>> > then.  I am aware of Ulrich's work towards a new `info mappings'
>> > command, but since I've got no response from him yet, I decided to
>> > continue my efforts here.
>> 
>> Ping.
>
> First of all, sorry for the late reply.  I ran into some issues with
> implementing the proposed "info mappings" and then got side-tracked
> ...

No problem.

> Anyway, here's my current thoughts on the issues and some options on
> how to move forward.

After reading the message at least three times, I like your approach in
general.  It seems a lot cleaner than what is current implemented.  Only
a few comments.

> Today we have the following set of commands:
>
> - generate-core-file
>
> This uses the target_find_memory_regions callback, which is implemented
> on Linux native by reading /proc/.../maps.   My pending patch would have
> implemented this callback on remote as well, via a new packet type
> qXfer:process-map:read, implemented on Linux gdbserver by reading
> /proc/.../maps
>
> - info proc ...  (in particular, info proc mappings)
>
> This is implemented on Linux native target only, where it reads from
> /proc directly.   Note that:
> * this reads a variety of other files beyond /proc/.../maps
> * it can actually read from a completely different process
>   (info proc takes an optional PID argument)
>
> (There is also a separate implementation of info proc for procfs
> targets, i.e. Solaris, Irix, Tru64).
>
> - info core ... (info core mappings and info core exe)
>
> This would be implemented by Sergio's patch, and available for
> core file targets only (any platform).   It would support only
> "exe" and "mappings", and only the current core file.

Maybe this is obvious, but I believe the patch can be (easily?) extended
in order to support other core files than the current one.  Anyway, this
is probably not important now.

> This state of affairs, even assuming both my and Sergio's pending
> patch sets were applied, has a couple of drawbacks:
>
> - there is still no "info proc mappings" like command for the remote
>   target, even if the remote side runs gdbserver on Linux
>
> - there is still code duplication between the "info proc mappings"
>   and target_find_memory_regions implementations in linux-nat.c
>
>
> To fix these, I had been thinking along the lines of implementing
> the following set of features ("Option 1"):
>
> a) Extend target_find_memory_regions to provide mapped file names
>    This requires extending the implementation in linux-nat.c, and
>    updating other existing implementations and users.
>
> b) Implement target_find_memory_regions for remote targets
>    This would use xfer with a new object type TARGET_OBJECT_PROCESS_MAP,
>    implemented via a packet type qXfer:process-map:read providing a new
>    XML formatted <process-map>.  Implementation on gdbserver in my patch.
>
> c) Implement target_find_memory_regions in corefile.c
>    This would use techniques similar to Sergio's current patch.
>
> d) Implement new "info mappings" in core GDB
>    This would be independent of existing "info proc" commands.  It would
>    be implemented across all targets, and simply call into the (newly
>    extended) target_find_memory_regions to get its data.
>
>
> This would fix the first of the two problems mentioned above, in that
> we now also have a working "info mappings" with remote gdbserver targets.
>
> However, we still have code duplication; in fact the duplication is now
> even user-visible in the sense that we now have a generic "info mappings"
> in addition to the Linux-specific "info proc mappings".
>
> Another drawback is that we do not have anything like Sergio's proposed
> "info core exe" command; nor do have anything like "info proc ..." for
> any other the *other* commands except "mappings" for remote targets.
>
> Finally, as a (very minor) drawback: in non-XML builds of GDB, the
> "info mappings" command would not work with remote targets.
>
>
> I have been thinking about ways to address these, and come up with one
> that would basically export arbitrary /proc files via xfer ("Option 2"):
>
> a) Implement TARGET_OBJECT_PROC xfer object.
>    This would use the name of the /proc file as "Annex" (e.g. "maps" /
>    "exe" / "cmdline" ...).  On Linux native targets, this can be directly
>    implemented via reads from /proc.  For remote targets, this would be
>    implemented via a new qXfer:proc:read packet which simply returns raw
>    contents of the requested /proc file (no XML format required).  For
>    core file targets, we could synthesize Linux /proc/.../maps and
>    /proc/.../exe contents via something like Sergio's patch.
>
> b) Implement gdbarch target_find_memory_regions fallback
>    This can be implemented on Linux targets (linux-tdep.c) via reading
>    TARGET_OBJECT_PROC annex "maps", which would then automatically work
>    for native, remote, and core file targets.  The implementation in
>    linux-nat.c would then be superfluous.
>
> c) Implement generic "info proc ..." command
>    This would call out to a gdbarch architecture-specific implementation.
>    (We'd need to take care that on procfs targets, we still use the
>    original implementation in procfs.c instead of the generic one.)
>    A Linux (linux-tdep.c) implementation of that callback would then use
>    TARGET_OBJECT_PROC xfers (and thus work native, remote, and core).
>    This would make the linux-nat.c implementation superfluous.
>
>
> This fixes all the drawbacks mentioned with Option 1 above: there are
> no new commands, no more code duplication, we support "info proc exe"
> for core files, and we don't even require XML.
>
> However, there are still problems with this option:
>
> - "info proc mappings" and "info proc exe" would work on core files,
>   but only *Linux* core files -- at least until other targets implement
>   equivalent support.  Sergio's "info core ..." would work anywhere.
>
> - "info proc ..." would lose the possibility to query properties of
>   *other* processes except the current one (i.e. the "info proc PID"
>   variant could no longer be supported)

This last drawback, as explained later by you, is not a big one IMO.

> The first problem doesn't look really serious to me: if we wouldn't
> support "info proc" on a native target, it doesn't seem important
> to support it on a core file produced on that target (in particular
> if the information we can synthesize is rather sparse anyway).
>
> The second problem also may not be really serious any more: with
> current GDB, the user could always use multi-inferior support to 
> (temporarily) attach to the process they want to see info about,
> instead of specifying the PID in the command.  However, this would
> indeed reflect a UI change ...
>
>
> There is a variant of Option 2 that would actually solve that latter
> problem as well: we might encode a target PID in the TARGET_OBJECT_PROC
> request, e.g. by using "PID/maps" instead of plain "maps" as the Annex.
>
> (As minor variations, we could keep TARGET_OBJECT_PROC as-is and add
> a new TARGET_OBJECT_REMOTE_PROC that takes the PID argument.)
>
> The only drawback of this method seems to be that it would introduce
> a somewhat "alien" remote protocol packet type: all the xfer commands
> usually refer to the current inferior, not some random other process
> ...

No, I believe we should not follow this path.  Being able to display the
memory mapping of one process while debugging another is not something
used that often, I think.  Also, there is always the possibility to use
the multi-inferior feature as you mentioned above.  This probably
deserves a mention in the documentation, probably.

Thanks,

Sergio.