From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 23692 invoked by alias); 30 May 2013 18:06:31 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 23679 invoked by uid 89); 30 May 2013 18:06:30 -0000 X-Spam-SWARE-Status: No, score=-8.1 required=5.0 tests=AWL,BAYES_00,KHOP_THREADED,RCVD_IN_HOSTKARMA_W,RCVD_IN_HOSTKARMA_WL,RP_MATCHES_RCVD,SPF_HELO_PASS,SPF_PASS autolearn=ham version=3.3.1 Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.84/v0.84-167-ge50287c) with ESMTP; Thu, 30 May 2013 18:06:27 +0000 Received: from int-mx12.intmail.prod.int.phx2.redhat.com (int-mx12.intmail.prod.int.phx2.redhat.com [10.5.11.25]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r4UI6PoG025912 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Thu, 30 May 2013 14:06:25 -0400 Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx12.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id r4UI6Nrd026118; Thu, 30 May 2013 14:06:24 -0400 Message-ID: <51A7951F.2030006@redhat.com> Date: Thu, 30 May 2013 18:06:00 -0000 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130311 Thunderbird/17.0.4 MIME-Version: 1.0 To: Yao Qi CC: gdb-patches@sourceware.org Subject: Re: [PATCH 0/3] mips hardware watchpoint support in gdbserver References: <1369881867-11372-1-git-send-email-yao@codesourcery.com> In-Reply-To: <1369881867-11372-1-git-send-email-yao@codesourcery.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit X-SW-Source: 2013-05/txt/msg01094.txt.bz2 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 > > 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 > #include > +#include "mips-linux-watch.h" > #include (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