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

Jan Kratochvil
2011-12-12T17:23:24Z

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