[RFA] procfs.c: TARGET_CAN_USE_HARDWARE_WATCHPOINT via target vector

[RFA] procfs.c: TARGET_CAN_USE_HARDWARE_WATCHPOINT via target vector

[Kevin Buettner]

On Irix, it's possible to debug processes using the o32, n32, and n64
ABIs from a single gdb.  Unfortunately, due to some limitations with
the watchpoint support in procfs.c, it is not possible to use hardware
watchpoints in all cases.  (See patch comments for details.)

My first cut at a patch simply changed the macro in
config/mips/nm-irix5.h.  However, since this same solution will be
required for each platform requiring procfs.c, I decided to use the
newly added hardware breakpoint facilities in the target vector.

Okay to commit?

Kevin

	* procfs.c (procfs_can_use_hw_breakpoint): New function.
	(init_procfs_ops): Define ``to_can_use_hw_breakpoint'' for procfs
	target vector.
	* config/mips/nm-irix5.h (TARGET_CAN_USE_HARDWARE_WATCHPOINT):
	Delete.  Add comment regarding this now-deleted target method.

Index: procfs.c
===================================================================
RCS file: /cvs/src/src/gdb/procfs.c,v
retrieving revision 1.38
diff -u -p -r1.38 procfs.c
--- procfs.c	11 Jul 2002 13:50:49 -0000	1.38
+++ procfs.c	12 Aug 2002 21:18:34 -0000
@@ -136,6 +136,8 @@ static int proc_find_memory_regions (int
 
 static char * procfs_make_note_section (bfd *, int *);
 
+static int procfs_can_use_hw_breakpoint (int, int, int);
+
 struct target_ops procfs_ops;		/* the target vector */
 
 static void
@@ -183,6 +185,7 @@ init_procfs_ops (void)
   procfs_ops.to_has_thread_control  = tc_schedlock;
   procfs_ops.to_find_memory_regions = proc_find_memory_regions;
   procfs_ops.to_make_corefile_notes = procfs_make_note_section;
+  procfs_ops.to_can_use_hw_breakpoint = procfs_can_use_hw_breakpoint;
   procfs_ops.to_magic               = OPS_MAGIC;
 }
 
