[PATCH/doc] Remove fixme of packet "k"

[PATCH/doc] Remove fixme of packet "k"

[Hui Zhu]

Current introduction of 'k' is:
‘k’
Kill request.
FIXME: There is no description of how to operate when a specific thread context has been selected (i.e. does 'k' kill only that thread?).

I checked the code about this part:
In GDB side:
static void
remote_kill (struct target_ops *ops)
{
   volatile struct gdb_exception ex;

   /* Catch errors so the user can quit from gdb even when we
      aren't on speaking terms with the remote system.  */
   TRY_CATCH (ex, RETURN_MASK_ERROR)
     {
       putpkt ("k");
     }
   if (ex.reason < 0)
     {
       if (ex.error == TARGET_CLOSE_ERROR)
	{
	  /* If we got an (EOF) error that caused the target
	     to go away, then we're done, that's what we wanted.
	     "k" is susceptible to cause a premature EOF, given
	     that the remote server isn't actually required to
	     reply to "k", and it can happen that it doesn't
	     even get to reply ACK to the "k".  */
	  return;
	}

	/* Otherwise, something went wrong.  We didn't actually kill
	   the target.  Just propagate the exception, and let the
	   user o:r higher layers decide what to do.  */
	throw_exception (ex);
     }

   /* We've killed the remote end, we get to mourn it.  Since this is
      target remote, single-process, mourning the inferior also
      unpushes remote_ops.  */
   target_mourn_inferior ();
}

static void
extended_remote_kill (struct target_ops *ops)
{
   int res;
   int pid = ptid_get_pid (inferior_ptid);
   struct remote_state *rs = get_remote_state ();

   res = remote_vkill (pid, rs);
   if (res == -1 && !(rs->extended && remote_multi_process_p (rs)))
     {
       /* Don't try 'k' on a multi-process aware stub -- it has no way
	 to specify the pid.  */

       putpkt ("k");
#if 0
       getpkt (&rs->buf, &rs->buf_size, 0);
       if (rs->buf[0] != 'O' || rs->buf[0] != 'K')
	res = 1;
#else
       /* Don't wait for it to die.  I'm not really sure it matters whether
	 we do or not.  For the existing stubs, kill is a noop.  */
       res = 0;
#endif
     }

   if (res != 0)
     error (_("Can't kill process"));

   target_mourn_inferior ();
}

In gdbserver side:
      fprintf (stderr, "Killing all inferiors\n");
       for_each_inferior (&all_processes, kill_inferior_callback);

       /* When using the extended protocol, we wait with no program
	 running.  The traditional protocol will exit instead.  */
       if (extended_protocol)
	{
	  last_status.kind = TARGET_WAITKIND_EXITED;
	  last_status.value.sig = GDB_SIGNAL_KILL;
	  return 0;
	}
       else
	exit (0);

So make a patch update doc of 'k' to:
‘k’
Kill all processes.

The ‘k’ packet has no reply.

Thanks,
Hui

2014-03-14  Hui Zhu  <hui@codesourcery.com>

	* gdb.texinfo (Packets): Update introduction of 'k'.

--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -33952,11 +33952,9 @@ step packet}.
  
  @item k
  @cindex @samp{k} packet
-Kill request.
+Kill all processes.
  
-FIXME: @emph{There is no description of how to operate when a specific
-thread context has been selected (i.e.@: does 'k' kill only that
-thread?)}.
+The @samp{k} packet has no reply.
  
  @item m @var{addr},@var{length}
  @cindex @samp{m} packet

Re: [PATCH/doc] Remove fixme of packet "k"

[Eli Zaretskii]

> Date: Fri, 14 Mar 2014 15:22:41 +0800
> From: Hui Zhu <hui_zhu@mentor.com>
> CC: Eli Zaretskii <eliz@gnu.org>
> 
> So make a patch update doc of 'k' to:
> ‘k’
> Kill all processes.
> 
> The ‘k’ packet has no reply.

Fine with me, assuming that someone who knows that code will confirm
your understanding.

Re: [PATCH/doc] Remove fixme of packet "k"

[Stan Shebs]

On 3/14/14 12:22 AM, Hui Zhu wrote:

[...]
> So make a patch update doc of 'k' to:
> ‘k’
> Kill all processes.
> 
> The ‘k’ packet has no reply.

For old packets in the protocol, we need to be careful not to tweak the
description so as to change the meaning.  In this case, the effect of
'k' is not precisely specified; while the current version of GDBserver
has taken it to mean "all inferiors", that may not be true of the stub
I shipped to a customer four years ago.

As a replacement for the FIXME, I suggest something like

