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

Andrew Burgess
2016-03-17T23:27:24Z

* 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