[PATCH 1/2] Remove dead code and "current" field from MI thread output doc

[PATCH 1/2] Remove dead code and "current" field from MI thread output doc

[Simon Marchi]

The MI documentation says that -thread-info output contains a "current"
field in the current thread tuple, with the value "*".  Current GDB
master does not do this, and I couldn't find any GDB version that did.
I suspect that it was never the case.

The code that would correspond to this in print_thread_info_1 is
essentially dead code.  The calls to uiout->text end up in
mi_out::do_text, which is empty.

This patch removes the documentation bit and the dead code.  This
"current" field is not necessary, since -thread-info outputs a
"current-thread-id" field.

gdb/ChangeLog:

	* thread.c (print_thread_info_1): Remove dead code.

gdb/doc/ChangeLog:

	* gdb.texinfo (GDB/MI Thread Commands): Remove "current" field
	from -thread-info output.
---
 gdb/doc/gdb.texinfo |  3 ---
 gdb/thread.c        | 10 +---------
 2 files changed, 1 insertion(+), 12 deletions(-)

diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 300d78eefb..9e872763b2 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -28088,9 +28088,6 @@ The result is a list of threads.  The following attributes are
 defined for a given thread:
 
 @table @samp
-@item current
-This field exists only for the current thread.  It has the value @samp{*}.
-
 @item id
 The global identifier that @value{GDBN} uses to refer to the thread.
 