@@ -5136,6 +5139,37 @@ procfs_set_watchpoint (ptid_t ptid, CORE
 #endif /* AIX5 */
 #endif /* UNIXWARE */
   return 0;
+}
+
+/* Return non-zero if we can set a hardware watchpoint of type TYPE.  TYPE
+   is one of bp_hardware_watchpoint, bp_read_watchpoint, bp_write_watchpoint,
+   or bp_hardware_watchpoint.  CNT is the number of watchpoints used so
+   far.
+   
+   Note:  procfs_can_use_hw_breakpoint() is not yet used by all
+   procfs.c targets due to the fact that some of them still define
+   TARGET_CAN_USE_HARDWARE_WATCHPOINT.  */
+
+static int
+procfs_can_use_hw_breakpoint (int type, int cnt, int othertype)
+{
+#ifndef TARGET_HAS_HARDWARE_WATCHPOINTS
+  return 0;
+#else
+  /* Due to the way that proc_set_watchpoint() is implemented, host
+     and target pointers must be of the same size.  If they are not,
+     we can't use hardware watchpoints.  This limitation is due to the
+     fact that proc_set_watchpoint() calls address_to_host_pointer();
+     a close inspection of address_to_host_pointer will reveal that
+     an internal error will be generated when the host and target
+     pointer sizes are different.  */
+  if (sizeof (void *) != TYPE_LENGTH (builtin_type_void_data_ptr))
+    return 0;
+
+  /* Other tests here???  */
+
+  return 1;
+#endif
 }
 
 /*
Index: config/mips/nm-irix5.h
===================================================================
RCS file: /cvs/src/src/gdb/config/mips/nm-irix5.h,v
retrieving revision 1.5
diff -u -p -r1.5 nm-irix5.h
--- config/mips/nm-irix5.h	5 Jun 2002 19:18:24 -0000	1.5
+++ config/mips/nm-irix5.h	12 Aug 2002 21:18:34 -0000
@@ -24,7 +24,9 @@
 
 #define TARGET_HAS_HARDWARE_WATCHPOINTS
 
-#define TARGET_CAN_USE_HARDWARE_WATCHPOINT(type, cnt, ot) 1
+/* TARGET_CAN_USE_HARDWARE_WATCHPOINT is now defined to go through
+   the target vector.  For Irix5, procfs_can_use_hw_watchpoint()
+   should be invoked.  */
 
 /* When a hardware watchpoint fires off the PC will be left at the
    instruction which caused the watchpoint.  It will be necessary for

Re: [RFA] procfs.c: TARGET_CAN_USE_HARDWARE_WATCHPOINT via target vector

[Michael Snyder]

Kevin Buettner wrote:
> 
> On Irix, it's possible to debug processes using the o32, n32, and n64
> ABIs from a single gdb.  Unfortunately, due to some limitations with
> the watchpoint support in procfs.c, it is not possible to use hardware
> watchpoints in all cases.  (See patch comments for details.)
> 
> My first cut at a patch simply changed the macro in
> config/mips/nm-irix5.h.  However, since this same solution will be
> required for each platform requiring procfs.c, I decided to use the
> newly added hardware breakpoint facilities in the target vector.
> 
> Okay to commit?

Sure -- but don't you think it should default to zero?
Or is being able to do it the norm?  (I don't remember).

> 
> Kevin
> 
>         * procfs.c (procfs_can_use_hw_breakpoint): New function.
>         (init_procfs_ops): Define ``to_can_use_hw_breakpoint'' for procfs
>         target vector.
>         * config/mips/nm-irix5.h (TARGET_CAN_USE_HARDWARE_WATCHPOINT):
>         Delete.  Add comment regarding this now-deleted target method.
> 
> Index: procfs.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/procfs.c,v
> retrieving revision 1.38
> diff -u -p -r1.38 procfs.c
> --- procfs.c    11 Jul 2002 13:50:49 -0000      1.38
> +++ procfs.c    12 Aug 2002 21:18:34 -0000
> @@ -136,6 +136,8 @@ static int proc_find_memory_regions (int
> 
>  static char * procfs_make_note_section (bfd *, int *);
> 
> +static int procfs_can_use_hw_breakpoint (int, int, int);
> +
>  struct target_ops procfs_ops;          /* the target vector */
> 
>  static void
> @@ -183,6 +185,7 @@ init_procfs_ops (void)
>    procfs_ops.to_has_thread_control  = tc_schedlock;
>    procfs_ops.to_find_memory_regions = proc_find_memory_regions;
>    procfs_ops.to_make_corefile_notes = procfs_make_note_section;
> +  procfs_ops.to_can_use_hw_breakpoint = procfs_can_use_hw_breakpoint;
>    procfs_ops.to_magic               = OPS_MAGIC;
>  }
> 
> @@ -5136,6 +5139,37 @@ procfs_set_watchpoint (ptid_t ptid, CORE
>  #endif /* AIX5 */
>  #endif /* UNIXWARE */
>    return 0;
> +}
> +
> +/* Return non-zero if we can set a hardware watchpoint of type TYPE.  TYPE
> +   is one of bp_hardware_watchpoint, bp_read_watchpoint, bp_write_watchpoint,
> +   or bp_hardware_watchpoint.  CNT is the number of watchpoints used so
> +   far.
> +
> +   Note:  procfs_can_use_hw_breakpoint() is not yet used by all
> +   procfs.c targets due to the fact that some of them still define
> +   TARGET_CAN_USE_HARDWARE_WATCHPOINT.  */
> +
> +static int
> +procfs_can_use_hw_breakpoint (int type, int cnt, int othertype)
> +{
> +#ifndef TARGET_HAS_HARDWARE_WATCHPOINTS
> +  return 0;
> +#else
> +  /* Due to the way that proc_set_watchpoint() is implemented, host
> +     and target pointers must be of the same size.  If they are not,
> +     we can't use hardware watchpoints.  This limitation is due to the
> +     fact that proc_set_watchpoint() calls address_to_host_pointer();
> +     a close inspection of address_to_host_pointer will reveal that
> +     an internal error will be generated when the host and target
> +     pointer sizes are different.  */
> +  if (sizeof (void *) != TYPE_LENGTH (builtin_type_void_data_ptr))
> +    return 0;
> +
> +  /* Other tests here???  */
> +
> +  return 1;
> +#endif
>  }
> 
>  /*
> Index: config/mips/nm-irix5.h
> ===================================================================
> RCS file: /cvs/src/src/gdb/config/mips/nm-irix5.h,v
> retrieving revision 1.5
> diff -u -p -r1.5 nm-irix5.h
> --- config/mips/nm-irix5.h      5 Jun 2002 19:18:24 -0000       1.5
> +++ config/mips/nm-irix5.h      12 Aug 2002 21:18:34 -0000
> @@ -24,7 +24,9 @@
> 
>  #define TARGET_HAS_HARDWARE_WATCHPOINTS
> 
> -#define TARGET_CAN_USE_HARDWARE_WATCHPOINT(type, cnt, ot) 1
> +/* TARGET_CAN_USE_HARDWARE_WATCHPOINT is now defined to go through
> +   the target vector.  For Irix5, procfs_can_use_hw_watchpoint()
> +   should be invoked.  */
> 
>  /* When a hardware watchpoint fires off the PC will be left at the
>     instruction which caused the watchpoint.  It will be necessary for

Re: [RFA] procfs.c: TARGET_CAN_USE_HARDWARE_WATCHPOINT via target vector

[Kevin Buettner]

On Aug 12,  5:21pm, Michael Snyder wrote:

> Kevin Buettner wrote:
> > 
> > On Irix, it's possible to debug processes using the o32, n32, and n64
> > ABIs from a single gdb.  Unfortunately, due to some limitations with
> > the watchpoint support in procfs.c, it is not possible to use hardware
> > watchpoints in all cases.  (See patch comments for details.)
> > 
> > My first cut at a patch simply changed the macro in
> > config/mips/nm-irix5.h.  However, since this same solution will be
> > required for each platform requiring procfs.c, I decided to use the
> > newly added hardware breakpoint facilities in the target vector.
> > 
> > Okay to commit?
> 
> Sure -- but don't you think it should default to zero?
> Or is being able to do it the norm?  (I don't remember).

Here's the part of the patch to look at:

> > +/* Return non-zero if we can set a hardware watchpoint of type TYPE.  TYPE
> > +   is one of bp_hardware_watchpoint, bp_read_watchpoint, bp_write_watchpoint,
> > +   or bp_hardware_watchpoint.  CNT is the number of watchpoints used so
> > +   far.
> > +
> > +   Note:  procfs_can_use_hw_breakpoint() is not yet used by all
> > +   procfs.c targets due to the fact that some of them still define
> > +   TARGET_CAN_USE_HARDWARE_WATCHPOINT.  */
> > +
> > +static int
> > +procfs_can_use_hw_breakpoint (int type, int cnt, int othertype)
> > +{
> > +#ifndef TARGET_HAS_HARDWARE_WATCHPOINTS
> > +  return 0;
> > +#else
> > +  /* Due to the way that proc_set_watchpoint() is implemented, host
> > +     and target pointers must be of the same size.  If they are not,
> > +     we can't use hardware watchpoints.  This limitation is due to the
> > +     fact that proc_set_watchpoint() calls address_to_host_pointer();
> > +     a close inspection of address_to_host_pointer will reveal that
> > +     an internal error will be generated when the host and target
> > +     pointer sizes are different.  */
> > +  if (sizeof (void *) != TYPE_LENGTH (builtin_type_void_data_ptr))
> > +    return 0;
> > +
> > +  /* Other tests here???  */
> > +
> > +  return 1;
> > +#endif
> >  }

