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

[Andrew Burgess <andrew.burgess@embecosm.com>]

* 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