[PATCH] gdb/remote: Don't use vKill if multi-process features are disabled

[PATCH] gdb/remote: Don't use vKill if multi-process features are disabled

[Andrew Burgess]

The below was tested using native gdbserver on x86-64 Fedora Linux
with no regressions.

---

The gdb remote protocol documentation is clear that the vKill command
should not be used unless the multi-process feature is reported as
supported by the remote target.

Currently within gdb we check to see if the vKill packet is enabled or
not before using the vKill command, however, the only way to disable
vKill is from the gdb console, the result is that vKill will be sent to
targets that don't support it, and never claimed to support it.

After this commit I guard use of vKill with a check to see if the
multi-process feature is enabled or not.  I have removed the ability to
disable vkill specifically from the console, the user must now disable
the whole multi-process feature set as one.

I did consider leaving the separate vKill control switch in addition to
the multi-process control switch, but this seemed unnecessary, and I
worried that in the future another bug could be introduced where
PACKET_vKill was used to guard sending a vKill.

Removed:

  set remote kill-packet (auto|on|off)
  show remote kill-packet

Instead Use:

  set remote multiprocess-feature-packet (auto|on|off)
  show set remote multiprocess-feature-packet

gdb/ChangeLog:

	* remote.c (PACKET enum): Remove PACKET_vKill.
	(remote_kill): Use remote_multi_process_p.
	(remote_vKill): Use PACKET_multiprocess_feature.
	(_initialize_remote): Remove vKill configuration.
	* NEWS: Mention removed commands.
---
 gdb/ChangeLog |  8 ++++++++
 gdb/NEWS      |  9 +++++++++
 gdb/remote.c  | 10 +++-------
 3 files changed, 20 insertions(+), 7 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 2df6ccd..db2768c 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,11 @@
+2016-03-17  Andrew Burgess  <andrew.burgess@embecosm.com>
+
+	* remote.c (PACKET enum): Remove PACKET_vKill.
+	(remote_kill): Use remote_multi_process_p.
+	(remote_vKill): Use PACKET_multiprocess_feature.
+	(_initialize_remote): Remove vKill configuration.
+	* NEWS: Mention removed commands.
+
 2016-03-17  Jan Kratochvil  <jan.kratochvil@redhat.com>
 
 	* linux-thread-db.c (check_pid_namespace_match): Extend the message.
diff --git a/gdb/NEWS b/gdb/NEWS
index 34c5a8d..be10da6 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -24,6 +24,15 @@
    Bounds: [lower = 0x7fffffffc390, upper = 0x7fffffffc3a3]
    0x0000000000400d7c in upper () at i386-mpx-sigsegv.c:68
 
+* The following commands which disabled use of the remote vKill packet
+  has been removed:
+      set remote kill-packet (auto|on|off)
+      show remote kill-packet
+  Instead, vKill is part of a larged feature set which can be enabled
+  or disabled as one using:
+      set remote multiprocess-feature-packet (auto|on|off)
+      show set remote multiprocess-feature-packet
+
 * New commands
 
 skip -file file
