[PATCH v1 0/6] gdb: introduce file_reader_t to read procfs files

[PATCH v1 0/6] gdb: introduce file_reader_t to read procfs files

[Matthieu Longo]

Those patches were extracted from a previous patch series [1].
Patches 1 and 2 are prerequisites to the next patches.
Patch 3 introduces class file_reader_t, and patches 4, 5, and 5 migrates existing code to use this new class.

Changes diff against [1]:
- remove reimplementation of std::replace, and use std::replace for gdb::ranges::replace.
- addressed comments from Christina Schimpe and Chirstina Joos regarding the interface of class file_reader_t.
- fix the implementation of extract_string_view_from_buffer after Luis found bugs in the previous implementation.

This patch series depends on [2].

[1]: https://inbox.sourceware.org/gdb-patches/20260707154900.94542-1-matthieu.longo@arm.com/
[2]: https://inbox.sourceware.org/gdb-patches/20260728143317.245389-1-matthieu.longo@arm.com/

Regards,
Matthieu

Matthieu Longo (6):
  target_fileio_read_stralloc: add an optional length parameter
  gdb support: add gdb::ranges::replace algorithm
  gdb: introduce helper class file_reader_t
  gdb/linux-tdep: migrate linux_info_proc to file_reader_t
  gdb/linux-tdep: migrate linux_find_memory_regions_full to file_reader_t
  gdb/linux-tdep: remove legacy parse_smaps_data overload

 gdb/amd64-linux-tdep.c  |  12 +-
 gdb/linux-tdep.c        | 285 +++++++++++++++++++++-------------------
 gdb/sparc64-tdep.c      |  13 +-
 gdb/target.c            |  11 +-
 gdb/target.h            |  99 ++++++++++++--
 gdbsupport/array-view.h |  16 +++
 6 files changed, 273 insertions(+), 163 deletions(-)

-- 
2.55.0

[PATCH v1 1/6] target_fileio_read_stralloc: add an optional length parameter

[Matthieu Longo]

Extend target_fileio_read_stralloc with an optional output parameter
that returns the number of bytes read, excluding the terminating NUL
byte.

Most callers only need the returned NUL-terminated buffer and can
ignore the new parameter, but callers that need the number of bytes
read can now obtain it without recomputing it.

While updating the function, make a couple of minor cleanups by using
'\0' instead of 0 for character literals and clarifying the function
documentation.
---
 gdb/target.c | 11 ++++++++---
 gdb/target.h | 20 ++++++++++++--------
 2 files changed, 20 insertions(+), 11 deletions(-)

