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

Jeff Johnston
2004-06-14T21:40:22Z

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.