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

[Matthieu Longo <matthieu.longo@arm.com>]

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