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

[Pedro Franco de Carvalho <pedromfc at linux dot ibm dot com>]

"Ulrich Weigand" <uweigand@de.ibm.com> writes:

> Can we simply store the installed slots map in here, instead of requiring
> a whole new per-lwp map in m_installed_hw_bps?

I had considered doing this, however, low_new_fork needs to copy the
per-lwp state in case the debug registers are copied across forks, and
this function is called before the lwp_info object for the new forked
thread is constructed, which only happens in
linux_nat_target::follow_fork.

> But it would seem cleaner to make this explicit by having an
> explicit "initialize" or "detect" call, which gets called in
> those places we expect to be "first", and which gets passed
> a ptid_t to use (where the callers will still pass inferior_ptid,
> but then at least the dependency will be explicit.

Agreed.  I'm investigating the best way to do this.

> I'm wondering if it might be preferable to have a single map from pid_t
> to a "per-process HW break/watchpoint" structure, which tracks the
> lifetime of the process (cleaned up completely in low_forget_process),
> and holds all the data (list of ppc_hw_breakpoint structs, plus a WP
> value)?

Yes, that would probably be cleaner.

> [ *Maybe* (and I'm not sure here) it would even make sense to move the
> ppc_linux_dreg_interface into that per-process struct, to clearly
> associate it with the pid that was used to query the kernel? ]

I'm not yet sure about this one, I have to think a bit more.

>> +ppc_linux_nat_target::hwdebug_point_cmp
>> +(const struct ppc_hw_breakpoint &a, const struct ppc_hw_breakpoint &b)
>
> You're using this style in a number of places, but I don't think this
> complies with the GNU coding style ...   (The '(' should not be in the
> first column.)

I will change this.  I had done this because even if I broke the line
after the first argument, the line still had more than the soft limit of
columns (74):

ppc_linux_nat_target::hwdebug_point_cmp (const struct ppc_hw_breakpoint &a,
					 const struct ppc_hw_breakpoint &b)

Is this a reasonable reason to exceed the soft limit column limit?  It's
under the hard limit (80).  If it's not reasonable, I'll have to do
something like:

bool
ppc_linux_nat_target::hwdebug_point_cmp (const struct
					 ppc_hw_breakpoint &a,
					 const struct
					 ppc_hw_breakpoint &b)


Thanks a lot for the review!

--
Pedro Franco de Carvalho