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>

2 files changed, 378 insertions, 0 deletions

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 0000000..5ac23b4
--- /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 0000000..0ebca92
--- /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"
+}