Re: [PATCH 1/2] Warn when accessing binaries over RSP

Re: [PATCH 1/2] Warn when accessing binaries over RSP

[Doug Evans]

Gary Benson writes:
  > Andrew Burgess wrote:
  > > * Gary Benson <gbenson@redhat.com> [2015-08-05 16:28:15 +0100]:
  > > >
  > > > diff --git a/gdb/gdb_bfd.c b/gdb/gdb_bfd.c
  > > > index 1781d80..b511777 100644
  > > > --- a/gdb/gdb_bfd.c
  > > > +++ b/gdb/gdb_bfd.c
  > > > @@ -219,13 +219,38 @@ gdb_bfd_iovec_fileio_open (struct bfd *abfd,  
void *inferior)
  > > >    const char *filename = bfd_get_filename (abfd);
  > > >    int fd, target_errno;
  > > >    int *stream;
  > > > +  struct target_ops *ops = find_target_at (process_stratum);
  > > >
  > > >    gdb_assert (is_target_filename (filename));
  > > > +  filename += strlen (TARGET_SYSROOT_PREFIX);
  > > > +
  > > > +  /* GDB provides no indicator of progress during file operations,  
and
  > > > +     can appear to have locked up during slow remote transfers, so  
we
  > > > +     inform the user what is happening and suggest a way out.  It's
  > > > +     unpleasant that we need to detect remote targets this way  
(rather
  > > > +     than putting the warnings in remote_hostio_open), but it's not
  > > > +     possible for remote_hostio_open to differentiate between
  > > > +     accessing inferior binaries (which can be bypassed) and  
accessing
  > > > +     things like /proc/ (which is unavoidable).  */
  > > > +  if (strcmp (ops->to_shortname, "remote") == 0
  > > > +      || strcmp (ops->to_shortname, "extended-remote") == 0)
  > > > +    {
  > > > +      static int warning_issued = 0;
  > > > +
  > > > +      printf_unfiltered (_("Reading %s from remote target\n"),
  > > > +			 filename);
  > > > +
  > > > +      if (!warning_issued)
  > > > +	{
  > > > +	  warning (_("File transfers from remote targets can be slow.\n"
  > > > +		     "Use \"set sysroot\" to access files locally"
  > > > +		     " instead."));
  > > > +	  warning_issued = 1;
  > > > +	}
  > > > +    }
  > >
  > > Altering the behaviour based on to_shortname feels like breaking
  > > this nice target OO model we have.
  >
  > Yeah... :-/
  >
  > > Could the warning not be moved down into target_fileio_open instead?
  >
  > Not so much target_fileio_open as remote_hostio_open; only remote
  > targets need the warning.  And originally I thought no, the warning
  > couldn't go there, because target_fileio_open/remote_hostio_open is
  > used for various internal things such as /proc/ file reads on Linux
  > that the user shouldn't see.
  >
  > ...however...
  >
  > remote_hostio_open *can* differentiate between reading inferior
  > binaries and reading internal stuff because the internal stuff is
  > accessed with the INF argument NULL and binaries are accessed with
  > a non-NULL INF.
  >
  > So I can do that, if it doesn't seem too hacky.
  >
  > > Or if that's really not an appropriate place should we add a new
  > > target method?
  >
  > I considered that but couldn't think of a good name :-)
  > target_fileio_warn_if_slow ??
  > I can do that too.

FAOD, target_fileio_open_warn_if_slow?

[PATCH 0/2] Better handling of slow remote transfers

[Gary Benson]

Hi all,

Since March or so GDB has been able to access inferior binaries for
remote targets without having to be explicitly told to.  This caused
problems for some people with slow connections:

  https://sourceware.org/ml/gdb/2015-07/msg00038.html

The first patch in this series adds the warning messages requested
in that thread.  The second commit should make long transfers
interruptible.

Built and regtested on RHEL 6.6 x86_64.

Ok to commit?

Thanks,
Gary

--
http://gbenson.net/

[PATCH 1/2] Warn when accessing binaries over RSP

[Gary Benson]

GDB provides no indicator of progress during file operations, and can
appear to have locked up during slow remote transfers.  This commit
updates GDB to print a warning each time a file is accessed over RSP.
An additional message detailing how to avoid remote transfers is
printed for the first transfer only.

gdb/ChangeLog:

	* gdb_bfd.c (gdb_bfd_iovec_fileio_open): Print warnings when
	BFDs are opened via the remote protocol.

gdb/testsuite/ChangeLog:

	* gdb.trace/pending.exp: Cope with remote transfer warnings.
---
 gdb/ChangeLog                       |    5 +++++
 gdb/gdb_bfd.c                       |   33 +++++++++++++++++++++++++++++----
 gdb/testsuite/ChangeLog             |    4 ++++
 gdb/testsuite/gdb.trace/pending.exp |    8 ++++----
 4 files changed, 42 insertions(+), 8 deletions(-)

