[PATCH v4 0/2] Add amd64 LAM watchpoint support

[PATCH v4 0/2] Add amd64 LAM watchpoint support

[Schimpe, Christina]

Hi all,

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

Changes since V3:

* Improve error handling.
* Removed unnecessary comment.

v3 of this series can be found here:
https://sourceware.org/pipermail/gdb-patches/2024-June/210056.html

Best Regards
Christina

Christina Schimpe (2):
  gdb: Make tagged pointer support configurable.
  LAM: Enable tagged pointer support for watchpoints.

 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               | 64 +++++++++++++++++++++++++++
 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, 392 insertions(+), 46 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 v4 1/2] 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.

Reviewed-By: Felix Willgerodt <felix.willgerodt@intel.com>
---
 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, 168 insertions(+), 46 deletions(-)

diff --git a/gdb/aarch64-linux-nat.c b/gdb/aarch64-linux-nat.c
index 2e6541f53c3..dac42b4f48b 100644
--- a/gdb/aarch64-linux-nat.c
+++ b/gdb/aarch64-linux-nat.c
@@ -945,7 +945,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 a1296a8f0c7..bf5ce72aa5f 100644
--- a/gdb/aarch64-linux-tdep.c
+++ b/gdb/aarch64-linux-tdep.c
@@ -2456,7 +2456,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))
@@ -2478,7 +2478,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;
@@ -2517,7 +2517,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:
@@ -2566,7 +2566,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 ())
@@ -2640,8 +2640,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 e4bca6c6632..ba1dc3d5844 100644
--- a/gdb/aarch64-tdep.c
+++ b/gdb/aarch64-tdep.c
@@ -4088,10 +4088,9 @@ 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.  */
+/* See aarch64-tdep.h.  */
 
-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
@@ -4585,9 +4584,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 a973518ac5f..8c2d9d247ec 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 1b5aa11ed6f..b622de81e11 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -1608,7 +1608,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 v4 2/2] 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               | 64 ++++++++++++++++++++++++++++
 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, 224 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 47677cb773a..fcbb89e8bf8 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.
+
 * Debugger Adapter Protocol changes
 
   ** The "scopes" request will now return a scope holding global
diff --git a/gdb/amd64-linux-tdep.c b/gdb/amd64-linux-tdep.c
index d7662cac572..88e14251d7d 100644
--- a/gdb/amd64-linux-tdep.c
+++ b/gdb/amd64-linux-tdep.c
@@ -43,6 +43,7 @@
 #include "target-descriptions.h"
 #include "expop.h"
 #include "arch/amd64-linux-tdesc.h"
+#include "inferior.h"
 
 /* The syscall's XML filename for i386.  */
 #define XML_SYSCALL_FILENAME_AMD64 "syscalls/amd64-linux.xml"
@@ -50,6 +51,10 @@
 #include "record-full.h"
 #include "linux-record.h"
 
+#include <string_view>
+
+#define DEFAULT_TAG_MASK 0xffffffffffffffffULL
+
 /* Mapping between the general-purpose registers in `struct user'
    format and GDB's register cache layout.  */
 
@@ -1765,6 +1770,62 @@ 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;
+
+  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)
+	error (_("Failed to parse untag_mask from file %s."),
+	       std::string (filename).c_str ());
+
+      return result;
+    }
+
+   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)
@@ -1819,6 +1880,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 dfe19c9410d..4cfda74fc43 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -4154,6 +4154,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

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

[Willgerodt, Felix]

> -----Original Message-----
> From: Schimpe, Christina <christina.schimpe@intel.com>
> Sent: Donnerstag, 4. Juli 2024 12:18
> To: gdb-patches@sourceware.org
> Cc: Willgerodt, Felix <felix.willgerodt@intel.com>; luis.machado@arm.com
> Subject: [PATCH v4 2/2] 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               | 64 ++++++++++++++++++++++++++++
>  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, 224 insertions(+)
>  create mode 100755 gdb/testsuite/gdb.arch/amd64-lam.c
>  create mode 100644 gdb/testsuite/gdb.arch/amd64-lam.exp

Approved-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 v4 2/2] LAM: Enable tagged pointer support for watchpoints.

[Eli Zaretskii]

