[patch] Suggest newer gdbserver if it has no qXfer:exec-file:read

[patch] Suggest newer gdbserver if it has no qXfer:exec-file:read

[Jan Kratochvil]

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

Hi,

currently:
	$ gdbserver-7.9 :1234 true &
	$ gdb -q -ex 'target remote :1234' # that -q is not relevant here
	Remote debugging using :1234
	warning: Could not load vsyscall page because no executable was specified
	try using the "file" command first.
	0x00007ffff7ddcc80 in ?? ()
	(gdb) b main
	No symbol table is loaded.  Use the "file" command.
	Make breakpoint pending on future shared library load? (y or [n]) _

While one may not realize a newer gdbserver would fix that:
	$ gdbserver-7.10 :1234 true &
	$ gdb -q -ex 'target remote :1234' # that -q is not relevant here
	Remote debugging using :1234
	Reading /usr/bin/true from remote target...
	warning: File transfers from remote targets can be slow. Use "set sysroot" to access files locally instead.
	Reading /usr/bin/true from remote target...
	Reading symbols from target:/usr/bin/true...Reading symbols from /usr/lib/debug/usr/bin/true.debug...done.
	done.
	Reading /lib64/ld-linux-x86-64.so.2 from remote target...
	Reading /lib64/ld-linux-x86-64.so.2 from remote target...
	Reading symbols from target:/lib64/ld-linux-x86-64.so.2...Reading symbols from /usr/lib/debug/usr/lib64/ld-2.22.so.debug...done.
	done.
	0x00007ffff7ddcc80 in _start () from target:/lib64/ld-linux-x86-64.so.2
	(gdb) b main
	Breakpoint 1 at 0x555555555650: file src/true.c, line 59.
	(gdb) _

This can be more common case with the popular containers.  Therefore
suggesting to print there also:
	warning: No executable has been specified (see the "file" command) and remote gdbserver does not support packet "qXfer:exec-file:read" - please use FSF gdbserver version 7.10 or later.

OK for check-in?

No regressions on {x86_64,x86_64-m32,i686}-fedora23-linux-gnu.

The "qXfer:exec-file:read" support in GDB and gdbserver was implemented by:
	commit c78fa86a213db1bdef328437ac262a4f54577827
	Author: Gary Benson <gbenson@redhat.com>
	Date:   Fri Apr 17 09:47:30 2015 +0100
	    Implement remote_pid_to_exec_file using qXfer:exec-file:read


Thanks,
Jan