diff --git a/gdb/gdb_bfd.c b/gdb/gdb_bfd.c
index 1781d80..b511777 100644
--- a/gdb/gdb_bfd.c
+++ b/gdb/gdb_bfd.c
@@ -219,13 +219,38 @@ gdb_bfd_iovec_fileio_open (struct bfd *abfd, void *inferior)
   const char *filename = bfd_get_filename (abfd);
   int fd, target_errno;
   int *stream;
+  struct target_ops *ops = find_target_at (process_stratum);
 
   gdb_assert (is_target_filename (filename));
+  filename += strlen (TARGET_SYSROOT_PREFIX);
+
+  /* GDB provides no indicator of progress during file operations, and
+     can appear to have locked up during slow remote transfers, so we
+     inform the user what is happening and suggest a way out.  It's
+     unpleasant that we need to detect remote targets this way (rather
+     than putting the warnings in remote_hostio_open), but it's not
+     possible for remote_hostio_open to differentiate between
+     accessing inferior binaries (which can be bypassed) and accessing
+     things like /proc/ (which is unavoidable).  */
+  if (strcmp (ops->to_shortname, "remote") == 0
+      || strcmp (ops->to_shortname, "extended-remote") == 0)
+    {
+      static int warning_issued = 0;
+
+      printf_unfiltered (_("Reading %s from remote target\n"),
+			 filename);
+
+      if (!warning_issued)
+	{
+	  warning (_("File transfers from remote targets can be slow.\n"
+		     "Use \"set sysroot\" to access files locally"
+		     " instead."));
+	  warning_issued = 1;
+	}
+    }
 
-  fd = target_fileio_open ((struct inferior *) inferior,
-			   filename + strlen (TARGET_SYSROOT_PREFIX),
-			   FILEIO_O_RDONLY, 0,
-			   &target_errno);
+  fd = target_fileio_open ((struct inferior *) inferior, filename,
+			   FILEIO_O_RDONLY, 0, &target_errno);
   if (fd == -1)
     {
       errno = fileio_errno_to_host (target_errno);
diff --git a/gdb/testsuite/gdb.trace/pending.exp b/gdb/testsuite/gdb.trace/pending.exp
index 0399807..68e8c7b 100644
--- a/gdb/testsuite/gdb.trace/pending.exp
+++ b/gdb/testsuite/gdb.trace/pending.exp
@@ -221,7 +221,7 @@ proc pending_tracepoint_resolved_during_trace { trace_type } \
 		fail $test
 	    }
 	}
-	-re "Continuing.\r\n\r\nBreakpoint.*marker.*at.*$srcfile.*$gdb_prompt $" {
+	-re "Continuing.\r\n(Reading .* from remote target\r\n)?\r\nBreakpoint.*marker.*at.*$srcfile.*$gdb_prompt $" {
 	    pass $test
 	}
     }
@@ -294,7 +294,7 @@ proc pending_tracepoint_installed_during_trace { trace_type } \
 		fail $test
 	    }
 	}
-	-re "Continuing.\r\n\r\nBreakpoint.*marker.*at.*$srcfile.*$gdb_prompt $" {
+	-re "Continuing.\r\n(Reading .* from remote target\r\n)?\r\nBreakpoint.*marker.*at.*$srcfile.*$gdb_prompt $" {
            pass $test
        }
     }
@@ -391,7 +391,7 @@ proc pending_tracepoint_disconnect_after_resolved { trace_type } \
 
     gdb_test "continue" "Continuing.\r\n\r\nBreakpoint.*marker.*at.*pending.c.*" \
 	"continue to marker 1"
-    gdb_test "continue" "Continuing.\r\n\r\nBreakpoint.*marker.*at.*pending.c.*" \
+    gdb_test "continue" "Continuing.\r\n(Reading .* from remote target\r\n)?\r\nBreakpoint.*marker.*at.*pending.c.*" \
 	"continue to marker 2"
 
     # There should be no pending tracepoint, so no warning should be emitted.
@@ -473,7 +473,7 @@ proc pending_tracepoint_with_action_resolved { trace_type } \
 		fail $test
             }
 	}
