[2/8] gdb: don't duplicate 'thread' field in MI breakpoint output

  When creating a thread-specific breakpoint with a single location, the
'thread' field would be repeated in the MI output.  This can be seen
in two existing tests gdb.mi/mi-nsmoribund.exp and
gdb.mi/mi-pending.exp, e.g.:

  (gdb)
  -break-insert -p 1 bar
  ^done,bkpt={number="1",type="breakpoint",disp="keep",
	      enabled="y",
	      addr="0x000000000040110a",func="bar",
	      file="/tmp/mi-thread-specific-bp.c",
	      fullname="/tmp/mi-thread-specific-bp.c",
	      line="32",thread-groups=["i1"],
	      thread="1",thread="1",  <================ DUPLICATION!
	      times="0",original-location="bar"}

I know we need to be careful when adjusting MI output, but I'm hopeful
in this case, as the field is duplicated, and the field contents are
always identical, that we might get away with removing one of the
duplicates.

The change in GDB is a fairly trivial condition change.

We did have a couple of tests that contained the duplicate fields in
their expected output, but given there was no comment pointing out
this oddity either in the GDB code, or in the test, I suspect this was
more a case of copying whatever output GDB produced and using that as
the expected results.  I've updated these tests to remove the
duplication.

I've update lib/mi-support.exp to provide support for building
breakpoint patterns that contain the thread field, and I've made use
of this in a new test I've added that is just about creating
thread-specific breakpoints and checking the results.  The two tests I
mentioned above as being updated could also use the new
lib/mi-support.exp functionality, but I'm going to do that in a later
patch, this was it is clear what changes I'm actually proposing to
make to the expected output.

As I said, I hope that frontends will be able to handle this change,
but I still think its worth adding a NEWS entry, that way, if someone
runs into problems, there's a chance they can figure out what's going
on.

This should not impact CLI output at all.
---
 gdb/NEWS                                      |  8 +++
 gdb/breakpoint.c                              |  2 +-
 gdb/testsuite/gdb.mi/mi-nsmoribund.exp        |  2 +-
 gdb/testsuite/gdb.mi/mi-pending.exp           |  2 +-
 gdb/testsuite/gdb.mi/mi-thread-specific-bp.c  | 44 +++++++++++++++++
 .../gdb.mi/mi-thread-specific-bp.exp          | 49 +++++++++++++++++++
 gdb/testsuite/lib/mi-support.exp              | 32 ++++++++----
 7 files changed, 127 insertions(+), 12 deletions(-)
 create mode 100644 gdb/testsuite/gdb.mi/mi-thread-specific-bp.c
 create mode 100644 gdb/testsuite/gdb.mi/mi-thread-specific-bp.exp
  

> Cc: Andrew Burgess <aburgess@redhat.com>
> Date: Mon, 20 Feb 2023 14:13:47 +0000
> From: Andrew Burgess via Gdb-patches <gdb-patches@sourceware.org>
> 
> When creating a thread-specific breakpoint with a single location, the
> 'thread' field would be repeated in the MI output.  This can be seen
> in two existing tests gdb.mi/mi-nsmoribund.exp and
> gdb.mi/mi-pending.exp, e.g.:
> 
>   (gdb)
>   -break-insert -p 1 bar
>   ^done,bkpt={number="1",type="breakpoint",disp="keep",
> 	      enabled="y",
> 	      addr="0x000000000040110a",func="bar",
> 	      file="/tmp/mi-thread-specific-bp.c",
> 	      fullname="/tmp/mi-thread-specific-bp.c",
> 	      line="32",thread-groups=["i1"],
> 	      thread="1",thread="1",  <================ DUPLICATION!
> 	      times="0",original-location="bar"}
> 
> I know we need to be careful when adjusting MI output, but I'm hopeful
> in this case, as the field is duplicated, and the field contents are
> always identical, that we might get away with removing one of the
> duplicates.
> 
> The change in GDB is a fairly trivial condition change.
> 
> We did have a couple of tests that contained the duplicate fields in
> their expected output, but given there was no comment pointing out
> this oddity either in the GDB code, or in the test, I suspect this was
> more a case of copying whatever output GDB produced and using that as
> the expected results.  I've updated these tests to remove the
> duplication.
> 
> I've update lib/mi-support.exp to provide support for building
> breakpoint patterns that contain the thread field, and I've made use
> of this in a new test I've added that is just about creating
> thread-specific breakpoints and checking the results.  The two tests I
> mentioned above as being updated could also use the new
> lib/mi-support.exp functionality, but I'm going to do that in a later
> patch, this was it is clear what changes I'm actually proposing to
> make to the expected output.
> 
> As I said, I hope that frontends will be able to handle this change,
> but I still think its worth adding a NEWS entry, that way, if someone
> runs into problems, there's a chance they can figure out what's going
> on.
> 
> This should not impact CLI output at all.
> ---
>  gdb/NEWS                                      |  8 +++
>  gdb/breakpoint.c                              |  2 +-
>  gdb/testsuite/gdb.mi/mi-nsmoribund.exp        |  2 +-
>  gdb/testsuite/gdb.mi/mi-pending.exp           |  2 +-
>  gdb/testsuite/gdb.mi/mi-thread-specific-bp.c  | 44 +++++++++++++++++
>  .../gdb.mi/mi-thread-specific-bp.exp          | 49 +++++++++++++++++++
>  gdb/testsuite/lib/mi-support.exp              | 32 ++++++++----
>  7 files changed, 127 insertions(+), 12 deletions(-)
>  create mode 100644 gdb/testsuite/gdb.mi/mi-thread-specific-bp.c
>  create mode 100644 gdb/testsuite/gdb.mi/mi-thread-specific-bp.exp

