Re: [PATCH 0/3] mips hardware watchpoint support in gdbserver

[Pedro Alves <palves@redhat.com>]

On 05/30/2013 03:44 AM, Yao Qi wrote:
> Hi,
> This patch series is to post Jie and Dan's work to support mips hardware
> watchpoint in gdbserver.  The patch exists in the codesourcery tree for a
> while, when I rebase the patch for on top of FSF GDB trunk, I find
> that kernel header structs are defined in gdb while they are not defined
> in gdbserver because it includes asm/ptrace.h.  After some thought, it
> should be safe to include asm/ptrace.h in gdb to get rid of these local
> structs, so this is what patch 1/3 does.  It is a cleanup one.  See more
> explanations in the patch itself.
> 
> Then, I find there are some duplications between gdb and gdbserver on
> manipulating h/w watchpoints, so I merge the common part to
> common/mips-linux-watch.[h,c].  gdb and gdbserver supports h/w watchpoints
> for some targets, such as i386 and arm, but no one shares common code in
> common/ directory before.  Ulrich Weigand expressed the intention for
> sharing for arm h/w watchpoint support in gdbserver
> <http://sourceware.org/ml/gdb-patches/2011-09/msg00200.html>
> I am not sure upstream maintainers' opinion on this.  That is what
> patch 2/3 does.

I'd prefer we shared the whole target backend, but while that
doesn't happen, I'll take pieces.  :-)

> Finally, patch 3/3 is about the rest of gdbserver stuff for h/w watchpoint.
> 
> The patch series are tested on some mips boards with gdbserver, with a
> hack that force proc skip_hw_watchpoint_tests return false.
> Many fails are fixed and no regressions (note that there is a regression
> in gdb.base/watchpoint.exp, but it is caused by a previous internal
> error).  I also mange to run testsuite native mips gdb on a mips board,
> watchpoint related tests seem OK.  The whole testsuite is not run because
> of the very slow speed.  Is It OK?

I've skimmed the whole series, and it looked reasonable to me.
Specifically, it uses the modern update-registers-on-resume
watchpoint mechanisms in GDBserver, which is nice.

I'd be nice if mips_show_dr was shared too, and on GDBserver hooked
with the existing debug_hw_points / "monitor set debug-hw-points 1",
currently only used by x86.

I've noticed a couple unexplained odd placements for includes, like

>  #include <sgidefs.h>
>  #include <sys/ptrace.h>
> +#include "mips-linux-watch.h"
>  #include <asm/ptrace.h>

(there's another in GDBserver) and a couple formatting
violations (e.g., missing space after cast, spurious space in
ChangeLog).

Please consider the "generic" parts of the change approved.

I'll defer review/approval to Maciej.

-- 
Pedro Alves