diff mbox

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

Message ID 1458252144-3496-1-git-send-email-andrew.burgess@embecosm.com
State New
Headers show

Commit Message

Andrew Burgess March 17, 2016, 10:02 p.m. UTC
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(-)

Comments

Pedro Alves March 17, 2016, 10:21 p.m. UTC | #1
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
diff mbox

Patch

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);