RE: [PATCH v1 3/6] gdb: introduce helper class file_reader_t

["Joos, Christina" <christina.joos@intel.com>]

Hi Matthieu,

Please find my feedback below.

> -----Original Message-----
> From: Matthieu Longo <matthieu.longo@arm.com>
> Sent: Dienstag, 28. Juli 2026 17:17
> To: gdb-patches@sourceware.org
> Cc: Luis Machado <luis.machado@amd.com>; Luis Machado
> <luis.machado.foss@gmail.com>; Thiago Jung Bauermann
> <thiago.bauermann@linaro.org>; Simon Marchi <simark@simark.ca>; Kevin
> Buettner <kevinb@redhat.com>; Joos, Christina <christina.joos@intel.com>;
> Joos, Christina <christina.joos@intel.com>; Matthieu Longo
> <matthieu.longo@arm.com>
> Subject: [PATCH v1 3/6] gdb: introduce helper class file_reader_t
> 
> Wrap all the boilerplate code required to read a file in a new helper
> class: file_reader_t. The class owns the file contents together with the file path,
> and provides convenient accessors for the data, size and typed views. It
> supports both null-terminated text files and binary files.
> 
> This helper eliminates repeated calls to target_fileio_read_stralloc and
> target_fileio_read_alloc, remove explicit memory management with
> gdb::unique_xmalloc_ptr, and simplifies the casting logic when working with
> binary data.
> 
> The patch converts some of the existing Linux, AMD64, and SPARC code that
> reads files from /proc to use file_reader_t.
> ---
>  gdb/amd64-linux-tdep.c |  12 ++---
>  gdb/linux-tdep.c       | 109 ++++++++++++++++++-----------------------
>  gdb/sparc64-tdep.c     |  13 +++--
>  gdb/target.h           |  79 +++++++++++++++++++++++++++++
>  4 files changed, 137 insertions(+), 76 deletions(-)
> 
> diff --git a/gdb/amd64-linux-tdep.c b/gdb/amd64-linux-tdep.c index
> 9b23db72bbe..52f16c953d1 100644
> --- a/gdb/amd64-linux-tdep.c
> +++ b/gdb/amd64-linux-tdep.c
> @@ -1848,14 +1848,11 @@ amd64_linux_lam_untag_mask ()
>    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)
> +  file_reader_t<char> proc_status (string_printf ("/proc/%d/status",
> + inf->pid));  if (!proc_status)
>      return DEFAULT_TAG_MASK;
> 
> -  std::string_view status_file_view (status_file.get ());
> +  std::string_view status_file_view (proc_status.data ());
>    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)
> @@ -1867,7 +1864,8 @@ amd64_linux_lam_untag_mask ()
>        unsigned long long result = std::strtoul (start, &endptr, 0);
>        if (errno != 0 || endptr == start)
>  	error (_("Failed to parse untag_mask from file %ps."),
> -	       styled_string (file_name_style.style (), filename.c_str ()));
> +	       styled_string (file_name_style.style (),
> +			      proc_status.c_filepath ()));
> 
>        return result;
>      }

There is a minor behavioural change in the function
amd64_linux_lam_untag_mask, since we now return earlier in case
the file is empty.
In this case it's an improvement, so it's good to keep in my opinion.

> diff --git a/gdb/linux-tdep.c b/gdb/linux-tdep.c index
> 9bdcc55a0e1..11f0a6952ea 100644
> --- a/gdb/linux-tdep.c
> +++ b/gdb/linux-tdep.c
> @@ -1698,6 +1698,12 @@ parse_smaps_data (const char *data,
>    return smaps;
>  }
> 
> +static std::vector<struct smaps_data>

Nit: we should omit the struct keyword here.

