[PATCH v1 00/10] gdb: bugfix 31207 and various refactoring in linux-tdep

[PATCH v1 00/10] gdb: bugfix 31207 and various refactoring in linux-tdep

[Matthieu Longo]

This patch series contains:
- a fix for bug 31207.
- a number of refactorings in linux-tdep, and the way procfs files are read.
- preparatory changes for upcoming AArch64 features support (POE, a.k.a Memory Protection Keys [2]
  and POE2) in GDB:
  - parsing of ProtectionKey field in /proc/PID/smaps
  - helpers to read AT_HWCAP3 and AT_HWCAP4

This is a spin-off of a previous patch series [3] where patches 1/8 and 4/8 were abandoned.
Also on demand of Yury Khrustalev, a helper for AT_HWCAP4 was added.
Thanks to Linaro's CIs, a regression was detected in test gcore-stale-thread and after investigation,
the root cause appears to be the same as bug 31207. Patch 2/10 address the root cause, and 7/10 fixes
a discovered issue in the logic of linux_find_memory_regions_full if an reading issue occurs.

All changes were tested by building GDB on x86_64 and AArch64 and running the existing GDB testsuite.

[1]: https://sourceware.org/bugzilla/show_bug.cgi?id=31207
[2]: https://docs.kernel.org/core-api/protection-keys.html
[3]: https://inbox.sourceware.org/gdb-patches/20260703185853.450440-1-matthieu.longo@arm.com/

Regards,
Matthieu


Matthieu Longo (10):
  gdb/linux-tdep: change linux_fill_prpsinfo to return bool
  gdb: rely on the first alive thread TPID when reading Linux procfs files
  target_fileio_read_stralloc: add an optional length parameter
  gdb support: add gdb::replace algorithm for iterators and ranges
  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: migrate parse_smaps_data to file_reader_t
  gdb/linux-tdep: parse ProtectionKey in /proc/PID/smaps
  gdb/linux: add helpers to read AT_HWCAP3 and AT_HWCAP4

 gdb/amd64-linux-tdep.c  |  12 +-
 gdb/inferior.c          |  12 ++
 gdb/inferior.h          |   9 +
 gdb/linux-tdep.c        | 356 ++++++++++++++++++++++------------------
 gdb/linux-tdep.h        |  22 +++
 gdb/sparc64-tdep.c      |  12 +-
 gdb/target.c            |  10 +-
 gdb/target.h            |  67 +++++++-
 gdbserver/linux-low.cc  |  28 ++++
 gdbserver/linux-low.h   |   8 +
 gdbsupport/array-view.h |  26 +++
 11 files changed, 386 insertions(+), 176 deletions(-)

-- 
2.55.0

[PATCH v1 01/10] gdb/linux-tdep: change linux_fill_prpsinfo to return bool

[Matthieu Longo]

Change linux_fill_prpsinfo() to return a boolean instead of an integer, since
it only reports success or failure.
Replace the returned integer values 1 and 0 with true and false respectively.
---
 gdb/linux-tdep.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/gdb/linux-tdep.c b/gdb/linux-tdep.c
index d5b0e6e7011..155d6e874d9 100644
--- a/gdb/linux-tdep.c
+++ b/gdb/linux-tdep.c
@@ -2258,7 +2258,7 @@ linux_corefile_parse_exec_context (struct gdbarch *gdbarch, bfd *cbfd)
    return 1 since some information was already recorded.  It will only return
    0 iff nothing can be gathered.  */
 
-static int
+static bool
 linux_fill_prpsinfo (struct elf_internal_linux_prpsinfo *p)
 {
   /* The filename which we will use to obtain some info about the process.
@@ -2297,14 +2297,14 @@ linux_fill_prpsinfo (struct elf_internal_linux_prpsinfo *p)
     {
       /* No program name was read, so we won't be able to retrieve more
 	 information about the process.  */
-      return 0;
+      return false;
     }
   if (fname.get ()[buf_len - 1] != '\0')
     {
       warning (_("target file %s "
 		 "does not contain a trailing null character"),
 	       filename);
-      return 0;
+      return false;
     }
 
   memset (p, 0, sizeof (*p));
@@ -2336,9 +2336,9 @@ linux_fill_prpsinfo (struct elf_internal_linux_prpsinfo *p)
   if (proc_stat == NULL || *proc_stat == '\0')
     {
       /* Despite being unable to read more information about the
-	 process, we return 1 here because at least we have its
+	 process, we return true here because at least we have its
 	 command line, PID and arguments.  */
-      return 1;
+      return true;
     }
 
   /* Ok, we have the stats.  It's time to do a little parsing of the
@@ -2359,7 +2359,7 @@ linux_fill_prpsinfo (struct elf_internal_linux_prpsinfo *p)
   /* ps command also relies on no trailing fields ever contain ')'.  */
   proc_stat = strrchr (proc_stat, ')');
   if (proc_stat == NULL)
-    return 1;
+    return true;
   proc_stat++;
 
   proc_stat = skip_spaces (proc_stat);
@@ -2384,8 +2384,8 @@ linux_fill_prpsinfo (struct elf_internal_linux_prpsinfo *p)
     {
       /* Again, we couldn't read the complementary information about
 	 the process state.  However, we already have minimal
-	 information, so we just return 1 here.  */
-      return 1;
+	 information, so we just return true here.  */
+      return true;
     }
 
   /* Filling the structure fields.  */
@@ -2413,8 +2413,8 @@ linux_fill_prpsinfo (struct elf_internal_linux_prpsinfo *p)
 
   if (proc_status == NULL || *proc_status == '\0')
     {
-      /* Returning 1 since we already have a bunch of information.  */
-      return 1;
+      /* Returning true since we already have a bunch of information.  */
+      return true;
     }
 
   /* Extracting the UID.  */
@@ -2443,7 +2443,7 @@ linux_fill_prpsinfo (struct elf_internal_linux_prpsinfo *p)
 	p->pr_gid = strtol (tmpstr, &tmpstr, 10);
     }
 
-  return 1;
+  return true;
 }
 
 /* Build the note section for a corefile, and return it in a malloc
-- 
2.55.0

[PATCH v1 02/10] gdb: rely on the first alive thread TPID when reading Linux procfs files

[Matthieu Longo]

On Linux, /proc/<pid> is keyed by the thread-group leader PID. When
the leader has exited, some /proc/<pid>/... entries become unavailable
even though another thread is still alive. This can happen, for instance,
when the main thread calls pthread_exit() and another thread continues
the execution (existing test: gcore-stale-thread).

This causes GDB to fail to read procfs entries such as cmdline, cwd,
exe, maps, and smaps when it uses 'current_inferior ()->pid' after the
thread-group leader has exited.

Fix this by adding inferior::first_alive_thread(), which returns the
PTID of the first non-exited thread of the inferior. Use its LWP ID
when accessing procfs entries that only need a representative live LWP
belonging to the process.

Update linux_info_proc, linux_process_address_in_memtag_page, and
linux_find_memory_regions_full to use this live-thread LWP ID instead
of the inferior PID when constructing procfs paths.

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31207
---
 gdb/inferior.c   | 12 ++++++++++++
 gdb/inferior.h   |  9 +++++++++
 gdb/linux-tdep.c | 38 ++++++++++++++++++--------------------
 3 files changed, 39 insertions(+), 20 deletions(-)

diff --git a/gdb/inferior.c b/gdb/inferior.c
index 1481f46cdd1..3b6ec60fb1b 100644
--- a/gdb/inferior.c
+++ b/gdb/inferior.c
@@ -250,6 +250,18 @@ inferior::find_thread (ptid_t ptid)
 
 /* See inferior.h.  */
 
+ptid_t
+inferior::first_alive_thread () const
+{
+  auto it = this->ptid_thread_map.cbegin ();
+  if (it != this->ptid_thread_map.cend ())
+    return it->first;
+  else
+    return ptid_t::make_null ();
+}
+
+/* See inferior.h.  */
+
 void
 inferior::clear_thread_list ()
 {
diff --git a/gdb/inferior.h b/gdb/inferior.h
index 9c031035a23..305b1d31830 100644
--- a/gdb/inferior.h
+++ b/gdb/inferior.h
@@ -513,6 +513,15 @@ class inferior : public refcounted_object,
   /* Find (non-exited) thread PTID of this inferior.  */
   thread_info *find_thread (ptid_t ptid);
 
+  /* Return the first (non-exited) thread PTID of this inferior.
+
+     This method should be used in place of current_inferior ()->pid for any
+     features relying only on the PID like the reading of procfs files.
+     On Linux, /proc/<pid> is keyed by the thread-group leader PID. When the
+     leader has exited, some /proc/<pid>/... entries become unavailable even
+     though another thread is still alive.  */
+  ptid_t first_alive_thread () const;
+
   /* Delete all threads in the thread list, silently.  */
   void clear_thread_list ();
 
diff --git a/gdb/linux-tdep.c b/gdb/linux-tdep.c
index 155d6e874d9..b89f4ee0717 100644
--- a/gdb/linux-tdep.c
+++ b/gdb/linux-tdep.c
@@ -842,9 +842,7 @@ static void
 linux_info_proc (struct gdbarch *gdbarch, const char *args,
 		 enum info_proc_what what)
 {
-  /* A long is used for pid instead of an int to avoid a loss of precision
-     compiler warning from the output of strtoul.  */
-  long pid;
+  ptid_t ptid;
   int cmdline_f = (what == IP_MINIMAL || what == IP_CMDLINE || what == IP_ALL);
   int cwd_f = (what == IP_MINIMAL || what == IP_CWD || what == IP_ALL);
   int environ_f = (what == IP_ENVIRON || what == IP_ALL);
@@ -859,7 +857,8 @@ linux_info_proc (struct gdbarch *gdbarch, const char *args,
     {
       char *tem;
 
-      pid = strtoul (args, &tem, 10);
+      auto pid = strtoul (args, &tem, 10);
+      ptid = ptid_t (pid, pid);
       args = tem;
     }
   else
@@ -869,17 +868,17 @@ linux_info_proc (struct gdbarch *gdbarch, const char *args,
       if (current_inferior ()->fake_pid_p)
 	error (_("Can't determine the current process's PID: you must name one."));
 
-      pid = current_inferior ()->pid;
+      ptid = current_inferior ()->first_alive_thread ();
     }
 
   args = skip_spaces (args);
   if (args && args[0])
     error (_("Too many parameters: %s"), args);
 
-  gdb_printf (_("process %ld\n"), pid);
+  gdb_printf (_("process %d\n"), ptid.pid ());
   if (cmdline_f)
     {
-      xsnprintf (filename, sizeof filename, "/proc/%ld/cmdline", pid);
+      xsnprintf (filename, sizeof filename, "/proc/%ld/cmdline", ptid.lwp ());
       gdb_byte *buffer;
       LONGEST len = target_fileio_read_alloc (nullptr, filename, &buffer);
 
@@ -901,7 +900,7 @@ linux_info_proc (struct gdbarch *gdbarch, const char *args,
     }
   if (cwd_f)
     {
-      xsnprintf (filename, sizeof filename, "/proc/%ld/cwd", pid);
+      xsnprintf (filename, sizeof filename, "/proc/%ld/cwd", ptid.lwp ());
       std::optional<std::string> contents
 	= target_fileio_readlink (NULL, filename, &target_errno);
       if (contents.has_value ())
@@ -911,7 +910,7 @@ linux_info_proc (struct gdbarch *gdbarch, const char *args,
     }
   if (environ_f)
     {
-      xsnprintf (filename, sizeof filename, "/proc/%ld/environ", pid);
+      xsnprintf (filename, sizeof filename, "/proc/%ld/environ", ptid.lwp ());
       gdb_byte *buffer;
       LONGEST len = target_fileio_read_alloc (nullptr, filename, &buffer);
 
@@ -935,7 +934,7 @@ linux_info_proc (struct gdbarch *gdbarch, const char *args,
     }
   if (exe_f)
     {
-      xsnprintf (filename, sizeof filename, "/proc/%ld/exe", pid);
+      xsnprintf (filename, sizeof filename, "/proc/%ld/exe", ptid.lwp ());
       std::optional<std::string> contents
 	= target_fileio_readlink (NULL, filename, &target_errno);
       if (contents.has_value ())
@@ -945,7 +944,7 @@ linux_info_proc (struct gdbarch *gdbarch, const char *args,
     }
   if (mappings_f)
     {
-      xsnprintf (filename, sizeof filename, "/proc/%ld/maps", pid);
+      xsnprintf (filename, sizeof filename, "/proc/%ld/maps", ptid.lwp ());
       gdb::unique_xmalloc_ptr<char> map
 	= target_fileio_read_stralloc (NULL, filename);
       if (map != NULL)
@@ -990,7 +989,7 @@ linux_info_proc (struct gdbarch *gdbarch, const char *args,
     }
   if (status_f)
     {
-      xsnprintf (filename, sizeof filename, "/proc/%ld/status", pid);
+      xsnprintf (filename, sizeof filename, "/proc/%ld/status", ptid.lwp ());
       gdb::unique_xmalloc_ptr<char> status
 	= target_fileio_read_stralloc (NULL, filename);
       if (status)
@@ -1000,7 +999,7 @@ linux_info_proc (struct gdbarch *gdbarch, const char *args,
     }
   if (stat_f)
     {
-      xsnprintf (filename, sizeof filename, "/proc/%ld/stat", pid);
+      xsnprintf (filename, sizeof filename, "/proc/%ld/stat", ptid.lwp ());
       gdb::unique_xmalloc_ptr<char> statstr
 	= target_fileio_read_stralloc (NULL, filename);
       if (statstr)
@@ -1671,9 +1670,9 @@ linux_process_address_in_memtag_page (CORE_ADDR address)
   if (current_inferior ()->fake_pid_p)
     return false;
 
-  pid_t pid = current_inferior ()->pid;
+  ptid_t ptid = current_inferior ()->first_alive_thread ();
 
-  std::string smaps_file = string_printf ("/proc/%d/smaps", pid);
+  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 ());
@@ -1731,7 +1730,6 @@ linux_find_memory_regions_full (struct gdbarch *gdbarch,
 				linux_dump_mapping_p_ftype *should_dump_mapping_p,
 				linux_find_memory_region_ftype func)
 {
-  pid_t pid;
   /* Default dump behavior of coredump_filter (0x33), according to
      Documentation/filesystems/proc.txt from the Linux kernel
      tree.  */
@@ -1744,12 +1742,12 @@ linux_find_memory_regions_full (struct gdbarch *gdbarch,
   if (current_inferior ()->fake_pid_p)
     return false;
 
-  pid = current_inferior ()->pid;
+  ptid_t ptid = current_inferior ()->first_alive_thread ();
 
   if (use_coredump_filter)
     {
       std::string core_dump_filter_name
-	= string_printf ("/proc/%d/coredump_filter", pid);
+	= 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 ());
@@ -1763,7 +1761,7 @@ linux_find_memory_regions_full (struct gdbarch *gdbarch,
 	}
     }
 
-  std::string maps_filename = string_printf ("/proc/%d/smaps", pid);
+  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 ());
@@ -1771,7 +1769,7 @@ linux_find_memory_regions_full (struct gdbarch *gdbarch,
   if (data == NULL)
     {
       /* Older Linux kernels did not support /proc/PID/smaps.  */
-      maps_filename = string_printf ("/proc/%d/maps", pid);
+      maps_filename = string_printf ("/proc/%ld/maps", ptid.lwp ());
       data = target_fileio_read_stralloc (NULL, maps_filename.c_str ());
 
       if (data == nullptr)
-- 
2.55.0

[PATCH v1 03/10] target_fileio_read_stralloc: add an optional length parameter

[Matthieu Longo]

---
 gdb/target.c | 10 +++++++---
 gdb/target.h |  2 +-
 2 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/gdb/target.c b/gdb/target.c
index 5d937f3ae85..e4907ca815a 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,
+			     size_t *len)
 {
   gdb_byte *buffer;
   char *bufstr;
@@ -3556,17 +3557,20 @@ target_fileio_read_stralloc (struct inferior *inf, const char *filename)
   transferred = target_fileio_read_alloc_1 (inf, filename, &buffer, 1);
   bufstr = (char *) buffer;
 
+  if (len != nullptr)
+    *len = (transferred < 0 ? 0 : 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 22653138491..4215553033c 100644
--- a/gdb/target.h
+++ b/gdb/target.h
@@ -2336,7 +2336,7 @@ extern LONGEST target_fileio_read_alloc (struct inferior *inf,
    are returned as allocated but empty strings.  A warning is issued
    if the result contains any 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, size_t *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 04/10] gdb support: add gdb::replace algorithm for iterators and ranges

[Matthieu Longo]

Add a gdb::replace helper that mirrors the behavior of std::replace
for iterator pairs, together with a convenience overload accepting a
range.

This provides a C++17-compatible replacement for the C++20 std::replace/
std::ranges::replace algorithms, allowing callers to use a consistent
interface until GDB transitions to C++20. The helpers should be removed
once the C++ standard library implementations become available.
---
 gdbsupport/array-view.h | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/gdbsupport/array-view.h b/gdbsupport/array-view.h
index 8431d7f5add..61119d7a8e2 100644
--- a/gdbsupport/array-view.h
+++ b/gdbsupport/array-view.h
@@ -225,6 +225,32 @@ void copy (gdb::array_view<U> src, gdb::array_view<T> dest)
     std::copy_backward (src.begin (), src.end (), dest.end ());
 }
 
+/* Replace all occurrences of a value in the provided range.
+
+   Note: this helper is a reimplementation of std::replace, only available
+   from C++20 onwards, and consequently, should be removed once we switch
+   to C++20.  */
+
+template <class ForwardIt, typename T>
+void replace (ForwardIt first, ForwardIt last,
+	      const T &old_value, const T &new_value)
+{
+  for (auto it = first; it != last; ++it)
+  {
+    if (*it == old_value)
+      *it = new_value;
+  }
+}
+
+/* Replace all occurrences of a value in the provided array view.
+   Note: from C++20 onwards, std::ranges::replace should be used instead.  */
+
+template <class Range, typename T>
+void replace (Range r, const T &old_value, const T &new_value)
+{
+  replace (r.begin (), r.end (), old_value, new_value);
+}
+
 /* 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 05/10] 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       | 102 +++++++++++++++++------------------------
 gdb/sparc64-tdep.c     |  12 ++---
 gdb/target.h           |  65 ++++++++++++++++++++++++++
 4 files changed, 117 insertions(+), 74 deletions(-)

diff --git a/gdb/amd64-linux-tdep.c b/gdb/amd64-linux-tdep.c
index a5ac26654cf..e7841917eaf 100644
--- a/gdb/amd64-linux-tdep.c
+++ b/gdb/amd64-linux-tdep.c
@@ -1851,14 +1851,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)
@@ -1870,7 +1867,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 b89f4ee0717..a12a69f03a2 100644
--- a/gdb/linux-tdep.c
+++ b/gdb/linux-tdep.c
@@ -1661,6 +1661,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.  */
 
@@ -1672,17 +1678,13 @@ linux_process_address_in_memtag_page (CORE_ADDR address)
 
   ptid_t ptid = current_inferior ()->first_alive_thread ();
 
-  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)
     {
@@ -1746,17 +1748,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;
 	}
     }
@@ -2259,9 +2257,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.  */
@@ -2285,23 +2280,24 @@ linux_fill_prpsinfo (struct elf_internal_linux_prpsinfo *p)
 
   /* Obtaining PID and filename.  */
   pid = inferior_ptid.pid ();
-  xsnprintf (filename, sizeof (filename), "/proc/%d/cmdline", (int) pid);
-  /* 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/%d/cmdline", pid));
+  if (!cmdline_freader)
+    return false;
 
-  if (buf_len < 1 || fname.get () == nullptr || fname.get ()[0] == '\0')
+  /* The full name of the program which generated the corefile.  */
+  gdb::array_view<char> cmdline = cmdline_freader.cast_view<char> ();
+  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;
     }
 
@@ -2311,27 +2307,23 @@ linux_fill_prpsinfo (struct elf_internal_linux_prpsinfo *p)
   p->pr_pid = 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/%d/stat", (int) pid);
-  /* 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/%d/stat", pid));
+  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
@@ -2403,13 +2395,9 @@ 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/%d/status", (int) pid);
-  /* 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/%d/status", pid));
+  char *proc_status = status_freader.data ();
+  if (!status_freader || *proc_status == '\0')
     {
       /* Returning true since we already have a bunch of information.  */
       return true;
@@ -2802,9 +2790,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;
 
@@ -2842,7 +2827,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
@@ -2852,15 +2837,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)
     {
       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))
 	{
@@ -2879,7 +2863,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;
 }
@@ -3168,16 +3153,11 @@ linux_address_in_shadow_stack_mem_range
 
   const int pid = current_inferior ()->pid;
 
-  std::string smaps_file = string_printf ("/proc/%d/smaps", pid);
-
-  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/%d/smaps", pid));
+  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..811b76f27d8 100644
--- a/gdb/sparc64-tdep.c
+++ b/gdb/sparc64-tdep.c
@@ -305,14 +305,13 @@ adi_is_addr_mapped (CORE_ADDR vaddr, size_t cnt)
   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)
     {
       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 +328,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 4215553033c..d6fc101f205 100644
--- a/gdb/target.h
+++ b/gdb/target.h
@@ -2338,6 +2338,71 @@ 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, size_t *len = nullptr);
 
+/* Helper class for reading a file.  */
+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;
+  /* Size of the data.  */
+  size_t 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));
+      }
+  }
+
+  /* Return true if the file was read successfully.  */
+  operator bool () const noexcept
+  { return m_data != nullptr && m_size > 0; }
+
+  /* Return a pointer to the data.  */
+  T *data () const noexcept
+  { return m_data.get (); }
+
+  /* Return the size of the data.  */
+  size_t 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, cast to a different type.  */
+  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 filepath of the file that was read.  */
+  const std::string &filepath () const noexcept
+  { return m_filepath; }
+
+  /* Return the filepath 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 06/10] gdb/linux-tdep: migrate linux_info_proc to file_reader_t

[Matthieu Longo]

The patch migratse 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 | 120 +++++++++++++++++++++++++++--------------------
 1 file changed, 69 insertions(+), 51 deletions(-)

diff --git a/gdb/linux-tdep.c b/gdb/linux-tdep.c
index a12a69f03a2..a2af1586d35 100644
--- a/gdb/linux-tdep.c
+++ b/gdb/linux-tdep.c
@@ -836,6 +836,27 @@ dump_note_entry_p (filter_flags filterflags, const smaps_data &map)
   return true;
 }
 
+/* In a character buffer where entries are separated by a SEPARATOR character,
+   extract the string view starting at START.
+   Return the extracted view and the iterator to the next entry.  */
+
+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')
+{
+  if (start < buffer.begin () || start >= buffer.end ())
+    return std::make_pair (gdb::array_view<char> (), buffer.end ());
+
+  auto it = std::find (start, buffer.end (), separator);
+  if (it == buffer.end ())
+    return std::make_pair (gdb::array_view<char> (), buffer.end ());
+
+  auto next_start = std::next (it);
+  return std::make_pair
+    (gdb::array_view<char> (start, next_start), next_start);
+}
+
 /* Implement the "info proc" command.  */
 
 static void
@@ -878,25 +899,20 @@ linux_info_proc (struct gdbarch *gdbarch, const char *args,
   gdb_printf (_("process %d\n"), ptid.pid ());
   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);
+	  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::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)
     {
@@ -910,27 +926,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)
     {
@@ -944,10 +958,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");
@@ -961,12 +974,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);
@@ -985,26 +1002,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)));
@@ -1131,7 +1148,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 07/10] 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 codedump 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 is 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 a2af1586d35..f1cea3040f9 100644
--- a/gdb/linux-tdep.c
+++ b/gdb/linux-tdep.c
@@ -1777,25 +1777,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 08/10] gdb/linux-tdep: migrate parse_smaps_data to file_reader_t

