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

[Wu Zhou <woodzltc@cn.ibm.com>]

Daniel,

I committed the two patches you approved.  Here is a seperate patch for 
gdbint.texinfo:

2006-02-08  Wu Zhou  <woodzltc@cn.ibm.com>

	* gdbint.texinfo (TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT): Delete.

Index: gdbint.texinfo
===================================================================
RCS file: /cvs/src/src/gdb/doc/gdbint.texinfo,v
retrieving revision 1.238
diff -c -p -r1.238 gdbint.texinfo
*** gdbint.texinfo	6 Feb 2006 22:14:31 -0000	1.238
--- gdbint.texinfo	9 Feb 2006 05:32:27 -0000
*************** the same time).
*** 465,477 ****
  Return non-zero if hardware watchpoints can be used to watch a region
  whose address is @var{addr} and whose length in bytes is @var{len}.
  
- @findex TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT
- @item TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT (@var{size})
- Return non-zero if hardware watchpoints can be used to watch a region
- whose size is @var{size}.  @value{GDBN} only uses this macro as a
- fall-back, in case @code{TARGET_REGION_OK_FOR_HW_WATCHPOINT} is not
- defined.
- 
  @cindex insert or remove hardware watchpoint
  @findex target_insert_watchpoint
  @findex target_remove_watchpoint
--- 465,470 ----


Besides this, I am also thinking of a little more cleanup work in 
config/i386/nm-i386.h: macro TARGET_REGION_OK_FOR_HW_WATCHPOINT
can be replaced by a target vector initialization in i386-nat.c or 
somewhere else.

Maybe some other macros can also be replaced, such as 
STOPPED_BY_WATCHPOINT, target_stopped_data_address(target, x) and so on...

What is your thought on this?

Regards
- Wu Zhou

On Wed, 8 Feb 2006, Wu Zhou wrote:

