Add "thread-group id" and "id in thread-group" info to -thread-info output

Message ID 20170119144130.4341-1-simon.marchi@ericsson.com
State New, archived
Headers

Commit Message

Simon Marchi Jan. 19, 2017, 2:41 p.m. UTC
  The CLI has been showing inferior-qualified thread ids for a while, but
front-ends have no way to do the same, as only the global thread ids are
included in the MI output.  In front-ends in general, but mostly in
those that include a gdb console, it would be interesting to show the
same thread id in the GUI than what "info threads" displays.  The user
would then see consistent thread ids across the two UIs.

This patch adds two extra fields to the -thread-info MI command output.
In this snippet of prettified -thread-info output, the lines marked with
a + are new:

    {
+     tg-id = "2",
+     id-in-tg = "1",
      id = "3",
      target-id = "Thread 0x2aaaaab04640 (LWP 3228)",
      name = "mi-qualified-th",
      ...
    },

Here, the thread has the global id 3 (monotonously increasing across all
inferiors) and the inferior-qualified id 2.1.  The global thread id is
still the one used throughout the rest of MI.  The new fields are simply
intended to be presented to the user.

We chose adding the new information as two separate fields rather than a
single field with the value "2.1".  We think that it gives the front-end
more flexibility about how it want to present the info.

Note thatt I renamed the existing CLI field "id-in-tg" to "qualified-id"
and named one of the new MI field "id-in-tg", because I found that
"id-in-tg" described better the Y in X.Y, and "qualified-id" described
better the whole thing.  It's not a big deal, as the field name is never
shown in the CLI.  We have to carefully choose the new names however,
since they'll be exposed in the MI.

gdb/ChangeLog:

	* thread.c (print_thread_info_1): Rename field id-in-tg to
	qualified-id.  Add new fields id-in-tg and tg-id when printing
	on MI.

gdb/testsuite/ChangeLog:

	* mi-qualified-thread-id.exp: New file.
	* mi-qualified-thread-id.c: New file.
---
 gdb/testsuite/gdb.mi/mi-qualified-thread-id.c   | 57 +++++++++++++++++++
 gdb/testsuite/gdb.mi/mi-qualified-thread-id.exp | 76 +++++++++++++++++++++++++
 gdb/thread.c                                    | 11 +++-
 3 files changed, 141 insertions(+), 3 deletions(-)
 create mode 100644 gdb/testsuite/gdb.mi/mi-qualified-thread-id.c
 create mode 100644 gdb/testsuite/gdb.mi/mi-qualified-thread-id.exp
  

Comments

