Add mips stub

[bet4it]

You don't want to do this, let me help you🙂

[daniel5151]

Thanks for pulling these changes in - now I can properly examine and understand the behavior on MIPS.

And after playing around with it, I finally understand the point you were trying to get through in daniel5151/gdbstub#92. Namely: MIPS is an interesting case where the mainline GDB client never uses the s vCont packet, regardless if vCont? return support for s;S.

Fascinating.

Presumably, the rationale was "well, MIPS hardware doesn't have hardware assisted single step, so why even consider that code path in the GDB client? Keep the code simple, and just always use the breakpoint + continue approach!". Of course, as it turns out, not all remote targets need abide by the rules of "real" hardware. e.g: GDB stubs can be very useful in emulated contexts as well, where you can easily support hardware-level single step (even on a MIPS platform).

Arguably, this is something the upstream GDB client might want to look into fixing / adding support for. While not strictly a bug, I'd argue that if a MIPS target does support single-step via vCont? (e.g: in the case of a MIPS emulator), then the GDB client should use make use of that functionality (i.e: without resorting to a more heavyweight breakpoint + continue flow).

---

Now, as to how this related back to the various questions / concerns you had regarding supports_optional_single_step: based on this information, the MIPS arch in gdbstub_arch should be updated to have this set to true, as single step support is flat out disabled!

And to address your concern in daniel5151/gdbstub#92 (comment):

And if a user uses gdbstub on MIPS, he will find that step still can't work even if he implements the step function, because GDB will only use breakpoints to implement single step on such architectures, and doesn't consider if you support step by yourself. You use supports_optional_single_step to make sure step can work, then do we need another guard rail on MIPS to enforce the user implement breakpoint so step can work?

I finally understand what you meant by this.

And in this case, the guard rail that covers this is use_implicit_sw_breakpoints. i.e: on MIPS, if the user hasn't implemented breakpoints, then they will get a loud, angry error that says "hey, GDB will try to set breakpoints by overriding memory! this probably isn't what you want, so you should implement software breakpoint support!"

That said, I think there is merit to making this point more obvious, so I'll update the docs of use_implicit_sw_breakpoints to mention that GDB has multiple commands that set breakpoints - not just the explicit b command. e.g: adding some docs like:

Note that GDB may set breakpoints as part of other commands. e.g: Attempting to single-step (via stepi) on a target without single-step support will result in the GDB client "emulating" the single-step by setting a temporary breakpoint.

---

Now, an interesting follow-up question is whether it makes sense to extend supports_optional_single_step to support this "third scenario" that MIPS falls under. i.e: instead of returning a boolean true or false, it returns a SingleStepGdbBehavior enum that looks something like:

/// Describes how the mainline GDB client handles single-step support (or lack thereof) on a particular architecture.
enum SingleStepGdbBehavior {
    /// GDB will use single-stepping if available.
    ///
    /// e.g: ARM
    Optional,
    /// GDB will unconditionally send single-step packets, and the target is _required_ to handle these requests.
    ///
    /// e.g: x86/x64
    Required,
    /// GDB never uses single-stepping, regardless if it's supported by the stub.
    ///
    /// e.g: MIPS
    Ignored, 
}

If we do this, then we can add some other guard-rail code to warn users that've implemented the single-step IDET while using a platform where it will never be invoked. i.e: compliment the existing use_optional_single_step override with a use_ignored_single_step which must be explicitly overridden to true in the case that a user wishes to implement single-step on a platform that the GDB client will ignore it on (e.g: MIPS).

[daniel5151]

I've updated this PR with some cleanup + README updates. Please let me know if things look good, and I'll merge ASAP.

[bet4it]

instead of returning a boolean true or false, it returns a SingleStepGdbBehavior enum

Yeah! That's what I want. And the name supports_optional_single_step also need to be changed? So I said:

I just don't want to see the situation that you find the name of supports_optional_single_step is improper and need to change it after you release 0.6.

---

If we do this, then we can add some other guard-rail code to warn users that've implemented the single-step IDET while using a platform where it will never be invoked. i.e: compliment the existing use_optional_single_step override with a use_ignored_single_step which must be explicitly overridden to true in the case that a user wishes to implement single-step on a platform that the GDB client will ignore it on (e.g: MIPS).

It's better than now, but the thing gets to be a bit too complicated, so I suggested that we can just remove all the guard rails and let the user choose what they want (Oh, I know you won't accept this, so just ignore me!)

---

I've updated this PR with some cleanup + README updates. Please let me know if things look good, and I'll merge ASAP.

LGTM😃

[daniel5151]

Alrighty, I've updated gdbstub with those changes I outlined above, and things seem to be working great.

I've also tweaked the method names + docs to be very clear about their purposes (i.e: including the term guard_rail_ right in the method name), and I think the docs + error messages will do a reasonably good job at funneling folks towards the right solution.

Note that one thing included in these changes that I didn't mention in the comment above is the addition of SingleStepGdbBehavior::Unknown, which is the default value returned by any Arch implementation that doesn't explicitly set this value.

Ideally, we want all Arch implementations in gdbstub_arch to document how the GDB client reacts to the presence / lack of single-step support, but since I can't test all the existing arches myself, and using a default value of Required/Optional/Ignored would be flat-out incorrect, this new Unknown variant is a placeholder that pushes users towards contributing back to gdbstub_arch.

---

Thanks again for putting in the legwork to provide a working example of this behavior. English is a messy way to exchange ideas and concepts - code seems to work much better 😄

[bet4it]

Ideally, we want all Arch implementations in gdbstub_arch to document how the GDB client reacts to the presence / lack of single-step support, but since I can't test all the existing arches myself, and using a default value of Required/Optional/Ignored would be flat-out incorrect, this new Unknown variant is a placeholder that pushes users towards contributing back to gdbstub_arch.

Why you can't test all the existing arches by yourself? All you need is just copy & paste gdb_arm.rs to other arches with little changes. You can compare gdb_arm.rs and gdb_mips.rs to see the tiny difference between them.

[daniel5151]

Yeah... I admit, compared to the last time something like this happened (i.e: the introduction of RegId), there is certainly far less "leg work" to test these changes.

That said, i'm back on the 9-5 grind after a nice long weekend, which means I'm back to being busy. My plan now is to find an hour to squeeze in some last minute fixes (I discovered that watchpoints have been kinda broken in multithreaded contexts for a while now...), and finally push 0.6 out the door. It's loooong overdue for a release, and I hate having it on my backlog.

That said, given that these changes would all be non-breaking and land in gdbstub_arch, it's fine to have some Unknowns for now and update things as folks have time to look into each arch.