Re: [PATCH 1/3] gdb: Make tagged pointer support configurable.

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

Hi,


On 3/27/24 07:47, Schimpe, Christina wrote:
> From: Christina Schimpe <christina.schimpe@intel.com>
> 
> The gdbarch function gdbarch_remove_non_address_bits adjusts addresses to
> enable debugging of programs with tagged pointers on Linux, for instance for
> ARM's feature top byte ignore (TBI).
> Once the function is implemented for an architecture, it adjusts addresses for
> memory access, breakpoints and watchpoints.
> 
> Linear address masking (LAM) is Intel's (R) implementation of tagged
> pointer support.  It requires certain adaptions to GDB's tagged pointer
> support due to the following:
> - LAM supports address tagging for data accesses only.  Thus, specifying
>   breakpoints on tagged addresses is not a valid use case.
> - In contrast to the implementation for ARM's TBI, the kernel supports tagged
>   pointers for memory access.
> 
> This patch makes GDB's tagged pointer support configurable such that it is
> possible to enable the address adjustment for a specific feature only (e.g
> memory access, breakpoints or watchpoints).


Thanks for the series. More of a general question, we're splitting the more general
handling of bit removal from addresses into 3 different categories in gdb. Isn't gdbserver
also affected by this change? At least for aarch64 we have non-address-bits removal.

Do we need to do anything in gdbserver so it can properly handle removal of non-address-bits
on its own, without the help of gdb?

Also, another more general comment is related to the naming of the hooks. I suppose it is down
to personal preference, but spelling out the entire word is likely to make the meaning of a
function/hook clearer. For instance:

gdbarch_remove_non_address_bits -> gdbarch_remove_non_addr_bits_memory

Maybe we should...

gdbarch_remove_non_address_bits -> gdbarch_remove_non_address_bits_memory

The same applies to the other hooks and function names.

wpt -> watchpoint

or

wpt -> hw_watchpoint

bpt -> breakpoint

or

bpt -> hw_breakpoint

