Re: [PATCH v2 4/4] gdb: Add new remote packet to check if address is tagged

[Thiago Jung Bauermann <thiago.bauermann@linaro.org>]

Hello Gustavo,

I started reviewing the patch series backwards...
I'll provide comments on this patch first.

Overall, it looks great. I just have some localised comments in a few
places below.

Also, for ease of review I suggest splitting this patch in two:

- one that introduces the new check_memtag_addr target hook and converts
  the existing code to use it,
- and another that adds a check_memtag_addr target hook implementation
  to the remote target.

Gustavo Romero <gustavo.romero@linaro.org> writes:

> This commit adds a new packet qMemTagAddrCheck allowing GDB remote
> targets to use it to query gdbservers if a given address is tagged.
>
> It also adds a new GDB remote feature, 'memory-tagging-check-add+',
> which must be advertised by the GDB servers to inform GDB they can reply
> to address checks via the new qMemTagAddrCheck remote packet.

You will need to document the remote protocol changes in gdb.texinfo, in
the "Remote Serial Protocol" appendix.

> Currently, this address check is done via a read query, where the
> contents of /proc/<PID>/smaps is read and the flags in there are
> inspected for MTE-related flags that indicate the address is in a tagged
> memory region.
>
> This is not ideal, for example, on QEMU gdbstub and in other cases,
> like in baremetal debugging, where there is no notion of any OS file
> like smaps. Hence, qMemTagAddrCheck packet allows check addresses in
> an OS-agnostic way.
>
> For supporting the new packet, a new target hook is introduced,
> check_memtag_addr, which is used instead of the gdbarch_tagged_address_p
> gdbarch hook in the upper layers (printcmd.c).
>
> The new target hook is then specialized per target, for remote.c,
> aarch64-linux-nat.c, and corelow.c targets (the current targets that
> are MTE-aware).
>
> The target hook in remote.c uses the qMemTagAddrCheck packet to check
> an address if the server advertised the 'memory-tagging-check-add+'
> feature, otherwise it falls back to using the current mechanism, i.e. it
> reads the /proc/<PID>/smaps contents.
>
> In the aarch64-linux-nat.c and corelow.c the target hook uses the
> gdbarch_tagged_address_p gdbarch hook, so there is no change regarding
> how an address is checked in these targets. Just the
> gdbarch_tagged_address_p signature is changed for convenience, since
> target_check_memtag_addr takes the address to be checked as a CORE_ADDR
> type.

I agree that a CORE_ADDR argument is more convenient.