Thanks, the NEWS part is OK.

Reviewed-By: Eli Zaretskii <eliz@gnu.org>
  

Hi,

On 2023-02-20 2:13 p.m., Andrew Burgess via Gdb-patches wrote:
> When creating a thread-specific breakpoint with a single location, the
> 'thread' field would be repeated in the MI output.  This can be seen
> in two existing tests gdb.mi/mi-nsmoribund.exp and
> gdb.mi/mi-pending.exp, e.g.:
> 
>   (gdb)
>   -break-insert -p 1 bar
>   ^done,bkpt={number="1",type="breakpoint",disp="keep",
> 	      enabled="y",
> 	      addr="0x000000000040110a",func="bar",
> 	      file="/tmp/mi-thread-specific-bp.c",
> 	      fullname="/tmp/mi-thread-specific-bp.c",
> 	      line="32",thread-groups=["i1"],
> 	      thread="1",thread="1",  <================ DUPLICATION!
> 	      times="0",original-location="bar"}
> 
> I know we need to be careful when adjusting MI output, but I'm hopeful
> in this case, as the field is duplicated, and the field contents are
> always identical, that we might get away with removing one of the
> duplicates.
> 
> The change in GDB is a fairly trivial condition change.
> 
> We did have a couple of tests that contained the duplicate fields in
> their expected output, but given there was no comment pointing out
> this oddity either in the GDB code, or in the test, I suspect this was
> more a case of copying whatever output GDB produced and using that as
> the expected results.  I've updated these tests to remove the
> duplication.
> 
> I've update lib/mi-support.exp to provide support for building
> breakpoint patterns that contain the thread field, and I've made use
> of this in a new test I've added that is just about creating
> thread-specific breakpoints and checking the results.  The two tests I
> mentioned above as being updated could also use the new
> lib/mi-support.exp functionality, but I'm going to do that in a later
> patch, this was it is clear what changes I'm actually proposing to

"this was" -> "this way"

> make to the expected output.
> 
> As I said, I hope that frontends will be able to handle this change,
> but I still think its worth adding a NEWS entry, that way, if someone
> runs into problems, there's a chance they can figure out what's going
> on.
> 
> This should not impact CLI output at all.

Just a few nits below.  Otherwise:

 Approved-By: Pedro Alves <pedro@palves.net>

