This is the mail archive of the
gdb-patches@sourceware.cygnus.com
mailing list for the GDB project.
Re: Hardware breakpoints and watchpoints: patches
- To: msnyder at cygnus dot com
- Subject: Re: Hardware breakpoints and watchpoints: patches
- From: Eli Zaretskii <eliz at gnu dot org>
- Date: Sat, 6 Nov 1999 10:44:40 -0500
- CC: gdb-patches at sourceware dot cygnus dot com
One of the changes from the patch I sent back in August, and which was
accepted (see below), apparently didn't make it into the current CVS
sources. The first hunk of the changes removed the "break;" statement
which was breaking out of the loop as soon as one watchpoint needed to
watch a particular value chain failed to insert. This can cause
trouble because the code then removes ALL the watchpoints for that
chain. Since part of the watchpoints were never inserted, some
implementations of watchpoints might fail or become confused.
Here's a suggested change:
--- gdb/breakpoi.c~ Wed Nov 3 07:51:18 1999
+++ gdb/breakpoi.c Sat Nov 6 13:32:40 1999
@@ -987,8 +987,12 @@ insert_breakpoints ()
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;
- break;
}
val = 0;
}
> From msnyder@cygnus.comSun Aug 22 11:38:32 1999
> Date: Thu, 19 Aug 1999 12:49:29 -0700
> From: Michael Snyder <msnyder@cygnus.com>
> Cc: gdb-patches@sourceware.cygnus.com
>
> 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.