[PATCH] gdb/mi: check thread exists when creating thread-specific b/p

[Andrew Burgess <aburgess@redhat.com>]

I noticed the following behaviour:

  $ gdb -q -i=mi /tmp/hello.x
  =thread-group-added,id="i1"
  =cmd-param-changed,param="print pretty",value="on"
  ~"Reading symbols from /tmp/hello.x...\n"
  (gdb)
  -break-insert -p 99 main
  ^done,bkpt={number="1",type="breakpoint",disp="keep",enabled="y",addr="0x0000000000401198",func="main",file="/tmp/hello.c",fullname="/tmp/hello.c",line="18",thread-groups=["i1"],thread="99",times="0",original-location="main"}
  (gdb)
  info breakpoints
  &"info breakpoints\n"
  ~"Num     Type           Disp Enb Address            What\n"
  ~"1       breakpoint     keep y   0x0000000000401198 in main at /tmp/hello.c:18\n"
  &"../../src/gdb/thread.c:1434: internal-error: print_thread_id: Assertion `thr != nullptr' failed.\nA problem internal to GDB has been detected,\nfurther debugging may prove unreliable."
  &"\n"
  &"----- Backtrace -----\n"
  &"Backtrace unavailable\n"
  &"---------------------\n"
  &"\nThis is a bug, please report it."
  &"  For instructions, see:\n"
  &"<https://www.gnu.org/software/gdb/bugs/>.\n\n"
  Aborted (core dumped)

What we see here is that when using the MI a user can create
thread-specific breakpoints for non-existent threads.  Then if we try
to use the CLI 'info breakpoints' command GDB throws an assertion.
The assert is a result of the print_thread_id call when trying to
build the 'stop only in thread xx.yy' line; print_thread_id requires a
valid thread_info pointer, which we can't have for a non-existent
thread.

In contrast, when using the CLI we see this behaviour:

  $ gdb -q /tmp/hello.x
  Reading symbols from /tmp/hello.x...
  (gdb) break main thread 99
  Unknown thread 99.
  (gdb)

The CLI doesn't allow a breakpoint to be created for a non-existent
thread.  So the 'info breakpoints' command is always fine.

Interestingly, the MI -break-info command doesn't crash, this is
because the MI uses global thread-ids, and so never calls
print_thread_id.  However, GDB does support using CLI and MI in
parallel, so we need to solve this problem.

One option would be to change the CLI behaviour to allow printing
breakpoints for non-existent threads.  This would preserve the current
MI behaviour.

The other option is to pull the MI into line with the CLI and prevent
breakpoints being created for non-existent threads.  This is good for
consistency, but is a breaking change for the MI.

In the end I figured that it was probably better to retain the
consistent CLI behaviour, and just made the MI reject requests to
place a breakpoint on a non-existent thread.  The only test we had
that depended on the old behaviour was
gdb.mi/mi-thread-specific-bp.exp, which was added by me in commit:

  commit 2fd9a436c8d24eb0af85ccb3a2fbdf9a9c679a6c
  Date:   Fri Feb 17 10:48:06 2023 +0000

      gdb: don't duplicate 'thread' field in MI breakpoint output

I certainly didn't intend for this test to rely on this feature of the
MI, so I propose to update this test to only create breakpoints for
threads that exist.

Actually, I've added a new test that checks the MI rejects creating a
breakpoint for a non-existent thread, and I've also extended the test
to run with the separate MI/CLI UIs, and then tested 'info
breakpoints' to ensure this command doesn't crash.

I've extended the documentation of the `-p` flag to explain the
constraints better.

I have also added a NEWS entry just in case someone runs into this
issue, at least then they'll know this change in behaviour was
intentional.

One thing that I did wonder about while writing this patch, is whether
we should treat requests like this, on both the MI and CLI, as another
form of pending breakpoint, something like:

  (gdb) break foo thread 9
  Thread 9 does not exist.
  Make breakpoint pending on future thread creation? (y or [n]) y
  Breakpoint 1 (foo thread 9) pending.
  (gdb) info breakpoints
  Num     Type           Disp Enb Address    What
  1       breakpoint     keep y   <PENDING>  foo thread 9

Don't know if folk think that would be a useful idea or not?  Either
way, I think that would be a separate patch from this one.
---
 gdb/NEWS                                      |  6 ++
 gdb/doc/gdb.texinfo                           |  5 +-
 gdb/mi/mi-cmd-break.c                         |  2 +
 .../gdb.mi/mi-thread-specific-bp.exp          | 68 +++++++++++++++----
 4 files changed, 67 insertions(+), 14 deletions(-)

