Remove / Rename synthetic `DoneStep` stop reason

[jonathanzetier]

While developing a PR for multiprocess support and testing it with the armv4t_multithreaded example project, I found that every time I switched to thread 2 (the coprocessor thread) and stepped, the client would switch back to thread 1 (the cpu thread). Re-checking out dev/0.8 and I confirmed it still exists.

With set debug remote on in the GDB client, here's the most relevant excerpt that shows stepping thread 2 (while continuing the other processes):

[remote] Sending packet: $vCont;s:p1.2;c:p1.-1#f8
[remote] wait: enter
[remote] wait: exit
[remote] wait: enter
  [remote] Packet received: S05
  [remote] select_thread_for_ambiguous_stop_reply: enter
    [remote] select_thread_for_ambiguous_stop_reply: process_wide_stop = 0
    [remote] select_thread_for_ambiguous_stop_reply: first resumed thread is Thread 1.1
    [remote] select_thread_for_ambiguous_stop_reply: is this guess ambiguous? = 1
warning: multi-threaded target stopped without sending a thread-id, using first non-exited thread
  [remote] select_thread_for_ambiguous_stop_reply: exit

Granted, this was sent with scheduler-locking mode off (with scheduler-locking mode on, thread 2 remains the active thread), which means that the scheduler could schedule anything, and so I'm not complaining about the fact that the thread switches in this case, for armv4t_multithreaded.

However, stubs might be able to provide more information to the GDB client (and this also happens when I step with gdb and gdbserver locally): the stub might be able to say that the step concluded and also say that the thread that was requested to be stepped actually was. This should make the GDB client user's experience a little better, since the likeliest case of a step is that the step concluded before any other thread could stop, and the user wouldn't have to keep switching back to the thread they're actually trying to work with.

It seems like giving DoneStep an Option<Tid> (or adding a DoneStepWithThread, like with Signal) would give stubs the ability to do this. If we're already breaking the API in v0.8 for multiprocess support, this might be another quick+simple to migrate breakage we could sneak in too. A current workaround exists of issuing a SwBreak or a HwBreak instead of a DoneStep, but this isn't intuitive and seems like it would be easier for Target implementations to misuse.

[daniel5151]

Truthfully, I think a better approach would be to remove the DoneStep stop reason entirely 😅

DoneStep is just an alias for StopReason::Signal(Signal::SIGTRAP). The GDB RSP doesn't have a special step-specific stop reason. This is an entirely gdbstub-level abstraction... and not a particularly useful one at that.

