Re: [PATCH v3 03/24] Add GDB-side remote target support for memory tagging

[Simon Marchi via Gdb-patches <gdb-patches@sourceware.org>]

On 2020-11-09 12:04 p.m., Luis Machado via Gdb-patches wrote:
> Updates for v2:
> 
> - Add type field to target hooks.
> - Add type data to qMemTags and QMemTags. The packets now look like this:
> 
> qMemTags:<address>,<length>:<type>
> QMemTags:<address>,<length>:<type>:<uninterpreted tag bytes>
> 
> --
> 
> This patch adds memory tagging support to GDB's remote side, with
> packet string checks, new packet support and an implementation of
> the two new tags methods fetch_atags and store_atags.

fetch_memtags / store_memtags ?

> 
> GDBserver needs to know how to read/write allocation tags, since that is
> done via ptrace.  It doesn't need to know about logical tags.
> 
> The new packets are:
> 
> qMemTags:<address>,<length>
> --
> 
> Reads tags from the address range [<address>, <address + length>)
> 
> QMemTags:<address>,<length>:<uninterpreted tag bytes>
> --
> Writes the tags represented by the uninterpreted bytes to the address range
> [<address>, <address + length>).

Should the packet description above be updated to include the "type" field?

> @@ -14404,7 +14422,65 @@ set_range_stepping (const char *ignore_args, int from_tty,
>  bool
>  remote_target::supports_memory_tagging ()
>  {
> -  return false;
> +  return remote_memory_tagging_p ();
> +}
> +
> +/* Create the qMemTags packet given ADDRESS, LEN and TYPE.
> +
> +   Return 0 if successful, non-zero otherwise.  */
> +
> +static void
> +create_fmemtags_request (gdb::char_vector &packet, CORE_ADDR address,
> +			 size_t len, int type)

The comment is wrong, the function does not return anything.

Why "fmemtags"?  Oh, it's for "fetch".  I'd prefer if you spelled
it completely (create_fetch_memtags_request), it's not very clear
otherwise (same for smemtags).

> +{
> +  int addr_size = gdbarch_addr_bit (target_gdbarch ()) / 8;
> +
> +  std::string request = string_printf ("qMemTags:%s,%s:%s",
> +				       phex_nz (address, addr_size),
> +				       phex_nz (len, sizeof (len)),
> +				       phex_nz (type, sizeof (type)));
> +
> +  strcpy (packet.data (), request.c_str ());
> +}
> +
> +/* Parse the qMemTags packet reply into TAGS.
> +
> +   Return 0 if successful, non-zero otherwise.  */
> +
> +static int
> +parse_fmemtags_reply (gdb::char_vector &reply, gdb::byte_vector &tags)

reply should be const, the return value should be bool.

Otherwise, LGTM.

Simon