RE: [PATCH v2 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.

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

I actually agree and don't remember if there was a reason why I chose this approach.
So yes, we can do better here.

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

I thought that if strtoul would fail here it should be a kernel bug providing
an untag mask which cannot be parsed. So I thought we don't have to handle
that in GDB, but am also not sure how we deal with that in general. 
But I have absolutely no problem adding that check here.

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

As I said before I am not sure how we should handle those error conditions. 
Maybe somebody else can help us here?

Besides that I tested the code that you suggested Felix, and it works fine.
I'd use that approach once the error handling is clarified and there are no
further objections.

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