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

[Pedro Alves <pedro at codesourcery dot com>]

On Wednesday 08 June 2011 23:55:11, Philippe Waroquiers wrote:
> I think there is still something wrong with the DR registers, at least
> with gdbserver.

Yes.  Many thanks for noticing this.

> 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].

> 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).

The point was that we need to set the DR[0-3] register
when activating the watchpoint.  If going from refcount
1 to 2, say, then the address hasn't changed, and, we
already know it contains the correct address (set when
we went from 0 to 1).

The problem is actually that I forgot one important line of
code when porting the changes from gdb to gdbserver.

Index: src/gdb/gdbserver/i386-low.c
===================================================================
--- src.orig/gdb/gdbserver/i386-low.c   2011-06-01 15:19:13.000000000 +0100
+++ src/gdb/gdbserver/i386-low.c        2011-06-09 00:36:18.008128016 +0100
@@ -458,6 +458,8 @@ update_inferior (struct i386_debug_reg_s
       inf_state->dr_control_mirror = new_state->dr_control_mirror;
       i386_dr_low_set_control (inf_state);
     }
+
+  *inf_state = *new_state;
 }
 
 /* Insert a watchpoint to watch a memory region which starts at

That is, I missed the "commit" from the mirror of the mirror to
the regular process mirror.  It's present on the gdb side
of the changes as:

static void
update_inferior (struct i386_debug_reg_state *state)
{
...
  dr_mirror = *state;
}

Sorry about that. :-P

I'll take a better look at this tomorrow and check if I
missed anything else.  I'll try to add yet some other test
that catches this.

> 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' ?

Sounds like a good idea.

> 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.

In the context of that comment, a high level watchpoint would be
a watchpoint on the GDB side (e.g., watch $expression creates
one high level watchpoint, with possibly many low level watchpoints,
or watchpoint locations).  This expression may require watching more
than one memory region.  Each of those regions will end up as a watchpoint
location, and a "low level" watchpoint inserted on the target for each
of them (a Z2 packet for each).  The bug happens when not enough
debug registers are available to cover a single Z2 packet / low level
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?

-- 
Pedro Alves