Re: [PATCH 5/5] MIPS GDBserver watchpoint

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

On Thu, 25 Jul 2013, Yao Qi wrote:

> gdb/gdbserver:
> 
> 2013-07-25  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.

 You've lost an entry for mips_linux_watch_h:

	(mips_linux_watch_h): New variable.

should do.

> 	* 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.

 Please name entity types, e.g.: "New function.", "New variables.", etc.
as applicable.

> diff --git a/gdb/gdbserver/linux-mips-low.c b/gdb/gdbserver/linux-mips-low.c
> index 1010528..dd6ebaf 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'.  */
> +
> +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;
> +    }
> +
> +  gdb_assert_not_reached ("unhandled RSP breakpoint type");
> +  return -1;
> +}

 gdb_assert_not_reached is `__attribute__ ((noreturn))', so there's no 
need for the `return -1' statement.  Please remove it.

 OK with this change and the ChangeLog updates, thanks for your work.

  Maciej