[v4,6/8] gdb: Add qIsAddressTagged packet

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

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);
>  }
>
  

Hi Luis,

On 4/16/24 3:04 PM, Luis Machado wrote:
> 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.

Thanks, fixed in v5.


>> +{
>> +  if (packet_check_result (packet).status () != PACKET_OK)
> 
> This function signature is incorrect and leads to a build error. This function has two
> arguments.

argh, not sure how that happened, fixed in v5.


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

Error replies (Exx) and empty replies are treat the same: they fail the
check. As a consequence, the fallback mechanism will be used.


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

hmm, I missed GDB's "auto" mechanism for this. Thanks, fixed in v5.


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

This if/else struct can't be used afaics. It is problematic on the first time
it is executed, because if the packet is not supported by the stub (the check
is performed inside the if statement, not before it) the fallback code must be
called, but since the fallback code is inside the else {} statement, and if {}
is already taken, it won't be executed. So, it must be instead:

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

/* Use the fallback smaps method.  */

Please see v5.


Cheers,
Gustavo