[PATCH 0/3] Add amd64 LAM watchpoint support

[PATCH 0/3] Add amd64 LAM watchpoint support

[Schimpe, Christina]

Hi all,

This is a series to add watchpoint support with Liner Address Masking (LAM)
on amd64.

I am looking forward to your feedback!

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              |  3 +-
 gdb/aarch64-linux-tdep.c             | 14 ++---
 gdb/aarch64-tdep.c                   | 12 +++--
 gdb/amd64-linux-tdep.c               | 80 ++++++++++++++++++++++++++++
 gdb/breakpoint.c                     |  4 +-
 gdb/gdbarch-gen.h                    | 50 +++++++++++++----
 gdb/gdbarch.c                        | 66 +++++++++++++++++++----
 gdb/gdbarch_components.py            | 54 ++++++++++++++++---
 gdb/target.c                         |  4 +-
 gdb/testsuite/gdb.arch/amd64-lam.c   | 49 +++++++++++++++++
 gdb/testsuite/gdb.arch/amd64-lam.exp | 45 ++++++++++++++++
 gdb/testsuite/lib/gdb.exp            | 62 +++++++++++++++++++++
 13 files changed, 401 insertions(+), 44 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 <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

[PATCH 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 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).
---
 gdb/aarch64-linux-nat.c   |  3 +-
 gdb/aarch64-linux-tdep.c  | 14 +++++----
 gdb/aarch64-tdep.c        | 12 +++++--
 gdb/breakpoint.c          |  4 +--
 gdb/gdbarch-gen.h         | 50 ++++++++++++++++++++++-------
 gdb/gdbarch.c             | 66 ++++++++++++++++++++++++++++++++-------
 gdb/gdbarch_components.py | 54 +++++++++++++++++++++++++++-----
 gdb/target.c              |  4 +--
 8 files changed, 163 insertions(+), 44 deletions(-)

diff --git a/gdb/aarch64-linux-nat.c b/gdb/aarch64-linux-nat.c
index 3face34ce79..bd02a7b2d08 100644
--- a/gdb/aarch64-linux-nat.c
+++ b/gdb/aarch64-linux-nat.c
@@ -959,7 +959,8 @@ aarch64_linux_nat_target::stopped_data_address (CORE_ADDR *addr_p)
      kernel can potentially be tagged addresses.  */
   struct gdbarch *gdbarch = thread_architecture (inferior_ptid);
   const CORE_ADDR addr_trap
-    = gdbarch_remove_non_address_bits (gdbarch, (CORE_ADDR) siginfo.si_addr);
+    = gdbarch_remove_non_addr_bits_memory (gdbarch,
+					   (CORE_ADDR) siginfo.si_addr);
 
   /* Check if the address matches any watched address.  */
   state = aarch64_get_debug_reg_state (inferior_ptid.pid ());
diff --git a/gdb/aarch64-linux-tdep.c b/gdb/aarch64-linux-tdep.c
index 0b9784f38e4..d2d42efe305 100644
--- a/gdb/aarch64-linux-tdep.c
+++ b/gdb/aarch64-linux-tdep.c
@@ -2458,7 +2458,7 @@ aarch64_linux_tagged_address_p (struct gdbarch *gdbarch, struct value *address)
   CORE_ADDR addr = value_as_address (address);
 
   /* Remove the top byte for the memory range check.  */
-  addr = gdbarch_remove_non_address_bits (gdbarch, addr);
+  addr = gdbarch_remove_non_addr_bits_memory (gdbarch, addr);
 
   /* Check if the page that contains ADDRESS is mapped with PROT_MTE.  */
   if (!linux_address_in_memtag_page (addr))
