Re: [RFC]: x86 threaded watchpoint support [2/3]

[Jeff Johnston <jjohnstn at redhat dot com>]

Eli Zaretskii wrote:
> > Date: Fri, 11 Jun 2004 17:33:34 -0400
> > From: Jeff Johnston <jjohnstn@redhat.com>
> >
> > The most major change is that a check has been added for a
> > hardware_watchpoint to ensure that the stopped data address matches
> > the watchpoint address.
>
>
> I don't necessarily object to this change, but could you first explain
> why is this needed, while it was never needed before?  (I have my
> guess for the answer, but I'd like to hear yours.)

I have since found out that certain platforms do not actually know the stopped 
data address.  Thus, the original code was needed to support such platforms.  as 
you had to figure out which values changed manually.  This always applies to 
software watchpoints.

The change is needed because with the threading model, you can have multiple 
events to process.  So, if you check your watchpoint values, all of them may 
have changed but you end up reporting an invalid thread location.  For example, 
I was getting watchpoints changing at the same time of a new thread event (it 
couldn't discern).  The reported location was __nptl_create_event (not very 
useful).  An end-user would have to do a info thread to find out the location 
for each thread and determine where the real culprit was.  If you have a lot of 
threads, you can see this being very frustrating.

In the case of x86, we do know the stopped data address so we shouldn't bother 
looking at the changed value of a hardware watchpoint that doesn't match.  With 
the fix, it is possible to see the proper location of where a single watchpoint 
location was modified.  Very useful.

> I have also a minor comment about the change itself, see below.
>
>
> > @@ -2683,45 +2688,100 @@ bpstat_stop_status (CORE_ADDR bp_addr, p
> >     if (b->type == bp_watchpoint ||
> > 	b->type == bp_hardware_watchpoint)
> >       {
> > -	char *message = xstrprintf ("Error evaluating expression for watchpoint %d\n",
> > +	CORE_ADDR addr;
> > +	struct value *v;
> > +	int found = 0;
> > +
> > +	/* If we have a hardware watchpoint, ensure that the address
> > +	   being watched caused the trap event.  */
> > +	if (b->type == bp_hardware_watchpoint)
> > +	  {
> > +	    addr = target_stopped_data_address ();
> > +	    if (addr == 0)
> > +	      {
> > +	        /* Don't stop.  */
> > +	        bs->print_it = print_it_noop;
> > +	        bs->stop = 0;
> > +	        continue;
> > +	      }
> > +	    for (v = b->val_chain; v; v = v->next)
>
>
> It looks to me that this change makes the bp_hardware_watchpoint case
> exactly identical to bp_read_watchpoint and bp_access_watchpoint, is
> that right?  If so, why not add bp_hardware_watchpoint to the if
> clause that handles read and access watchpoints, and leave only
> bp_watchpoint alone with the current code?

I definitely could have combined the code more efficiently for the two if 
clauses after making my changes.

I have talked to Andrew and he suggests that the target_stopped_data_address 
code should be modified to add a status code instead of the current 0 addr 
return value.  The address of 0 could be a valid watchpoint or could indicate no 
watchpoint triggered or failure or the lower level code "doesn't know" so the 
current interface isn't robust enough. I will be performing another version of 
the change to add this and that should handle platforms such as S390 but allow 
platforms like the x86 to work properly.  I will also combine the two if clauses 
better this time :)

-- Jeff J.