[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 0/2] Better handling of slow remote transfers

[Doug Evans]

Gary Benson writes:
  > 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?

For 7.10, one thought is to maintain the behaviour of 7.9
and give ourselves more time to address this.
IOW, can we have (or is there already) a configure
option that controls the default behaviour,
and can we default it to what 7.9 does
(not auto-fetch files) ?

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

[Jan Kratochvil]

On Tue, 11 Aug 2015 19:22:54 +0200, Doug Evans wrote:
> IOW, can we have (or is there already) a configure
> option that controls the default behaviour,
> and can we default it to what 7.9 does
> (not auto-fetch files) ?

I would like to pinpoint that for patches targeting source distribution such
an additional behavior change switch is a QA nightmare.


Jan

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

[Doug Evans]

On Tue, Aug 11, 2015 at 11:55 AM, Jan Kratochvil
<jan.kratochvil@redhat.com> wrote:
> On Tue, 11 Aug 2015 19:22:54 +0200, Doug Evans wrote:
>> IOW, can we have (or is there already) a configure
>> option that controls the default behaviour,
>> and can we default it to what 7.9 does
>> (not auto-fetch files) ?
>
> I would like to pinpoint that for patches targeting source distribution such
> an additional behavior change switch is a QA nightmare.

I'm curious what the QA costs of, for example, --with-sysroot are.
[And in particular are they nightmarish?]

At any rate, I think the default behaviour for 7.10
has to be the default behaviour of 7.9
(given that, for example, we're not going to make file transfer
more adequately interruptible for 7.10).

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

[Joel Brobecker]

> At any rate, I think the default behaviour for 7.10
> has to be the default behaviour of 7.9
> (given that, for example, we're not going to make file transfer
> more adequately interruptible for 7.10).

That makes sense to me.

-- 
Joel

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

[Gary Benson]

Joel Brobecker wrote:
> > At any rate, I think the default behaviour for 7.10
> > has to be the default behaviour of 7.9
> > (given that, for example, we're not going to make file transfer
> > more adequately interruptible for 7.10).
> 
> That makes sense to me.

If we are to reset the default sysroot to "" then please consider the
series I posted that added the auto-target-prefix functionality:

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

With these patches the "target:" prefix is only enabled if the user
does "target remote" with no sysroot or file specified, a case that
in 7.9 would result in a debug session with no symbols.
  
This has the following matrix of behaviours:

  * User wants to supply the file and not have libraries fetched
    (Sandra Loosemore's use case):

      bash$ gdb
      (gdb) file ./a.out
      (gdb) target remote :9999

    and:
      
      bash$ gdb ./a.out
      (gdb) target remote :9999

    7.9  -> no files transferred
    7.10 -> no files transferred

  * User wants to specify a file AND have GDB pull libraries from
    the remote:

      bash$ gdb
      (gdb) file ./a.out
      (gdb) set sysroot remote:
      (gdb) target remote :9999

    and:
    
      bash$ gdb ./a.out
      (gdb) set sysroot remote:
      (gdb) target remote :9999
      
    7.9  -> libraries transferred, executable read locally
    7.10 -> libraries transferred, executable read locally

  * User wants to connect to remote target and have GDB do the work:

      bash$ gdb
      (gdb) target remote :9999

    7.9  -> no files transferred, debug session with no symbols
    7.10 -> all files transferred, debug session with symbols

If the user actually wants to debug with no symbols at all they can
do this:

  bash$ gdb
  (gdb) set auto-target-prefix off
  (gdb) target remote :9999

With this series all 7.9 use cases are supported, and only the use
case where the user wants no symbols requires extra typing.  Most
users are going to want symbols, and any user capable of debugging
without symbols is capable of adding a line to their .gdbinit.

Thanks,
Gary
   
-- 
http://gbenson.net/

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

[Pedro Alves]

On 08/12/2015 10:48 AM, Gary Benson wrote:
> Joel Brobecker wrote:
>>> At any rate, I think the default behaviour for 7.10
>>> has to be the default behaviour of 7.9
>>> (given that, for example, we're not going to make file transfer
>>> more adequately interruptible for 7.10).
>>
>> That makes sense to me.
> 
> If we are to reset the default sysroot to "" then please consider the
> series I posted that added the auto-target-prefix functionality:
> 
>   https://sourceware.org/ml/gdb-patches/2015-07/msg00828.html

I'd really prefer not adding magic at the last minute to
the 7.10 release.  That would leave no breathing space to
sort out further design mistakes, which I'm sure we'll
trip on.

I think we need to unblock 7.10 as soon as possible so that 7.11
with all the neat sysroot features happens sooner too.  :-)

Thanks,
Pedro Alves

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

[Gary Benson]

Pedro Alves wrote:
> On 08/12/2015 10:48 AM, Gary Benson wrote:
> > Joel Brobecker wrote:
> > > > At any rate, I think the default behaviour for 7.10 has to be
> > > > the default behaviour of 7.9 (given that, for example, we're
> > > > not going to make file transfer more adequately interruptible
> > > > for 7.10).
> > >
> > > That makes sense to me.
> > 
> > If we are to reset the default sysroot to "" then please
> > consider the series I posted that added the auto-target-prefix
> > functionality:
> > 
> >   https://sourceware.org/ml/gdb-patches/2015-07/msg00828.html
> 
> I'd really prefer not adding magic at the last minute to the 7.10
> release.  That would leave no breathing space to sort out further
> design mistakes, which I'm sure we'll trip on.

The only real "magic" that series adds is this:

+  if (target_filesystem_is_local ())
 ...
+  else if (auto_target_prefix && *gdb_sysroot == '\0')
+    {
+      /* Set the absolute prefix to "target:" for executable files
+	 and for shared libraries whose executable filename has a
+	 "target:"-prefix.  */
+      if (!is_solib
+	  || (exec_filename != NULL
+	      && is_target_filename (exec_filename)))
+	{
+	  sysroot = xstrdup (TARGET_FILENAME_PREFIX);
+	  make_cleanup (xfree, sysroot);
+	}
+    }

*If* it proves to be a problem then we can deprecate the set/show
auto-target-prefix boolean.

It seems like you're saying this series is a big change, but it's
really not: the core of it is that little snippet of logic, which
is easy enough to reason about:

  IF target filesystem is remote
     AND auto_target_prefix is enabled
     AND no sysroot is set
     AND (we're looking for an executable
          OR we're looking for a solib loaded by a target-prefixed executable):
       Prefix the filename with "target:"

It's certainly way less invasive a change than making transfers
interruptible would be.

> I think we need to unblock 7.10 as soon as possible so that 7.11
> with all the neat sysroot features happens sooner too.  :-)

Sure, but why not unblock it this way so that 7.10 users can have
the neat sysroot features, *if and only if* they use GDB in a way
that didn't make sense in 7.9?

Thanks,
Gary

-- 
http://gbenson.net/

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

[Pedro Alves]

On 08/12/2015 11:38 AM, Gary Benson wrote:

> It seems like you're saying this series is a big change, but it's
> really not: the core of it is that little snippet of logic, which
> is easy enough to reason about:
> 
>   IF target filesystem is remote
>      AND auto_target_prefix is enabled
>      AND no sysroot is set
>      AND (we're looking for an executable
>           OR we're looking for a solib loaded by a target-prefixed executable):
>        Prefix the filename with "target:"
> 

IIUC, it still auto fetches the executable and then the solibs from the
target by default (e.g., after "attach"), so still subject to lack
of interruptibility?

> It's certainly way less invasive a change than making transfers
> interruptible would be.

I was only OK with trying to make transfers interruptible in the
branch assuming it was something non-invasive, like a missing QUIT
here and there.

>> I think we need to unblock 7.10 as soon as possible so that 7.11
>> with all the neat sysroot features happens sooner too.  :-)
> 
> Sure, but why not unblock it this way so that 7.10 users can have
> the neat sysroot features, *if and only if* they use GDB in a way
> that didn't make sense in 7.9?

Thanks,
Pedro Alves

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

[Gary Benson]

