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

[Michael Snyder <msnyder at redhat dot com>]

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)