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

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

[Pedro Alves]

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

---
I propose renaming the testcase to
gdb.mi/mi-thread-inferior-specific-bp.exp, but I'm not doing it in
this patch, as by default git diff isn't able to detect that this
would be a rename + modifications, instead presenting the diff as a
delete + new file.

Change-Id: I468e62bfb02e85d97bd43c54ea8c2e4532972434
commit-id:15849578
---
 gdb/doc/gdb.texinfo                           |  3 +-
 gdb/inferior.c                                |  9 ++
 gdb/inferior.h                                |  3 +
 gdb/mi/mi-cmd-break.c                         |  3 +
 .../gdb.mi/mi-thread-specific-bp.exp          | 88 +++++++++++++------
 5 files changed, 76 insertions(+), 30 deletions(-)

diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index ab0216ff477..435265bd74f 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -33515,7 +33515,8 @@ time the breakpoint is requested.  Breakpoints created with a
 thread exits.
 @item -g @var{thread-group-id}
 Restrict the breakpoint to the thread group with the specified
-@var{thread-group-id}.
+@var{thread-group-id}.  @var{thread-group-id} must be a valid
+thread-group-id at the time the breakpoint is requested.
 @item --qualified
 This option makes @value{GDBN} interpret a function name specified as
 a complete fully-qualified name.
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;
+}
+
 struct inferior *
 find_inferior_pid (process_stratum_target *targ, int pid)
 {
diff --git a/gdb/inferior.h b/gdb/inferior.h
index 9c031035a23..a483e60a7ef 100644
--- a/gdb/inferior.h
+++ b/gdb/inferior.h
@@ -754,6 +754,9 @@ extern struct inferior *find_inferior_ptid (process_stratum_target *targ,
 /* Search function to lookup an inferior by GDB 'num'.  */
 extern struct inferior *find_inferior_id (int num);
 
+/* Return true if inferior NUM exists, false otherwise.  */
+extern bool valid_inferior_id (int num);
+
 /* Find an inferior bound to PSPACE, giving preference to the current
    inferior.  */
 extern struct inferior *
diff --git a/gdb/mi/mi-cmd-break.c b/gdb/mi/mi-cmd-break.c
index 1771e5208af..2231e90c192 100644
--- a/gdb/mi/mi-cmd-break.c
+++ b/gdb/mi/mi-cmd-break.c
@@ -29,6 +29,7 @@
 #include "location.h"
 #include "linespec.h"
 #include "tracepoint.h"
+#include "inferior.h"
 
 enum
   {
@@ -248,6 +249,8 @@ mi_cmd_break_insert_1 (int dprintf, const char *command,
 	  break;
 	case THREAD_GROUP_OPT:
 	  thread_group = mi_parse_thread_group_id (oarg);
+	  if (!valid_inferior_id (thread_group))
+	    error (_("Unknown thread-group %d."), thread_group);
 	  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 9f250912b2c..53a0c7878d0 100644
--- a/gdb/testsuite/gdb.mi/mi-thread-specific-bp.exp
+++ b/gdb/testsuite/gdb.mi/mi-thread-specific-bp.exp
@@ -13,8 +13,8 @@
 # You should have received a copy of the GNU General Public License
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
-# This test is for creating thread-specific breakpoint using the MI,
-# and checking the results from GDB.
+# Create thread- and inferior-specific breakpoints using the MI, and
+# check the results from GDB.
 
 load_lib mi-support.exp
 set MIFLAGS "-i=mi"
@@ -29,16 +29,41 @@ if {[build_executable ${testfile}.exp ${binfile} ${srcfile}]} {
     return -1
 }
 
-proc make_loc {num} {
-    return [mi_make_breakpoint_loc -thread "1" -number "$::decimal\\.$num"]
+# SPECIFICITY_KIND can be "inferior" or "thread".
+proc make_loc {specificity_kind num} {
+    return [mi_make_breakpoint_loc -$specificity_kind "1" -number "$::decimal\\.$num"]
 }
 
-proc do_test { mode } {
+# Return the -break-insert option for inserting a breakpoint that is
+# specific to NUM, of SPECIFICITY_KIND kind.
+proc make_opt {specificity_kind num} {
+    if { $specificity_kind == "thread" } {
+	return "-p $num"
+    } elseif { $specificity_kind == "inferior" } {
+	return "-g i$num"
+    } else {
+	error "unknown specificity kind $specificity_kind"
+    }
+}
+
+# Convert SPECIFICITY_KIND to the corresponding MI attribute.
+proc as_mi_kind {specificity_kind} {
+    if { $specificity_kind == "inferior" } {
+	return "thread-group"
+    } else {
+	return $specificity_kind
+    }
+}
 
-    # 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\\.\""
+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\\.\""
+    }
 
     # If we have a separate CLI UI then run the 'info breakpoints'
     # command.  There was a time when the previous breakpoint request
@@ -55,20 +80,22 @@ proc do_test { mode } {
 	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\\.\""
+    # Ensure we get an error when placing a b/p for a thread/inferior
+    # that doesn't exist.
+    mi_gdb_test "-break-insert [make_opt $specificity_kind 999] bar" \
+	"\\^error,msg=\"Unknown [as_mi_kind $specificity_kind] 999\\.\""
 
-    mi_create_breakpoint "-p 1 bar" "thread-specific b/p on bar" \
-	-thread "1"
+    mi_create_breakpoint "[make_opt $specificity_kind 1] bar" \
+	"inferior-specific b/p on bar" \
+	-$specificity_kind "1"
 
-    set loc1 [make_loc 1]
-    set loc2 [make_loc 2]
-    set loc3 [make_loc 3]
+    set loc1 [make_loc $specificity_kind 1]
+    set loc2 [make_loc $specificity_kind 2]
+    set loc3 [make_loc $specificity_kind 3]
 
-    mi_create_breakpoint_multi "-p 1 foo" "thread-specific b/p on foo" \
-	-thread "1" \
+    mi_create_breakpoint_multi "[make_opt $specificity_kind 1] foo" \
+	"$specificity_kind-specific b/p on foo" \
+	-$specificity_kind "1" \
 	-locations "\\\[$loc1,$loc2,$loc3\\\]"
 
     # Check that 'info breakpoints' on the CLI succeeds.
@@ -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
+	}
     }
 }

base-commit: 8c0ac471835ec86a67c5b42713d9f138f31e4014
-- 
2.53.0

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

[Eli Zaretskii]

> From: Pedro Alves <pedro@palves.net>
> Date: Mon,  4 May 2026 15:52:42 +0100
> 
>  gdb/doc/gdb.texinfo                           |  3 +-
>  gdb/inferior.c                                |  9 ++
>  gdb/inferior.h                                |  3 +
>  gdb/mi/mi-cmd-break.c                         |  3 +
>  .../gdb.mi/mi-thread-specific-bp.exp          | 88 +++++++++++++------
>  5 files changed, 76 insertions(+), 30 deletions(-)

Thanks, the gdb.texinfo part is approved.

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

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

[Simon Marchi]

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

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

[Pedro Alves]

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.

From 7942a0a716a9c05e69f24cde39c32b20d2dfad24 Mon Sep 17 00:00:00 2001
From: Pedro Alves <pedro@palves.net>
Date: Wed, 9 Apr 2025 14:29:53 +0100
Subject: [PATCH v2] Fix MI "-break-insert -g i<UNKNOWN>" assertion failure

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

---
I propose renaming the testcase to
gdb.mi/mi-thread-inferior-specific-bp.exp, but I'm not doing it in
this patch, as by default git diff isn't able to detect that this
would be a rename + modifications, instead presenting the diff as a
delete + new file.

Change-Id: I468e62bfb02e85d97bd43c54ea8c2e4532972434
commit-id:15849578
---
 gdb/doc/gdb.texinfo                           |  3 +-
 gdb/inferior.c                                |  8 ++
 gdb/inferior.h                                |  3 +
 gdb/mi/mi-cmd-break.c                         |  3 +
 .../gdb.mi/mi-thread-specific-bp.exp          | 85 ++++++++++++-------
 5 files changed, 72 insertions(+), 30 deletions(-)

diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index ab0216ff477..435265bd74f 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -33515,7 +33515,8 @@ time the breakpoint is requested.  Breakpoints created with a
 thread exits.
 @item -g @var{thread-group-id}
 Restrict the breakpoint to the thread group with the specified
-@var{thread-group-id}.
+@var{thread-group-id}.  @var{thread-group-id} must be a valid
+thread-group-id at the time the breakpoint is requested.
 @item --qualified
 This option makes @value{GDBN} interpret a function name specified as
 a complete fully-qualified name.
diff --git a/gdb/inferior.c b/gdb/inferior.c
index 1481f46cdd1..c0596b96158 100644
--- a/gdb/inferior.c
+++ b/gdb/inferior.c
@@ -389,6 +389,14 @@ find_inferior_id (int num)
   return NULL;
 }
 
+/* See inferior.h.  */
+
+bool
+valid_inferior_id (int num)
+{
+  return find_inferior_id (num) != nullptr;
+}
+
 struct inferior *
 find_inferior_pid (process_stratum_target *targ, int pid)
 {
diff --git a/gdb/inferior.h b/gdb/inferior.h
index 9c031035a23..a483e60a7ef 100644
--- a/gdb/inferior.h
+++ b/gdb/inferior.h
@@ -754,6 +754,9 @@ extern struct inferior *find_inferior_ptid (process_stratum_target *targ,
 /* Search function to lookup an inferior by GDB 'num'.  */
 extern struct inferior *find_inferior_id (int num);
 
+/* Return true if inferior NUM exists, false otherwise.  */
+extern bool valid_inferior_id (int num);
+
 /* Find an inferior bound to PSPACE, giving preference to the current
    inferior.  */
 extern struct inferior *
diff --git a/gdb/mi/mi-cmd-break.c b/gdb/mi/mi-cmd-break.c
index 1771e5208af..2231e90c192 100644
--- a/gdb/mi/mi-cmd-break.c
+++ b/gdb/mi/mi-cmd-break.c
@@ -29,6 +29,7 @@
 #include "location.h"
 #include "linespec.h"
 #include "tracepoint.h"
+#include "inferior.h"
 
 enum
   {
@@ -248,6 +249,8 @@ mi_cmd_break_insert_1 (int dprintf, const char *command,
 	  break;
 	case THREAD_GROUP_OPT:
 	  thread_group = mi_parse_thread_group_id (oarg);
+	  if (!valid_inferior_id (thread_group))
+	    error (_("Unknown thread-group %d."), thread_group);
 	  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 9f250912b2c..9213c55adc5 100644
--- a/gdb/testsuite/gdb.mi/mi-thread-specific-bp.exp
+++ b/gdb/testsuite/gdb.mi/mi-thread-specific-bp.exp
@@ -13,8 +13,8 @@
 # You should have received a copy of the GNU General Public License
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
-# This test is for creating thread-specific breakpoint using the MI,
-# and checking the results from GDB.
+# Create thread- and inferior-specific breakpoints using the MI, and
+# check the results from GDB.
 
 load_lib mi-support.exp
 set MIFLAGS "-i=mi"
@@ -29,16 +29,42 @@ if {[build_executable ${testfile}.exp ${binfile} ${srcfile}]} {
     return -1
 }
 
-proc make_loc {num} {
-    return [mi_make_breakpoint_loc -thread "1" -number "$::decimal\\.$num"]
+# SPECIFICITY_KIND can be "inferior" or "thread".
+proc make_loc {specificity_kind num} {
+    return [mi_make_breakpoint_loc -$specificity_kind "1" -number "$::decimal\\.$num"]
 }
 
-proc do_test { mode } {
+# Return the -break-insert option for inserting a breakpoint that is
+# specific to NUM, of SPECIFICITY_KIND kind.
+proc make_opt {specificity_kind num} {
+    if { $specificity_kind == "thread" } {
+	return "-p $num"
+    } elseif { $specificity_kind == "inferior" } {
+	return "-g i$num"
+    } else {
+	error "unknown specificity kind $specificity_kind"
+    }
+}
 
-    # 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\\.\""
+# Convert SPECIFICITY_KIND to the corresponding MI attribute.
+proc as_mi_kind {specificity_kind} {
+    if { $specificity_kind == "inferior" } {
+	return "thread-group"
+    } else {
+	return $specificity_kind
+    }
+}
+
+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 no threads exist yet.  This test doesn't make
+	# sense for inferior-specific breakpoints, as there is always
+	# at least one inferior.
+	mi_gdb_test "-break-insert -p 1 bar" \
+	    "\\^error,msg=\"Unknown thread 1\\.\""
+    }
 
     # If we have a separate CLI UI then run the 'info breakpoints'
     # command.  There was a time when the previous breakpoint request
@@ -55,20 +81,22 @@ proc do_test { mode } {
 	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\\.\""
+    # Ensure we get an error when placing a b/p for a thread/inferior
+    # that doesn't exist.
+    mi_gdb_test "-break-insert [make_opt $specificity_kind 999] bar" \
+	"\\^error,msg=\"Unknown [as_mi_kind $specificity_kind] 999\\.\""
 
-    mi_create_breakpoint "-p 1 bar" "thread-specific b/p on bar" \
-	-thread "1"
+    mi_create_breakpoint "[make_opt $specificity_kind 1] bar" \
+	"inferior-specific b/p on bar" \
+	-$specificity_kind "1"
 
-    set loc1 [make_loc 1]
-    set loc2 [make_loc 2]
-    set loc3 [make_loc 3]
+    set loc1 [make_loc $specificity_kind 1]
+    set loc2 [make_loc $specificity_kind 2]
+    set loc3 [make_loc $specificity_kind 3]
 
-    mi_create_breakpoint_multi "-p 1 foo" "thread-specific b/p on foo" \
-	-thread "1" \
+    mi_create_breakpoint_multi "[make_opt $specificity_kind 1] foo" \
+	"$specificity_kind-specific b/p on foo" \
+	-$specificity_kind "1" \
 	-locations "\\\[$loc1,$loc2,$loc3\\\]"
 
     # Check that 'info breakpoints' on the CLI succeeds.
@@ -90,17 +118,16 @@ 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
+	}
 
-    # 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
+	do_test $mode $specificity_kind
 
-    if { $res == -1 } {
-	break
+	# 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
     }
 }

base-commit: 8c0ac471835ec86a67c5b42713d9f138f31e4014
-- 
2.53.0

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

[Simon Marchi]

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