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

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

[Doug Evans]

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

FAOD, target_fileio_open_warn_if_slow?

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/

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

[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