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

[Antoine Tremblay <antoine dot tremblay at ericsson dot 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?