Re: [RFA] Unified watchpoints for x86 platforms

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

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

Hi Eli,

Sorry for not responding sooner.  I've been suffering from a bad flu
:-(.  But I do have some comcerns though.

> In response to concerns sent to me by Peter Schauer, I introduced a
> new macro, I386_USE_GENERIC_WATCHPOINTS, which targets should define
> to get the new watchpoint code in i386-tdep.c compiled.  Thus, targets
> which don't define that macro, will continue to use their current
> watchpoint support.  This avoids breaking any such targets, in case
> the code I wrote conflicts with what they have now.

That addresses one of my concerns :-).  However, why are you
conditionalizing the code both on I386_USE_GENERIC_WATCHPOINTS and
TARGET_HAS_HARDWARE_WATCHPOINTS?

> In other words, targets which want to use the code I wrote need to say
> "I do" explicitly ;-)

That's good!

I'm still not convinced that this stuff belongs in i386-tdep.c.  While
the code defenitely is an improvement over what's currently in GDB, it
has some limitations.  The fact that you rely on the I386_DR_LOW_*
macro's means that adding these functions to i386-tdep.c is moving us
farther away from multi-arching the i386 stuff.  To those that
suggested putting this code in i386-tdep.c: please take another look
at the support for hardware watchpoints in GDB.  The only real support
for hardware watchpoints is for native targets.

Another limitation is the use of global varaibles to keep reference
counts and debug register mirrors.  This means that the current
implementation can only work for a single target with a single thread
of control.  This is another reason why I'd rather not put this stuff
in i386-tdep.c.

Rather than asking Eli to address these issues, I again suggest
putting them in i386-nat.c.  If Fernando (or somebody else) really
wants this code in i386-tdep.c, he'll have to fix GDB to properly
support hardware watchpoints in a more native-independent fashion first.

Anyway, I also have some comments on the the code itself:

> +#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 */

Please get rid of this crap.  You're completely free to define these
things yourself.  There is absolutely no need to rely on constants
defined in those possibly broken headers.  Simply assume there are
four address registers and the bits are laid out as in the Intel
documentation.

> +/* 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

Remove this crap.  You don't use offsetof() in your code.

> +/* 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);
> +

If these are exported they should live in a public header
(i386-nat.h?), and you shouldn't provide them here.

> +/* 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)

The use of EINVAL seems odd to me since it is used nowhere.  If WHAT
is not a valid value, this should be flagged as an internal error I
think (using gdb_assert() is probably appropriate here.

> +/* 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;
> +}

What's this crap about EBUSY?

Mark