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

["Willgerodt, Felix" <felix.willgerodt@intel.com>]

> -----Original Message-----
> From: Schimpe, Christina <christina.schimpe@intel.com>
> Sent: Mittwoch, 27. März 2024 08:48
> To: gdb-patches@sourceware.org
> Cc: Schimpe, Christina <christina.schimpe@intel.com>
> Subject: [PATCH 1/3] gdb: Make tagged pointer support configurable.
> 
> 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.

Should we write the "Linux kernel"?

I feel like one of the main reasons for this patch is missing. That on x86
(or some architectures in general) we want to avoid the overhead of doing
this unconditionally. E.g. as we don't want to parse a file repeatedly on x86.


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

I wouldn't mention memory_xfer_partial, just to avoid having to keep the
name up-to-date in this comment. It is easy enough to find the usages.


> +""",
> +    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);

When I apply your patch I get this warning here:

.git/rebase-apply/patch:372: space before tab in indent.
                                                 memaddr);

That should be fixed.

And in general, I can't apply this to the current master anymore:

error: patch failed: gdb/aarch64-linux-tdep.c:2458
error: gdb/aarch64-linux-tdep.c: patch does not apply
error: patch failed: gdb/breakpoint.c:2234
error: gdb/breakpoint.c: patch does not apply
Patch failed at 0001 gdb: Make tagged pointer support configurable.

On an older commit it applied just fine.
I didn't check if the conflicts are trivial or require a new round of reviews.

Thanks,
Felix
Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928