@@ -2484,7 +2484,8 @@ aarch64_linux_memtag_matches_p (struct gdbarch *gdbarch,
 
   /* Fetch the allocation tag for ADDRESS.  */
   std::optional<CORE_ADDR> atag
-    = aarch64_mte_get_atag (gdbarch_remove_non_address_bits (gdbarch, addr));
+    = aarch64_mte_get_atag (gdbarch_remove_non_addr_bits_memory (gdbarch,
+								 addr));
 
   if (!atag.has_value ())
     return true;
@@ -2523,7 +2524,7 @@ aarch64_linux_set_memtags (struct gdbarch *gdbarch, struct value *address,
   else
     {
       /* Remove the top byte.  */
-      addr = gdbarch_remove_non_address_bits (gdbarch, addr);
+      addr = gdbarch_remove_non_addr_bits_memory (gdbarch, addr);
 
       /* Make sure we are dealing with a tagged address to begin with.  */
       if (!aarch64_linux_tagged_address_p (gdbarch, address))
@@ -2580,7 +2581,7 @@ aarch64_linux_get_memtag (struct gdbarch *gdbarch, struct value *address,
 	return nullptr;
 
       /* Remove the top byte.  */
-      addr = gdbarch_remove_non_address_bits (gdbarch, addr);
+      addr = gdbarch_remove_non_addr_bits_memory (gdbarch, addr);
       std::optional<CORE_ADDR> atag = aarch64_mte_get_atag (addr);
 
       if (!atag.has_value ())
@@ -2654,8 +2655,9 @@ aarch64_linux_report_signal_info (struct gdbarch *gdbarch,
       uiout->text ("\n");
 
       std::optional<CORE_ADDR> atag
-	= aarch64_mte_get_atag (gdbarch_remove_non_address_bits (gdbarch,
-								 fault_addr));
+	= aarch64_mte_get_atag (
+	    gdbarch_remove_non_addr_bits_memory (gdbarch, fault_addr));
+
       gdb_byte ltag = aarch64_mte_get_ltag (fault_addr);
 
       if (!atag.has_value ())
diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
index 545ec872fd8..86d620bd181 100644
--- a/gdb/aarch64-tdep.c
+++ b/gdb/aarch64-tdep.c
@@ -4582,9 +4582,15 @@ aarch64_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
     tdep->ra_sign_state_regnum = ra_sign_state_offset + num_regs;
 
   /* Architecture hook to remove bits of a pointer that are not part of the
-     address, like memory tags (MTE) and pointer authentication signatures.  */
-  set_gdbarch_remove_non_address_bits (gdbarch,
-				       aarch64_remove_non_address_bits);
+     address, like memory tags (MTE) and pointer authentication signatures.
+     Configure address adjustment for watch-, breakpoints and memory
+     transfer.  */
+  set_gdbarch_remove_non_addr_bits_wpt (gdbarch,
+					aarch64_remove_non_address_bits);
+  set_gdbarch_remove_non_addr_bits_bpt (gdbarch,
+					aarch64_remove_non_address_bits);
+  set_gdbarch_remove_non_addr_bits_memory (gdbarch,
+					   aarch64_remove_non_address_bits);
 
   /* SME pseudo-registers.  */
   if (tdep->has_sme ())
diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 053d17df03e..24b322b9bb4 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -2234,7 +2234,7 @@ update_watchpoint (struct watchpoint *b, bool reparse)
 		  loc->gdbarch = v->type ()->arch ();
 		  loc->pspace = frame_pspace;
 		  loc->address
-		    = gdbarch_remove_non_address_bits (loc->gdbarch, addr);
+		    = gdbarch_remove_non_addr_bits_wpt (loc->gdbarch, addr);
 		  b->add_location (*loc);
 
 		  if (bitsize != 0)
@@ -7473,7 +7473,7 @@ adjust_breakpoint_address (struct gdbarch *gdbarch,
 	}
 
       adjusted_bpaddr
-	= gdbarch_remove_non_address_bits (gdbarch, adjusted_bpaddr);
+	= gdbarch_remove_non_addr_bits_bpt (gdbarch, adjusted_bpaddr);
 
       /* An adjusted breakpoint address can significantly alter
 	 a user's expectations.  Print a warning if an adjustment
diff --git a/gdb/gdbarch-gen.h b/gdb/gdbarch-gen.h
index ebcff80bb9e..ec296c86668 100644
--- a/gdb/gdbarch-gen.h
+++ b/gdb/gdbarch-gen.h
@@ -684,19 +684,47 @@ extern CORE_ADDR gdbarch_addr_bits_remove (struct gdbarch *gdbarch, CORE_ADDR ad
 extern void set_gdbarch_addr_bits_remove (struct gdbarch *gdbarch, gdbarch_addr_bits_remove_ftype *addr_bits_remove);
 
 /* On some architectures, not all bits of a pointer are significant.
-   On AArch64, for example, the top bits of a pointer may carry a "tag", which
-   can be ignored by the kernel and the hardware.  The "tag" can be regarded as
-   additional data associated with the pointer, but it is not part of the address.
+   On AArch64 and amd64, for example, the top bits of a pointer may carry a
+   "tag", which can be ignored by the kernel and the hardware.  The "tag" can be
+   regarded as additional data associated with the pointer, but it is not part
+   of the address.
 
    Given a pointer for the architecture, this hook removes all the
-   non-significant bits and sign-extends things as needed.  It gets used to remove
-   non-address bits from data pointers (for example, removing the AArch64 MTE tag
-   bits from a pointer) and from code pointers (removing the AArch64 PAC signature
-   from a pointer containing the return address). */
-
-typedef CORE_ADDR (gdbarch_remove_non_address_bits_ftype) (struct gdbarch *gdbarch, CORE_ADDR pointer);
-extern CORE_ADDR gdbarch_remove_non_address_bits (struct gdbarch *gdbarch, CORE_ADDR pointer);
-extern void set_gdbarch_remove_non_address_bits (struct gdbarch *gdbarch, gdbarch_remove_non_address_bits_ftype *remove_non_address_bits);
+   non-significant bits and sign-extends things as needed.  It gets used to
+   remove non-address bits from pointers used for watchpoints. */
+
+typedef CORE_ADDR (gdbarch_remove_non_addr_bits_wpt_ftype) (struct gdbarch *gdbarch, CORE_ADDR pointer);
+extern CORE_ADDR gdbarch_remove_non_addr_bits_wpt (struct gdbarch *gdbarch, CORE_ADDR pointer);
+extern void set_gdbarch_remove_non_addr_bits_wpt (struct gdbarch *gdbarch, gdbarch_remove_non_addr_bits_wpt_ftype *remove_non_addr_bits_wpt);
+
+/* On some architectures, not all bits of a pointer are significant.
+   On AArch64 and amd64, for example, the top bits of a pointer may carry a
+   "tag", which can be ignored by the kernel and the hardware.  The "tag" can be
+   regarded as additional data associated with the pointer, but it is not part
+   of the address.
+
+   Given a pointer for the architecture, this hook removes all the
+   non-significant bits and sign-extends things as needed.  It gets used to
+   remove non-address bits from pointers used for breakpoints. */
+
+typedef CORE_ADDR (gdbarch_remove_non_addr_bits_bpt_ftype) (struct gdbarch *gdbarch, CORE_ADDR pointer);
+extern CORE_ADDR gdbarch_remove_non_addr_bits_bpt (struct gdbarch *gdbarch, CORE_ADDR pointer);
+extern void set_gdbarch_remove_non_addr_bits_bpt (struct gdbarch *gdbarch, gdbarch_remove_non_addr_bits_bpt_ftype *remove_non_addr_bits_bpt);
+
+/* On some architectures, not all bits of a pointer are significant.
+   On AArch64 and amd64, for example, the top bits of a pointer may carry a
+   "tag", which can be ignored by the kernel and the hardware.  The "tag" can be
+   regarded as additional data associated with the pointer, but it is not part
+   of the address.
+
+   Given a pointer for the architecture, this hook removes all the
+   non-significant bits and sign-extends things as needed.  It gets used to
+   remove non-address bits from any pointer used to access memory (called in
+   memory_xfer_partial). */
+
+typedef CORE_ADDR (gdbarch_remove_non_addr_bits_memory_ftype) (struct gdbarch *gdbarch, CORE_ADDR pointer);
+extern CORE_ADDR gdbarch_remove_non_addr_bits_memory (struct gdbarch *gdbarch, CORE_ADDR pointer);
+extern void set_gdbarch_remove_non_addr_bits_memory (struct gdbarch *gdbarch, gdbarch_remove_non_addr_bits_memory_ftype *remove_non_addr_bits_memory);
 
 /* Return a string representation of the memory tag TAG. */
 
diff --git a/gdb/gdbarch.c b/gdb/gdbarch.c
index 9319571deba..f59e090d294 100644
--- a/gdb/gdbarch.c
+++ b/gdb/gdbarch.c
@@ -143,7 +143,9 @@ struct gdbarch
   int frame_red_zone_size = 0;
   gdbarch_convert_from_func_ptr_addr_ftype *convert_from_func_ptr_addr = convert_from_func_ptr_addr_identity;
   gdbarch_addr_bits_remove_ftype *addr_bits_remove = core_addr_identity;
-  gdbarch_remove_non_address_bits_ftype *remove_non_address_bits = default_remove_non_address_bits;
+  gdbarch_remove_non_addr_bits_wpt_ftype *remove_non_addr_bits_wpt = default_remove_non_address_bits;
+  gdbarch_remove_non_addr_bits_bpt_ftype *remove_non_addr_bits_bpt = default_remove_non_address_bits;
+  gdbarch_remove_non_addr_bits_memory_ftype *remove_non_addr_bits_memory = default_remove_non_address_bits;
   gdbarch_memtag_to_string_ftype *memtag_to_string = default_memtag_to_string;
   gdbarch_tagged_address_p_ftype *tagged_address_p = default_tagged_address_p;
   gdbarch_memtag_matches_p_ftype *memtag_matches_p = default_memtag_matches_p;
@@ -407,7 +409,9 @@ verify_gdbarch (struct gdbarch *gdbarch)
   /* Skip verify of frame_red_zone_size, invalid_p == 0 */
   /* Skip verify of convert_from_func_ptr_addr, invalid_p == 0 */
   /* Skip verify of addr_bits_remove, invalid_p == 0 */
-  /* Skip verify of remove_non_address_bits, invalid_p == 0 */
+  /* Skip verify of remove_non_addr_bits_wpt, invalid_p == 0 */
+  /* Skip verify of remove_non_addr_bits_bpt, invalid_p == 0 */
+  /* Skip verify of remove_non_addr_bits_memory, invalid_p == 0 */
   /* Skip verify of memtag_to_string, invalid_p == 0 */
   /* Skip verify of tagged_address_p, invalid_p == 0 */
   /* Skip verify of memtag_matches_p, invalid_p == 0 */
@@ -910,8 +914,14 @@ gdbarch_dump (struct gdbarch *gdbarch, struct ui_file *file)
 	      "gdbarch_dump: addr_bits_remove = <%s>\n",
 	      host_address_to_string (gdbarch->addr_bits_remove));
   gdb_printf (file,
-	      "gdbarch_dump: remove_non_address_bits = <%s>\n",
-	      host_address_to_string (gdbarch->remove_non_address_bits));
+	      "gdbarch_dump: remove_non_addr_bits_wpt = <%s>\n",
+	      host_address_to_string (gdbarch->remove_non_addr_bits_wpt));
+  gdb_printf (file,
+	      "gdbarch_dump: remove_non_addr_bits_bpt = <%s>\n",
+	      host_address_to_string (gdbarch->remove_non_addr_bits_bpt));
+  gdb_printf (file,
+	      "gdbarch_dump: remove_non_addr_bits_memory = <%s>\n",
+	      host_address_to_string (gdbarch->remove_non_addr_bits_memory));
   gdb_printf (file,
 	      "gdbarch_dump: memtag_to_string = <%s>\n",
 	      host_address_to_string (gdbarch->memtag_to_string));
@@ -3198,20 +3208,54 @@ set_gdbarch_addr_bits_remove (struct gdbarch *gdbarch,
 }
 
 CORE_ADDR
-gdbarch_remove_non_address_bits (struct gdbarch *gdbarch, CORE_ADDR pointer)
+gdbarch_remove_non_addr_bits_wpt (struct gdbarch *gdbarch, CORE_ADDR pointer)
+{
+  gdb_assert (gdbarch != NULL);
+  gdb_assert (gdbarch->remove_non_addr_bits_wpt != NULL);
+  if (gdbarch_debug >= 2)
+    gdb_printf (gdb_stdlog, "gdbarch_remove_non_addr_bits_wpt called\n");
+  return gdbarch->remove_non_addr_bits_wpt (gdbarch, pointer);
+}
+
+void
+set_gdbarch_remove_non_addr_bits_wpt (struct gdbarch *gdbarch,
+				      gdbarch_remove_non_addr_bits_wpt_ftype remove_non_addr_bits_wpt)
+{
+  gdbarch->remove_non_addr_bits_wpt = remove_non_addr_bits_wpt;
+}
+
+CORE_ADDR
+gdbarch_remove_non_addr_bits_bpt (struct gdbarch *gdbarch, CORE_ADDR pointer)
+{
+  gdb_assert (gdbarch != NULL);
+  gdb_assert (gdbarch->remove_non_addr_bits_bpt != NULL);
+  if (gdbarch_debug >= 2)
+    gdb_printf (gdb_stdlog, "gdbarch_remove_non_addr_bits_bpt called\n");
+  return gdbarch->remove_non_addr_bits_bpt (gdbarch, pointer);
+}
+
+void
+set_gdbarch_remove_non_addr_bits_bpt (struct gdbarch *gdbarch,
+				      gdbarch_remove_non_addr_bits_bpt_ftype remove_non_addr_bits_bpt)
+{
+  gdbarch->remove_non_addr_bits_bpt = remove_non_addr_bits_bpt;
+}
+
+CORE_ADDR
+gdbarch_remove_non_addr_bits_memory (struct gdbarch *gdbarch, CORE_ADDR pointer)
 {
   gdb_assert (gdbarch != NULL);
-  gdb_assert (gdbarch->remove_non_address_bits != NULL);
+  gdb_assert (gdbarch->remove_non_addr_bits_memory != NULL);
   if (gdbarch_debug >= 2)
-    gdb_printf (gdb_stdlog, "gdbarch_remove_non_address_bits called\n");
-  return gdbarch->remove_non_address_bits (gdbarch, pointer);
+    gdb_printf (gdb_stdlog, "gdbarch_remove_non_addr_bits_memory called\n");
+  return gdbarch->remove_non_addr_bits_memory (gdbarch, pointer);
 }
 
 void
-set_gdbarch_remove_non_address_bits (struct gdbarch *gdbarch,
-				     gdbarch_remove_non_address_bits_ftype remove_non_address_bits)
+set_gdbarch_remove_non_addr_bits_memory (struct gdbarch *gdbarch,
+					 gdbarch_remove_non_addr_bits_memory_ftype remove_non_addr_bits_memory)
 {
-  gdbarch->remove_non_address_bits = remove_non_address_bits;
+  gdbarch->remove_non_addr_bits_memory = remove_non_addr_bits_memory;
 }
 
 std::string
diff --git a/gdb/gdbarch_components.py b/gdb/gdbarch_components.py
index 7d913ade621..555bc4707c5 100644
--- a/gdb/gdbarch_components.py
+++ b/gdb/gdbarch_components.py
@@ -1232,18 +1232,56 @@ possible it should be in TARGET_READ_PC instead).
 Method(
     comment="""
 On some architectures, not all bits of a pointer are significant.
-On AArch64, for example, the top bits of a pointer may carry a "tag", which
-can be ignored by the kernel and the hardware.  The "tag" can be regarded as
-additional data associated with the pointer, but it is not part of the address.
+On AArch64 and amd64, for example, the top bits of a pointer may carry a
+"tag", which can be ignored by the kernel and the hardware.  The "tag" can be
+regarded as additional data associated with the pointer, but it is not part
+of the address.
 
 Given a pointer for the architecture, this hook removes all the
-non-significant bits and sign-extends things as needed.  It gets used to remove
-non-address bits from data pointers (for example, removing the AArch64 MTE tag
-bits from a pointer) and from code pointers (removing the AArch64 PAC signature
-from a pointer containing the return address).
+non-significant bits and sign-extends things as needed.  It gets used to
+remove non-address bits from pointers used for watchpoints.
 """,
     type="CORE_ADDR",
-    name="remove_non_address_bits",
+    name="remove_non_addr_bits_wpt",
+    params=[("CORE_ADDR", "pointer")],
+    predefault="default_remove_non_address_bits",
+    invalid=False,
+)
+
+Method(
+    comment="""
+On some architectures, not all bits of a pointer are significant.
+On AArch64 and amd64, for example, the top bits of a pointer may carry a
+"tag", which can be ignored by the kernel and the hardware.  The "tag" can be
+regarded as additional data associated with the pointer, but it is not part
+of the address.
+
+Given a pointer for the architecture, this hook removes all the
+non-significant bits and sign-extends things as needed.  It gets used to
+remove non-address bits from pointers used for breakpoints.
+""",
+    type="CORE_ADDR",
+    name="remove_non_addr_bits_bpt",
+    params=[("CORE_ADDR", "pointer")],
+    predefault="default_remove_non_address_bits",
+    invalid=False,
+)
+
+Method(
+    comment="""
+On some architectures, not all bits of a pointer are significant.
+On AArch64 and amd64, for example, the top bits of a pointer may carry a
+"tag", which can be ignored by the kernel and the hardware.  The "tag" can be
+regarded as additional data associated with the pointer, but it is not part
+of the address.
+
+Given a pointer for the architecture, this hook removes all the
+non-significant bits and sign-extends things as needed.  It gets used to
+remove non-address bits from any pointer used to access memory (called in
+memory_xfer_partial).
+""",
+    type="CORE_ADDR",
+    name="remove_non_addr_bits_memory",
     params=[("CORE_ADDR", "pointer")],
     predefault="default_remove_non_address_bits",
     invalid=False,
diff --git a/gdb/target.c b/gdb/target.c
index 107a84b3ca1..586eee2ee73 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -1597,8 +1597,8 @@ memory_xfer_partial (struct target_ops *ops, enum target_object object,
   if (len == 0)
     return TARGET_XFER_EOF;
 
-  memaddr = gdbarch_remove_non_address_bits (current_inferior ()->arch (),
-					     memaddr);
+  memaddr = gdbarch_remove_non_addr_bits_memory (current_inferior ()->arch (),
+				  		 memaddr);
 
   /* Fill in READBUF with breakpoint shadows, or WRITEBUF with
      breakpoint insns, thus hiding out from higher layers whether
-- 
2.34.1

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

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

[Luis Machado]

Hi,


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


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

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

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

gdbarch_remove_non_address_bits -> gdbarch_remove_non_addr_bits_memory

Maybe we should...

gdbarch_remove_non_address_bits -> gdbarch_remove_non_address_bits_memory

The same applies to the other hooks and function names.

wpt -> watchpoint

or

wpt -> hw_watchpoint

bpt -> breakpoint

or

bpt -> hw_breakpoint

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

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

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

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

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

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

[Schimpe, Christina]

Hi Luis,

Thanks a lot for your feedback!

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

Hm, so far I don't see an issue with gdbserver.
IIUC, the aarch64 non-address-bits removal with gdbserver is due to the exception
"for signals raised in response to watchpoint debug exceptions" described in commit
"Fix TBI handling for watchpoints".  Is there anything else that I might have missed?
For amd64 we don't have this exception.
 
> Do we need to do anything in gdbserver so it can properly handle removal of
> non-address-bits on its own, without the help of gdb?
> 
> Also, another more general comment is related to the naming of the hooks. I
> suppose it is down to personal preference, but spelling out the entire word is
> likely to make the meaning of a function/hook clearer. For instance:
> 
> gdbarch_remove_non_address_bits ->
> gdbarch_remove_non_addr_bits_memory
> 
> Maybe we should...
> 
> gdbarch_remove_non_address_bits ->
> gdbarch_remove_non_address_bits_memory
> 
> The same applies to the other hooks and function names.
> 
> wpt -> watchpoint
> 
> or
> 
> wpt -> hw_watchpoint
> 
> bpt -> breakpoint
> 
> or
> 
> bpt -> hw_breakpoint
> 

I thought that "addr", "bpt" and "wpt" are well known abbreviations in GDB and
preferred to have shorter function names in that case.
But I am fine with adapting the function names, too.

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

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

[Luis Machado]

On 4/4/24 09:18, Schimpe, Christina wrote:
> Hi Luis,
> 
> Thanks a lot for your feedback!
> 
>> Thanks for the series. More of a general question, we're splitting the more
>> general handling of bit removal from addresses into 3 different categories in
>> gdb. Isn't gdbserver also affected by this change? At least for aarch64 we have
>> non-address-bits removal.
> 
> Hm, so far I don't see an issue with gdbserver.
> IIUC, the aarch64 non-address-bits removal with gdbserver is due to the exception
> "for signals raised in response to watchpoint debug exceptions" described in commit
> "Fix TBI handling for watchpoints".  Is there anything else that I might have missed?
> For amd64 we don't have this exception.
>  

Ah, indeed. You're right.

>> Do we need to do anything in gdbserver so it can properly handle removal of
>> non-address-bits on its own, without the help of gdb?
>>
>> Also, another more general comment is related to the naming of the hooks. I
>> suppose it is down to personal preference, but spelling out the entire word is
>> likely to make the meaning of a function/hook clearer. For instance:
>>
>> gdbarch_remove_non_address_bits ->
>> gdbarch_remove_non_addr_bits_memory
>>
>> Maybe we should...
>>
>> gdbarch_remove_non_address_bits ->
>> gdbarch_remove_non_address_bits_memory
>>
>> The same applies to the other hooks and function names.
>>
>> wpt -> watchpoint
>>
>> or
>>
>> wpt -> hw_watchpoint
>>
>> bpt -> breakpoint
>>
>> or
>>
>> bpt -> hw_breakpoint
>>
> 
> I thought that "addr", "bpt" and "wpt" are well known abbreviations in GDB and
> preferred to have shorter function names in that case.
> But I am fine with adapting the function names, too.

It is (and used to be). Nowadays there seems to be a slight preference for being a bit more verbose and clear.

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

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

[Willgerodt, Felix]

> -----Original Message-----
> From: Schimpe, Christina <christina.schimpe@intel.com>
> Sent: Mittwoch, 27. März 2024 08:48
> To: gdb-patches@sourceware.org
> Cc: Schimpe, Christina <christina.schimpe@intel.com>
> Subject: [PATCH 1/3] gdb: Make tagged pointer support configurable.
> 
> From: Christina Schimpe <christina.schimpe@intel.com>
> 
> The gdbarch function gdbarch_remove_non_address_bits adjusts addresses to
> enable debugging of programs with tagged pointers on Linux, for instance for
> ARM's feature top byte ignore (TBI).
> Once the function is implemented for an architecture, it adjusts addresses for
> memory access, breakpoints and watchpoints.
> 
> Linear address masking (LAM) is Intel's (R) implementation of tagged
> pointer support.  It requires certain adaptions to GDB's tagged pointer
> support due to the following:
> - LAM supports address tagging for data accesses only.  Thus, specifying
>   breakpoints on tagged addresses is not a valid use case.
> - In contrast to the implementation for ARM's TBI, the kernel supports tagged
>   pointers for memory access.

Should we write the "Linux kernel"?

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


> diff --git a/gdb/gdbarch_components.py b/gdb/gdbarch_components.py
> index 7d913ade621..555bc4707c5 100644
> --- a/gdb/gdbarch_components.py
> +++ b/gdb/gdbarch_components.py
> @@ -1232,18 +1232,56 @@ possible it should be in TARGET_READ_PC instead).
>  Method(
>      comment="""
>  On some architectures, not all bits of a pointer are significant.
> -On AArch64, for example, the top bits of a pointer may carry a "tag", which
> -can be ignored by the kernel and the hardware.  The "tag" can be regarded as
> -additional data associated with the pointer, but it is not part of the address.
> +On AArch64 and amd64, for example, the top bits of a pointer may carry a
> +"tag", which can be ignored by the kernel and the hardware.  The "tag" can be
> +regarded as additional data associated with the pointer, but it is not part
> +of the address.
> 
>  Given a pointer for the architecture, this hook removes all the
> -non-significant bits and sign-extends things as needed.  It gets used to remove
> -non-address bits from data pointers (for example, removing the AArch64 MTE tag
> -bits from a pointer) and from code pointers (removing the AArch64 PAC signature
> -from a pointer containing the return address).
> +non-significant bits and sign-extends things as needed.  It gets used to
> +remove non-address bits from pointers used for watchpoints.
>  """,
>      type="CORE_ADDR",
> -    name="remove_non_address_bits",
> +    name="remove_non_addr_bits_wpt",
> +    params=[("CORE_ADDR", "pointer")],
> +    predefault="default_remove_non_address_bits",
> +    invalid=False,
> +)
> +
> +Method(
> +    comment="""
> +On some architectures, not all bits of a pointer are significant.
> +On AArch64 and amd64, for example, the top bits of a pointer may carry a
> +"tag", which can be ignored by the kernel and the hardware.  The "tag" can be
> +regarded as additional data associated with the pointer, but it is not part
> +of the address.
> +
> +Given a pointer for the architecture, this hook removes all the
> +non-significant bits and sign-extends things as needed.  It gets used to
> +remove non-address bits from pointers used for breakpoints.
> +""",
> +    type="CORE_ADDR",
> +    name="remove_non_addr_bits_bpt",
> +    params=[("CORE_ADDR", "pointer")],
> +    predefault="default_remove_non_address_bits",
> +    invalid=False,
> +)
> +
> +Method(
> +    comment="""
> +On some architectures, not all bits of a pointer are significant.
> +On AArch64 and amd64, for example, the top bits of a pointer may carry a
> +"tag", which can be ignored by the kernel and the hardware.  The "tag" can be
> +regarded as additional data associated with the pointer, but it is not part
> +of the address.
> +
> +Given a pointer for the architecture, this hook removes all the
> +non-significant bits and sign-extends things as needed.  It gets used to
> +remove non-address bits from any pointer used to access memory (called in
> +memory_xfer_partial).

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


> +""",
> +    type="CORE_ADDR",
> +    name="remove_non_addr_bits_memory",
>      params=[("CORE_ADDR", "pointer")],
>      predefault="default_remove_non_address_bits",
>      invalid=False,
> diff --git a/gdb/target.c b/gdb/target.c
> index 107a84b3ca1..586eee2ee73 100644
> --- a/gdb/target.c
> +++ b/gdb/target.c
> @@ -1597,8 +1597,8 @@ memory_xfer_partial (struct target_ops *ops, enum
> target_object object,
>    if (len == 0)
>      return TARGET_XFER_EOF;
> 
> -  memaddr = gdbarch_remove_non_address_bits (current_inferior ()->arch (),
> -					     memaddr);
> +  memaddr = gdbarch_remove_non_addr_bits_memory (current_inferior ()-
> >arch (),
> +				  		 memaddr);

When I apply your patch I get this warning here:

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

That should be fixed.

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

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

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

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

[PATCH 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               | 67 ++++++++++++++++++++++++++++
 gdb/testsuite/gdb.arch/amd64-lam.c   | 49 ++++++++++++++++++++
 gdb/testsuite/gdb.arch/amd64-lam.exp | 45 +++++++++++++++++++
 gdb/testsuite/lib/gdb.exp            | 62 +++++++++++++++++++++++++
 5 files changed, 225 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 feb3a37393a..295f2147def 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -3,6 +3,8 @@
 
 *** Changes since GDB 14
 
+* GDB now supports watchpoints for tagged data pointers on amd64.
+
 * The MPX commands "show/set mpx bound" have been deprecated, as Intel
   listed MPX as removed in 2019.
 
diff --git a/gdb/amd64-linux-tdep.c b/gdb/amd64-linux-tdep.c
index 9d560ac4fbf..94c62277623 100644
--- a/gdb/amd64-linux-tdep.c
+++ b/gdb/amd64-linux-tdep.c
@@ -41,6 +41,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"
@@ -48,6 +49,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.  */
 
@@ -1794,6 +1799,65 @@ 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;
+
+  /* Construct status file name and read the file's content.  */
+  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 tagging mode which is currently
+   enabled.  For now, linear address masking (LAM) is the only feature
+   which allows to store metadata in pointer values for amd64.  Thus, we
+   adjust the watchpoint address based on the currently active LAM mode
+   using the untag mask provided by the linux kernel.  Check each time for
+   a new mask, as LAM is enabled at runtime.  Also, the LAM configuration
+   may change when entering an enclave.  No untagging will be applied if
+   this function is called while we don't have execution.  */
+
+static CORE_ADDR
+amd64_linux_remove_non_addr_bits_wpt (gdbarch *gdbarch, CORE_ADDR addr)
+{
+  /* 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.  */
+  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)
@@ -1848,6 +1912,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_addr_bits_wpt (gdbarch,
+					amd64_linux_remove_non_addr_bits_wpt);
 }
 
 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..28786389a9a
--- /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 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..8274c3adf97
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/amd64-lam.exp
@@ -0,0 +1,45 @@
+# Copyright 2023 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 tagged and untagged address with hit on untagged
+# and tagged adress.
+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 d48ea37c0cc..c7d15f82da1 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -9285,6 +9285,68 @@ gdb_caching_proc allow_ctf_tests {} {
 
     return $can_ctf
 }
+# 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
+}
 
 # Return 1 if compiler supports -gstatement-frontiers.  Otherwise,
 # return 0.
-- 
2.34.1

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

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

[Eli Zaretskii]

> From: "Schimpe, Christina" <christina.schimpe@intel.com>
> Cc: christina.schimpe@intel.com
> Date: Wed, 27 Mar 2024 07:47:38 +0000
> 
> 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               | 67 ++++++++++++++++++++++++++++
>  gdb/testsuite/gdb.arch/amd64-lam.c   | 49 ++++++++++++++++++++
>  gdb/testsuite/gdb.arch/amd64-lam.exp | 45 +++++++++++++++++++
>  gdb/testsuite/lib/gdb.exp            | 62 +++++++++++++++++++++++++
>  5 files changed, 225 insertions(+)
>  create mode 100755 gdb/testsuite/gdb.arch/amd64-lam.c
>  create mode 100644 gdb/testsuite/gdb.arch/amd64-lam.exp

Thanks, the NEWS part is approved.

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

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

[Luis Machado]

On 3/27/24 07:47, Schimpe, Christina wrote:
> 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               | 67 ++++++++++++++++++++++++++++
>  gdb/testsuite/gdb.arch/amd64-lam.c   | 49 ++++++++++++++++++++
>  gdb/testsuite/gdb.arch/amd64-lam.exp | 45 +++++++++++++++++++
>  gdb/testsuite/lib/gdb.exp            | 62 +++++++++++++++++++++++++
>  5 files changed, 225 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 feb3a37393a..295f2147def 100644
> --- a/gdb/NEWS
> +++ b/gdb/NEWS
> @@ -3,6 +3,8 @@
>  
>  *** Changes since GDB 14
>  
> +* GDB now supports watchpoints for tagged data pointers on amd64.
> +
>  * The MPX commands "show/set mpx bound" have been deprecated, as Intel
>    listed MPX as removed in 2019.
>  
> diff --git a/gdb/amd64-linux-tdep.c b/gdb/amd64-linux-tdep.c
> index 9d560ac4fbf..94c62277623 100644
> --- a/gdb/amd64-linux-tdep.c
> +++ b/gdb/amd64-linux-tdep.c
> @@ -41,6 +41,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"
> @@ -48,6 +49,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.  */
>  
> @@ -1794,6 +1799,65 @@ 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;
> +
> +  /* Construct status file name and read the file's content.  */
> +  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 tagging mode which is currently
> +   enabled.  For now, linear address masking (LAM) is the only feature
> +   which allows to store metadata in pointer values for amd64.  Thus, we
> +   adjust the watchpoint address based on the currently active LAM mode
> +   using the untag mask provided by the linux kernel.  Check each time for
> +   a new mask, as LAM is enabled at runtime.  Also, the LAM configuration
> +   may change when entering an enclave.  No untagging will be applied if
> +   this function is called while we don't have execution.  */
> +
> +static CORE_ADDR
> +amd64_linux_remove_non_addr_bits_wpt (gdbarch *gdbarch, CORE_ADDR addr)
> +{
> +  /* 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.  */
> +  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)
> @@ -1848,6 +1912,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_addr_bits_wpt (gdbarch,
> +					amd64_linux_remove_non_addr_bits_wpt);
>  }
>  
>  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..28786389a9a
> --- /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 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..8274c3adf97
> --- /dev/null
> +++ b/gdb/testsuite/gdb.arch/amd64-lam.exp
> @@ -0,0 +1,45 @@
> +# Copyright 2023 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 tagged and untagged address with hit on untagged
> +# and tagged adress.
> +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 d48ea37c0cc..c7d15f82da1 100644
> --- a/gdb/testsuite/lib/gdb.exp
> +++ b/gdb/testsuite/lib/gdb.exp
> @@ -9285,6 +9285,68 @@ gdb_caching_proc allow_ctf_tests {} {
>  
>      return $can_ctf
>  }
> +# 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
> +}
>  
>  # Return 1 if compiler supports -gstatement-frontiers.  Otherwise,
>  # return 0.

