[1/2,gdbserver] Rename supports_conditional_breakpoints to supports_hardware_single_step

  In my patch https://sourceware.org/ml/gdb-patches/2015-04/msg01110.html
a new target_ops hook supports_conditional_breakpoints was added to
disable conditional breakpoints if target doesn't have hardware single
step.  This patch is to generalize this hook from
supports_conditional_breakpoints to supports_hardware_single_step,
so that the following patch can use it.

gdb/gdbserver:

2015-09-01  Yao Qi  <yao.qi@linaro.org>

	* linux-low.c (linux_supports_conditional_breakpoints): Rename
	it to ...
	(linux_supports_hardware_single_step): ... New function.
	(linux_target_ops): Update.
	* lynx-low.c (lynx_target_ops): Set field
	supports_hardware_single_step to target_can_do_hardware_single_step.
	* nto-low.c (nto_target_ops): Likewise.
	* spu-low.c (spu_target_ops): Likewise.
	* win32-low.c (win32_target_ops): Likewise.
	* target.c (target_can_do_hardware_single_step): New function.
	* target.h (struct target_ops) <supports_conditional_breakpoints>:
	Remove.  <supports_hardware_single_step>: New field.
	(target_supports_conditional_breakpoints): Remove.
	(target_supports_hardware_single_step): New macro.
	(target_can_do_hardware_single_step): Declare.
	* server.c (handle_query): Use target_supports_hardware_single_step
	instead of target_supports_conditional_breakpoints.
---
 gdb/gdbserver/linux-low.c | 11 +++--------
 gdb/gdbserver/lynx-low.c  |  5 +----
 gdb/gdbserver/nto-low.c   |  5 +----
 gdb/gdbserver/server.c    | 12 +++++++++---
 gdb/gdbserver/spu-low.c   |  2 +-
 gdb/gdbserver/target.c    |  8 ++++++++
 gdb/gdbserver/target.h    | 13 +++++++------
 gdb/gdbserver/win32-low.c |  5 +----
 8 files changed, 31 insertions(+), 30 deletions(-)
  

On 09/01/2015 04:41 AM, Yao Qi wrote:
> In my patch https://sourceware.org/ml/gdb-patches/2015-04/msg01110.html
> a new target_ops hook supports_conditional_breakpoints was added to
> disable conditional breakpoints if target doesn't have hardware single
> step.  This patch is to generalize this hook from
> supports_conditional_breakpoints to supports_hardware_single_step,
> so that the following patch can use it.
>

Could we generalize this even more to supports_single_step like your 
next patch ?

Since I'm working on software single stepping for ARM, if this patch 
goes in I'll need to implement a supports_software_single_step and 
enable ConditionalBreakpoints for this case too...

Is there a need to know that it's really hardware or just knowing that 
it can single step would be ok ?

Regards,
Antoine
  

On 09/02/2015 03:22 PM, Antoine Tremblay wrote:
>
> On 09/01/2015 04:41 AM, Yao Qi wrote:
>> In my patch https://sourceware.org/ml/gdb-patches/2015-04/msg01110.html
>> a new target_ops hook supports_conditional_breakpoints was added to
>> disable conditional breakpoints if target doesn't have hardware single
>> step.  This patch is to generalize this hook from
>> supports_conditional_breakpoints to supports_hardware_single_step,
>> so that the following patch can use it.
>>
>
> Could we generalize this even more to supports_single_step like your
> next patch ?
>
> Since I'm working on software single stepping for ARM, if this patch
> goes in I'll need to implement a supports_software_single_step and
> enable ConditionalBreakpoints for this case too...
>
> Is there a need to know that it's really hardware or just knowing that
> it can single step would be ok ?
>

Note also that this way would force supports_conditional_breakpoints to 
check for more than can_hardware_single_step and require a target 
function set manually to 1 on targets that we know have a proper 
software or hardware single step like :

static int
linux_supports_conditional_breakpoints (void)
{
   return the_low_target.supports_conditional_breakpoints ();
}

