gdb: aarch64: Support MTE on baremetal

  This commit moves aarch64_linux_memtag_matches_p,
aarch64_linux_set_memtags, aarch64_linux_get_memtag, and
aarch64_linux_memtag_to_string hooks (plus aarch64_mte_get_atag
function used by them), along with the setting of the memtag granule
size, from aarch64-linux-tdep.c to aarch64-tdep.c, making MTE available
on baremetal targets. Since the aarch64-linux-tdep.c layer inherits
these hooks from aarch64-tdep.c, there is no effective change for
aarch64-linux targets.

Signed-off-by: Gustavo Romero <gustavo.romero@linaro.org>
---
 gdb/aarch64-linux-tdep.c | 167 --------------------------------------
 gdb/aarch64-tdep.c       | 168 +++++++++++++++++++++++++++++++++++++++
 gdb/aarch64-tdep.h       |   3 +
 3 files changed, 171 insertions(+), 167 deletions(-)
  

Hi,

Thanks for the patch.

On 7/22/24 15:41, Gustavo Romero wrote:
> This commit moves aarch64_linux_memtag_matches_p,
> aarch64_linux_set_memtags, aarch64_linux_get_memtag, and
> aarch64_linux_memtag_to_string hooks (plus aarch64_mte_get_atag
> function used by them), along with the setting of the memtag granule
> size, from aarch64-linux-tdep.c to aarch64-tdep.c, making MTE available
> on baremetal targets. Since the aarch64-linux-tdep.c layer inherits
> these hooks from aarch64-tdep.c, there is no effective change for
> aarch64-linux targets.
> 
> Signed-off-by: Gustavo Romero <gustavo.romero@linaro.org>
> ---
>  gdb/aarch64-linux-tdep.c | 167 --------------------------------------
>  gdb/aarch64-tdep.c       | 168 +++++++++++++++++++++++++++++++++++++++
>  gdb/aarch64-tdep.h       |   3 +
>  3 files changed, 171 insertions(+), 167 deletions(-)
> 
> diff --git a/gdb/aarch64-linux-tdep.c b/gdb/aarch64-linux-tdep.c
> index a1296a8f0c7..63defd5a31f 100644
> --- a/gdb/aarch64-linux-tdep.c
> +++ b/gdb/aarch64-linux-tdep.c
> @@ -2427,29 +2427,6 @@ aarch64_linux_gcc_target_options (struct gdbarch *gdbarch)
>    return {};
>  }
>  
> -/* Helper to get the allocation tag from a 64-bit ADDRESS.
> -
> -   Return the allocation tag if successful and nullopt otherwise.  */
> -
> -static std::optional<CORE_ADDR>
> -aarch64_mte_get_atag (CORE_ADDR address)
> -{
> -  gdb::byte_vector tags;
> -
> -  /* Attempt to fetch the allocation tag.  */
> -  if (!target_fetch_memtags (address, 1, tags,
> -			     static_cast<int> (memtag_type::allocation)))
> -    return {};
> -
> -  /* Only one tag should've been returned.  Make sure we got exactly that.  */
> -  if (tags.size () != 1)
> -    error (_("Target returned an unexpected number of tags."));
> -
> -  /* Although our tags are 4 bits in size, they are stored in a
> -     byte.  */
> -  return tags[0];
> -}
> -
>  /* Implement the tagged_address_p gdbarch method.  */
>  
>  static bool
> @@ -2466,132 +2443,6 @@ aarch64_linux_tagged_address_p (struct gdbarch *gdbarch, CORE_ADDR address)
>    return true;
>  }
>  
> -/* Implement the memtag_matches_p gdbarch method.  */
> -
> -static bool
> -aarch64_linux_memtag_matches_p (struct gdbarch *gdbarch,
> -				struct value *address)
> -{
> -  gdb_assert (address != nullptr);
> -
> -  CORE_ADDR addr = value_as_address (address);
> -
> -  /* Fetch the allocation tag for ADDRESS.  */
> -  std::optional<CORE_ADDR> atag
> -    = aarch64_mte_get_atag (gdbarch_remove_non_address_bits (gdbarch, addr));
> -
> -  if (!atag.has_value ())
> -    return true;
> -
> -  /* Fetch the logical tag for ADDRESS.  */
> -  gdb_byte ltag = aarch64_mte_get_ltag (addr);
> -
> -  /* Are the tags the same?  */
> -  return ltag == *atag;
> -}
> -
> -/* Implement the set_memtags gdbarch method.  */
> -
> -static bool
> -aarch64_linux_set_memtags (struct gdbarch *gdbarch, struct value *address,
> -			   size_t length, const gdb::byte_vector &tags,
> -			   memtag_type tag_type)
> -{
> -  gdb_assert (!tags.empty ());
> -  gdb_assert (address != nullptr);
> -
> -  CORE_ADDR addr = value_as_address (address);
> -
> -  /* Set the logical tag or the allocation tag.  */
> -  if (tag_type == memtag_type::logical)
> -    {
> -      /* When setting logical tags, we don't care about the length, since
> -	 we are only setting a single logical tag.  */
> -      addr = aarch64_mte_set_ltag (addr, tags[0]);
> -
> -      /* Update the value's content with the tag.  */
> -      enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
> -      gdb_byte *srcbuf = address->contents_raw ().data ();
> -      store_unsigned_integer (srcbuf, sizeof (addr), byte_order, addr);
> -    }
> -  else
> -    {
> -      /* Remove the top byte.  */
> -      addr = gdbarch_remove_non_address_bits (gdbarch, addr);
> -
> -      /* With G being the number of tag granules and N the number of tags
> -	 passed in, we can have the following cases:
> -
> -	 1 - G == N: Store all the N tags to memory.
> -
> -	 2 - G < N : Warn about having more tags than granules, but write G
> -		     tags.
> -
> -	 3 - G > N : This is a "fill tags" operation.  We should use the tags
> -		     as a pattern to fill the granules repeatedly until we have
> -		     written G tags to memory.
> -      */
> -
> -      size_t g = aarch64_mte_get_tag_granules (addr, length,
> -					       AARCH64_MTE_GRANULE_SIZE);
> -      size_t n = tags.size ();
> -
> -      if (g < n)
> -	warning (_("Got more tags than memory granules.  Tags will be "
> -		   "truncated."));
> -      else if (g > n)
> -	warning (_("Using tag pattern to fill memory range."));
> -
> -      if (!target_store_memtags (addr, length, tags,
> -				 static_cast<int> (memtag_type::allocation)))
> -	return false;
> -    }
> -  return true;
> -}
> -
> -/* Implement the get_memtag gdbarch method.  */
> -
> -static struct value *
> -aarch64_linux_get_memtag (struct gdbarch *gdbarch, struct value *address,
> -			  memtag_type tag_type)
> -{
> -  gdb_assert (address != nullptr);
> -
> -  CORE_ADDR addr = value_as_address (address);
> -  CORE_ADDR tag = 0;
> -
> -  /* Get the logical tag or the allocation tag.  */
> -  if (tag_type == memtag_type::logical)
> -    tag = aarch64_mte_get_ltag (addr);
> -  else
> -    {
> -      /* Remove the top byte.  */
> -      addr = gdbarch_remove_non_address_bits (gdbarch, addr);
> -      std::optional<CORE_ADDR> atag = aarch64_mte_get_atag (addr);
> -
> -      if (!atag.has_value ())
> -	return nullptr;
> -
> -      tag = *atag;
> -    }
> -
> -  /* Convert the tag to a value.  */
> -  return value_from_ulongest (builtin_type (gdbarch)->builtin_unsigned_int,
> -			      tag);
> -}
> -
> -/* Implement the memtag_to_string gdbarch method.  */
> -
> -static std::string
> -aarch64_linux_memtag_to_string (struct gdbarch *gdbarch, struct value *tag_value)
> -{
> -  if (tag_value == nullptr)
> -    return "";
> -
> -  CORE_ADDR tag = value_as_address (tag_value);
> -
> -  return string_printf ("0x%s", phex_nz (tag, sizeof (tag)));
> -}
>  
>  /* AArch64 Linux implementation of the report_signal_info gdbarch
>     hook.  Displays information about possible memory tag violations.  */
> @@ -2900,24 +2751,6 @@ aarch64_linux_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
>        /* Register a hook for checking if an address is tagged or not.  */
>        set_gdbarch_tagged_address_p (gdbarch, aarch64_linux_tagged_address_p);
>  
> -      /* Register a hook for checking if there is a memory tag match.  */
> -      set_gdbarch_memtag_matches_p (gdbarch,
> -				    aarch64_linux_memtag_matches_p);
> -
> -      /* Register a hook for setting the logical/allocation tags for
> -	 a range of addresses.  */
> -      set_gdbarch_set_memtags (gdbarch, aarch64_linux_set_memtags);
> -
> -      /* Register a hook for extracting the logical/allocation tag from an
> -	 address.  */
> -      set_gdbarch_get_memtag (gdbarch, aarch64_linux_get_memtag);
> -
> -      /* Set the allocation tag granule size to 16 bytes.  */
> -      set_gdbarch_memtag_granule_size (gdbarch, AARCH64_MTE_GRANULE_SIZE);
> -
> -      /* Register a hook for converting a memory tag to a string.  */
> -      set_gdbarch_memtag_to_string (gdbarch, aarch64_linux_memtag_to_string);
> -
>        set_gdbarch_report_signal_info (gdbarch,
>  				      aarch64_linux_report_signal_info);
>  
> diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
> index e4bca6c6632..f6227a5b82d 100644
> --- a/gdb/aarch64-tdep.c
> +++ b/gdb/aarch64-tdep.c
> @@ -45,6 +45,7 @@
>  
>  #include "aarch64-tdep.h"
>  #include "aarch64-ravenscar-thread.h"
> +#include "arch/aarch64-mte-linux.h"

Including a more specific scope into a generic one is incorrect. In this
particular case we're including a *linux* header into a supposedly generic
aarch64-tdep.c file.

If we need to use things contained in the Linux files, we should make sure they can be made
generic and then extract those out from arch/aarch64-mte-linux.[c|h] and then move them
to, say, arch/aarch64-mte.[c|h].

>  
>  #include "record.h"
>  #include "record-full.h"
> @@ -4088,6 +4089,156 @@ aarch64_stack_frame_destroyed_p (struct gdbarch *gdbarch, CORE_ADDR pc)
>    return streq (inst.opcode->name, "ret");
>  }
>  
> +/* Helper to get the allocation tag from a 64-bit ADDRESS.
> +
> +   Return the allocation tag if successful and nullopt otherwise.  */
> +
> +std::optional<CORE_ADDR>
> +aarch64_mte_get_atag (CORE_ADDR address)
> +{
> +  gdb::byte_vector tags;
> +
> +  /* Attempt to fetch the allocation tag.  */
> +  if (!target_fetch_memtags (address, 1, tags,
> +			     static_cast<int> (memtag_type::allocation)))
> +    return {};
> +
> +  /* Only one tag should've been returned.  Make sure we got exactly that.  */
> +  if (tags.size () != 1)
> +    error (_("Target returned an unexpected number of tags."));
> +
> +  /* Although our tags are 4 bits in size, they are stored in a
> +     byte.  */
> +  return tags[0];
> +}
> +
> +/* Implement the memtag_matches_p gdbarch method.  */
> +
> +static bool
> +aarch64_memtag_matches_p (struct gdbarch *gdbarch,
> +				struct value *address)

There seems to be something off with indentation above.

> +{
> +  gdb_assert (address != nullptr);
> +
> +  CORE_ADDR addr = value_as_address (address);
> +
> +  /* Fetch the allocation tag for ADDRESS.  */
> +  std::optional<CORE_ADDR> atag
> +    = aarch64_mte_get_atag (gdbarch_remove_non_address_bits (gdbarch, addr));
> +
> +  if (!atag.has_value ())
> +    return true;
> +
> +  /* Fetch the logical tag for ADDRESS.  */
> +  gdb_byte ltag = aarch64_mte_get_ltag (addr);
> +
> +  /* Are the tags the same?  */
> +  return ltag == *atag;
> +}
> +
> +/* Implement the set_memtags gdbarch method.  */
> +
> +static bool
> +aarch64_set_memtags (struct gdbarch *gdbarch, struct value *address,
> +			   size_t length, const gdb::byte_vector &tags,
> +			   memtag_type tag_type)

Indentation above looks off too.

> +{
> +  gdb_assert (!tags.empty ());
> +  gdb_assert (address != nullptr);
> +
> +  CORE_ADDR addr = value_as_address (address);
> +
> +  /* Set the logical tag or the allocation tag.  */
> +  if (tag_type == memtag_type::logical)
> +    {
> +      /* When setting logical tags, we don't care about the length, since
> +	 we are only setting a single logical tag.  */
> +      addr = aarch64_mte_set_ltag (addr, tags[0]);
> +
> +      /* Update the value's content with the tag.  */
> +      enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
> +      gdb_byte *srcbuf = address->contents_raw ().data ();
> +      store_unsigned_integer (srcbuf, sizeof (addr), byte_order, addr);
> +    }
> +  else
> +    {
> +      /* Remove the top byte.  */
> +      addr = gdbarch_remove_non_address_bits (gdbarch, addr);
> +
> +      /* With G being the number of tag granules and N the number of tags
> +	 passed in, we can have the following cases:
> +
> +	 1 - G == N: Store all the N tags to memory.
> +
> +	 2 - G < N : Warn about having more tags than granules, but write G
> +		     tags.
> +
> +	 3 - G > N : This is a "fill tags" operation.  We should use the tags
> +		     as a pattern to fill the granules repeatedly until we have
> +		     written G tags to memory.
> +      */
> +
> +      size_t g = aarch64_mte_get_tag_granules (addr, length,
> +					       AARCH64_MTE_GRANULE_SIZE);
> +      size_t n = tags.size ();
> +
> +      if (g < n)
> +	warning (_("Got more tags than memory granules.  Tags will be "
> +		   "truncated."));
> +      else if (g > n)
> +	warning (_("Using tag pattern to fill memory range."));
> +
> +      if (!target_store_memtags (addr, length, tags,
> +				 static_cast<int> (memtag_type::allocation)))
> +	return false;
> +    }
> +  return true;
> +}
> +
> +/* Implement the get_memtag gdbarch method.  */
> +
> +static struct value *
> +aarch64_get_memtag (struct gdbarch *gdbarch, struct value *address,
> +			  memtag_type tag_type)