diff --git a/gdb/target.c b/gdb/target.c
index 5d937f3ae85..ebf0f30092c 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -3547,7 +3547,8 @@ target_fileio_read_alloc (struct inferior *inf, const char *filename,
 /* See target.h.  */
 
 gdb::unique_xmalloc_ptr<char>
-target_fileio_read_stralloc (struct inferior *inf, const char *filename)
+target_fileio_read_stralloc (struct inferior *inf, const char *filename,
+			     LONGEST *len)
 {
   gdb_byte *buffer;
   char *bufstr;
@@ -3556,17 +3557,21 @@ target_fileio_read_stralloc (struct inferior *inf, const char *filename)
   transferred = target_fileio_read_alloc_1 (inf, filename, &buffer, 1);
   bufstr = (char *) buffer;
 
+  /* Note: on failure, target_fileio_read_alloc_1 returns -1.  */
+  if (len != nullptr)
+    *len = transferred;
+
   if (transferred < 0)
     return gdb::unique_xmalloc_ptr<char> (nullptr);
 
   if (transferred == 0)
     return make_unique_xstrdup ("");
 
-  bufstr[transferred] = 0;
+  bufstr[transferred] = '\0';
 
   /* Check for embedded NUL bytes; but allow trailing NULs.  */
   for (i = strlen (bufstr); i < transferred; i++)
-    if (bufstr[i] != 0)
+    if (bufstr[i] != '\0')
       {
 	warning (_("target file %s "
 		   "contained unexpected null characters"),
diff --git a/gdb/target.h b/gdb/target.h
index 6d1c21f29f6..819279c08fc 100644
--- a/gdb/target.h
+++ b/gdb/target.h
@@ -2327,15 +2327,19 @@ extern LONGEST target_fileio_read_alloc (struct inferior *inf,
 					 const char *filename,
 					 gdb_byte **buf_p);
 
-/* Read target file FILENAME, in the filesystem as seen by INF.  If
-   INF is NULL, use the filesystem seen by the debugger (GDB or, for
-   remote targets, the remote stub).  The result is NUL-terminated and
-   returned as a string, allocated using xmalloc.  If an error occurs
-   or the transfer is unsupported, NULL is returned.  Empty objects
-   are returned as allocated but empty strings.  A warning is issued
-   if the result contains any embedded NUL bytes.  */
+/* Read the content of the target file FILENAME from the filesystem as
+   seen by INF.  If INF is NULL, use the filesystem seen by the debugger
+   (GDB or, for remote targets, the remote stub).
+
+   If LEN is not NULL, store the number of bytes read, excluding the
+   terminating NUL byte.
+
+   The returned buffer is NUL-terminated and allocated using xmalloc.
+   On error, or if the transfer is unsupported, return NULL and set
+   LEN to -1.  Empty files are returned as allocated but empty strings.
+   A warning is issued if the file content contains embedded NUL bytes.  */
 extern gdb::unique_xmalloc_ptr<char> target_fileio_read_stralloc
-    (struct inferior *inf, const char *filename);
+    (struct inferior *inf, const char *filename, LONGEST *len = nullptr);
 
 /* Invalidate the target associated with open handles that were open
    on target TARG, since we're about to close (and maybe destroy) the
-- 
2.55.0

[PATCH v1 2/6] gdb support: add gdb::ranges::replace algorithm

[Matthieu Longo]

Provide a C++17-compatible replacement for the C++20 std::ranges::replace
algorithm, allowing callers to use a consistent interface until GDB
transitions to C++20. The helper should be removed once the C++ standard
library implementation become available.

https://en.cppreference.com/cpp/algorithm/ranges/replace
---
 gdbsupport/array-view.h | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/gdbsupport/array-view.h b/gdbsupport/array-view.h
index 8431d7f5add..f9842ecff30 100644
--- a/gdbsupport/array-view.h
+++ b/gdbsupport/array-view.h
@@ -225,6 +225,22 @@ void copy (gdb::array_view<U> src, gdb::array_view<T> dest)
     std::copy_backward (src.begin (), src.end (), dest.end ());
 }
 
+namespace ranges {
+
+/* Replace all occurrences of a value in the provided range.
+
+   Note: this helper is a reimplementation of std::ranges::replace, only
+   available from C++20 onwards, and consequently, should be replaced by
+   std::ranges::replace once GDB switches to C++20.  */
+
+template <class Range, typename T>
+void replace (Range r, const T &old_value, const T &new_value)
+{
+  std::replace (r.begin (), r.end (), old_value, new_value);
+}
+
+} /* namespace ranges */
+
 /* Compare LHS and RHS for (deep) equality.  That is, whether LHS and
    RHS have the same sizes, and whether each pair of elements of LHS
    and RHS at the same position compares equal.  */
-- 
2.55.0

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

[Matthieu Longo]

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;
     }
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>
+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);
 
   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 ())
     {
       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)