> @@ -1071,6 +1073,12 @@ aarch64_linux_nat_target::store_memtags (CORE_ADDR address, size_t len,
>    return false;
>  }
>
> +bool
> +aarch64_linux_nat_target::check_memtag_addr (CORE_ADDR address)
> +{
> +  return gdbarch_tagged_address_p (current_inferior ()->arch (), address);

I think it's better to pass the gdbarch as an argument to the
check_memtag_addr hook rather than getting it from current_inferior
here, even if in practice your patch is equivalent to the existing
code.

The reason is that we are trying to move calls to current_* functions
(which is global state in disguise) up the stack, so that most of GDB
needs to reference only local state.

Then if callers have a gdbarch available in their context they can pass
it to the hook, or else they can use current_inferior ()->arch ().

> @@ -1410,6 +1412,12 @@ core_target::fetch_memtags (CORE_ADDR address, size_t len,
>    return false;
>  }
>
> +bool
> +core_target::check_memtag_addr (CORE_ADDR address)
> +{
> +  return gdbarch_tagged_address_p (current_inferior ()->arch (), address);

Same comment here, of course.

> diff --git a/gdb/printcmd.c b/gdb/printcmd.c
> index ae4d640ccf2..c81c75afc5d 100644
> --- a/gdb/printcmd.c
> +++ b/gdb/printcmd.c
> @@ -1132,7 +1132,7 @@ do_examine (struct format_data fmt, struct gdbarch *gdbarch, CORE_ADDR addr)
>  	    = value_from_ulongest (builtin_type (gdbarch)->builtin_data_ptr,
>  				   tag_laddr);
>
> -	  if (gdbarch_tagged_address_p (current_inferior ()->arch  (), v_addr))
> +	  if (target_check_memtag_addr (value_as_address(v_addr)))

Missing space between "value_as_address" and the opening parens.

Also, not a problem introduced by you, but I don't understand why the code
uses the gdbarch from current_inferior if it was passed a gdbarch in the
arguments.

So I'd add a separate patch before this one to fix this code to use the
gdbarch that was passed as a parameter. Then this patch can also pass it
as a parameter to target_check_memtag_addr as I mentioned in an earlier
comment.

>  	    {
>  	      /* Fetch the allocation tag.  */
>  	      struct value *tag
> @@ -1289,7 +1289,7 @@ should_validate_memtags (struct value *value)
>      return false;
>
>    /* We do.  Check whether it includes any tags.  */
> -  return gdbarch_tagged_address_p (current_inferior ()->arch  (), value);
> +  return target_check_memtag_addr (value_as_address(value));

Here there's no gdbarch available in context, but since there's only one
caller to this function, it's easy to add the gdbarch parameter to it
and make the caller pass it down. The caller does get it from
current_inferior, so perhaps I'm being too pedantic here but IMHO it's
worth it.

Also, a space is missing before the opening parens.

>  }
>
>  /* Helper for parsing arguments for print_command_1.  */
> @@ -2946,9 +2946,10 @@ memory_tag_print_tag_command (const char *args, enum memtag_type tag_type)
>       flag, it is no use trying to access/manipulate its allocation tag.
>
>       It is OK to manipulate the logical tag though.  */
> +  CORE_ADDR addr = value_as_address(val);

Missing space before the opening parens.

>    if (tag_type == memtag_type::allocation
> -      && !gdbarch_tagged_address_p (arch, val))
> -    show_addr_not_tagged (value_as_address (val));
> +      && !target_check_memtag_addr(addr))

Missing space before the opening parens.

> +    show_addr_not_tagged (addr);
>
>    value *tag_value = gdbarch_get_memtag (arch, val, tag_type);
>    std::string tag = gdbarch_memtag_to_string (arch, tag_value);
> @@ -3104,8 +3105,9 @@ parse_set_allocation_tag_input (const char *args, struct value **val,
>
>    /* If the address is not in a region memory mapped with a memory tagging
>       flag, it is no use trying to access/manipulate its allocation tag.  */
> -  if (!gdbarch_tagged_address_p (current_inferior ()->arch (), *val))
> -    show_addr_not_tagged (value_as_address (*val));
> +  CORE_ADDR addr = value_as_address (*val);
> +  if (!target_check_memtag_addr (addr))
> +    show_addr_not_tagged (addr);

This is another instance where I'd suggest making the caller pass
gdbarch as an argument so that it can be used here, since this function
only has one caller.

>  }
>
>  /* Implement the "memory-tag set-allocation-tag" command.
> @@ -3129,8 +3131,9 @@ memory_tag_set_allocation_tag_command (const char *args, int from_tty)
>
>    /* If the address is not in a region memory mapped with a memory tagging
>       flag, it is no use trying to manipulate its allocation tag.  */
> -  if (!gdbarch_tagged_address_p (current_inferior ()->arch (), val)) {
> -    show_addr_not_tagged (value_as_address(val));
> +  CORE_ADDR addr = value_as_address (val);
> +  if (!target_check_memtag_addr (addr)) {
> +    show_addr_not_tagged (addr);
>    }

This is a preexisting issue in the code, but since you're touching it:
the GNU style is to not use curly braces when there's only one statement
in the if block.

> @@ -15532,6 +15547,19 @@ create_store_memtags_request (gdb::char_vector &packet, CORE_ADDR address,
>    strcpy (packet.data (), request.c_str ());
>  }
>
> +static void
> +create_check_memtag_addr_request (gdb::char_vector &packet, CORE_ADDR address)
> +{
> +  int addr_size = gdbarch_addr_bit (current_inferior ()->arch()) / 8;
> +
> +  std::string request = string_printf ("qMemTagAddrCheck:%s", phex_nz (address, addr_size));
> +
> +  if (packet.size () < request.length ())

There's an off-by-one error here: packet is expected to be
null-terminated, but request.length doesn't count a terminating null
byte so a "+ 1" needs to be added here.

> +    error (_("Contents too big for packet qMemTagAddrCheck."));
> +
> +  strcpy (packet.data (), request.c_str ());
> +}
> +
>  /* Implement the "fetch_memtags" target_ops method.  */
>
>  bool
> @@ -15573,6 +15601,36 @@ remote_target::store_memtags (CORE_ADDR address, size_t len,
>    return packet_check_result (rs->buf).status () == PACKET_OK;
>  }
>
> +bool
> +remote_target::check_memtag_addr (CORE_ADDR address)
> +{
> +  struct remote_state *rs = get_remote_state ();
> +
> +  if (!m_features.remote_memory_tagging_check_addr_p ())
> +    /* Fallback to reading /proc/<PID>/smaps for checking if an address is
> +       tagged or not.  */

Currently this comment is accurate, but if some other architecture adds
a gdbarch method that doesn't use /proc/<PID>/smaps then it won't be
anymore.

I suggest saying something like "Fallback to arch-specific method of
checking whether an address is tagged".

> +    return gdbarch_tagged_address_p (current_inferior ()->arch (), address);
> +
> +  create_check_memtag_addr_request (rs->buf, address);
> +
> +  putpkt (rs->buf);
> +  getpkt (&rs->buf);
> +
> +  /* Check if reply is OK.  */
> +  if ((packet_check_result (rs->buf).status () != PACKET_OK) || rs->buf.empty())

Missing space between "empty" and opening parens.

But I don't understand why check whether buf is empty. Looking at
remote_target::getpkt, it doesn't look like buf is ever emptied.

> +    return false;
> +
> +  gdb_byte tagged_addr;
> +  /* Convert only 2 hex digits, i.e. 1 byte in hex format.  */
> +  hex2bin(rs->buf.data(), &tagged_addr , 1);

Missing space between "hex2bin", "data" and the opening parens.

> +  if (tagged_addr)
> +    /* 01 means address is tagged.  */
> +    return true;
> +  else
> +    /* 00 means address is not tagged.  */
> +    return false;

The above can be simplified to "return tagged_addr != 0".

--
Thiago