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

[Pedro Alves <palves@redhat.com>]

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