Re: [PATCH v4 6/8] gdb: Add qIsAddressTagged packet

[Luis Machado <luis.machado@arm.com>]

Close, by I still have a few comments below.

On 4/16/24 15:07, Gustavo Romero wrote:
> This commit adds a new packet, qIsAddressTagged, allowing GDB remote
> targets to use it to query the stub if a given address is tagged.
> 
> Currently, the memory tagging address check is done via a read query,
> where the contents of /proc/<PID>/smaps is read and the flags are
> inspected for memory tagging-related flags that indicate the address is
> in a memory tagged region.
> 
> This is not ideal, for example, for QEMU stub and other cases, such as
> on bare-metal, where there is no notion of an OS file like 'smaps.'
> Hence, the introduction of qIsAddressTagged packet allows checking
> if an address is tagged in an agnostic way.
> 
> The is_address_tagged target hook in remote.c attempts to use the
> qIsAddressTagged packet first for checking if an address is tagged and
> if the stub does not support such a packet (reply is empty) it falls
> back to using the current mechanism that reads the contents of
> /proc/<PID>/smaps via vFile requests.
> 
> Signed-off-by: Gustavo Romero <gustavo.romero@linaro.org>
> ---
>  gdb/remote.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 49 insertions(+)
> 
> diff --git a/gdb/remote.c b/gdb/remote.c
> index 9717db55e27..63799ac5e3f 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -15534,6 +15534,40 @@ create_store_memtags_request (gdb::char_vector &packet, CORE_ADDR address,
>    strcpy (packet.data (), request.c_str ());
>  }
>  
> +static void
> +create_is_address_tagged_request (gdbarch *gdbarch, gdb::char_vector &packet,
> +				  CORE_ADDR address)
> +{
> +  int addr_size;
> +  std::string request;
> +
> +  addr_size = gdbarch_addr_bit (gdbarch) / 8;
> +  request = string_printf ("qIsAddressTagged:%s", phex_nz (address, addr_size));
> +
> +  if (packet.size () < request.length () + 1)
> +    error (_("Contents too big for packet qIsAddressTagged."));
> +
> +  strcpy (packet.data (), request.c_str ());
> +}
> +
> +static bool
> +check_is_address_tagged_reply (gdb::char_vector &packet, bool *tagged)

Instead of passing TAGGED as pointer, make it a reference. It is safer.

> +{
> +  if (packet_check_result (packet).status () != PACKET_OK)

This function signature is incorrect and leads to a build error. This function has two
arguments.

Also, this check will return false if the packet yields an error and if the packet is not
supported. We need to be able to distinguish between unsupported and error here, right?

> +    return false;
> +
> +  gdb_byte reply;
> +  /* Convert only 2 hex digits, i.e. 1 byte in hex format.  */
> +  hex2bin (packet.data (), &reply, 1);
> +
> +  if (reply == 0x00 || reply == 0x01) {
> +    *tagged = !!reply;

Passing tagged as reference just use tagged instead of *tagged here.

> +    return true;
> +  }
> +
> +  return false;
> +}
> +
>  /* Implement the "fetch_memtags" target_ops method.  */
>  
>  bool
> @@ -15580,6 +15614,21 @@ remote_target::store_memtags (CORE_ADDR address, size_t len,
>  bool
>  remote_target::is_address_tagged (gdbarch *gdbarch, CORE_ADDR address)
>  {
> +  struct remote_state *rs = get_remote_state ();
> +  bool is_addr_tagged;
> +

Before sending the packet, we need to check if the packet is supported. Generally gdb
will send it the first time around, but if the packet isn't supported gdb shouldn't
keep sending these packets if the stub is gonna reply empty again.

See remote_target::remote_query_attached for an example of how we deal with this.

First you need to have a new enum PACKET_qIsAddressTagged, so we can register if
the packet is supported or not at runtime.

Then at the start of the function:

if (m_features.packet_support (PACKET_qIsAddressTagged) != PACKET_DISABLE)
  {
    /* Use the qIsTaggedAddress packet.  */
  }
else
  {
    /* Use the fallback smaps method.  */
  }

That way gdb only sends the qIsTaggedAddress packet once. If it works, then gdb
keeps using it. Otherwise it always uses the fallback.

> +  /* Firstly, attempt to check the address using the qIsAddressTagged
> +     packet.  */
> +  create_is_address_tagged_request (gdbarch, rs->buf, address);
> +
> +  putpkt (rs->buf);
> +  getpkt (&rs->buf);
> +
> +  if (check_is_address_tagged_reply (rs->buf, &is_addr_tagged))

We should pass is_addr_tagged by reference instead.

> +    return is_addr_tagged;> +
> +  /* Fallback to arch-specific method of checking whether an address is tagged
> +     if qIsAddressTagged fails.  */
>    return gdbarch_tagged_address_p (gdbarch, address);
>  }
>