I'll leave the amd64-specific bits for the responsible maintainers. But I noticed the
copyright year is wrong (2023 -> 2024).

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

[Willgerodt, Felix]

> +/* 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;
> +
> +  /* Construct status file name and read the file's content.  */

I would remove this comment, the code is clear enough in what it does.

> +  std::string filename = string_printf ("/proc/%d/status", inf->pid);

Can this be const?

> +  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 ());

Reading this again makes me wonder why we don't use std::ifstream here.
E.g. something like this looks simpler:

std::ifstream status_file (filename);
if (!status_file.is_open())
  return DEFAULT_TAG_MASK;

Though the current way works fine as well. And I don't know if there
ever was any guidance/discussions on such a thing in GDB already.
Most file parsing is still "c style".

> +  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 tagging mode which is currently
> +   enabled.  For now, linear address masking (LAM) is the only feature
> +   which allows to store metadata in pointer values for amd64.  Thus, we
> +   adjust the watchpoint address based on the currently active LAM mode
> +   using the untag mask provided by the linux kernel.  Check each time for
> +   a new mask, as LAM is enabled at runtime.  Also, the LAM configuration
> +   may change when entering an enclave.  No untagging will be applied if
> +   this function is called while we don't have execution.  */

To me this is a bit long and cumbersome to read and has duplicate information.