I had initially added that in my set but we could change it for 
the_low_target_.can_single_step () ?

This way targets that have proper software single step can be handled.
Otherwise every software single step implementation is deemed too simple 
to be used...

Would that seem ok ?
  

On 09/03/2015 11:13 AM, Antoine Tremblay wrote:
>
>
> On 09/02/2015 03:22 PM, Antoine Tremblay wrote:
>>
>> On 09/01/2015 04:41 AM, Yao Qi wrote:
>>> In my patch https://sourceware.org/ml/gdb-patches/2015-04/msg01110.html
>>> a new target_ops hook supports_conditional_breakpoints was added to
>>> disable conditional breakpoints if target doesn't have hardware single
>>> step.  This patch is to generalize this hook from
>>> supports_conditional_breakpoints to supports_hardware_single_step,
>>> so that the following patch can use it.
>>>
>>
>> Could we generalize this even more to supports_single_step like your
>> next patch ?
>>
>> Since I'm working on software single stepping for ARM, if this patch
>> goes in I'll need to implement a supports_software_single_step and
>> enable ConditionalBreakpoints for this case too...
>>
>> Is there a need to know that it's really hardware or just knowing that
>> it can single step would be ok ?
>>
>
> Note also that this way would force supports_conditional_breakpoints to
> check for more than can_hardware_single_step and require a target
> function set manually to 1 on targets that we know have a proper
> software or hardware single step like :
>
> static int
> linux_supports_conditional_breakpoints (void)
> {
>    return the_low_target.supports_conditional_breakpoints ();
> }
>
> I had initially added that in my set but we could change it for
> the_low_target_.can_single_step () ?
>
> This way targets that have proper software single step can be handled.
> Otherwise every software single step implementation is deemed too simple
> to be used...
>
> Would that seem ok ?
>
>

After more analysis, I think ConditionalBreakpoints should be enabled 
for software/hardware single step.

But for the vCont s:S I don't think we can assume that gdbserver's 
software single step is better then GDB's, it should be the opposite. 
So it should only be enabled when hardware single step is present so we 
can't do away with supports_hardware_single_step.

So to support all features I suggest we have in GDBServer :

supports_hardware_single_step
supports_software_single_step

Enable ConditionalBreakpoints for supports_hardware_single_step || 
supports_software_single_step

And enable vCont s:s only for supports_hardware_single_step

I can add the supports_software_single_step in my patchset..

Does that sounds right ? So your current patch would be fine..
  

On 09/04/2015 10:40 AM, Antoine Tremblay wrote:
>
>
> On 09/03/2015 11:13 AM, Antoine Tremblay wrote:
>>
>>
>> On 09/02/2015 03:22 PM, Antoine Tremblay wrote:
>>>
>>> On 09/01/2015 04:41 AM, Yao Qi wrote:
>>>> In my patch https://sourceware.org/ml/gdb-patches/2015-04/msg01110.html
>>>> a new target_ops hook supports_conditional_breakpoints was added to
>>>> disable conditional breakpoints if target doesn't have hardware single
>>>> step.  This patch is to generalize this hook from
>>>> supports_conditional_breakpoints to supports_hardware_single_step,
>>>> so that the following patch can use it.
>>>>
>>>
>>> Could we generalize this even more to supports_single_step like your
>>> next patch ?
>>>
>>> Since I'm working on software single stepping for ARM, if this patch
>>> goes in I'll need to implement a supports_software_single_step and
>>> enable ConditionalBreakpoints for this case too...
>>>
>>> Is there a need to know that it's really hardware or just knowing that
>>> it can single step would be ok ?
>>>
>>
>> Note also that this way would force supports_conditional_breakpoints to
>> check for more than can_hardware_single_step and require a target
>> function set manually to 1 on targets that we know have a proper
>> software or hardware single step like :
>>
>> static int
>> linux_supports_conditional_breakpoints (void)
>> {
>>    return the_low_target.supports_conditional_breakpoints ();
>> }
>>
>> I had initially added that in my set but we could change it for
>> the_low_target_.can_single_step () ?
>>
>> This way targets that have proper software single step can be handled.
>> Otherwise every software single step implementation is deemed too simple
>> to be used...
>>
>> Would that seem ok ?
>>
>>
>
> After more analysis, I think ConditionalBreakpoints should be enabled
> for software/hardware single step.
>
> But for the vCont s:S I don't think we can assume that gdbserver's
> software single step is better then GDB's, it should be the opposite. So
> it should only be enabled when hardware single step is present so we
> can't do away with supports_hardware_single_step.
>
> So to support all features I suggest we have in GDBServer :
>
> supports_hardware_single_step
> supports_software_single_step
>
> Enable ConditionalBreakpoints for supports_hardware_single_step ||
> supports_software_single_step
>
> And enable vCont s:s only for supports_hardware_single_step
>
> I can add the supports_software_single_step in my patchset..
>
> Does that sounds right ? So your current patch would be fine..
>