Pedro Alves wrote:
> On 08/12/2015 11:38 AM, Gary Benson wrote:
> > It seems like you're saying this series is a big change, but it's
> > really not: the core of it is that little snippet of logic, which
> > is easy enough to reason about:
> > 
> >   IF target filesystem is remote
> >      AND auto_target_prefix is enabled
> >      AND no sysroot is set
> >      AND (we're looking for an executable
> >           OR we're looking for a solib loaded by a target-prefixed executable):
> >        Prefix the filename with "target:"
> > 
> 
> IIUC, it still auto fetches the executable and then the solibs from
> the target by default (e.g., after "attach"), so still subject to
> lack of interruptibility?

Yes and no.  It will fetch the executable from the remote iff one has
not been otherwise specified (i.e. by "file", or on the command line).
It will *only* fetch libraries from the remote if the parent executable
has a target prefix.  So:

  (gdb) file a.out
  (gdb) target remote :9999

   - exec_filename is "a.out"
   - exec_filename has no "target:" prefix
   - "target:" prefix is NOT applied to shared libraries
   - solib paths end up as "/path/to/libsolib.so.1"
   - solibs are NOT fetched over RSP

But:

  (gdb) target remote :9999

    - exec_filename is set to, e.g., "target:/path/to/a.out"
    - exec_filename HAS a "target:" prefix
    - "target:" prefix IS applied to shared libraries
    - solib paths end up as "target:/path/to/libsolib.so.1"
    - solibs ARE fetched over RSP

Basically it fetches the libraries over RSP if and only if the
executable was fetched over RSP.  So it works as Sandra expects
when she uses GDB her way, but it still has the automatic
executable filename discovery and automatic fetch-from-remote
for users who just do "target remote ..." on it's own (which is
something that doesn't make much sense in 7.9).
       
> > It's certainly way less invasive a change than making transfers
> > interruptible would be.
> 
> I was only OK with trying to make transfers interruptible in the
> branch assuming it was something non-invasive, like a missing QUIT
> here and there.

No, gdbserver sends the data in PBUFSIZ chunks, but GDB reads the
data a character at a time.

Cheers,
Gary

-- 
http://gbenson.net/

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

[Pedro Alves]

On 08/12/2015 01:32 PM, Gary Benson wrote:
> Pedro Alves wrote:
>> On 08/12/2015 11:38 AM, Gary Benson wrote:
>>> It seems like you're saying this series is a big change, but it's
>>> really not: the core of it is that little snippet of logic, which
>>> is easy enough to reason about:
>>>
>>>   IF target filesystem is remote
>>>      AND auto_target_prefix is enabled
>>>      AND no sysroot is set
>>>      AND (we're looking for an executable
>>>           OR we're looking for a solib loaded by a target-prefixed executable):
>>>        Prefix the filename with "target:"
>>>
>>
>> IIUC, it still auto fetches the executable and then the solibs from
>> the target by default (e.g., after "attach"), so still subject to
>> lack of interruptibility?
> 
> Yes and no.  It will fetch the executable from the remote iff one has
> not been otherwise specified (i.e. by "file", or on the command line).
> It will *only* fetch libraries from the remote if the parent executable
> has a target prefix.  So:
> 
>   (gdb) file a.out
>   (gdb) target remote :9999
> 
>    - exec_filename is "a.out"
>    - exec_filename has no "target:" prefix
>    - "target:" prefix is NOT applied to shared libraries
>    - solib paths end up as "/path/to/libsolib.so.1"
>    - solibs are NOT fetched over RSP
> 

But to me it looks like GDB _should_ retrieve the libraries
out of the target in this case.  You'll usually have a local copy
of the executable, because you just compiled it, but not of
the shared libraries.  It seems to me we're only considering
this option because we didn't make transfers interruptible?

>> I was only OK with trying to make transfers interruptible in the
>> branch assuming it was something non-invasive, like a missing QUIT
>> here and there.
> 
> No, gdbserver sends the data in PBUFSIZ chunks, but GDB reads the
> data a character at a time.

Can you expand on this?  What code is it that reads the data a
character at a time?  What data is gdb getting at when it does that?

Thanks,
Pedro Alves

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

[Gary Benson]

Pedro Alves wrote:
> On 08/12/2015 01:32 PM, Gary Benson wrote:
> > Pedro Alves wrote:
> > > IIUC, it still auto fetches the executable and then the solibs
> > > from the target by default (e.g., after "attach"), so still
> > > subject to lack of interruptibility?
> > 
> > Yes and no.  It will fetch the executable from the remote iff one
> > has not been otherwise specified (i.e. by "file", or on the
> > command line).  It will *only* fetch libraries from the remote if
> > the parent executable has a target prefix.  So:
> > 
> >   (gdb) file a.out
> >   (gdb) target remote :9999
> > 
> >    - exec_filename is "a.out"
> >    - exec_filename has no "target:" prefix
> >    - "target:" prefix is NOT applied to shared libraries
> >    - solib paths end up as "/path/to/libsolib.so.1"
> >    - solibs are NOT fetched over RSP
> > 
> 
> But to me it looks like GDB _should_ retrieve the libraries out of
> the target in this case.  You'll usually have a local copy of the
> executable, because you just compiled it, but not of the shared
> libraries.  It seems to me we're only considering this option
> because we didn't make transfers interruptible?

Right, but users who have been doing that will be using "set sysroot
remote:":

  (gdb) file a.out
  (gdb) set sysroot remote:
  (gdb) target remote :9999

  - gdb_sysroot is "target:"
  - solib paths end up as "target:/path/to/libsolib.so.1"
  - solibs ARE fetched over RSP

This series basically means that:

  a) users get to type whatever they were typing before and have the
     same thing happen, with the exception of the 0.001% of users who
     have been typing "target remote :9999" with no "file" or "set sysroot"
     commands and debugging with no symbols whatsoever.

  b) those users can add "set auto-target-prefix off" in their .gdbinit

  c) users who want GDB to connect to a remote target and Just Work
     get to type "target remote :9999" without messing about with
     "file" and "set sysroot" commands.

> > > I was only OK with trying to make transfers interruptible in the
> > > branch assuming it was something non-invasive, like a missing
> > > QUIT here and there.
> > 
> > No, gdbserver sends the data in PBUFSIZ chunks, but GDB reads the
> > data a character at a time.
> 
> Can you expand on this?  What code is it that reads the data a
> character at a time?  What data is gdb getting at when it does that?

I was looking in getpkt_or_notif_sane_1, but I think maybe I misread
it.  I'll get back to you on this...

Cheers,
Gary

-- 
http://gbenson.net/

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

[Pedro Alves]

On 08/12/2015 02:02 PM, Gary Benson wrote:
>>>> > > > I was only OK with trying to make transfers interruptible in the
>>>> > > > branch assuming it was something non-invasive, like a missing
>>>> > > > QUIT here and there.
>>> > > 
>>> > > No, gdbserver sends the data in PBUFSIZ chunks, but GDB reads the
>>> > > data a character at a time.
>> > 
>> > Can you expand on this?  What code is it that reads the data a
>> > character at a time?  What data is gdb getting at when it does that?
> I was looking in getpkt_or_notif_sane_1, but I think maybe I misread
> it.  I'll get back to you on this...

That's the very low level of RSP packets, which as you noted will
have a reasonable cap.  It sounds to me there's a loop somewhere in
a higher layer that is missing a QUIT.  E.g., we have quits
in dwarf2read.c which allow interrupting reading big binaries,
even if locally.  So what is the higher level operation that
gdb is doing when you try to interrupt, but can't?

Thanks,
Pedro Alves

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

[Gary Benson]

Pedro Alves wrote:
> On 08/12/2015 02:02 PM, Gary Benson wrote:
> >>>> > > > I was only OK with trying to make transfers interruptible in the
> >>>> > > > branch assuming it was something non-invasive, like a missing
> >>>> > > > QUIT here and there.
> >>> > > 
> >>> > > No, gdbserver sends the data in PBUFSIZ chunks, but GDB reads the
> >>> > > data a character at a time.
> >> > 
> >> > Can you expand on this?  What code is it that reads the data a
> >> > character at a time?  What data is gdb getting at when it does that?
> > I was looking in getpkt_or_notif_sane_1, but I think maybe I misread
> > it.  I'll get back to you on this...
> 
> That's the very low level of RSP packets, which as you noted will
> have a reasonable cap.  It sounds to me there's a loop somewhere in
> a higher layer that is missing a QUIT.  E.g., we have quits
> in dwarf2read.c which allow interrupting reading big binaries,
> even if locally.  So what is the higher level operation that
> gdb is doing when you try to interrupt, but can't?

remote_hostio_pread.  I'm trying to make remote_hostio_pread
interruptible.  BFD is doing large remote_hostio_pread which
are resulting in large vFile:pread: packet responses.

Cheers,
Gary

-- 
http://gbenson.net/

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

[Gary Benson]

Gary Benson wrote:
> Pedro Alves wrote:
> > On 08/12/2015 02:02 PM, Gary Benson wrote:
> > > I was looking in getpkt_or_notif_sane_1, but I think maybe I
> > > misread it.  I'll get back to you on this...
> > 
> > That's the very low level of RSP packets, which as you noted will
> > have a reasonable cap.  It sounds to me there's a loop somewhere
> > in a higher layer that is missing a QUIT.  E.g., we have quits in
> > dwarf2read.c which allow interrupting reading big binaries, even
> > if locally.  So what is the higher level operation that gdb is
> > doing when you try to interrupt, but can't?
> 
> remote_hostio_pread.  I'm trying to make remote_hostio_pread
> interruptible.  BFD is doing large remote_hostio_pread which
> are resulting in large vFile:pread: packet responses.

To elaborate:
  remote_hostio_pread calls remote_hostio_send_command once
  remote_hostio_send_command calls getpkt_sane once
  getpkt_sane calls getpkt_or_notif_sane_1 once

I've already posted a patch that did QUIT once per vFile:pread:
but that wasn't good enough.  For anything finer-grained the
QUIT needs to be in getpkt_or_notif_sane_1 but this doesn't seem
workable given how the protocol is.

The alternative is for remote_hostio_pread to make multiple
vFile:pread: packets, but that's going to introduce extra
traffic and latency.

Cheers,
Gary

-- 
http://gbenson.net/

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

[Pedro Alves]

On 08/12/2015 02:38 PM, Gary Benson wrote:
> Pedro Alves wrote:
>> On 08/12/2015 02:02 PM, Gary Benson wrote:
>>>>>>>>> I was only OK with trying to make transfers interruptible in the
>>>>>>>>> branch assuming it was something non-invasive, like a missing
>>>>>>>>> QUIT here and there.
>>>>>>>
>>>>>>> No, gdbserver sends the data in PBUFSIZ chunks, but GDB reads the
>>>>>>> data a character at a time.
>>>>>
>>>>> Can you expand on this?  What code is it that reads the data a
>>>>> character at a time?  What data is gdb getting at when it does that?
>>> I was looking in getpkt_or_notif_sane_1, but I think maybe I misread
>>> it.  I'll get back to you on this...
>>
>> That's the very low level of RSP packets, which as you noted will
>> have a reasonable cap.  It sounds to me there's a loop somewhere in
>> a higher layer that is missing a QUIT.  E.g., we have quits
>> in dwarf2read.c which allow interrupting reading big binaries,
>> even if locally.  So what is the higher level operation that
>> gdb is doing when you try to interrupt, but can't?
> 
> remote_hostio_pread.  I'm trying to make remote_hostio_pread
> interruptible.  BFD is doing large remote_hostio_pread which
> are resulting in large vFile:pread: packet responses.

