Re: [RFA]: Change to_stopped_data_address ABI

Eli Zaretskii
2004-10-06T16:09:59Z

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