Re: x86 watchpoints bug (Re: ping: Re: PATCH : allow to set length of hw watchpoints (e.g. for Valgrind gdbserver))

["Philippe Waroquiers" <philippe.waroquiers@skynet.be>]

Thanks for the feedback, behaviour looks to be better
when adding the missing assignment. But I suspect I found another
bug in the area of "high level" to "low level" to "hw level" watchpoints.

>> I suspect the problem might be in the following piece of code:
>> static void
>> update_inferior (struct i386_debug_reg_state *inf_state,
>>    struct i386_debug_reg_state *new_state)
>> {
>>   int i;
>> 
>>   ALL_DEBUG_REGISTERS (i)
>>     {
>>       if (new_state->dr_mirror[i] != inf_state->dr_mirror[i]
>>    || (new_state->dr_ref_count[i] != 0
>>        && inf_state->dr_ref_count[i] == 0))
>>  {
>> 
>> The dr_mirror is the address being watched.
>> But if address being watched is 0x0, then a 'busy' register
>> watching 0x0 and a non-busy register will have equal dr_mirror.
>> Then the || condition is bizarre as the ref.count will be updated
>> only if the current inf_state ref.count is 0.
> 
> Not the ref.count.  The address to watch, DR[0-3].

Without the  *inf_state = *new_state,
I had some difficulties to understand the above code.

From what I understand now, the idea of this piece of code
is (only) to change the real value of the hw register.
But if inf_state->dr_mirror properly mirrors the value of the hw
register, then the inequality of the dr_mirror[i] should
be good enough to detect the need to change the hw register.

And if setting the address is only to be done when activating
the watchpoint, then the inequality on the ref count should be
good enough (and the assert new_state->dr_ref_count[i] == 1
should hold when changing the hw addr value).

Well it seems I still have difficulty to understand the code :).

But even if I do not understand the code, after adding 
the *inf_state = *new_state, the behaviour looks better.

>> +# registers were available to cover a single (low level) watchpoint.
> watchpoint.  So the comment was correct if you think of high and
> low level watchpoints like I was thinking.  Maybe you were thinking
> of a high level watchpoint as what the target sees?

Yes, I was interpreting "high level" being a Z2 packet, and "low level"
being the "hw" watchpoint.
Now, I understand that we have:
   high level watchpoints = "user defined watchpoints in gdb" = watched expressions
   low level watchpoints = "memory region watchpoints needed to implement the high level" = Z2 packets
   hw level  watchpoints =  "hw watchpoint(s) needed to implement the low level watchpoint(s)"

Thanks for the clarification


Doing some additional checks, I found something else slightly strange, but it seems
to be wrong at the mapping between "high level watchpoints" and "low level watchpoints".

In the below, you see that 3 identical watchpoints results in 1 single Z2
packet, but disabling the 3 watchpoints gives 3 z2 packets (sent
when the last watchpoint is disabled).
The reason for this assymetry looks not very clear to me, but that might
just be an implementation detail.
I however suspect there is still a bug, as after, when sharing a debug register
between two non-identical watchpoints, we are losing a part of the to be watched
zone : we still have a user level watchpoint of 16 bytes at 0x0, but the hw registers
are only watching 8 bytes at 0x8.
(the below is done with a patched gdb/gdbserver containing the "dr busy" fix
+ the missing assignment + the "set length" patch to allow watching 16 bytes
with gdbserver).
Note that the bug seems also present in native debugging (see native gdb session at the end).



(gdb) tar rem :1234
Remote debugging using :1234
Reading symbols from /lib64/ld-linux-x86-64.so.2...(no debugging symbols found)...done.
Loaded symbols for /lib64/ld-linux-x86-64.so.2
0x00007ffff7de2a60 in ?? () from /lib64/ld-linux-x86-64.so.2
(gdb) set breakpoint always-inserted  on
(gdb) mo set debug-hw-points 1
H/W point debugging output enabled.
(gdb) watch *(long *) 0x0
Hardware watchpoint 1: *(long *) 0x0
(gdb) watch *(long *) 0x0
Hardware watchpoint 2: *(long *) 0x0
(gdb) watch *(long *) 0x0
Hardware watchpoint 3: *(long *) 0x0
(gdb) dis 1
(gdb) dis 2
(gdb) dis 3
(gdb) enabl
(gdb) watch * (char (*)[16]) 0x0
Hardware watchpoint 4: * (char (*)[16]) 0x0
(gdb) dis 1
(gdb) dis 2
(gdb) dis 3
(gdb) 


Listening on port 1234
Remote debugging from host 127.0.0.1
insert_watchpoint (addr=0, len=8, type=data-write):
 CONTROL (DR7): 00090101          STATUS (DR6): 00000000
 DR0: addr=0x0, ref.count=1  DR1: addr=0x0, ref.count=0
 DR2: addr=0x0, ref.count=0  DR3: addr=0x0, ref.count=0
remove_watchpoint (addr=0, len=8, type=data-write):
 CONTROL (DR7): 00090100          STATUS (DR6): 00000000
 DR0: addr=0x0, ref.count=0  DR1: addr=0x0, ref.count=0
 DR2: addr=0x0, ref.count=0  DR3: addr=0x0, ref.count=0
remove_watchpoint (addr=0, len=8, type=data-write):
 CONTROL (DR7): 00090100          STATUS (DR6): 00000000
 DR0: addr=0x0, ref.count=0  DR1: addr=0x0, ref.count=0
 DR2: addr=0x0, ref.count=0  DR3: addr=0x0, ref.count=0
remove_watchpoint (addr=0, len=8, type=data-write):
 CONTROL (DR7): 00090100          STATUS (DR6): 00000000
 DR0: addr=0x0, ref.count=0  DR1: addr=0x0, ref.count=0
 DR2: addr=0x0, ref.count=0  DR3: addr=0x0, ref.count=0
insert_watchpoint (addr=0, len=8, type=data-write):
 CONTROL (DR7): 00090101          STATUS (DR6): 00000000
 DR0: addr=0x0, ref.count=1  DR1: addr=0x0, ref.count=0
 DR2: addr=0x0, ref.count=0  DR3: addr=0x0, ref.count=0
insert_watchpoint (addr=0, len=16, type=data-write):
 CONTROL (DR7): 00990105          STATUS (DR6): 00000000
 DR0: addr=0x0, ref.count=2  DR1: addr=0x8, ref.count=1
 DR2: addr=0x0, ref.count=0  DR3: addr=0x0, ref.count=0
remove_watchpoint (addr=0, len=8, type=data-write):
 CONTROL (DR7): 00990105          STATUS (DR6): 00000000
 DR0: addr=0x0, ref.count=1  DR1: addr=0x8, ref.count=1
 DR2: addr=0x0, ref.count=0  DR3: addr=0x0, ref.count=0
remove_watchpoint (addr=0, len=8, type=data-write):
 CONTROL (DR7): 00990104          STATUS (DR6): 00000000
 DR0: addr=0x0, ref.count=0  DR1: addr=0x8, ref.count=1
 DR2: addr=0x0, ref.count=0  DR3: addr=0x0, ref.count=0
remove_watchpoint (addr=0, len=8, type=data-write):
 CONTROL (DR7): 00990104          STATUS (DR6): 00000000
 DR0: addr=0x0, ref.count=0  DR1: addr=0x8, ref.count=1
 DR2: addr=0x0, ref.count=0  DR3: addr=0x0, ref.count=0



(gdb) watch * (char (*)[16]) 0x0
Hardware watchpoint 6: * (char (*)[16]) 0x0
insert_watchpoint (addr=0, len=16, type=data-write):
 CONTROL (DR7): 0000000000990105          STATUS (DR6): 00000000ffff4ff0
 DR0: addr=0x0000000000000000, ref.count=2  DR1: addr=0x0000000000000008, ref.count=1
 DR2: addr=0x0000000000000000, ref.count=0  DR3: addr=0x0000000000000000, ref.count=0
(gdb) info wat
Num     Type           Disp Enb Address            What
1       hw watchpoint  keep y                      *(long *) 0x0
3       hw watchpoint  keep y                      *(long *) 0x0
4       hw watchpoint  keep y                      *(long *) 0x0
6       hw watchpoint  keep y                      * (char (*)[16]) 0x0
(gdb) dis 1
(gdb) dis 3
(gdb) dis 4
remove_watchpoint (addr=0, len=8, type=data-write):
 CONTROL (DR7): 0000000000990105          STATUS (DR6): 00000000ffff4ff0
 DR0: addr=0x0000000000000000, ref.count=1  DR1: addr=0x0000000000000008, ref.count=1
 DR2: addr=0x0000000000000000, ref.count=0  DR3: addr=0x0000000000000000, ref.count=0
remove_watchpoint (addr=0, len=8, type=data-write):
 CONTROL (DR7): 0000000000990104          STATUS (DR6): 00000000ffff4ff0
 DR0: addr=0x0000000000000000, ref.count=0  DR1: addr=0x0000000000000008, ref.count=1
 DR2: addr=0x0000000000000000, ref.count=0  DR3: addr=0x0000000000000000, ref.count=0
remove_watchpoint (addr=0, len=8, type=data-write):
 CONTROL (DR7): 0000000000990104          STATUS (DR6): 00000000ffff4ff0
 DR0: addr=0x0000000000000000, ref.count=0  DR1: addr=0x0000000000000008, ref.count=1
 DR2: addr=0x0000000000000000, ref.count=0  DR3: addr=0x0000000000000000, ref.count=0
(gdb) info watch
Num     Type           Disp Enb Address            What
1       hw watchpoint  keep n                      *(long *) 0x0
3       hw watchpoint  keep n                      *(long *) 0x0
4       hw watchpoint  keep n                      *(long *) 0x0
6       hw watchpoint  keep y                      * (char (*)[16]) 0x0
(gdb)