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

[Gustavo Romero <gustavo.romero@linaro.org>]

Hi Thiago,

Thanks a lot for the nice review. I think it helped a lot the
series organization.

On 3/29/24 8:35 PM, Thiago Jung Bauermann wrote:
> 
> 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.

Done in v3.

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

Done in v3.

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

Done in v3 in commit "gdb: Use passed gdbarch instead of calling current_inferior".


>>   	    {
>>   	      /* 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.

OK, done in v3.


> 
>>   }
>>
>>   /* 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.

Good catch, thanks. Fixed in v3.

  
>> +    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".

Done in v3.


>> +    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.

I removed rs->buf.empty() in v3.


>> +    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".

Done in v3.


Cheers,
Gustavo