[Matthieu Longo]

---
 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 f1cea3040f9..dbc69a5a7fc 100644
--- a/gdb/linux-tdep.c
+++ b/gdb/linux-tdep.c
@@ -1530,18 +1530,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;
 
@@ -1597,8 +1596,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;
 	    }
 
@@ -1612,12 +1611,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;
 
@@ -1628,8 +1627,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)
@@ -1679,12 +1679,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

[PATCH v1 09/10] gdb/linux-tdep: parse ProtectionKey in /proc/PID/smaps

[Matthieu Longo]

Memory Protection Keys provide a mechanism for enforcing page-based
protections without requiring modification of the page tables
when an application changes protection domains. [1]

The "ProtectionKey" field may be present in /proc/PID/smaps x86_64
and AArch64 systems since Linux 4.9, when the kernel is built with
Memory Protection Keys support.

Add a new 'pkey' field to `struct smaps_data', and populate it when
the "ProtectionKey" field is present.

This prepares for displaying the protection key in 'info proc mappings'.

[1]: https://docs.kernel.org/core-api/protection-keys.html,
     https://lkml.iu.edu/1512.0/03058.html
---
 gdb/linux-tdep.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/gdb/linux-tdep.c b/gdb/linux-tdep.c
index dbc69a5a7fc..d89c5ae8504 100644
--- a/gdb/linux-tdep.c
+++ b/gdb/linux-tdep.c
@@ -124,6 +124,7 @@ struct smaps_data
 
   ULONGEST rss;
   ULONGEST swap;
+  std::optional<int> pkey;
 };
 
 /* Whether to take the /proc/PID/coredump_filter into account when
@@ -1553,6 +1554,7 @@ parse_smaps_data (const file_reader_t<char> &freader)
       int mapping_file_p;
       ULONGEST rss = -1;
       ULONGEST swap = -1;
+      int pkey = -1;
 
       memset (&v, 0, sizeof (v));
       struct mapping m = read_mapping (line);
@@ -1653,6 +1655,16 @@ parse_smaps_data (const file_reader_t<char> &freader)
 		  mapping_anon_p = 1;
 		}
 	    }
+
+	  if (streq (keyword, "ProtectionKey:"))
+	    {
+	      if (sscanf (line, "%*s%d", &pkey) != 1)
+		{
+		  warning (_("Error parsing %s's value in {s,}maps file '%s'"),
+			   keyword, freader.c_filepath ());
+		  break;
+		}
+	    }
 	}
       /* Save the smaps entry to the vector.  */
 	struct smaps_data map;
@@ -1672,6 +1684,7 @@ parse_smaps_data (const file_reader_t<char> &freader)
 	map.inode = m.inode;
 	map.rss = rss;
 	map.swap = swap;
+	map.pkey.emplace (pkey);
 
 	smaps.emplace_back (map);
     }
-- 
2.55.0

[PATCH v1 10/10] gdb/linux: add helpers to read AT_HWCAP3 and AT_HWCAP4

[Matthieu Longo]

Add linux_get_hwcap3 and linux_get_hwcap4 helpers to GDB and gdbserver,
mirroring the existing linux_get_hwcap and linux_get_hwcap2 interfaces.

The new helpers retrieve the AT_HWCAP3 and AT_HWCAP4 auxiliary vector
entry either from explicitly supplied auxv data or from the current
inferior.

This prepares for future features that depend on HWCAP3 and HWCAP4
capability bits.
---
 gdb/linux-tdep.c       | 38 ++++++++++++++++++++++++++++++++++++++
 gdb/linux-tdep.h       | 22 ++++++++++++++++++++++
 gdbserver/linux-low.cc | 28 ++++++++++++++++++++++++++++
 gdbserver/linux-low.h  |  8 ++++++++
 4 files changed, 96 insertions(+)

diff --git a/gdb/linux-tdep.c b/gdb/linux-tdep.c
index d89c5ae8504..24774ad3c3d 100644
--- a/gdb/linux-tdep.c
+++ b/gdb/linux-tdep.c
@@ -3142,6 +3142,44 @@ linux_get_hwcap2 ()
 			   current_inferior ()->arch ());
 }
 
+/* See linux-tdep.h.  */
+
+CORE_ADDR
+linux_get_hwcap3 (const std::optional<gdb::byte_vector> &auxv,
+		  target_ops *target, gdbarch *gdbarch)
+{
+  return linux_get_hwcap_helper (auxv, target, gdbarch, AT_HWCAP3);
+}
+
+/* See linux-tdep.h.  */
+
+CORE_ADDR
+linux_get_hwcap3 ()
+{
+  return linux_get_hwcap3 (target_read_auxv (),
+			   current_inferior ()->top_target (),
+			   current_inferior ()->arch ());
+}
+
+/* See linux-tdep.h.  */
+
+CORE_ADDR
+linux_get_hwcap4 (const std::optional<gdb::byte_vector> &auxv,
+		  target_ops *target, gdbarch *gdbarch)
+{
+  return linux_get_hwcap_helper (auxv, target, gdbarch, AT_HWCAP4);
+}
+
+/* See linux-tdep.h.  */
+
+CORE_ADDR
+linux_get_hwcap4 ()
+{
+  return linux_get_hwcap4 (target_read_auxv (),
+			   current_inferior ()->top_target (),
+			   current_inferior ()->arch ());
+}
+
 /* Display whether the gcore command is using the
    /proc/PID/coredump_filter file.  */
 
diff --git a/gdb/linux-tdep.h b/gdb/linux-tdep.h
index c19839fde2c..9b30392fb04 100644
--- a/gdb/linux-tdep.h
+++ b/gdb/linux-tdep.h
@@ -91,6 +91,28 @@ extern CORE_ADDR linux_get_hwcap2 (const std::optional<gdb::byte_vector> &auxv,
 
 extern CORE_ADDR linux_get_hwcap2 ();
 
+/* Fetch the AT_HWCAP3 entry from auxv data AUXV.  Use TARGET and GDBARCH to
+   parse auxv entries.
+
+   On error, 0 is returned.  */
+extern CORE_ADDR linux_get_hwcap3 (const std::optional<gdb::byte_vector> &auxv,
+				   struct target_ops *target, gdbarch *gdbarch);
+
+/* Same as the above, but obtain all the inputs from the current inferior.  */
+
+extern CORE_ADDR linux_get_hwcap3 ();
+
+/* Fetch the AT_HWCAP4 entry from auxv data AUXV.  Use TARGET and GDBARCH to
+   parse auxv entries.
+
+   On error, 0 is returned.  */
+extern CORE_ADDR linux_get_hwcap4 (const std::optional<gdb::byte_vector> &auxv,
+				   struct target_ops *target, gdbarch *gdbarch);
+
+/* Same as the above, but obtain all the inputs from the current inferior.  */
+
+extern CORE_ADDR linux_get_hwcap4 ();
+
 /* Returns true if ADDR belongs to a shadow stack memory range.  If this
    is the case, assign the shadow stack memory range to RANGE
    [start_address, end_address).  */
diff --git a/gdbserver/linux-low.cc b/gdbserver/linux-low.cc
index ade5e9e2a1c..029f64321c2 100644
--- a/gdbserver/linux-low.cc
+++ b/gdbserver/linux-low.cc
@@ -70,6 +70,14 @@
 #define AT_HWCAP2 26
 #endif
 
+#ifndef AT_HWCAP3
+#define AT_HWCAP3 29
+#endif
+
+#ifndef AT_HWCAP4
+#define AT_HWCAP4 30
+#endif
+
 /* Some targets did not define these ptrace constants from the start,
    so gdbserver defines them locally here.  In the future, these may
    be removed after they are added to asm/ptrace.h.  */
@@ -7198,6 +7206,26 @@ linux_get_hwcap2 (int pid, int wordsize)
   return hwcap2;
 }
 
+/* See linux-low.h.  */
+
+CORE_ADDR
+linux_get_hwcap3 (int pid, int wordsize)
+{
+  CORE_ADDR hwcap3 = 0;
+  linux_get_auxv (pid, wordsize, AT_HWCAP3, &hwcap3);
+  return hwcap3;
+}
+
+/* See linux-low.h.  */
+
+CORE_ADDR
+linux_get_hwcap4 (int pid, int wordsize)
+{
+  CORE_ADDR hwcap4 = 0;
+  linux_get_auxv (pid, wordsize, AT_HWCAP4, &hwcap4);
+  return hwcap4;
+}
+
 #ifdef HAVE_LINUX_REGSETS
 void
 initialize_regsets_info (struct regsets_info *info)
diff --git a/gdbserver/linux-low.h b/gdbserver/linux-low.h
index 03e11202955..c2ebdc228bc 100644
--- a/gdbserver/linux-low.h
+++ b/gdbserver/linux-low.h
@@ -977,4 +977,12 @@ CORE_ADDR linux_get_hwcap (int pid, int wordsize);
 
 CORE_ADDR linux_get_hwcap2 (int pid, int wordsize);
 
+/* Fetch the AT_HWCAP3 entry from the auxv vector, where entries are length
+   WORDSIZE, of process with pid PID.  If no entry was found, return 0.  */
+CORE_ADDR linux_get_hwcap3 (int pid, int wordsize);
+
+/* Fetch the AT_HWCAP4 entry from the auxv vector, where entries are length
+   WORDSIZE, of process with pid PID.  If no entry was found, return 0.  */
+CORE_ADDR linux_get_hwcap4 (int pid, int wordsize);
+
 #endif /* GDBSERVER_LINUX_LOW_H */
-- 
2.55.0

Re: [PATCH v1 01/10] gdb/linux-tdep: change linux_fill_prpsinfo to return bool

[Thiago Jung Bauermann]

Hello Matthieu,

Thank you for these patches.

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

> Change linux_fill_prpsinfo() to return a boolean instead of an integer, since
> it only reports success or failure.
> Replace the returned integer values 1 and 0 with true and false respectively.
> ---
>  gdb/linux-tdep.c | 22 +++++++++++-----------
>  1 file changed, 11 insertions(+), 11 deletions(-)

In his review of an analogous patch, Simon suggested that this kind of
change could be pushed as obvious:

https://inbox.sourceware.org/gdb-patches/53a86cae-2927-49ec-83a2-22ff6526c0e6@simark.ca/

I would agree, but I don't know if that's a generally accepted view.

In any case:

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

-- 
Thiago
(he/him)

Re: [PATCH v1 02/10] gdb: rely on the first alive thread TPID when reading Linux procfs files

[Thiago Jung Bauermann]

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

> On Linux, /proc/<pid> is keyed by the thread-group leader PID. When
> the leader has exited, some /proc/<pid>/... entries become unavailable
> even though another thread is still alive. This can happen, for instance,
> when the main thread calls pthread_exit() and another thread continues
> the execution (existing test: gcore-stale-thread).
>
> This causes GDB to fail to read procfs entries such as cmdline, cwd,
> exe, maps, and smaps when it uses 'current_inferior ()->pid' after the
> thread-group leader has exited.
>
> Fix this by adding inferior::first_alive_thread(), which returns the
> PTID of the first non-exited thread of the inferior. Use its LWP ID
> when accessing procfs entries that only need a representative live LWP
> belonging to the process.
>
> Update linux_info_proc, linux_process_address_in_memtag_page, and
> linux_find_memory_regions_full to use this live-thread LWP ID instead
> of the inferior PID when constructing procfs paths.
>
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31207
> ---
>  gdb/inferior.c   | 12 ++++++++++++
>  gdb/inferior.h   |  9 +++++++++
>  gdb/linux-tdep.c | 38 ++++++++++++++++++--------------------
>  3 files changed, 39 insertions(+), 20 deletions(-)

It's a pity that the patch originally proposted to solve this bug:

https://inbox.sourceware.org/gdb-patches/20250412201140.31510-1-dominik.b.czarnota@gmail.com/

was approved but never committed. But your version is more
comprehensive, since it also makes the change for other procfs files.
It also explicitly looks for a live thread.

I have a couple of comments below, but regardless:

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

> diff --git a/gdb/linux-tdep.c b/gdb/linux-tdep.c
> index 155d6e874d9..b89f4ee0717 100644
> --- a/gdb/linux-tdep.c
> +++ b/gdb/linux-tdep.c
> @@ -842,9 +842,7 @@ static void
>  linux_info_proc (struct gdbarch *gdbarch, const char *args,
>  		 enum info_proc_what what)
>  {
> -  /* A long is used for pid instead of an int to avoid a loss of precision
> -     compiler warning from the output of strtoul.  */
> -  long pid;
> +  ptid_t ptid;
>    int cmdline_f = (what == IP_MINIMAL || what == IP_CMDLINE || what == IP_ALL);
>    int cwd_f = (what == IP_MINIMAL || what == IP_CWD || what == IP_ALL);
>    int environ_f = (what == IP_ENVIRON || what == IP_ALL);
> @@ -859,7 +857,8 @@ linux_info_proc (struct gdbarch *gdbarch, const char *args,
>      {
>        char *tem;
>  
> -      pid = strtoul (args, &tem, 10);
> +      auto pid = strtoul (args, &tem, 10);

The actual type of pid here will be long, right? Unless there's a an
advantage to using auto, I think in this case the code is clearer if the
type is explicitly mentioned.

> +      ptid = ptid_t (pid, pid);
>        args = tem;
>      }
>    else
> @@ -869,17 +868,17 @@ linux_info_proc (struct gdbarch *gdbarch, const char *args,
>        if (current_inferior ()->fake_pid_p)
>  	error (_("Can't determine the current process's PID: you must name one."));
>  
> -      pid = current_inferior ()->pid;
> +      ptid = current_inferior ()->first_alive_thread ();
>      }
>  
>    args = skip_spaces (args);
>    if (args && args[0])
>      error (_("Too many parameters: %s"), args);
>  
> -  gdb_printf (_("process %ld\n"), pid);
> +  gdb_printf (_("process %d\n"), ptid.pid ());
>    if (cmdline_f)
>      {
> -      xsnprintf (filename, sizeof filename, "/proc/%ld/cmdline", pid);
> +      xsnprintf (filename, sizeof filename, "/proc/%ld/cmdline", ptid.lwp ());
>        gdb_byte *buffer;
>        LONGEST len = target_fileio_read_alloc (nullptr, filename, &buffer);
>  
> @@ -901,7 +900,7 @@ linux_info_proc (struct gdbarch *gdbarch, const char *args,
>      }
>    if (cwd_f)
>      {
> -      xsnprintf (filename, sizeof filename, "/proc/%ld/cwd", pid);
> +      xsnprintf (filename, sizeof filename, "/proc/%ld/cwd", ptid.lwp ());

proc_pid_cwd(5) says:

  In a multithreaded process, the contents of this symbolic
  link are not available if the main thread has already
  terminated (typically by calling pthread_exit(3)).

I think it's ok to leave the code as is for consistency, but it's worth
mentioning this detail in a comment.

>        std::optional<std::string> contents
>  	= target_fileio_readlink (NULL, filename, &target_errno);
>        if (contents.has_value ())
> @@ -911,7 +910,7 @@ linux_info_proc (struct gdbarch *gdbarch, const char *args,
>      }
>    if (environ_f)
>      {
> -      xsnprintf (filename, sizeof filename, "/proc/%ld/environ", pid);
> +      xsnprintf (filename, sizeof filename, "/proc/%ld/environ", ptid.lwp ());
>        gdb_byte *buffer;
>        LONGEST len = target_fileio_read_alloc (nullptr, filename, &buffer);
>  
> @@ -935,7 +934,7 @@ linux_info_proc (struct gdbarch *gdbarch, const char *args,
>      }
>    if (exe_f)
>      {
> -      xsnprintf (filename, sizeof filename, "/proc/%ld/exe", pid);
> +      xsnprintf (filename, sizeof filename, "/proc/%ld/exe", ptid.lwp ());

Same comment here. proc_pid_exe(5) has the same observation.

>        std::optional<std::string> contents
>  	= target_fileio_readlink (NULL, filename, &target_errno);
>        if (contents.has_value ())

-- 
Thiago
(he/him)

Re: [PATCH v1 03/10] target_fileio_read_stralloc: add an optional length parameter

[Thiago Jung Bauermann]

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

> diff --git a/gdb/target.h b/gdb/target.h
> index 22653138491..4215553033c 100644
> --- a/gdb/target.h
> +++ b/gdb/target.h
> @@ -2336,7 +2336,7 @@ extern LONGEST target_fileio_read_alloc (struct inferior *inf,
>     are returned as allocated but empty strings.  A warning is issued
>     if the result contains any 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, size_t *len = nullptr);

It's worth updating the documentation comment to mention the new parameter.

>  
>  /* Invalidate the target associated with open handles that were open
>     on target TARG, since we're about to close (and maybe destroy) the

-- 
Thiago
(he/him)

Re: [PATCH v1 04/10] gdb support: add gdb::replace algorithm for iterators and ranges

[Thiago Jung Bauermann]

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

> Add a gdb::replace helper that mirrors the behavior of std::replace
> for iterator pairs, together with a convenience overload accepting a
> range.
>
> This provides a C++17-compatible replacement for the C++20 std::replace/
> std::ranges::replace algorithms, allowing callers to use a consistent
> interface until GDB transitions to C++20. The helpers should be removed
> once the C++ standard library implementations become available.
> ---
>  gdbsupport/array-view.h | 26 ++++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)

This looks good to me, but I'm not among the most knowledgeable C++
developers here so I'd rather not provide a Reviewed-by.

-- 
Thiago
(he/him)

Re: [PATCH v1 05/10] 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       | 102 +++++++++++++++++------------------------
>  gdb/sparc64-tdep.c     |  12 ++---
>  gdb/target.h           |  65 ++++++++++++++++++++++++++
>  4 files changed, 117 insertions(+), 74 deletions(-)

With the changes I suggested below:

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

> @@ -2285,23 +2280,24 @@ linux_fill_prpsinfo (struct elf_internal_linux_prpsinfo *p)
>  
>    /* Obtaining PID and filename.  */
>    pid = inferior_ptid.pid ();
> -  xsnprintf (filename, sizeof (filename), "/proc/%d/cmdline", (int) pid);
> -  /* 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/%d/cmdline", pid));

I was scratching my head for a bit trying to understand why not use
file_reader_t<char> here and avoid having to use cast_view<char>
below. I think it's worth a comment mentioning that it's because cmdline
has '\0' separating the arguments.

> +  if (!cmdline_freader)
> +    return false;
>  
> -  if (buf_len < 1 || fname.get () == nullptr || fname.get ()[0] == '\0')
> +  /* The full name of the program which generated the corefile.  */
> +  gdb::array_view<char> cmdline = cmdline_freader.cast_view<char> ();
> +  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;
>      }
>  
⋮
> diff --git a/gdb/sparc64-tdep.c b/gdb/sparc64-tdep.c
> index 93db3417a2a..811b76f27d8 100644
> --- a/gdb/sparc64-tdep.c
> +++ b/gdb/sparc64-tdep.c
> @@ -305,14 +305,13 @@ adi_is_addr_mapped (CORE_ADDR vaddr, size_t cnt)
>    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);

This change leaves filename unused and I get a build error when using
"configure --enable-targets=all":

  CXX    sparc64-tdep.o
/home/bauermann/src/binutils-gdb-wt-2/gdb/sparc64-tdep.c: In function 'bool adi_is_addr_mapped(CORE_ADDR, size_t)':
/home/bauermann/src/binutils-gdb-wt-2/gdb/sparc64-tdep.c:304:8: error: unused variable 'filename' [-Werror=unused-variable]
  304 |   char filename[MAX_PROC_NAME_SIZE];
      |        ^~~~~~~~
cc1plus: all warnings being treated as errors
make[2]: *** [Makefile:2100: sparc64-tdep.o] Error 1

> -  if (data)
> +  file_reader_t<char> adi_maps_freader
> +    (string_printf ("/proc/%d/adi/maps", pid));

I'll just note that this patch changes the printf pattern from "%ld" and
"(long) pid" to "%d" and "pid". I think the change is for the better and
I can't think of why it was done the other way before.

> +  if (adi_maps_freader)
>      {
>        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))
>  	{

-- 
Thiago
(he/him)

Re: [PATCH v1 06/10] gdb/linux-tdep: migrate linux_info_proc to file_reader_t

[Thiago Jung Bauermann]

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

> The patch migratse the code of linux_info_proc to use file_reader_t to

Typo: migrates

> 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 | 120 +++++++++++++++++++++++++++--------------------
>  1 file changed, 69 insertions(+), 51 deletions(-)

Nice improvement, thanks!

Just a couple of nits, but:

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

> diff --git a/gdb/linux-tdep.c b/gdb/linux-tdep.c
> index a12a69f03a2..a2af1586d35 100644
> --- a/gdb/linux-tdep.c
> +++ b/gdb/linux-tdep.c
> @@ -836,6 +836,27 @@ dump_note_entry_p (filter_flags filterflags, const smaps_data &map)
>    return true;
>  }
>  
> +/* In a character buffer where entries are separated by a SEPARATOR character,
> +   extract the string view starting at START.
> +   Return the extracted view and the iterator to the next entry.  */
> +
> +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')
> +{
> +  if (start < buffer.begin () || start >= buffer.end ())
> +    return std::make_pair (gdb::array_view<char> (), buffer.end ());
> +
> +  auto it = std::find (start, buffer.end (), separator);
> +  if (it == buffer.end ())
> +    return std::make_pair (gdb::array_view<char> (), buffer.end ());
> +
> +  auto next_start = std::next (it);
> +  return std::make_pair
> +    (gdb::array_view<char> (start, next_start), next_start);

This line fits 80 columns and doesn't need to be broken.

> +}
> +
>  /* Implement the "info proc" command.  */
>  
>  static void
> @@ -878,25 +899,20 @@ linux_info_proc (struct gdbarch *gdbarch, const char *args,
>    gdb_printf (_("process %d\n"), ptid.pid ());
>    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);
> +	  gdb::array_view<char> cmdline = cmdline_freader.cast_view<char> ();
> +	  gdb_assert (cmdline[ cmdline.size () - 1] == '\0');

Extraneous space after '['.

> +	  /* Replace null characters splitting the arguments in the command
> +	     line by spaces, except for the last one.  */
> +	  gdb::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)
>      {

-- 
Thiago
(he/him)

Re: [PATCH v1 07/10] 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 codedump generation via
> gcore command (see gcore-stale-thread test for more details).

Did you run into this problem, or just noticed it by inspecting the
code? If the former, do you think it's worth adding a testcase to make
sure this problem doesn't appear again?

> 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 is reading any of the procfs files fails.

Typo: is → if

> ---
>  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 08/10] gdb/linux-tdep: migrate parse_smaps_data to file_reader_t

[Thiago Jung Bauermann]

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

Suggestion: mention in the commit log that since the previous commit,
the old variant of parse_smaps_data is unused.

> ---
>  gdb/linux-tdep.c | 30 ++++++++++++------------------
>  1 file changed, 12 insertions(+), 18 deletions(-)

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

> @@ -1628,8 +1627,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 ());

Nice improvement to the error message, thanks.

>  		  break;
>  		}
>  	      if (number > 0)

-- 
Thiago
(he/him)

Re: [PATCH v1 09/10] gdb/linux-tdep: parse ProtectionKey in /proc/PID/smaps

[Thiago Jung Bauermann]

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

> Memory Protection Keys provide a mechanism for enforcing page-based
> protections without requiring modification of the page tables
> when an application changes protection domains. [1]
>
> The "ProtectionKey" field may be present in /proc/PID/smaps x86_64
> and AArch64 systems since Linux 4.9, when the kernel is built with
> Memory Protection Keys support.

/me from a previous life would feel the need to point out that it may
also be present on POWER systems. :)

> Add a new 'pkey' field to `struct smaps_data', and populate it when
> the "ProtectionKey" field is present.
>
> This prepares for displaying the protection key in 'info proc mappings'.
>
> [1]: https://docs.kernel.org/core-api/protection-keys.html,
>      https://lkml.iu.edu/1512.0/03058.html

