[PATCH] Watchpoint followed by catchpoint misreports watchpoint (PR gdb/28621)

[PATCH] Watchpoint followed by catchpoint misreports watchpoint (PR gdb/28621)

[Pedro Alves]

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);
 \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 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 <http://www.gnu.org/licenses/>.  */
+
+#include <stdio.h>
+#include <unistd.h>
+#include <stdlib.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
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 <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

base-commit: 3db1354160aa48995ff37f032310213f0b1cf768
-- 
2.26.2

Re: [PATCH] Watchpoint followed by catchpoint misreports watchpoint (PR gdb/28621)

[Pedro Alves]

On 2022-03-07 16:56, Pedro Alves wrote:
> 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

I've merged this to master.

Pedro Alvse