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

[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/

[PATCH v2] Warn when accessing binaries from remote targets

[Gary Benson]

Hi all,

This is an updated version of the patch I previously mailed to add
a warning message when GDB initiates transfers from remote targets.
This version adds a new function, target_fileio_open_warn_if_slow,
as suggested by Andrew and Doug, and moves the message printing
code into remote_hostio_open.

Is this ok to commit?  To HEAD?  To the branch?  To both?

Cheers,
Gary

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

	* target.h (struct target_ops) <to_fileio_open>: New argument
	warn_if_slow.  Update comment.  All implementations updated.
	(target_fileio_open_warn_if_slow): New declaration.
	* target.c (target_fileio_open): Renamed as...
	(target_fileio_open_1): ...this.  New argument warn_if_slow.
	Pass warn_if_slow to implementation.  Update debug printing.
	(target_fileio_open): New function.
	(target_fileio_open_warn_if_slow): Likewise.
	* gdb_bfd.c (gdb_bfd_iovec_fileio_open): Use new function
	target_fileio_open_warn_if_slow.

gdb/testsuite/ChangeLog:

	* gdb.trace/pending.exp: Cope with remote transfer warnings.
---
 gdb/ChangeLog                       |   13 ++++++++++++
 gdb/gdb_bfd.c                       |    9 ++++---
 gdb/inf-child.c                     |    3 +-
 gdb/linux-nat.c                     |    3 +-
 gdb/remote.c                        |   26 ++++++++++++++++++++---
 gdb/target.c                        |   38 ++++++++++++++++++++++++++++------
 gdb/target.h                        |   17 ++++++++++++--
 gdb/testsuite/ChangeLog             |    4 +++
 gdb/testsuite/gdb.trace/pending.exp |    8 +++---
 9 files changed, 97 insertions(+), 24 deletions(-)

diff --git a/gdb/gdb_bfd.c b/gdb/gdb_bfd.c
index 1781d80..264b611 100644
--- a/gdb/gdb_bfd.c
+++ b/gdb/gdb_bfd.c
@@ -222,10 +222,11 @@ gdb_bfd_iovec_fileio_open (struct bfd *abfd, void *inferior)
 
   gdb_assert (is_target_filename (filename));
 
-  fd = target_fileio_open ((struct inferior *) inferior,
-			   filename + strlen (TARGET_SYSROOT_PREFIX),
-			   FILEIO_O_RDONLY, 0,
-			   &target_errno);
+  fd = target_fileio_open_warn_if_slow ((struct inferior *) inferior,
+					filename
+					+ strlen (TARGET_SYSROOT_PREFIX),
+					FILEIO_O_RDONLY, 0,
+					&target_errno);
   if (fd == -1)
     {
       errno = fileio_errno_to_host (target_errno);
diff --git a/gdb/inf-child.c b/gdb/inf-child.c
index 0326a93..ada570d 100644
--- a/gdb/inf-child.c
+++ b/gdb/inf-child.c
@@ -209,7 +209,8 @@ inf_child_pid_to_exec_file (struct target_ops *self, int pid)
 static int
 inf_child_fileio_open (struct target_ops *self,
 		       struct inferior *inf, const char *filename,
-		       int flags, int mode, int *target_errno)
+		       int flags, int mode, int warn_if_slow,
+		       int *target_errno)
 {
   int nat_flags;
   mode_t nat_mode;
diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
index be7a915..f21b1c5 100644
--- a/gdb/linux-nat.c
+++ b/gdb/linux-nat.c
@@ -4920,7 +4920,8 @@ linux_nat_fileio_pid_of (struct inferior *inf)
 static int
 linux_nat_fileio_open (struct target_ops *self,
 		       struct inferior *inf, const char *filename,
-		       int flags, int mode, int *target_errno)
+		       int flags, int mode, int warn_if_slow,
+		       int *target_errno)
 {
   int nat_flags;
   mode_t nat_mode;
diff --git a/gdb/remote.c b/gdb/remote.c
index ca1f0df..d9a8bb7 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -10359,12 +10359,29 @@ remote_hostio_set_filesystem (struct inferior *inf, int *remote_errno)
 static int
 remote_hostio_open (struct target_ops *self,
 		    struct inferior *inf, const char *filename,
-		    int flags, int mode, int *remote_errno)
+		    int flags, int mode, int warn_if_slow,
+		    int *remote_errno)
 {
   struct remote_state *rs = get_remote_state ();
   char *p = rs->buf;
   int left = get_remote_packet_size () - 1;
 
+  if (warn_if_slow)
+    {
+      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."
+		     " Use \"set sysroot\" with no arguments to access"
+		     " files locally instead."));
+	  warning_issued = 1;
+	}
+    }
+
   if (remote_hostio_set_filesystem (inf, remote_errno) != 0)
     return -1;
 