Indentation is off above.

> +{
> +  gdb_assert (address != nullptr);
> +
> +  CORE_ADDR addr = value_as_address (address);
> +  CORE_ADDR tag = 0;
> +
> +  /* Get the logical tag or the allocation tag.  */
> +  if (tag_type == memtag_type::logical)
> +    tag = aarch64_mte_get_ltag (addr);
> +  else
> +    {
> +      /* Remove the top byte.  */
> +      addr = gdbarch_remove_non_address_bits (gdbarch, addr);
> +      std::optional<CORE_ADDR> atag = aarch64_mte_get_atag (addr);
> +
> +      if (!atag.has_value ())
> +	return nullptr;
> +
> +      tag = *atag;
> +    }
> +
> +  /* Convert the tag to a value.  */
> +  return value_from_ulongest (builtin_type (gdbarch)->builtin_unsigned_int,
> +			      tag);
> +}
> +
> +/* Implement the memtag_to_string gdbarch method.  */
> +
> +static std::string
> +aarch64_memtag_to_string (struct gdbarch *gdbarch, struct value *tag_value)
> +{
> +  if (tag_value == nullptr)
> +    return "";
> +
> +  CORE_ADDR tag = value_as_address (tag_value);
> +
> +  return string_printf ("0x%s", phex_nz (tag, sizeof (tag)));
> +}
> +
>  /* AArch64 implementation of the remove_non_address_bits gdbarch hook.  Remove
>     non address bits from a pointer value.  */
>  
> @@ -4504,6 +4655,23 @@ aarch64_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
>  					aarch64_pseudo_register_reggroup_p);
>    set_gdbarch_cannot_store_register (gdbarch, aarch64_cannot_store_register);
>  
> +  /* Set the allocation tag granule size to 16 bytes.  */
> +  set_gdbarch_memtag_granule_size (gdbarch, AARCH64_MTE_GRANULE_SIZE);
> +
> +  /* Register a hook for checking if there is a memory tag match.  */
> +  set_gdbarch_memtag_matches_p (gdbarch, aarch64_memtag_matches_p);
> +
> +  /* Register a hook for setting the logical/allocation tags for
> +     a range of addresses.  */
> +  set_gdbarch_set_memtags (gdbarch, aarch64_set_memtags);
> +
> +  /* Register a hook for extracting the logical/allocation tag from an
> +     address.  */
> +  set_gdbarch_get_memtag (gdbarch, aarch64_get_memtag);
> +
> +  /* Register a hook for converting a memory tag to a string.  */
> +  set_gdbarch_memtag_to_string (gdbarch, aarch64_memtag_to_string);
> +

These gdbarch hooks are being registered regardless of whether the target supports
MTE or not. In aarch64-linux-tdep.c they were registered if MTE support was confirmed
by the presence of the XML feature. I think this should be the same, even if we have
some safety checks prior to using these hooks.

>    /* ABI */
>    set_gdbarch_short_bit (gdbarch, 16);
>    set_gdbarch_int_bit (gdbarch, 32);
> diff --git a/gdb/aarch64-tdep.h b/gdb/aarch64-tdep.h
> index 0e6024bfcbc..0072834be9c 100644
> --- a/gdb/aarch64-tdep.h
> +++ b/gdb/aarch64-tdep.h
> @@ -203,4 +203,7 @@ void aarch64_displaced_step_fixup (struct gdbarch *gdbarch,
>  
>  bool aarch64_displaced_step_hw_singlestep (struct gdbarch *gdbarch);
>  
> +std::optional<CORE_ADDR>
> +aarch64_mte_get_atag (CORE_ADDR address);
> +

The above declaration should fit in one line.

>  #endif /* aarch64-tdep.h */

I'm running some tests on my end, but these are things I spotted while going through the
changes.
  

Hi Luis!

On 7/23/24 6:46 AM, Luis Machado wrote:
> Hi,
> 
> Thanks for the patch.

Thanks a lot for the review :)