I suggest linking to the commit that went upstream instead of the
mailing list patch:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=c1192f842841

> ---
>  gdb/linux-tdep.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
>
> diff --git a/gdb/linux-tdep.c b/gdb/linux-tdep.c
> index dbc69a5a7fc..d89c5ae8504 100644
> --- a/gdb/linux-tdep.c
> +++ b/gdb/linux-tdep.c
> @@ -124,6 +124,7 @@ struct smaps_data
>  
>    ULONGEST rss;
>    ULONGEST swap;
> +  std::optional<int> pkey;

There's nothing in this patch series that uses the parsed pkey, so IMHO
(other maintainers may disagree) it makes more sense if this patch is
committed together with a patch that makes use of the field.

>  };
>  
>  /* Whether to take the /proc/PID/coredump_filter into account when
> @@ -1553,6 +1554,7 @@ parse_smaps_data (const file_reader_t<char> &freader)
>        int mapping_file_p;
>        ULONGEST rss = -1;
>        ULONGEST swap = -1;
> +      int pkey = -1;
>  
>        memset (&v, 0, sizeof (v));
>        struct mapping m = read_mapping (line);
> @@ -1653,6 +1655,16 @@ parse_smaps_data (const file_reader_t<char> &freader)
>  		  mapping_anon_p = 1;
>  		}
>  	    }
> +
> +	  if (streq (keyword, "ProtectionKey:"))
> +	    {
> +	      if (sscanf (line, "%*s%d", &pkey) != 1)
> +		{
> +		  warning (_("Error parsing %s's value in {s,}maps file '%s'"),
> +			   keyword, freader.c_filepath ());
> +		  break;
> +		}
> +	    }
>  	}
>        /* Save the smaps entry to the vector.  */
>  	struct smaps_data map;
> @@ -1672,6 +1684,7 @@ parse_smaps_data (const file_reader_t<char> &freader)
>  	map.inode = m.inode;
>  	map.rss = rss;
>  	map.swap = swap;
> +	map.pkey.emplace (pkey);

Shouldn't map.pkey be set only if pkey != -1?

>  
>  	smaps.emplace_back (map);
>      }

-- 
Thiago
(he/him)

Re: [PATCH v1 10/10] gdb/linux: add helpers to read AT_HWCAP3 and AT_HWCAP4

[Thiago Jung Bauermann]

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

> Add linux_get_hwcap3 and linux_get_hwcap4 helpers to GDB and gdbserver,
> mirroring the existing linux_get_hwcap and linux_get_hwcap2 interfaces.
>
> The new helpers retrieve the AT_HWCAP3 and AT_HWCAP4 auxiliary vector
> entry either from explicitly supplied auxv data or from the current
> inferior.
>
> This prepares for future features that depend on HWCAP3 and HWCAP4
> capability bits.
> ---
>  gdb/linux-tdep.c       | 38 ++++++++++++++++++++++++++++++++++++++
>  gdb/linux-tdep.h       | 22 ++++++++++++++++++++++
>  gdbserver/linux-low.cc | 28 ++++++++++++++++++++++++++++
>  gdbserver/linux-low.h  |  8 ++++++++
>  4 files changed, 96 insertions(+)

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

Like in the previous patch, there's nothing in this patch series that
uses these helpers, so IMHO (other maintainers may disagree) it makes
more sense if this patch is committed together with a patch that makes
use of them.

-- 
Thiago
(he/him)

Re: [PATCH v1 01/10] gdb/linux-tdep: change linux_fill_prpsinfo to return bool

[Simon Marchi]

On 2026-07-09 02:26, Thiago Jung Bauermann wrote:
> Hello Matthieu,
> 
> Thank you for these patches.
> 
> Matthieu Longo <matthieu.longo@arm.com> writes:
> 
>> Change linux_fill_prpsinfo() to return a boolean instead of an integer, since
>> it only reports success or failure.
>> Replace the returned integer values 1 and 0 with true and false respectively.
>> ---
>>  gdb/linux-tdep.c | 22 +++++++++++-----------
>>  1 file changed, 11 insertions(+), 11 deletions(-)
> 
> In his review of an analogous patch, Simon suggested that this kind of
> change could be pushed as obvious:
> 
> https://inbox.sourceware.org/gdb-patches/53a86cae-2927-49ec-83a2-22ff6526c0e6@simark.ca/
> 
> I would agree, but I don't know if that's a generally accepted view.

Yeah, you still need to be careful, I did manage once to mess up and
change a 0 into true or vice versa.

Also, try to change comments that use 0/1, or "zero"/"non-zero" so they
say true/false.  I see you did it for comments inside the function, but
the comment above the function also has thing that would need to be
updated.

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

With the comment updated:

Approved-By: Simon Marchi <simon.marchi@efficios.com>

Simon

Re: [PATCH v1 02/10] gdb: rely on the first alive thread TPID when reading Linux procfs files

[Simon Marchi]

On 2026-07-07 11:48, Matthieu Longo wrote:
> On Linux, /proc/<pid> is keyed by the thread-group leader PID. When
> the leader has exited, some /proc/<pid>/... entries become unavailable
> even though another thread is still alive. This can happen, for instance,
> when the main thread calls pthread_exit() and another thread continues
> the execution (existing test: gcore-stale-thread).
> 
> This causes GDB to fail to read procfs entries such as cmdline, cwd,
> exe, maps, and smaps when it uses 'current_inferior ()->pid' after the
> thread-group leader has exited.
> 
> Fix this by adding inferior::first_alive_thread(), which returns the
> PTID of the first non-exited thread of the inferior. Use its LWP ID
> when accessing procfs entries that only need a representative live LWP
> belonging to the process.
> 
> Update linux_info_proc, linux_process_address_in_memtag_page, and
> linux_find_memory_regions_full to use this live-thread LWP ID instead
> of the inferior PID when constructing procfs paths.

First comment, can we have a test for this?  I think it would be
straightforward to write.

This is an improvement, but I can still imagine some cases it would
fail.  A thread could have exited, gdb (or gdbserver) could have reaped
it status already, but the information might not have made it all the
way to the core yet.  So the "first alive thread" you select might not
actually be alive on the system.  I can't think of a way to fix that
problem generally, especially in the remote case.  If GDB checks in
advance "is this thread alive" and then tries to access is, I feel like
there will always be a TOCTOU problem.

I'm not opposed to merging a simple fix like this that takes care of the
most obvious cases (the main thread has exited, all threads are stopped,
and it obviously doesn't work).  But I think we should capture the
shortcomings we know about as comments in the code.

Then, I wondered if everything we access through /proc is going to give
the same response when we access them through another thread than the
leader.  I asked ChatGPT for a summary:

    Entry              Scope             Notes
    -----------------  ----------------  --------------------------------------
    cmdline            Process-wide      Same for all threads; comes from the
                                         shared address space (mm_struct).

    cwd                Per-thread        Usually shared, but can differ if
                                         threads use unshare(CLONE_FS).

    environ            Process-wide      Same for all threads; comes from the
                                         shared address space (mm_struct).

    exe                Process-wide      Same executable for all threads.

    maps               Process-wide      Describes the shared address space
                                         (mm_struct); same for all threads.

    status             Mixed             Contains both per-thread fields
                                         (Pid, State, SigPnd, etc.) and
                                         thread-group fields (VmSize, Threads,
                                         etc.).

    stat               Per-thread       Describes the specific task
                                         (/proc/<tid>/stat).

    smaps              Process-wide     Same mappings as maps; based on the
                                         shared address space.

    coredump_filter    Process-wide     Stored in the shared memory descriptor;
                                           same for all threads.

For most of the info it should be fine, as they are shared between all
threads.

For cwd, it's shared unless some threads call `unshared(CLONE_FS)`, I
don't know if it's common to do that.

For stat and status it looks a bit odd, because we print "process
<pid>", and then the line right below it (Process with a capital P),
which comes from /proc/<tid>/stat, gives a different number.

   (gdb) info proc stat
   process 1989928
   Process: 1991838
   Exec file: a.out
   State: t
   Parent process: 1989898
   Process group: 1989928
   Session id: 126340
   ...

In any case, we could perhaps improve the "process <pid>" line that we
print to indicate which thread we obtained the information from, in the
"auto-select a thread" case.

Finally, linux_fill_prpsinfo still uses the ptid.pid():

  pid = inferior_ptid.pid ();
  xsnprintf (filename, sizeof (filename), "/proc/%d/cmdline", (int) pid);

Should it be changed too?

And linux_address_in_shadow_stack_mem_range too?

Perhaps it would be useful to have a function "read me a whole file from
/proc" that takes an `inferior *` and returns a string, encapsulating
the logic of finding a thread to read from.

> diff --git a/gdb/inferior.h b/gdb/inferior.h
> index 9c031035a23..305b1d31830 100644
> --- a/gdb/inferior.h
> +++ b/gdb/inferior.h
> @@ -513,6 +513,15 @@ class inferior : public refcounted_object,
>    /* Find (non-exited) thread PTID of this inferior.  */
>    thread_info *find_thread (ptid_t ptid);
>  
> +  /* Return the first (non-exited) thread PTID of this inferior.
> +
> +     This method should be used in place of current_inferior ()->pid for any
> +     features relying only on the PID like the reading of procfs files.
> +     On Linux, /proc/<pid> is keyed by the thread-group leader PID. When the
> +     leader has exited, some /proc/<pid>/... entries become unavailable even
> +     though another thread is still alive.  */
> +  ptid_t first_alive_thread () const;

I think this comment is too specific to Linux and the problem at hand in
particular for this location.  It should just say "return the first
non-exited thread of the inferior" or something like that.

Function any_thread_of_inferior already does more or less what you want,
but it prefers the currently selected thread, which might not be what we
want (we perhaps want to prefer the leader).  That function could be
renamed to any_non_exited_thread_of_inferior to be clearer.

I would prefer if you renamed first_alive_thread to
first_non_exited_thread, that's the terminology we use elsewhere.
"live" makes me think of the "target_thread_alive" target function,
which actually pokes the target to see if the thread is alive right now,
that's different.

Simon

Re: [PATCH v1 04/10] gdb support: add gdb::replace algorithm for iterators and ranges

[Kevin Buettner]

Hi Matthieu,

I am not a C++ expert, but I'd like to understand this better...

On Tue, 7 Jul 2026 16:48:54 +0100
Matthieu Longo <matthieu.longo@arm.com> wrote:

> Add a gdb::replace helper that mirrors the behavior of std::replace
> for iterator pairs, together with a convenience overload accepting a
> range.
> 
> This provides a C++17-compatible replacement for the C++20 std::replace/
> std::ranges::replace algorithms, allowing callers to use a consistent
> interface until GDB transitions to C++20. The helpers should be removed
> once the C++ standard library implementations become available.
> ---
>  gdbsupport/array-view.h | 26 ++++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
> 
> diff --git a/gdbsupport/array-view.h b/gdbsupport/array-view.h
> index 8431d7f5add..61119d7a8e2 100644
> --- a/gdbsupport/array-view.h
> +++ b/gdbsupport/array-view.h
> @@ -225,6 +225,32 @@ void copy (gdb::array_view<U> src, gdb::array_view<T> dest)
>      std::copy_backward (src.begin (), src.end (), dest.end ());
>  }
>  
> +/* Replace all occurrences of a value in the provided range.
> +
> +   Note: this helper is a reimplementation of std::replace, only available
> +   from C++20 onwards, and consequently, should be removed once we switch
> +   to C++20.  */
> +
> +template <class ForwardIt, typename T>
> +void replace (ForwardIt first, ForwardIt last,
> +	      const T &old_value, const T &new_value)
> +{
> +  for (auto it = first; it != last; ++it)
> +  {
> +    if (*it == old_value)
> +      *it = new_value;
> +  }
> +}

How does this differ from pre-C++20 std::replace?

Kevin

Re: [PATCH v1 05/10] gdb: introduce helper class file_reader_t

[Kevin Buettner]

On Tue, 7 Jul 2026 16:48:55 +0100
Matthieu Longo <matthieu.longo@arm.com> wrote:

> diff --git a/gdb/target.h b/gdb/target.h
> index 4215553033c..d6fc101f205 100644
> --- a/gdb/target.h
> +++ b/gdb/target.h
> @@ -2338,6 +2338,71 @@ 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, size_t *len = nullptr);
>  
> +/* Helper class for reading a file.  */
> +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;
> +  /* Size of the data.  */
> +  size_t 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_size is a size_t, which is unsigned.  But target_fileio_read_alloc
returns a signed value and if it fails, the return value will be -1,
right?  I think you need to handle this case somehow...

