From patchwork Sat Dec 2 10:42:15 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Andrew Burgess X-Patchwork-Id: 81186 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 4C183386187A for ; Sat, 2 Dec 2023 10:43:22 +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 D69163861874 for ; Sat, 2 Dec 2023 10:42:43 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org D69163861874 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 D69163861874 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=1701513766; cv=none; b=VTifMQmNTCwzIA0ELlkrkjyAmKaZK/6MXOtWpm5H2hO45kKg0GieyKNl12z/pv+drKeJYi06Vc2VGlPzPysPTHQbwJExh6rhRc2Rp0wPBUy5LTF+JXQdNYBBcAmnCX0rT+o0bLZpuXSqJH7XfYBSRmndFm0HcFpp9zW6x104FG8= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1701513766; c=relaxed/simple; bh=cL9eGgZamOITv8xCD7d/gJ4IzNFubVT9EIsvnNz8sOo=; h=DKIM-Signature:From:To:Subject:Date:Message-Id:MIME-Version; b=TtiPBfQqaNUoYKN27CAk+DRBS0NBHc6IFKUHCFMVdE4n9xXlqAHWC/LwRQedtsLyqGzpMqTtoHqo8DTcsjnTyMnrlgh/TrsVReiGvoWwVtiCAvJ1QWMrPaR9yNWoQUvbtgwLM4MlX4VBuqMofxW7kUN2HH1hEYjHWVWk288vh3M= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1701513763; 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=FrMTJ+OPjccl4ai0NOwOgYmTNddTZsJmJHOfLfztMP4=; b=B2Mx1Vpmg1h1+/tt4lVdNRR1Sshlz+/57Vi95pmcY9NmqITd9qEu1mvrB1pwO+QGIagfbe PoP3//E/Mzi/LaAQrW6ETmbm5XcHU+QpZ8X67798CsJG/jf9f8ILvUkItzUvme7+Vd1klY Ihv0aFOrfFIfV26QxVx3SzkZ1PDaS/4= Received: from mail-lj1-f199.google.com (mail-lj1-f199.google.com [209.85.208.199]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-1-WnocY90lOM241tsETubEFw-1; Sat, 02 Dec 2023 05:42:42 -0500 X-MC-Unique: WnocY90lOM241tsETubEFw-1 Received: by mail-lj1-f199.google.com with SMTP id 38308e7fff4ca-2c9b18161b6so33455861fa.3 for ; Sat, 02 Dec 2023 02:42:42 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1701513760; x=1702118560; 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=FrMTJ+OPjccl4ai0NOwOgYmTNddTZsJmJHOfLfztMP4=; b=NCvY1Mhq6YdI0yfHUp7Jo4I9kTUhyMCfHF6qULT/kU242zijJY7V70jlmxSWVK0v58 hw/ZPoce4cRBL6n7X48lNVsRXIClMyU+pW6JWdkfUKk/RDYhbSxR7CBEhcNgk/MrDBbh 24e2tx53DrKq7W9PJcEe2gzwgoqeJkH0189JBNyOwqrGYGy5DO8Bs14AUpEtIK4Fdro5 uVKJnFN2W28DMVkkqVVr1FDqjP7pq03WsHx5bKnt9HZt/6VuyOvgd0zs+PTjqq8dprer TCUJi4sUpoosgR3m5MkjRn25rhecJUYQg1EnS4KueeTUChYCPil8EPwPHd17KeWnY1qY xr0w== X-Gm-Message-State: AOJu0YwfMNyZ8sbQSimfQFhDlMtA8EaH1JFv9Gb55bwNGHbp2mjIKfqH 2Jj6mmDBUjjbkFRVXPLbwR8vx3fVTm4q38jtCsYD6Vl10fy5REUMFnzWRd4cREgQOjRaJMEmi5D fO0Nb7FYse1ZsvcC+YPJtwayr1fdqWoDM++wUTcVYt8CgKHuaT2y58xAuJUC5cIAfRNTSwTGF3y b1PFkmtw== X-Received: by 2002:a2e:3511:0:b0:2c9:b69d:987d with SMTP id z17-20020a2e3511000000b002c9b69d987dmr1559126ljz.3.1701513760438; Sat, 02 Dec 2023 02:42:40 -0800 (PST) X-Google-Smtp-Source: AGHT+IFcNIqXoBp91z3v/FEPrlVLC/UpVkJcG2rQjEgHh5fKB+cXaZVRAku1NyW4ib7qiWDUMGVBAQ== X-Received: by 2002:a2e:3511:0:b0:2c9:b69d:987d with SMTP id z17-20020a2e3511000000b002c9b69d987dmr1559120ljz.3.1701513759945; Sat, 02 Dec 2023 02:42:39 -0800 (PST) Received: from localhost (92.40.184.5.threembb.co.uk. [92.40.184.5]) by smtp.gmail.com with ESMTPSA id e20-20020a1709067e1400b00a17ca6996a2sm2952895ejr.178.2023.12.02.02.42.39 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sat, 02 Dec 2023 02:42:39 -0800 (PST) From: Andrew Burgess To: gdb-patches@sourceware.org Cc: Andrew Burgess Subject: [PATCHv6 07/10] gdb: don't set breakpoint::pspace for in create_breakpoint Date: Sat, 2 Dec 2023 10:42:15 +0000 Message-Id: <864815443474b68aedb8dbff07442be8d688a209.1701513410.git.aburgess@redhat.com> 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=-8.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_ABUSEAT, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H4, RCVD_IN_MSPIKE_WL, RCVD_IN_SBL_CSS, 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 While working on the previous commit, I ran into 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 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 e2cd4d360f2..bbf28d82558 100644 --- a/gdb/breakpoint.c +++ b/gdb/breakpoint.c @@ -9207,9 +9207,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 95f98b59e41..2bf63b90072 100644 --- a/gdb/breakpoint.h +++ b/gdb/breakpoint.h @@ -823,9 +823,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..51b6492085f --- /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 () +{ + return 0; +} + +int +main () +{ + 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..cb1f119ff94 --- /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), selected 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 created 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 created 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. 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 +}