> On 7/22/24 15:41, Gustavo Romero wrote:
>> This commit moves aarch64_linux_memtag_matches_p,
>> aarch64_linux_set_memtags, aarch64_linux_get_memtag, and
>> aarch64_linux_memtag_to_string hooks (plus aarch64_mte_get_atag
>> function used by them), along with the setting of the memtag granule
>> size, from aarch64-linux-tdep.c to aarch64-tdep.c, making MTE available
>> on baremetal targets. Since the aarch64-linux-tdep.c layer inherits
>> these hooks from aarch64-tdep.c, there is no effective change for
>> aarch64-linux targets.
>>
>> Signed-off-by: Gustavo Romero <gustavo.romero@linaro.org>
>> ---
>>   gdb/aarch64-linux-tdep.c | 167 --------------------------------------
>>   gdb/aarch64-tdep.c       | 168 +++++++++++++++++++++++++++++++++++++++
>>   gdb/aarch64-tdep.h       |   3 +
>>   3 files changed, 171 insertions(+), 167 deletions(-)
>>
>> diff --git a/gdb/aarch64-linux-tdep.c b/gdb/aarch64-linux-tdep.c
>> index a1296a8f0c7..63defd5a31f 100644
>> --- a/gdb/aarch64-linux-tdep.c
>> +++ b/gdb/aarch64-linux-tdep.c
>> @@ -2427,29 +2427,6 @@ aarch64_linux_gcc_target_options (struct gdbarch *gdbarch)
>>     return {};
>>   }
>>   
>> -/* Helper to get the allocation tag from a 64-bit ADDRESS.
>> -
>> -   Return the allocation tag if successful and nullopt otherwise.  */
>> -
>> -static std::optional<CORE_ADDR>
>> -aarch64_mte_get_atag (CORE_ADDR address)
>> -{
>> -  gdb::byte_vector tags;
>> -
>> -  /* Attempt to fetch the allocation tag.  */
>> -  if (!target_fetch_memtags (address, 1, tags,
>> -			     static_cast<int> (memtag_type::allocation)))
>> -    return {};
>> -
>> -  /* Only one tag should've been returned.  Make sure we got exactly that.  */
>> -  if (tags.size () != 1)
>> -    error (_("Target returned an unexpected number of tags."));
>> -
>> -  /* Although our tags are 4 bits in size, they are stored in a
>> -     byte.  */
>> -  return tags[0];
>> -}
>> -
>>   /* Implement the tagged_address_p gdbarch method.  */
>>   
>>   static bool
>> @@ -2466,132 +2443,6 @@ aarch64_linux_tagged_address_p (struct gdbarch *gdbarch, CORE_ADDR address)
>>     return true;
>>   }
>>   
>> -/* Implement the memtag_matches_p gdbarch method.  */
>> -
>> -static bool
>> -aarch64_linux_memtag_matches_p (struct gdbarch *gdbarch,
>> -				struct value *address)
>> -{
>> -  gdb_assert (address != nullptr);
>> -
>> -  CORE_ADDR addr = value_as_address (address);
>> -
>> -  /* Fetch the allocation tag for ADDRESS.  */
>> -  std::optional<CORE_ADDR> atag
>> -    = aarch64_mte_get_atag (gdbarch_remove_non_address_bits (gdbarch, addr));
>> -
>> -  if (!atag.has_value ())
>> -    return true;
>> -
>> -  /* Fetch the logical tag for ADDRESS.  */
>> -  gdb_byte ltag = aarch64_mte_get_ltag (addr);
>> -
>> -  /* Are the tags the same?  */
>> -  return ltag == *atag;
>> -}
>> -
>> -/* Implement the set_memtags gdbarch method.  */
>> -
>> -static bool
>> -aarch64_linux_set_memtags (struct gdbarch *gdbarch, struct value *address,
>> -			   size_t length, const gdb::byte_vector &tags,
>> -			   memtag_type tag_type)
>> -{
>> -  gdb_assert (!tags.empty ());
>> -  gdb_assert (address != nullptr);
>> -
>> -  CORE_ADDR addr = value_as_address (address);
>> -
>> -  /* Set the logical tag or the allocation tag.  */
>> -  if (tag_type == memtag_type::logical)
>> -    {
>> -      /* When setting logical tags, we don't care about the length, since
>> -	 we are only setting a single logical tag.  */
>> -      addr = aarch64_mte_set_ltag (addr, tags[0]);
>> -
>> -      /* Update the value's content with the tag.  */
>> -      enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
>> -      gdb_byte *srcbuf = address->contents_raw ().data ();
>> -      store_unsigned_integer (srcbuf, sizeof (addr), byte_order, addr);
>> -    }
>> -  else
>> -    {
>> -      /* Remove the top byte.  */
>> -      addr = gdbarch_remove_non_address_bits (gdbarch, addr);
>> -
>> -      /* With G being the number of tag granules and N the number of tags
>> -	 passed in, we can have the following cases:
>> -
>> -	 1 - G == N: Store all the N tags to memory.
>> -
>> -	 2 - G < N : Warn about having more tags than granules, but write G
>> -		     tags.
>> -
>> -	 3 - G > N : This is a "fill tags" operation.  We should use the tags
>> -		     as a pattern to fill the granules repeatedly until we have
>> -		     written G tags to memory.
>> -      */
>> -
>> -      size_t g = aarch64_mte_get_tag_granules (addr, length,
>> -					       AARCH64_MTE_GRANULE_SIZE);
>> -      size_t n = tags.size ();
>> -
>> -      if (g < n)
>> -	warning (_("Got more tags than memory granules.  Tags will be "
>> -		   "truncated."));
>> -      else if (g > n)
>> -	warning (_("Using tag pattern to fill memory range."));
>> -
>> -      if (!target_store_memtags (addr, length, tags,
>> -				 static_cast<int> (memtag_type::allocation)))
>> -	return false;
>> -    }
>> -  return true;
>> -}
>> -
>> -/* Implement the get_memtag gdbarch method.  */
>> -
>> -static struct value *
>> -aarch64_linux_get_memtag (struct gdbarch *gdbarch, struct value *address,
>> -			  memtag_type tag_type)
>> -{
>> -  gdb_assert (address != nullptr);
>> -
>> -  CORE_ADDR addr = value_as_address (address);
>> -  CORE_ADDR tag = 0;
>> -
>> -  /* Get the logical tag or the allocation tag.  */
>> -  if (tag_type == memtag_type::logical)
>> -    tag = aarch64_mte_get_ltag (addr);
>> -  else
>> -    {
>> -      /* Remove the top byte.  */
>> -      addr = gdbarch_remove_non_address_bits (gdbarch, addr);
>> -      std::optional<CORE_ADDR> atag = aarch64_mte_get_atag (addr);
>> -
>> -      if (!atag.has_value ())
>> -	return nullptr;
>> -
>> -      tag = *atag;
>> -    }
>> -
>> -  /* Convert the tag to a value.  */
>> -  return value_from_ulongest (builtin_type (gdbarch)->builtin_unsigned_int,
>> -			      tag);
>> -}
>> -
>> -/* Implement the memtag_to_string gdbarch method.  */
>> -
>> -static std::string
>> -aarch64_linux_memtag_to_string (struct gdbarch *gdbarch, struct value *tag_value)
>> -{
>> -  if (tag_value == nullptr)
>> -    return "";
>> -
>> -  CORE_ADDR tag = value_as_address (tag_value);
>> -
>> -  return string_printf ("0x%s", phex_nz (tag, sizeof (tag)));
>> -}
>>   
>>   /* AArch64 Linux implementation of the report_signal_info gdbarch
>>      hook.  Displays information about possible memory tag violations.  */
>> @@ -2900,24 +2751,6 @@ aarch64_linux_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
>>         /* Register a hook for checking if an address is tagged or not.  */
>>         set_gdbarch_tagged_address_p (gdbarch, aarch64_linux_tagged_address_p);
>>   
>> -      /* Register a hook for checking if there is a memory tag match.  */
>> -      set_gdbarch_memtag_matches_p (gdbarch,
>> -				    aarch64_linux_memtag_matches_p);
>> -
>> -      /* Register a hook for setting the logical/allocation tags for
>> -	 a range of addresses.  */
>> -      set_gdbarch_set_memtags (gdbarch, aarch64_linux_set_memtags);
>> -
>> -      /* Register a hook for extracting the logical/allocation tag from an
>> -	 address.  */
>> -      set_gdbarch_get_memtag (gdbarch, aarch64_linux_get_memtag);
>> -
>> -      /* Set the allocation tag granule size to 16 bytes.  */
>> -      set_gdbarch_memtag_granule_size (gdbarch, AARCH64_MTE_GRANULE_SIZE);
>> -
>> -      /* Register a hook for converting a memory tag to a string.  */
>> -      set_gdbarch_memtag_to_string (gdbarch, aarch64_linux_memtag_to_string);
>> -
>>         set_gdbarch_report_signal_info (gdbarch,
>>   				      aarch64_linux_report_signal_info);
>>   
>> diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
>> index e4bca6c6632..f6227a5b82d 100644
>> --- a/gdb/aarch64-tdep.c
>> +++ b/gdb/aarch64-tdep.c
>> @@ -45,6 +45,7 @@
>>   
>>   #include "aarch64-tdep.h"
>>   #include "aarch64-ravenscar-thread.h"
>> +#include "arch/aarch64-mte-linux.h"
> 
> Including a more specific scope into a generic one is incorrect. In this
> particular case we're including a *linux* header into a supposedly generic
> aarch64-tdep.c file.
> 
> If we need to use things contained in the Linux files, we should make sure they can be made
> generic and then extract those out from arch/aarch64-mte-linux.[c|h] and then move them
> to, say, arch/aarch64-mte.[c|h].
> 
>>   
>>   #include "record.h"
>>   #include "record-full.h"
>> @@ -4088,6 +4089,156 @@ aarch64_stack_frame_destroyed_p (struct gdbarch *gdbarch, CORE_ADDR pc)
>>     return streq (inst.opcode->name, "ret");
>>   }
>>   
>> +/* Helper to get the allocation tag from a 64-bit ADDRESS.
>> +
>> +   Return the allocation tag if successful and nullopt otherwise.  */
>> +
>> +std::optional<CORE_ADDR>
>> +aarch64_mte_get_atag (CORE_ADDR address)
>> +{
>> +  gdb::byte_vector tags;
>> +
>> +  /* Attempt to fetch the allocation tag.  */
>> +  if (!target_fetch_memtags (address, 1, tags,
>> +			     static_cast<int> (memtag_type::allocation)))
>> +    return {};
>> +
>> +  /* Only one tag should've been returned.  Make sure we got exactly that.  */
>> +  if (tags.size () != 1)
>> +    error (_("Target returned an unexpected number of tags."));
>> +
>> +  /* Although our tags are 4 bits in size, they are stored in a
>> +     byte.  */
>> +  return tags[0];
>> +}
>> +
>> +/* Implement the memtag_matches_p gdbarch method.  */
>> +
>> +static bool
>> +aarch64_memtag_matches_p (struct gdbarch *gdbarch,
>> +				struct value *address)
> 
> There seems to be something off with indentation above.
> 
>> +{
>> +  gdb_assert (address != nullptr);
>> +
>> +  CORE_ADDR addr = value_as_address (address);
>> +
>> +  /* Fetch the allocation tag for ADDRESS.  */
>> +  std::optional<CORE_ADDR> atag
>> +    = aarch64_mte_get_atag (gdbarch_remove_non_address_bits (gdbarch, addr));
>> +
>> +  if (!atag.has_value ())
>> +    return true;
>> +
>> +  /* Fetch the logical tag for ADDRESS.  */
>> +  gdb_byte ltag = aarch64_mte_get_ltag (addr);
>> +
>> +  /* Are the tags the same?  */
>> +  return ltag == *atag;
>> +}
>> +
>> +/* Implement the set_memtags gdbarch method.  */
>> +
>> +static bool
>> +aarch64_set_memtags (struct gdbarch *gdbarch, struct value *address,
>> +			   size_t length, const gdb::byte_vector &tags,
>> +			   memtag_type tag_type)
> 
> Indentation above looks off too.
> 
>> +{
>> +  gdb_assert (!tags.empty ());
>> +  gdb_assert (address != nullptr);
>> +
>> +  CORE_ADDR addr = value_as_address (address);
>> +
>> +  /* Set the logical tag or the allocation tag.  */
>> +  if (tag_type == memtag_type::logical)
>> +    {
>> +      /* When setting logical tags, we don't care about the length, since
>> +	 we are only setting a single logical tag.  */
>> +      addr = aarch64_mte_set_ltag (addr, tags[0]);
>> +
>> +      /* Update the value's content with the tag.  */
>> +      enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
>> +      gdb_byte *srcbuf = address->contents_raw ().data ();
>> +      store_unsigned_integer (srcbuf, sizeof (addr), byte_order, addr);
>> +    }
>> +  else
>> +    {
>> +      /* Remove the top byte.  */
>> +      addr = gdbarch_remove_non_address_bits (gdbarch, addr);
>> +
>> +      /* With G being the number of tag granules and N the number of tags
>> +	 passed in, we can have the following cases:
>> +
>> +	 1 - G == N: Store all the N tags to memory.
>> +
>> +	 2 - G < N : Warn about having more tags than granules, but write G
>> +		     tags.
>> +
>> +	 3 - G > N : This is a "fill tags" operation.  We should use the tags
>> +		     as a pattern to fill the granules repeatedly until we have
>> +		     written G tags to memory.
>> +      */
>> +
>> +      size_t g = aarch64_mte_get_tag_granules (addr, length,
>> +					       AARCH64_MTE_GRANULE_SIZE);
>> +      size_t n = tags.size ();
>> +
>> +      if (g < n)
>> +	warning (_("Got more tags than memory granules.  Tags will be "
>> +		   "truncated."));
>> +      else if (g > n)
>> +	warning (_("Using tag pattern to fill memory range."));
>> +
>> +      if (!target_store_memtags (addr, length, tags,
>> +				 static_cast<int> (memtag_type::allocation)))
>> +	return false;
>> +    }
>> +  return true;
>> +}
>> +
>> +/* Implement the get_memtag gdbarch method.  */
>> +
>> +static struct value *
>> +aarch64_get_memtag (struct gdbarch *gdbarch, struct value *address,
>> +			  memtag_type tag_type)
> 
> Indentation is off above.
> 
>> +{
>> +  gdb_assert (address != nullptr);
>> +
>> +  CORE_ADDR addr = value_as_address (address);
>> +  CORE_ADDR tag = 0;
>> +
>> +  /* Get the logical tag or the allocation tag.  */
>> +  if (tag_type == memtag_type::logical)
>> +    tag = aarch64_mte_get_ltag (addr);
>> +  else
>> +    {
>> +      /* Remove the top byte.  */
>> +      addr = gdbarch_remove_non_address_bits (gdbarch, addr);
>> +      std::optional<CORE_ADDR> atag = aarch64_mte_get_atag (addr);
>> +
>> +      if (!atag.has_value ())
>> +	return nullptr;
>> +
>> +      tag = *atag;
>> +    }
>> +
>> +  /* Convert the tag to a value.  */
>> +  return value_from_ulongest (builtin_type (gdbarch)->builtin_unsigned_int,
>> +			      tag);
>> +}
>> +
>> +/* Implement the memtag_to_string gdbarch method.  */
>> +
>> +static std::string
>> +aarch64_memtag_to_string (struct gdbarch *gdbarch, struct value *tag_value)
>> +{
>> +  if (tag_value == nullptr)
>> +    return "";
>> +
>> +  CORE_ADDR tag = value_as_address (tag_value);
>> +
>> +  return string_printf ("0x%s", phex_nz (tag, sizeof (tag)));
>> +}
>> +
>>   /* AArch64 implementation of the remove_non_address_bits gdbarch hook.  Remove
>>      non address bits from a pointer value.  */
>>   
>> @@ -4504,6 +4655,23 @@ aarch64_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
>>   					aarch64_pseudo_register_reggroup_p);
>>     set_gdbarch_cannot_store_register (gdbarch, aarch64_cannot_store_register);
>>   
>> +  /* Set the allocation tag granule size to 16 bytes.  */
>> +  set_gdbarch_memtag_granule_size (gdbarch, AARCH64_MTE_GRANULE_SIZE);
>> +
>> +  /* Register a hook for checking if there is a memory tag match.  */
>> +  set_gdbarch_memtag_matches_p (gdbarch, aarch64_memtag_matches_p);
>> +
>> +  /* Register a hook for setting the logical/allocation tags for
>> +     a range of addresses.  */
>> +  set_gdbarch_set_memtags (gdbarch, aarch64_set_memtags);
>> +
>> +  /* Register a hook for extracting the logical/allocation tag from an
>> +     address.  */
>> +  set_gdbarch_get_memtag (gdbarch, aarch64_get_memtag);
>> +
>> +  /* Register a hook for converting a memory tag to a string.  */
>> +  set_gdbarch_memtag_to_string (gdbarch, aarch64_memtag_to_string);
>> +
> 
> These gdbarch hooks are being registered regardless of whether the target supports
> MTE or not. In aarch64-linux-tdep.c they were registered if MTE support was confirmed
> by the presence of the XML feature. I think this should be the same, even if we have
> some safety checks prior to using these hooks.

tdep->has_mte() does not work in aarch64-tdep.c because the XML advertises the 'tag_ctl'
pseudo-register, meant to change the Tag Check Fault behavior at runtime. It doesn't
make sense to have it in baremetal, since it's not a real register and was introduced
to emulate Linux's prctl() PR_SET_TAGGED_ADDR_CTRL option. In that sense, QEMU system
mode doesn't advertise it for this reason either.

As you said, all memory-tag subcommands check if MTE is supported by the target calling
target_supports_memory_tagging() and so all goes well in aarch64-tdep.c

I don't have any idea on how to address what you're asking for. Actually, I was inclined
to remove the if (tdep->has_mte()) check in aarch64-linux-tdep.c when registering the
hooks. I don't see any harm on always registering them and with has_mte() in place we
are kind duplicating the check: one check is done via XML advertisement of 'tag_ctl' and
the other check (in the hooks) uses "memory-tagging+" feature in qSupported.