> From: "Schimpe, Christina" <christina.schimpe@intel.com>
> Cc: felix.willgerodt@intel.com,
> 	luis.machado@arm.com
> Date: Thu,  4 Jul 2024 10:17:36 +0000
> 
>  gdb/NEWS                             |  2 +
>  gdb/amd64-linux-tdep.c               | 64 ++++++++++++++++++++++++++++
>  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, 224 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 47677cb773a..fcbb89e8bf8 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.
> +
>  * Debugger Adapter Protocol changes

Thanks.  The above NEWS text is okay, but I wonder whether we explain
anywhere what are "tagged data pointers".

Reviewed-By: Eli Zaretskii <eliz@gnu.org>

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

[Schimpe, Christina]

> > diff --git a/gdb/NEWS b/gdb/NEWS
> > index 47677cb773a..fcbb89e8bf8 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.
> > +
> >  * Debugger Adapter Protocol changes
> 
> Thanks.  The above NEWS text is okay, but I wonder whether we explain
> anywhere what are "tagged data pointers".
> 
> Reviewed-By: Eli Zaretskii <eliz@gnu.org>

Thanks for the review. I understand this can be a bit confusing. 
Is " GDB now supports watchpoints for tagged pointers used for data accesses on amd64." better?
Or, we could also just write: " GDB now supports watchpoints for tagged pointers on amd64.", as watchpoints are usually just data breakpoints.

What do you think?

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 v4 2/2] LAM: Enable tagged pointer support for watchpoints.

[Schimpe, Christina]

> > > diff --git a/gdb/NEWS b/gdb/NEWS
> > > index 47677cb773a..fcbb89e8bf8 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.
> > > +
> > >  * Debugger Adapter Protocol changes
> >
> > Thanks.  The above NEWS text is okay, but I wonder whether we explain
> > anywhere what are "tagged data pointers".
> >
> > Reviewed-By: Eli Zaretskii <eliz@gnu.org>
> 
> Thanks for the review. I understand this can be a bit confusing.
> Is " GDB now supports watchpoints for tagged pointers used for data accesses
> on amd64." better?
> Or, we could also just write: " GDB now supports watchpoints for tagged
> pointers on amd64.", as watchpoints are usually just data breakpoints.
> 
> What do you think?
> 
> Christina

So to answer your question properly: No, we don't explain the term " tagged data pointers" anywhere.
But we write for instance in the commit message "tagged pointers used for data accesses",
which is understandable I think.

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 v4 2/2] LAM: Enable tagged pointer support for watchpoints.

[Eli Zaretskii]

> From: "Schimpe, Christina" <christina.schimpe@intel.com>
> CC: "gdb-patches@sourceware.org" <gdb-patches@sourceware.org>, "Willgerodt,
>  Felix" <felix.willgerodt@intel.com>, "luis.machado@arm.com"
> 	<luis.machado@arm.com>
> Date: Thu, 4 Jul 2024 13:15:20 +0000
> 
> So to answer your question properly: No, we don't explain the term " tagged data pointers" anywhere.
> But we write for instance in the commit message "tagged pointers used for data accesses",
> which is understandable I think.

Well, I'm sorry to say that it doesn't explain this enough for me.
I'm quite a heavy user of GDB, but I have no idea what is a tagged
pointer for data access.  Do we have in the manual anything related to
such pointers, even if we use different words to talk about them?  If
so, could you point me to those descriptions in the manual?  Maybe if
I read what we say there, I could suggest wording for NEWS that will
be consistent with our terminology in the manual.

Thanks.

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

[Schimpe, Christina]

> > From: "Schimpe, Christina" <christina.schimpe@intel.com>
> > CC: "gdb-patches@sourceware.org" <gdb-patches@sourceware.org>,
> > "Willgerodt,  Felix" <felix.willgerodt@intel.com>, "luis.machado@arm.com"
> > 	<luis.machado@arm.com>
> > Date: Thu, 4 Jul 2024 13:15:20 +0000
> >
> > So to answer your question properly: No, we don't explain the term "
> tagged data pointers" anywhere.
> > But we write for instance in the commit message "tagged pointers used
> > for data accesses", which is understandable I think.
> 
> Well, I'm sorry to say that it doesn't explain this enough for me.
> I'm quite a heavy user of GDB, but I have no idea what is a tagged pointer for
> data access.  Do we have in the manual anything related to such pointers,
> even if we use different words to talk about them?  If so, could you point me
> to those descriptions in the manual?  Maybe if I read what we say there, I
> could suggest wording for NEWS that will be consistent with our terminology
> in the manual.
> 
> Thanks.