Luis Machado Jan. 19, 2017, 2:54 p.m. UTC | #1
On 01/19/2017 08:41 AM, Simon Marchi wrote:
> diff --git a/gdb/testsuite/gdb.mi/mi-qualified-thread-id.exp b/gdb/testsuite/gdb.mi/mi-qualified-thread-id.exp
> new file mode 100644
> index 0000000000..43c1409786
> --- /dev/null
> +++ b/gdb/testsuite/gdb.mi/mi-qualified-thread-id.exp
> @@ -0,0 +1,76 @@
> +# Copyright 2017 Free Software Foundation, Inc.
> +
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +
> +load_lib mi-support.exp
> +set MIFLAGS "-i=mi"
> +
> +standard_testfile
> +
> +if {[use_gdb_stub]} {
> +    # There is no point in testing inferior-qualified thread ids if we can't
> +    # have multiple inferiors.
> +    untested "multi-inferior not supported"
> +    return
> +}
> +
> +if  { [gdb_compile_pthreads "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable {debug}] != "" } {
> +     untested ${testfile}

untested "failed to compile"

> +     return -1
> +}
> +
> +# Prepare the inferiors.  Start and run them until the "thread_started" function.
> +
> +proc setup { } {
> +    global binfile
> +    global srcdir
> +    global subdir
> +    global srcfile
> +
> +    gdb_exit
> +    mi_gdb_start
> +    mi_gdb_reinitialize_dir $srcdir/$subdir
> +    mi_gdb_load ${binfile}
> +
> +    mi_create_breakpoint "thread_started" "insert breakpoint at thread_started"
> +
> +    # Start inferior 1
> +    mi_run_cmd
> +    mi_expect_stop "breakpoint-hit" "thread_started" ".*" "${srcfile}" ".*" {"" "disp=\"keep\""} "inferior 1 stops"
> +
> +    # Add and start inferior 2
> +    mi_gdb_test "add-inferior -exec ${binfile}" ".*=thread-group-added,id=\"i2\".*" "add second inferior"
> +    mi_gdb_test "inferior 2" ".*" "switch to inferior 2"
> +    mi_run_cmd
> +    mi_expect_stop "breakpoint-hit" "thread_started" ".*" "${srcfile}" ".*" {"" "disp=\"keep\""} "inferior 2 stops"
> +}
> +
> +proc test_thread_info_qualified_thread_id { } {
> +    setup
> +
> +    set expected [join \
> +	{ "111\\^done,threads=.*" \
> +	  "tg-id=\"1\",id-in-tg=\"1\",id=\"1\".*" \
> +	  "tg-id=\"1\",id-in-tg=\"2\",id=\"2\".*" \
> +	  "tg-id=\"2\",id-in-tg=\"1\",id=\"3\".*" \
> +	  "tg-id=\"2\",id-in-tg=\"2\",id=\"4\".*" \
> +	} ""]
> +
> +    mi_gdb_test \
> +	"111-thread-info" \
> +	 $expected \
> +	"thread ids in -thread-info"
> +}
> +
> +test_thread_info_qualified_thread_id
> diff --git a/gdb/thread.c b/gdb/thread.c
> index e45b25750e..29cfa61e13 100644
> --- a/gdb/thread.c
> +++ b/gdb/thread.c
> @@ -1247,7 +1247,7 @@ print_thread_info_1 (struct ui_out *uiout, char *requested_threads,
>        uiout->table_header (1, ui_left, "current", "");
>
>        if (!uiout->is_mi_like_p ())
> -	uiout->table_header (4, ui_left, "id-in-tg", "Id");
> +	uiout->table_header (4, ui_left, "qualified-id", "Id");
>        if (show_global_ids || uiout->is_mi_like_p ())
>  	uiout->table_header (4, ui_left, "id", "GId");
>        uiout->table_header (17, ui_left, "target-id", "Target Id");
> @@ -1282,8 +1282,13 @@ print_thread_info_1 (struct ui_out *uiout, char *requested_threads,
>  	    uiout->field_skip ("current");
>  	}
>
> -      if (!uiout->is_mi_like_p ())
> -	uiout->field_string ("id-in-tg", print_thread_id (tp));
> +      if (uiout->is_mi_like_p ())
> +	{
> +	  uiout->field_int ("tg-id", tp->inf->num);
> +	  uiout->field_int ("id-in-tg", tp->per_inf_num);
> +	}
> +      else
> +	uiout->field_string ("qualified-id", print_thread_id (tp));
>
>        if (show_global_ids || uiout->is_mi_like_p ())
>  	uiout->field_int ("id", tp->global_num);
>

FWIW, it sounds OK to me.
  
Simon Marchi Jan. 19, 2017, 2:56 p.m. UTC | #2
On 17-01-19 09:54 AM, Luis Machado wrote:
> On 01/19/2017 08:41 AM, Simon Marchi wrote:
>> diff --git a/gdb/testsuite/gdb.mi/mi-qualified-thread-id.exp b/gdb/testsuite/gdb.mi/mi-qualified-thread-id.exp
>> new file mode 100644
>> index 0000000000..43c1409786
>> --- /dev/null
>> +++ b/gdb/testsuite/gdb.mi/mi-qualified-thread-id.exp
>> @@ -0,0 +1,76 @@
>> +# Copyright 2017 Free Software Foundation, Inc.
>> +
>> +# This program is free software; you can redistribute it and/or modify
>> +# it under the terms of the GNU General Public License as published by
>> +# the Free Software Foundation; either version 3 of the License, or
>> +# (at your option) any later version.
>> +#
>> +# This program is distributed in the hope that it will be useful,
>> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
>> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> +# GNU General Public License for more details.
>> +#
>> +# You should have received a copy of the GNU General Public License
>> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
>> +
>> +load_lib mi-support.exp
>> +set MIFLAGS "-i=mi"
>> +
>> +standard_testfile
>> +
>> +if {[use_gdb_stub]} {
>> +    # There is no point in testing inferior-qualified thread ids if we can't
>> +    # have multiple inferiors.
>> +    untested "multi-inferior not supported"
>> +    return
>> +}
>> +
>> +if  { [gdb_compile_pthreads "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable {debug}] != "" } {
>> +     untested ${testfile}
> 
> untested "failed to compile"

Whoops, bad practice I copied before your cleanup.  Thanks for your vigilance!
  
Simon Marchi Feb. 20, 2017, 4:17 p.m. UTC | #3
On 17-01-19 09:41 AM, Simon Marchi wrote:
> The CLI has been showing inferior-qualified thread ids for a while, but
> front-ends have no way to do the same, as only the global thread ids are
> included in the MI output.  In front-ends in general, but mostly in
> those that include a gdb console, it would be interesting to show the
> same thread id in the GUI than what "info threads" displays.  The user
> would then see consistent thread ids across the two UIs.
> 
> This patch adds two extra fields to the -thread-info MI command output.
> In this snippet of prettified -thread-info output, the lines marked with
> a + are new:
> 
>     {
> +     tg-id = "2",
> +     id-in-tg = "1",
>       id = "3",
>       target-id = "Thread 0x2aaaaab04640 (LWP 3228)",
>       name = "mi-qualified-th",
>       ...
>     },
> 
> Here, the thread has the global id 3 (monotonously increasing across all
> inferiors) and the inferior-qualified id 2.1.  The global thread id is
> still the one used throughout the rest of MI.  The new fields are simply
> intended to be presented to the user.
> 
> We chose adding the new information as two separate fields rather than a
> single field with the value "2.1".  We think that it gives the front-end
> more flexibility about how it want to present the info.
> 
> Note thatt I renamed the existing CLI field "id-in-tg" to "qualified-id"
> and named one of the new MI field "id-in-tg", because I found that
> "id-in-tg" described better the Y in X.Y, and "qualified-id" described
> better the whole thing.  It's not a big deal, as the field name is never
> shown in the CLI.  We have to carefully choose the new names however,
> since they'll be exposed in the MI.
> 
> gdb/ChangeLog:
> 
> 	* thread.c (print_thread_info_1): Rename field id-in-tg to
> 	qualified-id.  Add new fields id-in-tg and tg-id when printing
> 	on MI.
> 
> gdb/testsuite/ChangeLog:
> 
> 	* mi-qualified-thread-id.exp: New file.
> 	* mi-qualified-thread-id.c: New file.
> ---
>  gdb/testsuite/gdb.mi/mi-qualified-thread-id.c   | 57 +++++++++++++++++++
>  gdb/testsuite/gdb.mi/mi-qualified-thread-id.exp | 76 +++++++++++++++++++++++++
>  gdb/thread.c                                    | 11 +++-
>  3 files changed, 141 insertions(+), 3 deletions(-)
>  create mode 100644 gdb/testsuite/gdb.mi/mi-qualified-thread-id.c
>  create mode 100644 gdb/testsuite/gdb.mi/mi-qualified-thread-id.exp
> 
> diff --git a/gdb/testsuite/gdb.mi/mi-qualified-thread-id.c b/gdb/testsuite/gdb.mi/mi-qualified-thread-id.c
> new file mode 100644
> index 0000000000..cb50e50c32
> --- /dev/null
> +++ b/gdb/testsuite/gdb.mi/mi-qualified-thread-id.c
> @@ -0,0 +1,57 @@
> +/* Copyright 2017 Free Software Foundation, Inc.
> +
> +   This file is part of GDB.
> +
> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; either version 3 of the License, or
> +   (at your option) any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
> +
> +#include <pthread.h>
> +
> +static pthread_barrier_t barrier;
> +
> +static void *
> +thread_func (void *arg)
> +{
> +  /* Notify the main thread that this thread has started.  */
> +  pthread_barrier_wait (&barrier);
> +
> +  /* Wait until main gives us the signal that we can quit.  */
> +  pthread_barrier_wait (&barrier);
> +  return NULL;
> +}
> +
> +static void
> +thread_started ()
> +{
> +}
> +
> +int main ()
> +{
> +  pthread_t thread;
> +
> +  pthread_barrier_init (&barrier, NULL, 2);
> +
> +  pthread_create (&thread, NULL, thread_func, NULL);
> +
> +  /* Wait until the thread has started.  */
> +  pthread_barrier_wait (&barrier);
> +
> +  thread_started ();
> +
> +  /* Tell the thread that it can quit.  */
> +  pthread_barrier_wait (&barrier);
> +
> +  pthread_join (thread, NULL);
> +
> +  return 0;
> +}
> diff --git a/gdb/testsuite/gdb.mi/mi-qualified-thread-id.exp b/gdb/testsuite/gdb.mi/mi-qualified-thread-id.exp
> new file mode 100644
> index 0000000000..43c1409786
> --- /dev/null
> +++ b/gdb/testsuite/gdb.mi/mi-qualified-thread-id.exp
> @@ -0,0 +1,76 @@
> +# Copyright 2017 Free Software Foundation, Inc.
> +
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +
> +load_lib mi-support.exp
> +set MIFLAGS "-i=mi"
> +
> +standard_testfile
> +
> +if {[use_gdb_stub]} {
> +    # There is no point in testing inferior-qualified thread ids if we can't
> +    # have multiple inferiors.
> +    untested "multi-inferior not supported"
> +    return
> +}
> +
> +if  { [gdb_compile_pthreads "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable {debug}] != "" } {
> +     untested ${testfile}
> +     return -1
> +}
> +
> +# Prepare the inferiors.  Start and run them until the "thread_started" function.
> +
> +proc setup { } {
> +    global binfile
> +    global srcdir
> +    global subdir
> +    global srcfile
> +
> +    gdb_exit
> +    mi_gdb_start
> +    mi_gdb_reinitialize_dir $srcdir/$subdir
> +    mi_gdb_load ${binfile}
> +
> +    mi_create_breakpoint "thread_started" "insert breakpoint at thread_started"
> +
> +    # Start inferior 1
> +    mi_run_cmd
> +    mi_expect_stop "breakpoint-hit" "thread_started" ".*" "${srcfile}" ".*" {"" "disp=\"keep\""} "inferior 1 stops"
> +
> +    # Add and start inferior 2
> +    mi_gdb_test "add-inferior -exec ${binfile}" ".*=thread-group-added,id=\"i2\".*" "add second inferior"
> +    mi_gdb_test "inferior 2" ".*" "switch to inferior 2"
> +    mi_run_cmd
> +    mi_expect_stop "breakpoint-hit" "thread_started" ".*" "${srcfile}" ".*" {"" "disp=\"keep\""} "inferior 2 stops"
> +}
> +
> +proc test_thread_info_qualified_thread_id { } {
> +    setup
> +
> +    set expected [join \
> +	{ "111\\^done,threads=.*" \
> +	  "tg-id=\"1\",id-in-tg=\"1\",id=\"1\".*" \
> +	  "tg-id=\"1\",id-in-tg=\"2\",id=\"2\".*" \
> +	  "tg-id=\"2\",id-in-tg=\"1\",id=\"3\".*" \
> +	  "tg-id=\"2\",id-in-tg=\"2\",id=\"4\".*" \
> +	} ""]
> +
> +    mi_gdb_test \
> +	"111-thread-info" \
> +	 $expected \
> +	"thread ids in -thread-info"
> +}
> +
> +test_thread_info_qualified_thread_id
> diff --git a/gdb/thread.c b/gdb/thread.c
> index e45b25750e..29cfa61e13 100644
> --- a/gdb/thread.c
> +++ b/gdb/thread.c
> @@ -1247,7 +1247,7 @@ print_thread_info_1 (struct ui_out *uiout, char *requested_threads,
>        uiout->table_header (1, ui_left, "current", "");
>  
>        if (!uiout->is_mi_like_p ())
> -	uiout->table_header (4, ui_left, "id-in-tg", "Id");
> +	uiout->table_header (4, ui_left, "qualified-id", "Id");
>        if (show_global_ids || uiout->is_mi_like_p ())
>  	uiout->table_header (4, ui_left, "id", "GId");
>        uiout->table_header (17, ui_left, "target-id", "Target Id");
> @@ -1282,8 +1282,13 @@ print_thread_info_1 (struct ui_out *uiout, char *requested_threads,
>  	    uiout->field_skip ("current");
>  	}
>  
> -      if (!uiout->is_mi_like_p ())
> -	uiout->field_string ("id-in-tg", print_thread_id (tp));
> +      if (uiout->is_mi_like_p ())
> +	{
> +	  uiout->field_int ("tg-id", tp->inf->num);
> +	  uiout->field_int ("id-in-tg", tp->per_inf_num);
> +	}
> +      else
> +	uiout->field_string ("qualified-id", print_thread_id (tp));
>  
>        if (show_global_ids || uiout->is_mi_like_p ())
>  	uiout->field_int ("id", tp->global_num);
> 

Ping.
  
Pedro Alves Feb. 23, 2017, 7:49 p.m. UTC | #4
Hi Simon,

On 02/20/2017 04:17 PM, Simon Marchi wrote:

> 
> Ping.
> 

The code itself looks fine to me.

However, this needs manual and NEWS changes.

And, I think you should also test what is printed before you
add the second inferior.  This is because while the CLI
only prints qualified IDs after adding the second inferior,
MI is always printing them.  We should add a test to make sure
that continues to be the case even if someone changes/refactors
the code.

Thanks,
Pedro Alves
  
Simon Marchi Feb. 24, 2017, 8:22 p.m. UTC | #5
On 2017-02-23 14:49, Pedro Alves wrote:
> Hi Simon,
> 
> On 02/20/2017 04:17 PM, Simon Marchi wrote:
> 
>> 
>> Ping.
>> 
> 
> The code itself looks fine to me.
> 
> However, this needs manual and NEWS changes.

Of course, forgot about that.

> And, I think you should also test what is printed before you
> add the second inferior.  This is because while the CLI
> only prints qualified IDs after adding the second inferior,
> MI is always printing them.  We should add a test to make sure
> that continues to be the case even if someone changes/refactors
> the code.

Ok.

While writing the documentation parts, these question popped to mind:

- The type of the id-in-tg field (a string) assumes that a thread 
belongs to a single group.  Currently, the only kind of "thread-group" 
we have is inferiors.  The thread-group terminology suggests that in the 
future we might have other kinds of thread groups, like user-defined 
arbitrary groups.  Otherwise, why would MI use "thread group" instead of 
"inferior"?  Because of this, do you think that the type of the field 
will be limiting?  For example, should we document right that this field 
can be a list, when a thread is part of multiple groups, for example?

- The format of the id-in-tg field is "1" for inferior/thread-group 1.  
In -list-thread-groups, the id is shown as "i1".  Again, if there are 
other kinds of groups later, we can assume that we'll need to 
differentiate the type, so just "1" won't work.  For this reason and for 
consistency with the result of -list-thread-groups, should we show "i1" 
here as well?

Thanks,

Simon
  
Simon Marchi Feb. 24, 2017, 8:32 p.m. UTC | #6
On 2017-02-24 15:22, Simon Marchi wrote:
> - The type of the id-in-tg field (a string) assumes that a thread
> belongs to a single group.  Currently, the only kind of "thread-group"
> we have is inferiors.  The thread-group terminology suggests that in
> the future we might have other kinds of thread groups, like
> user-defined arbitrary groups.  Otherwise, why would MI use "thread
> group" instead of "inferior"?  Because of this, do you think that the
> type of the field will be limiting?  For example, should we document
> right that this field can be a list, when a thread is part of multiple
> groups, for example?

Sorry, this paragraph should have referred to the "tg-id" field.  
Although the "id-in-tg" makes sense when talking about an inferior 
thread group, but not so much when talking about other kinds of thread 
groups.  If we have additional kinds of thread groups, they might not 
assign their own specific ids to threads.
  
Simon Marchi March 15, 2017, 9:36 p.m. UTC | #7
On 2017-02-24 15:22, Simon Marchi wrote:
> While writing the documentation parts, these question popped to mind:
> 
> - The type of the id-in-tg field (a string) assumes that a thread
> belongs to a single group.  Currently, the only kind of "thread-group"
> we have is inferiors.  The thread-group terminology suggests that in
> the future we might have other kinds of thread groups, like
> user-defined arbitrary groups.  Otherwise, why would MI use "thread
> group" instead of "inferior"?  Because of this, do you think that the
> type of the field will be limiting?  For example, should we document
> right that this field can be a list, when a thread is part of multiple
> groups, for example?
> 
> - The format of the id-in-tg field is "1" for inferior/thread-group 1.
>  In -list-thread-groups, the id is shown as "i1".  Again, if there are
> other kinds of groups later, we can assume that we'll need to
> differentiate the type, so just "1" won't work.  For this reason and
> for consistency with the result of -list-thread-groups, should we show
> "i1" here as well?

I thought about this a bit more, and I wonder if the solution is not to 
just use "inferior" here.

In MI, so far, the wording "thread-group" was used instead of inferior.  
An inferior is one kind (and the only one at the moment) of 
thread-group.  Using "thread-group" instead of "inferior" allows to have 
new kinds of thread-groups in the future, and still have commands that 
make sense (e.g. -list-thread-groups, or the --thread-group switch).

However, in this case, the information we want to convey is specifically 
about inferiors, it will never be about other kinds of thread groups (if 
some ever exist).  Since a thread always has exactly one parent inferior 
and exactly one id in this inferior, we don't have the single value vs 
list problem.  So I think it would make sense to use "id-in-inf" or 
instead of "id-in-tg" and "inf-num"/"inf-id" instead of "tg-id".

And I think that also takes care of the format issue, "1" vs "i1".  "i1" 
is the thread-group id.  It is necessary to have the i in case other 
kinds of thread groups appear.  But if the field is "inf-num" or 
"inf-id", then we no longer have the include the i.

Thoughts?

Simon
  
Pedro Alves March 17, 2017, 3:26 p.m. UTC | #8
On 02/24/2017 08:22 PM, Simon Marchi wrote:

> While writing the documentation parts, these question popped to mind:
> 
> - The type of the id-in-tg field (a string) assumes that a thread
> belongs to a single group.  Currently, the only kind of "thread-group"
> we have is inferiors.  The thread-group terminology suggests that in the
> future we might have other kinds of thread groups, like user-defined
> arbitrary groups.  Otherwise, why would MI use "thread group" instead of
> "inferior"?  

The concept of MI thread groups was added way before the concept
of "inferiors" and inferior numbers was exposed to the user:

  https://sourceware.org/ml/gdb-patches/2008-11/msg00044.html

Back then the thread list was global, and we were starting to add
multi-process support to GDB.  The OS we were targeting (from Ericsson,
no less) had all processes running the same code (as if a bunch of forks),
which was convenient as it allowed for incremental progress all the
way until we got to real multi-inferior support a few releases later.  Back
then, MI thread groups mapped to "processes", and thread groups were
only created when we'd attach to a process.

Later on, when we added real GNU/Linux multi-inferior support (with the
addition of program spaces), we exposed the "prototype inferior" concept
to the user (i.e., the inferior now exists before the user types "run"), and
"thread groups" were re-purposed.  See:

  https://sourceware.org/ml/gdb-patches/2010-01/msg00358.html

Thanks,
Pedro Alves
  
Pedro Alves March 17, 2017, 3:27 p.m. UTC | #9
On 03/15/2017 09:36 PM, Simon Marchi wrote:
> On 2017-02-24 15:22, Simon Marchi wrote:
>> While writing the documentation parts, these question popped to mind:
>>
>> - The type of the id-in-tg field (a string) assumes that a thread
>> belongs to a single group.  Currently, the only kind of "thread-group"
>> we have is inferiors.  The thread-group terminology suggests that in
>> the future we might have other kinds of thread groups, like
>> user-defined arbitrary groups.  Otherwise, why would MI use "thread
>> group" instead of "inferior"?  Because of this, do you think that the
>> type of the field will be limiting?  For example, should we document
>> right that this field can be a list, when a thread is part of multiple
>> groups, for example?
>>
>> - The format of the id-in-tg field is "1" for inferior/thread-group 1.
>>  In -list-thread-groups, the id is shown as "i1".  Again, if there are
>> other kinds of groups later, we can assume that we'll need to
>> differentiate the type, so just "1" won't work.  For this reason and
>> for consistency with the result of -list-thread-groups, should we show
>> "i1" here as well?
> 
> I thought about this a bit more, and I wonder if the solution is not to
> just use "inferior" here.
> 
> In MI, so far, the wording "thread-group" was used instead of inferior. 
> An inferior is one kind (and the only one at the moment) of
> thread-group.  Using "thread-group" instead of "inferior" allows to have
> new kinds of thread-groups in the future, and still have commands that
> make sense (e.g. -list-thread-groups, or the --thread-group switch).
> 
> However, in this case, the information we want to convey is specifically
> about inferiors, it will never be about other kinds of thread groups (if
> some ever exist).  Since a thread always has exactly one parent inferior
> and exactly one id in this inferior, we don't have the single value vs
> list problem.  So I think it would make sense to use "id-in-inf" or
> instead of "id-in-tg" and "inf-num"/"inf-id" instead of "tg-id".
> 
> And I think that also takes care of the format issue, "1" vs "i1".  "i1"
> is the thread-group id.  It is necessary to have the i in case other
> kinds of thread groups appear.  But if the field is "inf-num" or
> "inf-id", then we no longer have the include the i.
> 
> Thoughts?

Makes sense to me.

Thanks,
Pedro Alves
  

Patch

diff --git a/gdb/testsuite/gdb.mi/mi-qualified-thread-id.c b/gdb/testsuite/gdb.mi/mi-qualified-thread-id.c
new file mode 100644
index 0000000000..cb50e50c32
--- /dev/null
+++ b/gdb/testsuite/gdb.mi/mi-qualified-thread-id.c
@@ -0,0 +1,57 @@ 
+/* Copyright 2017 Free Software Foundation, Inc.
+
+   This file is part of GDB.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#include <pthread.h>
+
+static pthread_barrier_t barrier;
+
+static void *
+thread_func (void *arg)
+{
+  /* Notify the main thread that this thread has started.  */
+  pthread_barrier_wait (&barrier);
+
+  /* Wait until main gives us the signal that we can quit.  */
+  pthread_barrier_wait (&barrier);
+  return NULL;
+}
+
+static void
+thread_started ()
+{
+}
+
+int main ()
+{
+  pthread_t thread;
+
+  pthread_barrier_init (&barrier, NULL, 2);
+
+  pthread_create (&thread, NULL, thread_func, NULL);
+
+  /* Wait until the thread has started.  */
+  pthread_barrier_wait (&barrier);
+
+  thread_started ();
+
+  /* Tell the thread that it can quit.  */
+  pthread_barrier_wait (&barrier);
+
+  pthread_join (thread, NULL);
+
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.mi/mi-qualified-thread-id.exp b/gdb/testsuite/gdb.mi/mi-qualified-thread-id.exp
new file mode 100644
index 0000000000..43c1409786
--- /dev/null
+++ b/gdb/testsuite/gdb.mi/mi-qualified-thread-id.exp
@@ -0,0 +1,76 @@ 
+# Copyright 2017 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+load_lib mi-support.exp
+set MIFLAGS "-i=mi"
+
+standard_testfile
+
+if {[use_gdb_stub]} {
+    # There is no point in testing inferior-qualified thread ids if we can't
+    # have multiple inferiors.
+    untested "multi-inferior not supported"
+    return
+}
+
+if  { [gdb_compile_pthreads "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable {debug}] != "" } {
+     untested ${testfile}
+     return -1
+}
+
+# Prepare the inferiors.  Start and run them until the "thread_started" function.
+
+proc setup { } {
+    global binfile
+    global srcdir
+    global subdir
+    global srcfile
+
+    gdb_exit
+    mi_gdb_start
+    mi_gdb_reinitialize_dir $srcdir/$subdir
+    mi_gdb_load ${binfile}
+
+    mi_create_breakpoint "thread_started" "insert breakpoint at thread_started"
+
+    # Start inferior 1
+    mi_run_cmd
+    mi_expect_stop "breakpoint-hit" "thread_started" ".*" "${srcfile}" ".*" {"" "disp=\"keep\""} "inferior 1 stops"
+
+    # Add and start inferior 2
+    mi_gdb_test "add-inferior -exec ${binfile}" ".*=thread-group-added,id=\"i2\".*" "add second inferior"
+    mi_gdb_test "inferior 2" ".*" "switch to inferior 2"
+    mi_run_cmd
+    mi_expect_stop "breakpoint-hit" "thread_started" ".*" "${srcfile}" ".*" {"" "disp=\"keep\""} "inferior 2 stops"
+}
+
+proc test_thread_info_qualified_thread_id { } {
+    setup
+
+    set expected [join \
+	{ "111\\^done,threads=.*" \
+	  "tg-id=\"1\",id-in-tg=\"1\",id=\"1\".*" \
+	  "tg-id=\"1\",id-in-tg=\"2\",id=\"2\".*" \
+	  "tg-id=\"2\",id-in-tg=\"1\",id=\"3\".*" \
+	  "tg-id=\"2\",id-in-tg=\"2\",id=\"4\".*" \
+	} ""]
+
+    mi_gdb_test \
+	"111-thread-info" \
+	 $expected \
+	"thread ids in -thread-info"
+}
+
+test_thread_info_qualified_thread_id
diff --git a/gdb/thread.c b/gdb/thread.c
index e45b25750e..29cfa61e13 100644
--- a/gdb/thread.c
+++ b/gdb/thread.c
@@ -1247,7 +1247,7 @@  print_thread_info_1 (struct ui_out *uiout, char *requested_threads,
       uiout->table_header (1, ui_left, "current", "");
 
       if (!uiout->is_mi_like_p ())
-	uiout->table_header (4, ui_left, "id-in-tg", "Id");
+	uiout->table_header (4, ui_left, "qualified-id", "Id");
       if (show_global_ids || uiout->is_mi_like_p ())
 	uiout->table_header (4, ui_left, "id", "GId");
       uiout->table_header (17, ui_left, "target-id", "Target Id");
@@ -1282,8 +1282,13 @@  print_thread_info_1 (struct ui_out *uiout, char *requested_threads,
 	    uiout->field_skip ("current");
 	}
 
-      if (!uiout->is_mi_like_p ())
-	uiout->field_string ("id-in-tg", print_thread_id (tp));
+      if (uiout->is_mi_like_p ())
+	{
+	  uiout->field_int ("tg-id", tp->inf->num);
+	  uiout->field_int ("id-in-tg", tp->per_inf_num);
+	}
+      else
+	uiout->field_string ("qualified-id", print_thread_id (tp));
 
       if (show_global_ids || uiout->is_mi_like_p ())
 	uiout->field_int ("id", tp->global_num);