>>     /* ABI */
>>     set_gdbarch_short_bit (gdbarch, 16);
>>     set_gdbarch_int_bit (gdbarch, 32);
>> diff --git a/gdb/aarch64-tdep.h b/gdb/aarch64-tdep.h
>> index 0e6024bfcbc..0072834be9c 100644
>> --- a/gdb/aarch64-tdep.h
>> +++ b/gdb/aarch64-tdep.h
>> @@ -203,4 +203,7 @@ void aarch64_displaced_step_fixup (struct gdbarch *gdbarch,
>>   
>>   bool aarch64_displaced_step_hw_singlestep (struct gdbarch *gdbarch);
>>   
>> +std::optional<CORE_ADDR>
>> +aarch64_mte_get_atag (CORE_ADDR address);
>> +
> 
> The above declaration should fit in one line.
> 
>>   #endif /* aarch64-tdep.h */
> 
> I'm running some tests on my end, but these are things I spotted while going through the
> changes.

Thanks! I've addressed all your comments in v2, except the has_mte() check, since we are
still discussing it.


Cheers,
Gustavo
  

On 7/24/24 01:45, Gustavo Romero wrote:
> Hi Luis!
> 
> On 7/23/24 6:46 AM, Luis Machado wrote:
>> Hi,
>>
>> Thanks for the patch.
> 
> Thanks a lot for the review :)
> 
> 
>> On 7/22/24 15:41, Gustavo Romero wrote:
>>> This commit moves aarch64_linux_memtag_matches_p,
>>> aarch64_linux_set_memtags, aarch64_linux_get_memtag, and
>>> aarch64_linux_memtag_to_string hooks (plus aarch64_mte_get_atag
>>> function used by them), along with the setting of the memtag granule
>>> size, from aarch64-linux-tdep.c to aarch64-tdep.c, making MTE available
>>> on baremetal targets. Since the aarch64-linux-tdep.c layer inherits
>>> these hooks from aarch64-tdep.c, there is no effective change for
>>> aarch64-linux targets.
>>>
>>> Signed-off-by: Gustavo Romero <gustavo.romero@linaro.org>
>>> ---
>>>   gdb/aarch64-linux-tdep.c | 167 --------------------------------------
>>>   gdb/aarch64-tdep.c       | 168 +++++++++++++++++++++++++++++++++++++++
>>>   gdb/aarch64-tdep.h       |   3 +
>>>   3 files changed, 171 insertions(+), 167 deletions(-)
>>>
>>> diff --git a/gdb/aarch64-linux-tdep.c b/gdb/aarch64-linux-tdep.c
>>> index a1296a8f0c7..63defd5a31f 100644
>>> --- a/gdb/aarch64-linux-tdep.c
>>> +++ b/gdb/aarch64-linux-tdep.c
>>> @@ -2427,29 +2427,6 @@ aarch64_linux_gcc_target_options (struct gdbarch *gdbarch)
>>>     return {};
>>>   }
>>>   -/* Helper to get the allocation tag from a 64-bit ADDRESS.
>>> -
>>> -   Return the allocation tag if successful and nullopt otherwise.  */
>>> -
>>> -static std::optional<CORE_ADDR>
>>> -aarch64_mte_get_atag (CORE_ADDR address)
>>> -{
>>> -  gdb::byte_vector tags;
>>> -
>>> -  /* Attempt to fetch the allocation tag.  */
>>> -  if (!target_fetch_memtags (address, 1, tags,
>>> -                 static_cast<int> (memtag_type::allocation)))
>>> -    return {};
>>> -
>>> -  /* Only one tag should've been returned.  Make sure we got exactly that.  */
>>> -  if (tags.size () != 1)
>>> -    error (_("Target returned an unexpected number of tags."));
>>> -
>>> -  /* Although our tags are 4 bits in size, they are stored in a
>>> -     byte.  */
>>> -  return tags[0];
>>> -}
>>> -
>>>   /* Implement the tagged_address_p gdbarch method.  */
>>>     static bool
>>> @@ -2466,132 +2443,6 @@ aarch64_linux_tagged_address_p (struct gdbarch *gdbarch, CORE_ADDR address)
>>>     return true;
>>>   }
>>>   -/* Implement the memtag_matches_p gdbarch method.  */
>>> -
>>> -static bool
>>> -aarch64_linux_memtag_matches_p (struct gdbarch *gdbarch,
>>> -                struct value *address)
>>> -{
>>> -  gdb_assert (address != nullptr);
>>> -
>>> -  CORE_ADDR addr = value_as_address (address);
>>> -
>>> -  /* Fetch the allocation tag for ADDRESS.  */
>>> -  std::optional<CORE_ADDR> atag
>>> -    = aarch64_mte_get_atag (gdbarch_remove_non_address_bits (gdbarch, addr));
>>> -
>>> -  if (!atag.has_value ())
>>> -    return true;
>>> -
>>> -  /* Fetch the logical tag for ADDRESS.  */
>>> -  gdb_byte ltag = aarch64_mte_get_ltag (addr);
>>> -
>>> -  /* Are the tags the same?  */
>>> -  return ltag == *atag;
>>> -}
>>> -
>>> -/* Implement the set_memtags gdbarch method.  */
>>> -
>>> -static bool
>>> -aarch64_linux_set_memtags (struct gdbarch *gdbarch, struct value *address,
>>> -               size_t length, const gdb::byte_vector &tags,
>>> -               memtag_type tag_type)
>>> -{
>>> -  gdb_assert (!tags.empty ());
>>> -  gdb_assert (address != nullptr);
>>> -
>>> -  CORE_ADDR addr = value_as_address (address);
>>> -
>>> -  /* Set the logical tag or the allocation tag.  */
>>> -  if (tag_type == memtag_type::logical)
>>> -    {
>>> -      /* When setting logical tags, we don't care about the length, since
>>> -     we are only setting a single logical tag.  */
>>> -      addr = aarch64_mte_set_ltag (addr, tags[0]);
>>> -
>>> -      /* Update the value's content with the tag.  */
>>> -      enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
>>> -      gdb_byte *srcbuf = address->contents_raw ().data ();
>>> -      store_unsigned_integer (srcbuf, sizeof (addr), byte_order, addr);
>>> -    }
>>> -  else
>>> -    {
>>> -      /* Remove the top byte.  */
>>> -      addr = gdbarch_remove_non_address_bits (gdbarch, addr);
>>> -
>>> -      /* With G being the number of tag granules and N the number of tags
>>> -     passed in, we can have the following cases:
>>> -
>>> -     1 - G == N: Store all the N tags to memory.
>>> -
>>> -     2 - G < N : Warn about having more tags than granules, but write G
>>> -             tags.
>>> -
>>> -     3 - G > N : This is a "fill tags" operation.  We should use the tags
>>> -             as a pattern to fill the granules repeatedly until we have
>>> -             written G tags to memory.
>>> -      */
>>> -
>>> -      size_t g = aarch64_mte_get_tag_granules (addr, length,
>>> -                           AARCH64_MTE_GRANULE_SIZE);
>>> -      size_t n = tags.size ();
>>> -
>>> -      if (g < n)
>>> -    warning (_("Got more tags than memory granules.  Tags will be "
>>> -           "truncated."));
>>> -      else if (g > n)
>>> -    warning (_("Using tag pattern to fill memory range."));
>>> -
>>> -      if (!target_store_memtags (addr, length, tags,
>>> -                 static_cast<int> (memtag_type::allocation)))
>>> -    return false;
>>> -    }
>>> -  return true;
>>> -}
>>> -
>>> -/* Implement the get_memtag gdbarch method.  */
>>> -
>>> -static struct value *
>>> -aarch64_linux_get_memtag (struct gdbarch *gdbarch, struct value *address,
>>> -              memtag_type tag_type)
>>> -{
>>> -  gdb_assert (address != nullptr);
>>> -
>>> -  CORE_ADDR addr = value_as_address (address);
>>> -  CORE_ADDR tag = 0;
>>> -
>>> -  /* Get the logical tag or the allocation tag.  */
>>> -  if (tag_type == memtag_type::logical)
>>> -    tag = aarch64_mte_get_ltag (addr);
>>> -  else
>>> -    {
>>> -      /* Remove the top byte.  */
>>> -      addr = gdbarch_remove_non_address_bits (gdbarch, addr);
>>> -      std::optional<CORE_ADDR> atag = aarch64_mte_get_atag (addr);
>>> -
>>> -      if (!atag.has_value ())
>>> -    return nullptr;
>>> -
>>> -      tag = *atag;
>>> -    }
>>> -
>>> -  /* Convert the tag to a value.  */
>>> -  return value_from_ulongest (builtin_type (gdbarch)->builtin_unsigned_int,
>>> -                  tag);
>>> -}
>>> -
>>> -/* Implement the memtag_to_string gdbarch method.  */
>>> -
>>> -static std::string
>>> -aarch64_linux_memtag_to_string (struct gdbarch *gdbarch, struct value *tag_value)
>>> -{
>>> -  if (tag_value == nullptr)
>>> -    return "";
>>> -
>>> -  CORE_ADDR tag = value_as_address (tag_value);
>>> -
>>> -  return string_printf ("0x%s", phex_nz (tag, sizeof (tag)));
>>> -}
>>>     /* AArch64 Linux implementation of the report_signal_info gdbarch
>>>      hook.  Displays information about possible memory tag violations.  */
>>> @@ -2900,24 +2751,6 @@ aarch64_linux_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
>>>         /* Register a hook for checking if an address is tagged or not.  */
>>>         set_gdbarch_tagged_address_p (gdbarch, aarch64_linux_tagged_address_p);
>>>   -      /* Register a hook for checking if there is a memory tag match.  */
>>> -      set_gdbarch_memtag_matches_p (gdbarch,
>>> -                    aarch64_linux_memtag_matches_p);
>>> -
>>> -      /* Register a hook for setting the logical/allocation tags for
>>> -     a range of addresses.  */
>>> -      set_gdbarch_set_memtags (gdbarch, aarch64_linux_set_memtags);
>>> -
>>> -      /* Register a hook for extracting the logical/allocation tag from an
>>> -     address.  */
>>> -      set_gdbarch_get_memtag (gdbarch, aarch64_linux_get_memtag);
>>> -
>>> -      /* Set the allocation tag granule size to 16 bytes.  */
>>> -      set_gdbarch_memtag_granule_size (gdbarch, AARCH64_MTE_GRANULE_SIZE);
>>> -
>>> -      /* Register a hook for converting a memory tag to a string.  */
>>> -      set_gdbarch_memtag_to_string (gdbarch, aarch64_linux_memtag_to_string);
>>> -
>>>         set_gdbarch_report_signal_info (gdbarch,
>>>                         aarch64_linux_report_signal_info);
>>>   diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
>>> index e4bca6c6632..f6227a5b82d 100644
>>> --- a/gdb/aarch64-tdep.c
>>> +++ b/gdb/aarch64-tdep.c
>>> @@ -45,6 +45,7 @@
>>>     #include "aarch64-tdep.h"
>>>   #include "aarch64-ravenscar-thread.h"
>>> +#include "arch/aarch64-mte-linux.h"
>>
>> Including a more specific scope into a generic one is incorrect. In this
>> particular case we're including a *linux* header into a supposedly generic
>> aarch64-tdep.c file.
>>
>> If we need to use things contained in the Linux files, we should make sure they can be made
>> generic and then extract those out from arch/aarch64-mte-linux.[c|h] and then move them
>> to, say, arch/aarch64-mte.[c|h].
>>
>>>     #include "record.h"
>>>   #include "record-full.h"
>>> @@ -4088,6 +4089,156 @@ aarch64_stack_frame_destroyed_p (struct gdbarch *gdbarch, CORE_ADDR pc)
>>>     return streq (inst.opcode->name, "ret");
>>>   }
>>>   +/* Helper to get the allocation tag from a 64-bit ADDRESS.
>>> +
>>> +   Return the allocation tag if successful and nullopt otherwise.  */
>>> +
>>> +std::optional<CORE_ADDR>
>>> +aarch64_mte_get_atag (CORE_ADDR address)
>>> +{
>>> +  gdb::byte_vector tags;
>>> +
>>> +  /* Attempt to fetch the allocation tag.  */
>>> +  if (!target_fetch_memtags (address, 1, tags,
>>> +                 static_cast<int> (memtag_type::allocation)))
>>> +    return {};
>>> +
>>> +  /* Only one tag should've been returned.  Make sure we got exactly that.  */
>>> +  if (tags.size () != 1)
>>> +    error (_("Target returned an unexpected number of tags."));
>>> +
>>> +  /* Although our tags are 4 bits in size, they are stored in a
>>> +     byte.  */
>>> +  return tags[0];
>>> +}
>>> +
>>> +/* Implement the memtag_matches_p gdbarch method.  */
>>> +
>>> +static bool
>>> +aarch64_memtag_matches_p (struct gdbarch *gdbarch,
>>> +                struct value *address)
>>
>> There seems to be something off with indentation above.
>>
>>> +{
>>> +  gdb_assert (address != nullptr);
>>> +
>>> +  CORE_ADDR addr = value_as_address (address);
>>> +
>>> +  /* Fetch the allocation tag for ADDRESS.  */
>>> +  std::optional<CORE_ADDR> atag
>>> +    = aarch64_mte_get_atag (gdbarch_remove_non_address_bits (gdbarch, addr));
>>> +
>>> +  if (!atag.has_value ())
>>> +    return true;
>>> +
>>> +  /* Fetch the logical tag for ADDRESS.  */
>>> +  gdb_byte ltag = aarch64_mte_get_ltag (addr);
>>> +
>>> +  /* Are the tags the same?  */
>>> +  return ltag == *atag;
>>> +}
>>> +
>>> +/* Implement the set_memtags gdbarch method.  */
>>> +
>>> +static bool
>>> +aarch64_set_memtags (struct gdbarch *gdbarch, struct value *address,
>>> +               size_t length, const gdb::byte_vector &tags,
>>> +               memtag_type tag_type)
>>
>> Indentation above looks off too.
>>
>>> +{
>>> +  gdb_assert (!tags.empty ());
>>> +  gdb_assert (address != nullptr);
>>> +
>>> +  CORE_ADDR addr = value_as_address (address);
>>> +
>>> +  /* Set the logical tag or the allocation tag.  */
>>> +  if (tag_type == memtag_type::logical)
>>> +    {
>>> +      /* When setting logical tags, we don't care about the length, since
>>> +     we are only setting a single logical tag.  */
>>> +      addr = aarch64_mte_set_ltag (addr, tags[0]);
>>> +
>>> +      /* Update the value's content with the tag.  */
>>> +      enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
>>> +      gdb_byte *srcbuf = address->contents_raw ().data ();
>>> +      store_unsigned_integer (srcbuf, sizeof (addr), byte_order, addr);
>>> +    }
>>> +  else
>>> +    {
>>> +      /* Remove the top byte.  */
>>> +      addr = gdbarch_remove_non_address_bits (gdbarch, addr);
>>> +
>>> +      /* With G being the number of tag granules and N the number of tags
>>> +     passed in, we can have the following cases:
>>> +
>>> +     1 - G == N: Store all the N tags to memory.
>>> +
>>> +     2 - G < N : Warn about having more tags than granules, but write G
>>> +             tags.
>>> +
>>> +     3 - G > N : This is a "fill tags" operation.  We should use the tags
>>> +             as a pattern to fill the granules repeatedly until we have
>>> +             written G tags to memory.
>>> +      */
>>> +
>>> +      size_t g = aarch64_mte_get_tag_granules (addr, length,
>>> +                           AARCH64_MTE_GRANULE_SIZE);
>>> +      size_t n = tags.size ();
>>> +
>>> +      if (g < n)
>>> +    warning (_("Got more tags than memory granules.  Tags will be "
>>> +           "truncated."));
>>> +      else if (g > n)
>>> +    warning (_("Using tag pattern to fill memory range."));
>>> +
>>> +      if (!target_store_memtags (addr, length, tags,
>>> +                 static_cast<int> (memtag_type::allocation)))
>>> +    return false;
>>> +    }
>>> +  return true;
>>> +}
>>> +
>>> +/* Implement the get_memtag gdbarch method.  */
>>> +
>>> +static struct value *
>>> +aarch64_get_memtag (struct gdbarch *gdbarch, struct value *address,
>>> +              memtag_type tag_type)
>>
>> Indentation is off above.
>>
>>> +{
>>> +  gdb_assert (address != nullptr);
>>> +
>>> +  CORE_ADDR addr = value_as_address (address);
>>> +  CORE_ADDR tag = 0;
>>> +
>>> +  /* Get the logical tag or the allocation tag.  */
>>> +  if (tag_type == memtag_type::logical)
>>> +    tag = aarch64_mte_get_ltag (addr);
>>> +  else
>>> +    {
>>> +      /* Remove the top byte.  */
>>> +      addr = gdbarch_remove_non_address_bits (gdbarch, addr);
>>> +      std::optional<CORE_ADDR> atag = aarch64_mte_get_atag (addr);
>>> +
>>> +      if (!atag.has_value ())
>>> +    return nullptr;
>>> +
>>> +      tag = *atag;
>>> +    }
>>> +
>>> +  /* Convert the tag to a value.  */
>>> +  return value_from_ulongest (builtin_type (gdbarch)->builtin_unsigned_int,
>>> +                  tag);
>>> +}
>>> +
>>> +/* Implement the memtag_to_string gdbarch method.  */
>>> +
>>> +static std::string
>>> +aarch64_memtag_to_string (struct gdbarch *gdbarch, struct value *tag_value)
>>> +{
>>> +  if (tag_value == nullptr)
>>> +    return "";
>>> +
>>> +  CORE_ADDR tag = value_as_address (tag_value);
>>> +
>>> +  return string_printf ("0x%s", phex_nz (tag, sizeof (tag)));
>>> +}
>>> +
>>>   /* AArch64 implementation of the remove_non_address_bits gdbarch hook.  Remove
>>>      non address bits from a pointer value.  */
>>>   @@ -4504,6 +4655,23 @@ aarch64_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
>>>                       aarch64_pseudo_register_reggroup_p);
>>>     set_gdbarch_cannot_store_register (gdbarch, aarch64_cannot_store_register);
>>>   +  /* Set the allocation tag granule size to 16 bytes.  */
>>> +  set_gdbarch_memtag_granule_size (gdbarch, AARCH64_MTE_GRANULE_SIZE);
>>> +
>>> +  /* Register a hook for checking if there is a memory tag match.  */
>>> +  set_gdbarch_memtag_matches_p (gdbarch, aarch64_memtag_matches_p);
>>> +
>>> +  /* Register a hook for setting the logical/allocation tags for
>>> +     a range of addresses.  */
>>> +  set_gdbarch_set_memtags (gdbarch, aarch64_set_memtags);
>>> +
>>> +  /* Register a hook for extracting the logical/allocation tag from an
>>> +     address.  */
>>> +  set_gdbarch_get_memtag (gdbarch, aarch64_get_memtag);
>>> +
>>> +  /* Register a hook for converting a memory tag to a string.  */
>>> +  set_gdbarch_memtag_to_string (gdbarch, aarch64_memtag_to_string);
>>> +
>>
>> These gdbarch hooks are being registered regardless of whether the target supports
>> MTE or not. In aarch64-linux-tdep.c they were registered if MTE support was confirmed
>> by the presence of the XML feature. I think this should be the same, even if we have
>> some safety checks prior to using these hooks.
> 
> tdep->has_mte() does not work in aarch64-tdep.c because the XML advertises the 'tag_ctl'
> pseudo-register, meant to change the Tag Check Fault behavior at runtime. It doesn't
> make sense to have it in baremetal, since it's not a real register and was introduced
> to emulate Linux's prctl() PR_SET_TAGGED_ADDR_CTRL option. In that sense, QEMU system
> mode doesn't advertise it for this reason either.

