[RFC] Unified watchpoints for x86 platforms

[RFC] Unified watchpoints for x86 platforms

[Eli Zaretskii]

[Yes, I know: it's been a looong time since I promised to make this
happen.  Still, it's better late than never ;-)]

I started working on the unified support for hardware-assisted
breakpoints and watchpoints on x86 platforms (see TODO).  Since I
don't feel I know enough about all the aspects of this on any platform
but DJGPP, I thought I'd better get the framework agreed to before I
start coding.

Here's the API I suggest for use by higher-level GDB code:

  (Note: I'm not good at inventing names, so please suggest better
  ones if you want.)

  int i386_hwbp_insert (int pid, CORE_ADDR addr, int len, int kind);

  This function inserts a breakpoint or watchpoint to watch memory
  region starting at address ADDR whose length is LEN bytes.  The
  watchpoint will watch said memory region for accesses whose type
  is defined by KIND:

    HW_READ	     break if the region is accessed for reading[1]
    HW_WRITE	     break if the region is accessed for writing
    HW_ACCESS	     break if the region is accessed for either
		     reading or writing
    HW_IO_ACCESS     same as HW_ACCESS type, but for I/O read/write
		     access[2]
    HW_EXECUTE       instruction execution breakpoint

  The function returns 0 upon success, else -1.

  Notes:
  [1] Since x86 doesn't support read data watchpoints, HW_READ will
      actually be implemented as a read/write watchpoint, and relies
      on higher-level GDB code to distinguish between reads and
      writes.  The infrastructure to support this is already in place
      in breakpoint.c, since GDB 5.0.

  [2] I/O watchpoints are not currently supported (AFAIK) by GDB on
      any x86 platform.  I can provide the code to handle it, but do
      people think we should define a command to access this feature?
      If so, should we provide separate read, write, and access types
      of watchpoints, or a single access type (the only one supported
      by x86's debug registers) is enough?

      Note that I/O watchpoints require that the DE (debug extensions)
      flag in the CR4 register be set.  I don't know what platforms
      set it and under what circumstances.

  int i386_hwbp_remove (int pid, CORE_ADDR addr, int len, int kind);

  This function removes a breakpoint of watchpoint at address ADDR
  which watches a memory region of LEN bytes and whose type is given
  by KIND.  It returns 0 upon success, else -1.

  int i386_hwbp_region_ok (CORE_ADDR addr, int len);

  This function tests whether a memory region of LEN bytes starting at
  ADDR can be watched with debug registers.  It returns 1 if the
  region can be watched, 0 otherwise.

  int i386_hwbp_stopped_by_watchpoint (int pid);

  This function returns the address of a breakpoint or watchpoint
  which could have stopped the debuggee.  If no watchpoint triggered,
  it returns 0.

To actually insert and remove breakpoints and watchpoints, I need
low-level system-dependent functions.  Here's the API I suggest for
this low-levwl layer.  (These are macros so that targets could define
them on their nm-*.h files.  On a typical Unix or GNU/Linux system,
each of these macros will call `ptrace' with suitable arguments.)

  void HWBP_SET_ADDR (int pid, int dr_num, CORE_ADDR addr);

  This macro sets the debug register DR_NUM in the inferior to watch
  the address ADDR.  DR_NUM can be in the range [0..3].

  void HWBP_SET_CONTROL (int pid, unsigned int val);

  This macro sets the DR7 debug control register in the inferior to
  the value VAL.

  unsigned int HWBP_GET_STATUS (int pid);

  This macro returns the value of the DR6 debug status register from
  the inferior.

  In the discussion we had back in September, Mark said that the
  status register should be per thread.  Does that mean that we need
  an additional argument (int tid?) to pass to HWBP_GET_STATUS?  If
  so, how will this argument get into the i386_hwbp_* functions which
  will call these macros?

  Or maybe the target end can figure out the thread id by itself with
  some TIDGET(pid) magic?


Comments?  Suggestions?  Flames?

Re: [RFC] Unified watchpoints for x86 platforms

[H . J . Lu]

On Sat, Feb 10, 2001 at 10:33:52AM -0500, Eli Zaretskii wrote:
>   unsigned int HWBP_GET_STATUS (int pid);
> 
>   This macro returns the value of the DR6 debug status register from
>   the inferior.
> 
>   In the discussion we had back in September, Mark said that the
>   status register should be per thread.  Does that mean that we need
>   an additional argument (int tid?) to pass to HWBP_GET_STATUS?  If
>   so, how will this argument get into the i386_hwbp_* functions which
>   will call these macros?

What is the difference between pid and tid in this case? Can we derive
tid from pid?

> 
>   Or maybe the target end can figure out the thread id by itself with
>   some TIDGET(pid) magic?
> 
> 
> Comments?  Suggestions?  Flames?

I haven't looked at it in detail. I guess Linux can live with the one
everyone agrees up on. If it turns out it is not true, I hope we can
still change it :-(. Unfortunately, I don't have the time to try it
out now. However, I will give it a try when the OS independent part is
checked in and noone is working on Linux.

Thanks.


-- 
H.J. Lu (hjl@valinux.com)

Re: [RFC] Unified watchpoints for x86 platforms

[Eli Zaretskii]

> Date: Sat, 10 Feb 2001 10:18:37 -0800
> From: "H . J . Lu" <hjl@valinux.com>
> 
> On Sat, Feb 10, 2001 at 10:33:52AM -0500, Eli Zaretskii wrote:
> >   unsigned int HWBP_GET_STATUS (int pid);
> > 
> >   This macro returns the value of the DR6 debug status register from
> >   the inferior.
> > 
> >   In the discussion we had back in September, Mark said that the
> >   status register should be per thread.  Does that mean that we need
> >   an additional argument (int tid?) to pass to HWBP_GET_STATUS?  If
> >   so, how will this argument get into the i386_hwbp_* functions which
> >   will call these macros?
> 
> What is the difference between pid and tid in this case? Can we derive
> tid from pid?

That's what I'd like to know as well.  Anyone?

Re: [RFC] Unified watchpoints for x86 platforms

[Eli Zaretskii]

Ping!

No one posted any approvals or disprovals of this design.  Do I take
the silence as a sign of agreement and start coding?
 
> I started working on the unified support for hardware-assisted
> breakpoints and watchpoints on x86 platforms (see TODO).  Since I
> don't feel I know enough about all the aspects of this on any platform
> but DJGPP, I thought I'd better get the framework agreed to before I
> start coding.
> 
> Here's the API I suggest for use by higher-level GDB code:
> 
>   (Note: I'm not good at inventing names, so please suggest better
>   ones if you want.)
> 
>   int i386_hwbp_insert (int pid, CORE_ADDR addr, int len, int kind);
> 
>   This function inserts a breakpoint or watchpoint to watch memory
>   region starting at address ADDR whose length is LEN bytes.  The
>   watchpoint will watch said memory region for accesses whose type
>   is defined by KIND:
> 
>     HW_READ	     break if the region is accessed for reading[1]
>     HW_WRITE	     break if the region is accessed for writing
>     HW_ACCESS	     break if the region is accessed for either
> 		     reading or writing
>     HW_IO_ACCESS     same as HW_ACCESS type, but for I/O read/write
> 		     access[2]
>     HW_EXECUTE       instruction execution breakpoint
> 
>   The function returns 0 upon success, else -1.
> 
>   Notes:
>   [1] Since x86 doesn't support read data watchpoints, HW_READ will
>       actually be implemented as a read/write watchpoint, and relies
>       on higher-level GDB code to distinguish between reads and
>       writes.  The infrastructure to support this is already in place
>       in breakpoint.c, since GDB 5.0.
> 
>   [2] I/O watchpoints are not currently supported (AFAIK) by GDB on
>       any x86 platform.  I can provide the code to handle it, but do
>       people think we should define a command to access this feature?
>       If so, should we provide separate read, write, and access types
>       of watchpoints, or a single access type (the only one supported
>       by x86's debug registers) is enough?
> 
>       Note that I/O watchpoints require that the DE (debug extensions)
>       flag in the CR4 register be set.  I don't know what platforms
>       set it and under what circumstances.
> 
>   int i386_hwbp_remove (int pid, CORE_ADDR addr, int len, int kind);
> 
>   This function removes a breakpoint of watchpoint at address ADDR
>   which watches a memory region of LEN bytes and whose type is given
>   by KIND.  It returns 0 upon success, else -1.
> 
>   int i386_hwbp_region_ok (CORE_ADDR addr, int len);
> 
>   This function tests whether a memory region of LEN bytes starting at
>   ADDR can be watched with debug registers.  It returns 1 if the
>   region can be watched, 0 otherwise.
> 
>   int i386_hwbp_stopped_by_watchpoint (int pid);
> 
>   This function returns the address of a breakpoint or watchpoint
>   which could have stopped the debuggee.  If no watchpoint triggered,
>   it returns 0.
> 
> To actually insert and remove breakpoints and watchpoints, I need
> low-level system-dependent functions.  Here's the API I suggest for
> this low-levwl layer.  (These are macros so that targets could define
> them on their nm-*.h files.  On a typical Unix or GNU/Linux system,
> each of these macros will call `ptrace' with suitable arguments.)
> 
>   void HWBP_SET_ADDR (int pid, int dr_num, CORE_ADDR addr);
> 
>   This macro sets the debug register DR_NUM in the inferior to watch
>   the address ADDR.  DR_NUM can be in the range [0..3].
> 
>   void HWBP_SET_CONTROL (int pid, unsigned int val);
> 
>   This macro sets the DR7 debug control register in the inferior to
>   the value VAL.
> 
>   unsigned int HWBP_GET_STATUS (int pid);
> 
>   This macro returns the value of the DR6 debug status register from
>   the inferior.
> 
>   In the discussion we had back in September, Mark said that the
>   status register should be per thread.  Does that mean that we need
>   an additional argument (int tid?) to pass to HWBP_GET_STATUS?  If
>   so, how will this argument get into the i386_hwbp_* functions which
>   will call these macros?
> 
>   Or maybe the target end can figure out the thread id by itself with
>   some TIDGET(pid) magic?
> 
> 
> Comments?  Suggestions?  Flames?
> 

Re: [RFC] Unified watchpoints for x86 platforms

[Mark Kettenis]

Eli Zaretskii <eliz@is.elta.co.il> writes:

> Ping!
> 
> No one posted any approvals or disprovals of this design.  Do I take
> the silence as a sign of agreement and start coding?

Sorry for not responding earlier.  In general, your proposal looks
fine, but I think it is best to export functions similar to GDB's
target_* functions:

int i386_remove_watchpoint (CORE_ADDR addr, int len,
			    enum target_hw_bp_type type);
int i386_insert_watchpoint (CORE_ADDR addr, int len,
                            enum target_hw_bp_type type);

int i386_insert_hw_breakpoint (CORE_ADDR addr, void *shadow);
int i386_remove_hw_breakpoint (CORE_ADDR addr, void *shadow);

etc.

Of course you can implement those on top of the functions mentioned below.

> > I started working on the unified support for hardware-assisted
> > breakpoints and watchpoints on x86 platforms (see TODO).  Since I
> > don't feel I know enough about all the aspects of this on any platform
> > but DJGPP, I thought I'd better get the framework agreed to before I
> > start coding.
> > 
> > Here's the API I suggest for use by higher-level GDB code:
> > 
> >   (Note: I'm not good at inventing names, so please suggest better
> >   ones if you want.)
> > 
> >   int i386_hwbp_insert (int pid, CORE_ADDR addr, int len, int kind);

Is there any particular reason why you need the PID argument?  AFAICS
it will always be equal to INFERIOR_PID, so I think we can do without
it.  This is also true for the other i386_hwbp_* functions you're
proposing.

> > 
> >   This function inserts a breakpoint or watchpoint to watch memory
> >   region starting at address ADDR whose length is LEN bytes.  The
> >   watchpoint will watch said memory region for accesses whose type
> >   is defined by KIND:
> > 
> >     HW_READ	     break if the region is accessed for reading[1]
> >     HW_WRITE	     break if the region is accessed for writing
> >     HW_ACCESS	     break if the region is accessed for either
> > 		     reading or writing
> >     HW_IO_ACCESS     same as HW_ACCESS type, but for I/O read/write
> > 		     access[2]
> >     HW_EXECUTE       instruction execution breakpoint

Please consider using an enum instead of an int.  It looks as if GDB's
convention is to use lower-case names for enum values.

> >   The function returns 0 upon success, else -1.
> > 
> >   Notes:
> >   [1] Since x86 doesn't support read data watchpoints, HW_READ will
> >       actually be implemented as a read/write watchpoint, and relies
> >       on higher-level GDB code to distinguish between reads and
> >       writes.  The infrastructure to support this is already in place
> >       in breakpoint.c, since GDB 5.0.

Sounds OK.

> >   [2] I/O watchpoints are not currently supported (AFAIK) by GDB on
> >       any x86 platform.  I can provide the code to handle it, but do
> >       people think we should define a command to access this feature?
> >       If so, should we provide separate read, write, and access types
> >       of watchpoints, or a single access type (the only one supported
> >       by x86's debug registers) is enough?

I think this can be added later if people want it.

> >       Note that I/O watchpoints require that the DE (debug extensions)
> >       flag in the CR4 register be set.  I don't know what platforms
> >       set it and under what circumstances.

I don't think you can do this in any of the x86 Unixoid systems.

> > 
> >   int i386_hwbp_remove (int pid, CORE_ADDR addr, int len, int kind);
> > 
> >   This function removes a breakpoint of watchpoint at address ADDR
> >   which watches a memory region of LEN bytes and whose type is given
> >   by KIND.  It returns 0 upon success, else -1.
> > 
> >   int i386_hwbp_region_ok (CORE_ADDR addr, int len);
> > 
> >   This function tests whether a memory region of LEN bytes starting at
> >   ADDR can be watched with debug registers.  It returns 1 if the
> >   region can be watched, 0 otherwise.
> > 
> >   int i386_hwbp_stopped_by_watchpoint (int pid);
> > 
> >   This function returns the address of a breakpoint or watchpoint
> >   which could have stopped the debuggee.  If no watchpoint triggered,
> >   it returns 0.
> > 
> > To actually insert and remove breakpoints and watchpoints, I need
> > low-level system-dependent functions.  Here's the API I suggest for
> > this low-levwl layer.  (These are macros so that targets could define
> > them on their nm-*.h files.  On a typical Unix or GNU/Linux system,
> > each of these macros will call `ptrace' with suitable arguments.)
> > 
> >   void HWBP_SET_ADDR (int pid, int dr_num, CORE_ADDR addr);
> > 
> >   This macro sets the debug register DR_NUM in the inferior to watch
> >   the address ADDR.  DR_NUM can be in the range [0..3].
> > 
> >   void HWBP_SET_CONTROL (int pid, unsigned int val);
> > 
> >   This macro sets the DR7 debug control register in the inferior to
> >   the value VAL.
> > 
> >   unsigned int HWBP_GET_STATUS (int pid);
> > 
> >   This macro returns the value of the DR6 debug status register from
> >   the inferior.

Why not have simply long I386_GET_DR(int regnum) and I386_SET_DR(int
regnum, long value) and let the system-dependent decide how to map the
debug register number ([0..7], as used in the Intel documentation)
into whatever is needed?

> >   In the discussion we had back in September, Mark said that the
> >   status register should be per thread.  Does that mean that we need
> >   an additional argument (int tid?) to pass to HWBP_GET_STATUS?  If
> >   so, how will this argument get into the i386_hwbp_* functions which
> >   will call these macros?

I don't think an additional argument is needed.  When calling
HWBP_GET_STATUS, it is the current thread that has encountered a trap,
and INFERIOR_PID should be set appropriately.

> >   Or maybe the target end can figure out the thread id by itself with
> >   some TIDGET(pid) magic?

Indeed.

Mark

Re: [RFC] Unified watchpoints for x86 platforms

[Eli Zaretskii]

> From: Mark Kettenis <kettenis@wins.uva.nl>
> Date: 15 Feb 2001 17:17:17 +0100
> 
> In general, your proposal looks
> fine, but I think it is best to export functions similar to GDB's
> target_* functions:
> 
> int i386_remove_watchpoint (CORE_ADDR addr, int len,
> 			    enum target_hw_bp_type type);
> int i386_insert_watchpoint (CORE_ADDR addr, int len,
>                             enum target_hw_bp_type type);
> 
> int i386_insert_hw_breakpoint (CORE_ADDR addr, void *shadow);
> int i386_remove_hw_breakpoint (CORE_ADDR addr, void *shadow);

I didn't want to use those names because they are taken by
i386v-nat.c.  If we use these names, we will have to modify
i386v-nat.c, which is used by many x86 platforms.  I didn't want such
a profound effect; some platform maintainers might not want the new
interface, and will be less than happy to rewrite their code for no
good reason.

> > >   int i386_hwbp_insert (int pid, CORE_ADDR addr, int len, int kind);
>
> Is there any particular reason why you need the PID argument?

Just the existing interfaces.  DJGPP obviously doesn't use that
argument.

> > >     HW_READ	     break if the region is accessed for reading[1]
> > >     HW_WRITE	     break if the region is accessed for writing
> > >     HW_ACCESS	     break if the region is accessed for either
> > > 		     reading or writing
> > >     HW_IO_ACCESS     same as HW_ACCESS type, but for I/O read/write
> > > 		     access[2]
> > >     HW_EXECUTE       instruction execution breakpoint
> 
> Please consider using an enum instead of an int.  It looks as if GDB's
> convention is to use lower-case names for enum values.

I intended to use an enum.  I believe the upper-case names were just a
coincidence.

> > >   void HWBP_SET_ADDR (int pid, int dr_num, CORE_ADDR addr);
> > > 
> > >   This macro sets the debug register DR_NUM in the inferior to watch
> > >   the address ADDR.  DR_NUM can be in the range [0..3].
> > > 
> > >   void HWBP_SET_CONTROL (int pid, unsigned int val);
> > > 
> > >   This macro sets the DR7 debug control register in the inferior to
> > >   the value VAL.
> > > 
> > >   unsigned int HWBP_GET_STATUS (int pid);
> > > 
> > >   This macro returns the value of the DR6 debug status register from
> > >   the inferior.
> 
> Why not have simply long I386_GET_DR(int regnum) and I386_SET_DR(int
> regnum, long value) and let the system-dependent decide how to map the
> debug register number ([0..7], as used in the Intel documentation)
> into whatever is needed?

Why bother the target end to do that?  They will all do the same, and
the code I'll write knows exactly when it needs what register.

In addition, I386_GET_DR might mislead someone into thinking that it
actually gets the value of any DRi from the inferior.  That was not my
intention: I don't need to fetch any debug register except DR6.

Thanks for the feedback.

I think I will start coding this weekend, so if someone has more
input, please hurry ;-)

Re: [RFC] Unified watchpoints for x86 platforms

[Mark Kettenis]

   Date: Thu, 15 Feb 2001 12:32:03 -0500 (EST)
   From: Eli Zaretskii <eliz@delorie.com>

   > From: Mark Kettenis <kettenis@wins.uva.nl>
   > Date: 15 Feb 2001 17:17:17 +0100
   > 
   > In general, your proposal looks
   > fine, but I think it is best to export functions similar to GDB's
   > target_* functions:
   > 
   > int i386_remove_watchpoint (CORE_ADDR addr, int len,
   > 			    enum target_hw_bp_type type);
   > int i386_insert_watchpoint (CORE_ADDR addr, int len,
   >                             enum target_hw_bp_type type);
   > 
   > int i386_insert_hw_breakpoint (CORE_ADDR addr, void *shadow);
   > int i386_remove_hw_breakpoint (CORE_ADDR addr, void *shadow);

   I didn't want to use those names because they are taken by
   i386v-nat.c.  If we use these names, we will have to modify
   i386v-nat.c, which is used by many x86 platforms.  I didn't want such
   a profound effect; some platform maintainers might not want the new
   interface, and will be less than happy to rewrite their code for no
   good reason.

Ah, but that isn't an issue here.  Targets using the old code from
i386v-nat.c won't use the generic code and vice versa.  Moreover, I
think I'll re-implement the stuff in i386v-nat.c with the help of the
generic code.  And as a last resort we can always rename the i386_*
functions in i386v-nat.c into i386v_*.  And really the only platforms
that actually use that code Linux and SCO Open Server 5.

So please use the names I suggest above.  I think they're much clearer.

   > > >   int i386_hwbp_insert (int pid, CORE_ADDR addr, int len, int kind);
   >
   > Is there any particular reason why you need the PID argument?

   Just the existing interfaces.  DJGPP obviously doesn't use that
   argument.

Linux and SCO Open Server don't need it either.  So please drop it.

   > > >     HW_READ	     break if the region is accessed for reading[1]
   > > >     HW_WRITE	     break if the region is accessed for writing
   > > >     HW_ACCESS	     break if the region is accessed for either
   > > > 		     reading or writing
   > > >     HW_IO_ACCESS     same as HW_ACCESS type, but for I/O read/write
   > > > 		     access[2]
   > > >     HW_EXECUTE       instruction execution breakpoint
   > 
   > Please consider using an enum instead of an int.  It looks as if GDB's
   > convention is to use lower-case names for enum values.

   I intended to use an enum.  I believe the upper-case names were just a
   coincidence.

See Andrews reply on that.

   > > >   void HWBP_SET_ADDR (int pid, int dr_num, CORE_ADDR addr);
   > > > 
   > > >   This macro sets the debug register DR_NUM in the inferior to watch
   > > >   the address ADDR.  DR_NUM can be in the range [0..3].
   > > > 
   > > >   void HWBP_SET_CONTROL (int pid, unsigned int val);
   > > > 
   > > >   This macro sets the DR7 debug control register in the inferior to
   > > >   the value VAL.
   > > > 
   > > >   unsigned int HWBP_GET_STATUS (int pid);
   > > > 
   > > >   This macro returns the value of the DR6 debug status register from
   > > >   the inferior.
   > 
   > Why not have simply long I386_GET_DR(int regnum) and I386_SET_DR(int
   > regnum, long value) and let the system-dependent decide how to map the
   > debug register number ([0..7], as used in the Intel documentation)
   > into whatever is needed?

   Why bother the target end to do that?  They will all do the same, and
   the code I'll write knows exactly when it needs what register.

It would make the implementation on Linux easier :-).  But the
difference isn't really that big.

   In addition, I386_GET_DR might mislead someone into thinking that it
   actually gets the value of any DRi from the inferior.  That was not my
   intention: I don't need to fetch any debug register except DR6.

Good point.

Mark

Re: [RFC] Unified watchpoints for x86 platforms

[Eli Zaretskii]

> Date: Thu, 15 Feb 2001 19:33:40 +0100
> From: Mark Kettenis <kettenis@wins.uva.nl>
> 
> So please use the names I suggest above.

Will do.

> I think they're much clearer.

Yes, I prefer them as well.

>    > Why not have simply long I386_GET_DR(int regnum) and I386_SET_DR(int
>    > regnum, long value) and let the system-dependent decide how to map the
>    > debug register number ([0..7], as used in the Intel documentation)
>    > into whatever is needed?
> 
>    Why bother the target end to do that?  They will all do the same, and
>    the code I'll write knows exactly when it needs what register.
> 
> It would make the implementation on Linux easier :-).

Could you please explain in a few words why?  Even if I don't change
the interface, I think it is important for me to know about such
subtle aspects.

What I see in i386v-nat.c is that each of these macros maps directly
into a ptrace call with appropriate arguments.  For example, here's
how DR7 (the debug control register) is set:

  ptrace (6, pid, offsetof (struct user, u_debugreg[DR_CONTROL]),
	  debug_control_mirror);

and here's how DR6, the debug status register, is fetched:

  status = ptrace (3, pid, offsetof (struct user, u_debugreg[DR_STATUS]), 0);

Even ia64-linux-nat.c does something very similar, even though it is
not one of the potential customers of what I'm about to write.

Re: [RFC] Unified watchpoints for x86 platforms

[Kevin Buettner]

On Feb 15,  5:17pm, Mark Kettenis wrote:

> > > I started working on the unified support for hardware-assisted
> > > breakpoints and watchpoints on x86 platforms (see TODO).  Since I
> > > don't feel I know enough about all the aspects of this on any platform
> > > but DJGPP, I thought I'd better get the framework agreed to before I
> > > start coding.
> > > 
> > > Here's the API I suggest for use by higher-level GDB code:
> > > 
> > >   (Note: I'm not good at inventing names, so please suggest better
> > >   ones if you want.)
> > > 
> > >   int i386_hwbp_insert (int pid, CORE_ADDR addr, int len, int kind);
> 
> Is there any particular reason why you need the PID argument?  AFAICS
> it will always be equal to INFERIOR_PID, so I think we can do without
> it.  This is also true for the other i386_hwbp_* functions you're
> proposing.

I think it'd be better to not rely on ``inferior_pid''.  I would
rather see the explicitly passed.  There will come a day when GDB
is able to debug more than one process at a time and to perpetuate
reliance on inferior pid would be short sighted.

> > >   In the discussion we had back in September, Mark said that the
> > >   status register should be per thread.  Does that mean that we need
> > >   an additional argument (int tid?) to pass to HWBP_GET_STATUS?  If
> > >   so, how will this argument get into the i386_hwbp_* functions which
> > >   will call these macros?
> 
> I don't think an additional argument is needed.  When calling
> HWBP_GET_STATUS, it is the current thread that has encountered a trap,
> and INFERIOR_PID should be set appropriately.
> 
> > >   Or maybe the target end can figure out the thread id by itself with
> > >   some TIDGET(pid) magic?

Hopefully I'll find time to merge my pid/tid/lwp patch sometime soon.
When this occurs, you'll be able to extract the thread id from what is
now the pid argument.

I have read the rest of Eli's proposal as well as Mark's comments and
I agree with the rest of Mark's remarks.

Kevin

Re: [RFC] Unified watchpoints for x86 platforms

[Eli Zaretskii]

> Date: Thu, 15 Feb 2001 11:41:35 -0700
> From: Kevin Buettner <kevinb@cygnus.com>
> > 
> > Is there any particular reason why you need the PID argument?  AFAICS
> > it will always be equal to INFERIOR_PID, so I think we can do without
> > it.  This is also true for the other i386_hwbp_* functions you're
> > proposing.
> 
> I think it'd be better to not rely on ``inferior_pid''.  I would
> rather see the explicitly passed.  There will come a day when GDB
> is able to debug more than one process at a time and to perpetuate
> reliance on inferior pid would be short sighted.

I have two opposite opinions here.  We need to resolve this somehow.

> I have read the rest of Eli's proposal as well as Mark's comments and
> I agree with the rest of Mark's remarks.

Thanks for the feedback.

Re: [RFC] Unified watchpoints for x86 platforms

[J.T. Conklin]

>>>>> "Eli" == Eli Zaretskii <eliz@delorie.com> writes:
>> > Is there any particular reason why you need the PID argument?  AFAICS
>> > it will always be equal to INFERIOR_PID, so I think we can do without
>> > it.  This is also true for the other i386_hwbp_* functions you're
>> > proposing.
>> 
>> I think it'd be better to not rely on ``inferior_pid''.  I would
>> rather see the explicitly passed.  There will come a day when GDB
>> is able to debug more than one process at a time and to perpetuate
>> reliance on inferior pid would be short sighted.

Eli> I have two opposite opinions here.  We need to resolve this somehow.

We're going to need to pass a PID, or perhaps some new representation
of a execution context, to a lot of code functions that don't allready
have such an argument.  It is not clear to me that adding such an
argument "because it will be needed" is correct, considering that the
design has not yet started.  The truth is we don't know "what" will be
needed, so we'll have to revisit this function (among many others)
down the line anyway.


        --jtc

-- 
J.T. Conklin
RedBack Networks

Re: [RFC] Unified watchpoints for x86 platforms

[Kevin Buettner]

On Feb 15,  2:45pm, J.T. Conklin wrote:

> Subject: Re: [RFC] Unified watchpoints for x86 platforms
> >>>>> "Eli" == Eli Zaretskii <eliz@delorie.com> writes:
> >> > Is there any particular reason why you need the PID argument?  AFAICS
> >> > it will always be equal to INFERIOR_PID, so I think we can do without
> >> > it.  This is also true for the other i386_hwbp_* functions you're
> >> > proposing.
> >> 
> >> I think it'd be better to not rely on ``inferior_pid''.  I would
> >> rather see the explicitly passed.  There will come a day when GDB
> >> is able to debug more than one process at a time and to perpetuate
> >> reliance on inferior pid would be short sighted.
> 
> Eli> I have two opposite opinions here.  We need to resolve this somehow.
> 
> We're going to need to pass a PID, or perhaps some new representation
> of a execution context, to a lot of code functions that don't allready
> have such an argument.  It is not clear to me that adding such an
> argument "because it will be needed" is correct, considering that the
> design has not yet started.  The truth is we don't know "what" will be
> needed, so we'll have to revisit this function (among many others)
> down the line anyway.

Eli,

I think the answer is to use your best judgement.  Regardless of
whether you pass the pid in or simply use inferior_pid, your new
watchpoint code is going to have to eventually be changed to use
a different representation of the execution context.  I happen to
think that it might actually change less if you pass a parameter, but
after thinking about it a bit more, I can see why someone else might
hold the opposite opinion.

Kevin

Re: [RFC] Unified watchpoints for x86 platforms

[Eli Zaretskii]

> Date: Thu, 15 Feb 2001 17:09:53 -0700
> From: Kevin Buettner <kevinb@cygnus.com>
> 
> I think the answer is to use your best judgement.  Regardless of
> whether you pass the pid in or simply use inferior_pid, your new
> watchpoint code is going to have to eventually be changed to use
> a different representation of the execution context.  I happen to
> think that it might actually change less if you pass a parameter, but
> after thinking about it a bit more, I can see why someone else might
> hold the opposite opinion.

Okay, thanks for the feedback.  I'll sleep on this and then decide.

Re: [RFC] Unified watchpoints for x86 platforms

[Eli Zaretskii]

While writing the code for the x86 watchpoint support, a question
popped up regarding the Global Enable and Local Enable flags in the
DR7 (Debug Control) register.

For those who might not remember what those flags are: Each
hardware-assisted breakpoint or watchpoint should be enabled by
setting bits in the DR7 register.  A watchpoint can be enabled locally
or globally, depending on which bit in DR7 is set for that watchpoint.
A watchpoint that is enabled locally will only break if it is hit in
the current task.  A watchpoint that is enabled globally will break in
any task.

Now, I understand that, by and large, x86 platforms enable watchpoints
locally, to avoid spurious traps triggered by other processes.  If
that is true, should the code I write use the Local Enable flag
unconditionally?  Or perhaps it would be useful to define an API that
enables a target to control whether the global or local enable flag is
used?

Opinions?

Re: [RFC] Unified watchpoints for x86 platforms

[Mark Kettenis]

Eli Zaretskii <eliz@delorie.com> writes:

> While writing the code for the x86 watchpoint support, a question
> popped up regarding the Global Enable and Local Enable flags in the
> DR7 (Debug Control) register.
> 
> For those who might not remember what those flags are: Each
> hardware-assisted breakpoint or watchpoint should be enabled by
> setting bits in the DR7 register.  A watchpoint can be enabled locally
> or globally, depending on which bit in DR7 is set for that watchpoint.
> A watchpoint that is enabled locally will only break if it is hit in
> the current task.  A watchpoint that is enabled globally will break in
> any task.

Makes me wonder if global watchpoints would really work if hardware
task switching isn't used.

Anyway, I guess most x86 OS'es wouldn't allow any real global
watchpoints.  For Linux it doesn't matter whether you set the local or
the global bit.

> Now, I understand that, by and large, x86 platforms enable watchpoints
> locally, to avoid spurious traps triggered by other processes.  If
> that is true, should the code I write use the Local Enable flag
> unconditionally?  Or perhaps it would be useful to define an API that
> enables a target to control whether the global or local enable flag is
> used?

In principle a GDB command to control whether to enable watchpoints
locally or globally wouldn't be a bad idea.  But I don't know any
targets where this would be useful.  So if it isn't useful for DJGPP
either, I wouldn't bother.  A comment about the issue at the right
spot would be nice though.

Mark

Re: [RFC] Unified watchpoints for x86 platforms

[Eli Zaretskii]

> From: Mark Kettenis <kettenis@wins.uva.nl>
> Date: 27 Feb 2001 12:28:43 +0100
> > 
> > For those who might not remember what those flags are: Each
> > hardware-assisted breakpoint or watchpoint should be enabled by
> > setting bits in the DR7 register.  A watchpoint can be enabled locally
> > or globally, depending on which bit in DR7 is set for that watchpoint.
> > A watchpoint that is enabled locally will only break if it is hit in
> > the current task.  A watchpoint that is enabled globally will break in
> > any task.
> 
> Makes me wonder if global watchpoints would really work if hardware
> task switching isn't used.

I guess the answer depends on how exactly is the software task
switching implemented.  Namely, does it reset the GE and LE bits in
DR7 when the task switch happens.

> In principle a GDB command to control whether to enable watchpoints
> locally or globally wouldn't be a bad idea.  But I don't know any
> targets where this would be useful.  So if it isn't useful for DJGPP
> either, I wouldn't bother.  A comment about the issue at the right
> spot would be nice though.

That was my tendency as well.  Thanks for the feedback.

Re: [RFA] Unified watchpoints for x86 platforms

[Eli Zaretskii]

This is the first cut of the code to support hardware breakpoints and
watchpoints on all x86 platforms.  It works for me in the DJGPP port.
Please critique as you see fit.

I'm still working on the doco changes for gdbint.texinfo, but I
thought there's no reason not to start the review process right away.

Note that the names of the functions and macros have changed a little
since the draft API I posted here.  Also, since Fernando said that
there are remote targets which might want to use hardware watchpoints,
I decided to put the code on i386-tdep.c.

I'd like to thank everybody who participated in discussions and
returned feedback.

2001-03-03  Eli Zaretskii  <eliz@is.elta.co.il>

	Unified support for hardware breakpoints and watchpoints on
	x86 targets:

	* config/i386/tm-i386.h (TARGET_HAS_HARDWARE_WATCHPOINTS):
	Define.
	(i386_cleanup_dregs, i386_insert_watchpoint)
	(i386_remove_watchpoint, i386_region_ok_for_watchpoint)
	(i386_stopped_by_hwbp, i386_stopped_data_address)
	(i386_insert_hw_breakpoint, i386_remove_hw_breakpoint): Declare
	prototypes.
	(TARGET_CAN_USE_HARDWARE_WATCHPOINT) 
	(TARGET_REGION_OK_FOR_HW_WATCHPOINT, HAVE_CONTINUABLE_WATCHPOINT) 
	(STOPPED_BY_WATCHPOINT, target_stopped_data_address) 
	(target_insert_watchpoint, target_remove_watchpoint) 
	(target_insert_hw_breakpoint, target_remove_hw_breakpoint): Define
	to call the appropriate i386_* functions.

	* i386-tdep.c (I386_DR_CONTROL_MASK, I386_DR_LOCAL_ENABLE) 
	(I386_DR_GLOBAL_ENABLE, I386_DR_DISABLE, I386_DR_SET_RW_LEN) 
	(I386_DR_GET_RW_LEN, I386_DR_WATCH_HIT): New macros.
	(dr_mirror, dr_status_mirror, dr_control_mirror, dr_ref_count)
	(maint_show_dr): New variables.
	(i386_cleanup_dregs, i386_show_dr, i386_length_and_rw_bits) 
	(i386_insert_aligned_watchpoint, i386_remove_aligned_watchpoint) 
	(i386_handle_nonaligned_watchpoint, i386_insert_watchpoint) 
	(i386_remove_watchpoint, i386_region_ok_for_watchpoint) 
	(i386_stopped_data_address, i386_stopped_by_hwbp) 
	(i386_insert_hw_breakpoint, i386_remove_hw_breakpoint): New
	functions.
	(_initialize_i386_tdep): Add new maint command `show-debug-regs',
	sets maint_show_dr to non-zero value and activates debugging
	print-outs in functions which insert, remove, and test
	watchpoints and hardware breakpoints.


--- gdb/i386-tdep.c~0	Thu Dec 21 22:52:58 2000
+++ gdb/i386-tdep.c	Sat Mar  3 23:18:12 2001
@@ -1,5 +1,5 @@
 /* Intel 386 target-dependent stuff.
-   Copyright (C) 1988, 1989, 1991, 1994, 1995, 1996, 1998
+   Copyright (C) 1988, 1989, 1991, 1994, 1995, 1996, 1998, 2001
    Free Software Foundation, Inc.
 
    This file is part of GDB.
@@ -858,6 +858,659 @@ i386_register_convert_to_raw (struct typ
   memcpy (to, from, FPU_REG_RAW_SIZE);
 }
 
+
+/* Support for hardware watchpoints and breakpoints using the x86
+   debug registers.
+
+   This provides several functions for inserting and removing
+   hardware-assisted breakpoints and watchpoints, testing if
+   one or more of the watchpoints triggerd and at what address,
+   checking whether a given region can be watched, etc.  See
+   the section "Exported API functions" below.
+
+   A target which wants to use these functions should define
+   several macros, such as `target_insert_watchpoint' and
+   `target_stopped_data_address', listed in target.h, to call
+   the appropriate functions below.
+
+   Each target should also provide several low-level macros
+   that will be called to insert watchpoints and hardware
+   breakpoints into the inferior, remove them, and check their
+   status.  These macros are:
+
+      I386_DR_LOW_SET_CONTROL  -- set the debug control (DR7)
+				  register to a given value
+
+      I386_DR_LOW_SET_ADDR     -- put an address into one debug
+				  register
+
+      I386_DR_LOW_RESET_ADDR   -- reset the address stored in
+				  one debug register
+
+      I386_DR_LOW_GET_STATUS   -- return the value of the debug
+				  status (DR6) register.
+
+   The functions below implement debug registers sharing by
+   reference counts, and allow to watch regions up to 16 bytes
+   long.  */
+
+#ifdef TARGET_HAS_HARDWARE_WATCHPOINTS
+
+#ifdef HAVE_PTRACE_H
+#include <ptrace.h>
+#else
+#ifdef HAVE_SYS_PTRACE_H
+#include <sys/ptrace.h>
+#endif
+#endif
+
+#ifdef HAVE_SYS_USER
+#include <sys/user.h>
+#endif
+
+/* FIXME: The following should be just "#include <sys/debugreg.h>",
+   but the the Linux 2.1.x kernel and glibc 2.0.x are not in sync;
+   including <sys/debugreg.h> will result in an error.  With luck,
+   these losers will get their act together and we can trash this hack
+   in the near future.
+
+   --jsm 1998-10-21; modified by eliz 2001-02-24.  */
+
+#ifdef HAVE_ASM_DEBUGREG_H
+#include <asm/debugreg.h>
+#else  /* !HAVE_ASM_DEBUGREG_H */
+
+#ifdef HAVE_SYS_DEBUGREG_H
+#include <sys/debugreg.h>
+#else  /* !HAVE_SYS_DEBUGREG_H */
+
+/* Provide definitions for platforms which don't have debugreg.h, such
+   as DJGPP.  As a side effect, explain what each macro means or does.  */
+
+/* Debug registers' indices.  */
+#define DR_FIRSTADDR		0  /* index of first debug address register */
+#define DR_LASTADDR		3  /* index of last debug address register */
+#define DR_STATUS		6  /* index of debug status register (DR6) */
+#define DR_CONTROL		7  /* index of debug control register (DR7) */
+
+/* DR7 Debug Control register fields.  */
+
+/* How many bits to skip in DR7 to get to R/W and LEN fields.  */
+#define DR_CONTROL_SHIFT	16
+/* How many bits in DR7 per R/W and LEN field for each watchpoint.  */
+#define DR_CONTROL_SIZE		4
+
+/* Watchpoint/breakpoint read/write fields in DR7.  */
+#define DR_RW_EXECUTE		(0x0) /* break on instruction execution */
+#define DR_RW_WRITE		(0x1) /* break on data writes */
+#define DR_RW_READ		(0x3) /* break on data reads or writes */
+
+/* Watchpoint/breakpoint length fields in DR7.  The 2-bit left shift
+   is so we could OR this with the read/write field defined above.  */
+#define DR_LEN_1		(0x0 << 2) /* 1-byte region watch or breakpt */
+#define DR_LEN_2		(0x1 << 2) /* 2-byte region watch */
+#define DR_LEN_4		(0x3 << 2) /* 4-byte region watch */
+
+/* Local and Global Enable flags in DR7.
+
+   When the Local Enable flag is set, the breakpoint/watchpoint is
+   enabled only for the current task; the processor automatically
+   clears this flag on every task switch.  When the Global Enable
+   flag is set, the breakpoint/watchpoint is enabled for all tasks;
+   the processor never clears this flag.
+
+   Currently, all watchpoint are locally enabled.  If you need to
+   enable them globally, read the comment which pertains to this in
+   i386_insert_aligned_watchpoint below.  */
+#define DR_LOCAL_ENABLE_SHIFT	0   /* extra shift to the local enable bit */
+#define DR_GLOBAL_ENABLE_SHIFT	1   /* extra shift to the global enable bit */
+#define DR_ENABLE_SIZE		2   /* 2 enable bits per debug register */
+
+/* Local and global exact breakpoint enable flags (a.k.a. slowdown
+   flags).  These are only required on i386, to allow detection of the
+   exact instruction which caused a watchpoint to break; i486 and
+   later processors do that automatically.  We set these flags for
+   back compatibility.  */
+#define DR_LOCAL_SLOWDOWN	(0x100)
+#define DR_GLOBAL_SLOWDOWN	(0x200)
+
+/* Fields reserved by Intel.  This includes the GD (General Detect
+   Enable) flag, which causes a debug exception to be generated when a
+   MOV instruction accesses one of the debug registers.
+
+   FIXME: My Intel manual says we should use 0xF800, not 0xFC00.  */
+#define DR_CONTROL_RESERVED	(0xFC00)
+
+#endif /* !HAVE_SYS_DEBUGREG_H */
+
+#endif /* !HAVE_ASM_DEBUGREG_H */
+
+/* This is here for completeness.  No platform supports this
+   functionality yet (as of Feb-2001).  Note that the DE flag in the
+   CR4 register needs to be set to support this.  */
+#ifndef DR_RW_IORW
+#define DR_RW_IORW		(0x2) /* break on I/O reads or writes */
+#endif
+
+/* Auxiliary helper macros.  */
+
+/* A value that masks all fields in DR7 that are reserved by Intel.  */
+#define I386_DR_CONTROL_MASK		(~DR_CONTROL_RESERVED)
+
+/* The I'th debug register is vacant if its Local and Global Enable
+   bits are reset in the Debug Control register.  */
+#define I386_DR_VACANT(i) \
+  ((dr_control_mirror & (3 << (DR_ENABLE_SIZE * (i)))) == 0)
+
+/* Locally enable the break/watchpoint in the I'th debug register.  */
+#define I386_DR_LOCAL_ENABLE(i) \
+  dr_control_mirror |= (1 << (DR_LOCAL_ENABLE_SHIFT + DR_ENABLE_SIZE * (i)))
+
+/* Globally enable the break/watchpoint in the I'th debug register.  */
+#define I386_DR_GLOBAL_ENABLE(i) \
+  dr_control_mirror |= (1 << (DR_GLOBAL_ENABLE_SHIFT + DR_ENABLE_SIZE * (i)))
+
+/* Disable the break/watchpoint in the I'th debug register.  */
+#define I386_DR_DISABLE(i) \
+  dr_control_mirror &= ~(3 << (DR_ENABLE_SIZE * (i)))
+
+/* Set in DR7 the RW and LEN fields for the I'th debug register.  */
+#define I386_DR_SET_RW_LEN(i,rwlen) \
+  do { \
+    dr_control_mirror &= ~(0x0f << (DR_CONTROL_SHIFT+DR_CONTROL_SIZE*(i)));   \
+    dr_control_mirror |= ((rwlen) << (DR_CONTROL_SHIFT+DR_CONTROL_SIZE*(i))); \
+  } while (0)
+
+/* Get from DR7 the RW and LEN fields for the I'th debug register.  */
+#define I386_DR_GET_RW_LEN(i) \
+  ((dr_control_mirror >> (DR_CONTROL_SHIFT + DR_CONTROL_SIZE * (i))) & 0x0f)
+
+/* Did the watchpoint whose address is in the I'th register break?  */
+#define I386_DR_WATCH_HIT(i)	(dr_status_mirror & (1 << (i)))
+
+/* A macro to loop over all debug registers.  */
+#define ALL_DEBUG_REGISTERS(i)	for (i = 0; i <= DR_LASTADDR-DR_FIRSTADDR; i++)
+
+/* This is in i386v-nat.c, so let's have it here, just in case.  */
+#if !defined (offsetof)
+#define offsetof(TYPE, MEMBER) ((unsigned long) &((TYPE *)0)->MEMBER)
+#endif
+
+/* Mirror the inferior's DRi registers.  We keep the status and
+   control registers separated because they don't hold addresses.  */
+static CORE_ADDR dr_mirror[DR_LASTADDR - DR_FIRSTADDR + 1];
+static unsigned	 dr_status_mirror, dr_control_mirror;
+
+/* Reference counts for each debug register.  */
+static int	 dr_ref_count[DR_LASTADDR - DR_FIRSTADDR + 1];
+
+/* Whether or not to print the mirrored debug registers.  */
+static int	 maint_show_dr;
+
+/* Types of operations supported by i386_handle_nonaligned_watchpoint.  */
+typedef enum { WP_INSERT, WP_REMOVE, WP_COUNT } i386_wp_op_t;
+
+/* Exported API functions.  */
+
+/* Clear the reference counts and forget everything we knew about DRi.  */
+void i386_cleanup_dregs (void);
+
+/* Insert a watchpoint to watch a memory region which starts at
+   address ADDR and whose length is LEN bytes.  Watch memory accesses
+   of the type TYPE.  Return 0 on success, -1 on failure.  */
+int  i386_insert_watchpoint (CORE_ADDR addr, int len, int type);
+
+/* Remove a watchpoint that watched the memory region which starts at
+   address ADDR, whose length is LEN bytes, and for accesses of the
+   type TYPE.  Return 0 on success, -1 on failure.  */
+int  i386_remove_watchpoint (CORE_ADDR addr, int len, int type);
+
+/* Return non-zero if we can watch a memory region that starts at
+   address ADDR and whose length is LEN bytes.  */
+int  i386_region_ok_for_watchpoint (CORE_ADDR addr, int len);
+
+/* Return non-zero if the inferior has some break/watchpoint that
+   triggered.  */
+int  i386_stopped_by_hwbp (void);
+
+/* If the inferior has some break/watchpoint that triggered, return
+   the address associated with that break/watchpoint.  Otherwise,
+   return zero.  */
+CORE_ADDR i386_stopped_data_address (void);
+
+/* Insert a hardware-assisted breakpoint at address ADDR.  SHADOW is
+   unused.  Return 0 on success, EBUSY on failure.  */
+int  i386_insert_hw_breakpoint (CORE_ADDR addr, void *shadow);
+
+/* Remove a hardware-assisted breakpoint at address ADDR.  SHADOW is
+   unused.  Return 0 on success, -1 on failure.  */
+int  i386_remove_hw_breakpoint (CORE_ADDR addr, void *shadow);
+
+/* Internal functions.  */
+
+/* Return the value of a 4-bit field for DR7 suitable for watching a
+   region of LEN bytes for accesses of type TYPE.  LEN is assumed
+   to have the value of 1, 2, or 4.  */
+static unsigned i386_length_and_rw_bits (int len, enum target_hw_bp_type type);
+
+/* Insert a watchpoint at address ADDR, which is assumed to be aligned
+   according to the length of the region to watch.  LEN_RW_BITS is the
+   value of the bit-field from DR7 which describes the length and
+   access type of the region to be watched by this watchpoint.  Return
+   0 on success, -1 on failure.  */
+static int i386_insert_aligned_watchpoint (CORE_ADDR addr,
+					   unsigned len_rw_bits);
+
+/* Remove a watchpoint at address ADDR, which is assumed to be aligned
+   according to the length of the region to watch.  LEN_RW_BITS is the
+   value of the bits from DR7 which describes the length and access
+   type of the region watched by this watchpoint.  Return 0 on
+   success, -1 on failure.  */
+static int i386_remove_aligned_watchpoint (CORE_ADDR addr,
+					   unsigned len_rw_bits);
+
+/* Insert or remove a (possibly non-aligned) watchpoint, or count the
+   number of debug registers required to watch a region at address
+   ADDR whose length is LEN for accesses of type TYPE.  Return 0 on
+   successful insertion or removal, a positive number when queried
+   about the number of registers, or -1 on failure.  If WHAT is not
+   a valid value, returns EINVAL.  */
+static int i386_handle_nonaligned_watchpoint (i386_wp_op_t what,
+					      CORE_ADDR addr, int len,
+					      enum target_hw_bp_type type);
+
+/* Implementation.  */
+
+/* Clear the reference counts and forget everything we knew about
+   the debug registers.  */
+void
+i386_cleanup_dregs (void)
+{
+  int i;
+
+  ALL_DEBUG_REGISTERS(i)
+    {
+      dr_mirror[i] = 0;
+      dr_ref_count[i] = 0;
+    }
+  dr_control_mirror = 0;
+  dr_status_mirror  = 0;
+}
+
+/* Print the values of the mirrored debug registers.
+   This is called when maint_show_dr is non-zero.  To set that
+   up, type "maint show-debug-regs" at GDB's prompt.  */
+static void
+i386_show_dr (const char *func, CORE_ADDR addr,
+	      int len, enum target_hw_bp_type type)
+{
+  int i;
+
+  puts_unfiltered (func);
+  if (addr || len)
+    printf_unfiltered (" (addr=%lx, len=%d, type=%s)",
+		       /* This code is for ia32, so casting CORE_ADDR
+			  to unsigned long should be okay.  */
+		       (unsigned long)addr, len,
+		       type == hw_write ? "data-write"
+		       : (type == hw_read ? "data-read"
+			  : (type == hw_access ? "data-read/write"
+			     : (type == hw_execute ? "instruction-execute"
+				/* FIXME: if/when I/O read/write
+				   watchpoints are supported, add them
+				   here.  */
+				: "??unknown??"))));
+  puts_unfiltered (":\n");
+  printf_unfiltered ("CONTROL (DR7): %08x          STATUS (DR6): %08x\n",
+		     dr_control_mirror, dr_status_mirror);
+  ALL_DEBUG_REGISTERS(i)
+    {
+      printf_unfiltered ("DR%d: addr=%08lx, ref.count=%d  DR%d: addr=%08lx, ref.count=%d\n",
+			 i, dr_mirror[i], dr_ref_count[i],
+			 i+1, dr_mirror[i+1], dr_ref_count[i+1]);
+      i++;
+    }
+}
+
+/* Return the value of a 4-bit field for DR7 suitable for watching a
+   region of LEN bytes for accesses of type TYPE.  LEN is assumed
+   to have the value of 1, 2, or 4.  */
+static unsigned
+i386_length_and_rw_bits (int len, enum target_hw_bp_type type)
+{
+  unsigned rw;
+
+  switch (type)
+    {
+      case hw_execute:
+	rw = DR_RW_EXECUTE;
+	break;
+      case hw_write:
+	rw = DR_RW_WRITE;
+	break;
+      case hw_read:	 /* x86 doesn't support data-read watchpoints */
+      case hw_access:
+	rw = DR_RW_READ;
+	break;
+#if 0
+      case hw_io_access: /* not yet supported */
+	rw = DR_RW_IORW;
+	break;
+#endif
+      default:
+	internal_error ("\
+Invalid hw breakpoint type %d in i386_length_and_rw_bits.\n", (int)type);
+    }
+
+  switch (len)
+    {
+      case 4:
+	return (DR_LEN_4 | rw);
+      case 2:
+	return (DR_LEN_2 | rw);
+      case 1:
+	return (DR_LEN_1 | rw);
+      default:
+	internal_error ("\
+Invalid hw breakpoint length %d in i386_length_and_rw_bits.\n", len);
+    }
+}
+
+/* Insert a watchpoint at address ADDR, which is assumed to be aligned
+   according to the length of the region to watch.  LEN_RW_BITS is the
+   value of the bits from DR7 which describes the length and access
+   type of the region to be watched by this watchpoint.  Return 0 on
+   success, -1 on failure.  */
+static int
+i386_insert_aligned_watchpoint (CORE_ADDR addr, unsigned len_rw_bits)
+{
+  int i;
+
+  /* First, look for an occupied debug register with the same address
+     and the same RW and LEN definitions.  If we find one, we can
+     reuse it for this watchpoint as well (and save a register).  */
+  ALL_DEBUG_REGISTERS(i)
+    {
+      if (!I386_DR_VACANT (i)
+	  && dr_mirror[i] == addr
+	  && I386_DR_GET_RW_LEN (i) == len_rw_bits)
+	{
+	  dr_ref_count[i]++;
+	  return 0;
+	}
+    }
+
+  /* Next, look for a vacant debug register.  */
+  ALL_DEBUG_REGISTERS(i)
+    {
+      if (I386_DR_VACANT (i))
+	break;
+    }
+
+  /* No more debug registers!  */
+  if (i > DR_LASTADDR - DR_FIRSTADDR)
+    return -1;
+
+  /* Now set up the register I to watch our region.  */
+
+  /* Record the info in our local mirrored array.  */
+  dr_mirror[i] = addr;
+  dr_ref_count[i] = 1;
+  I386_DR_SET_RW_LEN (i, len_rw_bits);
+  /* Note: we only enable the watchpoint locally, i.e. in the current
+     task.  Currently, no x86 target allows or supports global
+     watchpoints; however, if any target would want that in the
+     future, GDB should probably provide a command to control whether
+     to enable watchpoints globally or locally, and the code below
+     should use global or local enable and slow-down flags as
+     appropriate.  */
+  I386_DR_LOCAL_ENABLE (i);
+  dr_control_mirror |= DR_LOCAL_SLOWDOWN;
+  dr_control_mirror &= I386_DR_CONTROL_MASK;
+
+  /* Finally, actually pass the info to the inferior.  */
+  I386_DR_LOW_SET_CONTROL (dr_control_mirror);
+  I386_DR_LOW_SET_ADDR (i + DR_FIRSTADDR, addr);
+
+  return 0;
+}
+
+/* Remove a watchpoint at address ADDR, which is assumed to be aligned
+   according to the length of the region to watch.  LEN_RW_BITS is the
+   value of the bits from DR7 which describes the length and access
+   type of the region watched by this watchpoint.  Return 0 on
+   success, -1 on failure.  */
+static int
+i386_remove_aligned_watchpoint (CORE_ADDR addr, unsigned len_rw_bits)
+{
+  int i, retval = -1;
+
+  ALL_DEBUG_REGISTERS(i)
+    {
+      if (!I386_DR_VACANT (i)
+	  && dr_mirror[i] == addr
+	  && I386_DR_GET_RW_LEN (i) == len_rw_bits)
+	{
+	  if (--dr_ref_count[i] == 0) /* no longer in use? */
+	    {
+	      /* Reset our mirror.  */
+	      dr_mirror[i] = 0;
+	      I386_DR_DISABLE (i);
+	      /* Reset it in the inferior.  */
+	      I386_DR_LOW_RESET_ADDR (i + DR_FIRSTADDR);
+	      I386_DR_LOW_SET_CONTROL (dr_control_mirror);
+	    }
+	  retval = 0;
+	}
+    }
+
+  return retval;
+}
+
+/* Insert or remove a (possibly non-aligned) watchpoint, or count the
+   number of debug registers required to watch a region at address
+   ADDR whose length is LEN for accesses of type TYPE.  Return 0 on
+   successful insertion or removal, a positive number when queried
+   about the number of registers, or -1 on failure.  If WHAT is not
+   a valid value, returns EINVAL.  */
+static int
+i386_handle_nonaligned_watchpoint (i386_wp_op_t what, CORE_ADDR addr, int len,
+				   enum target_hw_bp_type type)
+{
+  int align;
+  int size;
+  int rv = 0, status = 0;
+
+  static int size_try_array[4][4] =
+  {
+    { 1, 1, 1, 1 },		/* trying size one */
+    { 2, 1, 2, 1 },		/* trying size two */
+    { 2, 1, 2, 1 },		/* trying size three */
+    { 4, 1, 2, 1 }		/* trying size four */
+  };
+
+  while (len > 0)
+    {
+      align = addr % 4;
+      /* Four is the maximum length an x86 debug register can watch.  */
+      size = size_try_array[len > 4 ? 3 : len - 1][align];
+      if (what == WP_COUNT)
+	/* size_try_array[] is defined so that each iteration through
+	   the loop is guaranteed to produce an address and a size
+	   that can be watched with a single debug register.  Thus,
+	   for counting the registers required to watch a region, we
+	   simply need to increment the count on each iteration.  */
+	rv++;
+      else
+	{
+	  unsigned len_rw = i386_length_and_rw_bits (size, type);
+
+	  if (what == WP_INSERT)
+	    status = i386_insert_aligned_watchpoint (addr, len_rw);
+	  else if (what == WP_REMOVE)
+	    status = i386_remove_aligned_watchpoint (addr, len_rw);
+	  else
+	    status = EINVAL;
+	  /* We keep the loop going even after a failure, because some
+	     of the other aligned watchpoints might still succeed
+	     (e.g. if they watch addresses that are already watched,
+	     in which case we just increment the reference counts of
+	     occupied debug registers).  If we break out of the loop
+	     too early, we could cause those addresses watched by
+	     other watchpoints to be disabled when breakpoint.c reacts
+	     to our failure to insert this watchpoint and tries to
+	     remove it.  */
+	  if (status)
+	    rv = status;
+	}
+      addr += size;
+      len -= size;
+    }
+  return rv;
+}
+
+/* Insert a watchpoint to watch a memory region which starts at
+   address ADDR and whose length is LEN bytes.  Watch memory accesses
+   of the type TYPE.  Return 0 on success, -1 on failure.  */
+int
+i386_insert_watchpoint (CORE_ADDR addr, int len, int type)
+{
+  int retval;
+
+  if (len == 3 || len > 4 || addr % len != 0)
+    retval = i386_handle_nonaligned_watchpoint (WP_INSERT, addr, len, type);
+  else
+    {
+      unsigned len_rw = i386_length_and_rw_bits (len, type);
+
+      retval = i386_insert_aligned_watchpoint (addr, len_rw);
+    }
+
+  if (maint_show_dr)
+    i386_show_dr ("insert_watchpoint", addr, len, type);
+
+  return retval;
+}
+
+/* Remove a watchpoint that watched the memory region which starts at
+   address ADDR, whose length is LEN bytes, and for accesses of the
+   type TYPE.  Return 0 on success, -1 on failure.  */
+int
+i386_remove_watchpoint (CORE_ADDR addr, int len, int type)
+{
+  int retval;
+
+  if (len == 3 || len > 4 || addr % len != 0)
+    retval = i386_handle_nonaligned_watchpoint (WP_REMOVE, addr, len, type);
+  else
+    {
+      unsigned len_rw = i386_length_and_rw_bits (len, type);
+
+      retval = i386_remove_aligned_watchpoint (addr, len_rw);
+    }
+
+  if (maint_show_dr)
+    i386_show_dr ("remove_watchpoint", addr, len, type);
+
+  return retval;
+}
+
+/* Return non-zero if we can watch a memory region that starts at
+   address ADDR and whose length is LEN bytes.  */
+int
+i386_region_ok_for_watchpoint (CORE_ADDR addr, int len)
+{
+  /* Compute how many aligned watchpoints we would need to cover this
+     region.  */
+  int nregs = i386_handle_nonaligned_watchpoint (WP_COUNT, addr, len,
+						 hw_write);
+
+  return nregs <= 4 ? 1 : 0;
+}
+
+/* If the inferior has some watchpoint that triggered, return the
+   address associated with that watchpoint.  Otherwise, return
+   zero.  */
+CORE_ADDR
+i386_stopped_data_address (void)
+{
+  int i;
+  CORE_ADDR ret = 0;
+
+  dr_status_mirror = I386_DR_LOW_GET_STATUS ();
+
+  ALL_DEBUG_REGISTERS(i)
+    {
+      if (I386_DR_WATCH_HIT (i)
+	  /* This second condition makes sure DRi is set up for a data
+	     watchpoint, not a hardware breakpoint.  The reason is
+	     that GDB doesn't call the target_stopped_data_address
+	     method except for data watchpoints.  In fact, if this
+	     function returns non-zero for a hardware breakpoint, GDB
+	     will become confused.  You _have_ been warned!  */
+	  && I386_DR_GET_RW_LEN (i) != 0)
+	{
+	  ret = dr_mirror[i];
+	  if (maint_show_dr)
+	    i386_show_dr ("watchpoint_hit", ret, -1, hw_write);
+	}
+    }
+  if (maint_show_dr && ret == 0)
+    i386_show_dr ("stopped_data_addr", 0, 0, hw_write);
+
+  return ret;
+}
+
+/* Return non-zero if the inferior has some break/watchpoint that
+   triggered.  */
+int
+i386_stopped_by_hwbp (void)
+{
+  int i;
+
+  dr_status_mirror = I386_DR_LOW_GET_STATUS ();
+  if (maint_show_dr)
+    i386_show_dr ("stopped_by_hwbp", 0, 0, hw_execute);
+
+  ALL_DEBUG_REGISTERS(i)
+    {
+      if (I386_DR_WATCH_HIT (i))
+	return 1;
+    }
+
+  return 0;
+}
+
+/* Insert a hardware-assisted breakpoint at address ADDR.  SHADOW is
+   unused.  Return 0 on success, EBUSY on failure.  */
+int
+i386_insert_hw_breakpoint (CORE_ADDR addr, void *shadow)
+{
+  unsigned len_rw = i386_length_and_rw_bits (1, hw_execute);
+  int retval = i386_insert_aligned_watchpoint (addr, len_rw) ? EBUSY : 0;
+
+  if (maint_show_dr)
+    i386_show_dr ("insert_hwbp", addr, 1, hw_execute);
+
+  return retval;
+}
+
+/* Remove a hardware-assisted breakpoint at address ADDR.  SHADOW is
+   unused.  Return 0 on success, -1 on failure.  */
+int
+i386_remove_hw_breakpoint (CORE_ADDR addr, void *shadow)
+{
+  unsigned len_rw = i386_length_and_rw_bits (1, hw_execute);
+  int retval = i386_remove_aligned_watchpoint (addr, len_rw);
+
+  if (maint_show_dr)
+    i386_show_dr ("remove_hwbp", addr, 1, hw_execute);
+
+  return retval;
+}
+
+#endif /* TARGET_HAS_HARDWARE_WATCHPOINTS */
+
      
 #ifdef I386V4_SIGTRAMP_SAVED_PC
 /* Get saved user PC for sigtramp from the pushed ucontext on the stack
@@ -1014,4 +1667,15 @@ and the default value is \"att\".",
      in the disassembly_flavor variable */
 
   set_disassembly_flavor ();
+
+  /* A maintenance command to enable printing the internal DRi mirror
+     variables.  */
+  add_set_cmd ("show-debug-regs", class_maintenance,
+	       var_boolean, (char *) &maint_show_dr,
+	       "\
+Set whether to show variables that mirror the x86 debug registers.\n\
+Use \"on\" to enable, \"off\" to disable.\n\
+If enabled, the debug registers values are shown when GDB inserts\n\
+or removes a hardware breakpoint or watchpoint, and when the inferior\n\
+triggers a breakpoint or watchpoint.", &maintenancelist);
 }
--- gdb/config/i386/tm-i386.h~0	Thu Jan  4 17:46:20 2001
+++ gdb/config/i386/tm-i386.h	Sat Mar  3 17:50:52 2001
@@ -1,5 +1,5 @@
 /* Macro definitions for GDB on an Intel i[345]86.
-   Copyright (C) 1995, 1996, 2000 Free Software Foundation, Inc.
+   Copyright (C) 1995, 1996, 2000, 2001 Free Software Foundation, Inc.
 
    This file is part of GDB.
 
@@ -418,4 +418,97 @@ extern void print_387_status_word (unsig
 
 #define SP_ARG0 (1 * 4)
 
+
+
+/* Hardware-assisted breakpoints and watchpoints.  */
+
+#ifdef TARGET_HAS_HARDWARE_WATCHPOINTS
+
+/* Clear the reference counts and forget everything we knew about DRi.  */
+extern void i386_cleanup_dregs (void);
+
+/* Insert a watchpoint to watch a memory region which starts at
+   address ADDR and whose length is LEN bytes.  Watch memory accesses
+   of the type TYPE.  Return 0 on success, -1 on failure.  */
+extern int  i386_insert_watchpoint (CORE_ADDR addr, int len, int type);
+
+/* Remove a watchpoint that watched the memory region which starts at
+   address ADDR, whose length is LEN bytes, and for accesses of the
+   type TYPE.  Return 0 on success, -1 on failure.  */
+extern int  i386_remove_watchpoint (CORE_ADDR addr, int len, int type);
+
+/* Return non-zero if we can watch a memory region that starts at
+   address ADDR and whose length is LEN bytes.  */
+extern int  i386_region_ok_for_watchpoint (CORE_ADDR addr, int len);
+
+/* Return non-zero if the inferior has some break/watchpoint that
+   triggered.  */
+extern int  i386_stopped_by_hwbp (void);
+
+/* If the inferior has some break/watchpoint that triggered, return
+   the address associated with that break/watchpoint.  Otherwise,
+   return zero.  */
+extern CORE_ADDR i386_stopped_data_address (void);
+
+/* Insert a hardware-assisted breakpoint at address ADDR.  SHADOW is
+   unused.  Return 0 on success, EBUSY on failure.  */
+extern int  i386_insert_hw_breakpoint (CORE_ADDR addr, void *shadow);
+
+/* Remove a hardware-assisted breakpoint at address ADDR.  SHADOW is
+   unused.  Return 0 on success, -1 on failure.  */
+extern int  i386_remove_hw_breakpoint (CORE_ADDR addr, void *shadow);
+
+/* Returns the number of hardware watchpoints of type TYPE that we can
+   set.  Value is positive if we can set CNT watchpoints, zero if
+   setting watchpoints of type TYPE is not supported, and negative if
+   CNT is more than the maximum number of watchpoints of type TYPE
+   that we can support.  TYPE is one of bp_hardware_watchpoint,
+   bp_read_watchpoint, bp_write_watchpoint, or bp_hardware_breakpoint.
+   CNT is the number of such watchpoints used so far (including this
+   one).  OTHERTYPE is non-zero if other types of watchpoints are
+   currently enabled.
+
+   We always return 1 here because we don't have enough information
+   about possible overlap of addresses that they want to watch.  As
+   an extreme example, consider the case where all the watchpoints
+   watch the same address and the same region length: then we can
+   handle a virtually unlimited number of watchpoints, due to debug
+   register sharing implemented via reference counts in i386-tdep.c.  */
+
+#define TARGET_CAN_USE_HARDWARE_WATCHPOINT(type, cnt, ot) 1
+
+/* Returns non-zero if we can use hardware watchpoints to watch a region
+   whose address is ADDR and whose length is LEN.  */
+
+#define TARGET_REGION_OK_FOR_HW_WATCHPOINT(addr,len) \
+	i386_region_ok_for_watchpoint(addr,len)
+
+/* After a watchpoint trap, the PC points to the instruction after the
+   one that caused the trap.  Therefore we don't need to step over it.
+   But we do need to reset the status register to avoid another trap.  */
+
+#define HAVE_CONTINUABLE_WATCHPOINT
+
+#define STOPPED_BY_WATCHPOINT(W)       (i386_stopped_data_address () != 0)
+
+#define target_stopped_data_address()  i386_stopped_data_address ()
+
+/* Use these macros for watchpoint insertion/removal.  */
+
+#define target_insert_watchpoint(addr, len, type)  \
+  i386_insert_watchpoint (addr, len, type)
+
+#define target_remove_watchpoint(addr, len, type)  \
+  i386_remove_watchpoint (addr, len, type)
+
+#define target_insert_hw_breakpoint(addr, shadow)  \
+  i386_insert_hw_breakpoint(addr, shadow)
+
+#define target_remove_hw_breakpoint(addr, shadow)  \
+  i386_remove_hw_breakpoint(addr, shadow)
+
+#define DECR_PC_AFTER_HW_BREAK 0
+
+#endif /* TARGET_HAS_HARDWARE_WATCHPOINTS */
+
 #endif /* ifndef TM_I386_H */

Re: [RFC] Unified watchpoints for x86 platforms

[Eli Zaretskii]

> From: jtc@redback.com (J.T. Conklin)
> Date: 15 Feb 2001 14:45:16 -0800
> 
> We're going to need to pass a PID, or perhaps some new representation
> of a execution context, to a lot of code functions that don't allready
> have such an argument.

Sorry, I'm not sure I'm following: why do you envision we'll need to
pass the PID to functions that don't receive it today?  What
function(s) did you have in mind?

The current watchpoint implementation on i386v-nat.c, for example,
does accept PID as an argument.

Re: [RFC] Unified watchpoints for x86 platforms

[Kevin Buettner]

On Feb 16,  2:29am, Eli Zaretskii wrote:

> > We're going to need to pass a PID, or perhaps some new representation
> > of a execution context, to a lot of code functions that don't allready
> > have such an argument.
> 
> Sorry, I'm not sure I'm following: why do you envision we'll need to
> pass the PID to functions that don't receive it today?  What
> function(s) did you have in mind?

The idea (I think) is to make most uses of ``inferior_pid'' go away. 
In its place will be occurrences of PIDGET (ptid) (or something along
these lines) where ptid is passed from somewhere else.  As a result,
it will very likely become necessary to pass the execution context (or
perhaps an identifier representing the execution context) as a
parameter or perhaps as a member of some larger structure.

See http://sources.redhat.com/ml/gdb-patches/2000-10/msg00014.html

This patch doesn't address the elimination of inferior_pid, but it
does take a first step towards providing a more robust execution
context identifier in the sense that it is now possible to encode a
process id, thread id, and lightweight process id in one of these
identifiers.  At the moment, we use a single int to encode the
following:

 - a process id (alone)
 - a process id and a thread id
 - a process id and an lwp id

IIRC, there are also times where the int in question ends up
containing just the thread id or just the lwp id.

Kevin

Re: [RFC] Unified watchpoints for x86 platforms

[J.T. Conklin]

>>>>> "Eli" == Eli Zaretskii <eliz@delorie.com> writes:
>> From: jtc@redback.com (J.T. Conklin)
>> Date: 15 Feb 2001 14:45:16 -0800
>> 
>> We're going to need to pass a PID, or perhaps some new representation
>> of a execution context, to a lot of code functions that don't allready
>> have such an argument.

Eli> Sorry, I'm not sure I'm following: why do you envision we'll need to
Eli> pass the PID to functions that don't receive it today?  What
Eli> function(s) did you have in mind?

I was speaking in generalities.  

But if GDB is ever going to be able to debug multiple independent
processes (perhaps with multiple threads) as has been a stated long
term goal, we are going to have to entirely revamp how a "execution
context" is represented, and how target functions know what context
they are operating on.  IMO, an inferior_pid global, and passing pids
to various functions is not enough.

        --jtc

-- 
J.T. Conklin
RedBack Networks

Re: [RFC] Unified watchpoints for x86 platforms

[Mark Kettenis]

   From: jtc@redback.com (J.T. Conklin)
   Date: 15 Feb 2001 14:45:16 -0800

   >>>>> "Eli" == Eli Zaretskii <eliz@delorie.com> writes:
   >> > Is there any particular reason why you need the PID argument?  AFAICS
   >> > it will always be equal to INFERIOR_PID, so I think we can do without
   >> > it.  This is also true for the other i386_hwbp_* functions you're
   >> > proposing.
   >> 
   >> I think it'd be better to not rely on ``inferior_pid''.  I would
   >> rather see the explicitly passed.  There will come a day when GDB
   >> is able to debug more than one process at a time and to perpetuate
   >> reliance on inferior pid would be short sighted.

   Eli> I have two opposite opinions here.  We need to resolve this somehow.

   We're going to need to pass a PID, or perhaps some new representation
   of a execution context, to a lot of code functions that don't allready
   have such an argument.  It is not clear to me that adding such an
   argument "because it will be needed" is correct, considering that the
   design has not yet started.  The truth is we don't know "what" will be
   needed, so we'll have to revisit this function (among many others)
   down the line anyway.

I agree with J.T. here.

Mark

Re: [RFC] Unified watchpoints for x86 platforms

[H . J . Lu]

On Thu, Feb 15, 2001 at 01:46:27PM +0200, Eli Zaretskii wrote:
> Ping!
> 
> No one posted any approvals or disprovals of this design.  Do I take
> the silence as a sign of agreement and start coding?
>  

Why not?


H.J.