Re: [PATCH v3 5/7] gdb: Introduce is_address_tagged target hook

[Luis Machado <luis.machado@arm.com>]

On 4/4/24 07:48, Gustavo Romero wrote:
> This commit introduces a new target hook, target_is_address_tagged,
> which is used instead of the gdbarch_tagged_address_p gdbarch hook in
> the upper layer (printcmd.c).
> 
> This change allows the memory tagging address checking to be specialized
> easily per target in the future. Since target_is_address_tagged
> continues to use the gdbarch_tagged_address_p hook there is no change
> in behavior for the targets using the new target hook (the remote.c,
> aarch64-linux-nat.c, and corelow.c targets).

The above block...

> 
> This change enables easy specialization of memory tagging address
> check per target in the future. As target_is_address_tagged continues
> to utilize the gdbarch_tagged_address_p hook, there is no change in
> behavior for all the targets that use the new target hook (i.e., the
> remote.c, aarch64-linux-nat.c, and corelow.c targets).

... seems to be somewhat duplicated above in the commit message.

Also, as general rule, we usually make updates clear at the top of the
commit message. for instace:

Updates in v3:

- Something something.
- Fixed breakage.

And then those update blocks don't get pushed when the series is
approved (sometimes some do push it).

> 
> Just the gdbarch_tagged_address_p signature is changed for convenience,
> since target_is_address_tagged takes the address to be checked as a
> CORE_ADDR type.
> 
> Signed-off-by: Gustavo Romero <gustavo.romero@linaro.org>
> ---
>  gdb/aarch64-linux-nat.c   |  8 ++++++++
>  gdb/aarch64-linux-tdep.c  | 10 ++++------
>  gdb/arch-utils.c          |  2 +-
>  gdb/arch-utils.h          |  2 +-
>  gdb/corelow.c             |  8 ++++++++
>  gdb/gdbarch-gen.h         |  4 ++--
>  gdb/gdbarch.c             |  2 +-
>  gdb/gdbarch_components.py |  2 +-
>  gdb/printcmd.c            | 31 +++++++++++++++++--------------
>  gdb/remote.c              | 10 ++++++++++
>  gdb/target-delegates.c    | 30 ++++++++++++++++++++++++++++++
>  gdb/target.c              |  6 ++++++
>  gdb/target.h              |  6 ++++++
>  13 files changed, 95 insertions(+), 26 deletions(-)
> 
> diff --git a/gdb/aarch64-linux-nat.c b/gdb/aarch64-linux-nat.c
> index 3face34ce79..19c099832a5 100644
> --- a/gdb/aarch64-linux-nat.c
> +++ b/gdb/aarch64-linux-nat.c
> @@ -110,6 +110,8 @@ class aarch64_linux_nat_target final
>    /* Write allocation tags to memory via PTRACE.  */
>    bool store_memtags (CORE_ADDR address, size_t len,
>  		      const gdb::byte_vector &tags, int type) override;
> +  /* Check if an address is tagged.  */
> +  bool is_address_tagged (gdbarch *gdbarch, CORE_ADDR address) override;
>  };
>  
>  static aarch64_linux_nat_target the_aarch64_linux_nat_target;
> @@ -1071,6 +1073,12 @@ aarch64_linux_nat_target::store_memtags (CORE_ADDR address, size_t len,
>    return false;
>  }
>  
> +bool
> +aarch64_linux_nat_target::is_address_tagged (gdbarch *gdbarch, CORE_ADDR address)
> +{
> +  return gdbarch_tagged_address_p (gdbarch, address);

I'd add a comment here on why we take a detour going to the linux-tdep layer
to read the smaps file, because we don't have a better way to get that information.

In the future, if this check is ever made available through PTRACE, one can come
here and drop the smaps path.

> +}
> +
>  void _initialize_aarch64_linux_nat ();
>  void
>  _initialize_aarch64_linux_nat ()
> diff --git a/gdb/aarch64-linux-tdep.c b/gdb/aarch64-linux-tdep.c
> index fc60e602748..2a47c3f0845 100644
> --- a/gdb/aarch64-linux-tdep.c
> +++ b/gdb/aarch64-linux-tdep.c
> @@ -2451,17 +2451,15 @@ aarch64_mte_get_atag (CORE_ADDR address)
>  /* Implement the tagged_address_p gdbarch method.  */
>  
>  static bool
> -aarch64_linux_tagged_address_p (struct gdbarch *gdbarch, struct value *address)
> +aarch64_linux_tagged_address_p (struct gdbarch *gdbarch, CORE_ADDR address)
>  {
> -  gdb_assert (address != nullptr);
> -
> -  CORE_ADDR addr = value_as_address (address);
> +  gdb_assert (address);

No need to assert for a non-zero address. We used to assert that we had a valid pointer,
but since we're no longer dealing with a pointer, we don't have to worry about it.

Plus, 0x0 is a valid address (at least for bare-metal. We could run into an assertion if
the user explicitly tries to check for tags in a 0x0 address. See below:

(gdb) memory-tag check 0x0
../../../repos/binutils-gdb/gdb/aarch64-linux-tdep.c:2456: internal-error: aarch64_linux_tagged_address_p: Assertion `address' failed.
A problem internal to GDB has been detected,
further debugging may prove unreliable.
----- Backtrace -----
0xaaaac69ccf97 _Z22gdb_internal_backtracev
0xaaaac6e443e7 _ZL17internal_vproblemP16internal_problemPKciS2_St9__va_list
0xaaaac6e4464f _Z15internal_verrorPKciS0_St9__va_list
0xaaaac734a8b3 _Z18internal_error_locPKciS0_z
0xaaaac68dc407 _ZL30aarch64_linux_tagged_address_pP7gdbarchm
0xaaaac6c849ab _ZL24memory_tag_check_commandPKci
0xaaaac69fb8ab _Z8cmd_funcP16cmd_list_elementPKci
0xaaaac6de5ed3 _Z15execute_commandPKci
0xaaaac6adc203 _Z15command_handlerPKc
0xaaaac6add45f _Z20command_line_handlerOSt10unique_ptrIcN3gdb13xfree_deleterIcEEE
0xaaaac6adccd7 _ZL23gdb_rl_callback_handlerPc
0xaaaac6ea7eeb rl_callback_read_char
0xaaaac6adbc7b _ZL42gdb_rl_callback_read_char_wrapper_noexceptv
0xaaaac6adcb3b _ZL33gdb_rl_callback_read_char_wrapperPv
0xaaaac6e1ef5f _ZL19stdin_event_handleriPv
0xaaaac734b29f _ZL18gdb_wait_for_eventi.part.0
0xaaaac734bc7f _Z16gdb_do_one_eventi
0xaaaac6bdd977 _ZL21captured_command_loopv
0xaaaac6bdf6bb _Z8gdb_mainP18captured_main_args
0xaaaac68cee47 main

>  
>    /* Remove the top byte for the memory range check.  */
> -  addr = gdbarch_remove_non_address_bits (gdbarch, addr);
> +  address = gdbarch_remove_non_address_bits (gdbarch, address);
>  
>    /* Check if the page that contains ADDRESS is mapped with PROT_MTE.  */
> -  if (!linux_address_in_memtag_page (addr))
> +  if (!linux_address_in_memtag_page (address))
>      return false;
>  
>    /* We have a valid tag in the top byte of the 64-bit address.  */
> diff --git a/gdb/arch-utils.c b/gdb/arch-utils.c
> index 456bfe971ff..cb149c36bc9 100644
> --- a/gdb/arch-utils.c
> +++ b/gdb/arch-utils.c
> @@ -102,7 +102,7 @@ default_memtag_to_string (struct gdbarch *gdbarch, struct value *tag)
>  /* See arch-utils.h */
>  
>  bool
> -default_tagged_address_p (struct gdbarch *gdbarch, struct value *address)
> +default_tagged_address_p (struct gdbarch *gdbarch, CORE_ADDR address)
>  {
>    /* By default, assume the address is untagged.  */
>    return false;
> diff --git a/gdb/arch-utils.h b/gdb/arch-utils.h
> index 2dcd8f6dc53..467be40c688 100644
> --- a/gdb/arch-utils.h
> +++ b/gdb/arch-utils.h
> @@ -141,7 +141,7 @@ extern std::string default_memtag_to_string (struct gdbarch *gdbarch,
>  					     struct value *tag);
>  
>  /* Default implementation of gdbarch_tagged_address_p.  */
> -bool default_tagged_address_p (struct gdbarch *gdbarch, struct value *address);
> +bool default_tagged_address_p (struct gdbarch *gdbarch, CORE_ADDR address);
>  
>  /* Default implementation of gdbarch_memtag_matches_p.  */
>  extern bool default_memtag_matches_p (struct gdbarch *gdbarch,
> diff --git a/gdb/corelow.c b/gdb/corelow.c
> index f4e8273d962..b13d0124471 100644
> --- a/gdb/corelow.c
> +++ b/gdb/corelow.c
> @@ -109,6 +109,8 @@ class core_target final : public process_stratum_target
>    bool fetch_memtags (CORE_ADDR address, size_t len,
>  		      gdb::byte_vector &tags, int type) override;
>  
> +  bool is_address_tagged (gdbarch *gdbarch, CORE_ADDR address) override;
> +

Maybe add a comment to this function stating what we're looking for:

/* If the architecture supports it, check if ADDRESS is within a memory range
   mapped with tags.  For example,  MTE tags for AArch64.  */

Or some other variation of the above.

The reason being that the corelow target is a bit special because it is generic, hence
why we see an override for a x86 target hook in there as well.

>    x86_xsave_layout fetch_x86_xsave_layout () override;
>  
>    /* A few helpers.  */
> @@ -1410,6 +1412,12 @@ core_target::fetch_memtags (CORE_ADDR address, size_t len,
>    return false;
>  }
>  
> +bool
> +core_target::is_address_tagged (gdbarch *gdbarch, CORE_ADDR address)
> +{
> +  return gdbarch_tagged_address_p (gdbarch, address);
> +}
> +
>  /* Implementation of the "fetch_x86_xsave_layout" target_ops method.  */
>  
>  x86_xsave_layout
> diff --git a/gdb/gdbarch-gen.h b/gdb/gdbarch-gen.h
> index ebcff80bb9e..63fab26987f 100644
> --- a/gdb/gdbarch-gen.h
> +++ b/gdb/gdbarch-gen.h
> @@ -707,8 +707,8 @@ extern void set_gdbarch_memtag_to_string (struct gdbarch *gdbarch, gdbarch_memta
>  /* Return true if ADDRESS contains a tag and false otherwise.  ADDRESS
>     must be either a pointer or a reference type. */
>  
> -typedef bool (gdbarch_tagged_address_p_ftype) (struct gdbarch *gdbarch, struct value *address);
> -extern bool gdbarch_tagged_address_p (struct gdbarch *gdbarch, struct value *address);
> +typedef bool (gdbarch_tagged_address_p_ftype) (struct gdbarch *gdbarch, CORE_ADDR address);
> +extern bool gdbarch_tagged_address_p (struct gdbarch *gdbarch, CORE_ADDR address);
>  extern void set_gdbarch_tagged_address_p (struct gdbarch *gdbarch, gdbarch_tagged_address_p_ftype *tagged_address_p);
>  
>  /* Return true if the tag from ADDRESS matches the memory tag for that
> diff --git a/gdb/gdbarch.c b/gdb/gdbarch.c
> index 9319571deba..2d92f604c49 100644
> --- a/gdb/gdbarch.c
> +++ b/gdb/gdbarch.c
> @@ -3232,7 +3232,7 @@ set_gdbarch_memtag_to_string (struct gdbarch *gdbarch,
>  }
>  
>  bool
> -gdbarch_tagged_address_p (struct gdbarch *gdbarch, struct value *address)
> +gdbarch_tagged_address_p (struct gdbarch *gdbarch, CORE_ADDR address)
>  {
>    gdb_assert (gdbarch != NULL);
>    gdb_assert (gdbarch->tagged_address_p != NULL);
> diff --git a/gdb/gdbarch_components.py b/gdb/gdbarch_components.py
> index 7d913ade621..24e979431b6 100644
> --- a/gdb/gdbarch_components.py
> +++ b/gdb/gdbarch_components.py
> @@ -1267,7 +1267,7 @@ must be either a pointer or a reference type.
>  """,
>      type="bool",
>      name="tagged_address_p",
> -    params=[("struct value *", "address")],
> +    params=[("CORE_ADDR", "address")],
>      predefault="default_tagged_address_p",
>      invalid=False,
>  )
> diff --git a/gdb/printcmd.c b/gdb/printcmd.c
> index 62fbcb98cfb..24eac7c8421 100644
> --- a/gdb/printcmd.c
> +++ b/gdb/printcmd.c
> @@ -1132,7 +1132,7 @@ do_examine (struct format_data fmt, struct gdbarch *gdbarch, CORE_ADDR addr)
>  	    = value_from_ulongest (builtin_type (gdbarch)->builtin_data_ptr,
>  				   tag_laddr);
>  
> -	  if (gdbarch_tagged_address_p (gdbarch, v_addr))
> +	  if (target_is_address_tagged (gdbarch, value_as_address (v_addr)))
>  	    {
>  	      /* Fetch the allocation tag.  */
>  	      struct value *tag
> @@ -1268,7 +1268,7 @@ print_value (value *val, const value_print_options &opts)
>  /* Returns true if memory tags should be validated.  False otherwise.  */
>  
>  static bool
> -should_validate_memtags (struct value *value)
> +should_validate_memtags (gdbarch *gdbarch, struct value *value)
>  {
>    gdb_assert (value != nullptr && value->type () != nullptr);
>  
> @@ -1289,7 +1289,7 @@ should_validate_memtags (struct value *value)
>      return false;
>  
>    /* We do.  Check whether it includes any tags.  */
> -  return gdbarch_tagged_address_p (current_inferior ()->arch  (), value);
> +  return target_is_address_tagged (gdbarch, value_as_address (value));
>  }
>  
>  /* Helper for parsing arguments for print_command_1.  */
> @@ -1346,7 +1346,7 @@ print_command_1 (const char *args, int voidprint)
>  	    {
>  	      gdbarch *arch = current_inferior ()->arch ();
>  
> -	      if (should_validate_memtags (val)
> +	      if (should_validate_memtags (arch, val)
>  		  && !gdbarch_memtag_matches_p (arch, val))
>  		{
>  		  /* Fetch the logical tag.  */
> @@ -2946,9 +2946,10 @@ memory_tag_print_tag_command (const char *args, enum memtag_type tag_type)
>       flag, it is no use trying to access/manipulate its allocation tag.
>  
>       It is OK to manipulate the logical tag though.  */
> +  CORE_ADDR addr = value_as_address (val);
>    if (tag_type == memtag_type::allocation
> -      && !gdbarch_tagged_address_p (arch, val))
> -    show_addr_not_tagged (value_as_address (val));
> +      && !target_is_address_tagged (arch, addr))
> +    show_addr_not_tagged (addr);
>  
>    value *tag_value = gdbarch_get_memtag (arch, val, tag_type);
>    std::string tag = gdbarch_memtag_to_string (arch, tag_value);
> @@ -3104,8 +3105,9 @@ parse_set_allocation_tag_input (const char *args, struct value **val,
>  
>    /* If the address is not in a region memory mapped with a memory tagging
>       flag, it is no use trying to access/manipulate its allocation tag.  */
> -  if (!gdbarch_tagged_address_p (current_inferior ()->arch (), *val))
> -    show_addr_not_tagged (value_as_address (*val));
> +  CORE_ADDR addr = value_as_address (*val);
> +  if (!target_is_address_tagged (current_inferior ()->arch (), addr))
> +    show_addr_not_tagged (addr);
>  }
>  
>  /* Implement the "memory-tag set-allocation-tag" command.
> @@ -3129,8 +3131,9 @@ memory_tag_set_allocation_tag_command (const char *args, int from_tty)
>  
>    /* If the address is not in a region memory mapped with a memory tagging
>       flag, it is no use trying to manipulate its allocation tag.  */
> -  if (!gdbarch_tagged_address_p (current_inferior ()->arch (), val))
> -    show_addr_not_tagged (value_as_address(val));
> +  CORE_ADDR addr = value_as_address (val);
> +  if (!target_is_address_tagged (current_inferior ()-> arch(), addr))
> +    show_addr_not_tagged (addr);
>  
>    if (!gdbarch_set_memtags (current_inferior ()->arch (), val, length, tags,
>  			    memtag_type::allocation))
> @@ -3157,12 +3160,12 @@ memory_tag_check_command (const char *args, int from_tty)
>    struct value *val = process_print_command_args (args, &print_opts, true);
>    gdbarch *arch = current_inferior ()->arch ();
>  
> +  CORE_ADDR addr = value_as_address (val);
> +
>    /* If the address is not in a region memory mapped with a memory tagging
>       flag, it is no use trying to access/manipulate its allocation tag.  */
> -  if (!gdbarch_tagged_address_p (arch, val))
> -    show_addr_not_tagged (value_as_address (val));
> -
> -  CORE_ADDR addr = value_as_address (val);
> +  if (!target_is_address_tagged (current_inferior ()->arch (), addr))
> +    show_addr_not_tagged (addr);

I noticed the above checks happen at least 3 times in the code. Maybe a future
cleanup possibility to split the check into a separate function.

>  
>    /* Check if the tag is valid.  */
>    if (!gdbarch_memtag_matches_p (arch, val))
> diff --git a/gdb/remote.c b/gdb/remote.c
> index e278711df7b..9717db55e27 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -1084,6 +1084,8 @@ class remote_target : public process_stratum_target
>    bool store_memtags (CORE_ADDR address, size_t len,
>  		      const gdb::byte_vector &tags, int type) override;
>  
> +  bool is_address_tagged (gdbarch *gdbarch, CORE_ADDR address) override;
> +
>  public: /* Remote specific methods.  */
>  
>    void remote_download_command_source (int num, ULONGEST addr,
> @@ -15573,6 +15575,14 @@ remote_target::store_memtags (CORE_ADDR address, size_t len,
>    return packet_check_result (rs->buf).status () == PACKET_OK;
>  }
>  
> +/* Implement the "is_address_tagged" target_ops method.  */
> +
> +bool
> +remote_target::is_address_tagged (gdbarch *gdbarch, CORE_ADDR address)
> +{
> +  return gdbarch_tagged_address_p (gdbarch, address);
> +}
> +
>  /* Return true if remote target T is non-stop.  */
>  
>  bool
> diff --git a/gdb/target-delegates.c b/gdb/target-delegates.c
> index 59ea70458ad..e322bbbe481 100644
> --- a/gdb/target-delegates.c
> +++ b/gdb/target-delegates.c
> @@ -197,6 +197,7 @@ struct dummy_target : public target_ops
>    bool supports_memory_tagging () override;
>    bool fetch_memtags (CORE_ADDR arg0, size_t arg1, gdb::byte_vector &arg2, int arg3) override;
>    bool store_memtags (CORE_ADDR arg0, size_t arg1, const gdb::byte_vector &arg2, int arg3) override;
> +  bool is_address_tagged (gdbarch *arg0, CORE_ADDR arg1) override;
>    x86_xsave_layout fetch_x86_xsave_layout () override;
>  };
>  
> @@ -373,6 +374,7 @@ struct debug_target : public target_ops
>    bool supports_memory_tagging () override;
>    bool fetch_memtags (CORE_ADDR arg0, size_t arg1, gdb::byte_vector &arg2, int arg3) override;
>    bool store_memtags (CORE_ADDR arg0, size_t arg1, const gdb::byte_vector &arg2, int arg3) override;
> +  bool is_address_tagged (gdbarch *arg0, CORE_ADDR arg1) override;
>    x86_xsave_layout fetch_x86_xsave_layout () override;
>  };
>  
> @@ -4562,6 +4564,34 @@ debug_target::store_memtags (CORE_ADDR arg0, size_t arg1, const gdb::byte_vector
>    return result;
>  }
>  
> +bool
> +target_ops::is_address_tagged (gdbarch *arg0, CORE_ADDR arg1)
> +{
> +  return this->beneath ()->is_address_tagged (arg0, arg1);
> +}
> +
> +bool
> +dummy_target::is_address_tagged (gdbarch *arg0, CORE_ADDR arg1)
> +{
> +  tcomplain ();
> +}
> +
> +bool
> +debug_target::is_address_tagged (gdbarch *arg0, CORE_ADDR arg1)
> +{
> +  gdb_printf (gdb_stdlog, "-> %s->is_address_tagged (...)\n", this->beneath ()->shortname ());
> +  bool result
> +    = this->beneath ()->is_address_tagged (arg0, arg1);
> +  gdb_printf (gdb_stdlog, "<- %s->is_address_tagged (", this->beneath ()->shortname ());
> +  target_debug_print_gdbarch_p (arg0);
> +  gdb_puts (", ", gdb_stdlog);
> +  target_debug_print_CORE_ADDR (arg1);
> +  gdb_puts (") = ", gdb_stdlog);
> +  target_debug_print_bool (result);
> +  gdb_puts ("\n", gdb_stdlog);
> +  return result;
> +}
> +
>  x86_xsave_layout
>  target_ops::fetch_x86_xsave_layout ()
>  {
> diff --git a/gdb/target.c b/gdb/target.c
> index 107a84b3ca1..5c3c1a57dbd 100644
> --- a/gdb/target.c
> +++ b/gdb/target.c
> @@ -796,6 +796,12 @@ target_store_memtags (CORE_ADDR address, size_t len,
>    return current_inferior ()->top_target ()->store_memtags (address, len, tags, type);
>  }
>  
> +bool
> +target_is_address_tagged (gdbarch *gdbarch, CORE_ADDR address)
> +{
> +  return current_inferior ()->top_target ()->is_address_tagged (gdbarch, address);
> +}
> +
>  x86_xsave_layout
>  target_fetch_x86_xsave_layout ()
>  {
> diff --git a/gdb/target.h b/gdb/target.h
> index c9eaff16346..486a0a687b0 100644
> --- a/gdb/target.h
> +++ b/gdb/target.h
> @@ -1334,6 +1334,10 @@ struct target_ops
>  				const gdb::byte_vector &tags, int type)
>        TARGET_DEFAULT_NORETURN (tcomplain ());
>  
> +    /* Returns true if ADDRESS is tagged, otherwise returns false.  */
> +    virtual bool is_address_tagged (gdbarch *gdbarch, CORE_ADDR address)
> +      TARGET_DEFAULT_NORETURN (tcomplain ());
> +

Should the default for is_address_tagged by to return false, meaning the address is never
tagged?

>      /* Return the x86 XSAVE extended state area layout.  */
>      virtual x86_xsave_layout fetch_x86_xsave_layout ()
>        TARGET_DEFAULT_RETURN (x86_xsave_layout ());
> @@ -2317,6 +2321,8 @@ extern bool target_fetch_memtags (CORE_ADDR address, size_t len,
>  extern bool target_store_memtags (CORE_ADDR address, size_t len,
>  				  const gdb::byte_vector &tags, int type);
>  
> +extern bool target_is_address_tagged (gdbarch *gdbarch, CORE_ADDR address);
> +
>  extern x86_xsave_layout target_fetch_x86_xsave_layout ();
>  
>  /* Command logging facility.  */