The attached patches address your comments below.  Ok now?

-- Jeff J.

Eli Zaretskii wrote:
>>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.
>