"Kill the target process or processes."

"The exact effect of this packet is not specified.  For a single-process
target, it will kill that process if possible.  A multiple-process
target may choose to kill just one process, or all that that
are under GDB's control.  For more precise control, use the vKill packet."

"The ‘k’ packet has no reply."

It seems like it should say something about inferiors as well, but
I couldn't think of how to express it clearly.

Stan
stan@codesourcery.com

Re: [PATCH/doc] Remove fixme of packet "k"

[Hui Zhu]

On 03/19/14 04:52, Stan Shebs wrote:
> On 3/14/14 12:22 AM, Hui Zhu wrote:
>
> [...]
>> So make a patch update doc of 'k' to:
>> ‘k’
>> Kill all processes.
>>
>> The ‘k’ packet has no reply.
>
> For old packets in the protocol, we need to be careful not to tweak the
> description so as to change the meaning.  In this case, the effect of
> 'k' is not precisely specified; while the current version of GDBserver
> has taken it to mean "all inferiors", that may not be true of the stub
> I shipped to a customer four years ago.
>
> As a replacement for the FIXME, I suggest something like
>
> "Kill the target process or processes."
>
> "The exact effect of this packet is not specified.  For a single-process
> target, it will kill that process if possible.  A multiple-process
> target may choose to kill just one process, or all that that
> are under GDB's control.  For more precise control, use the vKill packet."
>
> "The ‘k’ packet has no reply."
>
> It seems like it should say something about inferiors as well, but
> I couldn't think of how to express it clearly.
>
> Stan
> stan@codesourcery.com
>

Hi Stan,

According to your comments.  I make a new patch.

Thanks,
Hui

2014-03-19  Stan Shebs  <stan@codesourcery.com>
	    Hui Zhu  <hui@codesourcery.com>

	* gdb.texinfo (k packet): Remove fixme and update introduction.
	(vKill packet): Add anchor.

--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -33952,11 +33952,16 @@ step packet}.
  
  @item k
  @cindex @samp{k} packet
-Kill request.
+Kill the target process or processes.
  
-FIXME: @emph{There is no description of how to operate when a specific
-thread context has been selected (i.e.@: does 'k' kill only that
-thread?)}.
+The exact effect of this packet is not specified.  For a single-process
+target, it will kill that process if possible.
+
+A multiple-process target may choose to kill just one process, or all
+that that are under GDB's control.  For more precise control, use the
+vKill packet.(@pxref{vKill packet})
+
+The @samp{k} packet has no reply.
  
  @item m @var{addr},@var{length}
  @cindex @samp{m} packet
@@ -34258,6 +34263,7 @@ request is completed.
  
  @item vKill;@var{pid}
  @cindex @samp{vKill} packet
+@anchor{vKill packet}
  Kill the process with the specified process ID.  @var{pid} is a
  hexadecimal integer identifying the process.  This packet is used in
  preference to @samp{k} when multiprocess protocol extensions are

Re: [PATCH/doc] Remove fixme of packet "k"

[Eli Zaretskii]

> Date: Wed, 19 Mar 2014 16:02:29 +0800
> From: Hui Zhu <hui_zhu@mentor.com>
> CC: Eli Zaretskii <eliz@gnu.org>
> 
> Hi Stan,
> 
> According to your comments.  I make a new patch.
> 
> Thanks,
> Hui
> 
> 2014-03-19  Stan Shebs  <stan@codesourcery.com>
> 	    Hui Zhu  <hui@codesourcery.com>
> 
> 	* gdb.texinfo (k packet): Remove fixme and update introduction.
> 	(vKill packet): Add anchor.

Thanks.  But these changes are both in the node called "Packets", not
"k packet" or "vKill packet", so please change the text in parentheses
to name the node (just once).

> +A multiple-process target may choose to kill just one process, or all
> +that that are under GDB's control.  For more precise control, use the
   ^^^^^^^^^
One "that" is redundant.

> +vKill packet.(@pxref{vKill packet})

@pxref should be before the period, like this:

  vKill packet (@pxref{vKill packet}).

OK with those changes.

Re: [PATCH/doc] Remove fixme of packet "k"

[Hui Zhu]

