Re: [PATCH v3 05/24] GDBserver remote packet support for memory tagging

Luis Machado via Gdb-patches
2020-12-28T17:46:12Z

On 12/25/20 2:50 AM, Simon Marchi wrote:
>> diff --git a/gdbserver/server.cc b/gdbserver/server.cc
>> index 95db9807a9..7eddd0e01f 100644
>> --- a/gdbserver/server.cc
>> +++ b/gdbserver/server.cc
>> @@ -547,12 +547,71 @@ handle_btrace_conf_general_set (char *own_buf)
>> return 1;
>> }
>>
>> +/* Create the qMemTags packet reply given TAGS. */
>> +
>> +static int
>> +create_fmemtags_reply (char *reply, const gdb::byte_vector &tags)
>> +{
>> + /* It is an error to pass a zero-sized tag vector. */
>> + if (tags.size () == 0)
>> + return 1;
>
> If this catches a logic error in GDBserver, this should be an assert.
>

That makes sense. Fixed with an assertion.

>> +
>> + std::string packet ("m");
>> +
>> + /* Write the tag data. */
>> + packet += bin2hex (tags.data (), tags.size ());
>> +
>> + /* Check if the reply is too big for the packet to handle. */
>> + if (PBUFSIZ < packet.size ())
>> + return 1;
>> +
>> + strcpy (reply, packet.c_str ());
>> + return 0;
>> +}
>> +
>> +/* Parse the QMemTags request into ADDR, LEN and TAGS.
>> +
>> + Return 0 if successful, non-zero otherwise. */
>> +
>> +static int
>> +parse_smemtags_request (char *request, CORE_ADDR *addr, size_t *len,
>> + gdb::byte_vector &tags, int *type)
>> +{
>> + if (!startswith (request, "QMemTags:"))
>> + return 1;
>
> That check seems unnecessary, or maybe I'd convert it to an assert.
>

I've converted both occurrences of this to an assertion, just to be safe.

>> diff --git a/gdbserver/server.h b/gdbserver/server.h
>> index 22228050a8..3d4a086e18 100644
>> --- a/gdbserver/server.h
>> +++ b/gdbserver/server.h
>> @@ -190,6 +190,9 @@ struct client_state
>>
>> int current_traceframe = -1;
>>
>> + /* If true, memory tagging features are supported. */
>> + bool memory_tagging_feature = false;
>
> Just a note that as of this patch, this field is unnecessary. If it gets
> used later, that's fine.
>

Right. The way the series was put together, each part builds on top of
the previous one. So this will be used in later patches.

>> @@ -499,6 +500,19 @@ class process_stratum_target
>>
>> /* Return tdesc index for IPA. */
>> virtual int get_ipa_tdesc_idx ();
>> +
>> + /* Returns true if the target supports memory tagging facilities. */
>> + virtual bool supports_memory_tagging ();
>> +
>> + /* Return the allocated memory tags of type TYPE associated with
>> + [ADDRESS, ADDRESS + LEN) in TAGS. */
>> + virtual int fetch_memtags (CORE_ADDR address, size_t len,
>> + gdb::byte_vector &tags, int type);
>> +
>> + /* Write the allocation tags of type TYPE contained in TAGS to the
>> + memory range [ADDRESS, ADDRESS + LEN). */
>> + virtual int store_memtags (CORE_ADDR address, size_t len,
>> + const gdb::byte_vector &tags, int type);
>
> I suppose that these should return bool.
>
> Simon
>

Yes. Updated to match what was done for the GDB changes. I also updated
the documentation for functions and the naming to spell it completely.