[-- Attachment #2: gdbexec.patch --]
[-- Type: text/plain, Size: 789 bytes --]

gdb/ChangeLog
2016-03-19  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* remote.c (remote_pid_to_exec_file): Print warning for unsupported
	PACKET_qXfer_exec_file.

diff --git a/gdb/remote.c b/gdb/remote.c
index af0a08a..d267736 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -12977,7 +12977,13 @@ remote_pid_to_exec_file (struct target_ops *self, int pid)
   char *annex = NULL;
 
   if (packet_support (PACKET_qXfer_exec_file) != PACKET_ENABLE)
-    return NULL;
+    {
+      warning (_("No executable has been specified (see the \"file\" command) "
+                 "and remote gdbserver does not "
+		 "support packet \"qXfer:exec-file:read\""
+		 " - please use FSF gdbserver version 7.10 or later."));
+      return NULL;
+    }
 
   if (filename != NULL)
     xfree (filename);

Re: [patch] Suggest newer gdbserver if it has no qXfer:exec-file:read

[Gary Benson]

Jan Kratochvil wrote:
> gdb/ChangeLog
> 2016-03-19  Jan Kratochvil  <jan.kratochvil@redhat.com>
> 
> 	* remote.c (remote_pid_to_exec_file): Print warning for unsupported
> 	PACKET_qXfer_exec_file.
> 
> diff --git a/gdb/remote.c b/gdb/remote.c
> index af0a08a..d267736 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -12977,7 +12977,13 @@ remote_pid_to_exec_file (struct target_ops *self, int pid)
>    char *annex = NULL;
>  
>    if (packet_support (PACKET_qXfer_exec_file) != PACKET_ENABLE)
> -    return NULL;
> +    {
> +      warning (_("No executable has been specified (see the \"file\" command) "
> +                 "and remote gdbserver does not "
> +		 "support packet \"qXfer:exec-file:read\""
> +		 " - please use FSF gdbserver version 7.10 or later."));
> +      return NULL;
> +    }
>  
>    if (filename != NULL)
>      xfree (filename);

This looks good to me.

Thanks,
Gary

-- 
http://gbenson.net/

Re: [patch] Suggest newer gdbserver if it has no qXfer:exec-file:read

[Pedro Alves]

On 03/19/2016 08:18 PM, Jan Kratochvil wrote:
> Hi,
>
> currently:
> 	$ gdbserver-7.9 :1234 true &
> 	$ gdb -q -ex 'target remote :1234' # that -q is not relevant here
> 	Remote debugging using :1234
> 	warning: Could not load vsyscall page because no executable was specified
> 	try using the "file" command first.
> 	0x00007ffff7ddcc80 in ?? ()
> 	(gdb) b main
> 	No symbol table is loaded.  Use the "file" command.
> 	Make breakpoint pending on future shared library load? (y or [n]) _
>
> While one may not realize a newer gdbserver would fix that:
> 	$ gdbserver-7.10 :1234 true &
> 	$ gdb -q -ex 'target remote :1234' # that -q is not relevant here
> 	Remote debugging using :1234
> 	Reading /usr/bin/true from remote target...
> 	warning: File transfers from remote targets can be slow. Use "set sysroot" to access files locally instead.
> 	Reading /usr/bin/true from remote target...
> 	Reading symbols from target:/usr/bin/true...Reading symbols from /usr/lib/debug/usr/bin/true.debug...done.
> 	done.
> 	Reading /lib64/ld-linux-x86-64.so.2 from remote target...
> 	Reading /lib64/ld-linux-x86-64.so.2 from remote target...
> 	Reading symbols from target:/lib64/ld-linux-x86-64.so.2...Reading symbols from /usr/lib/debug/usr/lib64/ld-2.22.so.debug...done.
> 	done.
> 	0x00007ffff7ddcc80 in _start () from target:/lib64/ld-linux-x86-64.so.2
> 	(gdb) b main
> 	Breakpoint 1 at 0x555555555650: file src/true.c, line 59.
> 	(gdb) _
>
> This can be more common case with the popular containers.  Therefore
> suggesting to print there also:
> 	warning: No executable has been specified (see the "file" command) and remote gdbserver does not support packet "qXfer:exec-file:read" - please use FSF gdbserver version 7.10 or later.
>
> OK for check-in?
>
> No regressions on {x86_64,x86_64-m32,i686}-fedora23-linux-gnu.
>
> The "qXfer:exec-file:read" support in GDB and gdbserver was implemented by:
> 	commit c78fa86a213db1bdef328437ac262a4f54577827
> 	Author: Gary Benson<gbenson@redhat.com>
> 	Date:   Fri Apr 17 09:47:30 2015 +0100
> 	    Implement remote_pid_to_exec_file using qXfer:exec-file:read
>
>
> Thanks,
> Jan
>
>
> gdbexec.patch
>
>
> gdb/ChangeLog
> 2016-03-19  Jan Kratochvil<jan.kratochvil@redhat.com>
>
> 	* remote.c (remote_pid_to_exec_file): Print warning for unsupported
> 	PACKET_qXfer_exec_file.
>
> diff --git a/gdb/remote.c b/gdb/remote.c
> index af0a08a..d267736 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -12977,7 +12977,13 @@ remote_pid_to_exec_file (struct target_ops *self, int pid)
>     char *annex = NULL;
>
>     if (packet_support (PACKET_qXfer_exec_file) != PACKET_ENABLE)
> -    return NULL;
> +    {
> +      warning (_("No executable has been specified (see the \"file\" command) "
> +                 "and remote gdbserver does not "
> +		 "support packet \"qXfer:exec-file:read\""
> +		 " - please use FSF gdbserver version 7.10 or later."));
> +      return NULL;
> +    }

I think this will print the warning after connecting to any
random stub, not just gdbserver.  Won't it be confusing
to suggest FSF gdbserver in that case?

Thanks,
Pedro Alves

Re: [patch] Suggest newer gdbserver if it has no qXfer:exec-file:read

[Jan Kratochvil]

On Tue, 22 Mar 2016 13:24:03 +0100, Pedro Alves wrote:
> On 03/19/2016 08:18 PM, Jan Kratochvil wrote:
> >    if (packet_support (PACKET_qXfer_exec_file) != PACKET_ENABLE)
> >-    return NULL;
> >+    {
> >+      warning (_("No executable has been specified (see the \"file\" command) "
> >+                 "and remote gdbserver does not "
> >+		 "support packet \"qXfer:exec-file:read\""
> >+		 " - please use FSF gdbserver version 7.10 or later."));
> >+      return NULL;
> >+    }
> 
> I think this will print the warning after connecting to any
> random stub, not just gdbserver.  Won't it be confusing
> to suggest FSF gdbserver in that case?

(1) I think this message can only appear during a mistake.  Is it right?
In fact this is my primary concern with this patch.
In such case I find any info better than no info.

(2) Still it may suggest they could for example implement qXfer:exec-file:read
in their gdbserver stub if appropriate.  I believe that people who use custom
gdbserver stub are more aware of how to fix it than normal
(=desktop/enterprise) OS developers who just try to debug some programs.

(3) Do you have a better idea?  One could add "if approproate" in that
message but I find that excessive.  One could detect FSF gdbserver
(if possible, I do not think it is, BTW it could be good to identify
variant+version of gdbserver over the protocol) but then still if it either is
or is not a FSF gdbserver that message may be relevant in some cases.


Thanks,
Jan

Re: [patch] Suggest newer gdbserver if it has no qXfer:exec-file:read

[Pedro Alves]

On 03/22/2016 01:16 PM, Jan Kratochvil wrote:
> On Tue, 22 Mar 2016 13:24:03 +0100, Pedro Alves wrote:
>> On 03/19/2016 08:18 PM, Jan Kratochvil wrote:
>>>     if (packet_support (PACKET_qXfer_exec_file) != PACKET_ENABLE)
>>> -    return NULL;
>>> +    {
>>> +      warning (_("No executable has been specified (see the \"file\" command) "
>>> +                 "and remote gdbserver does not "
>>> +		 "support packet \"qXfer:exec-file:read\""
>>> +		 " - please use FSF gdbserver version 7.10 or later."));
>>> +      return NULL;
>>> +    }
>>
>> I think this will print the warning after connecting to any
>> random stub, not just gdbserver.  Won't it be confusing
>> to suggest FSF gdbserver in that case?
> 
> (1) I think this message can only appear during a mistake.  Is it right?
> In fact this is my primary concern with this patch.
> In such case I find any info better than no info.
> 
> (2) Still it may suggest they could for example implement qXfer:exec-file:read
> in their gdbserver stub if appropriate.  I believe that people who use custom
> gdbserver stub are more aware of how to fix it than normal
> (=desktop/enterprise) OS developers who just try to debug some programs.
> 
> (3) Do you have a better idea?  One could add "if approproate" in that
> message but I find that excessive.  One could detect FSF gdbserver
> (if possible, I do not think it is, BTW it could be good to identify
> variant+version of gdbserver over the protocol) but then still if it either is
> or is not a FSF gdbserver that message may be relevant in some cases.
> 

- It's usually a mistake, though you can genuinely not have a
  binary available.  Not "only", but "usually".

- Random stubs may not know at all the executable that is running -- the
  remote end is often just bare metal raw memory, no concept of elf, etc.
  So it's not just a matter of implementing a packet - more tooling might
  and I suspect will, be necessary.  OTOH, there are OSs where it's just not
  possible, by design, to retrieve the name of the executable a process
  is running, like OpenBSD (I find it odd not to allow a ptracer access
  to that, but, alas).

- I think the important points are:

  - The user did not specify the executable manually.

  - The target/server does not support automatic executable
    retrieval. 

- I see that at least the following choices to correct the situation:

  #1 - Upgrade server to some version that supports automatic automatic
       executable retrieval.

  #2 - Hack on stub/server yourself to add support for automatic 
       executable retrieval, if possible on the target.

  #3 - Use the "file" command.

If you're connecting with a new gdb to an older gdbserver, it usually
means that installing a newer gdbserver is more than a couple
seconds work, and may not even be possible.  I think #3 will be the
path most often taken.

So I'd suggest:

   warning: No executable has been specified and target does not support
   determining executable automatically.  Try using the \"file\" command.

Seeing this, users that can hack on a remote stub will probably
realize that there's now some way for gdb to automatically retrieve
the executable.  We don't need to expose implementation details for those
users; they'll be savvy enough to find the necessary info in the RSP
manual.  For other users, talking about implementation details may
largely be noise.

Thinking of local/remote parity (and perhaps some day using gdbserver
for local debugging), that text is also generic enough that it could
be emitted by common code instead.

WDYT?

Thanks,
Pedro Alves

Re: [patch] Suggest newer gdbserver if it has no qXfer:exec-file:read

[Jan Kratochvil]

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

On Tue, 22 Mar 2016 14:56:46 +0100, Pedro Alves wrote:
> - Random stubs may not know at all the executable that is running -- the
>   remote end is often just bare metal raw memory, no concept of elf, etc.
>   So it's not just a matter of implementing a packet - more tooling might
>   and I suspect will, be necessary.  OTOH, there are OSs where it's just not
>   possible, by design, to retrieve the name of the executable a process
>   is running, like OpenBSD (I find it odd not to allow a ptracer access
>   to that, but, alas).

IIUC for such an embedded target without any filesystem qXfer:exec-file:read
would need to generate a bogus filename which would be then
recognized/accepted by vFile:open.

Sending packet: $qXfer:exec-file:read:67:0,fff#f7...Packet received: l/root/redhat/threadit
Reading /root/redhat/threadit from remote target...
Sending packet: $vFile:open:2f726f6f742f7265646861742f7468726561646974,0,0#7e...Packet received: F5
Sending packet: $vFile:pread:5,3fff,0#98...Packet received: F27f8;\177ELF\002\001\001\000

Just stating that, nothing interesting.


> - I think the important points are:
> 
>   - The user did not specify the executable manually.
> 
>   - The target/server does not support automatic executable
>     retrieval. 
> 
> - I see that at least the following choices to correct the situation:
> 
>   #1 - Upgrade server to some version that supports automatic automatic
>        executable retrieval.
> 
>   #2 - Hack on stub/server yourself to add support for automatic 
>        executable retrieval, if possible on the target.
> 
>   #3 - Use the "file" command.
> 
> If you're connecting with a new gdb to an older gdbserver, it usually
> means that installing a newer gdbserver is more than a couple
> seconds work, and may not even be possible.  I think #3 will be the
> path most often taken.
> 
> So I'd suggest:
> 
>    warning: No executable has been specified and target does not support
>    determining executable automatically.  Try using the \"file\" command.
> 
> Seeing this, users that can hack on a remote stub will probably
> realize that there's now some way for gdb to automatically retrieve
> the executable.  We don't need to expose implementation details for those
> users; they'll be savvy enough to find the necessary info in the RSP
> manual.  For other users, talking about implementation details may
> largely be noise.

I still do not see there any hint that a newer FSF gdbserver would also fix the
problem.

Particularly because the "file" command is not the whole truth, plain
executable is sufficient mostly only for embedded systems.  For normal OSes
one needs also the shared libraries and those all are IMO not easy to access
without new FSF gdbserver.  But that's a whole new bug/issue:

With current FSF GDB HEAD and old FSF gdbserver I expected I could do:
	gdb -ex 'file target:/root/redhat/threadit' -ex 'target remote :1234' 
(supplying that unsupported qXfer:exec-file:read by "file")
But that does not work because:
	Sending packet: $vFile:setfs:0#bf...Packet received: OK
	Packet vFile:setfs (hostio-setfs) is supported
	...
	Sending packet: $vFile:setfs:104#24...Packet received: OK
	"target:/root/redhat/threadit": could not open as an executable file: Invalid argument

GDB documentation says:
	The valid responses to Host I/O packets are:
	An empty response indicates that this operation is not recognized.

OT: I do not see why "empty response" is response "OK" but apparently it is.

You can see that gdbserver above said "setfs" is unsupported but GDB
recognized it as a response gdbserver does support it.  Later
remote_hostio_set_filesystem() reports -1 as it is confused by the invalid
empty response for setfs while it sees no PACKET_DISABLE indication.
And GDB errors on it as remote_hostio_set_filesystem() returns 0 on
PACKET_DISABLE.

Are you aware of this general PACKET_DISABLE bug?  I can prepare some more
general real patch if not.

With the attached hack in packet_ok() I really can debug fine:
with unpatched old FSF gdbserver and patched FSF GDB HEAD:
	gdb -ex 'file target:/root/redhat/threadit' -ex 'target remote :1234' 
	Sending packet: $vFile:setfs:0#bf...Packet received: OK
	Packet vFile:setfs (hostio-setfs) is NOT supported
	...
	(gdb) info sharedlibrary 
	From                To                  Syms Read   Shared Object Library
	0x00007ffff7ddbae0  0x00007ffff7df627a  Yes (*)     target:/lib64/ld-linux-x86-64.so.2
	0x00007ffff7bc48a0  0x00007ffff7bcf514  Yes (*)     target:/lib64/libpthread.so.0


> Thinking of local/remote parity (and perhaps some day using gdbserver
> for local debugging), that text is also generic enough that it could
> be emitted by common code instead.

That's a good idea.  So we may be talking about two messages.


Attached patch prints messages as:

Remote debugging using :1234
warning: Remote gdbserver does not support determining executable automatically.
FSF gdbserver version 7.10 or later would support that.
warning: No executable has been specified and target does not support
determining executable automatically.  Try using the "file" command.
warning: Could not load vsyscall page because no executable was specified
0x00007ffff7ddcc80 in ?? ()
(gdb) _


Thanks,
Jan

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

gdb/ChangeLog
2016-03-23  Jan Kratochvil  <jan.kratochvil@redhat.com>
	    Pedro Alves  <palves@redhat.com>

	* exec.c (exec_file_locate_attach): Print warning for unsupported
	target_pid_to_exec_file.
	* remote.c (remote_pid_to_exec_file): Print warning for unsupported
	PACKET_qXfer_exec_file.
	* symfile-mem.c (add_vsyscall_page): Remove the "file" command
	message part.

diff --git a/gdb/exec.c b/gdb/exec.c
index 90811c0..a10ab9b 100644
--- a/gdb/exec.c
+++ b/gdb/exec.c
@@ -151,7 +151,13 @@ exec_file_locate_attach (int pid, int from_tty)
   /* Try to determine a filename from the process itself.  */
   exec_file = target_pid_to_exec_file (pid);
   if (exec_file == NULL)
-    return;
+    {
+      warning (_("No executable has been specified and target does not "
+		 "support\n"
+		 "determining executable automatically.  "
+		 "Try using the \"file\" command."));
+      return;
+    }
 
   /* If gdb_sysroot is not empty and the discovered filename
      is absolute then prefix the filename with gdb_sysroot.  */
diff --git a/gdb/remote.c b/gdb/remote.c
index af0a08a..4e44ffd 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -1311,7 +1311,10 @@ packet_ok (const char *buf, struct packet_config *config)
     internal_error (__FILE__, __LINE__,
 		    _("packet_ok: attempt to use a disabled packet"));
 
-  result = packet_check_result (buf);
+  if (strcmp (buf, "OK") == 0 && startswith (config->name, "vFile:"))
+    result = PACKET_UNKNOWN;
+  else
+    result = packet_check_result (buf);
   switch (result)
     {
     case PACKET_OK:
@@ -12977,7 +12980,12 @@ remote_pid_to_exec_file (struct target_ops *self, int pid)
   char *annex = NULL;
 
   if (packet_support (PACKET_qXfer_exec_file) != PACKET_ENABLE)
-    return NULL;
+    {
+      warning (_("Remote gdbserver does not support determining executable "
+		 "automatically.\n"
+		 "FSF gdbserver version 7.10 or later would support that."));
+      return NULL;
+    }
 
   if (filename != NULL)
     xfree (filename);
diff --git a/gdb/symfile-mem.c b/gdb/symfile-mem.c
index 8eb5176..79739a6 100644
--- a/gdb/symfile-mem.c
+++ b/gdb/symfile-mem.c
@@ -214,8 +214,7 @@ add_vsyscall_page (struct target_ops *target, int from_tty)
 	  format should fix this.  */
 	{
 	  warning (_("Could not load vsyscall page "
-		     "because no executable was specified\n"
-		     "try using the \"file\" command first."));
+		     "because no executable was specified"));
 	  return;
 	}
       args.bfd = bfd;

