Re: [PATCH] Fix MI "-break-insert -g i<UNKNOWN>" assertion failure

[Simon Marchi <simark@simark.ca>]

On 5/4/26 10:52 AM, Pedro Alves wrote:
> Passing a non-existing inferior to -break-insert's -g option trips an
> assertion:
> 
>  (gdb) interpreter-exec mi "222-break-insert -g i100 foo"
>  &"../../src/gdb/breakpoint.c:9165: internal-error: find_program_space_for_breakpoint: Assertion `inf != nullptr' failed.\nA problem internal to GDB has been detected,\nfurther debugging may prove unreliable."
>  &"\n"
>  ...
> 
> From here:
> 
>  (top-gdb) bt
>  #0  internal_error_loc (file=0x555556187e56 "../../src/gdb/breakpoint.c", line=9165, fmt=0x555556187bc8 "%s: Assertion `%s' failed.") at ../../src/gdbsupport/errors.cc:53
>  #1  0x0000555555791a55 in find_program_space_for_breakpoint (thread=-1, inferior=100) at ../../src/gdb/breakpoint.c:9165
>  #2  0x0000555555791f96 in create_breakpoint (gdbarch=0x5555568db900, locspec=0x5555567e7100, cond_string=0x0, thread=-1, inferior=100, extra_string=0x0, force_condition=false, parse_extra=0, tempflag=0, type_wanted=bp_breakpoint, ignore_count=0, pending_break_support=AUTO_BOOLEAN_FALSE, ops=0x555556642aa0 <code_breakpoint_ops>, from_tty=0, enabled=1, internal=0, flags=0) at ../../src/gdb/breakpoint.c:9275
>  #3  0x0000555555bb9d15 in mi_cmd_break_insert_1 (dprintf=0, command=0x5555567e6fd0 "break-insert", argv=0x5555567e7070, argc=3) at ../../src/gdb/mi/mi-cmd-break.c:366
>  #4  0x0000555555bb9e15 in mi_cmd_break_insert (command=0x5555567e6fd0 "break-insert", argv=0x5555567e7070, argc=3) at ../../src/gdb/mi/mi-cmd-break.c:383
>  ...
> 
> This commit fixes it by adding an input validation check to
> mi_cmd_break_insert_1, similar to how we validate global thread
> numbers for "-p THREAD", just a few lines above.
> 
> gdb.mi/mi-thread-specific-bp.exp already exercises the similar case
> for thread-specific breakpoints.  Extended it to test
> inferior-specific breakpoints too.
> 
> In the GDB manual, describe that the inferior passed to `-g` must be
> valid, exactly like commit 00cdd79a5d ("gdb/mi: check thread exists
> when creating thread-specific b/p") did for `-p THREAD`
> 
> Change-Id: Ibde0d4d098bf0b5d7b057e818a77a63c84806a3c
> commit-id:b791b7ee

Is this "commit-id" trailer on purpose?

It confused "b4 shazam", because when I applied the patch locally it
converted it to:

    commit-id:b791b7ee

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

Maybe it's the lack of space after the colon.

The patch LGTM, I noted some minor comments below.

Approved-By: Simon Marchi <simon.marchi@efficios.com>

> diff --git a/gdb/inferior.c b/gdb/inferior.c
> index 1481f46cdd1..931115f46c1 100644
> --- a/gdb/inferior.c
> +++ b/gdb/inferior.c
> @@ -389,6 +389,15 @@ find_inferior_id (int num)
>    return NULL;
>  }
>  
> +/* See inferior.h.  */
> +
> +bool
> +valid_inferior_id (int num)
> +{
> +  inferior *inf = find_inferior_id (num);
> +  return inf != nullptr;
> +}

IMO you can get rid of the inf variable (but it's fine if it was a
conscious choice, sometimes intermediate variables make debugging
easier because that gives you something to print).

> +proc do_test { mode specificity_kind } {
> +
> +    if { $specificity_kind == "thread" } {
> +	# Ensure we get an error when placing a b/p for thread 1 at a
> +	# point where thread 1 doesn't exist.  This test doesn't make
> +	# sense for inferior-specific breakpoints.
> +	mi_gdb_test "-break-insert -p 1 bar" \
> +	    "\\^error,msg=\"Unknown thread 1\\.\""
> +    }

The comment above is not clear to me.  Does this mean to test adding a
thread specific breakpoints when _no_ threads exist at all?  Because
below we have another similar test, but when threads exist.  If so, the
comment could say "at a point where threads don't exist" instead of
"where thread 1 doesn't exist".  And then I would understand why it
doesn't make sense for inferiors: because there is always at least one
inferior.

> @@ -90,17 +117,20 @@ foreach_mi_ui_mode mode {
>  	set start_ops ""
>      }
>  
> -    if {[mi_clean_restart $::testfile $start_ops]} {
> -	break
> -    }
> +    foreach_with_prefix specificity_kind {"inferior" "thread" } {
>  
> -    set res [do_test $mode]
> +	if {[mi_clean_restart $::testfile $start_ops]} {
> +	    break
> +	}
> +
> +	set res [do_test $mode $specificity_kind]
>  
> -    # mi_clean_restart and gdb_finish call gdb_exit, which doesn't work for
> -    # separate-mi-tty.  Use mi_gdb_exit instead.
> -    mi_gdb_exit
> +	# mi_clean_restart and gdb_finish call gdb_exit, which doesn't
> +	# work for separate-mi-tty.  Use mi_gdb_exit instead.
> +	mi_gdb_exit
>  
> -    if { $res == -1 } {
> -	break
> +	if { $res == -1 } {
> +	    break

Not a big deal but: I guess those "break"s were meant to exist the test
case completely when something goes wrong?  Now, with the nested for
loops, it won't do that.  We typically don't do that, unless we know for
a fact that letting the test run after some failure will cause lengthy,
cascading failures.

Simon