This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
[RFA] Use multiple locations for hardware watchpoints.
- From: Vladimir Prus <vladimir at codesourcery dot com>
- To: gdb-patches at sources dot redhat dot com
- Date: Tue, 20 Nov 2007 20:32:26 +0300
- Subject: [RFA] Use multiple locations for hardware watchpoints.
In order to implement watchpoint for a given expression, GDB
might need several hardware watchpoints. Presently, that list
of values to watch for is keep in val_chain field of struct
breakpoint. There are at least three places that walk over
this list, decide if this value should actually be watched
for, compute address and length, and actually insert, remove,
or check watchpoint. This patch changes that to store locations
inside breakpoint->location. In the sake of incremental development,
it does not tries to access all FIXMEs that say "when watchpoints
use multiple locations". The patch also recomputes the locations
inside insert_breakpoints -- which is essentially the same
as current approach to compute locations in insert_bp_locations.
Some more cleanup is a topic for future. OK?
- Volodya
Use multiple locations for hardware watchpoints.
This eliminates the need to traverse value chain, doing
various checks, in three different places.
* breakpoint.h (struct bp_location): New fields
lengths and watchpoint_type.
(struct breakpoint): Remove the val_chain field.
* breakpoint.c (is_hardware_watchpoint): New.
(free_valchain): Remove.
(update_watchpoint_locations): New.
(insert_bp_location): For hardware watchpoint, just
directly insert it.
(insert_breakpoints): Call update_watchpoint_locations
on all watchpoints. If we have failed to insert
any location of a hardware watchpoint, remove all inserted
locations.
(remove_breakpoint): For hardware watchpoints, directly
remove location.
(watchpoints_triggered): Iterate over locations.
(bpstat_stop_status): Use only first location of
a resource watchpoint.
(delete_breakpoint): Don't call free_valchain.
(print_one_breakpoint): Don't print all
locations for watchpoints.
---
gdb/breakpoint.c | 410 +++++++++++++++++++++++++++---------------------------
gdb/breakpoint.h | 9 +-
2 files changed, 213 insertions(+), 206 deletions(-)
diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 32f4dd1..601bc49 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -202,6 +202,15 @@ static void free_bp_location (struct bp_location *loc);
static void mark_breakpoints_out (void);
+static struct bp_location *
+allocate_bp_location (struct breakpoint *bpt, enum bptype bp_type);
+
+static void
+unlink_locations_from_global_list (struct breakpoint *bpt);
+
+static int
+is_hardware_watchpoint (struct breakpoint *bpt);
+
/* Prototypes for exported functions. */
/* If FALSE, gdb will not use hardware support for watchpoints, even
@@ -810,24 +819,137 @@ insert_catchpoint (struct ui_out *uo, void *args)
}
}
-/* Helper routine: free the value chain for a breakpoint (watchpoint). */
+static int
+is_hardware_watchpoint (struct breakpoint *bpt)
+{
+ return (bpt->type == bp_hardware_watchpoint
+ || bpt->type == bp_read_watchpoint
+ || bpt->type == bp_access_watchpoint);
+}
+/* Assuming that B is a breakpoint of type bp_hardware_breakpoint,
+ update B->loc to the list of locations that must
+ be watched. */
static void
-free_valchain (struct bp_location *b)
+update_watchpoint_locations (struct breakpoint *b)
{
- struct value *v;
- struct value *n;
+ int within_current_scope;
+ struct value *mark = value_mark ();
+ struct frame_id saved_frame_id;
+ struct bp_location *loc;
+ bpstat bs;
+
+ unlink_locations_from_global_list (b);
+ for (loc = b->loc; loc;)
+ {
+ struct bp_location *loc_next = loc->next;
+ remove_breakpoint (loc, mark_uninserted);
+ xfree (loc);
+ loc = loc_next;
+ }
+ b->loc = NULL;
+
+ /* Save the current frame's ID so we can restore it after
+ evaluating the watchpoint expression on its own frame. */
+ /* FIXME drow/2003-09-09: It would be nice if evaluate_expression
+ took a frame parameter, so that we didn't have to change the
+ selected frame. */
+ saved_frame_id = get_frame_id (get_selected_frame (NULL));
+
+ /* Determine if the watchpoint is within scope. */
+ if (b->exp_valid_block == NULL)
+ within_current_scope = 1;
+ else
+ {
+ struct frame_info *fi;
+ fi = frame_find_by_id (b->watchpoint_frame);
+ within_current_scope = (fi != NULL);
+ if (within_current_scope)
+ select_frame (fi);
+ }
+
+ if (within_current_scope)
+ {
+ struct value *v, *top_value;
+
+ /* Evaluate the expression and cut the chain of values
+ produced off from the value chain.
+
+ Make sure the value returned isn't lazy; we use
+ laziness to determine what memory GDB actually needed
+ in order to compute the value of the expression. */
+ top_value = v = evaluate_expression (b->exp);
+ value_contents (v);
+ value_release_to_mark (mark);
+
+ /* Look at each value on the value chain. */
+ for (; v; v = value_next (v))
+ {
+ /* If it's a memory location, and GDB actually needed
+ its contents to evaluate the expression, then we
+ must watch it. */
+ if (VALUE_LVAL (v) == lval_memory
+ && ! value_lazy (v))
+ {
+ struct type *vtype = check_typedef (value_type (v));
+
+ /* We only watch structs and arrays if user asked
+ for it explicitly, never if they just happen to
+ appear in the middle of some value chain. */
+ if (v == top_value
+ || (TYPE_CODE (vtype) != TYPE_CODE_STRUCT
+ && TYPE_CODE (vtype) != TYPE_CODE_ARRAY))
+ {
+ CORE_ADDR addr;
+ int len, type;
+ struct bp_location *loc, **tmp;
- /* Free the saved value chain. We will construct a new one
- the next time the watchpoint is inserted. */
- for (v = b->owner->val_chain; v; v = n)
+ addr = VALUE_ADDRESS (v) + value_offset (v);
+ len = TYPE_LENGTH (value_type (v));
+ type = hw_write;
+ if (b->type == bp_read_watchpoint)
+ type = hw_read;
+ else if (b->type == bp_access_watchpoint)
+ type = hw_access;
+
+ loc = allocate_bp_location (b, bp_hardware_watchpoint);
+ for (tmp = &(b->loc); *tmp != NULL; tmp = &((*tmp)->next))
+ ;
+ *tmp = loc;
+ loc->address = addr;
+ loc->length = len;
+ loc->watchpoint_type = type;
+ }
+ }
+ }
+
+ if (b->cond_string != NULL)
+ {
+ char *s = b->cond_string;
+ if (b->loc->cond)
+ {
+ xfree (b->loc->cond);
+ b->loc->cond = NULL;
+ }
+ b->loc->cond = parse_exp_1 (&s, b->exp_valid_block, 0);
+ }
+ }
+ else
{
- n = value_next (v);
- value_free (v);
+ printf_filtered (_("\
+Hardware watchpoint %d deleted because the program has left the block \n\
+in which its expression is valid.\n"),
+ b->number);
+ if (b->related_breakpoint)
+ b->related_breakpoint->disposition = disp_del_at_next_stop;
+ b->disposition = disp_del_at_next_stop;
}
- b->owner->val_chain = NULL;
+
+ /* Restore the selected frame. */
+ select_frame (frame_find_by_id (saved_frame_id));
}
+
/* Insert a low-level "breakpoint" of some type. BPT is the breakpoint.
Any error messages are printed to TMP_ERROR_STREAM; and DISABLED_BREAKS,
PROCESS_WARNING, and HW_BREAKPOINT_ERROR are used to report problems.
@@ -1020,136 +1142,10 @@ Note: automatically using hardware breakpoints for read-only addresses.\n"));
watchpoints. It's not clear that it's necessary... */
&& bpt->owner->disposition != disp_del_at_next_stop)
{
- /* FIXME drow/2003-09-08: This code sets multiple hardware watchpoints
- based on the expression. Ideally this should happen at a higher level,
- and there should be one bp_location for each computed address we
- must watch. As soon as a many-to-one mapping is available I'll
- convert this. */
-
- int within_current_scope;
- struct value *mark = value_mark ();
- struct value *v;
- struct frame_id saved_frame_id;
-
- /* Save the current frame's ID so we can restore it after
- evaluating the watchpoint expression on its own frame. */
- /* FIXME drow/2003-09-09: It would be nice if evaluate_expression
- took a frame parameter, so that we didn't have to change the
- selected frame. */
- saved_frame_id = get_frame_id (get_selected_frame (NULL));
-
- /* Determine if the watchpoint is within scope. */
- if (bpt->owner->exp_valid_block == NULL)
- within_current_scope = 1;
- else
- {
- struct frame_info *fi;
- fi = frame_find_by_id (bpt->owner->watchpoint_frame);
- within_current_scope = (fi != NULL);
- if (within_current_scope)
- select_frame (fi);
- }
-
- if (within_current_scope)
- {
- free_valchain (bpt);
-
- /* Evaluate the expression and cut the chain of values
- produced off from the value chain.
-
- Make sure the value returned isn't lazy; we use
- laziness to determine what memory GDB actually needed
- in order to compute the value of the expression. */
- v = evaluate_expression (bpt->owner->exp);
- value_contents (v);
- value_release_to_mark (mark);
-
- bpt->owner->val_chain = v;
- bpt->inserted = 1;
-
- /* Look at each value on the value chain. */
- for (; v; v = value_next (v))
- {
- /* If it's a memory location, and GDB actually needed
- its contents to evaluate the expression, then we
- must watch it. */
- if (VALUE_LVAL (v) == lval_memory
- && ! value_lazy (v))
- {
- struct type *vtype = check_typedef (value_type (v));
-
- /* We only watch structs and arrays if user asked
- for it explicitly, never if they just happen to
- appear in the middle of some value chain. */
- if (v == bpt->owner->val_chain
- || (TYPE_CODE (vtype) != TYPE_CODE_STRUCT
- && TYPE_CODE (vtype) != TYPE_CODE_ARRAY))
- {
- CORE_ADDR addr;
- int len, type;
-
- addr = VALUE_ADDRESS (v) + value_offset (v);
- len = TYPE_LENGTH (value_type (v));
- type = hw_write;
- if (bpt->owner->type == bp_read_watchpoint)
- type = hw_read;
- else if (bpt->owner->type == bp_access_watchpoint)
- type = hw_access;
-
- val = target_insert_watchpoint (addr, len, type);
- if (val == -1)
- {
- /* Don't exit the loop, try to insert
- every value on the value chain. That's
- because we will be removing all the
- watches below, and removing a
- watchpoint we didn't insert could have
- adverse effects. */
- bpt->inserted = 0;
- }
- val = 0;
- }
- }
- }
-
- if (bpt->owner->cond_string != NULL)
- {
- char *s = bpt->owner->cond_string;
- if (bpt->cond)
- {
- xfree (bpt->cond);
- bpt->cond = NULL;
- }
- bpt->cond = parse_exp_1 (&s, bpt->owner->exp_valid_block, 0);
- }
-
- /* Failure to insert a watchpoint on any memory value in the
- value chain brings us here. */
- if (!bpt->inserted)
- {
- remove_breakpoint (bpt, mark_uninserted);
- *hw_breakpoint_error = 1;
- fprintf_unfiltered (tmp_error_stream,
- "Could not insert hardware watchpoint %d.\n",
- bpt->owner->number);
- val = -1;
- }
- }
- else
- {
- printf_filtered (_("\
-Hardware watchpoint %d deleted because the program has left the block \n\
-in which its expression is valid.\n"),
- bpt->owner->number);
- if (bpt->owner->related_breakpoint)
- bpt->owner->related_breakpoint->disposition = disp_del_at_next_stop;
- bpt->owner->disposition = disp_del_at_next_stop;
- }
-
- /* Restore the selected frame. */
- select_frame (frame_find_by_id (saved_frame_id));
-
- return val;
+ val = target_insert_watchpoint (bpt->address,
+ bpt->length,
+ bpt->watchpoint_type);
+ bpt->inserted = (val != -1);
}
else if (ep_is_exception_catchpoint (bpt->owner))
@@ -1237,6 +1233,7 @@ in which its expression is valid.\n"),
int
insert_breakpoints (void)
{
+ struct breakpoint *bpt;
struct bp_location *b, *temp;
int return_val = 0; /* return success code. */
int val = 0;
@@ -1251,6 +1248,10 @@ insert_breakpoints (void)
there was an error. */
fprintf_unfiltered (tmp_error_stream, "Warning:\n");
+ ALL_BREAKPOINTS (bpt)
+ if (is_hardware_watchpoint (bpt))
+ update_watchpoint_locations (bpt);
+
ALL_BP_LOCATIONS_SAFE (b, temp)
{
/* Permanent breakpoints cannot be inserted or removed. Disabled
@@ -1284,6 +1285,39 @@ insert_breakpoints (void)
return_val = val;
}
+ /* If we failed to insert all locations of a watchpoint,
+ remove them, as half-inserted watchpoint is of limited use. */
+ ALL_BREAKPOINTS (bpt)
+ {
+ int some_failed = 0;
+ struct bp_location *loc;
+
+ if (!is_hardware_watchpoint (bpt))
+ continue;
+
+ if (bpt->enable_state != bp_enabled)
+ continue;
+
+ for (loc = bpt->loc; loc; loc = loc->next)
+ if (!loc->inserted)
+ {
+ some_failed = 1;
+ break;
+ }
+ if (some_failed)
+ {
+ for (loc = bpt->loc; loc; loc = loc->next)
+ if (loc->inserted)
+ remove_breakpoint (loc, mark_uninserted);
+
+ hw_breakpoint_error = 1;
+ fprintf_unfiltered (tmp_error_stream,
+ "Could not insert hardware watchpoint %d.\n",
+ bpt->number);
+ return_val = -1;
+ }
+ }
+
if (return_val)
{
/* If a hardware breakpoint or watchpoint was inserted, add a
@@ -1577,46 +1611,15 @@ remove_breakpoint (struct bp_location *b, insertion_state_t is)
return val;
b->inserted = (is == mark_inserted);
}
- else if (b->loc_type == bp_loc_hardware_watchpoint
- && breakpoint_enabled (b->owner)
- && !b->duplicate)
+ else if (b->loc_type == bp_loc_hardware_watchpoint)
{
struct value *v;
struct value *n;
b->inserted = (is == mark_inserted);
- /* Walk down the saved value chain. */
- for (v = b->owner->val_chain; v; v = value_next (v))
- {
- /* For each memory reference remove the watchpoint
- at that address. */
- if (VALUE_LVAL (v) == lval_memory
- && ! value_lazy (v))
- {
- struct type *vtype = check_typedef (value_type (v));
-
- if (v == b->owner->val_chain
- || (TYPE_CODE (vtype) != TYPE_CODE_STRUCT
- && TYPE_CODE (vtype) != TYPE_CODE_ARRAY))
- {
- CORE_ADDR addr;
- int len, type;
-
- addr = VALUE_ADDRESS (v) + value_offset (v);
- len = TYPE_LENGTH (value_type (v));
- type = hw_write;
- if (b->owner->type == bp_read_watchpoint)
- type = hw_read;
- else if (b->owner->type == bp_access_watchpoint)
- type = hw_access;
+ val = target_remove_watchpoint (b->address, b->length,
+ b->watchpoint_type);
- val = target_remove_watchpoint (addr, len, type);
- if (val == -1)
- b->inserted = 1;
- val = 0;
- }
- }
- }
/* Failure to remove any of the hardware watchpoints comes here. */
if ((is == mark_uninserted) && (b->inserted))
warning (_("Could not remove hardware watchpoint %d."),
@@ -2591,33 +2594,19 @@ watchpoints_triggered (struct target_waitstatus *ws)
|| b->type == bp_read_watchpoint
|| b->type == bp_access_watchpoint)
{
+ struct bp_location *loc;
struct value *v;
b->watchpoint_triggered = watch_triggered_no;
- for (v = b->val_chain; v; v = value_next (v))
- {
- if (VALUE_LVAL (v) == lval_memory && ! value_lazy (v))
- {
- struct type *vtype = check_typedef (value_type (v));
-
- if (v == b->val_chain
- || (TYPE_CODE (vtype) != TYPE_CODE_STRUCT
- && TYPE_CODE (vtype) != TYPE_CODE_ARRAY))
- {
- CORE_ADDR vaddr;
-
- vaddr = VALUE_ADDRESS (v) + value_offset (v);
- /* Exact match not required. Within range is
- sufficient. */
- if (addr >= vaddr
- && addr < vaddr + TYPE_LENGTH (value_type (v)))
- {
- b->watchpoint_triggered = watch_triggered_yes;
- break;
- }
- }
- }
- }
+ for (loc = b->loc; loc; loc = loc->next)
+ /* Exact match not required. Within range is
+ sufficient. */
+ if (addr >= loc->address
+ && addr < loc->address + loc->length)
+ {
+ b->watchpoint_triggered = watch_triggered_yes;
+ break;
+ }
}
return 1;
@@ -2864,6 +2853,15 @@ bpstat_stop_status (CORE_ADDR bp_addr, ptid_t ptid)
!(current_exception_event = target_get_current_exception_event ()))
continue;
+ /* For hardware watchpoints, we look only at the first location.
+ The watchpoint_check function will work on entire expression,
+ not the individual locations. For read watchopints, the
+ watchpoints_triggered function have checked all locations
+ alrea
+ */
+ if (b->type == bp_hardware_watchpoint && bl != b->loc)
+ continue;
+
/* Come here if it's a watchpoint, or if the break address matches */
bs = bpstat_alloc (bl, bs); /* Alloc a bpstat to explain stop */
@@ -3059,6 +3057,10 @@ bpstat_stop_status (CORE_ADDR bp_addr, ptid_t ptid)
|| bs->breakpoint_at->owner->type == bp_read_watchpoint
|| bs->breakpoint_at->owner->type == bp_access_watchpoint))
{
+ /* remove/insert can invalidate bs->breakpoint_at, if this
+ location is no longer used by the watchpoint. Prevent
+ further code from trying to use it. */
+ bs->breakpoint_at = NULL;
remove_breakpoints ();
insert_breakpoints ();
break;
@@ -3829,10 +3831,14 @@ print_one_breakpoint (struct breakpoint *b,
disabled, we print it as if it had
several locations, since otherwise it's hard to
represent "breakpoint enabled, location disabled"
- situation. */
+ situation.
+ Note that while hardware watchpoints have
+ several locations internally, that's no a property
+ exposed to user. */
if (b->loc
+ && !is_hardware_watchpoint (b)
&& (b->loc->next || !b->loc->enabled)
- && !ui_out_is_mi_like_p (uiout))
+ && !ui_out_is_mi_like_p (uiout))
{
struct bp_location *loc;
int n = 1;
@@ -7114,9 +7120,7 @@ delete_breakpoint (struct breakpoint *bpt)
{
if (loc->inserted)
remove_breakpoint (loc, mark_inserted);
-
- free_valchain (loc);
-
+
if (loc->cond)
xfree (loc->cond);
diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
index f13d41b..e9e1e65 100644
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -280,6 +280,12 @@ struct bp_location
bp_loc_other. */
CORE_ADDR address;
+ /* For hardware watchpoints, the size of data ad ADDRESS being watches. */
+ int length;
+
+ /* Type of hardware watchpoint. */
+ enum target_hw_bp_type watchpoint_type;
+
/* For any breakpoint type with an address, this is the BFD section
associated with the address. Used primarily for overlay debugging. */
asection *section;
@@ -395,9 +401,6 @@ struct breakpoint
/* Value of the watchpoint the last time we checked it. */
struct value *val;
- /* Holds the value chain for a hardware watchpoint expression. */
- struct value *val_chain;
-
/* Holds the address of the related watchpoint_scope breakpoint
when using watchpoints on local variables (might the concept
of a related breakpoint be useful elsewhere, if not just call
--
1.5.3.5