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

[Ulrich Weigand]

Pedro Franco de Carvalho wrote:

> This patch fixes some issues with debug register handling for the powerpc
> linux native target.

This looks quite good to me, I just have a couple of comments / questions
on implementation details remaining, primarily about the data structures.

> 	* ppc-linux-nat.c: Include <algorithm>, <unordered_map>, and
> 	<list>.  Remove inlcusion of observable.h.
Typo.

> +/* Private arch info associated with each thread lwp_info object, used
> +   for debug register handling.  */
> +
> +struct arch_lwp_info
> +{
> +  /* When true, indicates that the debug registers installed in the
> +     thread no longer correspond to the watchpoints and breakpoints
> +     requested by GDB.  */
> +  bool debug_regs_stale;
> +
> +  /* We need a back-reference to the PTID of the thread so that we can
> +     cleanup the debug register state of the thread in
> +     low_delete_thread.  */
> +  ptid_t lwp_ptid;

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

> +/* Class used to detect which set of ptrace requests that
> +   ppc_linux_nat_target will use to install and remove hardware
> +   breakpoints and watchpoints.
> +
> +   The first time the interface is requested it is detected, and
> +   subsequent requests will return the same value.  This value can
> +   indicate that no interface is available.

The one thing I'm wondering about here is that in order to detect
the interface, the code will rely on inferior_ptid.  This introduces
a new use of the inferior_ptid deeply buried within a function that
is called from a large number of places -- are we sure all those
callers actually always have a (correct) inferior_ptid in place?

Now, in practice, inferior_ptid is only needed for the *first*
call, and that first call will likely happen only from a small
subset of all the possible callers here.

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.


> +  /* The ptrace interface we'll use to install hardware watchpoints and
> +     breakpoints (debug registers).  */
> +  ppc_linux_dreg_interface m_dreg_interface;
> +
> +  /* A map from pid numbers to the list of hardware watchpoints and
> +     breakpoints that GDB requested for each process.
> +
> +     Only used when the interface is HWDEBUG.  */
> +  std::unordered_map<pid_t, std::list<ppc_hw_breakpoint>>
> +  m_requested_hw_bps;
> +
> +  /* A map from pid numbers to the watchpoint value that GDB requested
> +     for each process.
> +
> +     Only used when the interface is DEBUGREG.  */
> +  std::unordered_map<pid_t, long> m_requested_wp_vals;

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)?

[ *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? ]


> +  /* Callable object to hash ptids by their lwp number.  */
> +  struct ptid_hash
> +  {
> +    std::size_t operator() (const ptid_t &ptid) const
> +    {
> +      return std::hash<long>{} (ptid.lwp ());
> +    }
> +  };
> +
> +  /* A map from ptid_t objects to a list of pairs of slots and hardware
> +     breakpoint objects.  This keeps track of which hardware breakpoints
> +     and watchpoints were last installed in each slot of each thread.
> +
> +     Only used when the interface is HWDEBUG.  */
> +  std::unordered_map <ptid_t,
> +		      std::list<std::pair<long, ppc_hw_breakpoint>>,
> +		      ptid_hash> m_installed_hw_bps;

As mentioned above, why not move this to arch_lwp_info?


> +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.)

> +  bool stale_dregs = false;
Initializer unnecessary here.
> +
> +  stale_dregs = (lp->stop_reason == TARGET_STOPPED_BY_WATCHPOINT
> +		 || lp->stop_reason == TARGET_STOPPED_BY_HW_BREAKPOINT
> +		 || lp_arch_info->debug_regs_stale);
> +
> +  if (!stale_dregs)
>      return;

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU/Linux compilers and toolchain
  Ulrich.Weigand@de.ibm.com