> ---
>  gdb/aarch64-linux-nat.c   |  3 +-
>  gdb/aarch64-linux-tdep.c  | 14 +++++----
>  gdb/aarch64-tdep.c        | 12 +++++--
>  gdb/breakpoint.c          |  4 +--
>  gdb/gdbarch-gen.h         | 50 ++++++++++++++++++++++-------
>  gdb/gdbarch.c             | 66 ++++++++++++++++++++++++++++++++-------
>  gdb/gdbarch_components.py | 54 +++++++++++++++++++++++++++-----
>  gdb/target.c              |  4 +--
>  8 files changed, 163 insertions(+), 44 deletions(-)
> 
> diff --git a/gdb/aarch64-linux-nat.c b/gdb/aarch64-linux-nat.c
> index 3face34ce79..bd02a7b2d08 100644
> --- a/gdb/aarch64-linux-nat.c
> +++ b/gdb/aarch64-linux-nat.c
> @@ -959,7 +959,8 @@ aarch64_linux_nat_target::stopped_data_address (CORE_ADDR *addr_p)
>       kernel can potentially be tagged addresses.  */
>    struct gdbarch *gdbarch = thread_architecture (inferior_ptid);
>    const CORE_ADDR addr_trap
> -    = gdbarch_remove_non_address_bits (gdbarch, (CORE_ADDR) siginfo.si_addr);
> +    = gdbarch_remove_non_addr_bits_memory (gdbarch,
> +					   (CORE_ADDR) siginfo.si_addr);
>  
>    /* Check if the address matches any watched address.  */
>    state = aarch64_get_debug_reg_state (inferior_ptid.pid ());
> diff --git a/gdb/aarch64-linux-tdep.c b/gdb/aarch64-linux-tdep.c
> index 0b9784f38e4..d2d42efe305 100644
> --- a/gdb/aarch64-linux-tdep.c
> +++ b/gdb/aarch64-linux-tdep.c
> @@ -2458,7 +2458,7 @@ aarch64_linux_tagged_address_p (struct gdbarch *gdbarch, struct value *address)
>    CORE_ADDR addr = value_as_address (address);
>  
>    /* Remove the top byte for the memory range check.  */
> -  addr = gdbarch_remove_non_address_bits (gdbarch, addr);
> +  addr = gdbarch_remove_non_addr_bits_memory (gdbarch, addr);
>  
>    /* Check if the page that contains ADDRESS is mapped with PROT_MTE.  */
>    if (!linux_address_in_memtag_page (addr))
> @@ -2484,7 +2484,8 @@ aarch64_linux_memtag_matches_p (struct gdbarch *gdbarch,
>  
>    /* Fetch the allocation tag for ADDRESS.  */
>    std::optional<CORE_ADDR> atag
> -    = aarch64_mte_get_atag (gdbarch_remove_non_address_bits (gdbarch, addr));
> +    = aarch64_mte_get_atag (gdbarch_remove_non_addr_bits_memory (gdbarch,
> +								 addr));
>  
>    if (!atag.has_value ())
>      return true;
> @@ -2523,7 +2524,7 @@ aarch64_linux_set_memtags (struct gdbarch *gdbarch, struct value *address,
>    else
>      {
>        /* Remove the top byte.  */
> -      addr = gdbarch_remove_non_address_bits (gdbarch, addr);
> +      addr = gdbarch_remove_non_addr_bits_memory (gdbarch, addr);
>  
>        /* Make sure we are dealing with a tagged address to begin with.  */
>        if (!aarch64_linux_tagged_address_p (gdbarch, address))
> @@ -2580,7 +2581,7 @@ aarch64_linux_get_memtag (struct gdbarch *gdbarch, struct value *address,
>  	return nullptr;
>  
>        /* Remove the top byte.  */
> -      addr = gdbarch_remove_non_address_bits (gdbarch, addr);
> +      addr = gdbarch_remove_non_addr_bits_memory (gdbarch, addr);
>        std::optional<CORE_ADDR> atag = aarch64_mte_get_atag (addr);
>  
>        if (!atag.has_value ())
> @@ -2654,8 +2655,9 @@ aarch64_linux_report_signal_info (struct gdbarch *gdbarch,
>        uiout->text ("\n");
>  
>        std::optional<CORE_ADDR> atag
> -	= aarch64_mte_get_atag (gdbarch_remove_non_address_bits (gdbarch,
> -								 fault_addr));
> +	= aarch64_mte_get_atag (
> +	    gdbarch_remove_non_addr_bits_memory (gdbarch, fault_addr));
> +
>        gdb_byte ltag = aarch64_mte_get_ltag (fault_addr);
>  
>        if (!atag.has_value ())
> diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
> index 545ec872fd8..86d620bd181 100644
> --- a/gdb/aarch64-tdep.c
> +++ b/gdb/aarch64-tdep.c
> @@ -4582,9 +4582,15 @@ aarch64_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
>      tdep->ra_sign_state_regnum = ra_sign_state_offset + num_regs;
>  
>    /* Architecture hook to remove bits of a pointer that are not part of the
> -     address, like memory tags (MTE) and pointer authentication signatures.  */
> -  set_gdbarch_remove_non_address_bits (gdbarch,
> -				       aarch64_remove_non_address_bits);
> +     address, like memory tags (MTE) and pointer authentication signatures.
> +     Configure address adjustment for watch-, breakpoints and memory
> +     transfer.  */
> +  set_gdbarch_remove_non_addr_bits_wpt (gdbarch,
> +					aarch64_remove_non_address_bits);
> +  set_gdbarch_remove_non_addr_bits_bpt (gdbarch,
> +					aarch64_remove_non_address_bits);
> +  set_gdbarch_remove_non_addr_bits_memory (gdbarch,
> +					   aarch64_remove_non_address_bits);
>  
>    /* SME pseudo-registers.  */
>    if (tdep->has_sme ())
> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
> index 053d17df03e..24b322b9bb4 100644
> --- a/gdb/breakpoint.c
> +++ b/gdb/breakpoint.c
> @@ -2234,7 +2234,7 @@ update_watchpoint (struct watchpoint *b, bool reparse)
>  		  loc->gdbarch = v->type ()->arch ();
>  		  loc->pspace = frame_pspace;
>  		  loc->address
> -		    = gdbarch_remove_non_address_bits (loc->gdbarch, addr);
> +		    = gdbarch_remove_non_addr_bits_wpt (loc->gdbarch, addr);
>  		  b->add_location (*loc);
>  
>  		  if (bitsize != 0)
> @@ -7473,7 +7473,7 @@ adjust_breakpoint_address (struct gdbarch *gdbarch,
>  	}
>  
>        adjusted_bpaddr
> -	= gdbarch_remove_non_address_bits (gdbarch, adjusted_bpaddr);
> +	= gdbarch_remove_non_addr_bits_bpt (gdbarch, adjusted_bpaddr);
>  
>        /* An adjusted breakpoint address can significantly alter
>  	 a user's expectations.  Print a warning if an adjustment
> diff --git a/gdb/gdbarch-gen.h b/gdb/gdbarch-gen.h
> index ebcff80bb9e..ec296c86668 100644
> --- a/gdb/gdbarch-gen.h
> +++ b/gdb/gdbarch-gen.h
> @@ -684,19 +684,47 @@ extern CORE_ADDR gdbarch_addr_bits_remove (struct gdbarch *gdbarch, CORE_ADDR ad
>  extern void set_gdbarch_addr_bits_remove (struct gdbarch *gdbarch, gdbarch_addr_bits_remove_ftype *addr_bits_remove);
>  
>  /* On some architectures, not all bits of a pointer are significant.
> -   On AArch64, for example, the top bits of a pointer may carry a "tag", which
> -   can be ignored by the kernel and the hardware.  The "tag" can be regarded as
> -   additional data associated with the pointer, but it is not part of the address.
> +   On AArch64 and amd64, for example, the top bits of a pointer may carry a
> +   "tag", which can be ignored by the kernel and the hardware.  The "tag" can be
> +   regarded as additional data associated with the pointer, but it is not part
> +   of the address.
>  
>     Given a pointer for the architecture, this hook removes all the
> -   non-significant bits and sign-extends things as needed.  It gets used to remove
> -   non-address bits from data pointers (for example, removing the AArch64 MTE tag
> -   bits from a pointer) and from code pointers (removing the AArch64 PAC signature
> -   from a pointer containing the return address). */
> -
> -typedef CORE_ADDR (gdbarch_remove_non_address_bits_ftype) (struct gdbarch *gdbarch, CORE_ADDR pointer);
> -extern CORE_ADDR gdbarch_remove_non_address_bits (struct gdbarch *gdbarch, CORE_ADDR pointer);
> -extern void set_gdbarch_remove_non_address_bits (struct gdbarch *gdbarch, gdbarch_remove_non_address_bits_ftype *remove_non_address_bits);
> +   non-significant bits and sign-extends things as needed.  It gets used to
> +   remove non-address bits from pointers used for watchpoints. */
> +

Do we need to make a distinction between software watchpoint / hardware watchpoints?

> +typedef CORE_ADDR (gdbarch_remove_non_addr_bits_wpt_ftype) (struct gdbarch *gdbarch, CORE_ADDR pointer);
> +extern CORE_ADDR gdbarch_remove_non_addr_bits_wpt (struct gdbarch *gdbarch, CORE_ADDR pointer);
> +extern void set_gdbarch_remove_non_addr_bits_wpt (struct gdbarch *gdbarch, gdbarch_remove_non_addr_bits_wpt_ftype *remove_non_addr_bits_wpt);
> +
> +/* On some architectures, not all bits of a pointer are significant.
> +   On AArch64 and amd64, for example, the top bits of a pointer may carry a
> +   "tag", which can be ignored by the kernel and the hardware.  The "tag" can be
> +   regarded as additional data associated with the pointer, but it is not part
> +   of the address.
> +
> +   Given a pointer for the architecture, this hook removes all the
> +   non-significant bits and sign-extends things as needed.  It gets used to
> +   remove non-address bits from pointers used for breakpoints. */
> +

Similarly, do we need to make a distinction between software breakpoints and hardware breakpoints?

> +typedef CORE_ADDR (gdbarch_remove_non_addr_bits_bpt_ftype) (struct gdbarch *gdbarch, CORE_ADDR pointer);
> +extern CORE_ADDR gdbarch_remove_non_addr_bits_bpt (struct gdbarch *gdbarch, CORE_ADDR pointer);
> +extern void set_gdbarch_remove_non_addr_bits_bpt (struct gdbarch *gdbarch, gdbarch_remove_non_addr_bits_bpt_ftype *remove_non_addr_bits_bpt);
> +
> +/* On some architectures, not all bits of a pointer are significant.
> +   On AArch64 and amd64, for example, the top bits of a pointer may carry a
> +   "tag", which can be ignored by the kernel and the hardware.  The "tag" can be
> +   regarded as additional data associated with the pointer, but it is not part
> +   of the address.
> +
> +   Given a pointer for the architecture, this hook removes all the
> +   non-significant bits and sign-extends things as needed.  It gets used to
> +   remove non-address bits from any pointer used to access memory (called in
> +   memory_xfer_partial). */
> +
> +typedef CORE_ADDR (gdbarch_remove_non_addr_bits_memory_ftype) (struct gdbarch *gdbarch, CORE_ADDR pointer);
> +extern CORE_ADDR gdbarch_remove_non_addr_bits_memory (struct gdbarch *gdbarch, CORE_ADDR pointer);
> +extern void set_gdbarch_remove_non_addr_bits_memory (struct gdbarch *gdbarch, gdbarch_remove_non_addr_bits_memory_ftype *remove_non_addr_bits_memory);
>  
>  /* Return a string representation of the memory tag TAG. */
>  
> diff --git a/gdb/gdbarch.c b/gdb/gdbarch.c
> index 9319571deba..f59e090d294 100644
> --- a/gdb/gdbarch.c
> +++ b/gdb/gdbarch.c
> @@ -143,7 +143,9 @@ struct gdbarch
>    int frame_red_zone_size = 0;
>    gdbarch_convert_from_func_ptr_addr_ftype *convert_from_func_ptr_addr = convert_from_func_ptr_addr_identity;
>    gdbarch_addr_bits_remove_ftype *addr_bits_remove = core_addr_identity;
> -  gdbarch_remove_non_address_bits_ftype *remove_non_address_bits = default_remove_non_address_bits;
> +  gdbarch_remove_non_addr_bits_wpt_ftype *remove_non_addr_bits_wpt = default_remove_non_address_bits;
> +  gdbarch_remove_non_addr_bits_bpt_ftype *remove_non_addr_bits_bpt = default_remove_non_address_bits;
> +  gdbarch_remove_non_addr_bits_memory_ftype *remove_non_addr_bits_memory = default_remove_non_address_bits;
>    gdbarch_memtag_to_string_ftype *memtag_to_string = default_memtag_to_string;
>    gdbarch_tagged_address_p_ftype *tagged_address_p = default_tagged_address_p;
>    gdbarch_memtag_matches_p_ftype *memtag_matches_p = default_memtag_matches_p;
> @@ -407,7 +409,9 @@ verify_gdbarch (struct gdbarch *gdbarch)
>    /* Skip verify of frame_red_zone_size, invalid_p == 0 */
>    /* Skip verify of convert_from_func_ptr_addr, invalid_p == 0 */
>    /* Skip verify of addr_bits_remove, invalid_p == 0 */
> -  /* Skip verify of remove_non_address_bits, invalid_p == 0 */
> +  /* Skip verify of remove_non_addr_bits_wpt, invalid_p == 0 */
> +  /* Skip verify of remove_non_addr_bits_bpt, invalid_p == 0 */
> +  /* Skip verify of remove_non_addr_bits_memory, invalid_p == 0 */
>    /* Skip verify of memtag_to_string, invalid_p == 0 */
>    /* Skip verify of tagged_address_p, invalid_p == 0 */
>    /* Skip verify of memtag_matches_p, invalid_p == 0 */
> @@ -910,8 +914,14 @@ gdbarch_dump (struct gdbarch *gdbarch, struct ui_file *file)
>  	      "gdbarch_dump: addr_bits_remove = <%s>\n",
>  	      host_address_to_string (gdbarch->addr_bits_remove));
>    gdb_printf (file,
> -	      "gdbarch_dump: remove_non_address_bits = <%s>\n",
> -	      host_address_to_string (gdbarch->remove_non_address_bits));
> +	      "gdbarch_dump: remove_non_addr_bits_wpt = <%s>\n",
> +	      host_address_to_string (gdbarch->remove_non_addr_bits_wpt));
> +  gdb_printf (file,
> +	      "gdbarch_dump: remove_non_addr_bits_bpt = <%s>\n",
> +	      host_address_to_string (gdbarch->remove_non_addr_bits_bpt));
> +  gdb_printf (file,
> +	      "gdbarch_dump: remove_non_addr_bits_memory = <%s>\n",
> +	      host_address_to_string (gdbarch->remove_non_addr_bits_memory));
>    gdb_printf (file,
>  	      "gdbarch_dump: memtag_to_string = <%s>\n",
>  	      host_address_to_string (gdbarch->memtag_to_string));
> @@ -3198,20 +3208,54 @@ set_gdbarch_addr_bits_remove (struct gdbarch *gdbarch,
>  }
>  
>  CORE_ADDR
> -gdbarch_remove_non_address_bits (struct gdbarch *gdbarch, CORE_ADDR pointer)
> +gdbarch_remove_non_addr_bits_wpt (struct gdbarch *gdbarch, CORE_ADDR pointer)
> +{
> +  gdb_assert (gdbarch != NULL);
> +  gdb_assert (gdbarch->remove_non_addr_bits_wpt != NULL);
> +  if (gdbarch_debug >= 2)
> +    gdb_printf (gdb_stdlog, "gdbarch_remove_non_addr_bits_wpt called\n");
> +  return gdbarch->remove_non_addr_bits_wpt (gdbarch, pointer);
> +}
> +
> +void
> +set_gdbarch_remove_non_addr_bits_wpt (struct gdbarch *gdbarch,
> +				      gdbarch_remove_non_addr_bits_wpt_ftype remove_non_addr_bits_wpt)
> +{
> +  gdbarch->remove_non_addr_bits_wpt = remove_non_addr_bits_wpt;
> +}
> +
> +CORE_ADDR
> +gdbarch_remove_non_addr_bits_bpt (struct gdbarch *gdbarch, CORE_ADDR pointer)
> +{
> +  gdb_assert (gdbarch != NULL);
> +  gdb_assert (gdbarch->remove_non_addr_bits_bpt != NULL);
> +  if (gdbarch_debug >= 2)
> +    gdb_printf (gdb_stdlog, "gdbarch_remove_non_addr_bits_bpt called\n");
> +  return gdbarch->remove_non_addr_bits_bpt (gdbarch, pointer);
> +}
> +
> +void
> +set_gdbarch_remove_non_addr_bits_bpt (struct gdbarch *gdbarch,
> +				      gdbarch_remove_non_addr_bits_bpt_ftype remove_non_addr_bits_bpt)
> +{
> +  gdbarch->remove_non_addr_bits_bpt = remove_non_addr_bits_bpt;
> +}
> +
> +CORE_ADDR
> +gdbarch_remove_non_addr_bits_memory (struct gdbarch *gdbarch, CORE_ADDR pointer)
>  {
>    gdb_assert (gdbarch != NULL);
> -  gdb_assert (gdbarch->remove_non_address_bits != NULL);
> +  gdb_assert (gdbarch->remove_non_addr_bits_memory != NULL);
>    if (gdbarch_debug >= 2)
> -    gdb_printf (gdb_stdlog, "gdbarch_remove_non_address_bits called\n");
> -  return gdbarch->remove_non_address_bits (gdbarch, pointer);
> +    gdb_printf (gdb_stdlog, "gdbarch_remove_non_addr_bits_memory called\n");
> +  return gdbarch->remove_non_addr_bits_memory (gdbarch, pointer);
>  }
>  
>  void
> -set_gdbarch_remove_non_address_bits (struct gdbarch *gdbarch,
> -				     gdbarch_remove_non_address_bits_ftype remove_non_address_bits)
> +set_gdbarch_remove_non_addr_bits_memory (struct gdbarch *gdbarch,
> +					 gdbarch_remove_non_addr_bits_memory_ftype remove_non_addr_bits_memory)
>  {
> -  gdbarch->remove_non_address_bits = remove_non_address_bits;
> +  gdbarch->remove_non_addr_bits_memory = remove_non_addr_bits_memory;
>  }
>  
>  std::string
> diff --git a/gdb/gdbarch_components.py b/gdb/gdbarch_components.py
> index 7d913ade621..555bc4707c5 100644
> --- a/gdb/gdbarch_components.py
> +++ b/gdb/gdbarch_components.py
> @@ -1232,18 +1232,56 @@ possible it should be in TARGET_READ_PC instead).
>  Method(
>      comment="""
>  On some architectures, not all bits of a pointer are significant.
> -On AArch64, for example, the top bits of a pointer may carry a "tag", which
> -can be ignored by the kernel and the hardware.  The "tag" can be regarded as
> -additional data associated with the pointer, but it is not part of the address.
> +On AArch64 and amd64, for example, the top bits of a pointer may carry a
> +"tag", which can be ignored by the kernel and the hardware.  The "tag" can be
> +regarded as additional data associated with the pointer, but it is not part
> +of the address.
>  
>  Given a pointer for the architecture, this hook removes all the
> -non-significant bits and sign-extends things as needed.  It gets used to remove
> -non-address bits from data pointers (for example, removing the AArch64 MTE tag
> -bits from a pointer) and from code pointers (removing the AArch64 PAC signature
> -from a pointer containing the return address).
> +non-significant bits and sign-extends things as needed.  It gets used to
> +remove non-address bits from pointers used for watchpoints.
>  """,
>      type="CORE_ADDR",
> -    name="remove_non_address_bits",
> +    name="remove_non_addr_bits_wpt",
> +    params=[("CORE_ADDR", "pointer")],
> +    predefault="default_remove_non_address_bits",
> +    invalid=False,
> +)
> +
> +Method(
> +    comment="""
> +On some architectures, not all bits of a pointer are significant.
> +On AArch64 and amd64, for example, the top bits of a pointer may carry a
> +"tag", which can be ignored by the kernel and the hardware.  The "tag" can be
> +regarded as additional data associated with the pointer, but it is not part
> +of the address.
> +
> +Given a pointer for the architecture, this hook removes all the
> +non-significant bits and sign-extends things as needed.  It gets used to
> +remove non-address bits from pointers used for breakpoints.
> +""",
> +    type="CORE_ADDR",
> +    name="remove_non_addr_bits_bpt",
> +    params=[("CORE_ADDR", "pointer")],
> +    predefault="default_remove_non_address_bits",
> +    invalid=False,
> +)
> +
> +Method(
> +    comment="""
> +On some architectures, not all bits of a pointer are significant.
> +On AArch64 and amd64, for example, the top bits of a pointer may carry a
> +"tag", which can be ignored by the kernel and the hardware.  The "tag" can be
> +regarded as additional data associated with the pointer, but it is not part
> +of the address.
> +
> +Given a pointer for the architecture, this hook removes all the
> +non-significant bits and sign-extends things as needed.  It gets used to
> +remove non-address bits from any pointer used to access memory (called in
> +memory_xfer_partial).
> +""",
> +    type="CORE_ADDR",
> +    name="remove_non_addr_bits_memory",
>      params=[("CORE_ADDR", "pointer")],
>      predefault="default_remove_non_address_bits",
>      invalid=False,
> diff --git a/gdb/target.c b/gdb/target.c
> index 107a84b3ca1..586eee2ee73 100644
> --- a/gdb/target.c
> +++ b/gdb/target.c
> @@ -1597,8 +1597,8 @@ memory_xfer_partial (struct target_ops *ops, enum target_object object,
>    if (len == 0)
>      return TARGET_XFER_EOF;
>  
> -  memaddr = gdbarch_remove_non_address_bits (current_inferior ()->arch (),
> -					     memaddr);
> +  memaddr = gdbarch_remove_non_addr_bits_memory (current_inferior ()->arch (),
> +				  		 memaddr);
>  
>    /* Fill in READBUF with breakpoint shadows, or WRITEBUF with
>       breakpoint insns, thus hiding out from higher layers whether