And what is BFD doing that ends up in those remote_hostio_pread
calls?  Reading the elf headers, parsing the symbol table, etc?
Or maybe something else?

GDB will usually cap the transfers to before they get to the
lower layers.  E.g., look for 4096 in memory_xfer_partial,
target_read_alloc_1 and target_fileio_read_alloc_1.

As this request is coming from the BFD side, we should probably
make remote_hostio_pread also cap the size of the vFile:pread
request.  A reasonable number like a few KBs should not
introduce any noticeable slow down.

Thanks,
Pedro Alves

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

[Pedro Alves]

On 08/12/2015 02:58 PM, Pedro Alves wrote:
> On 08/12/2015 02:38 PM, Gary Benson wrote:
>> Pedro Alves wrote:
>>> On 08/12/2015 02:02 PM, Gary Benson wrote:
>>>>>>>>>> I was only OK with trying to make transfers interruptible in the
>>>>>>>>>> branch assuming it was something non-invasive, like a missing
>>>>>>>>>> QUIT here and there.
>>>>>>>>
>>>>>>>> No, gdbserver sends the data in PBUFSIZ chunks, but GDB reads the
>>>>>>>> data a character at a time.
>>>>>>
>>>>>> Can you expand on this?  What code is it that reads the data a
>>>>>> character at a time?  What data is gdb getting at when it does that?
>>>> I was looking in getpkt_or_notif_sane_1, but I think maybe I misread
>>>> it.  I'll get back to you on this...
>>>
>>> That's the very low level of RSP packets, which as you noted will
>>> have a reasonable cap.  It sounds to me there's a loop somewhere in
>>> a higher layer that is missing a QUIT.  E.g., we have quits
>>> in dwarf2read.c which allow interrupting reading big binaries,
>>> even if locally.  So what is the higher level operation that
>>> gdb is doing when you try to interrupt, but can't?
>>
>> remote_hostio_pread.  I'm trying to make remote_hostio_pread
>> interruptible.  BFD is doing large remote_hostio_pread which
>> are resulting in large vFile:pread: packet responses.
> 
> And what is BFD doing that ends up in those remote_hostio_pread
> calls?  Reading the elf headers, parsing the symbol table, etc?
> Or maybe something else?
> 
> GDB will usually cap the transfers to before they get to the
> lower layers.  E.g., look for 4096 in memory_xfer_partial,
> target_read_alloc_1 and target_fileio_read_alloc_1.
> 
> As this request is coming from the BFD side, we should probably
> make remote_hostio_pread also cap the size of the vFile:pread
> request.  A reasonable number like a few KBs should not
> introduce any noticeable slow down.

But wait, I'm now confused -- isn't this a red herring?  Since
gdbserver is already limiting transfers to PBUFSIZE, this
change should have no practical effect, right?

How can BFD's large remote_hostio_pread result in large
vFile:pread: packet responses then?

Thanks,
Pedro Alves

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

[Gary Benson]

Pedro Alves wrote:
> On 08/12/2015 02:58 PM, Pedro Alves wrote:
> > GDB will usually cap the transfers to before they get to the
> > lower layers.  E.g., look for 4096 in memory_xfer_partial,
> > target_read_alloc_1 and target_fileio_read_alloc_1.
> > 
> > As this request is coming from the BFD side, we should probably
> > make remote_hostio_pread also cap the size of the vFile:pread
> > request.  A reasonable number like a few KBs should not
> > introduce any noticeable slow down.
> 
> But wait, I'm now confused -- isn't this a red herring?  Since
> gdbserver is already limiting transfers to PBUFSIZE, this change
> should have no practical effect, right?
> 
> How can BFD's large remote_hostio_pread result in large vFile:pread:
> packet responses then?

I think gdbserver is returning multiple packets but something in GDB
(getpkt_or_notif_sane_1?) is concatenating them together somehow.

Cheers,
Gary

-- 
http://gbenson.net/

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

[Pedro Alves]

On 08/12/2015 04:08 PM, Gary Benson wrote:
> Pedro Alves wrote:
>> On 08/12/2015 02:58 PM, Pedro Alves wrote:
>>> GDB will usually cap the transfers to before they get to the
>>> lower layers.  E.g., look for 4096 in memory_xfer_partial,
>>> target_read_alloc_1 and target_fileio_read_alloc_1.
>>>
>>> As this request is coming from the BFD side, we should probably
>>> make remote_hostio_pread also cap the size of the vFile:pread
>>> request.  A reasonable number like a few KBs should not
>>> introduce any noticeable slow down.
>>
>> But wait, I'm now confused -- isn't this a red herring?  Since
>> gdbserver is already limiting transfers to PBUFSIZE, this change
>> should have no practical effect, right?
>>
>> How can BFD's large remote_hostio_pread result in large vFile:pread:
>> packet responses then?
> 
> I think gdbserver is returning multiple packets but something in GDB
> (getpkt_or_notif_sane_1?) is concatenating them together somehow.

No, getpkt_or_notif_sane_1 will return as soon as it has a single
packet, which should then be bubbling up the layers and reaching
gdb_bfd_iovec_fileio_pread.  Something else is going on.
Either the QUIT is being lost/eaten, or ... hmm ... maybe the
SIGINT handler is set to remote.c:async_handle_remote_sigint
when the ctrl-c is typed, which means the ctrl-c doesn't
actually set_quit_flag()?

Thanks,
Pedro Alves

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

[Gary Benson]

Pedro Alves wrote:
> On 08/12/2015 04:08 PM, Gary Benson wrote:
> > Pedro Alves wrote:
> > > On 08/12/2015 02:58 PM, Pedro Alves wrote:
> > > > GDB will usually cap the transfers to before they get to the
> > > > lower layers.  E.g., look for 4096 in memory_xfer_partial,
> > > > target_read_alloc_1 and target_fileio_read_alloc_1.
> > > >
> > > > As this request is coming from the BFD side, we should
> > > > probably make remote_hostio_pread also cap the size of the
> > > > vFile:pread request.  A reasonable number like a few KBs
> > > > should not introduce any noticeable slow down.
> > >
> > > But wait, I'm now confused -- isn't this a red herring?  Since
> > > gdbserver is already limiting transfers to PBUFSIZE, this change
> > > should have no practical effect, right?
> > >
> > > How can BFD's large remote_hostio_pread result in large
> > > vFile:pread: packet responses then?
> > 
> > I think gdbserver is returning multiple packets but something in
> > GDB (getpkt_or_notif_sane_1?) is concatenating them together
> > somehow.
> 
> No, getpkt_or_notif_sane_1 will return as soon as it has a single
> packet, which should then be bubbling up the layers and reaching
> gdb_bfd_iovec_fileio_pread.  Something else is going on.  Either the
> QUIT is being lost/eaten, or ... hmm ... maybe the SIGINT handler is
> set to remote.c:async_handle_remote_sigint when the ctrl-c is typed,
> which means the ctrl-c doesn't actually set_quit_flag()?

I've no idea.  Really I haven't.

I have to finish for the day now.  I'll be back in 16 hours.
Maybe somebody who'll benefit from interruptible remote transfers
could look into this while I'm away.  Sandra?  Pedro?  Doug?

Thanks,
Gary

-- 
http://gbenson.net/

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

[Gary Benson]

Pedro Alves wrote:
> On 08/12/2015 01:32 PM, Gary Benson wrote:
> > Pedro Alves wrote:
> > > I was only OK with trying to make transfers interruptible in the
> > > branch assuming it was something non-invasive, like a missing
> > > QUIT here and there.
> > 
> > No, gdbserver sends the data in PBUFSIZ chunks, but GDB reads the
> > data a character at a time.
> 
> Can you expand on this?  What code is it that reads the data a
> character at a time?  What data is gdb getting at when it does that?

getpkt_or_notif_sane_1 in gdb/remote.c.  If I'm reading it right GDB
is reading every single byte coming over RSP individually.  It doesn't
work to put a QUIT in there as there's still stuff coming over the
wire.  If gdbserver sends data, GDB has to read it.

You're way more familiar with RSP than I am.  Do you know any way to
make vFile:pread: interruptible?

Cheers,
Gary

-- 
http://gbenson.net/

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

[Joel Brobecker]

> If we are to reset the default sysroot to "" then please consider the
> series I posted that added the auto-target-prefix functionality:
> 
>   https://sourceware.org/ml/gdb-patches/2015-07/msg00828.html
> 
> With these patches the "target:" prefix is only enabled if the user
> does "target remote" with no sysroot or file specified, a case that
> in 7.9 would result in a debug session with no symbols.

Trying to summarize where we are, right now:

   1. Part of the discussion was about trying to figure out why
      C-c would not interrupt file transfers midway through;

   2. Before that, there was some discussion about whether or not
      we should transfer all files in the case where the no
      executable was provided and sysroot is empty.

      I'm unclear whether this part was generally accepted or not.
      I have a feeling that having (1) resolved would go a long way
      towards accepting (2).

That being said, I understand Pedro's concerns regarding adding
this extra logic that late in the game, even if it is fairly simple.
But if he or others agree with it, then I would be fine with it too.
It does seem to only affect a corner case.

-- 
Joel

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

[Sandra Loosemore]

On 08/14/2015 12:26 PM, Joel Brobecker wrote:
>> If we are to reset the default sysroot to "" then please consider the
>> series I posted that added the auto-target-prefix functionality:
>>
>>    https://sourceware.org/ml/gdb-patches/2015-07/msg00828.html
>>
>> With these patches the "target:" prefix is only enabled if the user
>> does "target remote" with no sysroot or file specified, a case that
>> in 7.9 would result in a debug session with no symbols.
>
> Trying to summarize where we are, right now:
>
>     1. Part of the discussion was about trying to figure out why
>        C-c would not interrupt file transfers midway through;
>
>     2. Before that, there was some discussion about whether or not
>        we should transfer all files in the case where the no
>        executable was provided and sysroot is empty.
>
>        I'm unclear whether this part was generally accepted or not.
>        I have a feeling that having (1) resolved would go a long way
>        towards accepting (2).
>
> That being said, I understand Pedro's concerns regarding adding
> this extra logic that late in the game, even if it is fairly simple.
> But if he or others agree with it, then I would be fine with it too.
> It does seem to only affect a corner case.