Re: [patch] Suggest newer gdbserver if it has no qXfer:exec-file:read

[Jan Kratochvil]

On Wed, 23 Mar 2016 22:15:47 +0100, Jan Kratochvil wrote:
> With current FSF GDB HEAD and old FSF gdbserver I expected I could do:
> 	gdb -ex 'file target:/root/redhat/threadit' -ex 'target remote :1234' 
> (supplying that unsupported qXfer:exec-file:read by "file")
> But that does not work because:
> 	Sending packet: $vFile:setfs:0#bf...Packet received: OK
> 	Packet vFile:setfs (hostio-setfs) is supported
> 	...
> 	Sending packet: $vFile:setfs:104#24...Packet received: OK
> 	"target:/root/redhat/threadit": could not open as an executable file: Invalid argument

Filed as:
	7.10 regression: gdb remote.c due to "setfs" with gdbserver <=7.9
	https://sourceware.org/bugzilla/show_bug.cgi?id=19863

[patch] Workaround gdbserver<7.7 for setfs [Re: [patch] Suggest newer gdbserver if it has no qXfer:exec-file:read]

[Jan Kratochvil]

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

On Wed, 23 Mar 2016 22:15:47 +0100, Jan Kratochvil wrote:
> With current FSF GDB HEAD and old FSF gdbserver I expected I could do:
> 	gdb -ex 'file target:/root/redhat/threadit' -ex 'target remote :1234' 
> (supplying that unsupported qXfer:exec-file:read by "file")
> But that does not work because:
> 	Sending packet: $vFile:setfs:0#bf...Packet received: OK
> 	Packet vFile:setfs (hostio-setfs) is supported
> 	...
> 	Sending packet: $vFile:setfs:104#24...Packet received: OK
> 	"target:/root/redhat/threadit": could not open as an executable file: Invalid argument
> 
> GDB documentation says:
> 	The valid responses to Host I/O packets are:
> 	An empty response indicates that this operation is not recognized.
> 
> OT: I do not see why "empty response" is response "OK" but apparently it is.

This "empty response" vs. "OK" was a bug in gdbserver < 7.7.  It was fixed by:
	commit e7f0d979dd5cc4f8b658df892e93db69d6d660b7
	Author: Yao Qi <yao@codesourcery.com>
	Date:   Tue Dec 10 21:59:20 2013 +0800
	    Fix a bug in matching notifications.
	Message-ID: <1386684626-11415-1-git-send-email-yao@codesourcery.com>
	https://sourceware.org/ml/gdb-patches/2013-12/msg00373.html
	2013-12-10  Yao Qi  <yao@codesourcery.com>
		* notif.c (handle_notif_ack): Return 0 if no notification
		matches.

But I would prefer to make gdb compatible with gdbserver-7.6.1.

OK for check-in?


Thanks,
Jan

