Re: [patch 1/4] Fix hw watchpoints: [x86*] repeated rwatch output

[Joel Brobecker <brobecker at adacore dot com>]

> Intel Developer's Manual Volume 3B: System Programming Guide:
> 	"debug handlers should clear the [DR6] register before returning to
> 	the interrupted task"

As I mentioned humorously on IRC, this is an instance where the submitter
is the teacher and I'm the student. Sorry you're stuck with me :).

> This currently does not happen for x86* targets on the watchpoint trigger so:
> * "watch" (read/write): the referenced value is read on each SIGTRAP to
>   silently find out it did not change.
> * "rwatch" (read access): The hit is falsely duplicated on each SIGTRAP
>   unrelated of its reason.

If I understand correctly the problem, the Bn bits in DR6 should be
cleared after we have received a debug exception where they triggered;
Otherwise, these bits remain set, and we think we keep hitting this
watchpoint.  Correct?

Of course, there is no hook for resetting these bits "before returning
to the interrupted task", so what you are doing instead is that you
clear this flag when the watchpoint gets inserted.  But does this work
in breakpoint always-inserted on mode?

> @@ -387,6 +394,11 @@ i386_remove_aligned_watchpoint (CORE_ADDR addr, unsigned len_rw_bits)
>  	      i386_dr_low.set_control (dr_control_mirror);
>  	      if (i386_dr_low.reset_addr)
>  		i386_dr_low.reset_addr (i);
> +
> +	      /* Status must be already queried for each LWP.  Otherwise it will
> +	         be lost in all-stop mode + breakpoint always-inserted off.  */
> +	      if (i386_dr_low.unset_status)
> +		i386_dr_low.unset_status (I386_DR_WATCH_MASK (i));

I don't understand why you clear the bit when removing the watchpoint,
however. Can you explain?

> +  volatile int dummy;
> +
> +  dummy = watchee;
> +  dummy = 1;
> +  dummy = 2;	/* break-at-exit */

Is that to prevent watchee from being optimized away, or does this
serve any other purpose? A comment explaining the purpose of this code
would be nice (you know me :-).

> +# Before the inferior gets started we would get:
> +# 	Target does not support this type of hardware watchpoint.
> +gdb_test "delete 1"

I don't quite understand this. After thinking about it, maybe it's just
because you want to try a read watchpoint instead of the read/write that
we have setup earlier? Oh, perhaps you are meaning to say that you cannot
do the "rwatch" at the same time the other watchpoint was set, because
it would trigger an error while starting the inferior?  Why would that
be, btw?

-- 
Joel