[v2,3/4] rsp: add 'E' to escaped characters

  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(-)
  

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