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

[Andrew Burgess <aburgess@redhat.com>]

Andrew Burgess <aburgess@redhat.com> writes:

> 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.
> ---

I've gone ahead and pushed this change.  This was blocking another
series I'd like to post to the list.

Let me know if there are any problems.

Thanks,
Andrew



>  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