[binutils-gdb] gdb: don't duplicate 'thread' field in MI breakpoint output

[binutils-gdb] gdb: don't duplicate 'thread' field in MI breakpoint output

[Andrew Burgess]

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=2fd9a436c8d24eb0af85ccb3a2fbdf9a9c679a6c

commit 2fd9a436c8d24eb0af85ccb3a2fbdf9a9c679a6c
Author: Andrew Burgess <aburgess@redhat.com>
Date:   Fri Feb 17 10:48:06 2023 +0000

    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 way 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.
    
    Reviewed-By: Eli Zaretskii <eliz@gnu.org>
    Approved-By: Pedro Alves <pedro@palves.net>

Diff:
---
 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/testsuite/gdb.mi/mi-thread-specific-bp.exp | 49 ++++++++++++++++++++++
 gdb/testsuite/lib/mi-support.exp               | 58 ++++++++++++++++++--------
 7 files changed, 145 insertions(+), 20 deletions(-)

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 7d11f7199fe..36c53e5dae6 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..3e20afa7049 100644
--- a/gdb/testsuite/lib/mi-support.exp
+++ b/gdb/testsuite/lib/mi-support.exp
@@ -2498,8 +2498,11 @@ proc mi_build_kv_pairs {attr_list {joiner ,}} {
 # locations.
 #
 # All arguments for the breakpoint location may be specified using the
-# options number, enabled, addr, func, file, fullname, line and
-# thread-groups.
+# options: number, enabled, addr, func, file, fullname, line,
+# thread-groups, and thread.
+#
+# For the option -thread the corresponding output field is only added
+# if the option is present and not set to the empty string.
 #
 # Example: mi_make_breakpoint_loc -number 2.1 -file ".*/myfile.c" -line 3
 # will return the breakpoint location:
@@ -2509,7 +2512,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 ""}}
 
     set attr_list {}
     foreach attr [list number enabled addr func file \
@@ -2517,18 +2520,31 @@ 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 COND is not preset, do not output it.
+    # If THREAD is not present, do not output it.
+    if {[string length $thread] > 0} {
+	append result ","
+	append result [mi_build_kv_pairs [list "thread" $thread]]
+    }
+
+    # If COND is not present, do not output it.
     if {[string length $cond] > 0} {
 	append result ","
 	append result [mi_build_kv_pairs [list "cond" $cond]]
@@ -2574,13 +2590,17 @@ proc mi_make_breakpoint_1 {attr_list cond evaluated-by times \
 # -dprintf-insert, or -break-info with breakpoints with multiple
 # locations.
 #
-# All arguments for the breakpoint may be specified using the options
-# number, type, disp, enabled, func, cond, evaluated-by, times,
-# ignore, script and locations.
+# All arguments for the breakpoint may be specified using the options:
+# number, type, disp, enabled, times, ignore, script,
+# original-location, cond, evaluated-by, locations, and thread.
 #
 # Only if -script and -ignore are given will they appear in the output.
 # Otherwise, this procedure will skip them using ".*".
 #
+# For the options -thread and -cond the corresponding output fields
+# are only added if the options are present and not set to the empty
+# string.
+#
 # Example: mi_make_breakpoint_multi -number 2 -locations "$loc"
 # will return the breakpoint:
 # bkpt={number="2",type=".*",disp=".*",enabled=".*",addr="<MULTIPLE>",
@@ -2592,7 +2612,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 ""}}
 
     set attr_list {}
     foreach attr [list number type disp enabled] {
@@ -2602,7 +2622,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 ","
@@ -2616,8 +2636,11 @@ proc mi_make_breakpoint_multi {args} {
 # be used to test the output of -break-insert, -dprintf-insert, or
 # -break-info for pending breakpoints.
 #
-# Arguments for the breakpoint may be specified using the options
-# number, type, disp, enabled, pending.
+# Arguments for the breakpoint may be specified using the options:
+# number, type, disp, enabled, pending, original-location, and thread.
+#
+# For the option -thread the corresponding output field is only
+# included if the option is present and not the empty string.
 #
 # Example: mi_make_breakpoint_pending -number 2 -pending func
 # will return the breakpoint:
@@ -2626,7 +2649,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 ""}}
 
     set attr_list {}
     foreach attr [list number type disp enabled] {
@@ -2646,7 +2669,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 +2697,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 +2707,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 "\\\}"