[rfc] Fix qC handling in gdbserver

[rfc] Fix qC handling in gdbserver

[Markus Deuling]

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

Hi,

I had some regressions when running the testsuite on x86 via gdbserver 
and began to look at it. Some FAILs occur because the testcases cannot 
be run via gdbserver (eg. follow fork because we dont have that in 
gdbserver). I'll come up with a patch for the testcases.

When looking at gdb.base/info-proc.exp I found a bug in remote target.

GDB tries to get the initial pid after "target remote :<port>". It does
that by set_thread (-1, 0), which results in a "Hc-1" packet. On 
gdbserver side each Hc packet with '0' or '1' results in E01 packet as 
reply.

Later on remote_current_thread (inferior_ptid); is called which always 
comes back with MAGIC_NULL_PID. The reason for that is the missing 
support for 'qC' packets in gdbserver. Manual says the reply for qC
is 'QC pid'. If no pid returns MAGIC_NULL_PID is taken.

Because of that 'info proc' is broken directly after connecting to
gdbserver (it tries to access /proc/42000), thats why the testcase failes.

This patch adds handling for qC packets in gdbserver and removes the
unnecessary set_thread() call.

Is this ok to commit?

ChangeLog:

	* gdbserver/server.c (handle_query): Add handling for qC
	  packets.
	* remote.c (remote_start_remote): Remove useless set_thread.




-- 
Markus Deuling
GNU Toolchain for Linux on Cell BE
deuling@de.ibm.com


