This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

[RFC] Rework debug register accounting for hardware watchpoints


Hi,

This patch implements Pedro's suggestion on how GDB should check
available hardware resources [1].

It's work in progress because I'm still working on the hardware
breakpoints side of things so at this moment I think GDB will get
confused if the user adds both hardware breakpoints and watchpoints.
Still, I wanted to post it to see if you think this is going in the
right direction.

There are no regressions on i386-linux.

[1] - http://sourceware.org/ml/gdb-patches/2011-05/msg00136.html

-- 
[]'s
Thiago Jung Bauermann
IBM Linux Technology Center


2011-07-31  Thiago Jung Bauermann  <bauerman@br.ibm.com>

	Rework debug register accounting for hardware watchpoints.
gdb/
	* breakpoint.c (insert_bp_location): Add prototype.
	(update_watchpoint): Insert all bp_locations before trying
	to insert the watchpoint being updated.
	(insert_bp_location): Go through breakpoints list instead of
	bp_locations list when looking for a duplicate watchpoint.
	(insert_breakpoint_locations): Add `only_hardware' and
	`except' arguments.  Skip bp_locations of EXCEPT breakpoint
	and of software breakpoints and watchpoints when requested.
	Update all callers.
	(hw_watchpoint_used_count): Remove function.

testsuite/
	* gdb.arch/i386-break-watch.exp: New testcase.
	* gdb.arch/i386-break-watch.c: New test source file.
	* lib/gdb.exp (gdb_continue_to_watchpoint): New procedure.

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 8fe16e6..007901c 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -176,8 +176,6 @@ static void maintenance_info_breakpoints (char *, int);
 
 static int hw_breakpoint_used_count (void);
 
-static int hw_watchpoint_used_count (enum bptype, int *);
-
 static void hbreak_command (char *, int);
 
 static void thbreak_command (char *, int);
@@ -214,7 +212,12 @@ static void update_global_location_list_nothrow (int);
 
 static int is_hardware_watchpoint (const struct breakpoint *bpt);
 
-static void insert_breakpoint_locations (void);
+static void insert_breakpoint_locations (int only_hardware,
+					 struct breakpoint *except);
+
+static int insert_bp_location (struct bp_location *bl,
+			       struct ui_file *tmp_error_stream,
+			       int *hw_breakpoint_error);
 
 static int syscall_catchpoint_p (struct breakpoint *b);
 
@@ -1457,37 +1460,57 @@ update_watchpoint (struct watchpoint *b, int reparse)
 
 	  if (reg_cnt)
 	    {
-	      int i, target_resources_ok, other_type_used;
+	      int err = 0, dummy = 0;
+	      struct ui_file *tmp_error_stream;
+	      struct cleanup *cleanups;
 
 	      /* Use an exact watchpoint when there's only one memory region to be
 		 watched, and only one debug register is needed to watch it.  */
 	      b->exact = target_exact_watchpoints && reg_cnt == 1;
 
-	      /* We need to determine how many resources are already
-		 used for all other hardware watchpoints plus this one
-		 to see if we still have enough resources to also fit
-		 this watchpoint in as well.  To guarantee the
-		 hw_watchpoint_used_count call below counts this
-		 watchpoint, make sure that it is marked as a hardware
-		 watchpoint.  */
+	      if (!breakpoints_always_inserted_mode ()
+		  || (!have_live_inferiors ()
+		      && !gdbarch_has_global_breakpoints (target_gdbarch)))
+		insert_breakpoint_locations (1, &b->base);
+
+	      /* Explicitly mark the warning -- this will only be printed if
+	       there was an error.  */
+	      tmp_error_stream = mem_fileopen ();
+	      cleanups = make_cleanup_ui_file_delete (tmp_error_stream);
+	      fprintf_unfiltered (tmp_error_stream, "Warning:\n");
+
+	      save_current_space_and_thread ();
+	      switch_to_program_space_and_thread (b->base.loc->pspace);
+
 	      if (b->base.type == bp_watchpoint)
-		b->base.type = bp_hardware_watchpoint;
+		{
+		  b->base.type = bp_hardware_watchpoint;
+		  for (bl = b->base.loc; bl; bl = bl->next)
+		    bl->loc_type = bp_loc_hardware_watchpoint;
+		}
 
-	      i = hw_watchpoint_used_count (b->base.type, &other_type_used);
-	      target_resources_ok = target_can_use_hardware_watchpoint
-		    (b->base.type, i, other_type_used);
-	      if (target_resources_ok <= 0)
+	      for (bl = b->base.loc; bl && !err; bl = bl->next)
+		err = insert_bp_location (bl, tmp_error_stream, &dummy);
+
+	      if (!breakpoints_always_inserted_mode ()
+		  || (!have_live_inferiors ()
+		      && !gdbarch_has_global_breakpoints (target_gdbarch)))
+		remove_breakpoints ();
+
+	      do_cleanups(cleanups);
+
+	      if (err)
 		{
 		  int sw_mode = b->base.ops->works_in_software_mode (&b->base);
 
-		  if (target_resources_ok == 0 && !sw_mode)
+		  if (sw_mode)
+		    b->base.type = bp_watchpoint;
+		  else if (err == 1)
 		    error (_("Target does not support this type of "
 			     "hardware watchpoint."));
-		  else if (target_resources_ok < 0 && !sw_mode)
+		  else if (err == -1)
 		    error (_("There are not enough available hardware "
 			     "resources for this watchpoint."));
-		  else
-		    b->base.type = bp_watchpoint;
 		}
 	    }
 	  else if (!b->base.ops->works_in_software_mode (&b->base))
