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

[Wu Zhou <woodzltc at cn dot ibm dot com>]

Hi Daniel,

On Sun, 22 Jan 2006, Daniel Jacobowitz wrote:

> Most of this looks good.  A couple bits don't though.

Thanks for reviewing this patch.  I will try to address your concerns.

> 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.

p630 is one kind of POWER4 based pSeriese server. It is currently the only 
available ppc machine I can get.  :-)

In fact, I am not sure before if the ppc arch has nonsteppable watchpoints 
or not. But testing on my p630 box, it did had nonsteppable ones.  Now 
that an architecture either have or doesn't have nonsteppable watchpoints, 
can we get from this test a result that ppc architecture has nonsteppable 
watchpoints?

If so, maybe I can just remove the stupid conditional statement below.  
(my original intention is to verify that v->mach equals bfd_mach_ppc_630 :-)

> 
> > +   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.

OK.  
 
> 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.

Yes.  You are right.  Will update this with a new patch.
  
> > +   /* 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.  */

Sorry for the bad english.  :-)

Will update this with the new patch.

> 
> > + 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.

Function to_region_ok_for_hw_watchpoint is not in the current target 
vector (I means struct target_ops).  Maybe we can add it into
target_ops? There are a few other archs also use this.  But they had to 
include it in nm-xxx-yyy.h.  If not, the only method I can think of is 
also include its definition in nm-ppc64-linux.h.  So what about the 
following patch section?

      int (*to_region_size_ok_for_hw_watchpoint) (int);
+     int (*to_region_ok_for_hw_watchpoint) (CORE_ADDR *, int);
      void (*to_terminal_init) (void);

> That would let you remove some failure cases from
> target_insert_watchpoint.

You said it.

> Also, please remove the commented out version of
> ppc_linux_stopped_data_address.

OK. I will do this in a new patch a while later.

Regards
- Wu Zhou