This is the mail archive of the
gdb@sourceware.cygnus.com
mailing list for the GDB project.
Re: A patch for ia32 hardware watchpoint.
- To: hjl at valinux dot com
- Subject: Re: A patch for ia32 hardware watchpoint.
- From: Eli Zaretskii <eliz at delorie dot com>
- Date: Wed, 8 Mar 2000 05:12:44 -0500 (EST)
- CC: gdb-patches at sourceware dot cygnus dot com, gdb at sourceware dot cygnus dot com
- References: <20000307132613.B20282@valinux.com>
- Reply-to: Eli Zaretskii <eliz at is dot elta dot co dot il>
> This is a patch for
>
> http://sourceware.cygnus.com/ml/gdb/2000-q1/msg00564.html
I have problems with this patch. See below.
> I only did it for Linux since it is not a perfect solution.
No, you also changed nm-go32.h, which is not related to Linux (AFAIK),
and changed the global definition of two macros on breakpoint.c.
> +#ifdef NEED_WATCHPOINT_NUMBER
> + val = target_insert_watchpoint (b->number, addr,
> + len, type);
> +#else
Please explain why is this needed. The DJGPP version works well
without knowing the breakpoint number, but if there's a good reason
for passing b->number, it should be done on all x86 platforms. So
let's discuss this.
> + /* Don't return an error if we fail to insert
> + a hardware watchpoint due to the limited number
> + of hardware watchpoints availabel. */
> + val = (errno == EBUSY) ? 0 : -1;
> + }
Why is this a good idea? The result of this is that GDB will not know
that it cannot insert a watchpoint until it actually resumes the
debuggee, which is too late in many cases; and the user gets confusing
error messages. x86 doesn't have a good way of checking whether the
debug registers are enough to cover the requests, but whenever it
does, why not use it?
> @@ -5500,13 +5513,13 @@ watch_command_1 (arg, accessflag, from_t
> in hardware return zero. */
>
> #if !defined(TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT)
> -#define TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT(BYTE_SIZE) \
> - ((BYTE_SIZE) <= (REGISTER_SIZE))
> +#define TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT(VAL) \
> + (TYPE_LENGTH (VALUE_TYPE (VAL)) <= (REGISTER_SIZE))
> #endif
>
> #if !defined(TARGET_REGION_OK_FOR_HW_WATCHPOINT)
> -#define TARGET_REGION_OK_FOR_HW_WATCHPOINT(ADDR,LEN) \
> - TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT(LEN)
> +#define TARGET_REGION_OK_FOR_HW_WATCHPOINT(VAL) \
> + TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT(VAL)
> #endif
These are IMHO wrong: the number of debug registers required for a
particular region is a function of the address, not only size (e.g., a
single x86 debug register cannot watch a 32-bit region that isn't
aligned on 4-byte boundary). If Linux, for some reason, doesn't need
the address (although I cannot see how could this be right, at least
for native debugging), please define a platform-specific macro instead
of overwriting system-wide defaults.
The DJGPP version actually *uses* the ADDR part of the above
definition, since it knows how to cover a region with several
watchpoints.
> --- config/i386/nm-go32.h 2000/03/07 18:42:21 1.1.1.2
> +++ config/i386/nm-go32.h 2000/03/07 18:53:48
> @@ -44,10 +44,10 @@
> #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. */
> + which represents VAL. */
>
> -#define TARGET_REGION_OK_FOR_HW_WATCHPOINT(addr,len) \
> - go32_region_ok_for_watchpoint(addr,len)
> +#define TARGET_REGION_OK_FOR_HW_WATCHPOINT(val) \
> + go32_region_ok_for_watchpoint((VALUE_ADDRESS (val) + VALUE_OFFSET (val)),TYPE_LENGTH (VALUE_TYPE (val)))
Please do not commit this one, it disables a valuable feature in the
DJGPP version.