So...

It returns zero for targets without hardware watchpoint support.

It returns 1 for targets with hardware watchpoint support,...

UNLESS the host and target pointer sizes don't match, in which case it
returns 0.

I think the above is what procfs.c targets do for 
TARGET_CAN_USE_HARDWARE_WATCHPOINT with the exception of the "UNLESS"
clause which is new.

BTW, it seems to me that we should have some other tests (for type,
size, number of existing hardware watchpoints, etc) in the "Other
tests here???" section, but I don't really know what these tests
should be yet.  Also, none of the other procfs.c targets currently
seem to care about this, so the functionality will be the same as
what we currently have (right or wrong).

Thanks for the quick reply.

Kevin

Re: [RFA] procfs.c: TARGET_CAN_USE_HARDWARE_WATCHPOINT via target vector

[Michael Snyder]

Kevin Buettner wrote:
> 
> On Aug 12,  5:21pm, Michael Snyder wrote:
> 
> > Kevin Buettner wrote:
> > >
> > > On Irix, it's possible to debug processes using the o32, n32, and n64
> > > ABIs from a single gdb.  Unfortunately, due to some limitations with
> > > the watchpoint support in procfs.c, it is not possible to use hardware
> > > watchpoints in all cases.  (See patch comments for details.)
> > >
> > > My first cut at a patch simply changed the macro in
> > > config/mips/nm-irix5.h.  However, since this same solution will be
> > > required for each platform requiring procfs.c, I decided to use the
> > > newly added hardware breakpoint facilities in the target vector.
> > >
> > > Okay to commit?
> >
> > Sure -- but don't you think it should default to zero?
> > Or is being able to do it the norm?  (I don't remember).
> 
> Here's the part of the patch to look at:
> 
> > > +/* Return non-zero if we can set a hardware watchpoint of type TYPE.  TYPE
> > > +   is one of bp_hardware_watchpoint, bp_read_watchpoint, bp_write_watchpoint,
> > > +   or bp_hardware_watchpoint.  CNT is the number of watchpoints used so
> > > +   far.
> > > +
> > > +   Note:  procfs_can_use_hw_breakpoint() is not yet used by all
> > > +   procfs.c targets due to the fact that some of them still define
> > > +   TARGET_CAN_USE_HARDWARE_WATCHPOINT.  */
> > > +
> > > +static int
> > > +procfs_can_use_hw_breakpoint (int type, int cnt, int othertype)
> > > +{
> > > +#ifndef TARGET_HAS_HARDWARE_WATCHPOINTS
> > > +  return 0;
> > > +#else
> > > +  /* Due to the way that proc_set_watchpoint() is implemented, host
> > > +     and target pointers must be of the same size.  If they are not,
> > > +     we can't use hardware watchpoints.  This limitation is due to the
> > > +     fact that proc_set_watchpoint() calls address_to_host_pointer();
> > > +     a close inspection of address_to_host_pointer will reveal that
> > > +     an internal error will be generated when the host and target
> > > +     pointer sizes are different.  */
> > > +  if (sizeof (void *) != TYPE_LENGTH (builtin_type_void_data_ptr))
> > > +    return 0;
> > > +
> > > +  /* Other tests here???  */
> > > +
> > > +  return 1;
> > > +#endif
> > >  }
> 
> So...
> 
> It returns zero for targets without hardware watchpoint support.
> 
> It returns 1 for targets with hardware watchpoint support,...
> 
> UNLESS the host and target pointer sizes don't match, in which case it
> returns 0.

Oh yeah.  Sorry, I missed that.

> I think the above is what procfs.c targets do for
> TARGET_CAN_USE_HARDWARE_WATCHPOINT with the exception of the "UNLESS"
> clause which is new.
> 
> BTW, it seems to me that we should have some other tests (for type,
> size, number of existing hardware watchpoints, etc) in the "Other
> tests here???" section, but I don't really know what these tests
> should be yet.  Also, none of the other procfs.c targets currently
> seem to care about this, so the functionality will be the same as
> what we currently have (right or wrong).

Yeah, if you take a survey, I think you'll find that virtually no
existing targets test for things like whether we've used up our
available watchpoint registers.  They mostly either always say
true, or always say false.

Re: [RFA] procfs.c: TARGET_CAN_USE_HARDWARE_WATCHPOINT via target vector

[Andrew Cagney]

> +#ifndef TARGET_HAS_HARDWARE_WATCHPOINTS

I just think the macro should be `largely' renamed: 
``PROCFS_HAS_HARDWARE_WATCHPOINTS''.

Largely?  There are a few exceptions -- i386v-nat.c and mips/tm-embed.h 
which could be left alone.

enjoy,
Andrew

Re: [RFA] procfs.c: TARGET_CAN_USE_HARDWARE_WATCHPOINT via target vector

[Kevin Buettner]

On Aug 13,  5:11pm, Andrew Cagney wrote:

> > +#ifndef TARGET_HAS_HARDWARE_WATCHPOINTS
> 
> I just think the macro should be `largely' renamed: 
> ``PROCFS_HAS_HARDWARE_WATCHPOINTS''.
> 
> Largely?  There are a few exceptions -- i386v-nat.c and mips/tm-embed.h 
> which could be left alone.

For the procfs related uses, I suspect that we can eliminate the use
of the macro altogether.  (I'd prefer to get Michael's opinion though
before attempting such a change.)

Kevin

Re: [RFA] procfs.c: TARGET_CAN_USE_HARDWARE_WATCHPOINT via target vector

[Andrew Cagney]

> On Aug 13,  5:11pm, Andrew Cagney wrote:
> 
> 
>> > +#ifndef TARGET_HAS_HARDWARE_WATCHPOINTS
> 
>> 
>> I just think the macro should be `largely' renamed: 
>> ``PROCFS_HAS_HARDWARE_WATCHPOINTS''.
>> 
>> Largely?  There are a few exceptions -- i386v-nat.c and mips/tm-embed.h 
>> which could be left alone.
> 
> 
> For the procfs related uses, I suspect that we can eliminate the use
> of the macro altogether.  (I'd prefer to get Michael's opinion though
> before attempting such a change.)

That would be even better!   I'm just trying to ensure that someone 
doesn't try to sneak in a reference to ..._HAS_HARDWARE_WATCHPOINTS 
where they shouldn't (ie. outside of procfs.c :-).

enjoy,
Andrew

Re: [RFA] procfs.c: TARGET_CAN_USE_HARDWARE_WATCHPOINT via target vector

[Michael Snyder]

Kevin Buettner wrote:
> 
> On Aug 13,  5:11pm, Andrew Cagney wrote:
> 
> > > +#ifndef TARGET_HAS_HARDWARE_WATCHPOINTS
> >
> > I just think the macro should be `largely' renamed:
> > ``PROCFS_HAS_HARDWARE_WATCHPOINTS''.
> >
> > Largely?  There are a few exceptions -- i386v-nat.c and mips/tm-embed.h
> > which could be left alone.
> 
> For the procfs related uses, I suspect that we can eliminate the use
> of the macro altogether.  (I'd prefer to get Michael's opinion though
> before attempting such a change.)

I don't see how you can eliminate the macro.
How will you distinguish which procfs targets can support HW
watchpoints?