[PATCH,gdbserver] Put 'multiprocess+' in to qSupported reply if GDB supports multiprocess

[PATCH,gdbserver] Put 'multiprocess+' in to qSupported reply if GDB supports multiprocess

[Yao Qi]

GDBserver put 'multiprocess+' in to qSupported reply regardless of
whether GDB supports multiprocess.  It doesn't cause any problems
because if GDB doesn't support multiprocess, GDB doesn't understand
'multiprocess+'.  However, I feel it is better not to send
'multiprocess+' to GDB if GDB doesn't support multiprocess.

In this patch, GDBserver only sends 'multiprocess+' back to GDB if
both GDB and the target of GDBserver supports multiprocess.  Beside
this change, this patch also add a new struct 'gdb_supports' to
represent various features GDB supports.  (it paves the way for my
patches on querying supported notifications.)

Regression tested with board file native-gdbserver,
native-extended-gdbserver and unix on x86_64-linux.  Is it OK to
apply?

gdb/gdbserver:

2013-01-15  Yao Qi  <yao@codesourcery.com>

	* server.c (handle_query): New 'struct gdb_supports'.
	Update.  Set 'gdb_supports.multi_process' if GDB supports
	multiprocess.
	Add 'multiprocess+' to the reply if both GDBserver target
	and GDB support multiprocess.
---
 gdb/gdbserver/server.c |   15 +++++++++++----
 1 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c
index 4af9436..b149a1b 100644
--- a/gdb/gdbserver/server.c
+++ b/gdb/gdbserver/server.c
@@ -1527,7 +1527,12 @@ handle_query (char *own_buf, int packet_len, int *new_packet_len_p)
       && (own_buf[10] == ':' || own_buf[10] == '\0'))
     {
       char *p = &own_buf[10];
-      int gdb_supports_qRelocInsn = 0;
+      /* Features that GDB support.  */
+      struct gdb_supports
+      {
+	int qRelocInsn;
+	int multi_process;
+      } gdb_supports = { 0, 0 };
 
       /* Start processing qSupported packet.  */
       target_process_qsupported (NULL);
@@ -1559,13 +1564,14 @@ handle_query (char *own_buf, int packet_len, int *new_packet_len_p)
 		{
 		  /* GDB supports and wants multi-process support if
 		     possible.  */
+		  gdb_supports.multi_process = 1;
 		  if (target_supports_multi_process ())
 		    multi_process = 1;
 		}
 	      else if (strcmp (p, "qRelocInsn+") == 0)
 		{
 		  /* GDB supports relocate instruction requests.  */
-		  gdb_supports_qRelocInsn = 1;
+		  gdb_supports.qRelocInsn = 1;
 		}
 	      else
 		target_process_qsupported (p);
@@ -1613,7 +1619,8 @@ handle_query (char *own_buf, int packet_len, int *new_packet_len_p)
       if (the_target->qxfer_osdata != NULL)
 	strcat (own_buf, ";qXfer:osdata:read+");
 
-      if (target_supports_multi_process ())
+      if (gdb_supports.multi_process
+	  && target_supports_multi_process ())
 	strcat (own_buf, ";multiprocess+");
 
       if (target_supports_non_stop ())
@@ -1630,7 +1637,7 @@ handle_query (char *own_buf, int packet_len, int *new_packet_len_p)
 	  strcat (own_buf, ";TraceStateVariables+");
 	  strcat (own_buf, ";TracepointSource+");
 	  strcat (own_buf, ";DisconnectedTracing+");
-	  if (gdb_supports_qRelocInsn && target_supports_fast_tracepoints ())
+	  if (gdb_supports.qRelocInsn && target_supports_fast_tracepoints ())
 	    strcat (own_buf, ";FastTracepoints+");
 	  strcat (own_buf, ";StaticTracepoints+");
 	  strcat (own_buf, ";InstallInTrace+");
-- 
1.7.7.6

Re: [PATCH,gdbserver] Put 'multiprocess+' in to qSupported reply if GDB supports multiprocess

[Pedro Alves]

On 01/15/2013 09:01 AM, Yao Qi wrote:
> GDBserver put 'multiprocess+' in to qSupported reply regardless of
> whether GDB supports multiprocess.  It doesn't cause any problems
> because if GDB doesn't support multiprocess, GDB doesn't understand
> 'multiprocess+'.  However, I feel it is better not to send
> 'multiprocess+' to GDB if GDB doesn't support multiprocess.

I disagree, and it's a dangerous path to follow.   It may prove useful
to know what exactly does a target support even if your gdb doesn't
support it for instance, as a debugging aid.  Or GDB itself may know
of a feature, but choose to not enable it (and therefore not broadcast
support in its qSupported), but still infer something about the
target from the target's reported features.  So it's more prudent to
make the qSupported reported features as stateless as possible.

-- 
Pedro Alves

Re: [PATCH,gdbserver] Put 'multiprocess+' in to qSupported reply if GDB supports multiprocess

[Yao Qi]

On 01/16/2013 03:55 AM, Pedro Alves wrote:
> I disagree, and it's a dangerous path to follow.   It may prove useful
> to know what exactly does a target support even if your gdb doesn't
> support it for instance, as a debugging aid.  Or GDB itself may know
> of a feature, but choose to not enable it (and therefore not broadcast
> support in its qSupported), but still infer something about the
> target from the target's reported features.  So it's more prudent to

I can't figure out why GDB wants/has to know all features that GDBserver 
supports, even some of features are not supported by GDB.  I don't find 
out a case that GDB has to lie to GDBserver, that GDB doesn't know about 
feature FOO, but is still interested in the feature FOO internally.

> make the qSupported reported features as stateless as possible.

Once we start to add features into qSupported packet (sent from GDB to 
GDBserver), the qSupoorted reply became stateful.  If the qSupported 
reply is exactly about what the remote target supports, GDB doesn't to 
tell GDBserver what features GDB supports by means of qSupported.

b.t.w, GDBserver only puts ";FastTracepoints+" into qSupported reply if 
both GDB sends "qRelocInsn+" and the target supports fast tracepoints.

This issue jumps into my eyes when I think about the query of supported 
notifications on both sides.  Both GDB and GDBserver knows different 
notifications and each of them should know what notifications supported 
in the other side.  The protocol design is as follows:

Supposing GDB knows notification N1, N2, and N3, while GDBserver knows 
notification N1, N2, and N4.

   --> qSupported:notifications=N1,N2,N3;
   <-- XXX;Notificaitons=N1,N2;

as a result, GDBserver knows N3 is not supported by GDB, so doesn't send 
it.  In the RSP interaction, I thought GDBserver doesn't have to tell 
GDB that GDBserver supports N4.  What do you think?

-- 
Yao (齐尧)

Re: [PATCH,gdbserver] Put 'multiprocess+' in to qSupported reply if GDB supports multiprocess

[Pedro Alves]

On 01/16/2013 08:30 AM, Yao Qi wrote:
> On 01/16/2013 03:55 AM, Pedro Alves wrote:
>> I disagree, and it's a dangerous path to follow.   It may prove
>> useful to know what exactly does a target support even if your gdb
>> doesn't support it for instance, as a debugging aid.  Or GDB itself
>> may know of a feature, but choose to not enable it (and therefore
>> not broadcast support in its qSupported), but still infer something
>> about the target from the target's reported features.  So it's more
>> prudent to
> 
> I can't figure out why GDB wants/has to know all features that
> GDBserver supports, even some of features are not supported by GDB.

I didn't say GDB, but for you/me, GDB hackers, as a debugging aid.

> I don't find out a case that GDB has to lie to GDBserver, that GDB
> doesn't know about feature FOO, but is still interested in the
> feature FOO internally.

In the past when discussing some changes, and backward (in)compatibility
implications, things like "Y was never listed as qSupported
feature, but if we know GDB supports X, then we know it supports Y too,
because Y postdates it" have been put in the table.  It's a hack,
and certainly not bullet proof, and so far we managed to avoid needing
it, but it's been a last resort option (can't recall specific cases now).
That's why I said it's more _prudent_ to leave the option open.  IOW,
not paint ourselves into a corner if there's no real benefit.

