* [PATCH 1/8] gdb: remove an out of date comment about disp_del_at_next_stop
2023-02-20 14:13 [PATCH 0/8] Fix missing MI =breakpoint-deleted notifications Andrew Burgess
@ 2023-02-20 14:13 ` Andrew Burgess
2023-02-20 14:13 ` [PATCH 2/8] gdb: don't duplicate 'thread' field in MI breakpoint output Andrew Burgess
` (7 subsequent siblings)
8 siblings, 0 replies; 19+ messages in thread
From: Andrew Burgess @ 2023-02-20 14:13 UTC (permalink / raw)
To: gdb-patches; +Cc: Andrew Burgess
Delete an out of date comment about disp_del_at_next_stop, the comment
says:
/* NOTE drow/2003-09-08: This state only exists for removing
watchpoints. It's not clear that it's necessary... */
I'm sure this was true when the comment was added, but today the
disp_del_at_next_stop state is not just used for deleting watchpoints,
which leaves us with "It's not clear that it's necessary...", which
doesn't really help at all.
And then this comment is located on one random place where
disp_del_at_next_stop is used, rather than at its definition site.
Lets just delete the comment.
No user visible changes after this commit.
---
gdb/breakpoint.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 0db3adaf916..902119c7d2c 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -2938,8 +2938,6 @@ insert_bp_location (struct bp_location *bl,
}
else if (bl->loc_type == bp_loc_hardware_watchpoint
- /* NOTE drow/2003-09-08: This state only exists for removing
- watchpoints. It's not clear that it's necessary... */
&& bl->owner->disposition != disp_del_at_next_stop)
{
int val;
--
2.25.4
^ permalink raw reply [flat|nested] 19+ messages in thread* [PATCH 2/8] gdb: don't duplicate 'thread' field in MI breakpoint output
2023-02-20 14:13 [PATCH 0/8] Fix missing MI =breakpoint-deleted notifications Andrew Burgess
2023-02-20 14:13 ` [PATCH 1/8] gdb: remove an out of date comment about disp_del_at_next_stop Andrew Burgess
@ 2023-02-20 14:13 ` Andrew Burgess
2023-02-20 14:47 ` Eli Zaretskii
2023-02-22 15:18 ` Pedro Alves
2023-02-20 14:13 ` [PATCH 3/8] gdb/testsuite: make more use of mi-support.exp Andrew Burgess
` (6 subsequent siblings)
8 siblings, 2 replies; 19+ messages in thread
From: Andrew Burgess @ 2023-02-20 14:13 UTC (permalink / raw)
To: gdb-patches; +Cc: Andrew Burgess
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
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 ""}}
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.
+ 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.
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 ""}}
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 ""}}
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 "\\\}"
--
2.25.4
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH 2/8] gdb: don't duplicate 'thread' field in MI breakpoint output
2023-02-20 14:13 ` [PATCH 2/8] gdb: don't duplicate 'thread' field in MI breakpoint output Andrew Burgess
@ 2023-02-20 14:47 ` Eli Zaretskii
2023-02-22 15:18 ` Pedro Alves
1 sibling, 0 replies; 19+ messages in thread
From: Eli Zaretskii @ 2023-02-20 14:47 UTC (permalink / raw)
To: Andrew Burgess; +Cc: gdb-patches
> 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>
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH 2/8] gdb: don't duplicate 'thread' field in MI breakpoint output
2023-02-20 14:13 ` [PATCH 2/8] gdb: don't duplicate 'thread' field in MI breakpoint output Andrew Burgess
2023-02-20 14:47 ` Eli Zaretskii
@ 2023-02-22 15:18 ` Pedro Alves
1 sibling, 0 replies; 19+ messages in thread
From: Pedro Alves @ 2023-02-22 15:18 UTC (permalink / raw)
To: Andrew Burgess, gdb-patches
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 "\\\}"
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 3/8] gdb/testsuite: make more use of mi-support.exp
2023-02-20 14:13 [PATCH 0/8] Fix missing MI =breakpoint-deleted notifications Andrew Burgess
2023-02-20 14:13 ` [PATCH 1/8] gdb: remove an out of date comment about disp_del_at_next_stop Andrew Burgess
2023-02-20 14:13 ` [PATCH 2/8] gdb: don't duplicate 'thread' field in MI breakpoint output Andrew Burgess
@ 2023-02-20 14:13 ` Andrew Burgess
2023-02-20 14:13 ` [PATCH 4/8] gdb/testsuite: extend the use of mi_clean_restart Andrew Burgess
` (5 subsequent siblings)
8 siblings, 0 replies; 19+ messages in thread
From: Andrew Burgess @ 2023-02-20 14:13 UTC (permalink / raw)
To: gdb-patches; +Cc: Andrew Burgess
Building on the previous commit, now that the breakpoint related
support functions in lib/mi-support.exp can now help creating the
patterns for thread specific breakpoints, make use of this
functionality for gdb.mi/mi-nsmoribund.exp and gdb.mi/mi-pending.exp.
There should be no changes in what is tested after this commit.
---
gdb/testsuite/gdb.mi/mi-nsmoribund.exp | 7 ++++---
gdb/testsuite/gdb.mi/mi-pending.exp | 6 ++++--
2 files changed, 8 insertions(+), 5 deletions(-)
diff --git a/gdb/testsuite/gdb.mi/mi-nsmoribund.exp b/gdb/testsuite/gdb.mi/mi-nsmoribund.exp
index 55450e4621a..a08ba8ab2e7 100644
--- a/gdb/testsuite/gdb.mi/mi-nsmoribund.exp
+++ b/gdb/testsuite/gdb.mi/mi-nsmoribund.exp
@@ -73,9 +73,10 @@ mi_gdb_test "-thread-select 5" "\\^done.*" "select thread 5"
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\",times=\"0\",original-location=\".*\"\}" \
- "thread specific breakpoint at thread_function"
+mi_create_breakpoint "-p 5 $srcfile:$bkpt_line" \
+ "thread specific breakpoint at thread_function" \
+ -number "3" -type "breakpoint" -disp "keep" -enabled "y" \
+ -func "thread_function" -line "$bkpt_line" -thread "5" -times "0"
# 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 cd1301c21e3..a5c6ee5c906 100644
--- a/gdb/testsuite/gdb.mi/mi-pending.exp
+++ b/gdb/testsuite/gdb.mi/mi-pending.exp
@@ -97,8 +97,10 @@ mi_expect_stop "breakpoint-hit" "thread_func" ".*" ".*" ".*" \
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\",times=\"0\",original-location=\"pendfunc3\"\}"\
+set bp [mi_make_breakpoint_pending -number "4" -type "breakpoint" \
+ -disp "keep" -enabled "y" -pending "pendfunc3" -thread "2" \
+ -times "0" -original-location "pendfunc3"]
+mi_gdb_test "-break-insert -p 2 -f pendfunc3" ".*\\^done,$bp"\
"MI pending breakpoint on pendfunc3"
mi_send_resuming_command "exec-continue" "continuing execution to thread condition"
--
2.25.4
^ permalink raw reply [flat|nested] 19+ messages in thread* [PATCH 4/8] gdb/testsuite: extend the use of mi_clean_restart
2023-02-20 14:13 [PATCH 0/8] Fix missing MI =breakpoint-deleted notifications Andrew Burgess
` (2 preceding siblings ...)
2023-02-20 14:13 ` [PATCH 3/8] gdb/testsuite: make more use of mi-support.exp Andrew Burgess
@ 2023-02-20 14:13 ` Andrew Burgess
2023-02-20 14:13 ` [PATCH 5/8] gdb/testsuite introduce foreach_mi_ui_mode helper proc Andrew Burgess
` (4 subsequent siblings)
8 siblings, 0 replies; 19+ messages in thread
From: Andrew Burgess @ 2023-02-20 14:13 UTC (permalink / raw)
To: gdb-patches; +Cc: Andrew Burgess
The mi_clean_restart proc calls the mi_gdb_start proc passing no
arguments.
In this commit I add an extra (optional) argument to the
mi_clean_restart proc, and pass this through to mi_gdb_start.
The benefit of this is that we can now use mi_clean_restart when we
also want to pass the 'separate-mi-tty' or 'separate-inferior-tty'
flags to mi_gdb_start, and avoids having to otherwise duplicate the
contents of mi_clean_restart in different tests.
I've updated the obvious places where this new functionality can be
used, and I'm seeing no test regressions.
---
gdb/testsuite/gdb.mi/mi-break.exp | 8 +-------
gdb/testsuite/gdb.mi/mi-exec-run.exp | 1 -
gdb/testsuite/gdb.mi/mi-multi-commands.exp | 3 +--
gdb/testsuite/gdb.mi/mi-watch.exp | 8 +-------
gdb/testsuite/gdb.mi/new-ui-mi-sync.exp | 8 +-------
gdb/testsuite/gdb.mi/user-selected-context-sync.exp | 8 +-------
gdb/testsuite/lib/mi-support.exp | 4 ++--
7 files changed, 7 insertions(+), 33 deletions(-)
diff --git a/gdb/testsuite/gdb.mi/mi-break.exp b/gdb/testsuite/gdb.mi/mi-break.exp
index 122a7dd77a4..5b5d3aad2e4 100644
--- a/gdb/testsuite/gdb.mi/mi-break.exp
+++ b/gdb/testsuite/gdb.mi/mi-break.exp
@@ -393,21 +393,15 @@ proc_with_prefix test_forced_conditions {} {
proc test_break {mi_mode} {
global srcdir subdir binfile
- mi_gdb_exit
-
if {$mi_mode == "separate"} {
set start_ops "separate-mi-tty"
} else {
set start_ops ""
}
- if [mi_gdb_start $start_ops] {
+ if [mi_clean_restart $binfile $start_ops ] {
return
}
- mi_delete_breakpoints
- mi_gdb_reinitialize_dir $srcdir/$subdir
- mi_gdb_load ${binfile}
-
test_tbreak_creation_and_listing
test_ignore_count
diff --git a/gdb/testsuite/gdb.mi/mi-exec-run.exp b/gdb/testsuite/gdb.mi/mi-exec-run.exp
index e3e6d9d6a21..722634a472b 100644
--- a/gdb/testsuite/gdb.mi/mi-exec-run.exp
+++ b/gdb/testsuite/gdb.mi/mi-exec-run.exp
@@ -73,7 +73,6 @@ proc test {inftty_mode mi_mode force_fail} {
mi_delete_breakpoints
mi_gdb_reinitialize_dir $srcdir/$subdir
- mi_gdb_reinitialize_dir $srcdir/$subdir
mi_gdb_load ${bin}
# Useful for debugging:
diff --git a/gdb/testsuite/gdb.mi/mi-multi-commands.exp b/gdb/testsuite/gdb.mi/mi-multi-commands.exp
index 00ab37cb64d..f07130d6fde 100644
--- a/gdb/testsuite/gdb.mi/mi-multi-commands.exp
+++ b/gdb/testsuite/gdb.mi/mi-multi-commands.exp
@@ -38,8 +38,7 @@ proc run_test { args } {
global mi_gdb_prompt
global decimal
- gdb_exit
- if [mi_gdb_start $args] {
+ if [mi_clean_restart "" $args] {
return
}
diff --git a/gdb/testsuite/gdb.mi/mi-watch.exp b/gdb/testsuite/gdb.mi/mi-watch.exp
index 5303a41e86e..aaac7611015 100644
--- a/gdb/testsuite/gdb.mi/mi-watch.exp
+++ b/gdb/testsuite/gdb.mi/mi-watch.exp
@@ -150,14 +150,12 @@ proc test_watchpoint_all {mi_mode type} {
return
}
- mi_gdb_exit
-
if {$mi_mode == "separate"} {
set start_ops "separate-mi-tty"
} else {
set start_ops ""
}
- if [mi_gdb_start $start_ops] {
+ if [mi_clean_restart ${binfile} $start_ops] {
return
}
@@ -170,10 +168,6 @@ proc test_watchpoint_all {mi_mode type} {
"567\\^done" \
"hw watchpoints toggle"
- mi_delete_breakpoints
- mi_gdb_reinitialize_dir $srcdir/$subdir
- mi_gdb_load ${binfile}
-
mi_runto callee4
test_watchpoint_creation_and_listing
#test_rwatch_creation_and_listing
diff --git a/gdb/testsuite/gdb.mi/new-ui-mi-sync.exp b/gdb/testsuite/gdb.mi/new-ui-mi-sync.exp
index 07b316c8ce9..a19bbb9f597 100644
--- a/gdb/testsuite/gdb.mi/new-ui-mi-sync.exp
+++ b/gdb/testsuite/gdb.mi/new-ui-mi-sync.exp
@@ -40,17 +40,11 @@ proc do_test {sync_command} {
global gdb_spawn_id gdb_main_spawn_id mi_spawn_id inferior_spawn_id
global gdb_prompt mi_gdb_prompt
- mi_gdb_exit
-
- if {[mi_gdb_start "separate-mi-tty"] != 0} {
+ if {[mi_clean_restart $binfile "separate-mi-tty"] != 0} {
fail "could not start gdb"
return
}
- mi_delete_breakpoints
- mi_gdb_reinitialize_dir $srcdir/$subdir
- mi_gdb_load $binfile
-
# Start a synchronous run/continue on the MI UI.
set test "send synchronous execution command"
if {$sync_command == "run"} {
diff --git a/gdb/testsuite/gdb.mi/user-selected-context-sync.exp b/gdb/testsuite/gdb.mi/user-selected-context-sync.exp
index 91ca9b16bf8..9bcc90f06b4 100644
--- a/gdb/testsuite/gdb.mi/user-selected-context-sync.exp
+++ b/gdb/testsuite/gdb.mi/user-selected-context-sync.exp
@@ -396,22 +396,16 @@ proc_with_prefix test_setup { mode } {
set any "\[^\r\n\]*"
- mi_gdb_exit
-
save_vars { GDBFLAGS } {
if { $mode == "non-stop" } {
set GDBFLAGS [concat $GDBFLAGS " -ex \"set non-stop 1\""]
}
- if { [mi_gdb_start "separate-mi-tty"] != 0 } {
+ if { [mi_clean_restart $binfile "separate-mi-tty"] != 0 } {
return -1
}
}
- mi_delete_breakpoints
- mi_gdb_reinitialize_dir $srcdir/$subdir
- mi_gdb_load $binfile
-
if { [mi_runto_main] < 0 } {
return -1
}
diff --git a/gdb/testsuite/lib/mi-support.exp b/gdb/testsuite/lib/mi-support.exp
index 0963701140e..1a0dc584a4b 100644
--- a/gdb/testsuite/lib/mi-support.exp
+++ b/gdb/testsuite/lib/mi-support.exp
@@ -1012,7 +1012,7 @@ proc mi_run_with_cli {args} {
# EXECUTABLE is the basename of the binary.
# Return -1 if starting gdb or loading the executable failed.
-proc mi_clean_restart {{executable ""}} {
+proc mi_clean_restart {{executable ""} {flags {}}} {
global srcdir
global subdir
global errcnt
@@ -1024,7 +1024,7 @@ proc mi_clean_restart {{executable ""}} {
set errcnt 0
set warncnt 0
- if {[mi_gdb_start]} {
+ if {[mi_gdb_start $flags]} {
return -1
}
--
2.25.4
^ permalink raw reply [flat|nested] 19+ messages in thread* [PATCH 5/8] gdb/testsuite introduce foreach_mi_ui_mode helper proc
2023-02-20 14:13 [PATCH 0/8] Fix missing MI =breakpoint-deleted notifications Andrew Burgess
` (3 preceding siblings ...)
2023-02-20 14:13 ` [PATCH 4/8] gdb/testsuite: extend the use of mi_clean_restart Andrew Burgess
@ 2023-02-20 14:13 ` Andrew Burgess
2023-02-22 15:18 ` Pedro Alves
2023-02-20 14:13 ` [PATCH 6/8] gdb/testsuite: introduce is_target_non_stop " Andrew Burgess
` (3 subsequent siblings)
8 siblings, 1 reply; 19+ messages in thread
From: Andrew Burgess @ 2023-02-20 14:13 UTC (permalink / raw)
To: gdb-patches; +Cc: Andrew Burgess
Introduce foreach_mi_ui_mode, a helper proc which can be used when
tests are going to be repeated once with the MI in the main UI, and
once with the MI on a separate UI.
The proc is used like this:
foreach_mi_ui_mode VAR {
# BODY
}
The BODY will be run twice, once with VAR set to 'main' and once with
VAR set to 'separate', inside BODY we can then change the behaviour
based on the current UI mode.
The point of this proc is that we sometimes shouldn't run the separate
UI tests (when gdb_debug_enabled is true), and this proc hides all
this logic. If the separate UI mode should not be used then BODY will
be run just once with VAR set to 'main'.
I've updated two tests that can make use of this helper proc. I'm
going to add another similar test in a later commit.
There should be no change to what is tested with this commit.
---
gdb/testsuite/gdb.mi/mi-break.exp | 9 +------
gdb/testsuite/gdb.mi/mi-watch.exp | 9 +------
gdb/testsuite/lib/mi-support.exp | 44 +++++++++++++++++++++++++++++++
3 files changed, 46 insertions(+), 16 deletions(-)
diff --git a/gdb/testsuite/gdb.mi/mi-break.exp b/gdb/testsuite/gdb.mi/mi-break.exp
index 5b5d3aad2e4..3400e2e6f7c 100644
--- a/gdb/testsuite/gdb.mi/mi-break.exp
+++ b/gdb/testsuite/gdb.mi/mi-break.exp
@@ -419,13 +419,6 @@ proc test_break {mi_mode} {
test_forced_conditions
}
-if [gdb_debug_enabled] {
- # gdb debug doesn't work for separate-mi-tty.
- set modes {"main"}
-} else {
- set modes {"main" "separate"}
-}
-
-foreach_with_prefix mi-mode $modes {
+foreach_mi_ui_mode mi-mode {
test_break ${mi-mode}
}
diff --git a/gdb/testsuite/gdb.mi/mi-watch.exp b/gdb/testsuite/gdb.mi/mi-watch.exp
index aaac7611015..12b9781e95a 100644
--- a/gdb/testsuite/gdb.mi/mi-watch.exp
+++ b/gdb/testsuite/gdb.mi/mi-watch.exp
@@ -175,16 +175,9 @@ proc test_watchpoint_all {mi_mode type} {
test_watchpoint_triggering
}
-if [gdb_debug_enabled] {
- # gdb debug doesn't work for separate-mi-tty.
- set modes {"main"}
-} else {
- set modes {"main" "separate"}
-}
-
# Run the tests twice, once using software watchpoints, and another
# with hardware watchpoints.
-foreach_with_prefix mi-mode $modes {
+foreach_mi_ui_mode mi-mode {
foreach_with_prefix wp-type {"sw" "hw"} {
test_watchpoint_all ${mi-mode} ${wp-type}
}
diff --git a/gdb/testsuite/lib/mi-support.exp b/gdb/testsuite/lib/mi-support.exp
index 1a0dc584a4b..2ab751749e9 100644
--- a/gdb/testsuite/lib/mi-support.exp
+++ b/gdb/testsuite/lib/mi-support.exp
@@ -2785,3 +2785,47 @@ proc mi_get_valueof { fmt exp default {test ""} } {
}
return ${val}
}
+
+# Some MI tests should be run in the normal way, on the main UI, while
+# other tests should be run twice, once when the MI is on the main UI,
+# and once with the MI on a secondary UI, this proc facilitates that.
+#
+# Use as:
+#
+# foreach_mi_ui_mode mode {
+# # ... body ...
+# }
+#
+# The BODY with then be run multiple times with MODE set to either
+# 'main' or 'separate'.
+#
+# However, there are times when we know using the 'separate' UI will
+# not work. This proc handles figuring that out, if the 'separate' UI
+# is known not to work then the 'separate' mode will be skipped and
+# only 'main' will be run.
+
+proc foreach_mi_ui_mode { var_name body } {
+ upvar 1 $var_name var
+
+ if [gdb_debug_enabled] {
+ # gdb debug doesn't work for separate-mi-tty.
+ set modes {"main"}
+ } else {
+ set modes {"main" "separate"}
+ }
+
+ foreach var $modes {
+ with_test_prefix "$var_name=$var" {
+ set code [catch {uplevel 1 $body} result]
+ }
+
+ if {$code == 1} {
+ global errorInfo errorCode
+ return -code $code -errorinfo $errorInfo -errorcode $errorCode $result
+ } elseif {$code == 3} {
+ break
+ } elseif {$code == 2} {
+ return -code $code $result
+ }
+ }
+}
--
2.25.4
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH 5/8] gdb/testsuite introduce foreach_mi_ui_mode helper proc
2023-02-20 14:13 ` [PATCH 5/8] gdb/testsuite introduce foreach_mi_ui_mode helper proc Andrew Burgess
@ 2023-02-22 15:18 ` Pedro Alves
0 siblings, 0 replies; 19+ messages in thread
From: Pedro Alves @ 2023-02-22 15:18 UTC (permalink / raw)
To: Andrew Burgess, gdb-patches
On 2023-02-20 2:13 p.m., Andrew Burgess via Gdb-patches wrote:
> Introduce foreach_mi_ui_mode, a helper proc which can be used when
> tests are going to be repeated once with the MI in the main UI, and
> once with the MI on a separate UI.
>
> The proc is used like this:
>
> foreach_mi_ui_mode VAR {
> # BODY
> }
>
> The BODY will be run twice, once with VAR set to 'main' and once with
> VAR set to 'separate', inside BODY we can then change the behaviour
> based on the current UI mode.
>
> The point of this proc is that we sometimes shouldn't run the separate
> UI tests (when gdb_debug_enabled is true), and this proc hides all
> this logic. If the separate UI mode should not be used then BODY will
> be run just once with VAR set to 'main'.
>
> I've updated two tests that can make use of this helper proc. I'm
> going to add another similar test in a later commit.
>
> There should be no change to what is tested with this commit.
> ---
> gdb/testsuite/gdb.mi/mi-break.exp | 9 +------
> gdb/testsuite/gdb.mi/mi-watch.exp | 9 +------
> gdb/testsuite/lib/mi-support.exp | 44 +++++++++++++++++++++++++++++++
> 3 files changed, 46 insertions(+), 16 deletions(-)
>
> diff --git a/gdb/testsuite/gdb.mi/mi-break.exp b/gdb/testsuite/gdb.mi/mi-break.exp
> index 5b5d3aad2e4..3400e2e6f7c 100644
> --- a/gdb/testsuite/gdb.mi/mi-break.exp
> +++ b/gdb/testsuite/gdb.mi/mi-break.exp
> @@ -419,13 +419,6 @@ proc test_break {mi_mode} {
> test_forced_conditions
> }
>
> -if [gdb_debug_enabled] {
> - # gdb debug doesn't work for separate-mi-tty.
> - set modes {"main"}
> -} else {
> - set modes {"main" "separate"}
> -}
> -
> -foreach_with_prefix mi-mode $modes {
> +foreach_mi_ui_mode mi-mode {
> test_break ${mi-mode}
> }
> diff --git a/gdb/testsuite/gdb.mi/mi-watch.exp b/gdb/testsuite/gdb.mi/mi-watch.exp
> index aaac7611015..12b9781e95a 100644
> --- a/gdb/testsuite/gdb.mi/mi-watch.exp
> +++ b/gdb/testsuite/gdb.mi/mi-watch.exp
> @@ -175,16 +175,9 @@ proc test_watchpoint_all {mi_mode type} {
> test_watchpoint_triggering
> }
>
> -if [gdb_debug_enabled] {
> - # gdb debug doesn't work for separate-mi-tty.
> - set modes {"main"}
> -} else {
> - set modes {"main" "separate"}
> -}
> -
> # Run the tests twice, once using software watchpoints, and another
> # with hardware watchpoints.
> -foreach_with_prefix mi-mode $modes {
> +foreach_mi_ui_mode mi-mode {
> foreach_with_prefix wp-type {"sw" "hw"} {
> test_watchpoint_all ${mi-mode} ${wp-type}
> }
> diff --git a/gdb/testsuite/lib/mi-support.exp b/gdb/testsuite/lib/mi-support.exp
> index 1a0dc584a4b..2ab751749e9 100644
> --- a/gdb/testsuite/lib/mi-support.exp
> +++ b/gdb/testsuite/lib/mi-support.exp
> @@ -2785,3 +2785,47 @@ proc mi_get_valueof { fmt exp default {test ""} } {
> }
> return ${val}
> }
> +
> +# Some MI tests should be run in the normal way, on the main UI, while
> +# other tests should be run twice, once when the MI is on the main UI,
> +# and once with the MI on a secondary UI, this proc facilitates that.
> +#
> +# Use as:
> +#
> +# foreach_mi_ui_mode mode {
> +# # ... body ...
> +# }
> +#
> +# The BODY with then be run multiple times with MODE set to either
> +# 'main' or 'separate'.
"with then" -> "will then" ?
"multiple times" gave me pause, like, it reads to me like this is saying that
the body will be run multiple times with mode set to 'main', and then
multiple times set to 'separate'. I think this would be improved by
rephrasing it in way that it is clear the body will run at most once
in each mode.
Otherwise:
Approved-By: Pedro Alves <pedro@palves.net>
Thanks,
Pedro Alves
> +#
> +# However, there are times when we know using the 'separate' UI will
> +# not work. This proc handles figuring that out, if the 'separate' UI
> +# is known not to work then the 'separate' mode will be skipped and
> +# only 'main' will be run.
> +
> +proc foreach_mi_ui_mode { var_name body } {
> + upvar 1 $var_name var
> +
> + if [gdb_debug_enabled] {
> + # gdb debug doesn't work for separate-mi-tty.
> + set modes {"main"}
> + } else {
> + set modes {"main" "separate"}
> + }
> +
> + foreach var $modes {
> + with_test_prefix "$var_name=$var" {
> + set code [catch {uplevel 1 $body} result]
> + }
> +
> + if {$code == 1} {
> + global errorInfo errorCode
> + return -code $code -errorinfo $errorInfo -errorcode $errorCode $result
> + } elseif {$code == 3} {
> + break
> + } elseif {$code == 2} {
> + return -code $code $result
> + }
> + }
> +}
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 6/8] gdb/testsuite: introduce is_target_non_stop helper proc
2023-02-20 14:13 [PATCH 0/8] Fix missing MI =breakpoint-deleted notifications Andrew Burgess
` (4 preceding siblings ...)
2023-02-20 14:13 ` [PATCH 5/8] gdb/testsuite introduce foreach_mi_ui_mode helper proc Andrew Burgess
@ 2023-02-20 14:13 ` Andrew Burgess
2023-02-22 15:19 ` Pedro Alves
2023-02-20 14:13 ` [PATCH 7/8] gdb/testsuite: fix failure in gdb.mi/mi-pending.exp with extended-remote Andrew Burgess
` (2 subsequent siblings)
8 siblings, 1 reply; 19+ messages in thread
From: Andrew Burgess @ 2023-02-20 14:13 UTC (permalink / raw)
To: gdb-patches; +Cc: Andrew Burgess
I noticed that several test included copy & pasted code to run the
'maint show target-non-stop' command, and then switch based on the
result.
In this commit I factor this code out into a helper proc in
lib/gdb.exp, and update all the places I could find that used this
pattern to make use of the helper proc.
There should be no change in what is tested after this commit.
---
gdb/testsuite/gdb.base/access-mem-running.exp | 10 +++-------
gdb/testsuite/gdb.base/fork-running-state.exp | 11 +++--------
.../access-mem-running-thread-exit.exp | 10 +++-------
.../gdb.threads/clone-attach-detach.exp | 11 +++--------
.../gdb.threads/detach-step-over.exp | 8 ++------
gdb/testsuite/lib/gdb.exp | 19 +++++++++++++++++++
6 files changed, 33 insertions(+), 36 deletions(-)
diff --git a/gdb/testsuite/gdb.base/access-mem-running.exp b/gdb/testsuite/gdb.base/access-mem-running.exp
index 90ddedc470d..d0f7871fc0f 100644
--- a/gdb/testsuite/gdb.base/access-mem-running.exp
+++ b/gdb/testsuite/gdb.base/access-mem-running.exp
@@ -46,13 +46,9 @@ proc test { non_stop } {
&& ([target_info gdb_protocol] == "remote"
|| [target_info gdb_protocol] == "extended-remote")} {
- gdb_test_multiple "maint show target-non-stop" "" {
- -wrap -re "(is|currently) on.*" {
- }
- -wrap -re "(is|currently) off.*" {
- unsupported "can't issue commands while target is running"
- return 0
- }
+ if {![is_target_non_stop]} {
+ unsupported "can't issue commands while target is running"
+ return 0
}
}
diff --git a/gdb/testsuite/gdb.base/fork-running-state.exp b/gdb/testsuite/gdb.base/fork-running-state.exp
index b8045ad490f..9a18193c4dc 100644
--- a/gdb/testsuite/gdb.base/fork-running-state.exp
+++ b/gdb/testsuite/gdb.base/fork-running-state.exp
@@ -46,14 +46,9 @@ proc do_test { detach_on_fork follow_fork non_stop schedule_multiple } {
&& ([target_info gdb_protocol] == "remote"
|| [target_info gdb_protocol] == "extended-remote")} {
- set test "maint show target-non-stop"
- gdb_test_multiple "maint show target-non-stop" $test {
- -re "(is|currently) on.*$gdb_prompt $" {
- }
- -re "(is|currently) off.*$gdb_prompt $" {
- unsupported "can't issue info threads while target is running"
- return 0
- }
+ if {![is_target_non_stop]} {
+ unsupported "can't issue info threads while target is running"
+ return 0
}
}
diff --git a/gdb/testsuite/gdb.threads/access-mem-running-thread-exit.exp b/gdb/testsuite/gdb.threads/access-mem-running-thread-exit.exp
index 09b05480c04..2d3ace44ccd 100644
--- a/gdb/testsuite/gdb.threads/access-mem-running-thread-exit.exp
+++ b/gdb/testsuite/gdb.threads/access-mem-running-thread-exit.exp
@@ -65,13 +65,9 @@ proc test { non_stop } {
&& ([target_info gdb_protocol] == "remote"
|| [target_info gdb_protocol] == "extended-remote")} {
- gdb_test_multiple "maint show target-non-stop" "" {
- -wrap -re "(is|currently) on.*" {
- }
- -wrap -re "(is|currently) off.*" {
- unsupported "can't issue commands while target is running"
- return 0
- }
+ if {![is_target_non_stop]} {
+ unsupported "can't issue commands while target is running"
+ return 0
}
}
diff --git a/gdb/testsuite/gdb.threads/clone-attach-detach.exp b/gdb/testsuite/gdb.threads/clone-attach-detach.exp
index ac9b92d5f57..a71713ea8ba 100644
--- a/gdb/testsuite/gdb.threads/clone-attach-detach.exp
+++ b/gdb/testsuite/gdb.threads/clone-attach-detach.exp
@@ -64,14 +64,9 @@ if {[target_info exists gdb_protocol]
&& ([target_info gdb_protocol] == "remote"
|| [target_info gdb_protocol] == "extended-remote")} {
- set test "maint show target-non-stop"
- gdb_test_multiple "maint show target-non-stop" $test {
- -re "(is|currently) on.*$gdb_prompt $" {
- }
- -re "(is|currently) off.*$gdb_prompt $" {
- unsupported "bg attach: can't issue info threads while target is running"
- return 0
- }
+ if {![is_target_non_stop]} {
+ unsupported "bg attach: can't issue info threads while target is running"
+ return 0
}
}
diff --git a/gdb/testsuite/gdb.threads/detach-step-over.exp b/gdb/testsuite/gdb.threads/detach-step-over.exp
index ed9dc1aab88..bf5ef6b06a1 100644
--- a/gdb/testsuite/gdb.threads/detach-step-over.exp
+++ b/gdb/testsuite/gdb.threads/detach-step-over.exp
@@ -314,12 +314,8 @@ proc_with_prefix test_detach_quit {condition_eval target_non_stop \
start_gdb_for_test $condition_eval $target_non_stop \
$non_stop $displaced
- gdb_test_multiple "maint show target-non-stop" "" {
- -wrap -re "(is|currently) on.*" {
- }
- -wrap -re "(is|currently) off.*" {
- return
- }
+ if {![is_target_non_stop]} {
+ return
}
}
diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index e48228ed4f6..ef62174301e 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -9303,6 +9303,25 @@ proc gdb_step_until { regexp {test_name ""} {max_steps 10} } {
}
}
+# Return true if the current target is operating in non-stop mode,
+# otherwise, return false.
+#
+# The inferior will need to have started running in order to get the
+# correct result.
+
+proc is_target_non_stop { {testname ""} } {
+ set is_non_stop false
+ gdb_test_multiple "maint show target-non-stop" $testname {
+ -wrap -re "(is|currently) on.*" {
+ set is_non_stop true
+ }
+ -wrap -re "(is|currently) off.*" {
+ set is_non_stop false
+ }
+ }
+ return $is_non_stop
+}
+
# Check if the compiler emits epilogue information associated
# with the closing brace or with the last statement line.
#
--
2.25.4
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH 6/8] gdb/testsuite: introduce is_target_non_stop helper proc
2023-02-20 14:13 ` [PATCH 6/8] gdb/testsuite: introduce is_target_non_stop " Andrew Burgess
@ 2023-02-22 15:19 ` Pedro Alves
2023-02-27 14:58 ` Andrew Burgess
0 siblings, 1 reply; 19+ messages in thread
From: Pedro Alves @ 2023-02-22 15:19 UTC (permalink / raw)
To: Andrew Burgess, gdb-patches
On 2023-02-20 2:13 p.m., Andrew Burgess via Gdb-patches wrote:
> I noticed that several test included copy & pasted code to run the
"several test" -> "several tests"
> 'maint show target-non-stop' command, and then switch based on the
> result.
>
> In this commit I factor this code out into a helper proc in
> lib/gdb.exp, and update all the places I could find that used this
> pattern to make use of the helper proc.
>
> There should be no change in what is tested after this commit.
> ---
> gdb/testsuite/gdb.base/access-mem-running.exp | 10 +++-------
> gdb/testsuite/gdb.base/fork-running-state.exp | 11 +++--------
> .../access-mem-running-thread-exit.exp | 10 +++-------
> .../gdb.threads/clone-attach-detach.exp | 11 +++--------
> .../gdb.threads/detach-step-over.exp | 8 ++------
> gdb/testsuite/lib/gdb.exp | 19 +++++++++++++++++++
> 6 files changed, 33 insertions(+), 36 deletions(-)
>
> diff --git a/gdb/testsuite/gdb.base/access-mem-running.exp b/gdb/testsuite/gdb.base/access-mem-running.exp
> index 90ddedc470d..d0f7871fc0f 100644
> --- a/gdb/testsuite/gdb.base/access-mem-running.exp
> +++ b/gdb/testsuite/gdb.base/access-mem-running.exp
> @@ -46,13 +46,9 @@ proc test { non_stop } {
> && ([target_info gdb_protocol] == "remote"
> || [target_info gdb_protocol] == "extended-remote")} {
>
> - gdb_test_multiple "maint show target-non-stop" "" {
> - -wrap -re "(is|currently) on.*" {
> - }
> - -wrap -re "(is|currently) off.*" {
> - unsupported "can't issue commands while target is running"
> - return 0
> - }
> + if {![is_target_non_stop]} {
> + unsupported "can't issue commands while target is running"
> + return 0
> }
> }
>
> diff --git a/gdb/testsuite/gdb.base/fork-running-state.exp b/gdb/testsuite/gdb.base/fork-running-state.exp
> index b8045ad490f..9a18193c4dc 100644
> --- a/gdb/testsuite/gdb.base/fork-running-state.exp
> +++ b/gdb/testsuite/gdb.base/fork-running-state.exp
> @@ -46,14 +46,9 @@ proc do_test { detach_on_fork follow_fork non_stop schedule_multiple } {
> && ([target_info gdb_protocol] == "remote"
> || [target_info gdb_protocol] == "extended-remote")} {
>
> - set test "maint show target-non-stop"
> - gdb_test_multiple "maint show target-non-stop" $test {
> - -re "(is|currently) on.*$gdb_prompt $" {
> - }
> - -re "(is|currently) off.*$gdb_prompt $" {
> - unsupported "can't issue info threads while target is running"
> - return 0
> - }
> + if {![is_target_non_stop]} {
> + unsupported "can't issue info threads while target is running"
> + return 0
> }
> }
>
> diff --git a/gdb/testsuite/gdb.threads/access-mem-running-thread-exit.exp b/gdb/testsuite/gdb.threads/access-mem-running-thread-exit.exp
> index 09b05480c04..2d3ace44ccd 100644
> --- a/gdb/testsuite/gdb.threads/access-mem-running-thread-exit.exp
> +++ b/gdb/testsuite/gdb.threads/access-mem-running-thread-exit.exp
> @@ -65,13 +65,9 @@ proc test { non_stop } {
> && ([target_info gdb_protocol] == "remote"
> || [target_info gdb_protocol] == "extended-remote")} {
>
> - gdb_test_multiple "maint show target-non-stop" "" {
> - -wrap -re "(is|currently) on.*" {
> - }
> - -wrap -re "(is|currently) off.*" {
> - unsupported "can't issue commands while target is running"
> - return 0
> - }
> + if {![is_target_non_stop]} {
> + unsupported "can't issue commands while target is running"
> + return 0
> }
> }
>
> diff --git a/gdb/testsuite/gdb.threads/clone-attach-detach.exp b/gdb/testsuite/gdb.threads/clone-attach-detach.exp
> index ac9b92d5f57..a71713ea8ba 100644
> --- a/gdb/testsuite/gdb.threads/clone-attach-detach.exp
> +++ b/gdb/testsuite/gdb.threads/clone-attach-detach.exp
> @@ -64,14 +64,9 @@ if {[target_info exists gdb_protocol]
> && ([target_info gdb_protocol] == "remote"
> || [target_info gdb_protocol] == "extended-remote")} {
>
> - set test "maint show target-non-stop"
> - gdb_test_multiple "maint show target-non-stop" $test {
> - -re "(is|currently) on.*$gdb_prompt $" {
> - }
> - -re "(is|currently) off.*$gdb_prompt $" {
> - unsupported "bg attach: can't issue info threads while target is running"
> - return 0
> - }
> + if {![is_target_non_stop]} {
> + unsupported "bg attach: can't issue info threads while target is running"
> + return 0
> }
> }
>
> diff --git a/gdb/testsuite/gdb.threads/detach-step-over.exp b/gdb/testsuite/gdb.threads/detach-step-over.exp
> index ed9dc1aab88..bf5ef6b06a1 100644
> --- a/gdb/testsuite/gdb.threads/detach-step-over.exp
> +++ b/gdb/testsuite/gdb.threads/detach-step-over.exp
> @@ -314,12 +314,8 @@ proc_with_prefix test_detach_quit {condition_eval target_non_stop \
> start_gdb_for_test $condition_eval $target_non_stop \
> $non_stop $displaced
>
> - gdb_test_multiple "maint show target-non-stop" "" {
> - -wrap -re "(is|currently) on.*" {
> - }
> - -wrap -re "(is|currently) off.*" {
> - return
> - }
> + if {![is_target_non_stop]} {
> + return
> }
> }
>
> diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
> index e48228ed4f6..ef62174301e 100644
> --- a/gdb/testsuite/lib/gdb.exp
> +++ b/gdb/testsuite/lib/gdb.exp
> @@ -9303,6 +9303,25 @@ proc gdb_step_until { regexp {test_name ""} {max_steps 10} } {
> }
> }
>
> +# Return true if the current target is operating in non-stop mode,
> +# otherwise, return false.
> +#
> +# The inferior will need to have started running in order to get the
> +# correct result.
> +
> +proc is_target_non_stop { {testname ""} } {
> + set is_non_stop false
> + gdb_test_multiple "maint show target-non-stop" $testname {
> + -wrap -re "(is|currently) on.*" {
> + set is_non_stop true
> + }
> + -wrap -re "(is|currently) off.*" {
> + set is_non_stop false
> + }
> + }
> + return $is_non_stop
> +}
The original code defaulted to "true", while this defaults to "false".
I.e., a plain refactor that doesn't change behavior would look like this:
proc is_target_non_stop { {testname ""} } {
gdb_test_multiple "maint show target-non-stop" $testname {
-wrap -re "(is|currently) on.*" {
}
-wrap -re "(is|currently) off.*" {
return false
}
}
return true
}
The original code was written that way so that we'd only skip
the remainder of the tests if we're absolutely sure they
wouldn't work. If the maint command failed for some reason, we
don't know that, so we continue with the testcase.
I think that for a refactoring patch, that detail shouldn't be
changed.
Pedro Alves
> +
> # Check if the compiler emits epilogue information associated
> # with the closing brace or with the last statement line.
> #
>
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH 6/8] gdb/testsuite: introduce is_target_non_stop helper proc
2023-02-22 15:19 ` Pedro Alves
@ 2023-02-27 14:58 ` Andrew Burgess
2023-02-27 16:18 ` Pedro Alves
0 siblings, 1 reply; 19+ messages in thread
From: Andrew Burgess @ 2023-02-27 14:58 UTC (permalink / raw)
To: Pedro Alves, gdb-patches
Pedro Alves <pedro@palves.net> writes:
> On 2023-02-20 2:13 p.m., Andrew Burgess via Gdb-patches wrote:
>> I noticed that several test included copy & pasted code to run the
>
> "several test" -> "several tests"
>
Thanks, fixed.
>> 'maint show target-non-stop' command, and then switch based on the
>> result.
>>
>> In this commit I factor this code out into a helper proc in
>> lib/gdb.exp, and update all the places I could find that used this
>> pattern to make use of the helper proc.
>>
>> There should be no change in what is tested after this commit.
>> ---
>> gdb/testsuite/gdb.base/access-mem-running.exp | 10 +++-------
>> gdb/testsuite/gdb.base/fork-running-state.exp | 11 +++--------
>> .../access-mem-running-thread-exit.exp | 10 +++-------
>> .../gdb.threads/clone-attach-detach.exp | 11 +++--------
>> .../gdb.threads/detach-step-over.exp | 8 ++------
>> gdb/testsuite/lib/gdb.exp | 19 +++++++++++++++++++
>> 6 files changed, 33 insertions(+), 36 deletions(-)
>>
>> diff --git a/gdb/testsuite/gdb.base/access-mem-running.exp b/gdb/testsuite/gdb.base/access-mem-running.exp
>> index 90ddedc470d..d0f7871fc0f 100644
>> --- a/gdb/testsuite/gdb.base/access-mem-running.exp
>> +++ b/gdb/testsuite/gdb.base/access-mem-running.exp
>> @@ -46,13 +46,9 @@ proc test { non_stop } {
>> && ([target_info gdb_protocol] == "remote"
>> || [target_info gdb_protocol] == "extended-remote")} {
>>
>> - gdb_test_multiple "maint show target-non-stop" "" {
>> - -wrap -re "(is|currently) on.*" {
>> - }
>> - -wrap -re "(is|currently) off.*" {
>> - unsupported "can't issue commands while target is running"
>> - return 0
>> - }
>> + if {![is_target_non_stop]} {
>> + unsupported "can't issue commands while target is running"
>> + return 0
>> }
>> }
>>
>> diff --git a/gdb/testsuite/gdb.base/fork-running-state.exp b/gdb/testsuite/gdb.base/fork-running-state.exp
>> index b8045ad490f..9a18193c4dc 100644
>> --- a/gdb/testsuite/gdb.base/fork-running-state.exp
>> +++ b/gdb/testsuite/gdb.base/fork-running-state.exp
>> @@ -46,14 +46,9 @@ proc do_test { detach_on_fork follow_fork non_stop schedule_multiple } {
>> && ([target_info gdb_protocol] == "remote"
>> || [target_info gdb_protocol] == "extended-remote")} {
>>
>> - set test "maint show target-non-stop"
>> - gdb_test_multiple "maint show target-non-stop" $test {
>> - -re "(is|currently) on.*$gdb_prompt $" {
>> - }
>> - -re "(is|currently) off.*$gdb_prompt $" {
>> - unsupported "can't issue info threads while target is running"
>> - return 0
>> - }
>> + if {![is_target_non_stop]} {
>> + unsupported "can't issue info threads while target is running"
>> + return 0
>> }
>> }
>>
>> diff --git a/gdb/testsuite/gdb.threads/access-mem-running-thread-exit.exp b/gdb/testsuite/gdb.threads/access-mem-running-thread-exit.exp
>> index 09b05480c04..2d3ace44ccd 100644
>> --- a/gdb/testsuite/gdb.threads/access-mem-running-thread-exit.exp
>> +++ b/gdb/testsuite/gdb.threads/access-mem-running-thread-exit.exp
>> @@ -65,13 +65,9 @@ proc test { non_stop } {
>> && ([target_info gdb_protocol] == "remote"
>> || [target_info gdb_protocol] == "extended-remote")} {
>>
>> - gdb_test_multiple "maint show target-non-stop" "" {
>> - -wrap -re "(is|currently) on.*" {
>> - }
>> - -wrap -re "(is|currently) off.*" {
>> - unsupported "can't issue commands while target is running"
>> - return 0
>> - }
>> + if {![is_target_non_stop]} {
>> + unsupported "can't issue commands while target is running"
>> + return 0
>> }
>> }
>>
>> diff --git a/gdb/testsuite/gdb.threads/clone-attach-detach.exp b/gdb/testsuite/gdb.threads/clone-attach-detach.exp
>> index ac9b92d5f57..a71713ea8ba 100644
>> --- a/gdb/testsuite/gdb.threads/clone-attach-detach.exp
>> +++ b/gdb/testsuite/gdb.threads/clone-attach-detach.exp
>> @@ -64,14 +64,9 @@ if {[target_info exists gdb_protocol]
>> && ([target_info gdb_protocol] == "remote"
>> || [target_info gdb_protocol] == "extended-remote")} {
>>
>> - set test "maint show target-non-stop"
>> - gdb_test_multiple "maint show target-non-stop" $test {
>> - -re "(is|currently) on.*$gdb_prompt $" {
>> - }
>> - -re "(is|currently) off.*$gdb_prompt $" {
>> - unsupported "bg attach: can't issue info threads while target is running"
>> - return 0
>> - }
>> + if {![is_target_non_stop]} {
>> + unsupported "bg attach: can't issue info threads while target is running"
>> + return 0
>> }
>> }
>>
>> diff --git a/gdb/testsuite/gdb.threads/detach-step-over.exp b/gdb/testsuite/gdb.threads/detach-step-over.exp
>> index ed9dc1aab88..bf5ef6b06a1 100644
>> --- a/gdb/testsuite/gdb.threads/detach-step-over.exp
>> +++ b/gdb/testsuite/gdb.threads/detach-step-over.exp
>> @@ -314,12 +314,8 @@ proc_with_prefix test_detach_quit {condition_eval target_non_stop \
>> start_gdb_for_test $condition_eval $target_non_stop \
>> $non_stop $displaced
>>
>> - gdb_test_multiple "maint show target-non-stop" "" {
>> - -wrap -re "(is|currently) on.*" {
>> - }
>> - -wrap -re "(is|currently) off.*" {
>> - return
>> - }
>> + if {![is_target_non_stop]} {
>> + return
>> }
>> }
>>
>> diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
>> index e48228ed4f6..ef62174301e 100644
>> --- a/gdb/testsuite/lib/gdb.exp
>> +++ b/gdb/testsuite/lib/gdb.exp
>> @@ -9303,6 +9303,25 @@ proc gdb_step_until { regexp {test_name ""} {max_steps 10} } {
>> }
>> }
>>
>> +# Return true if the current target is operating in non-stop mode,
>> +# otherwise, return false.
>> +#
>> +# The inferior will need to have started running in order to get the
>> +# correct result.
>> +
>> +proc is_target_non_stop { {testname ""} } {
>> + set is_non_stop false
>> + gdb_test_multiple "maint show target-non-stop" $testname {
>> + -wrap -re "(is|currently) on.*" {
>> + set is_non_stop true
>> + }
>> + -wrap -re "(is|currently) off.*" {
>> + set is_non_stop false
>> + }
>> + }
>> + return $is_non_stop
>> +}
>
> The original code defaulted to "true", while this defaults to "false".
>
> I.e., a plain refactor that doesn't change behavior would look like this:
>
> proc is_target_non_stop { {testname ""} } {
> gdb_test_multiple "maint show target-non-stop" $testname {
> -wrap -re "(is|currently) on.*" {
> }
> -wrap -re "(is|currently) off.*" {
> return false
> }
> }
> return true
> }
>
> The original code was written that way so that we'd only skip
> the remainder of the tests if we're absolutely sure they
> wouldn't work. If the maint command failed for some reason, we
> don't know that, so we continue with the testcase.
>
> I think that for a refactoring patch, that detail shouldn't be
> changed.
Thanks for the feedback. How's the patch below? The only change is in
testsuite/lib/gdb.exp, everything else is the same as before.
Thanks,
Andrew
---
commit 40b895a3386c8472e54bc8bb5c3eae01d7123b9a
Author: Andrew Burgess <aburgess@redhat.com>
Date: Sat Feb 18 20:52:40 2023 +0000
gdb/testsuite: introduce is_target_non_stop helper proc
I noticed that several tests included copy & pasted code to run the
'maint show target-non-stop' command, and then switch based on the
result.
In this commit I factor this code out into a helper proc in
lib/gdb.exp, and update all the places I could find that used this
pattern to make use of the helper proc.
There should be no change in what is tested after this commit.
diff --git a/gdb/testsuite/gdb.base/access-mem-running.exp b/gdb/testsuite/gdb.base/access-mem-running.exp
index 90ddedc470d..d0f7871fc0f 100644
--- a/gdb/testsuite/gdb.base/access-mem-running.exp
+++ b/gdb/testsuite/gdb.base/access-mem-running.exp
@@ -46,13 +46,9 @@ proc test { non_stop } {
&& ([target_info gdb_protocol] == "remote"
|| [target_info gdb_protocol] == "extended-remote")} {
- gdb_test_multiple "maint show target-non-stop" "" {
- -wrap -re "(is|currently) on.*" {
- }
- -wrap -re "(is|currently) off.*" {
- unsupported "can't issue commands while target is running"
- return 0
- }
+ if {![is_target_non_stop]} {
+ unsupported "can't issue commands while target is running"
+ return 0
}
}
diff --git a/gdb/testsuite/gdb.base/fork-running-state.exp b/gdb/testsuite/gdb.base/fork-running-state.exp
index b8045ad490f..9a18193c4dc 100644
--- a/gdb/testsuite/gdb.base/fork-running-state.exp
+++ b/gdb/testsuite/gdb.base/fork-running-state.exp
@@ -46,14 +46,9 @@ proc do_test { detach_on_fork follow_fork non_stop schedule_multiple } {
&& ([target_info gdb_protocol] == "remote"
|| [target_info gdb_protocol] == "extended-remote")} {
- set test "maint show target-non-stop"
- gdb_test_multiple "maint show target-non-stop" $test {
- -re "(is|currently) on.*$gdb_prompt $" {
- }
- -re "(is|currently) off.*$gdb_prompt $" {
- unsupported "can't issue info threads while target is running"
- return 0
- }
+ if {![is_target_non_stop]} {
+ unsupported "can't issue info threads while target is running"
+ return 0
}
}
diff --git a/gdb/testsuite/gdb.threads/access-mem-running-thread-exit.exp b/gdb/testsuite/gdb.threads/access-mem-running-thread-exit.exp
index 09b05480c04..2d3ace44ccd 100644
--- a/gdb/testsuite/gdb.threads/access-mem-running-thread-exit.exp
+++ b/gdb/testsuite/gdb.threads/access-mem-running-thread-exit.exp
@@ -65,13 +65,9 @@ proc test { non_stop } {
&& ([target_info gdb_protocol] == "remote"
|| [target_info gdb_protocol] == "extended-remote")} {
- gdb_test_multiple "maint show target-non-stop" "" {
- -wrap -re "(is|currently) on.*" {
- }
- -wrap -re "(is|currently) off.*" {
- unsupported "can't issue commands while target is running"
- return 0
- }
+ if {![is_target_non_stop]} {
+ unsupported "can't issue commands while target is running"
+ return 0
}
}
diff --git a/gdb/testsuite/gdb.threads/clone-attach-detach.exp b/gdb/testsuite/gdb.threads/clone-attach-detach.exp
index ac9b92d5f57..a71713ea8ba 100644
--- a/gdb/testsuite/gdb.threads/clone-attach-detach.exp
+++ b/gdb/testsuite/gdb.threads/clone-attach-detach.exp
@@ -64,14 +64,9 @@ if {[target_info exists gdb_protocol]
&& ([target_info gdb_protocol] == "remote"
|| [target_info gdb_protocol] == "extended-remote")} {
- set test "maint show target-non-stop"
- gdb_test_multiple "maint show target-non-stop" $test {
- -re "(is|currently) on.*$gdb_prompt $" {
- }
- -re "(is|currently) off.*$gdb_prompt $" {
- unsupported "bg attach: can't issue info threads while target is running"
- return 0
- }
+ if {![is_target_non_stop]} {
+ unsupported "bg attach: can't issue info threads while target is running"
+ return 0
}
}
diff --git a/gdb/testsuite/gdb.threads/detach-step-over.exp b/gdb/testsuite/gdb.threads/detach-step-over.exp
index ed9dc1aab88..bf5ef6b06a1 100644
--- a/gdb/testsuite/gdb.threads/detach-step-over.exp
+++ b/gdb/testsuite/gdb.threads/detach-step-over.exp
@@ -314,12 +314,8 @@ proc_with_prefix test_detach_quit {condition_eval target_non_stop \
start_gdb_for_test $condition_eval $target_non_stop \
$non_stop $displaced
- gdb_test_multiple "maint show target-non-stop" "" {
- -wrap -re "(is|currently) on.*" {
- }
- -wrap -re "(is|currently) off.*" {
- return
- }
+ if {![is_target_non_stop]} {
+ return
}
}
diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index 669a5f606d6..19c782bea46 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -9298,6 +9298,28 @@ proc gdb_step_until { regexp {test_name ""} {max_steps 10} } {
}
}
+# Return false if the current target is not operating in non-stop
+# mode, otherwise, return true.
+#
+# The inferior will need to have started running in order to get the
+# correct result.
+
+proc is_target_non_stop { {testname ""} } {
+ # For historical reasons we assume non-stop mode is on. If the
+ # maintenance command fails for any reason then we're going to
+ # return true.
+ set is_non_stop true
+ gdb_test_multiple "maint show target-non-stop" $testname {
+ -wrap -re "(is|currently) on.*" {
+ set is_non_stop true
+ }
+ -wrap -re "(is|currently) off.*" {
+ set is_non_stop false
+ }
+ }
+ return $is_non_stop
+}
+
# Check if the compiler emits epilogue information associated
# with the closing brace or with the last statement line.
#
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH 6/8] gdb/testsuite: introduce is_target_non_stop helper proc
2023-02-27 14:58 ` Andrew Burgess
@ 2023-02-27 16:18 ` Pedro Alves
0 siblings, 0 replies; 19+ messages in thread
From: Pedro Alves @ 2023-02-27 16:18 UTC (permalink / raw)
To: Andrew Burgess, gdb-patches
On 2023-02-27 2:58 p.m., Andrew Burgess wrote:
>> The original code was written that way so that we'd only skip
>> the remainder of the tests if we're absolutely sure they
>> wouldn't work. If the maint command failed for some reason, we
>> don't know that, so we continue with the testcase.
>>
>> I think that for a refactoring patch, that detail shouldn't be
>> changed.
>
> Thanks for the feedback. How's the patch below? The only change is in
> testsuite/lib/gdb.exp, everything else is the same as before.
LGTM.
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 7/8] gdb/testsuite: fix failure in gdb.mi/mi-pending.exp with extended-remote
2023-02-20 14:13 [PATCH 0/8] Fix missing MI =breakpoint-deleted notifications Andrew Burgess
` (5 preceding siblings ...)
2023-02-20 14:13 ` [PATCH 6/8] gdb/testsuite: introduce is_target_non_stop " Andrew Burgess
@ 2023-02-20 14:13 ` Andrew Burgess
2023-02-22 15:19 ` Pedro Alves
2023-02-20 14:13 ` [PATCH 8/8] gdb: fix mi breakpoint-deleted notifications for thread-specific b/p Andrew Burgess
2023-02-22 15:23 ` [PATCH 0/8] Fix missing MI =breakpoint-deleted notifications Pedro Alves
8 siblings, 1 reply; 19+ messages in thread
From: Andrew Burgess @ 2023-02-20 14:13 UTC (permalink / raw)
To: gdb-patches; +Cc: Andrew Burgess
I currently see this failure when running the gdb.mi/mi-pending.exp
test using the native-extended-remote board:
-break-insert -f -c x==4 mi-pendshr.c:pendfunc2
&"No source file named mi-pendshr.c.\n"
^done,bkpt={number="2",type="breakpoint",disp="keep",enabled="y",addr="<PENDING>",pending="mi-pendshr.c:pendfunc2",cond="x==4",evaluated-by="host",times="0",original-location="mi-pendshr.c:pendfunc2"}
(gdb)
FAIL: gdb.mi/mi-pending.exp: MI pending breakpoint on mi-pendshr.c:pendfunc2 if x==4 (unexpected output)
The failure is caused by the 'evaluated-by="host"' string, which only
appears in the output when the test is run using the
native-extended-remote board.
I could fix this by just updating the pattern in
gdb.mi/mi-pending.exp, but I have instead updated mi-pending.exp to
make more use of the support procs in mi-support.exp. This did
require making a couple of adjustments to mi-support.exp, but I think
the result is that mi-pending.exp is now easier to read, and I see no
failures with native-extended-remote anymore.
One of the test names has changed after this work, I think the old
test name was wrong - it described a breakpoint as pending when the
breakpoint was not pending, I suspect a copy & paste error.
But there's no changes to what is actually being tested after this
patch.
---
gdb/testsuite/gdb.mi/mi-pending.exp | 34 +++++++++++++++++++++--------
gdb/testsuite/lib/mi-support.exp | 11 ++++++++--
2 files changed, 34 insertions(+), 11 deletions(-)
diff --git a/gdb/testsuite/gdb.mi/mi-pending.exp b/gdb/testsuite/gdb.mi/mi-pending.exp
index a5c6ee5c906..71c3d45fe44 100644
--- a/gdb/testsuite/gdb.mi/mi-pending.exp
+++ b/gdb/testsuite/gdb.mi/mi-pending.exp
@@ -52,19 +52,35 @@ mi_load_shlibs $lib_sl1
mi_load_shlibs $lib_sl2
# Set pending breakpoint via MI.
-mi_gdb_test "-break-insert -f pendfunc1" \
- ".*\\^done,bkpt=\{number=\"1\",type=\"breakpoint\",disp=\"keep\",enabled=\"y\",addr=\"<PENDING>\",pending=\"pendfunc1\",times=\"0\",original-location=\"pendfunc1\"\}"\
- "MI pending breakpoint on pendfunc1"
+mi_create_breakpoint_pending "-f pendfunc1" \
+ "MI pending breakpoint on pendfunc1" \
+ -number "1" \
+ -type "breakpoint" \
+ -disp "keep" \
+ -enabled "y" \
+ -pending "pendfunc1" \
+ -original-location "pendfunc1"
# Set pending breakpoint with a condition via MI.
-mi_gdb_test "-break-insert -f -c x==4 ${libfile1}.c:pendfunc2" \
- ".*\\^done,bkpt=\{number=\"2\",type=\"breakpoint\",disp=\"keep\",enabled=\"y\",addr=\"<PENDING>\",pending=\"${libfile1}.c:pendfunc2\",cond=\"x==4\",times=\"0\",original-location=\"${libfile1}.c:pendfunc2\"\}"\
- "MI pending breakpoint on ${libfile1}.c:pendfunc2 if x==4"
+mi_create_breakpoint_pending "-f -c x==4 ${libfile1}.c:pendfunc2" \
+ "MI pending breakpoint on ${libfile1}.c:pendfunc2 if x==4" \
+ -number "2" \
+ -type "breakpoint" \
+ -disp "keep" \
+ -enabled "y" \
+ -pending "${libfile1}.c:pendfunc2" \
+ -cond "x==4" \
+ -original-location "${libfile1}.c:pendfunc2"
# Set breakpoint so that we can stop when the thread is created
-mi_gdb_test "-break-insert -f thread_func" \
- ".*\\^done,bkpt=\{number=\"3\",type=\"breakpoint\",disp=\"keep\",enabled=\"y\",addr=\"${hex}\",func=\"thread_func\".*\}"\
- "MI pending breakpoint on thread_func"
+mi_create_breakpoint "-f thread_func" \
+ "MI breakpoint on thread_func" \
+ -number "3" \
+ -type "breakpoint" \
+ -disp "keep" \
+ -enabled "y" \
+ -addr "$hex" \
+ -func "thread_func"
mi_run_cmd
diff --git a/gdb/testsuite/lib/mi-support.exp b/gdb/testsuite/lib/mi-support.exp
index 2ab751749e9..c072da08a04 100644
--- a/gdb/testsuite/lib/mi-support.exp
+++ b/gdb/testsuite/lib/mi-support.exp
@@ -1390,6 +1390,14 @@ proc mi_create_breakpoint_multi {location test args} {
return $bp
}
+# Like mi_create_breakpoint, but creates a pending breakpoint.
+
+proc mi_create_breakpoint_pending {location test args} {
+ set bp [eval mi_make_breakpoint_pending $args]
+ mi_gdb_test "222-break-insert $location" ".*\r\n222\\^done,$bp" $test
+ return $bp
+}
+
# Creates varobj named NAME for EXPRESSION.
# Name cannot be "-".
proc mi_create_varobj { name expression testname } {
@@ -2639,7 +2647,7 @@ proc mi_make_breakpoint_multi {args} {
proc mi_make_breakpoint_pending {args} {
parse_args {{number .*} {type .*} {disp .*} {enabled .*}
- {pending .*} {original-location .*} {thread ""}}
+ {pending .*} {original-location .*} {thread ""} {cond ""}}
set attr_list {}
foreach attr [list number type disp enabled] {
@@ -2655,7 +2663,6 @@ proc mi_make_breakpoint_pending {args} {
set ignore 0
set times 0
set script ""
- set cond ""
set evaluated-by ""
set result [mi_make_breakpoint_1 \
--
2.25.4
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH 7/8] gdb/testsuite: fix failure in gdb.mi/mi-pending.exp with extended-remote
2023-02-20 14:13 ` [PATCH 7/8] gdb/testsuite: fix failure in gdb.mi/mi-pending.exp with extended-remote Andrew Burgess
@ 2023-02-22 15:19 ` Pedro Alves
0 siblings, 0 replies; 19+ messages in thread
From: Pedro Alves @ 2023-02-22 15:19 UTC (permalink / raw)
To: Andrew Burgess, gdb-patches
On 2023-02-20 2:13 p.m., Andrew Burgess via Gdb-patches wrote:
> I currently see this failure when running the gdb.mi/mi-pending.exp
> test using the native-extended-remote board:
>
> -break-insert -f -c x==4 mi-pendshr.c:pendfunc2
> &"No source file named mi-pendshr.c.\n"
> ^done,bkpt={number="2",type="breakpoint",disp="keep",enabled="y",addr="<PENDING>",pending="mi-pendshr.c:pendfunc2",cond="x==4",evaluated-by="host",times="0",original-location="mi-pendshr.c:pendfunc2"}
> (gdb)
> FAIL: gdb.mi/mi-pending.exp: MI pending breakpoint on mi-pendshr.c:pendfunc2 if x==4 (unexpected output)
>
> The failure is caused by the 'evaluated-by="host"' string, which only
> appears in the output when the test is run using the
> native-extended-remote board.
>
> I could fix this by just updating the pattern in
> gdb.mi/mi-pending.exp, but I have instead updated mi-pending.exp to
> make more use of the support procs in mi-support.exp. This did
> require making a couple of adjustments to mi-support.exp, but I think
> the result is that mi-pending.exp is now easier to read, and I see no
> failures with native-extended-remote anymore.
>
> One of the test names has changed after this work, I think the old
> test name was wrong - it described a breakpoint as pending when the
> breakpoint was not pending, I suspect a copy & paste error.
>
> But there's no changes to what is actually being tested after this
> patch.
> ---
> gdb/testsuite/gdb.mi/mi-pending.exp | 34 +++++++++++++++++++++--------
> gdb/testsuite/lib/mi-support.exp | 11 ++++++++--
> 2 files changed, 34 insertions(+), 11 deletions(-)
>
> diff --git a/gdb/testsuite/gdb.mi/mi-pending.exp b/gdb/testsuite/gdb.mi/mi-pending.exp
> index a5c6ee5c906..71c3d45fe44 100644
> --- a/gdb/testsuite/gdb.mi/mi-pending.exp
> +++ b/gdb/testsuite/gdb.mi/mi-pending.exp
> @@ -52,19 +52,35 @@ mi_load_shlibs $lib_sl1
> mi_load_shlibs $lib_sl2
>
> # Set pending breakpoint via MI.
> -mi_gdb_test "-break-insert -f pendfunc1" \
> - ".*\\^done,bkpt=\{number=\"1\",type=\"breakpoint\",disp=\"keep\",enabled=\"y\",addr=\"<PENDING>\",pending=\"pendfunc1\",times=\"0\",original-location=\"pendfunc1\"\}"\
> - "MI pending breakpoint on pendfunc1"
> +mi_create_breakpoint_pending "-f pendfunc1" \
> + "MI pending breakpoint on pendfunc1" \
> + -number "1" \
> + -type "breakpoint" \
> + -disp "keep" \
> + -enabled "y" \
> + -pending "pendfunc1" \
> + -original-location "pendfunc1"
>
> # Set pending breakpoint with a condition via MI.
> -mi_gdb_test "-break-insert -f -c x==4 ${libfile1}.c:pendfunc2" \
> - ".*\\^done,bkpt=\{number=\"2\",type=\"breakpoint\",disp=\"keep\",enabled=\"y\",addr=\"<PENDING>\",pending=\"${libfile1}.c:pendfunc2\",cond=\"x==4\",times=\"0\",original-location=\"${libfile1}.c:pendfunc2\"\}"\
> - "MI pending breakpoint on ${libfile1}.c:pendfunc2 if x==4"
> +mi_create_breakpoint_pending "-f -c x==4 ${libfile1}.c:pendfunc2" \
> + "MI pending breakpoint on ${libfile1}.c:pendfunc2 if x==4" \
> + -number "2" \
> + -type "breakpoint" \
> + -disp "keep" \
> + -enabled "y" \
> + -pending "${libfile1}.c:pendfunc2" \
> + -cond "x==4" \
> + -original-location "${libfile1}.c:pendfunc2"
>
> # Set breakpoint so that we can stop when the thread is created
> -mi_gdb_test "-break-insert -f thread_func" \
> - ".*\\^done,bkpt=\{number=\"3\",type=\"breakpoint\",disp=\"keep\",enabled=\"y\",addr=\"${hex}\",func=\"thread_func\".*\}"\
> - "MI pending breakpoint on thread_func"
> +mi_create_breakpoint "-f thread_func" \
> + "MI breakpoint on thread_func" \
> + -number "3" \
> + -type "breakpoint" \
> + -disp "keep" \
> + -enabled "y" \
> + -addr "$hex" \
> + -func "thread_func"
>
> mi_run_cmd
>
> diff --git a/gdb/testsuite/lib/mi-support.exp b/gdb/testsuite/lib/mi-support.exp
> index 2ab751749e9..c072da08a04 100644
> --- a/gdb/testsuite/lib/mi-support.exp
> +++ b/gdb/testsuite/lib/mi-support.exp
> @@ -1390,6 +1390,14 @@ proc mi_create_breakpoint_multi {location test args} {
> return $bp
> }
>
> +# Like mi_create_breakpoint, but creates a pending breakpoint.
> +
> +proc mi_create_breakpoint_pending {location test args} {
> + set bp [eval mi_make_breakpoint_pending $args]
> + mi_gdb_test "222-break-insert $location" ".*\r\n222\\^done,$bp" $test
> + return $bp
> +}
> +
> # Creates varobj named NAME for EXPRESSION.
> # Name cannot be "-".
> proc mi_create_varobj { name expression testname } {
> @@ -2639,7 +2647,7 @@ proc mi_make_breakpoint_multi {args} {
>
> proc mi_make_breakpoint_pending {args} {
> parse_args {{number .*} {type .*} {disp .*} {enabled .*}
> - {pending .*} {original-location .*} {thread ""}}
> + {pending .*} {original-location .*} {thread ""} {cond ""}}
Intro comment needs update.
Otherwise:
Approved-By: Pedro Alves <pedro@palves.net>
Pedro Alves
>
> set attr_list {}
> foreach attr [list number type disp enabled] {
> @@ -2655,7 +2663,6 @@ proc mi_make_breakpoint_pending {args} {
> set ignore 0
> set times 0
> set script ""
> - set cond ""
> set evaluated-by ""
>
> set result [mi_make_breakpoint_1 \
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 8/8] gdb: fix mi breakpoint-deleted notifications for thread-specific b/p
2023-02-20 14:13 [PATCH 0/8] Fix missing MI =breakpoint-deleted notifications Andrew Burgess
` (6 preceding siblings ...)
2023-02-20 14:13 ` [PATCH 7/8] gdb/testsuite: fix failure in gdb.mi/mi-pending.exp with extended-remote Andrew Burgess
@ 2023-02-20 14:13 ` Andrew Burgess
2023-02-22 15:20 ` Pedro Alves
2023-02-22 15:23 ` [PATCH 0/8] Fix missing MI =breakpoint-deleted notifications Pedro Alves
8 siblings, 1 reply; 19+ messages in thread
From: Andrew Burgess @ 2023-02-20 14:13 UTC (permalink / raw)
To: gdb-patches; +Cc: Andrew Burgess
Background
----------
When a thread-specific breakpoint is deleted as a result of the
specific thread exiting the function remove_threaded_breakpoints is
called which sets the disposition of the breakpoint to
disp_del_at_next_stop and sets the breakpoint number to 0. Setting
the breakpoint number to zero has the effect of hiding the breakpoint
from the user. We also print a message indicating that the breakpoint
has been deleted.
It was brought to my attention during a review of another patch[1]
that setting a breakpoints number to zero will suppress the MI
breakpoint-deleted notification for that breakpoint, and indeed, this
can be seen to be true, in delete_breakpoint, if the breakpoint number
is zero, then GDB will not notify the breakpoint_deleted observer.
It seems wrong that a user created, thread-specific breakpoint, will
have a =breakpoint-created notification, but will not have a
=breakpoint-deleted notification. I suspect that this is a bug.
[1] https://sourceware.org/pipermail/gdb-patches/2023-February/196560.html
The First Problem
-----------------
During my initial testing I wanted to see how GDB handled the
breakpoint after it's number was set to zero. To do this I created
the testcase gdb.threads/thread-bp-deleted.exp. This test creates a
worker thread, which immediately exits. After the worker thread has
exited the main thread spins in a loop.
In GDB I break once the worker thread has been created and place a
thread-specific breakpoint, then use 'continue&' to resume the
inferior in non-stop mode. The worker thread then exits, but the main
thread never stops - instead it sits in the spin. I then tried to use
'maint info breakpoints' to see what GDB thought of the
thread-specific breakpoint.
Unfortunately, GDB crashed like this:
(gdb) continue&
Continuing.
(gdb) [Thread 0x7ffff7c5d700 (LWP 1202458) exited]
Thread-specific breakpoint 3 deleted - thread 2 no longer in the thread list.
maint info breakpoints
... snip some output ...
Fatal signal: Segmentation fault
----- Backtrace -----
0x5ffb62 gdb_internal_backtrace_1
../../src/gdb/bt-utils.c:122
0x5ffc05 _Z22gdb_internal_backtracev
../../src/gdb/bt-utils.c:168
0x89965e handle_fatal_signal
../../src/gdb/event-top.c:964
0x8997ca handle_sigsegv
../../src/gdb/event-top.c:1037
0x7f96f5971b1f ???
/usr/src/debug/glibc-2.30-2-gd74461fa34/nptl/../sysdeps/unix/sysv/linux/x86_64/sigaction.c:0
0xe602b0 _Z15print_thread_idP11thread_info
../../src/gdb/thread.c:1439
0x5b3d05 print_one_breakpoint_location
../../src/gdb/breakpoint.c:6542
0x5b462e print_one_breakpoint
../../src/gdb/breakpoint.c:6702
0x5b5354 breakpoint_1
../../src/gdb/breakpoint.c:6924
0x5b58b8 maintenance_info_breakpoints
../../src/gdb/breakpoint.c:7009
... etc ...
As the thread-specific breakpoint is set to disp_del_at_next_stop, and
GDB hasn't stopped yet, then the breakpoint still exists in the global
breakpoint list.
The breakpoint will not show in 'info breakpoints' as its number is
zero, but it will show in 'maint info breakpoints'.
As GDB prints the breakpoint, the thread-id for the breakpoint is
printed as part of the 'stop only in thread ...' line. Printing the
thread-id involves calling find_thread_global_id to convert the global
thread-id into a thread_info*. Then calling print_thread_id to
convert the thread_info* into a string.
The problem is that find_thread_global_id returns nullptr as the
thread for the thread-specific breakpoint has exited. The
print_thread_id assumes it will be passed a non-nullptr. As a result
GDB crashes.
In this commit I've added an assert to print_thread_id (gdb/thread.c)
to check that the pointed passed in is not nullptr. This assert would
have triggered in the above case before GDB crashed.
MI Notifications: The Dangers Of Changing A Breakpoint's Number
---------------------------------------------------------------
Currently the delete_breakpoint function doesn't trigger the
breakpoint_deleted observer for any breakpoint with the number zero.
There is a comment explaining why this is the case in the code; it's
something about watchpoints. But I did consider just removing the 'is
the number zero' guard and always triggering the breakpoint_deleted
observer, figuring that I'd then fix the watchpoint issue some other
way.
But I realised this wasn't going to be good enough. When the MI
notification was delivered the number would be zero, so any frontend
parsing the notifications would not be able to match
=breakpoint-deleted notification to the earlier =breakpoint-created
notification.
What this means is that, at the point the breakpoint_deleted observer
is called, the breakpoint's number must be correct.
MI Notifications: The Dangers Of Delaying Deletion
--------------------------------------------------
The test I used to expose the above crash also brought another problem
to my attention. In the above test we used 'continue&' to resume,
after which a thread exited, but the inferior didn't stop. Recreating
the same test in the MI looks like this:
-break-insert -p 2 main
^done,bkpt={number="2",type="breakpoint",disp="keep",...<snip>...}
(gdb)
-exec-continue
^running
*running,thread-id="all"
(gdb)
~"[Thread 0x7ffff7c5d700 (LWP 987038) exited]\n"
=thread-exited,id="2",group-id="i1"
~"Thread-specific breakpoint 2 deleted - thread 2 no longer in the thread list.\n"
At this point the we have a single thread left, which is still
running:
-thread-info
^done,threads=[{id="1",target-id="Thread 0x7ffff7c5eb80 (LWP 987035)",name="thread-bp-delet",state="running",core="4"}],current-thread-id="1"
(gdb)
Notice that we got the =thread-exited notification from GDB as soon as
the thread exited. We also saw the CLI line from GDB, the line
explaining that breakpoint 2 was deleted. But, as expected, we didn't
see the =breakpoint-deleted notification.
I say "as expected" because the number was set to zero. But, even if
the number was not set to zero we still wouldn't see the
notification. The MI notification is driven by the breakpoint_deleted
observer, which is only called when we actually delete the breakpoint,
which is only done the next time GDB stops.
Now, maybe this is fine. The notification is delivered a little
late. But remember, by setting the number to zero the breakpoint will
be hidden from the user, for example, the breakpoint is removed from
the MI's -break-info command output.
This means that GDB is in a position where the breakpoint doesn't show
up in the breakpoint table, but a =breakpoint-deleted notification has
not yet been sent out. This doesn't seem right to me.
What this means is that, when the thread exits, we should immediately
be sending out the =breakpoint-deleted notification. We should not
wait for GDB to next stop before sending the notification.
The Solution
------------
My proposed solution is this; in remove_threaded_breakpoints, instead
of setting the disposition to disp_del_at_next_stop and setting the
number to zero, we now just call delete_breakpoint directly.
The notification will now be sent out immediately; as soon as the
thread exits.
As the number has not changed when delete_breakpoint is called, the
notification will have the correct number.
And as the breakpoint is immediately removed from the breakpoint list,
we no longer need to worry about 'maint info breakpoints' trying to
print the thread-id for an exited thread.
My only concern is that calling delete_breakpoint directly seems so
obvious that I wonder why the original patch (that added
remove_threaded_breakpoints) didn't take this approach. This code was
added in commit 49fa26b0411d, but the commit message offers no clues
to why this approach was taken, and the original email thread offers
no insights either[2]. There are no test regressions after making
this change, so I'm hopeful that this is going to be fine.
[2] https://sourceware.org/pipermail/gdb-patches/2013-September/106493.html
The Complication
----------------
Of course, it couldn't be that simple.
The script gdb.python/py-finish-breakpoint.exp had some regressions
during testing.
The problem was with the FinishBreakpoint.out_of_scope callback
implementation. This callback is supposed to trigger whenever the
FinishBreakpoint goes out of scope; and this includes when the thread
for the breakpoint exits.
The problem I ran into is the Python FinishBreakpoint implementation.
Specifically, after this change I was loosing some of the out_of_scope
calls.
The problem is that the out_of_scope call (of which I'm interested) is
triggered from the inferior_exit observer. Before my change the
observers were called in this order:
thread_exit
inferior_exit
breakpoint_deleted
The inferior_exit would trigger the out_of_scope call.
After my change the breakpoint_deleted notification (for
thread-specific breakpoints) occurs earlier, as soon as the
thread-exits, so now the order is:
thread_exit
breakpoint_deleted
inferior_exit
Currently, after the breakpoint_deleted call the Python object
associated with the breakpoint is released, so, when we get to the
inferior_exit observer, there's no longer a Python object to call the
out_of_scope method on.
My solution is to follow the model for how bpfinishpy_pre_stop_hook
and bpfinishpy_post_stop_hook are called, this is done from
gdbpy_breakpoint_cond_says_stop in py-breakpoint.c.
I've now added a new bpfinishpy_pre_delete_hook
gdbpy_breakpoint_deleted in py-breakpoint.c, and from this new hook
function I check and where needed call the out_of_scope method.
With this fix in place I now see the
gdb.python/py-finish-breakpoint.exp test fully passing again.
Testing
-------
Tested on x86-64/Linux with unix, native-gdbserver, and
native-extended-gdbserver boards.
New tests added to covers all the cases I've discussed above.
---
gdb/breakpoint.c | 6 +-
gdb/python/py-breakpoint.c | 3 +
gdb/python/py-finishbreakpoint.c | 33 ++-
gdb/python/python-internal.h | 1 +
gdb/testsuite/gdb.mi/mi-thread-bp-deleted.c | 88 ++++++
gdb/testsuite/gdb.mi/mi-thread-bp-deleted.exp | 268 ++++++++++++++++++
gdb/testsuite/gdb.threads/thread-bp-deleted.c | 88 ++++++
.../gdb.threads/thread-bp-deleted.exp | 206 ++++++++++++++
gdb/thread.c | 2 +
9 files changed, 683 insertions(+), 12 deletions(-)
create mode 100644 gdb/testsuite/gdb.mi/mi-thread-bp-deleted.c
create mode 100644 gdb/testsuite/gdb.mi/mi-thread-bp-deleted.exp
create mode 100644 gdb/testsuite/gdb.threads/thread-bp-deleted.c
create mode 100644 gdb/testsuite/gdb.threads/thread-bp-deleted.exp
diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index f3121c8d6eb..dfa62356aca 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -3249,14 +3249,10 @@ remove_threaded_breakpoints (struct thread_info *tp, int silent)
{
if (b->thread == tp->global_num && user_breakpoint_p (b))
{
- b->disposition = disp_del_at_next_stop;
-
gdb_printf (_("\
Thread-specific breakpoint %d deleted - thread %s no longer in the thread list.\n"),
b->number, print_thread_id (tp));
-
- /* Hide it from the user. */
- b->number = 0;
+ delete_breakpoint (b);
}
}
}
diff --git a/gdb/python/py-breakpoint.c b/gdb/python/py-breakpoint.c
index ecf52a4637c..880f1b5c1e2 100644
--- a/gdb/python/py-breakpoint.c
+++ b/gdb/python/py-breakpoint.c
@@ -1199,6 +1199,9 @@ gdbpy_breakpoint_deleted (struct breakpoint *b)
gdbpy_ref<gdbpy_breakpoint_object> bp_obj (bp->py_bp_object);
if (bp_obj != NULL)
{
+ if (bp_obj->is_finish_bp)
+ bpfinishpy_pre_delete_hook (bp_obj.get ());
+
if (!evregpy_no_listeners_p (gdb_py_events.breakpoint_deleted))
{
if (evpy_emit_event ((PyObject *) bp_obj.get (),
diff --git a/gdb/python/py-finishbreakpoint.c b/gdb/python/py-finishbreakpoint.c
index d4d129110e9..963d8efb216 100644
--- a/gdb/python/py-finishbreakpoint.c
+++ b/gdb/python/py-finishbreakpoint.c
@@ -340,16 +340,19 @@ bpfinishpy_out_of_scope (struct finish_breakpoint_object *bpfinish_obj)
if (meth_result == NULL)
gdbpy_print_stack ();
}
-
- delete_breakpoint (bpfinish_obj->py_bp.bp);
}
/* Callback for `bpfinishpy_detect_out_scope'. Triggers Python's
- `B->out_of_scope' function if B is a FinishBreakpoint out of its scope. */
+ `B->out_of_scope' function if B is a FinishBreakpoint out of its scope.
+
+ When DELETE_BP is true then breakpoint B will be deleted if B is a
+ FinishBreakpoint and it is out of scope, otherwise B will not be
+ deleted. */
static void
bpfinishpy_detect_out_scope_cb (struct breakpoint *b,
- struct breakpoint *bp_stopped)
+ struct breakpoint *bp_stopped,
+ bool delete_bp)
{
PyObject *py_bp = (PyObject *) b->py_bp_object;
@@ -370,7 +373,11 @@ bpfinishpy_detect_out_scope_cb (struct breakpoint *b,
if (b->pspace == current_inferior ()->pspace
&& (!target_has_registers ()
|| frame_find_by_id (initiating_frame) == NULL))
- bpfinishpy_out_of_scope (finish_bp);
+ {
+ bpfinishpy_out_of_scope (finish_bp);
+ if (delete_bp)
+ delete_breakpoint (finish_bp->py_bp.bp);
+ }
}
catch (const gdb_exception &except)
{
@@ -381,6 +388,17 @@ bpfinishpy_detect_out_scope_cb (struct breakpoint *b,
}
}
+/* Called when gdbpy_breakpoint_deleted is about to delete a breakpoint. A
+ chance to trigger the out_of_scope callback (if appropriate) for the
+ associated Python object. */
+
+void
+bpfinishpy_pre_delete_hook (struct gdbpy_breakpoint_object *bp_obj)
+{
+ breakpoint *bp = bp_obj->bp;
+ bpfinishpy_detect_out_scope_cb (bp, nullptr, false);
+}
+
/* Attached to `stop' notifications, check if the execution has run
out of the scope of any FinishBreakpoint before it has been hit. */
@@ -390,7 +408,8 @@ bpfinishpy_handle_stop (struct bpstat *bs, int print_frame)
gdbpy_enter enter_py;
for (breakpoint *bp : all_breakpoints_safe ())
- bpfinishpy_detect_out_scope_cb (bp, bs == NULL ? NULL : bs->breakpoint_at);
+ bpfinishpy_detect_out_scope_cb
+ (bp, bs == NULL ? NULL : bs->breakpoint_at, true);
}
/* Attached to `exit' notifications, triggers all the necessary out of
@@ -402,7 +421,7 @@ bpfinishpy_handle_exit (struct inferior *inf)
gdbpy_enter enter_py (target_gdbarch ());
for (breakpoint *bp : all_breakpoints_safe ())
- bpfinishpy_detect_out_scope_cb (bp, nullptr);
+ bpfinishpy_detect_out_scope_cb (bp, nullptr, true);
}
/* Initialize the Python finish breakpoint code. */
diff --git a/gdb/python/python-internal.h b/gdb/python/python-internal.h
index 8fb09796b15..1dde2a57c59 100644
--- a/gdb/python/python-internal.h
+++ b/gdb/python/python-internal.h
@@ -778,6 +778,7 @@ extern const struct value_print_options *gdbpy_current_print_options;
void bpfinishpy_pre_stop_hook (struct gdbpy_breakpoint_object *bp_obj);
void bpfinishpy_post_stop_hook (struct gdbpy_breakpoint_object *bp_obj);
+void bpfinishpy_pre_delete_hook (struct gdbpy_breakpoint_object *bp_obj);
extern PyObject *gdbpy_doc_cst;
extern PyObject *gdbpy_children_cst;
diff --git a/gdb/testsuite/gdb.mi/mi-thread-bp-deleted.c b/gdb/testsuite/gdb.mi/mi-thread-bp-deleted.c
new file mode 100644
index 00000000000..5ac23b4ab25
--- /dev/null
+++ b/gdb/testsuite/gdb.mi/mi-thread-bp-deleted.c
@@ -0,0 +1,88 @@
+/* Copyright 2023 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>
+#include <unistd.h>
+#include <stdlib.h>
+
+#define NTHREAD 1
+
+/* Barrier for synchronising threads. */
+static pthread_barrier_t barrier;
+
+/* Wrapper around pthread_barrier_wait that aborts if the wait returns an
+ error. */
+static void
+barrier_wait (pthread_barrier_t *barrier)
+{
+ int res = pthread_barrier_wait (barrier);
+ if (res != PTHREAD_BARRIER_SERIAL_THREAD && res != 0)
+ abort ();
+}
+
+/* GDB can set this to 0 to force the 'spin' function to return. */
+volatile int do_spin = 1;
+
+void
+breakpt ()
+{
+ /* Nothing. */
+}
+
+/* Spin for a reasonably long while (this lets GDB run some commands in
+ async mode), but return early if/when do_spin is set to 0 (which is done
+ by GDB). */
+
+void
+spin ()
+{
+ int i;
+
+ for (i = 0; i < 300 && do_spin; ++i)
+ sleep (1);
+}
+
+void *
+thread_worker (void *arg)
+{
+ barrier_wait (&barrier);
+ return NULL;
+}
+
+int
+main ()
+{
+ pthread_t thr[NTHREAD];
+ int i;
+
+ if (pthread_barrier_init (&barrier, NULL, NTHREAD + 1) != 0)
+ abort ();
+
+ for (i = 0; i < NTHREAD; ++i)
+ pthread_create (&thr[i], NULL, thread_worker, NULL);
+
+ breakpt (); /* First breakpoint. */
+
+ barrier_wait (&barrier);
+
+ for (i = 0; i < NTHREAD; ++i)
+ pthread_join (thr[i], NULL);
+
+ spin ();
+
+ breakpt ();
+}
diff --git a/gdb/testsuite/gdb.mi/mi-thread-bp-deleted.exp b/gdb/testsuite/gdb.mi/mi-thread-bp-deleted.exp
new file mode 100644
index 00000000000..b8986170a8c
--- /dev/null
+++ b/gdb/testsuite/gdb.mi/mi-thread-bp-deleted.exp
@@ -0,0 +1,268 @@
+# 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/>.
+
+# Check that the =breakpoint-deleted notification for a thread-specific
+# breakpoint is sent as soon as the related thread exits, and not when the
+# inferior next stops.
+#
+# This test is based on gdb.threads/thread-bp-deleted.exp.
+
+load_lib mi-support.exp
+set MIFLAGS "-i=mi"
+
+# We need to do things a little differently when using the remote protocol.
+set is_remote \
+ [expr [target_info exists gdb_protocol] \
+ && ([string equal [target_info gdb_protocol] "remote"] \
+ || [string equal [target_info gdb_protocol] "extended-remote"])]
+
+standard_testfile
+
+if { [build_executable "failed to prepare" $testfile $srcfile \
+ {debug pthreads}] } {
+ return -1
+}
+
+foreach_mi_ui_mode mode {
+ mi_gdb_exit
+
+ if {$mode eq "separate"} {
+ set start_ops "separate-mi-tty"
+ } else {
+ set start_ops ""
+ }
+
+ # Restart, but enable non-stop mode, we need it for background
+ # execution.
+ save_vars { GDBFLAGS } {
+ append GDBFLAGS " -ex \"maint set target-non-stop on\""
+ append GDBFLAGS " -ex \"set mi-async on\""
+ mi_clean_restart $binfile $start_ops
+ }
+
+ mi_runto_main
+
+ if {![mi_detect_async]} {
+ unsupported "async-mode is required"
+ continue
+ }
+
+ mi_delete_breakpoints
+
+ # Place a breakpoint on 'breakpt' and run to this breakpoint.
+ mi_create_breakpoint "breakpt" "place breakpoint on breakpt"
+ set breakpt_num [mi_get_valueof "/d" "\$bpnum" "INVALID" \
+ "get number for breakpt breakpoint"]
+ mi_execute_to "exec-continue" "breakpoint-hit" "breakpt" "" \
+ ".*" ".*" {"" "disp=\"keep\""} \
+ "continue to breakpoint in breakpt"
+
+ # Now drain all the pending output from the CLI if we are using a separate
+ # UI.
+ if {$mode eq "separate"} {
+ with_spawn_id $gdb_main_spawn_id {
+ gdb_test_multiple "" "drain CLI output upto breakpoint" {
+ -re "Thread 1 \[^\r\n\]+ hit Breakpoint $decimal,\
+ breakpt \\(\\) at\
+ \[^\r\n\]+\r\n$decimal\\s+\[^\r\n\]+\r\n" {
+ pass $gdb_test_name
+ }
+ }
+ }
+ }
+
+ # This is just for convenience, this refers to the second thread the
+ # inferior spawns.
+ set worker_thread 2
+
+ # Create a thread-specific breakpoint.
+ mi_create_breakpoint "-p $worker_thread main" \
+ "place thread breakpoint on main" \
+ -thread "$worker_thread"
+ set bpnum [mi_get_valueof "/d" "\$bpnum" "INVALID" \
+ "get number for thread-specific breakpoint"]
+
+ set loc1 [mi_make_breakpoint -number "$breakpt_num"]
+ set loc2 [mi_make_breakpoint -number "$bpnum" -thread "$worker_thread"]
+ set table_2_locs [mi_make_breakpoint_table [list $loc1 $loc2]]
+ set table_1_locs [mi_make_breakpoint_table [list $loc1]]
+
+ mi_gdb_test "-break-info" \
+ "\\^done,$table_2_locs" \
+ "-break-info, expecting two locations"
+
+ # Resume the inferior, at this point the inferior will spin while
+ # we interact with it.
+ mi_send_resuming_command "exec-continue" "continue"
+
+ # Look for the thread-exited notification and the breakpoint-deleted
+ # notification. When using a single UI we see both the MI and CLI
+ # messages. When using a separate MI UI we only see the MI messages.
+ set saw_cli_thread_exited false
+ set saw_mi_thread_exited false
+ set saw_cli_bp_deleted false
+ set saw_mi_bp_deleted false
+
+ # When running with a remote target, the thread-exited event doesn't
+ # appear to be pushed from the target to GDB; instead GDB has to fetch the
+ # thread list from the target and spot that a thread exited.
+ #
+ # In order to achieve this, when running with a remote target we run the
+ # '-thread-info 99' command. There isn't a thread 99, but GDB doesn't
+ # know that until it fetches the thread list. By fetching the thread list
+ # GDB will spot that the thread we are interested in has exited.
+ if {$is_remote} {
+ set cmd "-thread-info 99"
+ set attempt_count 5
+ } else {
+ set cmd ""
+ set attempt_count 0
+ }
+
+ gdb_test_multiple $cmd "collect thread exited output" \
+ -prompt "$::mi_gdb_prompt$" {
+
+ -re "^~\"\\\[Thread \[^\r\n\]+ exited\\\]\\\\n\"\r\n" {
+ set saw_cli_thread_exited true
+ exp_continue
+ }
+
+ -re "^=thread-exited,id=\"$worker_thread\",group-id=\"i1\"\r\n" {
+ set saw_mi_thread_exited true
+ exp_continue
+ }
+
+ -re "^~\"Thread-specific breakpoint $bpnum deleted -\
+ thread $worker_thread no longer in the thread list\\.\\\\n\"\r\n" {
+ set saw_cli_bp_deleted true
+ exp_continue
+ }
+
+ -re "^=breakpoint-deleted,id=\"3\"\r\n" {
+ set saw_mi_bp_deleted true
+
+ # In remote mode we are still waiting for the result of the
+ # '-thread-info 99' command.
+ if {$is_remote} {
+ exp_continue
+ }
+
+ # We get here with a native target; check we saw all the output
+ # that we expected.
+ if {$mode eq "separate"} {
+ gdb_assert { $saw_mi_thread_exited && $saw_mi_bp_deleted \
+ && !$saw_cli_thread_exited \
+ && !$saw_cli_bp_deleted } \
+ $gdb_test_name
+ } else {
+ gdb_assert { $saw_mi_thread_exited && $saw_mi_bp_deleted \
+ && $saw_cli_thread_exited \
+ && $saw_cli_bp_deleted } \
+ $gdb_test_name
+ }
+ }
+
+ -re "^-thread-info 99\r\n" {
+ # This is the command being echoed back. This should only happen
+ # in remote mode, as for native targets we don't use the
+ # '-thread-info 99' command trick.
+ if {!$is_remote} {
+ fail "$gdb_test_name (unexpected output)"
+ }
+ exp_continue
+ }
+
+ -re "^\\^done,threads=\\\[\\\]\r\n$::mi_gdb_prompt$" {
+
+ # This is the result of the '-thread-info 99' trick, which is only
+ # used in remote mode. If we see this in native mode then
+ # something has gone wrong.
+ if {!$is_remote} {
+ fail "$gdb_test_name (unexpected output)"
+ }
+
+ # If we've not seen any of the expected output yet then maybe the
+ # remote thread just hasn't exited yet. Wait a short while and
+ # try again.
+ if { !$saw_mi_thread_exited && !$saw_mi_bp_deleted \
+ && $attempt_count > 0 } {
+ sleep 1
+ incr attempt_count -1
+ send_gdb "$cmd\n"
+ exp_continue
+ }
+
+ # The output has arrived! Check how we did. There are other bugs
+ # that come into play here which change which output we'll see.
+ if {$mode eq "separate" && !$is_remote} {
+ gdb_assert { $saw_mi_thread_exited && $saw_mi_bp_deleted \
+ && !$saw_cli_thread_exited \
+ && !$saw_cli_bp_deleted } \
+ $gdb_test_name
+ } else {
+ if { $saw_mi_thread_exited && $saw_mi_bp_deleted \
+ && $saw_cli_thread_exited \
+ && $saw_cli_bp_deleted } {
+ kpass "gdb/30129" $gdb_test_name
+ } elseif { $saw_mi_thread_exited && $saw_mi_bp_deleted \
+ && !$saw_cli_thread_exited \
+ && $saw_cli_bp_deleted } {
+ kfail "gdb/30129" $gdb_test_name
+ } else {
+ fail "$gdb_test_name (XXX)"
+ }
+ }
+ }
+ }
+
+ # When the MI is running on a separate UI the CLI message will be seen
+ # over there, but only if we are not running remote. When we are running
+ # remote then the thread-exited event will only be triggered as a result
+ # of user triggering a refresh of the thread list (hence the '-thread-info
+ # 99' trick above). By typing a command we change the current UI to the
+ # terminal we are typing at, as a result these CLI style message will
+ # actually appear on the MI when using a remote target.
+ if {$mode eq "separate" && !$is_remote} {
+ with_spawn_id $gdb_main_spawn_id {
+ set saw_thread_exited false
+ gdb_test_multiple "" "collect cli thread exited output" {
+ -re "\\\[Thread \[^\r\n\]+ exited\\\]\r\n" {
+ set saw_thread_exited true
+ exp_continue
+ }
+
+ -re "^Thread-specific breakpoint $bpnum deleted -\
+ thread $worker_thread no longer in the thread list\\.\r\n" {
+ gdb_assert { $saw_thread_exited } \
+ $gdb_test_name
+ }
+ }
+ }
+ }
+
+ mi_gdb_test "-break-info" \
+ "\\^done,$table_1_locs" \
+ "-break-info, expecting one location"
+
+ # Set 'do_spin' to zero, this allows the inferior to progress again; we
+ # should then hit the breakpoint in 'breakpt' again.
+ mi_gdb_test "set var do_spin = 0" \
+ [multi_line \
+ ".*=memory-changed,thread-group=\"i${decimal}\".addr=\"${hex}\",len=\"${hex}\"" \
+ "\\^done"] \
+ "set do_spin variable in inferior, inferior should now finish"
+ mi_expect_stop "breakpoint-hit" "breakpt" ".*" ".*" "$::decimal" \
+ {"" "disp=\"keep\""} "stop in breakpt at the end of the test"
+}
diff --git a/gdb/testsuite/gdb.threads/thread-bp-deleted.c b/gdb/testsuite/gdb.threads/thread-bp-deleted.c
new file mode 100644
index 00000000000..5ac23b4ab25
--- /dev/null
+++ b/gdb/testsuite/gdb.threads/thread-bp-deleted.c
@@ -0,0 +1,88 @@
+/* Copyright 2023 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>
+#include <unistd.h>
+#include <stdlib.h>
+
+#define NTHREAD 1
+
+/* Barrier for synchronising threads. */
+static pthread_barrier_t barrier;
+
+/* Wrapper around pthread_barrier_wait that aborts if the wait returns an
+ error. */
+static void
+barrier_wait (pthread_barrier_t *barrier)
+{
+ int res = pthread_barrier_wait (barrier);
+ if (res != PTHREAD_BARRIER_SERIAL_THREAD && res != 0)
+ abort ();
+}
+
+/* GDB can set this to 0 to force the 'spin' function to return. */
+volatile int do_spin = 1;
+
+void
+breakpt ()
+{
+ /* Nothing. */
+}
+
+/* Spin for a reasonably long while (this lets GDB run some commands in
+ async mode), but return early if/when do_spin is set to 0 (which is done
+ by GDB). */
+
+void
+spin ()
+{
+ int i;
+
+ for (i = 0; i < 300 && do_spin; ++i)
+ sleep (1);
+}
+
+void *
+thread_worker (void *arg)
+{
+ barrier_wait (&barrier);
+ return NULL;
+}
+
+int
+main ()
+{
+ pthread_t thr[NTHREAD];
+ int i;
+
+ if (pthread_barrier_init (&barrier, NULL, NTHREAD + 1) != 0)
+ abort ();
+
+ for (i = 0; i < NTHREAD; ++i)
+ pthread_create (&thr[i], NULL, thread_worker, NULL);
+
+ breakpt (); /* First breakpoint. */
+
+ barrier_wait (&barrier);
+
+ for (i = 0; i < NTHREAD; ++i)
+ pthread_join (thr[i], NULL);
+
+ spin ();
+
+ breakpt ();
+}
diff --git a/gdb/testsuite/gdb.threads/thread-bp-deleted.exp b/gdb/testsuite/gdb.threads/thread-bp-deleted.exp
new file mode 100644
index 00000000000..5f841cd1b35
--- /dev/null
+++ b/gdb/testsuite/gdb.threads/thread-bp-deleted.exp
@@ -0,0 +1,206 @@
+# Copyright (C) 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/>.
+
+# Check that 'maint info breakpoints' succeeds in the period of time
+# between a thread-specific breakpoint being deleted, and GDB next
+# stopping.
+#
+# There used to be a bug where GDB would try to lookup the thread for
+# the thread-specific breakpoint, the thread of course having been
+# deleted, couldn't be found, and GDB would end up dereferencing a
+# nullptr.
+
+standard_testfile
+
+if {[build_executable "failed to prepare" $testfile $srcfile \
+ {debug pthreads}] == -1} {
+ return -1
+}
+
+# We need to do things a little differently when using the remote protocol.
+set is_remote \
+ [expr [target_info exists gdb_protocol] \
+ && ([string equal [target_info gdb_protocol] "remote"] \
+ || [string equal [target_info gdb_protocol] "extended-remote"])]
+
+# This test requires background execution, which relies on non-stop mode.
+save_vars { GDBFLAGS } {
+ append GDBFLAGS " -ex \"maint set target-non-stop on\""
+ clean_restart ${binfile}
+}
+
+if {![runto_main]} {
+ return -1
+}
+
+# Check we hace non-stop mode. We do try to force this on above, but maybe
+# the target doesn't support non-stop mode, in which case (hopefully)
+# non-stop mode will still show as off, and this test should not be run.
+if {![is_target_non_stop]} {
+ unsupported "required non-stop mode"
+ return -1
+}
+
+delete_breakpoints
+
+gdb_breakpoint "breakpt"
+gdb_continue_to_breakpoint "continue to first breakpt call"
+set breakpt_num [get_integer_valueof "\$bpnum" "INVALID" \
+ "get number for breakpoint in breakpt"]
+
+# Check info threads just to confirm the thread numbering. The rest
+# of this script just assumes we have threads numbered 1 and 2.
+gdb_test "info threads" \
+ [multi_line \
+ "\\* 1\\s+Thread \[^\r\n\]+" \
+ " 2\\s+Thread \[^\r\n\]+"]
+
+set main_thread 1
+set worker_thread 2
+
+# Check the 'info breakpoints' output for the thread-specific breakpoint
+# numbered BPNUM. If EXPECTED is true then the breakpoint is expected to be
+# present, otherwise, the breakpoint is expected not to be present.
+
+proc check_for_thread_specific_breakpoint { testname bpnum expected } {
+ set saw_thread_specific_bp false
+ gdb_test_multiple "info breakpoints" $testname {
+ -re "^(\[^\r\n\]+)\r\n" {
+ set line $expect_out(1,string)
+ if { [regexp "$bpnum\\s+breakpoint\[^\r\n\]+ $::hex in main\
+ at \[^\r\n\]+" $line] } {
+ set saw_thread_specific_bp true
+ }
+ exp_continue
+ }
+ -re "^$::gdb_prompt $" {
+ set result [expr $expected ? $saw_thread_specific_bp \
+ : !$saw_thread_specific_bp]
+ gdb_assert { $result } $gdb_test_name
+ }
+ }
+}
+
+# Create a thread-specific breakpoint. This will never actually be hit; we
+# don't care, we just want to see GDB auto-delete this breakpoint.
+gdb_breakpoint "main thread $worker_thread" \
+ "create a thread-specific breakpoint"
+set bpnum [get_integer_valueof "\$bpnum" "INVALID" \
+ "get number for thread-specific breakpoint"]
+
+# Check the thread-specific breakpoint is present in 'info breakpoints'.
+check_for_thread_specific_breakpoint \
+ "check for thread-specific b/p before thread exit" $bpnum true
+
+# Continue in async mode. After this the worker thread will exit.
+gdb_test "continue&" "Continuing\\."
+
+if {$is_remote} {
+ # Collect the output from GDB telling us that the thread exited.
+ # Unfortunately in the remote protocol the thread-exited event doesn't
+ # appear to be pushed to GDB, instead we rely on GDB asking about the
+ # threads (which isn't great).
+ #
+ # So, what we do here is ask about thread 99, which hopefully shouldn't
+ # exist, however, in order to answer that question GDB has to grab the
+ # thread list from the remote, at which point GDB will spot that one of
+ # the threads has exited, and will tell us about it.
+ #
+ # However, we might be too quick sending the 'info threads 99' command,
+ # so, if we see the output of that command without any thread exited
+ # text, we wait for a short while and try again. We wait for upto 5
+ # seconds (5 tries). However, this might mean on a _really_ slow
+ # machine that the thread still hasn't exited. I guess if we start
+ # seeing that then we can just update ATTEMPT_COUNT below.
+ set saw_thread_exited false
+ set saw_bp_deleted false
+ set attempt_count 5
+ gdb_test_multiple "info threads 99" "collect thread exited output" {
+ -re "info threads 99\r\n" {
+ exp_continue
+ }
+
+ -re "^\\\[Thread \[^\r\n\]+ exited\\\]\r\n" {
+ set saw_thread_exited true
+ exp_continue
+ }
+
+ -re "^Thread-specific breakpoint $bpnum deleted -\
+ thread $worker_thread no longer in the thread list\\.\r\n" {
+ set saw_bp_deleted true
+ exp_continue
+ }
+
+ -re "No threads match '99'\\.\r\n$gdb_prompt $" {
+ if {!$saw_thread_exited && !$saw_bp_deleted && $attempt_count > 0} {
+ sleep 1
+ incr attempt_count -1
+ send_gdb "info threads 99\n"
+ exp_continue
+ }
+
+ # When PR gdb/30129 is fixed then this can all be collapsed down
+ # into a single gdb_assert call. This is split out like this
+ # because the SAW_BP_DELETED part is working, and we want to
+ # spot if that stops working.
+ if { $saw_thread_exited && $saw_bp_deleted } {
+ kpass "gdb/30129" $gdb_test_name
+ } elseif {!$saw_thread_exited && $saw_bp_deleted} {
+ kfail "gdb/30129" $gdb_test_name
+ } else {
+ fail $gdb_test_name
+ }
+ }
+ }
+} else {
+ # Collect the output from GDB telling us that the thread exited.
+ set saw_thread_exited false
+ gdb_test_multiple "" "collect thread exited output" {
+ -re "\\\[Thread \[^\r\n\]+ exited\\\]\r\n" {
+ set saw_thread_exited true
+ exp_continue
+ }
+
+ -re "^Thread-specific breakpoint $bpnum deleted -\
+ thread $worker_thread no longer in the thread list\\.\r\n" {
+ gdb_assert { $saw_thread_exited } \
+ $gdb_test_name
+ }
+ }
+}
+
+# Check the thread-specific breakpoint is no longer present in 'info
+# breakpoints' output.
+check_for_thread_specific_breakpoint \
+ "check for thread-specific b/p before after exit" $bpnum false
+
+# Check the thread-specific breakpoint doesn't show up in the 'maint
+# info breakpoints' output. And also that this doesn't cause GDB to
+# crash, which it did at one point.
+gdb_test_lines "maint info breakpoints" "" ".*" \
+ -re-not "breakpoint\\s+keep\\s+y\\s+$hex\\s+in main at "
+
+# Set the do_spin variable in the inferior. This will cause it to drop out
+# of its spin loop and hit the next breakpoint. Remember, at this point the
+# inferior is still executing.
+gdb_test "print do_spin = 0" "\\\$$decimal = 0"
+
+# Collect the notification that the inferior has stopped.
+gdb_test_multiple "" "wait for stop" {
+ -re "Thread $main_thread \[^\r\n\]+ hit Breakpoint ${breakpt_num},\
+ breakpt \\(\\) \[^\r\n\]+\r\n$decimal\\s+\[^\r\n\]+\r\n" {
+ pass $gdb_test_name
+ }
+}
diff --git a/gdb/thread.c b/gdb/thread.c
index 1a852f70206..5b472150a62 100644
--- a/gdb/thread.c
+++ b/gdb/thread.c
@@ -1431,6 +1431,8 @@ show_inferior_qualified_tids (void)
const char *
print_thread_id (struct thread_info *thr)
{
+ gdb_assert (thr != nullptr);
+
char *s = get_print_cell ();
if (show_inferior_qualified_tids ())
--
2.25.4
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH 8/8] gdb: fix mi breakpoint-deleted notifications for thread-specific b/p
2023-02-20 14:13 ` [PATCH 8/8] gdb: fix mi breakpoint-deleted notifications for thread-specific b/p Andrew Burgess
@ 2023-02-22 15:20 ` Pedro Alves
0 siblings, 0 replies; 19+ messages in thread
From: Pedro Alves @ 2023-02-22 15:20 UTC (permalink / raw)
To: Andrew Burgess, gdb-patches
Hi!
On 2023-02-20 2:13 p.m., Andrew Burgess via Gdb-patches wrote:
> My proposed solution is this; in remove_threaded_breakpoints, instead
> of setting the disposition to disp_del_at_next_stop and setting the
> number to zero, we now just call delete_breakpoint directly.
>
> The notification will now be sent out immediately; as soon as the
> thread exits.
>
> As the number has not changed when delete_breakpoint is called, the
> notification will have the correct number.
>
> And as the breakpoint is immediately removed from the breakpoint list,
> we no longer need to worry about 'maint info breakpoints' trying to
> print the thread-id for an exited thread.
>
> My only concern is that calling delete_breakpoint directly seems so
> obvious that I wonder why the original patch (that added
> remove_threaded_breakpoints) didn't take this approach. This code was
> added in commit 49fa26b0411d, but the commit message offers no clues
> to why this approach was taken, and the original email thread offers
> no insights either[2]. There are no test regressions after making
> this change, so I'm hopeful that this is going to be fine.
>
> [2] https://sourceware.org/pipermail/gdb-patches/2013-September/106493.html
I can't be absolutely sure there weren't other reasons, but I think that was because back
then we couldn't access inferior memory if the current thread was running or had exited, when
native debugging on GNU/Linux. One of the original versions of the step-over-thread-exit
series had that problem as well, which was what lead to changing gdb to always access
memory via /proc/pid/mem instead of ptrace, eliminating that class of problems.
>
> The Complication
> ----------------
>
> Of course, it couldn't be that simple.
It never is. :-)
> + # The output has arrived! Check how we did. There are other bugs
> + # that come into play here which change which output we'll see.
> + if {$mode eq "separate" && !$is_remote} {
> + gdb_assert { $saw_mi_thread_exited && $saw_mi_bp_deleted \
> + && !$saw_cli_thread_exited \
> + && !$saw_cli_bp_deleted } \
> + $gdb_test_name
> + } else {
> + if { $saw_mi_thread_exited && $saw_mi_bp_deleted \
> + && $saw_cli_thread_exited \
> + && $saw_cli_bp_deleted } {
> + kpass "gdb/30129" $gdb_test_name
> + } elseif { $saw_mi_thread_exited && $saw_mi_bp_deleted \
> + && !$saw_cli_thread_exited \
> + && $saw_cli_bp_deleted } {
> + kfail "gdb/30129" $gdb_test_name
> + } else {
> + fail "$gdb_test_name (XXX)"
I guess you meant to remove the XX or replace it with something else?
Otherwise LGTM.
Approved-By: Pedro Alves <pedro@palves.net>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 0/8] Fix missing MI =breakpoint-deleted notifications
2023-02-20 14:13 [PATCH 0/8] Fix missing MI =breakpoint-deleted notifications Andrew Burgess
` (7 preceding siblings ...)
2023-02-20 14:13 ` [PATCH 8/8] gdb: fix mi breakpoint-deleted notifications for thread-specific b/p Andrew Burgess
@ 2023-02-22 15:23 ` Pedro Alves
2023-02-28 11:09 ` Andrew Burgess
8 siblings, 1 reply; 19+ messages in thread
From: Pedro Alves @ 2023-02-22 15:23 UTC (permalink / raw)
To: Andrew Burgess, gdb-patches
On 2023-02-20 2:13 p.m., Andrew Burgess via Gdb-patches wrote:
> This series all sprang from a single sentence in this patch review:
>
> https://sourceware.org/pipermail/gdb-patches/2023-February/196560.html
>
> In this patch (adding inferior specific breakpoints), at one point, I
> changed a breakpoints number to zero. Pedro asks the very good question:
>
> Doesn't this mess up MI breakpoint deleted notifications?
>
> Clearly what I wrote was wrong. But, confession: I copied the code in
> question from the code to handle thread-specific breakpoints. And if
> Pedro's point was true for my inferior-speicifc b/p, then it must be
> true for thread-specific b/p too, which means we have a bug.
>
> Patch #1 is a trivial drive by clean up.
>
> Patch #2 is an interesting issue I ran into relating to the MI output
> while writing tests for this issue. This patch is worth a review
> because I'm proposing a slight change to the MI output.
>
> Patches #3 to #7 are just testsuite cleanup. There's no GDB changes
> in here. This is all about making it easier for me to write the tests
> in the last patch. You can probably skip these if you're short on
> review time.
>
> Patch #8 is the important one. This adjusts how we handle
> thread-specific breakpoints to avoid the issue Pedro pointed out
> above. This fix was mostly trivial, except for a nasty knock-on
> problem in the Python FinishBreakpoints code.
I read the whole series and replied to a number of patches where I
had something to say (though nothing major). Those I didn't reply to LGTM as is.
Thanks for doing this. Appreciated.
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH 0/8] Fix missing MI =breakpoint-deleted notifications
2023-02-22 15:23 ` [PATCH 0/8] Fix missing MI =breakpoint-deleted notifications Pedro Alves
@ 2023-02-28 11:09 ` Andrew Burgess
0 siblings, 0 replies; 19+ messages in thread
From: Andrew Burgess @ 2023-02-28 11:09 UTC (permalink / raw)
To: Pedro Alves, gdb-patches
Pedro Alves <pedro@palves.net> writes:
> On 2023-02-20 2:13 p.m., Andrew Burgess via Gdb-patches wrote:
>> This series all sprang from a single sentence in this patch review:
>>
>> https://sourceware.org/pipermail/gdb-patches/2023-February/196560.html
>>
>> In this patch (adding inferior specific breakpoints), at one point, I
>> changed a breakpoints number to zero. Pedro asks the very good question:
>>
>> Doesn't this mess up MI breakpoint deleted notifications?
>>
>> Clearly what I wrote was wrong. But, confession: I copied the code in
>> question from the code to handle thread-specific breakpoints. And if
>> Pedro's point was true for my inferior-speicifc b/p, then it must be
>> true for thread-specific b/p too, which means we have a bug.
>>
>> Patch #1 is a trivial drive by clean up.
>>
>> Patch #2 is an interesting issue I ran into relating to the MI output
>> while writing tests for this issue. This patch is worth a review
>> because I'm proposing a slight change to the MI output.
>>
>> Patches #3 to #7 are just testsuite cleanup. There's no GDB changes
>> in here. This is all about making it easier for me to write the tests
>> in the last patch. You can probably skip these if you're short on
>> review time.
>>
>> Patch #8 is the important one. This adjusts how we handle
>> thread-specific breakpoints to avoid the issue Pedro pointed out
>> above. This fix was mostly trivial, except for a nasty knock-on
>> problem in the Python FinishBreakpoints code.
>
> I read the whole series and replied to a number of patches where I
> had something to say (though nothing major). Those I didn't reply to LGTM as is.
>
> Thanks for doing this. Appreciated.
I made the changes you suggested and pushed this series.
Thanks,
Andrew
^ permalink raw reply [flat|nested] 19+ messages in thread