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

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

> +/* 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