Re: [PATCH] PR threads/10729: x86 hw watchpoints and non-stop mode

[Jan Kratochvil <jan.kratochvil@redhat.com>]

On Mon, 12 Dec 2011 17:56:44 +0100, Pedro Alves wrote:
> I'm trying out this patch:

That's great, thanks.


> > Deleting this part is a regression.  Testcase for that part is attached.

I will yet update the testcase to support gdbserver.


> > > +	  && I386_DR_GET_RW_LEN (control, i) != 0)
> > >  	{
> > > -	  addr = state->dr_mirror[i];
> > > +	  addr = i386_dr_low.get_addr (i);
> > 
> > Why to do this change?  Why we can no longer trust DR_MIRROR?  This is
> > a performance regression.
> 
> This is non-stop, so threads can be running while we change the
> global state->dr_mirror (and friends).  Say, we set a watchpoint,
> and let the threads rusume.  Now, say you delete the watchpoint, or
> add/remove watchpoints such that state->dr_mirror[*] changes.  Inserting/deleting
> watchpoines updates state->dr_mirror[*].  Now threads haven't been updated
> with the mirror yet, and say a thread has meanwhile hit an old watchpoint,
> but we haven't handled the SIGTRAP yet.  If we trusted state->dr_mirror[*],
> we'd mistake the real trapped address to whatever was
> currently state->dr_mirror[i].  So state->dr_mirror now represents
> intention.  To get at the address that trapped, we need to read the
> state the thread had when it trapped.  I'll add some comments to the code.

Thanks for the explanation, yes, comment would be great.

As there is a state for each inferior in the multi-inferior patch of mine it
may be useful to change it to be per-TID so these ptrace reads can be avoided.


Thanks,
Jan