Re: A patch for ia32 hardware watchpoint.

[Eli Zaretskii <eliz@delorie.com>]

> 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.