[RFC] gdb, rsp: clarify a 0-length memory access

  Currently GDB uses a 0-length write access to probe for the 'X' packet
support.  However, it is not clear from the document what a 0-length
read or write attempt should do.  Clarify the document that it is
an error.  Also update gdbserver's implementation to return an error.

Note that for probing, returning an error is fine.  It successfully
shows that the packet is supported.

Regression-tested on X86-64 Linux using the default (unix) and
native-extended-gdbserver board files.
---
 gdb/doc/gdb.texinfo | 9 +++++++++
 gdbserver/target.cc | 9 ++++++---
 2 files changed, 15 insertions(+), 3 deletions(-)
  

> From: Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>
> Date: Thu, 21 Mar 2024 14:30:18 +0100
> 
> Currently GDB uses a 0-length write access to probe for the 'X' packet
> support.  However, it is not clear from the document what a 0-length
> read or write attempt should do.  Clarify the document that it is
> an error.  Also update gdbserver's implementation to return an error.
> 
> Note that for probing, returning an error is fine.  It successfully
> shows that the packet is supported.
> 
> Regression-tested on X86-64 Linux using the default (unix) and
> native-extended-gdbserver board files.
> ---
>  gdb/doc/gdb.texinfo | 9 +++++++++
>  gdbserver/target.cc | 9 ++++++---
>  2 files changed, 15 insertions(+), 3 deletions(-)

Thanks, the documentation part is okay.

Reviewed-By: Eli Zaretskii <eliz@gnu.org>
  

Hi Tom,

On Thursday, March 21, 2024 5:49 PM, Eli Zaretskii wrote:
> > From: Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>
> > Date: Thu, 21 Mar 2024 14:30:18 +0100
> >
> > Currently GDB uses a 0-length write access to probe for the 'X' packet
> > support.  However, it is not clear from the document what a 0-length
> > read or write attempt should do.  Clarify the document that it is
> > an error.  Also update gdbserver's implementation to return an error.
> >
> > Note that for probing, returning an error is fine.  It successfully
> > shows that the packet is supported.
> >
> > Regression-tested on X86-64 Linux using the default (unix) and
> > native-extended-gdbserver board files.
> > ---
> >  gdb/doc/gdb.texinfo | 9 +++++++++
> >  gdbserver/target.cc | 9 ++++++---
> >  2 files changed, 15 insertions(+), 3 deletions(-)
> 
> Thanks, the documentation part is okay.
> 
> Reviewed-By: Eli Zaretskii <eliz@gnu.org>

Would the code parts be ok?  This is based on the discussion we had stemming
from the 'x' packet.

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
  

Tankut Baris Aktemur <tankut.baris.aktemur@intel.com> writes:

> Currently GDB uses a 0-length write access to probe for the 'X' packet
> support.  However, it is not clear from the document what a 0-length
> read or write attempt should do.  Clarify the document that it is
> an error.  Also update gdbserver's implementation to return an error.

We're usually pretty conservative about changing existing remote
protocol behaviour.

If I understand the current behaviour correctly, we treat the zero
length access as always succeeding, but you propose to change this to
always fail.

What's the motivation for this change?  Does the existing behaviour
cause some problem?

Usually, when the docs are ambiguous we update the docs to reflect GDB's
current behaviour, unless the current behaviour is clearly wrong.

Thanks,
Andrew


