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

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

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

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

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

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

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

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

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

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

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