Kevin

Re: [PATCH v1 02/10] gdb: rely on the first alive thread TPID when reading Linux procfs files

[Matthieu Longo]

On 09/07/2026 07:29, Thiago Jung Bauermann wrote:
> Matthieu Longo <matthieu.longo@arm.com> writes:
> 
>> On Linux, /proc/<pid> is keyed by the thread-group leader PID. When
>> the leader has exited, some /proc/<pid>/... entries become unavailable
>> even though another thread is still alive. This can happen, for instance,
>> when the main thread calls pthread_exit() and another thread continues
>> the execution (existing test: gcore-stale-thread).
>>
>> This causes GDB to fail to read procfs entries such as cmdline, cwd,
>> exe, maps, and smaps when it uses 'current_inferior ()->pid' after the
>> thread-group leader has exited.
>>
>> Fix this by adding inferior::first_alive_thread(), which returns the
>> PTID of the first non-exited thread of the inferior. Use its LWP ID
>> when accessing procfs entries that only need a representative live LWP
>> belonging to the process.
>>
>> Update linux_info_proc, linux_process_address_in_memtag_page, and
>> linux_find_memory_regions_full to use this live-thread LWP ID instead
>> of the inferior PID when constructing procfs paths.
>>
>> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31207
>> ---
>>  gdb/inferior.c   | 12 ++++++++++++
>>  gdb/inferior.h   |  9 +++++++++
>>  gdb/linux-tdep.c | 38 ++++++++++++++++++--------------------
>>  3 files changed, 39 insertions(+), 20 deletions(-)
> 
> It's a pity that the patch originally proposed to solve this bug:
> 
> https://inbox.sourceware.org/gdb-patches/20250412201140.31510-1-dominik.b.czarnota@gmail.com/
> 
> was approved but never committed. But your version is more
> comprehensive, since it also makes the change for other procfs files.
> It also explicitly looks for a live thread.
> 
> I have a couple of comments below, but regardless:
> 
> Reviewed-by: Thiago Jung Bauermann <thiago.bauermann@linaro.org>
> 
>> diff --git a/gdb/linux-tdep.c b/gdb/linux-tdep.c
>> index 155d6e874d9..b89f4ee0717 100644
>> --- a/gdb/linux-tdep.c
>> +++ b/gdb/linux-tdep.c
>> @@ -842,9 +842,7 @@ static void
>>  linux_info_proc (struct gdbarch *gdbarch, const char *args,
>>  		 enum info_proc_what what)
>>  {
>> -  /* A long is used for pid instead of an int to avoid a loss of precision
>> -     compiler warning from the output of strtoul.  */
>> -  long pid;
>> +  ptid_t ptid;
>>    int cmdline_f = (what == IP_MINIMAL || what == IP_CMDLINE || what == IP_ALL);
>>    int cwd_f = (what == IP_MINIMAL || what == IP_CWD || what == IP_ALL);
>>    int environ_f = (what == IP_ENVIRON || what == IP_ALL);
>> @@ -859,7 +857,8 @@ linux_info_proc (struct gdbarch *gdbarch, const char *args,
>>      {
>>        char *tem;
>>  
>> -      pid = strtoul (args, &tem, 10);
>> +      auto pid = strtoul (args, &tem, 10);
> 
> The actual type of pid here will be long, right? Unless there's a an
> advantage to using auto, I think in this case the code is clearer if the
> type is explicitly mentioned.
> 

Fixed.

>> +      ptid = ptid_t (pid, pid);
>>        args = tem;
>>      }
>>    else
>> @@ -869,17 +868,17 @@ linux_info_proc (struct gdbarch *gdbarch, const char *args,
>>        if (current_inferior ()->fake_pid_p)
>>  	error (_("Can't determine the current process's PID: you must name one."));
>>  
>> -      pid = current_inferior ()->pid;
>> +      ptid = current_inferior ()->first_alive_thread ();
>>      }
>>  
>>    args = skip_spaces (args);
>>    if (args && args[0])
>>      error (_("Too many parameters: %s"), args);
>>  
>> -  gdb_printf (_("process %ld\n"), pid);
>> +  gdb_printf (_("process %d\n"), ptid.pid ());
>>    if (cmdline_f)
>>      {
>> -      xsnprintf (filename, sizeof filename, "/proc/%ld/cmdline", pid);
>> +      xsnprintf (filename, sizeof filename, "/proc/%ld/cmdline", ptid.lwp ());
>>        gdb_byte *buffer;
>>        LONGEST len = target_fileio_read_alloc (nullptr, filename, &buffer);
>>  
>> @@ -901,7 +900,7 @@ linux_info_proc (struct gdbarch *gdbarch, const char *args,
>>      }
>>    if (cwd_f)
>>      {
>> -      xsnprintf (filename, sizeof filename, "/proc/%ld/cwd", pid);
>> +      xsnprintf (filename, sizeof filename, "/proc/%ld/cwd", ptid.lwp ());
> 
> proc_pid_cwd(5) says:
> 
>   In a multithreaded process, the contents of this symbolic
>   link are not available if the main thread has already
>   terminated (typically by calling pthread_exit(3)).
> 
> I think it's ok to leave the code as is for consistency, but it's worth
> mentioning this detail in a comment.
> 

The manual seems incomplete for others threads will still have those files available until they
exit. I could not find a written proof in the manual or
https://docs.kernel.org/filesystems/proc.html, but the files are still valid in the non-exited threads.

See also the reply from Simon about this.

>>        std::optional<std::string> contents
>>  	= target_fileio_readlink (NULL, filename, &target_errno);
>>        if (contents.has_value ())
>> @@ -911,7 +910,7 @@ linux_info_proc (struct gdbarch *gdbarch, const char *args,
>>      }
>>    if (environ_f)
>>      {
>> -      xsnprintf (filename, sizeof filename, "/proc/%ld/environ", pid);
>> +      xsnprintf (filename, sizeof filename, "/proc/%ld/environ", ptid.lwp ());
>>        gdb_byte *buffer;
>>        LONGEST len = target_fileio_read_alloc (nullptr, filename, &buffer);
>>  
>> @@ -935,7 +934,7 @@ linux_info_proc (struct gdbarch *gdbarch, const char *args,
>>      }
>>    if (exe_f)
>>      {
>> -      xsnprintf (filename, sizeof filename, "/proc/%ld/exe", pid);
>> +      xsnprintf (filename, sizeof filename, "/proc/%ld/exe", ptid.lwp ());
> 
> Same comment here. proc_pid_exe(5) has the same observation.
> 

See answer above.

>>        std::optional<std::string> contents
>>  	= target_fileio_readlink (NULL, filename, &target_errno);
>>        if (contents.has_value ())
> 

Matthieu

Re: [PATCH v1 03/10] target_fileio_read_stralloc: add an optional length parameter

[Matthieu Longo]

On 09/07/2026 07:30, Thiago Jung Bauermann wrote:
> Matthieu Longo <matthieu.longo@arm.com> writes:
> 
>> diff --git a/gdb/target.h b/gdb/target.h
>> index 22653138491..4215553033c 100644
>> --- a/gdb/target.h
>> +++ b/gdb/target.h
>> @@ -2336,7 +2336,7 @@ extern LONGEST target_fileio_read_alloc (struct inferior *inf,
>>     are returned as allocated but empty strings.  A warning is issued
>>     if the result contains any 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, size_t *len = nullptr);
> 
> It's worth updating the documentation comment to mention the new parameter.
> 

See the updated diff in gdb/target.h
>>  
>>  /* Invalidate the target associated with open handles that were open
>>     on target TARG, since we're about to close (and maybe destroy) the
> 

diff --git a/gdb/target.h b/gdb/target.h
index 22653138491..0df5a654f75 100644
--- a/gdb/target.h
+++ b/gdb/target.h
@@ -2328,15 +2328,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.  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, size_t *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

Matthieu

RE: [PATCH v1 05/10] gdb: introduce helper class file_reader_t

[Schimpe, Christina]

Hi Matthieu,

Please find my feedback below.

> -----Original Message-----
> From: Matthieu Longo <matthieu.longo@arm.com>
> Sent: Dienstag, 7. Juli 2026 17:49
> To: gdb-patches@sourceware.org
> Cc: Luis Machado <luis.machado@amd.com>; Luis Machado
> <luis.machado.foss@gmail.com>; Andrew Burgess <aburgess@redhat.com>;
> Yury Khrustalev <yury.khrustalev@arm.com>; Pedro Alves
> <pedro@palves.net>; Tom Tromey <tom@tromey.com>; Matthieu Longo
> <matthieu.longo@arm.com>
> Subject: [PATCH v1 05/10] 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       | 102 +++++++++++++++++------------------------
>  gdb/sparc64-tdep.c     |  12 ++---
>  gdb/target.h           |  65 ++++++++++++++++++++++++++
>  4 files changed, 117 insertions(+), 74 deletions(-)
> 
> diff --git a/gdb/amd64-linux-tdep.c b/gdb/amd64-linux-tdep.c index
> a5ac26654cf..e7841917eaf 100644
> --- a/gdb/amd64-linux-tdep.c
> +++ b/gdb/amd64-linux-tdep.c
> @@ -1851,14 +1851,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)
> @@ -1870,7 +1867,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
> b89f4ee0717..a12a69f03a2 100644
> --- a/gdb/linux-tdep.c
> +++ b/gdb/linux-tdep.c
> @@ -1661,6 +1661,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.  */
> 
> @@ -1672,17 +1678,13 @@ linux_process_address_in_memtag_page
> (CORE_ADDR address)
> 
>    ptid_t ptid = current_inferior ()->first_alive_thread ();
> 
> -  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)
>      {
> @@ -1746,17 +1748,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;
>  	}
>      }
> @@ -2259,9 +2257,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.  */
> @@ -2285,23 +2280,24 @@ linux_fill_prpsinfo (struct
> elf_internal_linux_prpsinfo *p)
> 
>    /* Obtaining PID and filename.  */
>    pid = inferior_ptid.pid ();
> -  xsnprintf (filename, sizeof (filename), "/proc/%d/cmdline", (int) pid);
> -  /* 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/%d/cmdline", pid));  if (!cmdline_freader)
> +    return false;
> 
> -  if (buf_len < 1 || fname.get () == nullptr || fname.get ()[0] == '\0')
> +  /* The full name of the program which generated the corefile.  */
> + gdb::array_view<char> cmdline = cmdline_freader.cast_view<char> ();
> + 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;
>      }
> 
> @@ -2311,27 +2307,23 @@ linux_fill_prpsinfo (struct
> elf_internal_linux_prpsinfo *p)
>    p->pr_pid = 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/%d/stat", (int) pid);
> -  /* 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/%d/stat",
> + pid));  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 @@ -
> 2403,13 +2395,9 @@ 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/%d/status", (int) pid);
> -  /* 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/%d/status",
> + pid));  char *proc_status = status_freader.data ();  if
> + (!status_freader || *proc_status == '\0')
>      {
>        /* Returning true since we already have a bunch of information.  */
>        return true;
> @@ -2802,9 +2790,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;
> 
> @@ -2842,7 +2827,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 @@ -
> 2852,15 +2837,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)
>      {
>        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))
>  	{
> @@ -2879,7 +2863,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 ());

IIUC, before your change, if the file was empty, we did not show a warning, since
target_fileio_read_stralloc handles the case like this:
"Empty objects are returned as allocated but empty strings."

Based on your file_reader_t implementation, I think we get here in case we can
open the file but the file is empty. Not sure if in that case the file can be empty
at all, but it is a behavioural change that should be avoided in my opinion.
There are further similar cases in this patch. 
Would it make sense to change the file_reader_t bool operator to return true in case the
file is empty? Maybe we could then additionally introduce a new method bool empty ().

>    return false;
>  }
> @@ -3168,16 +3153,11 @@ linux_address_in_shadow_stack_mem_range
> 
>    const int pid = current_inferior ()->pid;
> 
> -  std::string smaps_file = string_printf ("/proc/%d/smaps", pid);
> -
> -  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/%d/smaps",
> + pid));  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..811b76f27d8 100644
> --- a/gdb/sparc64-tdep.c
> +++ b/gdb/sparc64-tdep.c
> @@ -305,14 +305,13 @@ adi_is_addr_mapped (CORE_ADDR vaddr, size_t
> cnt)
>    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)
>      {
>        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 +328,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 4215553033c..d6fc101f205
> 100644
> --- a/gdb/target.h
> +++ b/gdb/target.h
> @@ -2338,6 +2338,71 @@ 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, size_t *len = nullptr);
> 
> +/* Helper class for reading a file.  */ 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;
> +  /* Size of the data.  */
> +  size_t 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));
> +      }
> +  }
> +
> +  /* Return true if the file was read successfully.  */  operator bool
> + () const noexcept  { return m_data != nullptr && m_size > 0; }

I think we should make this explicit.

> +
> +  /* Return a pointer to the data.  */
> +  T *data () const noexcept
> +  { return m_data.get (); }
> +
> +  /* Return the size of the data.  */
> +  size_t 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, cast to a different type.  */
> +  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 filepath of the file that was read.  */  const
> + std::string &filepath () const noexcept  { return m_filepath; }
> +
> +  /* Return the filepath 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
> 

Regards,
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 04/10] gdb support: add gdb::replace algorithm for iterators and ranges

[Matthieu Longo]

On 10/07/2026 22:21, Kevin Buettner wrote:
> Hi Matthieu,
> 
> I am not a C++ expert, but I'd like to understand this better...
> 
> On Tue, 7 Jul 2026 16:48:54 +0100
> Matthieu Longo <matthieu.longo@arm.com> wrote:
> 
>> Add a gdb::replace helper that mirrors the behavior of std::replace
>> for iterator pairs, together with a convenience overload accepting a
>> range.
>>
>> This provides a C++17-compatible replacement for the C++20 std::replace/
>> std::ranges::replace algorithms, allowing callers to use a consistent
>> interface until GDB transitions to C++20. The helpers should be removed
>> once the C++ standard library implementations become available.
>> ---
>>  gdbsupport/array-view.h | 26 ++++++++++++++++++++++++++
>>  1 file changed, 26 insertions(+)
>>
>> diff --git a/gdbsupport/array-view.h b/gdbsupport/array-view.h
>> index 8431d7f5add..61119d7a8e2 100644
>> --- a/gdbsupport/array-view.h
>> +++ b/gdbsupport/array-view.h
>> @@ -225,6 +225,32 @@ void copy (gdb::array_view<U> src, gdb::array_view<T> dest)
>>      std::copy_backward (src.begin (), src.end (), dest.end ());
>>  }
>>  
>> +/* Replace all occurrences of a value in the provided range.
>> +
>> +   Note: this helper is a reimplementation of std::replace, only available
>> +   from C++20 onwards, and consequently, should be removed once we switch
>> +   to C++20.  */
>> +
>> +template <class ForwardIt, typename T>
>> +void replace (ForwardIt first, ForwardIt last,
>> +	      const T &old_value, const T &new_value)
>> +{
>> +  for (auto it = first; it != last; ++it)
>> +  {
>> +    if (*it == old_value)
>> +      *it = new_value;
>> +  }
>> +}
> 
> How does this differ from pre-C++20 std::replace?
> 
> Kevin
> 

I clearly misread the doc => https://en.cppreference.com/cpp/algorithm/replace

template< class ForwardIt, class T >
void replace( ForwardIt first, ForwardIt last,
              const T& old_value, const T& new_value );

	(1) 	(until C++26)
                (constexpr since C++20)

It was not added in C++20 but became constexpr since C++20.
Since the range version was added in C++20, I must have mixed it up in my head.

I removed this duplicate from the patch, and made the range version use std::replace instead.

commit 8854b859aec1a3cbcd3edeb571dc1388ac5b81f7
Author: Matthieu Longo <matthieu.longo@arm.com>
Date:   Fri Jul 3 16:02:13 2026 +0100

    gdb support: add gdb::ranges::replace algorithm

    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

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


Matthieu.

Re: [PATCH v1 05/10] gdb: introduce helper class file_reader_t

[Matthieu Longo]

On 10/07/2026 22:43, Kevin Buettner wrote:
> On Tue, 7 Jul 2026 16:48:55 +0100
> Matthieu Longo <matthieu.longo@arm.com> wrote:
> 
>> diff --git a/gdb/target.h b/gdb/target.h
>> index 4215553033c..d6fc101f205 100644
>> --- a/gdb/target.h
>> +++ b/gdb/target.h
>> @@ -2338,6 +2338,71 @@ 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, size_t *len = nullptr);
>>  
>> +/* Helper class for reading a file.  */
>> +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;
>> +  /* Size of the data.  */
>> +  size_t 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_size is a size_t, which is unsigned.  But target_fileio_read_alloc
> returns a signed value and if it fails, the return value will be -1,
> right?  I think you need to handle this case somehow...
> 
> Kevin
> 

Given that you raise this question, it makes me question whether the semantic of
target_fileio_read_stralloc is not misleading.

target_fileio_read_stralloc always returns a positive value.
A size of 0 is set when an issue occurs.
An error can be detected when checking the returned value for nullptr. Setting the size to -1 is
superfluous.
Hence this method in file_reader_t to check whether the object is valid.

  /* Return true if the file was read successfully.  */
  operator bool () const noexcept
  { return m_data != nullptr && m_size > 0; }

Note that an empty file would means that m_data is not null, but its size would be 0.
I decided to treat both of them in the same way because we cannot do much with an empty procfs file.

Matthieu

Re: [PATCH v1 05/10] gdb: introduce helper class file_reader_t

[Matthieu Longo]

On 13/07/2026 16:50, Schimpe, Christina wrote:
>> @@ -2802,9 +2790,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;
>>
>> @@ -2842,7 +2827,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 @@ -
>> 2852,15 +2837,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)
>>      {
>>        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))
>>  	{
>> @@ -2879,7 +2863,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 ());
> 
> IIUC, before your change, if the file was empty, we did not show a warning, since
> target_fileio_read_stralloc handles the case like this:
> "Empty objects are returned as allocated but empty strings."
> 
> Based on your file_reader_t implementation, I think we get here in case we can
> open the file but the file is empty. Not sure if in that case the file can be empty
> at all, but it is a behavioural change that should be avoided in my opinion.
> There are further similar cases in this patch.

An empty file is normally fine, but in most of cases, pretty useless hence the proposed change in
behavior.

However, I can see a reason when a file is empty and a process still alive.
From man 5 proc_pid_cmdline:
  > This  read-only  file  holds the complete command line for the process, unless the
  > process is a zombie.  In the latter case, there is nothing in this file: that is,
  > a read on this file will return 0 characters.
  > ...
  > If, after an execve(2), the process modifies its argv strings, those changes will
  > show up here.
  > ...
  > Furthermore, a process may change the memory location that this file refers via
  > prctl(2) operations such as PR_SET_MM_ARG_START.

The last sentence suggests that an empty file is still valid, and can have a different cause than a
deceased process. There might be others similar cases.

I will change the behavior back to what it was for empty files.

> Would it make sense to change the file_reader_t bool operator to return true in case the
> file is empty? Maybe we could then additionally introduce a new method bool empty ().
>

Yes, it would make sense to add a new method empty() and even error ().
I will also change the behavior of target_fileio_read_stralloc in case of errors (see comment from
Kevin Buettner).

bool empty () const
{ m_data != nullptr && m_size > 0; }

bool error () const
{ m_data == nullptr && m_size == -1; }

And bool operator() could be the combination of both:

bool operator () const
{ return !(error () ||empty ()); }

>> +/* Helper class for reading a file.  */ 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;
>> +  /* Size of the data.  */
>> +  size_t 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));
>> +      }
>> +  }
>> +
>> +  /* Return true if the file was read successfully.  */  operator bool
>> + () const noexcept  { return m_data != nullptr && m_size > 0; }
> 
> I think we should make this explicit.
> 

However, making the boolean operator explicit would make it very inconvenient.
I would prefer to avoid this extra verbose:

if (!static_cast<bool>(freader))
  {
    // handle the error or empty
  }

Matthieu

Re: [PATCH v1 05/10] gdb: introduce helper class file_reader_t

[Matthieu Longo]

