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

[Daniel Jacobowitz <drow at mvista dot com>]

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