Re: [PATCH 3/3] MIPS h/w watchpoint in GDBserver

["Maciej W. Rozycki" <macro at codesourcery dot com>]

On Thu, 13 Jun 2013, Yao Qi wrote:

> Pedro mentioned that there are "unexplained odd placements for
> includes", however, the only 'oddity' I can see is the order of include
> mips-linux-watch.h and endian.h.  In this version, I exchange the order
> of them.  I also add a news entry for it.

 This is mostly OK, thanks.  Please see individual notes below for some 
issues, mostly minor style ones; code itself looks good to me.

> 2013-06-13  Jie Zhang  <jie@codesourcery.com>
> 	    Daniel Jacobowitz  <dan@codesourcery.com>
> 	    Yao Qi  <yao@codesourcery.com>
> 
> 	* Makefile.in (SFILES): Add common/mips-linux-watch.c.
> 	(mips-linux-watch.o): New rule.
> 	* configure.srv (srv_tgtobj): Add mips-linux-watch.o

 Use:

	* configure.srv <mips*-*-linux*>: Add mips-linux-watch.o to 
	srv_tgtobj.

> diff --git a/gdb/NEWS b/gdb/NEWS
> index eea9917..d56e18a 100644
> --- a/gdb/NEWS
> +++ b/gdb/NEWS
> @@ -108,6 +108,8 @@ qXfer:libraries-svr4:read's annex
>    ** GDBserver now supports target-assisted range stepping.  Currently
>       enabled on x86/x86_64 GNU/Linux targets.
>  
> +  ** GDBserver now supports hardware watchpoint on mips GNU/Linux target.

 Please capitalise MIPS.

> diff --git a/gdb/gdbserver/Makefile.in b/gdb/gdbserver/Makefile.in
> index e8470a8..08a475f 100644
> --- a/gdb/gdbserver/Makefile.in
> +++ b/gdb/gdbserver/Makefile.in
> @@ -556,6 +556,9 @@ agent.o: ../common/agent.c
>  linux-btrace.o: ../common/linux-btrace.c $(linux_btrace_h) $(server_h)
>  	$(CC) -c $(CPPFLAGS) $(INTERNAL_CFLAGS) $<
>  
> +mips-linux-watch.o: ../common/mips-linux-watch.c ../common/mips-linux-watch.h $(server_h)
> +	$(CC) -c $(CPPFLAGS) $(INTERNAL_CFLAGS) $<
> +

 Please define and use $(mips_linux_watch_h) as with other header deps.

> diff --git a/gdb/gdbserver/configure.srv b/gdb/gdbserver/configure.srv
> index 879d0de..8dee65b 100644
> --- a/gdb/gdbserver/configure.srv
> +++ b/gdb/gdbserver/configure.srv
> @@ -183,7 +183,7 @@ case "${target}" in
>  			srv_regobj="${srv_regobj} mips64-linux.o"
>  			srv_regobj="${srv_regobj} mips64-dsp-linux.o"
>  			srv_tgtobj="linux-low.o linux-osdata.o linux-mips-low.o linux-procfs.o"
> -			srv_tgtobj="${srv_tgtobj} linux-ptrace.o"
> +			srv_tgtobj="${srv_tgtobj} linux-ptrace.o mips-linux-watch.o"

 This extends beyond 79th column, add mips-linux-watch.o on another line.