@@ -1751,8 +1774,6 @@ insert_bp_location (struct bp_location *bl,
 	}
       else
 	bl->inserted = 1;
-
-      return val;
     }
 
   else if (bl->loc_type == bp_loc_hardware_watchpoint
@@ -1769,23 +1790,28 @@ insert_bp_location (struct bp_location *bl,
 	 supported, try emulating one with an access watchpoint.  */
       if (val == 1 && bl->watchpoint_type == hw_read)
 	{
-	  struct bp_location *loc, **loc_temp;
+	  struct breakpoint *b;
+	  struct bp_location *loc;
 
 	  /* But don't try to insert it, if there's already another
 	     hw_access location that would be considered a duplicate
-	     of this one.  */
-	  ALL_BP_LOCATIONS (loc, loc_temp)
-	    if (loc != bl
-		&& loc->watchpoint_type == hw_access
-		&& watchpoint_locations_match (bl, loc))
-	      {
-		bl->duplicate = 1;
-		bl->inserted = 1;
-		bl->target_info = loc->target_info;
-		bl->watchpoint_type = hw_access;
-		val = 0;
-		break;
-	      }
+	     of this one.  We don't use ALL_BP_LOCATIONS because if
+	     we were called from  update_watchpoint, the bp_location
+	     list still has the old locations from the breakpoint and
+	     we will surely find a duplicate here.  */
+	  ALL_BREAKPOINTS (b)
+	    for (loc = b->loc; loc->next; loc = loc->next)
+	      if (loc != bl
+		  && loc->watchpoint_type == hw_access
+		  && watchpoint_locations_match (bl, loc))
+		{
+		  bl->duplicate = 1;
+		  bl->inserted = 1;
+		  bl->target_info = loc->target_info;
+		  bl->watchpoint_type = hw_access;
+		  val = 0;
+		  break;
+		}
 
 	  if (val == 1)
 	    {
@@ -1827,7 +1853,7 @@ of catchpoint."), bl->owner->number);
       return 0;
     }
 
-  return 0;
+  return val;
 }
 
 /* This function is called when program space PSPACE is about to be
@@ -1896,13 +1922,19 @@ insert_breakpoints (void)
      always_inserted_mode is not enabled.  Explicitly insert them
      now.  */
   if (!breakpoints_always_inserted_mode ())
-    insert_breakpoint_locations ();
+    insert_breakpoint_locations (0, NULL);
 }
 
-/* Used when starting or continuing the program.  */
+/* Used when starting or continuing the program.  If ONLY_HARDWARE is 1,
+   insert only the breakpoint locations corresponding to hardware
+   breakpoints or watchpoints.  If EXCEPT is not NULL, don't insert
+   breakpoint locations belonging to the given breakpoint.
+
+   Returns zero if successful, or an `errno' value if could not write
+   the inferior.  */
 
 static void
-insert_breakpoint_locations (void)
+insert_breakpoint_locations (int only_hardware, struct breakpoint *except)
 {
   struct breakpoint *bpt;
   struct bp_location *bl, **blp_tmp;
@@ -1924,6 +1956,9 @@ insert_breakpoint_locations (void)
       if (!should_be_inserted (bl) || bl->inserted)
 	continue;
 
+      if (except && bl->owner == except)
+	continue;
+
       /* There is no point inserting thread-specific breakpoints if
 	 the thread no longer exists.  ALL_BP_LOCATIONS bp_location
 	 has BL->OWNER always non-NULL.  */
@@ -1931,6 +1966,10 @@ insert_breakpoint_locations (void)
 	  && !valid_thread_id (bl->owner->thread))
 	continue;
 
+      if (only_hardware && !is_hardware_watchpoint (bl->owner)
+	  && bl->loc_type != bp_loc_hardware_breakpoint)
+	continue;
+
       switch_to_program_space_and_thread (bl->pspace);
 
       /* For targets that support global breakpoints, there's no need
@@ -1962,7 +2001,15 @@ insert_breakpoint_locations (void)
 
       if (bpt->disposition == disp_del_at_next_stop)
 	continue;
-      
+
+      /* watch_command_1 creates a watchpoint but only sets its number if
+         update_watchpoint succeeds in creating its bp_locations.  */
+      if (bpt->number == 0)
+	continue;
+
+      if (except && bpt == except)
+	continue;
+
       for (loc = bpt->loc; loc; loc = loc->next)
 	if (!loc->inserted && should_be_inserted (loc))
 	  {
@@ -1979,7 +2026,6 @@ insert_breakpoint_locations (void)
 	  fprintf_unfiltered (tmp_error_stream,
 			      "Could not insert hardware watchpoint %d.\n", 
 			      bpt->number);
-	  error = -1;
 	}
     }
 
