public inbox for gdb-cvs@sourceware.org
help / color / mirror / Atom feed
* [binutils-gdb] gdb: fix mi breakpoint-deleted notifications for thread-specific b/p
@ 2023-02-28 11:09 Andrew Burgess
0 siblings, 0 replies; only message in thread
From: Andrew Burgess @ 2023-02-28 11:09 UTC (permalink / raw)
To: gdb-cvs
https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=2968b79fca38cf18e8eef360c36de7a6e3846d3c
commit 2968b79fca38cf18e8eef360c36de7a6e3846d3c
Author: Andrew Burgess <aburgess@redhat.com>
Date: Thu Feb 16 13:06:29 2023 +0000
gdb: fix mi breakpoint-deleted notifications for thread-specific b/p
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.
Approved-By: Pedro Alves <pedro@palves.net>
Diff:
---
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 | 290 ++++++++++++++++++++++++
gdb/testsuite/gdb.threads/thread-bp-deleted.c | 88 +++++++
gdb/testsuite/gdb.threads/thread-bp-deleted.exp | 210 +++++++++++++++++
gdb/thread.c | 2 +
9 files changed, 709 insertions(+), 12 deletions(-)
diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 36c53e5dae6..a42d26fd25a 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 159164e8009..7122fa820f6 100644
--- a/gdb/python/py-finishbreakpoint.c
+++ b/gdb/python/py-finishbreakpoint.c
@@ -349,16 +349,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;
@@ -379,7 +382,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)
{
@@ -390,6 +397,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. */
@@ -399,7 +417,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
@@ -411,7 +430,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 55bbe78a7a5..258f5c42537 100644
--- a/gdb/python/python-internal.h
+++ b/gdb/python/python-internal.h
@@ -777,6 +777,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..0ebca924801
--- /dev/null
+++ b/gdb/testsuite/gdb.mi/mi-thread-bp-deleted.exp
@@ -0,0 +1,290 @@
+# 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-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 "^=thread-exited,id=\"$worker_thread\",group-id=\"i1\"\r\n" {
+ set saw_mi_thread_exited true
+
+ # The order of the MI notifications depends on the order in which
+ # the observers where registered within GDB. If we have not seen
+ # the other MI notification yet then keep looking.
+ #
+ # Additionally, for remote targets, we're going to wait for the
+ # output of the '-thread-info 99' command before we check the
+ # results.
+ if {!$saw_mi_bp_deleted || $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 "^=breakpoint-deleted,id=\"3\"\r\n" {
+ set saw_mi_bp_deleted true
+
+ # The order of the MI notifications depends on the order in which
+ # the observers where registered within GDB. If we have not seen
+ # the other MI notification yet then keep looking.
+ #
+ # Additionally, for remote targets, we're going to wait for the
+ # output of the '-thread-info 99' command before we check the
+ # results.
+ if {!$saw_mi_thread_exited || $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" {
+ if {!$is_remote} {
+ fail "$gdb_test_name (unexpected output)"
+ }
+ # This is the command being echoed back, ignore it.
+ 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 \
+ && !$saw_cli_thread_exited && !$saw_cli_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 what output we'll see.
+ 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"
+ }
+ }
+ }
+
+ # 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..019bdddee81
--- /dev/null
+++ b/gdb/testsuite/gdb.threads/thread-bp-deleted.exp
@@ -0,0 +1,210 @@
+# 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.
+# The -no-prompt-anchor is needed here as sometimes the exit of the
+# worker thread will happen so quickly that expect will see the
+# 'thread exited' message immediately after the prompt, which breaks
+# the normal gdb_test prompt anchoring.
+gdb_test -no-prompt-anchor "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 ())
^ permalink raw reply [flat|nested] only message in thread
only message in thread, other threads:[~2023-02-28 11:09 UTC | newest]
Thread overview: (only message) (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-02-28 11:09 [binutils-gdb] gdb: fix mi breakpoint-deleted notifications for thread-specific b/p Andrew Burgess
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).