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

[Pedro Alves <pedro@palves.net>]

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