[RFC] design question re: watchpoint target methods

[RFC] design question re: watchpoint target methods

[Joel Brobecker]

Hello everyone,

Jan said:
> [...] I find the single functionality being split into two target
> functions (to_stopped_by_watchpoint and to_stopped_data_address) to be
> confusing.  Chose a new name to easily be able to keep the old
> deprecated implementations working until its host maintainers can get
> to update them as I cannot even compile some of the host files.

I tend to agree because I do not know of any reason why this separation
would be needed.  Any reason why we should reject Jan's suggestion to
have a single target operation instead of two? His proposal is to mark
the following methods as deprecated:

    int (*to_stopped_by_watchpoint) (void);
    int (*to_stopped_data_address) (struct target_ops *, CORE_ADDR *);

And to replace them by by a new operation:

    enum stopped_by_watchpoint (*to_thread_stopped_by_watchpoint)
        (ptid_t ptid, CORE_ADDR *data_address_p);

Actually, I think that all target_ops operations should take a struct
target_ops parameter, even if not needed, at least for consistency, but
also to facilitate transitions if this parameter ever becomes needed
later on.

One difference in the new operation is that it is now explicitly
applicable to a specific ptid rather than being implicitly applicable
to the current_ptid.

enum stopped_by_watchpoint is proposed to be:
> +enum stopped_by_watchpoint
> +  {
> +    stopped_by_watchpoint_no,
> +    stopped_by_watchpoint_yes_address_unknown,
> +    stopped_by_watchpoint_yes_address_known
> +  };

This is not a critical piece to Jan's suggestion, but I thought I would
provide this piece of info, to be complete.

-- 
Joel

Re: [RFC] design question re: watchpoint target methods

[Jan Kratochvil]

On Thu, 29 Oct 2009 15:58:17 +0100, Joel Brobecker wrote:
>     enum stopped_by_watchpoint (*to_thread_stopped_by_watchpoint)
>         (ptid_t ptid, CORE_ADDR *data_address_p);
> 
> Actually, I think that all target_ops operations should take a struct
> target_ops parameter, even if not needed, at least for consistency, but
> also to facilitate transitions if this parameter ever becomes needed
> later on.

OK, I had some wrong assumptions, I see it fits more the targets abstractions,
I agree with adding it there.


Thanks,
Jan

Re: [RFC] design question re: watchpoint target methods

[David Daney]

Joel Brobecker wrote:
> Hello everyone,
> 
> Jan said:
>> [...] I find the single functionality being split into two target
>> functions (to_stopped_by_watchpoint and to_stopped_data_address) to be
>> confusing.  Chose a new name to easily be able to keep the old
>> deprecated implementations working until its host maintainers can get
>> to update them as I cannot even compile some of the host files.
> 
> I tend to agree because I do not know of any reason why this separation
> would be needed.  Any reason why we should reject Jan's suggestion to
> have a single target operation instead of two? His proposal is to mark
> the following methods as deprecated:
> 
>     int (*to_stopped_by_watchpoint) (void);
>     int (*to_stopped_data_address) (struct target_ops *, CORE_ADDR *);
> 
> And to replace them by by a new operation:
> 
>     enum stopped_by_watchpoint (*to_thread_stopped_by_watchpoint)
>         (ptid_t ptid, CORE_ADDR *data_address_p);
> 
> Actually, I think that all target_ops operations should take a struct
> target_ops parameter, even if not needed, at least for consistency, but
> also to facilitate transitions if this parameter ever becomes needed
> later on.
> 
> One difference in the new operation is that it is now explicitly
> applicable to a specific ptid rather than being implicitly applicable
> to the current_ptid.
> 
> enum stopped_by_watchpoint is proposed to be:
>> +enum stopped_by_watchpoint
>> +  {
>> +    stopped_by_watchpoint_no,
>> +    stopped_by_watchpoint_yes_address_unknown,
>> +    stopped_by_watchpoint_yes_address_known
>> +  };

FWIW, I think it would be a good change.

I am somewhat ignorant about the namespace of ptid, but in the presence 
of multi-process debugging many of the target operations need to 
differentiate for which process the operation is intended.  I would hope 
that ptid would serve that purpose.

Certainly  mips-linux-nat.c would be affected by such a change.


Thanks,
David Daney

Re: [RFC] design question re: watchpoint target methods

[Daniel Jacobowitz]

On Thu, Oct 29, 2009 at 07:58:17AM -0700, Joel Brobecker wrote:
> And to replace them by by a new operation:
> 
>     enum stopped_by_watchpoint (*to_thread_stopped_by_watchpoint)
>         (ptid_t ptid, CORE_ADDR *data_address_p);
> 
> Actually, I think that all target_ops operations should take a struct
> target_ops parameter, even if not needed, at least for consistency, but
> also to facilitate transitions if this parameter ever becomes needed
> later on.

This sounds sensible to me.

-- 
Daniel Jacobowitz
CodeSourcery