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

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

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

I see, it wouldn't. I somehow didn't realize that we aren't in shared code.
Forget my comment.


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

I still find that a bit long and cumbersome. Can we merge the first and second
sentence somehow? To me they say the same thing with varying level of details.
And I don't think we need to say "for amd64", we are in an
amd64 file.

And I would drop the enclaves sentence, from GDB perspective it is just
another type of change at runtime. 


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

I missed that your comment is supposed to cover that.
You could add an "each", e.g. something like "with a hit on a tagged and 
untagged address each". I think that would already help a lot.
And should it be addresses (plural)?

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