@@ -10613,7 +10630,7 @@ remote_filesystem_is_local (struct target_ops *self)
 	     filename is irrelevant, we only care about whether
 	     the stub recognizes the packet or not.  */
 	  fd = remote_hostio_open (self, NULL, "just probing",
-				   FILEIO_O_RDONLY, 0700,
+				   FILEIO_O_RDONLY, 0700, 0,
 				   &remote_errno);
 
 	  if (fd >= 0)
@@ -10735,7 +10752,7 @@ remote_file_put (const char *local_file, const char *remote_file, int from_tty)
   fd = remote_hostio_open (find_target_at (process_stratum), NULL,
 			   remote_file, (FILEIO_O_WRONLY | FILEIO_O_CREAT
 					 | FILEIO_O_TRUNC),
-			   0700, &remote_errno);
+			   0700, 0, &remote_errno);
   if (fd == -1)
     remote_hostio_error (remote_errno);
 
@@ -10819,7 +10836,8 @@ remote_file_get (const char *remote_file, const char *local_file, int from_tty)
     error (_("command can only be used with remote target"));
 
   fd = remote_hostio_open (find_target_at (process_stratum), NULL,
-			   remote_file, FILEIO_O_RDONLY, 0, &remote_errno);
+			   remote_file, FILEIO_O_RDONLY, 0, 0,
+			   &remote_errno);
   if (fd == -1)
     remote_hostio_error (remote_errno);
 
diff --git a/gdb/target.c b/gdb/target.c
index e41a338..a0a55d9 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -2779,11 +2779,13 @@ release_fileio_fd (int fd, fileio_fh_t *fh)
 #define fileio_fd_to_fh(fd) \
   VEC_index (fileio_fh_t, fileio_fhandles, (fd))
 
-/* See target.h.  */
+/* Helper for target_fileio_open and
+   target_fileio_open_warn_if_slow.  */
 
-int
-target_fileio_open (struct inferior *inf, const char *filename,
-		    int flags, int mode, int *target_errno)
+static int
+target_fileio_open_1 (struct inferior *inf, const char *filename,
+		      int flags, int mode, int warn_if_slow,
+		      int *target_errno)
 {
   struct target_ops *t;
 
@@ -2792,7 +2794,7 @@ target_fileio_open (struct inferior *inf, const char *filename,
       if (t->to_fileio_open != NULL)
 	{
 	  int fd = t->to_fileio_open (t, inf, filename, flags, mode,
-				      target_errno);
+				      warn_if_slow, target_errno);
 
 	  if (fd < 0)
 	    fd = -1;
@@ -2801,11 +2803,12 @@ target_fileio_open (struct inferior *inf, const char *filename,
 
 	  if (targetdebug)
 	    fprintf_unfiltered (gdb_stdlog,
-				"target_fileio_open (%d,%s,0x%x,0%o)"
+				"target_fileio_open (%d,%s,0x%x,0%o,%d)"
 				" = %d (%d)\n",
 				inf == NULL ? 0 : inf->num,
 				filename, flags, mode,
-				fd, fd != -1 ? 0 : *target_errno);
+				warn_if_slow, fd,
+				fd != -1 ? 0 : *target_errno);
 	  return fd;
 	}
     }