> +parse_smaps_data (const file_reader_t<char> &freader) {
> +  return parse_smaps_data (freader.data (), freader.filepath ()); }
> +
>  /* Helper that checks if an address is in a memory tag page for a live
>     process.  */
> 
> @@ -1709,17 +1715,13 @@ linux_process_address_in_memtag_page
> (CORE_ADDR address)
> 
>    ptid_t ptid = get_process_reference_ptid ();
> 
> -  std::string smaps_file = string_printf ("/proc/%ld/smaps", ptid.lwp ());
> -
> -  gdb::unique_xmalloc_ptr<char> data
> -    = target_fileio_read_stralloc (NULL, smaps_file.c_str ());
> -
> -  if (data == nullptr)
> +  file_reader_t<char> smaps_freader
> +    (string_printf ("/proc/%ld/smaps", ptid.lwp ()));  if
> + (!smaps_freader)
>      return false;
> 
>    /* Parse the contents of smaps into a vector.  */
> -  std::vector<struct smaps_data> smaps
> -    = parse_smaps_data (data.get (), smaps_file);
> +  std::vector<struct smaps_data> smaps = parse_smaps_data
> + (smaps_freader);

Like amd64_linux_lam_untag_mask we have a minor behavioural change here.
But I again see it as an improvement, as the result should be the same, we just
return earlier. There might be some more cases in linux-tdep.c, but I did not check
all of them.

I think it's worth pointing out in the commit message.

