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

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

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