diff --git a/gdb/NEWS b/gdb/NEWS
index c32ff92c98a..e4a78dca2df 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -96,6 +96,12 @@ show always-read-ctf
    without a thread restriction.  The same is also true for the 'task'
    field of an Ada task-specific breakpoint.
 
+** It is no longer possible to create a thread-specific breakpoint for
+   a thread that doesn't exist using '-break-insert -p ID'.  Creating
+   breakpoints for non-existent threads is not allowed when using the
+   CLI, that the MI allowed it was a long standing bug, which has now
+   been fixed.
+
 *** Changes in GDB 13
 
 * MI version 1 is deprecated, and will be removed in GDB 14.
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index bfda7edc4f7..4a81580b061 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -32161,7 +32161,10 @@
 Initialize the @var{ignore-count}.
 @item -p @var{thread-id}
 Restrict the breakpoint to the thread with the specified global
-@var{thread-id}.
+@var{thread-id}.  @var{thread-id} must be a valid thread-id at the
+time the breakpoint is requested.  Breakpoints created with a
+@var{thread-id} will automatically be deleted when the corresponding
+thread exits.
 @item --qualified
 This option makes @value{GDBN} interpret a function name specified as
 a complete fully-qualified name.
diff --git a/gdb/mi/mi-cmd-break.c b/gdb/mi/mi-cmd-break.c
index 75957b75bad..e5432d58990 100644
--- a/gdb/mi/mi-cmd-break.c
+++ b/gdb/mi/mi-cmd-break.c
@@ -243,6 +243,8 @@ mi_cmd_break_insert_1 (int dprintf, const char *command, char **argv, int argc)
 	  break;
 	case THREAD_OPT:
 	  thread = atol (oarg);
+	  if (!valid_global_thread_id (thread))
+	    error (_("Unknown thread %d."), thread);
 	  break;
 	case PENDING_OPT:
 	  pending = 1;
diff --git a/gdb/testsuite/gdb.mi/mi-thread-specific-bp.exp b/gdb/testsuite/gdb.mi/mi-thread-specific-bp.exp
index 4586fa44bbc..9c04d6f8c2a 100644
--- a/gdb/testsuite/gdb.mi/mi-thread-specific-bp.exp
+++ b/gdb/testsuite/gdb.mi/mi-thread-specific-bp.exp
@@ -29,21 +29,63 @@ 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]
+foreach_mi_ui_mode mode {
+
+    if {$mode == "separate"} {
+	set start_ops "separate-mi-tty"
+    } else {
+	set start_ops ""
+    }
+
+    if {[mi_clean_restart $binfile $start_ops]} {
+	return -1
+    }
+
+    # Ensure we get an error when placing a b/p for thread 1 at a point
+    # where thread 1 doesn't exist.
+    mi_gdb_test "-break-insert -p 1 bar" \
+	"\\^error,msg=\"Unknown thread 1\\.\""
 
-mi_create_breakpoint_multi "-p 1 foo" "thread-specific b/p on foo" \
-    -thread "1" \
-    -locations "\\\[$loc1,$loc2,$loc3\\\]"
+    # If we have a separate CLI UI then run the 'info breakpoints'
+    # command.  There was a time when the previous breakpoint request
+    # would succeed, and then 'info breakpoint' on the CLI would
+    # trigger an assertion.
+    if {$mode eq "separate"} {
+	with_spawn_id $gdb_main_spawn_id {
+	    gdb_test "info breakpoints" "No breakpoints or watchpoints\\." \
+		"check CLI 'info breakpoints' when there are no breakpoints"
+	}
+    }
+
+    if {[mi_runto_main] == -1} {
+	return -1
+    }
+
+    # Ensure we get an error when placing a b/p for a thread that doesn't
+    # exist (when other threads do exist).
+    mi_gdb_test "-break-insert -p 999 bar" \
+	"\\^error,msg=\"Unknown thread 999\\.\""
+
+    mi_create_breakpoint "-p 1 bar" "thread-specific b/p on bar" \
+	-thread "1"
+
+    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\\\]"
+
+    # Check that 'info breakpoints' on the CLI succeeds.
+    if {$mode eq "separate"} {
+	with_spawn_id $gdb_main_spawn_id {
+	    gdb_test "info breakpoints" ".*" \
+		"check CLI 'info breakpoints' when there are some breakpoints"
+	}
+    }
+}

base-commit: 6a208145d24c47912c8beb4f1f4b9abeb8d51134
-- 
2.25.4