> diff --git a/gdb/gdbserver/linux-mips-low.c b/gdb/gdbserver/linux-mips-low.c
> index 1010528..5ff9974 100644
> --- a/gdb/gdbserver/linux-mips-low.c
> +++ b/gdb/gdbserver/linux-mips-low.c
> @@ -159,6 +160,39 @@ get_usrregs_info (void)
>    return regs_info->usrregs;
>  }
>  
> +/* Per-process arch-specific data we want to keep.  */
> +
> +struct arch_process_info
> +{
> +  /* -1 if the kernel and/or CPU do not support watch registers.
> +      1 if watch_readback is valid and we can read style, num_valid
> +	and the masks.
> +      0 if we need to read the watch_readback.  */
> +
> +  int watch_readback_valid;
> +
> +  /* Cached watch register read values.  */
> +
> +  struct pt_watch_regs watch_readback;
> +
> +  /* Current watchpoint requests for this process.  */
> +
> +  struct mips_watchpoint *current_watches;
> +
> +  /*  The current set of watch register values for writing the
> +      registers.  */
> +
> +  struct pt_watch_regs watch_mirror;
> +};
> +
> +/* Per-thread arch-specific data we want to keep.  */
> +
> +struct arch_lwp_info
> +{
> +  /* Non-zero if our copy differs from what's recorded in the thread.  */
> +  int debug_registers_changed;

 Rename to watch_registers_changed.

 Rather than "debug registers" please refer to "watch registers", which is 
the proper architectural name of this facility.  On MIPS processors debug 
registers are the EJTAG register set.  Apply throughout as required, I 
won't play a `sed' role and point out individual cases.

> @@ -257,6 +291,303 @@ mips_breakpoint_at (CORE_ADDR where)
>    return 0;
>  }
>  
> +/* Mark the debug registers of lwp, represented by ENTRY, is changed,

 s/is changed/as changed/ presumably?

> +   if the lwp's process id is *PID_P.  */
> +
> +static int
> +update_debug_registers_callback (struct inferior_list_entry *entry,
> +				 void *pid_p)
> +{
> +  struct lwp_info *lwp = (struct lwp_info *) entry;
> +  int pid = *(int *) pid_p;
> +
> +  /* Only update the threads of this process.  */
> +  if (pid_of (lwp) == pid)
> +    {
> +      /* The actual update is done later just before resuming the lwp,
> +	 we just mark that the registers need updating.  */
> +      lwp->arch_private->debug_registers_changed = 1;
> +
> +      /* If the lwp isn't stopped, force it to momentarily pause, so
> +	 we can update its debug registers.  */
> +      if (!lwp->stopped)
> +	linux_stop_lwp (lwp);
> +    }
> +
> +  return 0;
> +}
> +
> +/* This is the implementation of linux_target_ops method
> +   new_process.  */
> +
> +static struct arch_process_info *
> +mips_linux_new_process (void)
> +{
> +  struct arch_process_info *info = xcalloc (1, sizeof (*info));
> +
> +  return info;
> +}
> +
> +/* This is the implementation of linux_target_ops method new_thread.
> +   Mark the debug registers are changed, so the threads' copies will

 s/are changed/as changed/?

> +   be updated.  */
> +
> +static struct arch_lwp_info *
> +mips_linux_new_thread (void)
> +{
> +  struct arch_lwp_info *info = xcalloc (1, sizeof (*info));
> +
> +  info->debug_registers_changed = 1;
> +
> +  return info;
> +}
> +
> +/* This is the implementation of linux_target_ops method
> +   prepare_to_resume.  If the debug regs have changed, update the
> +   thread's copies.  */
> +
> +static void
> +mips_linux_prepare_to_resume (struct lwp_info *lwp)
> +{
> +  ptid_t ptid = ptid_of (lwp);
> +  struct process_info *proc = find_process_pid (ptid_get_pid (ptid));
> +  struct arch_process_info *private = proc->private->arch_private;
> +
> +  if (lwp->arch_private->debug_registers_changed)
> +    {
> +      /* Only update the debug registers if we have set or unset a
> +	 watchpoint already.  */
> +      if (mips_linux_watch_get_num_valid (&private->watch_mirror) > 0)
> +	{
> +	  /* Write the mirrored watch register values.  */
> +	  int tid = ptid_get_lwp (ptid);
> +
> +	  if (ptrace (PTRACE_SET_WATCH_REGS, tid, &private->watch_mirror) == -1)

 Oversize line.

> +	    perror_with_name ("Couldn't write debug register");

 "Couldn't write watch registers"

> +	}
> +
> +      lwp->arch_private->debug_registers_changed = 0;
> +    }
> +}
> +
> +/* This is the implementation of linux_target_ops method
> +   insert_point.  */
> +
> +static int
> +mips_insert_point (char type, CORE_ADDR addr, int len)
> +{
> +  struct process_info *proc = current_process ();
> +  struct arch_process_info *private = proc->private->arch_private;
> +  struct pt_watch_regs regs;
> +  struct mips_watchpoint *new_watch;
> +  struct mips_watchpoint **pw;
> +  int pid;
> +  long lwpid;
> +
> +  /* Breakpoint/watchpoint types:
> +       '0' - software-breakpoint (not supported)
> +       '1' - hardware-breakpoint (not supported)
> +       '2' - write watchpoint (supported)
> +       '3' - read watchpoint (supported)
> +       '4' - access watchpoint (supported).  */
> +
> +  if (type < '2' || type > '4')
> +    {
> +      /* Unsupported.  */
> +      return 1;
> +    }
> +
> +  lwpid = lwpid_of (get_thread_lwp (current_inferior));
> +  if (!mips_linux_read_watch_registers (lwpid,
> +					&private->watch_readback,
> +					&private->watch_readback_valid,
> +					0))
> +    return -1;
> +
> +  if (len <= 0)
> +    return -1;
> +
> +  regs = private->watch_readback;
> +  /* Add the current watches.  */
> +  mips_linux_watch_populate_regs (private->current_watches, &regs);
> +
> +  /* Now try to add the new watch.  */
> +  if (!mips_linux_watch_try_one_watch (&regs, addr, len,
> +				       mips_linux_watch_type_to_irw (type)))
> +    return -1;
> +
> +  /* It fit.  Stick it on the end of the list.  */
> +  new_watch = (struct mips_watchpoint *)
> +    xmalloc (sizeof (struct mips_watchpoint));

 Extraneous cast.

> +  new_watch->addr = addr;
> +  new_watch->len = len;
> +  new_watch->type = type;
> +  new_watch->next = NULL;
> +
> +  pw = &private->current_watches;
> +  while (*pw != NULL)
> +    pw = &(*pw)->next;
> +  *pw = new_watch;
> +
> +  private->watch_mirror = regs;
> +
> +  /* Only update the threads of this process.  */
> +  pid = pid_of (proc);
> +  find_inferior (&all_lwps, update_debug_registers_callback, &pid);
> +
> +  return 0;
> +}
> +
> +/* This is the implementation of linux_target_ops method
> +   remove_point.  */
> +
> +static int
> +mips_remove_point (char type, CORE_ADDR addr, int len)
> +{
> +  struct process_info *proc = current_process ();
> +  struct arch_process_info *private = proc->private->arch_private;
> +
> +  int deleted_one;
> +  int pid;
> +
> +  struct mips_watchpoint **pw;
> +  struct mips_watchpoint *w;
> +
> +  /* Breakpoint/watchpoint types:
> +       '0' - software-breakpoint (not supported)
> +       '1' - hardware-breakpoint (not supported)
> +       '2' - write watchpoint (supported)
> +       '3' - read watchpoint (supported)
> +       '4' - access watchpoint (supported).  */
> +
> +  if (type < '2' || type > '4')
> +    {
> +      /* Unsupported.  */
> +      return 1;
> +    }
> +
> +  /* Search for a known watch that matches.  Then unlink and free
> +     it.  */

 No need to wrap this line.

> +  deleted_one = 0;
> +  pw = &private->current_watches;
> +  while ((w = *pw))
> +    {
> +      if (w->addr == addr && w->len == len && w->type == type)
> +	{
> +	  *pw = w->next;
> +	  free (w);
> +	  deleted_one = 1;
> +	  break;
> +	}
> +      pw = &(w->next);
> +    }
> +
> +  if (!deleted_one)
> +    return -1;  /* We don't know about it, fail doing nothing.  */
> +
> +  /* At this point watch_readback is known to be valid because we
> +     could not have added the watch without reading it.  */
> +  gdb_assert (private->watch_readback_valid == 1);
> +
> +  private->watch_mirror = private->watch_readback;
> +  mips_linux_watch_populate_regs (private->current_watches,
> +				  &private->watch_mirror);
> +
> +  /* Only update the threads of this process.  */
> +  pid = pid_of (proc);
> +  find_inferior (&all_lwps, update_debug_registers_callback, &pid);
> +  return 0;
> +}
> +
> +/* This is the implementation of linux_target_ops method
> +   stopped_by_watchpoint.  The watchhi R and W bits indicate
> +   the watch register triggered. */
> +
> +static int
> +mips_stopped_by_watchpoint (void)
> +{
> +  struct process_info *proc = current_process ();
> +  struct arch_process_info *private = proc->private->arch_private;
> +  int n;
> +  int num_valid;
> +  long lwpid = lwpid_of (get_thread_lwp (current_inferior));
> +
> +  if (!mips_linux_read_watch_registers (lwpid,
> +					&private->watch_readback,
> +					&private->watch_readback_valid,
> +					1))
> +    return 0;
> +
> +  num_valid = mips_linux_watch_get_num_valid (&private->watch_readback);
> +
> +  for (n = 0; n < MAX_DEBUG_REGISTER && n < num_valid; n++)
> +    if (mips_linux_watch_get_watchhi (&private->watch_readback, n)
> +	& (R_MASK | W_MASK))
> +      return 1;
> +
> +  return 0;
> +}
> +
> +/* This is the implementation of linux_target_ops method
> +   stopped_data_address.  */
> +
> +static CORE_ADDR
> +mips_stopped_data_address (void)
> +{
> +  struct process_info *proc = current_process ();
> +  struct arch_process_info *private = proc->private->arch_private;
> +  int n;
> +  int num_valid;
> +  long lwpid = lwpid_of (get_thread_lwp (current_inferior));
> +
> +  /* On mips we don't know the low order 3 bits of the data address.

 Capitalise MIPS.

> +     GDB does not support remote targets that can't report the
> +     watchpoint address.  So, make our best guess; return the starting
> +     address of a watchpoint request which overlaps the one that
> +     triggered.  */
> +
> +  if (!mips_linux_read_watch_registers (lwpid,
> +					&private->watch_readback,
> +					&private->watch_readback_valid,
> +					0))
> +    return 0;
> +
> +  num_valid = mips_linux_watch_get_num_valid (&private->watch_readback);
> +
> +  for (n = 0; n < MAX_DEBUG_REGISTER && n < num_valid; n++)
> +    if (mips_linux_watch_get_watchhi (&private->watch_readback, n)
> +	& (R_MASK | W_MASK))
> +      {
> +	CORE_ADDR t_low, t_hi;
> +	int t_irw;
> +	struct mips_watchpoint *watch;
> +
> +	t_low = mips_linux_watch_get_watchlo (&private->watch_readback, n);
> +	t_irw = t_low & IRW_MASK;
> +	t_hi = (mips_linux_watch_get_watchhi (&private->watch_readback, n)
> +		| IRW_MASK);
> +	t_low &= ~(CORE_ADDR)t_hi;
> +
> +	for (watch = private->current_watches;
> +	     watch != NULL;
> +	     watch = watch->next)
> +	  {
> +	    CORE_ADDR addr = watch->addr;
> +	    CORE_ADDR last_byte = addr + watch->len - 1;
> +	    if ((t_irw & mips_linux_watch_type_to_irw (watch->type))
> +		== 0)

 No need to wrap the line here.

 Please resend with the requested changes applied.

  Maciej