Re: [PATCH v3 01/24] New target methods for memory tagging support

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

On 12/25/20 1:26 AM, Simon Marchi wrote:
> On 2020-11-09 12:04 p.m., Luis Machado via Gdb-patches wrote:
>> Updates on v3:
>>
>> - Updated the code documentation for the fetch_memtags and store_memtags
>>    methods.
>>
>> Updates on v2:
>>
>> - Added type parameter to fetch_memtags/store_memtags hooks.
>>
>> --
>>
>> This patch starts adding some of the generic pieces to accomodate memory
>> tagging.
>>
>> We have three new target methods:
>>
>> - supports_memory_tagging: Checks if the target supports memory tagging. This
>>    defaults to false for targets that don't support memory tagging.
>>
>> - fetch_memtags: Fetches the allocation tags associated with a particular
>>    memory range [address, address + length).
>>
>>    The default is to return 0 without returning any tags. This should only
>>    be called if memory tagging is supported.
>>
>> - store_memtags: Stores a set of allocation tags for a particular memory
>>    range [address, address + length).
>>
>>    The default is to return 0. This should only
>>    be called if memory tagging is supported.
> 
> If fetch/store should only be called on targets for which supports_memory_tagging
> returns true, why is the default to return 0?

The text tries to make it more obvious that the architecture needs to 
support memory tagging before invoking these functions. It isn't the 
case for native targets, but we will always have target implementations 
for the remote targets.

There is a corner case for extended-remote where we connect to the 
target, but don't have any inferiors running. That means we don't get 
qSupported features and can't really be sure if the target supports 
memory tagging or not.

I think it is unlikely someone will invoke these functions incorrectly 
though.

> 
> It seems to me like it should be either:
> 
> - it's ok to call fetch/store on any target, if the target doesn't implement memtags,
>    the default of returning 0 is used
> - it's only ok to call fetch/store on targets that implement memtags, in which case
>    I would expect the default to maybe be tcomplain or even an assert
> 
> Or is there something I don't see?
> 

No. That makes sense. I've changed this to a tcomplain for both 
implementations.

>>
>> It also adds a control option for enabling/disabling memory tagging
>> manually: set memory-tagging on/off.
>>
>> The default is "on", with GDB making its use conditional to the
>> architecture supporting memory tagging.
>>
>> gdb/ChangeLog:
>>
>> YYYY-MM-DD  Luis Machado  <luis.machado@linaro.org>
>>
>> 	* printcmd.c (memtag): New static global.
>> 	(show_memtag): New function.
>> 	(_initialize_printcmd): Add set/show memory-tagging command.
>> 	* remote.c (remote_target) <supports_memory_tagging>: New method
>> 	override.
>> 	<fetch_memtags>: New method override.
>> 	<store_memtags>: New method override.
>> 	(remote_target::supports_memory_tagging): New method.
>> 	(remote_target::fetch_memtags): New method.
>> 	(remote_target::store_memtags): New method.
>> 	* target-delegates.c
>> 	(dummy_target) <supports_memory_tagging>: New method override.
>> 	<fetch_memtags>: New method override.
>> 	<store_memtags>: New method override.
>> 	(debug_target) <supports_memory_tagging>: New method override.
>> 	<fetch_memtags>: New method override.
>> 	<store_memtags>: New method override.
>> 	(target_ops::supports_memory_tagging): New method.
>> 	(target_ops::fetch_memtags): New method.
>> 	(target_ops::store_memtags): New method.
>> 	(dummy_target::supports_memory_tagging): New method.
>> 	(dummy_target::fetch_memtags): New method.
>> 	(dummy_target::store_memtags): New method.
>> 	(debug_target::supports_memory_tagging): New method.
>> 	(debug_target::fetch_memtags): New method.
>> 	(debug_target::store_memtags): New method.
> 
> Oh, you wrote this by hand :(.  target-delegates.c is generated (see the
> command at the top of the file).

Yikes. I see that now. Glad to not have to write one more piece of 
ChangeLog! :-)

> 
> Otherwise, that LGTM.
> 
> Simon
>
About returning bool... I recall we touched this subject before. The 
integer return was supposed to cover cases where we wanted to return 
error codes.

But, given we don't have any clearly defined error codes at the moment, 
I'll stick with bool for success/failure.