Re: read watchpoints don't work on targets that support read watchpoints

[Pedro Alves <pedro@codesourcery.com>]

On Monday 22 February 2010 19:24:16, Eli Zaretskii wrote:
> > From: Pedro Alves <pedro@codesourcery.com>
> > +      /* If trying to set a read-watchpoint, and it turns out it's not
> > +	 supported, try emulating one with an access watchpoint.  */
> > +      if (val == 1 && bpt->watchpoint_type == hw_read)
> > +	{
> > +	  struct bp_location *loc, **loc_temp;
> > +
> > +	  /* But don't try to insert it, if there's already another
> > +	     hw_access location that would be considered a duplicate
> > +	     of this one.  */
> 
> What does the last comment above mean for the use-case where I set a
> read watchpoint and an access watchpoint at the same address (but with
> different conditions)?  I probably am missing something, because the
> above seems to say this use will be impossible?

Note that this is about locations, not high-level user
breakpoints.  E.g.,  two high-level (user) access watchpoints
watching the same memory, can share the same watchpoint location
on the target, even if the condition is different.

E.g.:

 watch global if global == 3
 watch global if global == 4

A simple low-level watchpoint is inserted on the target.

This code is merely applying the same duplicates detection logic
as the last loop of update_global_location_list, but this
is reached after update_global_location_list has been called,
so since we're changing the location's
type (hw_read -> hw_access) very late (at insertion time),
we rescan duplicates here.  (with target accelarated watchpoint
conditions, the duplicates detection will need to be updated to
only consider duplicates locations that have the same condition
expression, but, that change must be done in all places that
do duplicate detection, not just here.)

> > +		  if (bl->watchpoint_type == hw_read)
> > +		    {
> > +		      CORE_ADDR addr;
> > +		      int res;
> > +
> > +		      /* A real read watchpoint.  Check if we're also
> > +			 trapping the same memory for writes.  */
> > +
> > +		      res = target_stopped_data_address (&current_target,
> > +							 &addr);
> > +		      /* We can only find a read watchpoint triggering
> > +			 if we know the address that trapped.  We
> > +			 shouldn't get here otherwise.  */
> > +		      gdb_assert (res);
> > +
> > +		      ALL_BREAKPOINTS (b)
> > +			if (breakpoint_enabled (b)
> > +			    && (b->type == bp_hardware_watchpoint
> > +				|| b->type == bp_access_watchpoint))
> 
> Why do you need to scan ALL_BREAKPOINTS HERE?  Can't you scan only the
> list returned by watchpoints_triggered?

Hmm, watchpoints_triggered doesn't return a list that would
save us iterating over all breakpoints, it does exactly the same
loop and check as below (in fact, I just copied it from there,
comment and all), and stores the result in
b->watchpoint_triggered (as watch_triggered_yes).  So, I guess
I could indeed iterate over all breakpoints as:

		  if (bl->watchpoint_type == hw_read)
		    {
		      struct breakpoint *other_b;

		       ALL_BREAKPOINTS (other_b)
			if ((other_b->type == bp_hardware_watchpoint
			     || other_b->type == bp_access_watchpoint)
			    && (other_b->watchpoint_triggered
				== watch_triggered_yes))
			  {
			    other_write_watchpoint = 1;
			    break;
			  }
		    }

Should be a bit more efficient, and simpler, yes.  Thanks.  I've done
this change, and checked the new test still passes OK on my hacked
PPC gdb.

> 
> > +			    /* Exact match not required.  Within range
> > +			       is sufficient.  */
> > +			    for (loc = b->loc; loc; loc = loc->next)
> > +			      if (target_watchpoint_addr_within_range
> > +				  (&current_target,
> > +				   addr,
> > +				   loc->address,
> > +				   loc->length))
> 
> And why are we checking watchpoints that couldn't have triggered,
> because they watch a different, albeit close, address?  What would be
> the use-case where such a watchpoint could be relevant to deciding
> which watchpoint to announce?

I'm not sure I understand this question.  Does the confusion arise from
the comment quoted above?  This is the exact same check
watchpoints_triggered does.  ADDR is checked for being in
the [loc->address, loc->address+loc->length) range.  If it is, then
watchpoint B is watching ADDR. 

Does this answer your questions?

-- 
Pedro Alves