[PATCH v2 0/3] Add amd64 LAM watchpoint support

[PATCH v2 0/3] Add amd64 LAM watchpoint support

[Schimpe, Christina]

Hi all,

this is my v2 of the series "Add amd64 LAM watchpoint support".
It implements the feedback of Luis and Felix. The NEWS part has already
been approved by Eli.

Also I noticed that the gdbarch function 'gdbarch_remove_non_addr_bits_memory'
should not be used in several functions of aarch64* files (patch 1) due
to the following:

The function description for 'gdbarch_remove_non_addr_bits_memory' states:

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

We don't know if the pointer passed to 'gdbarch_remove_non_addr_bits_memory'
by 'gdbarch_tagged_address_p' will be used to access memory.  That's why it's
wrong to call this function in 'gdbarch_tagged_address_p'.

There are several similar wrong calls of 'gdbarch_remove_non_addr_bits_memory'
in the changes of patch 1.  I replaced all of them with
'aarch64_remove_non_address_bits'.

Changes since V1:
 * Replace gdbarch_remove_non_addr_bits_memory with
   aarch64_remove_non_address_bits in aarch64* files.
 * Improve comments and commit messages.
 * Move 'allow_lam_tests' procedure in gdb.exp to the correct location.
 * Remove abbreviations in gdbarch function names.
 * Update copyright years.

V1 of this series can be found here:
https://sourceware.org/pipermail/gdb-patches/2024-March/207589.html

I am looking forward to your feedback.

Thanks + Best Regards,
Christina


Christina Schimpe (3):
  gdb: Make tagged pointer support configurable.
  LAM: Enable tagged pointer support for watchpoints.
  LAM: Support kernel space debugging

 gdb/NEWS                             |  2 +
 gdb/aarch64-linux-nat.c              |  2 +-
 gdb/aarch64-linux-tdep.c             | 13 ++---
 gdb/aarch64-tdep.c                   | 17 ++++---
 gdb/aarch64-tdep.h                   |  6 +++
 gdb/amd64-linux-tdep.c               | 75 ++++++++++++++++++++++++++++
 gdb/breakpoint.c                     |  5 +-
 gdb/gdbarch-gen.h                    | 49 ++++++++++++++----
 gdb/gdbarch.c                        | 66 ++++++++++++++++++++----
 gdb/gdbarch_components.py            | 53 +++++++++++++++++---
 gdb/target.c                         |  3 +-
 gdb/testsuite/gdb.arch/amd64-lam.c   | 49 ++++++++++++++++++
 gdb/testsuite/gdb.arch/amd64-lam.exp | 46 +++++++++++++++++
 gdb/testsuite/lib/gdb.exp            | 63 +++++++++++++++++++++++
 14 files changed, 402 insertions(+), 47 deletions(-)
 create mode 100755 gdb/testsuite/gdb.arch/amd64-lam.c
 create mode 100644 gdb/testsuite/gdb.arch/amd64-lam.exp

-- 
2.34.1

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

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

