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

  * 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(-)
  

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
  

* 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