From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm1-f54.google.com (mail-wm1-f54.google.com [209.85.128.54]) by sourceware.org (Postfix) with ESMTPS id D4CCA3857C5F for ; Mon, 7 Mar 2022 16:56:04 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org D4CCA3857C5F Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=palves.net Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=gmail.com Received: by mail-wm1-f54.google.com with SMTP id i66so9572684wma.5 for ; Mon, 07 Mar 2022 08:56:04 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:from:to:subject:date:message-id:mime-version :content-transfer-encoding; bh=DmYwTJRfM55uiZGg+Bvnbq9hMgib21pA8eMBPrE54mE=; b=QRzN927ji1WTrhsMUJ1qkKxTZHcfvumGq8bYaZiAyWzvfTSTUqxNUbDKvWokBea/wv OliYVTnYaTRZigAm1Dl8hqz9xYVefn4t3RBNy2f3p7EeQNhbB+NPf/Us/nrAaQZIC+t+ tfIHNadDH25XcFCM8hgpmii96CRpEk5Kuvu64ybHoR4uZwZVeLnySvjPkooG3QFdnUvf 3kozYbJC0FEuaKmKFtaRos6rUeVvPbDkuY3SAjjyV5wWnFbs+OSSg7DKR25+1OjTMVLX hHgr0/bKB44lvvlVq01mcsvzgW/jOPb1fEqqIdgjIscEqqhXn2NTK+GMUbIBUaLoPRKa 1w+Q== X-Gm-Message-State: AOAM530raX8cEB8Aq8750XS6Fw/1IByY9pJqx3RTuIWiAch8eeV4sRXG lwQP/+fic14i70yPtuX6VrZJjoLgnL8= X-Google-Smtp-Source: ABdhPJyaaH19ktmRK2jAlAs7T14DHRhH9aAvTrbhdFS9v2TBkHz+H+fOMToscJxpm+pkmsaZKa57wg== X-Received: by 2002:a1c:c911:0:b0:389:8f96:28f3 with SMTP id f17-20020a1cc911000000b003898f9628f3mr10320366wmb.49.1646672162943; Mon, 07 Mar 2022 08:56:02 -0800 (PST) Received: from localhost ([2001:8a0:f924:2600:209d:85e2:409e:8726]) by smtp.gmail.com with ESMTPSA id l16-20020a05600c1d1000b003816edb5711sm20907790wms.26.2022.03.07.08.56.01 for (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 07 Mar 2022 08:56:02 -0800 (PST) From: Pedro Alves To: gdb-patches@sourceware.org Subject: [PATCH] Watchpoint followed by catchpoint misreports watchpoint (PR gdb/28621) Date: Mon, 7 Mar 2022 16:56:00 +0000 Message-Id: <20220307165600.906162-1-pedro@palves.net> X-Mailer: git-send-email 2.26.2 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-9.8 required=5.0 tests=BAYES_00, FREEMAIL_FORGED_FROMDOMAIN, FREEMAIL_FROM, GIT_PATCH_0, HEADER_FROM_DIFFERENT_DOMAINS, KAM_DMARC_STATUS, KAM_SHORT, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H2, SPF_HELO_NONE, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) 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-List-Received-Date: Mon, 07 Mar 2022 16:56:08 -0000 If GDB reports a watchpoint hit, and then the next event is not TARGET_WAITKIND_STOPPED, but instead some event for which there's a catchpoint, such that GDB calls bpstat_stop_status, GDB mistakenly thinks the watchpoint triggered. Vis, using foll-fork.c: (gdb) awatch v Hardware access (read/write) watchpoint 2: v (gdb) catch fork Catchpoint 3 (fork) (gdb) c Continuing. Hardware access (read/write) watchpoint 2: v Old value = 0 New value = 5 main () at gdb.base/foll-fork.c:16 16 pid = fork (); (gdb) Continuing. Hardware access (read/write) watchpoint 2: v <<<< <<<< these lines are spurious Value = 5 <<<< Catchpoint 3 (forked process 1712369), arch_fork (ctid=0x7ffff7fa4810) at arch-fork.h:49 49 arch-fork.h: No such file or directory. (gdb) The problem is that when we handle the fork event, nothing called watchpoints_triggered before calling bpstat_stop_status. Thus, each watchpoint's watchpoint_triggered field was still set to watch_triggered_yes from the previous (real) watchpoint stop. watchpoint_triggered is only current called in the handle_signal_stop path, when handling TARGET_WAITKIND_STOPPED. This fixes it by adding watchpoint_triggered calls in the other events paths that call bpstat_stop_status. But instead of adding them explicitly, it adds a new function bpstat_stop_status_nowatch that wraps bpstat_stop_status and calls watchpoint_triggered, and then replaces most calls to bpstat_stop_status with calls to bpstat_stop_status_nowatch. This required constifying watchpoints_triggered. New test included, which fails without the fix. Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=28621 Change-Id: I282b38c2eee428d25319af3bc842f9feafed461c --- gdb/breakpoint.c | 15 +++ gdb/breakpoint.h | 25 ++++- gdb/infrun.c | 24 ++--- gdb/testsuite/gdb.base/watch-before-fork.c | 30 ++++++ gdb/testsuite/gdb.base/watch-before-fork.exp | 99 ++++++++++++++++++++ 5 files changed, 178 insertions(+), 15 deletions(-) create mode 100644 gdb/testsuite/gdb.base/watch-before-fork.c create mode 100644 gdb/testsuite/gdb.base/watch-before-fork.exp diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c index a3cfeea6989..ebcefdee54d 100644 --- a/gdb/breakpoint.c +++ b/gdb/breakpoint.c @@ -5530,6 +5530,21 @@ bpstat_stop_status (const address_space *aspace, return bs_head; } +/* See breakpoint.h. */ + +bpstat * +bpstat_stop_status_nowatch (const address_space *aspace, CORE_ADDR bp_addr, + thread_info *thread, const target_waitstatus &ws) +{ + gdb_assert (!target_stopped_by_watchpoint ()); + + /* Clear all watchpoints' 'watchpoint_triggered' value from a + previous stop to avoid confusing bpstat_stop_status. */ + watchpoints_triggered (ws); + + return bpstat_stop_status (aspace, bp_addr, thread, ws); +} + static void handle_jit_event (CORE_ADDR address) { diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h index ba28219c236..e412c4d4113 100644 --- a/gdb/breakpoint.h +++ b/gdb/breakpoint.h @@ -968,13 +968,31 @@ extern bpstat *build_bpstat_chain (const address_space *aspace, several reasons concurrently.) Each element of the chain has valid next, breakpoint_at, - commands, FIXME??? fields. */ + commands, FIXME??? fields. + + watchpoints_triggered must be called beforehand to set up each + watchpoint's watchpoint_triggered value. + +*/ extern bpstat *bpstat_stop_status (const address_space *aspace, CORE_ADDR pc, thread_info *thread, const target_waitstatus &ws, bpstat *stop_chain = nullptr); + +/* Like bpstat_stop_status, but clears all watchpoints' + watchpoint_triggered flag. Unlike with bpstat_stop_status, there's + no need to call watchpoint_triggered beforehand. You'll typically + use this variant when handling a known-non-watchpoint event, like a + fork or exec event. */ + +extern bpstat *bpstat_stop_status_nowatch (const address_space *aspace, + CORE_ADDR bp_addr, + thread_info *thread, + const target_waitstatus &ws); + + /* This bpstat_what stuff tells wait_for_inferior what to do with a breakpoint (a challenging task). @@ -1607,8 +1625,9 @@ extern void insert_single_step_breakpoint (struct gdbarch *, otherwise, return false. */ extern int insert_single_step_breakpoints (struct gdbarch *); -/* Check if any hardware watchpoints have triggered, according to the - target. */ +/* Check whether any hardware watchpoints have triggered or not, + according to the target, and record it in each watchpoint's + 'watchpoint_triggered' field. */ int watchpoints_triggered (const target_waitstatus &); /* Helper for transparent breakpoint hiding for memory read and write diff --git a/gdb/infrun.c b/gdb/infrun.c index e3c1db73749..13811eacba3 100644 --- a/gdb/infrun.c +++ b/gdb/infrun.c @@ -4496,9 +4496,9 @@ handle_syscall_event (struct execution_control_state *ecs) infrun_debug_printf ("syscall number=%d", syscall_number); ecs->event_thread->control.stop_bpstat - = bpstat_stop_status (regcache->aspace (), - ecs->event_thread->stop_pc (), - ecs->event_thread, ecs->ws); + = bpstat_stop_status_nowatch (regcache->aspace (), + ecs->event_thread->stop_pc (), + ecs->event_thread, ecs->ws); if (handle_stop_requested (ecs)) return false; @@ -5296,9 +5296,9 @@ handle_inferior_event (struct execution_control_state *ecs) ecs->event_thread->set_stop_pc (regcache_read_pc (regcache)); ecs->event_thread->control.stop_bpstat - = bpstat_stop_status (regcache->aspace (), - ecs->event_thread->stop_pc (), - ecs->event_thread, ecs->ws); + = bpstat_stop_status_nowatch (regcache->aspace (), + ecs->event_thread->stop_pc (), + ecs->event_thread, ecs->ws); if (handle_stop_requested (ecs)) return; @@ -5539,9 +5539,9 @@ handle_inferior_event (struct execution_control_state *ecs) (regcache_read_pc (get_thread_regcache (ecs->event_thread))); ecs->event_thread->control.stop_bpstat - = bpstat_stop_status (get_current_regcache ()->aspace (), - ecs->event_thread->stop_pc (), - ecs->event_thread, ecs->ws); + = bpstat_stop_status_nowatch (get_current_regcache ()->aspace (), + ecs->event_thread->stop_pc (), + ecs->event_thread, ecs->ws); if (handle_stop_requested (ecs)) return; @@ -5650,9 +5650,9 @@ handle_inferior_event (struct execution_control_state *ecs) (regcache_read_pc (get_thread_regcache (ecs->event_thread))); ecs->event_thread->control.stop_bpstat - = bpstat_stop_status (get_current_regcache ()->aspace (), - ecs->event_thread->stop_pc (), - ecs->event_thread, ecs->ws); + = bpstat_stop_status_nowatch (get_current_regcache ()->aspace (), + ecs->event_thread->stop_pc (), + ecs->event_thread, ecs->ws); if (handle_stop_requested (ecs)) return; diff --git a/gdb/testsuite/gdb.base/watch-before-fork.c b/gdb/testsuite/gdb.base/watch-before-fork.c new file mode 100644 index 00000000000..11f50a3f8d1 --- /dev/null +++ b/gdb/testsuite/gdb.base/watch-before-fork.c @@ -0,0 +1,30 @@ +/* This testcase is part of GDB, the GNU debugger. + + Copyright 2021-2022 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 . */ + +#include +#include +#include + +volatile int global_var = 5; + +int +main (void) +{ + global_var = 1; + fork (); + return 0; +} diff --git a/gdb/testsuite/gdb.base/watch-before-fork.exp b/gdb/testsuite/gdb.base/watch-before-fork.exp new file mode 100644 index 00000000000..7c2a481e454 --- /dev/null +++ b/gdb/testsuite/gdb.base/watch-before-fork.exp @@ -0,0 +1,99 @@ +# Copyright 2021-2022 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 . + +# Regression test for PR gdb/28621. Test that GDB does not misreport +# a watchpoint hit when a previous watchpoint hit is immediately +# followed by a catchpoint hit. + +# This test uses "awatch". +if {[skip_hw_watchpoint_access_tests]} { + return +} + +standard_testfile + +if {[build_executable "failed to prepare" $testfile $srcfile debug]} { + return +} + +# Check that fork catchpoints are supported. Returns 1 if they are. +# Returns 0 and issues unsupported if they are not supported. If it +# couldn't be determined, returns 0 (but does not call unsupported). + +proc_with_prefix catch_fork_supported {} { + clean_restart $::testfile + + if { ![runto_main] } { + return 0 + } + + # Verify that the system supports "catch fork". + gdb_test "catch fork" "Catchpoint \[0-9\]* \\(fork\\)" "insert first fork catchpoint" + set has_fork_catchpoints -1 + gdb_test_multiple "continue" "continue to first fork catchpoint" { + -re -wrap ".*Your system does not support this type\r\nof catchpoint.*" { + set has_fork_catchpoints 0 + pass $gdb_test_name + } + -re -wrap ".*Catchpoint.*" { + set has_fork_catchpoints 1 + pass $gdb_test_name + } + } + + if {$has_fork_catchpoints == 1} { + return 1 + } elseif {$has_fork_catchpoints == -1} { + return 0 + } else { + unsupported "catch fork not supported" + return 0 + } +} + +# The test proper. + +proc_with_prefix test {} { + clean_restart $::testfile + + if { ![runto_main] } { + return 0 + } + + gdb_test "awatch global_var" \ + "Hardware access \\(read/write\\) watchpoint .*: global_var.*" \ + "watchpoint on global variable" + + gdb_test "continue" \ + "Hardware access \\(read/write\\) watchpoint .*: global_var.*" \ + "continue to watchpoint" + + set seen_watchpoint 0 + gdb_test_multiple "continue" "continue to catch fork" { + -re "watchpoint" { + set seen_watchpoint 1 + exp_continue + } + -re "$::gdb_prompt " { + gdb_assert { !$seen_watchpoint } $gdb_test_name + } + } +} + +if {![catch_fork_supported] } { + return +} + +test base-commit: 3db1354160aa48995ff37f032310213f0b1cf768 -- 2.26.2