On 03/20/14 00:12, Eli Zaretskii wrote:
>> Date: Wed, 19 Mar 2014 16:02:29 +0800
>> From: Hui Zhu <hui_zhu@mentor.com>
>> CC: Eli Zaretskii <eliz@gnu.org>
>>
>> Hi Stan,
>>
>> According to your comments.  I make a new patch.
>>
>> Thanks,
>> Hui
>>
>> 2014-03-19  Stan Shebs  <stan@codesourcery.com>
>> 	    Hui Zhu  <hui@codesourcery.com>
>>
>> 	* gdb.texinfo (k packet): Remove fixme and update introduction.
>> 	(vKill packet): Add anchor.
>
> Thanks.  But these changes are both in the node called "Packets", not
> "k packet" or "vKill packet", so please change the text in parentheses
> to name the node (just once).
>
>> +A multiple-process target may choose to kill just one process, or all
>> +that that are under GDB's control.  For more precise control, use the
>     ^^^^^^^^^
> One "that" is redundant.
>
>> +vKill packet.(@pxref{vKill packet})
>
> @pxref should be before the period, like this:
>
>    vKill packet (@pxref{vKill packet}).
>
> OK with those changes.
>

Updated a new version according to your comments.
Please help me review it.

Thanks,
Hui

2014-03-20  Stan Shebs  <stan@codesourcery.com>
	    Hui Zhu  <hui@codesourcery.com>

	* gdb.texinfo (Packets): Remove fixme and update introduction of
	"k packet".  Add anchor to "vKill packet".

--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -33952,11 +33952,16 @@ step packet}.
  
  @item k
  @cindex @samp{k} packet
-Kill request.
+Kill the target process or processes.
  
-FIXME: @emph{There is no description of how to operate when a specific
-thread context has been selected (i.e.@: does 'k' kill only that
-thread?)}.
+The exact effect of this packet is not specified.  For a single-process
+target, it will kill that process if possible.
+
+A multiple-process target may choose to kill just one process, or all
+that are under GDB's control.  For more precise control, use the
+vKill packet (@pxref{vKill packet}).
+
+The @samp{k} packet has no reply.
  
  @item m @var{addr},@var{length}
  @cindex @samp{m} packet
@@ -34258,6 +34263,7 @@ request is completed.
  
  @item vKill;@var{pid}
  @cindex @samp{vKill} packet
+@anchor{vKill packet}
  Kill the process with the specified process ID.  @var{pid} is a
  hexadecimal integer identifying the process.  This packet is used in
  preference to @samp{k} when multiprocess protocol extensions are

Re: [PATCH/doc] Remove fixme of packet "k"

[Pedro Alves]

On 03/20/2014 03:44 AM, Hui Zhu wrote:

>   @item k
>   @cindex @samp{k} packet
> -Kill request.
> +Kill the target process or processes.
>   
> -FIXME: @emph{There is no description of how to operate when a specific
> -thread context has been selected (i.e.@: does 'k' kill only that
> -thread?)}.
> +The exact effect of this packet is not specified.  For a single-process
> +target, it will kill that process if possible.

This is looking good to me too, but I'd like to suggest further
extending it some.  As is I think this may leave bare metal stub
developers wondering what does "kill the single-process target"
means.  I'd leave the "Kill request" title unmodified.  I'd
suggest using "may" instead of "will".  I think we should mention
that even not replying an ack is OK.  So here's my suggestion:

@item k
@cindex @samp{k} packet
Kill request.

The exact effect of this packet is not specified.

For a bare-metal target, it may power cycle or reset the target
system.  For that reason, the @samp{k} packet has no reply.

For a single-process target, it may kill that process if possible.

A multiple-process target may choose to kill just one process, or all
that are under GDB's control.  For more precise control, use the vKill
packet (@pxref{vKill packet}).

If the target system immediately closes the connection in response to
@samp{k}, @value{GDBN} does not consider the lack of packet
acknowledgment to be an error, and assumes the kill was successful.

If connected using @kbd{target extended-remote}, and the target does
not close the connection in response to a kill request, @value{GDBN}
probes the target state as if a new connection was opened (pxfer to
the status/"?" packet here).

> +
> +A multiple-process target may choose to kill just one process, or all
> +that are under GDB's control.  For more precise control, use the

( Should be @value{GDBN}. )

-- 
Pedro Alves

Re: [PATCH/doc] Remove fixme of packet "k"

[Eli Zaretskii]

> Date: Thu, 20 Mar 2014 11:44:23 +0800
> From: Hui Zhu <hui_zhu@mentor.com>
> CC: <stanshebs@earthlink.net>, <gdb-patches@sourceware.org>
> 
> Updated a new version according to your comments.

This version is OK, thanks.

Re: [PATCH/doc] Remove fixme of packet "k"

[Eli Zaretskii]

> Date: Thu, 20 Mar 2014 10:49:13 +0000
> From: Pedro Alves <palves@redhat.com>
> CC: Eli Zaretskii <eliz@gnu.org>, stanshebs@earthlink.net,
>         gdb-patches@sourceware.org
> 
> This is looking good to me too, but I'd like to suggest further
> extending it some.