1) I don't really see how there can be another tagging feature next to LAM for
amd64. And even if there will be, we can worry about that then. I would
remove the second sentence. And adjust the next one accordingly.
2) I would remove the last sentence, we already mentioned this for the other
function.
3) Do we need to say it is "provided by the linux kernel". We are in a linux file
and the other function provides enough details.

> +static CORE_ADDR
> +amd64_linux_remove_non_addr_bits_wpt (gdbarch *gdbarch, CORE_ADDR
> addr)
> +{
> +  /* 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.  */

Similarly, I wonder if the last sentence is needed. The code you add doesn't
seem to do anything special for kernel space addresses.

> +  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)
> @@ -1848,6 +1912,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_addr_bits_wpt (gdbarch,
> +
> 	amd64_linux_remove_non_addr_bits_wpt);
>  }
> 
>  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..28786389a9a
> --- /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 Free Software Foundation, Inc.

Do we need to add 2024?

> +
> + 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..8274c3adf97
> --- /dev/null
> +++ b/gdb/testsuite/gdb.arch/amd64-lam.exp
> @@ -0,0 +1,45 @@
> +# Copyright 2023 Free Software Foundation, Inc.

Same.

> +
> +# 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 tagged and untagged address with hit on untagged
> +# and tagged adress.
> +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
> +}

