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

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

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.