On 09/07/2026 07:33, Thiago Jung Bauermann wrote:
> 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       | 102 +++++++++++++++++------------------------
>>  gdb/sparc64-tdep.c     |  12 ++---
>>  gdb/target.h           |  65 ++++++++++++++++++++++++++
>>  4 files changed, 117 insertions(+), 74 deletions(-)
> 
> With the changes I suggested below:
> 
> Reviewed-by: Thiago Jung Bauermann <thiago.bauermann@linaro.org>
> 
>> @@ -2285,23 +2280,24 @@ linux_fill_prpsinfo (struct elf_internal_linux_prpsinfo *p)
>>  
>>    /* Obtaining PID and filename.  */
>>    pid = inferior_ptid.pid ();
>> -  xsnprintf (filename, sizeof (filename), "/proc/%d/cmdline", (int) pid);
>> -  /* 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/%d/cmdline", pid));
> 
> I was scratching my head for a bit trying to understand why not use
> file_reader_t<char> here and avoid having to use cast_view<char>
> below. I think it's worth a comment mentioning that it's because cmdline
> has '\0' separating the arguments.
> 

Added comments for the next revision.

>> +  if (!cmdline_freader)
>> +    return false;
>>  
>> -  if (buf_len < 1 || fname.get () == nullptr || fname.get ()[0] == '\0')
>> +  /* The full name of the program which generated the corefile.  */
>> +  gdb::array_view<char> cmdline = cmdline_freader.cast_view<char> ();
>> +  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;
>>      }
>>  
> ⋮
>> diff --git a/gdb/sparc64-tdep.c b/gdb/sparc64-tdep.c
>> index 93db3417a2a..811b76f27d8 100644
>> --- a/gdb/sparc64-tdep.c
>> +++ b/gdb/sparc64-tdep.c
>> @@ -305,14 +305,13 @@ adi_is_addr_mapped (CORE_ADDR vaddr, size_t cnt)
>>    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);
> 
> This change leaves filename unused and I get a build error when using
> "configure --enable-targets=all":

Thanks I didn't build with this flags. I will from now on.

> 
>   CXX    sparc64-tdep.o
> /home/bauermann/src/binutils-gdb-wt-2/gdb/sparc64-tdep.c: In function 'bool adi_is_addr_mapped(CORE_ADDR, size_t)':
> /home/bauermann/src/binutils-gdb-wt-2/gdb/sparc64-tdep.c:304:8: error: unused variable 'filename' [-Werror=unused-variable]
>   304 |   char filename[MAX_PROC_NAME_SIZE];
>       |        ^~~~~~~~
> cc1plus: all warnings being treated as errors
> make[2]: *** [Makefile:2100: sparc64-tdep.o] Error 1
> 

Fixed.

>> -  if (data)
>> +  file_reader_t<char> adi_maps_freader
>> +    (string_printf ("/proc/%d/adi/maps", pid));
> 
> I'll just note that this patch changes the printf pattern from "%ld" and
> "(long) pid" to "%d" and "pid". I think the change is for the better and
> I can't think of why it was done the other way before.
> 
>> +  if (adi_maps_freader)
>>      {
>>        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))
>>  	{
> 

Thanks for the review.

Matthieu

Re: [PATCH v1 06/10] gdb/linux-tdep: migrate linux_info_proc to file_reader_t

[Matthieu Longo]

On 09/07/2026 07:35, Thiago Jung Bauermann wrote:
> Matthieu Longo <matthieu.longo@arm.com> writes:
> 
>> The patch migratse the code of linux_info_proc to use file_reader_t to
> 
> Typo: migrates
> 

Fixed.

>> 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 | 120 +++++++++++++++++++++++++++--------------------
>>  1 file changed, 69 insertions(+), 51 deletions(-)
> 
> Nice improvement, thanks!
> 
> Just a couple of nits, but:
> 
> Reviewed-by: Thiago Jung Bauermann <thiago.bauermann@linaro.org>
> 
>> diff --git a/gdb/linux-tdep.c b/gdb/linux-tdep.c
>> index a12a69f03a2..a2af1586d35 100644
>> --- a/gdb/linux-tdep.c
>> +++ b/gdb/linux-tdep.c
>> @@ -836,6 +836,27 @@ dump_note_entry_p (filter_flags filterflags, const smaps_data &map)
>>    return true;
>>  }
>>  
>> +/* In a character buffer where entries are separated by a SEPARATOR character,
>> +   extract the string view starting at START.
>> +   Return the extracted view and the iterator to the next entry.  */
>> +
>> +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')
>> +{
>> +  if (start < buffer.begin () || start >= buffer.end ())
>> +    return std::make_pair (gdb::array_view<char> (), buffer.end ());
>> +
>> +  auto it = std::find (start, buffer.end (), separator);
>> +  if (it == buffer.end ())
>> +    return std::make_pair (gdb::array_view<char> (), buffer.end ());
>> +
>> +  auto next_start = std::next (it);
>> +  return std::make_pair
>> +    (gdb::array_view<char> (start, next_start), next_start);
> 
> This line fits 80 columns and doesn't need to be broken.
> 

Fixed.

>> +}
>> +
>>  /* Implement the "info proc" command.  */
>>  
>>  static void
>> @@ -878,25 +899,20 @@ linux_info_proc (struct gdbarch *gdbarch, const char *args,
>>    gdb_printf (_("process %d\n"), ptid.pid ());
>>    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);
>> +	  gdb::array_view<char> cmdline = cmdline_freader.cast_view<char> ();
>> +	  gdb_assert (cmdline[ cmdline.size () - 1] == '\0');
> 
> Extraneous space after '['.
> 

Fixed.

>> +	  /* Replace null characters splitting the arguments in the command
>> +	     line by spaces, except for the last one.  */
>> +	  gdb::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)
>>      {
> 

Matthieu

Re: [PATCH v1 07/10] gdb/linux-tdep: migrate linux_find_memory_regions_full to file_reader_t

[Matthieu Longo]

On 09/07/2026 07:36, Thiago Jung Bauermann wrote:
> 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 codedump generation via
>> gcore command (see gcore-stale-thread test for more details).
> 
> Did you run into this problem, or just noticed it by inspecting the
> code? If the former, do you think it's worth adding a testcase to make
> sure this problem doesn't appear again?
> 

The command stopped working after I migrated the function to file_reader_t.
As Christina Schimpe mentioned it, bool operator () now returns false if the buffer is null, or if
the size returned is 0. This last is a change with the previous behavior.
When the thread-group leader exits, the /proc/pid/[s]maps files still exist, but become empty.
In such a case, the function should return false instead of trying to parse an empty string, which
results in no smaps data entry being processed, and so no callback called. This behavior is
erroneous in my opinion.

I don't think that it is worth adding a testcase for this, because the root cause was the wrong pid
being used.
>> 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 is reading any of the procfs files fails.
> 
> Typo: is → if
> 

Fixed.

>> ---
>>  gdb/linux-tdep.c | 23 ++++++++++-------------
>>  1 file changed, 10 insertions(+), 13 deletions(-)
> 
> Reviewed-by: Thiago Jung Bauermann <thiago.bauermann@linaro.org>
> 

Matthieu

Re: [PATCH v1 08/10] gdb/linux-tdep: migrate parse_smaps_data to file_reader_t

[Matthieu Longo]

On 09/07/2026 07:41, Thiago Jung Bauermann wrote:
> Matthieu Longo <matthieu.longo@arm.com> writes:
> 
> Suggestion: mention in the commit log that since the previous commit,
> the old variant of parse_smaps_data is unused.
> 
What about :

gdb/linux-tdep: remove legacy parse_smaps_data overload

    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.

Matthieu

Re: [PATCH v1 09/10] gdb/linux-tdep: parse ProtectionKey in /proc/PID/smaps

[Matthieu Longo]

On 09/07/2026 07:42, Thiago Jung Bauermann wrote:
> Matthieu Longo <matthieu.longo@arm.com> writes:
> 
>> Memory Protection Keys provide a mechanism for enforcing page-based
>> protections without requiring modification of the page tables
>> when an application changes protection domains. [1]
>>
>> The "ProtectionKey" field may be present in /proc/PID/smaps x86_64
>> and AArch64 systems since Linux 4.9, when the kernel is built with
>> Memory Protection Keys support.
> 
> /me from a previous life would feel the need to point out that it may
> also be present on POWER systems. :)
> 

Added PowerPC to the list.

  The "ProtectionKey" field may be present in /proc/PID/smaps x86_64,
  PowerPC, and AArch64 systems since Linux 4.9, when the kernel is
  built with Memory Protection Keys support.

>> Add a new 'pkey' field to `struct smaps_data', and populate it when
>> the "ProtectionKey" field is present.
>>
>> This prepares for displaying the protection key in 'info proc mappings'.
>>
>> [1]: https://docs.kernel.org/core-api/protection-keys.html,
>>      https://lkml.iu.edu/1512.0/03058.html
> 
> I suggest linking to the commit that went upstream instead of the
> mailing list patch:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=c1192f842841
> 

Fixed.

>> ---
>>  gdb/linux-tdep.c | 13 +++++++++++++
>>  1 file changed, 13 insertions(+)
>>
>> diff --git a/gdb/linux-tdep.c b/gdb/linux-tdep.c
>> index dbc69a5a7fc..d89c5ae8504 100644
>> --- a/gdb/linux-tdep.c
>> +++ b/gdb/linux-tdep.c
>> @@ -124,6 +124,7 @@ struct smaps_data
>>  
>>    ULONGEST rss;
>>    ULONGEST swap;
>> +  std::optional<int> pkey;
> 
> There's nothing in this patch series that uses the parsed pkey, so IMHO
> (other maintainers may disagree) it makes more sense if this patch is
> committed together with a patch that makes use of the field.
> 

Nothing uses it yet because it is still unclear how to expose it to the users.
I assume that "info proc mappings" is a good place to do it.

As a reminder those are the existing columns:
Start Addr         End Addr           Size               Offset             Perms File

Should we add a new column "Protection Key" or "PKey" between "Offset" and "Perms" ?
What do you think ?

>>  };
>>  
>>  /* Whether to take the /proc/PID/coredump_filter into account when
>> @@ -1553,6 +1554,7 @@ parse_smaps_data (const file_reader_t<char> &freader)
>>        int mapping_file_p;
>>        ULONGEST rss = -1;
>>        ULONGEST swap = -1;
>> +      int pkey = -1;
>>  
>>        memset (&v, 0, sizeof (v));
>>        struct mapping m = read_mapping (line);
>> @@ -1653,6 +1655,16 @@ parse_smaps_data (const file_reader_t<char> &freader)
>>  		  mapping_anon_p = 1;
>>  		}
>>  	    }
>> +
>> +	  if (streq (keyword, "ProtectionKey:"))
>> +	    {
>> +	      if (sscanf (line, "%*s%d", &pkey) != 1)
>> +		{
>> +		  warning (_("Error parsing %s's value in {s,}maps file '%s'"),
>> +			   keyword, freader.c_filepath ());
>> +		  break;
>> +		}
>> +	    }
>>  	}
>>        /* Save the smaps entry to the vector.  */
>>  	struct smaps_data map;
>> @@ -1672,6 +1684,7 @@ parse_smaps_data (const file_reader_t<char> &freader)
>>  	map.inode = m.inode;
>>  	map.rss = rss;
>>  	map.swap = swap;
>> +	map.pkey.emplace (pkey);
> 
> Shouldn't map.pkey be set only if pkey != -1?
> 

Agree. Fixed.

>>  
>>  	smaps.emplace_back (map);
>>      }
> 

Matthieu

Re: [PATCH v1 09/10] gdb/linux-tdep: parse ProtectionKey in /proc/PID/smaps

[Matthieu Longo]

On 14/07/2026 10:12, Matthieu Longo wrote:
> On 09/07/2026 07:42, Thiago Jung Bauermann wrote:
>>
>> There's nothing in this patch series that uses the parsed pkey, so IMHO
>> (other maintainers may disagree) it makes more sense if this patch is
>> committed together with a patch that makes use of the field.
>>
> 
> Nothing uses it yet because it is still unclear how to expose it to the users.
> I assume that "info proc mappings" is a good place to do it.
> 
> As a reminder those are the existing columns:
> Start Addr         End Addr           Size               Offset             Perms File
> 
> Should we add a new column "Protection Key" or "PKey" between "Offset" and "Perms" ?
> What do you think ?
> 

Additionally, if we were to print the permissions associated with a PKey, what do you think about
the following view ?

Start Addr         End Addr           Size               Offset    PKey     Perms File
0x0000000000400000 0x0000000000483000 0x83000            0x0       0        r-xp  /path/to/file
    ...
0x00000000004a2000 0x00000000004a7000 0x5000             0x0       1        rw-p
    Thread 1: effective: r--  overlay: r--
    Thread 3: effective: rw-  overlay: rw-
    Thread 4: effective: r--  overlay: r-x

Effective being the effective permissions resulting from the ANDing of the base permissions (coming
from "Perms") AND the overlay permissions attached to a thread (permissions associated to the PKey.
The look-up of those overlay permissions is platform-specific).

If no protection key support exists on the target, the PKey column would not be printed.
Same for the threads' permissions (effective and overlay).

Does this approach look fine to you ? Does it overload the view ?
Should the dumping of effective and overlay permissions be part of a generic or platform-specific
command ?

Matthieu

Re: [PATCH v1 02/10] gdb: rely on the first alive thread TPID when reading Linux procfs files

[Matthieu Longo]

On 09/07/2026 15:24, Simon Marchi wrote:
> 
> 
> On 2026-07-07 11:48, Matthieu Longo wrote:
>> On Linux, /proc/<pid> is keyed by the thread-group leader PID. When
>> the leader has exited, some /proc/<pid>/... entries become unavailable
>> even though another thread is still alive. This can happen, for instance,
>> when the main thread calls pthread_exit() and another thread continues
>> the execution (existing test: gcore-stale-thread).
>>
>> This causes GDB to fail to read procfs entries such as cmdline, cwd,
>> exe, maps, and smaps when it uses 'current_inferior ()->pid' after the
>> thread-group leader has exited.
>>
>> Fix this by adding inferior::first_alive_thread(), which returns the
>> PTID of the first non-exited thread of the inferior. Use its LWP ID
>> when accessing procfs entries that only need a representative live LWP
>> belonging to the process.
>>
>> Update linux_info_proc, linux_process_address_in_memtag_page, and
>> linux_find_memory_regions_full to use this live-thread LWP ID instead
>> of the inferior PID when constructing procfs paths.
> 
> First comment, can we have a test for this?  I think it would be
> straightforward to write.
> 

Fixed in the next revision.

> This is an improvement, but I can still imagine some cases it would
> fail.  A thread could have exited, gdb (or gdbserver) could have reaped
> it status already, but the information might not have made it all the
> way to the core yet.  So the "first alive thread" you select might not
> actually be alive on the system.  I can't think of a way to fix that
> problem generally, especially in the remote case.  If GDB checks in
> advance "is this thread alive" and then tries to access is, I feel like
> there will always be a TOCTOU problem.
> 
> I'm not opposed to merging a simple fix like this that takes care of the
> most obvious cases (the main thread has exited, all threads are stopped,
> and it obviously doesn't work).  But I think we should capture the
> shortcomings we know about as comments in the code.
> 

I proposed this comment:

  /* Return the first non-exited thread of this inferior.

     This is only a best-effort choice of a thread that is expected to still
     exist in the target.  A thread may have exited after GDB last updated its
     thread list, or GDB/gdbserver may have observed the exit but not yet
     propagated it through all layers.  Therefore the returned thread is not
     guaranteed to still be alive when it is later accessed.  This avoids the
     common case where the current thread has exited, but callers must still be
     prepared for the selected thread to no longer exist.  */
  ptid_t first_non_exited_thread () const;


> Then, I wondered if everything we access through /proc is going to give
> the same response when we access them through another thread than the
> leader.  I asked ChatGPT for a summary:
> 
>     Entry              Scope             Notes
>     -----------------  ----------------  --------------------------------------
>     cmdline            Process-wide      Same for all threads; comes from the
>                                          shared address space (mm_struct).
> 
>     cwd                Per-thread        Usually shared, but can differ if
>                                          threads use unshare(CLONE_FS).
> 
>     environ            Process-wide      Same for all threads; comes from the
>                                          shared address space (mm_struct).
> 
>     exe                Process-wide      Same executable for all threads.
> 
>     maps               Process-wide      Describes the shared address space
>                                          (mm_struct); same for all threads.
> 
>     status             Mixed             Contains both per-thread fields
>                                          (Pid, State, SigPnd, etc.) and
>                                          thread-group fields (VmSize, Threads,
>                                          etc.).
> 
>     stat               Per-thread       Describes the specific task
>                                          (/proc/<tid>/stat).
> 
>     smaps              Process-wide     Same mappings as maps; based on the
>                                          shared address space.
> 
>     coredump_filter    Process-wide     Stored in the shared memory descriptor;
>                                            same for all threads.
> 
> For most of the info it should be fine, as they are shared between all
> threads.
> 
> For cwd, it's shared unless some threads call `unshared(CLONE_FS)`, I
> don't know if it's common to do that.
> 
> For stat and status it looks a bit odd, because we print "process
> <pid>", and then the line right below it (Process with a capital P),
> which comes from /proc/<tid>/stat, gives a different number.
> 
>    (gdb) info proc stat
>    process 1989928
>    Process: 1991838
>    Exec file: a.out
>    State: t
>    Parent process: 1989898
>    Process group: 1989928
>    Session id: 126340
>    ...
> 
> In any case, we could perhaps improve the "process <pid>" line that we
> print to indicate which thread we obtained the information from, in the
> "auto-select a thread" case.
> 

Indeed it might be confusing, but still better than nothing at all.
I propose the following change to the line printing the process ID.

diff --git a/gdb/linux-tdep.c b/gdb/linux-tdep.c
index 86532e1e0d3..b11473fd22d 100644
--- a/gdb/linux-tdep.c
+++ b/gdb/linux-tdep.c
@@ -875,7 +875,14 @@ linux_info_proc (struct gdbarch *gdbarch, const char *args,
   if (args && args[0])
     error (_("Too many parameters: %s"), args);

-  gdb_printf (_("process %d\n"), ptid.pid ());
+  thread_info *thr = inferior_thread ();
+  if (thr->ptid == ptid)
+    gdb_printf (_("process %d\n"), ptid.pid ());
+  else
+    gdb_printf (_("process %d [Note: information where gathered from LWP %ld " \
+                 "as the thread-group leader (LWP=%ld) already exited.]\n"),
+               thr->ptid.pid (), ptid.lwp (), thr->ptid.lwp ());
+
   if (cmdline_f)
     {
       xsnprintf (filename, sizeof filename, "/proc/%ld/cmdline", ptid.lwp ());

Output before the thread-group leader exists:
  process 814

Output for any info proc commands after the thread-group leader exited:
  process 814 [Note: information where gathered from LWP 816 as the thread-group leader (LWP=814)
already exited.]

> Finally, linux_fill_prpsinfo still uses the ptid.pid():
> 
>   pid = inferior_ptid.pid ();
>   xsnprintf (filename, sizeof (filename), "/proc/%d/cmdline", (int) pid);
> 
> Should it be changed too?
> 

Yes, it should. Fixed in the next revision.

> And linux_address_in_shadow_stack_mem_range too?
> 

Fixed in the revision.

> Perhaps it would be useful to have a function "read me a whole file from
> /proc" that takes an `inferior *` and returns a string, encapsulating
> the logic of finding a thread to read from.
> 

Please have a look at patch 5/10. There might be something to do there.
For now, I will try to keep the change to the minimum needed.

>> diff --git a/gdb/inferior.h b/gdb/inferior.h
>> index 9c031035a23..305b1d31830 100644
>> --- a/gdb/inferior.h
>> +++ b/gdb/inferior.h
>> @@ -513,6 +513,15 @@ class inferior : public refcounted_object,
>>    /* Find (non-exited) thread PTID of this inferior.  */
>>    thread_info *find_thread (ptid_t ptid);
>>  
>> +  /* Return the first (non-exited) thread PTID of this inferior.
>> +
>> +     This method should be used in place of current_inferior ()->pid for any
>> +     features relying only on the PID like the reading of procfs files.
>> +     On Linux, /proc/<pid> is keyed by the thread-group leader PID. When the
>> +     leader has exited, some /proc/<pid>/... entries become unavailable even
>> +     though another thread is still alive.  */
>> +  ptid_t first_alive_thread () const;
> 
> I think this comment is too specific to Linux and the problem at hand in
> particular for this location.  It should just say "return the first
> non-exited thread of the inferior" or something like that.
> 

See previous comment above.

> Function any_thread_of_inferior already does more or less what you want,
> but it prefers the currently selected thread, which might not be what we
> want (we perhaps want to prefer the leader).  That function could be
> renamed to any_non_exited_thread_of_inferior to be clearer.
> 

Fixed in the next revision.

> I would prefer if you renamed first_alive_thread to
> first_non_exited_thread, that's the terminology we use elsewhere.

Fixed in the next revision.

> "live" makes me think of the "target_thread_alive" target function,
> which actually pokes the target to see if the thread is alive right now,
> that's different.
> 
> Simon

Matthieu

Re: [PATCH v1 03/10] target_fileio_read_stralloc: add an optional length parameter

[Luis]

Empty commit message...

On 07/07/2026 16:48, Matthieu Longo wrote:
> ---
>   gdb/target.c | 10 +++++++---
>   gdb/target.h |  2 +-
>   2 files changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/gdb/target.c b/gdb/target.c
> index 5d937f3ae85..e4907ca815a 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,
> +			     size_t *len)
>   {
>     gdb_byte *buffer;
>     char *bufstr;
> @@ -3556,17 +3557,20 @@ target_fileio_read_stralloc (struct inferior *inf, const char *filename)
>     transferred = target_fileio_read_alloc_1 (inf, filename, &buffer, 1);
>     bufstr = (char *) buffer;
>   
> +  if (len != nullptr)
> +    *len = (transferred < 0 ? 0 : transferred);
> +
>     if (transferred < 0)
>       return gdb::unique_xmalloc_ptr<char> (nullptr);
>   
>     if (transferred == 0)
>       return make_unique_xstrdup ("");
>   
> -  bufstr[transferred] = 0;
> +  bufstr[transferred] = '\0';

Why 0 -> \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')

Likewise, why 0 -> \0?

>         {
>   	warning (_("target file %s "
>   		   "contained unexpected null characters"),
> diff --git a/gdb/target.h b/gdb/target.h
> index 22653138491..4215553033c 100644
> --- a/gdb/target.h
> +++ b/gdb/target.h
> @@ -2336,7 +2336,7 @@ extern LONGEST target_fileio_read_alloc (struct inferior *inf,
>      are returned as allocated but empty strings.  A warning is issued
>      if the result contains any 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, size_t *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

Re: [PATCH v1 04/10] gdb support: add gdb::replace algorithm for iterators and ranges

[Luis]

Drive-by review, and C++20 issues aside.

On 07/07/2026 16:48, Matthieu Longo wrote:
> Add a gdb::replace helper that mirrors the behavior of std::replace
> for iterator pairs, together with a convenience overload accepting a
> range.
> 
> This provides a C++17-compatible replacement for the C++20 std::replace/
> std::ranges::replace algorithms, allowing callers to use a consistent
> interface until GDB transitions to C++20. The helpers should be removed
> once the C++ standard library implementations become available.
> ---
>   gdbsupport/array-view.h | 26 ++++++++++++++++++++++++++
>   1 file changed, 26 insertions(+)
> 
> diff --git a/gdbsupport/array-view.h b/gdbsupport/array-view.h
> index 8431d7f5add..61119d7a8e2 100644
> --- a/gdbsupport/array-view.h
> +++ b/gdbsupport/array-view.h
> @@ -225,6 +225,32 @@ void copy (gdb::array_view<U> src, gdb::array_view<T> dest)
>       std::copy_backward (src.begin (), src.end (), dest.end ());
>   }
>   
> +/* Replace all occurrences of a value in the provided range.
> +
> +   Note: this helper is a reimplementation of std::replace, only available
> +   from C++20 onwards, and consequently, should be removed once we switch
> +   to C++20.  */
> +
> +template <class ForwardIt, typename T>
> +void replace (ForwardIt first, ForwardIt last,
> +	      const T &old_value, const T &new_value)
> +{
> +  for (auto it = first; it != last; ++it)
> +  {
> +    if (*it == old_value)
> +      *it = new_value;
> +  }

Formatting: Identation of the braces is off.

> +}
> +
> +/* Replace all occurrences of a value in the provided array view.
> +   Note: from C++20 onwards, std::ranges::replace should be used instead.  */
> +
> +template <class Range, typename T>
> +void replace (Range r, const T &old_value, const T &new_value)
> +{
> +  replace (r.begin (), r.end (), old_value, new_value);
> +}
> +
>   /* 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.  */

Re: [PATCH v1 06/10] gdb/linux-tdep: migrate linux_info_proc to file_reader_t

[Luis]

Drive-by review.

On 07/07/2026 16:48, Matthieu Longo wrote:
> The patch migratse the code of linux_info_proc to use file_reader_t to

Typo: migratse

> 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 | 120 +++++++++++++++++++++++++++--------------------
>   1 file changed, 69 insertions(+), 51 deletions(-)
> 
> diff --git a/gdb/linux-tdep.c b/gdb/linux-tdep.c
> index a12a69f03a2..a2af1586d35 100644
> --- a/gdb/linux-tdep.c
> +++ b/gdb/linux-tdep.c
> @@ -836,6 +836,27 @@ dump_note_entry_p (filter_flags filterflags, const smaps_data &map)
>     return true;
>   }
>   
> +/* In a character buffer where entries are separated by a SEPARATOR character,
> +   extract the string view starting at START.
> +   Return the extracted view and the iterator to the next entry.  */
> +
> +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')
> +{
> +  if (start < buffer.begin () || start >= buffer.end ())
> +    return std::make_pair (gdb::array_view<char> (), buffer.end ());
> +
> +  auto it = std::find (start, buffer.end (), separator);
> +  if (it == buffer.end ())
> +    return std::make_pair (gdb::array_view<char> (), buffer.end ());
> +
> +  auto next_start = std::next (it);
> +  return std::make_pair
> +    (gdb::array_view<char> (start, next_start), next_start);
> +}
> +
>   /* Implement the "info proc" command.  */
>   
>   static void
> @@ -878,25 +899,20 @@ linux_info_proc (struct gdbarch *gdbarch, const char *args,
>     gdb_printf (_("process %d\n"), ptid.pid ());
>     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);
> +	  gdb::array_view<char> cmdline = cmdline_freader.cast_view<char> ();
> +	  gdb_assert (cmdline[ cmdline.size () - 1] == '\0');

Formatting: Stray space before cmdline.size ()

> +	  /* Replace null characters splitting the arguments in the command
> +	     line by spaces, except for the last one.  */
> +	  gdb::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)
>       {
> @@ -910,27 +926,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)
>       {
> @@ -944,10 +958,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");
> @@ -961,12 +974,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;
> +

Is there a risk we will drop a final chunk of the data when the buffer 
does not end in \n here (or more generally, does not end in whatever 
separator we're looking for), comparing it with the old strtok_r behavior?

It's a corner case, but I thought I´d check.

> +	      /* 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);
> @@ -985,26 +1002,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)));
> @@ -1131,7 +1148,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());

Formatting: Space before parens. Multiple cases.

>       }
>   }
>   

Re: [PATCH v1 07/10] gdb/linux-tdep: migrate linux_find_memory_regions_full to file_reader_t

[Luis]

On 07/07/2026 16:48, Matthieu Longo wrote:
> 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 codedump generation via

Typo: codedump -> coredump

> 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 is 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 a2af1586d35..f1cea3040f9 100644
> --- a/gdb/linux-tdep.c
> +++ b/gdb/linux-tdep.c
> @@ -1777,25 +1777,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.  */

Re: [PATCH v1 08/10] gdb/linux-tdep: migrate parse_smaps_data to file_reader_t

[Luis]

Empty commit message.

On 07/07/2026 16:48, Matthieu Longo wrote:
> ---
>   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 f1cea3040f9..dbc69a5a7fc 100644
> --- a/gdb/linux-tdep.c
> +++ b/gdb/linux-tdep.c
> @@ -1530,18 +1530,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;
>   
> @@ -1597,8 +1596,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;
>   	    }
>   
> @@ -1612,12 +1611,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;
>   
> @@ -1628,8 +1627,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)
> @@ -1679,12 +1679,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.  */
>   

Otherwise looks reasonable.

Reviewed-By: Luis Machado <luis.machado.foss@gmail.com>

Re: [PATCH v1 09/10] gdb/linux-tdep: parse ProtectionKey in /proc/PID/smaps

[Luis]

Drive-by review.

On 07/07/2026 16:48, Matthieu Longo wrote:
> Memory Protection Keys provide a mechanism for enforcing page-based
> protections without requiring modification of the page tables
> when an application changes protection domains. [1]
> 
> The "ProtectionKey" field may be present in /proc/PID/smaps x86_64
> and AArch64 systems since Linux 4.9, when the kernel is built with
> Memory Protection Keys support.
> 
> Add a new 'pkey' field to `struct smaps_data', and populate it when
> the "ProtectionKey" field is present.
> 
> This prepares for displaying the protection key in 'info proc mappings'.
> 
> [1]: https://docs.kernel.org/core-api/protection-keys.html,
>       https://lkml.iu.edu/1512.0/03058.html
> ---
>   gdb/linux-tdep.c | 13 +++++++++++++
>   1 file changed, 13 insertions(+)
> 
> diff --git a/gdb/linux-tdep.c b/gdb/linux-tdep.c
> index dbc69a5a7fc..d89c5ae8504 100644
> --- a/gdb/linux-tdep.c
> +++ b/gdb/linux-tdep.c
> @@ -124,6 +124,7 @@ struct smaps_data
>   
>     ULONGEST rss;
>     ULONGEST swap;
> +  std::optional<int> pkey;
>   };
>   
>   /* Whether to take the /proc/PID/coredump_filter into account when
> @@ -1553,6 +1554,7 @@ parse_smaps_data (const file_reader_t<char> &freader)
>         int mapping_file_p;
>         ULONGEST rss = -1;
>         ULONGEST swap = -1;
> +      int pkey = -1;
>   
>         memset (&v, 0, sizeof (v));
>         struct mapping m = read_mapping (line);
> @@ -1653,6 +1655,16 @@ parse_smaps_data (const file_reader_t<char> &freader)
>   		  mapping_anon_p = 1;
>   		}
>   	    }
> +
> +	  if (streq (keyword, "ProtectionKey:"))
> +	    {
> +	      if (sscanf (line, "%*s%d", &pkey) != 1)
> +		{
> +		  warning (_("Error parsing %s's value in {s,}maps file '%s'"),
> +			   keyword, freader.c_filepath ());

If keyword has the trailing colon this will read...

Error parsing ProtectionKey:'s value

... right?

> +		  break;
> +		}
> +	    }
>   	}
>         /* Save the smaps entry to the vector.  */
>   	struct smaps_data map;
> @@ -1672,6 +1684,7 @@ parse_smaps_data (const file_reader_t<char> &freader)
>   	map.inode = m.inode;
>   	map.rss = rss;
>   	map.swap = swap;
> +	map.pkey.emplace (pkey);

