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

[Luis Machado via Gdb-patches <gdb-patches@sourceware.org>]

On 12/25/20 2:08 AM, Simon Marchi wrote:
> On 2020-11-09 12:04 p.m., Luis Machado via Gdb-patches 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.
> 
> fetch_memtags / store_memtags ?
> 

Yeah. This (and others below) are an artifact of some back-and-forth on 
reasonable names. Fixed now.

>>
>> 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>).
> 
> Should the packet description above be updated to include the "type" field?
> 

Yes. Fixed now.

>> @@ -14404,7 +14422,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)
> 
> The comment is wrong, the function does not return anything.
> 
> Why "fmemtags"?  Oh, it's for "fetch".  I'd prefer if you spelled
> it completely (create_fetch_memtags_request), it's not very clear
> otherwise (same for smemtags).
> 

Exactly. Updated now to spell it completely.

>> +{
>> +  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)
> 
> reply should be const, the return value should be bool.
> 

Fixed now.

> Otherwise, LGTM.
> 
> Simon
> 

Thanks!