From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 10413 invoked by alias); 22 Jan 2006 20:56:46 -0000 Received: (qmail 10405 invoked by uid 22791); 22 Jan 2006 20:56:45 -0000 X-Spam-Check-By: sourceware.org Received: from nevyn.them.org (HELO nevyn.them.org) (66.93.172.17) by sourceware.org (qpsmtpd/0.31.1) with ESMTP; Sun, 22 Jan 2006 20:56:44 +0000 Received: from drow by nevyn.them.org with local (Exim 4.54) id 1F0mGP-0007kn-8S; Sun, 22 Jan 2006 15:56:41 -0500 Date: Sun, 22 Jan 2006 20:56:00 -0000 From: Daniel Jacobowitz To: Wu Zhou Cc: Eli Zaretskii , gdb-patches@sources.redhat.com, mark@xs4all.nl, bje@au1.ibm.com, anton@au1.ibm.com Subject: Re: [RFC] GDB patches for hw watchpoints - revised Message-ID: <20060122205641.GF27224@nevyn.them.org> Mail-Followup-To: Wu Zhou , Eli Zaretskii , gdb-patches@sources.redhat.com, mark@xs4all.nl, bje@au1.ibm.com, anton@au1.ibm.com References: Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.8i X-IsSubscribed: yes Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2006-01/txt/msg00308.txt.bz2 Most of this looks good. A couple bits don't though. On Thu, Dec 22, 2005 at 12:47:18PM +0800, Wu Zhou wrote: > 2005-12-22 Ben Elliston > Wu Zhou > > * rs6000-tdep.c (rs6000_gdbarch_init): If the macn is p630, set > gdbarch to have nonsteppable watchpoint. First, please don't abbreviate in changelogs. Second, this code doesn't make sense. It sounds like you've only tested on p630, whatever that is, which is fine - but watchpoints have nothing to do with bfd_mach_ppc_p630. Either the architecture has nonsteppable watchpoints, or it doesn't. > + if (bfd_mach_ppc_630) > + set_gdbarch_have_nonsteppable_watchpoint (gdbarch, 1); BTW: ../bfd/bfd-in2.h:#define bfd_mach_ppc_630 630 So I don't think this is testing what you wanted to, anyway :-) > * ppc-linux-nat.c: Define three macro: PTRACE_GET_DEBUGREG, > PTRACE_SET_DEBUGREG and PTRACE_GETSIGINFO. Define one static > variable last_stopped_data_address. Please use: (PTRACE_GET_DEBUGREG, PTRACE_SET_DEBUGREG, PTRACE_GETSIGINFO): Define. (last_stopped_data_address): New. Can all the new functions in ppc-linux-nat.c be static? > + /* DABR (data address breakpoint register) is optional for PPC variations. > + Some variation have one DABR, others have none. So CNT can't be larger > + than 1. */ I believe you want "variants" in both places. > + /* We need to know whether ptrace syscall support PTRACE_SET_DEBUGREG and > + whether the ppc arch have DABR. If either answer is no, the ptrace call > + will return -1. Return 0 for that. */ /* We need to know whether ptrace supports PTRACE_SET_DEBUGREG and whether the target has DABR. If either answer is no, the ptrace call will return -1. Fail in that case. */ > + static int > + ppc_linux_region_size_ok_for_hw_watchpoint (int cnt) > + { > + return 1; > + } The argument is LEN, not CNT. It would be nice to do a useful check here; I think that to do that, you'd need to move TARGET_REGION_OK_FOR_HW_WATCHPOINT into the target vector. You could probably replace TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT and have the current implementations ignore the address. That would let you remove some failure cases from target_insert_watchpoint. Also, please remove the commented out version of ppc_linux_stopped_data_address. -- Daniel Jacobowitz CodeSourcery