Are we always unconditionally adding a key by design, even when no keys 
were found? Then we add the sentinel -1. Or am I missing something?

>   
>   	smaps.emplace_back (map);
>       }

And I agree with Thiago. This patch should live alongside Srinath's series.

Re: [PATCH v1 10/10] gdb/linux: add helpers to read AT_HWCAP3 and AT_HWCAP4

[Luis]

On 09/07/2026 07:44, Thiago Jung Bauermann wrote:
> Matthieu Longo <matthieu.longo@arm.com> writes:
> 
>> Add linux_get_hwcap3 and linux_get_hwcap4 helpers to GDB and gdbserver,
>> mirroring the existing linux_get_hwcap and linux_get_hwcap2 interfaces.
>>
>> The new helpers retrieve the AT_HWCAP3 and AT_HWCAP4 auxiliary vector
>> entry either from explicitly supplied auxv data or from the current
>> inferior.
>>
>> This prepares for future features that depend on HWCAP3 and HWCAP4
>> capability bits.
>> ---
>>   gdb/linux-tdep.c       | 38 ++++++++++++++++++++++++++++++++++++++
>>   gdb/linux-tdep.h       | 22 ++++++++++++++++++++++
>>   gdbserver/linux-low.cc | 28 ++++++++++++++++++++++++++++
>>   gdbserver/linux-low.h  |  8 ++++++++
>>   4 files changed, 96 insertions(+)
> 
> Reviewed-by: Thiago Jung Bauermann <thiago.bauermann@linaro.org>
> 
> Like in the previous patch, there's nothing in this patch series that
> uses these helpers, so IMHO (other maintainers may disagree) it makes
> more sense if this patch is committed together with a patch that makes
> use of them.
> 

Agreed. Or should be its own patch instead of a part of this series.

RE: [PATCH v1 05/10] gdb: introduce helper class file_reader_t

[Joos, Christina]

> -----Original Message-----
> From: Matthieu Longo <matthieu.longo@arm.com>
> Sent: Montag, 13. Juli 2026 19:13
> To: Schimpe, Christina <christina.schimpe@intel.com>; gdb-
> patches@sourceware.org
> Cc: Luis Machado <luis.machado@amd.com>; Luis Machado
> <luis.machado.foss@gmail.com>; Andrew Burgess <aburgess@redhat.com>;
> Yury Khrustalev <yury.khrustalev@arm.com>; Pedro Alves
> <pedro@palves.net>; Tom Tromey <tom@tromey.com>; Kevin Buettner
> <kevinb@redhat.com>; Thiago Jung Bauermann
> <thiago.bauermann@linaro.org>
> Subject: Re: [PATCH v1 05/10] gdb: introduce helper class file_reader_t
> 
> On 13/07/2026 16:50, Schimpe, Christina wrote:
> >> @@ -2802,9 +2790,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;
> >>
> >> @@ -2842,7 +2827,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
> >> @@ -
> >> 2852,15 +2837,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)
> >>      {
> >>        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))
> >>  	{
> >> @@ -2879,7 +2863,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 ());
> >
> > IIUC, before your change, if the file was empty, we did not show a
> > warning, since target_fileio_read_stralloc handles the case like this:
> > "Empty objects are returned as allocated but empty strings."
> >
> > Based on your file_reader_t implementation, I think we get here in
> > case we can open the file but the file is empty. Not sure if in that
> > case the file can be empty at all, but it is a behavioural change that should
> be avoided in my opinion.
> > There are further similar cases in this patch.
> 
> An empty file is normally fine, but in most of cases, pretty useless hence the
> proposed change in behavior.
> 
> However, I can see a reason when a file is empty and a process still alive.
> From man 5 proc_pid_cmdline:
>   > This  read-only  file  holds the complete command line for the process,
> unless the
>   > process is a zombie.  In the latter case, there is nothing in this file: that is,
>   > a read on this file will return 0 characters.
>   > ...
>   > If, after an execve(2), the process modifies its argv strings, those changes
> will
>   > show up here.
>   > ...
>   > Furthermore, a process may change the memory location that this file
> refers via
>   > prctl(2) operations such as PR_SET_MM_ARG_START.
> 
> The last sentence suggests that an empty file is still valid, and can have a
> different cause than a deceased process. There might be others similar cases.
> 
> I will change the behavior back to what it was for empty files.
> 
> > Would it make sense to change the file_reader_t bool operator to
> > return true in case the file is empty? Maybe we could then additionally
> introduce a new method bool empty ().
> >
> 
> Yes, it would make sense to add a new method empty() and even error ().
> I will also change the behavior of target_fileio_read_stralloc in case of errors
> (see comment from Kevin Buettner).
> 
> bool empty () const
> { m_data != nullptr && m_size > 0; }
> 
> bool error () const
> { m_data == nullptr && m_size == -1; }
> 
> And bool operator() could be the combination of both:
> 
> bool operator () const
> { return !(error () ||empty ()); }
> 
> >> +/* Helper class for reading a file.  */ 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;
> >> +  /* Size of the data.  */
> >> +  size_t 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));
> >> +      }
> >> +  }
> >> +
> >> +  /* Return true if the file was read successfully.  */  operator
> >> + bool
> >> + () const noexcept  { return m_data != nullptr && m_size > 0; }
> >
> > I think we should make this explicit.
> >
> 
> However, making the boolean operator explicit would make it very
> inconvenient.
> I would prefer to avoid this extra verbose:
> 
> if (!static_cast<bool>(freader))
>   {
>     // handle the error or empty
>   }

Hi Matthieu,

This part I cannot fully follow, but maybe I am missing something.
AFAIK no static_cast is necessary if we make this explicit.

Example in gdb:
~~~
class frame_info_ptr : public intrusive_list_node<frame_info_ptr>
{
public:
  /* Create a frame_info_ptr from a raw pointer.  */
  explicit frame_info_ptr (struct frame_info *ptr);
[...]