[-- Attachment #2: notif2.patch --]
[-- Type: text/plain, Size: 753 bytes --]

gdb/ChangeLog
2016-03-24  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* remote.c (packet_ok): Add workaround for vFile:setfs.

diff --git a/gdb/remote.c b/gdb/remote.c
index af0a08a..be5cc09 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -1311,7 +1311,14 @@ packet_ok (const char *buf, struct packet_config *config)
     internal_error (__FILE__, __LINE__,
 		    _("packet_ok: attempt to use a disabled packet"));
 
-  result = packet_check_result (buf);
+  if (strcmp (config->name, "vFile:setfs") == 0 && strcmp (buf, "OK") == 0)
+    {
+      /* Workaround gdbserver < 7.7 before its fix from 2013-12-11.  */
+      result = PACKET_UNKNOWN;
+    }
+  else
+    result = packet_check_result (buf);
+
   switch (result)
     {
     case PACKET_OK:

[patchv2 1/2] Workaround gdbserver<7.7 for setfs

[Jan Kratochvil]

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

There was a bug in patchv1.

[-- Attachment #2: move1.patch --]
[-- Type: text/plain, Size: 3700 bytes --]

gdb/ChangeLog
2016-03-24  Jan Kratochvil  <jan.kratochvil@redhat.com>

	Code cleanup.
	* remote.c (packet_ok): Move it after remote_protocol_packets.

diff --git a/gdb/remote.c b/gdb/remote.c
index af0a08a..bb027cf 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -1301,59 +1301,6 @@ packet_check_result (const char *buf)
     return PACKET_UNKNOWN;
 }
 
-static enum packet_result
-packet_ok (const char *buf, struct packet_config *config)
-{
-  enum packet_result result;
-
-  if (config->detect != AUTO_BOOLEAN_TRUE
-      && config->support == PACKET_DISABLE)
-    internal_error (__FILE__, __LINE__,
-		    _("packet_ok: attempt to use a disabled packet"));
-
-  result = packet_check_result (buf);
-  switch (result)
-    {
-    case PACKET_OK:
-    case PACKET_ERROR:
-      /* The stub recognized the packet request.  */
-      if (config->support == PACKET_SUPPORT_UNKNOWN)
-	{
-	  if (remote_debug)
-	    fprintf_unfiltered (gdb_stdlog,
-				"Packet %s (%s) is supported\n",
-				config->name, config->title);
-	  config->support = PACKET_ENABLE;
-	}
-      break;
-    case PACKET_UNKNOWN:
-      /* The stub does not support the packet.  */
-      if (config->detect == AUTO_BOOLEAN_AUTO
-	  && config->support == PACKET_ENABLE)
-	{
-	  /* If the stub previously indicated that the packet was
-	     supported then there is a protocol error.  */
-	  error (_("Protocol error: %s (%s) conflicting enabled responses."),
-		 config->name, config->title);
-	}
-      else if (config->detect == AUTO_BOOLEAN_TRUE)
-	{
-	  /* The user set it wrong.  */
-	  error (_("Enabled packet %s (%s) not recognized by stub"),
-		 config->name, config->title);
-	}
-
-      if (remote_debug)
-	fprintf_unfiltered (gdb_stdlog,
-			    "Packet %s (%s) is NOT supported\n",
-			    config->name, config->title);
-      config->support = PACKET_DISABLE;
-      break;
-    }
-
-  return result;
-}
-
 enum {
   PACKET_vCont = 0,
   PACKET_X,
@@ -1496,6 +1443,59 @@ enum {
 
 static struct packet_config remote_protocol_packets[PACKET_MAX];
 
+static enum packet_result
+packet_ok (const char *buf, struct packet_config *config)
+{
+  enum packet_result result;
+
+  if (config->detect != AUTO_BOOLEAN_TRUE
+      && config->support == PACKET_DISABLE)
+    internal_error (__FILE__, __LINE__,
+		    _("packet_ok: attempt to use a disabled packet"));
+
+  result = packet_check_result (buf);
+  switch (result)
+    {
+    case PACKET_OK:
+    case PACKET_ERROR:
+      /* The stub recognized the packet request.  */
+      if (config->support == PACKET_SUPPORT_UNKNOWN)
+	{
+	  if (remote_debug)
+	    fprintf_unfiltered (gdb_stdlog,
+				"Packet %s (%s) is supported\n",
+				config->name, config->title);
+	  config->support = PACKET_ENABLE;
+	}
+      break;
+    case PACKET_UNKNOWN:
+      /* The stub does not support the packet.  */
+      if (config->detect == AUTO_BOOLEAN_AUTO
+	  && config->support == PACKET_ENABLE)
+	{
+	  /* If the stub previously indicated that the packet was
+	     supported then there is a protocol error.  */
+	  error (_("Protocol error: %s (%s) conflicting enabled responses."),
+		 config->name, config->title);
+	}
+      else if (config->detect == AUTO_BOOLEAN_TRUE)
+	{
+	  /* The user set it wrong.  */
+	  error (_("Enabled packet %s (%s) not recognized by stub"),
+		 config->name, config->title);
+	}
+
+      if (remote_debug)
+	fprintf_unfiltered (gdb_stdlog,
+			    "Packet %s (%s) is NOT supported\n",
+			    config->name, config->title);
+      config->support = PACKET_DISABLE;
+      break;
+    }
+
+  return result;
+}
+
 /* Returns the packet's corresponding "set remote foo-packet" command
    state.  See struct packet_config for more details.  */
 

[patchv2 2/2] Workaround gdbserver<7.7 for setfs

[Jan Kratochvil]

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

There was a bug in patchv1.

[-- Attachment #2: move2.patch --]
[-- Type: text/plain, Size: 780 bytes --]

gdb/ChangeLog
2016-03-24  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* remote.c (packet_ok): Add workaround for PACKET_vFile_setfs.

diff --git a/gdb/remote.c b/gdb/remote.c
index bb027cf..f80fee8 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -1453,7 +1453,15 @@ packet_ok (const char *buf, struct packet_config *config)
     internal_error (__FILE__, __LINE__,
 		    _("packet_ok: attempt to use a disabled packet"));
 
-  result = packet_check_result (buf);
+  if (config == &remote_protocol_packets[PACKET_vFile_setfs]
+      && strcmp (buf, "OK") == 0)
+    {
+      /* Workaround gdbserver < 7.7 before its fix from 2013-12-11.  */
+      result = PACKET_UNKNOWN;
+    }
+  else
+    result = packet_check_result (buf);
+
   switch (result)
     {
     case PACKET_OK:

Re: [patchv2 2/2] Workaround gdbserver<7.7 for setfs

[Pedro Alves]

On 03/24/2016 10:32 PM, Jan Kratochvil wrote:
> There was a bug in patchv1.
> 
> 
> move2.patch
> 

Please include self-contained a commit/rationale along with patch
posts (and reposts).  You had context in your intro to v1 that was
lost in v2.

> 
> gdb/ChangeLog
> 2016-03-24  Jan Kratochvil  <jan.kratochvil@redhat.com>
> 
> 	* remote.c (packet_ok): Add workaround for PACKET_vFile_setfs.
> 
> diff --git a/gdb/remote.c b/gdb/remote.c
> index bb027cf..f80fee8 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -1453,7 +1453,15 @@ packet_ok (const char *buf, struct packet_config *config)
>      internal_error (__FILE__, __LINE__,
>  		    _("packet_ok: attempt to use a disabled packet"));
>  
> -  result = packet_check_result (buf);
> +  if (config == &remote_protocol_packets[PACKET_vFile_setfs]
> +      && strcmp (buf, "OK") == 0)
> +    {
> +      /* Workaround gdbserver < 7.7 before its fix from 2013-12-11.  */
> +      result = PACKET_UNKNOWN;
> +    }

This comment could use more detail.

E.g., reading this I'm left wondering, did it always respond OK to
unknown vFile packets, or to all unknown packets?  I think it was
actually the latter.

AFAICS from the commit you pointed at in v1, the "OK" was
gdbserver mistaking any unknown packet for a vStopped packet,
with vStopped being the notification ack for the "%Stop" RSP async
notification.  So it could also happen that gdb sends the setfs
packet while gdbserver had a pending notification, and then
gdbserver replies back a stop reply instead of "OK"...

We may need to guarantee an early enough setfs is attempted.
Is that already the case?

If I'm right and gdbserver mishandled _any_ unknown packet,
then I wonder whether you fix this one, but will trip on another
when you get past initial connection and actually do any serious
debugging?

If not, this may be sufficient.  Otherwise, we may need to come up with
a different workaround, maybe based on sending an early probe packet,
like "MustReplyEmpty", to which well behaved stubs reply empty, just because
that's not a known packet to them.  If a stub replies something other than
empty to that one, then maybe we should disable all other
auto-probed packets...  That may force-disable too much functionality though...

So in sum:

- Expand comment a bit / include commit log with rationale in the patch.

- Give assurance that this is sufficient and that we won't trip on the
  same thing with other packets anyway.  Otherwise we may need to think
  of something else.

Thanks,
Pedro Alves

Re: [patchv2 2/2] Workaround gdbserver<7.7 for setfs

[Jan Kratochvil]

On Wed, 30 Mar 2016 16:17:11 +0200, Pedro Alves wrote:
> E.g., reading this I'm left wondering, did it always respond OK to
> unknown vFile packets, or to all unknown packets?  I think it was
> actually the latter.

Yes.


> AFAICS from the commit you pointed at in v1, the "OK" was
> gdbserver mistaking any unknown packet for a vStopped packet,
> with vStopped being the notification ack for the "%Stop" RSP async
> notification.  So it could also happen that gdb sends the setfs
> packet while gdbserver had a pending notification, and then
> gdbserver replies back a stop reply instead of "OK"...

OK, I did not realize this possible regression.


> We may need to guarantee an early enough setfs is attempted.
> Is that already the case?

For linux targets it is because they read:
	/proc/29202/smaps
Although you are right that does not need to be the case for non-linux
targets.  setfs packet seems to be implemented linux-independently.


> If I'm right and gdbserver mishandled _any_ unknown packet,
> then I wonder whether you fix this one, but will trip on another
> when you get past initial connection and actually do any serious
> debugging?

That would mean gdbserver < 7.7 did not work for "any serious debugging".
I have seen the regression only since "setfs" but I admit I did not do "any
serious debugging".


> If not, this may be sufficient.  Otherwise, we may need to come up with
> a different workaround, maybe based on sending an early probe packet,
> like "MustReplyEmpty", to which well behaved stubs reply empty, just because
> that's not a known packet to them.  If a stub replies something other than
> empty to that one, then maybe we should disable all other
> auto-probed packets...  That may force-disable too much functionality though...
> 
> So in sum:

The patch was fixing a common use case with RHEL<=7 targets.  You have
provided out a counterexample that it may hypothetically regress in a racy
case of non-linux FSF gdbserver.  Normally I would find this workaround
applicable only for RHEL GDBs but now that everything needs to be upstream
first the RHEL workaround needs to be implemented in FSF GDB first (where it
does not belong much IMNSHO).  I will therefore try to implement the
"MustReplyEmpty" packet but I have no idea what effect will have your
mentioned "disable all other auto-probed packets".


Thanks,
Jan

[patchv3] Workaround gdbserver<7.7 for setfs

[Jan Kratochvil]

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

On Wed, 30 Mar 2016 16:17:11 +0200, Pedro Alves wrote:
> E.g., reading this I'm left wondering, did it always respond OK to
> unknown vFile packets, or to all unknown packets?  I think it was
> actually the latter.

To all vFile packets.  Other packets produce on gdbserver-7.6:
	Sending packet: $MustReplyEmpty#c4...Ack
	Packet received: E01

Is the patch OK for check-in this way?


Thanks,
Jan

[-- Attachment #2: vfile.patch --]
[-- Type: text/plain, Size: 2227 bytes --]

gdb/gdbserver/ChangeLog
2016-04-04  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* remote.c (unknown_vfile_replies_ok): New variable.
	(packet_config_support): Read it.
	(remote_start_remote): Set it.

diff --git a/gdb/remote.c b/gdb/remote.c
index 5c407b6..8d949f6 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -1496,6 +1496,15 @@ enum {
 
 static struct packet_config remote_protocol_packets[PACKET_MAX];
 
+/* gdbserver < 7.7 (before its fix from 2013-12-11) did reply to any
+   unknown vFile packet with string "OK".  "OK" gets interpreted by GDB
+   as a reply to known packet.  For packet "vFile:setfs:" it is an
+   invalid reply and GDB would return error in
+   remote_hostio_set_filesystem, making remote files access impossible.
+   If this variable is non-zero it means the remote gdbserver is buggy
+   and any not yet detected packets are assumed as unsupported.  */
+static int unknown_vfile_replies_ok;
+
 /* Returns the packet's corresponding "set remote foo-packet" command
    state.  See struct packet_config for more details.  */
 
@@ -1519,6 +1528,9 @@ packet_config_support (struct packet_config *config)
     case AUTO_BOOLEAN_FALSE:
       return PACKET_DISABLE;
     case AUTO_BOOLEAN_AUTO:
+      if (unknown_vfile_replies_ok && config->name != NULL
+	  && startswith (config->name, "vFile:"))
+	return PACKET_DISABLE;
       return config->support;
     default:
       gdb_assert_not_reached (_("bad switch"));
@@ -4023,6 +4035,21 @@ remote_start_remote (int from_tty, struct target_ops *target, int extended_p)
   if (packet_support (PACKET_QAllow) != PACKET_DISABLE)
     remote_set_permissions (target);
 
+  /* See unknown_vfile_replies_ok description.  */
+  {
+    const char vfile_mustreplyempty[] = "vFile:MustReplyEmpty:";
+
+    putpkt (vfile_mustreplyempty);
+    getpkt (&rs->buf, &rs->buf_size, 0);
+    if (strcmp (rs->buf, "OK") == 0)
+      unknown_vfile_replies_ok = 1;
+    else if (strcmp (rs->buf, "") == 0)
+      unknown_vfile_replies_ok = 0;
+    else
+      error (_("Remote replied unexpectedly to '%s': %s"), vfile_mustreplyempty,
+	     rs->buf);
+  }
+
   /* Next, we possibly activate noack mode.
 
      If the QStartNoAckMode packet configuration is set to AUTO,

Re: [patchv3] Workaround gdbserver<7.7 for setfs

[Pedro Alves]

On 04/04/2016 10:14 PM, Jan Kratochvil wrote:
> On Wed, 30 Mar 2016 16:17:11 +0200, Pedro Alves wrote:
>> E.g., reading this I'm left wondering, did it always respond OK to
>> unknown vFile packets, or to all unknown packets?  I think it was
>> actually the latter.
> 
> To all vFile packets.  Other packets produce on gdbserver-7.6:
> 	Sending packet: $MustReplyEmpty#c4...Ack
> 	Packet received: E01

MustReplyEmpty is actually not a useful probe packet, because
that's actually an M (write memory) packet, which is then
misinterpreted and fails the write.  So not an unknown packet.

Try qMustReplyEmpty or something like that instead.

Thanks,
Pedro Alves

Re: [patch] Suggest newer gdbserver if it has no qXfer:exec-file:read

[Pedro Alves]

Closing the loop on this one:

On 03/23/2016 09:15 PM, Jan Kratochvil wrote:
> On Tue, 22 Mar 2016 14:56:46 +0100, Pedro Alves wrote:
>> - Random stubs may not know at all the executable that is running -- the
>>   remote end is often just bare metal raw memory, no concept of elf, etc.
>>   So it's not just a matter of implementing a packet - more tooling might
>>   and I suspect will, be necessary.  OTOH, there are OSs where it's just not
>>   possible, by design, to retrieve the name of the executable a process
>>   is running, like OpenBSD (I find it odd not to allow a ptracer access
>>   to that, but, alas).
> 
> IIUC for such an embedded target without any filesystem qXfer:exec-file:read
> would need to generate a bogus filename which would be then
> recognized/accepted by vFile:open.
> 
> Sending packet: $qXfer:exec-file:read:67:0,fff#f7...Packet received: l/root/redhat/threadit
> Reading /root/redhat/threadit from remote target...
> Sending packet: $vFile:open:2f726f6f742f7265646861742f7468726561646974,0,0#7e...Packet received: F5
> Sending packet: $vFile:pread:5,3fff,0#98...Packet received: F27f8;\177ELF\002\001\001\000
> 
> Just stating that, nothing interesting.

That'd assume that there's a structured elf on the target, while on bare
metal, you don't have that; no sections, no segments, etc.  Nothing other
than unstructured raw memory, much like what the "dump memory" would
give you.

Thanks,
Pedro Alves

Re: [patch] Suggest newer gdbserver if it has no qXfer:exec-file:read

[Pedro Alves]

On 03/23/2016 09:15 PM, Jan Kratochvil wrote:
> On Tue, 22 Mar 2016 14:56:46 +0100, Pedro Alves wrote:

>> - I think the important points are:
>>
>>   - The user did not specify the executable manually.
>>
>>   - The target/server does not support automatic executable
>>     retrieval. 
>>
>> - I see that at least the following choices to correct the situation:
>>
>>   #1 - Upgrade server to some version that supports automatic automatic
>>        executable retrieval.
>>
>>   #2 - Hack on stub/server yourself to add support for automatic 
>>        executable retrieval, if possible on the target.
>>
>>   #3 - Use the "file" command.
>>
>> If you're connecting with a new gdb to an older gdbserver, it usually
>> means that installing a newer gdbserver is more than a couple
>> seconds work, and may not even be possible.  I think #3 will be the
>> path most often taken.
>>
>> So I'd suggest:
>>
>>    warning: No executable has been specified and target does not support
>>    determining executable automatically.  Try using the \"file\" command.
>>
>> Seeing this, users that can hack on a remote stub will probably
>> realize that there's now some way for gdb to automatically retrieve
>> the executable.  We don't need to expose implementation details for those
>> users; they'll be savvy enough to find the necessary info in the RSP
>> manual.  For other users, talking about implementation details may
>> largely be noise.
> 
> I still do not see there any hint that a newer FSF gdbserver would also fix the
> problem.

That's because I don't think it's a good approach.  If we followed
that direction going forward, we'd end up with:

 warning: Remote gdbserver does not support determining executable automatically.
 FSF gdbserver version 7.10 or later would support that.
 warning: Remote gdbserver does not support foo.
 FSF gdbserver version 6.5 or later would support that.
 warning: Remote gdbserver does not support bar.
 FSF gdbserver version 6.8 or later would support that.

Old version numbers shown on purpose -- that's what 7.10
will feel like in a couple years.  I think it's not a good
idea to show version numbers, nor am I convinced mentioning
gdbserver is a good idea either.  There's bare metal targets, and
then there's also other servers like qemu, Valgrind, RR, etc.

Sorry for pushing back, but I think warnings should be centered
on features, not tools and versions.

> Attached patch prints messages as:
> 
> Remote debugging using :1234
> warning: Remote gdbserver does not support determining executable automatically.
> FSF gdbserver version 7.10 or later would support that.
> warning: No executable has been specified and target does not support
> determining executable automatically.  Try using the "file" command.
> warning: Could not load vsyscall page because no executable was specified
> 0x00007ffff7ddcc80 in ?? ()
> (gdb) _
> 



> diff --git a/gdb/exec.c b/gdb/exec.c
> index 90811c0..a10ab9b 100644
> --- a/gdb/exec.c
> +++ b/gdb/exec.c
> @@ -151,7 +151,13 @@ exec_file_locate_attach (int pid, int from_tty)
>    /* Try to determine a filename from the process itself.  */
>    exec_file = target_pid_to_exec_file (pid);
>    if (exec_file == NULL)
> -    return;
> +    {
> +      warning (_("No executable has been specified and target does not "
> +		 "support\n"
> +		 "determining executable automatically.  "
> +		 "Try using the \"file\" command."));
> +      return;
> +    }

This bit is OK.

> diff --git a/gdb/symfile-mem.c b/gdb/symfile-mem.c
> index 8eb5176..79739a6 100644
> --- a/gdb/symfile-mem.c
> +++ b/gdb/symfile-mem.c
> @@ -214,8 +214,7 @@ add_vsyscall_page (struct target_ops *target, int from_tty)
>  	  format should fix this.  */
>  	{
>  	  warning (_("Could not load vsyscall page "
> -		     "because no executable was specified\n"
> -		     "try using the \"file\" command first."));
> +		     "because no executable was specified"));
>  	  return;

This bit is OK.  Please split them out and push them.

Please don't push the other hunks in.  I think we'll need more
discussion on those and I'd rather if we found some other way
to address this, if we must.

Thanks,
Pedro Alves

Re: [patch] Suggest newer gdbserver if it has no qXfer:exec-file:read

[Jan Kratochvil]

On Tue, 05 Apr 2016 18:32:05 +0200, Pedro Alves wrote:
> On 03/23/2016 09:15 PM, Jan Kratochvil wrote:
> > Sending packet: $qXfer:exec-file:read:67:0,fff#f7...Packet received: l/root/redhat/threadit
> > Reading /root/redhat/threadit from remote target...
> > Sending packet: $vFile:open:2f726f6f742f7265646861742f7468726561646974,0,0#7e...Packet received: F5
> > Sending packet: $vFile:pread:5,3fff,0#98...Packet received: F27f8;\177ELF\002\001\001\000
> > 
> > Just stating that, nothing interesting.
> 
> That'd assume that there's a structured elf on the target, while on bare
> metal, you don't have that; no sections, no segments, etc.  Nothing other
> than unstructured raw memory, much like what the "dump memory" would
> give you.

Yes, reading raw memory without structure is not useful as an exec-file (I see
now gdbserver protocol "qXfer:exec-file:read:" maps to the GDB command "file"
and not to the GDB command "exec-file") which serves mostly as a symbol file.
But for example vDSO is a memory readable ELF file.  I remember some other
ROMs which also had structured format, parseable by BFD but not really
containing a filesystem. Amiga ROM contained a list of libraries, each one
with some PLTs etc.


Jan

[patchv4] Workaround gdbserver<7.7 for setfs

[Jan Kratochvil]

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

On Tue, 05 Apr 2016 18:29:51 +0200, Pedro Alves wrote:
> MustReplyEmpty is actually not a useful probe packet, because
> that's actually an M (write memory) packet, which is then
> misinterpreted and fails the write.  So not an unknown packet.

Oops, OK.


> Try qMustReplyEmpty or something like that instead.

With these requirements on this workaround I have followed the gdbserver-7.6
sources and the bug was in function handle_notif_ack which is called only from
handle_v_requests, therefore for any packets /^v/.

OK to check in this one?


Thanks,
Jan

[-- Attachment #2: v.patch --]
[-- Type: text/plain, Size: 2170 bytes --]

gdb/gdbserver/ChangeLog
2016-04-05  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* remote.c (unknown_v_replies_ok): New variable.
	(packet_config_support): Read it.
	(remote_start_remote): Set it.

diff --git a/gdb/remote.c b/gdb/remote.c
index 5c407b6..a88f4cd 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -1496,6 +1496,15 @@ enum {
 
 static struct packet_config remote_protocol_packets[PACKET_MAX];
 
+/* gdbserver < 7.7 (before its fix from 2013-12-11) did reply to any
+   unknown 'v' packet with string "OK".  "OK" gets interpreted by GDB
+   as a reply to known packet.  For packet "vFile:setfs:" it is an
+   invalid reply and GDB would return error in
+   remote_hostio_set_filesystem, making remote files access impossible.
+   If this variable is non-zero it means the remote gdbserver is buggy
+   and any not yet detected packets are assumed as unsupported.  */
+static int unknown_v_replies_ok;
+
 /* Returns the packet's corresponding "set remote foo-packet" command
    state.  See struct packet_config for more details.  */
 
@@ -1519,6 +1528,9 @@ packet_config_support (struct packet_config *config)
     case AUTO_BOOLEAN_FALSE:
       return PACKET_DISABLE;
     case AUTO_BOOLEAN_AUTO:
+      if (unknown_v_replies_ok && config->name != NULL
+	  && config->name[0] == 'v')
+	return PACKET_DISABLE;
       return config->support;
     default:
       gdb_assert_not_reached (_("bad switch"));
@@ -4023,6 +4035,21 @@ remote_start_remote (int from_tty, struct target_ops *target, int extended_p)
   if (packet_support (PACKET_QAllow) != PACKET_DISABLE)
     remote_set_permissions (target);
 
+  /* See unknown_v_replies_ok description.  */
+  {
+    const char v_mustreplyempty[] = "vMustReplyEmpty";
+
+    putpkt (v_mustreplyempty);
+    getpkt (&rs->buf, &rs->buf_size, 0);
+    if (strcmp (rs->buf, "OK") == 0)
+      unknown_v_replies_ok = 1;
+    else if (strcmp (rs->buf, "") == 0)
+      unknown_v_replies_ok = 0;
+    else
+      error (_("Remote replied unexpectedly to '%s': %s"), v_mustreplyempty,
+	     rs->buf);
+  }
+
   /* Next, we possibly activate noack mode.
 
      If the QStartNoAckMode packet configuration is set to AUTO,

Re: [patchv4] Workaround gdbserver<7.7 for setfs

[Pedro Alves]

On 04/06/2016 02:49 PM, Jan Kratochvil wrote:

>> > Try qMustReplyEmpty or something like that instead.
> With these requirements on this workaround I have followed the gdbserver-7.6
> sources and the bug was in function handle_notif_ack which is called only from
> handle_v_requests, therefore for any packets /^v/.

Ah.

> OK to check in this one?

OK with a change.

> diff --git a/gdb/remote.c b/gdb/remote.c
> index 5c407b6..a88f4cd 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -1496,6 +1496,15 @@ enum {
>  
>  static struct packet_config remote_protocol_packets[PACKET_MAX];
>  
> +/* gdbserver < 7.7 (before its fix from 2013-12-11) did reply to any
> +   unknown 'v' packet with string "OK".  "OK" gets interpreted by GDB
> +   as a reply to known packet.  For packet "vFile:setfs:" it is an
> +   invalid reply and GDB would return error in
> +   remote_hostio_set_filesystem, making remote files access impossible.
> +   If this variable is non-zero it means the remote gdbserver is buggy
> +   and any not yet detected packets are assumed as unsupported.  */
> +static int unknown_v_replies_ok;

This comment looks great now, thanks.  It helps the reader understand
exactly what is being worked around, and it'll help us decide what to
do in the future if this ever gets in the way.

Please make this new variable a field of 'struct remote_state' instead
of a global.

OK with that change.

Thanks,
Pedro Alves

[commit] Suggest newer gdbserver if it has no qXfer:exec-file:read

[Jan Kratochvil]

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

On Tue, 05 Apr 2016 18:57:53 +0200, Pedro Alves wrote:
> On 03/23/2016 09:15 PM, Jan Kratochvil wrote:
> > I still do not see there any hint that a newer FSF gdbserver would also fix the
> > problem.
> 
> That's because I don't think it's a good approach.
> 
> If we followed that direction going forward, we'd end up with:
> 
>  warning: Remote gdbserver does not support determining executable automatically.
>  FSF gdbserver version 7.10 or later would support that.
>  warning: Remote gdbserver does not support foo.
>  FSF gdbserver version 6.5 or later would support that.
>  warning: Remote gdbserver does not support bar.
>  FSF gdbserver version 6.8 or later would support that.
> 
> Old version numbers shown on purpose -- that's what 7.10
> will feel like in a couple years.  I think it's not a good
> idea to show version numbers,

In mail
	[patch] Suggest newer gdbserver if it has no qXfer:exec-file:read
	https://sourceware.org/ml/gdb-patches/2016-03/msg00379.html
	Message-ID: <20160319201842.GA16540@host1.jankratochvil.net>
I was suggesting exactly one message (with one version number).

Only to get it through upon your request in mail
	Re: [patch] Suggest newer gdbserver if it has no qXfer:exec-file:read
	https://sourceware.org/ml/gdb-patches/2016-03/msg00487.html
	Message-ID: <20160323211547.GA17400@host1.jankratochvil.net>
there were two messages (but still one version number).

I never proposed any patch mentioning multiple versions which you claim now to
be a disadvantage of the patch of mine - that is exactly a straw man argument
case.


> nor am I convinced mentioning
> gdbserver is a good idea either.  There's bare metal targets, and
> then there's also other servers like qemu, Valgrind, RR, etc.
> 
> Sorry for pushing back, but I think warnings should be centered
> on features, not tools and versions.

That is technically the right approach but (I think) that does not work for
laypeople.  But I also think laypeople do not use (at least not directly) GDB
anyway so trying to make GDB userfriendly is probably a vain attempt
I sometimes try to do.


> This bit is OK.
+
> This bit is OK.  Please split them out and push them.

Checked in [attached].


Jan

[-- Attachment #2: Type: message/rfc822, Size: 4790 bytes --]

From: Jan Kratochvil <jan.kratochvil@redhat.com>
Subject: [PATCH] Print the "file" command suggestion in exec_file_locate_attach
Date: Wed, 6 Apr 2016 15:57:08 +0200

currently:
	$ gdbserver-7.9 :1234 true &
	$ gdb -q -ex 'target remote :1234' # that -q is not relevant here
	Remote debugging using :1234
	warning: Could not load vsyscall page because no executable was specified
	try using the "file" command first.
	0x00007ffff7ddcc80 in ?? ()
	(gdb) b main
	No symbol table is loaded.  Use the "file" command.
	Make breakpoint pending on future shared library load? (y or [n]) _

Provide more suggestive message to use the "file" command.

gdb/ChangeLog
2016-04-06  Jan Kratochvil  <jan.kratochvil@redhat.com>
	    Pedro Alves  <palves@redhat.com>

	* exec.c (exec_file_locate_attach): Print warning for unsupported
	target_pid_to_exec_file.
	* symfile-mem.c (add_vsyscall_page): Remove the "file" command
	message part.
---
 gdb/ChangeLog     |  8 ++++++++
 gdb/exec.c        |  8 +++++++-
 gdb/remote.c      | 27 +++++++++++++++++++++++++++
 gdb/symfile-mem.c |  3 +--
 4 files changed, 43 insertions(+), 3 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index fc9448d..c59249f 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,11 @@
+2016-04-06  Jan Kratochvil  <jan.kratochvil@redhat.com>
+	    Pedro Alves  <palves@redhat.com>
+
+	* exec.c (exec_file_locate_attach): Print warning for unsupported
+	target_pid_to_exec_file.
+	* symfile-mem.c (add_vsyscall_page): Remove the "file" command
+	message part.
+
 2016-04-04  Simon Marchi  <simon.marchi@ericsson.com>
 
 	* cli/cli-decode.c (help_cmd_list): Fix function doc and remove
diff --git a/gdb/exec.c b/gdb/exec.c
index 90811c0..a10ab9b 100644
--- a/gdb/exec.c
+++ b/gdb/exec.c
@@ -151,7 +151,13 @@ exec_file_locate_attach (int pid, int from_tty)
   /* Try to determine a filename from the process itself.  */
   exec_file = target_pid_to_exec_file (pid);
   if (exec_file == NULL)
-    return;
+    {
+      warning (_("No executable has been specified and target does not "
+		 "support\n"
+		 "determining executable automatically.  "
+		 "Try using the \"file\" command."));
+      return;
+    }
 
   /* If gdb_sysroot is not empty and the discovered filename
      is absolute then prefix the filename with gdb_sysroot.  */
diff --git a/gdb/remote.c b/gdb/remote.c
index 5c407b6..a88f4cd 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -1496,6 +1496,15 @@ enum {
 
 static struct packet_config remote_protocol_packets[PACKET_MAX];
 
+/* gdbserver < 7.7 (before its fix from 2013-12-11) did reply to any
+   unknown 'v' packet with string "OK".  "OK" gets interpreted by GDB
+   as a reply to known packet.  For packet "vFile:setfs:" it is an
+   invalid reply and GDB would return error in
+   remote_hostio_set_filesystem, making remote files access impossible.
+   If this variable is non-zero it means the remote gdbserver is buggy
+   and any not yet detected packets are assumed as unsupported.  */
+static int unknown_v_replies_ok;
+
 /* Returns the packet's corresponding "set remote foo-packet" command
    state.  See struct packet_config for more details.  */
 
@@ -1519,6 +1528,9 @@ packet_config_support (struct packet_config *config)
     case AUTO_BOOLEAN_FALSE:
       return PACKET_DISABLE;
     case AUTO_BOOLEAN_AUTO:
+      if (unknown_v_replies_ok && config->name != NULL
+	  && config->name[0] == 'v')
+	return PACKET_DISABLE;
       return config->support;
     default:
       gdb_assert_not_reached (_("bad switch"));
@@ -4023,6 +4035,21 @@ remote_start_remote (int from_tty, struct target_ops *target, int extended_p)
   if (packet_support (PACKET_QAllow) != PACKET_DISABLE)
     remote_set_permissions (target);
 
+  /* See unknown_v_replies_ok description.  */
+  {
+    const char v_mustreplyempty[] = "vMustReplyEmpty";
+
+    putpkt (v_mustreplyempty);
+    getpkt (&rs->buf, &rs->buf_size, 0);
+    if (strcmp (rs->buf, "OK") == 0)
+      unknown_v_replies_ok = 1;
+    else if (strcmp (rs->buf, "") == 0)
+      unknown_v_replies_ok = 0;
+    else
+      error (_("Remote replied unexpectedly to '%s': %s"), v_mustreplyempty,
+	     rs->buf);
+  }
+
   /* Next, we possibly activate noack mode.
 
      If the QStartNoAckMode packet configuration is set to AUTO,
diff --git a/gdb/symfile-mem.c b/gdb/symfile-mem.c
index 8eb5176..79739a6 100644
--- a/gdb/symfile-mem.c
+++ b/gdb/symfile-mem.c
@@ -214,8 +214,7 @@ add_vsyscall_page (struct target_ops *target, int from_tty)
 	  format should fix this.  */
 	{
 	  warning (_("Could not load vsyscall page "
-		     "because no executable was specified\n"
-		     "try using the \"file\" command first."));
+		     "because no executable was specified"));
 	  return;
 	}
       args.bfd = bfd;
-- 
2.5.5

[commit fix] Revert check-in by a mistake in the previous commit [Re: [commit] Suggest newer gdbserver if it has no qXfer:exec-file:read]

[Jan Kratochvil]

[-- Attachment #0: Type: message/rfc822, Size: 2848 bytes --]

From: Jan Kratochvil <jan.kratochvil@redhat.com>
Subject: [PATCH] Revert check-in by a mistake in the previous commit.
Date: Wed, 6 Apr 2016 16:48:27 +0200

gdb/ChangeLog
2016-04-06  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* remote.c: Revert check-in by a mistake in the previous commit.
---
 gdb/ChangeLog |  4 ++++
 gdb/remote.c  | 27 ---------------------------
 2 files changed, 4 insertions(+), 27 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index c59249f..e362360 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,4 +1,8 @@
 2016-04-06  Jan Kratochvil  <jan.kratochvil@redhat.com>
+
+	* remote.c: Revert check-in by a mistake in the previous commit.
+
+2016-04-06  Jan Kratochvil  <jan.kratochvil@redhat.com>
 	    Pedro Alves  <palves@redhat.com>
 
 	* exec.c (exec_file_locate_attach): Print warning for unsupported
diff --git a/gdb/remote.c b/gdb/remote.c
index a88f4cd..5c407b6 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -1496,15 +1496,6 @@ enum {
 
 static struct packet_config remote_protocol_packets[PACKET_MAX];
 
-/* gdbserver < 7.7 (before its fix from 2013-12-11) did reply to any
-   unknown 'v' packet with string "OK".  "OK" gets interpreted by GDB
-   as a reply to known packet.  For packet "vFile:setfs:" it is an
-   invalid reply and GDB would return error in
-   remote_hostio_set_filesystem, making remote files access impossible.
-   If this variable is non-zero it means the remote gdbserver is buggy
-   and any not yet detected packets are assumed as unsupported.  */
-static int unknown_v_replies_ok;
-
 /* Returns the packet's corresponding "set remote foo-packet" command
    state.  See struct packet_config for more details.  */
 
@@ -1528,9 +1519,6 @@ packet_config_support (struct packet_config *config)
     case AUTO_BOOLEAN_FALSE:
       return PACKET_DISABLE;
     case AUTO_BOOLEAN_AUTO:
-      if (unknown_v_replies_ok && config->name != NULL
-	  && config->name[0] == 'v')
-	return PACKET_DISABLE;
       return config->support;
     default:
       gdb_assert_not_reached (_("bad switch"));
@@ -4035,21 +4023,6 @@ remote_start_remote (int from_tty, struct target_ops *target, int extended_p)
   if (packet_support (PACKET_QAllow) != PACKET_DISABLE)
     remote_set_permissions (target);
 
-  /* See unknown_v_replies_ok description.  */
-  {
-    const char v_mustreplyempty[] = "vMustReplyEmpty";
-
-    putpkt (v_mustreplyempty);
-    getpkt (&rs->buf, &rs->buf_size, 0);
-    if (strcmp (rs->buf, "OK") == 0)
-      unknown_v_replies_ok = 1;
-    else if (strcmp (rs->buf, "") == 0)
-      unknown_v_replies_ok = 0;
-    else
-      error (_("Remote replied unexpectedly to '%s': %s"), v_mustreplyempty,
-	     rs->buf);
-  }
-
   /* Next, we possibly activate noack mode.
 
      If the QStartNoAckMode packet configuration is set to AUTO,
-- 
2.5.5

Re: [commit] Suggest newer gdbserver if it has no qXfer:exec-file:read

[Pedro Alves]

On 04/06/2016 03:34 PM, Jan Kratochvil wrote:
> On Tue, 05 Apr 2016 18:57:53 +0200, Pedro Alves wrote:
>> On 03/23/2016 09:15 PM, Jan Kratochvil wrote:
>>> I still do not see there any hint that a newer FSF gdbserver would also fix the
>>> problem.
>>
>> That's because I don't think it's a good approach.
>>
>> If we followed that direction going forward, we'd end up with:
>>
>>  warning: Remote gdbserver does not support determining executable automatically.
>>  FSF gdbserver version 7.10 or later would support that.
>>  warning: Remote gdbserver does not support foo.
>>  FSF gdbserver version 6.5 or later would support that.
>>  warning: Remote gdbserver does not support bar.
>>  FSF gdbserver version 6.8 or later would support that.
>>
>> Old version numbers shown on purpose -- that's what 7.10
>> will feel like in a couple years.  I think it's not a good
>> idea to show version numbers,
> 

> 
> I never proposed any patch mentioning multiple versions which you claim now to
> be a disadvantage of the patch of mine - that is exactly a straw man argument
> case.

No, it is not.  

I never said _you_ proposed a patch mentioning multiple versions.
I said "If we followed that direction going forward".  Thinking of how a patch
impacts future direction is not a straw man, and must be part of the development
and review process.

The patch mentioning a gdbserver version opens a precedent.  It'd be reasonable
then for other cases to get treated the same way once that direction is set,
and multiple mentions of versions would be what we'd get against some stubs.
Thus, coupled with not all stubs being gdbserver, I think that patch would
set up for the wrong direction, hence the push back.

>> nor am I convinced mentioning
>> gdbserver is a good idea either.  There's bare metal targets, and
>> then there's also other servers like qemu, Valgrind, RR, etc.
>>
>> Sorry for pushing back, but I think warnings should be centered
>> on features, not tools and versions.
> 
> That is technically the right approach but (I think) that does not work for
> laypeople.  But I also think laypeople do not use (at least not directly) GDB
> anyway so trying to make GDB userfriendly is probably a vain attempt
> I sometimes try to do.

Making GDB more user friendly is definitely a good goal, and
much appreciated.

Thanks,
Pedro Alves

[commit] [patchv4] Workaround gdbserver<7.7 for setfs

[Jan Kratochvil]

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

On Wed, 06 Apr 2016 16:31:07 +0200, Pedro Alves wrote:
> Please make this new variable a field of 'struct remote_state' instead
> of a global.

I did not know there is the global function get_remote_state().

Checked in.


Thanks,
Jan

[-- Attachment #2: Type: message/rfc822, Size: 4751 bytes --]

From: Jan Kratochvil <jan.kratochvil@redhat.com>
Subject: [PATCH] Workaround gdbserver<7.7 for setfs
Date: Wed, 6 Apr 2016 17:13:12 +0200

With current FSF GDB HEAD and old FSF gdbserver I expected I could do:
	gdb -ex 'file target:/root/redhat/threadit' -ex 'target remote :1234'
(supplying that unsupported qXfer:exec-file:read by "file")
But that does not work because:
	Sending packet: $vFile:setfs:0#bf...Packet received: OK
	Packet vFile:setfs (hostio-setfs) is supported
	...
	Sending packet: $vFile:setfs:104#24...Packet received: OK
	"target:/root/redhat/threadit": could not open as an executable file: Invalid argument

GDB documentation says:
	The valid responses to Host I/O packets are:
	An empty response indicates that this operation is not recognized.

This "empty response" vs. "OK" was a bug in gdbserver < 7.7.  It was fixed by:
	commit e7f0d979dd5cc4f8b658df892e93db69d6d660b7
	Author: Yao Qi <yao@codesourcery.com>
	Date:   Tue Dec 10 21:59:20 2013 +0800
	    Fix a bug in matching notifications.
	Message-ID: <1386684626-11415-1-git-send-email-yao@codesourcery.com>
	https://sourceware.org/ml/gdb-patches/2013-12/msg00373.html
	2013-12-10  Yao Qi  <yao@codesourcery.com>
		* notif.c (handle_notif_ack): Return 0 if no notification
		matches.

with unpatched old FSF gdbserver and patched FSF GDB HEAD:
	gdb -ex 'file target:/root/redhat/threadit' -ex 'target remote :1234'
	Sending packet: $vFile:setfs:0#bf...Packet received: OK
	Packet vFile:setfs (hostio-setfs) is NOT supported
	...
	(gdb) info sharedlibrary
	From                To                  Syms Read   Shared Object Library
	0x00007ffff7ddbae0  0x00007ffff7df627a  Yes (*)     target:/lib64/ld-linux-x86-64.so.2
	0x00007ffff7bc48a0  0x00007ffff7bcf514  Yes (*)     target:/lib64/libpthread.so.0

gdb/ChangeLog
2016-04-06  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* remote.c (struct remote_state): New field unknown_v_replies_ok.
	(packet_config_support): Read it.
	(remote_start_remote): Set it.
---
 gdb/ChangeLog |  6 ++++++
 gdb/remote.c  | 31 +++++++++++++++++++++++++++++++
 2 files changed, 37 insertions(+)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index e362360..e3848ec 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,11 @@
 2016-04-06  Jan Kratochvil  <jan.kratochvil@redhat.com>
 
+	* remote.c (struct remote_state): New field unknown_v_replies_ok.
+	(packet_config_support): Read it.
+	(remote_start_remote): Set it.
+
+2016-04-06  Jan Kratochvil  <jan.kratochvil@redhat.com>
+
 	* remote.c: Revert check-in by a mistake in the previous commit.
 
 2016-04-06  Jan Kratochvil  <jan.kratochvil@redhat.com>
diff --git a/gdb/remote.c b/gdb/remote.c
index 5c407b6..ea7f5b8 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -426,6 +426,15 @@ struct remote_state
      request/reply nature of the RSP.  We only cache data for a single
      file descriptor at a time.  */
   struct readahead_cache readahead_cache;
+
+  /* gdbserver < 7.7 (before its fix from 2013-12-11) did reply to any
+     unknown 'v' packet with string "OK".  "OK" gets interpreted by GDB
+     as a reply to known packet.  For packet "vFile:setfs:" it is an
+     invalid reply and GDB would return error in
+     remote_hostio_set_filesystem, making remote files access impossible.
+     If this variable is non-zero it means the remote gdbserver is buggy
+     and any not yet detected packets are assumed as unsupported.  */
+  int unknown_v_replies_ok;
 };
 
 /* Private data that we'll store in (struct thread_info)->private.  */
@@ -1519,6 +1528,13 @@ packet_config_support (struct packet_config *config)
     case AUTO_BOOLEAN_FALSE:
       return PACKET_DISABLE;
     case AUTO_BOOLEAN_AUTO:
+      {
+	struct remote_state *rs = get_remote_state ();
+
+	if (rs->unknown_v_replies_ok && config->name != NULL
+	    && config->name[0] == 'v')
+	  return PACKET_DISABLE;
+      }
       return config->support;
     default:
       gdb_assert_not_reached (_("bad switch"));
@@ -4023,6 +4039,21 @@ remote_start_remote (int from_tty, struct target_ops *target, int extended_p)
   if (packet_support (PACKET_QAllow) != PACKET_DISABLE)
     remote_set_permissions (target);
 
+  /* See unknown_v_replies_ok description.  */
+  {
+    const char v_mustreplyempty[] = "vMustReplyEmpty";
+
+    putpkt (v_mustreplyempty);
+    getpkt (&rs->buf, &rs->buf_size, 0);
+    if (strcmp (rs->buf, "OK") == 0)
+      rs->unknown_v_replies_ok = 1;
+    else if (strcmp (rs->buf, "") == 0)
+      rs->unknown_v_replies_ok = 0;
+    else
+      error (_("Remote replied unexpectedly to '%s': %s"), v_mustreplyempty,
+	     rs->buf);
+  }
+
   /* Next, we possibly activate noack mode.
 
      If the QStartNoAckMode packet configuration is set to AUTO,
-- 
2.5.5

Re: [commit] Suggest newer gdbserver if it has no qXfer:exec-file:read

[Jan Kratochvil]

On Wed, 06 Apr 2016 17:04:22 +0200, Pedro Alves wrote:
> The patch mentioning a gdbserver version opens a precedent.  It'd be reasonable
> then for other cases to get treated the same way once that direction is set,
> and multiple mentions of versions would be what we'd get against some stubs.

In all cases I would also find OK to just print "Latest stable FSF gdbserver
version supports that.".  Just to print what the user should do, not only what
the problem is.

Besides that GDB also already prints 14 lines of useless text upon its start
and ~200 lines of useless "Reading symbols ..." text during attachment
- in a comparison with few more lines indicating a real problem.


> Thus, coupled with not all stubs being gdbserver, I think that patch would
> set up for the wrong direction, hence the push back.

That is all mathematically correct but (I think) not helpful for a casual user.


Jan

[revert] [patchv4] Workaround gdbserver<7.7 for setfs

[Jan Kratochvil]

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

Hi,

it no longer transferred any files with old gdbserver after this commit.
(I do not have setup a regression testsuite for old gdbserver.)

Therefore reverting it - as it may be breaking currently working
functionalities with old gdbservers - and the workaround may get reimplemented
some other way later.


Jan

[-- Attachment #2: Type: message/rfc822, Size: 3157 bytes --]

From: Jan Kratochvil <jan.kratochvil@redhat.com>
Subject: [PATCH] Revert the previous commit adding unknown_v_replies_ok.
Date: Wed, 6 Apr 2016 21:05:16 +0200

It broke the compatibility with gdbserver-7.6 due to:
	warning: remote target does not support file transfer, attempting to access files from local filesystem.

gdb/ChangeLog
2016-04-06  Jan Kratochvil  <jan.kratochvil@redhat.com>

	Revert the previous commit adding unknown_v_replies_ok.
---
 gdb/ChangeLog |  4 ++++
 gdb/remote.c  | 31 -------------------------------
 2 files changed, 4 insertions(+), 31 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index e3848ec..0af0c79 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,9 @@
 2016-04-06  Jan Kratochvil  <jan.kratochvil@redhat.com>
 
+	Revert the previous commit adding unknown_v_replies_ok.
+
+2016-04-06  Jan Kratochvil  <jan.kratochvil@redhat.com>
+
 	* remote.c (struct remote_state): New field unknown_v_replies_ok.
 	(packet_config_support): Read it.
 	(remote_start_remote): Set it.
diff --git a/gdb/remote.c b/gdb/remote.c
index ea7f5b8..5c407b6 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -426,15 +426,6 @@ struct remote_state
      request/reply nature of the RSP.  We only cache data for a single
      file descriptor at a time.  */
   struct readahead_cache readahead_cache;
-
-  /* gdbserver < 7.7 (before its fix from 2013-12-11) did reply to any
-     unknown 'v' packet with string "OK".  "OK" gets interpreted by GDB
-     as a reply to known packet.  For packet "vFile:setfs:" it is an
-     invalid reply and GDB would return error in
-     remote_hostio_set_filesystem, making remote files access impossible.
-     If this variable is non-zero it means the remote gdbserver is buggy
-     and any not yet detected packets are assumed as unsupported.  */
-  int unknown_v_replies_ok;
 };
 
 /* Private data that we'll store in (struct thread_info)->private.  */
@@ -1528,13 +1519,6 @@ packet_config_support (struct packet_config *config)
     case AUTO_BOOLEAN_FALSE:
       return PACKET_DISABLE;
     case AUTO_BOOLEAN_AUTO:
-      {
-	struct remote_state *rs = get_remote_state ();
-
-	if (rs->unknown_v_replies_ok && config->name != NULL
-	    && config->name[0] == 'v')
-	  return PACKET_DISABLE;
-      }
       return config->support;
     default:
       gdb_assert_not_reached (_("bad switch"));
@@ -4039,21 +4023,6 @@ remote_start_remote (int from_tty, struct target_ops *target, int extended_p)
   if (packet_support (PACKET_QAllow) != PACKET_DISABLE)
     remote_set_permissions (target);
 
-  /* See unknown_v_replies_ok description.  */
-  {
-    const char v_mustreplyempty[] = "vMustReplyEmpty";
-
-    putpkt (v_mustreplyempty);
-    getpkt (&rs->buf, &rs->buf_size, 0);
-    if (strcmp (rs->buf, "OK") == 0)
-      rs->unknown_v_replies_ok = 1;
-    else if (strcmp (rs->buf, "") == 0)
-      rs->unknown_v_replies_ok = 0;
-    else
-      error (_("Remote replied unexpectedly to '%s': %s"), v_mustreplyempty,
-	     rs->buf);
-  }
-
   /* Next, we possibly activate noack mode.
 
      If the QStartNoAckMode packet configuration is set to AUTO,
-- 
2.5.5

[patchv5] Workaround gdbserver<7.7 for setfs

[Jan Kratochvil]

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

Hi Pedro,

On Wed, 06 Apr 2016 21:09:05 +0200, Jan Kratochvil wrote:
> it no longer transferred any files with old gdbserver after this commit.
> (I do not have setup a regression testsuite for old gdbserver.)
> 
> Therefore reverting it

posting a new implementation based on Pedro's off-list comment:
	hmm.  guess we could still salvage it, by 1) still doing the detection
	early; 2) accept OK as "unsupported" in setfs only?

OK for check-in?


Thanks,
Jan

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

With current FSF GDB HEAD and old FSF gdbserver I expected I could do:
	gdb -ex 'file target:/root/redhat/threadit' -ex 'target remote :1234'
(supplying that unsupported qXfer:exec-file:read by "file")
But that does not work because:
	Sending packet: $vFile:setfs:0#bf...Packet received: OK
	Packet vFile:setfs (hostio-setfs) is supported
	...
	Sending packet: $vFile:setfs:104#24...Packet received: OK
	"target:/root/redhat/threadit": could not open as an executable file: Invalid argument

GDB documentation says:
	The valid responses to Host I/O packets are:
	An empty response indicates that this operation is not recognized.

This "empty response" vs. "OK" was a bug in gdbserver < 7.7.  It was fixed by:
	commit e7f0d979dd5cc4f8b658df892e93db69d6d660b7
	Author: Yao Qi <yao@codesourcery.com>
	Date:   Tue Dec 10 21:59:20 2013 +0800
	    Fix a bug in matching notifications.
	Message-ID: <1386684626-11415-1-git-send-email-yao@codesourcery.com>
	https://sourceware.org/ml/gdb-patches/2013-12/msg00373.html
	2013-12-10  Yao Qi  <yao@codesourcery.com>
		* notif.c (handle_notif_ack): Return 0 if no notification
		matches.

with unpatched old FSF gdbserver and patched FSF GDB HEAD:
	gdb -ex 'file target:/root/redhat/threadit' -ex 'target remote :1234'
	Sending packet: $vFile:setfs:0#bf...Packet received: OK
	Packet vFile:setfs (hostio-setfs) is NOT supported
	...
	(gdb) info sharedlibrary
	From                To                  Syms Read   Shared Object Library
	0x00007ffff7ddbae0  0x00007ffff7df627a  Yes (*)     target:/lib64/ld-linux-x86-64.so.2
	0x00007ffff7bc48a0  0x00007ffff7bcf514  Yes (*)     target:/lib64/libpthread.so.0

gdb/ChangeLog
2016-04-26  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* remote.c (remote_start_remote): Detect PACKET_vFile_setfs.support.

diff --git a/gdb/remote.c b/gdb/remote.c
index e5bf15d..a9b7201 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -4031,6 +4031,25 @@ remote_start_remote (int from_tty, struct target_ops *target, int extended_p)
   if (packet_support (PACKET_QAllow) != PACKET_DISABLE)
     remote_set_permissions (target);
 
+  /* gdbserver < 7.7 (before its fix from 2013-12-11) did reply to any
+     unknown 'v' packet with string "OK".  "OK" gets interpreted by GDB
+     as a reply to known packet.  For packet "vFile:setfs:" it is an
+     invalid reply and GDB would return error in
+     remote_hostio_set_filesystem, making remote files access impossible.
+     Disable "vFile:setfs:" in such case.  Do not disable other 'v' packets as
+     other "vFile" packets get correctly detected even on gdbserver < 7.7.  */
+  {
+    const char v_mustreplyempty[] = "vMustReplyEmpty";
+
+    putpkt (v_mustreplyempty);
+    getpkt (&rs->buf, &rs->buf_size, 0);
+    if (strcmp (rs->buf, "OK") == 0)
+      remote_protocol_packets[PACKET_vFile_setfs].support = PACKET_DISABLE;
+    else if (strcmp (rs->buf, "") != 0)
+      error (_("Remote replied unexpectedly to '%s': %s"), v_mustreplyempty,
+	     rs->buf);
+  }
+
   /* Next, we possibly activate noack mode.
 
      If the QStartNoAckMode packet configuration is set to AUTO,

Re: [patchv5] Workaround gdbserver<7.7 for setfs

[Pedro Alves]

On 04/26/2016 10:29 PM, Jan Kratochvil wrote:
> Hi Pedro,
> 
> On Wed, 06 Apr 2016 21:09:05 +0200, Jan Kratochvil wrote:
>> it no longer transferred any files with old gdbserver after this commit.
>> (I do not have setup a regression testsuite for old gdbserver.)
>>
>> Therefore reverting it
> 
> posting a new implementation based on Pedro's off-list comment:
> 	hmm.  guess we could still salvage it, by 1) still doing the detection
> 	early; 2) accept OK as "unsupported" in setfs only?
> 
> OK for check-in?

OK.

Thanks,
Pedro Alves

[commit+7.11] [patchv5] Workaround gdbserver<7.7 for setfs

[Jan Kratochvil]

On Wed, 27 Apr 2016 11:58:57 +0200, Pedro Alves wrote:
> On 04/26/2016 10:29 PM, Jan Kratochvil wrote:
> > posting a new implementation based on Pedro's off-list comment:
> > 	hmm.  guess we could still salvage it, by 1) still doing the detection
> > 	early; 2) accept OK as "unsupported" in setfs only?
> > 
> > OK for check-in?
> 
> OK.

Commit:
	57809e5e5a506664eb54433ded81ab0785168a83
and 7.11:
	a6ff23076f49c6322d96a76e0098f8019139bc4e


Thanks,
Jan

Re: [commit+7.11] [patchv5] Workaround gdbserver<7.7 for setfs

[Gary Benson]

Jan Kratochvil wrote:
> On Wed, 27 Apr 2016 11:58:57 +0200, Pedro Alves wrote:
> > On 04/26/2016 10:29 PM, Jan Kratochvil wrote:
> > > posting a new implementation based on Pedro's off-list comment:
> > > 	hmm.  guess we could still salvage it, by 1) still doing the detection
> > > 	early; 2) accept OK as "unsupported" in setfs only?
> > > 
> > > OK for check-in?
> > 
> > OK.
> 
> Commit:
> 	57809e5e5a506664eb54433ded81ab0785168a83
> and 7.11:
> 	a6ff23076f49c6322d96a76e0098f8019139bc4e

Thanks Jan.

Cheers,
Gary

-- 
http://gbenson.net/