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

Alan Hayward via Gdb-patches
2020-10-29T14:22:31Z

> On 22 Oct 2020, at 20:59, Luis Machado <luis.machado@linaro.org> 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.
>
> 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
> [ , ).
>
> The interpretation of what to do with the tag bytes is up to the arch-specific
> code.
>
> Note that these new packets consider the case of packet size overflow as an
> error, given the common use case is to read/write only a few memory tags at
> a time. Having to use a couple new packets for multi-part transfers wouldn't
> make sense for the little use it would have.
>

Minor nit: "Having to use a couple new packets”. Even after adding an “of” I’m
not fully sure if that’s what you meant to write.

> gdb/ChangeLog:
>
> YYYY-MM-DD Luis Machado <luis.machado@linaro.org>
>
> * remote.c (PACKET_memory_tagging_feature): New enum.
> (remote_memory_tagging_p): New function.
> (remote_protocol_features): New "memory-tagging" entry.
> (remote_target::remote_query_supported): Handle memory tagging
> support.
> (remote_target::supports_memory_tagging): Implement.
> (create_fmemtags_request, parse_fmemtags_reply)
> (create_smemtags_request): New functions.
> (remote_target::fetch_memtags): Implement.
> (remote_target::store_memtags): Implement.
> (_initialize_remote): Add new "memory-tagging-feature"
> config command.
> ---
> gdb/remote.c | 109 ++++++++++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 108 insertions(+), 1 deletion(-)
>
> diff --git a/gdb/remote.c b/gdb/remote.c
> index 6e37e5f4e6..fdc7645d47 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -2104,6 +2104,10 @@ enum {
> /* Support TARGET_WAITKIND_NO_RESUMED. */
> PACKET_no_resumed,
>
> + /* Support for memory tagging, allocation tag fetch/store
> + packets and the tag violation stop replies. */
> + PACKET_memory_tagging_feature,
> +
> PACKET_MAX
> };
>
> @@ -2245,6 +2249,14 @@ remote_exec_event_p (struct remote_state *rs)
> return packet_support (PACKET_exec_event_feature) == PACKET_ENABLE;
> }
>
> +/* Returns true if memory tagging is supported. */
> +
> +static bool
> +remote_memory_tagging_p (void)
> +{
> + return packet_support (PACKET_memory_tagging_feature) == PACKET_ENABLE;
> +}
> +
> /* Insert fork catchpoint target routine. If fork events are enabled
> then return success, nothing more to do. */
>
> @@ -5246,6 +5258,8 @@ static const struct protocol_feature remote_protocol_features[] = {
> { "vContSupported", PACKET_DISABLE, remote_supported_packet, PACKET_vContSupported },
> { "QThreadEvents", PACKET_DISABLE, remote_supported_packet, PACKET_QThreadEvents },
> { "no-resumed", PACKET_DISABLE, remote_supported_packet, PACKET_no_resumed },
> + { "memory-tagging", PACKET_DISABLE, remote_supported_packet,
> + PACKET_memory_tagging_feature },
> };
>
> static char *remote_support_xml;
> @@ -5340,6 +5354,10 @@ remote_target::remote_query_supported ()
> if (packet_set_cmd_state (PACKET_no_resumed) != AUTO_BOOLEAN_FALSE)
> remote_query_supported_append (&q, "no-resumed+");
>
> + if (packet_set_cmd_state (PACKET_memory_tagging_feature)
> + != AUTO_BOOLEAN_FALSE)
> + remote_query_supported_append (&q, "memory-tagging+");
> +
> /* Keep this one last to work around a gdbserver <= 7.10 bug in
> the qSupported:xmlRegisters=i386 handling. */
> if (remote_support_xml != NULL
> @@ -14402,7 +14420,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)
> +{
> + 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)
> +{
> + if (reply.empty () || reply[0] == 'E' || reply[0] != 'm')
> + return 1;
> +
> + /* Copy the tag data. */
> + tags = hex2bin (reply.data () + 1);
> +
> + return 0;
> +}
> +
> +/* Create the QMemTags packet given ADDRESS, LEN, TYPE and TAGS.
> +
> + Return 0 if successful, non-zero otherwise. */
> +
> +static void
> +create_smemtags_request (gdb::char_vector &packet, CORE_ADDR address,
> + size_t len, int type, const gdb::byte_vector &tags)
> +{
> + int addr_size = gdbarch_addr_bit (target_gdbarch ()) / 8;
> +
> + /* Put together the main packet, address and length. */
> + std::string request = string_printf ("QMemTags:%s,%s:%s:",
> + phex_nz (address, addr_size),
> + phex_nz (len, sizeof (len)),
> + phex_nz (type, sizeof (type)));
> + request += bin2hex (tags.data (), tags.size ());
> +
> + /* Check if we have exceeded the maximum packet size. */
> + if (packet.size () < request.length ())
> + error (_("Contents too big for packet QMemTags."));
> +
> + strcpy (packet.data (), request.c_str ());
> }
>
> /* Implement the "fetch_memtags" target_ops method. */
> @@ -14411,6 +14487,19 @@ int
> remote_target::fetch_memtags (CORE_ADDR address, size_t len,
> gdb::byte_vector &tags, int type)
> {
> + /* Make sure the qMemTags packet is supported. */
> + if (!remote_memory_tagging_p ())
> + gdb_assert_not_reached ("remote fetch_memtags called with packet disabled");
> +
> + struct remote_state *rs = get_remote_state ();
> +
> + create_fmemtags_request (rs->buf, address, len, type);
> +
> + putpkt (rs->buf);
> + getpkt (&rs->buf, 0);
> +
> + parse_fmemtags_reply (rs->buf, tags);
> +
> return 0;
> }
>
> @@ -14420,6 +14509,21 @@ int
> remote_target::store_memtags (CORE_ADDR address, size_t len,
> const gdb::byte_vector &tags, int type)
> {
> + /* Make sure the QMemTags packet is supported. */
> + if (!remote_memory_tagging_p ())
> + gdb_assert_not_reached ("remote store_memtags called with packet disabled");
> +
> + struct remote_state *rs = get_remote_state ();
> +
> + create_smemtags_request (rs->buf, address, len, type, tags);
> +
> + putpkt (rs->buf);
> + getpkt (&rs->buf, 0);
> +
> + /* Verify if the request was successful. */
> + if (packet_check_result (rs->buf.data ()) != PACKET_OK)
> + return 1;
> +
> return 0;
> }
>
> @@ -14822,6 +14926,9 @@ Show the maximum size of the address (in bits) in a memory packet."), NULL,
> add_packet_config_cmd (&remote_protocol_packets[PACKET_no_resumed],
> "N stop reply", "no-resumed-stop-reply", 0);
>
> + add_packet_config_cmd (&remote_protocol_packets[PACKET_memory_tagging_feature],
> + "memory-tagging-feature", "memory-tagging-feature", 0);
> +
> /* Assert that we've registered "set remote foo-packet" commands
> for all packet configs. */
> {
> --
> 2.17.1
>