Re: [RFA]: Change to_stopped_data_address ABI

Andrew Cagney
2004-09-03T21:20:31Z

> 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