Re: [PATCH 3/3] [PowerPC] Fix debug register issues in ppc-linux-nat

Ulrich Weigand
2019-08-09T11:04:43Z

> "Ulrich Weigand" <uweigand@de.ibm.com> writes:
>
> > This looks generally good to me, just two questions:
> >
> > - As mentioned in the 1/3 patch, why do you need the low_new_clone
> > callback? As I understand it, you'll get low_new_thread called
> > immediatedly afterwards, which will mark the thread as "stale",
> > and once it is scheduled again, all debug regs will be set up
> > from scratch anyway ...
>
> The reason I did this is so that we have the lwp object of the parent
> thread, so that we can copy the correct debug register state. The
> arguments for low_new_thread don't include the parent. I think other
> targets always know how to clear all the debug registers without keeping
> track of anything, but we need to know which slots might already be
> installed in a new thread.

I may still be missing something, but why exactly *do* we need to know
which slots might already be installed? I'd have assumed that when we
get to low_prepare_to_resume, and the lwp is marked stale, we just throw
away everything and install the complete desired state.

> Another reason is that add_lwp (and therefore low_new_thread) is also
> called in cases other than a ptrace clone event.

Well, yes, but those cases *also* need to be handled, right? This is
e.g. when you attach to an already multi-threaded process while there
are already watchpoints set up. In that case, you'll need to install
those watchpoints into all those threads.

> > - We currently do not support hardware watchpoints in gdbserver,
> > even though we really should. Ideally, the low-level code to
> > handle debug regs should be shared between gdb and gdbserver,
> > as is done e.g. on x86. Now, I'm not saying that handling
> > gdbserver is a pre-req for this patch (fixing GDB first is of
> > course fine!), but I'm wondering if it would make sense, given
> > that you're refactoring a lot of this code anyway, to think
> > about whether this setup would help or hinder a future merge
> > with gdbserver.
>
> Ok, I'll review this and see if this can be easily ported to gdbserver.

Thanks!

Bye,