Hardware breakpoints and watchpoints: patches
Michael Snyder
msnyder@cygnus.com
Thu Aug 19 12:53:00 GMT 1999
Eli Zaretskii wrote:
>
> The following patches make watchpoints work much better for me. The
> original code would occasionally trigger incorrect watchpoints or
> leave watchpoints unremoved in the debug registers, which would then
> cause SIGTRAP and othere calamities.
OK, I'm going to address these changes individually.
> *** gdb/breakpoint.c~0 Fri Mar 26 06:23:50 1999
> --- gdb/breakpoint.c Sat Aug 14 17:52:58 1999
> *************** insert_breakpoints ()
> *** 796,803 ****
> val = target_insert_watchpoint (addr, len, type);
> if (val == -1)
> {
> b->inserted = 0;
> - break;
> }
> val = 0;
> }
> --- 796,807 ----
> 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. */
> b->inserted = 0;
> }
> val = 0;
> }
> *************** insert_breakpoints ()
> *** 805,812 ****
> /* Failure to insert a watchpoint on any memory value in the
> value chain brings us here. */
> if (!b->inserted)
> ! warning ("Hardware watchpoint %d: Could not insert watchpoint\n",
> ! b->number);
> }
> else
> {
> --- 809,825 ----
> /* Failure to insert a watchpoint on any memory value in the
> value chain brings us here. */
> if (!b->inserted)
> ! {
> ! warning ("\
> ! Hardware watchpoint %d: Could not insert watchpoint\n", b->number);
> ! /* This watchpoint couldn't be inserted, but some of
> ! the values on its value chain might have their
> ! watchpoints inserted. We must remove them,
> ! otherwise the resources they use will remain
> ! occupied. */
> ! remove_breakpoint (b, mark_uninserted);
> ! val = -1; /* to return failure indication */
> ! }
> }
> else
> {
This change is OK in principal, and does fix a bug (thank you).
Unfortunately, it exposes another bug: you can't just set val to
-1 and hope for that value to be returned, since you're inside
a loop in which val is frequently changed.
I'll rework this a bit and check it in.
> *************** bpstat_stop_status (pc, not_a_breakpoint
> *** 2124,2130 ****
> bs->print = 1;
>
> sprintf (message, message1, b->number);
> ! if (b->type == bp_watchpoint || b->type == bp_hardware_watchpoint)
> {
> switch (catch_errors (watchpoint_check, bs, message, RETURN_MASK_ALL))
> {
> --- 2137,2143 ----
> bs->print = 1;
>
> sprintf (message, message1, b->number);
> ! if (b->type == bp_watchpoint)
> {
> switch (catch_errors (watchpoint_check, bs, message, RETURN_MASK_ALL))
> {
This change is not OK.
You might think that it makes more sense to treat a
hardware watchpoint like a hardware read/access watchpoint,
rather than like a software watchpoint. But if I make this
change, then the old vs. new values are not reported.
Can't accept that. It does no harm to treat the
hardware watchpoint like the software watchpoint in this
context, since the code will only execute when and if the
hardware watchpoint "fires".
> *************** bpstat_stop_status (pc, not_a_breakpoint
> *** 2141,2148 ****
> /* Don't stop. */
> bs->print_it = print_it_noop;
> bs->stop = 0;
> - /* Don't consider this a hit. */
> - --(b->hit_count);
> continue;
> default:
> /* Can't happen. */
> --- 2154,2159 ----
Can you tell me why you want to make this change?
> *************** bpstat_stop_status (pc, not_a_breakpoint
> *** 2160,2173 ****
> break;
> }
> }
> ! else if (b->type == bp_read_watchpoint || b->type == bp_access_watchpoint)
> {
> CORE_ADDR addr;
> value_ptr v;
> int found = 0;
>
> addr = target_stopped_data_address();
> ! if (addr == 0) continue;
> for (v = b->val_chain; v; v = v->next)
> {
> if (v->lval == lval_memory)
> --- 2171,2192 ----
> break;
> }
> }
> ! else if (b->type == bp_read_watchpoint
> ! || b->type == bp_access_watchpoint
> ! || b->type == bp_hardware_watchpoint)
> {
> CORE_ADDR addr;
> value_ptr v;
> int found = 0;
>
> addr = target_stopped_data_address();
> ! if (addr == 0)
> ! {
> ! /* Don't stop. */
> ! bs->print_it = print_it_noop;
> ! bs->stop = 0;
> ! continue;
> ! }
> for (v = b->val_chain; v; v = v->next)
> {
> if (v->lval == lval_memory)
Regarding the change around "if (addr == 0)"
this will break targets that don't define
"target_stopped_data_address".
The downside without this change is that, when a read/access BP
fires, all read/access BP's will announce themselves.
The downside WITH the change (and without the macro defined),
is that NONE of them will announce, and indeed the target
won't stop.
Downside #2 is worse than downside #1.
> *************** bpstat_stop_status (pc, not_a_breakpoint
> *** 2175,2181 ****
> CORE_ADDR vaddr;
>
> vaddr = VALUE_ADDRESS (v) + VALUE_OFFSET (v);
> ! if (addr == vaddr)
> found = 1;
> }
> }
> --- 2194,2205 ----
> CORE_ADDR vaddr;
>
> vaddr = VALUE_ADDRESS (v) + VALUE_OFFSET (v);
> ! /* If VADDR is watched as a sequence of several
> ! addresses (e.g., with several debug registers),
> ! the address that triggered might be anywhere
> ! in the region that V defines. */
> ! if (addr >= vaddr
> ! && addr < vaddr + TYPE_LENGTH (VALUE_TYPE (v)))
> found = 1;
> }
> }
This is a good change and I will check it in.
> *************** bpstat_stop_status (pc, not_a_breakpoint
> *** 2188,2199 ****
> /* Stop. */
> break;
> case WP_VALUE_CHANGED:
> case WP_VALUE_NOT_CHANGED:
> ! /* Stop. */
> ++(b->hit_count);
> break;
> default:
> /* Can't happen. */
> case 0:
> /* Error from catch_errors. */
> printf_filtered ("Watchpoint %d deleted.\n", b->number);
> --- 2212,2251 ----
> /* Stop. */
> break;
> case WP_VALUE_CHANGED:
> + if (b->type == bp_read_watchpoint)
> + {
> + /* Don't stop: read watchpoints shouldn't fire if
> + the value has changed. This is for targets
> + which cannot set read-only watchpoints. */
> + bs->print_it = print_it_noop;
> + bs->stop = 0;
> + continue;
> + }
> + ++(b->hit_count);
> + break;
This is no good. If there is a write, followed by a read,
GDB will decide not to stop for the read. It is better to
get a false positive (unnecessary stop) than a false negative
(fail to stop when you should.)
> case WP_VALUE_NOT_CHANGED:
> ! /* Stop, but not if this is a write-only watchpoint.
> ! FIXME: this causes GDB to not report watchpoints
> ! if the memory was written with the same value.
> ! However, to correct this, we need maintain a
> ! one-to-one correspeondence between the watchpoint
> ! and the target resources used to implement it, and
> ! let the target tell us which watchpoint(s) fired.
> ! With the current design, if we don't check that the
> ! value has changed, we might erroneously say that a
> ! read watchpoint triggered if it watches the same
> ! address as some other write watchpoint. */
> ! if (b->type == bp_hardware_watchpoint)
> ! {
> ! bs->print_it = print_it_noop;
> ! bs->stop = 0;
> ! continue;
> ! }
Again, it is better to stop when you don't need to,
than to keep going when you should have stopped.
> ++(b->hit_count);
> break;
> default:
> /* Can't happen. */
> + /* FALLTHROUGH */
> case 0:
> /* Error from catch_errors. */
> printf_filtered ("Watchpoint %d deleted.\n", b->number);
> *************** bpstat_stop_status (pc, not_a_breakpoint
> *** 2204,2209 ****
> --- 2256,2271 ----
> bs->print_it = print_it_done;
> break;
> }
> + else
> + {
> + /* This is a case where some watchpoint(s) triggered,
> + but not at the address of this watchpoint (FOUND
> + was left zero). So don't print anything for this
> + watchpoint. */
> + bs->print_it = print_it_noop;
> + bs->stop = 0;
> + continue;
> + }
> }
> else
> {
This change is good, and I will check it in.
> *************** watch_command_1 (arg, accessflag, from_t
> *** 4393,4398 ****
> --- 4455,4465 ----
> ((byte_size) <= (REGISTER_SIZE))
> #endif
>
> + #if !defined(TARGET_REGION_OK_FOR_HW_WATCHPOINT)
> + #define TARGET_REGION_OK_FOR_HW_WATCHPOINT(addr,len) \
> + TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT(len)
> + #endif
> +
> static int
> can_use_hardware_watchpoint (v)
> struct value *v;
> *************** can_use_hardware_watchpoint (v)
> *** 4411,4418 ****
> {
> if (v->lval == lval_memory)
> {
> ! if (TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT (TYPE_LENGTH (VALUE_TYPE (v))))
> ! found_memory_cnt++;
> }
> else if (v->lval != not_lval && v->modifiable == 0)
> return 0;
> --- 4478,4489 ----
> {
> if (v->lval == lval_memory)
> {
> ! int vaddr = VALUE_ADDRESS (v) + VALUE_OFFSET (v);
> ! int len = TYPE_LENGTH (VALUE_TYPE (v));
> !
> ! if (!TARGET_REGION_OK_FOR_HW_WATCHPOINT (vaddr, len))
> ! return 0;
> ! found_memory_cnt++;
> }
> else if (v->lval != not_lval && v->modifiable == 0)
> return 0;
This is also good, I'll check it in.
> *************** delete_breakpoint (bpt)
> *** 5667,5673 ****
> && b->enable != call_disabled)
> {
> int val;
> ! val = target_insert_breakpoint (b->address, b->shadow_contents);
> if (val != 0)
> {
> target_terminal_ours_for_output ();
> --- 5738,5746 ----
> && b->enable != call_disabled)
> {
> int val;
> ! val = b->type == bp_hardware_breakpoint
> ! ? target_insert_hw_breakpoint (b->address, b->shadow_contents)
> ! : target_insert_breakpoint (b->address, b->shadow_contents);
> if (val != 0)
> {
> target_terminal_ours_for_output ();
This is good too -- however it makes me think:
Should we extract the inner part of the loop in "insert_breakpoints"
and make it a separate function, analogous to "delete_breakpoint"?
Then call that function here?
Want to volunteer to do it?
> *** gdb/target.h~0 Fri Mar 26 06:26:02 1999
> --- gdb/target.h Sat Aug 14 14:46:04 1999
> *************** extern char *normal_pid_to_str PARAMS ((
> *** 1046,1055 ****
>
> #ifndef TARGET_HAS_HARDWARE_WATCHPOINTS
>
> ! /* Returns non-zero if we can set a hardware watchpoint of type TYPE. TYPE is
> ! one of bp_hardware_watchpoint, bp_read_watchpoint, bp_write_watchpoint, or
> ! bp_hardware_breakpoint. CNT is the number of such watchpoints used so far
> ! (including this one?). OTHERTYPE is who knows what... */
>
> #define TARGET_CAN_USE_HARDWARE_WATCHPOINT(TYPE,CNT,OTHERTYPE) 0
>
> --- 1046,1061 ----
>
> #ifndef TARGET_HAS_HARDWARE_WATCHPOINTS
>
> ! /* Returns the number of hardware watchpoints of type TYPE that we can
> ! set. Value is positive if we can set CNT watchpoints, zero if
> ! setting watchpoints of type TYPE is not supported, and negative if
> ! CNT is more than the maximum number of watchpoints of type TYPE
> ! that we can support. TYPE is one of bp_hardware_watchpoint,
> ! bp_read_watchpoint, bp_write_watchpoint, or bp_hardware_breakpoint.
> ! CNT is the number of such watchpoints used so far (including this
> ! one). OTHERTYPE is non-zero if other types of watchpoints are
> ! currently enabled (this is for targets where all watchpoints need to
> ! be of the same type, for example). */
>
> #define TARGET_CAN_USE_HARDWARE_WATCHPOINT(TYPE,CNT,OTHERTYPE) 0
>
This is incorrect. The macro is boolean, it only returns zero
or non-zero. Well, one implementation returns 1, 0, or -1, but
that distinction isn't well-documented...
> *************** extern char *normal_pid_to_str PARAMS ((
> *** 1058,1063 ****
> --- 1064,1076 ----
> (LONGEST)(byte_count) <= REGISTER_SIZE
> #endif
>
> + /* Returns non-zero if we can use hardware watchpoints to watch a region
> + whose address is ADDR and whose length is LEN. */
> + #if !defined(TARGET_REGION_OK_FOR_HW_WATCHPOINT)
> + #define TARGET_REGION_OK_FOR_HW_WATCHPOINT(addr,len) \
> + TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT(len)
> + #endif
> +
> /* However, some addresses may not be profitable to use hardware to watch,
> or may be difficult to understand when the addressed object is out of
> scope, and hence should be unwatched. On some targets, this may have
This change is a duplicate.
You've already defined this macro once earlier.
I will send you a patch including the changes that I've accepted.
Michael
More information about the Gdb-patches
mailing list