Re: [PATCH v3 07/17] Misc switch_back_to_stepped_thread cleanups

[Doug Evans <dje@google.com>]

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!