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

[Andrew Burgess <aburgess@redhat.com>]

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