Re: [PATCH v3 6/7] gdb: Add qMemTagAddrCheck packet

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

On 4/4/24 07:48, Gustavo Romero wrote:
> This commit adds a new packet, qMemTagAddrCheck, allowing GDB remote
> targets to use it to query the stub if a given address is tagged.

I'm not a big fan of the packet name, but naming those things is sometimes hard.

So here goes my attempt. Based on the target hook, how about qAddressIsTagged?

> 
> It also adds a new GDB remote feature, 'memory-tagging-check-addr',

And the above could be adjusted to "memory-tagging-address-check+"?

Just trying to make things a bit more clear and less abbreviated (guilty of
doing that myself sometimes!).

> which must be advertised by the GDB stub to inform it can reply to
> memory tagging address checks via the new qMemTagCheckAddr packet.
> 
> 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 qMemTagCheckAddr packet allows checking addresses in an
> OS-agnostic way.
> 
> The is_address_tagged target hook in remote.c uses the qMemTagCheckAddr
> packet to check an address if the stub advertises the
> 'memory-tagging-check-add+' feature, otherwise it falls back to using
> the current mechanism, which reads the contents of /proc/<PID>/smaps via
> vFile requests.
> 
> Signed-off-by: Gustavo Romero <gustavo.romero@linaro.org>
> ---
>  gdb/remote.c | 54 +++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 53 insertions(+), 1 deletion(-)
> 
> diff --git a/gdb/remote.c b/gdb/remote.c
> index 9717db55e27..94ac8520740 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -337,6 +337,9 @@ enum {
>       packets and the tag violation stop replies.  */
>    PACKET_memory_tagging_feature,
>  
> +  /* Support checking if an address is tagged via qMemTagCheckAddr packet.  */
> +  PACKET_memory_tagging_check_addr_feature,
> +
>    PACKET_MAX
>  };
>  
> @@ -758,6 +761,10 @@ struct remote_features
>    bool remote_memory_tagging_p () const
>    { return packet_support (PACKET_memory_tagging_feature) == PACKET_ENABLE; }
>  
> +  bool remote_memory_tagging_check_addr_p () const
> +  { return packet_support (PACKET_memory_tagging_check_addr_feature) ==
> +			   PACKET_ENABLE; }
> +
>    /* Reset all packets back to "unknown support".  Called when opening a
>       new connection to a remote target.  */
>    void reset_all_packet_configs_support ();
> @@ -5764,6 +5771,8 @@ static const struct protocol_feature remote_protocol_features[] = {
>    { "no-resumed", PACKET_DISABLE, remote_supported_packet, PACKET_no_resumed },
>    { "memory-tagging", PACKET_DISABLE, remote_supported_packet,
>      PACKET_memory_tagging_feature },
> +  { "memory-tagging-check-addr", PACKET_DISABLE, remote_supported_packet,
> +    PACKET_memory_tagging_check_addr_feature },
>  };
>  
>  static char *remote_support_xml;
> @@ -5875,6 +5884,10 @@ remote_target::remote_query_supported ()
>  	  != AUTO_BOOLEAN_FALSE)
>  	remote_query_supported_append (&q, "memory-tagging+");
>  
> +      if (m_features.packet_set_cmd_state (PACKET_memory_tagging_check_addr_feature)
> +	  != AUTO_BOOLEAN_FALSE)
> +	remote_query_supported_append (&q, "memory-tagging-check-addr+");
> +
>        /* Keep this one last to work around a gdbserver <= 7.10 bug in
>  	 the qSupported:xmlRegisters=i386 handling.  */
>        if (remote_support_xml != NULL
> @@ -15534,6 +15547,21 @@ create_store_memtags_request (gdb::char_vector &packet, CORE_ADDR address,
>    strcpy (packet.data (), request.c_str ());
>  }
>  
> +static void
> +create_is_address_tagged_request (gdb::char_vector &packet, CORE_ADDR address)
> +{
> +  int addr_size;
> +  std::string request;
> +
> +  addr_size = gdbarch_addr_bit (current_inferior ()->arch()) / 8;

You should add another argument to create_is_address_tagged_request to pass in the gdbarch, then
you can use it when invoking gdbarch_addr_bit.

> +  request = string_printf ("qMemTagCheckAddr:%s", phex_nz (address, addr_size));
> +
> +  if (packet.size () < request.length () + 1)
> +    error (_("Contents too big for packet qMemTagCheckAddr."));
> +
> +  strcpy (packet.data (), request.c_str ());
> +}
> +
>  /* Implement the "fetch_memtags" target_ops method.  */
>  
>  bool
> @@ -15580,7 +15608,27 @@ remote_target::store_memtags (CORE_ADDR address, size_t len,
>  bool
>  remote_target::is_address_tagged (gdbarch *gdbarch, CORE_ADDR address)
>  {
> -  return gdbarch_tagged_address_p (gdbarch, address);
> +  struct remote_state *rs = get_remote_state ();
> +
> +  if (!m_features.remote_memory_tagging_check_addr_p ())
> +    /* Fallback to arch-specific method of checking whether an address is
> +       tagged.  */
> +    return gdbarch_tagged_address_p (gdbarch, address);
> +
> +  create_is_address_tagged_request (rs->buf, address);

Here you can pass the gdbarch you've already obtained from the incoming argument
of is_address_tagged.

> +
> +  putpkt (rs->buf);
> +  getpkt (&rs->buf);
> +
> +  /* Check if reply is OK.  */
> +  if (packet_check_result (rs->buf).status () != PACKET_OK)
> +    return false;
> +
> +  gdb_byte tagged_addr;
> +  /* Convert only 2 hex digits, i.e. 1 byte in hex format.  */
> +  hex2bin (rs->buf.data (), &tagged_addr , 1);
> +
> +  return tagged_addr != 0;

The current code assumes any reply != 0 means we have a tagged address. I think we
need to make that a bit more strict/explicit (in the documentation as well).

If we get a reply that is not 00 or 01, should we error out or issue a complaint?

If we plan on extending the packet to account for other return values, we should make
that known in the code and in the documentationm abd take appropriate action. That way
we avoid causing confusion for remote debugging stub implementors.

>  }>  
>  /* Return true if remote target T is non-stop.  */
> @@ -16066,6 +16114,10 @@ Show the maximum size of the address (in bits) in a memory packet."), NULL,
>    add_packet_config_cmd (PACKET_memory_tagging_feature,
>  			 "memory-tagging-feature", "memory-tagging-feature", 0);
>  
> +  add_packet_config_cmd (PACKET_memory_tagging_check_addr_feature,
> +			 "memory-tagging-check-addr-feature",
> +			 "memory-tagging-check-addr-feature", 0);
> +
>    /* Assert that we've registered "set remote foo-packet" commands
>       for all packet configs.  */
>    {

To make sure the new packet is exercised correctly when the gdb testsuite is executed,
it is worth adding some self tests. See remote.c:test_memory_tagging_functions for an
example of how it is done.

Basically we want to validate that gdb always handles things correctly and fails gracefully.

For example, we should handle the following replies:

- EXX (error)
- 00/01 (the expected return values)
- Any other numeric combinations of any arbitrary length
- Any other mix of numeric/non-numeric data of any arbitrary length

Pending the items above, the code looks OK to me.

Also, I've validated things on my end as well, and MTE still works fine for
native/core/remote (gdbserver).