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

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

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