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

[Simon Marchi via Gdb-patches <gdb-patches@sourceware.org>]

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?

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?

> 
> 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).  

Otherwise, that LGTM.

Simon