Re: [PATCH 5/5] MIPS GDBserver watchpoint

Maciej W. Rozycki
2013-07-25T21:20:14Z

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