Allow Target to override Arch's target description

[DrChat]

Continued from #31. Related to #12.
Currently we can only report a single and static target description to GDB that gets determined at compile-time.
This however does not work for systems that determine the running architecture at runtime (e.g. for virtualization).

As such, we can add a new function to Target that can return any arbitrary String description (and by default it will use the old static method).

Something worth debating is whether or not we should return a &'static str or a String from Target::description_xml().
The former doesn't require an extra copy or allocation, while the latter allows a Target to build a custom XML description string.

[daniel5151]

Ah, that's quite some timing you've got there! I'd just responded to your last comment on #31 right as you opened this PR.

I'm a bit swamped with Work / Life at the moment, but I should have time to review / comment on the PR sometime after work today.

That being said, right off the bat, this first pass definitely work work. As you might have guessed from the CI errors, the current changes doesn't pass the #![no_std] test. In Rust lingo, no_std means that the library can run without the standard library, and since the library also doesn't rely on the alloc crate, it also can't use dynamic allocation. That means types like Vec, HashMap, and in this case, String, can't be used.

gdbstub follows a "progressive enhancement" approach to no_std, where the library should be "feature complete" in no_std contexts, and only provide various qaulity-of-life improvements when running in alloc or std environments.

[DrChat]

Yeah - I just saw the errors for no_std :)
It appears that my approach of returning Option<String> won't be an option (heh), so I'll have to make it return &'static str (and later maybe take the approach you described in #12 to build the description with a closure).

Appreciate the write-up as always!

[daniel5151]

Nice 👍
I like that this approach this semver compatible and non-breaking.

Aside from the concrete comment I left on the PR, I do have one broader concern. Namely, this PR doesn't address #12, and will result in additional API churn down the line when the API is tweaked.

Now, to be clear, this isn't a hard blocker, as the changes in this PR are all semver compatible and mirror the current Arch::target_description_xml API. If this is a feature you'd like to use ASAP without maintaining a custom fork, I don't mind merging it in as-is.

That said, the RegId size issue that you pointed out in #31 (comment) will most-likely require a semver breaking change, and would have to land alongside the other 0.5 work. If you're planning on maintaining a fork regardless, it might be better to hold off on upstreaming this feature until #12 is addressed.

As an FYI, I don't have a set deadline for when I'm planning on getting 0.5 out (I'm a bit busy with work / life stuff at the moment), but if I had to guess, it might be another month or two.

I leave it up to you whether you want to merge this now or later.

P.S: Could you open a new issue for those RegId changes? I'd like to track that somewhere, as it is definitely something I'd want to address ahead of 0.5

[daniel5151]

Every other method on Target is an IDET "toggle" method (i.e: fn feature(&mut self) -> Option<FeatureOps<Self>> { None }), with actual functionality split off into a dedicated IDET. For consistency's sake, could you please extract the method into a separate IDET?

[DrChat]

Hmm - do you think this function belongs in BaseOps instead in that case?

[daniel5151]

BaseOps is moreso intended to be a place for "fundamental" GDB operations, i.e: operations that must be implemented to have the GDB client handshake with the server. target_description_xml is technically an optional feature, and doesn't have to be implemented to talk to a GDB client.

inb4 "but what about {read,write}_register - they are optional and have default impls under BaseOps"?

I'd actually realized that inconsistency, and fixed it for 0.5. You can take a peek at the updated API on the dev/0.5 branch

[DrChat]

Haha :)
Hm - where have you moved those functions to? description_xml is not necessary, but a really nice to have, kinda like those functions I suppose.

[daniel5151]

Yes indeed, hence why they were originally part of BaseOps directly :)

The WIP 0.5 transition guide should offer a good starting point: https://github.com/daniel5151/gdbstub/blob/dev/0.5/docs/transition_guide.md#single-register-access-methods-readwrite_register-are-now-a-separate-singleregisteraccess-trait

And yes, while in a perfect world I'd want to see all end-users implement description_xml, the reality of the fact is that folks can be lazy, and one of gdbstub's main selling points is that it's super easy to get a basic debugging session up and running.

[DrChat]

Perfect! I will check out that guide.
Yeah, gdbstub has been very easy to integrate. The hardest part on my project is writing FFI bindings, GDB has been a breeze.

[daniel5151]

Good to hear!

And just to be clear: that transition guide is from 0.4 to the currently unreleased 0.5. Things in that doc are still subject to change, and more pertinently, don't have any relation to this PR (assuming you're hoping to land these changes as part of a hypothetical 0.4.5 release)

[DrChat]

So I'm currently looking at implementing another Ops structure for the target's description, but I now have one question:
How do you implement these ops by default for an Arch that provides a description, so users don't have to manually opt-in to this feature?

Edit: On second thought, I think I found a way. If a Target implements TargetDescriptionOps, the core will use that when returning a string to GDB. Otherwise, it simply falls back to the old method of retrieving the description from the Arch.
The limitation here is that a Target cannot simply disable the description string, though I'm unsure why anyone would want to do this.

[daniel5151]

Hey @DrChat, I managed to claw back some free time this afternoon, so if you don't mind, I'll go ahead and whip up a quick PR for this feature myself. Plus, I've got an idea how to tackle #12 using some fun somewhat-advanced Rust shenanigans, so I don't mind taking over this work.

I'll add you as a reviewer on the PR, so that you can give me feedback on whether or not it'll fit your use-case 😃

[DrChat]

Ah okay, sounds good :)
Take a look at the work I've done on the branch, I got it into a state that I'm happy with. Maybe you can start from this?