Re: [PATCH 5/5] MIPS GDBserver watchpoint

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

On Sat, 29 Jun 2013, Yao Qi wrote:

> gdb/gdbserver:
> 
> 2013-06-29  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_h): New.
> 	(mips-linux-watch.o): New rule.
> 	* configure.srv <mips*-*-linux*>: Add mips-linux-watch.o to
> 	srv_tgtobj.
> 	* linux-mips-low.c: Include mips-linux-watch.h.
> 	(struct arch_process_info, struct arch_lwp_info): New.
> 	(update_watch_registers_callback): New.
> 	(mips_linux_new_process, mips_linux_new_thread) New.
> 	(mips_linux_prepare_to_resume, mips_insert_point): New.
> 	(mips_remove_point, mips_stopped_by_watchpoint): New.
> 	(rsp_bp_type_to_target_hw_bp_type): New.
> 	(mips_stopped_data_address): New.
> 	(the_low_target): Add watchpoint support functions.
> 
> gdb:
> 
> 2013-06-29  Yao Qi  <yao@codesourcery.com>
> 
> 	* NEWS: Mention it.
> ---
>  gdb/NEWS                       |    3 +
>  gdb/gdbserver/Makefile.in      |    7 +-
>  gdb/gdbserver/configure.srv    |    1 +
>  gdb/gdbserver/linux-mips-low.c |  366 ++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 376 insertions(+), 1 deletions(-)
> 
> diff --git a/gdb/gdbserver/linux-mips-low.c b/gdb/gdbserver/linux-mips-low.c
> index 1010528..6e6e3e0 100644
> --- a/gdb/gdbserver/linux-mips-low.c
> +++ b/gdb/gdbserver/linux-mips-low.c
> @@ -257,6 +291,328 @@ mips_breakpoint_at (CORE_ADDR where)
>    return 0;
>  }
>  
> +/* Mark the watch registers of lwp, represented by ENTRY, as changed,
> +   if the lwp's process id is *PID_P.  */
> +
> +static int
> +update_watch_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->watch_registers_changed = 1;
> +
> +      /* If the lwp isn't stopped, force it to momentarily pause, so
> +	 we can update its watch 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 watch registers as changed, so the threads' copies will
> +   be updated.  */
> +
> +static struct arch_lwp_info *
> +mips_linux_new_thread (void)
> +{
> +  struct arch_lwp_info *info = xcalloc (1, sizeof (*info));
> +
> +  info->watch_registers_changed = 1;
> +
> +  return info;
> +}
> +
> +/* This is the implementation of linux_target_ops method
> +   prepare_to_resume.  If the watch 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->watch_registers_changed)
> +    {
> +      /* Only update the watch 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 (-1 == ptrace (PTRACE_SET_WATCH_REGS, tid,
> +			    &private->watch_mirror))
> +	    perror_with_name ("Couldn't write watch register");
> +	}
> +
> +      lwp->arch_private->watch_registers_changed = 0;
> +    }
> +}
> +
> +/* Translate breakpoint type TYPE in rsp to 'enum target_hw_bp_type'.  Return
> +   -1 if translation failed.  */
> +
> +static enum target_hw_bp_type
> +rsp_bp_type_to_target_hw_bp_type (char type)
> +{
> +  switch (type)
> +    {
> +    case '2':
> +      return hw_write;
> +    case '3':
> +      return hw_read;
> +    case '4':
> +      return hw_access;
> +    }
> +
> +  return -1;

 This looks unclean to me, you're returning a value that's outside the 
valid range of the enum type used.  By the structure of the callers this 
code is expected to be dead though, so please use gdb_assert_not_reached.

 Thanks for going ahead and adding rsp_bp_type_to_target_hw_bp_type, 
however in the future please try to name in the patch description any 
changes you've made (non-trivial ones that is, obvious typo fixes, 
formatting adjustments, etc. need no mention, although there'll be no harm 
from making an overall statement that you've made such clean-ups) beyond 
ones requested in the review.  This will make the review process easier 
for me as otherwise I may assume proposed changes I have already accepted 
will have remained the same in the updated patch.

 Please resend with the adjustment requested, the change looks otherwise 
OK to me.

  Maciej