From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id Q5ebMZHh+GmPIxMAWB0awg (envelope-from ) for ; Mon, 04 May 2026 14:12:33 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=simark.ca; s=mail; t=1777918353; bh=Zh5RH+rZSPnMsDN1xgqar0dnxQ0haTtoE2ovqHvFhOU=; h=Date:Subject:To:References:From:In-Reply-To:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From; b=npk7GKfO8M+yWgxInQBKAAmhwZldqwAcQedLiwp0NhosWnNLMaAI7TWex7gs/9As4 AYqfeHExMNEquCWkOiyoSa4z0iPatsF3hAjSTt4msfkbImGXoyFwB0QeLzDE6h5EEI dCW03HIlTmmK5Adt2qZauQmoEhCM+LuIU62RfnaA= Received: by simark.ca (Postfix, from userid 112) id B10FE1E0BA; Mon, 04 May 2026 14:12:33 -0400 (EDT) X-Spam-Checker-Version: SpamAssassin 4.0.1 (2024-03-25) on simark.ca X-Spam-Level: X-Spam-Status: No, score=-2.4 required=5.0 tests=ARC_SIGNED,ARC_VALID,BAYES_00, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,MAILING_LIST_MULTI, RCVD_IN_DNSWL_MED,RCVD_IN_VALIDITY_CERTIFIED_BLOCKED, RCVD_IN_VALIDITY_RPBL_BLOCKED,RCVD_IN_VALIDITY_SAFE_BLOCKED,WEIRD_PORT autolearn=ham autolearn_force=no version=4.0.1 Authentication-Results: simark.ca; dkim=pass (1024-bit key; unprotected) header.d=simark.ca header.i=@simark.ca header.a=rsa-sha256 header.s=mail header.b=WYOp7lAI; dkim-atps=neutral Received: from vm01.sourceware.org (vm01.sourceware.org [38.145.34.32]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange x25519 server-signature ECDSA (prime256v1) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPS id 8DDA31E067 for ; Mon, 04 May 2026 14:12:32 -0400 (EDT) Received: from vm01.sourceware.org (localhost [127.0.0.1]) by sourceware.org (Postfix) with ESMTP id 0C7804BB1C18 for ; Mon, 4 May 2026 18:12:32 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 0C7804BB1C18 Authentication-Results: sourceware.org; dkim=pass (1024-bit key, unprotected) header.d=simark.ca header.i=@simark.ca header.a=rsa-sha256 header.s=mail header.b=WYOp7lAI Received: from simark.ca (simark.ca [158.69.221.121]) by sourceware.org (Postfix) with ESMTPS id 8791C4BAD176 for ; Mon, 4 May 2026 18:11:57 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 8791C4BAD176 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=simark.ca Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=simark.ca ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 8791C4BAD176 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=158.69.221.121 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1777918317; cv=none; b=rCP7tsNkUnWsuC1q3FwTFrjjDkCfbtKbSOlJFgRz/38kdXGNNWuRqweMFbj1+rHvYl+g+8RVDWpUgXynxOKlK77kbd9AmXXAGBh96X2bCmmCp4455EzjVkiXwf2XxKTrOMhw/mBZsFtxAqn1U+8sZPCDfxZin4dsfGWPaYvEzjs= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1777918317; c=relaxed/simple; bh=Zh5RH+rZSPnMsDN1xgqar0dnxQ0haTtoE2ovqHvFhOU=; h=DKIM-Signature:Message-ID:Date:MIME-Version:Subject:To:From; b=N6cxfKPt0fnqj/wlCa+HVTYBwFF26DoeHGauyo05Ml3NmUMjxCrPRxTC5FYnAecDCGityPZxPFTaMj80GTVDXS8uOrIkY40n5cX6//rVEYNgkRQ4T3Qn3zNSr4NyPgWGxudGXp7ONtjPnvrWtZcyQReXRhz0mR2sJYsshFFK6d0= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 8791C4BAD176 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=simark.ca; s=mail; t=1777918316; bh=Zh5RH+rZSPnMsDN1xgqar0dnxQ0haTtoE2ovqHvFhOU=; h=Date:Subject:To:References:From:In-Reply-To:From; b=WYOp7lAI2mHbfLWWp9z8XaQPrF4rfhhIrV/tCrXwIHJx9K/6ewxo+7cCRlCYYw6Eu ogoLvNqfSHO97HLvm8ulFGBocvpZqve+t61QjFxG4zgx4z42cnQSm+0F8WQVQEMqka m7QndhNSF6QlEV++GGcyzfzV66fKZPYIfs3ljodQ= Received: by simark.ca (Postfix) id 623471E067; Mon, 04 May 2026 14:11:56 -0400 (EDT) Message-ID: <5f04d8b8-428b-4885-9def-8be34411b548@simark.ca> Date: Mon, 4 May 2026 14:11:55 -0400 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v2] Fix MI "-break-insert -g i" assertion failure To: Pedro Alves , gdb-patches@sourceware.org References: <20260504145242.1253541-1-pedro@palves.net> <15ed223f-5dcf-4e9d-927b-cdbfcbe57ae3@simark.ca> <94e4c888-cde1-4551-9cd8-170a11fa9074@palves.net> Content-Language: fr From: Simon Marchi In-Reply-To: <94e4c888-cde1-4551-9cd8-170a11fa9074@palves.net> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.30 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: gdb-patches-bounces~public-inbox=simark.ca@sourceware.org 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 , 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 >> >> 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 >> >>> 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