@@ -2817,6 +2820,27 @@ target_fileio_open (struct inferior *inf, const char *filename,
 /* See target.h.  */
 
 int
+target_fileio_open (struct inferior *inf, const char *filename,
+		    int flags, int mode, int *target_errno)
+{
+  return target_fileio_open_1 (inf, filename, flags, mode, 0,
+			       target_errno);
+}
+
+/* See target.h.  */
+
+int
+target_fileio_open_warn_if_slow (struct inferior *inf,
+				 const char *filename,
+				 int flags, int mode, int *target_errno)
+{
+  return target_fileio_open_1 (inf, filename, flags, mode, 1,
+			       target_errno);
+}
+
+/* See target.h.  */
+
+int
 target_fileio_pwrite (int fd, const gdb_byte *write_buf, int len,
 		      ULONGEST offset, int *target_errno)
 {
diff --git a/gdb/target.h b/gdb/target.h
index e283c86..1fdaf00 100644
--- a/gdb/target.h
+++ b/gdb/target.h
@@ -873,11 +873,14 @@ struct target_ops
     /* Open FILENAME on the target, in the filesystem as seen by INF,
        using FLAGS and MODE.  If INF is NULL, use the filesystem seen
        by the debugger (GDB or, for remote targets, the remote stub).
-       Return a target file descriptor, or -1 if an error occurs (and
-       set *TARGET_ERRNO).  */
+       If WARN_IF_SLOW is nonzero, print a warning message if the file
+       is being accessed over a link that may be slow.  Return a
+       target file descriptor, or -1 if an error occurs (and set
+       *TARGET_ERRNO).  */
     int (*to_fileio_open) (struct target_ops *,
 			   struct inferior *inf, const char *filename,
-			   int flags, int mode, int *target_errno);
+			   int flags, int mode, int warn_if_slow,
+			   int *target_errno);
 
     /* Write up to LEN bytes from WRITE_BUF to FD on the target.
        Return the number of bytes written, or -1 if an error occurs
@@ -2007,6 +2010,14 @@ extern int target_fileio_open (struct inferior *inf,
 			       const char *filename, int flags,
 			       int mode, int *target_errno);
 
+/* Like target_fileio_open, but print a warning message if the
+   file is being accessed over a link that may be slow.  */
+extern int target_fileio_open_warn_if_slow (struct inferior *inf,
+					    const char *filename,
+					    int flags,
+					    int mode,
+					    int *target_errno);
+
 /* Write up to LEN bytes from WRITE_BUF to FD on the target.
    Return the number of bytes written, or -1 if an error occurs
    (and set *TARGET_ERRNO).  */
diff --git a/gdb/testsuite/gdb.trace/pending.exp b/gdb/testsuite/gdb.trace/pending.exp
index 0399807..9938c5a 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 v2] Warn when accessing binaries from remote targets

[Andrew Burgess]

* Gary Benson <gbenson@redhat.com> [2015-08-13 14:23:59 +0100]:

> This version adds a new function, target_fileio_open_warn_if_slow,
> as suggested by Andrew and Doug, and moves the message printing
> code into remote_hostio_open.

I'm not a maintainer, but this revision addresses my earlier concerns.

Thanks,
Andrew

Re: [PATCH v2] Warn when accessing binaries from remote targets

[Pedro Alves]

On 08/13/2015 02:23 PM, Gary Benson wrote:

> +  if (warn_if_slow)
> +    {
> +      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."
> +		     " Use \"set sysroot\" with no arguments to access"
> +		     " files locally instead."));

I wonder whether "with no arguments" is necessary here.  I'd suggest dropping
that bit.  It think it may even be confusing, as what the user should do is
point at a local copy of the target filesystem, using "set sysroot /path/to/copy".
The host's filesystem is not necessarily the correct one, and often isn't.

Otherwise this looks good to me.

Thanks,
Pedro Alves

[pushed] Warn when accessing binaries from remote targets

[Gary Benson]

