Re: [RFA 3/3] Implement support for PowerPC BookE masked watchpoints

["Ulrich Weigand" <uweigand at de dot ibm dot com>]

Thiago Jung Bauermann wrote:

> > We've recently added the -location feature that already prevents
> > GDB from fully tracking the expression, but rather just evaluate
> > it one time for its address.   Maybe one simple fix could be to
> > restrict the "mask" feature to watch -location (or have the 
> > presence of "mask" automatically imply -location, or maybe have
> > instead a new keyword -masked-location or so).
> 
> -location is indeed a good fit for this patch. I don't think we need a
> separate -masked-location keyword, so in this patch I made the mask
> parameter imply -location. Do you think this addresses your concerns
> regarding the semantics of masked watchpoints?

OK, that's fine with me.  I guess that ought to mentioned in the
documentation somewhere ...

> > However, even so there are still differences: even with -location,
> > GDB watchpoint code will still treat a watchpoint as refering to
> > one value, and track it (e.g. remember the value we saw last time;
> > re-evaluate it each time execution stops etc.).  This is really
> > not appropriate for the masked watchpoint case, because we do not
> > *have* any contiguous region that would define any single value.
> > Therefore, it seems to me that all this code ought to be disabled
> > for masked watchpoints.
> 
> Indeed. With this new patch, the only place that evaluates the
> watchpoint expression is update_watchpoint. This is necessary because
> the resulting value chain is then used to create the bp_locations and
> also by can_use_hardware_watchpoint when deciding whether to set a
> software or hardware watchpoint.
> 
> I could manually create a bp_location for masked watchpoints and
> implement a version of can_use_hardware_watchpoint just for masked
> watchpoints, but is it worth the extra special casing and complexity in
> update_watchpoint?

No, this is good.  Evaluating the expression just for it's value's
location(s) is fine ...

> +	  {
> +	    CORE_ADDR newaddr, start;
> +
> +	    if (is_masked_watchpoint (loc->owner))
> +	      {
> +		newaddr = addr & loc->owner->hw_wp_mask;
> +		start = loc->address & loc->owner->hw_wp_mask;
> +	      }
> +	    else
> +	      {
> +		newaddr = addr;
> +		start = loc->address;
> +	      }
> +
> +	    /* Exact match not required.  Within range is
> +	       sufficient.  */
> +	    if (target_watchpoint_addr_within_range (&current_target,
> +						     newaddr, start,
> +						     loc->length))
> +	      {
> +		b->watchpoint_triggered = watch_triggered_yes;
> +		break;
> +	      }
> +	  }

Hmmm, so for masked watchpoints we implement the masking operation
directly here, and then also call the target callback (which may use
the length, and do some additional masking on powerpc)?  That seems
a bit odd to me ...

Shouldn't we either:
 - just compare masked addresses for equality, directly (which
   implements that stated semantics)
 - or else pass the mask into target_watchpoint_addr_within_range
   and allow the target to implement whatever matching is appropriate


> +static enum print_stop_action
> +print_it_masked_watchpoint (struct breakpoint *b)
> +{
> +  struct ui_stream *stb;
> +  struct cleanup *old_chain;
> +
> +  /* Masked watchpoints have only one location.  */
> +  gdb_assert (b->loc && b->loc->next == NULL);
> +
> +  stb = ui_out_stream_new (uiout);
> +  old_chain = make_cleanup_ui_out_stream_delete (stb);

These aren't used anywhere, are they?

> +ppc_linux_masked_watch_num_registers (struct target_ops *target,
> +				      CORE_ADDR addr, CORE_ADDR mask)
> +{
> +  if (!have_ptrace_booke_interface ()
> +	   || (booke_debug_info.features & PPC_DEBUG_FEATURE_DATA_BP_MASK) == 0)
> +    return -1;
> +  else if ((mask & 0xC0000000) != 0xC0000000)
> +    {
> +      warning (_("\
> +The given mask covers kernel address space and cannot be used.\n\
> +You have to delete the masked watchpoint to continue the debugging session."));

This second sentence shouldn't be neccessary now: the watchpoint with the
invalid mask will never be successfully created, right?

Otherwise this looks all good to me now.

Thanks,
Ulrich

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