Your extensions are fine with me.

Thanks.

Re: [PATCH/doc] Remove fixme of packet "k"

[Hui Zhu]

On 03/21/14 00:06, Eli Zaretskii wrote:
>> Date: Thu, 20 Mar 2014 11:44:23 +0800
>> From: Hui Zhu <hui_zhu@mentor.com>
>> CC: <stanshebs@earthlink.net>, <gdb-patches@sourceware.org>
>>
>> Updated a new version according to your comments.
>
> This version is OK, thanks.
>

Make a new patch according to your comments.
Please help me review it.

Thanks,
Hui

2014-03-21  Pedro Alves  <palves@redhat.com>
	    Stan Shebs  <stan@codesourcery.com>
	    Hui Zhu  <hui@codesourcery.com>

	* gdb.texinfo (Packets): Add anchor to "? packet".
	Remove fixme and update introduction of "k packet".
	Add anchor to "vKill packet".

--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -33742,6 +33742,7 @@ The remote target both supports and has
  
  @item ?
  @cindex @samp{?} packet
+@anchor{? packet}
  Indicate the reason the target halted.  The reply is the same as for
  step and continue.  This packet has a special interpretation when the
  target is in non-stop mode; see @ref{Remote Non-Stop}.
@@ -33954,9 +33955,25 @@ step packet}.
  @cindex @samp{k} packet
  Kill request.
  
-FIXME: @emph{There is no description of how to operate when a specific
-thread context has been selected (i.e.@: does 'k' kill only that
-thread?)}.
+The exact effect of this packet is not specified.
+
+For a bare-metal target, it may power cycle or reset the target
+system.  For that reason, the @samp{k} packet has no reply.
+
+For a single-process target, it may kill that process if possible.
+
+A multiple-process target may choose to kill just one process, or all
+that are under @value{GDBN}'s control.  For more precise control, use
+the vKill packet (@pxref{vKill packet}).
+
+If the target system immediately closes the connection in response to
+@samp{k}, @value{GDBN} does not consider the lack of packet
+acknowledgment to be an error, and assumes the kill was successful.
+
+If connected using @kbd{target extended-remote}, and the target does
+not close the connection in response to a kill request, @value{GDBN}
+probes the target state as if a new connection was opened
+(@pxref{? packet}).
  
  @item m @var{addr},@var{length}
  @cindex @samp{m} packet
@@ -34258,6 +34275,7 @@ request is completed.
  
  @item vKill;@var{pid}
  @cindex @samp{vKill} packet
+@anchor{vKill packet}
  Kill the process with the specified process ID.  @var{pid} is a
  hexadecimal integer identifying the process.  This packet is used in
  preference to @samp{k} when multiprocess protocol extensions are

Re: [PATCH/doc] Remove fixme of packet "k"

[Eli Zaretskii]

> Date: Fri, 21 Mar 2014 16:25:52 +0800
> From: Hui Zhu <hui_zhu@mentor.com>
> CC: <gdb-patches@sourceware.org>
> 
> On 03/21/14 00:06, Eli Zaretskii wrote:
> >> Date: Thu, 20 Mar 2014 11:44:23 +0800
> >> From: Hui Zhu <hui_zhu@mentor.com>
> >> CC: <stanshebs@earthlink.net>, <gdb-patches@sourceware.org>
> >>
> >> Updated a new version according to your comments.
> >
> > This version is OK, thanks.
> >
> 
> Make a new patch according to your comments.

OK.

Re: [PATCH/doc] Remove fixme of packet "k"

[Hui Zhu]

On 03/21/14 16:38, Eli Zaretskii wrote:
>> Date: Fri, 21 Mar 2014 16:25:52 +0800
>> From: Hui Zhu <hui_zhu@mentor.com>
>> CC: <gdb-patches@sourceware.org>
>>
>> On 03/21/14 00:06, Eli Zaretskii wrote:
>>>> Date: Thu, 20 Mar 2014 11:44:23 +0800
>>>> From: Hui Zhu <hui_zhu@mentor.com>
>>>> CC: <stanshebs@earthlink.net>, <gdb-patches@sourceware.org>
>>>>
>>>> Updated a new version according to your comments.
>>>
>>> This version is OK, thanks.
>>>
>>
>> Make a new patch according to your comments.
>
> OK.
>

Thanks for your help.
Committed to https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=36cb1214c9f26b4e9b42d146dcf64621b36b91df

Best,
Hui