@@ -6793,33 +6839,6 @@ hw_breakpoint_used_count (void)
   return i;
 }
 
-static int
-hw_watchpoint_used_count (enum bptype type, int *other_type_used)
-{
-  int i = 0;
-  struct breakpoint *b;
-  struct bp_location *bl;
-
-  *other_type_used = 0;
-  ALL_BREAKPOINTS (b)
-    {
-      if (!breakpoint_enabled (b))
-	continue;
-
-	if (b->type == type)
-	  for (bl = b->loc; bl; bl = bl->next)
-	    {
-	      /* Special types of hardware watchpoints may use more than
-		 one register.  */
-	      i += b->ops->resources_needed (bl);
-	    }
-	else if (is_hardware_watchpoint (b))
-	  *other_type_used = 1;
-    }
-
-  return i;
-}
-
 void
 disable_watchpoints_before_interactive_call_start (void)
 {
@@ -10547,7 +10566,7 @@ update_global_location_list (int should_insert)
   if (breakpoints_always_inserted_mode () && should_insert
       && (have_live_inferiors ()
 	  || (gdbarch_has_global_breakpoints (target_gdbarch))))
-    insert_breakpoint_locations ();
+    insert_breakpoint_locations (0, NULL);
 
   do_cleanups (cleanups);
 }
