[RFC,v4,06/15] gdbserver: Implement p and P packets

GDB will need the p packet to individually request "load early" registers
before using the g packet.

Not sure if P is necessary, but if p is supported, why not implement P?

Alternatively, to be more efficient there could be a packet where GDB can
specify a list of registers it wants do load or set.  Or there could be a
register to request/set the expedited registers.
---
 gdbserver/regcache.cc | 44 +++++++++++++++++++++++++
 gdbserver/regcache.h  |  9 ++++++
 gdbserver/server.cc   | 75 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 128 insertions(+)

On 11/2/24 02:56, Thiago Jung Bauermann wrote:
> GDB will need the p packet to individually request "load early" registers
> before using the g packet.
> 
> Not sure if P is necessary, but if p is supported, why not implement P?
> 
> Alternatively, to be more efficient there could be a packet where GDB can
> specify a list of registers it wants do load or set.  Or there could be a
> register to request/set the expedited registers.
> ---
>  gdbserver/regcache.cc | 44 +++++++++++++++++++++++++
>  gdbserver/regcache.h  |  9 ++++++
>  gdbserver/server.cc   | 75 +++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 128 insertions(+)
> 
> diff --git a/gdbserver/regcache.cc b/gdbserver/regcache.cc
> index 6a1526246867..ec19864bd690 100644
> --- a/gdbserver/regcache.cc
> +++ b/gdbserver/regcache.cc
> @@ -208,6 +208,50 @@ find_register_by_number (const struct target_desc *tdesc, int n)
>  
>  #ifndef IN_PROCESS_AGENT
>  
> +static gdb::array_view<gdb_byte> register_data (const struct regcache *regcache,
> +						int n);
> +
> +/* See regcache.h.  */
> +
> +void
> +register_to_string (struct regcache *regcache, int regnum, char *buf)
> +{
> +  if (regcache->register_status[regnum] == REG_VALID)
> +    {
> +      gdb::array_view<gdb_byte> value = register_data (regcache, regnum);
> +
> +      bin2hex (value.data (), buf, value.size ());
> +      buf[value.size () * 2] = '\0';
> +    }
> +  else
> +    {
> +      int len = regcache->register_size (regnum) * 2;
> +
> +      memset (buf, 'x', len);
> +      buf[len] = '\0';
> +    }
> +}
> +
> +/* See regcache.h.  */
> +
> +void
> +register_from_string (struct regcache *regcache, int regnum, char *buf)
> +{
> +  int len = strlen (buf);
> +  gdb::array_view<gdb_byte> value = register_data (regcache, regnum);
> +  int expected_len = value.size () * 2;
> +
> +  if (len != expected_len)
> +    {
> +      warning ("Wrong sized packet for register %d (expected %d bytes, got %d)",
> +	       regnum, expected_len, len);
> +      if (len > expected_len)
> +	len = expected_len;

If we have more data than expected, is it something we want to allow? Should we
at least warn about it in case something needs to be adjusted?

> +    }
> +
> +  hex2bin (buf, value.data (), len / 2);
> +}
> +
>  void
>  registers_to_string (struct regcache *regcache, char *buf)
>  {
> diff --git a/gdbserver/regcache.h b/gdbserver/regcache.h
> index df0feca102e0..5de8fd3d127e 100644
> --- a/gdbserver/regcache.h
> +++ b/gdbserver/regcache.h
> @@ -96,6 +96,15 @@ void regcache_invalidate (void);
>  
>  void regcache_release (void);
>  
> +/* Save contents of register REGNUM to BUF as an hexadecimal string.  */
> +
> +void register_to_string (struct regcache *regcache, int regnum, char *buf);
> +
> +/* Set contents of register REGNUM from BUF, interpreted as an hexadecimal
> +   string.  */
> +
> +void register_from_string (struct regcache *regcache, int regnum, char *buf);
> +
>  /* Convert all registers to a string in the currently specified remote
>     format.  */
>  
> diff --git a/gdbserver/server.cc b/gdbserver/server.cc
> index 5190df4aed5f..4d075fce2359 100644
> --- a/gdbserver/server.cc
> +++ b/gdbserver/server.cc
> @@ -4712,6 +4712,81 @@ process_serial_event (void)
>  	    }
>  	}
>        break;
> +    case 'p':
> +      {
> +	require_running_or_break (cs.own_buf);
> +	if (cs.current_traceframe >= 0)
> +	  {
> +	    write_enn (cs.own_buf);
> +	    break;
> +	  }
> +	if (!set_desired_thread ())
> +	  {
> +	    write_enn (cs.own_buf);
> +	    break;
> +	  }
> +
> +	int i = 1, regnum = 0;
> +	char c;
> +	while ((c = cs.own_buf[i++]) != '\0')
> +	  {
> +	    regnum = regnum << 4;
> +	    regnum |= fromhex (c) & 0x0f;
> +	  }
> +
> +	struct regcache *regcache = get_thread_regcache (current_thread, true);
> +
> +	if (regnum < 0 || regnum >= regcache->tdesc->reg_defs.size ())
> +	  {
> +	    write_enn (cs.own_buf);
> +	    break;
> +	  }
> +
> +	fetch_inferior_registers (regcache, regnum);
> +	register_to_string (regcache, regnum, cs.own_buf);
> +      }
> +      break;
> +    case 'P':
> +      {
> +	require_running_or_break (cs.own_buf);
> +	if (cs.current_traceframe >= 0)
> +	  {
> +	    write_enn (cs.own_buf);
> +	    break;
> +	  }
> +	if (!set_desired_thread ())
> +	  {
> +	    write_enn (cs.own_buf);
> +	    break;
> +	  }
> +	if (strchr (cs.own_buf, '=') == nullptr)
> +	  {
> +	    write_enn (cs.own_buf);
> +	    break;
> +	  }
> +
> +	int i = 1, regnum = 0;
> +	char c;
> +	while ((c = cs.own_buf[i++]) != '=')
> +	  {
> +	    regnum = regnum << 4;
> +	    regnum |= fromhex (c) & 0x0f;
> +	  }
> +
> +	struct regcache *regcache = get_thread_regcache (current_thread, true);
> +
> +	if (regnum < 0 || regnum >= regcache->tdesc->reg_defs.size ())
> +	  {
> +	    write_enn (cs.own_buf);
> +	    break;
> +	  }
> +
> +	register_from_string (regcache, regnum, &cs.own_buf[i]);
> +	/* FIXME: Why doesn't the G packet need this as well?  */
> +	store_inferior_registers (regcache, regnum);
> +	write_ok (cs.own_buf);
> +      }
> +      break;
>      case 'm':
>        {
>  	require_running_or_break (cs.own_buf);

Otherwise the rest of the patch looks OK.

Reviewed-By: Luis Machado <luis.machado@arm.com>

Luis Machado <luis.machado@arm.com> writes:

> On 11/2/24 02:56, Thiago Jung Bauermann wrote:
>> +/* See regcache.h.  */
>> +
>> +void
>> +register_from_string (struct regcache *regcache, int regnum, char *buf)
>> +{
>> +  int len = strlen (buf);
>> +  gdb::array_view<gdb_byte> value = register_data (regcache, regnum);
>> +  int expected_len = value.size () * 2;
>> +
>> +  if (len != expected_len)
>> +    {
>> +      warning ("Wrong sized packet for register %d (expected %d bytes, got %d)",
>> +	       regnum, expected_len, len);
>> +      if (len > expected_len)
>> +	len = expected_len;
>
> If we have more data than expected, is it something we want to allow?

Thinking about it again, now I think it's better not to allow it. If the
remote stub and GDB have different ideas of the register size, then
something isn't right.

Or perhaps it makes sense to try to be a bit resilient? I'm a bit on the
fence. If we want to try to move forward even if this error occurs, we
could accept the incoming value if it's smaller than the register (and
then pad the most significant bits with 0), also if it's bigger but the
most significant bits that are going to be discarded are 0. And reject
in other cases.

> Should we at least warn about it in case something needs to be
> adjusted?

The patch currently warns about it, but truncates the value. The
truncation could make sense for little endian targets if the MSBs are 0
(though this isn't checked), but would certainly be wrong for big endian
targets.

>> +    }
>> +
>> +  hex2bin (buf, value.data (), len / 2);
>> +}
>> +

> Otherwise the rest of the patch looks OK.
>
> Reviewed-By: Luis Machado <luis.machado@arm.com>

Thanks!

--
Thiago

On Saturday, November 2, 2024 3:56 AM, Thiago Jung Bauermann wrote:
> GDB will need the p packet to individually request "load early" registers
> before using the g packet.
> 
> Not sure if P is necessary, but if p is supported, why not implement P?
> 
> Alternatively, to be more efficient there could be a packet where GDB can
> specify a list of registers it wants do load or set.  Or there could be a
> register to request/set the expedited registers.

Hi Thiago,

In the downstream debugger we have implemented an 'e' packet to fetch
the expedited registers (you mentioned the comment from the Cauldron talk
in the cover letter of this series, too).

In our case, the purpose of the 'e' packet was mostly performance.  Just for
the reference:

  https://github.com/intel/gdb/commit/5f251f4dae3a3fae4ff4ef3ed957fb5b04c10d7d
  https://github.com/intel/gdb/commit/5311b6c43e1e50a1952e4624f7ecf89dc787cdf0

These packets have not been submitted to upstream, yet.

> ---
>  gdbserver/regcache.cc | 44 +++++++++++++++++++++++++
>  gdbserver/regcache.h  |  9 ++++++
>  gdbserver/server.cc   | 75 +++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 128 insertions(+)
> 
> diff --git a/gdbserver/regcache.cc b/gdbserver/regcache.cc
> index 6a1526246867..ec19864bd690 100644
> --- a/gdbserver/regcache.cc
> +++ b/gdbserver/regcache.cc
> @@ -208,6 +208,50 @@ find_register_by_number (const struct target_desc *tdesc, int n)
> 
>  #ifndef IN_PROCESS_AGENT
> 
> +static gdb::array_view<gdb_byte> register_data (const struct regcache *regcache,
> +						int n);
> +
> +/* See regcache.h.  */
> +
> +void
> +register_to_string (struct regcache *regcache, int regnum, char *buf)
> +{
> +  if (regcache->register_status[regnum] == REG_VALID)
> +    {
> +      gdb::array_view<gdb_byte> value = register_data (regcache, regnum);
> +
> +      bin2hex (value.data (), buf, value.size ());
> +      buf[value.size () * 2] = '\0';
> +    }
> +  else
> +    {
> +      int len = regcache->register_size (regnum) * 2;
> +
> +      memset (buf, 'x', len);
> +      buf[len] = '\0';
> +    }
> +}

I had previously submitted a very similar patch:
https://inbox.sourceware.org/gdb-patches/20230620124933.2792496-1-tankut.baris.aktemur@intel.com/

Then Tom Tromey had commented if it would be better to do it in another way:
https://inbox.sourceware.org/gdb-patches/87jzvy2oxa.fsf@tromey.com/

And so I had posted the revision:
https://inbox.sourceware.org/gdb-patches/20230620155457.1023518-1-tankut.baris.aktemur@intel.com/

Maybe something to consider to have more code reuse between 
register_to_string (or collect_register_as_string, depending on which
patch you're looking at) and registers_to_string.

> +
> +/* See regcache.h.  */
> +
> +void
> +register_from_string (struct regcache *regcache, int regnum, char *buf)
> +{
> +  int len = strlen (buf);
> +  gdb::array_view<gdb_byte> value = register_data (regcache, regnum);
> +  int expected_len = value.size () * 2;
> +
> +  if (len != expected_len)
> +    {
> +      warning ("Wrong sized packet for register %d (expected %d bytes, got %d)",
> +	       regnum, expected_len, len);
> +      if (len > expected_len)
> +	len = expected_len;
> +    }
> +
> +  hex2bin (buf, value.data (), len / 2);
> +}
> +
>  void
>  registers_to_string (struct regcache *regcache, char *buf)
>  {
> diff --git a/gdbserver/regcache.h b/gdbserver/regcache.h
> index df0feca102e0..5de8fd3d127e 100644
> --- a/gdbserver/regcache.h
> +++ b/gdbserver/regcache.h
> @@ -96,6 +96,15 @@ void regcache_invalidate (void);
> 
>  void regcache_release (void);
> 
> +/* Save contents of register REGNUM to BUF as an hexadecimal string.  */
> +
> +void register_to_string (struct regcache *regcache, int regnum, char *buf);
> +
> +/* Set contents of register REGNUM from BUF, interpreted as an hexadecimal
> +   string.  */
> +
> +void register_from_string (struct regcache *regcache, int regnum, char *buf);
> +
>  /* Convert all registers to a string in the currently specified remote
>     format.  */
> 
> diff --git a/gdbserver/server.cc b/gdbserver/server.cc
> index 5190df4aed5f..4d075fce2359 100644
> --- a/gdbserver/server.cc
> +++ b/gdbserver/server.cc
> @@ -4712,6 +4712,81 @@ process_serial_event (void)
>  	    }
>  	}
>        break;
> +    case 'p':
> +      {
> +	require_running_or_break (cs.own_buf);
> +	if (cs.current_traceframe >= 0)
> +	  {
> +	    write_enn (cs.own_buf);
> +	    break;
> +	  }
> +	if (!set_desired_thread ())
> +	  {
> +	    write_enn (cs.own_buf);
> +	    break;
> +	  }

I think there is some opportunity now to make these checks common
to 'g', 'G', 'p', and 'P' to reduce repetition?

> +
> +	int i = 1, regnum = 0;
> +	char c;
> +	while ((c = cs.own_buf[i++]) != '\0')
> +	  {
> +	    regnum = regnum << 4;
> +	    regnum |= fromhex (c) & 0x0f;
> +	  }
> +
> +	struct regcache *regcache = get_thread_regcache (current_thread, true);
> +
> +	if (regnum < 0 || regnum >= regcache->tdesc->reg_defs.size ())
> +	  {
> +	    write_enn (cs.own_buf);
> +	    break;
> +	  }
> +
> +	fetch_inferior_registers (regcache, regnum);
> +	register_to_string (regcache, regnum, cs.own_buf);
> +      }
> +      break;
> +    case 'P':
> +      {
> +	require_running_or_break (cs.own_buf);
> +	if (cs.current_traceframe >= 0)
> +	  {
> +	    write_enn (cs.own_buf);
> +	    break;
> +	  }
> +	if (!set_desired_thread ())
> +	  {
> +	    write_enn (cs.own_buf);
> +	    break;
> +	  }
> +	if (strchr (cs.own_buf, '=') == nullptr)
> +	  {
> +	    write_enn (cs.own_buf);
> +	    break;
> +	  }
> +
> +	int i = 1, regnum = 0;
> +	char c;
> +	while ((c = cs.own_buf[i++]) != '=')
> +	  {
> +	    regnum = regnum << 4;
> +	    regnum |= fromhex (c) & 0x0f;
> +	  }
> +
> +	struct regcache *regcache = get_thread_regcache (current_thread, true);
> +
> +	if (regnum < 0 || regnum >= regcache->tdesc->reg_defs.size ())
> +	  {
> +	    write_enn (cs.own_buf);
> +	    break;
> +	  }
> +
> +	register_from_string (regcache, regnum, &cs.own_buf[i]);
> +	/* FIXME: Why doesn't the G packet need this as well?  */

I think it's because with 'G' we are only changing the cache.  The cache is flushed
into the target when it's invalidated (e.g. with regcache_invalidate_thread)
before a thread is resumed.  Did you notice any bugs if you remove the call to store?

> +	store_inferior_registers (regcache, regnum);
> +	write_ok (cs.own_buf);
> +      }
> +      break;
>      case 'm':
>        {
>  	require_running_or_break (cs.own_buf);

Regards
-Baris




Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Sean Fennelly, Jeffrey Schneiderman, Tiffany Doon Silva
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928