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

[Simon Marchi <simark@simark.ca>]

On 5/4/26 1:49 PM, Pedro Alves wrote:
> Hi!
> 
> On 2026-05-04 17:22, Simon Marchi wrote:
>> 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?
> 
> Yes, it's for "git spr".
> 
>>
>> 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.
> 
> Could well be.  git spr used to be picky until very recently and use (and require) the
> non-standard format with no space.  That was fixed very recently, I'll look into rebasing my
> fork to pick the fix.
> 
>>
>> 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).
> 
> I don't mind either way.  I'll change it.
> 
>>
>>> +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?  
> 
> Yes, I think so.
> 
>> 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.
> 
> Done.
> 
>>
>>> @@ -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.
>>
> I think we should just remove the early break.  We always clean-restart for
> each iteration, so it's not a case of cascading failures.
> 
> Here's a v2 with those changes.

We are not using Gerrit and I can't easily diff between versions, so I
trust that you did the corrections correctly :).

Simon