Re: [RFC] Unified watchpoints for x86 platforms

[Mark Kettenis <kettenis at wins dot uva dot nl>]

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