-	-re "Continuing.\r\n\r\nBreakpoint.*marker.*at.*$srcfile.*$gdb_prompt $" {
+	-re "Continuing.\r\n(Reading .* from remote target\r\n)?\r\nBreakpoint.*marker.*at.*$srcfile.*$gdb_prompt $" {
 	    pass "continue to marker 2"
 	}
 
-- 
1.7.1

Re: [PATCH 1/2] Warn when accessing binaries over RSP

[Andrew Burgess]

* Gary Benson <gbenson@redhat.com> [2015-08-05 16:28:15 +0100]:

> 
> diff --git a/gdb/gdb_bfd.c b/gdb/gdb_bfd.c
> index 1781d80..b511777 100644
> --- a/gdb/gdb_bfd.c
> +++ b/gdb/gdb_bfd.c
> @@ -219,13 +219,38 @@ gdb_bfd_iovec_fileio_open (struct bfd *abfd, void *inferior)
>    const char *filename = bfd_get_filename (abfd);
>    int fd, target_errno;
>    int *stream;
> +  struct target_ops *ops = find_target_at (process_stratum);
>  
>    gdb_assert (is_target_filename (filename));
> +  filename += strlen (TARGET_SYSROOT_PREFIX);
> +
> +  /* GDB provides no indicator of progress during file operations, and
> +     can appear to have locked up during slow remote transfers, so we
> +     inform the user what is happening and suggest a way out.  It's
> +     unpleasant that we need to detect remote targets this way (rather
> +     than putting the warnings in remote_hostio_open), but it's not
> +     possible for remote_hostio_open to differentiate between
> +     accessing inferior binaries (which can be bypassed) and accessing
> +     things like /proc/ (which is unavoidable).  */
> +  if (strcmp (ops->to_shortname, "remote") == 0
> +      || strcmp (ops->to_shortname, "extended-remote") == 0)
> +    {
> +      static int warning_issued = 0;
> +
> +      printf_unfiltered (_("Reading %s from remote target\n"),
> +			 filename);
> +
> +      if (!warning_issued)
> +	{
> +	  warning (_("File transfers from remote targets can be slow.\n"
> +		     "Use \"set sysroot\" to access files locally"
> +		     " instead."));
> +	  warning_issued = 1;
> +	}
> +    }

Altering the behaviour based on to_shortname feels like breaking this
nice target OO model we have.

Could the warning not be moved down into target_fileio_open instead?

Or if that's really not an appropriate place should we add a new
target method?

Thanks,
Andrew

Re: [PATCH 1/2] Warn when accessing binaries over RSP

[Gary Benson]

Andrew Burgess wrote:
> * Gary Benson <gbenson@redhat.com> [2015-08-05 16:28:15 +0100]:
> > 
> > diff --git a/gdb/gdb_bfd.c b/gdb/gdb_bfd.c
> > index 1781d80..b511777 100644
> > --- a/gdb/gdb_bfd.c
> > +++ b/gdb/gdb_bfd.c
> > @@ -219,13 +219,38 @@ gdb_bfd_iovec_fileio_open (struct bfd *abfd, void *inferior)
> >    const char *filename = bfd_get_filename (abfd);
> >    int fd, target_errno;
> >    int *stream;
> > +  struct target_ops *ops = find_target_at (process_stratum);
> >  
> >    gdb_assert (is_target_filename (filename));
> > +  filename += strlen (TARGET_SYSROOT_PREFIX);
> > +
> > +  /* GDB provides no indicator of progress during file operations, and
> > +     can appear to have locked up during slow remote transfers, so we
> > +     inform the user what is happening and suggest a way out.  It's
> > +     unpleasant that we need to detect remote targets this way (rather
> > +     than putting the warnings in remote_hostio_open), but it's not
> > +     possible for remote_hostio_open to differentiate between
> > +     accessing inferior binaries (which can be bypassed) and accessing
> > +     things like /proc/ (which is unavoidable).  */
> > +  if (strcmp (ops->to_shortname, "remote") == 0
> > +      || strcmp (ops->to_shortname, "extended-remote") == 0)
> > +    {
> > +      static int warning_issued = 0;
> > +
> > +      printf_unfiltered (_("Reading %s from remote target\n"),
> > +			 filename);
> > +
> > +      if (!warning_issued)
> > +	{
> > +	  warning (_("File transfers from remote targets can be slow.\n"
> > +		     "Use \"set sysroot\" to access files locally"
> > +		     " instead."));
> > +	  warning_issued = 1;
> > +	}
> > +    }
> 
> Altering the behaviour based on to_shortname feels like breaking
> this nice target OO model we have.

Yeah... :-/

> Could the warning not be moved down into target_fileio_open instead?

Not so much target_fileio_open as remote_hostio_open; only remote
targets need the warning.  And originally I thought no, the warning
couldn't go there, because target_fileio_open/remote_hostio_open is
used for various internal things such as /proc/ file reads on Linux
that the user shouldn't see.

...however...

remote_hostio_open *can* differentiate between reading inferior
binaries and reading internal stuff because the internal stuff is
accessed with the INF argument NULL and binaries are accessed with
a non-NULL INF.

So I can do that, if it doesn't seem too hacky.

> Or if that's really not an appropriate place should we add a new
> target method?

I considered that but couldn't think of a good name :-)
target_fileio_warn_if_slow ??
I can do that too.

Cheers,
Gary

-- 
http://gbenson.net/