Refactoring gdb_serialize function

[bet4it]

Currently the gdb_serialize function takes a write_byte callback to send packet out. As discussion in #70 , it's better to replace the callback with PacketBuf, collect all the registers in this buffer and send them out together.

[bet4it]

To implement this, we need to replace all the write_byte in gdb_serialize with copying the data to different part of a buffer. I want to use byteorder to do this, but the following code seems not work correctly:

fn gdb_serialize(&self, mut buf: &mut [u8]) {
    for reg in self.r.iter() {
        buf.write_u32::<LittleEndian>(*reg).unwrap();
    }
    ...
}

[daniel5151]

Ahh shoot, you know what, I totally remembered why I used the callback approach vs. writing data into the buffer.

The GDB spec allows returning XX for any register data that "wasn't available", or "could not be read". See https://sourceware.org/gdb/onlinedocs/gdb/Packets.html#read-registers-packet

The current callback API supports this use-case by allowing the implementation to call write_byte with None, which will emit a XX instead of an ASCII byte.

See https://github.com/daniel5151/gdbstub/blob/0db1406/gdbstub_arch/src/arm/reg/arm_core.rs#L44-L46 for an example of this being used.

This is an important bit of functionality, since returning a dummy value for those registers (e.g: 0) would be a correctness issue - GDB would think those registers have a value of zero, when in reality, those registers simply don't have a value.

---

I don't have a great solution for how your proposed API would handle this. Maybe you could have some kind of extra return value from gdb_serialize that is a &'static [(usize, usize)] list of "invalid ranges" into the byte buffer that should be replaced with XX?

---

But also, taking a step back for a second: is there any pressing need to make this API change? The current write_register API has some nice benefits that this new one wouldn't have - namely: it doesn't require any bounds checks when serializing data, since it serializes data directly "to the wire", without going through an intermediate buffer.

So, aside from the argument of "code elegance" of composing data in a output buffer vs. using a callback (which might not even hold true, given the aforementioned XX consideration that'd need to be taken into account), what other benefits would this API change have?

[daniel5151]

And with respect to write_all... I don't really see a pressing need to implement it.

The GDB RSP is fundamentally a byte-oriented protocol, whereby implementations only need to provide the equivalent of getchar and putchar to get things up and running.

Realistically, when gdbstub is running over anything but the simplest protocol (i.e: serial), you're already getting "free" write_all functionality via the underlying Connection - i.e: the fact that data is written using write(u8), and then once the packet is ready, flush() gets called. Under the hood, a connection is free to implement this however it likes - typically using some kind of internal buffering.

The only real benefit I can see of getting write_all working within gdbstub itself is that we could add "transparent" buffering to the aforementioned serial transport use-case, whereby we could potentially leverage the PacketBuf to compose a response prior to bit-banging it over the wire.

That said, I don't even think that would have any kind of performance benefit, since at the end of the day, you'd still have to emit those bytes sequentially...

[bet4it]

After I remove set_nodelay, things get better, there may be more bytes in a packet, but most packet only contain bytes less than 20.

The situation is quite reasonable. We send data byte by byte, so it's impossible for TCP to know where is the end of data, the only thing it can do is waiting for a timeout, which couldn't be too long. As I said before, if we want to solve this problem, we must collect all the data to be sent into a buffer and use something like write_all.

Just notice you mentioned it before:

gdbstub/src/protocol/response_writer.rs

Lines 16 to 17 in 9227dfd

	// TODO: add `write_all` method to Connection, and allow user to optionally pass outgoing
	// packet buffer? This could improve performance (instead of writing a single byte at a time)

If there are no write_all on UART, we could fallback to iteration of write:

pub fn write_all(&mut self, data: &[u8]) -> Result<(), Error<C::Error>> {
    for b in data.iter() {
        self.write(*b)?;
    }
    Ok(())
}

Consider that most users will use TCP, we should use write_all by default.

Originally posted by @bet4it in #69 (comment)

Do you think one TCP packet only sends one byte is reasonable?
And implement write_all doesn't conflict with simple protocol that only send one byte per time.

[daniel5151]

Ahh, yes, that comment...

If you look at the git blame history of that comment, I added this TODO a very long time ago (in 27a950f), back when gdbstub looked very different from how it looked today, and I had far less hands-on validation of gdbstub from myself + folks in the community.

I'll be honest: I don't think the "pass an outgoing packet buffer" aspect of that TODO is terribly important, for the reason I outline below...

We send data byte by byte, so it's impossible for TCP to know where is the end of data, the only thing it can do is waiting for a timeout, which couldn't be too long.

So, FYI, the std::io::Write::write_all API (that typically backs Connection::write/write_all - such as in the TCP case) doesn't mandate that the underlying stream immediately sends that data out. The concrete implementation can perform whatever internal buffering it likes. That's why there's also a std::io::Write::flush() method (and a corresponding Connection::flush() method), which can be called to force the data to be flushed over the underlying transport. ResponseWriter calls flush once the packet has been entirely composed, and is ready to be sent out.

To reiterate: just because ResponseWriter is calling a single-byte write on the underlying Connection vs. write_all, doesn't mean that the underlying packet is immediately being sent out. Instead, it's likely that the data is being implicitly buffered, either by the Connection implementation itself, or by the implementation's underlying TCP stack.

If you don't like the default TCP behavior, you can tweak it however you like by implementing a custom Connection implementation.

Basically, the only downside of having this buffering happen at the Connection level is that it makes it impossible to reuse PacketBuf to perform this kind of buffering, but I feel like the complexity required to support that would be exceptionally tricky, given that we'd be overloading the PacketBuf for yet another use-case (i.e: we are already using it as a staging-ground for unescaped data by passing it to various Target handlers, and now we'd want to simultaneously use it as a staging ground for escaped data as well? sounds hard).