Pedro Alves wrote:
> On 08/13/2015 02:23 PM, Gary Benson wrote:
> > +  if (warn_if_slow)
> > +    {
> > +      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."
> > +		     " Use \"set sysroot\" with no arguments to access"
> > +		     " files locally instead."));
> 
> I wonder whether "with no arguments" is necessary here.  I'd suggest
> dropping that bit.  It think it may even be confusing, as what the
> user should do is point at a local copy of the target filesystem,
> using "set sysroot /path/to/copy".  The host's filesystem is not
> necessarily the correct one, and often isn't.
> 
> Otherwise this looks good to me.

Pushed to master and 7.10 with that change.

Thanks,
Gary

-- 
http://gbenson.net/

[PATCH 2/2] Make remote file transfers interruptible

[Gary Benson]

This commit makes it possible to interrupt slow remote file transfers.

gdb/ChangeLog:

	* gdb_bfd.c (gdb_bfd_iovec_fileio_pread): Add QUIT call.
---
 gdb/ChangeLog |    4 ++++
 gdb/gdb_bfd.c |    2 ++
 2 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/gdb/gdb_bfd.c b/gdb/gdb_bfd.c
index b511777..575bd61 100644
--- a/gdb/gdb_bfd.c
+++ b/gdb/gdb_bfd.c
@@ -277,6 +277,8 @@ gdb_bfd_iovec_fileio_pread (struct bfd *abfd, void *stream, void *buf,
   pos = 0;
   while (nbytes > pos)
     {
+      QUIT;
+
       bytes = target_fileio_pread (fd, (gdb_byte *) buf + pos,
 				   nbytes - pos, offset + pos,
 				   &target_errno);
-- 
1.7.1

Re: [PATCH 2/2] Make remote file transfers interruptible

[Sandra Loosemore]

On 08/05/2015 09:28 AM, Gary Benson wrote:
> This commit makes it possible to interrupt slow remote file transfers.
>
> gdb/ChangeLog:
>
> 	* gdb_bfd.c (gdb_bfd_iovec_fileio_pread): Add QUIT call.

I think that this is the same patch that you sent earlier:

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

It still does not work for me.  :-(

-Sandra

Re: [PATCH 2/2] Make remote file transfers interruptible

[Gary Benson]

Hi Sandra,

Sandra Loosemore wrote:
> On 08/05/2015 09:28 AM, Gary Benson wrote:
> > This commit makes it possible to interrupt slow remote file transfers.
> > 
> > gdb/ChangeLog:
> > 
> > 	* gdb_bfd.c (gdb_bfd_iovec_fileio_pread): Add QUIT call.
> 
> I think that this is the same patch that you sent earlier:
> 
> https://sourceware.org/ml/gdb/2015-07/msg00043.html
> 
> It still does not work for me.  :-(

Ok, given the packet-size limit of RSP I was assuming GDB would read
the files in chunks the size of the server's packet buffer size (16k
for gdbserver) but it seems that that chunking happens at a lower
level, so the interrupt is only noticed between files rather than
between chunks.  I tested this patch using "remote get", but it seems
that that operates differently.  So I'm retracting this patch.

Making remote transfers properly interruptible looks like a fairly
involved task that's only slightly on my radar.  I've filed a bug
(https://sourceware.org/bugzilla/show_bug.cgi?id=18804) to keep the
issue from being forgotten.  I'll work on if I have some free time,
or someone else can do it.

The other patch in the series (the warning messages) can go in if
people find it useful.

Thanks,
Gary

-- 
http://gbenson.net/

[PATCH] Make remote transfers interruptible

[Gary Benson]

Hi Sandra,

Sandra Loosemore wrote:
> On 08/05/2015 09:28 AM, Gary Benson wrote:
> > This commit makes it possible to interrupt slow remote file transfers.
> >
> > gdb/ChangeLog:
> >
> > 	* gdb_bfd.c (gdb_bfd_iovec_fileio_pread): Add QUIT call.
> 
> It still does not work for me.  :-(

Could you please try this newer version and see if it allows you to
interrupt the remote transfers?

Thanks,
Gary

---
diff --git a/gdb/remote.c b/gdb/remote.c
index 69da508..7db1e25 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -10378,12 +10378,11 @@ remote_hostio_pwrite (struct target_ops *self,
 				     remote_errno, NULL, NULL);
 }
 
-/* Implementation of to_fileio_pread.  */
+/* Helper for remote_hostio_pread.  */
 
 static int
-remote_hostio_pread (struct target_ops *self,
-		     int fd, gdb_byte *read_buf, int len,
-		     ULONGEST offset, int *remote_errno)
+remote_hostio_pread_1 (int fd, gdb_byte *read_buf, int len,
+		       ULONGEST offset, int *remote_errno)
 {
   struct remote_state *rs = get_remote_state ();
   char *p = rs->buf;
@@ -10417,6 +10416,37 @@ remote_hostio_pread (struct target_ops *self,
   return ret;
 }
 
+/* Implementation of to_fileio_pread.  */
+
+static int
+remote_hostio_pread (struct target_ops *self,
+		     int fd, gdb_byte *read_buf, int len,
+		     ULONGEST offset, int *remote_errno)
+{
+  gdb_byte *buf = read_buf;
+
+  while (len > 0)
+    {
+      int ret;
+
+      QUIT;
+
+      ret = remote_hostio_pread_1 (fd, buf, min (len, 4096), offset,
+				   remote_errno);
+      if (ret < 0)
+	return ret;
+
+      if (ret == 0)
+	break;
+
+      buf += ret;
+      offset += ret;
+      len -= ret;
+    }
+
+  return buf - read_buf;
+}
+
 /* Implementation of to_fileio_close.  */
 
 static int
-- 
1.7.1

Re: [PATCH] Make remote transfers interruptible

[Sandra Loosemore]

On 08/12/2015 08:30 AM, Gary Benson wrote:
> Hi Sandra,
>
> Sandra Loosemore wrote:
>> On 08/05/2015 09:28 AM, Gary Benson wrote:
>>> This commit makes it possible to interrupt slow remote file transfers.
>>>
>>> gdb/ChangeLog:
>>>
>>> 	* gdb_bfd.c (gdb_bfd_iovec_fileio_pread): Add QUIT call.
>>
>> It still does not work for me.  :-(
>
> Could you please try this newer version and see if it allows you to
> interrupt the remote transfers?

This version still doesn't make the transfer interruptable with ^C. 
*But*, with this patch, the startup time is reduced from 4 minutes to 19 
seconds.  Huh?  Is it really transferring the entire file contents, or 
was the time being used for some GDB-side operation that is quadratic or 
exponential in the size of the read requested rather than the actual 
byte transfer?  Independently of the ^C issue, I think we need to better 
understand what is going on here and better tune the code on both sides 
of the RSP for large file transfers.  Even if a user asks for 
target-side libraries explicitly, 4 minutes to transfer one library 
doesn't provide a good user experience, and 19 seconds isn't so great 
either when you consider that some interactive applications link with 
dozens of GUI or multimedia libraries and not just glibc.

-Sandra

Re: [PATCH] Make remote transfers interruptible

[Doug Evans]

On Wed, Aug 12, 2015 at 10:31 AM, Sandra Loosemore
<sandra@codesourcery.com> wrote:
> On 08/12/2015 08:30 AM, Gary Benson wrote:
>>
>> Hi Sandra,
>>
>> Sandra Loosemore wrote:
>>>
>>> On 08/05/2015 09:28 AM, Gary Benson wrote:
>>>>
>>>> This commit makes it possible to interrupt slow remote file transfers.
>>>>
>>>> gdb/ChangeLog:
>>>>
>>>>         * gdb_bfd.c (gdb_bfd_iovec_fileio_pread): Add QUIT call.
>>>
>>>
>>> It still does not work for me.  :-(
>>
>>
>> Could you please try this newer version and see if it allows you to
>> interrupt the remote transfers?
>
>
> This version still doesn't make the transfer interruptable with ^C. *But*,
> with this patch, the startup time is reduced from 4 minutes to 19 seconds.
> Huh?  Is it really transferring the entire file contents, or was the time
> being used for some GDB-side operation that is quadratic or exponential in
> the size of the read requested rather than the actual byte transfer?
> Independently of the ^C issue, I think we need to better understand what is
> going on here and better tune the code on both sides of the RSP for large
> file transfers.  Even if a user asks for target-side libraries explicitly, 4
> minutes to transfer one library doesn't provide a good user experience, and
> 19 seconds isn't so great either when you consider that some interactive
> applications link with dozens of GUI or multimedia libraries and not just
> glibc.

Dozens?  How about thousands.  1/2 :-)

[just want to make sure such cases are in the mindset of the community]

Re: [PATCH] Make remote transfers interruptible

[Gary Benson]

Sandra Loosemore wrote:
> On 08/12/2015 08:30 AM, Gary Benson wrote:
> >Sandra Loosemore wrote:
> > >On 08/05/2015 09:28 AM, Gary Benson wrote:
> > > > This commit makes it possible to interrupt slow remote file
> > > > transfers.
> > > >
> > > > gdb/ChangeLog:
> > > >
> > > >	* gdb_bfd.c (gdb_bfd_iovec_fileio_pread): Add QUIT call.
> > >
> > > It still does not work for me.  :-(
> >
> > Could you please try this newer version and see if it allows you
> > to interrupt the remote transfers?
> 
> This version still doesn't make the transfer interruptable with ^C.

Then I am out of ideas :( I can interrupt on my setup with either
patch (I tested it by adding a 100ms usleep in gdbserver/hostio.c
handle_pread).

I will push a warning-printing patch to HEAD, so that users in your
situation at least have some feedback on what is happening.  Could
you please look at making the transfers interruptible?

Joel, would you like me to also push it to the 7.10 branch, or will
you do that?

> *But*, with this patch, the startup time is reduced from 4 minutes
> to 19 seconds.  Huh?  Is it really transferring the entire file
> contents, or was the time being used for some GDB-side operation
> that is quadratic or exponential in the size of the read requested
> rather than the actual byte transfer?  Independently of the ^C
> issue, I think we need to better understand what is going on here
> and better tune the code on both sides of the RSP for large file
> transfers.

That's pretty weird.  It sounds like there's real scope for you
to improve performance here.

Thanks,
Gary

-- 
http://gbenson.net/

Re: [PATCH] Make remote transfers interruptible

[Joel Brobecker]

> Joel, would you like me to also push it to the 7.10 branch, or will
> you do that?

Just answering in a general way: Any Global Maintainer has authority
to approve a patch for a release branch; and once approve, you can
cherry-pick yourself and apply to the branch. Not a problem.

-- 
Joel

Re: [PATCH] Make remote transfers interruptible

[Pedro Alves]

On 08/12/2015 06:31 PM, Sandra Loosemore wrote:
> On 08/12/2015 08:30 AM, Gary Benson wrote:
>> Hi Sandra,
>>
>> Sandra Loosemore wrote:
>>> On 08/05/2015 09:28 AM, Gary Benson wrote:
>>>> This commit makes it possible to interrupt slow remote file transfers.
>>>>
>>>> gdb/ChangeLog:
>>>>
>>>> 	* gdb_bfd.c (gdb_bfd_iovec_fileio_pread): Add QUIT call.
>>>
>>> It still does not work for me.  :-(
>>
>> Could you please try this newer version and see if it allows you to
>> interrupt the remote transfers?
> 
> This version still doesn't make the transfer interruptable with ^C. 
> *But*, with this patch, the startup time is reduced from 4 minutes to 19 
> seconds.  Huh?  Is it really transferring the entire file contents, or 
> was the time being used for some GDB-side operation that is quadratic or 
> exponential in the size of the read requested rather than the actual 
> byte transfer?  Independently of the ^C issue, I think we need to better 
> understand what is going on here and better tune the code on both sides 
> of the RSP for large file transfers.  Even if a user asks for 
> target-side libraries explicitly, 4 minutes to transfer one library 
> doesn't provide a good user experience, and 19 seconds isn't so great 
> either when you consider that some interactive applications link with 
> dozens of GUI or multimedia libraries and not just glibc.
> 

Now that's quite surprising.  It seems to make no difference to me.

Granted, I'm testing on the local machine, but, attaching to Firefox,
with:

 $ ./gdbserver/gdbserver --multi :9999

and then:

 $ ./gdb -data-directory=data-directory -ex "tar extended-rem :9999" -ex "set pagination off" -ex "attach 31613" -ex "info shared" -ex "set confirm off" -ex "detach" -ex "quit"

takes around 3-5 seconds with or without patch.  Around one second less if I do (add "set sysroot /"):

 $ ./gdb -data-directory=data-directory -ex "set sysroot /" -ex "tar extended-rem :9999" -ex "set pagination off" -ex "attach 31613" -ex "info shared" -ex "set confirm off" -ex "detach" -ex "quit"

I was previously assuming you were seeing this on multiple machines,
but looking back, I only find mention of nios2.

Could it be the slowdown you see is caused by some other local patches
you might have in the tree you're using?  Do you see it with pristine FSF?

If you try your example test with gdb 7.9, with "set sysroot remote:",
does it also take the 4 minutes to reach main?

Also, can you reproduce this with other machines?  E.g., what about
x86_64 GNU/Linux?  Wondering whether it's a kernel/libc/etc issue...

Does anyone else confirm the unexpected slowness Sandra is seeing?

Thanks,
Pedro Alves

Re: [PATCH] Make remote transfers interruptible

[Sandra Loosemore]

On 08/17/2015 10:00 AM, Pedro Alves wrote:
>
> I was previously assuming you were seeing this on multiple machines,
> but looking back, I only find mention of nios2.
>
> Could it be the slowdown you see is caused by some other local patches
> you might have in the tree you're using?  Do you see it with pristine FSF?

I initially saw this in a local branch, but I have been very careful to 
reproduce and test things with an unmodified FSF checkout.

> If you try your example test with gdb 7.9, with "set sysroot remote:",
> does it also take the 4 minutes to reach main?
>
> Also, can you reproduce this with other machines?  E.g., what about
> x86_64 GNU/Linux?  Wondering whether it's a kernel/libc/etc issue...

The other Linux-target build I have handy for testing right now is 
arm-none-linux-gnueabi with unmodified FSF head, testing on a 
PandaBoard.  There I found it takes about 8 seconds to start up the same 
hello-world program that takes 4 minutes on Nios II with the new sysroot 
default, and < 1 second if I do "set sysroot" first.  Aside from 
possible kernel issues or whatever, the Nios II board is theoretically a 
factor of 20 slower than the PandaBoard (processor supposedly clocked at 
10Mhz, and only one core versus 1Ghz and 2 cores for the PandaBoard), 
and the libc.so it's transferring is larger.

-Sandra

Re: [PATCH 2/2] Make remote file transfers interruptible

[Pedro Alves]

On 08/05/2015 04:28 PM, Gary Benson wrote:
> This commit makes it possible to interrupt slow remote file transfers.
> 
> gdb/ChangeLog:
> 
> 	* gdb_bfd.c (gdb_bfd_iovec_fileio_pread): Add QUIT call.

OK.

Hopefully BFD doesn't get corrupted when an exception crosses it.
If it does, then we'll need to catch all exceptions before returning
to BFD, and return some "silently abort whatever you're doing" error.

Thanks,
Pedro Alves

[pushed] Make remote file transfers interruptible

[Gary Benson]

Pedro Alves wrote:
> On 08/05/2015 04:28 PM, Gary Benson wrote:
> > This commit makes it possible to interrupt slow remote file transfers.
> > 
> > gdb/ChangeLog:
> > 
> > 	* gdb_bfd.c (gdb_bfd_iovec_fileio_pread): Add QUIT call.
> 
> OK.
> 
> Hopefully BFD doesn't get corrupted when an exception crosses it.
> If it does, then we'll need to catch all exceptions before returning
> to BFD, and return some "silently abort whatever you're doing"
> error.

Pushed to master and 7.10.

Thanks,
Gary

-- 
http://gbenson.net/

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?