From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by sourceware.org (Postfix) with ESMTPS id 1B0613858D39 for ; Mon, 3 Apr 2023 15:52:31 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 1B0613858D39 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1680537150; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=8RyUeT5U634u/aOLyI5S2PtXf3RGFmLCjEyHnD4u1z8=; b=Cawmq7ND9w+Zd/zaJAzAqWfsOk7GKqNHP8Gg2bsVMsm9GIXgnBsOdSPuVU69PZsd5DS7df fIJ1eUrkC+1HZ+Na+Sn9i6ZQHKy+/PTNJPuvYBogZXPZvxJVGYcl3RNloo3yk8lCAmmU8l bP696u84w1jGt0RC+Bj3bdbsVib5BC4= Received: from mail-qt1-f197.google.com (mail-qt1-f197.google.com [209.85.160.197]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-190-raFuyX7tNEizgVxKObNogA-1; Mon, 03 Apr 2023 11:52:29 -0400 X-MC-Unique: raFuyX7tNEizgVxKObNogA-1 Received: by mail-qt1-f197.google.com with SMTP id r22-20020ac85c96000000b003e638022bc9so8160151qta.5 for ; Mon, 03 Apr 2023 08:52:29 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; t=1680537148; h=mime-version:message-id:date:references:in-reply-to:subject:to:from :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=8RyUeT5U634u/aOLyI5S2PtXf3RGFmLCjEyHnD4u1z8=; b=uk2BgkmQsIcBST0w6rFn9oSdw4lkR5iwfpjrsyZstP7UhvokXdOzmVRS+XQuuA8uVy 7tVtan6SL0f3Ge+muPU57IBMAg47QsdY8telwKNfc73qOldQBFsO6SgJBgGczaDhnrmP m1yAdzuCJqzk/+8JeWcAK8RlJuCKCV569YPefh6zGH43Rz7rLwlM6l82LdWAuO2nKru+ OYj/7s35ULGaIKXptQJVV+jgKb4DSTS7D8YC2wA+CYtsT5D/8JDvrx2KHFMNMN7lbC47 zE81PFTEMNqZJFGNysOPgb/L88dMrPIoLLTuqoajI25AdzHuRqyloQEHODBpEzkjsg5k KQ2w== X-Gm-Message-State: AO0yUKVeuHBFbDXihavhaM8G5w2YMYseEQdkgdEsXeoAoPaKJkBy6r// H6poLVKBca3VGDV2ok2Th+gIWB58mncsvnK8+O2rqsxqZepDCUpXZ9utuPR0mYhvkNjovykY+DI mdjOY+wRe00+JSUTgZJZtpNF+M8CzYmRFawvd0MR7tfQXToEBaG0L/MHT9cVtd6bBeDfS0FSr6N q1BZevBQ== X-Received: by 2002:a05:622a:93:b0:3b6:3a58:912d with SMTP id o19-20020a05622a009300b003b63a58912dmr62217971qtw.0.1680537148471; Mon, 03 Apr 2023 08:52:28 -0700 (PDT) X-Google-Smtp-Source: AK7set9G0C3ZEKI4R/LfoHyjYWIpdy58Likcgggm76AbxyzU1BZ8n1RJD/P1JSkiOOi7RGAP8VUlpA== X-Received: by 2002:a05:622a:93:b0:3b6:3a58:912d with SMTP id o19-20020a05622a009300b003b63a58912dmr62217928qtw.0.1680537148029; Mon, 03 Apr 2023 08:52:28 -0700 (PDT) Received: from localhost (95.72.115.87.dyn.plus.net. [87.115.72.95]) by smtp.gmail.com with ESMTPSA id d69-20020a376848000000b0074688c36facsm2857681qkc.56.2023.04.03.08.52.27 for (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 03 Apr 2023 08:52:27 -0700 (PDT) From: Andrew Burgess To: gdb-patches@sourceware.org Subject: Re: [PATCH] gdb/mi: check thread exists when creating thread-specific b/p In-Reply-To: References: Date: Mon, 03 Apr 2023 16:52:26 +0100 Message-ID: <874jpxc69x.fsf@redhat.com> MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain X-Spam-Status: No, score=-11.7 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,KAM_SHORT,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_NONE,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: Ping! Thanks, Andrew Andrew Burgess writes: > I noticed the following behaviour: > > $ gdb -q -i=mi /tmp/hello.x > =thread-group-added,id="i1" > =cmd-param-changed,param="print pretty",value="on" > ~"Reading symbols from /tmp/hello.x...\n" > (gdb) > -break-insert -p 99 main > ^done,bkpt={number="1",type="breakpoint",disp="keep",enabled="y",addr="0x0000000000401198",func="main",file="/tmp/hello.c",fullname="/tmp/hello.c",line="18",thread-groups=["i1"],thread="99",times="0",original-location="main"} > (gdb) > info breakpoints > &"info breakpoints\n" > ~"Num Type Disp Enb Address What\n" > ~"1 breakpoint keep y 0x0000000000401198 in main at /tmp/hello.c:18\n" > &"../../src/gdb/thread.c:1434: internal-error: print_thread_id: Assertion `thr != nullptr' failed.\nA problem internal to GDB has been detected,\nfurther debugging may prove unreliable." > &"\n" > &"----- Backtrace -----\n" > &"Backtrace unavailable\n" > &"---------------------\n" > &"\nThis is a bug, please report it." > &" For instructions, see:\n" > &".\n\n" > Aborted (core dumped) > > What we see here is that when using the MI a user can create > thread-specific breakpoints for non-existent threads. Then if we try > to use the CLI 'info breakpoints' command GDB throws an assertion. > The assert is a result of the print_thread_id call when trying to > build the 'stop only in thread xx.yy' line; print_thread_id requires a > valid thread_info pointer, which we can't have for a non-existent > thread. > > In contrast, when using the CLI we see this behaviour: > > $ gdb -q /tmp/hello.x > Reading symbols from /tmp/hello.x... > (gdb) break main thread 99 > Unknown thread 99. > (gdb) > > The CLI doesn't allow a breakpoint to be created for a non-existent > thread. So the 'info breakpoints' command is always fine. > > Interestingly, the MI -break-info command doesn't crash, this is > because the MI uses global thread-ids, and so never calls > print_thread_id. However, GDB does support using CLI and MI in > parallel, so we need to solve this problem. > > One option would be to change the CLI behaviour to allow printing > breakpoints for non-existent threads. This would preserve the current > MI behaviour. > > The other option is to pull the MI into line with the CLI and prevent > breakpoints being created for non-existent threads. This is good for > consistency, but is a breaking change for the MI. > > In the end I figured that it was probably better to retain the > consistent CLI behaviour, and just made the MI reject requests to > place a breakpoint on a non-existent thread. The only test we had > that depended on the old behaviour was > gdb.mi/mi-thread-specific-bp.exp, which was added by me in commit: > > commit 2fd9a436c8d24eb0af85ccb3a2fbdf9a9c679a6c > Date: Fri Feb 17 10:48:06 2023 +0000 > > gdb: don't duplicate 'thread' field in MI breakpoint output > > I certainly didn't intend for this test to rely on this feature of the > MI, so I propose to update this test to only create breakpoints for > threads that exist. > > Actually, I've added a new test that checks the MI rejects creating a > breakpoint for a non-existent thread, and I've also extended the test > to run with the separate MI/CLI UIs, and then tested 'info > breakpoints' to ensure this command doesn't crash. > > I've extended the documentation of the `-p` flag to explain the > constraints better. > > I have also added a NEWS entry just in case someone runs into this > issue, at least then they'll know this change in behaviour was > intentional. > > One thing that I did wonder about while writing this patch, is whether > we should treat requests like this, on both the MI and CLI, as another > form of pending breakpoint, something like: > > (gdb) break foo thread 9 > Thread 9 does not exist. > Make breakpoint pending on future thread creation? (y or [n]) y > Breakpoint 1 (foo thread 9) pending. > (gdb) info breakpoints > Num Type Disp Enb Address What > 1 breakpoint keep y foo thread 9 > > Don't know if folk think that would be a useful idea or not? Either > way, I think that would be a separate patch from this one. > --- > gdb/NEWS | 6 ++ > gdb/doc/gdb.texinfo | 5 +- > gdb/mi/mi-cmd-break.c | 2 + > .../gdb.mi/mi-thread-specific-bp.exp | 68 +++++++++++++++---- > 4 files changed, 67 insertions(+), 14 deletions(-) > > diff --git a/gdb/NEWS b/gdb/NEWS > index c32ff92c98a..e4a78dca2df 100644 > --- a/gdb/NEWS > +++ b/gdb/NEWS > @@ -96,6 +96,12 @@ show always-read-ctf > without a thread restriction. The same is also true for the 'task' > field of an Ada task-specific breakpoint. > > +** It is no longer possible to create a thread-specific breakpoint for > + a thread that doesn't exist using '-break-insert -p ID'. Creating > + breakpoints for non-existent threads is not allowed when using the > + CLI, that the MI allowed it was a long standing bug, which has now > + been fixed. > + > *** Changes in GDB 13 > > * MI version 1 is deprecated, and will be removed in GDB 14. > diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo > index bfda7edc4f7..4a81580b061 100644 > --- a/gdb/doc/gdb.texinfo > +++ b/gdb/doc/gdb.texinfo > @@ -32161,7 +32161,10 @@ > Initialize the @var{ignore-count}. > @item -p @var{thread-id} > Restrict the breakpoint to the thread with the specified global > -@var{thread-id}. > +@var{thread-id}. @var{thread-id} must be a valid thread-id at the > +time the breakpoint is requested. Breakpoints created with a > +@var{thread-id} will automatically be deleted when the corresponding > +thread exits. > @item --qualified > This option makes @value{GDBN} interpret a function name specified as > a complete fully-qualified name. > diff --git a/gdb/mi/mi-cmd-break.c b/gdb/mi/mi-cmd-break.c > index 75957b75bad..e5432d58990 100644 > --- a/gdb/mi/mi-cmd-break.c > +++ b/gdb/mi/mi-cmd-break.c > @@ -243,6 +243,8 @@ mi_cmd_break_insert_1 (int dprintf, const char *command, char **argv, int argc) > break; > case THREAD_OPT: > thread = atol (oarg); > + if (!valid_global_thread_id (thread)) > + error (_("Unknown thread %d."), thread); > 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 4586fa44bbc..9c04d6f8c2a 100644 > --- a/gdb/testsuite/gdb.mi/mi-thread-specific-bp.exp > +++ b/gdb/testsuite/gdb.mi/mi-thread-specific-bp.exp > @@ -29,21 +29,63 @@ if [build_executable ${testfile}.exp ${binfile} ${srcfile}] { > return -1 > } > > -if {[mi_clean_restart $binfile]} { > - return -1 > -} > - > -mi_create_breakpoint "-p 1 bar" "thread-specific b/p on bar" \ > - -thread "1" > - > proc make_loc {num} { > return [mi_make_breakpoint_loc -thread "1" -number "$::decimal\\.$num"] > } > > -set loc1 [make_loc 1] > -set loc2 [make_loc 2] > -set loc3 [make_loc 3] > +foreach_mi_ui_mode mode { > + > + if {$mode == "separate"} { > + set start_ops "separate-mi-tty" > + } else { > + set start_ops "" > + } > + > + if {[mi_clean_restart $binfile $start_ops]} { > + return -1 > + } > + > + # 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\\.\"" > > -mi_create_breakpoint_multi "-p 1 foo" "thread-specific b/p on foo" \ > - -thread "1" \ > - -locations "\\\[$loc1,$loc2,$loc3\\\]" > + # If we have a separate CLI UI then run the 'info breakpoints' > + # command. There was a time when the previous breakpoint request > + # would succeed, and then 'info breakpoint' on the CLI would > + # trigger an assertion. > + if {$mode eq "separate"} { > + with_spawn_id $gdb_main_spawn_id { > + gdb_test "info breakpoints" "No breakpoints or watchpoints\\." \ > + "check CLI 'info breakpoints' when there are no breakpoints" > + } > + } > + > + if {[mi_runto_main] == -1} { > + 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\\.\"" > + > + mi_create_breakpoint "-p 1 bar" "thread-specific b/p on bar" \ > + -thread "1" > + > + set loc1 [make_loc 1] > + set loc2 [make_loc 2] > + set loc3 [make_loc 3] > + > + mi_create_breakpoint_multi "-p 1 foo" "thread-specific b/p on foo" \ > + -thread "1" \ > + -locations "\\\[$loc1,$loc2,$loc3\\\]" > + > + # Check that 'info breakpoints' on the CLI succeeds. > + if {$mode eq "separate"} { > + with_spawn_id $gdb_main_spawn_id { > + gdb_test "info breakpoints" ".*" \ > + "check CLI 'info breakpoints' when there are some breakpoints" > + } > + } > +} > > base-commit: 6a208145d24c47912c8beb4f1f4b9abeb8d51134 > -- > 2.25.4