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

Philippe Waroquiers
2011-06-08T22:55:29Z

> Here it is. New test, fixes for both gdbserver and gdb.
....
> Any comments on this?
>
> --
> Pedro Alves
>
> 2011-06-01 Pedro Alves <pedro@codesourcery.com>

I think there is still something wrong with the DR registers, at least
with gdbserver.

First it looks like addr 0 is special, and keeps the register
busy. See below the trace of first the gdb session,
followed by the gdbserver session.

Reading symbols from /home/philippe/set-length-limit/s...done.
(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 * (int *) 0
Hardware watchpoint 1: * (int *) 0
(gdb) watch * (long *) 0
Hardware watchpoint 2: * (long *) 0
(gdb) watch * (int *) 8
Hardware watchpoint 3: * (int *) 8
(gdb) watch * (long *) 8
Hardware watchpoint 4: * (long *) 8
(gdb) del
Delete all breakpoints? (y or n) y
(gdb)

philippe@gcc10:~/set-length-limit/cvs/dr_busy_fix$ ./install/bin/gdbserver :1234 /home/philippe/set-length-limit/s
Process /home/philippe/set-length-limit/s created; pid = 20075
Listening on port 1234
Remote debugging from host 127.0.0.1
insert_watchpoint (addr=0, len=4, type=data-write):
CONTROL (DR7): 000d0101 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=8, type=data-write):
CONTROL (DR7): 009d0105 STATUS (DR6): 00000000
DR0: addr=0x0, ref.count=1 DR1: addr=0x0, ref.count=1
DR2: addr=0x0, ref.count=0 DR3: addr=0x0, ref.count=0
insert_watchpoint (addr=8, len=4, type=data-write):
CONTROL (DR7): 0d9d0115 STATUS (DR6): 00000000
DR0: addr=0x0, ref.count=1 DR1: addr=0x0, ref.count=1
DR2: addr=0x8, ref.count=1 DR3: addr=0x0, ref.count=0
insert_watchpoint (addr=8, len=8, type=data-write):
CONTROL (DR7): 9d9d0155 STATUS (DR6): 00000000
DR0: addr=0x0, ref.count=1 DR1: addr=0x0, ref.count=1
DR2: addr=0x8, ref.count=1 DR3: addr=0x8, ref.count=1
remove_watchpoint (addr=0, len=4, type=data-write):
CONTROL (DR7): 9d9d0154 STATUS (DR6): 00000000
DR0: addr=0x0, ref.count=1 DR1: addr=0x0, ref.count=1
DR2: addr=0x8, ref.count=1 DR3: addr=0x8, ref.count=1
remove_watchpoint (addr=0, len=8, type=data-write):
CONTROL (DR7): 9d9d0150 STATUS (DR6): 00000000
DR0: addr=0x0, ref.count=1 DR1: addr=0x0, ref.count=1
DR2: addr=0x8, ref.count=1 DR3: addr=0x8, ref.count=1
remove_watchpoint (addr=8, len=4, type=data-write):
CONTROL (DR7): 9d9d0140 STATUS (DR6): 00000000
DR0: addr=0x0, ref.count=1 DR1: addr=0x0, ref.count=1
DR2: addr=0x0, ref.count=0 DR3: addr=0x8, ref.count=1
remove_watchpoint (addr=8, len=8, type=data-write):
CONTROL (DR7): 9d9d0100 STATUS (DR6): 00000000
DR0: addr=0x0, ref.count=1 DR1: addr=0x0, ref.count=1
DR2: addr=0x0, ref.count=0 DR3: addr=0x0, ref.count=0

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.
It looks to me it would be clearer (and correct?) to always also
enter in the block updating really the inferior
if the dr_ref_counts are != (similarly to the dr_mirror).

After that, I have done some tests combining the patch fixing
the DR busy and the patch that allows to set the length
of remote hw watchpoints. This allows to test gdbserver
when it has to share low level watchpoints.
It looks like in this case, gdbserver does not properly
maintain the ref.count:
(!!!! so the below is with a gdb which includes the two
patches, so that a watchpoint of 16 bytes is accepted
by gdb and transmitted to gdbserver).

