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

Simon Marchi via Gdb-patches
2020-12-25T04:26:48Z

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