I would add a short comment on why we test hitting the watchpoint twice.

> diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
> index d48ea37c0cc..c7d15f82da1 100644
> --- a/gdb/testsuite/lib/gdb.exp
> +++ b/gdb/testsuite/lib/gdb.exp
> @@ -9285,6 +9285,68 @@ gdb_caching_proc allow_ctf_tests {} {
> 
>      return $can_ctf
>  }
> +# 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
> +}

Let's move this function to the other x86 related allow_* functions.
E.g. to allow_avx512* and the rest.

Thanks,
Felix

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

[PATCH 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 | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/gdb/amd64-linux-tdep.c b/gdb/amd64-linux-tdep.c
index 94c62277623..3498a6d4632 100644
--- a/gdb/amd64-linux-tdep.c
+++ b/gdb/amd64-linux-tdep.c
@@ -51,6 +51,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'
@@ -1852,10 +1854,21 @@ amd64_linux_lam_untag_mask ()
 static CORE_ADDR
 amd64_linux_remove_non_addr_bits_wpt (gdbarch *gdbarch, CORE_ADDR addr)
 {
+  /* 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.  */
-  return (addr & amd64_linux_lam_untag_mask ());
+  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 <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

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

[Willgerodt, Felix]

> -----Original Message-----
> From: Schimpe, Christina <christina.schimpe@intel.com>
> Sent: Mittwoch, 27. März 2024 08:48
> To: gdb-patches@sourceware.org
> Cc: Schimpe, Christina <christina.schimpe@intel.com>
> Subject: [PATCH 3/3] LAM: Support kernel space debugging
> 
> From: Christina Schimpe <christina.schimpe@intel.com>
> 
> Sign-extend watchpoint address for kernel space support.
> ---
>  gdb/amd64-linux-tdep.c | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/gdb/amd64-linux-tdep.c b/gdb/amd64-linux-tdep.c
> index 94c62277623..3498a6d4632 100644
> --- a/gdb/amd64-linux-tdep.c
> +++ b/gdb/amd64-linux-tdep.c
> @@ -51,6 +51,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'
> @@ -1852,10 +1854,21 @@ amd64_linux_lam_untag_mask ()
>  static CORE_ADDR
>  amd64_linux_remove_non_addr_bits_wpt (gdbarch *gdbarch, CORE_ADDR
> addr)
>  {
> +  /* 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.  */
> -  return (addr & amd64_linux_lam_untag_mask ());
> +  addr &= mask;
> +
> +  /* Sign-extend if we have a kernel-space address.  */
> +  if (kernel_address)
> +    addr |= ~mask;
> +
> +  return addr;

Ah, here is probably where we should add the comment I complained about in
patch 2.

I don't really know much about how kernel space debugging works.
(We can't get a kernel address when doing user-space debugging, can we?)
While this probably doesn't hurt much, I wonder if it isn't just dead code.
Is this file (amd64-linux-tdep.c) even used when doing kernel debugging?
And we still read the mask from /proc/pid/status. Is that available?

Maybe someone with more experience on this can chime in.

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

RE: [PING][PATCH 0/3] Add amd64 LAM watchpoint support

[Schimpe, Christina]

Kindly pinging for feedback.

The NEWS part is already approved by Eli.
I also got some feedback from Luis and will apply his comments regarding gdbarch function naming.
But I'd wait for further feedback until I push a v2 of this series.

Thanks, 
Christina

> Hi all,
> 
> This is a series to add watchpoint support with Liner Address Masking (LAM)
> on amd64.
> 
> I am looking forward to your feedback!
> 
> 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              |  3 +-
>  gdb/aarch64-linux-tdep.c             | 14 ++---
>  gdb/aarch64-tdep.c                   | 12 +++--
>  gdb/amd64-linux-tdep.c               | 80 ++++++++++++++++++++++++++++
>  gdb/breakpoint.c                     |  4 +-
>  gdb/gdbarch-gen.h                    | 50 +++++++++++++----
>  gdb/gdbarch.c                        | 66 +++++++++++++++++++----
>  gdb/gdbarch_components.py            | 54 ++++++++++++++++---
>  gdb/target.c                         |  4 +-
>  gdb/testsuite/gdb.arch/amd64-lam.c   | 49 +++++++++++++++++
>  gdb/testsuite/gdb.arch/amd64-lam.exp | 45 ++++++++++++++++
>  gdb/testsuite/lib/gdb.exp            | 62 +++++++++++++++++++++
>  13 files changed, 401 insertions(+), 44 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 <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

RE: [PING*2][PATCH 0/3] Add amd64 LAM watchpoint support

[Schimpe, Christina]

Kindly pinging.

Regards,
Christina

> -----Original Message-----
> From: Schimpe, Christina
> Sent: Monday, April 15, 2024 1:26 PM
> To: gdb-patches@sourceware.org
> Cc: Eli Zaretskii <eliz@gnu.org>; Luis Machado <luis.machado@arm.com>
> Subject: RE: [PING][PATCH 0/3] Add amd64 LAM watchpoint support
> 
> Kindly pinging for feedback.
> 
> The NEWS part is already approved by Eli.
> I also got some feedback from Luis and will apply his comments regarding
> gdbarch function naming.
> But I'd wait for further feedback until I push a v2 of this series.
> 
> Thanks,
> Christina
> 
> > Hi all,
> >
> > This is a series to add watchpoint support with Liner Address Masking
> > (LAM) on amd64.
> >
> > I am looking forward to your feedback!
> >
> > 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              |  3 +-
> >  gdb/aarch64-linux-tdep.c             | 14 ++---
> >  gdb/aarch64-tdep.c                   | 12 +++--
> >  gdb/amd64-linux-tdep.c               | 80 ++++++++++++++++++++++++++++
> >  gdb/breakpoint.c                     |  4 +-
> >  gdb/gdbarch-gen.h                    | 50 +++++++++++++----
> >  gdb/gdbarch.c                        | 66 +++++++++++++++++++----
> >  gdb/gdbarch_components.py            | 54 ++++++++++++++++---
> >  gdb/target.c                         |  4 +-
> >  gdb/testsuite/gdb.arch/amd64-lam.c   | 49 +++++++++++++++++
> >  gdb/testsuite/gdb.arch/amd64-lam.exp | 45 ++++++++++++++++
> >  gdb/testsuite/lib/gdb.exp            | 62 +++++++++++++++++++++
> >  13 files changed, 401 insertions(+), 44 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 <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928