Yeah, since the feature was initially designed with Linux in mind, we have the tag_ctl
register and its associated register set that we need to read from/dump to core files.

With the feature being supported for baremetal now, that might need to become a Linux-specific
thing. Which, incidentally, as I mentioned above about the header file, is a bit of an intrusion
of Linux-specific knowledge on an otherwise generic aarch64-tdep.c.

> 
> As you said, all memory-tag subcommands check if MTE is supported by the target calling
> target_supports_memory_tagging() and so all goes well in aarch64-tdep.c
> 
> I don't have any idea on how to address what you're asking for. Actually, I was inclined
> to remove the if (tdep->has_mte()) check in aarch64-linux-tdep.c when registering the
> hooks. I don't see any harm on always registering them and with has_mte() in place we
> are kind duplicating the check: one check is done via XML advertisement of 'tag_ctl' and
> the other check (in the hooks) uses "memory-tagging+" feature in qSupported.

It might be possible to do the same check with the target method I suppose?

The remote target would advertise memory-tagging+, the native and linux layers would
advertise the presence of HWCAP2_MTE. The question is if the target is properly setup at
the point where we are checking these XML features. We'd need to check.

> 
> 
>>>     /* ABI */
>>>     set_gdbarch_short_bit (gdbarch, 16);
>>>     set_gdbarch_int_bit (gdbarch, 32);
>>> diff --git a/gdb/aarch64-tdep.h b/gdb/aarch64-tdep.h
>>> index 0e6024bfcbc..0072834be9c 100644
>>> --- a/gdb/aarch64-tdep.h
>>> +++ b/gdb/aarch64-tdep.h
>>> @@ -203,4 +203,7 @@ void aarch64_displaced_step_fixup (struct gdbarch *gdbarch,
>>>     bool aarch64_displaced_step_hw_singlestep (struct gdbarch *gdbarch);
>>>   +std::optional<CORE_ADDR>
>>> +aarch64_mte_get_atag (CORE_ADDR address);
>>> +
>>
>> The above declaration should fit in one line.
>>
>>>   #endif /* aarch64-tdep.h */
>>
>> I'm running some tests on my end, but these are things I spotted while going through the
>> changes.
> 
> Thanks! I've addressed all your comments in v2, except the has_mte() check, since we are
> still discussing it.
> 
> 
> Cheers,
> Gustavo
  

Hi Luis,

On 7/24/24 6:17 AM, Luis Machado wrote:
> On 7/24/24 01:45, Gustavo Romero wrote:
>> Hi Luis!
>>
>> On 7/23/24 6:46 AM, Luis Machado wrote:
>>> Hi,
>>>
>>> Thanks for the patch.
>>
>> Thanks a lot for the review :)
>>
>>
>>> On 7/22/24 15:41, Gustavo Romero wrote:
>>>> This commit moves aarch64_linux_memtag_matches_p,
>>>> aarch64_linux_set_memtags, aarch64_linux_get_memtag, and
>>>> aarch64_linux_memtag_to_string hooks (plus aarch64_mte_get_atag
>>>> function used by them), along with the setting of the memtag granule
>>>> size, from aarch64-linux-tdep.c to aarch64-tdep.c, making MTE available
>>>> on baremetal targets. Since the aarch64-linux-tdep.c layer inherits
>>>> these hooks from aarch64-tdep.c, there is no effective change for
>>>> aarch64-linux targets.
>>>>
>>>> Signed-off-by: Gustavo Romero <gustavo.romero@linaro.org>
>>>> ---
>>>>    gdb/aarch64-linux-tdep.c | 167 --------------------------------------
>>>>    gdb/aarch64-tdep.c       | 168 +++++++++++++++++++++++++++++++++++++++
>>>>    gdb/aarch64-tdep.h       |   3 +
>>>>    3 files changed, 171 insertions(+), 167 deletions(-)
>>>>
>>>> diff --git a/gdb/aarch64-linux-tdep.c b/gdb/aarch64-linux-tdep.c
>>>> index a1296a8f0c7..63defd5a31f 100644
>>>> --- a/gdb/aarch64-linux-tdep.c
>>>> +++ b/gdb/aarch64-linux-tdep.c
>>>> @@ -2427,29 +2427,6 @@ aarch64_linux_gcc_target_options (struct gdbarch *gdbarch)
>>>>      return {};
>>>>    }
>>>>    -/* Helper to get the allocation tag from a 64-bit ADDRESS.
>>>> -
>>>> -   Return the allocation tag if successful and nullopt otherwise.  */
>>>> -
>>>> -static std::optional<CORE_ADDR>
>>>> -aarch64_mte_get_atag (CORE_ADDR address)
>>>> -{
>>>> -  gdb::byte_vector tags;
>>>> -
>>>> -  /* Attempt to fetch the allocation tag.  */
>>>> -  if (!target_fetch_memtags (address, 1, tags,
>>>> -                 static_cast<int> (memtag_type::allocation)))
>>>> -    return {};
>>>> -
>>>> -  /* Only one tag should've been returned.  Make sure we got exactly that.  */
>>>> -  if (tags.size () != 1)
>>>> -    error (_("Target returned an unexpected number of tags."));
>>>> -
>>>> -  /* Although our tags are 4 bits in size, they are stored in a
>>>> -     byte.  */
>>>> -  return tags[0];
>>>> -}
>>>> -
>>>>    /* Implement the tagged_address_p gdbarch method.  */
>>>>      static bool
>>>> @@ -2466,132 +2443,6 @@ aarch64_linux_tagged_address_p (struct gdbarch *gdbarch, CORE_ADDR address)
>>>>      return true;
>>>>    }
>>>>    -/* Implement the memtag_matches_p gdbarch method.  */
>>>> -
>>>> -static bool
>>>> -aarch64_linux_memtag_matches_p (struct gdbarch *gdbarch,
>>>> -                struct value *address)
>>>> -{
>>>> -  gdb_assert (address != nullptr);
>>>> -
>>>> -  CORE_ADDR addr = value_as_address (address);
>>>> -
>>>> -  /* Fetch the allocation tag for ADDRESS.  */
>>>> -  std::optional<CORE_ADDR> atag
>>>> -    = aarch64_mte_get_atag (gdbarch_remove_non_address_bits (gdbarch, addr));
>>>> -
>>>> -  if (!atag.has_value ())
>>>> -    return true;
>>>> -
>>>> -  /* Fetch the logical tag for ADDRESS.  */
>>>> -  gdb_byte ltag = aarch64_mte_get_ltag (addr);
>>>> -
>>>> -  /* Are the tags the same?  */
>>>> -  return ltag == *atag;
>>>> -}
>>>> -
>>>> -/* Implement the set_memtags gdbarch method.  */
>>>> -
>>>> -static bool
>>>> -aarch64_linux_set_memtags (struct gdbarch *gdbarch, struct value *address,
>>>> -               size_t length, const gdb::byte_vector &tags,
>>>> -               memtag_type tag_type)
>>>> -{
>>>> -  gdb_assert (!tags.empty ());
>>>> -  gdb_assert (address != nullptr);
>>>> -
>>>> -  CORE_ADDR addr = value_as_address (address);
>>>> -
>>>> -  /* Set the logical tag or the allocation tag.  */
>>>> -  if (tag_type == memtag_type::logical)
>>>> -    {
>>>> -      /* When setting logical tags, we don't care about the length, since
>>>> -     we are only setting a single logical tag.  */
>>>> -      addr = aarch64_mte_set_ltag (addr, tags[0]);
>>>> -
>>>> -      /* Update the value's content with the tag.  */
>>>> -      enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
>>>> -      gdb_byte *srcbuf = address->contents_raw ().data ();
>>>> -      store_unsigned_integer (srcbuf, sizeof (addr), byte_order, addr);
>>>> -    }
>>>> -  else
>>>> -    {
>>>> -      /* Remove the top byte.  */
>>>> -      addr = gdbarch_remove_non_address_bits (gdbarch, addr);
>>>> -
>>>> -      /* With G being the number of tag granules and N the number of tags
>>>> -     passed in, we can have the following cases:
>>>> -
>>>> -     1 - G == N: Store all the N tags to memory.
>>>> -
>>>> -     2 - G < N : Warn about having more tags than granules, but write G
>>>> -             tags.
>>>> -
>>>> -     3 - G > N : This is a "fill tags" operation.  We should use the tags
>>>> -             as a pattern to fill the granules repeatedly until we have
>>>> -             written G tags to memory.
>>>> -      */
>>>> -
>>>> -      size_t g = aarch64_mte_get_tag_granules (addr, length,
>>>> -                           AARCH64_MTE_GRANULE_SIZE);
>>>> -      size_t n = tags.size ();
>>>> -
>>>> -      if (g < n)
>>>> -    warning (_("Got more tags than memory granules.  Tags will be "
>>>> -           "truncated."));
>>>> -      else if (g > n)
>>>> -    warning (_("Using tag pattern to fill memory range."));
>>>> -
>>>> -      if (!target_store_memtags (addr, length, tags,
>>>> -                 static_cast<int> (memtag_type::allocation)))
>>>> -    return false;
>>>> -    }
>>>> -  return true;
>>>> -}
>>>> -
>>>> -/* Implement the get_memtag gdbarch method.  */
>>>> -
>>>> -static struct value *
>>>> -aarch64_linux_get_memtag (struct gdbarch *gdbarch, struct value *address,
>>>> -              memtag_type tag_type)
>>>> -{
>>>> -  gdb_assert (address != nullptr);
>>>> -
>>>> -  CORE_ADDR addr = value_as_address (address);
>>>> -  CORE_ADDR tag = 0;
>>>> -
>>>> -  /* Get the logical tag or the allocation tag.  */
>>>> -  if (tag_type == memtag_type::logical)
>>>> -    tag = aarch64_mte_get_ltag (addr);
>>>> -  else
>>>> -    {
>>>> -      /* Remove the top byte.  */
>>>> -      addr = gdbarch_remove_non_address_bits (gdbarch, addr);
>>>> -      std::optional<CORE_ADDR> atag = aarch64_mte_get_atag (addr);
>>>> -
>>>> -      if (!atag.has_value ())
>>>> -    return nullptr;
>>>> -
>>>> -      tag = *atag;
>>>> -    }
>>>> -
>>>> -  /* Convert the tag to a value.  */
>>>> -  return value_from_ulongest (builtin_type (gdbarch)->builtin_unsigned_int,
>>>> -                  tag);
>>>> -}
>>>> -
>>>> -/* Implement the memtag_to_string gdbarch method.  */
>>>> -
>>>> -static std::string
>>>> -aarch64_linux_memtag_to_string (struct gdbarch *gdbarch, struct value *tag_value)
>>>> -{
>>>> -  if (tag_value == nullptr)
>>>> -    return "";
>>>> -
>>>> -  CORE_ADDR tag = value_as_address (tag_value);
>>>> -
>>>> -  return string_printf ("0x%s", phex_nz (tag, sizeof (tag)));
>>>> -}
>>>>      /* AArch64 Linux implementation of the report_signal_info gdbarch
>>>>       hook.  Displays information about possible memory tag violations.  */
>>>> @@ -2900,24 +2751,6 @@ aarch64_linux_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
>>>>          /* Register a hook for checking if an address is tagged or not.  */
>>>>          set_gdbarch_tagged_address_p (gdbarch, aarch64_linux_tagged_address_p);
>>>>    -      /* Register a hook for checking if there is a memory tag match.  */
>>>> -      set_gdbarch_memtag_matches_p (gdbarch,
>>>> -                    aarch64_linux_memtag_matches_p);
>>>> -
>>>> -      /* Register a hook for setting the logical/allocation tags for
>>>> -     a range of addresses.  */
>>>> -      set_gdbarch_set_memtags (gdbarch, aarch64_linux_set_memtags);
>>>> -
>>>> -      /* Register a hook for extracting the logical/allocation tag from an
>>>> -     address.  */
>>>> -      set_gdbarch_get_memtag (gdbarch, aarch64_linux_get_memtag);
>>>> -
>>>> -      /* Set the allocation tag granule size to 16 bytes.  */
>>>> -      set_gdbarch_memtag_granule_size (gdbarch, AARCH64_MTE_GRANULE_SIZE);
>>>> -
>>>> -      /* Register a hook for converting a memory tag to a string.  */
>>>> -      set_gdbarch_memtag_to_string (gdbarch, aarch64_linux_memtag_to_string);
>>>> -
>>>>          set_gdbarch_report_signal_info (gdbarch,
>>>>                          aarch64_linux_report_signal_info);
>>>>    diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
>>>> index e4bca6c6632..f6227a5b82d 100644
>>>> --- a/gdb/aarch64-tdep.c
>>>> +++ b/gdb/aarch64-tdep.c
>>>> @@ -45,6 +45,7 @@
>>>>      #include "aarch64-tdep.h"
>>>>    #include "aarch64-ravenscar-thread.h"
>>>> +#include "arch/aarch64-mte-linux.h"
>>>
>>> Including a more specific scope into a generic one is incorrect. In this
>>> particular case we're including a *linux* header into a supposedly generic
>>> aarch64-tdep.c file.
>>>
>>> If we need to use things contained in the Linux files, we should make sure they can be made
>>> generic and then extract those out from arch/aarch64-mte-linux.[c|h] and then move them
>>> to, say, arch/aarch64-mte.[c|h].
>>>
>>>>      #include "record.h"
>>>>    #include "record-full.h"
>>>> @@ -4088,6 +4089,156 @@ aarch64_stack_frame_destroyed_p (struct gdbarch *gdbarch, CORE_ADDR pc)
>>>>      return streq (inst.opcode->name, "ret");
>>>>    }
>>>>    +/* Helper to get the allocation tag from a 64-bit ADDRESS.
>>>> +
>>>> +   Return the allocation tag if successful and nullopt otherwise.  */
>>>> +
>>>> +std::optional<CORE_ADDR>
>>>> +aarch64_mte_get_atag (CORE_ADDR address)
>>>> +{
>>>> +  gdb::byte_vector tags;
>>>> +
>>>> +  /* Attempt to fetch the allocation tag.  */
>>>> +  if (!target_fetch_memtags (address, 1, tags,
>>>> +                 static_cast<int> (memtag_type::allocation)))
>>>> +    return {};
>>>> +
>>>> +  /* Only one tag should've been returned.  Make sure we got exactly that.  */
>>>> +  if (tags.size () != 1)
>>>> +    error (_("Target returned an unexpected number of tags."));
>>>> +
>>>> +  /* Although our tags are 4 bits in size, they are stored in a
>>>> +     byte.  */
>>>> +  return tags[0];
>>>> +}
>>>> +
>>>> +/* Implement the memtag_matches_p gdbarch method.  */
>>>> +
>>>> +static bool
>>>> +aarch64_memtag_matches_p (struct gdbarch *gdbarch,
>>>> +                struct value *address)
>>>
>>> There seems to be something off with indentation above.
>>>
>>>> +{
>>>> +  gdb_assert (address != nullptr);
>>>> +
>>>> +  CORE_ADDR addr = value_as_address (address);
>>>> +
>>>> +  /* Fetch the allocation tag for ADDRESS.  */
>>>> +  std::optional<CORE_ADDR> atag
>>>> +    = aarch64_mte_get_atag (gdbarch_remove_non_address_bits (gdbarch, addr));
>>>> +
>>>> +  if (!atag.has_value ())
>>>> +    return true;
>>>> +
>>>> +  /* Fetch the logical tag for ADDRESS.  */
>>>> +  gdb_byte ltag = aarch64_mte_get_ltag (addr);
>>>> +
>>>> +  /* Are the tags the same?  */
>>>> +  return ltag == *atag;
>>>> +}
>>>> +
>>>> +/* Implement the set_memtags gdbarch method.  */
>>>> +
>>>> +static bool
>>>> +aarch64_set_memtags (struct gdbarch *gdbarch, struct value *address,
>>>> +               size_t length, const gdb::byte_vector &tags,
>>>> +               memtag_type tag_type)
>>>
>>> Indentation above looks off too.
>>>
>>>> +{
>>>> +  gdb_assert (!tags.empty ());
>>>> +  gdb_assert (address != nullptr);
>>>> +
>>>> +  CORE_ADDR addr = value_as_address (address);
>>>> +
>>>> +  /* Set the logical tag or the allocation tag.  */
>>>> +  if (tag_type == memtag_type::logical)
>>>> +    {
>>>> +      /* When setting logical tags, we don't care about the length, since
>>>> +     we are only setting a single logical tag.  */
>>>> +      addr = aarch64_mte_set_ltag (addr, tags[0]);
>>>> +
>>>> +      /* Update the value's content with the tag.  */
>>>> +      enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
>>>> +      gdb_byte *srcbuf = address->contents_raw ().data ();
>>>> +      store_unsigned_integer (srcbuf, sizeof (addr), byte_order, addr);
>>>> +    }
>>>> +  else
>>>> +    {
>>>> +      /* Remove the top byte.  */
>>>> +      addr = gdbarch_remove_non_address_bits (gdbarch, addr);
>>>> +
>>>> +      /* With G being the number of tag granules and N the number of tags
>>>> +     passed in, we can have the following cases:
>>>> +
>>>> +     1 - G == N: Store all the N tags to memory.
>>>> +
>>>> +     2 - G < N : Warn about having more tags than granules, but write G
>>>> +             tags.
>>>> +
>>>> +     3 - G > N : This is a "fill tags" operation.  We should use the tags
>>>> +             as a pattern to fill the granules repeatedly until we have
>>>> +             written G tags to memory.
>>>> +      */
>>>> +
>>>> +      size_t g = aarch64_mte_get_tag_granules (addr, length,
>>>> +                           AARCH64_MTE_GRANULE_SIZE);
>>>> +      size_t n = tags.size ();
>>>> +
>>>> +      if (g < n)
>>>> +    warning (_("Got more tags than memory granules.  Tags will be "
>>>> +           "truncated."));
>>>> +      else if (g > n)
>>>> +    warning (_("Using tag pattern to fill memory range."));
>>>> +
>>>> +      if (!target_store_memtags (addr, length, tags,
>>>> +                 static_cast<int> (memtag_type::allocation)))
>>>> +    return false;
>>>> +    }
>>>> +  return true;
>>>> +}
>>>> +
>>>> +/* Implement the get_memtag gdbarch method.  */
>>>> +
>>>> +static struct value *
>>>> +aarch64_get_memtag (struct gdbarch *gdbarch, struct value *address,
>>>> +              memtag_type tag_type)
>>>
>>> Indentation is off above.
>>>
>>>> +{
>>>> +  gdb_assert (address != nullptr);
>>>> +
>>>> +  CORE_ADDR addr = value_as_address (address);
>>>> +  CORE_ADDR tag = 0;
>>>> +
>>>> +  /* Get the logical tag or the allocation tag.  */
>>>> +  if (tag_type == memtag_type::logical)
>>>> +    tag = aarch64_mte_get_ltag (addr);
>>>> +  else
>>>> +    {
>>>> +      /* Remove the top byte.  */
>>>> +      addr = gdbarch_remove_non_address_bits (gdbarch, addr);
>>>> +      std::optional<CORE_ADDR> atag = aarch64_mte_get_atag (addr);
>>>> +
>>>> +      if (!atag.has_value ())
>>>> +    return nullptr;
>>>> +
>>>> +      tag = *atag;
>>>> +    }
>>>> +
>>>> +  /* Convert the tag to a value.  */
>>>> +  return value_from_ulongest (builtin_type (gdbarch)->builtin_unsigned_int,
>>>> +                  tag);
>>>> +}
>>>> +
>>>> +/* Implement the memtag_to_string gdbarch method.  */
>>>> +
>>>> +static std::string
>>>> +aarch64_memtag_to_string (struct gdbarch *gdbarch, struct value *tag_value)
>>>> +{
>>>> +  if (tag_value == nullptr)
>>>> +    return "";
>>>> +
>>>> +  CORE_ADDR tag = value_as_address (tag_value);
>>>> +
>>>> +  return string_printf ("0x%s", phex_nz (tag, sizeof (tag)));
>>>> +}
>>>> +
>>>>    /* AArch64 implementation of the remove_non_address_bits gdbarch hook.  Remove
>>>>       non address bits from a pointer value.  */
>>>>    @@ -4504,6 +4655,23 @@ aarch64_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
>>>>                        aarch64_pseudo_register_reggroup_p);
>>>>      set_gdbarch_cannot_store_register (gdbarch, aarch64_cannot_store_register);
>>>>    +  /* Set the allocation tag granule size to 16 bytes.  */
>>>> +  set_gdbarch_memtag_granule_size (gdbarch, AARCH64_MTE_GRANULE_SIZE);
>>>> +
>>>> +  /* Register a hook for checking if there is a memory tag match.  */
>>>> +  set_gdbarch_memtag_matches_p (gdbarch, aarch64_memtag_matches_p);
>>>> +
>>>> +  /* Register a hook for setting the logical/allocation tags for
>>>> +     a range of addresses.  */
>>>> +  set_gdbarch_set_memtags (gdbarch, aarch64_set_memtags);
>>>> +
>>>> +  /* Register a hook for extracting the logical/allocation tag from an
>>>> +     address.  */
>>>> +  set_gdbarch_get_memtag (gdbarch, aarch64_get_memtag);
>>>> +
>>>> +  /* Register a hook for converting a memory tag to a string.  */
>>>> +  set_gdbarch_memtag_to_string (gdbarch, aarch64_memtag_to_string);
>>>> +
>>>
>>> These gdbarch hooks are being registered regardless of whether the target supports
>>> MTE or not. In aarch64-linux-tdep.c they were registered if MTE support was confirmed
>>> by the presence of the XML feature. I think this should be the same, even if we have
>>> some safety checks prior to using these hooks.
>>
>> tdep->has_mte() does not work in aarch64-tdep.c because the XML advertises the 'tag_ctl'
>> pseudo-register, meant to change the Tag Check Fault behavior at runtime. It doesn't
>> make sense to have it in baremetal, since it's not a real register and was introduced
>> to emulate Linux's prctl() PR_SET_TAGGED_ADDR_CTRL option. In that sense, QEMU system
>> mode doesn't advertise it for this reason either.
> 
> Yeah, since the feature was initially designed with Linux in mind, we have the tag_ctl
> register and its associated register set that we need to read from/dump to core files.
> 
> With the feature being supported for baremetal now, that might need to become a Linux-specific
> thing. Which, incidentally, as I mentioned above about the header file, is a bit of an intrusion
> of Linux-specific knowledge on an otherwise generic aarch64-tdep.c.
> 
>>
>> As you said, all memory-tag subcommands check if MTE is supported by the target calling
>> target_supports_memory_tagging() and so all goes well in aarch64-tdep.c
>>
>> I don't have any idea on how to address what you're asking for. Actually, I was inclined
>> to remove the if (tdep->has_mte()) check in aarch64-linux-tdep.c when registering the
>> hooks. I don't see any harm on always registering them and with has_mte() in place we
>> are kind duplicating the check: one check is done via XML advertisement of 'tag_ctl' and
>> the other check (in the hooks) uses "memory-tagging+" feature in qSupported.
> 
> It might be possible to do the same check with the target method I suppose?
> 
> The remote target would advertise memory-tagging+, the native and linux layers would
> advertise the presence of HWCAP2_MTE. The question is if the target is properly setup at
> the point where we are checking these XML features. We'd need to check

