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

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.

Reviewed-By: Eli Zaretskii <eliz@gnu.org>

1 files changed, 4 insertions, 1 deletions

diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 2dad3ef..b21823d 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -32215,7 +32215,10 @@ all of the breakpoint locations.
 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.