This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
[RFC] Rework debug register accounting for hardware watchpoints
- From: Thiago Jung Bauermann <bauerman at br dot ibm dot com>
- To: gdb-patches ml <gdb-patches at sourceware dot org>
- Date: Sun, 31 Jul 2011 17:43:54 -0300
- Subject: [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