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

Simon Marchi via Gdb-patches
2020-12-25T05:09:07Z

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: ,<length>:<type>
> QMemTags: ,<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: ,<length>
> --
>
> Reads tags from the address range [ , )
>
> QMemTags: ,<length>:<uninterpreted tag bytes>
> --
> Writes the tags represented by the uninterpreted bytes to the address range
> [ , ).

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