It actually looks to me like we are not any closer to having this 
resolved than we were at the beginning of the week.  Gary's last patch 
for ^C handling didn't work for me and he said he is out of ideas.  I am 
willing to try out patches, but I'm really swamped with other tasks 
right now as well as being totally unfamiliar with the internals of this 
code, so it's not reasonable to think I could fix this myself in time 
for the 7.10 release.  And AFAIK nobody else is working on this either.  :-(

-Sandra

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

[Joel Brobecker]

> It actually looks to me like we are not any closer to having this resolved
> than we were at the beginning of the week.  Gary's last patch for ^C
> handling didn't work for me and he said he is out of ideas.  I am willing to
> try out patches, but I'm really swamped with other tasks right now as well
> as being totally unfamiliar with the internals of this code, so it's not
> reasonable to think I could fix this myself in time for the 7.10 release.
> And AFAIK nobody else is working on this either.  :-(

I think the situation is a little better than you describe.
As far as I understand, there is one patch that changes the default
for sysroot back to ""; it is expected to restore the current behavior
in your case, but at the same time introduces a change in behavior
in one specific situation where someone is debugging remotely without
providing the executable to GDB (either through the command-line or
using the "file" command). The change of behavior and how to control it
is what's being debated at the moment, and is why you're still seeing
the issue.

Parallel to that, my understanding of the situation is that there is
a secondary issue of not being able interrupt a file transfer. That
is what you've been testing so far, I believe. Lack of success so far
is a little fustrating for everyone, I'm sure. But I am still wondering
whether you should even be in the situation where you need to interrupt
in the first place.

That's why, to me, the first discussion has a little more weight.
We'll want to figure out the mystery in the secondary issue, but
if we can find the right approach in the first discussion for 7.10,
that would buy us some extra time in terms of being able to interrupt
file transfers.

-- 
Joel

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

[Gary Benson]

Joel Brobecker wrote:
> Sandra Loosemore wrote:
> > It actually looks to me like we are not any closer to having this
> > resolved than we were at the beginning of the week.  Gary's last
> > patch for ^C handling didn't work for me and he said he is out of
> > ideas.  I am willing to try out patches, but I'm really swamped
> > with other tasks right now as well as being totally unfamiliar
> > with the internals of this code, so it's not reasonable to think I
> > could fix this myself in time for the 7.10 release.  And AFAIK
> > nobody else is working on this either.  :-(
> 
> I think the situation is a little better than you describe.  As far
> as I understand, there is one patch that changes the default for
> sysroot back to ""; it is expected to restore the current behavior
> in your case, but at the same time introduces a change in behavior
> in one specific situation where someone is debugging remotely
> without providing the executable to GDB (either through the
> command-line or using the "file" command). The change of behavior
> and how to control it is what's being debated at the moment, and is
> why you're still seeing the issue.

Just to clarify, the default sysroot of "target:" has two purposes:

 1. It allows GDB to locate and access binaries on remote targets
    without having to be told where they are.

 2. It allows GDB to locate and access binaries on native targets
    when the inferior is running in a container.

Resetting the default sysroot to "" disables both cases.  The second
is arguably more important, because without it users can attach to a
local process (with, e.g., gdb -p PID) but GDB can end up loading the
wrong symbols if binaries with the same paths exist both within the
container and on the host machine.

> Parallel to that, my understanding of the situation is that there is
> a secondary issue of not being able interrupt a file transfer. That
> is what you've been testing so far, I believe. Lack of success so
> far is a little fustrating for everyone, I'm sure. But I am still
> wondering whether you should even be in the situation where you need
> to interrupt in the first place.
> 
> That's why, to me, the first discussion has a little more weight.
> We'll want to figure out the mystery in the secondary issue, but
> if we can find the right approach in the first discussion for 7.10,
> that would buy us some extra time in terms of being able to interrupt
> file transfers.

It seems to me that being able to interrupt file transfers is polish.
With the warning patch alone, users will see the warning and the hint
about how to restore the previous default, which they can apply and
continue as before.  If they have to wait out a transfer then it will
presumably only be once.  I know some people use GDB on systems with
5,000+ shared libraries, and others use GDB on slow serial links, but
I don't think anybody combines these cases.

So, would the warning+hint patch alone be enough?

FWIW the reason I am out of ideas for making transfers interruptible
is that both QUIT patches I supplied allow me to interrupt transfers.
Something is obviously different on Sandra's setup to mine, but I
don't know what, and without a reproducer I'm just stabbing in the
dark.

Thanks,
Gary

-- 
http://gbenson.net/

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

[Sandra Loosemore]

On 08/17/2015 02:53 AM, Gary Benson wrote:

> It seems to me that being able to interrupt file transfers is polish.
> With the warning patch alone, users will see the warning and the hint
> about how to restore the previous default, which they can apply and
> continue as before.  If they have to wait out a transfer then it will
> presumably only be once.  I know some people use GDB on systems with
> 5,000+ shared libraries, and others use GDB on slow serial links, but
> I don't think anybody combines these cases.

FYI, I am not debugging over a "slow serial link".  I've been testing 
this on an Altera 3c120 board (Nios II) with 10/100 Ethernet; it 
NFS-mounts the sysroot under test and before now that has worked fine 
with no obvious responsiveness issues.

> So, would the warning+hint patch alone be enough?

Is it really user-friendly to make the user either wait 4 minutes or 
kill GDB through a separate terminal before they can act on the hint?

-Sandra

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

[Gary Benson]

Sandra Loosemore wrote:
> On 08/17/2015 02:53 AM, Gary Benson wrote:
> > It seems to me that being able to interrupt file transfers is
> > polish.  With the warning patch alone, users will see the warning
> > and the hint about how to restore the previous default, which they
> > can apply and continue as before.  If they have to wait out a
> > transfer then it will presumably only be once.  I know some people
> > use GDB on systems with 5,000+ shared libraries, and others use
> > GDB on slow serial links, but I don't think anybody combines these
> > cases.
> 
> FYI, I am not debugging over a "slow serial link".  I've been
> testing this on an Altera 3c120 board (Nios II) with 10/100
> Ethernet; it NFS-mounts the sysroot under test and before now that
> has worked fine with no obvious responsiveness issues.
> 
> > So, would the warning+hint patch alone be enough?
> 
> Is it really user-friendly to make the user either wait 4 minutes
> of kill GDB through a separate terminal before they can act on the
> hint?

This user-friendliness argument is almost a straw man.  Is it
user-friendly to make the user wait 4 minutes before they can update
their .gdbinit and continue as before?  Probably not.  Is it
user-friendly to make the user set up NFS on the target or copy all
the files across (and keep them synchronized)?  Also probably not.
Is it user friendly to expect users who want GDB to locate binaries
for them to add "set sysroot target:" to their .gdbinit?  Also
probably not.  Is it user friendly to expect users who want to use
GDB across containers to add "set sysroot target:" to their .gdbinit?
Also probably not.  So lets leave user-friendliness to one side and
talk about what's actually happening.

For some reason, the Altera 3c120 board you are using is very much
slower to transfer files over RSP than it is over NFS.

For some reason, neither of the two QUIT patches I mailed work on your
setup with this Altera 3c120 board you are using even though they work
just fine on this x86_64 machine I am using.

Your PandaBoard takes 8 seconds.  That doesn't seem so bad to me.
If this Altera board is the only one with the massive slowdown then
I don't think we should delay 7.10 any further on this issue--and I
certainly don't think we should lose the functionality that the
default sysroot of "target:" brings.

Thanks,
Gary

-- 
http://gbenson.net/

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

[Sandra Loosemore]

On 08/18/2015 03:58 AM, Gary Benson wrote:
>
> For some reason, the Altera 3c120 board you are using is very much
> slower to transfer files over RSP than it is over NFS.
>
> For some reason, neither of the two QUIT patches I mailed work on your
> setup with this Altera 3c120 board you are using even though they work
> just fine on this x86_64 machine I am using.

I would be happy to try to help debug what is going wrong here, but I'm 
not a GDB expert by any means and I'm totally unfamiliar with this part 
of the code and don't know where to start.  If somebody has a theory on 
this, is there someplace I could put in printfs to trace what is 
happening?  From the previous discussion, it seemed like the flow 
through the different layers of abstraction to what the RSP was actually 
doing was very obscure, in terms of where the chunking is happening, etc.

> Your PandaBoard takes 8 seconds.  That doesn't seem so bad to me.
> If this Altera board is the only one with the massive slowdown then
> I don't think we should delay 7.10 any further on this issue--and I
> certainly don't think we should lose the functionality that the
> default sysroot of "target:" brings.

FWIW, I have just tried on a random sampling of our MIPS target boards 
as well, with the same test program and same scenario of continuing to a 
breakpoint on main.  The timings were 1:40 (SEAD-3 LX110 board, M14Kc 
microMips), 1:38 (MALTA 74Kc MIPS16), and 1:20 (MALTA 5Kef MIPS64).  I 
could try some PowerPC boards as well if we need more datapoints.

In any case, I think we should be cautious about declaring that 
functionality that seems to work on x86_64 native should work 
everywhere, and if it doesn't it's the fault of the target or user 
instead of buggy code or poor design.  IMO this is especially true for 
remote debugging, where I think debugging an embedded target is a more 
common scenario than a native one.

-Sandra

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

[Pedro Alves]

[-- Attachment #1: Type: text/plain, Size: 8879 bytes --]

On 08/18/2015 05:50 PM, Sandra Loosemore wrote:

> I would be happy to try to help debug what is going wrong here, but I'm
> not a GDB expert by any means and I'm totally unfamiliar with this part 
> of the code and don't know where to start.  If somebody has a theory on 
> this, is there someplace I could put in printfs to trace what is 
> happening?  From the previous discussion, it seemed like the flow 
> through the different layers of abstraction to what the RSP was actually 
> doing was very obscure, in terms of where the chunking is happening, etc.

That was just a misunderstanding.  It's not really obscure.  GDB has a file io
abstraction that wires the open/close/pread/pwrite syscalls to RSP packets.
Callers just need to use the target_fileio_open/pread etc. functions instead
of the usual syscalls.  Whenever BFD or GDB accesses a file that is prefixed
target: or remote:, the open/read/write requests go through those calls.
For remote targets, those end up in remote.c:remote_hostio_pread, etc.  The only
chunking going on is the server trimming the pread results to the maximum packet
size it can return to gdb.  Both gdb and gdbserver have that maximum buffer
size hardcoded at 16384 for historical reasons.

>> Your PandaBoard takes 8 seconds.  That doesn't seem so bad to me.
>> If this Altera board is the only one with the massive slowdown then
>> I don't think we should delay 7.10 any further on this issue--and I
>> certainly don't think we should lose the functionality that the
>> default sysroot of "target:" brings.
> 
> FWIW, I have just tried on a random sampling of our MIPS target boards 
> as well, with the same test program and same scenario of continuing to a 
> breakpoint on main.  The timings were 1:40 (SEAD-3 LX110 board, M14Kc 
> microMips), 1:38 (MALTA 74Kc MIPS16), and 1:20 (MALTA 5Kef MIPS64).  I 
> could try some PowerPC boards as well if we need more datapoints.

Thanks for doing this.  As yet another data point, it'd be interesting to
know whether Gary's rate-limit patch speeds up any other of these boards
as well.

Did you try getting the nfs mount out of the picture?  That is, copy the
debug-build files to the target, and have gdb pull them automatically.

It's expected that reading a file out of remote filesystem
is going to be slower than not reading it.  Obviously some slowdown
is expected.  There's no free lunch.

The fact that Gary's chunk size limiting patch made things much
faster on the nios2 board is still mysterious to me.  I'd expect the
slowness to be latency bound, given the request/response nature of the RSP,
but then I'd expect that more chunking would slow things down, not speed
it up.

I'm trying to build a more complete picture of the potential for
trouble.  The more data, the better...

So tonight I played with this.  To amplify latency issues against a remote target,
I've built gdbserver on gcc76 on the gcc compile farm.  Anyone can request
access to the gcc compile farm machines and to reproduce this.  That's an x86-64
in France almost 2000km away from me.  I've got a ping time of ~85ms to
that machine, and I'm behind a ~16Mbit ADSL.

As an illustration, here's what it takes to download the gdbserver binary using ssh/scp.
Not the fastest file transfer method, but the most convenient:

$ time scp gcc76.fsffrance.org:/home/palves/gdb/build/gdb/gdbserver/gdbserver /tmp/gdbserver
gdbserver             100% 1451KB   1.4MB/s   00:01

real    0m1.296s
user    0m0.015s
sys     0m0.013s

Now let's compare with GDB's file retrieval:

[palves@gcc76] $ ./gdbserver --multi :9999

[palves@home] $ ssh -L 9999:localhost:9999 gcc76.fsffrance.org
[palves@home] $ cat /tmp/remote-get
define getfile
       shell date
       remote get /home/palves/gdb/build/gdb/gdbserver/gdbserver /tmp/gdbserver
       shell date
       end
[palves@home] $ ./gdb -data-directory=data-directory -ex "tar extended-rem :9999" -x /tmp/remote-get

(gdb) getfile
Detaching after fork from child process 31567.
Wed Aug 19 01:04:13 WEST 2015
Detaching after fork from child process 31568.
Wed Aug 19 01:04:23 WEST 2015
(gdb)

~10s GDB/vFile vs ~1.3s scp/ssh.  Not that impressive.

So then I tried bumping the RSP max packet buffer size.  That required
a patch to gdb and another to gdbserver.  I bumped the buffer size 16 times.
The result was:

(gdb) getfile
Wed Aug 19 01:13:01 WEST 2015
Wed Aug 19 01:13:04 WEST 2015
(gdb) getfile
Wed Aug 19 01:13:12 WEST 2015
Wed Aug 19 01:13:14 WEST 2015
(gdb) getfile
Wed Aug 19 01:13:19 WEST 2015
Wed Aug 19 01:13:21 WEST 2015

So 2s/3s.  Not bad.

Bumping the packet buffer size to 4MB, to make sure
a single vFile:pread covers the ~1.5MB file, I consistently
get ~1s:

(gdb) getfile
Wed Aug 19 01:17:52 WEST 2015
Wed Aug 19 01:17:53 WEST 2015
(gdb) getfile
Wed Aug 19 01:17:55 WEST 2015
Wed Aug 19 01:17:56 WEST 2015
(gdb) getfile
Wed Aug 19 01:17:57 WEST 2015
Wed Aug 19 01:17:58 WEST 2015
(gdb)

which is then in the ball park of scp.  Seems clear to me that for
whole-file transfers, the bottleneck (on this use case) is the number
of chunks / round trips.  For file transfer, obviously a stream-based
transfer, like scp's, that avoids roundtrip hiccups, wins.

For the use case of connecting to gdbserver with "target remote",
and having gdb fetch the binary out of gdbserver with vFile, I tried
something else.

This time, I'll simply debug gdbserver itself remotely, letting gdb
fetch the gdbserver binary, and run to main.  Once main is reached,
quit:

[palves@gcc76] $ ./gdbserver :9999 ./gdbserver
[palves@home] $ ssh -L 9999:localhost:9999 gcc76.fsffrance.org
[palves@home] time ./gdb -data-directory=data-directory -ex "tar rem :9999" -ex "b main" -ex "c" -ex "set confirm off" -ex "quit"

Pristine gdb 7.10.50.20150816-cvs gets me:
...
Remote debugging using :9999
Reading symbols from target:/home/palves/gdb/build/gdb/gdbserver/gdbserver...done.
Reading symbols from target:/lib64/ld-linux-x86-64.so.2...(no debugging symbols found)...done.
0x00007ffff7ddd190 in ?? () from target:/lib64/ld-linux-x86-64.so.2
Breakpoint 1 at 0x41200c: file ../../../src/gdb/gdbserver/server.c, line 3635.
Continuing.

Breakpoint 1, main (argc=1, argv=0x7fffffffe3d8) at ../../../src/gdb/gdbserver/server.c:3635
3635    ../../../src/gdb/gdbserver/server.c: No such file or directory.
/home/palves/gdb/build/gdb/gdbserver/gdbserver: No such file or directory.

real    0m55.315s
user    0m0.400s
sys     0m0.087s

Bumping the remote max packet buffer size to (16384*16) helps a bit,
though not impressive:

real    0m51.600s
user    0m0.404s
sys     0m0.085s

Bumping to 0x100000 helps a bit further:

real    0m48.335s
user    0m0.421s
sys     0m0.091s

Looking at "set debug remote 1" output, I noticed that gdb often issues
a sequence of small vFile:pread requests for the same file.  Ctrl-c'ing
gdb while it was doing that and getting a backtrace shows that those requests
often came from bfd.  So I thought that maybe adding a readahead buffer/cache
to gdb's vFile:pread requests could help.  And indeed, that dropped the
time for the same use case further down to:

real    0m29.055s
user    0m2.625s
sys     0m0.090s

I added a few counters to show cache hit/miss, and got:

 readahead cache miss 44
 readahead cache hit 377

Not sure exactly what makes the "user" time go up that much (maybe
reading too much ahead causes too much RSP escaping, etc.?)
If I repeat the same leaving gdbserver's maximum buffer size
set at the original 16384, I still get <30s:

real    0m27.169s
user    0m0.464s
sys     0m0.061s

though notice the user time goes down again.  Maybe this indicates
that bigger chunk sizes indeed increase CPU usage, which on slower boards
would have a more profound effect than on the x86_64 machines I'm using.
An observation in line with Sandra's observation of Gary's patch speeding
up the nios2 board.  Or maybe that's just a coincidence.

BTW, the transfers seem to be always interruptible for me, even without
Gary's patch, and even the slow ones.

And finally, here's the time using a local sysroot and specifying a local
program:

$ time $g -ex "set sysroot /" -ex "tar rem :9999" -ex "b main" -ex "c" -ex "set confirm off" -ex "quit" /tmp/gdbserver
Reading symbols from /tmp/gdbserver...done.
Remote debugging using :9999
Reading symbols from /lib64/ld-linux-x86-64.so.2...Reading symbols from /usr/lib/debug/lib64/ld-2.18.so.debug...done.
done.
0x00007ffff7ddd190 in ?? ()
Breakpoint 1 at 0x41200c: file ../../../src/gdb/gdbserver/server.c, line 3635.
Continuing.

Breakpoint 1, main (argc=1, argv=0x7fffffffe3d8) at ../../../src/gdb/gdbserver/server.c:3635
3635    ../../../src/gdb/gdbserver/server.c: No such file or directory.

real    0m7.385s
user    0m0.132s
sys     0m0.028s

I'm attaching the 3 patches that I used.  I wouldn't be surprised
if they contain obvious blunders; it's getting quite late here...

Thanks,
Pedro Alves


[-- Attachment #2: 0001-gdb-remove-packet-size-limit.patch --]
[-- Type: text/x-patch, Size: 4013 bytes --]

From 5fec4911ec7cd9ea2ee4b5d3be0502e7a7df1f9f Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Tue, 18 Aug 2015 22:53:56 +0100
Subject: [PATCH 1/3] gdb: remove packet size limit

---
 gdb/remote.c | 36 +++++++++++++++---------------------
 1 file changed, 15 insertions(+), 21 deletions(-)

diff --git a/gdb/remote.c b/gdb/remote.c
index ca1f0df..a839adf 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -894,14 +894,6 @@ get_memory_packet_size (struct memory_packet_config *config)
   struct remote_state *rs = get_remote_state ();
   struct remote_arch_state *rsa = get_remote_arch_state ();
 
-  /* NOTE: The somewhat arbitrary 16k comes from the knowledge (folk
-     law?) that some hosts don't cope very well with large alloca()
-     calls.  Eventually the alloca() code will be replaced by calls to
-     xmalloc() and make_cleanups() allowing this restriction to either
-     be lifted or removed.  */
-#ifndef MAX_REMOTE_PACKET_SIZE
-#define MAX_REMOTE_PACKET_SIZE 16384
-#endif
   /* NOTE: 20 ensures we can write at least one byte.  */
 #ifndef MIN_REMOTE_PACKET_SIZE
 #define MIN_REMOTE_PACKET_SIZE 20
@@ -910,7 +902,7 @@ get_memory_packet_size (struct memory_packet_config *config)
   if (config->fixed_p)
     {
       if (config->size <= 0)
-	what_they_get = MAX_REMOTE_PACKET_SIZE;
+	what_they_get = 16384;
       else
 	what_they_get = config->size;
     }
@@ -929,8 +921,6 @@ get_memory_packet_size (struct memory_packet_config *config)
 	  && what_they_get > rsa->actual_register_packet_size)
 	what_they_get = rsa->actual_register_packet_size;
     }
-  if (what_they_get > MAX_REMOTE_PACKET_SIZE)
-    what_they_get = MAX_REMOTE_PACKET_SIZE;
   if (what_they_get < MIN_REMOTE_PACKET_SIZE)
     what_they_get = MIN_REMOTE_PACKET_SIZE;
 
@@ -3911,6 +3901,7 @@ remote_check_symbols (void)
   char *msg, *reply, *tmp;
   struct bound_minimal_symbol sym;
   int end;
+  struct cleanup *old_chain;
 
   /* The remote side has no concept of inferiors that aren't running
      yet, it only knows about running processes.  If we're connected
@@ -3929,7 +3920,8 @@ remote_check_symbols (void)
 
   /* Allocate a message buffer.  We can't reuse the input buffer in RS,
      because we need both at the same time.  */
-  msg = alloca (get_remote_packet_size ());
+  msg = xmalloc (get_remote_packet_size ());
+  old_chain = make_cleanup (xfree, msg);
 
   /* Invite target to request symbol lookups.  */
 
@@ -3967,6 +3959,8 @@ remote_check_symbols (void)
       getpkt (&rs->buf, &rs->buf_size, 0);
       reply = rs->buf;
     }
+
+  do_cleanups (old_chain);
 }
 
 static struct serial *
@@ -4089,13 +4083,6 @@ remote_packet_size (const struct protocol_feature *feature,
       return;
     }
 
-  if (packet_size > MAX_REMOTE_PACKET_SIZE)
-    {
-      warning (_("limiting remote suggested packet size (%d bytes) to %d"),
-	       packet_size, MAX_REMOTE_PACKET_SIZE);
-      packet_size = MAX_REMOTE_PACKET_SIZE;
-    }
-
   /* Record the new maximum packet size.  */
   rs->explicit_packet_size = packet_size;
 }
