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

["Schimpe, Christina" <christina.schimpe@intel.com>]

Hi Felix, 

Thank you for the review. Please see my comments below.

> > +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.

Agreed. 

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

Yes.

> > +  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".

Hm, for these types of files (/proc/pid/*) we usually use target_fileio_read* functions
I think.  Does your suggestion work for remote targets?

> > +  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.

Alright, I will apply your comments as follows:

/* Adjust watchpoint address based on the tagging mode which is currently
   enabled.  For amd64, we adjust the watchpoint address based on the
   currently active linear address masking (LAM) mode using the untag
   mask.  Check each time for a new mask, as LAM is enabled at runtime.
   Also, the LAM configuration may change when entering an enclave.  */

> > +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.

Ah, yes. This does not belong to this patch. Will fix.

> >  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?

Yes, will fix.

> > +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.

Hm, I am not sure which information you exactly miss here.
Note, that this code is very similar to the code in "gdb.arch/aarch64-tagged-pointer.exp"
They have a longer comment:

# sp1 and p1 are untagged pointers, but sp2 and p2 are tagged pointers.
# Cycle through all of them to make sure the following combinations work:
#
# hw watch on untagged address, hit on untagged address.
# hw watch on tagged address, hit on untagged address.
# hw watch on untagged address, hit on tagged address.
# hw watch on tagged address, hit on tagged address.

But I would have hoped that this is covered by the existent comment:

# Test hw watchpoint for tagged and untagged address with hit on untagged
# and tagged address.

What exactly do you miss?

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

Yes, will fix.

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