Re: [RFA]: Change to_stopped_data_address ABI

[Andrew Cagney <cagney at gnu dot org>]

> I am proposing a change to the to_stopped_data_address target vector function. There are two reasons.  The first reason is that there is no way to determine if a platform actually supports to_stopped_data_address.  For example, the S390 supports hardware watchpoints, but doesn't support figuring out which address caused a watchpoint to trigger.  This level of detail is necessary for proper threaded watchpoint support to know whether to trust in the to_stopped_data_address function or whether to check all watchpoints for value changes.
>
> The second reason for the proposed change is that there is no way to watch address 0 since to_stopped_data_address currently returns the address 0 to indicate failure.
>
> 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.

Some notes:

- having an explicit success/fail is definitly a good idea.  We need 
more of that, ya!

- for GDB, rather than a magic calling convention, a separate predicate 
function is used when probing for a method vis:

	if (target_stopped_data_address_p (&current_target))
	  ... target_stopped_data_address (...);

- we're eliminating target macros replacing them with functions
Unfortunatly here we're also fighting existing tm-*.h / nm-*.h macros so 
some wiggling is required.

- we're making the ``target_ops'' an explicit parameter

so with the tweaks listed below (and the problem Eli identified 
addressed) it's ok ....

> Index: target
> ===================================================================
> RCS file: /cvs/src/src/gdb/target.c,v
> retrieving revision 1.78
> diff -u -p -r1.78 target.c
> --- target.c	3 Aug 2004 00:57:26 -0000	1.78
> +++ target.c	31 Aug 2004 18:49:51 -0000
> @@ -127,7 +127,7 @@ static int debug_to_remove_watchpoint (C
>  
>  static int debug_to_stopped_by_watchpoint (void);
>  
> -static CORE_ADDR debug_to_stopped_data_address (void);
> +static int debug_to_stopped_data_address (CORE_ADDR *);
>  
>  static int debug_to_region_size_ok_for_hw_watchpoint (int);
>  
> @@ -522,7 +522,7 @@ update_current_target (void)
>  	    (int (*) (void))
>  	    return_zero);
>    de_fault (to_stopped_data_address,
> -	    (CORE_ADDR (*) (void))
> +	    (int (*) (CORE_ADDR *))
>  	    return_zero);
>    de_fault (to_region_size_ok_for_hw_watchpoint,
>  	    default_region_size_ok_for_hw_watchpoint);
> @@ -1919,16 +1919,22 @@ debug_to_stopped_by_watchpoint (void)
>    return retval;
>  }

> -static CORE_ADDR
> -debug_to_stopped_data_address (void)
> +static int
> +debug_to_stopped_data_address (CORE_ADDR *addr)


>  {
> -  CORE_ADDR retval;
> +  int retval;
>  
> -  retval = debug_target.to_stopped_data_address ();
> +  retval = debug_target.to_stopped_data_address (addr);
>  
> -  fprintf_unfiltered (gdb_stdlog,
> -		      "target_stopped_data_address () = 0x%lx\n",
> -		      (unsigned long) retval);
> +  if (addr == NULL)
> +    fprintf_unfiltered (gdb_stdlog,
> +			"target_stopped_data_address (NULL) = %d\n",
> +			retval);
> +  else
> +    fprintf_unfiltered (gdb_stdlog,
> +			"target_stopped_data_address ([0x%lx]) = %ld\n",
> +			(unsigned long)*addr,
> +			(unsigned long)retval);
>    return retval;
>  }

Add something like (I'm guessing):

int
target_stopped_data_address_p (&current_target)
{
  if zero_func
	return 0;
  else if debug func wrapping zero func
	return 0;
  else
	return 1;
}

> Index: target.h
> ===================================================================
> RCS file: /cvs/src/src/gdb/target.h,v
> retrieving revision 1.60
> diff -u -p -r1.60 target.h
> --- target.h	7 Jun 2004 17:58:32 -0000	1.60
> +++ target.h	31 Aug 2004 18:49:51 -0000
> @@ -342,7 +342,7 @@ struct target_ops
>      int (*to_insert_watchpoint) (CORE_ADDR, int, int);
>      int (*to_stopped_by_watchpoint) (void);
>      int to_have_continuable_watchpoint;
> -    CORE_ADDR (*to_stopped_data_address) (void);
> +    int (*to_stopped_data_address) (CORE_ADDR *);
   int (*to_stopped_data_address) (struct target_ops *, CORE_ADDR *);
And update callers.
>      int (*to_region_size_ok_for_hw_watchpoint) (int);
>      void (*to_terminal_init) (void);
>      void (*to_terminal_inferior) (void);
> @@ -1084,8 +1084,8 @@ extern void (*deprecated_target_new_objf
>  #endif


>  #ifndef target_stopped_data_address
> -#define target_stopped_data_address() \
> -    (*current_target.to_stopped_data_address) ()
Add explicit current target parameter:
> +#define target_stopped_data_address(x) \
> +    (*current_target.to_stopped_data_address) (x)
extern int target_stopped_data_address_p (&current_target);
#else
/* Horrible hack to get around existing macros :-(.  */
#define target_stopped_data-address_p(CURRENT_TARGET) (1)
>  #endif
>  
>  /* This will only be defined by a target that supports catching vfork events,

Andrew