[v3,07/17] Misc switch_back_to_stepped_thread cleanups

  On 04/22/2015 06:21 AM, Doug Evans wrote:
> Pedro Alves writes:
>  > [...] it isn't ever correct to pass step=1 to target_resume
>  > on software single-step targets [...]
> 
> Sounds like a good thing to document in a comment or assert.

Yeah, we were discussing adding an assertion on the gdbserver
side here:

  https://sourceware.org/ml/gdb-patches/2015-04/msg00232.html

An assertion on the gdb side is complicated, and I'd rather that be
done separately.  gdbarch_software_single_step_p() returning true
does not mean that we can't use hardware step.  E.g., ppc installs
that hook for stepping past atomic regions.  We can't assert based on
software single-step breakpoints inserted, as infrun.c uses those
even on targets that don't implement gdbarch_software_single_step_p.
We could add a new target_can_hardware_single_step method, which
ideally for remote targets would be based on the reply to the "vCont?"
probe packet, but gdbserver always includes "s" actions in the reply
even on targets that can't hardware step, and we can't just make it
not do that, since remote.c:remote_vcont_probe has:

      /* If s, S, c, and C are not all supported, we can't use vCont.  Clearing
         BUF will make packet_ok disable the packet.  */
      if (!support_s || !support_S || !support_c || !support_C)
	buf[0] = 0;

so even if we fixed mainline, newer gdbserver against older gdb
would be broken...  So we need something else or in addition (probably
based on qSupported).

For comments, how about this?


Thanks,
Pedro Alves
  

Pedro Alves writes:
 > On 04/22/2015 06:21 AM, Doug Evans wrote:
 > > Pedro Alves writes:
 > >  > [...] it isn't ever correct to pass step=1 to target_resume
 > >  > on software single-step targets [...]
 > > 
 > > Sounds like a good thing to document in a comment or assert.
 > 
 > Yeah, we were discussing adding an assertion on the gdbserver
 > side here:
 > 
 >   https://sourceware.org/ml/gdb-patches/2015-04/msg00232.html
 > 
 > An assertion on the gdb side is complicated, and I'd rather that be
 > done separately.  gdbarch_software_single_step_p() returning true
 > does not mean that we can't use hardware step.  E.g., ppc installs
 > that hook for stepping past atomic regions.  We can't assert based on
 > software single-step breakpoints inserted, as infrun.c uses those
 > even on targets that don't implement gdbarch_software_single_step_p.
 > We could add a new target_can_hardware_single_step method, which
 > ideally for remote targets would be based on the reply to the "vCont?"
 > probe packet, but gdbserver always includes "s" actions in the reply
 > even on targets that can't hardware step, and we can't just make it
 > not do that, since remote.c:remote_vcont_probe has:
 > 
 >       /* If s, S, c, and C are not all supported, we can't use vCont.  Clearing
 >          BUF will make packet_ok disable the packet.  */
 >       if (!support_s || !support_S || !support_c || !support_C)
 > 	buf[0] = 0;
 > 
 > so even if we fixed mainline, newer gdbserver against older gdb
 > would be broken...  So we need something else or in addition (probably
 > based on qSupported).
 > 
 > For comments, how about this?
 > 
 > diff --git i/gdb/infrun.c w/gdb/infrun.c
 > index 0bf1274..fbe12ab 100644
 > --- i/gdb/infrun.c
 > +++ w/gdb/infrun.c
 > @@ -5839,7 +5839,9 @@ switch_back_to_stepped_thread (struct execution_control_state *ecs)
 >    return 0;
 >  }
 > 
 > -/* Is thread TP in the middle of single-stepping?  */
 > +/* Is thread TP in the middle of (software or hardware)
 > +   single-stepping?  (Note the result of this function must never be
 > +   passed directly as target_resume's STEP parameter.)  */
 > 
 >  static int
 >  currently_stepping (struct thread_info *tp)
 > diff --git i/gdb/target.h w/gdb/target.h
 > index 66bf91e..283f56f 100644
 > --- i/gdb/target.h
 > +++ w/gdb/target.h
 > @@ -1254,8 +1254,8 @@ extern void target_detach (const char *, int);
 >  extern void target_disconnect (const char *, int);
 > 
 >  /* Resume execution of the target process PTID (or a group of
 > -   threads).  STEP says whether to single-step or to run free; SIGGNAL
 > -   is the signal to be given to the target, or GDB_SIGNAL_0 for no
 > +   threads).  STEP says whether to hardware single-step or to run free;
 > +   SIGGNAL is the signal to be given to the target, or GDB_SIGNAL_0 for no
 >     signal.  The caller may not pass GDB_SIGNAL_DEFAULT.  A specific
 >     PTID means `step/resume only this process id'.  A wildcard PTID
 >     (all threads, or all threads of process) means `step/resume
 > 
 > Thanks,
 > Pedro Alves

Works for me.
Thanks!