This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [RFA] Don't reset watchpoint block on solib load.
- From: Vladimir Prus <vladimir at codesourcery dot com>
- To: Jim Blandy <jimb at codesourcery dot com>
- Cc: gdb-patches at sources dot redhat dot com, Daniel Jacobowitz <dan at codesourcery dot com>
- Date: Wed, 23 Jan 2008 12:57:27 +0300
- Subject: Re: [RFA] Don't reset watchpoint block on solib load.
- References: <200711202013.47537.vladimir@codesourcery.com> <200711281858.51145.vladimir@codesourcery.com> <m38x2qh83j.fsf@codesourcery.com>
On Wednesday 16 January 2008 04:28:32 Jim Blandy wrote:
>
> (Since it took me a bit to re-discover what this patch was about, I
> figure I should recap...)
>
> breakpoint_re_set_one gets called when the symbolic information
> available to GDB has changed: we've re-run or attached and the
> executable file has changed since we last read it, or the program has
> loaded or unloaded a shared library.
I think we've established that right now, breakpoint_re_set_one is
not called when a library is unloaded.
> The breakpoint_re_set_one
> function is responsible for updating all references to symbol table
> contents in breakpoints, watchpoints, etc.
>
> A watchpoint can refer to local variables that were in scope when it
> was set, so it may be associated with a particular frame. However,
> breakpoint_re_set_one re-parses watchpoint expressions using
> parse_expression, which parses in the scope of whatever frame happens
> to selected when it is called. This is very wrong: GDB can call
> breakpoint_re_set_one in many different circumstances, and the frame
> selected at that time has no necessary relationship to the frame in
> which the user set the watchpoint.
>
> In particular, GDB calls breakpoint_re_set_one when it stops for a
> shared library load or unload. At this point, the selected frame is
> the youngest frame, in the dynamic linker's debug breakpoint function.
> Since watchpoints' local variables are almost certainly not in scope
> in that function, GDB fails to re-parse watchpoint expressions that
> refer to local variables whenever we do a dlopen or dlclose. (And
> then fails to handle the error and dies.) For example:
>
> $ cat w1.c
> #include <dlfcn.h>
>
> int
> main (int argc, char **argv)
> {
> volatile int g = 1;
>
> g = 2;
>
> void *lib = dlopen ("libm.so", RTLD_NOW);
>
> g = 3;
>
> return g;
> }
> $ gcc -g w1.c -ldl -o w1
> $ ~/gdb/pub/nat/gdb/gdb w1
> GNU gdb 6.7.50.20080111-cvs
> Copyright (C) 2008 Free Software Foundation, Inc.
> License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
> This is free software: you are free to change and redistribute it.
> There is NO WARRANTY, to the extent permitted by law. Type "show copying"
> and "show warranty" for details.
> This GDB was configured as "i686-pc-linux-gnu"...
> (gdb) b 10
> Breakpoint 1 at 0x8048403: file w1.c, line 10.
> (gdb) run
> Starting program: /home/jimb/play/w1
>
> Breakpoint 1, main () at w1.c:10
> 10 void *lib = dlopen ("libm.so", RTLD_NOW);
> (gdb) watch g
> Hardware watchpoint 2: g
> (gdb) next
> Error in re-setting breakpoint 2: No symbol "g" in current context.
> Segmentation fault (core dumped)
> $
>
> I believe Vlad's most recent patch is this one:
>
> http://sourceware.org/ml/gdb-patches/2007-11/msg00522.html
>
> The patch has two effects:
>
> First, in breakpoint_re_set_one (w), if W->exp_valid_block is NULL
> (meaning that the watchpoint refers to no local variables), or if it
> is set and GDB can find W->watchpoint_frame on the stack, then it
> tries to re-parse the expression in exp_valid_block.
>
> Second, it re-parses watchpoint condition expressions (the Y in 'watch
> X if Y') when it inserts breakpoints.
>
> I have two questions about this patch.
>
> - If we dlclose a shared library while there are still frames in that
> library's code on the stack, couldn't exp_valid_block be pointing to
> garbage (i.e. a 'struct block' in the shared library whose debug
> info has now been freed), even though frame_find_by_id can find the
> frame?
It's possible. And it's pre-existing problem ;-) Basically, now both
dlopen and dlclose mess up watchpoints, and the patch is a step that
brings us closer to dlopen working.
> Granted, the user's program is unhappy, because it's going to return
> to code that isn't there any more, but GDB shouldn't crash --- after
> all, debuggers are meant for use on unhappy programs.
>
> If we can find the frame, and we can find the frame's block, could
> we use that instead?
Did you mean "if we can find the same, and *can't* find the block"?
Basically, if we have a watchpoint associated with frame id, and we can't
find that frame id, there's nothing we can do -- because we can't access
local variables relatively to frame base we don't know. However,
it's possible that frame is found, but in fact, the block is gone -- in
which case we should not try to use the gone block.
As I mention, this is pre-existing problem. We need a mechanism to get
notified when a block is gone. Note that it's strictly speaking
different from shared library unloaded, since syntab is not necessary
removed at this point. So, ideally, we'd need a new observer that's called
when symtab is removed. We'll plug into that observer and disable
all watchpoints which conditions use the gone blocks. In fact, I'm
not sure that this problem is only relevant to watchpoints. Ordinary
breakpoint might well have a condition that uses variables from shared
libraries, and such condition will become invalid when shared library
is removed.
So, I think it's a global existing problem, and we should postpone it.
> In the perverse case I outlined, I assume we'd
> be able to find the frame, but not the block.
>
> Certainly, for watchpoints that cite no local variables, we should
> just always try to re-parse. The prior conversation identified some
> ugly issues there, too, but that's an existing bug; let's take it
> one step at a time.
>
> - Why shouldn't we re-parse the condition at the same time we re-parse
> the expression, in the same block? Why should we re-parse the
> condition when we insert breakpoints?
>
> Although I agreed to reparsing the condition when we insert
> breakpoints earlier in the thread, I can't see now why it's a good
> idea. It seems wasteful, since we remove and insert breakpoints
> frequently, and since we do remove and re-insert breakpoints on
> dlopen/dlclose, it doesn't seem to be a significantly more
> auspicious time to do the parse.
Well, we reparse the entire watchpoint expression on each
insert already, so reparsing a condition is not a big deal. However,
you're right it's not needed, strictly speaking.
Honestly, the point of moving it for this patch was to place all
code for updating watchpoint in one place.
The next patch, that switches watchpoint to using multiple locations,
introduced update_watchpoint function which is a single place for
all watchpoint updates. Looking at the patch, however, I see that
I always reparse condition anyway. I've changed that patch to
reparse condition only when update_watchpoint is called with reparse=1,
which happens only from breakpoint_re_set_one, and I don't get any
regression as result.
So, I guess the solutions are either:
1. Omit the move of condition parsing from this patch, or
2. Keep condition parsing in this patch, and then commit
the use-multiple-locations-for-watchpoint patch which will
immediately make condition only reparsed when breakpoint_re_set_one
is called.
Given that use-multiple-locations-for-watchpoint patch is
approved except for the bit that makes reparsing of condition
optional, it seems that (2) is less work.
Jim, does this look good for you?
For convenience, I attach the current version of
use-multiple-locations-for-watchpoint patch. The difference
to the previous revision is as follows:
@@ -966,7 +966,7 @@ update_watchpoint (struct breakpoint *b, int reparse)
value_free (v);
}
- if (b->cond_string != NULL)
+ if (reparse && b->cond_string != NULL)
{
char *s = b->cond_string;
if (b->loc->cond)
Dan, is the patch still OK to commit?
- Volodya
From 536a6f0019cc9915ed52c8169af88f4c5c12f057 Mon Sep 17 00:00:00 2001
From: Vladimir Prus <vladimir@codesourcery.com>
Date: Tue, 20 Nov 2007 20:10:54 +0300
Subject: [RFA] Use multiple locations for hardware watchpoints.
To: gdb-patches@sources.redhat.com
X-KMail-Transport: CodeSourcery
X-KMail-Identity: 901867920
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): 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.
(breakpoint_re_set_one): Use update_watchpoint for
watchpoints.
testsuite/
* gdb.base/watchpoint-solib.exp: New.
* gdb.base/watchpoint-solib.c: New.
* gdb.base/watchpoint-solib-shr.c: New.
---
gdb/breakpoint.c | 502 +++++++++++++------------
gdb/breakpoint.h | 9 +-
gdb/testsuite/gdb.base/watchpoint-solib-shr.c | 25 ++
gdb/testsuite/gdb.base/watchpoint-solib.c | 68 ++++
gdb/testsuite/gdb.base/watchpoint-solib.exp | 89 +++++
5 files changed, 441 insertions(+), 252 deletions(-)
create mode 100644 gdb/testsuite/gdb.base/watchpoint-solib-shr.c
create mode 100644 gdb/testsuite/gdb.base/watchpoint-solib.c
create mode 100644 gdb/testsuite/gdb.base/watchpoint-solib.exp
diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index ecc2478..0bed4ef 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -200,6 +200,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
@@ -808,24 +817,182 @@ 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 hardware breakpoint:
+ - Reparse watchpoint expression, is REPARSE is non-zero
+ - Evaluate expression and store the result in B->val
+ - Update the list of values that must be watched in B->loc.
+
+ If the watchpoint is disabled, do nothing. If this is
+ local watchpoint that is out of scope, delete it. */
static void
-free_valchain (struct bp_location *b)
+update_watchpoint (struct breakpoint *b, int reparse)
{
- 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;
- /* 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)
+ if (b->disposition == disp_del_at_next_stop)
+ return;
+
+ /* 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
{
- n = value_next (v);
- value_free (v);
+ 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 && reparse)
+ {
+ char *s;
+ if (b->exp)
+ {
+ xfree (b->exp);
+ b->exp = NULL;
+ }
+ s = b->exp_string;
+ b->exp = parse_exp_1 (&s, b->exp_valid_block, 0);
+ /* If the meaning of expression itself changed, the old value is
+ no longer relevant. We don't want to report a watchpoint hit
+ to the user when the old value and the new value may actually
+ be completely different objects. */
+ value_free (b->val);
+ b->val = NULL;
}
- b->owner->val_chain = NULL;
+
+
+ /* If we failed to parse the expression, for example because
+ it refers to a global variable in a not-yet-loaded shared library,
+ don't try to insert watchpoint. We don't automatically delete
+ such watchpoint, though, since failure to parse expression
+ is different from out-of-scope watchpoint. */
+ if (within_current_scope && b->exp)
+ {
+ struct value *v, *next;
+
+ /* Evaluate the expression and make sure it's not lazy, so that
+ after target stops again, we have a non-lazy previous value
+ to compare with. Also, making the value non-lazy will fetch
+ intermediate values as needed, which we use to decide which
+ addresses to watch.
+
+ The value returned by evaluate_expression is stored in b->val.
+ In addition, we look at all values which were created
+ during evaluation, and set watchoints at addresses as needed.
+ Those values are explicitly deleted here. */
+ v = evaluate_expression (b->exp);
+ /* Avoid setting b->val if it's already set. The meaning of
+ b->val is 'the last value' user saw, and we should update
+ it only if we reported that last value to user. As it
+ happens, the code that reports it updates b->val directly. */
+ if (b->val == NULL)
+ b->val = v;
+ value_contents (v);
+ value_release_to_mark (mark);
+
+ /* Look at each value on the value chain. */
+ for (; v; v = next)
+ {
+ /* 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 == b->val
+ || (TYPE_CODE (vtype) != TYPE_CODE_STRUCT
+ && TYPE_CODE (vtype) != TYPE_CODE_ARRAY))
+ {
+ CORE_ADDR addr;
+ int len, type;
+ struct bp_location *loc, **tmp;
+
+ 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;
+ }
+ }
+
+ next = value_next (v);
+ if (v != b->val)
+ value_free (v);
+ }
+
+ if (reparse && 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 if (!within_current_scope)
+ {
+ 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;
+ }
+
+ /* 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.
@@ -1016,136 +1183,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 (bpt->owner->type == bp_catch_fork
@@ -1178,6 +1219,7 @@ in which its expression is valid.\n"),
void
insert_breakpoints (void)
{
+ struct breakpoint *bpt;
struct bp_location *b, *temp;
int error = 0;
int val = 0;
@@ -1192,6 +1234,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 (bpt, 0 /* don't reparse */);
+
ALL_BP_LOCATIONS_SAFE (b, temp)
{
if (!breakpoint_enabled (b->owner))
@@ -1203,19 +1249,6 @@ insert_breakpoints (void)
&& !valid_thread_id (b->owner->thread))
continue;
- /* FIXME drow/2003-10-07: This code should be pushed elsewhere when
- hardware watchpoints are split into multiple loc breakpoints. */
- if ((b->loc_type == bp_loc_hardware_watchpoint
- || b->owner->type == bp_watchpoint) && !b->owner->val)
- {
- struct value *val;
- val = evaluate_expression (b->owner->exp);
- release_value (val);
- if (value_lazy (val))
- value_fetch_lazy (val);
- b->owner->val = val;
- }
-
val = insert_bp_location (b, tmp_error_stream,
&disabled_breaks, &process_warning,
&hw_breakpoint_error);
@@ -1223,6 +1256,39 @@ insert_breakpoints (void)
error = 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);
+ error = -1;
+ }
+ }
+
if (error)
{
/* If a hardware breakpoint or watchpoint was inserted, add a
@@ -1508,46 +1574,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;
+ val = target_remove_watchpoint (b->address, b->length,
+ b->watchpoint_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 (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."),
@@ -2451,33 +2486,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;
@@ -2716,6 +2737,15 @@ bpstat_stop_status (CORE_ADDR bp_addr, ptid_t ptid)
&& !inferior_has_execd (PIDGET (inferior_ptid), &b->exec_pathname))
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 */
@@ -2909,6 +2939,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;
@@ -3629,10 +3663,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;
@@ -6829,9 +6867,7 @@ delete_breakpoint (struct breakpoint *bpt)
{
if (loc->inserted)
remove_breakpoint (loc, mark_inserted);
-
- free_valchain (loc);
-
+
if (loc->cond)
xfree (loc->cond);
@@ -7265,39 +7301,7 @@ breakpoint_re_set_one (void *bint)
Don't do anything about disabled watchpoints, since they will
be reevaluated again when enabled. */
-
- if (!breakpoint_enabled (b))
- break;
-
- if (b->exp_valid_block == NULL
- || frame_find_by_id (b->watchpoint_frame) != NULL)
- {
- if (b->exp)
- {
- xfree (b->exp);
- b->exp = NULL;
- }
- s = b->exp_string;
- b->exp = parse_exp_1 (&s, b->exp_valid_block, 0);
-
- /* Since we reparsed expression, we need to update the
- value. I'm not aware of any way a single solib load or unload
- can change a valid value into different valid value, but:
- - even if the value is no longer valid, we have to record
- this fact, so that when it becomes valid we reports this
- as value change
- - unloaded followed by load can change the value for sure.
-
- We set value to NULL, and insert_breakpoints will
- update the value. */
- if (b->val)
- value_free (b->val);
- b->val = NULL;
-
- /* Loading of new shared library change the meaning of
- watchpoint condition. However, insert_bp_location will
- recompute watchpoint condition anyway, nothing to do here. */
- }
+ update_watchpoint (b, 1 /* reparse */);
break;
/* We needn't really do anything to reset these, since the mask
that requests them is unaffected by e.g., new libraries being
diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
index 91667ab..7b72e19 100644
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -273,6 +273,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;
@@ -388,9 +394,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
diff --git a/gdb/testsuite/gdb.base/watchpoint-solib-shr.c b/gdb/testsuite/gdb.base/watchpoint-solib-shr.c
new file mode 100644
index 0000000..699f559
--- /dev/null
+++ b/gdb/testsuite/gdb.base/watchpoint-solib-shr.c
@@ -0,0 +1,25 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+ Copyright 2004, 2007 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 g = 0;
+
+void foo (int i)
+{
+ g = i;
+}
diff --git a/gdb/testsuite/gdb.base/watchpoint-solib.c b/gdb/testsuite/gdb.base/watchpoint-solib.c
new file mode 100644
index 0000000..b316024
--- /dev/null
+++ b/gdb/testsuite/gdb.base/watchpoint-solib.c
@@ -0,0 +1,68 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+ Copyright 2004, 2007 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 <stdlib.h>
+
+#ifdef __WIN32__
+#include <windows.h>
+#define dlopen(name, mode) LoadLibrary (TEXT (name))
+#ifdef _WIN32_WCE
+# define dlsym(handle, func) GetProcAddress (handle, TEXT (func))
+#else
+# define dlsym(handle, func) GetProcAddress (handle, func)
+#endif
+#define dlclose(handle) FreeLibrary (handle)
+#define dlerror() "error %d occurred", GetLastError ()
+#else
+#include <dlfcn.h>
+#endif
+
+
+void open_shlib ()
+{
+ void *handle;
+ void (*foo) (int);
+
+ handle = dlopen (SHLIB_NAME, RTLD_LAZY);
+
+ if (!handle)
+ {
+ fprintf (stderr, dlerror ());
+ exit (1);
+ }
+
+ foo = (void (*)(int))dlsym (handle, "foo");
+
+ if (!foo)
+ {
+ fprintf (stderr, dlerror ());
+ exit (1);
+ }
+
+ foo (1); // call to foo
+ foo (2);
+
+ dlclose (handle);
+}
+
+
+int main()
+{
+ open_shlib ();
+ return 0;
+}
diff --git a/gdb/testsuite/gdb.base/watchpoint-solib.exp b/gdb/testsuite/gdb.base/watchpoint-solib.exp
new file mode 100644
index 0000000..fb5d886
--- /dev/null
+++ b/gdb/testsuite/gdb.base/watchpoint-solib.exp
@@ -0,0 +1,89 @@
+# Copyright 2007 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 $tracelevel then {
+ strace $tracelevel
+}
+
+#
+# test running programs
+#
+set prms_id 0
+set bug_id 0
+
+if {[skip_shlib_tests]} {
+ return 0
+}
+
+# TODO: Use LoadLibrary on this target instead of dlopen.
+if {[istarget arm*-*-symbianelf*]} {
+ return 0
+}
+
+set testfile "watchpoint-solib"
+set libfile "watchpoint-solib-shr"
+set libname "${libfile}.sl"
+set libsrcfile ${libfile}.c
+set srcfile $srcdir/$subdir/$testfile.c
+set binfile $objdir/$subdir/$testfile
+set shlibdir ${objdir}/${subdir}
+set libsrc $srcdir/$subdir/$libfile.c
+set lib_sl $objdir/$subdir/$libname
+
+if [get_compiler_info ${binfile}] {
+ return -1
+}
+
+set lib_opts debug
+set exec_opts [list debug shlib_load additional_flags=-DSHLIB_NAME\=\"${libname}\"]
+
+if { [gdb_compile_shlib $libsrc $lib_sl $lib_opts] != ""
+ || [gdb_compile $srcfile $binfile executable $exec_opts] != ""} {
+ untested "Couldn't compile $libsrc or $srcfile."
+ return -1
+}
+
+# Start with a fresh gdb.
+
+gdb_exit
+gdb_start
+gdb_reinitialize_dir $srcdir/$subdir
+gdb_load ${binfile}
+gdb_load_shlibs $lib_sl
+
+if [target_info exists gdb_stub] {
+ gdb_step_for_stub;
+}
+
+runto_main
+
+
+gdb_test_multiple "break foo" "set pending breakpoint" {
+ -re ".*Make breakpoint pending.*y or \\\[n\\\]. $" {
+ gdb_test "y" "Breakpoint.*foo.*pending." "set pending breakpoint"
+ }
+}
+
+gdb_test "continue" ".*Breakpoint 2.*foo.*" "continue to foo"
+gdb_test "watch g" "Hardware watchpoint 3: g" "set watchpoint on g"
+gdb_test "continue" ".*New value = 1.*" "continue to watchpoint hit"
+rerun_to_main
+gdb_test "continue" ".*Breakpoint 2.*foo.*" "continue to foo again"
+gdb_test "continue" ".*New value = 1.*" "continue to watchpoint hit again"
+
+
+
+
+
--
1.5.3.5