| Message ID | 20191211160514.58373-3-damien.hedde@greensocs.com |
|---|---|
| State | New |
| Headers | show |
| Series | gdbstub: allow sending packet of arbitrary length | expand |
On 12/11/19 5:05 PM, Damien Hedde wrote: > Since we can now send packets of arbitrary length: > simplify gdb_monitor_write() and send the whole payload > in one packet. While we can send arbitrary length on the wire, we advertise PacketSize=MAX_PACKET_LENGTH in handle_query_supported(). We can raise this limit however. > Suggested-by: Luc Michel <luc.michel@greensocs.com> > Signed-off-by: Damien Hedde <damien.hedde@greensocs.com> > --- > gdbstub.c | 23 +++-------------------- > 1 file changed, 3 insertions(+), 20 deletions(-) > > diff --git a/gdbstub.c b/gdbstub.c > index 93b26f1b86..ef999abee2 100644 > --- a/gdbstub.c > +++ b/gdbstub.c > @@ -3200,28 +3200,11 @@ static void gdb_chr_event(void *opaque, int event) > } > } > > -static void gdb_monitor_output(GDBState *s, const char *msg, int len) > -{ > - g_autoptr(GString) buf = g_string_new("O"); > - memtohex(buf, (uint8_t *)msg, len); > - put_packet(buf->str); > -} > - > static int gdb_monitor_write(Chardev *chr, const uint8_t *buf, int len) > { > - const char *p = (const char *)buf; > - int max_sz; > - > - max_sz = (MAX_PACKET_LENGTH / 2) + 1; > - for (;;) { > - if (len <= max_sz) { > - gdb_monitor_output(&gdbserver_state, p, len); > - break; > - } > - gdb_monitor_output(&gdbserver_state, p, max_sz); > - p += max_sz; > - len -= max_sz; > - } > + g_autoptr(GString) hex_buf = g_string_new("O"); > + memtohex(hex_buf, buf, len); > + put_packet(hex_buf->str); > return len; > } > >
Damien Hedde <damien.hedde@greensocs.com> writes: > Since we can now send packets of arbitrary length: > simplify gdb_monitor_write() and send the whole payload > in one packet. Do we know gdb won't barf on us. Does the negotiated max packet size only apply to data sent to the gdbserver? > > Suggested-by: Luc Michel <luc.michel@greensocs.com> > Signed-off-by: Damien Hedde <damien.hedde@greensocs.com> > --- > gdbstub.c | 23 +++-------------------- > 1 file changed, 3 insertions(+), 20 deletions(-) > > diff --git a/gdbstub.c b/gdbstub.c > index 93b26f1b86..ef999abee2 100644 > --- a/gdbstub.c > +++ b/gdbstub.c > @@ -3200,28 +3200,11 @@ static void gdb_chr_event(void *opaque, int event) > } > } > > -static void gdb_monitor_output(GDBState *s, const char *msg, int len) > -{ > - g_autoptr(GString) buf = g_string_new("O"); > - memtohex(buf, (uint8_t *)msg, len); > - put_packet(buf->str); > -} > - > static int gdb_monitor_write(Chardev *chr, const uint8_t *buf, int len) > { > - const char *p = (const char *)buf; > - int max_sz; > - > - max_sz = (MAX_PACKET_LENGTH / 2) + 1; > - for (;;) { > - if (len <= max_sz) { > - gdb_monitor_output(&gdbserver_state, p, len); > - break; > - } > - gdb_monitor_output(&gdbserver_state, p, max_sz); > - p += max_sz; > - len -= max_sz; > - } > + g_autoptr(GString) hex_buf = g_string_new("O"); > + memtohex(hex_buf, buf, len); > + put_packet(hex_buf->str); > return len; > }
On 12/11/19 7:58 PM, Philippe Mathieu-Daudé wrote: > On 12/11/19 5:05 PM, Damien Hedde wrote: >> Since we can now send packets of arbitrary length: >> simplify gdb_monitor_write() and send the whole payload >> in one packet. > > While we can send arbitrary length on the wire, we advertise > PacketSize=MAX_PACKET_LENGTH in handle_query_supported(). > > We can raise this limit however. Hi Philippe, This parameter is only about the packet size we can handle (packets we can receive). -- Damien
On 12/11/19 7:59 PM, Alex Bennée wrote: > > Damien Hedde <damien.hedde@greensocs.com> writes: > >> Since we can now send packets of arbitrary length: >> simplify gdb_monitor_write() and send the whole payload >> in one packet. > > Do we know gdb won't barf on us. Does the negotiated max packet size > only apply to data sent to the gdbserver? Yes the negociated packet size is only about packet we can receive. Qutoting the gdb doc: | ‘PacketSize=bytes’ | | The remote stub can accept packets up to at least bytes in length. | GDB will send packets up to this size for bulk transfers, and will | never send larger packets. The qSupported doc also says that "Any GDB which sends a ‘qSupported’ packet supports receiving packets of unlimited length". I did some digging and qSupported appeared in gdb 6.6 (december 2006). And gdb supported arbitrary sized packet even before that (6.4.9 2006 too). > >> >> Suggested-by: Luc Michel <luc.michel@greensocs.com> >> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com> >> --- >> gdbstub.c | 23 +++-------------------- >> 1 file changed, 3 insertions(+), 20 deletions(-) >> >> diff --git a/gdbstub.c b/gdbstub.c >> index 93b26f1b86..ef999abee2 100644 >> --- a/gdbstub.c >> +++ b/gdbstub.c >> @@ -3200,28 +3200,11 @@ static void gdb_chr_event(void *opaque, int event) >> } >> } >> >> -static void gdb_monitor_output(GDBState *s, const char *msg, int len) >> -{ >> - g_autoptr(GString) buf = g_string_new("O"); >> - memtohex(buf, (uint8_t *)msg, len); >> - put_packet(buf->str); >> -} >> - >> static int gdb_monitor_write(Chardev *chr, const uint8_t *buf, int len) >> { >> - const char *p = (const char *)buf; >> - int max_sz; >> - >> - max_sz = (MAX_PACKET_LENGTH / 2) + 1; >> - for (;;) { >> - if (len <= max_sz) { >> - gdb_monitor_output(&gdbserver_state, p, len); >> - break; >> - } >> - gdb_monitor_output(&gdbserver_state, p, max_sz); >> - p += max_sz; >> - len -= max_sz; >> - } >> + g_autoptr(GString) hex_buf = g_string_new("O"); >> + memtohex(hex_buf, buf, len); >> + put_packet(hex_buf->str); >> return len; >> } > >
Damien Hedde <damien.hedde@greensocs.com> writes: > On 12/11/19 7:59 PM, Alex Bennée wrote: >> >> Damien Hedde <damien.hedde@greensocs.com> writes: >> >>> Since we can now send packets of arbitrary length: >>> simplify gdb_monitor_write() and send the whole payload >>> in one packet. >> >> Do we know gdb won't barf on us. Does the negotiated max packet size >> only apply to data sent to the gdbserver? > > Yes the negociated packet size is only about packet we can receive. > Qutoting the gdb doc: > | ‘PacketSize=bytes’ > | > | The remote stub can accept packets up to at least bytes in length. > | GDB will send packets up to this size for bulk transfers, and will > | never send larger packets. > > The qSupported doc also says that "Any GDB which sends a ‘qSupported’ > packet supports receiving packets of unlimited length". > I did some digging and qSupported appeared in gdb 6.6 (december 2006). > And gdb supported arbitrary sized packet even before that (6.4.9 2006 > too). I think that is worth a comment for the function gdb_monitor_write quoting the spec and the versions. With that comment: Reviewed-by: Alex Bennée <alex.bennee@linaro.org> > >> >>> >>> Suggested-by: Luc Michel <luc.michel@greensocs.com> >>> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com> >>> --- >>> gdbstub.c | 23 +++-------------------- >>> 1 file changed, 3 insertions(+), 20 deletions(-) >>> >>> diff --git a/gdbstub.c b/gdbstub.c >>> index 93b26f1b86..ef999abee2 100644 >>> --- a/gdbstub.c >>> +++ b/gdbstub.c >>> @@ -3200,28 +3200,11 @@ static void gdb_chr_event(void *opaque, int event) >>> } >>> } >>> >>> -static void gdb_monitor_output(GDBState *s, const char *msg, int len) >>> -{ >>> - g_autoptr(GString) buf = g_string_new("O"); >>> - memtohex(buf, (uint8_t *)msg, len); >>> - put_packet(buf->str); >>> -} >>> - >>> static int gdb_monitor_write(Chardev *chr, const uint8_t *buf, int len) >>> { >>> - const char *p = (const char *)buf; >>> - int max_sz; >>> - >>> - max_sz = (MAX_PACKET_LENGTH / 2) + 1; >>> - for (;;) { >>> - if (len <= max_sz) { >>> - gdb_monitor_output(&gdbserver_state, p, len); >>> - break; >>> - } >>> - gdb_monitor_output(&gdbserver_state, p, max_sz); >>> - p += max_sz; >>> - len -= max_sz; >>> - } >>> + g_autoptr(GString) hex_buf = g_string_new("O"); >>> + memtohex(hex_buf, buf, len); >>> + put_packet(hex_buf->str); >>> return len; >>> } >> >>
On 12/12/19 11:52 AM, Alex Bennée wrote: > > Damien Hedde <damien.hedde@greensocs.com> writes: > >> On 12/11/19 7:59 PM, Alex Bennée wrote: >>> >>> Damien Hedde <damien.hedde@greensocs.com> writes: >>> >>>> Since we can now send packets of arbitrary length: >>>> simplify gdb_monitor_write() and send the whole payload >>>> in one packet. >>> >>> Do we know gdb won't barf on us. Does the negotiated max packet size >>> only apply to data sent to the gdbserver? >> >> Yes the negociated packet size is only about packet we can receive. >> Qutoting the gdb doc: >> | ‘PacketSize=bytes’ >> | >> | The remote stub can accept packets up to at least bytes in length. >> | GDB will send packets up to this size for bulk transfers, and will >> | never send larger packets. >> >> The qSupported doc also says that "Any GDB which sends a ‘qSupported’ >> packet supports receiving packets of unlimited length". >> I did some digging and qSupported appeared in gdb 6.6 (december 2006). >> And gdb supported arbitrary sized packet even before that (6.4.9 2006 >> too). > > I think that is worth a comment for the function gdb_monitor_write > quoting the spec and the versions. With that comment: > > Reviewed-by: Alex Bennée <alex.bennee@linaro.org> > Good idea ! Is that ok if I add these comments in the 1st patch along with the gdbstate.last_packet field ? it seems a more central place. I can still add a short note for gdb_monitor_write(). Damien
diff --git a/gdbstub.c b/gdbstub.c index 93b26f1b86..ef999abee2 100644 --- a/gdbstub.c +++ b/gdbstub.c @@ -3200,28 +3200,11 @@ static void gdb_chr_event(void *opaque, int event) } } -static void gdb_monitor_output(GDBState *s, const char *msg, int len) -{ - g_autoptr(GString) buf = g_string_new("O"); - memtohex(buf, (uint8_t *)msg, len); - put_packet(buf->str); -} - static int gdb_monitor_write(Chardev *chr, const uint8_t *buf, int len) { - const char *p = (const char *)buf; - int max_sz; - - max_sz = (MAX_PACKET_LENGTH / 2) + 1; - for (;;) { - if (len <= max_sz) { - gdb_monitor_output(&gdbserver_state, p, len); - break; - } - gdb_monitor_output(&gdbserver_state, p, max_sz); - p += max_sz; - len -= max_sz; - } + g_autoptr(GString) hex_buf = g_string_new("O"); + memtohex(hex_buf, buf, len); + put_packet(hex_buf->str); return len; }
Since we can now send packets of arbitrary length: simplify gdb_monitor_write() and send the whole payload in one packet. Suggested-by: Luc Michel <luc.michel@greensocs.com> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com> --- gdbstub.c | 23 +++-------------------- 1 file changed, 3 insertions(+), 20 deletions(-)