[binutils-gdb] Forget watchpoint locations when inferior exits or is killed/detached

[Pedro Alves <palves at sourceware dot org>]

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=630008884535a5b26828325e48e729034c110536

commit 630008884535a5b26828325e48e729034c110536
Author: Pedro Alves <palves@redhat.com>
Date:   Fri Jul 1 11:16:32 2016 +0100

    Forget watchpoint locations when inferior exits or is killed/detached
    
    If you have two inferiors (or more), set watchpoints in one of the
    inferiors, and then that inferior exits, until you manually delete the
    watchpoint (or something forces a breakpoint re-set), you can't resume
    the other inferior.
    
    This is exercised by the test added by this commit.  Without the GDB
    fix, this test fails like this:
    
     FAIL: gdb.multi/watchpoint-multi-exit.exp: dispose=kill: continue to marker in inferior 1
     FAIL: gdb.multi/watchpoint-multi-exit.exp: dispose=detach: continue to marker in inferior 1
     FAIL: gdb.multi/watchpoint-multi-exit.exp: dispose=exit: continue to marker in inferior 1
    
    and gdb.log shows (in all three cases):
    
     (gdb) continue
     Continuing.
     Warning:
     Could not insert hardware watchpoint 2.
     Could not insert hardware breakpoints:
     You may have requested too many hardware breakpoints/watchpoints.
    
     Command aborted.
     (gdb) FAIL: gdb.multi/watchpoint-multi-exit.exp: dispose=kill: continue to marker in inferior 1
    
    The problem is that GDB doesn't forget about the locations of
    watchpoints set in the inferior that is now dead.  When we try to
    continue the inferior that is still alive, we reach
    insert_breakpoint_locations, which has the the loop that triggers the
    error:
    
      /* If we failed to insert all locations of a watchpoint, remove
         them, as half-inserted watchpoint is of limited use.  */
    
    That loop finds locations that are not marked inserted, but which
    according to should_be_inserted should have been inserted, and so
    errors out.
    
    gdb/ChangeLog:
    2016-07-01  Pedro Alves  <palves@redhat.com>
    
    	* breakpoint.c (breakpoint_init_inferior): Discard watchpoint
    	locations.
    	* infcmd.c (detach_command): Call breakpoint_init_inferior.
    
    gdb/testsuite/ChangeLog:
    2016-07-01  Pedro Alves  <palves@redhat.com>
    
    	* gdb.multi/watchpoint-multi-exit.c: New file.
    	* gdb.multi/watchpoint-multi-exit.exp: New file.

Diff:
---
 gdb/ChangeLog                                     |  6 ++
 gdb/breakpoint.c                                  | 26 ++++---
 gdb/infcmd.c                                      |  7 ++
 gdb/testsuite/ChangeLog                           |  5 ++
 gdb/testsuite/gdb.multi/watchpoint-multi-exit.c   | 66 +++++++++++++++++
 gdb/testsuite/gdb.multi/watchpoint-multi-exit.exp | 87 +++++++++++++++++++++++
 6 files changed, 189 insertions(+), 8 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 53e3951..978311a 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,11 @@
 2016-07-01  Pedro Alves  <palves@redhat.com>
 
+	* breakpoint.c (breakpoint_init_inferior): Discard watchpoint
+	locations.
+	* infcmd.c (detach_command): Call breakpoint_init_inferior.
+
+2016-07-01  Pedro Alves  <palves@redhat.com>
+
 	* darwin-nat.c (darwin_detach): Use target_announce_detach.
 	* inf-ptrace.c (inf_ptrace_detach): Likewise.
 	* nto-procfs.c (procfs_detach): Likewise.
diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 93dfba6..0b29a8a 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -4204,15 +4204,25 @@ breakpoint_init_inferior (enum inf_context context)
 	  /* Likewise for watchpoints on local expressions.  */
 	  if (w->exp_valid_block != NULL)
 	    delete_breakpoint (b);
-	  else if (context == inf_starting)
+	  else
 	    {
-	      /* Reset val field to force reread of starting value in
-		 insert_breakpoints.  */
-	      if (w->val)
-		value_free (w->val);
-	      w->val = NULL;
-	      w->val_valid = 0;
-	  }
+	      /* Get rid of existing locations, which are no longer
+		 valid.  New ones will be created in
+		 update_watchpoint, when the inferior is restarted.
+		 The next update_global_location_list call will
+		 garbage collect them.  */
+	      b->loc = NULL;
+
+	      if (context == inf_starting)
+		{
+		  /* Reset val field to force reread of starting value in
+		     insert_breakpoints.  */
+		  if (w->val)
+		    value_free (w->val);
+		  w->val = NULL;
+		  w->val_valid = 0;
+		}
+	    }
 	}
 	break;
       default:
diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index e9dcb46..58ba1cb 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -2998,6 +2998,13 @@ detach_command (char *args, int from_tty)
 
   target_detach (args, from_tty);
 