+  {}
+
+  /* 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 ()); }
+
+  /* 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

[PATCH v1 4/6] gdb/linux-tdep: migrate linux_info_proc to file_reader_t

[Matthieu Longo]

The patch migrates the code of linux_info_proc to use file_reader_t to
read the procfs files.
The availability of array_views allows to also simplify the logic in
several places, where null-terminating characters are replaced by spaces,
or where the file content is iterated line by line.
In the last case, a new helper function, extract_string_view_from_buffer,
encapsulates the logic for such iterations where string are separated by
tokens.
---
 gdb/linux-tdep.c | 135 +++++++++++++++++++++++++++++------------------
 1 file changed, 84 insertions(+), 51 deletions(-)

diff --git a/gdb/linux-tdep.c b/gdb/linux-tdep.c
index 11f0a6952ea..a8dab0d6d5e 100644
--- a/gdb/linux-tdep.c
+++ b/gdb/linux-tdep.c
@@ -874,6 +874,39 @@ dump_note_entry_p (filter_flags filterflags, const smaps_data &map)
   return true;
 }
 
+/* Extract a string view from BUFFER starting at START and ending at the
+   first occurrence of SEPARATOR.
+   Return the extracted view together with an iterator to the beginning of
+   the next entry, skipping any successive separators.  If no separator
+   is found, return the remainder of BUFFER starting at START.  If there is
+   no following entry, the returned iterator is BUFFER.end ().  */
+
+static std::pair<gdb::array_view<char>, gdb::array_view<char>::iterator>
+extract_string_view_from_buffer (gdb::array_view<char> &buffer,
+				 gdb::array_view<char>::iterator start,
+				 char separator = '\0')
+{
+  auto next_start = buffer.end ();
+
+  /* Reject a START iterator that does not point into BUFFER.  */
+  if (start < buffer.begin () || start >= buffer.end ())
+    return {gdb::array_view<char> (), next_start};
+
+  auto it = std::find (start, buffer.end (), separator);
+
+  /* If no separator is found, the remainder of BUFFER is the final string.  */
+  if (it != buffer.end ())
+    {
+      /* Otherwise, skip successive separators so that NEXT_START points to
+	 the beginning of the next string, if any.  */
+      for (next_start = std::next (it);
+	   next_start != buffer.end () && *next_start == separator;
+	   next_start = std::next (next_start));
+    }
+
+  return {gdb::array_view<char> (start, it), next_start};
+}
+
 /* Implement the "info proc" command.  */
 
 static void