Hi Eli, 

Sorry for the confusion here.
You are right, the term "tagged pointer" is not explained anywhere in the manual. 
Also I couldn't find a section that describes them but uses a different wording.

I think the first time we use this term in GDB is with commit
"Clear non-significant bits of address on memory access".  
The terms tagged address and tagged pointer are used.

A very similar commit for setting a  watchpoint on a tagged address but for AArch64 is:
"Clear non-significant bits of address in watchpoint"

For both commits I couldn't find any documentation/NEWS part.

I will try to come up with a section on tagged addresses/pointers in the manual.

Thanks,
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 v4 2/2] LAM: Enable tagged pointer support for watchpoints.

[Eli Zaretskii]

> From: "Schimpe, Christina" <christina.schimpe@intel.com>
> CC: "gdb-patches@sourceware.org" <gdb-patches@sourceware.org>, "Willgerodt,
>  Felix" <felix.willgerodt@intel.com>, "luis.machado@arm.com"
> 	<luis.machado@arm.com>
> Date: Fri, 5 Jul 2024 08:53:29 +0000
> 
> > > From: "Schimpe, Christina" <christina.schimpe@intel.com>
> > > CC: "gdb-patches@sourceware.org" <gdb-patches@sourceware.org>,
> > > "Willgerodt,  Felix" <felix.willgerodt@intel.com>, "luis.machado@arm.com"
> > > 	<luis.machado@arm.com>
> > > Date: Thu, 4 Jul 2024 13:15:20 +0000
> > >
> > > So to answer your question properly: No, we don't explain the term "
> > tagged data pointers" anywhere.
> > > But we write for instance in the commit message "tagged pointers used
> > > for data accesses", which is understandable I think.
> > 
> > Well, I'm sorry to say that it doesn't explain this enough for me.
> > I'm quite a heavy user of GDB, but I have no idea what is a tagged pointer for
> > data access.  Do we have in the manual anything related to such pointers,
> > even if we use different words to talk about them?  If so, could you point me
> > to those descriptions in the manual?  Maybe if I read what we say there, I
> > could suggest wording for NEWS that will be consistent with our terminology
> > in the manual.
> > 
> > Thanks.
> 
> Hi Eli, 
> 
> Sorry for the confusion here.
> You are right, the term "tagged pointer" is not explained anywhere in the manual. 
> Also I couldn't find a section that describes them but uses a different wording.
> 
> I think the first time we use this term in GDB is with commit
> "Clear non-significant bits of address on memory access".  
> The terms tagged address and tagged pointer are used.
> 
> A very similar commit for setting a  watchpoint on a tagged address but for AArch64 is:
> "Clear non-significant bits of address in watchpoint"
> 
> For both commits I couldn't find any documentation/NEWS part.
> 
> I will try to come up with a section on tagged addresses/pointers in the manual.

Thank you.

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

[Schimpe, Christina]

