From 00cdd79a5d3f910c1362b9c4654adea3db0a97de Mon Sep 17 00:00:00 2001 From: Andrew Burgess Date: Fri, 3 Mar 2023 23:17:39 +0000 Subject: [PATCH] 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" &".\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 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 --- 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 54b5da21245..e3c095de09e 100644 --- a/gdb/NEWS +++ b/gdb/NEWS @@ -113,6 +113,12 @@ info main 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. + * Python API ** The gdb.unwinder.Unwinder.name attribute is now read-only. diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo index 2dad3ef1677..b21823d966d 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. 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" + } + } +} -- 2.30.2