The variant has been around for a looooong time, and doesn't really jive with gdbstub's more recent design philosophy (as discussed in #124 (comment)). It was a reasonable shortcut when I was only doing single-threaded stuff, and worked "okay" in multi-thread mode... but even then, it feel like it's a bit of an anti-pattern to have gdbstub nudge people towards sending back less context vs. more context.

With that in mind: lets go remove the DoneStep stop reason, update docs / examples to use the Signal/SignalWithThread appropriately, and call out the consequences of omitting the thread-id where appropriate.

[daniel5151]

Then again... maybe it's worth including a simple synthetic stop reason that wraps StopReason::Signal(Signal::SIGTRAP), but calling it something other than DoneStep?

e.g: something like StopReason::SimpleStop that can be used to get something up and running with minimal thought / decision fatigue, with some docs pointing out that it should be replaced with a more nuanced stop reason at some later point during the integration process?

[jonathanzetier]

I just came up from a rabbithole that makes me think sending something like a SimpleStop with no thread ID might be asking for trouble. Here's another log of gdb attached to armv4t_multithread (on the dev/0.8 branch, with GDB 12.1):

$ gdb-multiarch --ex 'target extended-remote 127.0.0.1:9001'
Remote debugging using 127.0.0.1:9001
warning: No executable has been specified and target does not support
determining executable automatically.  Try using the "file" command.
0x55550000 in ?? ()
(gdb) c
Continuing.
^Cwarning: multi-threaded target stopped without sending a thread-id, using first non-exited thread

Thread 1 received signal SIGINT, Interrupt.
0x55550030 in ?? ()
(gdb) i thr
  Id   Target Id            Frame 
* 1    Thread 1.1 (CPU Cpu) 0x55550030 in ?? ()
  2    Thread 1.2 (CPU Cop) 0x55550068 in ?? ()
(gdb) thr 2
[Switching to thread 2 (Thread 1.2)]
#0  0x55550068 in ?? ()
(gdb) nexti

Thread 1 received signal SIGTRAP, Trace/breakpoint trap.
[Switching to Thread 1.1]
0x5555006c in ?? ()
(gdb) i thr
  Id   Target Id            Frame 
* 1    Thread 1.1 (CPU Cpu) 0x5555006c in ?? ()
  2    Thread 1.2 (CPU Cop) 0x5555006c in ?? ()
(gdb) maintenance flush register-cache 
Register cache flushed.
(gdb) i thr
  Id   Target Id            Frame 
* 1    Thread 1.1 (CPU Cpu) 0x55550034 in ?? () # Note that this is different than what it was before flushing the cache!
  2    Thread 1.2 (CPU Cop) 0x5555006c in ?? ()
(gdb)

What seems to be happening under the hood after the nexti command:

1. The current_mem_tid is set to thread 2.
2. The step completes, and sends back a thread ID-less stop reply.
3. GDB doesn't know what thread it got back, so it just arbitrarily picks thread 1
4. GDB doesn't send an Hg for this thread, it assumes that the stub is on thread 1. It gets the register state for what it thinks is thread 1, but the stub never had its current_mem_tid modified, so the stub replies with the registers for thread 2.
5. GDB then manually switches to thread 2 to get registers for thread 2 (since it thought it just got thread 1's registers). It gets the same exact registers.
6. Manually flushing the register cache tells GDB to manually ask for thread 1's registers again, so the Hg message gets sent, and the stub correctly sends thread 1's registers back.

I haven't looked more broadly to see if similar behavior occurs for any stop reasons that don't include a thread (which seem to be Signal, Exited, Terminated, ReplayLog, and CatchSyscall; Exited/Terminated we probably don't need to worry about, it would be weird for GDB to ask for the register state of processes that no longer exist, I'm not as familiar with ReplayLog or CatchSyscall behavior to tell for sure, Signal seems like it could be something to worry about; if one thread segfaults then getting registers for a totally different thread and needing to manually flush the cache with a diagnostic command is annoying and would probably take more time to track down than it would save by using it in the first place).

[daniel5151]

Excellent sleuthing!

This is a great example where gdbstub can tighten up an API to avoid exposing end users to what looks to be a pretty serious GDB RSP foot-gun.

Just because the protocol allows sending back a stop reason without a thread-id doesn't mean we strictly need to expose that capability (or, at least, make it the "easiest" option for users). I think it's fair to say that, in practice, users will always know the precise thread-id that an event came from (and if for whatever reason they don't - better to have that arbitrary choice made by the user's implementation, rather than opaquely by the GDB client itself).

I'm happy to expand the scope of this issue to be "audit / rethink how to handle stop reasons that don't include a thread-id".

---

P.S, semi related: I've been hacking on some stuff under the feat/type-safe-stop-reasons related to the API used to report stop reasons (trying to knock out #195, #185, #51, all in one fell swoop, hah). This is just a "FYI", as those changes might land sooner rather than later. TBD though - got quite a busy schedule myself nowadays, hah.

To be clear, there is no semantic dependency between these two workstreams, but I suspect that one of us will end up having to resolve some kind of merge conflict related to stop reasons at some point in the future 😅

(all the more reason to send out at PR related to fixing up this issue separately, ahead of any larger multi-process mode PRs, I suppose!)