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

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

> -----Original Message-----
> From: Schimpe, Christina <christina.schimpe@intel.com>
> Sent: Montag, 17. Juni 2024 11:24
> To: gdb-patches@sourceware.org
> Cc: luis.machado@arm.com; Willgerodt, Felix <felix.willgerodt@intel.com>
> Subject: [PATCH v3 2/2] LAM: Enable tagged pointer support for watchpoints.
> 
> From: Christina Schimpe <christina.schimpe@intel.com>
> 
> The Intel (R) linear address masking (LAM) feature modifies the checking
> applied to 64-bit linear addresses.  With this so-called "modified
> canonicality check" the processor masks the metadata bits in a pointer
> before using it as a linear address.  LAM supports two different modes that
> differ regarding which pointer bits are masked and can be used for
> metadata: LAM 48 resulting in a LAM width of 15 and LAM 57 resulting in a
> LAM width of 6.
> 
> This patch adjusts watchpoint addresses based on the currently enabled
> LAM mode using the untag mask provided in the /proc/<pid>/status file.
> As LAM can be enabled at runtime or as the configuration may change
> when entering an enclave, GDB checks enablement state each time a watchpoint
> is updated.
> 
> In contrast to the patch implemented for ARM's Top Byte Ignore "Clear
> non-significant bits of address on memory access", it is not necessary to
> adjust addresses before they are passed to the target layer cache, as
> for LAM tagged pointers are supported by the system call to read memory.
> Additionally, LAM applies only to addresses used for data accesses.
> Thus, it is sufficient to mask addresses used for watchpoints.
> 
> The following examples are based on a LAM57 enabled program.
> Before this patch tagged pointers were not supported for watchpoints:
> ~~~
> (gdb) print pi_tagged
> $2 = (int *) 0x10007ffffffffe004
> (gdb) watch *pi_tagged
> Hardware watchpoint 2: *pi_tagged
> (gdb) c
> Continuing.
> Couldn't write debug register: Invalid argument.
> ~~~~
> 
> Once LAM 48 or LAM 57 is enabled for the current program, GDB can now
> specify watchpoints for tagged addresses with LAM width 15 or 6,
> respectively.
> ---
>  gdb/NEWS                             |  2 +
>  gdb/amd64-linux-tdep.c               | 66 ++++++++++++++++++++++++++++
>  gdb/testsuite/gdb.arch/amd64-lam.c   | 49 +++++++++++++++++++++
>  gdb/testsuite/gdb.arch/amd64-lam.exp | 46 +++++++++++++++++++
>  gdb/testsuite/lib/gdb.exp            | 63 ++++++++++++++++++++++++++
>  5 files changed, 226 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 bb7c4a6a6a6..914a8985e04 100644
> --- a/gdb/NEWS
> +++ b/gdb/NEWS
> @@ -3,6 +3,8 @@
> 
>  *** Changes since GDB 15
> 
> +* GDB now supports watchpoints for tagged data pointers on amd64.
> +
>  * Debugger Adapter Protocol changes
> 
>    ** The "scopes" request will now return a scope holding global
> diff --git a/gdb/amd64-linux-tdep.c b/gdb/amd64-linux-tdep.c
> index d7662cac572..7937ba67f6f 100644
> --- a/gdb/amd64-linux-tdep.c
> +++ b/gdb/amd64-linux-tdep.c
> @@ -43,6 +43,7 @@
>  #include "target-descriptions.h"
>  #include "expop.h"
>  #include "arch/amd64-linux-tdesc.h"
> +#include "inferior.h"
> 
>  /* The syscall's XML filename for i386.  */
>  #define XML_SYSCALL_FILENAME_AMD64 "syscalls/amd64-linux.xml"
> @@ -50,6 +51,10 @@
>  #include "record-full.h"
>  #include "linux-record.h"
> 
> +#include <string_view>
> +
> +#define DEFAULT_TAG_MASK 0xffffffffffffffffULL
> +
>  /* Mapping between the general-purpose registers in `struct user'
>     format and GDB's register cache layout.  */
> 
> @@ -1765,6 +1770,64 @@ amd64_dtrace_parse_probe_argument (struct
> gdbarch *gdbarch,
>      }
>  }
> 
> +/* Extract the untagging mask based on the currently active linear address
> +   masking (LAM) mode, which is stored in the /proc/<pid>/status file.
> +   If we cannot extract the untag mask (for example, if we don't have
> +   execution), we assume address tagging is not enabled and return the
> +   DEFAULT_TAG_MASK.  */
> +
> +static CORE_ADDR
> +amd64_linux_lam_untag_mask ()
> +{
> +  if (!target_has_execution ())
> +    return DEFAULT_TAG_MASK;
> +
> +  inferior *inf = current_inferior ();
> +  if (inf->fake_pid_p)
> +    return DEFAULT_TAG_MASK;
> +
> +  const std::string filename = string_printf ("/proc/%d/status", inf->pid);
> +  gdb::unique_xmalloc_ptr<char> status_file
> +    = target_fileio_read_stralloc (nullptr, filename.c_str ());
> +
> +  if (status_file == nullptr)
> +    return DEFAULT_TAG_MASK;
> +
> +  /* Parse the status file line-by-line and look for the untag mask.  */

This is no longer line-by-line. And the comment is repeating what the
"fairly easy to read" code does, so I would even drop it completely.

> +  std::string_view status_file_view (status_file.get ());
> +  constexpr std::string_view untag_mask_str = "untag_mask:\t";
> +  const size_t found = status_file_view.find (untag_mask_str);
> +  if (found != std::string::npos)
> +    {
> +      const char* start = status_file_view.data() + found
> +			  + untag_mask_str.length ();
> +      char* endptr;
> +      errno = 0;
> +      unsigned long long result = std::strtoul (start, &endptr, 0);
> +      if (errno != 0)
> +	error (_("Error converting untag mask: %s."), safe_strerror (errno));
> +      else if (endptr == start)

When I applied this series I saw this:

Applying: LAM: Enable tagged pointer support for watchpoints.
.git/rebase-apply/patch:87: trailing whitespace.
      else if (endptr == start) 
warning: 1 line adds whitespace errors.


> +	error (_("Failed to parse untag mask."));
> +

A user might not be familiar with LAM, which would make it very
hard to understand these messages. I would mention the exact string
and the file that we try to parse it from in the error message, e.g.:
"Failed to parse untag_mask from %s".
(GDB already does that in other places.)
Then people can at least try to see what is going on in the file and 
maybe even figure out what the file and untag_mask are, which will
hopefully clear things up a bit.

I am fine if you want to amend errno to the message if we have it,
though most cpp websites seem to suggested that it will always be
ERANGE only. So I see that as optional. And I don't really see a
need to print two different error messages, the difference is a bit
too subtle to be of much help without looking at the file.

Apart from that, this still looks good to me overall.

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