@@ -915,25 +948,23 @@ linux_info_proc (struct gdbarch *gdbarch, const char *args,
 
   if (cmdline_f)
     {
-      xsnprintf (filename, sizeof filename, "/proc/%ld/cmdline", ptid.lwp ());
-      gdb_byte *buffer;
-      LONGEST len = target_fileio_read_alloc (nullptr, filename, &buffer);
-
-      if (len > 0)
+      file_reader_t<gdb_byte> cmdline_freader
+	(string_printf ("/proc/%ld/cmdline", ptid.lwp ()));
+      if (cmdline_freader)
 	{
-	  gdb::unique_xmalloc_ptr<char> cmdline ((char *) buffer);
-	  ssize_t pos;
-
-	  for (pos = 0; pos < len - 1; pos++)
-	    {
-	      if (buffer[pos] == '\0')
-		buffer[pos] = ' ';
-	    }
-	  buffer[len - 1] = '\0';
-	  gdb_printf ("cmdline = '%s'\n", buffer);
+	  /* /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> ();
+	  gdb_assert (cmdline[cmdline.size () - 1] == '\0');
+	  /* Replace null characters splitting the arguments in the command
+	     line by spaces, except for the last one.  */
+	  gdb::ranges::replace
+	    (cmdline.slice (0, cmdline.size () - 1), '\0', ' ');
+	  gdb_printf ("cmdline = '%s'\n", cmdline.data ());
 	}
       else
-	warning (_("unable to open /proc file '%s'"), filename);
+	warning (_("unable to open /proc file '%s'"),
+		 cmdline_freader.c_filepath ());
     }
   if (cwd_f)
     {
@@ -947,27 +978,25 @@ linux_info_proc (struct gdbarch *gdbarch, const char *args,
     }
   if (environ_f)
     {
-      xsnprintf (filename, sizeof filename, "/proc/%ld/environ", ptid.lwp ());
-      gdb_byte *buffer;
-      LONGEST len = target_fileio_read_alloc (nullptr, filename, &buffer);
-
-      if (len > 0)
+      file_reader_t<gdb_byte> environ_freader
+	(string_printf ("/proc/%ld/environ", ptid.lwp ()));
+      if (environ_freader)
 	{
-	  gdb::unique_xmalloc_ptr<char> dealloc ((char *) buffer);
 	  gdb_printf (_("Environment variables:\n\n"));
-
+	  gdb::array_view<char> buffer = environ_freader.cast_view<char> ();
 	  /* Entries are separated by the null character.
 	     Print each environment variable, line by line.  */
-	  gdb_byte *buffer_end = buffer + len;
-	  while (buffer < buffer_end)
+	  for (auto it = buffer.begin (); it != buffer.end ();)
 	    {
-	      gdb_printf ("  %s\n", buffer);
-	      /* +1 for the null character.  */
-	      buffer += strlen ((char *) buffer) + 1;
+	      auto [ntbs, next_start]
+		= extract_string_view_from_buffer (buffer, it, '\0');
+	      gdb_printf ("  %s\n", ntbs.data ());
+	      it = next_start;
 	    }
 	}
       else
-	warning (_("unable to open /proc file '%s'"), filename);
+	warning (_("unable to open /proc file '%s'"),
+		 environ_freader.c_filepath ());
     }
   if (exe_f)
     {
@@ -981,10 +1010,9 @@ linux_info_proc (struct gdbarch *gdbarch, const char *args,
     }
   if (mappings_f)
     {
-      xsnprintf (filename, sizeof filename, "/proc/%ld/maps", ptid.lwp ());
-      gdb::unique_xmalloc_ptr<char> map
-	= target_fileio_read_stralloc (NULL, filename);
-      if (map != NULL)
+      file_reader_t<char> map_freader
+	(string_printf ("/proc/%ld/maps", ptid.lwp ()));
+      if (map_freader)
 	{
 	  gdb_printf (_("Mapped address spaces:\n\n"));
 	  ui_out_emit_table emitter (current_uiout, 6, -1, "ProcMappings");
@@ -998,12 +1026,16 @@ linux_info_proc (struct gdbarch *gdbarch, const char *args,
 	  current_uiout->table_header (0, ui_left, "objfile", "File");
 	  current_uiout->table_body ();
 
-	  char *saveptr;
-	  for (const char *line = strtok_r (map.get (), "\n", &saveptr);
-	       line != nullptr;
-	       line = strtok_r (nullptr, "\n", &saveptr))
+	  auto content = map_freader.view ();
+	  for (auto it = content.begin (); it != content.end ();)
 	    {
-	      struct mapping m = read_mapping (line);
+	      auto [line, next_line_begin]
+		= extract_string_view_from_buffer (content, it, '\n');
+	      it = next_line_begin;
+
+	      /* read_mapping() expects a null-terminated string.  */
+	      *std::prev (it) = '\0';
+	      struct mapping m = read_mapping (line.data ());
 
 	      ui_out_emit_tuple tuple_emitter (current_uiout, nullptr);
 	      current_uiout->field_core_addr ("start", gdbarch, m.addr);
@@ -1022,26 +1054,26 @@ linux_info_proc (struct gdbarch *gdbarch, const char *args,
 	    }
 	}
       else
-	warning (_("unable to open /proc file '%s'"), filename);
+	warning (_("unable to open /proc file '%s'"),
+		 map_freader.c_filepath ());
     }
   if (status_f)
     {
-      xsnprintf (filename, sizeof filename, "/proc/%ld/status", ptid.lwp ());
-      gdb::unique_xmalloc_ptr<char> status
-	= target_fileio_read_stralloc (NULL, filename);
-      if (status)
-	gdb_puts (status.get ());
+      file_reader_t<char> status_freader
+	(string_printf ("/proc/%ld/status", ptid.lwp ()));
+      if (status_freader)
+	gdb_puts (status_freader.data ());
       else
-	warning (_("unable to open /proc file '%s'"), filename);
+	warning (_("unable to open /proc file '%s'"),
+		 status_freader.c_filepath ());
     }
   if (stat_f)
     {
-      xsnprintf (filename, sizeof filename, "/proc/%ld/stat", ptid.lwp ());
-      gdb::unique_xmalloc_ptr<char> statstr
-	= target_fileio_read_stralloc (NULL, filename);
-      if (statstr)
+      file_reader_t<char> stat_freader
+	(string_printf ("/proc/%ld/stat", ptid.lwp ()));
+      if (stat_freader)
 	{
-	  const char *p = statstr.get ();
+	  const char *p = stat_freader.data ();
 
 	  gdb_printf (_("Process: %s\n"),
 		      pulongest (strtoulst (p, &p, 10)));
@@ -1168,7 +1200,8 @@ linux_info_proc (struct gdbarch *gdbarch, const char *args,
 #endif
 	}
       else
-	warning (_("unable to open /proc file '%s'"), filename);
+	warning (_("unable to open /proc file '%s'"),
+		 stat_freader.c_filepath ());
     }
 }
 
-- 
2.55.0

[PATCH v1 5/6] gdb/linux-tdep: migrate linux_find_memory_regions_full to file_reader_t

[Matthieu Longo]

The previous implementation of linux_find_memory_regions_full could
still return success even when none of the /proc/PID/[s]maps files
existed, or all reads returned 0 bytes (this last case can happen on
Linux when the thread-group leader has exited).
As a result, the function could incorrectly succeed, allowing core
dump generation via the gcore command.
This logical defect was allowing, by chance, the function to return
success and hence, allowing fortuitously the coredump generation via
gcore command (see gcore-stale-thread test for more details).

A previous patch in this patch series fixed this issue by using the
first LWP ID of the current inferior instead of relying on the PID.
This patch adapts the code to use file_reader_t and makes the function
return an error if reading any of the procfs files fails.
---
 gdb/linux-tdep.c | 23 ++++++++++-------------
 1 file changed, 10 insertions(+), 13 deletions(-)

diff --git a/gdb/linux-tdep.c b/gdb/linux-tdep.c
index a8dab0d6d5e..628f73969d3 100644
--- a/gdb/linux-tdep.c
+++ b/gdb/linux-tdep.c
@@ -1829,25 +1829,22 @@ linux_find_memory_regions_full (struct gdbarch *gdbarch,
 	}
     }
 
-  std::string maps_filename = string_printf ("/proc/%ld/smaps", ptid.lwp ());
-
-  gdb::unique_xmalloc_ptr<char> data
-    = target_fileio_read_stralloc (NULL, maps_filename.c_str ());
+  std::vector<struct smaps_data> smaps;
 
-  if (data == NULL)
+  file_reader_t<char> smaps_freader
+    (string_printf ("/proc/%ld/smaps", ptid.lwp ()));
+  if (smaps_freader)
+    smaps = parse_smaps_data (smaps_freader);
+  else
     {
       /* Older Linux kernels did not support /proc/PID/smaps.  */
-      maps_filename = string_printf ("/proc/%ld/maps", ptid.lwp ());
-      data = target_fileio_read_stralloc (NULL, maps_filename.c_str ());
-
-      if (data == nullptr)
+      file_reader_t<char> maps_freader
+	(string_printf ("/proc/%ld/maps", ptid.lwp ()));
+      if (!maps_freader)
 	return false;
+      smaps = parse_smaps_data (maps_freader);
     }
 
-  /* Parse the contents of smaps into a vector.  */
-  std::vector<struct smaps_data> smaps
-    = parse_smaps_data (data.get (), maps_filename);
-
   for (const struct smaps_data &map : smaps)
     {
       /* Invoke the callback function to create the corefile segment.  */
-- 
2.55.0

[PATCH v1 6/6] gdb/linux-tdep: remove legacy parse_smaps_data overload

[Matthieu Longo]

Now that all callers use the file_reader_t overload of parse_smaps_data,
the legacy interface taking a raw buffer and filename separately is no
longer needed.

Fold its implementation into the file_reader_t version and remove the
obsolete wrapper.  This also simplifies the implementation by using the
file_reader_t accessors directly.

Reviewed-By: Thiago Jung Bauermann <thiago.bauermann@linaro.org>
Reviewed-By: Luis Machado <luis.machado.foss@gmail.com>
---
 gdb/linux-tdep.c | 30 ++++++++++++------------------
 1 file changed, 12 insertions(+), 18 deletions(-)

diff --git a/gdb/linux-tdep.c b/gdb/linux-tdep.c
index 628f73969d3..2e9cb14d5bf 100644
--- a/gdb/linux-tdep.c
+++ b/gdb/linux-tdep.c
@@ -1582,18 +1582,17 @@ parse_smaps_key_value (const char *keyword, const char *line,
 /* Helper function to parse the contents of /proc/<pid>/smaps into a data
    structure, for easy access.
 
-   DATA is the contents of the smaps file.  The parsed contents are stored
-   into the SMAPS vector.  */
+   FREADER is a wrapper around the contents of the smaps file.
+   The parsed contents are stored into the SMAPS vector.  */
 
 static std::vector<struct smaps_data>
-parse_smaps_data (const char *data,
-		  const std::string &maps_filename)
+parse_smaps_data (const file_reader_t<char> &freader)
 {
   char *line, *t;
 
-  gdb_assert (data != nullptr);
+  gdb_assert (freader);
 
-  line = strtok_r ((char *) data, "\n", &t);
+  line = strtok_r (freader.data (), "\n", &t);
 
   std::vector<struct smaps_data> smaps;
 
@@ -1649,8 +1648,8 @@ parse_smaps_data (const char *data,
 
 	  if (sscanf (line, "%64s", keyword) != 1)
 	    {
-	      warning (_("Error parsing {s,}maps file '%s'"),
-		       maps_filename.c_str ());
+	      warning (_("Error parsing keyword in {s,}maps file '%s'"),
+		       freader.c_filepath ());
 	      break;
 	    }
 
@@ -1664,12 +1663,12 @@ parse_smaps_data (const char *data,
 	    decode_vmflags (line, &v);
 
 	  if (parse_smaps_key_value (keyword, line, "Rss:",
-				     maps_filename,
+				     freader.filepath (),
 				     &rss))
 	    continue;
 
 	  if (parse_smaps_key_value (keyword, line, "Swap:",
-				     maps_filename,
+				     freader.filepath (),
 				     &swap))
 	    continue;
 
@@ -1680,8 +1679,9 @@ parse_smaps_data (const char *data,
 
 	      if (sscanf (line, "%*s%lu", &number) != 1)
 		{
-		  warning (_("Error parsing {s,}maps file '%s' number"),
-			   maps_filename.c_str ());
+		  warning (_("Error parsing numeric value associated with "
+			     "key '%s' in {s,}maps file '%s'"),
+			   keyword, freader.c_filepath ());
 		  break;
 		}
 	      if (number > 0)
@@ -1731,12 +1731,6 @@ parse_smaps_data (const char *data,
   return smaps;
 }
 
-static std::vector<struct smaps_data>
-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.  */
 
-- 
2.55.0

RE: [PATCH v1 0/6] gdb: introduce file_reader_t to read procfs files

[Joos, Christina]

> -----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 0/6] gdb: introduce file_reader_t to read procfs files
> 
> Those patches were extracted from a previous patch series [1].
> Patches 1 and 2 are prerequisites to the next patches.
> Patch 3 introduces class file_reader_t, and patches 4, 5, and 5 migrates existing
> code to use this new class.
> 
> Changes diff against [1]:
> - remove reimplementation of std::replace, and use std::replace for
> gdb::ranges::replace.
> - addressed comments from Christina Schimpe and Chirstina Joos regarding the
> interface of class file_reader_t.

Hi Matthieu, 

Just to avoid confusion: 
Christina Schimpe and Christina Joos is the same person. My name changed recently:
https://sourceware.org/pipermail/gdb-patches/2026-July/228900.html

Christina

Intel Deutschland GmbH

Registered Address: Dornacher Strasse 1, 85622 Feldkirchen, Germany
Tel: +49 89 991 430, www.intel.de
Managing Directors: Harry Demas, Jeffrey Schneiderman, Yin Chong Sorrell
Chairperson of the Supervisory Board: Nicole Lau
Registered Seat: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

Re: [PATCH v1 1/6] target_fileio_read_stralloc: add an optional length parameter

[Thiago Jung Bauermann]

Matthieu Longo <matthieu.longo@arm.com> writes:

> Extend target_fileio_read_stralloc with an optional output parameter
> that returns the number of bytes read, excluding the terminating NUL
> byte.
>
> Most callers only need the returned NUL-terminated buffer and can
> ignore the new parameter, but callers that need the number of bytes
> read can now obtain it without recomputing it.
>
> While updating the function, make a couple of minor cleanups by using
> '\0' instead of 0 for character literals and clarifying the function
> documentation.
> ---
>  gdb/target.c | 11 ++++++++---
>  gdb/target.h | 20 ++++++++++++--------
>  2 files changed, 20 insertions(+), 11 deletions(-)

Reviewed-by: Thiago Jung Bauermann <thiago.bauermann@linaro.org>

-- 
Thiago
(he/him)

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

[Thiago Jung Bauermann]

Matthieu Longo <matthieu.longo@arm.com> writes:

> 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(-)

Thank you for adding the comments I requested.

You addressed Christina's remarks about keeping the current behaviour
when reading empty files in the case of the task maps file and also in
the case of the SPARC-specific adi/maps file. I don't know whether this
is enough to resolve her concerns. Assuming it is:

Reviewed-by: Thiago Jung Bauermann <thiago.bauermann@linaro.org>

-- 
Thiago
(he/him)

Re: [PATCH v1 4/6] gdb/linux-tdep: migrate linux_info_proc to file_reader_t

[Thiago Jung Bauermann]

Matthieu Longo <matthieu.longo@arm.com> writes:

> The patch migrates the code of linux_info_proc to use file_reader_t to
> read the procfs files.
> The availability of array_views allows to also simplify the logic in
> several places, where null-terminating characters are replaced by spaces,
> or where the file content is iterated line by line.
> In the last case, a new helper function, extract_string_view_from_buffer,
> encapsulates the logic for such iterations where string are separated by
> tokens.
> ---
>  gdb/linux-tdep.c | 135 +++++++++++++++++++++++++++++------------------
>  1 file changed, 84 insertions(+), 51 deletions(-)

Thank you for addressing the review comments.

Reviewed-by: Thiago Jung Bauermann <thiago.bauermann@linaro.org>

-- 
Thiago
(he/him)

Re: [PATCH v1 5/6] gdb/linux-tdep: migrate linux_find_memory_regions_full to file_reader_t

[Thiago Jung Bauermann]

Matthieu Longo <matthieu.longo@arm.com> writes:

> The previous implementation of linux_find_memory_regions_full could
> still return success even when none of the /proc/PID/[s]maps files
> existed, or all reads returned 0 bytes (this last case can happen on
> Linux when the thread-group leader has exited).
> As a result, the function could incorrectly succeed, allowing core
> dump generation via the gcore command.
> This logical defect was allowing, by chance, the function to return
> success and hence, allowing fortuitously the coredump generation via
> gcore command (see gcore-stale-thread test for more details).
>
> A previous patch in this patch series fixed this issue by using the
> first LWP ID of the current inferior instead of relying on the PID.
> This patch adapts the code to use file_reader_t and makes the function
> return an error if reading any of the procfs files fails.
> ---
>  gdb/linux-tdep.c | 23 ++++++++++-------------
>  1 file changed, 10 insertions(+), 13 deletions(-)

Reviewed-by: Thiago Jung Bauermann <thiago.bauermann@linaro.org>

-- 
Thiago
(he/him)

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

[Joos, Christina]

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.

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

[Matthieu Longo]

On 10/08/2026 14:07, Joos, Christina wrote:
> 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/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.
> 

Fixed. And in others places where it is relevant.

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

Amended this paragraph in the commit message:

The patch converts some of the existing Linux, AMD64, and SPARC code that
reads files from /proc to use file_reader_t. As a side effect,
amd64_linux_lam_untag_mask and linux_process_address_in_memtag_page
may now return earlier in case the file is empty.

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

Yes, you're right.

However, I am wondering why the diagnostic message is not an error instead of warning.
Any idea ?

I would also like to change the program flow to something like:

  file_reader_t<char> adi_maps_freader
    (string_printf ("/proc/%d/adi/maps", pid));
  if (adi_maps_freader)
    {
      // This is skipped when the file is empty
      ...
    }
  else if (adi_maps_freader.error ())
    error (_("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 agree. Added.

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

Fixed as follows:

  file_reader_t (file_reader_t &&) = default;
  file_reader_t &operator= (file_reader_t &&) = default;

  DISABLE_COPY_AND_ASSIGN (file_reader_t);

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

The cases where we use .error (), the alternative should check whether the file is not empty. From
this perspective, including !.empty () in the boolean operator makes sense.

See for instance linux_vsyscall_range_raw or adi_is_addr_mapped, the loop with strtok_r() will be
skipped if the content is empty.

Matthieu

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

[Matthieu Longo]

On 14/08/2026 16:25, Matthieu Longo wrote:
> On 10/08/2026 14:07, Joos, Christina wrote:
>> 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/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.
>>
> 
> Fixed. And in others places where it is relevant.
> 
>>> +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.
>>
> 
> Amended this paragraph in the commit message:
> 
> The patch converts some of the existing Linux, AMD64, and SPARC code that
> reads files from /proc to use file_reader_t. As a side effect,
> amd64_linux_lam_untag_mask and linux_process_address_in_memtag_page
> may now return earlier in case the file is empty.
> 
>>> 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 ())
>>
>> ?
>>
> 
> Yes, you're right.
> 
> However, I am wondering why the diagnostic message is not an error instead of warning.
> Any idea ?
> 
> I would also like to change the program flow to something like:
> 
>   file_reader_t<char> adi_maps_freader
>     (string_printf ("/proc/%d/adi/maps", pid));
>   if (adi_maps_freader)
>     {
>       // This is skipped when the file is empty
>       ...
>     }
>   else if (adi_maps_freader.error ())
>     error (_("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 agree. Added.
> 
>> 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.
>>
> 
> Fixed as follows:
> 
>   file_reader_t (file_reader_t &&) = default;
>   file_reader_t &operator= (file_reader_t &&) = default;
> 
>   DISABLE_COPY_AND_ASSIGN (file_reader_t);
> 
>>> +  /* 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.
>>
>>
>> Christina
> 
> The cases where we use .error (), the alternative should check whether the file is not empty. From
> this perspective, including !.empty () in the boolean operator makes sense.
> 
> See for instance linux_vsyscall_range_raw or adi_is_addr_mapped, the loop with strtok_r() will be
> skipped if the content is empty.
> 
> Matthieu

Hi Christina,

Do you have further comments ?
I would like to publish a v2.

Regards,
Matthieu

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

[Joos, Christina]

> -----Original Message-----
> From: Matthieu Longo <matthieu.longo@arm.com>
> Sent: Freitag, 14. August 2026 17:26
> To: Joos, Christina <christina.joos@intel.com>; 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>
> Subject: Re: [PATCH v1 3/6] gdb: introduce helper class file_reader_t
> 
> On 10/08/2026 14:07, Joos, Christina wrote:
> > 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/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.
> >
> 
> Fixed. And in others places where it is relevant.
> 
> >> +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.
> >
> 
> Amended this paragraph in the commit message:
> 
> The patch converts some of the existing Linux, AMD64, and SPARC code that
> reads files from /proc to use file_reader_t. As a side effect,
> amd64_linux_lam_untag_mask and linux_process_address_in_memtag_page
> may now return earlier in case the file is empty.
> 
> >> 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 ())
> >
> > ?
> >
> 
> Yes, you're right.
> 
> However, I am wondering why the diagnostic message is not an error instead of
> warning.
> Any idea ?
> 
> I would also like to change the program flow to something like:
> 
>   file_reader_t<char> adi_maps_freader
>     (string_printf ("/proc/%d/adi/maps", pid));
>   if (adi_maps_freader)
>     {
>       // This is skipped when the file is empty
>       ...
>     }
>   else if (adi_maps_freader.error ())
>     error (_("unable to open /proc file '%s'"),
> 	     adi_maps_freader.c_filepath ());
>   return false;

Just a more general comment on this:
This rather sounds like something which can be a separate commit, since the reason
for this change is not the introduction of the helper class IIUC.
I know this seems like a super tiny nit, but it would make the review easier in my opinion.
Then you can point out in the commit message why you think a warning is better.

> >> 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 agree. Added.
> 
> > 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.
> >
> 
> Fixed as follows:
> 
>   file_reader_t (file_reader_t &&) = default;
>   file_reader_t &operator= (file_reader_t &&) = default;
> 
>   DISABLE_COPY_AND_ASSIGN (file_reader_t);
> 
> >> +  /* 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.
> >
> >
> > Christina
> 
> The cases where we use .error (), the alternative should check whether the file
> is not empty. From this perspective, including !.empty () in the boolean
> operator makes sense.
> 
> See for instance linux_vsyscall_range_raw or adi_is_addr_mapped, the loop
> with strtok_r() will be skipped if the content is empty.

As I said, I don't have a strong opinion on this. 😊

For the x86 related parts this patch lgtm, so for those parts:

Reviewed-by: Christina Joos <christina.joos@intel.com>

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.