Do you mean something like:

diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
index 865b1c0b13b..bd4baf2632a 100644
--- a/gdb/aarch64-tdep.c
+++ b/gdb/aarch64-tdep.c
@@ -4655,6 +4655,8 @@ aarch64_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
                                         aarch64_pseudo_register_reggroup_p);
    set_gdbarch_cannot_store_register (gdbarch, aarch64_cannot_store_register);
  
+  if (tdep->has_mte ())
+    {
    /* Set the allocation tag granule size to 16 bytes.  */
    set_gdbarch_memtag_granule_size (gdbarch, AARCH64_MTE_GRANULE_SIZE);
  
@@ -4671,6 +4673,7 @@ aarch64_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
  
    /* Register a hook for converting a memory tag to a string.  */
    set_gdbarch_memtag_to_string (gdbarch, aarch64_memtag_to_string);
+    }
  
    /* ABI */
    set_gdbarch_short_bit (gdbarch, 16);
diff --git a/gdb/aarch64-tdep.h b/gdb/aarch64-tdep.h
index 50166fb4f24..989ccc7a7b9 100644
--- a/gdb/aarch64-tdep.h
+++ b/gdb/aarch64-tdep.h
@@ -26,6 +26,7 @@
  #include "displaced-stepping.h"
  #include "infrun.h"
  #include "gdbarch.h"
