Re: [RFA]: Change to_stopped_data_address ABI

[Jeff Johnston <jjohnstn@redhat.com>]

Eli Zaretskii wrote:
>>Date: Tue, 31 Aug 2004 14:55:13 -0400
>>From: Jeff Johnston <jjohnstn@redhat.com>
>>
>>The proposed change is to change the prototype to be:
>>
>>int
>>to_stopped_data_address (CORE_ADDR *addr_p);
>>
>>If the input pointer is NULL, the function returns non-zero if it works on the 
>>given target, otherwise, it fails by returning 0.  The function also returns 0 
>>if unsuccessful.  By separating out the success/fail code from the address, the 
>>new prototype allows for succeeding and returning any address, including 0.
> 
> 
> Thanks.
> 
> The idea is okay with me, but the code tells a bit different story
> (unless I missed something, in which case I apologize).  From your
> description, I initially understood that you want to allow to return a
> zero address when the watchpoint triggers at that address.  For that,
> if no watchpoint triggered, to_stopped_data_address will return zero
> as its value, not put a NULL pointer into a place pointed to by its
> argument.  That would be okay with me, but your code does something
> different:
>

Perhaps my description isn't clear enough.  The function returns non-zero if 
successful (i.e. true).  It returns 0 (false) to indicate failure or no stopped 
data address.  It does not return an address at all; it is a true return code. 
The address is returned via the CORE_ADDR pointer argument.  One can tell if the 
function works on a platform by passing a NULL pointer.  The default function in 
target.c always returns 0 to indicate failure.

> 
>>@@ -2739,8 +2739,7 @@ bpstat_stop_status (CORE_ADDR bp_addr, p
>> 	struct value *v;
>> 	int found = 0;
>> 
>>-	addr = target_stopped_data_address ();
>>-	if (addr == 0)
>>+	if (!target_stopped_data_address (&addr))
>> 	  continue;
> 
> 
> This seems to say that target_stopped_data_address indeed returns a
> zero value for the case where no watchpoint triggered...
> 
> 
>>+int
>>+i386_stopped_data_address (CORE_ADDR *addr_p)
>> {
>>   CORE_ADDR addr = 0;
>>   int i;
>> 
>>+  if (addr_p == NULL)
>>+    return 1;
>>+
>>   dr_status_mirror = I386_DR_LOW_GET_STATUS ();
>> 
>>   ALL_DEBUG_REGISTERS(i)
>>@@ -593,7 +598,16 @@ i386_stopped_data_address (void)
>>   if (maint_show_dr && addr == 0)
>>     i386_show_dr ("stopped_data_addr", 0, 0, hw_write);
>> 
>>-  return addr;
>>+  *addr_p = addr;
>>+  return 1;
> 
> 
> ...but this returns 1 as the function's value and puts zero where the
> argument points.  Isn't that a contradiction?  And doesn't this code
> in i386_stopped_data_address still disallow support for a watchpoint
> at address zero by retaining the previous magic meaning of a zero
> address?  Or did I miss something?
> 
> 
>>+zero.  When @var{addr_p} is non-NULL, return non-zero if the data address
>>+for the triggered watchpoint is determined successfully, otherwise, return zero.
> 
> 
> I think "watchpoint is determined successfully" is not clear enough.
> Please rewrite the text to say exactly what does the zero value mean.
> The intent is to tell a GDB hacker how to handle the case of zero
> return value from target_stopped_data_address.
>