[Schimpe, Christina]

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-linux-nat.c b/gdb/aarch64-linux-nat.c
index 4b2a0ba9f7b..ec07a897094 100644
--- a/gdb/aarch64-linux-nat.c
+++ b/gdb/aarch64-linux-nat.c
@@ -961,7 +961,7 @@ 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);
+    = aarch64_remove_non_address_bits (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 ad55cf2caa3..db531ecc9f2 100644
--- a/gdb/aarch64-linux-tdep.c
+++ b/gdb/aarch64-linux-tdep.c
@@ -2455,7 +2455,7 @@ static bool
 aarch64_linux_tagged_address_p (struct gdbarch *gdbarch, CORE_ADDR address)
 {
   /* Remove the top byte for the memory range check.  */
-  address = gdbarch_remove_non_address_bits (gdbarch, address);
+  address = aarch64_remove_non_address_bits (gdbarch, address);
 
   /* Check if the page that contains ADDRESS is mapped with PROT_MTE.  */
   if (!linux_address_in_memtag_page (address))
@@ -2477,7 +2477,7 @@ 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 (aarch64_remove_non_address_bits (gdbarch, addr));
 
   if (!atag.has_value ())
     return true;
@@ -2516,7 +2516,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 = aarch64_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:
@@ -2565,7 +2565,7 @@ aarch64_linux_get_memtag (struct gdbarch *gdbarch, struct value *address,
   else
     {
       /* Remove the top byte.  */
-      addr = gdbarch_remove_non_address_bits (gdbarch, addr);
+      addr = aarch64_remove_non_address_bits (gdbarch, addr);
       std::optional<CORE_ADDR> atag = aarch64_mte_get_atag (addr);
 
       if (!atag.has_value ())
@@ -2639,8 +2639,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 (
+	    aarch64_remove_non_address_bits (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 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)
 {
   /* By default, we assume TBI and discard the top 8 bits plus the VA range
@@ -4583,9 +4580,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_address_bits_watchpoint
+    (gdbarch, aarch64_remove_non_address_bits);
+  set_gdbarch_remove_non_address_bits_breakpoint
+    (gdbarch, aarch64_remove_non_address_bits);
+  set_gdbarch_remove_non_address_bits_memory
+    (gdbarch, aarch64_remove_non_address_bits);
 
   /* SME pseudo-registers.  */
   if (tdep->has_sme ())
diff --git a/gdb/aarch64-tdep.h b/gdb/aarch64-tdep.h
index 0e6024bfcbc..a9c8815f8be 100644
--- a/gdb/aarch64-tdep.h
+++ b/gdb/aarch64-tdep.h
@@ -203,4 +203,10 @@ void aarch64_displaced_step_fixup (struct gdbarch *gdbarch,
 
 bool aarch64_displaced_step_hw_singlestep (struct gdbarch *gdbarch);
 
+/* AArch64 implementation of the remove_non_address_bits gdbarch hooks.
+   Remove non address bits from a pointer value.  */
+
+CORE_ADDR aarch64_remove_non_address_bits (struct gdbarch *gdbarch,
+					   CORE_ADDR pointer);
+
 #endif /* aarch64-tdep.h */
diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 5653842ce76..a59c7e3bb8b 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -2238,7 +2238,8 @@ update_watchpoint (struct watchpoint *b, bool reparse)
 		  loc->gdbarch = v->type ()->arch ();
 		  loc->pspace = wp_pspace;
 		  loc->address
-		    = gdbarch_remove_non_address_bits (loc->gdbarch, addr);
+		    = gdbarch_remove_non_address_bits_watchpoint (loc->gdbarch,
+								  addr);
 		  b->add_location (*loc);
 
 		  if (bitsize != 0)
@@ -7477,7 +7478,7 @@ adjust_breakpoint_address (struct gdbarch *gdbarch,
 	}
 
       adjusted_bpaddr
-	= gdbarch_remove_non_address_bits (gdbarch, adjusted_bpaddr);
+	= gdbarch_remove_non_address_bits_breakpoint (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 b982fd7cd09..9fda85f860f 100644
--- a/gdb/gdbarch-gen.h
+++ b/gdb/gdbarch-gen.h
@@ -684,19 +684,46 @@ 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. */
+
+typedef CORE_ADDR (gdbarch_remove_non_address_bits_watchpoint_ftype) (struct gdbarch *gdbarch, CORE_ADDR pointer);
+extern CORE_ADDR gdbarch_remove_non_address_bits_watchpoint (struct gdbarch *gdbarch, CORE_ADDR pointer);
+extern void set_gdbarch_remove_non_address_bits_watchpoint (struct gdbarch *gdbarch, gdbarch_remove_non_address_bits_watchpoint_ftype *remove_non_address_bits_watchpoint);
+
+/* 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. */
+
+typedef CORE_ADDR (gdbarch_remove_non_address_bits_breakpoint_ftype) (struct gdbarch *gdbarch, CORE_ADDR pointer);
+extern CORE_ADDR gdbarch_remove_non_address_bits_breakpoint (struct gdbarch *gdbarch, CORE_ADDR pointer);
+extern void set_gdbarch_remove_non_address_bits_breakpoint (struct gdbarch *gdbarch, gdbarch_remove_non_address_bits_breakpoint_ftype *remove_non_address_bits_breakpoint);
+
+/* 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. */
+
+typedef CORE_ADDR (gdbarch_remove_non_address_bits_memory_ftype) (struct gdbarch *gdbarch, CORE_ADDR pointer);
+extern CORE_ADDR gdbarch_remove_non_address_bits_memory (struct gdbarch *gdbarch, CORE_ADDR pointer);
+extern void set_gdbarch_remove_non_address_bits_memory (struct gdbarch *gdbarch, gdbarch_remove_non_address_bits_memory_ftype *remove_non_address_bits_memory);
 
 /* Return a string representation of the memory tag TAG. */
 
diff --git a/gdb/gdbarch.c b/gdb/gdbarch.c
index 58e9ebbdc59..99a54a38d61 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_address_bits_watchpoint_ftype *remove_non_address_bits_watchpoint = default_remove_non_address_bits;
+  gdbarch_remove_non_address_bits_breakpoint_ftype *remove_non_address_bits_breakpoint = default_remove_non_address_bits;
+  gdbarch_remove_non_address_bits_memory_ftype *remove_non_address_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_address_bits_watchpoint, invalid_p == 0 */
+  /* Skip verify of remove_non_address_bits_breakpoint, invalid_p == 0 */
+  /* Skip verify of remove_non_address_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_address_bits_watchpoint = <%s>\n",
+	      host_address_to_string (gdbarch->remove_non_address_bits_watchpoint));
+  gdb_printf (file,
+	      "gdbarch_dump: remove_non_address_bits_breakpoint = <%s>\n",
+	      host_address_to_string (gdbarch->remove_non_address_bits_breakpoint));
+  gdb_printf (file,
+	      "gdbarch_dump: remove_non_address_bits_memory = <%s>\n",
+	      host_address_to_string (gdbarch->remove_non_address_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_address_bits_watchpoint (struct gdbarch *gdbarch, CORE_ADDR pointer)
+{
+  gdb_assert (gdbarch != NULL);
+  gdb_assert (gdbarch->remove_non_address_bits_watchpoint != NULL);
+  if (gdbarch_debug >= 2)
+    gdb_printf (gdb_stdlog, "gdbarch_remove_non_address_bits_watchpoint called\n");
+  return gdbarch->remove_non_address_bits_watchpoint (gdbarch, pointer);
+}
+
+void
+set_gdbarch_remove_non_address_bits_watchpoint (struct gdbarch *gdbarch,
+						gdbarch_remove_non_address_bits_watchpoint_ftype remove_non_address_bits_watchpoint)
+{
+  gdbarch->remove_non_address_bits_watchpoint = remove_non_address_bits_watchpoint;
+}
+
+CORE_ADDR
+gdbarch_remove_non_address_bits_breakpoint (struct gdbarch *gdbarch, CORE_ADDR pointer)
+{
+  gdb_assert (gdbarch != NULL);
+  gdb_assert (gdbarch->remove_non_address_bits_breakpoint != NULL);
+  if (gdbarch_debug >= 2)
+    gdb_printf (gdb_stdlog, "gdbarch_remove_non_address_bits_breakpoint called\n");
+  return gdbarch->remove_non_address_bits_breakpoint (gdbarch, pointer);
+}
+
+void
+set_gdbarch_remove_non_address_bits_breakpoint (struct gdbarch *gdbarch,
+						gdbarch_remove_non_address_bits_breakpoint_ftype remove_non_address_bits_breakpoint)
+{
+  gdbarch->remove_non_address_bits_breakpoint = remove_non_address_bits_breakpoint;
+}
+
+CORE_ADDR
+gdbarch_remove_non_address_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_address_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_address_bits_memory called\n");
+  return gdbarch->remove_non_address_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_address_bits_memory (struct gdbarch *gdbarch,
+					    gdbarch_remove_non_address_bits_memory_ftype remove_non_address_bits_memory)
 {
-  gdbarch->remove_non_address_bits = remove_non_address_bits;
+  gdbarch->remove_non_address_bits_memory = remove_non_address_bits_memory;
 }
 
 std::string
diff --git a/gdb/gdbarch_components.py b/gdb/gdbarch_components.py
index 4006380076d..cc7c6d8677b 100644
--- a/gdb/gdbarch_components.py
+++ b/gdb/gdbarch_components.py
@@ -1232,18 +1232,55 @@ 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_address_bits_watchpoint",
+    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_address_bits_breakpoint",
+    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.
+""",
+    type="CORE_ADDR",
+    name="remove_non_address_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 a5c92a8a511..dac858c6b07 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -1607,7 +1607,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
+   = gdbarch_remove_non_address_bits_memory (current_inferior ()->arch (),
 					     memaddr);
 
   /* Fill in READBUF with breakpoint shadows, or WRITEBUF with
-- 
2.34.1

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

[PATCH v2 2/3] LAM: Enable tagged pointer support for watchpoints.

[Schimpe, Christina]

From: Christina Schimpe <christina.schimpe@intel.com>

The Intel (R) linear address masking (LAM) feature modifies the checking
applied to 64-bit linear addresses.  With this so-called "modified
canonicality check" the processor masks the metadata bits in a pointer
before using it as a linear address.  LAM supports two different modes that
differ regarding which pointer bits are masked and can be used for
metadata: LAM 48 resulting in a LAM width of 15 and LAM 57 resulting in a
LAM width of 6.

This patch adjusts watchpoint addresses based on the currently enabled
LAM mode using the untag mask provided in the /proc/<pid>/status file.
As LAM can be enabled at runtime or as the configuration may change
when entering an enclave, GDB checks enablement state each time a watchpoint
is updated.

In contrast to the patch implemented for ARM's Top Byte Ignore "Clear
non-significant bits of address on memory access", it is not necessary to
adjust addresses before they are passed to the target layer cache, as
for LAM tagged pointers are supported by the system call to read memory.
Additionally, LAM applies only to addresses used for data accesses.
Thus, it is sufficient to mask addresses used for watchpoints.

The following examples are based on a LAM57 enabled program.
Before this patch tagged pointers were not supported for watchpoints:
~~~
(gdb) print pi_tagged
$2 = (int *) 0x10007ffffffffe004
(gdb) watch *pi_tagged
Hardware watchpoint 2: *pi_tagged
(gdb) c
Continuing.
Couldn't write debug register: Invalid argument.
~~~~

Once LAM 48 or LAM 57 is enabled for the current program, GDB can now
specify watchpoints for tagged addresses with LAM width 15 or 6,
respectively.
---
 gdb/NEWS                             |  2 +
 gdb/amd64-linux-tdep.c               | 61 +++++++++++++++++++++++++++
 gdb/testsuite/gdb.arch/amd64-lam.c   | 49 ++++++++++++++++++++++
 gdb/testsuite/gdb.arch/amd64-lam.exp | 46 ++++++++++++++++++++
 gdb/testsuite/lib/gdb.exp            | 63 ++++++++++++++++++++++++++++
 5 files changed, 221 insertions(+)
 create mode 100755 gdb/testsuite/gdb.arch/amd64-lam.c
 create mode 100644 gdb/testsuite/gdb.arch/amd64-lam.exp

diff --git a/gdb/NEWS b/gdb/NEWS
index 408d1509234..6303c334fd4 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -3,6 +3,8 @@
 
 *** Changes since GDB 15
 
+* GDB now supports watchpoints for tagged data pointers on amd64.
+
 *** Changes in GDB 15
 
 * The MPX commands "show/set mpx bound" have been deprecated, as Intel
diff --git a/gdb/amd64-linux-tdep.c b/gdb/amd64-linux-tdep.c
index c52b0436872..b19b12842fd 100644
--- a/gdb/amd64-linux-tdep.c
+++ b/gdb/amd64-linux-tdep.c
@@ -42,6 +42,7 @@
 #include "arch/amd64.h"
 #include "target-descriptions.h"
 #include "expop.h"
+#include "inferior.h"
 
 /* The syscall's XML filename for i386.  */
 #define XML_SYSCALL_FILENAME_AMD64 "syscalls/amd64-linux.xml"
@@ -49,6 +50,10 @@
 #include "record-full.h"
 #include "linux-record.h"
 
+#include <sstream>
+
+#define DEFAULT_TAG_MASK 0xffffffffffffffffULL
+
 /* Mapping between the general-purpose registers in `struct user'
    format and GDB's register cache layout.  */
 
@@ -1795,6 +1800,59 @@ amd64_dtrace_parse_probe_argument (struct gdbarch *gdbarch,
     }
 }
 
+/* Extract the untagging mask based on the currently active linear address
+   masking (LAM) mode, which is stored in the /proc/<pid>/status file.
+   If we cannot extract the untag mask (for example, if we don't have
+   execution), we assume address tagging is not enabled and return the
+   DEFAULT_TAG_MASK.  */
+
+static CORE_ADDR
+amd64_linux_lam_untag_mask ()
+{
+  if (!target_has_execution ())
+    return DEFAULT_TAG_MASK;
+
+  inferior *inf = current_inferior ();
+  if (inf->fake_pid_p)
+    return DEFAULT_TAG_MASK;
+
+  const std::string filename = string_printf ("/proc/%d/status", inf->pid);
+  gdb::unique_xmalloc_ptr<char> status_file
+    = target_fileio_read_stralloc (nullptr, filename.c_str ());
+
+  if (status_file == nullptr)
+    return DEFAULT_TAG_MASK;
+
+  /* Parse the status file line-by-line and look for the untag mask.  */
+  std::istringstream strm_status_file (status_file.get ());
+  std::string line;
+  const std::string untag_mask_str ("untag_mask:\t");
+  while (std::getline (strm_status_file, line))
+    {
+      const size_t found = line.find (untag_mask_str);
+      if (found != std::string::npos)
+	{
+	  const size_t tag_length = untag_mask_str.length();
+	  return std::strtoul (&line[found + tag_length], nullptr, 0);
+	}
+    }
+
+   return DEFAULT_TAG_MASK;
+}
+
+/* Adjust watchpoint address based on the currently active linear address
+   masking (LAM) mode using the untag mask.  Check each time for a new
+   mask, as LAM is enabled at runtime.  */
+
+static CORE_ADDR
+amd64_linux_remove_non_address_bits_watchpoint (gdbarch *gdbarch,
+						CORE_ADDR addr)
+{
+  /* Clear insignificant bits of a target address using the untag
+     mask.  */
+  return (addr & amd64_linux_lam_untag_mask ());
+}
+
 static void
 amd64_linux_init_abi_common(struct gdbarch_info info, struct gdbarch *gdbarch,
 			    int num_disp_step_buffers)
@@ -1849,6 +1907,9 @@ amd64_linux_init_abi_common(struct gdbarch_info info, struct gdbarch *gdbarch,
 
   set_gdbarch_get_siginfo_type (gdbarch, x86_linux_get_siginfo_type);
   set_gdbarch_report_signal_info (gdbarch, i386_linux_report_signal_info);
+
+  set_gdbarch_remove_non_address_bits_watchpoint
+    (gdbarch, amd64_linux_remove_non_address_bits_watchpoint);
 }
 
 static void
diff --git a/gdb/testsuite/gdb.arch/amd64-lam.c b/gdb/testsuite/gdb.arch/amd64-lam.c
new file mode 100755
index 00000000000..0fe2bc6c2ad
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/amd64-lam.c
@@ -0,0 +1,49 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+ Copyright 2023-2024 Free Software Foundation, Inc.
+
+ This program is free software; you can redistribute it and/or modify
+ it under the terms of the GNU General Public License as published by
+ the Free Software Foundation; either version 3 of the License, or
+ (at your option) any later version.
+
+ This program is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ GNU General Public License for more details.
+
+ You should have received a copy of the GNU General Public License
+ along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#define _GNU_SOURCE
+#include <stdint.h>
+#include <unistd.h>
+#include <stdlib.h>
+#include <sys/syscall.h>
+#include <assert.h>
+#include <errno.h>
+#include <asm/prctl.h>
+
+int
+main (int argc, char **argv)
+{
+  int i;
+  int* pi = &i;
+  int* pi_tagged;
+
+  /* Enable LAM 57.  */
+  errno = 0;
+  syscall (SYS_arch_prctl, ARCH_ENABLE_TAGGED_ADDR, 6);
+  assert_perror (errno);
+
+  /* Add tagging at bit 61.  */
+  pi_tagged = (int *) ((uintptr_t) pi | (1LL << 60));
+
+  i = 0; /* Breakpoint here.  */
+  *pi = 1;
+  *pi_tagged = 2;
+  *pi = 3;
+  *pi_tagged = 4;
+
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.arch/amd64-lam.exp b/gdb/testsuite/gdb.arch/amd64-lam.exp
new file mode 100644
index 00000000000..0bcbb639b66
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/amd64-lam.exp
@@ -0,0 +1,46 @@
+# Copyright 2023-2024 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# Test Linear Address Masking (LAM) support.
+
+require allow_lam_tests
+
+standard_testfile amd64-lam.c
+
+# Test LAM 57.
+if { [prepare_for_testing "failed to prepare" ${testfile} ${srcfile}] } {
+    return -1
+}
+
+if { ![runto_main] } {
+    return -1
+}
+
+gdb_breakpoint [gdb_get_line_number "Breakpoint here"]
+gdb_continue_to_breakpoint "Breakpoint here"
+
+# Test hw watchpoint for a tagged and an untagged address with hit on a
+# tagged and an untagged address each.
+
+foreach symbol {"pi" "pi_tagged"} {
+    gdb_test "watch *${symbol}"
+    gdb_test "continue" \
+	"Continuing\\..*Hardware watchpoint \[0-9\]+.*" \
+	"run until watchpoint on ${symbol}"
+    gdb_test "continue" \
+	"Continuing\\..*Hardware watchpoint \[0-9\]+.*" \
+	"run until watchpoint on ${symbol}, 2nd hit"
+    delete_breakpoints
+}
diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index cdc3721a1cd..1dc5184e712 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -4093,6 +4093,69 @@ gdb_caching_proc allow_avx512fp16_tests {} {
     return $allow_avx512fp16_tests
 }
 
+# Run a test on the target to see if it supports LAM 57.  Return 1 if so,
+# 0 if it does not.  Based on the arch_prctl() handle ARCH_ENABLE_TAGGED_ADDR
+# to enable LAM which fails if the hardware or the OS does not support LAM.
+
+gdb_caching_proc allow_lam_tests {} {
+    global gdb_prompt inferior_exited_re
+
+    set me "allow_lam_tests"
+    if { ![istarget "x86_64-*-*"] } {
+	verbose "$me:  target does not support LAM, returning 1" 2
+	return 0
+    }
+
+    # Compile a test program.
+    set src {
+      #define _GNU_SOURCE
+      #include <sys/syscall.h>
+      #include <assert.h>
+      #include <errno.h>
+      #include <asm/prctl.h>
+
+      int configure_lam ()
+      {
+	errno = 0;
+	syscall (SYS_arch_prctl, ARCH_ENABLE_TAGGED_ADDR, 6);
+	assert_perror (errno);
+	return errno;
+      }
+
+      int
+      main () { return configure_lam (); }
+    }
+
+    if {![gdb_simple_compile $me $src executable ""]} {
+	return 0
+    }
+    # No error message, compilation succeeded so now run it via gdb.
+
+    set allow_lam_tests 0
+    clean_restart $obj
+    gdb_run_cmd
+    gdb_expect {
+	-re ".*$inferior_exited_re with code.*${gdb_prompt} $" {
+	    verbose -log "$me:  LAM support not detected."
+	}
+	-re ".*Program received signal SIGABRT, Aborted.*${gdb_prompt} $" {
+	    verbose -log "$me:  LAM support not detected."
+	}
+	-re ".*$inferior_exited_re normally.*${gdb_prompt} $" {
+	    verbose -log "$me:  LAM support detected."
+	    set allow_lam_tests 1
+	}
+	default {
+	    warning "\n$me:  default case taken."
+	}
+    }
+    gdb_exit
+    remote_file build delete $obj
+
+    verbose "$me:  returning $allow_lam_tests" 2
+    return $allow_lam_tests
+}
+
 # Run a test on the target to see if it supports btrace hardware.  Return 1 if so,
 # 0 if it does not.  Based on 'check_vmx_hw_available' from the GCC testsuite.
 
-- 
2.34.1

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

[PATCH v2 3/3] LAM: Support kernel space debugging

[Schimpe, Christina]

From: Christina Schimpe <christina.schimpe@intel.com>

Sign-extend watchpoint address for kernel space support.
---
 gdb/amd64-linux-tdep.c | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

diff --git a/gdb/amd64-linux-tdep.c b/gdb/amd64-linux-tdep.c
index b19b12842fd..285d3dd0ae2 100644
--- a/gdb/amd64-linux-tdep.c
+++ b/gdb/amd64-linux-tdep.c
@@ -52,6 +52,8 @@
 
 #include <sstream>
 
+/* Bit 63 is used to select between a kernel-space and user-space address.  */
+#define VA_RANGE_SELECT_BIT_MASK  0x8000000000000000UL
 #define DEFAULT_TAG_MASK 0xffffffffffffffffULL
 
 /* Mapping between the general-purpose registers in `struct user'
@@ -1848,9 +1850,21 @@ static CORE_ADDR
 amd64_linux_remove_non_address_bits_watchpoint (gdbarch *gdbarch,
 						CORE_ADDR addr)
 {
-  /* Clear insignificant bits of a target address using the untag
-     mask.  */
-  return (addr & amd64_linux_lam_untag_mask ());
+  /* The topmost bit tells us if we have a kernel-space address or a user-space
+     address.  */
+  const bool kernel_address = (addr & VA_RANGE_SELECT_BIT_MASK) != 0;
+
+  CORE_ADDR mask = amd64_linux_lam_untag_mask ();
+  /* Clear insignificant bits of a target address using the untag mask.
+     The untag mask preserves the topmost bit, which distinguishes user space
+     from kernel space address.  */
+  addr &= mask;
+
+  /* Sign-extend if we have a kernel-space address.  */
+  if (kernel_address)
+    addr |= ~mask;
+
+  return addr;
 }
 
 static void
-- 
2.34.1

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

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

[Willgerodt, Felix]

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

RE: [PATCH v2 2/3] LAM: Enable tagged pointer support for watchpoints.

[Willgerodt, Felix]

> -----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 2/3] LAM: Enable tagged pointer support for watchpoints.
> 
> From: Christina Schimpe <christina.schimpe@intel.com>
> 
> The Intel (R) linear address masking (LAM) feature modifies the checking
> applied to 64-bit linear addresses.  With this so-called "modified
> canonicality check" the processor masks the metadata bits in a pointer
> before using it as a linear address.  LAM supports two different modes that
> differ regarding which pointer bits are masked and can be used for
> metadata: LAM 48 resulting in a LAM width of 15 and LAM 57 resulting in a
> LAM width of 6.
> 
> This patch adjusts watchpoint addresses based on the currently enabled
> LAM mode using the untag mask provided in the /proc/<pid>/status file.
> As LAM can be enabled at runtime or as the configuration may change
> when entering an enclave, GDB checks enablement state each time a watchpoint
> is updated.
> 
> In contrast to the patch implemented for ARM's Top Byte Ignore "Clear
> non-significant bits of address on memory access", it is not necessary to
> adjust addresses before they are passed to the target layer cache, as
> for LAM tagged pointers are supported by the system call to read memory.
> Additionally, LAM applies only to addresses used for data accesses.
> Thus, it is sufficient to mask addresses used for watchpoints.
> 
> The following examples are based on a LAM57 enabled program.
> Before this patch tagged pointers were not supported for watchpoints:
> ~~~
> (gdb) print pi_tagged
> $2 = (int *) 0x10007ffffffffe004
> (gdb) watch *pi_tagged
> Hardware watchpoint 2: *pi_tagged
> (gdb) c
> Continuing.
> Couldn't write debug register: Invalid argument.
> ~~~~
> 
> Once LAM 48 or LAM 57 is enabled for the current program, GDB can now
> specify watchpoints for tagged addresses with LAM width 15 or 6,
> respectively.
> ---
>  gdb/NEWS                             |  2 +
>  gdb/amd64-linux-tdep.c               | 61 +++++++++++++++++++++++++++
>  gdb/testsuite/gdb.arch/amd64-lam.c   | 49 ++++++++++++++++++++++
>  gdb/testsuite/gdb.arch/amd64-lam.exp | 46 ++++++++++++++++++++
>  gdb/testsuite/lib/gdb.exp            | 63 ++++++++++++++++++++++++++++
>  5 files changed, 221 insertions(+)
>  create mode 100755 gdb/testsuite/gdb.arch/amd64-lam.c
>  create mode 100644 gdb/testsuite/gdb.arch/amd64-lam.exp
> 
> diff --git a/gdb/NEWS b/gdb/NEWS
> index 408d1509234..6303c334fd4 100644
> --- a/gdb/NEWS
> +++ b/gdb/NEWS
> @@ -3,6 +3,8 @@
> 
>  *** Changes since GDB 15
> 
> +* GDB now supports watchpoints for tagged data pointers on amd64.
> +
>  *** Changes in GDB 15
> 
>  * The MPX commands "show/set mpx bound" have been deprecated, as Intel
> diff --git a/gdb/amd64-linux-tdep.c b/gdb/amd64-linux-tdep.c
> index c52b0436872..b19b12842fd 100644
> --- a/gdb/amd64-linux-tdep.c
> +++ b/gdb/amd64-linux-tdep.c
> @@ -42,6 +42,7 @@
>  #include "arch/amd64.h"
>  #include "target-descriptions.h"
>  #include "expop.h"
> +#include "inferior.h"
> 
>  /* The syscall's XML filename for i386.  */
>  #define XML_SYSCALL_FILENAME_AMD64 "syscalls/amd64-linux.xml"
> @@ -49,6 +50,10 @@
>  #include "record-full.h"
>  #include "linux-record.h"
> 
> +#include <sstream>
> +
> +#define DEFAULT_TAG_MASK 0xffffffffffffffffULL
> +
>  /* Mapping between the general-purpose registers in `struct user'
>     format and GDB's register cache layout.  */
> 
> @@ -1795,6 +1800,59 @@ amd64_dtrace_parse_probe_argument (struct
> gdbarch *gdbarch,
>      }
>  }
> 
> +/* Extract the untagging mask based on the currently active linear address
> +   masking (LAM) mode, which is stored in the /proc/<pid>/status file.
> +   If we cannot extract the untag mask (for example, if we don't have
> +   execution), we assume address tagging is not enabled and return the
> +   DEFAULT_TAG_MASK.  */
> +
> +static CORE_ADDR
> +amd64_linux_lam_untag_mask ()
> +{
> +  if (!target_has_execution ())
> +    return DEFAULT_TAG_MASK;
> +
> +  inferior *inf = current_inferior ();
> +  if (inf->fake_pid_p)
> +    return DEFAULT_TAG_MASK;
> +
> +  const std::string filename = string_printf ("/proc/%d/status", inf->pid);
> +  gdb::unique_xmalloc_ptr<char> status_file
> +    = target_fileio_read_stralloc (nullptr, filename.c_str ());
> +
> +  if (status_file == nullptr)
> +    return DEFAULT_TAG_MASK;
> +
> +  /* Parse the status file line-by-line and look for the untag mask.  */
> +  std::istringstream strm_status_file (status_file.get ());
> +  std::string line;
> +  const std::string untag_mask_str ("untag_mask:\t");
> +  while (std::getline (strm_status_file, line))
> +    {
> +      const size_t found = line.find (untag_mask_str);
> +      if (found != std::string::npos)
> +	{
> +	  const size_t tag_length = untag_mask_str.length();
> +	  return std::strtoul (&line[found + tag_length], nullptr, 0);
> +	}
> +    }
> +
> +   return DEFAULT_TAG_MASK;
> +}

Sorry for not complaining earlier. I find it weird that we parse the file
into a buffer and then into a stringstream and then parse that line by line.
I think the line

+ std::istringstream strm_status_file (status_file.get ());

creates an extra copy. I don't see us gaining an advantage doing
this line by line, as we e.g. don't skip a line if it doesn't start with "untag_mask".
We still search the whole line, and therefore the whole file.
(Not sure how the file actually looks like or if we could do that. Or if we would
really save anything by doing that.) So why not search the file buffer directly?
Or did I miss something else that warrants the current way?

Another point is that strtoul could fail and return 0. I think some error checking
would be nice. And we can use std::string_view nowadays with C++17.

Here is some quick sketch I did while thinking about this:

-  /* Parse the status file line-by-line and look for the untag mask.  */
-  std::istringstream strm_status_file (status_file.get ());
-  std::string line;
-  const std::string untag_mask_str ("untag_mask:\t");
-  while (std::getline (strm_status_file, line))
-    {
-      const size_t found = line.find (untag_mask_str);
-      if (found != std::string::npos)
-       {
-         const size_t tag_length = untag_mask_str.length();
-         return std::strtoul (&line[found + tag_length], nullptr, 0);
-       }
-    }
+  std::string_view status_file_view (status_file.get ());
+  constexpr std::string_view untag_mask_str = "untag_mask:\t";
+  const size_t found = status_file_view.find (untag_mask_str);
+  if (found != std::string::npos)
+  {
+    const char* start = status_file_view.data() + found
+                       + untag_mask_str.length ();
+    char* endptr;
+    errno = 0;
+    unsigned long long result = std::strtoul (start, &endptr, 0);
+    if (errno == 0 && endptr != start)
+      return result;
+  }

Or maybe we even want to warn if we find the string but can't convert
it to a proper mask.

Regards,
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

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

[Schimpe, Christina]

Thank you for the review, Felix.

Also it seems that my series introduces a regression on arm detected by linaro CI:

https://ci.linaro.org/job/tcwg_gdb_check--master-arm-precommit/2549/artifact/artifacts/artifacts.precommit/notify/mail-body.txt

The reason is probably this patch, but I am unable to reproduce this locally and haven't been able to figure out what might have caused this ... 

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

Ok, let's wait for further comments here.

Christina
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

RE: [PATCH v2 2/3] LAM: Enable tagged pointer support for watchpoints.

[Schimpe, Christina]

Hi Felix,

Thank you for the review. Please see my comments below.

> > +/* Extract the untagging mask based on the currently active linear address
> > +   masking (LAM) mode, which is stored in the /proc/<pid>/status file.
> > +   If we cannot extract the untag mask (for example, if we don't have
> > +   execution), we assume address tagging is not enabled and return the
> > +   DEFAULT_TAG_MASK.  */
> > +
> > +static CORE_ADDR
> > +amd64_linux_lam_untag_mask ()
> > +{
> > +  if (!target_has_execution ())
> > +    return DEFAULT_TAG_MASK;
> > +
> > +  inferior *inf = current_inferior ();
> > +  if (inf->fake_pid_p)
> > +    return DEFAULT_TAG_MASK;
> > +
> > +  const std::string filename = string_printf ("/proc/%d/status", inf->pid);
> > +  gdb::unique_xmalloc_ptr<char> status_file
> > +    = target_fileio_read_stralloc (nullptr, filename.c_str ());
> > +
> > +  if (status_file == nullptr)
> > +    return DEFAULT_TAG_MASK;
> > +
> > +  /* Parse the status file line-by-line and look for the untag mask.  */
> > +  std::istringstream strm_status_file (status_file.get ());
> > +  std::string line;
> > +  const std::string untag_mask_str ("untag_mask:\t");
> > +  while (std::getline (strm_status_file, line))
> > +    {
> > +      const size_t found = line.find (untag_mask_str);
> > +      if (found != std::string::npos)
> > +	{
> > +	  const size_t tag_length = untag_mask_str.length();
> > +	  return std::strtoul (&line[found + tag_length], nullptr, 0);
> > +	}
> > +    }
> > +
> > +   return DEFAULT_TAG_MASK;
> > +}
> 
> Sorry for not complaining earlier. I find it weird that we parse the file
> into a buffer and then into a stringstream and then parse that line by line.
> I think the line
> 
> + std::istringstream strm_status_file (status_file.get ());
> 
> creates an extra copy. I don't see us gaining an advantage doing
> this line by line, as we e.g. don't skip a line if it doesn't start with
> "untag_mask".
> We still search the whole line, and therefore the whole file.
> (Not sure how the file actually looks like or if we could do that. Or if we
> would
> really save anything by doing that.) So why not search the file buffer directly?
> Or did I miss something else that warrants the current way?

I actually agree and don't remember if there was a reason why I chose this approach.
So yes, we can do better here.

> Another point is that strtoul could fail and return 0. I think some error
> checking
> would be nice. And we can use std::string_view nowadays with C++17.

I thought that if strtoul would fail here it should be a kernel bug providing
an untag mask which cannot be parsed. So I thought we don't have to handle
that in GDB, but am also not sure how we deal with that in general. 
But I have absolutely no problem adding that check here.

> Here is some quick sketch I did while thinking about this:
> 
> -  /* Parse the status file line-by-line and look for the untag mask.  */
> -  std::istringstream strm_status_file (status_file.get ());
> -  std::string line;
> -  const std::string untag_mask_str ("untag_mask:\t");
> -  while (std::getline (strm_status_file, line))
> -    {
> -      const size_t found = line.find (untag_mask_str);
> -      if (found != std::string::npos)
> -       {
> -         const size_t tag_length = untag_mask_str.length();
> -         return std::strtoul (&line[found + tag_length], nullptr, 0);
> -       }
> -    }
> +  std::string_view status_file_view (status_file.get ());
> +  constexpr std::string_view untag_mask_str = "untag_mask:\t";
> +  const size_t found = status_file_view.find (untag_mask_str);
> +  if (found != std::string::npos)
> +  {
> +    const char* start = status_file_view.data() + found
> +                       + untag_mask_str.length ();
> +    char* endptr;
> +    errno = 0;
> +    unsigned long long result = std::strtoul (start, &endptr, 0);
> +    if (errno == 0 && endptr != start)
> +      return result;
> +  }
> 
> Or maybe we even want to warn if we find the string but can't convert
> it to a proper mask.

As I said before I am not sure how we should handle those error conditions. 
Maybe somebody else can help us here?

Besides that I tested the code that you suggested Felix, and it works fine.
I'd use that approach once the error handling is clarified and there are no
further objections.

Christina

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

RE: [PATCH v2 2/3] LAM: Enable tagged pointer support for watchpoints.

[Willgerodt, Felix]

> > Or maybe we even want to warn if we find the string but can't convert
> > it to a proper mask.
> 
> As I said before I am not sure how we should handle those error conditions.
> Maybe somebody else can help us here?

I think we want to warn or error out. You can look at existing GDB code.
For example here GDB prints an error if it couldn't read an integer from
a Linux file:
https://sourceware.org/git/?p=binutils-gdb.git;a=blob;f=gdb/nat/linux-btrace.c;h=5715168d2f0887da2321694b5a8f3e21e74c4ad7;hb=8e8b2ab688b3d7e6035e6c782a2b42501d57c667#l601
Not sure if there are other examples or counter examples,
this just one I knew of.

In your case it is very unlikely that this will ever happen yes. But I think
GDB is just much better off to check this. Otherwise the function
might return some wrong mask silently. A warning or error makes it
much easier to understand, if there ever is a bug.

Is the untag_mask string in the file, even if the default mask is "active"?
In that case you could argue a warning is good enough, as we
don't actually know if the watchpoint would be set to a wrong address.

Though to be on the safe side, I lean towards an error.  We have the
indication that LAM is supported (or even active), but can't read the mask.
So we don't know if we can set a watchpoint on the right address.
Yes this prevents users from setting watchpoints, but since it is an unlikely
scenario I hope this is never a real problem for someone.

And note that my code was just a rough sketch, please double check it.

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

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

[Luis Machado]

On 6/3/24 09:40, Schimpe, Christina wrote:
> Thank you for the review, Felix.
> 
> Also it seems that my series introduces a regression on arm detected by linaro CI:
> 
> https://ci.linaro.org/job/tcwg_gdb_check--master-arm-precommit/2549/artifact/artifacts/artifacts.precommit/notify/mail-body.txt
> 
> The reason is probably this patch, but I am unable to reproduce this locally and haven't been able to figure out what might have caused this ... 
> 
>>> 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
> 
> Ok, let's wait for further comments here.

Sorry, only spotted this now.

The comment on the above function would be a repetition of the hook explanation, and
the gdbarch hook explanation covers all the details we need. But if the comment is being
removed due to it being in the header, we should point to the header instead.

"See aarch64-tdep.h" should do.

There is something odd about this patch though. The aarch64 code is being changed to
call aarch64_remove_non_address_bits directly, but 3 new hooks are being set:

+  set_gdbarch_remove_non_address_bits_watchpoint
+    (gdbarch, aarch64_remove_non_address_bits);
+  set_gdbarch_remove_non_address_bits_breakpoint
+    (gdbarch, aarch64_remove_non_address_bits);
+  set_gdbarch_remove_non_address_bits_memory
+    (gdbarch, aarch64_remove_non_address_bits);

But the above hooks (the gdbarch_remove_non_address_bits_memory at least) never get
used in aarch64 code. Is there a reason for that?

> 
> Christina
> 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

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

[Schimpe, Christina]

Hi Luis,

> >>> -/* 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
> >
> > Ok, let's wait for further comments here.
> 
> Sorry, only spotted this now.
> 
> The comment on the above function would be a repetition of the hook
> explanation, and the gdbarch hook explanation covers all the details we
> need. But if the comment is being removed due to it being in the header, we
> should point to the header instead.
> 
> "See aarch64-tdep.h" should do.

Ok, I will add that line.

> There is something odd about this patch though. The aarch64 code is being
> changed to call aarch64_remove_non_address_bits directly, but 3 new hooks
> are being set:
> 
> +  set_gdbarch_remove_non_address_bits_watchpoint
> +    (gdbarch, aarch64_remove_non_address_bits);
> + set_gdbarch_remove_non_address_bits_breakpoint
> +    (gdbarch, aarch64_remove_non_address_bits);
> + set_gdbarch_remove_non_address_bits_memory
> +    (gdbarch, aarch64_remove_non_address_bits);
> 
> But the above hooks (the gdbarch_remove_non_address_bits_memory at
> least) never get used in aarch64 code. Is there a reason for that?

I thought it's wrong to call the gdbarch_remove_non_address_bits_memory hook in aarch64 code.
The reason is described here: https://sourceware.org/pipermail/gdb-patches/2024-May/209420.html

"Also I noticed that the gdbarch function 'gdbarch_remove_non_addr_bits_memory'
should not be used in several functions of aarch64* files (patch 1) due
to the following:

The function description for 'gdbarch_remove_non_addr_bits_memory' states:

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

We don't know if the pointer passed to 'gdbarch_remove_non_addr_bits_memory'
by 'gdbarch_tagged_address_p' will be used to access memory.  That's why it's
wrong to call this function in 'gdbarch_tagged_address_p'.

There are several similar wrong calls of 'gdbarch_remove_non_addr_bits_memory'
in the changes of patch 1.  I replaced all of them with
'aarch64_remove_non_address_bits'."

Regards,
Christina

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

RE: [PATCH v2 2/3] LAM: Enable tagged pointer support for watchpoints.

[Schimpe, Christina]

> > > Or maybe we even want to warn if we find the string but can't
> > > convert it to a proper mask.
> >
> > As I said before I am not sure how we should handle those error
> conditions.
> > Maybe somebody else can help us here?
> 
> I think we want to warn or error out. You can look at existing GDB code.
> For example here GDB prints an error if it couldn't read an integer from a
> Linux file:
> https://sourceware.org/git/?p=binutils-gdb.git;a=blob;f=gdb/nat/linux-
> btrace.c;h=5715168d2f0887da2321694b5a8f3e21e74c4ad7;hb=8e8b2ab688b3
> d7e6035e6c782a2b42501d57c667#l601
> Not sure if there are other examples or counter examples, this just one I
> knew of.
> 
> In your case it is very unlikely that this will ever happen yes. But I think GDB
> is just much better off to check this. Otherwise the function might return
> some wrong mask silently. A warning or error makes it much easier to
> understand, if there ever is a bug.
> 
> Is the untag_mask string in the file, even if the default mask is "active"?
> In that case you could argue a warning is good enough, as we don't actually
> know if the watchpoint would be set to a wrong address.

As long as the kernel supports LAM the untag_mask string should be in the file,
even if LAM is not enabled.  So in practise we have to deal with both scenarios
with and without untag_mask string (older kernels).

> Though to be on the safe side, I lean towards an error.  We have the
> indication that LAM is supported (or even active), but can't read the mask.
> So we don't know if we can set a watchpoint on the right address.
> Yes this prevents users from setting watchpoints, but since it is an unlikely
> scenario I hope this is never a real problem for someone.

I am fine with an error here.

> And note that my code was just a rough sketch, please double check it.

Ok.

Christina
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

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

[Luis Machado]

On 6/3/24 15:13, Schimpe, Christina wrote:
> Hi Luis,
> 
>>>>> -/* 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
>>>
>>> Ok, let's wait for further comments here.
>>
>> Sorry, only spotted this now.
>>
>> The comment on the above function would be a repetition of the hook
>> explanation, and the gdbarch hook explanation covers all the details we
>> need. But if the comment is being removed due to it being in the header, we
>> should point to the header instead.
>>
>> "See aarch64-tdep.h" should do.
> 
> Ok, I will add that line.
> 
>> There is something odd about this patch though. The aarch64 code is being
>> changed to call aarch64_remove_non_address_bits directly, but 3 new hooks
>> are being set:
>>
>> +  set_gdbarch_remove_non_address_bits_watchpoint
>> +    (gdbarch, aarch64_remove_non_address_bits);
>> + set_gdbarch_remove_non_address_bits_breakpoint
>> +    (gdbarch, aarch64_remove_non_address_bits);
>> + set_gdbarch_remove_non_address_bits_memory
>> +    (gdbarch, aarch64_remove_non_address_bits);
>>
>> But the above hooks (the gdbarch_remove_non_address_bits_memory at
>> least) never get used in aarch64 code. Is there a reason for that?
> 
> I thought it's wrong to call the gdbarch_remove_non_address_bits_memory hook in aarch64 code.
> The reason is described here: https://sourceware.org/pipermail/gdb-patches/2024-May/209420.html
> 
> "Also I noticed that the gdbarch function 'gdbarch_remove_non_addr_bits_memory'
> should not be used in several functions of aarch64* files (patch 1) due
> to the following:
> 
> The function description for 'gdbarch_remove_non_addr_bits_memory' states:
> 
> "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. "
> 
> We don't know if the pointer passed to 'gdbarch_remove_non_addr_bits_memory'
> by 'gdbarch_tagged_address_p' will be used to access memory.  That's why it's
> wrong to call this function in 'gdbarch_tagged_address_p'.
> 
> There are several similar wrong calls of 'gdbarch_remove_non_addr_bits_memory'
> in the changes of patch 1.  I replaced all of them with
> 'aarch64_remove_non_address_bits'."
> 

That's fine, but previously the hook was correct for what is was being used
for in aarch64. This particular change means we no longer have a hook that
aarch64 can use for the same purposes it used before. This part feels a bit
odd. It feels like the memory-specific one would be appropriate, but it isn't
clear.

Also, from the 1/3 patch submission:

"- In contrast to the implementation for ARM's TBI, the Linux kernel supports
  tagged pointers for memory access."

Is there a negative impact if we feed the kernel an untagged pointer for a
particular memory access? If not, it sounds like it might be simpler (from gdb's
perspective) to let gdb always unmask things, no? Of course, there might be a
performance impact of checking if a region is tagged.

I'm just trying to understand if the additional hooks are really needed/useful.

Right now it looks as if we could make it work with the existing hook in place,
instead of splitting it into 3 different hooks.



> Regards,
> Christina
> 
> 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

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

[Schimpe, Christina]

> -----Original Message-----
> From: Luis Machado <luis.machado@arm.com>
> Sent: Monday, June 10, 2024 4:01 PM
> To: Schimpe, Christina <christina.schimpe@intel.com>; Willgerodt, Felix
> <felix.willgerodt@intel.com>; gdb-patches@sourceware.org
> Cc: eliz@gnu.org
> Subject: Re: [PATCH v2 1/3] gdb: Make tagged pointer support configurable.
> 
> On 6/3/24 15:13, Schimpe, Christina wrote:
> > Hi Luis,
> >
> >>>>> -/* 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
> >>>
> >>> Ok, let's wait for further comments here.
> >>
> >> Sorry, only spotted this now.
> >>
> >> The comment on the above function would be a repetition of the hook
> >> explanation, and the gdbarch hook explanation covers all the details
> >> we need. But if the comment is being removed due to it being in the
> >> header, we should point to the header instead.
> >>
> >> "See aarch64-tdep.h" should do.
> >
> > Ok, I will add that line.
> >
> >> There is something odd about this patch though. The aarch64 code is
> >> being changed to call aarch64_remove_non_address_bits directly, but 3
> >> new hooks are being set:
> >>
> >> +  set_gdbarch_remove_non_address_bits_watchpoint
> >> +    (gdbarch, aarch64_remove_non_address_bits);
> >> + set_gdbarch_remove_non_address_bits_breakpoint
> >> +    (gdbarch, aarch64_remove_non_address_bits);
> >> + set_gdbarch_remove_non_address_bits_memory
> >> +    (gdbarch, aarch64_remove_non_address_bits);
> >>
> >> But the above hooks (the gdbarch_remove_non_address_bits_memory at
> >> least) never get used in aarch64 code. Is there a reason for that?
> >
> > I thought it's wrong to call the gdbarch_remove_non_address_bits_memory
> hook in aarch64 code.
> > The reason is described here:
> > https://sourceware.org/pipermail/gdb-patches/2024-May/209420.html
> >
> > "Also I noticed that the gdbarch function
> 'gdbarch_remove_non_addr_bits_memory'
> > should not be used in several functions of aarch64* files (patch 1)
> > due to the following:
> >
> > The function description for 'gdbarch_remove_non_addr_bits_memory'
> states:
> >
> > "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. "
> >
> > We don't know if the pointer passed to
> 'gdbarch_remove_non_addr_bits_memory'
> > by 'gdbarch_tagged_address_p' will be used to access memory.  That's
> > why it's wrong to call this function in 'gdbarch_tagged_address_p'.
> >
> > There are several similar wrong calls of
> 'gdbarch_remove_non_addr_bits_memory'
> > in the changes of patch 1.  I replaced all of them with
> > 'aarch64_remove_non_address_bits'."
> >
> 
> That's fine, but previously the hook was correct for what is was being used for
> in aarch64. This particular change means we no longer have a hook that
> aarch64 can use for the same purposes it used before. This part feels a bit odd.
> It feels like the memory-specific one would be appropriate, but it isn't clear.

Hm, I thought it is not a problem to remove the hook in the aarch64 files, as it's target
specific code. 

> Also, from the 1/3 patch submission:
> 
> "- In contrast to the implementation for ARM's TBI, the Linux kernel supports
>   tagged pointers for memory access."
> 
> Is there a negative impact if we feed the kernel an untagged pointer for a
> particular memory access? If not, it sounds like it might be simpler (from gdb's
> perspective) to let gdb always unmask things, no? Of course, there might be a
> performance impact of checking if a region is tagged.
> I'm just trying to understand if the additional hooks are really needed/useful.
> 
> Right now it looks as if we could make it work with the existing hook in place,
> instead of splitting it into 3 different hooks.

I think in case of amd64 there is a negative impact.  In contrast to ARM's TBI,
LAM can be enabled at runtime.  So we would have to parse the /proc/<pid>/status
file every time when the pointer is used to access memory. With the 3 hooks I
wanted to avoid this.

I tried to make the purpose more clear by adding the two sentences in the commit
message for v2:

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

So yes, it does work with the existing hook, but when it comes to performance, I
think 3 hooks would be better.

Christina
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

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

[Schimpe, Christina]

Hi Luis, 

Do you still have concerns on the topic discussed below?
Otherwise, I'd apply Felix latest feedback and push a new version of this series.

Thanks,
Christina

> -----Original Message-----
> From: Schimpe, Christina
> Sent: Monday, June 10, 2024 5:06 PM
> To: Luis Machado <luis.machado@arm.com>; Willgerodt, Felix
> <FeliX.Willgerodt@intel.com>; gdb-patches@sourceware.org
> Cc: eliz@gnu.org
> Subject: RE: [PATCH v2 1/3] gdb: Make tagged pointer support configurable.
> 
> 
> 
> > -----Original Message-----
> > From: Luis Machado <luis.machado@arm.com>
> > Sent: Monday, June 10, 2024 4:01 PM
> > To: Schimpe, Christina <christina.schimpe@intel.com>; Willgerodt,
> > Felix <felix.willgerodt@intel.com>; gdb-patches@sourceware.org
> > Cc: eliz@gnu.org
> > Subject: Re: [PATCH v2 1/3] gdb: Make tagged pointer support configurable.
> >
> > On 6/3/24 15:13, Schimpe, Christina wrote:
> > > Hi Luis,
> > >
> > >>>>> -/* 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
> > >>>
> > >>> Ok, let's wait for further comments here.
> > >>
> > >> Sorry, only spotted this now.
> > >>
> > >> The comment on the above function would be a repetition of the hook
> > >> explanation, and the gdbarch hook explanation covers all the
> > >> details we need. But if the comment is being removed due to it
> > >> being in the header, we should point to the header instead.
> > >>
> > >> "See aarch64-tdep.h" should do.
> > >
> > > Ok, I will add that line.
> > >
> > >> There is something odd about this patch though. The aarch64 code is
> > >> being changed to call aarch64_remove_non_address_bits directly, but
> > >> 3 new hooks are being set:
> > >>
> > >> +  set_gdbarch_remove_non_address_bits_watchpoint
> > >> +    (gdbarch, aarch64_remove_non_address_bits);
> > >> + set_gdbarch_remove_non_address_bits_breakpoint
> > >> +    (gdbarch, aarch64_remove_non_address_bits);
> > >> + set_gdbarch_remove_non_address_bits_memory
> > >> +    (gdbarch, aarch64_remove_non_address_bits);
> > >>
> > >> But the above hooks (the gdbarch_remove_non_address_bits_memory
> at
> > >> least) never get used in aarch64 code. Is there a reason for that?
> > >
> > > I thought it's wrong to call the
> > > gdbarch_remove_non_address_bits_memory
> > hook in aarch64 code.
> > > The reason is described here:
> > > https://sourceware.org/pipermail/gdb-patches/2024-May/209420.html
> > >
> > > "Also I noticed that the gdbarch function
> > 'gdbarch_remove_non_addr_bits_memory'
> > > should not be used in several functions of aarch64* files (patch 1)
> > > due to the following:
> > >
> > > The function description for 'gdbarch_remove_non_addr_bits_memory'
> > states:
> > >
> > > "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. "
> > >
> > > We don't know if the pointer passed to
> > 'gdbarch_remove_non_addr_bits_memory'
> > > by 'gdbarch_tagged_address_p' will be used to access memory.  That's
> > > why it's wrong to call this function in 'gdbarch_tagged_address_p'.
> > >
> > > There are several similar wrong calls of
> > 'gdbarch_remove_non_addr_bits_memory'
> > > in the changes of patch 1.  I replaced all of them with
> > > 'aarch64_remove_non_address_bits'."
> > >
> >
> > That's fine, but previously the hook was correct for what is was being
> > used for in aarch64. This particular change means we no longer have a
> > hook that
> > aarch64 can use for the same purposes it used before. This part feels a bit
> odd.
> > It feels like the memory-specific one would be appropriate, but it isn't clear.
> 
> Hm, I thought it is not a problem to remove the hook in the aarch64 files, as
> it's target specific code.
> 
> > Also, from the 1/3 patch submission:
> >
> > "- In contrast to the implementation for ARM's TBI, the Linux kernel
> supports
> >   tagged pointers for memory access."
> >
> > Is there a negative impact if we feed the kernel an untagged pointer
> > for a particular memory access? If not, it sounds like it might be
> > simpler (from gdb's
> > perspective) to let gdb always unmask things, no? Of course, there
> > might be a performance impact of checking if a region is tagged.
> > I'm just trying to understand if the additional hooks are really
> needed/useful.
> >
> > Right now it looks as if we could make it work with the existing hook
> > in place, instead of splitting it into 3 different hooks.
> 
> I think in case of amd64 there is a negative impact.  In contrast to ARM's TBI,
> LAM can be enabled at runtime.  So we would have to parse the
> /proc/<pid>/status file every time when the pointer is used to access
> memory. With the 3 hooks I wanted to avoid this.
> 
> I tried to make the purpose more clear by adding the two sentences in the
> commit message for v2:
> 
> "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."
> 
> So yes, it does work with the existing hook, but when it comes to
> performance, I think 3 hooks would be better.
> 
> Christina
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

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

[Schimpe, Christina]

> Do you still have concerns on the topic discussed below?
> Otherwise, I'd apply Felix latest feedback and push a new version of this
> series.

Aah, I meant "post" and not "push" a new version of this series.

Christina
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

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

[Luis Machado]

On 6/14/24 10:38, Schimpe, Christina wrote:
> Hi Luis, 
> 
> Do you still have concerns on the topic discussed below?
> Otherwise, I'd apply Felix latest feedback and push a new version of this series.

Nothing that should block progress on this. My concern was mostly the odd situation
with the hooks. But I can revisit that if it turns out to be a problem.


> 
> Thanks,
> Christina
> 
>> -----Original Message-----
>> From: Schimpe, Christina
>> Sent: Monday, June 10, 2024 5:06 PM
>> To: Luis Machado <luis.machado@arm.com>; Willgerodt, Felix
>> <FeliX.Willgerodt@intel.com>; gdb-patches@sourceware.org
>> Cc: eliz@gnu.org
>> Subject: RE: [PATCH v2 1/3] gdb: Make tagged pointer support configurable.
>>
>>
>>
>>> -----Original Message-----
>>> From: Luis Machado <luis.machado@arm.com>
>>> Sent: Monday, June 10, 2024 4:01 PM
>>> To: Schimpe, Christina <christina.schimpe@intel.com>; Willgerodt,
>>> Felix <felix.willgerodt@intel.com>; gdb-patches@sourceware.org
>>> Cc: eliz@gnu.org
>>> Subject: Re: [PATCH v2 1/3] gdb: Make tagged pointer support configurable.
>>>
>>> On 6/3/24 15:13, Schimpe, Christina wrote:
>>>> Hi Luis,
>>>>
>>>>>>>> -/* 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
>>>>>>
>>>>>> Ok, let's wait for further comments here.
>>>>>
>>>>> Sorry, only spotted this now.
>>>>>
>>>>> The comment on the above function would be a repetition of the hook
>>>>> explanation, and the gdbarch hook explanation covers all the
>>>>> details we need. But if the comment is being removed due to it
>>>>> being in the header, we should point to the header instead.
>>>>>
>>>>> "See aarch64-tdep.h" should do.
>>>>
>>>> Ok, I will add that line.
>>>>
>>>>> There is something odd about this patch though. The aarch64 code is
>>>>> being changed to call aarch64_remove_non_address_bits directly, but
>>>>> 3 new hooks are being set:
>>>>>
>>>>> +  set_gdbarch_remove_non_address_bits_watchpoint
>>>>> +    (gdbarch, aarch64_remove_non_address_bits);
>>>>> + set_gdbarch_remove_non_address_bits_breakpoint
>>>>> +    (gdbarch, aarch64_remove_non_address_bits);
>>>>> + set_gdbarch_remove_non_address_bits_memory
>>>>> +    (gdbarch, aarch64_remove_non_address_bits);
>>>>>
>>>>> But the above hooks (the gdbarch_remove_non_address_bits_memory
>> at
>>>>> least) never get used in aarch64 code. Is there a reason for that?
>>>>
>>>> I thought it's wrong to call the
>>>> gdbarch_remove_non_address_bits_memory
>>> hook in aarch64 code.
>>>> The reason is described here:
>>>> https://sourceware.org/pipermail/gdb-patches/2024-May/209420.html
>>>>
>>>> "Also I noticed that the gdbarch function
>>> 'gdbarch_remove_non_addr_bits_memory'
>>>> should not be used in several functions of aarch64* files (patch 1)
>>>> due to the following:
>>>>
>>>> The function description for 'gdbarch_remove_non_addr_bits_memory'
>>> states:
>>>>
>>>> "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. "
>>>>
>>>> We don't know if the pointer passed to
>>> 'gdbarch_remove_non_addr_bits_memory'
>>>> by 'gdbarch_tagged_address_p' will be used to access memory.  That's
>>>> why it's wrong to call this function in 'gdbarch_tagged_address_p'.
>>>>
>>>> There are several similar wrong calls of
>>> 'gdbarch_remove_non_addr_bits_memory'
>>>> in the changes of patch 1.  I replaced all of them with
>>>> 'aarch64_remove_non_address_bits'."
>>>>
>>>
>>> That's fine, but previously the hook was correct for what is was being
>>> used for in aarch64. This particular change means we no longer have a
>>> hook that
>>> aarch64 can use for the same purposes it used before. This part feels a bit
>> odd.
>>> It feels like the memory-specific one would be appropriate, but it isn't clear.
>>
>> Hm, I thought it is not a problem to remove the hook in the aarch64 files, as
>> it's target specific code.
>>
>>> Also, from the 1/3 patch submission:
>>>
>>> "- In contrast to the implementation for ARM's TBI, the Linux kernel
>> supports
>>>   tagged pointers for memory access."
>>>
>>> Is there a negative impact if we feed the kernel an untagged pointer
>>> for a particular memory access? If not, it sounds like it might be
>>> simpler (from gdb's
>>> perspective) to let gdb always unmask things, no? Of course, there
>>> might be a performance impact of checking if a region is tagged.
>>> I'm just trying to understand if the additional hooks are really
>> needed/useful.
>>>
>>> Right now it looks as if we could make it work with the existing hook
>>> in place, instead of splitting it into 3 different hooks.
>>
>> I think in case of amd64 there is a negative impact.  In contrast to ARM's TBI,
>> LAM can be enabled at runtime.  So we would have to parse the
>> /proc/<pid>/status file every time when the pointer is used to access
>> memory. With the 3 hooks I wanted to avoid this.
>>
>> I tried to make the purpose more clear by adding the two sentences in the
>> commit message for v2:
>>
>> "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."
>>
>> So yes, it does work with the existing hook, but when it comes to
>> performance, I think 3 hooks would be better.
>>
>> Christina
> 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