> ---
>  gdb/NEWS                                      |  8 +++
>  gdb/breakpoint.c                              |  2 +-
>  gdb/testsuite/gdb.mi/mi-nsmoribund.exp        |  2 +-
>  gdb/testsuite/gdb.mi/mi-pending.exp           |  2 +-
>  gdb/testsuite/gdb.mi/mi-thread-specific-bp.c  | 44 +++++++++++++++++
>  .../gdb.mi/mi-thread-specific-bp.exp          | 49 +++++++++++++++++++
>  gdb/testsuite/lib/mi-support.exp              | 32 ++++++++----
>  7 files changed, 127 insertions(+), 12 deletions(-)
>  create mode 100644 gdb/testsuite/gdb.mi/mi-thread-specific-bp.c
>  create mode 100644 gdb/testsuite/gdb.mi/mi-thread-specific-bp.exp
> 
> diff --git a/gdb/NEWS b/gdb/NEWS
> index 75cd11b204e..bea604d7e75 100644
> --- a/gdb/NEWS
> +++ b/gdb/NEWS
> @@ -83,6 +83,14 @@ maintenance info frame-unwinders
>  ** mi now reports 'no-history' as a stop reason when hitting the end of the
>     reverse execution history.
>  
> +** When creating a thread-specific breakpoint using the '-p' option,
> +   the -break-insert command would report the 'thread' field twice in
> +   the reply.  The content of both fields was always identical.  This
> +   has now been fixed; the 'thread' field will be reported just once
> +   for thread-specific breakpoints, or not at all for breakpoints
> +   without a thread restriction.  The same is also true for the 'task'
> +   field of an Ada task-specific breakpoint.
> +
>  *** Changes in GDB 13
>  
>  * MI version 1 is deprecated, and will be removed in GDB 14.
> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
> index 902119c7d2c..f3121c8d6eb 100644
> --- a/gdb/breakpoint.c
> +++ b/gdb/breakpoint.c
> @@ -6480,7 +6480,7 @@ print_one_breakpoint_location (struct breakpoint *b,
>       For the CLI output, the thread/task information is printed on a
>       separate line, see the 'stop only in thread' and 'stop only in task'
>       output below.  */
> -  if (!header_of_multiple && uiout->is_mi_like_p ())
> +  if (part_of_multiple && uiout->is_mi_like_p ())
>      {
>        if (b->thread != -1)
>  	uiout->field_signed ("thread", b->thread);
> diff --git a/gdb/testsuite/gdb.mi/mi-nsmoribund.exp b/gdb/testsuite/gdb.mi/mi-nsmoribund.exp
> index 103aa45d7e1..55450e4621a 100644
> --- a/gdb/testsuite/gdb.mi/mi-nsmoribund.exp
> +++ b/gdb/testsuite/gdb.mi/mi-nsmoribund.exp
> @@ -74,7 +74,7 @@ mi_delete_breakpoints
>  
>  # Recreate the same breakpoint, but this time, specific to thread 5.
>  mi_gdb_test "234-break-insert -p 5 $srcfile:$bkpt_line" \
> -    "234\\^done,bkpt=\{number=\"3\",type=\"breakpoint\",disp=\"keep\",enabled=\"y\",addr=\".*\",func=\"thread_function\",file=\".*\",fullname=\".*\",line=\".*\",thread-groups=\\\[\".*\"\\\],thread=\"5\",thread=\"5\",times=\"0\",original-location=\".*\"\}" \
> +    "234\\^done,bkpt=\{number=\"3\",type=\"breakpoint\",disp=\"keep\",enabled=\"y\",addr=\".*\",func=\"thread_function\",file=\".*\",fullname=\".*\",line=\".*\",thread-groups=\\\[\".*\"\\\],thread=\"5\",times=\"0\",original-location=\".*\"\}" \
>      "thread specific breakpoint at thread_function" 
>  
>  # Resume all threads.  Only thread 5 should report a stop.
> diff --git a/gdb/testsuite/gdb.mi/mi-pending.exp b/gdb/testsuite/gdb.mi/mi-pending.exp
> index 153efdf45ce..cd1301c21e3 100644
> --- a/gdb/testsuite/gdb.mi/mi-pending.exp
> +++ b/gdb/testsuite/gdb.mi/mi-pending.exp
> @@ -98,7 +98,7 @@ mi_gdb_test "-break-delete 3" "\\^done" "delete breakpoint 3"
>  
>  # Set pending breakpoint with a thread via MI.
>  mi_gdb_test "-break-insert -p 2 -f pendfunc3" \
> -    ".*\\^done,bkpt=\{number=\"4\",type=\"breakpoint\",disp=\"keep\",enabled=\"y\",addr=\"<PENDING>\",pending=\"pendfunc3\",thread=\"2\",thread=\"2\",times=\"0\",original-location=\"pendfunc3\"\}"\
> +    ".*\\^done,bkpt=\{number=\"4\",type=\"breakpoint\",disp=\"keep\",enabled=\"y\",addr=\"<PENDING>\",pending=\"pendfunc3\",thread=\"2\",times=\"0\",original-location=\"pendfunc3\"\}"\
>      "MI pending breakpoint on pendfunc3"
>  
>  mi_send_resuming_command "exec-continue" "continuing execution to thread condition"
> diff --git a/gdb/testsuite/gdb.mi/mi-thread-specific-bp.c b/gdb/testsuite/gdb.mi/mi-thread-specific-bp.c
> new file mode 100644
> index 00000000000..8c87f01f14b
> --- /dev/null
> +++ b/gdb/testsuite/gdb.mi/mi-thread-specific-bp.c
> @@ -0,0 +1,44 @@
> +/* This testcase is part of GDB, the GNU debugger.
> +
> +   Copyright 2022-2023 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/>.  */
> +
> +volatile int global_var = 0;
> +
> +__attribute__((__always_inline__)) static inline void
> +foo (void)
> +{
> +  int i;
> +
> +  for (i = 0; i < 10; ++i)
> +    global_var = i;
> +}
> +
> +__attribute__((__noinline__)) static void
> +bar (void)
> +{
> +  global_var = 0;
> +  foo ();
> +}
> +
> +int
> +main (void)
> +{
> +  global_var = 0;
> +  foo ();
> +  bar ();
> +  foo ();
> +  return 0;
> +}
> diff --git a/gdb/testsuite/gdb.mi/mi-thread-specific-bp.exp b/gdb/testsuite/gdb.mi/mi-thread-specific-bp.exp
> new file mode 100644
> index 00000000000..4586fa44bbc
> --- /dev/null
> +++ b/gdb/testsuite/gdb.mi/mi-thread-specific-bp.exp
> @@ -0,0 +1,49 @@
> +# Copyright 2023 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/>.
> +
> +# This test is for creating thread-specific breakpoint using the MI,
> +# and checking the results from GDB.
> +
> +load_lib mi-support.exp
> +set MIFLAGS "-i=mi"
> +
> +if {[mi_clean_restart]} {
> +    return
> +}
> +
> +standard_testfile
> +
> +if [build_executable ${testfile}.exp ${binfile} ${srcfile}] {
> +    return -1
> +}
> +
> +if {[mi_clean_restart $binfile]} {
> +    return -1
> +}
> +
> +mi_create_breakpoint "-p 1 bar" "thread-specific b/p on bar" \
> +    -thread "1"
> +
> +proc make_loc {num} {
> +    return [mi_make_breakpoint_loc -thread "1" -number "$::decimal\\.$num"]
> +}
> +
> +set loc1 [make_loc 1]
> +set loc2 [make_loc 2]
> +set loc3 [make_loc 3]
> +
> +mi_create_breakpoint_multi "-p 1 foo" "thread-specific b/p on foo" \
> +    -thread "1" \
> +    -locations "\\\[$loc1,$loc2,$loc3\\\]"
> diff --git a/gdb/testsuite/lib/mi-support.exp b/gdb/testsuite/lib/mi-support.exp
> index 767ea72ff70..0963701140e 100644
> --- a/gdb/testsuite/lib/mi-support.exp
> +++ b/gdb/testsuite/lib/mi-support.exp
> @@ -2509,7 +2509,7 @@ proc mi_build_kv_pairs {attr_list {joiner ,}} {
>  proc mi_make_breakpoint_loc {args} {
>      parse_args {{number .*} {enabled .*} {addr .*}
>  	{func .*} {file .*} {fullname .*} {line .*}
> -	{thread-groups \\\[.*\\\]}}
> +	{thread-groups \\\[.*\\\]} {thread ""}}

The proc's leading comment needs to be updated as well.  Currently it reads:

 # All arguments for the breakpoint location may be specified using the
 # options number, enabled, addr, func, file, fullname, line and
 # thread-groups.

>  
>      set attr_list {}
>      foreach attr [list number enabled addr func file \
> @@ -2517,17 +2517,30 @@ proc mi_make_breakpoint_loc {args} {
>  	lappend attr_list $attr [set $attr]
>      }
>  
> -    return "{[mi_build_kv_pairs $attr_list]}"
> +    set result [mi_build_kv_pairs $attr_list]
> +
> +    if {[string length $thread] > 0} {
> +	append result ","
> +	append result [mi_build_kv_pairs [list "thread" $thread]]
> +    }
> +
> +    return "{$result}"
>  }
>  
>  # Bits shared between mi_make_breakpoint and mi_make_breakpoint_multi.
>  
> -proc mi_make_breakpoint_1 {attr_list cond evaluated-by times \
> +proc mi_make_breakpoint_1 {attr_list thread cond evaluated-by times \
>  			   ignore script original-location} {
>      set result "bkpt=\\\{[mi_build_kv_pairs $attr_list]"
>  
>      # There are always exceptions.
>  
> +    # If THREAD is not preset, do not output it.

Did you mean "present" instead of "preset" ?

> +    if {[string length $thread] > 0} {
> +	append result ","
> +	append result [mi_build_kv_pairs [list "thread" $thread]]
> +    }
> +
>      # If COND is not preset, do not output it.

Ah, I guess you copied this.  IMO, it's a typo here too.  Note further
below we have:

    # If SCRIPT and IGNORE are not present, do not output them.
                                   ^^^^^^^


>      if {[string length $cond] > 0} {
>  	append result ","
> @@ -2592,7 +2605,7 @@ proc mi_make_breakpoint_multi {args} {
>      parse_args {{number .*} {type .*} {disp .*} {enabled .*}
>  	{times .*} {ignore 0}
>  	{script ""} {original-location .*} {cond ""} {evaluated-by ""}
> -	{locations .*}}
> +	{locations .*} {thread ""}}

Needs intro comment update for this too.

>  
>      set attr_list {}
>      foreach attr [list number type disp enabled] {
> @@ -2602,7 +2615,7 @@ proc mi_make_breakpoint_multi {args} {
>      lappend attr_list "addr" "<MULTIPLE>"
>  
>      set result [mi_make_breakpoint_1 \
> -		    $attr_list $cond ${evaluated-by} $times \
> +		    $attr_list $thread $cond ${evaluated-by} $times \
>  		    $ignore $script ${original-location}]
>  
>      append result ","
> @@ -2626,7 +2639,7 @@ proc mi_make_breakpoint_multi {args} {
>  
>  proc mi_make_breakpoint_pending {args} {
>      parse_args {{number .*} {type .*} {disp .*} {enabled .*}
> -	{pending .*} {original-location .*}}
> +	{pending .*} {original-location .*} {thread ""}}

Ditto.

Pedro Alves

>  
>      set attr_list {}
>      foreach attr [list number type disp enabled] {
> @@ -2646,7 +2659,7 @@ proc mi_make_breakpoint_pending {args} {
>      set evaluated-by ""
>  
>      set result [mi_make_breakpoint_1 \
> -		    $attr_list $cond ${evaluated-by} $times \
> +		    $attr_list $thread $cond ${evaluated-by} $times \
>  		    $ignore $script ${original-location}]
>  
>      append result "\\\}"
> @@ -2674,7 +2687,8 @@ proc mi_make_breakpoint {args} {
>      parse_args {{number .*} {type .*} {disp .*} {enabled .*} {addr .*}
>  	{func .*} {file .*} {fullname .*} {line .*}
>  	{thread-groups \\\[.*\\\]} {times .*} {ignore 0}
> -	{script ""} {original-location .*} {cond ""} {evaluated-by ""}}
> +	{script ""} {original-location .*} {cond ""} {evaluated-by ""}
> +	{thread ""}}
>  
>      set attr_list {}
>      foreach attr [list number type disp enabled addr func file \
> @@ -2683,7 +2697,7 @@ proc mi_make_breakpoint {args} {
>      }
>  
>      set result [mi_make_breakpoint_1 \
> -		    $attr_list $cond ${evaluated-by} $times \
> +		    $attr_list $thread $cond ${evaluated-by} $times \
>  		    $ignore $script ${original-location}]
>  
>      append result "\\\}"
>