> > From: "Schimpe, Christina" <christina.schimpe@intel.com>
> > CC: "gdb-patches@sourceware.org" <gdb-patches@sourceware.org>,
> > "Willgerodt,  Felix" <felix.willgerodt@intel.com>, "luis.machado@arm.com"
> > 	<luis.machado@arm.com>
> > Date: Fri, 5 Jul 2024 08:53:29 +0000
> >
> > > > From: "Schimpe, Christina" <christina.schimpe@intel.com>
> > > > CC: "gdb-patches@sourceware.org" <gdb-patches@sourceware.org>,
> > > > "Willgerodt,  Felix" <felix.willgerodt@intel.com>,
> "luis.machado@arm.com"
> > > > 	<luis.machado@arm.com>
> > > > Date: Thu, 4 Jul 2024 13:15:20 +0000
> > > >
> > > > So to answer your question properly: No, we don't explain the term "
> > > tagged data pointers" anywhere.
> > > > But we write for instance in the commit message "tagged pointers
> > > > used for data accesses", which is understandable I think.
> > >
> > > Well, I'm sorry to say that it doesn't explain this enough for me.
> > > I'm quite a heavy user of GDB, but I have no idea what is a tagged
> > > pointer for data access.  Do we have in the manual anything related
> > > to such pointers, even if we use different words to talk about them?
> > > If so, could you point me to those descriptions in the manual?
> > > Maybe if I read what we say there, I could suggest wording for NEWS
> > > that will be consistent with our terminology in the manual.
> > >
> > > Thanks.
> >
> > Hi Eli,
> >
> > Sorry for the confusion here.
> > You are right, the term "tagged pointer" is not explained anywhere in the
> manual.
> > Also I couldn't find a section that describes them but uses a different
> wording.
> >
> > I think the first time we use this term in GDB is with commit "Clear
> > non-significant bits of address on memory access".
> > The terms tagged address and tagged pointer are used.
> >
> > A very similar commit for setting a  watchpoint on a tagged address but for
> AArch64 is:
> > "Clear non-significant bits of address in watchpoint"
> >
> > For both commits I couldn't find any documentation/NEWS part.
> >
> > I will try to come up with a section on tagged addresses/pointers in the
> manual.
> 
> Thank you.

Hi Eli, 

I discussed this internally and we still wonder whether we need to document tagged
pointers. I think I missed to mention one important detail, which is that tagged
pointers are no gdb concept but a generic concept: 
https://en.wikipedia.org/wiki/Tagged_pointer.

Do we need to document such generic concepts, too?

Thanks,
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 v4 2/2] LAM: Enable tagged pointer support for watchpoints.

[Eli Zaretskii]

> From: "Schimpe, Christina" <christina.schimpe@intel.com>
> CC: "gdb-patches@sourceware.org" <gdb-patches@sourceware.org>, "Willgerodt,
>  Felix" <felix.willgerodt@intel.com>, "luis.machado@arm.com"
> 	<luis.machado@arm.com>
> Date: Mon, 8 Jul 2024 08:52:17 +0000
> 
> > > I will try to come up with a section on tagged addresses/pointers in the
> > manual.
> > 
> > Thank you.
> 
> Hi Eli, 
> 
> I discussed this internally and we still wonder whether we need to document tagged
> pointers. I think I missed to mention one important detail, which is that tagged
> pointers are no gdb concept but a generic concept: 
> https://en.wikipedia.org/wiki/Tagged_pointer.
> 
> Do we need to document such generic concepts, too?

I think this discussion moved too far off the main issue here.  So let
me try to bring the main issue back into focus.

The original NEWS item said just this:

  * GDB now supports watchpoints for tagged data pointers on amd64.

Let's put ourselves in the shoes of a GDB user, which just happens to
run and debug programs on an amd64 machine, and try to imagine what
will such a user understand from this entry.  Here are some questions
which I think will come to his/her mind:

  . are all the pointers in my programs "tagged"?
  . if not all pointers are "tagged", when and how will I ever
    encounter "tagged" pointers in my programs?
  . what didn't work with "tagged" pointers until now and will work
    henceforth?

We need to give some hints to the readers, either in NEWS or in the
manual, that will help them answer these questions.  One possibility
is just to place the Wikipedia URL in NEWS.  Another possibility is to
mention "the Intel (R) linear address masking (LAM) feature", which
you so abundantly refer to in the commit log and in the comments, but
neither in NEWS nor in the manual, and maybe tell how to
"LAM57-enable" a program and why is that useful.  (If this is
described somewhere on the Internet, a URL could replace the actual
description.)

IOW, please compare the detailed and clear text you wrote for the
commit log message and the code comments with what you decided to put
in NEWS.  The text in the log basically answers all the questions I
think a reader will ask him/herself, and even includes what seems to
be important details, like which LAM widths are now supported.  Yet
none of that ended up in our docs.  Why not have in NEWS and/or in the
manual at least some of that helpful and clearly described
information?

Thanks.

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

[Schimpe, Christina]

