Re: [RFC] GDB patches for hw watchpoints - revised

[Daniel Jacobowitz <drow at false dot org>]

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  <bje@au1.ibm.com>
> 	    Wu Zhou  <woodzltc@cn.ibm.com>
> 
> 	* 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