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

[Alan Hayward via Gdb-patches <gdb-patches@sourceware.org>]

> 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:<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.
> 
> 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>).
> 
> 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
>