+  /* The current inferior process was just detached successfully.  Get
+     rid of breakpoints that no longer make sense.  Note we don't do
+     this within target_detach because that is also used when
+     following child forks, and in that case we will want to transfer
+     breakpoints to the child, not delete them.  */
+  breakpoint_init_inferior (inf_exited);
+
   /* If the solist is global across inferiors, don't clear it when we
      detach from a single inferior.  */
   if (!gdbarch_has_global_solist (target_gdbarch ()))
diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index 3f6cc80..611de52 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,8 @@
+2016-07-01  Pedro Alves  <palves@redhat.com>
+
+	* gdb.multi/watchpoint-multi-exit.c: New file.
+	* gdb.multi/watchpoint-multi-exit.exp: New file.
+
 2016-06-30  Pedro Alves  <palves@redhat.com>
 
 	* lib/gdbserver-support.exp (close_gdbserver, gdb_exit): Don't
diff --git a/gdb/testsuite/gdb.multi/watchpoint-multi-exit.c b/gdb/testsuite/gdb.multi/watchpoint-multi-exit.c
new file mode 100644
index 0000000..fa672ba
--- /dev/null
+++ b/gdb/testsuite/gdb.multi/watchpoint-multi-exit.c
@@ -0,0 +1,66 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2016 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 <unistd.h>
+#include <stdlib.h>
+#include <sys/types.h>
+#include <sys/wait.h>
+
+/* GDB sets watchpoint here.  */
+static volatile int globalvar;
+
+/* Whether it's expected that the child exits with a signal, vs
+   exiting normally.  GDB sets this.  */
+static volatile int expect_signaled;
+
+static void
+marker (void)
+{
+}
+
+static void
+child_function (void)
+{
+}
+
+int
+main (void)
+{
+  pid_t child;
+
+  child = fork ();
+  if (child == -1)
+    exit (1);
+  else if (child != 0)
+    {
+      int status, ret;
+
+      ret = waitpid (child, &status, 0);
+      if (ret == -1)
+	exit (2);
+      else if (expect_signaled && !WIFSIGNALED (status))
+	exit (3);
+      else if (!expect_signaled && !WIFEXITED (status))
+	exit (4);
+      else
+	marker ();
+    }
+  else
+    child_function ();
+
+  exit (0);
+}
diff --git a/gdb/testsuite/gdb.multi/watchpoint-multi-exit.exp b/gdb/testsuite/gdb.multi/watchpoint-multi-exit.exp
new file mode 100644
index 0000000..e811422
--- /dev/null
+++ b/gdb/testsuite/gdb.multi/watchpoint-multi-exit.exp
@@ -0,0 +1,87 @@
+# Copyright 2016 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/>.
+
+# Make sure that if an inferior exits or is detached/killed, its
+# watchpoints don't end up with stale locations, preventing resumption
+# of other inferiors.
+
+standard_testfile
+
+if {[build_executable "failed to build" $testfile $srcfile {debug}]} {
+    return -1
+}
+
+# The test proper.  DISPOSE indicates how to dispose of the fork
+# child.  Can be either "kill", "detach", or "exit" (to continue it to
+# normal exit).
+proc do_test {dispose} {
+    global binfile
+
+    clean_restart $binfile
+
+    gdb_test_no_output "set follow-fork child"
+    gdb_test_no_output "set detach-on-fork off"
+
+    if ![runto "child_function"] {
+	fail "Can't run to child_function"
+	return
+    }
+
+    gdb_test "watch globalvar" "atchpoint \[0-9\]+: globalvar" \
+	"set watchpoint on inferior 2"
+
+    # Dispose of the inferior.  This should get rid of this inferior's
+    # watchpoint locations.
+    if {$dispose == "kill"} {
+	gdb_test "kill" "" "kill inferior 2" \
+	    "Kill the program being debugged.*y or n. $" "y"
+    } elseif {$dispose == "detach"} {
+	gdb_test "detach" ".*" "detach inferior 2"
+    } elseif {$dispose == "exit"} {
+	gdb_test "continue" ".*exited normally.*" "run to exit inferior 2"
+    } else {
+	perror "unhandled dispose: $dispose"
+    }
+
+    gdb_test "inferior 1" "witching to inferior 1 .*" \
+	"switch back to inferior 1"
+
+    if {$dispose == "kill"} {
+	gdb_test "print expect_signaled = 1" " = 1"
+    }
+
+    gdb_breakpoint "marker"
+
+    # Now continue inferior 1.  Before GDB was fixed, watchpoints for
+    # inferior 2 managed to be left behind.  When GDB realized that
+    # they hadn't been inserted, the continue command was aborted:
+    #
+    #  (gdb) continue
+    #  Continuing.
+    #  Warning:
+    #  Could not insert hardware watchpoint 2.
+    #  Could not insert hardware breakpoints:
+    #  You may have requested too many hardware breakpoints/watchpoints.
+    #
+    #  Command aborted.
+    #  (gdb)
+    #
+    gdb_test "continue" "Breakpoint \[0-9\]+, marker .*" \
+	"continue in inferior 1"
+}
+
+foreach_with_prefix dispose {"kill" "detach" "exit"} {
+    do_test $dispose
+}