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

[Pedro Alves <pedro@codesourcery.com>]

Hi Ulrich,

sorry for my delays.

On Wednesday 07 December 2011 17:55:29, Ulrich Weigand wrote:
> Pedro Alves wrote:
> > On Tuesday 06 December 2011 16:46:04, Ulrich Weigand wrote:
> > > For remote, we could do something along those lines (we cannot directly
> > > use "remote:" because this is only implemented for BFD access -- but
> > > we could use the underlying remote_hostio_pread etc. routines).
> > 
> > Right, I did not mean to imply "remote:" literally.  
> > I kind of see "remote:/proc/PID/maps" as a target path mounted
> > on the host, so "remote:/proc/PID/maps" is a really a host path.
> > The "/proc/PID/maps" path, what the code in question wants to get at, is
> > a target path.  I was trying to say is that the gdbarch hook would always
> > open/read "/proc/PID/maps" from the target, instead of from the host.  On
> > native targets, that ends up falling back to the host filesystem.  For
> > remote targets, that would use the existing hostio mechanism, the same
> > as used by "remote:".  This suggests e.g., a target method to
> > fully open/read/close a target path and return the file's contents (like
> > you say), or, alternatively, wrap the hostio mechanism in a
> > ui-file (through open/read/write/close/... target methods perhaps), and
> > use that.
> 
> I see.  It still seems that the target object mechanism should be a good
> fit for that; but instead of talking about a specific TARGET_OBJECT_PROC,
> we're now talking about a generic TARGET_OBJECT_FILE -- which may have been
> part of the original target object design to start with, see e.g. the 
> comment in target.h:
> 
>   /* Possible future objects: TARGET_OBJECT_FILE, ...  */

Yeah.  Although, I'm not so sure a target object for reading a file
is a good idea.  The problem with it, is that there's no notion of
"open/session handle" in target objects.  That means that for each
partial transfer of a given target object, that can start at any offset,
and extend for any number of bytes, not necessarily the length of the
object (which might not be known upfront), the backend/server xfer method
needs to open/close the destination object.  If the file changes, or
even if deleted/recreated behind the client's back, the transfer
ends up corrupted.  (e.g., the program is running, and the mappings
change while the xfer is taking place; or, the PID program vanishes
and PID is reused.)

We already take this into consideration in some cases.  E.g., 

common/linux-osdata.c:
static LONGEST
linux_xfer_osdata_processes (gdb_byte *readbuf,
			     ULONGEST offset, LONGEST len)
{
  /* We make the process list snapshot when the object starts to be read.  */
  static const char *buf;
  static LONGEST len_avail = -1;
  static struct buffer buffer;

  if (offset == 0)
    {
      ... build snapshot ...
    }
  else
    {
       ... return a piece of the previously built snapshot ...
    }
}

Triggered with "(gdb) info os processes".

We've managed to get away with this so far, because we took the
assumption that GDB always reads the full object, and always
reads it sequencially starting at offset 0.  Another drawback with
this is the need to keep an in-memory snapshot of the object (the
static vars shown above).  We currently keep the snapshot in memory
until a new transfer of the same object starts.

I now notice the new TARGET_OBJECT_PROC implementation was not safe
against this issue.

If we assumed that GDB never transfers more than
one object at the same time (which is true today), we could make
the server discard any previous snapshot whenever it sees an
unrelated packet.  These workarounds don't sound like a good idea to
me for file access.

> > > [ I guess we could implement TARGET_OBJECT_PROC without a new packet type
> > > but implementing TARGET_OBJECT_PROC xfer in remote.c via remote_hostio_pread.
> > > This makes the assumption that the remote side always has a Linux-style /proc,
> > > however.  Not sure whether we should make that assumption ... ]
> > 
> > If the OS does has something else entirely, then the gdbarch method will
> > be aware, because it is the gdbarch method itself that retrieves
> > the data.  It may even use something completely different, not
> > based on filesystems at all.  Otherwise, we're back at making targets
> > fake a file system and proc interface they don't have, where we might
> > as well return some structured data.
> 
> Hmm, OK.  I agree.  So this option would be to:
> 
> - Define a new TARGET_OBJECT_FILE.  The "annex" is simply a full path name
>   of a file on the target system.
> 
> - Implement its xfer method for native targets, probably in inf-ptrace.c
>   (or maybe even as generic default in target.c), via native file I/O.

inf-child.c may be better than inf-ptrace.c.  target.c sounds fine too.
We'd probably want to make the methods be run on the default run target,
if no target is pushed.

> - Implement its xfer method for remote targets in remote.c, using the
>   hostio mechanism (no new packets required).
> 
> - Implement its xfer method for core files via a gdbarch callback allowing
>   the arch to synthesize contents of arbitrary files.
> 
> 
> Note that to fully implement "info proc", we also need to be able to
> read the link path names for files that happen to be symbolic links.
> For TARGET_OBJECT_PROC, I simply defined the target object that way;
> this doesn't really make sense for TARGET_OBJECT_FILE.  So we might
> want to add another target object type TARGET_OBJECT_SYMLINK that
> provides that capability.  Unfortunately, this does mean we need a
> new remote protocol packet, since hostio does not support readlink ...

That's unfortunate, though, it sounds like something generically useful
for other scenarios too.

> Pros of this method would be:
> 
> - New target objects are quite generic and well-defined.
> 
> - "info proc" now can go back to display info about arbitrary processes.
> 
> - No new remote protocol packets for TARGET_OBJECT_FILE.
> 
> 
> Cons of the method:
> 
> - Need new remote protocol packet for TARGET_OBJECT_SYMLINK after all.
> 
> - Synthesizing file contents for core files is a bit more awkward, since
>   you have to recognize particular /proc/PID/... file names.
> 
> 
> The alternative to TARGET_OBJECT_FILE/SYMLINK would be to provide a set
> of target_file_... accessor routines that map to native IO for native
> targets and hostio for remote targets, again with a gdbarch option to
> synthesize file contents from core files.

Right, that's what my "alternatively, wrap the hostio mechanism in a
ui-file (through open/read/write/close/... target methods perhaps), and
use that." comment aluded to.  Since you'd need to keep a reference
to an open handle to a file somewhere, I thought ui-file could be good
for that.  The core target could just use mem_fileopen() to return an
in-memory ui-file with the synthetic /proc/PID/... contents.  I guess
we'd need a new symlink ui-file type too.

> This would require the exact same set of remote protocol extensions
> as the above (i.e. a vFile:readlink or so), and allow the exact same
> set of capabilities; this is purely a question of what GDB-internal
> interface is preferable.

I lean torwards not using target objects for this.

-- 
Pedro Alves