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

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index a3cfeea6989aa0c1ae6fb50ace7e21d384bac2de..ebcefdee54d107a9ffb5f5bf33f1e6504eea3221 100644 (file)
--- 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 ba28219c23635de465657e51e75156667ae5b501..e412c4d4113ccaab125772debe7b3e3a0a511298 100644 (file)
--- 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);
 \f
+
+
 /* 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 104c29abf0a97cfac0bbb927c6f89d0b602a8fb3..737710f5bae7c7c7dd7f03fac540a6dc455c43cb 100644 (file)
--- 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 (file)
index 0000000..4d6c91e
--- /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 <http://www.gnu.org/licenses/>.  */
+
+#include <sys/types.h>
+#include <unistd.h>
+
+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 (file)
index 0000000..7c2a481
--- /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 <http://www.gnu.org/licenses/>.
+
+# 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