From patchwork Mon Mar 6 20:58:10 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Andrew Burgess X-Patchwork-Id: 66054 Return-Path: X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 54A4D3857838 for ; Mon, 6 Mar 2023 20:58:40 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 54A4D3857838 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1678136320; bh=H7LPMozdGgG3cK6cPTe7zZiE6jwvey+WAEjKj92BdIU=; h=To:Cc:Subject:Date:List-Id:List-Unsubscribe:List-Archive: List-Post:List-Help:List-Subscribe:From:Reply-To:From; b=Yib10A3o8nbnD9vH5DQttWJBeFXLnFF5CCKoBJpUAWp85V3QMlPYA5BmoW0HfForb CiZyT8ULFxKTVJk+/osdqJNjHSYTq0abgzdL20aHfgN/f8btLNcp5/eF8DBc37BCxD YlQeAAS1KjzH0TZb12vme5enLv0QzpJSQaJkuxkk= X-Original-To: gdb-patches@sourceware.org Delivered-To: gdb-patches@sourceware.org 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 EA6DB3858D39 for ; Mon, 6 Mar 2023 20:58:15 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org EA6DB3858D39 Received: from mail-ed1-f71.google.com (mail-ed1-f71.google.com [209.85.208.71]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-413-nOpz7SdNNiylvEtDSZxUWQ-1; Mon, 06 Mar 2023 15:58:14 -0500 X-MC-Unique: nOpz7SdNNiylvEtDSZxUWQ-1 Received: by mail-ed1-f71.google.com with SMTP id z14-20020a05640235ce00b004e07ddbc2f8so6959652edc.7 for ; Mon, 06 Mar 2023 12:58:14 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; t=1678136293; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=H7LPMozdGgG3cK6cPTe7zZiE6jwvey+WAEjKj92BdIU=; b=qmv85Cjeqx0MA85ypShVDZvKJbs2G9iKMdzPfYC0eZ4+wq5AKr9ktQQ1ajw3gPwmmh zHZ1rzDVcH03mQX9om0ODm+0z/Go8NqcvmGaDoHZjqy1/i0211s/ay074wPTR67A6adm gs8vjcu7/sWRm1JLcoz9kKkBFytV/aQaO6sjLUaFN3LyZ+vOaFMCjJDB9Q7aLagl6d/u 9zPHEcrajCWjn287iRdAzIXltVr8v8SfTC65RsSFqf9QhW6DYmmjaPAxFijNPJrbes+k gSYEAkby2X3P0kyt4c3u7A2hetgSzmQsukvkQwaV5RAGXRXGaaySd52Wyuppio/k28vD 7OIw== X-Gm-Message-State: AO0yUKWngd/1dnhGyKmFkczM1XdvOHLo3mtOXP3B9lRRPU1JoGJCR9jB sF/9bf4+yCALwnOCJVVAB8x01wB5fh8bt6PbAas06NzxeoduJ9elFR7vRgVnRObZclCj1jEXKJD oiCKmFzKrUk1jAjg9ZNN0KHOtGqPoUMxyIByCnKXW9aRtO8o/mCWpbiio4yz8NK6LTIBrVfC0Hf BL91ltyw== X-Received: by 2002:a05:6402:699:b0:4ad:66b:84a8 with SMTP id f25-20020a056402069900b004ad066b84a8mr12838417edy.6.1678136293020; Mon, 06 Mar 2023 12:58:13 -0800 (PST) X-Google-Smtp-Source: AK7set+qgU3wsfroMFv5MZxd6eVkoyNR5r5zX99jES3+TOENV4kOkgfvLT6XRn+3rjmIkSyikG5hnw== X-Received: by 2002:a05:6402:699:b0:4ad:66b:84a8 with SMTP id f25-20020a056402069900b004ad066b84a8mr12838396edy.6.1678136292479; Mon, 06 Mar 2023 12:58:12 -0800 (PST) Received: from localhost (95.72.115.87.dyn.plus.net. [87.115.72.95]) by smtp.gmail.com with ESMTPSA id 13-20020a508e0d000000b004af59c07495sm5682167edw.45.2023.03.06.12.58.11 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 06 Mar 2023 12:58:12 -0800 (PST) To: gdb-patches@sourceware.org Cc: Andrew Burgess Subject: [PATCH] gdb/mi: check thread exists when creating thread-specific b/p Date: Mon, 6 Mar 2023 20:58:10 +0000 Message-Id: X-Mailer: git-send-email 2.25.4 MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com 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 X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-Patchwork-Original-From: Andrew Burgess via Gdb-patches From: Andrew Burgess Reply-To: Andrew Burgess Errors-To: gdb-patches-bounces+patchwork=sourceware.org@sourceware.org Sender: "Gdb-patches" 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. Reviewed-By: Eli Zaretskii --- 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(-) base-commit: 6a208145d24c47912c8beb4f1f4b9abeb8d51134 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" + } + } +}