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.129.124]) by sourceware.org (Postfix) with ESMTPS id 1EB3A3858D37 for ; Fri, 28 Apr 2023 23:18:14 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 1EB3A3858D37 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=1682723893; 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=rw4zkIA7knEogR7hrF8aViQ6Ma7zaHg7qZQdNHsdsgM=; b=gysEfh9i6aS14xD7i85vm3osHMWagw9BrrMGOi91MskR0lxMTj7cF/LEUJfD10nKEJe/Eu /lvb/WGQ3QPVSiQp4oBcl1ltLDtF1VlVeY7XOY05B+k9mURc9Obh0jaT+RXLR+E1noZY92 gJjeFxqJPblF7BfvyuoLWmpcTl/I1wo= Received: from mail-wm1-f70.google.com (mail-wm1-f70.google.com [209.85.128.70]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-195-225BFOnfOfStBcGlAQ5Zgg-1; Fri, 28 Apr 2023 19:18:12 -0400 X-MC-Unique: 225BFOnfOfStBcGlAQ5Zgg-1 Received: by mail-wm1-f70.google.com with SMTP id 5b1f17b1804b1-3f175a24fd1so1482555e9.1 for ; Fri, 28 Apr 2023 16:18:12 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1682723891; x=1685315891; 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=rw4zkIA7knEogR7hrF8aViQ6Ma7zaHg7qZQdNHsdsgM=; b=kwqNlQPeIB78z6gllm6ZLZdnmfY4xxUFPfQ0OOIajJHFofMIjpqSCGVAwWyikascda jmzDxOtX/aonvWV1DpfXNHlWl+VsMJOQb8S9H/4ScibTfiSHnJ5f+2VFxQ/qXNgcapWB GFJ3JnrNEJ84JetUGKP8shP8zwMDjt2xqSFQRRkTNJxiz6PodcSwsIhl8lbj9yfP8/cD /QSOr7/mcI6BB+b1GgT0/L2yl+jSCj0NNQHllcXjnteNloGTfDO/r8JPJT0qAUSz8QAH BA3AIqAlCeSO90R/lPHjcWi6WIjiWDG+odC/aO8WB5K2UO0IsawNMnXxOyr9l8AbtILA lAww== X-Gm-Message-State: AC+VfDxPuiOChVPI381TYEtq/1tMlo61wRj65osd6n1rHxA54XZNJGPK NJrl0J1bVd71k2mlVATZASpFccrr9ibqF+Bmd2njhU1/xM7CSFg4tNhQq+F8ek21nBkOHbpZNeO YpSmWv288d2v9GfOGd4iEoidYyKXCTVhfmqq29jZFQTgQtk3gfm/IDy7Q8JoNl2E4c5vuEfWd81 DoHtdy3Q== X-Received: by 2002:a05:600c:b49:b0:3f1:8c59:4713 with SMTP id k9-20020a05600c0b4900b003f18c594713mr5108900wmr.12.1682723891077; Fri, 28 Apr 2023 16:18:11 -0700 (PDT) X-Google-Smtp-Source: ACHHUZ6jPr2nZ0rdqxpG6Deree4K3cQmjKSUCxXhEhQbbbf8c3TY2+e+pLh4vaYQiONf6VkbYy8fcg== X-Received: by 2002:a05:600c:b49:b0:3f1:8c59:4713 with SMTP id k9-20020a05600c0b4900b003f18c594713mr5108892wmr.12.1682723890655; Fri, 28 Apr 2023 16:18:10 -0700 (PDT) Received: from localhost (11.72.115.87.dyn.plus.net. [87.115.72.11]) by smtp.gmail.com with ESMTPSA id f22-20020a1c6a16000000b003f17e79d74asm25963388wmc.7.2023.04.28.16.18.09 for (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 28 Apr 2023 16:18:10 -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: Sat, 29 Apr 2023 00:18:08 +0100 Message-ID: <875y9fa95b.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.5 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,T_SCC_BODY_TEXT_LINE 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: 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. > --- I've gone ahead and pushed this change. This was blocking another series I'd like to post to the list. Let me know if there are any problems. Thanks, Andrew > 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