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

[Pedro Alves <pedro@codesourcery.com>]

On Tuesday 31 May 2011 21:25:30, Philippe Waroquiers wrote:
> 
> > Not sure I understand what is different between GDB and GDBserver
> > here.  A watchpoint, from breakpoint.c's perpective can be composed
> > of several low-level watchpoints.  E.g., if the expression the user
> > wants to watch requires trapping accesses to two disjoint memory
> > regions for changes, each of those memory regions will correspond
> > to one low-level hardware watchpoint.  In GDBserver's or i386-nat.c's
> > perpective, there will be two watchpoints.  If the second fails to
> > insert, then breakpoint.c in GDB rolls back the first.  This applies
> > to GDBserver as well.
> 
> > ../../../src/gdb/gdbserver/linux-x86-low.c:511: A problem internal to GDBserver has been detected.
> > Assertion `DR_FIRSTADDR <= regnum && regnum < DR_LASTADDR' failed.
> 
> Sorry for the somewhat wrong analysis of the bug.

Not to worry.  Thanks for tripping on this. :-)

> 
> I have applied your patch in the assert, and tested again.
> The GDBserver does not crash anymore (but it still keeps a DR register
> busy for no reason).

Yes, that's a bug to fix too.  I'll need a bit to take a better
look at your patch.

> 
> So, there is for sure still a difference of behaviour (probably in breakpoint.c
> placing a "local" watch and a "remote" watch).

It looks to me the bug is equality present on native gdb:

$ ./gdb ~/s 
GNU gdb (GDB) 7.3.50.20110527-cvs
...
(gdb) start
Temporary breakpoint 1 at 0x4004b8: file s.c, line 22.
Starting program: /home/pedro/s 

Temporary breakpoint 1, main () at s.c:22
22         char * p = s1000;
(gdb) set breakpoint always-inserted on 
(gdb) maint set show-debug-regs on 
(gdb) watch s1
Hardware watchpoint 2: s1
During symbol reading, incomplete CFI data; unspecified registers (e.g., rax) at 0x4004e3.
insert_watchpoint (addr=609a08, len=1, type=data-write):
        CONTROL (DR7): 0000000000010101          STATUS (DR6): 0000000000004000
        DR0: addr=0x0000000000609a08, ref.count=1  DR1: addr=0x0000000000000000, ref.count=0
        DR2: addr=0x0000000000000000, ref.count=0  DR3: addr=0x0000000000000000, ref.count=0
(gdb) watch s2
Hardware watchpoint 3: s2
insert_watchpoint (addr=60d8e8, len=2, type=data-write):
        CONTROL (DR7): 0000000000510105          STATUS (DR6): 0000000000004000
        DR0: addr=0x0000000000609a08, ref.count=1  DR1: addr=0x000000000060d8e8, ref.count=1
        DR2: addr=0x0000000000000000, ref.count=0  DR3: addr=0x0000000000000000, ref.count=0
(gdb) watch s4
Hardware watchpoint 4: s4
insert_watchpoint (addr=607280, len=4, type=data-write):
        CONTROL (DR7): 000000000d510115          STATUS (DR6): 0000000000004000
        DR0: addr=0x0000000000609a08, ref.count=1  DR1: addr=0x000000000060d8e8, ref.count=1
        DR2: addr=0x0000000000607280, ref.count=1  DR3: addr=0x0000000000000000, ref.count=0
(gdb) watch s3
Hardware watchpoint 5: s3
insert_watchpoint (addr=603768, len=3, type=data-write):
        CONTROL (DR7): 000000005d510155          STATUS (DR6): 0000000000004000
        DR0: addr=0x0000000000609a08, ref.count=1  DR1: addr=0x000000000060d8e8, ref.count=1
        DR2: addr=0x0000000000607280, ref.count=1  DR3: addr=0x0000000000603768, ref.count=1
Warning:
Could not insert hardware watchpoint 5.
Could not insert hardware breakpoints:
You may have requested too many hardware breakpoints/watchpoints.

(gdb) del
Delete all breakpoints? (y or n) y
remove_watchpoint (addr=609a08, len=1, type=data-write):
        CONTROL (DR7): 000000005d510154          STATUS (DR6): 0000000000004000
        DR0: addr=0x0000000000000000, ref.count=0  DR1: addr=0x000000000060d8e8, ref.count=1
        DR2: addr=0x0000000000607280, ref.count=1  DR3: addr=0x0000000000603768, ref.count=1
remove_watchpoint (addr=60d8e8, len=2, type=data-write):
        CONTROL (DR7): 000000005d510150          STATUS (DR6): 0000000000004000
        DR0: addr=0x0000000000000000, ref.count=0  DR1: addr=0x0000000000000000, ref.count=0
        DR2: addr=0x0000000000607280, ref.count=1  DR3: addr=0x0000000000603768, ref.count=1
remove_watchpoint (addr=607280, len=4, type=data-write):
        CONTROL (DR7): 000000005d510140          STATUS (DR6): 0000000000004000
        DR0: addr=0x0000000000000000, ref.count=0  DR1: addr=0x0000000000000000, ref.count=0
        DR2: addr=0x0000000000000000, ref.count=0  DR3: addr=0x0000000000603768, ref.count=1
(gdb) si
stopped_data_addr:
        CONTROL (DR7): 000000005d510140          STATUS (DR6): 0000000000004000
        DR0: addr=0x0000000000000000, ref.count=0  DR1: addr=0x0000000000000000, ref.count=0
        DR2: addr=0x0000000000000000, ref.count=0  DR3: addr=0x0000000000603768, ref.count=1
                                                             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
23         for (i = 0; i < 1000; i++)
(gdb) 

You just didn't trip on the assert there, because there's no such
assertion on gdb (because I had never ported the
patch that had added the assertion to gdbserver, back to gdb.  I'll need to
remember to do that, in the name of keeping the codebases as much in
sync as possible...)

> 
> Note that there is another similar (but I believe correct) assert in the code, but 
> slightly different. I am not sure to understand why regnum validity is tested
> differently in the below:
>   if (! (regnum >= 0 && regnum <= DR_LASTADDR - DR_FIRSTADDR))
>     fatal ("Invalid debug register %d", regnum);

Yeah, just different spellings of the same thing.

-- 
Pedro Alves