+#include "target.h"
  
  /* Forward declarations.  */
  struct gdbarch;
@@ -126,7 +127,7 @@ struct aarch64_gdbarch_tdep : gdbarch_tdep_base
    /* Returns true if the target supports MTE.  */
    bool has_mte () const
    {
-    return mte_reg_base != -1;
+    return target_supports_memory_tagging ();
    }
  
    /* TLS registers.  This is -1 if the TLS registers are not available.  */

?

Yes, this works because in remote_target::start_remote_1() (e.g. invoked on 'target remote :1234')
remote_query_supported(), which parses the qSupported  string, is called before
target_find_description() is called (which then calls aarch64_gdbarch_init()), hence when has_mte()
is called from aarch64_gdbarch_init() qSupported string is already parsed correctly.

I.e, in remote_target::start_remote_1() we have:

[...]

   /* The first packet we send to the target is the optional "supported
      packets" request.  If the target can answer this, it will tell us
      which later probes to skip.  */
   remote_query_supported ();

[...]

   /* Next, if the target can specify a description, read it.  We do
      this before anything involving memory or registers.  */
   target_find_description ();


Is this your suggestion?

Thanks.


Cheers,
Gustavo
  

On 7/25/24 01:54, Gustavo Romero wrote:
> Hi Luis,
> 
> On 7/24/24 6:17 AM, Luis Machado wrote:
>> On 7/24/24 01:45, Gustavo Romero wrote:
>>> Hi Luis!
>>>
>>> On 7/23/24 6:46 AM, Luis Machado wrote:
>>>> Hi,
>>>>
>>>> Thanks for the patch.
>>>
>>> Thanks a lot for the review :)
>>>
>>>
>>>> On 7/22/24 15:41, Gustavo Romero wrote:
>>>>> This commit moves aarch64_linux_memtag_matches_p,
>>>>> aarch64_linux_set_memtags, aarch64_linux_get_memtag, and
>>>>> aarch64_linux_memtag_to_string hooks (plus aarch64_mte_get_atag
>>>>> function used by them), along with the setting of the memtag granule
>>>>> size, from aarch64-linux-tdep.c to aarch64-tdep.c, making MTE available
>>>>> on baremetal targets. Since the aarch64-linux-tdep.c layer inherits
>>>>> these hooks from aarch64-tdep.c, there is no effective change for
>>>>> aarch64-linux targets.
>>>>>
>>>>> Signed-off-by: Gustavo Romero <gustavo.romero@linaro.org>
>>>>> ---
>>>>>    gdb/aarch64-linux-tdep.c | 167 --------------------------------------
>>>>>    gdb/aarch64-tdep.c       | 168 +++++++++++++++++++++++++++++++++++++++
>>>>>    gdb/aarch64-tdep.h       |   3 +
>>>>>    3 files changed, 171 insertions(+), 167 deletions(-)
>>>>>
>>>>> diff --git a/gdb/aarch64-linux-tdep.c b/gdb/aarch64-linux-tdep.c
>>>>> index a1296a8f0c7..63defd5a31f 100644
>>>>> --- a/gdb/aarch64-linux-tdep.c
>>>>> +++ b/gdb/aarch64-linux-tdep.c
>>>>> @@ -2427,29 +2427,6 @@ aarch64_linux_gcc_target_options (struct gdbarch *gdbarch)
>>>>>      return {};
>>>>>    }
>>>>>    -/* Helper to get the allocation tag from a 64-bit ADDRESS.
>>>>> -
>>>>> -   Return the allocation tag if successful and nullopt otherwise.  */
>>>>> -
>>>>> -static std::optional<CORE_ADDR>
>>>>> -aarch64_mte_get_atag (CORE_ADDR address)
>>>>> -{
>>>>> -  gdb::byte_vector tags;
>>>>> -
>>>>> -  /* Attempt to fetch the allocation tag.  */
>>>>> -  if (!target_fetch_memtags (address, 1, tags,
>>>>> -                 static_cast<int> (memtag_type::allocation)))
>>>>> -    return {};
>>>>> -
>>>>> -  /* Only one tag should've been returned.  Make sure we got exactly that.  */
>>>>> -  if (tags.size () != 1)
>>>>> -    error (_("Target returned an unexpected number of tags."));
>>>>> -
>>>>> -  /* Although our tags are 4 bits in size, they are stored in a
>>>>> -     byte.  */
>>>>> -  return tags[0];
>>>>> -}
>>>>> -
>>>>>    /* Implement the tagged_address_p gdbarch method.  */
>>>>>      static bool
>>>>> @@ -2466,132 +2443,6 @@ aarch64_linux_tagged_address_p (struct gdbarch *gdbarch, CORE_ADDR address)
>>>>>      return true;
>>>>>    }
>>>>>    -/* Implement the memtag_matches_p gdbarch method.  */
>>>>> -
>>>>> -static bool
>>>>> -aarch64_linux_memtag_matches_p (struct gdbarch *gdbarch,
>>>>> -                struct value *address)
>>>>> -{
>>>>> -  gdb_assert (address != nullptr);
>>>>> -
>>>>> -  CORE_ADDR addr = value_as_address (address);
>>>>> -
>>>>> -  /* Fetch the allocation tag for ADDRESS.  */
>>>>> -  std::optional<CORE_ADDR> atag
>>>>> -    = aarch64_mte_get_atag (gdbarch_remove_non_address_bits (gdbarch, addr));
>>>>> -
>>>>> -  if (!atag.has_value ())
>>>>> -    return true;
>>>>> -
>>>>> -  /* Fetch the logical tag for ADDRESS.  */
>>>>> -  gdb_byte ltag = aarch64_mte_get_ltag (addr);
>>>>> -
>>>>> -  /* Are the tags the same?  */
>>>>> -  return ltag == *atag;
>>>>> -}
>>>>> -
>>>>> -/* Implement the set_memtags gdbarch method.  */
>>>>> -
>>>>> -static bool
>>>>> -aarch64_linux_set_memtags (struct gdbarch *gdbarch, struct value *address,
>>>>> -               size_t length, const gdb::byte_vector &tags,
>>>>> -               memtag_type tag_type)
>>>>> -{
>>>>> -  gdb_assert (!tags.empty ());
>>>>> -  gdb_assert (address != nullptr);
>>>>> -
>>>>> -  CORE_ADDR addr = value_as_address (address);
>>>>> -
>>>>> -  /* Set the logical tag or the allocation tag.  */
>>>>> -  if (tag_type == memtag_type::logical)
>>>>> -    {
>>>>> -      /* When setting logical tags, we don't care about the length, since
>>>>> -     we are only setting a single logical tag.  */
>>>>> -      addr = aarch64_mte_set_ltag (addr, tags[0]);
>>>>> -
>>>>> -      /* Update the value's content with the tag.  */
>>>>> -      enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
>>>>> -      gdb_byte *srcbuf = address->contents_raw ().data ();
>>>>> -      store_unsigned_integer (srcbuf, sizeof (addr), byte_order, addr);
>>>>> -    }
>>>>> -  else
>>>>> -    {
>>>>> -      /* Remove the top byte.  */
>>>>> -      addr = gdbarch_remove_non_address_bits (gdbarch, addr);
>>>>> -
>>>>> -      /* With G being the number of tag granules and N the number of tags
>>>>> -     passed in, we can have the following cases:
>>>>> -
>>>>> -     1 - G == N: Store all the N tags to memory.
>>>>> -
>>>>> -     2 - G < N : Warn about having more tags than granules, but write G
>>>>> -             tags.
>>>>> -
>>>>> -     3 - G > N : This is a "fill tags" operation.  We should use the tags
>>>>> -             as a pattern to fill the granules repeatedly until we have
>>>>> -             written G tags to memory.
>>>>> -      */
>>>>> -
>>>>> -      size_t g = aarch64_mte_get_tag_granules (addr, length,
>>>>> -                           AARCH64_MTE_GRANULE_SIZE);
>>>>> -      size_t n = tags.size ();
>>>>> -
>>>>> -      if (g < n)
>>>>> -    warning (_("Got more tags than memory granules.  Tags will be "
>>>>> -           "truncated."));
>>>>> -      else if (g > n)
>>>>> -    warning (_("Using tag pattern to fill memory range."));
>>>>> -
>>>>> -      if (!target_store_memtags (addr, length, tags,
>>>>> -                 static_cast<int> (memtag_type::allocation)))
>>>>> -    return false;
>>>>> -    }
>>>>> -  return true;
>>>>> -}
>>>>> -
>>>>> -/* Implement the get_memtag gdbarch method.  */
>>>>> -
>>>>> -static struct value *
>>>>> -aarch64_linux_get_memtag (struct gdbarch *gdbarch, struct value *address,
>>>>> -              memtag_type tag_type)
>>>>> -{
>>>>> -  gdb_assert (address != nullptr);
>>>>> -
>>>>> -  CORE_ADDR addr = value_as_address (address);
>>>>> -  CORE_ADDR tag = 0;
>>>>> -
>>>>> -  /* Get the logical tag or the allocation tag.  */
>>>>> -  if (tag_type == memtag_type::logical)
>>>>> -    tag = aarch64_mte_get_ltag (addr);
>>>>> -  else
>>>>> -    {
>>>>> -      /* Remove the top byte.  */
>>>>> -      addr = gdbarch_remove_non_address_bits (gdbarch, addr);
>>>>> -      std::optional<CORE_ADDR> atag = aarch64_mte_get_atag (addr);
>>>>> -
>>>>> -      if (!atag.has_value ())
>>>>> -    return nullptr;
>>>>> -
>>>>> -      tag = *atag;
>>>>> -    }
>>>>> -
>>>>> -  /* Convert the tag to a value.  */
>>>>> -  return value_from_ulongest (builtin_type (gdbarch)->builtin_unsigned_int,
>>>>> -                  tag);
>>>>> -}
>>>>> -
>>>>> -/* Implement the memtag_to_string gdbarch method.  */
>>>>> -
>>>>> -static std::string
>>>>> -aarch64_linux_memtag_to_string (struct gdbarch *gdbarch, struct value *tag_value)
>>>>> -{
>>>>> -  if (tag_value == nullptr)
>>>>> -    return "";
>>>>> -
>>>>> -  CORE_ADDR tag = value_as_address (tag_value);
>>>>> -
>>>>> -  return string_printf ("0x%s", phex_nz (tag, sizeof (tag)));
>>>>> -}
>>>>>      /* AArch64 Linux implementation of the report_signal_info gdbarch
>>>>>       hook.  Displays information about possible memory tag violations.  */
>>>>> @@ -2900,24 +2751,6 @@ aarch64_linux_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
>>>>>          /* Register a hook for checking if an address is tagged or not.  */
>>>>>          set_gdbarch_tagged_address_p (gdbarch, aarch64_linux_tagged_address_p);
>>>>>    -      /* Register a hook for checking if there is a memory tag match.  */
>>>>> -      set_gdbarch_memtag_matches_p (gdbarch,
>>>>> -                    aarch64_linux_memtag_matches_p);
>>>>> -
>>>>> -      /* Register a hook for setting the logical/allocation tags for
>>>>> -     a range of addresses.  */
>>>>> -      set_gdbarch_set_memtags (gdbarch, aarch64_linux_set_memtags);
>>>>> -
>>>>> -      /* Register a hook for extracting the logical/allocation tag from an
>>>>> -     address.  */
>>>>> -      set_gdbarch_get_memtag (gdbarch, aarch64_linux_get_memtag);
>>>>> -
>>>>> -      /* Set the allocation tag granule size to 16 bytes.  */
>>>>> -      set_gdbarch_memtag_granule_size (gdbarch, AARCH64_MTE_GRANULE_SIZE);
>>>>> -
>>>>> -      /* Register a hook for converting a memory tag to a string.  */
>>>>> -      set_gdbarch_memtag_to_string (gdbarch, aarch64_linux_memtag_to_string);
>>>>> -
>>>>>          set_gdbarch_report_signal_info (gdbarch,
>>>>>                          aarch64_linux_report_signal_info);
>>>>>    diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
>>>>> index e4bca6c6632..f6227a5b82d 100644
>>>>> --- a/gdb/aarch64-tdep.c
>>>>> +++ b/gdb/aarch64-tdep.c
>>>>> @@ -45,6 +45,7 @@
>>>>>      #include "aarch64-tdep.h"
>>>>>    #include "aarch64-ravenscar-thread.h"
>>>>> +#include "arch/aarch64-mte-linux.h"
>>>>
>>>> Including a more specific scope into a generic one is incorrect. In this
>>>> particular case we're including a *linux* header into a supposedly generic
>>>> aarch64-tdep.c file.
>>>>
>>>> If we need to use things contained in the Linux files, we should make sure they can be made
>>>> generic and then extract those out from arch/aarch64-mte-linux.[c|h] and then move them
>>>> to, say, arch/aarch64-mte.[c|h].
>>>>
>>>>>      #include "record.h"
>>>>>    #include "record-full.h"
>>>>> @@ -4088,6 +4089,156 @@ aarch64_stack_frame_destroyed_p (struct gdbarch *gdbarch, CORE_ADDR pc)
>>>>>      return streq (inst.opcode->name, "ret");
>>>>>    }
>>>>>    +/* Helper to get the allocation tag from a 64-bit ADDRESS.
>>>>> +
>>>>> +   Return the allocation tag if successful and nullopt otherwise.  */
>>>>> +
>>>>> +std::optional<CORE_ADDR>
>>>>> +aarch64_mte_get_atag (CORE_ADDR address)
>>>>> +{
>>>>> +  gdb::byte_vector tags;
>>>>> +
>>>>> +  /* Attempt to fetch the allocation tag.  */
>>>>> +  if (!target_fetch_memtags (address, 1, tags,
>>>>> +                 static_cast<int> (memtag_type::allocation)))
>>>>> +    return {};
>>>>> +
>>>>> +  /* Only one tag should've been returned.  Make sure we got exactly that.  */
>>>>> +  if (tags.size () != 1)
>>>>> +    error (_("Target returned an unexpected number of tags."));
>>>>> +
>>>>> +  /* Although our tags are 4 bits in size, they are stored in a
>>>>> +     byte.  */
>>>>> +  return tags[0];
>>>>> +}
>>>>> +
>>>>> +/* Implement the memtag_matches_p gdbarch method.  */
>>>>> +
>>>>> +static bool
>>>>> +aarch64_memtag_matches_p (struct gdbarch *gdbarch,
>>>>> +                struct value *address)
>>>>
>>>> There seems to be something off with indentation above.
>>>>
>>>>> +{
>>>>> +  gdb_assert (address != nullptr);
>>>>> +
>>>>> +  CORE_ADDR addr = value_as_address (address);
>>>>> +
>>>>> +  /* Fetch the allocation tag for ADDRESS.  */
>>>>> +  std::optional<CORE_ADDR> atag
>>>>> +    = aarch64_mte_get_atag (gdbarch_remove_non_address_bits (gdbarch, addr));
>>>>> +
>>>>> +  if (!atag.has_value ())
>>>>> +    return true;
>>>>> +
>>>>> +  /* Fetch the logical tag for ADDRESS.  */
>>>>> +  gdb_byte ltag = aarch64_mte_get_ltag (addr);
>>>>> +
>>>>> +  /* Are the tags the same?  */
>>>>> +  return ltag == *atag;
>>>>> +}
>>>>> +
>>>>> +/* Implement the set_memtags gdbarch method.  */
>>>>> +
>>>>> +static bool
>>>>> +aarch64_set_memtags (struct gdbarch *gdbarch, struct value *address,
>>>>> +               size_t length, const gdb::byte_vector &tags,
>>>>> +               memtag_type tag_type)
>>>>
>>>> Indentation above looks off too.
>>>>
>>>>> +{
>>>>> +  gdb_assert (!tags.empty ());
>>>>> +  gdb_assert (address != nullptr);
>>>>> +
>>>>> +  CORE_ADDR addr = value_as_address (address);
>>>>> +
>>>>> +  /* Set the logical tag or the allocation tag.  */
>>>>> +  if (tag_type == memtag_type::logical)
>>>>> +    {
>>>>> +      /* When setting logical tags, we don't care about the length, since
>>>>> +     we are only setting a single logical tag.  */
>>>>> +      addr = aarch64_mte_set_ltag (addr, tags[0]);
>>>>> +
>>>>> +      /* Update the value's content with the tag.  */
>>>>> +      enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
>>>>> +      gdb_byte *srcbuf = address->contents_raw ().data ();
>>>>> +      store_unsigned_integer (srcbuf, sizeof (addr), byte_order, addr);
>>>>> +    }
>>>>> +  else
>>>>> +    {
>>>>> +      /* Remove the top byte.  */
>>>>> +      addr = gdbarch_remove_non_address_bits (gdbarch, addr);
>>>>> +
>>>>> +      /* With G being the number of tag granules and N the number of tags
>>>>> +     passed in, we can have the following cases:
>>>>> +
>>>>> +     1 - G == N: Store all the N tags to memory.
>>>>> +
>>>>> +     2 - G < N : Warn about having more tags than granules, but write G
>>>>> +             tags.
>>>>> +
>>>>> +     3 - G > N : This is a "fill tags" operation.  We should use the tags
>>>>> +             as a pattern to fill the granules repeatedly until we have
>>>>> +             written G tags to memory.
>>>>> +      */
>>>>> +
>>>>> +      size_t g = aarch64_mte_get_tag_granules (addr, length,
>>>>> +                           AARCH64_MTE_GRANULE_SIZE);
>>>>> +      size_t n = tags.size ();
>>>>> +
>>>>> +      if (g < n)
>>>>> +    warning (_("Got more tags than memory granules.  Tags will be "
>>>>> +           "truncated."));
>>>>> +      else if (g > n)
>>>>> +    warning (_("Using tag pattern to fill memory range."));
>>>>> +
>>>>> +      if (!target_store_memtags (addr, length, tags,
>>>>> +                 static_cast<int> (memtag_type::allocation)))
>>>>> +    return false;
>>>>> +    }
>>>>> +  return true;
>>>>> +}
>>>>> +
>>>>> +/* Implement the get_memtag gdbarch method.  */
>>>>> +
>>>>> +static struct value *
>>>>> +aarch64_get_memtag (struct gdbarch *gdbarch, struct value *address,
>>>>> +              memtag_type tag_type)
>>>>
>>>> Indentation is off above.
>>>>
>>>>> +{
>>>>> +  gdb_assert (address != nullptr);
>>>>> +
>>>>> +  CORE_ADDR addr = value_as_address (address);
>>>>> +  CORE_ADDR tag = 0;
>>>>> +
>>>>> +  /* Get the logical tag or the allocation tag.  */
>>>>> +  if (tag_type == memtag_type::logical)
>>>>> +    tag = aarch64_mte_get_ltag (addr);
>>>>> +  else
>>>>> +    {
>>>>> +      /* Remove the top byte.  */
>>>>> +      addr = gdbarch_remove_non_address_bits (gdbarch, addr);
>>>>> +      std::optional<CORE_ADDR> atag = aarch64_mte_get_atag (addr);
>>>>> +
>>>>> +      if (!atag.has_value ())
>>>>> +    return nullptr;
>>>>> +
>>>>> +      tag = *atag;
>>>>> +    }
>>>>> +
>>>>> +  /* Convert the tag to a value.  */
>>>>> +  return value_from_ulongest (builtin_type (gdbarch)->builtin_unsigned_int,
>>>>> +                  tag);
>>>>> +}
>>>>> +
>>>>> +/* Implement the memtag_to_string gdbarch method.  */
>>>>> +
>>>>> +static std::string
>>>>> +aarch64_memtag_to_string (struct gdbarch *gdbarch, struct value *tag_value)
>>>>> +{
>>>>> +  if (tag_value == nullptr)
>>>>> +    return "";
>>>>> +
>>>>> +  CORE_ADDR tag = value_as_address (tag_value);
>>>>> +
>>>>> +  return string_printf ("0x%s", phex_nz (tag, sizeof (tag)));
>>>>> +}
>>>>> +
>>>>>    /* AArch64 implementation of the remove_non_address_bits gdbarch hook.  Remove
>>>>>       non address bits from a pointer value.  */
>>>>>    @@ -4504,6 +4655,23 @@ aarch64_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
>>>>>                        aarch64_pseudo_register_reggroup_p);
>>>>>      set_gdbarch_cannot_store_register (gdbarch, aarch64_cannot_store_register);
>>>>>    +  /* Set the allocation tag granule size to 16 bytes.  */
>>>>> +  set_gdbarch_memtag_granule_size (gdbarch, AARCH64_MTE_GRANULE_SIZE);
>>>>> +
>>>>> +  /* Register a hook for checking if there is a memory tag match.  */
>>>>> +  set_gdbarch_memtag_matches_p (gdbarch, aarch64_memtag_matches_p);
>>>>> +
>>>>> +  /* Register a hook for setting the logical/allocation tags for
>>>>> +     a range of addresses.  */
>>>>> +  set_gdbarch_set_memtags (gdbarch, aarch64_set_memtags);
>>>>> +
>>>>> +  /* Register a hook for extracting the logical/allocation tag from an
>>>>> +     address.  */
>>>>> +  set_gdbarch_get_memtag (gdbarch, aarch64_get_memtag);
>>>>> +
>>>>> +  /* Register a hook for converting a memory tag to a string.  */
>>>>> +  set_gdbarch_memtag_to_string (gdbarch, aarch64_memtag_to_string);
>>>>> +
>>>>
>>>> These gdbarch hooks are being registered regardless of whether the target supports
>>>> MTE or not. In aarch64-linux-tdep.c they were registered if MTE support was confirmed
>>>> by the presence of the XML feature. I think this should be the same, even if we have
>>>> some safety checks prior to using these hooks.
>>>
>>> tdep->has_mte() does not work in aarch64-tdep.c because the XML advertises the 'tag_ctl'
>>> pseudo-register, meant to change the Tag Check Fault behavior at runtime. It doesn't
>>> make sense to have it in baremetal, since it's not a real register and was introduced
>>> to emulate Linux's prctl() PR_SET_TAGGED_ADDR_CTRL option. In that sense, QEMU system
>>> mode doesn't advertise it for this reason either.
>>
>> Yeah, since the feature was initially designed with Linux in mind, we have the tag_ctl
>> register and its associated register set that we need to read from/dump to core files.
>>
>> With the feature being supported for baremetal now, that might need to become a Linux-specific
>> thing. Which, incidentally, as I mentioned above about the header file, is a bit of an intrusion
>> of Linux-specific knowledge on an otherwise generic aarch64-tdep.c.
>>
>>>
>>> As you said, all memory-tag subcommands check if MTE is supported by the target calling
>>> target_supports_memory_tagging() and so all goes well in aarch64-tdep.c
>>>
>>> I don't have any idea on how to address what you're asking for. Actually, I was inclined
>>> to remove the if (tdep->has_mte()) check in aarch64-linux-tdep.c when registering the
>>> hooks. I don't see any harm on always registering them and with has_mte() in place we
>>> are kind duplicating the check: one check is done via XML advertisement of 'tag_ctl' and
>>> the other check (in the hooks) uses "memory-tagging+" feature in qSupported.
>>
>> It might be possible to do the same check with the target method I suppose?
>>
>> The remote target would advertise memory-tagging+, the native and linux layers would
>> advertise the presence of HWCAP2_MTE. The question is if the target is properly setup at
>> the point where we are checking these XML features. We'd need to check
> 
> Do you mean something like:
> 
> diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
> index 865b1c0b13b..bd4baf2632a 100644
> --- a/gdb/aarch64-tdep.c
> +++ b/gdb/aarch64-tdep.c
> @@ -4655,6 +4655,8 @@ aarch64_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
>                                         aarch64_pseudo_register_reggroup_p);
>    set_gdbarch_cannot_store_register (gdbarch, aarch64_cannot_store_register);
>  
> +  if (tdep->has_mte ())
> +    {
>    /* Set the allocation tag granule size to 16 bytes.  */
>    set_gdbarch_memtag_granule_size (gdbarch, AARCH64_MTE_GRANULE_SIZE);
>  
> @@ -4671,6 +4673,7 @@ aarch64_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
>  
>    /* Register a hook for converting a memory tag to a string.  */
>    set_gdbarch_memtag_to_string (gdbarch, aarch64_memtag_to_string);
> +    }
>  
>    /* ABI */
>    set_gdbarch_short_bit (gdbarch, 16);
> diff --git a/gdb/aarch64-tdep.h b/gdb/aarch64-tdep.h
> index 50166fb4f24..989ccc7a7b9 100644
> --- a/gdb/aarch64-tdep.h
> +++ b/gdb/aarch64-tdep.h
> @@ -26,6 +26,7 @@
>  #include "displaced-stepping.h"
>  #include "infrun.h"
>  #include "gdbarch.h"
> +#include "target.h"
>  
>  /* Forward declarations.  */
>  struct gdbarch;
> @@ -126,7 +127,7 @@ struct aarch64_gdbarch_tdep : gdbarch_tdep_base
>    /* Returns true if the target supports MTE.  */
>    bool has_mte () const
>    {
> -    return mte_reg_base != -1;
> +    return target_supports_memory_tagging ();
>    }
>  
>    /* TLS registers.  This is -1 if the TLS registers are not available.  */
> 
> ?
> 
> Yes, this works because in remote_target::start_remote_1() (e.g. invoked on 'target remote :1234')
> remote_query_supported(), which parses the qSupported  string, is called before
> target_find_description() is called (which then calls aarch64_gdbarch_init()), hence when has_mte()
> is called from aarch64_gdbarch_init() qSupported string is already parsed correctly.
> 
> I.e, in remote_target::start_remote_1() we have:
> 
> [...]
> 
>   /* The first packet we send to the target is the optional "supported
>      packets" request.  If the target can answer this, it will tell us
>      which later probes to skip.  */
>   remote_query_supported ();
> 
> [...]
> 
>   /* Next, if the target can specify a description, read it.  We do
>      this before anything involving memory or registers.  */
>   target_find_description ();
> 
> 
> Is this your suggestion?

I was thinking more about your suggestion to drop has_mte. Though that is mostly used for register sets.

Upon further analysis, I think it is fine to do registering of these generic hooks even if the tag_ctl is not
available.

Let me check v2.