>
> Note that for probing, returning an error is fine.  It successfully
> shows that the packet is supported.
>
> Regression-tested on X86-64 Linux using the default (unix) and
> native-extended-gdbserver board files.
> ---
>  gdb/doc/gdb.texinfo | 9 +++++++++
>  gdbserver/target.cc | 9 ++++++---
>  2 files changed, 15 insertions(+), 3 deletions(-)
>
> diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
> index 6099d125a60..0a08c73f8df 100644
> --- a/gdb/doc/gdb.texinfo
> +++ b/gdb/doc/gdb.texinfo
> @@ -42755,6 +42755,9 @@ suitable for accessing memory-mapped I/O devices.
>  @cindex size of remote memory accesses
>  @cindex memory, alignment and size of remote accesses
>  
> +The @var{length} argument has to be a positive value; otherwise, an
> +error is returned.
> +
>  Reply:
>  @table @samp
>  @item @var{XX@dots{}}
> @@ -42771,6 +42774,9 @@ Write @var{length} addressable memory units starting at address @var{addr}
>  (@pxref{addressable memory unit}).  The data is given by @var{XX@dots{}}; each
>  byte is transmitted as a two-digit hexadecimal number.
>  
> +The @var{length} argument has to be a positive value; otherwise, an
> +error is returned.
> +
>  Reply:
>  @table @samp
>  @item OK
> @@ -43127,6 +43133,9 @@ Memory is specified by its address @var{addr} and number of addressable memory
>  units @var{length} (@pxref{addressable memory unit});
>  @samp{@var{XX}@dots{}} is binary data (@pxref{Binary Data}).
>  
> +The @var{length} argument has to be a positive value; otherwise, an
> +error is returned.
> +
>  Reply:
>  @table @samp
>  @item OK
> diff --git a/gdbserver/target.cc b/gdbserver/target.cc
> index 23b699d33bb..6173ebb79f8 100644
> --- a/gdbserver/target.cc
> +++ b/gdbserver/target.cc
> @@ -89,7 +89,7 @@ read_inferior_memory (CORE_ADDR memaddr, unsigned char *myaddr, int len)
>       doesn't hurt to prevent problems if it ever does, or we're
>       connected to some client other than GDB that does.  */
>    if (len == 0)
> -    return 0;
> +    return EINVAL;
>  
>    int res = the_target->read_memory (memaddr, myaddr, len);
>    check_mem_read (memaddr, myaddr, len);
> @@ -121,9 +121,12 @@ target_write_memory (CORE_ADDR memaddr, const unsigned char *myaddr,
>    /* GDB may send X packets with LEN==0, for probing packet support.
>       If we let such a request go through, then buffer.data() below may
>       return NULL, which may confuse target implementations.  Handle it
> -     here to avoid lower levels having to care about this case.  */
> +     here to avoid lower levels having to care about this case.
> +
> +     Sending an error code is sufficient for GDB to conclude that the
> +     server supports the packet.  */
>    if (len == 0)
> -    return 0;
> +    return EINVAL;
>  
>    /* Make a copy of the data because check_mem_write may need to
>       update it.  */
> -- 
> 2.34.1
>
> 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
  

On Thursday, March 28, 2024 3:13 PM, Andrew Burgess wrote:
> Tankut Baris Aktemur <tankut.baris.aktemur@intel.com> writes:
> 
> > Currently GDB uses a 0-length write access to probe for the 'X' packet
> > support.  However, it is not clear from the document what a 0-length
> > read or write attempt should do.  Clarify the document that it is
> > an error.  Also update gdbserver's implementation to return an error.
> 
> We're usually pretty conservative about changing existing remote
> protocol behaviour.
> 
> If I understand the current behaviour correctly, we treat the zero
> length access as always succeeding, but you propose to change this to
> always fail.
> 
> What's the motivation for this change?  Does the existing behaviour
> cause some problem?
> 
> Usually, when the docs are ambiguous we update the docs to reflect GDB's
> current behaviour, unless the current behaviour is clearly wrong.
> 
> Thanks,
> Andrew

Hi Andrew,

The background of the submission is the thread linked below, where Tom expressed
his tendency to think that a 0-length access should be an error:

https://sourceware.org/pipermail/gdb-patches/2024-March/207411.html

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:

> On Thursday, March 28, 2024 3:13 PM, Andrew Burgess wrote:
>> Tankut Baris Aktemur <tankut.baris.aktemur@intel.com> writes:
>> 
>> > Currently GDB uses a 0-length write access to probe for the 'X' packet
>> > support.  However, it is not clear from the document what a 0-length
>> > read or write attempt should do.  Clarify the document that it is
>> > an error.  Also update gdbserver's implementation to return an error.
>> 
>> We're usually pretty conservative about changing existing remote
>> protocol behaviour.
>> 
>> If I understand the current behaviour correctly, we treat the zero
>> length access as always succeeding, but you propose to change this to
>> always fail.
>> 
>> What's the motivation for this change?  Does the existing behaviour
>> cause some problem?
>> 
>> Usually, when the docs are ambiguous we update the docs to reflect GDB's
>> current behaviour, unless the current behaviour is clearly wrong.
>> 
>> Thanks,
>> Andrew
>
> Hi Andrew,
>
> The background of the submission is the thread linked below, where Tom expressed
> his tendency to think that a 0-length access should be an error:
>
> https://sourceware.org/pipermail/gdb-patches/2024-March/207411.html

OK.  But here's my real worry.  Right now gdbserver always succeeds for
a zero length read/write, and it's possible that there exists other
remote targets that have copied this behaviour.

If we change the behaviour for this case, and an updated GDB, that
expects zero length will result in failure, connects to an old gdbserver
(or some other remote target), what happens?

Even if *this* patch doesn't introduce a dependency on the new
behaviour, future patches might, so the question I think is still a
valid one to ask.

Maybe we can show that older GDBs would _never_ send a zero length
request?  In that case maybe this is OK.

The solid solution would be to add a qSupported packet to control the
behaviour of a zero length access.  The default would continue the
current "success" strategy, while if the remote supports the new packet
the behaviour can switch to a "failure" strategy.

Thanks,
Andrew



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

On Friday, April 5, 2024 3:10 PM, Andrew Burgess wrote:
> "Aktemur, Tankut Baris" <tankut.baris.aktemur@intel.com> writes:
> 
> > On Thursday, March 28, 2024 3:13 PM, Andrew Burgess wrote:
> >> Tankut Baris Aktemur <tankut.baris.aktemur@intel.com> writes:
> >>
> >> > Currently GDB uses a 0-length write access to probe for the 'X' packet
> >> > support.  However, it is not clear from the document what a 0-length
> >> > read or write attempt should do.  Clarify the document that it is
> >> > an error.  Also update gdbserver's implementation to return an error.
> >>
> >> We're usually pretty conservative about changing existing remote
> >> protocol behaviour.
> >>
> >> If I understand the current behaviour correctly, we treat the zero
> >> length access as always succeeding, but you propose to change this to
> >> always fail.
> >>
> >> What's the motivation for this change?  Does the existing behaviour
> >> cause some problem?
> >>
> >> Usually, when the docs are ambiguous we update the docs to reflect GDB's
> >> current behaviour, unless the current behaviour is clearly wrong.
> >>
> >> Thanks,
> >> Andrew
> >
> > Hi Andrew,
> >
> > The background of the submission is the thread linked below, where Tom expressed
> > his tendency to think that a 0-length access should be an error:
> >
> > https://sourceware.org/pipermail/gdb-patches/2024-March/207411.html
> 
> OK.  But here's my real worry.  Right now gdbserver always succeeds for
> a zero length read/write, and it's possible that there exists other
> remote targets that have copied this behaviour.
> 
> If we change the behaviour for this case, and an updated GDB, that
> expects zero length will result in failure, connects to an old gdbserver
> (or some other remote target), what happens?
> 
> Even if *this* patch doesn't introduce a dependency on the new
> behaviour, future patches might, so the question I think is still a
> valid one to ask.
> 
> Maybe we can show that older GDBs would _never_ send a zero length
> request?  In that case maybe this is OK.

I'm not sure how we could show that feasibly.  Currently in 
`check_binary_download`, GDB sends a 0-length memory write ('X')
packet to see if the packet is supported.  Receiving a success or a failure
does not matter, they both denote support.  We can check the git history of
the `check_binary_download` function; it was most likely always like that.
But maybe there was a time an older GDB sent a 0-length access packet
somewhere else and explicitly expected success or failure, and that code was
removed later on, I don't know.  It seems very difficult to me to prove that
no such check existed in the past.

I can update the document to match gdbserver's current behavior of sending
success.  One glitch there is the 'm' packet, which replies with an empty
response if the length is 0; so, distinguishing success from unsupported
is not possible.

  (gdb) maintenance packet m01234,0
  sending: m01234,0
  received: ""
  (gdb) maintenance packet foo
  sending: foo
  received: ""
  (gdb)

But maybe 'm' is always supposed to be supported?

 
> The solid solution would be to add a qSupported packet to control the
> behaviour of a zero length access.  The default would continue the
> current "success" strategy, while if the remote supports the new packet
> the behaviour can switch to a "failure" strategy.
> 
> Thanks,
> Andrew


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