  /* This exists for compatibility with pre-existing code that checked
     a "frame_info *" like "if (ptr)".  */
  explicit operator bool () const
  {
    return !this->is_null ();
  }
~~~

Here the operator is explicit and used in several locations in gdb without cast.

Kind Regards,
Christina

Re: [PATCH v1 03/10] target_fileio_read_stralloc: add an optional length parameter

[Thiago Jung Bauermann]

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

> On 09/07/2026 07:30, Thiago Jung Bauermann wrote:
>> Matthieu Longo <matthieu.longo@arm.com> writes:
>> 
>>> diff --git a/gdb/target.h b/gdb/target.h
>>> index 22653138491..4215553033c 100644
>>> --- a/gdb/target.h
>>> +++ b/gdb/target.h
>>> @@ -2336,7 +2336,7 @@ extern LONGEST target_fileio_read_alloc (struct inferior *inf,
>>>     are returned as allocated but empty strings.  A warning is issued
>>>     if the result contains any 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, size_t *len = nullptr);
>> 
>> It's worth updating the documentation comment to mention the new parameter.
>
> See the updated diff in gdb/target.h
>
> diff --git a/gdb/target.h b/gdb/target.h
> index 22653138491..0df5a654f75 100644
> --- a/gdb/target.h
> +++ b/gdb/target.h
> @@ -2328,15 +2328,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.  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, size_t *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

Looks great, thanks!

My only suggestion is to take the opportunity to do s/NULL/nullptr/ in
the comment.

-- 
Thiago
(he/him)

Re: [PATCH v1 07/10] gdb/linux-tdep: migrate linux_find_memory_regions_full to file_reader_t

[Thiago Jung Bauermann]

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

> On 09/07/2026 07:36, Thiago Jung Bauermann wrote:
>> 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 codedump generation via
>>> gcore command (see gcore-stale-thread test for more details).
>> 
>> Did you run into this problem, or just noticed it by inspecting the
>> code? If the former, do you think it's worth adding a testcase to make
>> sure this problem doesn't appear again?
>> 
>
> The command stopped working after I migrated the function to file_reader_t.
> As Christina Schimpe mentioned it, bool operator () now returns false if the buffer is
> null, or if
> the size returned is 0. This last is a change with the previous behavior.
> When the thread-group leader exits, the /proc/pid/[s]maps files still exist, but become
> empty.
> In such a case, the function should return false instead of trying to parse an empty
> string, which
> results in no smaps data entry being processed, and so no callback called. This behavior is
> erroneous in my opinion.
>
> I don't think that it is worth adding a testcase for this, because the root cause was the
> wrong pid
> being used.

Ah, thanks for the explanation. I agree with you.

-- 
Thiago
(he/him)

Re: [PATCH v1 08/10] gdb/linux-tdep: migrate parse_smaps_data to file_reader_t

[Thiago Jung Bauermann]

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

> On 09/07/2026 07:41, Thiago Jung Bauermann wrote:
>> Matthieu Longo <matthieu.longo@arm.com> writes:
>> 
>> Suggestion: mention in the commit log that since the previous commit,
>> the old variant of parse_smaps_data is unused.
>> 
> What about :
>
> gdb/linux-tdep: remove legacy parse_smaps_data overload
>
>     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.

Looks good to me, thanks!

-- 
Thiago
(he/him)

Re: [PATCH v1 09/10] gdb/linux-tdep: parse ProtectionKey in /proc/PID/smaps

[Thiago Jung Bauermann]

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

> On 14/07/2026 10:12, Matthieu Longo wrote:
>> On 09/07/2026 07:42, Thiago Jung Bauermann wrote:
>>>
>>> There's nothing in this patch series that uses the parsed pkey, so IMHO
>>> (other maintainers may disagree) it makes more sense if this patch is
>>> committed together with a patch that makes use of the field.
>> 
>> Nothing uses it yet because it is still unclear how to expose it to the users.
>> I assume that "info proc mappings" is a good place to do it.

I agree.

>> As a reminder those are the existing columns:
>> Start Addr         End Addr           Size               Offset             Perms File
>> 
>> Should we add a new column "Protection Key" or "PKey" between "Offset" and "Perms" ?
>> What do you think ?

I like the idea. A short column name is better to avoid wasting precious
horizontal space. I think "PKey" is hard to guess if one isn't familiar
with the hardware feature, so I'd suggest "ProtKey". What do you think?

> Additionally, if we were to print the permissions associated with a PKey, what do you
> think about
> the following view ?
>
> Start Addr         End Addr           Size               Offset    PKey     Perms File
> 0x0000000000400000 0x0000000000483000 0x83000 0x0 0 r-xp /path/to/file
>     ...
> 0x00000000004a2000 0x00000000004a7000 0x5000             0x0       1        rw-p
>     Thread 1: effective: r--  overlay: r--
>     Thread 3: effective: rw-  overlay: rw-
>     Thread 4: effective: r--  overlay: r-x

I like it. Just a few comments:

> Effective being the effective permissions resulting from the ANDing of the base
> permissions (coming
> from "Perms") AND the overlay permissions attached to a thread (permissions associated to
> the PKey.

Maybe it's just me, but displaying the overlay permissions after the
effective permissions makes me think that the latter are the actually
effective ones (I guess because English is a left-to-right language).

So to me it's more intuitive either if the effective field comes later,
or alternatively if there's something to indicate that overlay is not
the main field. E.g., by using parentheses:

    Thread 1: effective permissions: r--  (overlay: r--)

Also, as seen above I think it's clearer if the word "permissions" is
added.

> The look-up of those overlay permissions is platform-specific).

Considering that there are differences in how protection keys are
implemented in different architectures (e.g., IIUC only Arm has overlay
permissions), the whole "effective: r-- overlay: r--" part of the line
should be printed by a gdbarch hook.

> If no protection key support exists on the target, the PKey column would not be printed.
> Same for the threads' permissions (effective and overlay).

Also even if protection key support is available, if there's no
protection key set for any mapping then the column shouldn't be printed
either. Or is there always a key associated with every mapping?

> Does this approach look fine to you ?

Yes, I like it.

> Does it overload the view ?

If the additional fields only appear for inferiors actively using
protection keys, I don't think it overloads the view.

> Should the dumping of effective and overlay permissions be part of a generic or
> platform-specific
> command ?

Considering that several architectures provide this feature, I think it
should be part of a generic command.

-- 
Thiago
(he/him)

Re: [PATCH v1 09/10] gdb/linux-tdep: parse ProtectionKey in /proc/PID/smaps

[Yury Khrustalev]

On Fri, Jul 24, 2026 at 02:58:38AM +0000, Thiago Jung Bauermann wrote:
> Matthieu Longo <matthieu.longo@arm.com> writes:
> 
> > On 14/07/2026 10:12, Matthieu Longo wrote:
> >> On 09/07/2026 07:42, Thiago Jung Bauermann wrote:
> >>>
> >>> There's nothing in this patch series that uses the parsed pkey, so IMHO
> >>> (other maintainers may disagree) it makes more sense if this patch is
> >>> committed together with a patch that makes use of the field.
> >> 
> >> Nothing uses it yet because it is still unclear how to expose it to the users.
> >> I assume that "info proc mappings" is a good place to do it.
> 
> I agree.
> 
> >> As a reminder those are the existing columns:
> >> Start Addr         End Addr           Size               Offset             Perms File
> >> 
> >> Should we add a new column "Protection Key" or "PKey" between "Offset" and "Perms" ?
> >> What do you think ?
> 
> I like the idea. A short column name is better to avoid wasting precious
> horizontal space. I think "PKey" is hard to guess if one isn't familiar
> with the hardware feature, so I'd suggest "ProtKey". What do you think?

PKey is a standard recognised term (see [1] for one example), so I think we should use it.

[1]: https://www.man7.org/linux/man-pages/man7/pkeys.7.html

> 
> > Additionally, if we were to print the permissions associated with a PKey, what do you
> > think about
> > the following view ?
> >
> > Start Addr         End Addr           Size               Offset    PKey     Perms File
> > 0x0000000000400000 0x0000000000483000 0x83000 0x0 0 r-xp /path/to/file
> >     ...
> > 0x00000000004a2000 0x00000000004a7000 0x5000             0x0       1        rw-p
> >     Thread 1: effective: r--  overlay: r--
> >     Thread 3: effective: rw-  overlay: rw-
> >     Thread 4: effective: r--  overlay: r-x
> 
> I like it. Just a few comments:
> 
> > Effective being the effective permissions resulting from the ANDing of the base
> > permissions (coming
> > from "Perms") AND the overlay permissions attached to a thread (permissions associated to
> > the PKey.
> 
> Maybe it's just me, but displaying the overlay permissions after the
> effective permissions makes me think that the latter are the actually
> effective ones (I guess because English is a left-to-right language).

The idea is that effective permissions are always displayed while
overlay and any other future types of permissions are optional and
might not be present. I think having left-most columns always present
and aligned will make it easier to read and parse.

> 
> So to me it's more intuitive either if the effective field comes later,
> or alternatively if there's something to indicate that overlay is not
> the main field. E.g., by using parentheses:
> 
>     Thread 1: effective permissions: r--  (overlay: r--)
> 
> Also, as seen above I think it's clearer if the word "permissions" is
> added.

I think parentheses and another "permissions" word repeated on every
line will only waste horizontal space. The format of output will be
documented of anyone is unsure what those symbols mean.

> > The look-up of those overlay permissions is platform-specific).
> 
> Considering that there are differences in how protection keys are
> implemented in different architectures (e.g., IIUC only Arm has overlay
> permissions), the whole "effective: r-- overlay: r--" part of the line
> should be printed by a gdbarch hook.
> 
> > If no protection key support exists on the target, the PKey column would not be printed.
> > Same for the threads' permissions (effective and overlay).
> 
> Also even if protection key support is available, if there's no
> protection key set for any mapping then the column shouldn't be printed
> either. Or is there always a key associated with every mapping?

I think the column should be just empty or have some placeholder like a
hyphen when mapping is not associated with a pkey.

> 
> > Does this approach look fine to you ?
> 
> Yes, I like it.
> 
> > Does it overload the view ?
> 
> If the additional fields only appear for inferiors actively using
> protection keys, I don't think it overloads the view.
> 
> > Should the dumping of effective and overlay permissions be part of a generic or
> > platform-specific
> > command ?
> 
> Considering that several architectures provide this feature, I think it
> should be part of a generic command.

I agree.

Thanks,
Yury

Re: [PATCH v1 09/10] gdb/linux-tdep: parse ProtectionKey in /proc/PID/smaps

[Thiago Jung Bauermann]

Yury Khrustalev <yury.khrustalev@arm.com> writes:

> On Fri, Jul 24, 2026 at 02:58:38AM +0000, Thiago Jung Bauermann wrote:
>> Matthieu Longo <matthieu.longo@arm.com> writes:
>> 
>> > On 14/07/2026 10:12, Matthieu Longo wrote:
>> >> On 09/07/2026 07:42, Thiago Jung Bauermann wrote:
>> >>>
>> >>> There's nothing in this patch series that uses the parsed pkey, so IMHO
>> >>> (other maintainers may disagree) it makes more sense if this patch is
>> >>> committed together with a patch that makes use of the field.
>> >> 
>> >> Nothing uses it yet because it is still unclear how to expose it to the users.
>> >> I assume that "info proc mappings" is a good place to do it.
>> 
>> I agree.
>> 
>> >> As a reminder those are the existing columns:
>> >> Start Addr         End Addr           Size               Offset             Perms File
>> >> 
>> >> Should we add a new column "Protection Key" or "PKey" between "Offset" and "Perms" ?
>> >> What do you think ?
>> 
>> I like the idea. A short column name is better to avoid wasting precious
>> horizontal space. I think "PKey" is hard to guess if one isn't familiar
>> with the hardware feature, so I'd suggest "ProtKey". What do you think?
>
> PKey is a standard recognised term (see [1] for one example), so I think we should use it.
>
> [1]: https://www.man7.org/linux/man-pages/man7/pkeys.7.html

Ah, even better. PKey is good then. Thanks for the reference.

>> > Additionally, if we were to print the permissions associated with a PKey, what do you
>> > think about
>> > the following view ?
>> >
>> > Start Addr         End Addr           Size               Offset    PKey     Perms File
>> > 0x0000000000400000 0x0000000000483000 0x83000 0x0 0 r-xp /path/to/file
>> >     ...
>> > 0x00000000004a2000 0x00000000004a7000 0x5000             0x0       1        rw-p
>> >     Thread 1: effective: r--  overlay: r--
>> >     Thread 3: effective: rw-  overlay: rw-
>> >     Thread 4: effective: r--  overlay: r-x
>> 
>> I like it. Just a few comments:
>> 
>> > Effective being the effective permissions resulting from the ANDing of the base
>> > permissions (coming
>> > from "Perms") AND the overlay permissions attached to a thread (permissions associated
>> > to
>> > the PKey.
>> 
>> Maybe it's just me, but displaying the overlay permissions after the
>> effective permissions makes me think that the latter are the actually
>> effective ones (I guess because English is a left-to-right language).
>
> The idea is that effective permissions are always displayed while
> overlay and any other future types of permissions are optional and
> might not be present. I think having left-most columns always present
> and aligned will make it easier to read and parse.

Ok, that makes sense. I'll only note that parsing isn't a concern,
because the regular GDB output is for humans. For parsing GDB has the
Machine Interface.

>> So to me it's more intuitive either if the effective field comes later,
>> or alternatively if there's something to indicate that overlay is not
>> the main field. E.g., by using parentheses:
>> 
>>     Thread 1: effective permissions: r--  (overlay: r--)
>> 
>> Also, as seen above I think it's clearer if the word "permissions" is
>> added.
>
> I think parentheses and another "permissions" word repeated on every
> line will only waste horizontal space. The format of output will be
> documented of anyone is unsure what those symbols mean.

Ok, makes sense too.

>> > The look-up of those overlay permissions is platform-specific).
>> 
>> Considering that there are differences in how protection keys are
>> implemented in different architectures (e.g., IIUC only Arm has overlay
>> permissions), the whole "effective: r-- overlay: r--" part of the line
>> should be printed by a gdbarch hook.
>> 
>> > If no protection key support exists on the target, the PKey column would not be printed.
>> > Same for the threads' permissions (effective and overlay).
>> 
>> Also even if protection key support is available, if there's no
>> protection key set for any mapping then the column shouldn't be printed
>> either. Or is there always a key associated with every mapping?
>
> I think the column should be just empty or have some placeholder like a
> hyphen when mapping is not associated with a pkey.

I don't see much use for an empty column (or one containing only
placeholder values), but I also don't feel strongly about it.

-- 
Thiago
(he/him)

Re: [PATCH v1 09/10] gdb/linux-tdep: parse ProtectionKey in /proc/PID/smaps

[Yury Khrustalev]

On Sat, Jul 25, 2026 at 06:40:33AM +0000, Thiago Jung Bauermann wrote:
> Yury Khrustalev <yury.khrustalev@arm.com> writes:
> 
> ...
>
> >> > The look-up of those overlay permissions is platform-specific).
> >> 
> >> Considering that there are differences in how protection keys are
> >> implemented in different architectures (e.g., IIUC only Arm has overlay
> >> permissions), the whole "effective: r-- overlay: r--" part of the line
> >> should be printed by a gdbarch hook.
> >> 
> >> > If no protection key support exists on the target, the PKey column would not be printed.
> >> > Same for the threads' permissions (effective and overlay).
> >> 
> >> Also even if protection key support is available, if there's no
> >> protection key set for any mapping then the column shouldn't be printed
> >> either. Or is there always a key associated with every mapping?
> >
> > I think the column should be just empty or have some placeholder like a
> > hyphen when mapping is not associated with a pkey.
> 
> I don't see much use for an empty column (or one containing only
> placeholder values), but I also don't feel strongly about it.

The point of the empty column (that is only shown when pkeys are
supported) is that it would show that none of your mappings use any
pkeys. I think this is useful piece of information. E.g. if I expect to
see a key but due to a bug it's not used, I don't want to not see this
column at all because I would think that my version of GDB does not
support it or something.

Re: [PATCH v1 01/10] gdb/linux-tdep: change linux_fill_prpsinfo to return bool

[Matthieu Longo]

On 09/07/2026 13:23, Simon Marchi wrote:
> 
> 
> On 2026-07-09 02:26, Thiago Jung Bauermann wrote:
>> Hello Matthieu,
>>
>> Thank you for these patches.
>>
>> Matthieu Longo <matthieu.longo@arm.com> writes:
>>
>>> Change linux_fill_prpsinfo() to return a boolean instead of an integer, since
>>> it only reports success or failure.
>>> Replace the returned integer values 1 and 0 with true and false respectively.
>>> ---
>>>  gdb/linux-tdep.c | 22 +++++++++++-----------
>>>  1 file changed, 11 insertions(+), 11 deletions(-)
>>
>> In his review of an analogous patch, Simon suggested that this kind of
>> change could be pushed as obvious:
>>
>> https://inbox.sourceware.org/gdb-patches/53a86cae-2927-49ec-83a2-22ff6526c0e6@simark.ca/
>>
>> I would agree, but I don't know if that's a generally accepted view.
> 
> Yeah, you still need to be careful, I did manage once to mess up and
> change a 0 into true or vice versa.
> 
> Also, try to change comments that use 0/1, or "zero"/"non-zero" so they
> say true/false.  I see you did it for comments inside the function, but
> the comment above the function also has thing that would need to be
> updated.
> 
>>
>> In any case:
>>
>> Reviewed-by: Thiago Jung Bauermann <thiago.bauermann@linaro.org>
> 
> With the comment updated:
> 
> Approved-By: Simon Marchi <simon.marchi@efficios.com>
> 
> Simon

Merged.
https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=067cdb3b087a78820e2e71ae5e7f7de7763712d5

Matthieu

Re: [PATCH v1 03/10] target_fileio_read_stralloc: add an optional length parameter

[Matthieu Longo]

