Re: [PATCH v3 05/24] GDBserver remote packet support for memory tagging

Simon Marchi via Gdb-patches
2020-12-25T05:50:56Z

> diff --git a/gdbserver/server.cc b/gdbserver/server.cc
> index 95db9807a9..7eddd0e01f 100644
> --- a/gdbserver/server.cc
> +++ b/gdbserver/server.cc
> @@ -547,12 +547,71 @@ handle_btrace_conf_general_set (char *own_buf)
> return 1;
> }
>
> +/* Create the qMemTags packet reply given TAGS. */
> +
> +static int
> +create_fmemtags_reply (char *reply, const gdb::byte_vector &tags)
> +{
> + /* It is an error to pass a zero-sized tag vector. */
> + if (tags.size () == 0)
> + return 1;

If this catches a logic error in GDBserver, this should be an assert.

> +
> + std::string packet ("m");
> +
> + /* Write the tag data. */
> + packet += bin2hex (tags.data (), tags.size ());
> +
> + /* Check if the reply is too big for the packet to handle. */
> + if (PBUFSIZ < packet.size ())
> + return 1;
> +
> + strcpy (reply, packet.c_str ());
> + return 0;
> +}
> +
> +/* Parse the QMemTags request into ADDR, LEN and TAGS.
> +
> + Return 0 if successful, non-zero otherwise. */
> +
> +static int
> +parse_smemtags_request (char *request, CORE_ADDR *addr, size_t *len,
> + gdb::byte_vector &tags, int *type)
> +{
> + if (!startswith (request, "QMemTags:"))
> + return 1;

That check seems unnecessary, or maybe I'd convert it to an assert.

> diff --git a/gdbserver/server.h b/gdbserver/server.h
> index 22228050a8..3d4a086e18 100644
> --- a/gdbserver/server.h
> +++ b/gdbserver/server.h
> @@ -190,6 +190,9 @@ struct client_state
>
> int current_traceframe = -1;
>
> + /* If true, memory tagging features are supported. */
> + bool memory_tagging_feature = false;

Just a note that as of this patch, this field is unnecessary. If it gets
used later, that's fine.

> @@ -499,6 +500,19 @@ class process_stratum_target
>
> /* Return tdesc index for IPA. */
> virtual int get_ipa_tdesc_idx ();
> +
> + /* Returns true if the target supports memory tagging facilities. */
> + virtual bool supports_memory_tagging ();
> +
> + /* Return the allocated memory tags of type TYPE associated with
> + [ADDRESS, ADDRESS + LEN) in TAGS. */
> + virtual int fetch_memtags (CORE_ADDR address, size_t len,
> + gdb::byte_vector &tags, int type);
> +
> + /* Write the allocation tags of type TYPE contained in TAGS to the
> + memory range [ADDRESS, ADDRESS + LEN). */
> + virtual int store_memtags (CORE_ADDR address, size_t len,
> + const gdb::byte_vector &tags, int type);

I suppose that these should return bool.

Simon