diff --git a/gdb/thread.c b/gdb/thread.c
index abfce71eda..74d0d65e02 100644
--- a/gdb/thread.c
+++ b/gdb/thread.c
@@ -1283,15 +1283,7 @@ print_thread_info_1 (struct ui_out *uiout, char *requested_threads,
 
       chain2 = make_cleanup_ui_out_tuple_begin_end (uiout, NULL);
 
-      if (uiout->is_mi_like_p ())
-	{
-	  /* Compatibility.  */
-	  if (tp->ptid == current_ptid)
-	    uiout->text ("* ");
-	  else
-	    uiout->text ("  ");
-	}
-      else
+      if (!uiout->is_mi_like_p ())
 	{
 	  if (tp->ptid == current_ptid)
 	    uiout->field_string ("current", "*");
-- 
2.11.0

[PATCH 2/2] doc: Improve documentation about MI thread output

[Simon Marchi]

I noticed that the documentation on how the info about threads is output
in MI is duplicated and not up to date.  The duplication is between the
"GDB/MI Thread Information" page and the -thread-info result
description.

I improved the "GDB/MI Thread Information" page a bit and referred to it
in the -thread-info doc.  This way, the -thread-info doc is more precise
(it did not mention the "threads" and "current-thread-id" attributes)
and concise.

gdb/doc/ChangeLog:

	* gdb.texinfo (GDB/MI Thread Information): Add missing
	fields, re-word some things.
	(GDB/MI Thread Commands): Describe fields found in the output of
	-thread-info, remove description of fields in the
	thread output tuple, replace with a cross-reference to "GDB/MI
	Thread Information".
---
 gdb/doc/gdb.texinfo | 71 ++++++++++++++++++++---------------------------------
 1 file changed, 26 insertions(+), 45 deletions(-)

diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 9e872763b2..b7a5d1bea7 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -26852,24 +26852,36 @@ corresponds to the frame's code address.  This field may be absent.
 @subsection @sc{gdb/mi} Thread Information
 
 Whenever @value{GDBN} has to report an information about a thread, it
-uses a tuple with the following fields:
+uses a tuple with the following fields.  The fields are always present unless
+stated otherwise.
 
 @table @code
 @item id
-The global numeric id assigned to the thread by @value{GDBN}.  This field is
-always present.
+The global numeric id assigned to the thread by @value{GDBN}.
 
 @item target-id
-Target-specific string identifying the thread.  This field is always present.
+The target-specific string identifying the thread.
 
 @item details
 Additional information about the thread provided by the target.
 It is supposed to be human-readable and not interpreted by the
 frontend.  This field is optional.
 
+@item name
+The name of the thread.  If the user specified a name using the
+@code{thread name} command, then this name is given.  Otherwise, if
+@value{GDBN} can extract the thread name from the target, then that
+name is given.  If @value{GDBN} cannot find the thread name, then this
+field is omitted.
+
 @item state
-Either @samp{stopped} or @samp{running}, depending on whether the
-thread is presently running.  This field is always present.
+The execution state of the thread, either @samp{stopped} or @samp{running},
+depending on whether the thread is presently running.
+
+@item frame
+The stack frame currently executing in the thread.  This field is only present
+if the thread is stopped.  Its format is documented in
+@ref{GDB/MI Frame Information}.
 
 @item core
 The value of this field is an integer number of the processor core the
@@ -28084,48 +28096,17 @@ about all threads.
 
 @subsubheading Result
 
-The result is a list of threads.  The following attributes are
-defined for a given thread:
+The result contains the following attributes:
 
 @table @samp
-@item id
-The global identifier that @value{GDBN} uses to refer to the thread.
-
-@item target-id
-The identifier that the target uses to refer to the thread.
-
-@item details
-Extra information about the thread, in a target-specific format.  This
-field is optional.
-
-@item name
-The name of the thread.  If the user specified a name using the
-@code{thread name} command, then this name is given.  Otherwise, if
-@value{GDBN} can extract the thread name from the target, then that
-name is given.  If @value{GDBN} cannot find the thread name, then this
-field is omitted.
-
-@item frame
-The stack frame currently executing in the thread.
-
-@item state
-The thread's state.  The @samp{state} field may have the following
-values:
-
-@table @code
-@item stopped
-The thread is stopped.  Frame information is available for stopped
-threads.
-
-@item running
-The thread is running.  There's no frame information for running
-threads.
-
-@end table
+@item threads
+A list of threads.  The format of the elements of the list is described in
+@ref{GDB/MI Thread Information}.
 
-@item core
-If @value{GDBN} can find the CPU core on which this thread is running,
-then this field is the core identifier.  This field is optional.
+@item current-thread-id
+The global id of the current selected thread.  This field is only present if
+there is a selected thread and no @var{thread-id} argument was passed to the
+command..
 
 @end table
 
-- 
2.11.0

Re: [PATCH 2/2] doc: Improve documentation about MI thread output

[Simon Marchi]

On 2017-04-12 14:06, Simon Marchi wrote:
> +@item current-thread-id
> +The global id of the current selected thread.  This field is only 
> present if
> +there is a selected thread and no @var{thread-id} argument was passed 
> to the
> +command..

Oops there are two periods here.  I removed the extra one locally.

Re: [PATCH 2/2] doc: Improve documentation about MI thread output

[Eli Zaretskii]

> From: Simon Marchi <simon.marchi@ericsson.com>
> CC: Simon Marchi <simon.marchi@ericsson.com>
> Date: Wed, 12 Apr 2017 14:06:10 -0400
> 
> I noticed that the documentation on how the info about threads is output
> in MI is duplicated and not up to date.  The duplication is between the
> "GDB/MI Thread Information" page and the -thread-info result
> description.
> 
> I improved the "GDB/MI Thread Information" page a bit and referred to it
> in the -thread-info doc.  This way, the -thread-info doc is more precise
> (it did not mention the "threads" and "current-thread-id" attributes)
> and concise.

Thanks.  This is okay, with a couple of minor gotchas:

> +@item current-thread-id
> +The global id of the current selected thread.  This field is only present if
                        ^^^^^^^
"currently"

> +there is a selected thread and no @var{thread-id} argument was passed to the

How can there not be a selected thread?

> +command..

One extra period at end of sentence.

Re: [PATCH 2/2] doc: Improve documentation about MI thread output

[Simon Marchi]

On 17-04-12 03:18 PM, Eli Zaretskii wrote:
>> From: Simon Marchi <simon.marchi@ericsson.com>
>> CC: Simon Marchi <simon.marchi@ericsson.com>
>> Date: Wed, 12 Apr 2017 14:06:10 -0400
>>
>> I noticed that the documentation on how the info about threads is output
>> in MI is duplicated and not up to date.  The duplication is between the
>> "GDB/MI Thread Information" page and the -thread-info result
>> description.
>>
>> I improved the "GDB/MI Thread Information" page a bit and referred to it
>> in the -thread-info doc.  This way, the -thread-info doc is more precise
>> (it did not mention the "threads" and "current-thread-id" attributes)
>> and concise.
> 
> Thanks.  This is okay, with a couple of minor gotchas:
> 
>> +@item current-thread-id
>> +The global id of the current selected thread.  This field is only present if
>                         ^^^^^^^
> "currently"

Ok.

>> +there is a selected thread and no @var{thread-id} argument was passed to the
> 
> How can there not be a selected thread?

When the currently selected inferior is not running.  The obvious case is the initial
state of gdb.  But it's also possible to have threads but none is selected, for example
when you add a second inferior and switch to it, while the first inferior is running.

The field is output if inferior_ptid != null_ptid, so maybe there are other situations
I am not aware of where there isn't a current thread.

>> +command..
> 
> One extra period at end of sentence.

Ok.

Thanks,

Simon

Re: [PATCH 2/2] doc: Improve documentation about MI thread output

[Eli Zaretskii]

> From: Simon Marchi <simon.marchi@ericsson.com>
> Date: Wed, 12 Apr 2017 15:26:08 -0400
> 
> >> +there is a selected thread and no @var{thread-id} argument was passed to the
> > 
> > How can there not be a selected thread?
> 
> When the currently selected inferior is not running.  The obvious case is the initial
> state of gdb.  But it's also possible to have threads but none is selected, for example
> when you add a second inferior and switch to it, while the first inferior is running.
> 
> The field is output if inferior_ptid != null_ptid, so maybe there are other situations
> I am not aware of where there isn't a current thread.

I think we need to describe at least the most "popular" situations
where this happens.  The initial state of GDB is not an interesting
case, but others are.  In particular, IMO it would be good to state
that when there's only one inferior being debugged that has been run
already, there will always be a selected thread.

Thanks.

Re: [PATCH 2/2] doc: Improve documentation about MI thread output

[Simon Marchi]

On 17-04-12 03:33 PM, Eli Zaretskii wrote:
>> From: Simon Marchi <simon.marchi@ericsson.com>
>> Date: Wed, 12 Apr 2017 15:26:08 -0400
>>
>>>> +there is a selected thread and no @var{thread-id} argument was passed to the
>>>
>>> How can there not be a selected thread?
>>
>> When the currently selected inferior is not running.  The obvious case is the initial
>> state of gdb.  But it's also possible to have threads but none is selected, for example
>> when you add a second inferior and switch to it, while the first inferior is running.
>>
>> The field is output if inferior_ptid != null_ptid, so maybe there are other situations
>> I am not aware of where there isn't a current thread.
> 
> I think we need to describe at least the most "popular" situations
> where this happens.  The initial state of GDB is not an interesting
> case, but others are.  In particular, IMO it would be good to state
> that when there's only one inferior being debugged that has been run
> already, there will always be a selected thread.

I agree that we could give an example of situation where there _isn't_
a selected thread.  Readers may, like you did, find that it's an odd
claim and wonder how it's possible that there isn't a selected thread.
But I don't think it's useful (and maybe even counterproductive) to try
to define some situation where the field will always be present.

The important thing that users of this API need to know is that the field
may not be there.  This will encourage them to program defensively and check
whether the field is present before trying to use it.  If we try to define a
green zone where the field is supposedly always be present, it will incite some
people to skip the check, which will potentially come back and bite them if the
behavior of GDB changes or there's a situation we haven't thought of where it
can happen.

Simon

Re: [PATCH 2/2] doc: Improve documentation about MI thread output

[Eli Zaretskii]

> CC: <gdb-patches@sourceware.org>
> From: Simon Marchi <simon.marchi@ericsson.com>
> Date: Wed, 12 Apr 2017 16:09:35 -0400
> 
> > I think we need to describe at least the most "popular" situations
> > where this happens.  The initial state of GDB is not an interesting
> > case, but others are.  In particular, IMO it would be good to state
> > that when there's only one inferior being debugged that has been run
> > already, there will always be a selected thread.
> 
> I agree that we could give an example of situation where there _isn't_
> a selected thread.  Readers may, like you did, find that it's an odd
> claim and wonder how it's possible that there isn't a selected thread.
> But I don't think it's useful (and maybe even counterproductive) to try
> to define some situation where the field will always be present.
> 
> The important thing that users of this API need to know is that the field
> may not be there.  This will encourage them to program defensively and check
> whether the field is present before trying to use it.  If we try to define a
> green zone where the field is supposedly always be present, it will incite some
> people to skip the check, which will potentially come back and bite them if the
> behavior of GDB changes or there's a situation we haven't thought of where it
> can happen.

IME, defensive programming aside, knowing that some field could or
could not appear is not always enough, sometimes you also need to
understand the semantics and the significance of whether it does or
doesn't, because you might need to interpret that in some way, e.g. to
present some meaningful message to a human user.

That said, I'm not going to argue.  It's up to you.

Re: [PATCH 2/2] doc: Improve documentation about MI thread output

[Simon Marchi]

On 17-04-12 04:35 PM, Eli Zaretskii wrote:
> IME, defensive programming aside, knowing that some field could or
> could not appear is not always enough, sometimes you also need to
> understand the semantics and the significance of whether it does or
> doesn't, because you might need to interpret that in some way, e.g. to
> present some meaningful message to a human user.

I agree that a clarification would be helpful to understand to make sense
of the description.  I just don't want it to be interpreted as a formal
specification of when the field appears.  That's why I'm not comfortable
with a claim such as "the field will always be present when X and Y".

What do you think of?

@item current-thread-id
The global id of the currently selected thread.  This field is omitted if there
is no selected thread (for example, when the selected inferior is not running,
and therefore has no threads) or if a @var{thread-id} argument was passed to
the command.

> That said, I'm not going to argue.  It's up to you.

Thanks,

Simon

Re: [PATCH 2/2] doc: Improve documentation about MI thread output

[Eli Zaretskii]

> CC: <gdb-patches@sourceware.org>
> From: Simon Marchi <simon.marchi@ericsson.com>
> Date: Wed, 12 Apr 2017 17:12:02 -0400
> 
> What do you think of?
> 
> @item current-thread-id
> The global id of the currently selected thread.  This field is omitted if there
> is no selected thread (for example, when the selected inferior is not running,
> and therefore has no threads) or if a @var{thread-id} argument was passed to
> the command.

Fine with me, thanks.

Re: [PATCH 2/2] doc: Improve documentation about MI thread output

[Simon Marchi]

On 2017-04-13 02:02, Eli Zaretskii wrote:
>> CC: <gdb-patches@sourceware.org>
>> From: Simon Marchi <simon.marchi@ericsson.com>
>> Date: Wed, 12 Apr 2017 17:12:02 -0400
>> 
>> What do you think of?
>> 
>> @item current-thread-id
>> The global id of the currently selected thread.  This field is omitted 
>> if there
>> is no selected thread (for example, when the selected inferior is not 
>> running,
>> and therefore has no threads) or if a @var{thread-id} argument was 
>> passed to
>> the command.
> 
> Fine with me, thanks.

Thanks, both patches are pushed.

Simon

Re: [PATCH 1/2] Remove dead code and "current" field from MI thread output doc

[Eli Zaretskii]

> From: Simon Marchi <simon.marchi@ericsson.com>
> CC: Simon Marchi <simon.marchi@ericsson.com>
> Date: Wed, 12 Apr 2017 14:06:09 -0400
> 
> The MI documentation says that -thread-info output contains a "current"
> field in the current thread tuple, with the value "*".  Current GDB
> master does not do this, and I couldn't find any GDB version that did.
> I suspect that it was never the case.
> 
> The code that would correspond to this in print_thread_info_1 is
> essentially dead code.  The calls to uiout->text end up in
> mi_out::do_text, which is empty.
> 
> This patch removes the documentation bit and the dead code.  This
> "current" field is not necessary, since -thread-info outputs a
> "current-thread-id" field.
> 
> gdb/ChangeLog:
> 
> 	* thread.c (print_thread_info_1): Remove dead code.
> 
> gdb/doc/ChangeLog:
> 
> 	* gdb.texinfo (GDB/MI Thread Commands): Remove "current" field
> 	from -thread-info output.

OK for the documentation part.

Thanks.