@@ -7645,7 +7632,8 @@ putpkt_binary (const char *buf, int cnt)
   struct remote_state *rs = get_remote_state ();
   int i;
   unsigned char csum = 0;
-  char *buf2 = alloca (cnt + 6);
+  char *buf2 = xmalloc (cnt + 6);
+  struct cleanup *old_chain = make_cleanup (xfree, buf2);
 
   int ch;
   int tcount = 0;
@@ -7738,6 +7726,7 @@ putpkt_binary (const char *buf, int cnt)
 	    case '+':
 	      if (remote_debug)
 		fprintf_unfiltered (gdb_stdlog, "Ack\n");
+	      do_cleanups (old_chain);
 	      return 1;
 	    case '-':
 	      if (remote_debug)
@@ -7746,7 +7735,10 @@ putpkt_binary (const char *buf, int cnt)
 	    case SERIAL_TIMEOUT:
 	      tcount++;
 	      if (tcount > 3)
-		return 0;
+		{
+		  do_cleanups (old_chain);
+		  return 0;
+		}
 	      break;		/* Retransmit buffer.  */
 	    case '$':
 	      {
@@ -7833,6 +7825,8 @@ putpkt_binary (const char *buf, int cnt)
 	}
 #endif
     }
+
+  do_cleanups (old_chain);
   return 0;
 }
 
