[v2,3/4] rsp: add 'E' to escaped characters
Checks
Context |
Check |
Description |
linaro-tcwg-bot/tcwg_gdb_build--master-arm |
success
|
Testing passed
|
linaro-tcwg-bot/tcwg_gdb_check--master-arm |
success
|
Testing passed
|
linaro-tcwg-bot/tcwg_gdb_build--master-aarch64 |
success
|
Testing passed
|
linaro-tcwg-bot/tcwg_gdb_check--master-aarch64 |
success
|
Testing passed
|
Commit Message
Add 'E' to the list of escaped characters when sending/receiving
binary data. This is a preparation for the next patch, to be able to
distinguish an error response from binary data that starts with 'E'.
Approved-By: Tom Tromey <tom@tromey.com>
---
gdb/doc/gdb.texinfo | 4 +++-
gdbsupport/rsp-low.cc | 2 +-
2 files changed, 4 insertions(+), 2 deletions(-)
Comments
Hi Tankut,
> Add 'E' to the list of escaped characters when sending/receiving
> binary data. This is a preparation for the next patch, to be able to
> distinguish an error response from binary data that starts with 'E'.
I wonder if there is a better way to do this, given that this is for a totally
new packet. My concerns are twofold:
1. The other 'escaped' characters are part of the RSP packetization layer,
which is conceptually below the message processing. This new packet is
looking at both escaped and non-escaped message in order to figure out
which type of response it is looking at, which is (in my opinion)
unnecessary layer crossing.
2. The idea of allowing this list to grow when there are other possible
solutions is just less scalable if other new binary packets are added
in the future.
Instead, I would propose a similar approach to other packets which would
have the potential for 'ambiguous' replies, such as 'qCRC', 'qMemTags' etc.
That is: Always have a leading byte (not just in the error case) in order
to disambiguate what type of reply the message contains.
So (for example) the message is something more like:
'E NN' for an error
'd XX...' for binary data
Does that sound reasonable?
(Note that I am not a maintainer, so would appreciate other opinions!)
Thanks,
Ciaran
> From: Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>
> Cc: tom@tromey.com
> Date: Thu, 14 Mar 2024 14:07:30 +0100
>
> Add 'E' to the list of escaped characters when sending/receiving
> binary data. This is a preparation for the next patch, to be able to
> distinguish an error response from binary data that starts with 'E'.
>
> Approved-By: Tom Tromey <tom@tromey.com>
> ---
> gdb/doc/gdb.texinfo | 4 +++-
> gdbsupport/rsp-low.cc | 2 +-
> 2 files changed, 4 insertions(+), 2 deletions(-)
Thanks.
> diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
> index f2b80db94ef..9a7d61be65e 100644
> --- a/gdb/doc/gdb.texinfo
> +++ b/gdb/doc/gdb.texinfo
> @@ -42389,7 +42389,9 @@ bytes @code{0x7d 0x5d}. The bytes @code{0x23} (@sc{ascii} @samp{#}),
> @samp{@}}) must always be escaped. Responses sent by the stub
> must also escape @code{0x2a} (@sc{ascii} @samp{*}), so that it
> is not interpreted as the start of a run-length encoded sequence
> -(described next).
> +(described next). The @code{0x45} (@sc{ascii} @samp{E}) byte should
> +also be escaped to distinguish it from an error reply that starts with
> +the @samp{E} character.
>
> Response @var{data} can be run-length encoded to save space.
> Run-length encoding replaces runs of identical characters with one
This part is okay.
Reviewed-By: Eli Zaretskii <eliz@gnu.org>
Hi Ciaran,
On Thursday, March 14, 2024 2:47 PM, Ciaran Woodward wrote:
> Hi Tankut,
>
> > Add 'E' to the list of escaped characters when sending/receiving
> > binary data. This is a preparation for the next patch, to be able to
> > distinguish an error response from binary data that starts with 'E'.
>
> I wonder if there is a better way to do this, given that this is for a totally
> new packet. My concerns are twofold:
>
> 1. The other 'escaped' characters are part of the RSP packetization layer,
> which is conceptually below the message processing. This new packet is
> looking at both escaped and non-escaped message in order to figure out
> which type of response it is looking at, which is (in my opinion)
> unnecessary layer crossing.
>
> 2. The idea of allowing this list to grow when there are other possible
> solutions is just less scalable if other new binary packets are added
> in the future.
>
> Instead, I would propose a similar approach to other packets which would
> have the potential for 'ambiguous' replies, such as 'qCRC', 'qMemTags' etc.
>
> That is: Always have a leading byte (not just in the error case) in order
> to disambiguate what type of reply the message contains.
>
> So (for example) the message is something more like:
>
> 'E NN' for an error
>
> 'd XX...' for binary data
>
> Does that sound reasonable?
> (Note that I am not a maintainer, so would appreciate other opinions!)
>
> Thanks,
> Ciaran
My goal was to keep similarity to packets like 'm', 'g', 'p'. Starting the reply with
a marker is certainly doable. Let's see what the maintainers will say. If that's the
direction, I can work on the change.
Thanks
-Baris
Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928
"Aktemur, Tankut Baris" <tankut.baris.aktemur@intel.com> writes:
> Hi Ciaran,
>
> On Thursday, March 14, 2024 2:47 PM, Ciaran Woodward wrote:
>> Hi Tankut,
>>
>> > Add 'E' to the list of escaped characters when sending/receiving
>> > binary data. This is a preparation for the next patch, to be able to
>> > distinguish an error response from binary data that starts with 'E'.
>>
>> I wonder if there is a better way to do this, given that this is for a totally
>> new packet. My concerns are twofold:
>>
>> 1. The other 'escaped' characters are part of the RSP packetization layer,
>> which is conceptually below the message processing. This new packet is
>> looking at both escaped and non-escaped message in order to figure out
>> which type of response it is looking at, which is (in my opinion)
>> unnecessary layer crossing.
>>
>> 2. The idea of allowing this list to grow when there are other possible
>> solutions is just less scalable if other new binary packets are added
>> in the future.
>>
>> Instead, I would propose a similar approach to other packets which would
>> have the potential for 'ambiguous' replies, such as 'qCRC', 'qMemTags' etc.
>>
>> That is: Always have a leading byte (not just in the error case) in order
>> to disambiguate what type of reply the message contains.
>>
>> So (for example) the message is something more like:
>>
>> 'E NN' for an error
>>
>> 'd XX...' for binary data
>>
>> Does that sound reasonable?
>> (Note that I am not a maintainer, so would appreciate other opinions!)
>>
>> Thanks,
>> Ciaran
>
> My goal was to keep similarity to packets like 'm', 'g', 'p'. Starting the reply with
> a marker is certainly doable. Let's see what the maintainers will say. If that's the
> direction, I can work on the change.
I'd vote for using a prefix character. Yes it means we diverge from
m/g/p, but those are some of our older packets, as you've pointed out in
other messages, there are problems with those packets (w.r.t. the empty
case). I think using a better solution is more important than
consistency.
Thanks,
Andrew
@@ -42389,7 +42389,9 @@ bytes @code{0x7d 0x5d}. The bytes @code{0x23} (@sc{ascii} @samp{#}),
@samp{@}}) must always be escaped. Responses sent by the stub
must also escape @code{0x2a} (@sc{ascii} @samp{*}), so that it
is not interpreted as the start of a run-length encoded sequence
-(described next).
+(described next). The @code{0x45} (@sc{ascii} @samp{E}) byte should
+also be escaped to distinguish it from an error reply that starts with
+the @samp{E} character.
Response @var{data} can be run-length encoded to save space.
Run-length encoding replaces runs of identical characters with one
@@ -171,7 +171,7 @@ bin2hex (const gdb_byte *bin, int count)
static int
needs_escaping (gdb_byte b)
{
- return b == '$' || b == '#' || b == '}' || b == '*';
+ return b == '$' || b == '#' || b == '}' || b == '*' || b == 'E';
}
/* See rsp-low.h. */