[-- Attachment #2: diff_gdbserver_fix --]
[-- Type: text/plain, Size: 1127 bytes --]

diff -urN src/gdb/gdbserver/server.c dev/gdb/gdbserver/server.c
--- src/gdb/gdbserver/server.c	2007-03-29 05:37:17.000000000 +0200
+++ dev/gdb/gdbserver/server.c	2007-04-27 09:36:47.000000000 +0200
@@ -259,6 +259,14 @@
 {
   static struct inferior_list_entry *thread_ptr;
 
+  /* Reply the current thread id.  */
+  if (own_buf[0] == 'q' && own_buf[1] == 'C')
+    {
+      sprintf (own_buf, "QC %lx", 
+        ((struct inferior_list_entry *) current_inferior)->id);
+      return;
+    }
+
   if (strcmp ("qSymbol::", own_buf) == 0)
     {
       if (the_target->look_up_symbols != NULL)
diff -urN src/gdb/remote.c dev/gdb/remote.c
--- src/gdb/remote.c	2007-03-28 05:42:54.000000000 +0200
+++ dev/gdb/remote.c	2007-04-27 09:40:33.000000000 +0200
@@ -2096,9 +2100,7 @@
   /* Ack any packet which the remote side has already sent.  */
   serial_write (remote_desc, "+", 1);
 
-  /* Let the stub know that we want it to return the thread.  */
-  set_thread (-1, 0);
-
+  /* Get the pid of the first thread.  */
   inferior_ptid = remote_current_thread (inferior_ptid);
 
   get_offsets ();		/* Get text, data & bss offsets.  */

Re: [rfc] Fix qC handling in gdbserver

[Pedro Alves]

On 4/27/07, Markus Deuling wrote:
> +  /* Reply the current thread id.  */
> +  if (own_buf[0] == 'q' && own_buf[1] == 'C')
> +    {

All the other tests in the function seem to check for an
extra '\0', ',' or ':' after the query name.  Shouldn't you do the
same here?  Otherwise you are answering
to all future queries starting with qC (Think or a gdbserver
installed in rom in a board in the field, and connecting
to it with gdb-cvs2020 :) )

something like:

> +  if (strcmp (own_buf, "qC") == 0)

or

> +  if (own_buf[0] == 'q' && own_buf[1] == 'C'&& own_buf[2] == '\0')

?

Cheers,
Pedro Alves

Re: [rfc] Fix qC handling in gdbserver

[Markus Deuling]

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

Hi Pedro,

Pedro Alves wrote:
> All the other tests in the function seem to check for an
> extra '\0', ',' or ':' after the query name.  Shouldn't you do the
> same here?  Otherwise you are answering
> to all future queries starting with qC (Think or a gdbserver
> installed in rom in a board in the field, and connecting
> to it with gdb-cvs2020 :) )

yes, your're right. I changed the patch to use strcmp now. I just
wanted to save the overhead of a function call for two characters. But I 
would have missed qC<whatever> :-)

Thanks for review!


-- 
Markus Deuling
GNU Toolchain for Linux on Cell BE
deuling@de.ibm.com


[-- Attachment #2: diff_gdbserver_fix --]
[-- Type: text/plain, Size: 1116 bytes --]

diff -urN src/gdb/gdbserver/server.c dev/gdb/gdbserver/server.c
--- src/gdb/gdbserver/server.c	2007-03-29 05:37:17.000000000 +0200
+++ dev/gdb/gdbserver/server.c	2007-04-27 12:31:45.000000000 +0200
@@ -259,6 +259,14 @@
 {
   static struct inferior_list_entry *thread_ptr;
 
+  /* Reply the current thread id.  */
+  if (strcmp ("qC", own_buf) == 0)
+    {
+      sprintf (own_buf, "QC %lx", 
+        ((struct inferior_list_entry *) current_inferior)->id);
+      return;
+    }
+
   if (strcmp ("qSymbol::", own_buf) == 0)
     {
       if (the_target->look_up_symbols != NULL)
diff -urN src/gdb/remote.c dev/gdb/remote.c
--- src/gdb/remote.c	2007-03-28 05:42:54.000000000 +0200
+++ dev/gdb/remote.c	2007-04-27 12:29:21.000000000 +0200
@@ -2096,9 +2096,7 @@
   /* Ack any packet which the remote side has already sent.  */
   serial_write (remote_desc, "+", 1);
 
-  /* Let the stub know that we want it to return the thread.  */
-  set_thread (-1, 0);
-
+  /* Get the pid of the first thread.  */
   inferior_ptid = remote_current_thread (inferior_ptid);
 
   get_offsets ();		/* Get text, data & bss offsets.  */

Re: [rfc] Fix qC handling in gdbserver

[Daniel Jacobowitz]

On Fri, Apr 27, 2007 at 10:01:12AM +0200, Markus Deuling wrote:
> When looking at gdb.base/info-proc.exp I found a bug in remote target.
> 
> GDB tries to get the initial pid after "target remote :<port>". It does
> that by set_thread (-1, 0), which results in a "Hc-1" packet. On gdbserver side 
> each Hc packet with '0' or '1' results in E01 packet as reply.

Consider the comment you're deleting.  There are remote stubs out
there that work with versions of GDB older than thread support - I
don't know if any of them are still in use, but I have no way to
check, either.  They will report thread information if they think GDB
supports it, but not otherwise.  I think the remote.c change is wrong.

> Later on remote_current_thread (inferior_ptid); is called which always comes 
> back with MAGIC_NULL_PID. The reason for that is the missing support for 'qC' 
> packets in gdbserver. Manual says the reply for qC
> is 'QC pid'. If no pid returns MAGIC_NULL_PID is taken.

You should have read the paragraph just above :-)

   Like the descriptions of the other packets, each description here
has a template showing the packet's overall syntax, followed by an
explanation of the packet's meaning.  We include spaces in some of the
templates for clarity; these are not part of the packet's syntax.  No
GDB packet uses spaces to separate its components.

So the reply doesn't have a space in it.  But there's a bigger problem
here.  Look at this:

> Because of that 'info proc' is broken directly after connecting to
> gdbserver (it tries to access /proc/42000), thats why the testcase failes.

GDB is opening /proc/pid on the _local_ machine.  That's silly.
gdbserver is usually running on a remote machine; it's only on the
local machine during native testing.  The testcase fails because info
proc only works for native targets.

We could fix this, since I was just discussing with Pedro some gdb
protocol extensions for opening remote files...

-- 
Daniel Jacobowitz
CodeSourcery

Re: [rfc] Fix qC handling in gdbserver

[Markus Deuling]

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

Hi,

Daniel Jacobowitz wrote:
> 
> GDB is opening /proc/pid on the _local_ machine.  That's silly.
> gdbserver is usually running on a remote machine; it's only on the
> local machine during native testing.  The testcase fails because info
> proc only works for native targets.

thank you again for your input. The patch attached disables gdb.base/info-proc.exp
for remote targets.

Is this ok?

ChangeLog:
	* gdb.base/info-proc.exp: Check is_remote.


Btw,
shall I rework the qC-Patch () and remove the space? Like

+  /* Reply the current thread id.  */
+  if (strcmp ("qC", own_buf) == 0)
+    {
+      sprintf (own_buf, "QC%lx", 
+        ((struct inferior_list_entry *) current_inferior)->id);
+      return;
+    }
? Would that be ok for mainline? I wonder why the manual contains qC packets
while gdb does not? Is this a new feature or one that should be removed from manual rather?
Thanks in advance.

-- 
Markus Deuling
GNU Toolchain for Linux on Cell BE
deuling@de.ibm.com


[-- Attachment #2: diff-remote-info-proc --]
[-- Type: text/plain, Size: 381 bytes --]

diff -urN src/gdb/testsuite/gdb.base/info-proc.exp dev/gdb/testsuite/gdb.base/info-proc.exp
--- src/gdb/testsuite/gdb.base/info-proc.exp	2007-01-09 18:59:11.000000000 +0100
+++ dev/gdb/testsuite/gdb.base/info-proc.exp	2007-05-02 18:04:33.000000000 +0200
@@ -24,6 +24,10 @@
 	strace $tracelevel
 }
 
+if { [is_remote target] } then {
+  continue
+}
+
 set prms_id 0
 set bug_id 0
 

Re: [rfc] Fix qC handling in gdbserver

[Daniel Jacobowitz]

On Wed, May 02, 2007 at 06:16:28PM +0200, Markus Deuling wrote:
> Hi,
> 
> Daniel Jacobowitz wrote:
> > GDB is opening /proc/pid on the _local_ machine.  That's silly.
> > gdbserver is usually running on a remote machine; it's only on the
> > local machine during native testing.  The testcase fails because info
> > proc only works for native targets.
> 
> thank you again for your input. The patch attached disables 
> gdb.base/info-proc.exp
> for remote targets.
> 
> Is this ok?
> 
> ChangeLog:
> 	* gdb.base/info-proc.exp: Check is_remote.

Yes, this is OK.  It's a little silly since the user can still type
"info proc" and will get garbage, but we don't need to fix that now.

> Btw,
> shall I rework the qC-Patch () and remove the space? Like

Yes please!

> ? Would that be ok for mainline? I wonder why the manual contains qC packets
> while gdb does not? Is this a new feature or one that should be removed from 
> manual rather?

GDB contains support for it; gdbserver doesn't use it, but other
remote stubs do.  For instance, I think RedBoot does.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [rfc] Fix qC handling in gdbserver

[Ulrich Weigand]

Markus Deuling wrote:

> 	* gdb.base/info-proc.exp: Check is_remote.

I've committed this now.

Thanks,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com