> Hi Daniel,
> 
> Sorry for the delayed reply.  I am just back from a vacation.
> 
> On Wed, 1 Feb 2006, Daniel Jacobowitz wrote:
> 
> > On Wed, Jan 25, 2006 at 04:34:14PM +0800, Wu Zhou wrote:
> > > I am also thinking of replace the macro 
> > > TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT (SIZE) with 
> > > TARGET_REGION_OK_FOR_HW_WATCHPOINT(ADDR, SIZE).  Thus the code will seem 
> > > to be more clean: we will only have one macro to see if the target region 
> > > is ok for watchpoint monitoring.  Following this way, function 
> > > default_region_ok_for_hw_watchpoint will also return back to its original 
> > > implementation.  What is your thought on this?
> > 
> > I'm not sure what you mean.  After these patches, the only reference to
> > TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT is in gdbint.texinfo (which I'd
> > appreciate if you fixed, in a separate patch - thanks in advance).
> > What are the two ways now?
> 
> What I mean is the second patch below, which you had said ok. :-) 
> That is the only way I can thought of at that time.  Now that you had said 
> ok, I don't need to find a second way.  :-)
> 
> BTW.  I will fix the reference to TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT 
> in gdbint.texinfo in a separate patch after commiting this.
> 
> > 
> > On Wed, Jan 25, 2006 at 11:18:49AM +0800, Wu Zhou wrote:
> > > OK to commit?
> > > 
> > > 2006-01-22  Ben Elliston  <bje@au1.ibm.com>
> > > 	    Wu Zhou  <woodzltc@cn.ibm.com>
> > > 
> > > 	* ppc-linux-nat.c (PTRACE_GET_DEBUGREG, PTRACE_SET_DEBUGREG,
> > > 	PTRACE_GETSIGINFO): Define.
> > > 	(last_stopped_data_address): New.
> > > 	(ppc_linux_check_watch_resources): New function.
> > > 	(ppc_linux_region_ok_for_hw_watchpoint): New function.
> > > 	(ppc_linux_insert_watchpoint): New function.
> > > 	(ppc_linux_remove_watchpoint): New function.
> > > 	(ppc_linux_stopped_data_address): New function.
> > > 	(ppc_linux_stopped_by_watchpoint): New function.
> > > 	(_initialize_ppc_linux_nat): Set the above hardware watchpoint
> > > 	related target vectors.
> > > 	* rs6000-tdep.c (rs6000_gdbarch_init): Set PPC architectures
> > > 	to have nonsteppable watchpoint.
> > > 	* target.c (default_region_ok_for_hw_watchpoint,
> > > 	debug_to_region_ok_for_hw_watchpoint): New prototypes.
> > > 	(update_current_target): Inherit to_region_ok_for_hw_watchpoint
> > > 	and set default to_region_ok_for_hw_watchpoint.
> > > 	(default_region_ok_for_hw_watchpoint): New function.
> > > 	(debug_to_region_ok_for_hw_watchpoint): New function.
> > > 	(setup_target_debug): Set to_region_ok_for_hw_watchpoint of 
> > > 	debug_target.
> > > 	* target.h (struct target_ops): Add a new target vector 
> > > 	to_region_ok_for_hw_watchpoint.
> > > 	(TARGET_REGION_OK_FOR_HW_WATCHPOINT): Define this if it is not
> > > 	defined anyplace else.
> > 
> > On Wed, Jan 25, 2006 at 04:34:14PM +0800, Wu Zhou wrote:
> > > 2006-01-25  Wu Zhou  <woodzltc@cn.ibm.com>
> > > 
> > > 	* breakpoint.c (TARGET_REGION_OK_FOR_HW_WATCHPOINT): Delete.
> > > 	* config/i386/nm-i386sol2.h (TARGET_REGION_OK_FOR_HW_WATCHPOINT): New.
> > > 	(TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT): Delete.
> > > 	* config/mips/nm-irix5.h (TARGET_REGION_OK_FOR_HW_WATCHPOINT): New.
> > > 	(TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT): Delete.
> > > 	* config/sparc/nm-sol2.h (TARGET_REGION_OK_FOR_HW_WATCHPOINT): New.
> > > 	(TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT): Delete.
> > > 	* inf-ttrace.c (inf_ttrace_region_ok_for_hw_watchpoint): New.
> > > 	(inf_ttrace_region_size_ok_for_hw_watchpoint): Delete.
> > > 	(inf_ttrace_target): Delete to_region_size_ok_for_hw_watchpoint and
> > > 	add to_region_ok_for_hw_watchpoint.
> > > 	* s390-nat.c (s390_region_size_ok_for_hw_watchpoint): Delete.
> > > 	(s390_region_ok_for_hw_watchpoint): New.
> > > 	(_initialize_s390_nat): Delete to_region_size_ok_for_hw_watchpoint
> > > 	and add to_region_ok_for_hw_watchpoint.
> > > 	* target.c (default_region_size_ok_for_hw_watchpoint, 
> > > 	debug_to_region_size_ok_for_hw_watchpoint): Delete prototype.
> > > 	(update_current_target): Delete to_region_size_ok_for_hw_watchpoint
> > > 	inheritance and default_region_size_ok_for_hw_watchpoint.
> > > 	(default_region_ok_for_hw_watchpoint): If len is less than or equal
> > > 	the length of void pointer, return ok.
> > > 	(default_region_size_ok_for_hw_watchpoint): Delete.
> > > 	(debug_to_region_size_ok_for_hw_watchpoint): Delete.
> > > 	(setup_target_debug): Delete to_region_size_ok_for_hw_watchpoint.
> > > 	* target.h (struct target_ops): Delete 
> > > 	to_region_size_ok_for_hw_watchpoint.
> > > 	(TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT): Delete.
> > 
> > These patches are both OK.  You might want to combine them - it's a
> > much smaller diff :-)  But it doesn't matter since you've already got
> > them separated out.
> 
> Thanks for reviewing that.  I would like to commit them one by one, thus I 
> don't need to think about how to re-describe them.  :-)
> 
> I also think that it is clearer to differentiate the purpose of these two 
> patches.  Combining them together seems a little confusing to me.
> 
> Best Regards
> - Wu Zhou