Watchpoint resource accounting broken (Re: [5/6] breakpoints_ops for all kinds of breakpoints: new watchpoints instance type)

["Ulrich Weigand" <uweigand@de.ibm.com>]

Pedro Alves wrote:

> 	(watch_command_1): Allocate and initialize a struct watchpoint
> 	instead of a struct breakpoint.  Use install_breakpoint.

>    /* Now set up the breakpoint.  */
> +
> +  w = XCNEW (struct watchpoint);
> +  b = &w->base;
>    if (use_mask)
> -    b = set_raw_breakpoint_without_location (NULL, bp_type,
> -					     &masked_watchpoint_breakpoint_ops);
> +    init_raw_breakpoint_without_location (b, NULL, bp_type,
> +					  &masked_watchpoint_breakpoint_ops);
>    else
> -    b = set_raw_breakpoint_without_location (NULL, bp_type,
> -					     &watchpoint_breakpoint_ops);
> +    init_raw_breakpoint_without_location (b, NULL, bp_type,
> +					  &watchpoint_breakpoint_ops);
[snip]
> @@ -9153,7 +9247,7 @@ watch_command_1 (char *arg, int accessfl
>      {
>        /* Finally update the new watchpoint.  This creates the locations
>  	 that should be inserted.  */
> -      update_watchpoint (b, 1);
> +      update_watchpoint (w, 1);
>      }
>    if (e.reason < 0)
>      {
> @@ -9161,15 +9255,7 @@ watch_command_1 (char *arg, int accessfl
>        throw_exception (e);
>      }
>  
> -  set_breakpoint_number (internal, b);
> -
> -  /* Do not mention breakpoints with a negative number, but do
> -     notify observers.  */
> -  if (!internal)
> -    mention (b);
> -  observer_notify_breakpoint_created (b);
> -
> -  update_global_location_list (1);
> +  install_breakpoint (internal, b);
>  }

Unfortunately this change breaks watchpoint resource accounting.
Note that in the old version, the watchpoint had already been
added to the breakpoint list (by set_raw_breakpoint_without_location)
*before* the call to update_watchpoint, while in the new version
the watchpoint is added *after* that call (by install_breakpoint).

However, update_watchpoint relies on having the watchpoint under
investigation be on the breakpoint list; see the comment:

              /* We need to determine how many resources are already
                 used for all other hardware watchpoints plus this one
                 to see if we still have enough resources to also fit
                 this watchpoint in as well.  To guarantee the
                 hw_watchpoint_used_count call below counts this
                 watchpoint, make sure that it is marked as a hardware
                 watchpoint.  */
              if (b->base.type == bp_watchpoint)
                b->base.type = bp_hardware_watchpoint;

              i = hw_watchpoint_used_count (b->base.type, &other_type_used);
              target_resources_ok = target_can_use_hardware_watchpoint
                    (b->base.type, i, other_type_used);

Note how just "i", the result of hw_watchpoint_used_count, is passed to
target_can_use_hardware_watchpoint -- this works only if the current
watchpoint is on the list that hw_watchpoint_used_count iterates over.

(You cannot just consider the current watchpoint in addition to the
result of hw_watchpoint_used_count either, because for other callers
to update_watchpoint, the current watchpoint *is* on the list.)

I'm sure sure how best to fix this; maybe go back to adding the
watchpoint to the breakpoint list earlier?

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com