>> make the qSupported reported features as stateless as possible.
> 
> Once we start to add features into qSupported packet (sent from GDB
> to GDBserver), the qSupoorted reply became stateful.  If the
> qSupported reply is exactly about what the remote target supports,
> GDB doesn't to tell GDBserver what features GDB supports by means of
> qSupported.

Some features the remote target supports depend on _other_ GDB features.

> b.t.w, GDBserver only puts ";FastTracepoints+" into qSupported reply
> if both GDB sends "qRelocInsn+" and the target supports fast
> tracepoints.

Right, that's an example.  GDBserver's implementation of fast tracepoints
on x86/x86-64 only works if GDB can help GDBserver relocate instructions.
Note the "as possible" remark.

> 
> This issue jumps into my eyes when I think about the query of
> supported notifications on both sides.  Both GDB and GDBserver knows
> different notifications and each of them should know what
> notifications supported in the other side.  The protocol design is as
> follows:
> 
> Supposing GDB knows notification N1, N2, and N3, while GDBserver
> knows notification N1, N2, and N4.
> 
> --> qSupported:notifications=N1,N2,N3; <-- XXX;Notificaitons=N1,N2;
> 
> as a result, GDBserver knows N3 is not supported by GDB, so doesn't
> send it.  In the RSP interaction, I thought GDBserver doesn't have to
> tell GDB that GDBserver supports N4.  What do you think?

I think it'll save a few characters on the wire, when an old GDB
connects to a more recent GDBserver.  We won't get the saving
with a GDB of the same vintage as GDBserver.  So what else does
doing that gain?

Because this:

  --> qSupported:N1+,N2+,N3+; <-- XXX;N1+,N2+,N4+;

can have exactly the same result.  GDBserver knows N3 is not
supported by GDB, so doesn't send it.  Or could it have other,
possibly ambiguous semantics?

But anyway, I'll agree that my reasoning may not be seen
as a strong one.  I'm just calling for caution/prudence.

-- 
Pedro Alves