From d37e084783a04c63ae137f953ebdb58bb6f7f704 Mon Sep 17 00:00:00 2001 From: Pedro Alves Date: Tue, 23 Nov 2021 14:19:07 +0000 Subject: [PATCH] Watchpoint followed by catchpoint misreports watchpoint (PR gdb/28621) 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 | 29 ++++++ gdb/testsuite/gdb.base/watch-before-fork.exp | 99 ++++++++++++++++++++ 5 files changed, 177 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 104c29abf0a..737710f5bae 100644 --- a/gdb/infrun.c +++ b/gdb/infrun.c @@ -4491,9 +4491,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; @@ -5288,9 +5288,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; @@ -5531,9 +5531,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; @@ -5642,9 +5642,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..4d6c91e1e6f --- /dev/null +++ b/gdb/testsuite/gdb.base/watch-before-fork.c @@ -0,0 +1,29 @@ +/* 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 + +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 -- 2.30.2