Another option would also be to remove the reinsert_addr implementations 
and have can_hardware_single_step still check if that implementation 
exists. Thus removing the need for a manual 
supports_software_single_step ...

If the implementations were disabled for Conditional Breakpoints, I 
don't see why they should be kept ?

I would prefer that option but maybe there's a scenario I"m not seeing?
  

Antoine Tremblay <antoine.tremblay@ericsson.com> writes:

Hi Antoine,

> Could we generalize this even more to supports_single_step like your
> next patch ?
>

I am not sure.

> Since I'm working on software single stepping for ARM, if this patch
> goes in I'll need to implement a supports_software_single_step and
> enable ConditionalBreakpoints for this case too...

Nowadays, GDBserver only support conditional breakpoint for HW single
step target.  Whether GDBserver support conditional breakpoint for SW
single step target is a separate issue, and we can decide this once we
have SW single step in GDBserver.

I believe GDBserver can compute the next instruction of $PC for SW
single step, but GDBserver execution control (target independent part)
will be more complicated if SW single step is involved in.
  

On 09/08/2015 10:31 AM, Yao Qi wrote:
> Antoine Tremblay <antoine.tremblay@ericsson.com> writes:
>
> Hi Antoine,
>
>> Could we generalize this even more to supports_single_step like your
>> next patch ?
>>
>
> I am not sure.
>
>> Since I'm working on software single stepping for ARM, if this patch
>> goes in I'll need to implement a supports_software_single_step and
>> enable ConditionalBreakpoints for this case too...
>
> Nowadays, GDBserver only support conditional breakpoint for HW single
> step target.  Whether GDBserver support conditional breakpoint for SW
> single step target is a separate issue, and we can decide this once we
> have SW single step in GDBserver.
>

Alright then, I'll base my patchset based on your patch and most likely 
add a supports_software_single_step, if it's in by the time I post most 
likely later this week...

> I believe GDBserver can compute the next instruction of $PC for SW
> single step, but GDBserver execution control (target independent part)
> will be more complicated if SW single step is involved in.
>

Execution control for software single step is already present in 
GDBServer and working fine as far as I know...?
  

> -  /* Although lynx has hardware single step, still disable this
> -     feature for lynx, because it is implemented in linux-low.c instead
> -     of in generic code.  */
> -  NULL,  /* supports_conditional_breakpoints */
> +  target_can_do_hardware_single_step,
>     NULL,  /* stopped_by_watchpoint */
>     NULL,  /* stopped_data_address */
>     NULL,  /* read_offsets */
...
> +
> +/* Target can do hardware single step.  */
> +
> +int
> +target_can_do_hardware_single_step (void)
> +{
> +  return 1;
> +}

Why would we enable conditional breakpoints on lynx,win32 etc.. since I 
think it's still valid that it's implemented in linux-low ?

Should we not default to return 0 in target_can_do_hardware_single_step ?