Sure, we could go through the extra effort of writing some kind of "optimized" ResponseWriter::write_all_inner method with the goal of slightly improve performance if you're sending out a lot of [already unescaped] data, honestly, that just seems like it would make the code larger and more complicated to maintain for no great gain...

[bet4it]

doesn't mandate that the underlying stream immediately sends that data out

That's why we need set_nodelay. After we set it, the data to be sent won't be buffered.

---

If you don't like the default TCP behavior, you can tweak it however you like by implementing a custom Connection implementation.

As what I said before:

it's impossible for TCP to know where is the end of data, the only thing it can do is waiting for a timeout, which couldn't be too long.

Even if I implement a custom Connection implementation, it's still nearly impossible for me to know when the data is well prepared and I can send it.
Certainly, I can check if the data in buffer end with #xx, but why I need to do this? Why not just use write_all in the implementation?

[daniel5151]

No, I don't think you understand: Connection has a flush method that is explicitly called when the data in the buffer is complete:

gdbstub/src/protocol/response_writer.rs

Line 70 in 4e46b72

self.inner.flush().map_err(Error)?;

If you want to buffer the data, you can implement something like:

struct BufferedTcp {
    buf: Vec<u8>,
    stream: TcpStream
}

// pseudocode, won't compile, but you get the gist 

impl Connection for BufferedTcp {
    fn write(&mut self, b: u8) {
        self.buf.push(b)
    }

    fn flush(&mut self) {
        self.stream.write_all(&self.buf)?;
        self.buf.clear();
    }
}

This is dead simple, and requires no changes to gdbstub, which continues to be entirely byte-oriented.

[daniel5151]

Note that this refactor might not even be worth doing, depending on the outcome of #90. i.e: there's a chance that the entire notion of having a separate Register struct goes away, and gdb_serialize/gdb_deserialize would have to be entirely rewritten/removed in favor of using RegId + direct calls to read/write_register