-- 
1.9.3


[-- Attachment #3: 0002-gdbserver-bump-max-packet-buffer-size.patch --]
[-- Type: text/x-patch, Size: 934 bytes --]

From 0becb8d88cc5e364ce413258de32d326c6395ad6 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Wed, 19 Aug 2015 00:04:18 +0200
Subject: [PATCH 2/3] gdbserver: bump max packet buffer size

---
 gdb/gdbserver/server.h | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/gdb/gdbserver/server.h b/gdb/gdbserver/server.h
index 9080151..9ec3708 100644
--- a/gdb/gdbserver/server.h
+++ b/gdb/gdbserver/server.h
@@ -127,6 +127,8 @@ extern int handle_target_event (int err, gdb_client_data client_data);
 /* Buffer sizes for transferring memory, registers, etc.   Set to a constant
    value to accomodate multiple register formats.  This value must be at least
    as large as the largest register set supported by gdbserver.  */
-#define PBUFSIZ 16384
-
+//#define PBUFSIZ (16384)
+#define PBUFSIZ (16384*16)
+//#define PBUFSIZ 0x100000
+//#define PBUFSIZ 0x400000
 #endif /* SERVER_H */
-- 
1.9.3


[-- Attachment #4: 0003-add-readahead-cache-to-gdb-s-vFile-pread.patch --]
[-- Type: text/x-patch, Size: 4511 bytes --]

From d426ce0a36830378a8ec2e2cbdd94d9fcb47b891 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Tue, 18 Aug 2015 23:27:20 +0100
Subject: [PATCH 3/3] add readahead cache to gdb's vFile:pread

---
 gdb/remote.c | 102 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 99 insertions(+), 3 deletions(-)

diff --git a/gdb/remote.c b/gdb/remote.c
index a839adf..8a739c8 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -10311,6 +10311,26 @@ remote_hostio_send_command (int command_bytes, int which_packet,
   return ret;
 }
 
+struct readahead_cache
+{
+  int fd;
+  gdb_byte *buf;
+  ULONGEST offset;
+  size_t bufsize;
+};
+
+static struct readahead_cache *readahead_cache;
+
+static void
+readahead_cache_invalidate (void)
+{
+  if (readahead_cache != NULL)
+    readahead_cache->fd = -1;
+}
+
+static unsigned int readahead_cache_hit_count;
+static unsigned int readahead_cache_miss_count;
+
 /* Set the filesystem remote_hostio functions that take FILENAME
    arguments will use.  Return 0 on success, or -1 if an error
    occurs (and set *REMOTE_ERRNO).  */
@@ -10325,6 +10345,8 @@ remote_hostio_set_filesystem (struct inferior *inf, int *remote_errno)
   char arg[9];
   int ret;
 
+  readahead_cache_invalidate ();
+
   if (packet_support (PACKET_vFile_setfs) == PACKET_DISABLE)
     return 0;
 
@@ -10389,6 +10411,9 @@ remote_hostio_pwrite (struct target_ops *self,
   int left = get_remote_packet_size ();
   int out_len;
 
+  if (readahead_cache != NULL && readahead_cache->fd == fd)
+    readahead_cache_invalidate ();
+
   remote_buffer_add_string (&p, &left, "vFile:pwrite:");
 
   remote_buffer_add_int (&p, &left, fd);
@@ -10407,9 +10432,9 @@ remote_hostio_pwrite (struct target_ops *self,
 /* 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)
+remote_hostio_pread_1 (struct target_ops *self,
+		       int fd, gdb_byte *read_buf, int len,
+		       ULONGEST offset, int *remote_errno)
 {
   struct remote_state *rs = get_remote_state ();
   char *p = rs->buf;
@@ -10443,6 +10468,74 @@ remote_hostio_pread (struct target_ops *self,
   return ret;
 }
 
+static int
+remote_hostio_pread_from_cache (struct readahead_cache *cache,
+				int fd, gdb_byte *read_buf, int len,
+				ULONGEST offset)
+{
+  if (cache->fd == fd
+      && cache->offset <= offset
+      && offset < cache->offset + cache->bufsize)
+    {
+      ULONGEST max = cache->offset + cache->bufsize;
+
+      if (offset + len > max)
+	len = max - offset;
+
+      if (remote_debug)
+	fprintf_unfiltered (gdb_stdlog,
+			    "readahead cache hit %d\n",
+			    ++readahead_cache_hit_count);
+      memcpy (read_buf, cache->buf + offset - cache->offset, len);
+      return len;
+    }
+
+  return -1;
+}
+
+static int
+remote_hostio_pread (struct target_ops *self,
+		     int fd, gdb_byte *read_buf, int len,
+		     ULONGEST offset, int *remote_errno)
+{
+  int ret;
+
+  if (readahead_cache != NULL)
+    {
+      ret = remote_hostio_pread_from_cache (readahead_cache, fd,
+					    read_buf, len, offset);
+      if (ret >= 0)
+	return ret;
+
+      readahead_cache_invalidate ();
+    }
+
+  if (remote_debug)
+    fprintf_unfiltered (gdb_stdlog, "readahead cache miss %d\n",
+			++readahead_cache_miss_count);
+
+  if (readahead_cache == NULL)
+    readahead_cache = XCNEW (struct readahead_cache);
+  readahead_cache->fd = fd;
+  readahead_cache->offset = offset;
+  readahead_cache->bufsize = get_remote_packet_size ();
+  readahead_cache->buf = xrealloc (readahead_cache->buf, readahead_cache->bufsize);
+
+  ret = remote_hostio_pread_1 (self,
+			       readahead_cache->fd,
+			       readahead_cache->buf, readahead_cache->bufsize,
+			       readahead_cache->offset, remote_errno);
+  if (ret <= 0)
+    {
+      readahead_cache_invalidate ();
+      return ret;
+    }
+
+  readahead_cache->bufsize = ret;
+  return remote_hostio_pread_from_cache (readahead_cache, fd,
+					 read_buf, len, offset);
+}
+
 /* Implementation of to_fileio_close.  */
 
 static int
@@ -10452,6 +10545,9 @@ remote_hostio_close (struct target_ops *self, int fd, int *remote_errno)
   char *p = rs->buf;
   int left = get_remote_packet_size () - 1;
 
+  if (readahead_cache != NULL && readahead_cache->fd == fd)
+    readahead_cache_invalidate ();
+
   remote_buffer_add_string (&p, &left, "vFile:close:");
 
   remote_buffer_add_int (&p, &left, fd);
-- 
1.9.3

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

[Gary Benson]

Pedro Alves wrote:
> BTW, the transfers seem to be always interruptible for me, even without
> Gary's patch, and even the slow ones.

Ok, I'm glad I'm not the only one :)

> From d426ce0a36830378a8ec2e2cbdd94d9fcb47b891 Mon Sep 17 00:00:00 2001
> From: Pedro Alves <palves@redhat.com>
> Date: Tue, 18 Aug 2015 23:27:20 +0100
> Subject: [PATCH 3/3] add readahead cache to gdb's vFile:pread

I tried this out on its own, and got similar hit/miss ratios, so it
looks like a good addition.  Comments below.

> diff --git a/gdb/remote.c b/gdb/remote.c
> index a839adf..8a739c8 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -10311,6 +10311,26 @@ remote_hostio_send_command (int command_bytes, int which_packet,
>    return ret;
>  }
>  
> +struct readahead_cache
> +{
> +  int fd;
> +  gdb_byte *buf;
> +  ULONGEST offset;
> +  size_t bufsize;
> +};
> +
> +static struct readahead_cache *readahead_cache;

Would this be better in struct remote_state (and maybe not allocated,
just inlined in the main remote_state--it's only 16 or 32 bytes)?

> @@ -10325,6 +10345,8 @@ remote_hostio_set_filesystem
>    char arg[9];
>    int ret;
>  
> +  readahead_cache_invalidate ();
> +
>    if (packet_support (PACKET_vFile_setfs) == PACKET_DISABLE)
>      return 0;
>  

This isn't necessary, file descriptors persist across setns calls.

> +      if (remote_debug)
> +	fprintf_unfiltered (gdb_stdlog,
> +			    "readahead cache hit %d\n",
> +			    ++readahead_cache_hit_count);
and
> +  if (remote_debug)
> +    fprintf_unfiltered (gdb_stdlog, "readahead cache miss %d\n",
> +			++readahead_cache_miss_count);

These only update the counters when debug printing is switched on.
Is this what you intended?

> Looking at "set debug remote 1" output, I noticed that gdb often
> issues a sequence of small vFile:pread requests for the same file.
> Ctrl-c'ing gdb while it was doing that and getting a backtrace shows
> that those requests often came from bfd.  So I thought that maybe
> adding a readahead buffer/cache to gdb's vFile:pread requests could
> help.  And indeed, that dropped the time for the same use case
> further down to:

Another thing I noticed when I was testing the warning messages patch
is that GDB seems to (always? often?) open each file twice:

    Remote debugging using :9999
  * Reading /home/gary/work/archer/fast-transfer/build64/gdb/gdbserver/gdbserver from remote target...
    warning: File transfers from remote targets can be slow. Use "set sysroot" with no arguments to access files locally instead.
  * Reading /home/gary/work/archer/fast-transfer/build64/gdb/gdbserver/gdbserver from remote target...
    Reading symbols from target:/home/gary/work/archer/fast-transfer/build64/gdb/gdbserver/gdbserver...done.
  * Reading /lib64/ld-linux-x86-64.so.2 from remote target...
  * Reading /lib64/ld-linux-x86-64.so.2 from remote target...
    ...etc

Figuring out why this is happening could save us some more time.

Cheers,
Gary

--
http://gbenson.net/

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

[Pedro Alves]

On 08/19/2015 02:42 PM, Gary Benson wrote:
> Pedro Alves wrote:
>> BTW, the transfers seem to be always interruptible for me, even without
>> Gary's patch, and even the slow ones.
> 
> Ok, I'm glad I'm not the only one :)
> 
>> From d426ce0a36830378a8ec2e2cbdd94d9fcb47b891 Mon Sep 17 00:00:00 2001
>> From: Pedro Alves <palves@redhat.com>
>> Date: Tue, 18 Aug 2015 23:27:20 +0100
>> Subject: [PATCH 3/3] add readahead cache to gdb's vFile:pread
> 
> I tried this out on its own, and got similar hit/miss ratios, so it
> looks like a good addition.  Comments below.
> 
>> diff --git a/gdb/remote.c b/gdb/remote.c
>> index a839adf..8a739c8 100644
>> --- a/gdb/remote.c
>> +++ b/gdb/remote.c
>> @@ -10311,6 +10311,26 @@ remote_hostio_send_command (int command_bytes, int which_packet,
>>    return ret;
>>  }
>>  
>> +struct readahead_cache
>> +{
>> +  int fd;
>> +  gdb_byte *buf;
>> +  ULONGEST offset;
>> +  size_t bufsize;
>> +};
>> +
>> +static struct readahead_cache *readahead_cache;
> 
> Would this be better in struct remote_state (and maybe not allocated,
> just inlined in the main remote_state--it's only 16 or 32 bytes)?

Definitely.

(At first I made the cache invalidation free
the cache object (and set it NULL).  Then I when I saw
that the CPU usage went up, I made the invalidation just
clear fd to -1.  The CPU did not go down.  Then I lowered
back the max packet size and the CPU usage went back down,
but I kept the cache invalidation as it was.)

> 
>> @@ -10325,6 +10345,8 @@ remote_hostio_set_filesystem
>>    char arg[9];
>>    int ret;
>>  
>> +  readahead_cache_invalidate ();
>> +
>>    if (packet_support (PACKET_vFile_setfs) == PACKET_DISABLE)
>>      return 0;
>>  
> 
> This isn't necessary, file descriptors persist across setns calls.

OK.

> 
>> +      if (remote_debug)
>> +	fprintf_unfiltered (gdb_stdlog,
>> +			    "readahead cache hit %d\n",
>> +			    ++readahead_cache_hit_count);
> and
>> +  if (remote_debug)
>> +    fprintf_unfiltered (gdb_stdlog, "readahead cache miss %d\n",
>> +			++readahead_cache_miss_count);
> 
> These only update the counters when debug printing is switched on.
> Is this what you intended?

Hey, that was a quick and dirty patch.  :-)

Here's a cleaned up version.  WDYT?

---
From d853f786962906127da99c0ac42745447a9bbd05 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Thu, 20 Aug 2015 14:15:48 +0100
Subject: [PATCH] Add readahead cache to gdb's vFile:pread

This patch almost halves the time it takes to "target remote + run to
main" on a higher-latency connection.

E.g., I've got a ping time of ~85ms to an x86-64 machine on the gcc
compile farm (almost 2000km away from me), and I'm behind a ~16Mbit
ADSL.  When I connect to a gdbserver debugging itself on that machine
and run to main, it takes almost 55 seconds:

 [palves@gcc76] $ ./gdbserver :9999 ./gdbserver
 [palves@home] $ ssh -L 9999:localhost:9999 gcc76.fsffrance.org
 [palves@home] time ./gdb -data-directory=data-directory -ex "tar rem :9999" -ex "b main" -ex "c" -ex "set confirm off" -ex "quit"

 Pristine gdb 7.10.50.20150820-cvs gets us:
 ...
 Remote debugging using :9999
 Reading symbols from target:/home/palves/gdb/build/gdb/gdbserver/gdbserver...done.
 Reading symbols from target:/lib64/ld-linux-x86-64.so.2...(no debugging symbols found)...done.
 0x00007ffff7ddd190 in ?? () from target:/lib64/ld-linux-x86-64.so.2
 Breakpoint 1 at 0x41200c: file ../../../src/gdb/gdbserver/server.c, line 3635.
 Continuing.

 Breakpoint 1, main (argc=1, argv=0x7fffffffe3d8) at ../../../src/gdb/gdbserver/server.c:3635
 3635    ../../../src/gdb/gdbserver/server.c: No such file or directory.
 /home/palves/gdb/build/gdb/gdbserver/gdbserver: No such file or directory.

 real    0m54.803s
 user    0m0.329s
 sys     0m0.064s

While the readahead cache added by this patch, it drops to:

 real    0m29.462s
 user    0m0.454s
 sys     0m0.054s

I added a few counters to show cache hit/miss, and got:

 readahead cache miss 142
 readahead cache hit 310

Tested on x86_64 Fedora 20.

gdb/ChangeLog:
2015-08-20  Pedro Alves  <palves@redhat.com>

	* remote.c (struct readahead_cache): New.
	(struct remote_state) <readahead_cache>: New field.
	(remote_open_1): Invalidate the cache.
	(readahead_cache_invalidate, readahead_cache_invalidate_fd): New
	functions.
	(remote_hostio_pwrite): Invalidate the readahead cache.
	(remote_hostio_pread): Rename to ...
	(remote_hostio_pread_vFile): ... this.
	(remote_hostio_pread_from_cache): New function.
	(remote_hostio_pread): Reimplement.
	(remote_hostio_close): Invalidate the readahead cache.
---
 gdb/remote.c | 139 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 135 insertions(+), 4 deletions(-)

diff --git a/gdb/remote.c b/gdb/remote.c
index 4e483fd..bfa5469 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -226,6 +226,8 @@ static int remote_can_run_breakpoint_commands (struct target_ops *self);
 
 static void remote_btrace_reset (void);
 
+static void readahead_cache_invalidate (void);
+
 /* For "remote".  */
 
 static struct cmd_list_element *remote_cmdlist;
@@ -262,6 +264,29 @@ typedef unsigned char threadref[OPAQUETHREADBYTES];
 
 #define MAXTHREADLISTRESULTS 32
 
+/* Data for the vFile:pread readahead cache.  */
+
+struct readahead_cache
+{
+  /* The file descriptor for the file that is being cached.  -1 if the
+     cache is invalid.  */
+  int fd;
+
+  /* The offset into the file that the cache buffer corresponds
+     to.  */
+  ULONGEST offset;
+
+  /* The buffer holding the cache contents.  */
+  gdb_byte *buf;
+  /* The buffer's size.  We try to read as much as fits into a packet
+     at a time.  */
+  size_t bufsize;
+
+  /* Cache hit and miss counters.  */
+  ULONGEST hit_count;
+  ULONGEST miss_count;
+};
+
 /* Description of the remote protocol state for the currently
    connected target.  This is per-target state, and independent of the
    selected architecture.  */
@@ -381,6 +406,14 @@ struct remote_state
      Initialized to -1 to indicate that no "vFile:setfs:" packet
      has yet been sent.  */
   int fs_pid;
+
+  /* A readahead cache for vFile:pread.  Often, reading a binary
+     involves a sequence of small reads.  E.g., when parsing an ELF
+     file.  A readahead cache helps mostly the case of remote
+     debugging on a connection with higher latency, due to the
+     request/reply nature of the RSP.  We only cache data for a single
+     file descriptor at a time.  */
+  struct readahead_cache readahead_cache;
 };
 
 /* Private data that we'll store in (struct thread_info)->private.  */
@@ -4498,6 +4531,8 @@ remote_open_1 (const char *name, int from_tty,
   rs->use_threadinfo_query = 1;
   rs->use_threadextra_query = 1;
 
+  readahead_cache_invalidate ();
+
   if (target_async_permitted)
     {
       /* With this target we start out by owning the terminal.  */
@@ -10329,6 +10364,27 @@ remote_hostio_send_command (int command_bytes, int which_packet,
   return ret;
 }
 
+/* Invalidate the readahead cache.  */
+
+static void
+readahead_cache_invalidate (void)
+{
+  struct remote_state *rs = get_remote_state ();
+
+  rs->readahead_cache.fd = -1;
+}
+
+/* Invalidate the readahead cache if it is holding data for FD.  */
+
+static void
+readahead_cache_invalidate_fd (int fd)
+{
+  struct remote_state *rs = get_remote_state ();
+
+  if (rs->readahead_cache.fd == fd)
+    rs->readahead_cache.fd = -1;
+}
+
 /* Set the filesystem remote_hostio functions that take FILENAME
    arguments will use.  Return 0 on success, or -1 if an error
    occurs (and set *REMOTE_ERRNO).  */
@@ -10407,6 +10463,8 @@ remote_hostio_pwrite (struct target_ops *self,
   int left = get_remote_packet_size ();
   int out_len;
 
+  readahead_cache_invalidate_fd (fd);
+
   remote_buffer_add_string (&p, &left, "vFile:pwrite:");
 
   remote_buffer_add_int (&p, &left, fd);
@@ -10422,12 +10480,13 @@ remote_hostio_pwrite (struct target_ops *self,
 				     remote_errno, NULL, NULL);
 }
 
-/* Implementation of to_fileio_pread.  */
+/* Helper for the implementation of to_fileio_pread.  Read the file
+   from the remote side with vFile: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_vFile (struct target_ops *self,
+			   int fd, gdb_byte *read_buf, int len,
+			   ULONGEST offset, int *remote_errno)
 {
   struct remote_state *rs = get_remote_state ();
   char *p = rs->buf;
@@ -10461,6 +10520,76 @@ remote_hostio_pread (struct target_ops *self,
   return ret;
 }
 
+/* Serve pread from the readahead cache.  Returns number of bytes
+   read, or 0 if the request can't be served from the cache.  */
+
+static int
+remote_hostio_pread_from_cache (struct remote_state *rs,
+				int fd, gdb_byte *read_buf, size_t len,
+				ULONGEST offset)
+{
+  struct readahead_cache *cache = &rs->readahead_cache;
+
+  if (cache->fd == fd
+      && cache->offset <= offset
+      && offset < cache->offset + cache->bufsize)
+    {
+      ULONGEST max = cache->offset + cache->bufsize;
+
+      if (offset + len > max)
+	len = max - offset;
+
+      memcpy (read_buf, cache->buf + offset - cache->offset, len);
+      return len;
+    }
+
+  return 0;
+}
+
+/* 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)
+{
+  int ret;
+  struct remote_state *rs = get_remote_state ();
+  struct readahead_cache *cache = &rs->readahead_cache;
+
+  ret = remote_hostio_pread_from_cache (rs, fd, read_buf, len, offset);
+  if (ret > 0)
+    {
+      cache->hit_count++;
+
+      if (remote_debug)
+	fprintf_unfiltered (gdb_stdlog, "readahead cache hit %s\n",
+			    pulongest (cache->hit_count));
+      return ret;
+    }
+
+  cache->miss_count++;
+  if (remote_debug)
+    fprintf_unfiltered (gdb_stdlog, "readahead cache miss %s\n",
+			pulongest (cache->miss_count));
+
+  cache->fd = fd;
+  cache->offset = offset;
+  cache->bufsize = get_remote_packet_size ();
+  cache->buf = xrealloc (cache->buf, cache->bufsize);
+
+  ret = remote_hostio_pread_vFile (self, cache->fd, cache->buf, cache->bufsize,
+				   cache->offset, remote_errno);
+  if (ret <= 0)
+    {
+      readahead_cache_invalidate_fd (fd);
+      return ret;
+    }
+
+  cache->bufsize = ret;
+  return remote_hostio_pread_from_cache (rs, fd, read_buf, len, offset);
+}
+
 /* Implementation of to_fileio_close.  */
 
 static int
@@ -10470,6 +10599,8 @@ remote_hostio_close (struct target_ops *self, int fd, int *remote_errno)
   char *p = rs->buf;
   int left = get_remote_packet_size () - 1;
 
+  readahead_cache_invalidate_fd (fd);
+
   remote_buffer_add_string (&p, &left, "vFile:close:");
 
   remote_buffer_add_int (&p, &left, fd);
-- 
1.9.3

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

[Gary Benson]

Pedro Alves wrote:
> Hey, that was a quick and dirty patch.  :-)

I know, no worries :)

> Here's a cleaned up version.  WDYT?
[snip
> This patch almost halves the time it takes to "target remote + run
> to main" on a higher-latency connection.
> 
> E.g., I've got a ping time of ~85ms to an x86-64 machine on the gcc
> compile farm (almost 2000km away from me), and I'm behind a ~16Mbit
> ADSL.  When I connect to a gdbserver debugging itself on that machine
> and run to main, it takes almost 55 seconds:
[snip]
>  real    0m54.803s
>  user    0m0.329s
>  sys     0m0.064s
> 
> While the readahead cache added by this patch, it drops to:
> 
>  real    0m29.462s
>  user    0m0.454s
>  sys     0m0.054s
> 
> I added a few counters to show cache hit/miss, and got:
> 
>  readahead cache miss 142
>  readahead cache hit 310
> 
> Tested on x86_64 Fedora 20.

Very nice, please commit!

Thanks,
Gary

-- 
http://gbenson.net/

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

[Jan Kratochvil]

On Tue, 11 Aug 2015 21:43:47 +0200, Doug Evans wrote:
> On Tue, Aug 11, 2015 at 11:55 AM, Jan Kratochvil <jan.kratochvil@redhat.com> wrote:
> > I would like to pinpoint that for patches targeting source distribution such
> > an additional behavior change switch is a QA nightmare.
> 
> I'm curious what the QA costs of, for example, --with-sysroot are.

You are right it would make sense to make some cross-check that --with-sysroot
containing the same data works the same.


> [And in particular are they nightmarish?]

It was just related to me personally because I am developing these days
a related patchset on top of it.
	https://sourceware.org/git/?p=archer.git;a=log;h=refs/heads/jankratochvil/gdbserverbuildid-locate


Jan

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

[Pedro Alves]

On 08/11/2015 06:22 PM, Doug Evans wrote:
> Gary Benson writes:
>   > 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?
> 
> For 7.10, one thought is to maintain the behaviour of 7.9
> and give ourselves more time to address this.

Agreed.  My opinion, as expressed elsewhere in the gdb@ thread,
is that in order to be able to have target: by default in 7.10, we'd
try to both sort out the interruptibility and add a suggestive
warning, assuming both were easy to do, and not invasive,
and check if that would be a sufficient resolution.  Seems like
the interruptibility issue isn't trivial to solve, so I think we
need to go do that -- change the default sysroot back to "", (and tweak
the docs/NEWS accordingly), and buy some time to sort this out on master.
Users can still then put "set sysroot target:" in .gdbinit with 7.10,
integrators should still be able to build with --with-sysroot=target: (or
revert the future-default-sysroot-reversion patch), and we can
continue addressing identified issues until "target:" (or something
around it, maybe building up on Jan's buildid work) can be made
the default, on master.

> IOW, can we have (or is there already) a configure
> option that controls the default behaviour,
> and can we default it to what 7.9 does
> (not auto-fetch files) ?

I think that would be the existing --with-sysroot.

Thanks,
Pedro Alves