Re: [PATCH 1/2] [gdbserver] Rename supports_conditional_breakpoints to supports_hardware_single_step

[Antoine Tremblay <antoine.tremblay@ericsson.com>]

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?