Re: [RFA] Use multiple locations for hardware watchpoints.

[Daniel Jacobowitz <drow at false dot org>]

On Tue, Nov 20, 2007 at 08:32:26PM +0300, Vladimir Prus wrote:
> In order to implement watchpoint for a given expression, GDB
> might need several hardware watchpoints. Presently, that list
> of values to watch for is keep in val_chain field of struct
> breakpoint. There are at least three places that walk over
> this list, decide if this value should actually be watched
> for, compute address and length, and actually insert, remove,
> or check watchpoint. This patch changes that to store locations
> inside breakpoint->location.

Thank you for doing this!

> +static struct bp_location *
> +allocate_bp_location (struct breakpoint *bpt, enum bptype bp_type);
> +
> +static void
> +unlink_locations_from_global_list (struct breakpoint *bpt);
> +
> +static int
> +is_hardware_watchpoint (struct breakpoint *bpt);

Formatting - the function name only goes in the first column at the
definition.

> @@ -2864,6 +2853,15 @@ bpstat_stop_status (CORE_ADDR bp_addr, ptid_t ptid)
>  	!(current_exception_event = target_get_current_exception_event ()))
>        continue;
>  
> +    /* For hardware watchpoints, we look only at the first location.
> +       The watchpoint_check function will work on entire expression,
> +       not the individual locations.  For read watchopints, the
> +       watchpoints_triggered function have checked all locations
> +       alrea
> +     */

Lost the end of this comment :-)

> @@ -3059,6 +3057,10 @@ bpstat_stop_status (CORE_ADDR bp_addr, ptid_t ptid)
>  	      || bs->breakpoint_at->owner->type == bp_read_watchpoint
>  	      || bs->breakpoint_at->owner->type == bp_access_watchpoint))
>  	{
> +	  /* remove/insert can invalidate bs->breakpoint_at, if this
> +	     location is no longer used by the watchpoint.  Prevent
> +	     further code from trying to use it.  */
> +	  bs->breakpoint_at = NULL;
>  	  remove_breakpoints ();
>  	  insert_breakpoints ();
>  	  break;

This might break things like auto-delete watchpoints (which don't
exist, but make sense); I can't see anything that does exist that
would break, though, so it should be OK.

> +	 Note that while hardware watchpoints have
> +	 several locations internally, that's no a property
> +	 exposed to user.  */

"not" instead of "no" please.

> -	  && !ui_out_is_mi_like_p (uiout))
> +	  && !ui_out_is_mi_like_p (uiout)) 

Looks like you added a trailing space here.

> -      free_valchain (loc);
> -
> +      
>        if (loc->cond)

Here too.

> +  /* For hardware watchpoints, the size of data ad ADDRESS being watches.  */
> +  int length;

Typo.

> +  /* Type of hardware watchpoint. */

Two spaces.

Otherwise, this looks OK.

-- 
Daniel Jacobowitz
CodeSourcery