From patchwork Tue Mar 5 15:21:44 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Andrew Burgess X-Patchwork-Id: 86809 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 373433858291 for ; Tue, 5 Mar 2024 15:24:18 +0000 (GMT) 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 029A5385840E for ; Tue, 5 Mar 2024 15:22:16 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 029A5385840E Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 029A5385840E Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=170.10.129.124 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1709652142; cv=none; b=liSD81J+ia7/ou2ZeJroETOkrc2x3mF7iCwxtRA/xDHqzg71e+oeIuSna1me8HuCKSjiAE20ffcVSCdEhsIvorgZczESghvLqtwU9eSdOiqer2Rd3c7GckBoeGfoSpN5IdcFY5Pz5yPUlToUNK/qefF7uGP83GsyIXqVgH1t0N4= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1709652142; c=relaxed/simple; bh=L254M+mW0usMzWarNV3mFQf07xABTsOx28zhJd/OphI=; h=DKIM-Signature:From:To:Subject:Date:Message-Id:MIME-Version; b=Rsj4NILbUTUGD644kzyPG2YExJG09oseuRPJ941NLmglQmyz8opOU8sig1R35LmocFafchGDqIG4HByncUtruDmZi0WWstsBtmyLBVLeVpnglLracLhhbTsTSzPEJ9FX/s0lXuF1mvH9Ld0lCOKbK7zN0HZ4aB+bgqKIW622SDM= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1709652136; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=8JGlAon5zxdcHBfmXZ3kl/Hfc4f3EkrzGyCR8wAp20Y=; b=ayguMrxE4L0QN7+kP6KTqlnH4yDYvI1M/K//w2p/ElaJSMe348YbCXUUrIukSmifMvLQAX oxN5oBUsr147srRxjsyZxE3qAibAltXN7wvILiaL1rmED+L4Lpll07nJWrOtv0/NZwWnJo m2Va9vD244I9nJgBei6bz7V3XcNXDG0= Received: from mail-ej1-f71.google.com (mail-ej1-f71.google.com [209.85.218.71]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-279-PCZr7WCBMEmiE7ZVCuXMdw-1; Tue, 05 Mar 2024 10:22:13 -0500 X-MC-Unique: PCZr7WCBMEmiE7ZVCuXMdw-1 Received: by mail-ej1-f71.google.com with SMTP id a640c23a62f3a-a450c660cdeso54738166b.3 for ; Tue, 05 Mar 2024 07:22:13 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1709652131; x=1710256931; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=8JGlAon5zxdcHBfmXZ3kl/Hfc4f3EkrzGyCR8wAp20Y=; b=XUWFhE+nG+X0vbiBRYJ7HHSdl82nZChZ9UzW1mvnjvyv9PfnlrtKuNILuFsz643b4G mstCUYIV65LZX2jNuQrWQ8pWDqXr8PA0le+TdglOk3U6ddRnQDu5FiuNOnDAWwDwW4tg jzM0SaWYVc5Oy5Do+MtoANzsx7q+FAmzd7cEWX58ZcibUPT/y41gGM9BDeFovuN/MIxv c6w5iKWA5NtG72INH6+86+HPXWboS/4m6dQOVwBTX9Zjd69R73zujaGVZ0yf9dkx6Pcz +sJzmpqdP/eckp/z9ny4tPocYmUu7n0UK1qkrwghprVk70Z82h3yPfbvV89F9+pD6IhX paFw== X-Gm-Message-State: AOJu0YxPxGllbcbgRKqyZV7KHWBzuha8nNL+UZvSG1hMaMKM1NyQtAqP itSSbxBter1+zRNElaO+xo6oXeHAR0cQURY3IPSuheb+JUHpB+4E68ZbhZkR6dvelaphpNyysF1 DMHn7nZe5I+yOXkdXMbxJywhrpuFU7TtRRIWAsHtzImvIs1vLE0sNDmLl53AYDLmqyavrWeYt91 vScGesTS/J7Mcbo2js3HaQcmgr5VEU27szZIhqQG4G1qg= X-Received: by 2002:a17:906:fa87:b0:a44:8ea2:23fa with SMTP id lt7-20020a170906fa8700b00a448ea223famr8914024ejb.46.1709652131037; Tue, 05 Mar 2024 07:22:11 -0800 (PST) X-Google-Smtp-Source: AGHT+IEI+hQBw2p7+W4PgolQ3joJcedwlRpY6YUB0o3FELYAyw/qpcAHe+k42XenIR+j2RAo3PsbJg== X-Received: by 2002:a17:906:fa87:b0:a44:8ea2:23fa with SMTP id lt7-20020a170906fa8700b00a448ea223famr8913991ejb.46.1709652130464; Tue, 05 Mar 2024 07:22:10 -0800 (PST) Received: from localhost (185.223.159.143.dyn.plus.net. [143.159.223.185]) by smtp.gmail.com with ESMTPSA id a15-20020a17090640cf00b00a4354b9893csm6139173ejk.74.2024.03.05.07.22.10 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 05 Mar 2024 07:22:10 -0800 (PST) From: Andrew Burgess To: gdb-patches@sourceware.org Cc: Andrew Burgess Subject: [PATCHv9 13/14] gdb: don't set breakpoint::pspace in create_breakpoint Date: Tue, 5 Mar 2024 15:21:44 +0000 Message-Id: X-Mailer: git-send-email 2.25.4 In-Reply-To: References: MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com X-Spam-Status: No, score=-12.3 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, 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 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+patchwork=sourceware.org@sourceware.org I spotted this code within create_breakpoint: if ((type_wanted != bp_breakpoint && type_wanted != bp_hardware_breakpoint) || thread != -1) b->pspace = current_program_space; this code is only executed when creating a pending breakpoint, and sets the breakpoint::pspace member variable. The above code gained the 'thread != -1' clause with this commit: commit cc72b2a2da6d6372cbdb1d14639a5fce84e1a325 Date: Fri Dec 23 17:06:16 2011 +0000 Introduce gdb.FinishBreakpoint in Python While the type_wanted checks were added with this commit: commit f8eba3c61629b3c03ac1f33853eab4d8507adb9c Date: Tue Dec 6 18:54:43 2011 +0000 the "ambiguous linespec" series Before this breakpoint::pspace was set unconditionally. If we look at how breakpoint::pspace is used today, some breakpoint types specifically set this field, either in their constructors, or in a wrapper function that calls the constructor. So, the watchpoint type and its sub-class set this variable, as does the catchpoint type, and all it's sub-classes. However, code_breakpoint doesn't specifically set this field within its constructor, though some sub-classes of code_breakpoint (ada_catchpoint, exception_catchpoint, internal_breakpoint, and momentary_breakpoint) do set this field. When I examine all the places that breakpoint::pspace is used, I believe that in every place where it is expected that this field is set, the breakpoint type will be one that specifically sets this field. Next, I observe two problems with the existing code. First, the above code is only hit for pending breakpoints, there's no equivalent code for non-pending breakpoints. This opens up the possibility of GDB entering non-consistent states; if a breakpoint is first created pending and then later gets a location, the pspace field will be set, while if the breakpoint is immediately non-pending, then the pspace field will never be set. Second, if we look at how breakpoint::pspace is used in the function breakpoint_program_space_exit, we see that when a program space is removed, any breakpoint with breakpoint::pspace set to the removed program space, will be deleted. This makes sense, but does mean we need to ensure breakpoint::pspace is only set for breakpoints that apply to a single program space. So, if I create a pending dprintf breakpoint (type bp_dprintf) then the breakpoint::pspace variable will be set even though the dprintf is not really tied to that one program space. As a result, when the matching program space is removed the dprintf is incorrectly removed. Also, if I create a thread specific breakpoint, then, thanks to the 'thread != -1' clause the wrong program space will be stored in breakpoint::pspace (the current program space is always used, which might not be the program space that corresponds to the selected thread), as a result, the thread specific breakpoint will be deleted when the matching program space is removed. If we look at commit cc72b2a2da6d which added the 'thread != -1' clause, we can see this change was entirely redundant, the breakpoint::pspace is also set in bpfinishpy_init after create_breakpoint has been called. As such, I think we can safely drop the 'thread != -1' clause. For the other problems, I'm proposing to be pretty aggressive - I'd like to drop the breakpoint::pspace assignment completely from create_breakpoint. Having looked at how this variable is used, I believe that it is already set elsewhere in all the cases that it is needed. Maybe this code was needed at one time, but I can't see how it's needed any more. There's tests to expose the issues I've spotted with this code, and there's no regressions in testing. --- gdb/breakpoint.c | 3 - gdb/breakpoint.h | 18 +- .../gdb.multi/pending-bp-del-inferior.c | 28 +++ .../gdb.multi/pending-bp-del-inferior.exp | 216 ++++++++++++++++++ 4 files changed, 259 insertions(+), 6 deletions(-) create mode 100644 gdb/testsuite/gdb.multi/pending-bp-del-inferior.c create mode 100644 gdb/testsuite/gdb.multi/pending-bp-del-inferior.exp diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c index cc853c3251e..e1efde66feb 100644 --- a/gdb/breakpoint.c +++ b/gdb/breakpoint.c @@ -9190,9 +9190,6 @@ create_breakpoint (struct gdbarch *gdbarch, b->disposition = tempflag ? disp_del : disp_donttouch; b->condition_not_parsed = 1; b->enable_state = enabled ? bp_enabled : bp_disabled; - if ((type_wanted != bp_breakpoint - && type_wanted != bp_hardware_breakpoint) || thread != -1) - b->pspace = current_program_space; if (b->type == bp_dprintf) update_dprintf_command_list (b.get ()); diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h index 24aeaf926ec..42b49144e79 100644 --- a/gdb/breakpoint.h +++ b/gdb/breakpoint.h @@ -831,9 +831,21 @@ struct breakpoint : public intrusive_list_node equals this. */ struct frame_id frame_id = null_frame_id; - /* The program space used to set the breakpoint. This is only set - for breakpoints which are specific to a program space; for - non-thread-specific ordinary breakpoints this is NULL. */ + /* The program space used to set the breakpoint. This is only set for + breakpoints that are not type bp_breakpoint or bp_hardware_breakpoint. + For thread or inferior specific breakpoints, the breakpoints are + managed via the thread and inferior member variables. */ + + /* If not nullptr then this is the program space for which this + breakpoint was created. All watchpoint and catchpoint sub-types set + this field, but not all of the code_breakpoint sub-types do; + generally, user created breakpoint types don't set this field, though + things might be more consistent if they did. + + When this variable is nullptr then a breakpoint might be associated + with multiple program spaces, though you need to check the thread, + inferior and task variables to see if a breakpoint was created for a + specific thread, inferior, or Ada task respectively. */ program_space *pspace = NULL; /* The location specification we used to set the breakpoint. */ diff --git a/gdb/testsuite/gdb.multi/pending-bp-del-inferior.c b/gdb/testsuite/gdb.multi/pending-bp-del-inferior.c new file mode 100644 index 00000000000..1d030007819 --- /dev/null +++ b/gdb/testsuite/gdb.multi/pending-bp-del-inferior.c @@ -0,0 +1,28 @@ +/* This testcase is part of GDB, the GNU debugger. + + Copyright 2023 Free Software Foundation, Inc. + + This program is free software; you can redistribute it and/or modify + it under the terms of the GNU General Public License as published by + the Free Software Foundation; either version 3 of the License, or + (at your option) any later version. + + This program is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU General Public License for more details. + + You should have received a copy of the GNU General Public License + along with this program. If not, see . */ + +int +foo (void) +{ + return 0; +} + +int +main (void) +{ + return foo (); +} diff --git a/gdb/testsuite/gdb.multi/pending-bp-del-inferior.exp b/gdb/testsuite/gdb.multi/pending-bp-del-inferior.exp new file mode 100644 index 00000000000..5fcd1ef2e39 --- /dev/null +++ b/gdb/testsuite/gdb.multi/pending-bp-del-inferior.exp @@ -0,0 +1,216 @@ +# Copyright 2023 Free Software Foundation, Inc. + +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 3 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see . + +# Setup two inferiors. Select one inferior and create a pending +# thread specific breakpoint in the other inferior. +# +# Delete the selected inferior (the one for which the thread specific +# breakpoint doesn't apply), and check that the breakpoint still exists. +# +# Repeat this process, but this time, create an inferior specific +# breakpoint. + +# The plain remote target can't do multiple inferiors. +require !use_gdb_stub + +standard_testfile + +if {[prepare_for_testing "failed to prepare" $testfile $srcfile]} { + return -1 +} + +# Setup for the tests. Create two inferiors, both running the global +# BINFILE, and proceed to main in both inferiors. Delete all +# breakpoints, and check that we do have two threads. +# +# Return true after a successful setup, otherwise, return false. +proc test_setup {} { + clean_restart $::binfile + + if {![runto_main]} { + return 0 + } + + gdb_test "add-inferior -exec ${::binfile}" "Added inferior 2.*" \ + "add inferior 2" + gdb_test "inferior 2" "Switching to inferior 2 .*" \ + "select inferior 2" + + if {![runto_main]} { + return 0 + } + + delete_breakpoints + + gdb_test "info threads" \ + [multi_line \ + " Id\\s+Target Id\\s+Frame\\s*" \ + " 1\\.1\\s+\[^\r\n\]+" \ + "\\* 2\\.1\\s+\[^\r\n\]+"] \ + "check we have the expected threads" + + return 1 +} + +# Assuming inferior 2 is already selected, kill the current inferior +# (inferior 2), select inferior 1, and then remove inferior 2. +proc kill_and_remove_inferior_2 {} { + gdb_test "kill" "" "kill inferior 2" \ + "Kill the program being debugged.*y or n. $" "y" + + gdb_test "inferior 1" "Switching to inferior 1 .*" \ + "select inferior 1" + + gdb_test_no_output "remove-inferiors 2" +} + +# Setup two inferiors, then create a breakpoint. If BP_PENDING is +# true then the breakpoint will be pending, otherwise, the breakpoint +# will be non-pending. +# +# BP_TYPE is either 'thread' or 'inferior', and indicates if the +# created breakpoint should be thread or inferior specific. +# +# The breakpoint is created while inferior 2 is selected, and the +# thread/inferior restriction always identifies inferior 1. +# +# Then inferior 2 is killed and removed. +# +# Finally, check that the breakpoint still exists and correctly refers +# to inferior 1. +proc do_bp_test { bp_type bp_pending } { + if {![test_setup]} { + return + } + + if { $bp_pending } { + set bp_func "bar" + } else { + set bp_func "foo" + } + + if { $bp_type eq "thread" } { + set bp_restriction "thread 1.1" + } else { + set bp_restriction "inferior 1" + } + + gdb_breakpoint "$bp_func $bp_restriction" allow-pending + set bp_number [get_integer_valueof "\$bpnum" "INVALID" \ + "get b/p number for previous breakpoint"] + + if { $bp_restriction eq "thread 1.1" } { + set bp_after_restriction "thread 1" + } else { + set bp_after_restriction $bp_restriction + } + + if { $bp_pending } { + set bp_pattern_before \ + [multi_line \ + "$bp_number\\s+breakpoint\\s+keep\\s+y\\s+\\s+${bp_func}" \ + "\\s+stop only in [string_to_regexp $bp_restriction]"] + set bp_pattern_after \ + [multi_line \ + "$bp_number\\s+breakpoint\\s+keep\\s+y\\s+\\s+${bp_func}" \ + "\\s+stop only in [string_to_regexp $bp_after_restriction]"] + } else { + set bp_pattern_before \ + [multi_line \ + "$bp_number\\s+breakpoint\\s+keep\\s+y\\s+\\s*" \ + "\\s+stop only in [string_to_regexp $bp_restriction]" \ + "$bp_number\\.1\\s+y\\s+$::hex in $bp_func at \[^\r\n\]+ inf 1" \ + "$bp_number\\.2\\s+y\\s+$::hex in $bp_func at \[^\r\n\]+ inf 2"] + + set bp_pattern_after \ + [multi_line \ + "$bp_number\\s+breakpoint\\s+keep\\s+y\\s+$::hex in $bp_func at \[^\r\n\]+" \ + "\\s+stop only in [string_to_regexp $bp_after_restriction]"] + } + + gdb_test "info breakpoints" $bp_pattern_before \ + "info breakpoints before inferior removal" + + kill_and_remove_inferior_2 + + gdb_test "info breakpoints" $bp_pattern_after \ + "info breakpoints after inferior removal" +} + +# Setup two inferiors, then create a dprintf. If BP_PENDING is +# true then the dprintf will be pending, otherwise, the dprintf +# will be non-pending. +# +# The dprintf is created while inferior 2 is selected. Then inferior +# 2 is killed and removed. +# +# Finally, check that the dprintf still exists. +proc do_dprintf_test { bp_pending } { + if {![test_setup]} { + return + } + + if { $bp_pending } { + set bp_func "bar" + + gdb_test "dprintf $bp_func,\"in $bp_func\"" ".*" \ + "create dprintf breakpoint" \ + "Make dprintf pending on future shared library load\\? \\(y or .n.\\) $" "y" + } else { + set bp_func "foo" + + gdb_test "dprintf $bp_func,\"in $bp_func\"" ".*" \ + "create dprintf breakpoint" + } + + set bp_number [get_integer_valueof "\$bpnum" "INVALID" \ + "get b/p number for previous breakpoint"] + + if { $bp_pending } { + set bp_pattern_before \ + [multi_line \ + "$bp_number\\s+dprintf\\s+keep\\s+y\\s+\\s+${bp_func}" \ + "\\s+printf \"in $bp_func\""] + set bp_pattern_after $bp_pattern_before + } else { + set bp_pattern_before \ + [multi_line \ + "$bp_number\\s+dprintf\\s+keep\\s+y\\s+\\s*" \ + "\\s+printf \"in $bp_func\"" \ + "$bp_number\\.1\\s+y\\s+$::hex in $bp_func at \[^\r\n\]+ inf 1" \ + "$bp_number\\.2\\s+y\\s+$::hex in $bp_func at \[^\r\n\]+ inf 2"] + + set bp_pattern_after \ + [multi_line \ + "$bp_number\\s+dprintf\\s+keep\\s+y\\s+$::hex in $bp_func at \[^\r\n\]+" \ + "\\s+printf \"in $bp_func\""] + } + + gdb_test "info breakpoints" $bp_pattern_before \ + "info breakpoints before inferior removal" + + kill_and_remove_inferior_2 + + gdb_test "info breakpoints" $bp_pattern_after \ + "info breakpoints after inferior removal" +} + +foreach_with_prefix bp_pending { true false } { + foreach_with_prefix bp_type { thread inferior } { + do_bp_test $bp_type $bp_pending + } + + do_dprintf_test $bp_pending +}