> > Hi Eli,
> >
> > I discussed this internally and we still wonder whether we need to
> > document tagged pointers. I think I missed to mention one important
> > detail, which is that tagged pointers are no gdb concept but a generic
> concept:
> > https://en.wikipedia.org/wiki/Tagged_pointer.
> >
> > Do we need to document such generic concepts, too?
> 
> I think this discussion moved too far off the main issue here.  So let me try to
> bring the main issue back into focus.
> 
> The original NEWS item said just this:
> 
>   * GDB now supports watchpoints for tagged data pointers on amd64.
> 
> Let's put ourselves in the shoes of a GDB user, which just happens to run and
> debug programs on an amd64 machine, and try to imagine what will such a
> user understand from this entry.  Here are some questions which I think will
> come to his/her mind:
> 
>   . are all the pointers in my programs "tagged"?
>   . if not all pointers are "tagged", when and how will I ever
>     encounter "tagged" pointers in my programs?
>   . what didn't work with "tagged" pointers until now and will work
>     henceforth?
> 
> We need to give some hints to the readers, either in NEWS or in the manual,
> that will help them answer these questions.  One possibility is just to place
> the Wikipedia URL in NEWS.  Another possibility is to mention "the Intel (R)
> linear address masking (LAM) feature", which you so abundantly refer to in
> the commit log and in the comments, but neither in NEWS nor in the
> manual, and maybe tell how to "LAM57-enable" a program and why is that
> useful.  (If this is described somewhere on the Internet, a URL could replace
> the actual
> description.)
> 
> IOW, please compare the detailed and clear text you wrote for the commit
> log message and the code comments with what you decided to put in NEWS.
> The text in the log basically answers all the questions I think a reader will ask
> him/herself, and even includes what seems to be important details, like
> which LAM widths are now supported.  Yet none of that ended up in our
> docs.  Why not have in NEWS and/or in the manual at least some of that
> helpful and clearly described information?
> 
> Thanks.

Hi Eli, 

Thanks a lot for your detailed explanations.

I understand that the NEWS text seems rather short, especially compared
to my commit message.
One reason why I kept the NEWS generic (without LAM and its specifics) is
because theoretically the patch should work for AMD's tagged pointer support, too. 
They call their tagged pointer support "upper address ignore" (UAI).

However, I still could mention LAM in that way in the NEWS:  

- GDB now supports watchpoints for tagged data pointers on amd64,
 e.g. as provided by Intel's linear address masking (LAM) feature.

or 

- GDB now supports watchpoints for tagged data pointers on amd64. 
For more information on tagged data pointers, see
https://en.wikipedia.org/wiki/Tagged_pointer.

Although given that I did not test UAI and don't know about its current
state, I think it's better to use my first suggestion.

If the user reads about LAM (or tagged pointer support in general), I 
hope he should be able to answer the questions you mentioned:

>   . are all the pointers in my programs "tagged"?
>   . if not all pointers are "tagged", when and how will I ever
>     encounter "tagged" pointers in my programs?
>   . what didn't work with "tagged" pointers until now and will work
>     henceforth?

What do you think?

If you don't like my current suggestion, I have no problem adding a
proper documentation on tagged pointers in the manual or adding
more info to the NEWS. 

And sorry for moving this discussion into a general direction.  I just
want to be sure that I understand this topic in general, such that I
will make it better next time. 😊

Thanks again,
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 v4 2/2] LAM: Enable tagged pointer support for watchpoints.

[Eli Zaretskii]

> From: "Schimpe, Christina" <christina.schimpe@intel.com>
> CC: "gdb-patches@sourceware.org" <gdb-patches@sourceware.org>, "Willgerodt,
>  Felix" <felix.willgerodt@intel.com>, "luis.machado@arm.com"
> 	<luis.machado@arm.com>
> Date: Mon, 8 Jul 2024 14:25:50 +0000
> 
> I understand that the NEWS text seems rather short, especially compared
> to my commit message.
> One reason why I kept the NEWS generic (without LAM and its specifics) is
> because theoretically the patch should work for AMD's tagged pointer support, too. 
> They call their tagged pointer support "upper address ignore" (UAI).
> 
> However, I still could mention LAM in that way in the NEWS:  
> 
> - GDB now supports watchpoints for tagged data pointers on amd64,
>  e.g. as provided by Intel's linear address masking (LAM) feature.
> 
> or 
> 
> - GDB now supports watchpoints for tagged data pointers on amd64. 
> For more information on tagged data pointers, see
> https://en.wikipedia.org/wiki/Tagged_pointer.
> 
> Although given that I did not test UAI and don't know about its current
> state, I think it's better to use my first suggestion.

