Move vgdb special case into remote_filesystem_is_local

  Hi all,

Valgrind GDB (vgdb) presents itself as a remote target but works on
the local filesystem.  gdb_bfd_open contained a special case to make
vgdb work with "target:" sysroots, but the implementation meant that
GDB would fall back to the local filesystem if *any* to_fileio_open
method failed with ENOSYS for *any* reason.  This commit moves the
vgdb special case to remote_filesystem_is_local to allow the fallback
to be restricted only to the specific case that remote file transfer
is unsupported.  This commit also adds a warning which is displayed
the first time the fallback is used.

Built and regtested on RHEL6.6 x86_64.

Ok to commit?

Cheers,
Gary


gdb/ChangeLog:

	* gdb_bfd.c (gdb_bfd_open): Move vgdb special case to...
	* remote.c (remote_filesystem_is_local): ...here.
---
 gdb/ChangeLog |    5 ++++
 gdb/gdb_bfd.c |   16 +-------------
 gdb/remote.c  |   62 ++++++++++++++++++++++++++++++++++++++++++++++++--------
 3 files changed, 59 insertions(+), 24 deletions(-)
  

Ping:
https://sourceware.org/ml/gdb-patches/2015-04/msg00991.html

Gary Benson wrote:
> Hi all,
> 
> Valgrind GDB (vgdb) presents itself as a remote target but works on
> the local filesystem.  gdb_bfd_open contained a special case to make
> vgdb work with "target:" sysroots, but the implementation meant that
> GDB would fall back to the local filesystem if *any* to_fileio_open
> method failed with ENOSYS for *any* reason.  This commit moves the
> vgdb special case to remote_filesystem_is_local to allow the fallback
> to be restricted only to the specific case that remote file transfer
> is unsupported.  This commit also adds a warning which is displayed
> the first time the fallback is used.
> 
> Built and regtested on RHEL6.6 x86_64.
> 
> Ok to commit?
> 
> Cheers,
> Gary
> 
> 
> gdb/ChangeLog:
> 
> 	* gdb_bfd.c (gdb_bfd_open): Move vgdb special case to...
> 	* remote.c (remote_filesystem_is_local): ...here.
> ---
>  gdb/ChangeLog |    5 ++++
>  gdb/gdb_bfd.c |   16 +-------------
>  gdb/remote.c  |   62 ++++++++++++++++++++++++++++++++++++++++++++++++--------
>  3 files changed, 59 insertions(+), 24 deletions(-)
> 
> diff --git a/gdb/gdb_bfd.c b/gdb/gdb_bfd.c
> index 3d5d23f..2cd91ef 100644
> --- a/gdb/gdb_bfd.c
> +++ b/gdb/gdb_bfd.c
> @@ -326,25 +326,11 @@ gdb_bfd_open (const char *name, const char *target, int fd)
>  	{
>  	  gdb_assert (fd == -1);
>  
> -	  abfd = gdb_bfd_openr_iovec (name, target,
> +	  return gdb_bfd_openr_iovec (name, target,
>  				      gdb_bfd_iovec_fileio_open, NULL,
>  				      gdb_bfd_iovec_fileio_pread,
>  				      gdb_bfd_iovec_fileio_close,
>  				      gdb_bfd_iovec_fileio_fstat);
> -
> -	  if (abfd != NULL || errno != ENOSYS)
> -	    return abfd;
> -
> -	  /* gdb_bfd_iovec_fileio_open failed with ENOSYS.  This can
> -	     happen, for example, with vgdb (Valgrind GDB), which
> -	     presents itself as a remote target but works on the local
> -	     filesystem: it does not implement remote get and users
> -	     are not expected to set gdb_sysroot.  To handle this case
> -	     we fall back to trying the local filesystem iff
> -	     gdb_sysroot is exactly TARGET_SYSROOT_PREFIX.  */
> -	  if (gdb_sysroot == NULL
> -	      || strcmp (gdb_sysroot, TARGET_SYSROOT_PREFIX) != 0)
> -	    return NULL;
>  	}
>  
>        name += strlen (TARGET_SYSROOT_PREFIX);
> diff --git a/gdb/remote.c b/gdb/remote.c
> index 3b2325f..099ddbb 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -9879,15 +9879,6 @@ remote_hostio_send_command (int command_bytes, int which_packet,
>    return ret;
>  }
>  
> -/* Return nonzero if the filesystem accessed by the target_fileio_*
> -   methods is the local filesystem, zero otherwise.  */
> -
> -static int
> -remote_filesystem_is_local (struct target_ops *self)
> -{
> -  return 0;
> -}
> -
>  /* Open FILENAME on the remote target, using FLAGS and MODE.  Return a
>     remote file descriptor, or -1 if an error occurs (and set
>     *REMOTE_ERRNO).  */
> @@ -10125,6 +10116,59 @@ remote_hostio_fstat (struct target_ops *self,
>    return 0;
>  }
>  
> +/* Return nonzero if the filesystem accessed by the target_fileio_*
> +   methods is the local filesystem, zero otherwise.  */
> +
> +static int
> +remote_filesystem_is_local (struct target_ops *self)
> +{
> +  /* Valgrind GDB presents itself as a remote target but works
> +     on the local filesystem: it does not implement remote get
> +     and users are not expected to set a sysroot.  To handle
> +     this case we treat the remote filesystem as local if the
> +     sysroot is exactly TARGET_SYSROOT_PREFIX and if the stub
> +     does not support vFile:open.  */
> +  if (gdb_sysroot != NULL
> +      && strcmp (gdb_sysroot, TARGET_SYSROOT_PREFIX) == 0)
> +    {
> +      enum packet_support ps = packet_support (PACKET_vFile_open);
> +
> +      if (ps == PACKET_SUPPORT_UNKNOWN)
> +	{
> +	  int fd, remote_errno;
> +
> +	  /* Try opening a file to probe support.  The supplied
> +	     filename is irrelevant, we only care about whether
> +	     the stub recognizes the packet or not.  */
> +	  fd = remote_hostio_open (self, "just probing",
> +				   FILEIO_O_RDONLY, 0700,
> +				   &remote_errno);
> +
> +	  if (fd >= 0)
> +	    remote_hostio_close (self, fd, &remote_errno);
> +
> +	  ps = packet_support (PACKET_vFile_open);
> +	}
> +
> +      if (ps == PACKET_DISABLE)
> +	{
> +	  static int warning_issued = 0;
> +
> +	  if (!warning_issued)
> +	    {
> +	      warning (_("remote target does not support file"
> +			 " transfer, attempting to access files"
> +			 " from local filesystem."));
> +	      warning_issued = 1;
> +	    }
> +
> +	  return 1;
> +	}
> +    }
> +
> +  return 0;
> +}
> +
>  static int
>  remote_fileio_errno_to_host (int errnum)
>  {
> -- 
> 1.7.1
>
  

On 04/27/2015 03:51 PM, Gary Benson wrote:
> Hi all,
> 
> Valgrind GDB (vgdb) presents itself as a remote target but works on
> the local filesystem.  gdb_bfd_open contained a special case to make
> vgdb work with "target:" sysroots, but the implementation meant that
> GDB would fall back to the local filesystem if *any* to_fileio_open
> method failed with ENOSYS for *any* reason. 

Can you give an example target where we'd want this to behave differently?

E.g,. what should happen with "target sim" ?

> This commit moves the
> vgdb special case to remote_filesystem_is_local to allow the fallback
> to be restricted only to the specific case that remote file transfer
> is unsupported.  This commit also adds a warning which is displayed
> the first time the fallback is used.

Thanks,
Pedro Alves
  

Pedro Alves wrote:
> On 04/27/2015 03:51 PM, Gary Benson wrote:
> > Valgrind GDB (vgdb) presents itself as a remote target but works
> > on the local filesystem.  gdb_bfd_open contained a special case
> > to make vgdb work with "target:" sysroots, but the implementation
> > meant that GDB would fall back to the local filesystem if *any*
> > to_fileio_open method failed with ENOSYS for *any* reason.
> 
> Can you give an example target where we'd want this to behave
> differently?
> 
> E.g,. what should happen with "target sim" ?

I don't understand what you're asking.  "target sim" doesn't use
remote_filesystem_is_local, (to_filesystem_is_local for sim is
the default, tdefault_filesystem_is_local, which returns 1 always).

> > This commit moves the vgdb special case to
> > remote_filesystem_is_local to allow the fallback to be restricted
> > only to the specific case that remote file transfer is
> > unsupported.  This commit also adds a warning which is displayed
> > the first time the fallback is used.

Thanks,
Gary
  

On 05/15/2015 10:02 AM, Gary Benson wrote:
> Pedro Alves wrote:
>> On 04/27/2015 03:51 PM, Gary Benson wrote:
>>> Valgrind GDB (vgdb) presents itself as a remote target but works
>>> on the local filesystem.  gdb_bfd_open contained a special case
>>> to make vgdb work with "target:" sysroots, but the implementation
>>> meant that GDB would fall back to the local filesystem if *any*
>>> to_fileio_open method failed with ENOSYS for *any* reason.
>>
>> Can you give an example target where we'd want this to behave
>> differently?
>>
>> E.g,. what should happen with "target sim" ?
> 
> I don't understand what you're asking.  "target sim" doesn't use
> remote_filesystem_is_local, (to_filesystem_is_local for sim is
> the default, tdefault_filesystem_is_local, which returns 1 always).

When you say:

 gdb_bfd_open contained a special case
 to make vgdb work with "target:" sysroots, but the implementation
 meant that GDB would fall back to the local filesystem if *any*
 to_fileio_open method failed with ENOSYS for *any* reason.

I'd prefer to get an example target for one of those "if *any*
to_fileio_open ... *any* reason".  I'd like to understand the
real motivation for the change.  Because otherwise I get to
wonder why would we handle any other target that goes through
this path differently.

Say for example, that gdb learns to open files from
the remote target with "target mips" (remote-mips.c) as well,
for remote stubs that support it.  Seems like we'd handle ENOSYS
the same way.

I may be misunderstanding what you meant.

In any case, I guess it does make more sense to move the checks
to target_filesystem_is_local, so the target_filesystem_is_local()
checks in core code get the right result before target_fileio_open
is called (or target_fileio_readlink, etc.).  But then the patch
should be justified on those grounds.

(Note that it's not correct to say that Valgrind _always_ works on
the local filesystem.  It's just more common to run Valgrind on
the local machine, but one can well connect to a Valgrind running
on a separate machine, even one of a different arch (e.g., an ARM
GNU/Linux dev board).  The fallback is really for any remote
target that doesn't support file retrieval, and is needed because
assuming local is a better default.  I'd guess that qemu needs
it too, for example.)

Thanks,
Pedro Alves
  

Pedro Alves wrote:
> On 05/15/2015 10:02 AM, Gary Benson wrote:
> > Pedro Alves wrote:
> > > On 04/27/2015 03:51 PM, Gary Benson wrote:
> > > > Valgrind GDB (vgdb) presents itself as a remote target but
> > > > works on the local filesystem.  gdb_bfd_open contained a
> > > > special case to make vgdb work with "target:" sysroots, but
> > > > the implementation meant that GDB would fall back to the local
> > > > filesystem if *any* to_fileio_open method failed with ENOSYS
> > > > for *any* reason.
> > >
> > > Can you give an example target where we'd want this to behave
> > > differently?
> > >
> > > E.g,. what should happen with "target sim" ?
> > 
> > I don't understand what you're asking.  "target sim" doesn't use
> > remote_filesystem_is_local, (to_filesystem_is_local for sim is the
> > default, tdefault_filesystem_is_local, which returns 1 always).
> 
> When you say:
> 
>  gdb_bfd_open contained a special case
>  to make vgdb work with "target:" sysroots, but the implementation
>  meant that GDB would fall back to the local filesystem if *any*
>  to_fileio_open method failed with ENOSYS for *any* reason.
> 
> I'd prefer to get an example target for one of those "if *any*
> to_fileio_open ... *any* reason".  I'd like to understand the
> real motivation for the change.  Because otherwise I get to
> wonder why would we handle any other target that goes through
> this path differently.

Ah, ok, I get what you're asking.

In what's upstream right now, the only path (I think) that you
can get to the point in gdb_bfd_open with the workaround is if
you're using a remote target that doesn't support file retrieval.
But, in the namespace-awareness series I posted, target_fileio_open
can fail with ENOSYS if setns is not available.  That's the reason
I made the change.

Thanks,
Gary
  

On 05/15/2015 02:19 PM, Gary Benson wrote:
> Pedro Alves wrote:
>> On 05/15/2015 10:02 AM, Gary Benson wrote:
>>> Pedro Alves wrote:
>>>> On 04/27/2015 03:51 PM, Gary Benson wrote:
>>>>> Valgrind GDB (vgdb) presents itself as a remote target but
>>>>> works on the local filesystem.  gdb_bfd_open contained a
>>>>> special case to make vgdb work with "target:" sysroots, but
>>>>> the implementation meant that GDB would fall back to the local
>>>>> filesystem if *any* to_fileio_open method failed with ENOSYS
>>>>> for *any* reason.
>>>>
>>>> Can you give an example target where we'd want this to behave
>>>> differently?
>>>>
>>>> E.g,. what should happen with "target sim" ?
>>>
>>> I don't understand what you're asking.  "target sim" doesn't use
>>> remote_filesystem_is_local, (to_filesystem_is_local for sim is the
>>> default, tdefault_filesystem_is_local, which returns 1 always).
>>
>> When you say:
>>
>>  gdb_bfd_open contained a special case
>>  to make vgdb work with "target:" sysroots, but the implementation
>>  meant that GDB would fall back to the local filesystem if *any*
>>  to_fileio_open method failed with ENOSYS for *any* reason.
>>
>> I'd prefer to get an example target for one of those "if *any*
>> to_fileio_open ... *any* reason".  I'd like to understand the
>> real motivation for the change.  Because otherwise I get to
>> wonder why would we handle any other target that goes through
>> this path differently.
> 
> Ah, ok, I get what you're asking.
> 
> In what's upstream right now, the only path (I think) that you
> can get to the point in gdb_bfd_open with the workaround is if
> you're using a remote target that doesn't support file retrieval.
> But, in the namespace-awareness series I posted, target_fileio_open
> can fail with ENOSYS if setns is not available.  That's the reason
> I made the change.

I'm still confused on that rationale, as it leaves one
important detail out: when target_fileio_open fails with ENOSYS
because setns is not available, I assume that gdb falls back
to the local filesystem.  But isn't that what should happen?

After your patch, we'll issue remote_hostio_open from within
remote_filesystem_is_local, and if the remote side doesn't support
setns, we'll get ENOSYS to "open", and thus fallback to local
anyway?

In any case, (I have yet to go read your v2 of that series), it
sounds wrong suspicious to return ENOSYS for that case.  ENOSYS would
indicate that "open" is not implemented, but that doesn't sound like the
case you have.  "open" is indeed implemented, but it happens that it
can't satisfy the requested path.  Thus, something like EINVAL,
EOPNOTSUPP or ENODEV may be more appropriate.

As I said, I agree with moving the checks to target_filesystem_is_local
time, but for a different rationale.

Thanks,
Pedro Alves
  

Pedro Alves wrote:
> On 05/15/2015 02:19 PM, Gary Benson wrote:
> > Pedro Alves wrote:
> > > When you say:
> > >
> > >  gdb_bfd_open contained a special case to make vgdb work with
> > >  "target:" sysroots, but the implementation meant that GDB would
> > >  fall back to the local filesystem if *any* to_fileio_open
> > >  method failed with ENOSYS for *any* reason.
> > >
> > > I'd prefer to get an example target for one of those "if *any*
> > > to_fileio_open ... *any* reason".  I'd like to understand the
> > > real motivation for the change.  Because otherwise I get to
> > > wonder why would we handle any other target that goes through
> > > this path differently.
> > 
> > Ah, ok, I get what you're asking.
> > 
> > In what's upstream right now, the only path (I think) that you can
> > get to the point in gdb_bfd_open with the workaround is if you're
> > using a remote target that doesn't support file retrieval.  But,
> > in the namespace-awareness series I posted, target_fileio_open can
> > fail with ENOSYS if setns is not available.  That's the reason I
> > made the change.
> 
> I'm still confused on that rationale, as it leaves one important
> detail out: when target_fileio_open fails with ENOSYS because setns
> is not available, I assume that gdb falls back to the local
> filesystem.  But isn't that what should happen?
> 
> After your patch, we'll issue remote_hostio_open from within
> remote_filesystem_is_local, and if the remote side doesn't support
> setns, we'll get ENOSYS to "open", and thus fallback to local
> anyway?

I'm trying to catch the specific case that a) you're using a remote
target, b) that doesn't support file retrieval, and c) the user has
not set any sysroot.  In that case the user is presumably using a
"remote" client that operates on the local filesystem... so GDB should
access the local filesystem.

For any other target_fileio_open failures GDB should not continue.
For example, the user attaches to a process in a container, and that
process's executable is "/bin/bash".  If GDB can't open /bin/bash
_in_that_container_ (because setns isn't implemented) then GDB should
not try to access /bin/bash in it's own container.  They might be
different files.

> In any case, (I have yet to go read your v2 of that series), it
> sounds wrong suspicious to return ENOSYS for that case.  ENOSYS
> would indicate that "open" is not implemented, but that doesn't
> sound like the case you have.  "open" is indeed implemented, but it
> happens that it can't satisfy the requested path.  Thus, something
> like EINVAL, EOPNOTSUPP or ENODEV may be more appropriate.

EOPNOTSUPP is for sockets I think.  ENODEV seems a better match than
EINVAL, but I don't really have a good feel for how these are used
in other cases.

I could change the subsequent series to detech ENOSYS from setns
and return with errno == ENODEV or EINVAL if you like?

> As I said, I agree with moving the checks to
> target_filesystem_is_local time, but for a different rationale.

Ok.

Cheers,
Gary
  

Gary Benson wrote:
> Pedro Alves wrote:
> > On 05/15/2015 02:19 PM, Gary Benson wrote:
> > > Pedro Alves wrote:
> > > > When you say:
> > > >
> > > >  gdb_bfd_open contained a special case to make vgdb work with
> > > >  "target:" sysroots, but the implementation meant that GDB
> > > >  would fall back to the local filesystem if *any*
> > > >  to_fileio_open method failed with ENOSYS for *any* reason.
> > > >
> > > > I'd prefer to get an example target for one of those "if *any*
> > > > to_fileio_open ... *any* reason".  I'd like to understand the
> > > > real motivation for the change.  Because otherwise I get to
> > > > wonder why would we handle any other target that goes through
> > > > this path differently.
> > > 
> > > In what's upstream right now, the only path (I think) that you
> > > can get to the point in gdb_bfd_open with the workaround is if
> > > you're using a remote target that doesn't support file
> > > retrieval.  But, in the namespace-awareness series I posted,
> > > target_fileio_open can fail with ENOSYS if setns is not
> > > available.  That's the reason I made the change.
> > 
> > I'm still confused on that rationale, as it leaves one important
> > detail out: when target_fileio_open fails with ENOSYS because
> > setns is not available, I assume that gdb falls back to the local
> > filesystem.  But isn't that what should happen?
> > 
> > After your patch, we'll issue remote_hostio_open from within
> > remote_filesystem_is_local, and if the remote side doesn't support
> > setns, we'll get ENOSYS to "open", and thus fallback to local
> > anyway?
> 
> I'm trying to catch the specific case that a) you're using a remote
> target, b) that doesn't support file retrieval, and c) the user has
> not set any sysroot.  In that case the user is presumably using a
> "remote" client that operates on the local filesystem... so GDB
> should access the local filesystem.
> 
> For any other target_fileio_open failures GDB should not continue.
> For example, the user attaches to a process in a container, and that
> process's executable is "/bin/bash".  If GDB can't open /bin/bash
> _in_that_container_ (because setns isn't implemented) then GDB
> should not try to access /bin/bash in it's own container.  They
> might be different files.

FWIW I've pushed the patch to move the special case, I'll address
the other stuff with the mount namespaces series.

Cheers,
Gary