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

[Jan Kratochvil <jan dot kratochvil at redhat dot com>]

Hi Joel,

I had this mail unsent but I see no reason why now; this patchset is now
dependent on posted:
	Introduce new target_thread_stopped_by_watchpoint.
	http://sourceware.org/ml/gdb-patches/2009-10/msg00235.html
	http://sourceware.org/ml/gdb-patches/2009-10/msg00274.html


On Wed, 07 Oct 2009 19:54:16 +0200, Joel Brobecker wrote:
> > > I don't understand why you clear the bit when removing the watchpoint,
> > > however. Can you explain?
[...]
> Would we have a reproducible bug if this code wasn't there?

Currently no longer as you state in this paragraph:

> But even that shouldn't a problem, since I assume that we've disabled the
> watchpoint anyway.  i386_stopped_data_address does verify that the
> associated DR is active before reporting a hit:
[...]
>           /* This third condition makes sure DRi is not vacant, this
>              avoids false positives in windows-nat.c.  */
>           && !I386_DR_VACANT (i))

This condition was not present in the FSF GDB code the time of the first post
of this patch this year.  The FSF GDB has now this part fixed by:
	http://sourceware.org/ml/gdb-cvs/2009-09/msg00176.html
	http://sourceware.org/ml/gdb-patches/2009-09/msg00699.html
	first post: http://sourceware.org/ml/gdb-patches/2009-06/msg00054.html

	The patch at these URLs fixed a subset of problems fixed by this
	patchset posted earlier.

I dropped this part of my patch now as it became redundant for the provided
testcases.

(I still believe such code should be there to prevent needless cluttering of
the DR registers but a patch with such goal can be a separate one with wider
coverage of the DR registers sanity checks.)


[...]
> > +   Hardware watchpoints (bp_read_watchpoint and bp_access_watchpoint; false hit
> > +   by bp_hardware_watchpoint is not user-visible - its hit is suppressed if the
> > +   memory content has not changed) must be reset exactly once after being
> > +   presented to the user, neither sooner nor later.
> 
> The digressions make it hard to see where you're trying to go. I'd
> suggest moving it to later. For instance;
> 
>       Hardware watchpoints must be reset exactly once after being presented
>       to the user, neither sooner nor later. Note that [...].

Done.

> I'd even get rid of the digression. It seems to me that this is only mindly
> related to the discussion, and that piece of information would be better
> located near the actual code where it matters.

I do not know a better place, this behavior is implicit to the watchpoint
framework.  This function update_watchpoint does the removing/reinserting
hardware watchpoints so it I find it the most close point for such note.


>       Hardware watchpoints must be reset exactly once after being presented
>       to the user.  It cannot be done sooner, because it would reset the
>       data used to present the watchpoint hit to the user.  And it must
>       not be done later because (?).

Done:
it could display the same single watchpoint hit during multiple GDB stops.


[...]
> > +   * Multiple hardware watchpoints can be hit on the first stop while GDB
> > +     presents only one reason on each stop event to the user.  The other hits
> > +     will get present to the user without resuming inferior (and thus removing
> 
> I am no longer familiar with that part of GDB, so the following is only
> a rewrite based on what you wrote. I am a little surprised, now that
> I think of it, as I thought all breakpoints (including watchpoints were
> removed at normal_stop:
> 
>   if (!breakpoints_always_inserted_mode () && target_has_execution)
>     {
>       if (remove_breakpoints ())

My comment was wrong, I probably thought about it this way but in real only
the specific threads having the watchpoint trigger still unprocessed are not
resumed.  Trigger is kept by save_sigtrap + linux_nat_stopped_data_address, no
longer in the hardware debug registers).

It is illustrated in the filtered debug dump below ("unset:LWP..." is a debug
message added there for this purpose).


Thanks,
Jan


infrun: stopped by watchpoint
infrun: stopped data address = 0x602710
[Switching to Thread 0x7ffff7fe1910 (LWP 4013)]
unset:LWP=4013,value=0xffff4ff1&=~0x1 = 0
unset:LWP=4014,value=0xffff4ff2&=~0x2 = 0
unset:LWP=4010,value=0xffff4ff0 = 0
Hardware read watchpoint 2: thread1_rwatch
Value = 0
0x0000000000401138 in thread1_func (unused=0x0) at ../.././gdb/testsuite/gdb.threads/watchthreads-reorder.c:98
98        rwatch_store = thread1_rwatch;
(gdb) PASS: gdb.threads/watchthreads-reorder.exp: reorder0: continue a
Continuing.
unset:LWP=4014,value=0xffff4ff0 = 0
unset:LWP=4013,value=0xffff4ff0 = 0
unset:LWP=4010,value=0xffff4ff0 = 0
infrun: resume (step=0, signal=0), trap_expected=0
LLR: Preparing to resume process 4010, 0, inferior_ptid Thread 0x7ffff7fe1910 (LWP 4013)
RC: Not resuming sibling Thread 0x7ffff75e0910 (LWP 4014) (has pending)
RC: Not resuming sibling Thread 0x7ffff7fe1910 (LWP 4013) (not stopped)
RC:  PTRACE_CONT Thread 0x7ffff7fe26f0 (LWP 4010), 0, 0 (resuming sibling)
RC:  PTRACE_CONT Thread 0x7ffff7fe26f0 (LWP 4010), 0, 0 (resume sibling)
LLR: PTRACE_CONT process 4013, 0 (resume event thread)
infrun: wait_for_inferior (treat_exec_as_sigtrap=0)
linux_nat_wait: [process -1]
LLW: Using pending wait status Trace/breakpoint trap (stopped) for Thread 0x7ffff75e0910 (LWP 4014).
LLW: Candidate event Trace/breakpoint trap (stopped) in Thread 0x7ffff75e0910 (LWP 4014).
SC:  kill Thread 0x7ffff7fe1910 (LWP 4013) **<SIGSTOP>**
SC:  lwp kill 0 ERRNO-OK
SC:  kill Thread 0x7ffff7fe26f0 (LWP 4010) **<SIGSTOP>**
SC:  lwp kill 0 ERRNO-OK
WL: waitpid Thread 0x7ffff7fe1910 (LWP 4013) received Stopped (signal) (stopped)
WL: waitpid Thread 0x7ffff7fe26f0 (LWP 4010) received Stopped (signal) (stopped)
LLW: trap ptid is Thread 0x7ffff75e0910 (LWP 4014).
infrun: stopped by watchpoint
infrun: stopped data address = 0x602714
[Switching to Thread 0x7ffff75e0910 (LWP 4014)]
unset:LWP=4014,value=0xffff4ff0 = 0
unset:LWP=4013,value=0xffff4ff0 = 0
unset:LWP=4010,value=0xffff4ff0 = 0
Hardware read watchpoint 3: thread2_rwatch
Value = 0
0x0000000000401201 in thread2_func (unused=0x0) at ../.././gdb/testsuite/gdb.threads/watchthreads-reorder.c:123
123       rwatch_store = thread2_rwatch;
(gdb) PASS: gdb.threads/watchthreads-reorder.exp: reorder0: continue b