On 21/07/2026 22:28, Luis wrote:
> Empty commit message...
> 
> On 07/07/2026 16:48, Matthieu Longo wrote:
>> ---
>>   gdb/target.c | 10 +++++++---
>>   gdb/target.h |  2 +-
>>   2 files changed, 8 insertions(+), 4 deletions(-)
>>
>> diff --git a/gdb/target.c b/gdb/target.c
>> index 5d937f3ae85..e4907ca815a 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,
>> +                 size_t *len)
>>   {
>>     gdb_byte *buffer;
>>     char *bufstr;
>> @@ -3556,17 +3557,20 @@ target_fileio_read_stralloc (struct inferior *inf, const char *filename)
>>     transferred = target_fileio_read_alloc_1 (inf, filename, &buffer, 1);
>>     bufstr = (char *) buffer;
>>   +  if (len != nullptr)
>> +    *len = (transferred < 0 ? 0 : transferred);
>> +
>>     if (transferred < 0)
>>       return gdb::unique_xmalloc_ptr<char> (nullptr);
>>       if (transferred == 0)
>>       return make_unique_xstrdup ("");
>>   -  bufstr[transferred] = 0;
>> +  bufstr[transferred] = '\0';
> 
> Why 0 -> \0?
> 

The null byte is set at the end of the string.
The null byte is usually noted as '\0'.
I replace the usage of 0 by '\0'.
Is there an issue with this ?

>>       /* Check for embedded NUL bytes; but allow trailing NULs.  */
>>     for (i = strlen (bufstr); i < transferred; i++)
>> -    if (bufstr[i] != 0)
>> +    if (bufstr[i] != '\0')
> 
> Likewise, why 0 -> \0?
> 

Same answer here.

Matthieu

Re: [PATCH v1 03/10] target_fileio_read_stralloc: add an optional length parameter

[Matthieu Longo]

On 24/07/2026 03:50, Thiago Jung Bauermann wrote:
> Matthieu Longo <matthieu.longo@arm.com> writes:
> 
>> On 09/07/2026 07:30, Thiago Jung Bauermann wrote:
>>> Matthieu Longo <matthieu.longo@arm.com> writes:
>>>
>>>> diff --git a/gdb/target.h b/gdb/target.h
>>>> index 22653138491..4215553033c 100644
>>>> --- a/gdb/target.h
>>>> +++ b/gdb/target.h
>>>> @@ -2336,7 +2336,7 @@ extern LONGEST target_fileio_read_alloc (struct inferior *inf,
>>>>     are returned as allocated but empty strings.  A warning is issued
>>>>     if the result contains any 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, size_t *len = nullptr);
>>>
>>> It's worth updating the documentation comment to mention the new parameter.
>>
>> See the updated diff in gdb/target.h
>>
>> diff --git a/gdb/target.h b/gdb/target.h
>> index 22653138491..0df5a654f75 100644
>> --- a/gdb/target.h
>> +++ b/gdb/target.h
>> @@ -2328,15 +2328,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.  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, size_t *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
> 
> Looks great, thanks!
> 
> My only suggestion is to take the opportunity to do s/NULL/nullptr/ in
> the comment.
> 

I don't remember having seen usages of nullptr inside the documentation comments.
Have you a previous example for this ?

Matthieu

Re: [PATCH v1 03/10] target_fileio_read_stralloc: add an optional length parameter

[Matthieu Longo]

On 21/07/2026 22:28, Luis wrote:
> Empty commit message...

Fixed in the next revision.

target_fileio_read_stralloc: add an optional length parameter

    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.

Re: [PATCH v1 04/10] gdb support: add gdb::replace algorithm for iterators and ranges

[Matthieu Longo]

On 21/07/2026 22:33, Luis wrote:
> Drive-by review, and C++20 issues aside.
> 
> On 07/07/2026 16:48, Matthieu Longo wrote:
>> Add a gdb::replace helper that mirrors the behavior of std::replace
>> for iterator pairs, together with a convenience overload accepting a
>> range.
>>
>> This provides a C++17-compatible replacement for the C++20 std::replace/
>> std::ranges::replace algorithms, allowing callers to use a consistent
>> interface until GDB transitions to C++20. The helpers should be removed
>> once the C++ standard library implementations become available.
>> ---
>>   gdbsupport/array-view.h | 26 ++++++++++++++++++++++++++
>>   1 file changed, 26 insertions(+)
>>
>> diff --git a/gdbsupport/array-view.h b/gdbsupport/array-view.h
>> index 8431d7f5add..61119d7a8e2 100644
>> --- a/gdbsupport/array-view.h
>> +++ b/gdbsupport/array-view.h
>> @@ -225,6 +225,32 @@ void copy (gdb::array_view<U> src, gdb::array_view<T> dest)
>>       std::copy_backward (src.begin (), src.end (), dest.end ());
>>   }
>>   +/* Replace all occurrences of a value in the provided range.
>> +
>> +   Note: this helper is a reimplementation of std::replace, only available
>> +   from C++20 onwards, and consequently, should be removed once we switch
>> +   to C++20.  */
>> +
>> +template <class ForwardIt, typename T>
>> +void replace (ForwardIt first, ForwardIt last,
>> +          const T &old_value, const T &new_value)
>> +{
>> +  for (auto it = first; it != last; ++it)
>> +  {
>> +    if (*it == old_value)
>> +      *it = new_value;
>> +  }
> 
> Formatting: Identation of the braces is off.
> 

This code will disappear in the next revision.

>> +}
>> +
>> +/* Replace all occurrences of a value in the provided array view.
>> +   Note: from C++20 onwards, std::ranges::replace should be used instead.  */
>> +
>> +template <class Range, typename T>
>> +void replace (Range r, const T &old_value, const T &new_value)
>> +{
>> +  replace (r.begin (), r.end (), old_value, new_value);

Replaced by std::replace (r.begin (), r.end (), old_value, new_value);

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

Matthieu

Re: [PATCH v1 05/10] gdb: introduce helper class file_reader_t

[Matthieu Longo]

On 22/07/2026 17:36, Joos, Christina wrote:
>> -----Original Message-----
>> From: Matthieu Longo <matthieu.longo@arm.com>
>> Sent: Montag, 13. Juli 2026 19:13
>> To: Schimpe, Christina <christina.schimpe@intel.com>; gdb-
>> patches@sourceware.org
>> Cc: Luis Machado <luis.machado@amd.com>; Luis Machado
>> <luis.machado.foss@gmail.com>; Andrew Burgess <aburgess@redhat.com>;
>> Yury Khrustalev <yury.khrustalev@arm.com>; Pedro Alves
>> <pedro@palves.net>; Tom Tromey <tom@tromey.com>; Kevin Buettner
>> <kevinb@redhat.com>; Thiago Jung Bauermann
>> <thiago.bauermann@linaro.org>
>> Subject: Re: [PATCH v1 05/10] gdb: introduce helper class file_reader_t
>>
>> On 13/07/2026 16:50, Schimpe, Christina wrote:
>>>> @@ -2802,9 +2790,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;
>>>>
>>>> @@ -2842,7 +2827,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
>>>> @@ -
>>>> 2852,15 +2837,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)
>>>>      {
>>>>        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))
>>>>  	{
>>>> @@ -2879,7 +2863,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 ());
>>>
>>> IIUC, before your change, if the file was empty, we did not show a
>>> warning, since target_fileio_read_stralloc handles the case like this:
>>> "Empty objects are returned as allocated but empty strings."
>>>
>>> Based on your file_reader_t implementation, I think we get here in
>>> case we can open the file but the file is empty. Not sure if in that
>>> case the file can be empty at all, but it is a behavioural change that should
>> be avoided in my opinion.
>>> There are further similar cases in this patch.
>>
>> An empty file is normally fine, but in most of cases, pretty useless hence the
>> proposed change in behavior.
>>
>> However, I can see a reason when a file is empty and a process still alive.
>> From man 5 proc_pid_cmdline:
>>   > This  read-only  file  holds the complete command line for the process,
>> unless the
>>   > process is a zombie.  In the latter case, there is nothing in this file: that is,
>>   > a read on this file will return 0 characters.
>>   > ...
>>   > If, after an execve(2), the process modifies its argv strings, those changes
>> will
>>   > show up here.
>>   > ...
>>   > Furthermore, a process may change the memory location that this file
>> refers via
>>   > prctl(2) operations such as PR_SET_MM_ARG_START.
>>
>> The last sentence suggests that an empty file is still valid, and can have a
>> different cause than a deceased process. There might be others similar cases.
>>
>> I will change the behavior back to what it was for empty files.
>>
>>> Would it make sense to change the file_reader_t bool operator to
>>> return true in case the file is empty? Maybe we could then additionally
>> introduce a new method bool empty ().
>>>
>>
>> Yes, it would make sense to add a new method empty() and even error ().
>> I will also change the behavior of target_fileio_read_stralloc in case of errors
>> (see comment from Kevin Buettner).
>>
>> bool empty () const
>> { m_data != nullptr && m_size > 0; }
>>
>> bool error () const
>> { m_data == nullptr && m_size == -1; }
>>
>> And bool operator() could be the combination of both:
>>
>> bool operator () const
>> { return !(error () ||empty ()); }
>>
>>>> +/* Helper class for reading a file.  */ 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;
>>>> +  /* Size of the data.  */
>>>> +  size_t 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));
>>>> +      }
>>>> +  }
>>>> +
>>>> +  /* Return true if the file was read successfully.  */  operator
>>>> + bool
>>>> + () const noexcept  { return m_data != nullptr && m_size > 0; }
>>>
>>> I think we should make this explicit.
>>>
>>
>> However, making the boolean operator explicit would make it very
>> inconvenient.
>> I would prefer to avoid this extra verbose:
>>
>> if (!static_cast<bool>(freader))
>>   {
>>     // handle the error or empty
>>   }
> 
> Hi Matthieu,
> 
> This part I cannot fully follow, but maybe I am missing something.
> AFAIK no static_cast is necessary if we make this explicit.
> 
> Example in gdb:
> ~~~
> class frame_info_ptr : public intrusive_list_node<frame_info_ptr>
> {
> public:
>   /* Create a frame_info_ptr from a raw pointer.  */
>   explicit frame_info_ptr (struct frame_info *ptr);
> [...]
> 
>   /* This exists for compatibility with pre-existing code that checked
>      a "frame_info *" like "if (ptr)".  */
>   explicit operator bool () const
>   {
>     return !this->is_null ();
>   }
> ~~~
> 
> Here the operator is explicit and used in several locations in gdb without cast.
> 
> Kind Regards,
> Christina

It seems that I have misunderstood the meaning of 'explicit' all those years.
I added explicit as you suggested, and it didn't trigger a compilation error.
Thanks for pointing this out.

Regards,
Matthieu

Re: [PATCH v1 06/10] gdb/linux-tdep: migrate linux_info_proc to file_reader_t

[Matthieu Longo]

On 21/07/2026 22:43, Luis wrote:
> Drive-by review.
> 
> On 07/07/2026 16:48, Matthieu Longo wrote:
>> The patch migratse the code of linux_info_proc to use file_reader_t to
> 
> Typo: migratse
> 

Fixed.

>> 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 | 120 +++++++++++++++++++++++++++--------------------
>>   1 file changed, 69 insertions(+), 51 deletions(-)
>>
>> diff --git a/gdb/linux-tdep.c b/gdb/linux-tdep.c
>> index a12a69f03a2..a2af1586d35 100644
>> --- a/gdb/linux-tdep.c
>> +++ b/gdb/linux-tdep.c
>> @@ -836,6 +836,27 @@ dump_note_entry_p (filter_flags filterflags, const smaps_data &map)
>>     return true;
>>   }
>>   +/* In a character buffer where entries are separated by a SEPARATOR character,
>> +   extract the string view starting at START.
>> +   Return the extracted view and the iterator to the next entry.  */
>> +
>> +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')
>> +{
>> +  if (start < buffer.begin () || start >= buffer.end ())
>> +    return std::make_pair (gdb::array_view<char> (), buffer.end ());
>> +
>> +  auto it = std::find (start, buffer.end (), separator);
>> +  if (it == buffer.end ())
>> +    return std::make_pair (gdb::array_view<char> (), buffer.end ());
>> +
>> +  auto next_start = std::next (it);
>> +  return std::make_pair
>> +    (gdb::array_view<char> (start, next_start), next_start);
>> +}
>> +
>>   /* Implement the "info proc" command.  */
>>     static void
>> @@ -878,25 +899,20 @@ linux_info_proc (struct gdbarch *gdbarch, const char *args,
>>     gdb_printf (_("process %d\n"), ptid.pid ());
>>     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);
>> +      gdb::array_view<char> cmdline = cmdline_freader.cast_view<char> ();
>> +      gdb_assert (cmdline[ cmdline.size () - 1] == '\0');
> 
> Formatting: Stray space before cmdline.size ()
> 

Fixed.

>> +      /* Replace null characters splitting the arguments in the command
>> +         line by spaces, except for the last one.  */
>> +      gdb::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)
>>       {
>> @@ -910,27 +926,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)
>>       {
>> @@ -944,10 +958,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");
>> @@ -961,12 +974,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;
>> +
> 
> Is there a risk we will drop a final chunk of the data when the buffer does not end in \n here (or
> more generally, does not end in whatever separator we're looking for), comparing it with the old
> strtok_r behavior?
> 
> It's a corner case, but I thought I´d check.
> 

You were right. There was a bug when the separator is not present at the end of the buffer.
See https://godbolt.org/z/fh5Y7fv8P for testing.
I will change the implementation in the next revision to the below.

/* 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};
}

>> +          /* 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);
>> @@ -985,26 +1002,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)));
>> @@ -1131,7 +1148,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());
> 
> Formatting: Space before parens. Multiple cases.
> 

Fixed.

>>       }
>>   }
>>   
> 

Matthieu

Re: [PATCH v1 07/10] gdb/linux-tdep: migrate linux_find_memory_regions_full to file_reader_t

[Matthieu Longo]

On 21/07/2026 22:47, Luis wrote:
> 
> On 07/07/2026 16:48, Matthieu Longo wrote:
>> 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 codedump generation via
> 
> Typo: codedump -> coredump
> 

Fixed.

Matthieu

Re: [PATCH v1 08/10] gdb/linux-tdep: migrate parse_smaps_data to file_reader_t

[Matthieu Longo]

On 21/07/2026 22:49, Luis wrote:
> Empty commit message.
> 
For the next revision.

gdb/linux-tdep: remove legacy parse_smaps_data overload

    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>

Re: [PATCH v1 10/10] gdb/linux: add helpers to read AT_HWCAP3 and AT_HWCAP4

[Matthieu Longo]

On 21/07/2026 22:58, Luis wrote:
> On 09/07/2026 07:44, Thiago Jung Bauermann wrote:
>> Matthieu Longo <matthieu.longo@arm.com> writes:
>>
>>> Add linux_get_hwcap3 and linux_get_hwcap4 helpers to GDB and gdbserver,
>>> mirroring the existing linux_get_hwcap and linux_get_hwcap2 interfaces.
>>>
>>> The new helpers retrieve the AT_HWCAP3 and AT_HWCAP4 auxiliary vector
>>> entry either from explicitly supplied auxv data or from the current
>>> inferior.
>>>
>>> This prepares for future features that depend on HWCAP3 and HWCAP4
>>> capability bits.
>>> ---
>>>   gdb/linux-tdep.c       | 38 ++++++++++++++++++++++++++++++++++++++
>>>   gdb/linux-tdep.h       | 22 ++++++++++++++++++++++
>>>   gdbserver/linux-low.cc | 28 ++++++++++++++++++++++++++++
>>>   gdbserver/linux-low.h  |  8 ++++++++
>>>   4 files changed, 96 insertions(+)
>>
>> Reviewed-by: Thiago Jung Bauermann <thiago.bauermann@linaro.org>
>>
>> Like in the previous patch, there's nothing in this patch series that
>> uses these helpers, so IMHO (other maintainers may disagree) it makes
>> more sense if this patch is committed together with a patch that makes
>> use of them.
>>
> 
> Agreed. Or should be its own patch instead of a part of this series.

Moved to its own patch:
https://inbox.sourceware.org/gdb-patches/20260727172831.121566-1-matthieu.longo@arm.com/

Matthieu

Re: [PATCH v1 09/10] gdb/linux-tdep: parse ProtectionKey in /proc/PID/smaps

[Matthieu Longo]

On 21/07/2026 22:57, Luis wrote:
> Drive-by review.
> 
> On 07/07/2026 16:48, Matthieu Longo wrote:
>> Memory Protection Keys provide a mechanism for enforcing page-based
>> protections without requiring modification of the page tables
>> when an application changes protection domains. [1]
>>
>> The "ProtectionKey" field may be present in /proc/PID/smaps x86_64
>> and AArch64 systems since Linux 4.9, when the kernel is built with
>> Memory Protection Keys support.
>>
>> Add a new 'pkey' field to `struct smaps_data', and populate it when
>> the "ProtectionKey" field is present.
>>
>> This prepares for displaying the protection key in 'info proc mappings'.
>>
>> [1]: https://docs.kernel.org/core-api/protection-keys.html,
>>       https://lkml.iu.edu/1512.0/03058.html
>> ---
>>   gdb/linux-tdep.c | 13 +++++++++++++
>>   1 file changed, 13 insertions(+)
>>
>> diff --git a/gdb/linux-tdep.c b/gdb/linux-tdep.c
>> index dbc69a5a7fc..d89c5ae8504 100644
>> --- a/gdb/linux-tdep.c
>> +++ b/gdb/linux-tdep.c
>> @@ -124,6 +124,7 @@ struct smaps_data
>>       ULONGEST rss;
>>     ULONGEST swap;
>> +  std::optional<int> pkey;
>>   };
>>     /* Whether to take the /proc/PID/coredump_filter into account when
>> @@ -1553,6 +1554,7 @@ parse_smaps_data (const file_reader_t<char> &freader)
>>         int mapping_file_p;
>>         ULONGEST rss = -1;
>>         ULONGEST swap = -1;
>> +      int pkey = -1;
>>           memset (&v, 0, sizeof (v));
>>         struct mapping m = read_mapping (line);
>> @@ -1653,6 +1655,16 @@ parse_smaps_data (const file_reader_t<char> &freader)
>>             mapping_anon_p = 1;
>>           }
>>           }
>> +
>> +      if (streq (keyword, "ProtectionKey:"))
>> +        {
>> +          if (sscanf (line, "%*s%d", &pkey) != 1)
>> +        {
>> +          warning (_("Error parsing %s's value in {s,}maps file '%s'"),
>> +               keyword, freader.c_filepath ());
> 
> If keyword has the trailing colon this will read...
> 
> Error parsing ProtectionKey:'s value
> 
> ... right?
> 

Yes, you are right.
Fixed in the next revision.

--- a/gdb/linux-tdep.c
+++ b/gdb/linux-tdep.c
@@ -1729,6 +1729,8 @@ parse_smaps_data (const file_reader_t<char> &freader)
            {
              if (sscanf (line, "%*s%d", &pkey) != 1)
                {
+                 /* Remove the trailing column when printing the warning.  */
+                 keyword[sizeof ("ProtectionKey:") - 2] = '\0';
                  warning (_("Error parsing %s's value in {s,}maps file '%s'"),
                           keyword, freader.c_filepath ());
                  break;

>> +          break;
>> +        }
>> +        }
>>       }
>>         /* Save the smaps entry to the vector.  */
>>       struct smaps_data map;
>> @@ -1672,6 +1684,7 @@ parse_smaps_data (const file_reader_t<char> &freader)
>>       map.inode = m.inode;
>>       map.rss = rss;
>>       map.swap = swap;
>> +    map.pkey.emplace (pkey);
> 
> Are we always unconditionally adding a key by design, even when no keys were found? Then we add the
> sentinel -1. Or am I missing something?
> 

Yes, my mistake. I should have checked on the sentinel value.
Fixed in the next revision.

>>         smaps.emplace_back (map);
>>       }
> 
> And I agree with Thiago. This patch should live alongside Srinath's series.

For the next revision, I actually display the value in the PKey column (no effective permissions or
overlay ones in this scope, just the PKey display for any version of Linux that expose it in
/proc/PID/smaps)

To do so, I refactored the code section in "if (mappings_f)". This refactoring relies on the
introduction of file_reader_t, so it would make sense to keep it in this patch series.
Please have a look at the next revision.

Matthieu

Re: [PATCH v1 09/10] gdb/linux-tdep: parse ProtectionKey in /proc/PID/smaps

[Thiago Jung Bauermann]

Yury Khrustalev <yury.khrustalev@arm.com> writes:

> On Sat, Jul 25, 2026 at 06:40:33AM +0000, Thiago Jung Bauermann wrote:
>> Yury Khrustalev <yury.khrustalev@arm.com> writes:
>> 
>> ...
>>
>> >> > The look-up of those overlay permissions is platform-specific).
>> >> 
>> >> Considering that there are differences in how protection keys are
>> >> implemented in different architectures (e.g., IIUC only Arm has overlay
>> >> permissions), the whole "effective: r-- overlay: r--" part of the line
>> >> should be printed by a gdbarch hook.
>> >> 
>> >> > If no protection key support exists on the target, the PKey column would not be
>> >> > printed.
>> >> > Same for the threads' permissions (effective and overlay).
>> >> 
>> >> Also even if protection key support is available, if there's no
>> >> protection key set for any mapping then the column shouldn't be printed
>> >> either. Or is there always a key associated with every mapping?
>> >
>> > I think the column should be just empty or have some placeholder like a
>> > hyphen when mapping is not associated with a pkey.
>> 
>> I don't see much use for an empty column (or one containing only
>> placeholder values), but I also don't feel strongly about it.
>
> The point of the empty column (that is only shown when pkeys are
> supported) is that it would show that none of your mappings use any
> pkeys. I think this is useful piece of information. E.g. if I expect to
> see a key but due to a bug it's not used, I don't want to not see this
> column at all because I would think that my version of GDB does not
> support it or something.

Ah, indeed it makes sense. Thanks for clarifying.

-- 
Thiago
(he/him)

Re: [PATCH v1 03/10] target_fileio_read_stralloc: add an optional length parameter

[Thiago Jung Bauermann]

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

> On 24/07/2026 03:50, Thiago Jung Bauermann wrote:
>> Matthieu Longo <matthieu.longo@arm.com> writes:
>> 
>>> On 09/07/2026 07:30, Thiago Jung Bauermann wrote:
>>>> Matthieu Longo <matthieu.longo@arm.com> writes:
>>>>
>>>>> diff --git a/gdb/target.h b/gdb/target.h
>>>>> index 22653138491..4215553033c 100644
>>>>> --- a/gdb/target.h
>>>>> +++ b/gdb/target.h
>>>>> @@ -2336,7 +2336,7 @@ extern LONGEST target_fileio_read_alloc (struct inferior *inf,
>>>>>     are returned as allocated but empty strings.  A warning is issued
>>>>>     if the result contains any 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, size_t *len = nullptr);
>>>>
>>>> It's worth updating the documentation comment to mention the new parameter.
>>>
>>> See the updated diff in gdb/target.h
>>>
>>> diff --git a/gdb/target.h b/gdb/target.h
>>> index 22653138491..0df5a654f75 100644
>>> --- a/gdb/target.h
>>> +++ b/gdb/target.h
>>> @@ -2328,15 +2328,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.  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, size_t *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
>> 
>> Looks great, thanks!
>> 
>> My only suggestion is to take the opportunity to do s/NULL/nullptr/ in
>> the comment.
>> 
>
> I don't remember having seen usages of nullptr inside the documentation comments.
> Have you a previous example for this ?

Yes, there are a number of examples:

gdb/block.h:

/* Return the compunit over whose static or global block the iterator currently
   iterates.  Return nullptr if the iteration is finished.  */
  struct compunit_symtab *compunit_symtab () const;

gdb/breakpoint.h:

  /* Reevaluate a breakpoint.  This is necessary after symbols change
     (e.g., an executable or DSO was loaded, or the inferior just
     started).

     If not nullptr, then FILTER_PSPACE is the program space in which
     symbols may have changed, we only need to add new locations in
     FILTER_PSPACE.

     If FILTER_PSPACE is nullptr then all program spaces may have changed,
     new locations need to be searched for in every program space.

     This is pure virtual as, at a minimum, each sub-class must recompute
     any cached condition expressions based off of the cond_string member
     variable.  */
  virtual void re_set (program_space *filter_pspace) = 0;

gdb/gdbcore.h:

  /* Constructor.  BUILD_ID is not nullptr, and is the build-id for the
     mapped file.  FILENAME is the location of the file that GDB loaded to
     provide the mapped file.  This might be different from the name of the
     mapped file mentioned in the core file, e.g. if GDB downloads a file
     from debuginfod then FILENAME would point into the debuginfod client
     cache.  The FILENAME can be the empty string if GDB was unable to find
     a file to provide the mapped file.  */

  core_target_mapped_file_info (const bfd_build_id *build_id,
				const std::string filename)

gdb/language.h:

  /* Set the default boolean type to be TYPE.  If NAME is not nullptr then
     before using TYPE a symbol called NAME will be looked up, and the type
     of this symbol will be used instead.  Should only be called once when
     performing setup for a particular language in combination with a
     particular gdbarch.  */
  void set_bool_type (struct type *type, const char *name = nullptr)

And others as well.

-- 
Thiago
(he/him)