I think we can do both:

  GDB now supports watchpoints for tagged data pointers (see
  https://en.wikipedia.org/wiki/Tagged_pointer) on amd64, such as the
  one used by the Linear Address Masking (LAM) feature provided by
  Intel.

> If you don't like my current suggestion, I have no problem adding a
> proper documentation on tagged pointers in the manual or adding
> more info to the NEWS. 

An informative enough NEWS entry should be fine in this case, I think.

Thanks.

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

[Schimpe, Christina]

> > From: "Schimpe, Christina" <christina.schimpe@intel.com>
> > CC: "gdb-patches@sourceware.org" <gdb-patches@sourceware.org>,
> > "Willgerodt,  Felix" <felix.willgerodt@intel.com>, "luis.machado@arm.com"
> > 	<luis.machado@arm.com>
> > Date: Mon, 8 Jul 2024 14:25:50 +0000
> >
> > I understand that the NEWS text seems rather short, especially
> > compared to my commit message.
> > One reason why I kept the NEWS generic (without LAM and its specifics)
> > is because theoretically the patch should work for AMD's tagged pointer
> support, too.
> > They call their tagged pointer support "upper address ignore" (UAI).
> >
> > However, I still could mention LAM in that way in the NEWS:
> >
> > - GDB now supports watchpoints for tagged data pointers on amd64,
> > e.g. as provided by Intel's linear address masking (LAM) feature.
> >
> > or
> >
> > - GDB now supports watchpoints for tagged data pointers on amd64.
> > For more information on tagged data pointers, see
> > https://en.wikipedia.org/wiki/Tagged_pointer.
> >
> > Although given that I did not test UAI and don't know about its
> > current state, I think it's better to use my first suggestion.
> 
> I think we can do both:
> 
>   GDB now supports watchpoints for tagged data pointers (see
>   https://en.wikipedia.org/wiki/Tagged_pointer) on amd64, such as the
>   one used by the Linear Address Masking (LAM) feature provided by
>   Intel.
> 
> > If you don't like my current suggestion, I have no problem adding a
> > proper documentation on tagged pointers in the manual or adding more
> > info to the NEWS.
> 
> An informative enough NEWS entry should be fine in this case, I think.
> 
> Thanks.

Sounds good to me, thank you. I'll post a new version of this series soon.

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 v4 2/2] LAM: Enable tagged pointer support for watchpoints.

[Eli Zaretskii]

> From: "Schimpe, Christina" <christina.schimpe@intel.com>
> CC: "gdb-patches@sourceware.org" <gdb-patches@sourceware.org>, "Willgerodt,
>  Felix" <felix.willgerodt@intel.com>, "luis.machado@arm.com"
> 	<luis.machado@arm.com>
> Date: Thu, 4 Jul 2024 13:05:35 +0000
> 
> > > diff --git a/gdb/NEWS b/gdb/NEWS
> > > index 47677cb773a..fcbb89e8bf8 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.
> > > +
> > >  * Debugger Adapter Protocol changes
> > 
> > Thanks.  The above NEWS text is okay, but I wonder whether we explain
> > anywhere what are "tagged data pointers".
> > 
> > Reviewed-By: Eli Zaretskii <eliz@gnu.org>
> 
> Thanks for the review. I understand this can be a bit confusing. 
> Is " GDB now supports watchpoints for tagged pointers used for data accesses on amd64." better?
> Or, we could also just write: " GDB now supports watchpoints for tagged pointers on amd64.", as watchpoints are usually just data breakpoints.
> 
> What do you think?

My bother is actually about what we say in the manual, not in NEWS.
If the manual explains this terminology, it is fine to leave it
unexplained in NEWS, as users are expected to turn to the manual for
the details of the NEWS entries they are interested in.  But if this
is not explained in the manual, then we should either explain it, or
do that in NEWS entry itself, and in the latter case I'm afraid your
suggestion above is not enough because "tagged pointers" are not
explained anywhere, either.