Re: [RFA]: Change to_stopped_data_address ABI

["Eli Zaretskii" <eliz at gnu dot org>]

> Date: Fri, 24 Sep 2004 18:41:53 -0400
> From: Jeff Johnston <jjohnstn@redhat.com>
> Cc: gdb-patches@sources.redhat.com

Sorry for a late response.

> Andrew/Eli, ok to commit?  Did I miss anything?

Thanks, most of the concerns I had with your original patch are now
gone.  Still, a few minor ones remain, see below.

> +int
> +frv_stopped_data_address (CORE_ADDR *addr_p)
>  {
>    CORE_ADDR brr, dbar0, dbar1, dbar2, dbar3;
>  
> @@ -1305,15 +1305,27 @@ frv_stopped_data_address (void)
>    dbar3 = read_register (dbar3_regnum);
>  
>    if (brr & (1<<11))
> -    return dbar0;
> +    *addr_p = dbar0;
>    else if (brr & (1<<10))
> -    return dbar1;
> +    *addr_p = dbar1;
>    else if (brr & (1<<9))
> -    return dbar2;
> +    *addr_p = dbar2;
>    else if (brr & (1<<8))
> -    return dbar3;
> +    *addr_p = dbar3;
>    else
> -    return 0;
> +    {
> +      *addr_p = 0;
> +      return 0;
> +    }
> +
> +  return 1;
> +}

I don't understand why do you put a zero into the address pointed by
addr_p in the case that no watchpoint has fired.  It is customary to
leave the arguments unaltered in such cases.  isn't it enough that you
return a zero as the function's value?

Similar code is in the other functions that return the stopped data
address; I have similar issue with them.

> RCS file: /cvs/src/src/gdb/config/i386/nm-i386.h,v
> retrieving revision 1.6
> diff -u -p -r1.6 nm-i386.h
> --- config/i386/nm-i386.h	13 Sep 2004 14:06:03 -0000	1.6
> +++ config/i386/nm-i386.h	24 Sep 2004 22:34:27 -0000
> @@ -47,10 +47,10 @@ extern int i386_region_ok_for_watchpoint
>     triggered.  */
>  extern int i386_stopped_by_hwbp (void);
>  
> -/* If the inferior has some break/watchpoint that triggered, return
> +/* If the inferior has some break/watchpoint that triggered, set
>     the address associated with that break/watchpoint.  Otherwise,
> -   return zero.  */
> -extern CORE_ADDR i386_stopped_data_address (void);
> +   set the watchpoint address to zero.  Always return true.  */

The last sentence is not true: we return zero (false) sometimes.

> +@findex i386_stopped_by_watchpoint
> +@item i386_stopped_by_watchpoint (void)
> +The macro @code{STOPPED_BY_WATCHPOINT}
> +is set to call this function.  The
> +argument passed to @code{STOPPED_BY_WATCHPOINT} is ignored.  This
> +function uses the same logic as @code{i386_stopped_data_address}.

I'd prefer if we describe the operation of i386_stopped_by_watchpoint
explicitly, not by a reference to the logic of i386_stopped_data_address.