Add support for memory maps #54
Conversation
There was a problem hiding this comment.
Thank you so much for the PR!
The core functionality looks good, though there are some organizational changes that'll need to happen before this can get merged.
One thing that this PR is missing is an example implementation for the in-tree armv4t example*. The armv4t example includes a "grab bag" of all protocol extensions currently implemented in gdbstub. It doesn't have to be a particularly "robust" implementation, but it should be good enough such that the GDB Client will try to interact with the feature, and someone running the example can confirm that the feature was in-fact used (e.g: by checking the protocol logs using RUST_LOG=trace, or through a eprintln! statement in the handler).
Moreover, having a working example in armv4t makes it super easy to validate whether the feature is working as intended, without having to fully integrate it into a larger project.
* I really ought to update the PR template with a checklist, since I think I have to mention this on each PR haha.
src/gdbstub_impl/ext/base.rs
Outdated
| @@ -143,6 +147,30 @@ impl<T: Target, C: Connection> GdbStubImpl<T, C> { | |||
| } | |||
| HandlerStatus::Handled | |||
| } | |||
| Base::qXferMemoryMapRead(cmd) => { | |||
There was a problem hiding this comment.
You'll have to move this hander out into a separate ext mod.
(meta): once this PR is merged, I'll pop-open a tracking task for myself to de-dupe the qXfer logic between the two packets. some copy-paste is fine for now, but I know that there'll be more of these packets coming down the line, and there'll need to be some way to keep the code DRY.
|
On a more meta note, I'm curious if it might make sense to at switch the signature of the memory_map_xml method to something like: fn memory_map_xml(&self) -> MemoryMap<'_>;where struct MemoryMapRegionKind {
Ram,
Rom,
Flash { blocksize: usize }
}
struct MemoryMapRegion {
start: usize,
length: usize,
kind: MemoryMapRegionKind
}
enum MemoryMap<'a> {
Raw(&'a str),
Structured(&'a [MemoryMapRegion])
}In this second case, the target wouldn't actually care about the actual underlying XML, and could defer those details over to I don't know much about how this feature will be used, and if most targets come with a pre-existing memory map XML, then this isn't likely to be super useful. That said, if you're expecting that most targets will end up having to generate their own XML on the fly, then it might make sense to lift XML construction into Let me know what you think. This doesn't need to go in as part of this PR. |
|
Generating the XML in Even for targets with a fixed memory map it's probably more convenient if you don't have to study the GDB documentation but can just provide a struct with the necessary information. |
|
Thanks for the quick turnaround! Would you be interested in implementing the XML generation as part of this PR, or should we punt it to a follow up PR instead? Also, for my own peace of mind, could you run the armv4t output |
|
This is from GDB, showing that the memory map was received: armv4t outputI can add the XML generation as well, I will do it in this PR. Might take a day or two however. |
|
Awesome 🎉 If you're willing to implement XML generation as part of this PR, I'd love to see it! It'd be nice to get it done as part of the initial implementation, as to avoid any future breaking API changes. Note that there is a small chance that I'll be cutting the EDIT: while I didn't end up cutting a |
daniel5151
force-pushed
the
dev/0.5
branch
2 times, most recently
from
50454e7 to
6f21623
Compare
|
Just wanted to give you a heads up that I'll be releasing Let me know if you think you'd be able to finish this PR up before late Saturday, and if so, I'll squeeze this feature in as part of |
|
I don't think I will manage to get this done today, unfortunately. Implementing the XML generation in a way that is compatible with |
|
All good 👍 If you need any help at all, or just want to riff on different API designs, feel free to show me what you've got so far! I'm well aware of just how much implementing these sorts of things in That said, I maaaay have nerd sniped myself into thinking about what you could have meant by "trickier than expected", so I ended up writing a whole bunch of my thoughts down... The API I sketched out in #54 (comment) has one pretty annoying limitation: the lifetime of the returned To illustrate this point with code, consider a target implementation that has a dynamically determined memory map: impl MemoryMapXml for MyTarget {
fn memory_map_xml(&self) -> MemoryMap<'_> {
let map = vec![
MemoryMapRegion { start: 0, length: 0x1000, kind: MemoryMapRegionKind::Ram },
MemoryMapRegion { start: 0x1000, length: 0x2000, kind: MemoryMapRegionKind::Rom },
..
];
MemoryMap::Structured(&map) // <-- fails to compile, returning reference to stack-allocated array
}
}
// instead, the data needs to be stashed somewhere within self...
struct MyTarget {
map: Vec<MemoryMapRegion>,
// ...
}
impl MemoryMapXml for MyTarget {
fn memory_map_xml(&self) -> MemoryMap<'_> {
let map = vec![
MemoryMapRegion { start: 0, length: 0x1000, kind: MemoryMapRegionKind::Ram },
MemoryMapRegion { start: 0x1000, length: 0x2000, kind: MemoryMapRegionKind::Rom },
..
];
self.map = map;
MemoryMap::Structured(&self.map) // <-- compiles fine, as data is stored on `self`
}
}An alternative approach that wouldn't require these ownership shenanigans would be to take a page out of functional programming's playbook and use a continuation-passing style API. Disclaimer: I kinda just banged out this code without testing it. It should work, or at the very least, be a reasonable starting point for a working implementation. // ZST used to ensure that the continuation has been called.
// Can only be constructed inside `gdbstub`
#[non_exhaustive]
struct MemoryMapResult {}
impl MemoryMapResult {
pub(crate) fn new() -> MemoryMapResult { MemoryMapResult {} }
}
trait MemoryMapXml: Target {
fn get_memory_map_xml(
&mut self,
cont: &dyn FnOnce(MemoryMap<'_>) -> TargetResult<MemoryMapResult, Self>,
) -> crate::target::TargetResult<MemoryMapResult, Self>;
}
// a rough sketch of how to consume this method within gdbstub
fn handle_memory_map_xml<T: Target, C: Connection>(
res: &mut ResponseWriter<C>,
ops: &mut dyn MemoryMapXml<Arch = T::Arch, Error = T::Error>,
) -> Result<HandlerStatus, Error<T::Error, C::Error>> {
let mut err: Result<_, Error<T::Error, C::Error>> = Ok(());
let res = ops.get_memory_map_xml(&|memory_map: MemoryMap| {
// would be replaced with a `try` block if they ever get stabilized
let e = (|| {
match memory_map {
MemoryMap::Raw(s) => { /* transmit raw string */ }
MemoryMap::Structured(map) => {
// transmit header
res.write_str("header data...")?;
for region in map {
// transmit region
}
// transmit trailing data
res.write_str("trailing data...")?;
Ok(())
}
};
})();
if e.is_err() {
err = e;
}
Ok(MemoryMapResult::new())
});
err?;
Ok(HandlerStatus::Handled)
}
// finally, outside of `gdbstub`, in the target implementation...
impl MemoryMapXml for MyTarget {
fn get_memory_map_xml(
&self,
cont: &dyn FnOnce(MemoryMap<'_>) -> TargetResult<MemoryMapResult, Self>,
) -> TargetResult<MemoryMapResult, Self> {
let map = vec![
MemoryMapRegion {
start: 0,
length: 0x1000,
kind: MemoryMapRegionKind::Ram,
},
MemoryMapRegion {
start: 0x1000,
length: 0x2000,
kind: MemoryMapRegionKind::Rom,
},
..,
];
// compiles fine, since the data is passed "down" the stack via a callback.
// also note that in the monomorphized case, this call will get inlined and/or tail-call-optimized out
cont(MemoryMap::Structured(&self.map))
}
}On a totally orthogonal note, while hacking away at this code, I realize that the other tricky complication is how the return data is transmitted back via the Given the structured and regular nature of the data, it is possible to do a first-pass through the MemoryMapRegion array and calculate the length of the generated strings without transmitting them. i.e: the XML header string has a fixed size, and then each section has a fixed size + a couple variable length number fields, who's hex-encoded-ascii length is easy to calculate depending on their size. If the chunk size is small enough that the client sends multiple Long story short, I now totally understand what you mean when you say that getting it working in As such, here are some ideas of how we can move forwards from here:
Obviously, the first option would be ideal, since from and end-user perspective it would be totally seamless. That said, it is a non-trivial engineering effort, which you may not want to sink more time into (totally understandable). The third option seems like a nice middle-ground between simply exposing the Memory Map XML protocol extension, and actually making it easier + less error-prone to use to end users. Yes, it won't be |
|
I just released gdbstub 0.5, so I took the liberty of losslessly switching the target branch from |
|
@Tiwalun, let me know what you want to do with this PR. Like I mentioned earlier, I'm cool with merging it in as-is. It is provably working, and is a great feature to support. We can punt any follow-up work (such as adding an XML "helper" class and/or an alternative |
|
In that case, I would suggest merging the PR as-is. I don't think I will have time to work on this in the next days, so it's probably better to merge this, and then it can be improved later. |
|
Alright, it's merged 🎉 Thanks again for your contribution! Let me know if you'd like me to cut a I'll also pop open a tracking issue that refers back to #54 (comment), in case someone wants to improve the API in the future. Cheers! |
For embedded targets, we need to be able to configure the memory map, as described in the GDB Docs.
Otherwise, GDB will try to use software breakpoints for code running from flash, which doesn't work.
This PR adds an extension trait based on the
target_description_xml_overrideextension to support memory maps.