diff --git a/gdb/testsuite/gdb.arch/i386-break-watch.c b/gdb/testsuite/gdb.arch/i386-break-watch.c
new file mode 100644
index 0000000..6ca3dc7
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/i386-break-watch.c
@@ -0,0 +1,46 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2011 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>
+
+int a = 1;
+short c = 42;
+char d = 'a';
+float f = 1.1;
+int vec[1] = { 30 };
+
+int main (int argc, char *argv[])
+{
+  a = 2;
+  d = 'x';
+  c = 31;
+  f = 2.2;
+  a = 40111222;
+  vec[0] = 2004;
+  a = -20000;
+  d = 'z';
+  f = 3.3;
+  c = 84;
+  a = 20000;
+  vec[0] = 5000005;
+  d = 'b';
+  c = 93;
+  f = 4.4;
+  a = 55000;
+
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.arch/i386-break-watch.exp b/gdb/testsuite/gdb.arch/i386-break-watch.exp
new file mode 100644
index 0000000..1f9092a
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/i386-break-watch.exp
@@ -0,0 +1,89 @@
+# Copyright 2011 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/>.
+
+if { [skip_hw_watchpoint_tests] || [skip_hw_watchpoint_multi_tests] || [skip_hw_watchpoint_access_tests] } {
+    verbose "Skipping hardware breakpoint and watchpoint tests."
+    return
+}
+
+# Do the tests only on one target so that we know how many debug regs
+# are available (four in the case of i386 and x86_64).
+if { ![istarget i?86-*-*] && ![istarget x86_64-*-* ] } {
+    verbose "Skipping hardware breakpoint and watchpoint tests."
+    return
+}
+
+set testfile i386-break-watch
+set srcfile ${testfile}.c
+if { [prepare_for_testing ${testfile}.exp ${testfile} ${srcfile}] } {
+    return -1
+}
+
+if ![runto_main] {
+    untested ${testfile}.exp
+    return -1
+}
+
+gdb_test "watch a" "Hardware watchpoint .: a" "Creating first watchpoint."
+gdb_test "watch c" "Hardware watchpoint .: c" "Creating second watchpoint."
+gdb_test "watch d" "Hardware watchpoint .: d" "Creating third watchpoint."
+gdb_test "watch f" "Hardware watchpoint .: f" "Creating fourth watchpoint."
+
+gdb_test "rwatch vec" "There are not enough available hardware resources for this watchpoint." \
+  "Trying to create one watchpoint more than possible."
+gdb_test_no_output "disable 3" "Disabling watchpoint 3."
+gdb_test "rwatch vec" "Hardware read watchpoint 6: vec" "Creating fifth watchpoint."
+
+# Continue, to guarantee that all watchpoints can be inserted.
+gdb_continue_to_watchpoint "continue 1"
+
+gdb_test_no_output "enable 3" "Enabling watchpoint 3."
+gdb_test "info breakpoints 3" "3.*   watchpoint.*keep y.* c" \
+  "Checking that hw watch was demoted to sw watch."
+
+# Free one debug register.
+gdb_test_no_output "disable 6" "Disabling watchpoint 6."
+
+# Check whether a software watchpoint will be upgraded to hardware watchpoint.
+gdb_test_no_output "disable 3" "Disabling watchpoint 3 (2nd time)."
+gdb_test_no_output "enable 3" "Enabling watchpoint 3 (2nd time)."
+gdb_test "info breakpoints 3" "3.*   hw watchpoint.*keep y.* c" \
+  "Checking that sw watch was promoted to hw watch."
+
+gdb_test "enable 6" "There are not enough available hardware resources for this watchpoint." \
+  "Enabling read watchpoint when there's no free debug register."
+
+delete_breakpoints
+
+# Test that when restarting GDB, a software watchpoint can later be
+# updated to a hardware watchpoint.
+gdb_test "watch a" "Hardware watchpoint .: a" "Creating first watchpoint again."
+gdb_test "watch c" "Hardware watchpoint .: c" "Creating second watchpoint again."
+gdb_test "watch f" "Hardware watchpoint .: f" "Creating fourth watchpoint again."
+gdb_test "watch vec" "Hardware watchpoint 10: vec" "Creating fourth watchpoint again."
+gdb_test "watch d" "Watchpoint 11: d" "Creating fifth watchpoint again."
+
+# Restart GDB.  This takes a while.
+set saved_timeout $timeout
+set timeout 180
+rerun_to_main
+set timeout $saved_timeout
+
+# Free one debug register and update watchpoint 11 to make it use it.
+gdb_test_no_output "disable 10" "Disabling watchpoint 10."
+gdb_test_no_output "disable 11" "Disabling watchpoint 11."
+gdb_test_no_output "enable 11" "Enabling watchpoint 11."
+gdb_test "info breakpoints 11" "11.*   hw watchpoint.*keep y.* d" \
+  "Checking that sw watch was promoted to hw watch after restart."
diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index ef5ad5c..1130cfa 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -497,6 +497,30 @@ proc gdb_continue_to_breakpoint {name {location_pattern .*}} {
 }
 
 
+### Continue, and expect to hit a watchpoint.
+### Report a pass or fail, depending on whether it seems to have
+### worked.  Use NAME as part of the test name; each call to
+### continue_to_watchpoint should use a NAME which is unique within
+### that test file.
+proc gdb_continue_to_watchpoint {name {location_pattern .*}} {
+    global gdb_prompt
+    set full_name "continue to watchpoint: $name"
+
+    send_gdb "continue\n"
+    gdb_expect {
+	-re "\[Ww\]atchpoint .*$location_pattern\r\n$gdb_prompt $" {
+	    pass $full_name
+	}
+	-re ".*$gdb_prompt $" {
+	    fail $full_name
+	}
+	timeout { 
+	    fail "$full_name (timeout)"
+	}
+    }
+}
+
+
 # gdb_internal_error_resync:
 #
 # Answer the questions GDB asks after it reports an internal error



Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]