diff --git a/gdb/remote.c b/gdb/remote.c
index af0a08a..87c1078 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -1398,7 +1398,6 @@ enum {
   PACKET_vAttach,
   PACKET_vRun,
   PACKET_QStartNoAckMode,
-  PACKET_vKill,
   PACKET_qXfer_siginfo_read,
   PACKET_qXfer_siginfo_write,
   PACKET_qAttached,
@@ -8907,7 +8906,7 @@ remote_kill (struct target_ops *ops)
   int pid = ptid_get_pid (inferior_ptid);
   struct remote_state *rs = get_remote_state ();
 
-  if (packet_support (PACKET_vKill) != PACKET_DISABLE)
+  if (remote_multi_process_p (rs))
     {
       /* If we're stopped while forking and we haven't followed yet,
 	 kill the child task.  We need to do this before killing the
@@ -8948,7 +8947,7 @@ remote_kill (struct target_ops *ops)
 static int
 remote_vkill (int pid, struct remote_state *rs)
 {
-  if (packet_support (PACKET_vKill) == PACKET_DISABLE)
+  if (!remote_multi_process_p (rs))
     return -1;
 
   /* Tell the remote target to detach.  */
@@ -8957,7 +8956,7 @@ remote_vkill (int pid, struct remote_state *rs)
   getpkt (&rs->buf, &rs->buf_size, 0);
 
   switch (packet_ok (rs->buf,
-		     &remote_protocol_packets[PACKET_vKill]))
+		     &remote_protocol_packets[PACKET_multiprocess_feature]))
     {
     case PACKET_OK:
       return 0;
@@ -13756,9 +13755,6 @@ Show the maximum size of the address (in bits) in a memory packet."), NULL,
   add_packet_config_cmd (&remote_protocol_packets[PACKET_QStartNoAckMode],
 			 "QStartNoAckMode", "noack", 0);
 
-  add_packet_config_cmd (&remote_protocol_packets[PACKET_vKill],
-			 "vKill", "kill", 0);
-
   add_packet_config_cmd (&remote_protocol_packets[PACKET_qAttached],
 			 "qAttached", "query-attached", 0);
 
-- 
2.5.1

Re: [PATCH] gdb/remote: Don't use vKill if multi-process features are disabled

[Pedro Alves]

On 03/17/2016 10:02 PM, Andrew Burgess wrote:
> The below was tested using native gdbserver on x86-64 Fedora Linux
> with no regressions.
> 
> ---
> 
> The gdb remote protocol documentation is clear that the vKill command
> should not be used unless the multi-process feature is reported as
> supported by the remote target.
> 
> Currently within gdb we check to see if the vKill packet is enabled or
> not before using the vKill command, however, the only way to disable
> vKill is from the gdb console, the result is that vKill will be sent to
> targets that don't support it, and never claimed to support it.

Why was that a problem?

> 
> After this commit I guard use of vKill with a check to see if the
> multi-process feature is enabled or not.  I have removed the ability to
> disable vkill specifically from the console, the user must now disable
> the whole multi-process feature set as one.
> 
> I did consider leaving the separate vKill control switch in addition to
> the multi-process control switch, but this seemed unnecessary, and I
> worried that in the future another bug could be introduced where
> PACKET_vKill was used to guard sending a vKill.

Please leave the control switch in.  It's useful for testing, to emulate
targets that don't support the packet.

Thanks,
Pedro Alves

Re: [PATCH] gdb/remote: Don't use vKill if multi-process features are disabled

[Andrew Burgess]

* Pedro Alves <palves@redhat.com> [2016-03-17 22:21:05 +0000]:

> On 03/17/2016 10:02 PM, Andrew Burgess wrote:
> > The below was tested using native gdbserver on x86-64 Fedora Linux
> > with no regressions.
> > 
> > ---
> > 
> > The gdb remote protocol documentation is clear that the vKill command
> > should not be used unless the multi-process feature is reported as
> > supported by the remote target.
> > 
> > Currently within gdb we check to see if the vKill packet is enabled or
> > not before using the vKill command, however, the only way to disable
> > vKill is from the gdb console, the result is that vKill will be sent to
> > targets that don't support it, and never claimed to support it.
> 
> Why was that a problem?

You're right, if my remote target was well behaved it shouldn't cause
a problem...

<quick fix to my remote target>

... I have a new patch that is now just about bringing gdb into line
with the documentation, that is, not sending vKill unless that remote
target says it's supported.

> 
> > 
> > After this commit I guard use of vKill with a check to see if the
> > multi-process feature is enabled or not.  I have removed the ability to
> > disable vkill specifically from the console, the user must now disable
> > the whole multi-process feature set as one.
> > 
> > I did consider leaving the separate vKill control switch in addition to
> > the multi-process control switch, but this seemed unnecessary, and I
> > worried that in the future another bug could be introduced where
> > PACKET_vKill was used to guard sending a vKill.
> 
> Please leave the control switch in.  It's useful for testing, to emulate
> targets that don't support the packet.

Done.

OK to apply?

Thanks,
Andrew

---

The gdb remote protocol documentation is clear that the vKill command
should not be used unless the multi-process feature is reported as
supported by the remote target.

Currently in gdb we only check that PACKET_vKill is enabled before
sending a vKill.  The problem is that when a remote does not support
multi-process features, it is PACKET_multi_process that is disabled.

There is a desire to keep the PACKET_vKill control as a separate
independent mechanism for controlling just the vKill packet, so this
commit makes sending a vKill dependent on checking that both
PACKET_vKill, and PACKET_multi_process are enabled.  This should prevent
sending vKill packets to targets that don't support them.

gdb/ChangeLog:

	* remote.c (remote_kill): Use remote_multi_process_p.
	(remote_vkill): Add check of remote_multi_process_p.
---
 gdb/ChangeLog | 5 +++++
 gdb/remote.c  | 5 +++--
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 2df6ccd..0bcb430 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,8 @@
+2016-03-17  Andrew Burgess  <andrew.burgess@embecosm.com>
+
+	* remote.c (remote_kill): Use remote_multi_process_p.
+	(remote_vkill): Add check of remote_multi_process_p.
+
 2016-03-17  Jan Kratochvil  <jan.kratochvil@redhat.com>
 
 	* linux-thread-db.c (check_pid_namespace_match): Extend the message.
diff --git a/gdb/remote.c b/gdb/remote.c
index af0a08a..8c9e073 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -8907,7 +8907,7 @@ remote_kill (struct target_ops *ops)
   int pid = ptid_get_pid (inferior_ptid);
   struct remote_state *rs = get_remote_state ();
 
-  if (packet_support (PACKET_vKill) != PACKET_DISABLE)
+  if (remote_multi_process_p (rs))
     {
       /* If we're stopped while forking and we haven't followed yet,
 	 kill the child task.  We need to do this before killing the
@@ -8948,7 +8948,8 @@ remote_kill (struct target_ops *ops)
 static int
 remote_vkill (int pid, struct remote_state *rs)
 {
-  if (packet_support (PACKET_vKill) == PACKET_DISABLE)
+  if (!remote_multi_process_p (rs)
+      || (packet_support (PACKET_vKill) == PACKET_DISABLE))
     return -1;
 
   /* Tell the remote target to detach.  */
-- 
2.5.1

Re: [PATCH] gdb/remote: Don't use vKill if multi-process features are disabled

[Pedro Alves]

On 03/17/2016 11:26 PM, Andrew Burgess wrote:

> The gdb remote protocol documentation is clear that the vKill command
> should not be used unless the multi-process feature is reported as
> supported by the remote target.

Sorry, you were too fast... :-/

I recalled a reason to keep it.  The original reason for the docs saying
that is that vKill includes a PID, and without multi-process extensions,
there's no known non-fake PID to send.  (GDB is presently sending a
fake PID, as you've probably noticed).

The other-than-the-pid advantage of vKill over "k", is that "k" has
no reply, and thus no way to return error, failure to kill, etc.

Note gdbserver is making use of vKill with multi-process off:

/* Kill process.  Return 1 if successful, 0 if failure.  */
static int
handle_v_kill (char *own_buf)
{
  int pid;
  char *p = &own_buf[6];
  if (multi_process)
    pid = strtol (p, NULL, 16);
  else
    pid = signal_pid;
  if (pid != 0 && kill_inferior (pid) == 0)

So we could just not include the "pid" part if multi-process
is off, similar to D vs D;pid.  We'd still need the ";", due
to gdbserver's:

  if (startswith (own_buf, "vKill;"))
    {

Thanks,
Pedro Alves

Re: [PATCH] gdb/remote: Don't use vKill if multi-process features are disabled

[Andrew Burgess]

* Pedro Alves <palves@redhat.com> [2016-03-18 00:12:54 +0000]:

> On 03/17/2016 11:26 PM, Andrew Burgess wrote:
> 
> > The gdb remote protocol documentation is clear that the vKill command
> > should not be used unless the multi-process feature is reported as
> > supported by the remote target.
> 
> Sorry, you were too fast... :-/
> 
> I recalled a reason to keep it.  The original reason for the docs saying
> that is that vKill includes a PID, and without multi-process extensions,
> there's no known non-fake PID to send.  (GDB is presently sending a
> fake PID, as you've probably noticed).
> 
> The other-than-the-pid advantage of vKill over "k", is that "k" has
> no reply, and thus no way to return error, failure to kill, etc.
> 
> Note gdbserver is making use of vKill with multi-process off:
> 
> /* Kill process.  Return 1 if successful, 0 if failure.  */
> static int
> handle_v_kill (char *own_buf)
> {
>   int pid;
>   char *p = &own_buf[6];
>   if (multi_process)
>     pid = strtol (p, NULL, 16);
>   else
>     pid = signal_pid;
>   if (pid != 0 && kill_inferior (pid) == 0)
> 
> So we could just not include the "pid" part if multi-process
> is off, similar to D vs D;pid.  We'd still need the ";", due
> to gdbserver's:
> 
>   if (startswith (own_buf, "vKill;"))
>     {

OK, armed with this new insight I read the docs again, and now I see
that indeed my initial interpretation was not correct.  I see if I can
come up with a wording that feels clearer.

Thank you for setting me straight.

Andrew