>    for (const smaps_data &map : smaps)
>      {
> @@ -1783,17 +1785,13 @@ linux_find_memory_regions_full (struct gdbarch
> *gdbarch,
> 
>    if (use_coredump_filter)
>      {
> -      std::string core_dump_filter_name
> -	= string_printf ("/proc/%ld/coredump_filter", ptid.lwp ());
> -
> -      gdb::unique_xmalloc_ptr<char> coredumpfilterdata
> -	= target_fileio_read_stralloc (NULL, core_dump_filter_name.c_str ());
> -
> -      if (coredumpfilterdata != NULL)
> +      file_reader_t<char> coredump_filter_freader
> +	(string_printf ("/proc/%ld/coredump_filter", ptid.lwp ()));
> +      if (coredump_filter_freader)
>  	{
>  	  unsigned int flags;
> 
> -	  sscanf (coredumpfilterdata.get (), "%x", &flags);
> +	  sscanf (coredump_filter_freader.data (), "%x", &flags);
>  	  filterflags = (enum filter_flag) flags;
>  	}
>      }
> @@ -2296,9 +2294,6 @@ linux_corefile_parse_exec_context (struct gdbarch
> *gdbarch, bfd *cbfd)  static bool  linux_fill_prpsinfo (struct
> elf_internal_linux_prpsinfo *p)  {
> -  /* The filename which we will use to obtain some info about the process.
> -     We will basically use this to store the `/proc/PID/FILENAME' file.  */
> -  char filename[100];
>    /* The basename of the executable.  */
>    const char *basename;
>    /* Temporary buffer.  */
> @@ -2320,24 +2315,27 @@ linux_fill_prpsinfo (struct
> elf_internal_linux_prpsinfo *p)
> 
>    gdb_assert (p != nullptr);
> 
> -  /* Obtaining PID and filename.  */
> -  xsnprintf (filename, sizeof (filename), "/proc/%ld/cmdline", ptid.lwp ());
> -  /* The full name of the program which generated the corefile.  */
> -  gdb_byte *buf = nullptr;
> -  LONGEST buf_len = target_fileio_read_alloc (nullptr, filename, &buf);
> -  gdb::unique_xmalloc_ptr<char> fname ((char *)buf);
> +  file_reader_t<gdb_byte> cmdline_freader
> +    (string_printf ("/proc/%ld/cmdline", ptid.lwp ()));  if
> + (!cmdline_freader)
> +    return false;
> 
> -  if (buf_len < 1 || fname.get () == nullptr || fname.get ()[0] == '\0')
> +  /* /proc/<pid>/cmdline stores the command-line arguments as a sequence
> of
> +     NUL-separated strings.  */
> +  gdb::array_view<char> cmdline = cmdline_freader.cast_view<char> ();
> +  /* The buffer points to the full name of the program which generated the
> +     corefile.  */
> +  if (cmdline.size () < 1 || cmdline[0] == '\0')
>      {
>        /* No program name was read, so we won't be able to retrieve more
>  	 information about the process.  */
>        return false;
>      }
> -  if (fname.get ()[buf_len - 1] != '\0')
> +  if (cmdline[cmdline.size () - 1] != '\0')
>      {
>        warning (_("target file %s "
>  		 "does not contain a trailing null character"),
> -	       filename);
> +	       cmdline_freader.c_filepath ());
>        return false;
>      }
> 
> @@ -2347,27 +2345,24 @@ linux_fill_prpsinfo (struct
> elf_internal_linux_prpsinfo *p)
>    p->pr_pid = ptid.pid ();
> 
>    /* Copying the program name.  Only the basename matters.  */
> -  basename = lbasename (fname.get ());
> +  basename = lbasename (cmdline.data ());
>    strncpy (p->pr_fname, basename, sizeof (p->pr_fname) - 1);
>    p->pr_fname[sizeof (p->pr_fname) - 1] = '\0';
> 
>    const std::string &infargs = current_inferior ()->args ();
> 
>    /* The arguments of the program.  */
> -  std::string psargs = fname.get ();
> +  std::string psargs = cmdline.data ();
>    if (!infargs.empty ())
>      psargs += ' ' + infargs;
> 
>    strncpy (p->pr_psargs, psargs.c_str (), sizeof (p->pr_psargs) - 1);
>    p->pr_psargs[sizeof (p->pr_psargs) - 1] = '\0';
> 
> -  xsnprintf (filename, sizeof (filename), "/proc/%ld/stat", ptid.lwp ());
> -  /* The contents of `/proc/PID/stat'.  */
> -  gdb::unique_xmalloc_ptr<char> proc_stat_contents
> -    = target_fileio_read_stralloc (NULL, filename);
> -  char *proc_stat = proc_stat_contents.get ();
> -
> -  if (proc_stat == NULL || *proc_stat == '\0')
> +  file_reader_t<char> stat_freader
> +    (string_printf ("/proc/%ld/stat", ptid.lwp ()));  const char
> + *proc_stat = stat_freader.data ();  if (!stat_freader || *proc_stat ==
> + '\0')
>      {
>        /* Despite being unable to read more information about the
>  	 process, we return true here because at least we have its @@ -
> 2439,13 +2434,10 @@ linux_fill_prpsinfo (struct elf_internal_linux_prpsinfo
> *p)
> 
>    /* Finally, obtaining the UID and GID.  For that, we read and parse the
>       contents of the `/proc/PID/status' file.  */
> -  xsnprintf (filename, sizeof (filename), "/proc/%ld/status", ptid.lwp ());
> -  /* The contents of `/proc/PID/status'.  */
> -  gdb::unique_xmalloc_ptr<char> proc_status_contents
> -    = target_fileio_read_stralloc (NULL, filename);
> -  char *proc_status = proc_status_contents.get ();
> -
> -  if (proc_status == NULL || *proc_status == '\0')
> +  file_reader_t<char> status_freader
> +    (string_printf ("/proc/%ld/status", ptid.lwp ()));  char
> + *proc_status = status_freader.data ();  if (!status_freader ||
> + *proc_status == '\0')
>      {
>        /* Returning true since we already have a bunch of information.  */
>        return true;
> @@ -2838,9 +2830,6 @@ linux_gdb_signal_to_target (struct gdbarch
> *gdbarch,  static bool  linux_vsyscall_range_raw (struct gdbarch *gdbarch,
> struct mem_range *range)  {
> -  char filename[100];
> -  long pid;
> -
>    if (target_auxv_search (AT_SYSINFO_EHDR, &range->start) <= 0)
>      return false;
> 
> @@ -2878,7 +2867,7 @@ linux_vsyscall_range_raw (struct gdbarch *gdbarch,
> struct mem_range *range)
>    if (current_inferior ()->fake_pid_p)
>      return false;
> 
> -  pid = current_inferior ()->pid;
> +  long pid = current_inferior ()->pid;
> 
>    /* Note that reading /proc/PID/task/PID/maps (1) is much faster than
>       reading /proc/PID/maps (2).  The later identifies thread stacks @@ -
> 2888,15 +2877,14 @@ linux_vsyscall_range_raw (struct gdbarch *gdbarch,
> struct mem_range *range)
>       a few thousand threads, (1) takes a few milliseconds, while (2)
>       takes several seconds.  Also note that "smaps", what we read for
>       determining core dump mappings, is even slower than "maps".  */
> -  xsnprintf (filename, sizeof filename, "/proc/%ld/task/%ld/maps", pid, pid);
> -  gdb::unique_xmalloc_ptr<char> data
> -    = target_fileio_read_stralloc (NULL, filename);
> -  if (data != NULL)
> +  file_reader_t<char> task_maps_freader
> +    (string_printf ("/proc/%ld/task/%ld/maps", pid, pid));  if
> + (!task_maps_freader.error ())
>      {
>        char *line;
>        char *saveptr = NULL;
> 
> -      for (line = strtok_r (data.get (), "\n", &saveptr);
> +      for (line = strtok_r (task_maps_freader.data (), "\n", &saveptr);
>  	   line != NULL;
>  	   line = strtok_r (NULL, "\n", &saveptr))
>  	{
> @@ -2915,7 +2903,8 @@ linux_vsyscall_range_raw (struct gdbarch *gdbarch,
> struct mem_range *range)
>  	}
>      }
>    else
> -    warning (_("unable to open /proc file '%s'"), filename);
> +    warning (_("unable to open /proc file '%s'"),
> +	     task_maps_freader.c_filepath ());
> 
>    return false;
>  }
> @@ -3242,16 +3231,12 @@ linux_address_in_shadow_stack_mem_range
> 
>    ptid_t ptid = get_process_reference_ptid ();
> 
> -  std::string smaps_file = string_printf ("/proc/%ld/smaps", ptid.lwp ());
> -
> -  gdb::unique_xmalloc_ptr<char> data
> -    = target_fileio_read_stralloc (nullptr, smaps_file.c_str ());
> -
> -  if (data == nullptr)
> +  file_reader_t<char> smaps_freader
> +    (string_printf ("/proc/%ld/smaps", ptid.lwp ()));  if
> + (!smaps_freader)
>      return false;
> 
> -  const std::vector<smaps_data> smaps
> -    = parse_smaps_data (data.get (), smaps_file);
> +  const std::vector<smaps_data> smaps = parse_smaps_data
> + (smaps_freader);
> 
>    auto find_addr_mem_range = [&addr] (const smaps_data &map)
>      {
> diff --git a/gdb/sparc64-tdep.c b/gdb/sparc64-tdep.c index
> 93db3417a2a..955631b87df 100644
> --- a/gdb/sparc64-tdep.c
> +++ b/gdb/sparc64-tdep.c
> @@ -301,18 +301,16 @@ adi_tag_fd ()
>  static bool
>  adi_is_addr_mapped (CORE_ADDR vaddr, size_t cnt)  {
> -  char filename[MAX_PROC_NAME_SIZE];
>    size_t i = 0;
> 
>    pid_t pid = inferior_ptid.pid ();
> -  snprintf (filename, sizeof filename, "/proc/%ld/adi/maps", (long) pid);
> -  gdb::unique_xmalloc_ptr<char> data
> -    = target_fileio_read_stralloc (NULL, filename);
> -  if (data)
> +  file_reader_t<char> adi_maps_freader
> +    (string_printf ("/proc/%d/adi/maps", pid));  if
> + (adi_maps_freader.error ())

Didn't you mean 
  if (!adi_maps_freader.error ())

?

>      {
>        adi_stat_t adi_stat = get_adi_info (pid);
>        char *saveptr;
> -      for (char *line = strtok_r (data.get (), "\n", &saveptr);
> +      for (char *line = strtok_r (adi_maps_freader.data (), "\n",
> + &saveptr);
>  	   line;
>  	   line = strtok_r (NULL, "\n", &saveptr))
>  	{
> @@ -329,7 +327,8 @@ adi_is_addr_mapped (CORE_ADDR vaddr, size_t cnt)
>  	}
>        }
>    else
> -    warning (_("unable to open /proc file '%s'"), filename);
> +    warning (_("unable to open /proc file '%s'"),
> +	     adi_maps_freader.c_filepath ());
> 
>    return false;
>  }
> diff --git a/gdb/target.h b/gdb/target.h index 819279c08fc..09830dfe3a9
> 100644
> --- a/gdb/target.h
> +++ b/gdb/target.h
> @@ -2341,6 +2341,85 @@ extern LONGEST target_fileio_read_alloc (struct
> inferior *inf,  extern gdb::unique_xmalloc_ptr<char> target_fileio_read_stralloc
>      (struct inferior *inf, const char *filename, LONGEST *len = nullptr);
> 
> +/* Helper class for reading the content of a file on the target.  */
> +template <typename T> class file_reader_t {
> +  /* The filepath of the file being read.  */
> +  std::string m_filepath;
> +  /* Smart pointer to the data.  */
> +  gdb::unique_xmalloc_ptr<T> m_data;
> +  /* Number of bytes read.  */
> +  LONGEST m_size;
> +
> +public:
> +  file_reader_t (const std::string &filepath)
> +    : m_filepath (filepath)
> +    , m_size (0)
> +  {
> +    if constexpr (std::is_same_v<T, char>)
> +      m_data = target_fileio_read_stralloc (nullptr, m_filepath.c_str (),
> +					    &m_size);
> +    else
> +      {
> +	gdb_byte *buf = nullptr;
> +	m_size = target_fileio_read_alloc (nullptr, m_filepath.c_str (), &buf);
> +	m_data = gdb::unique_xmalloc_ptr<T> (reinterpret_cast<T *>(buf));
> +      }
> +  }
> +
> +  file_reader_t (file_reader_t &&other)
> +    : m_filepath (std::move (other.m_filepath))
> +    , m_data (std::move (other.m_data))
> +    , m_size (other.m_size)
> +  {}

I hope I get the C++ rules right here:
AFAIK, since we have the move constructor, we implicitly delete the copy
constructor and the copy assignment operator.

Wouldn't it be clearer if we'd write this explicitly using DISABLE_COPY_AND_ASSIGN ?

I also wondered if the default move constructor is doing the same thing, so we could write sth. like:
file_reader_t (file_reader_t &&other) = default;

instead.

> +  /* Return true if the file was read successfully but contained no
> + data.  */  bool empty () const noexcept  { return m_data != nullptr &&
> + m_size == 0; }
> +
> +  /* Return true if the file could not be read.  */  bool error ()
> + const noexcept  { return m_data == nullptr || m_size < 0; }
> +  /* Return true if the file was read successfully and is non-empty.
> + */  explicit operator bool () const noexcept  { return !(error () ||
> + empty ()); }

To me it would feel more natural if we return true also in case the file is empty.
Then we could also omit the error function.

But I don't have a very strong opinion about this.

> +
> +  /* Return a pointer to the data.  */
> +  T *data () const noexcept
> +  { return m_data.get (); }
> +
> +  /* Return the number of bytes read.  */  LONGEST size () const
> + noexcept  {
> +    /* For char buffers, size() corresponds to the size of the read data. Some
> +       null-terminator characters are possibly scattered throughout the data.
> +       Consequently, strlen() might not reflect the actual size.  */
> +    return m_size;
> +  }
> +
> +  /* Return a span of the data.  */
> +  gdb::array_view<T> view () const noexcept  { return
> + gdb::array_view<T> (m_data.get (), size ()); }
> +
> +  /* Return a span of the data, reinterpreted as U objects.  */
> +  template <typename U>
> +  gdb::array_view<U> cast_view () const noexcept
> +  {
> +    return gdb::array_view<U> (reinterpret_cast<U *> (m_data.get ()),
> +			       size () * sizeof (T) / sizeof (U));
> +  }
> +
> +  /* Return the path of the file that was read.  */  const std::string
> + &filepath () const noexcept  { return m_filepath; }
> +
> +  /* Return the path of the file that was read as a C string.  */
> +  const char *c_filepath () const noexcept
> +  { return m_filepath.c_str (); }
> +};
> +
>  /* Invalidate the target associated with open handles that were open
>     on target TARG, since we're about to close (and maybe destroy) the
>     target.  The handles remain open from the client's perspective, but
> --
> 2.55.0

Christina

________________________________________
Intel Deutschland GmbH 

Registered Address: Dornacher Strasse 1, 85622 Feldkirchen, Germany 

Tel: +49 (89) 99143-0 

www.intel.de 

Managing Directors: Candice Moore, Jeffrey Schneiderman, Ramachandran Sitaraman

Chairperson of the Supervisory Board: Sonja Pierer

Registered Seat: Munich Commercial Register B: Amtsgericht Munich HRB 186928

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.