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

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

> -----Original Message-----
> From: Schimpe, Christina <christina.schimpe@intel.com>
> Sent: Montag, 27. Mai 2024 12:24
> To: gdb-patches@sourceware.org
> Cc: Willgerodt, Felix <felix.willgerodt@intel.com>; eliz@gnu.org;
> luis.machado@arm.com
> Subject: [PATCH v2 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 Linux 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).  This way, one can make sure
> that addresses are only adjusted when necessary.  In case of LAM, this
> avoids unnecessary parsing of the /proc/<pid>/status file to get the
> untag mask.
> ---
>  gdb/aarch64-linux-nat.c   |  2 +-
>  gdb/aarch64-linux-tdep.c  | 13 ++++----
>  gdb/aarch64-tdep.c        | 17 +++++-----
>  gdb/aarch64-tdep.h        |  6 ++++
>  gdb/breakpoint.c          |  5 +--
>  gdb/gdbarch-gen.h         | 49 ++++++++++++++++++++++-------
>  gdb/gdbarch.c             | 66 ++++++++++++++++++++++++++++++++-------
>  gdb/gdbarch_components.py | 53 ++++++++++++++++++++++++++-----
>  gdb/target.c              |  3 +-
>  9 files changed, 167 insertions(+), 47 deletions(-)
> 
> diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
> index 8d0553f3d7c..af62d3ae1ec 100644
> --- a/gdb/aarch64-tdep.c
> +++ b/gdb/aarch64-tdep.c
> @@ -4086,10 +4086,7 @@ aarch64_stack_frame_destroyed_p (struct
> gdbarch *gdbarch, CORE_ADDR pc)
>    return streq (inst.opcode->name, "ret");
>  }
> 
> -/* AArch64 implementation of the remove_non_address_bits gdbarch hook.
> Remove
> -   non address bits from a pointer value.  */
> -
> -static CORE_ADDR
> +CORE_ADDR
>  aarch64_remove_non_address_bits (struct gdbarch *gdbarch, CORE_ADDR
> pointer)

Shouldn't there still be some sort of comment for this function in the c file?
At least some "see header file"? Though it seems like no function in aarch64-tdep.h
has any comment, and all comments are in aarch64-tdep.c. I would also be fine
with that for consistency. Maybe Luis can comment how he prefers it.

But I am fine with this patch. Let's see if someone else objects this split.

Reviewed-By: Felix Willgerodt <felix.willgerodt@intel.com>

Thanks,
Felix
Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Sean Fennelly, Jeffrey Schneiderman, Tiffany Doon Silva
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928