philippe@gcc10:~/set-length-limit/cvs/dr_busy_fix_and_set_length_from_src$ ./install/bin/gdb ~/set-length-limit/s
GNU gdb (GDB) 7.3.50.20110608-cvs
Copyright (C) 2011 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law. Type "show copying"
and "show warranty" for details.
This GDB was configured as "x86_64-unknown-linux-gnu".
For bug reporting instructions, please see:
<http://www.gnu.org/software/gdb/bugs/>...
Reading symbols from /home/philippe/set-length-limit/s...done.
(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 * (char (*)[16]) 0x0
Hardware watchpoint 2: * (char (*)[16]) 0x0
(gdb) del
Delete all breakpoints? (y or n) y
(gdb)

philippe@gcc10:~/set-length-limit/cvs/dr_busy_fix_and_set_length_from_src$ ./install/bin/gdbserver :1234
/home/philippe/set-length-limit/s
Process /home/philippe/set-length-limit/s created; pid = 7504
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
insert_watchpoint (addr=0, len=16, 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=1 DR1: addr=0x8, ref.count=1
DR2: addr=0x0, ref.count=0 DR3: addr=0x0, ref.count=0
remove_watchpoint (addr=0, len=16, type=data-write):
CONTROL (DR7): 00990104 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

I double checked that I was testing with the gdbserver
containing the fix by attaching to gdbserver with another
gdb, and putting a break in the function update_inferior:
(gdb) attach 7499
Attaching to process 7499
Reading symbols from /home/philippe/set-length-limit/cvs/dr_busy_fix_and_set_length_from_src/install/bin/gdbserver...done.
Reading symbols from /lib/libdl.so.2...(no debugging symbols found)...done.
Loaded symbols for /lib/libdl.so.2
Reading symbols from /lib/libc.so.6...(no debugging symbols found)...done.
Loaded symbols for /lib/libc.so.6
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
0x00007ffff7953d03 in select () from /lib/libc.so.6
(gdb) break update_inferior
Breakpoint 1 at 0x422010: file i386-low.c, line 441.
(gdb)

We see that the gdbserver is properly re-using a low level watchpoint
for a 2nd high level watchpoint, but the ref.count is kept to 1,
while it should go to 2.

We have similar ref.count problem with an address != 0:

philippe@gcc10:~/set-length-limit/cvs/dr_busy_fix_and_set_length_from_src$ ./install/bin/gdb ~/set-length-limit/s
GNU gdb (GDB) 7.3.50.20110608-cvs
Copyright (C) 2011 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law. Type "show copying"
and "show warranty" for details.
This GDB was configured as "x86_64-unknown-linux-gnu".
For bug reporting instructions, please see:
<http://www.gnu.org/software/gdb/bugs/>...
Reading symbols from /home/philippe/set-length-limit/s...done.
(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 *) 0x8
Hardware watchpoint 1: * (long *) 0x8
(gdb) watch * (char (*)[16]) 0x8
Hardware watchpoint 2: * (char (*)[16]) 0x8
(gdb) del
Delete all breakpoints? (y or n) y
(gdb)

Process /home/philippe/set-length-limit/s created; pid = 9959
Listening on port 1234
Remote debugging from host 127.0.0.1
insert_watchpoint (addr=8, len=8, type=data-write):
CONTROL (DR7): 00090101 STATUS (DR6): 00000000
DR0: addr=0x8, ref.count=1 DR1: addr=0x0, ref.count=0
DR2: addr=0x0, ref.count=0 DR3: addr=0x0, ref.count=0
insert_watchpoint (addr=8, len=16, type=data-write):
CONTROL (DR7): 00990105 STATUS (DR6): 00000000
DR0: addr=0x8, ref.count=1 DR1: addr=0x10, ref.count=1
DR2: addr=0x0, ref.count=0 DR3: addr=0x0, ref.count=0
remove_watchpoint (addr=8, len=8, type=data-write):
CONTROL (DR7): 00990104 STATUS (DR6): 00000000
DR0: addr=0x0, ref.count=0 DR1: addr=0x10, ref.count=1
DR2: addr=0x0, ref.count=0 DR3: addr=0x0, ref.count=0
remove_watchpoint (addr=8, len=16, type=data-write):
CONTROL (DR7): 00990104 STATUS (DR6): 00000000
DR0: addr=0x0, ref.count=0 DR1: addr=0x10, ref.count=1
DR2: addr=0x0, ref.count=0 DR3: addr=0x0, ref.count=0

Maybe it would be possible to add some asserts in the handling
of the DR ? E.g. if the ref.count > 0, then the register must be 'active'
and if ref.count == 0, then register must be 'inactive' ?

Finally, a question about the below comment from a new test.
If I understood correctly, the 'single (low level)' should rather be 'single (high level)' ?

+# Regression test for a bug where the i386 watchpoints support backend
+# would leave some debug registers occupied even if not enough debug
+# registers were available to cover a single (low level) watchpoint.

Philippe