* [patch] Suggest newer gdbserver if it has no qXfer:exec-file:read @ 2016-03-19 20:18 Jan Kratochvil 2016-03-22 9:15 ` Gary Benson 2016-03-22 12:24 ` Pedro Alves 0 siblings, 2 replies; 29+ messages in thread From: Jan Kratochvil @ 2016-03-19 20:18 UTC (permalink / raw) To: gdb-patches; +Cc: Gary Benson [-- 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); ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [patch] Suggest newer gdbserver if it has no qXfer:exec-file:read 2016-03-19 20:18 [patch] Suggest newer gdbserver if it has no qXfer:exec-file:read Jan Kratochvil @ 2016-03-22 9:15 ` Gary Benson 2016-03-22 12:24 ` Pedro Alves 1 sibling, 0 replies; 29+ messages in thread From: Gary Benson @ 2016-03-22 9:15 UTC (permalink / raw) To: Jan Kratochvil; +Cc: gdb-patches 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/ ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [patch] Suggest newer gdbserver if it has no qXfer:exec-file:read 2016-03-19 20:18 [patch] Suggest newer gdbserver if it has no qXfer:exec-file:read Jan Kratochvil 2016-03-22 9:15 ` Gary Benson @ 2016-03-22 12:24 ` Pedro Alves 2016-03-22 13:16 ` Jan Kratochvil 1 sibling, 1 reply; 29+ messages in thread From: Pedro Alves @ 2016-03-22 12:24 UTC (permalink / raw) To: Jan Kratochvil, gdb-patches; +Cc: Gary Benson 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 ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [patch] Suggest newer gdbserver if it has no qXfer:exec-file:read 2016-03-22 12:24 ` Pedro Alves @ 2016-03-22 13:16 ` Jan Kratochvil 2016-03-22 13:56 ` Pedro Alves 0 siblings, 1 reply; 29+ messages in thread From: Jan Kratochvil @ 2016-03-22 13:16 UTC (permalink / raw) To: Pedro Alves; +Cc: gdb-patches, Gary Benson 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 ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [patch] Suggest newer gdbserver if it has no qXfer:exec-file:read 2016-03-22 13:16 ` Jan Kratochvil @ 2016-03-22 13:56 ` Pedro Alves 2016-03-23 21:15 ` Jan Kratochvil 0 siblings, 1 reply; 29+ messages in thread From: Pedro Alves @ 2016-03-22 13:56 UTC (permalink / raw) To: Jan Kratochvil; +Cc: gdb-patches, Gary Benson 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 ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [patch] Suggest newer gdbserver if it has no qXfer:exec-file:read 2016-03-22 13:56 ` Pedro Alves @ 2016-03-23 21:15 ` Jan Kratochvil 2016-03-24 16:59 ` Jan Kratochvil ` (3 more replies) 0 siblings, 4 replies; 29+ messages in thread From: Jan Kratochvil @ 2016-03-23 21:15 UTC (permalink / raw) To: Pedro Alves; +Cc: gdb-patches, Gary Benson [-- 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; ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [patch] Suggest newer gdbserver if it has no qXfer:exec-file:read 2016-03-23 21:15 ` Jan Kratochvil @ 2016-03-24 16:59 ` Jan Kratochvil 2016-03-24 22:09 ` [patch] Workaround gdbserver<7.7 for setfs [Re: [patch] Suggest newer gdbserver if it has no qXfer:exec-file:read] Jan Kratochvil ` (2 subsequent siblings) 3 siblings, 0 replies; 29+ messages in thread From: Jan Kratochvil @ 2016-03-24 16:59 UTC (permalink / raw) To: Pedro Alves; +Cc: gdb-patches, Gary Benson 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 ^ permalink raw reply [flat|nested] 29+ messages in thread
* [patch] Workaround gdbserver<7.7 for setfs [Re: [patch] Suggest newer gdbserver if it has no qXfer:exec-file:read] 2016-03-23 21:15 ` Jan Kratochvil 2016-03-24 16:59 ` Jan Kratochvil @ 2016-03-24 22:09 ` Jan Kratochvil 2016-03-24 22:32 ` [patchv2 2/2] Workaround gdbserver<7.7 for setfs Jan Kratochvil 2016-03-24 22:32 ` [patchv2 1/2] " Jan Kratochvil 2016-04-05 16:32 ` [patch] Suggest newer gdbserver if it has no qXfer:exec-file:read Pedro Alves 2016-04-05 16:58 ` Pedro Alves 3 siblings, 2 replies; 29+ messages in thread From: Jan Kratochvil @ 2016-03-24 22:09 UTC (permalink / raw) To: Pedro Alves; +Cc: gdb-patches, Gary Benson [-- 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: ^ permalink raw reply [flat|nested] 29+ messages in thread
* [patchv2 2/2] Workaround gdbserver<7.7 for setfs 2016-03-24 22:09 ` [patch] Workaround gdbserver<7.7 for setfs [Re: [patch] Suggest newer gdbserver if it has no qXfer:exec-file:read] Jan Kratochvil @ 2016-03-24 22:32 ` Jan Kratochvil 2016-03-30 14:17 ` Pedro Alves 2016-03-24 22:32 ` [patchv2 1/2] " Jan Kratochvil 1 sibling, 1 reply; 29+ messages in thread From: Jan Kratochvil @ 2016-03-24 22:32 UTC (permalink / raw) To: Pedro Alves; +Cc: gdb-patches, Gary Benson [-- 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: ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [patchv2 2/2] Workaround gdbserver<7.7 for setfs 2016-03-24 22:32 ` [patchv2 2/2] Workaround gdbserver<7.7 for setfs Jan Kratochvil @ 2016-03-30 14:17 ` Pedro Alves 2016-04-03 19:30 ` Jan Kratochvil 2016-04-04 21:14 ` [patchv3] " Jan Kratochvil 0 siblings, 2 replies; 29+ messages in thread From: Pedro Alves @ 2016-03-30 14:17 UTC (permalink / raw) To: Jan Kratochvil; +Cc: gdb-patches, Gary Benson 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 ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [patchv2 2/2] Workaround gdbserver<7.7 for setfs 2016-03-30 14:17 ` Pedro Alves @ 2016-04-03 19:30 ` Jan Kratochvil 2016-04-04 21:14 ` [patchv3] " Jan Kratochvil 1 sibling, 0 replies; 29+ messages in thread From: Jan Kratochvil @ 2016-04-03 19:30 UTC (permalink / raw) To: Pedro Alves; +Cc: gdb-patches, Gary Benson 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 ^ permalink raw reply [flat|nested] 29+ messages in thread
* [patchv3] Workaround gdbserver<7.7 for setfs 2016-03-30 14:17 ` Pedro Alves 2016-04-03 19:30 ` Jan Kratochvil @ 2016-04-04 21:14 ` Jan Kratochvil 2016-04-05 16:29 ` Pedro Alves 1 sibling, 1 reply; 29+ messages in thread From: Jan Kratochvil @ 2016-04-04 21:14 UTC (permalink / raw) To: Pedro Alves; +Cc: gdb-patches, Gary Benson [-- 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, ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [patchv3] Workaround gdbserver<7.7 for setfs 2016-04-04 21:14 ` [patchv3] " Jan Kratochvil @ 2016-04-05 16:29 ` Pedro Alves 2016-04-06 13:49 ` [patchv4] " Jan Kratochvil 0 siblings, 1 reply; 29+ messages in thread From: Pedro Alves @ 2016-04-05 16:29 UTC (permalink / raw) To: Jan Kratochvil; +Cc: gdb-patches, Gary Benson 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 ^ permalink raw reply [flat|nested] 29+ messages in thread
* [patchv4] Workaround gdbserver<7.7 for setfs 2016-04-05 16:29 ` Pedro Alves @ 2016-04-06 13:49 ` Jan Kratochvil 2016-04-06 14:31 ` Pedro Alves 0 siblings, 1 reply; 29+ messages in thread From: Jan Kratochvil @ 2016-04-06 13:49 UTC (permalink / raw) To: Pedro Alves; +Cc: gdb-patches, Gary Benson [-- 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, ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [patchv4] Workaround gdbserver<7.7 for setfs 2016-04-06 13:49 ` [patchv4] " Jan Kratochvil @ 2016-04-06 14:31 ` Pedro Alves 2016-04-06 15:19 ` [commit] " Jan Kratochvil 0 siblings, 1 reply; 29+ messages in thread From: Pedro Alves @ 2016-04-06 14:31 UTC (permalink / raw) To: Jan Kratochvil; +Cc: gdb-patches, Gary Benson 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 ^ permalink raw reply [flat|nested] 29+ messages in thread
* [commit] [patchv4] Workaround gdbserver<7.7 for setfs 2016-04-06 14:31 ` Pedro Alves @ 2016-04-06 15:19 ` Jan Kratochvil 2016-04-06 19:09 ` [revert] " Jan Kratochvil 0 siblings, 1 reply; 29+ messages in thread From: Jan Kratochvil @ 2016-04-06 15:19 UTC (permalink / raw) To: Pedro Alves; +Cc: gdb-patches, Gary Benson [-- 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 ^ permalink raw reply [flat|nested] 29+ messages in thread
* [revert] [patchv4] Workaround gdbserver<7.7 for setfs 2016-04-06 15:19 ` [commit] " Jan Kratochvil @ 2016-04-06 19:09 ` Jan Kratochvil 2016-04-26 21:29 ` [patchv5] " Jan Kratochvil 0 siblings, 1 reply; 29+ messages in thread From: Jan Kratochvil @ 2016-04-06 19:09 UTC (permalink / raw) To: Pedro Alves; +Cc: gdb-patches, Gary Benson [-- 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 ^ permalink raw reply [flat|nested] 29+ messages in thread
* [patchv5] Workaround gdbserver<7.7 for setfs 2016-04-06 19:09 ` [revert] " Jan Kratochvil @ 2016-04-26 21:29 ` Jan Kratochvil 2016-04-27 9:59 ` Pedro Alves 0 siblings, 1 reply; 29+ messages in thread From: Jan Kratochvil @ 2016-04-26 21:29 UTC (permalink / raw) To: Pedro Alves; +Cc: gdb-patches, Gary Benson [-- 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, ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [patchv5] Workaround gdbserver<7.7 for setfs 2016-04-26 21:29 ` [patchv5] " Jan Kratochvil @ 2016-04-27 9:59 ` Pedro Alves 2016-04-27 19:32 ` [commit+7.11] " Jan Kratochvil 0 siblings, 1 reply; 29+ messages in thread From: Pedro Alves @ 2016-04-27 9:59 UTC (permalink / raw) To: Jan Kratochvil; +Cc: gdb-patches, Gary Benson 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 ^ permalink raw reply [flat|nested] 29+ messages in thread
* [commit+7.11] [patchv5] Workaround gdbserver<7.7 for setfs 2016-04-27 9:59 ` Pedro Alves @ 2016-04-27 19:32 ` Jan Kratochvil 2016-04-28 10:36 ` Gary Benson 0 siblings, 1 reply; 29+ messages in thread From: Jan Kratochvil @ 2016-04-27 19:32 UTC (permalink / raw) To: Pedro Alves; +Cc: gdb-patches, Gary Benson 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 ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [commit+7.11] [patchv5] Workaround gdbserver<7.7 for setfs 2016-04-27 19:32 ` [commit+7.11] " Jan Kratochvil @ 2016-04-28 10:36 ` Gary Benson 0 siblings, 0 replies; 29+ messages in thread From: Gary Benson @ 2016-04-28 10:36 UTC (permalink / raw) To: Jan Kratochvil; +Cc: Pedro Alves, gdb-patches 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/ ^ permalink raw reply [flat|nested] 29+ messages in thread
* [patchv2 1/2] Workaround gdbserver<7.7 for setfs 2016-03-24 22:09 ` [patch] Workaround gdbserver<7.7 for setfs [Re: [patch] Suggest newer gdbserver if it has no qXfer:exec-file:read] Jan Kratochvil 2016-03-24 22:32 ` [patchv2 2/2] Workaround gdbserver<7.7 for setfs Jan Kratochvil @ 2016-03-24 22:32 ` Jan Kratochvil 1 sibling, 0 replies; 29+ messages in thread From: Jan Kratochvil @ 2016-03-24 22:32 UTC (permalink / raw) To: Pedro Alves; +Cc: gdb-patches, Gary Benson [-- 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. */ ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [patch] Suggest newer gdbserver if it has no qXfer:exec-file:read 2016-03-23 21:15 ` Jan Kratochvil 2016-03-24 16:59 ` Jan Kratochvil 2016-03-24 22:09 ` [patch] Workaround gdbserver<7.7 for setfs [Re: [patch] Suggest newer gdbserver if it has no qXfer:exec-file:read] Jan Kratochvil @ 2016-04-05 16:32 ` Pedro Alves 2016-04-05 17:14 ` Jan Kratochvil 2016-04-05 16:58 ` Pedro Alves 3 siblings, 1 reply; 29+ messages in thread From: Pedro Alves @ 2016-04-05 16:32 UTC (permalink / raw) To: Jan Kratochvil; +Cc: gdb-patches, Gary Benson 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 ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [patch] Suggest newer gdbserver if it has no qXfer:exec-file:read 2016-04-05 16:32 ` [patch] Suggest newer gdbserver if it has no qXfer:exec-file:read Pedro Alves @ 2016-04-05 17:14 ` Jan Kratochvil 0 siblings, 0 replies; 29+ messages in thread From: Jan Kratochvil @ 2016-04-05 17:14 UTC (permalink / raw) To: Pedro Alves; +Cc: gdb-patches, Gary Benson 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 ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [patch] Suggest newer gdbserver if it has no qXfer:exec-file:read 2016-03-23 21:15 ` Jan Kratochvil ` (2 preceding siblings ...) 2016-04-05 16:32 ` [patch] Suggest newer gdbserver if it has no qXfer:exec-file:read Pedro Alves @ 2016-04-05 16:58 ` Pedro Alves 2016-04-06 14:34 ` [commit] " Jan Kratochvil 3 siblings, 1 reply; 29+ messages in thread From: Pedro Alves @ 2016-04-05 16:58 UTC (permalink / raw) To: Jan Kratochvil; +Cc: gdb-patches, Gary Benson 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 ^ permalink raw reply [flat|nested] 29+ messages in thread
* [commit] Suggest newer gdbserver if it has no qXfer:exec-file:read 2016-04-05 16:58 ` Pedro Alves @ 2016-04-06 14:34 ` Jan Kratochvil 2016-04-06 14:49 ` [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 2016-04-06 15:04 ` [commit] Suggest newer gdbserver if it has no qXfer:exec-file:read Pedro Alves 0 siblings, 2 replies; 29+ messages in thread From: Jan Kratochvil @ 2016-04-06 14:34 UTC (permalink / raw) To: Pedro Alves; +Cc: gdb-patches, Gary Benson [-- 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 ^ permalink raw reply [flat|nested] 29+ messages in thread
* [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] 2016-04-06 14:34 ` [commit] " Jan Kratochvil @ 2016-04-06 14:49 ` Jan Kratochvil 2016-04-06 15:04 ` [commit] Suggest newer gdbserver if it has no qXfer:exec-file:read Pedro Alves 1 sibling, 0 replies; 29+ messages in thread From: Jan Kratochvil @ 2016-04-06 14:49 UTC (permalink / raw) To: Pedro Alves; +Cc: gdb-patches, Gary Benson [-- 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 ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [commit] Suggest newer gdbserver if it has no qXfer:exec-file:read 2016-04-06 14:34 ` [commit] " Jan Kratochvil 2016-04-06 14:49 ` [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 @ 2016-04-06 15:04 ` Pedro Alves 2016-04-06 15:29 ` Jan Kratochvil 1 sibling, 1 reply; 29+ messages in thread From: Pedro Alves @ 2016-04-06 15:04 UTC (permalink / raw) To: Jan Kratochvil; +Cc: gdb-patches, Gary Benson 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 ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [commit] Suggest newer gdbserver if it has no qXfer:exec-file:read 2016-04-06 15:04 ` [commit] Suggest newer gdbserver if it has no qXfer:exec-file:read Pedro Alves @ 2016-04-06 15:29 ` Jan Kratochvil 0 siblings, 0 replies; 29+ messages in thread From: Jan Kratochvil @ 2016-04-06 15:29 UTC (permalink / raw) To: Pedro Alves; +Cc: gdb-patches, Gary Benson 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 ^ permalink raw reply [flat|nested] 29+ messages in thread
end of thread, other threads:[~2016-04-28 10:36 UTC | newest] Thread overview: 29+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-03-19 20:18 [patch] Suggest newer gdbserver if it has no qXfer:exec-file:read Jan Kratochvil 2016-03-22 9:15 ` Gary Benson 2016-03-22 12:24 ` Pedro Alves 2016-03-22 13:16 ` Jan Kratochvil 2016-03-22 13:56 ` Pedro Alves 2016-03-23 21:15 ` Jan Kratochvil 2016-03-24 16:59 ` Jan Kratochvil 2016-03-24 22:09 ` [patch] Workaround gdbserver<7.7 for setfs [Re: [patch] Suggest newer gdbserver if it has no qXfer:exec-file:read] Jan Kratochvil 2016-03-24 22:32 ` [patchv2 2/2] Workaround gdbserver<7.7 for setfs Jan Kratochvil 2016-03-30 14:17 ` Pedro Alves 2016-04-03 19:30 ` Jan Kratochvil 2016-04-04 21:14 ` [patchv3] " Jan Kratochvil 2016-04-05 16:29 ` Pedro Alves 2016-04-06 13:49 ` [patchv4] " Jan Kratochvil 2016-04-06 14:31 ` Pedro Alves 2016-04-06 15:19 ` [commit] " Jan Kratochvil 2016-04-06 19:09 ` [revert] " Jan Kratochvil 2016-04-26 21:29 ` [patchv5] " Jan Kratochvil 2016-04-27 9:59 ` Pedro Alves 2016-04-27 19:32 ` [commit+7.11] " Jan Kratochvil 2016-04-28 10:36 ` Gary Benson 2016-03-24 22:32 ` [patchv2 1/2] " Jan Kratochvil 2016-04-05 16:32 ` [patch] Suggest newer gdbserver if it has no qXfer:exec-file:read Pedro Alves 2016-04-05 17:14 ` Jan Kratochvil 2016-04-05 16:58 ` Pedro Alves 2016-04-06 14:34 ` [commit] " Jan Kratochvil 2016-04-06 14:49 ` [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 2016-04-06 15:04 ` [commit] Suggest newer gdbserver if it has no qXfer:exec-file:read Pedro Alves 2016-04-06 15:29 ` Jan Kratochvil
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).