[RFA/breakpoint] Fix errors from disabled watchpoints

[RFA/breakpoint] Fix errors from disabled watchpoints

[Daniel Jacobowitz]

Right now, if you set and then disable a watchpoint, you'll still memory
errors from it in two places.  One is fatal, and comes from
insert_breakpoints (); the other is just noisy, and comes from
breakpoint_re_set_one ().  Neither really serves any purpose.  If a
watchpoint is disabled, we don't need to check what its value is; we'll
check when we insert it.

It would be nice to do the equivalet of a bp_shlib_disabled for watchpoints
on memory that isn't currently accessible but that's not really practical on
any OS I know of, so the user still has to hand-disable and hand-enable the
watchpoints.  But at least they don't have to _delete_ the watchpoints now.

Is this OK?  No surprises in the testsuite on i386-linux.

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

2002-12-28  Daniel Jacobowitz  <drow@mvista.com>

	* breakpoint.c (insert_breakpoints): Skip disabled breakpoints
	entirely.
	(breakpoint_re_set_one): Don't fetch the value for a disabled
	watchpoint.

Index: breakpoint.c
===================================================================
RCS file: /cvs/src/src/gdb/breakpoint.c,v
retrieving revision 1.104
diff -u -p -r1.104 breakpoint.c
--- breakpoint.c	17 Dec 2002 17:27:44 -0000	1.104
+++ breakpoint.c	28 Dec 2002 17:59:57 -0000
@@ -735,9 +735,11 @@ insert_breakpoints (void)
 
   ALL_BREAKPOINTS_SAFE (b, temp)
   {
-    if (b->enable_state == bp_permanent)
-      /* Permanent breakpoints cannot be inserted or removed.  */
+    /* Permanent breakpoints cannot be inserted or removed.  Disabled
+       breakpoints should not be inserted.  */
+    if (b->enable_state != bp_enabled)
       continue;
+
     if ((b->type == bp_watchpoint
 	 || b->type == bp_hardware_watchpoint
 	 || b->type == bp_read_watchpoint
@@ -759,9 +761,6 @@ insert_breakpoints (void)
 	&& b->type != bp_catch_exec
 	&& b->type != bp_catch_throw
 	&& b->type != bp_catch_catch
-	&& b->enable_state != bp_disabled
-	&& b->enable_state != bp_shlib_disabled
-	&& b->enable_state != bp_call_disabled
 	&& !b->inserted
 	&& !b->duplicate)
       {
@@ -880,9 +879,6 @@ insert_breakpoints (void)
 	  return_val = val;	/* remember failure */
       }
     else if (ep_is_exception_catchpoint (b)
-	     && b->enable_state != bp_disabled
-	     && b->enable_state != bp_shlib_disabled
-	     && b->enable_state != bp_call_disabled
 	     && !b->inserted
 	     && !b->duplicate)
 
@@ -940,7 +936,6 @@ insert_breakpoints (void)
     else if ((b->type == bp_hardware_watchpoint ||
 	      b->type == bp_read_watchpoint ||
 	      b->type == bp_access_watchpoint)
-	     && b->enable_state == bp_enabled
 	     && b->disposition != disp_del_at_next_stop
 	     && !b->inserted
 	     && !b->duplicate)
@@ -1059,7 +1054,6 @@ insert_breakpoints (void)
     else if ((b->type == bp_catch_fork
 	      || b->type == bp_catch_vfork
 	      || b->type == bp_catch_exec)
-	     && b->enable_state == bp_enabled
 	     && !b->inserted
 	     && !b->duplicate)
       {
@@ -7049,7 +7043,7 @@ breakpoint_re_set_one (PTR bint)
 	value_free (b->val);
       b->val = evaluate_expression (b->exp);
       release_value (b->val);
-      if (VALUE_LAZY (b->val))
+      if (VALUE_LAZY (b->val) && b->enable_state == bp_enabled)
 	value_fetch_lazy (b->val);
 
       if (b->cond_string != NULL)

Re: [RFA/breakpoint] Fix errors from disabled watchpoints

[Michael Snyder]

Daniel Jacobowitz wrote:
> 
> Right now, if you set and then disable a watchpoint, you'll still memory
> errors from it in two places.  One is fatal, and comes from
> insert_breakpoints (); the other is just noisy, and comes from
> breakpoint_re_set_one ().  Neither really serves any purpose.  If a
> watchpoint is disabled, we don't need to check what its value is; we'll
> check when we insert it.
> 
> It would be nice to do the equivalet of a bp_shlib_disabled for watchpoints
> on memory that isn't currently accessible but that's not really practical on
> any OS I know of, so the user still has to hand-disable and hand-enable the
> watchpoints.  But at least they don't have to _delete_ the watchpoints now.
> 
> Is this OK?  No surprises in the testsuite on i386-linux.

I'm not surprised that watchpoints were broken in this way,
but after looking at your changes, I am surprised that the 
problem didn't show up in any other context.

My only concern with your change is that you've changed 
the code from explicitly listing the excluded states, to
assuming that they are all excluded except for one.  The
problem that concerns me with that is, what if future states
are added?  

> 
> --
> Daniel Jacobowitz
> MontaVista Software                         Debian GNU/Linux Developer
> 
> 2002-12-28  Daniel Jacobowitz  <drow@mvista.com>
> 
>         * breakpoint.c (insert_breakpoints): Skip disabled breakpoints
>         entirely.
>         (breakpoint_re_set_one): Don't fetch the value for a disabled
>         watchpoint.
> 
> Index: breakpoint.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/breakpoint.c,v
> retrieving revision 1.104
> diff -u -p -r1.104 breakpoint.c
> --- breakpoint.c        17 Dec 2002 17:27:44 -0000      1.104
> +++ breakpoint.c        28 Dec 2002 17:59:57 -0000
> @@ -735,9 +735,11 @@ insert_breakpoints (void)
> 
>    ALL_BREAKPOINTS_SAFE (b, temp)
>    {
> -    if (b->enable_state == bp_permanent)
> -      /* Permanent breakpoints cannot be inserted or removed.  */
> +    /* Permanent breakpoints cannot be inserted or removed.  Disabled
> +       breakpoints should not be inserted.  */
> +    if (b->enable_state != bp_enabled)
>        continue;
> +
>      if ((b->type == bp_watchpoint
>          || b->type == bp_hardware_watchpoint
>          || b->type == bp_read_watchpoint
> @@ -759,9 +761,6 @@ insert_breakpoints (void)
>         && b->type != bp_catch_exec
>         && b->type != bp_catch_throw
>         && b->type != bp_catch_catch
> -       && b->enable_state != bp_disabled
> -       && b->enable_state != bp_shlib_disabled
> -       && b->enable_state != bp_call_disabled
>         && !b->inserted
>         && !b->duplicate)
>        {
> @@ -880,9 +879,6 @@ insert_breakpoints (void)
>           return_val = val;     /* remember failure */
>        }
>      else if (ep_is_exception_catchpoint (b)
> -            && b->enable_state != bp_disabled
> -            && b->enable_state != bp_shlib_disabled
> -            && b->enable_state != bp_call_disabled
>              && !b->inserted
>              && !b->duplicate)
> 
> @@ -940,7 +936,6 @@ insert_breakpoints (void)
>      else if ((b->type == bp_hardware_watchpoint ||
>               b->type == bp_read_watchpoint ||
>               b->type == bp_access_watchpoint)
> -            && b->enable_state == bp_enabled
>              && b->disposition != disp_del_at_next_stop
>              && !b->inserted
>              && !b->duplicate)
> @@ -1059,7 +1054,6 @@ insert_breakpoints (void)
>      else if ((b->type == bp_catch_fork
>               || b->type == bp_catch_vfork
>               || b->type == bp_catch_exec)
> -            && b->enable_state == bp_enabled
>              && !b->inserted
>              && !b->duplicate)
>        {
> @@ -7049,7 +7043,7 @@ breakpoint_re_set_one (PTR bint)
>         value_free (b->val);
>        b->val = evaluate_expression (b->exp);
>        release_value (b->val);
> -      if (VALUE_LAZY (b->val))
> +      if (VALUE_LAZY (b->val) && b->enable_state == bp_enabled)
>         value_fetch_lazy (b->val);
> 
>        if (b->cond_string != NULL)

Re: [RFA/breakpoint] Fix errors from disabled watchpoints

[Daniel Jacobowitz]

On Thu, Jan 02, 2003 at 12:08:30PM -0800, Michael Snyder wrote:
> Daniel Jacobowitz wrote:
> > 
> > Right now, if you set and then disable a watchpoint, you'll still memory
> > errors from it in two places.  One is fatal, and comes from
> > insert_breakpoints (); the other is just noisy, and comes from
> > breakpoint_re_set_one ().  Neither really serves any purpose.  If a
> > watchpoint is disabled, we don't need to check what its value is; we'll
> > check when we insert it.
> > 
> > It would be nice to do the equivalet of a bp_shlib_disabled for watchpoints
> > on memory that isn't currently accessible but that's not really practical on
> > any OS I know of, so the user still has to hand-disable and hand-enable the
> > watchpoints.  But at least they don't have to _delete_ the watchpoints now.
> > 
> > Is this OK?  No surprises in the testsuite on i386-linux.
> 
> I'm not surprised that watchpoints were broken in this way,
> but after looking at your changes, I am surprised that the 
> problem didn't show up in any other context.
> 
> My only concern with your change is that you've changed 
> the code from explicitly listing the excluded states, to
> assuming that they are all excluded except for one.  The
> problem that concerns me with that is, what if future states
> are added?  

We were already being pretty inconsistent about which we checked; see
how half the checks I deleted were inclusive and the other half were
exclusive?

If we start adding states I suspect we'll need BREAKPOINT_ENABLED
(bp->state), or something along those lines.

> 
> > 
> > --
> > Daniel Jacobowitz
> > MontaVista Software                         Debian GNU/Linux Developer
> > 
> > 2002-12-28  Daniel Jacobowitz  <drow@mvista.com>
> > 
> >         * breakpoint.c (insert_breakpoints): Skip disabled breakpoints
> >         entirely.
> >         (breakpoint_re_set_one): Don't fetch the value for a disabled
> >         watchpoint.
> > 
> > Index: breakpoint.c
> > ===================================================================
> > RCS file: /cvs/src/src/gdb/breakpoint.c,v
> > retrieving revision 1.104
> > diff -u -p -r1.104 breakpoint.c
> > --- breakpoint.c        17 Dec 2002 17:27:44 -0000      1.104
> > +++ breakpoint.c        28 Dec 2002 17:59:57 -0000
> > @@ -735,9 +735,11 @@ insert_breakpoints (void)
> > 
> >    ALL_BREAKPOINTS_SAFE (b, temp)
> >    {
> > -    if (b->enable_state == bp_permanent)
> > -      /* Permanent breakpoints cannot be inserted or removed.  */
> > +    /* Permanent breakpoints cannot be inserted or removed.  Disabled
> > +       breakpoints should not be inserted.  */
> > +    if (b->enable_state != bp_enabled)
> >        continue;
> > +
> >      if ((b->type == bp_watchpoint
> >          || b->type == bp_hardware_watchpoint
> >          || b->type == bp_read_watchpoint
> > @@ -759,9 +761,6 @@ insert_breakpoints (void)
> >         && b->type != bp_catch_exec
> >         && b->type != bp_catch_throw
> >         && b->type != bp_catch_catch
> > -       && b->enable_state != bp_disabled
> > -       && b->enable_state != bp_shlib_disabled
> > -       && b->enable_state != bp_call_disabled
> >         && !b->inserted
> >         && !b->duplicate)
> >        {
> > @@ -880,9 +879,6 @@ insert_breakpoints (void)
> >           return_val = val;     /* remember failure */
> >        }
> >      else if (ep_is_exception_catchpoint (b)
> > -            && b->enable_state != bp_disabled
> > -            && b->enable_state != bp_shlib_disabled
> > -            && b->enable_state != bp_call_disabled
> >              && !b->inserted
> >              && !b->duplicate)
> > 
> > @@ -940,7 +936,6 @@ insert_breakpoints (void)
> >      else if ((b->type == bp_hardware_watchpoint ||
> >               b->type == bp_read_watchpoint ||
> >               b->type == bp_access_watchpoint)
> > -            && b->enable_state == bp_enabled
> >              && b->disposition != disp_del_at_next_stop
> >              && !b->inserted
> >              && !b->duplicate)
> > @@ -1059,7 +1054,6 @@ insert_breakpoints (void)
> >      else if ((b->type == bp_catch_fork
> >               || b->type == bp_catch_vfork
> >               || b->type == bp_catch_exec)
> > -            && b->enable_state == bp_enabled
> >              && !b->inserted
> >              && !b->duplicate)
> >        {
> > @@ -7049,7 +7043,7 @@ breakpoint_re_set_one (PTR bint)
> >         value_free (b->val);
> >        b->val = evaluate_expression (b->exp);
> >        release_value (b->val);
> > -      if (VALUE_LAZY (b->val))
> > +      if (VALUE_LAZY (b->val) && b->enable_state == bp_enabled)
> >         value_fetch_lazy (b->val);
> > 
> >        if (b->cond_string != NULL)
> 

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

Re: [RFA/breakpoint] Fix errors from disabled watchpoints

[Michael Snyder]

Daniel Jacobowitz wrote:
> 
> On Thu, Jan 02, 2003 at 12:08:30PM -0800, Michael Snyder wrote:
> > Daniel Jacobowitz wrote:
> > >
> > > Right now, if you set and then disable a watchpoint, you'll still memory
> > > errors from it in two places.  One is fatal, and comes from
> > > insert_breakpoints (); the other is just noisy, and comes from
> > > breakpoint_re_set_one ().  Neither really serves any purpose.  If a
> > > watchpoint is disabled, we don't need to check what its value is; we'll
> > > check when we insert it.
> > >
> > > It would be nice to do the equivalet of a bp_shlib_disabled for watchpoints
> > > on memory that isn't currently accessible but that's not really practical on
> > > any OS I know of, so the user still has to hand-disable and hand-enable the
> > > watchpoints.  But at least they don't have to _delete_ the watchpoints now.
> > >
> > > Is this OK?  No surprises in the testsuite on i386-linux.
> >
> > I'm not surprised that watchpoints were broken in this way,
> > but after looking at your changes, I am surprised that the
> > problem didn't show up in any other context.
> >
> > My only concern with your change is that you've changed
> > the code from explicitly listing the excluded states, to
> > assuming that they are all excluded except for one.  The
> > problem that concerns me with that is, what if future states
> > are added?
> 
> We were already being pretty inconsistent about which we checked; see
> how half the checks I deleted were inclusive and the other half were
> exclusive?
> 
> If we start adding states I suspect we'll need BREAKPOINT_ENABLED
> (bp->state), or something along those lines.

Or BREAKPOINT_INSERTABLE, or something.  OK, I'll approve it.

Re: [RFA/breakpoint] Fix errors from disabled watchpoints

[Daniel Jacobowitz]

On Thu, Jan 02, 2003 at 03:13:31PM -0800, Michael Snyder wrote:
> Daniel Jacobowitz wrote:
> > 
> > On Thu, Jan 02, 2003 at 12:08:30PM -0800, Michael Snyder wrote:
> > > Daniel Jacobowitz wrote:
> > > >
> > > > Right now, if you set and then disable a watchpoint, you'll still memory
> > > > errors from it in two places.  One is fatal, and comes from
> > > > insert_breakpoints (); the other is just noisy, and comes from
> > > > breakpoint_re_set_one ().  Neither really serves any purpose.  If a
> > > > watchpoint is disabled, we don't need to check what its value is; we'll
> > > > check when we insert it.
> > > >
> > > > It would be nice to do the equivalet of a bp_shlib_disabled for watchpoints
> > > > on memory that isn't currently accessible but that's not really practical on
> > > > any OS I know of, so the user still has to hand-disable and hand-enable the
> > > > watchpoints.  But at least they don't have to _delete_ the watchpoints now.
> > > >
> > > > Is this OK?  No surprises in the testsuite on i386-linux.
> > >
> > > I'm not surprised that watchpoints were broken in this way,
> > > but after looking at your changes, I am surprised that the
> > > problem didn't show up in any other context.
> > >
> > > My only concern with your change is that you've changed
> > > the code from explicitly listing the excluded states, to
> > > assuming that they are all excluded except for one.  The
> > > problem that concerns me with that is, what if future states
> > > are added?
> > 
> > We were already being pretty inconsistent about which we checked; see
> > how half the checks I deleted were inclusive and the other half were
> > exclusive?
> > 
> > If we start adding states I suspect we'll need BREAKPOINT_ENABLED
> > (bp->state), or something along those lines.
> 
> Or BREAKPOINT_INSERTABLE, or something.  OK, I'll approve it.

Thanks, committed.

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer