Add a non-blocking interface to check_gdb_interrupt

[keiichiw]

The current SingleThreadOps::resume takes a callback check_gdb_interrupt which is expected to be called periodically to check if an interrupt packet is sent by the GDB client.
Instead of this design, can we have an API that lets us block on a select(), poll() or a similar syscall?

[daniel5151]

Yeah, this is something I've been thinking about as well.

More generally, this would fall under the Roadmap item "Exposing an async/await interface", which isn't necessarily select() or poll() specific (as those aren't available on all platforms / on no_std). I'll go ahead and change the issue title accordingly.

---

I haven't put too much thought into the implementation, but one simple approach might be to simply plumb-through the underlying Connection object via the check_gdb_interrupt type. e.g: if the Connection is a TcpStream, then check_gdb_interrupt could be modified to have a .as_conn(&mut) -> &mut C method. This would allow you to use whatever non-blocking wait mechanism the transport provides, including select()/poll(), and then call the .check() method once new data is available.

The tricky thing is how exactly that C type parameter should be plumbed-through into the Target. At the moment, the Target trait it completely independent of the Connection trait, which is important, as it allows a Target to work transparently with any connection. Adding an .as_conn method would have the domino-effect of requiring Target implementations to be connection-specific, which isn't the end of the world, but it's not something I'm particularly excited about.

---

The approach that I'm leaning towards would be to modify the Connection trait to implement Future<Output = u8>, and re-write the check_gdb_interrupt as a Future<Output = ()>. This is nice, as it doesn't leak any Connection specific details through the Target.

Individual Connection implementations could then decide how exactly they should implement the Future::poll method. Simple implementations might just use the transport's non-blocking peek() method to return Poll::Pending/Poll::Ready (i.e: what the current system does). More advanced implementation could use something like select() or poll() to block the thread, waiting for new messages to arrive.

---

The annoying part with both of these approaches is that I foolishly leaked the implementation details of check_gdb_interrupt into the Target interface by explicitly specifying that it's a &mut dyn FnMut() -> bool. I should have used an opaque wrapper type instead (e.g: CheckGdbInterrupt(&mut dyn FnMut() -> bool) with a .check() method), similar to what I ded in fec620c. Because of this, adding any sort of non-blocking check will require making a breaking API change, which isn't the end of the world, but nonetheless quite unfortunate. Ah well...

---

I'm not entirely sure when I'll have time to play around with these changes, but hopefully I've given you some idea of how you might tackle the problem.

The first approach is pretty quick-and-dirty, but if you're just looking to get something up and running locally, it might be the way to go. Alternatively, if you've got some time to spare and are willing to keep working on gdbstub, I'd love it if you could take a shot at implementing the second approach!

[daniel5151]

I found some time to sketch out a rough implementation of what this API might look like in the async-interrupt branch.

While the user-facing bits of the API are pretty clean, the internal implementation is an absolute mess. Check out the code in the vCont hander in gdbstub_impl - yikes! As expected, getting this feature in will require some breaking API changes, but that's to be expected.

One fundamental issue I encountered while working on this implementation is that I'd still want to support running gdbstub over non-async interfaces (such as std::net::TcpStream, or UARTs), while avoiding adding a whole new AsyncConnection trait. The current implementation in the async-interrupt does fulfill this design requirement through the use of (you guessed it) some Connection IDETs, but this has the side-effect of requiring some very gnarly code in the vCont handler. As for code bloat - the size of the no_std example went up by 300 bytes, which isn't too bad given the extent of the changes.

Long story short - while I'm not ready to merge any code to master just yet, I do have a fairly concrete idea of how to implement this API, and hope to find some more time and energy to work on it at some point down the line.

[daniel5151]

I just merged a new top-level state-machine based API to gdbstub into the dev/0.6 branch. While the state-machine API was initially conceived as a way to enable using gdbstub at the bare-metal (see #56), it ended up inadvertently solving a lot of lingering API design questions I've had in the back of my mind for a while now - including the one!

To quote the new GdbStub::run docs:

Using the more advanced GdbStubStateMachine API (alongside deferred
stop reasons) makes it possible to lift the responsibility of checking
for GDB Ctrl-C interrupts out of the target's resume implementation
(i.e: polling the callback function), and into the top-level gdbstub
event loop.

A key consequence of lifting this responsibility up the call-stack is
that the gdbstub event loop knows the concrete Connection type
being used, enabling implementations to leverage whatever
transport-specific efficient waiting mechanism it exposes. Compare this
with polling for interrupts in the target's resume implementation,
where the method doesn't have access to the the concrete Connection
type being used.

As such, once release 0.6 is published to crates.io, I'll be closing this issue.