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

Luis Machado via Gdb-patches
2020-12-28T16:28:33Z

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: ,<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.
>
> 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: ,<length>
>> --
>>
>> Reads tags from the address range [ , )
>>
>> QMemTags: ,<length>:<uninterpreted tag bytes>
>> --
>> Writes the tags represented by the uninterpreted bytes to the address range
>> [ , ).
>
> 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!