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

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

On 14/08/2026 16:25, Matthieu Longo wrote:
> On 10/08/2026 14:07, Joos, Christina wrote:
>> Hi Matthieu,
>>
>> Please find my feedback below.
>>
>>> -----Original Message-----
>>> From: Matthieu Longo <matthieu.longo@arm.com>
>>> Sent: Dienstag, 28. Juli 2026 17:17
>>> To: gdb-patches@sourceware.org
>>> Cc: Luis Machado <luis.machado@amd.com>; Luis Machado
>>> <luis.machado.foss@gmail.com>; Thiago Jung Bauermann
>>> <thiago.bauermann@linaro.org>; Simon Marchi <simark@simark.ca>; Kevin
>>> Buettner <kevinb@redhat.com>; Joos, Christina <christina.joos@intel.com>;
>>> Joos, Christina <christina.joos@intel.com>; Matthieu Longo
>>> <matthieu.longo@arm.com>
>>> Subject: [PATCH v1 3/6] gdb: introduce helper class file_reader_t
>>>
>>> Wrap all the boilerplate code required to read a file in a new helper
>>> class: file_reader_t. The class owns the file contents together with the file path,
>>> and provides convenient accessors for the data, size and typed views. It
>>> supports both null-terminated text files and binary files.
>>>
>>> This helper eliminates repeated calls to target_fileio_read_stralloc and
>>> target_fileio_read_alloc, remove explicit memory management with
>>> gdb::unique_xmalloc_ptr, and simplifies the casting logic when working with
>>> binary data.
>>>
>>> The patch converts some of the existing Linux, AMD64, and SPARC code that
>>> reads files from /proc to use file_reader_t.
>>> ---
>>>  gdb/amd64-linux-tdep.c |  12 ++---
>>>  gdb/linux-tdep.c       | 109 ++++++++++++++++++-----------------------
>>>  gdb/sparc64-tdep.c     |  13 +++--
>>>  gdb/target.h           |  79 +++++++++++++++++++++++++++++
>>>  4 files changed, 137 insertions(+), 76 deletions(-)
>>>
>>> diff --git a/gdb/linux-tdep.c b/gdb/linux-tdep.c index
>>> 9bdcc55a0e1..11f0a6952ea 100644
>>> --- a/gdb/linux-tdep.c
>>> +++ b/gdb/linux-tdep.c
>>> @@ -1698,6 +1698,12 @@ parse_smaps_data (const char *data,
>>>    return smaps;
>>>  }
>>>
>>> +static std::vector<struct smaps_data>
>>
>> Nit: we should omit the struct keyword here.
>>
> 
> Fixed. And in others places where it is relevant.
> 
>>> +parse_smaps_data (const file_reader_t<char> &freader) {
>>> +  return parse_smaps_data (freader.data (), freader.filepath ()); }
>>> +
>>>  /* Helper that checks if an address is in a memory tag page for a live
>>>     process.  */
>>>
>>> @@ -1709,17 +1715,13 @@ linux_process_address_in_memtag_page
>>> (CORE_ADDR address)
>>>
>>>    ptid_t ptid = get_process_reference_ptid ();
>>>
>>> -  std::string smaps_file = string_printf ("/proc/%ld/smaps", ptid.lwp ());
>>> -
>>> -  gdb::unique_xmalloc_ptr<char> data
>>> -    = target_fileio_read_stralloc (NULL, smaps_file.c_str ());
>>> -
>>> -  if (data == nullptr)
>>> +  file_reader_t<char> smaps_freader
>>> +    (string_printf ("/proc/%ld/smaps", ptid.lwp ()));  if
>>> + (!smaps_freader)
>>>      return false;
>>>
>>>    /* Parse the contents of smaps into a vector.  */
>>> -  std::vector<struct smaps_data> smaps
>>> -    = parse_smaps_data (data.get (), smaps_file);
>>> +  std::vector<struct smaps_data> smaps = parse_smaps_data
>>> + (smaps_freader);
>>
>> Like amd64_linux_lam_untag_mask we have a minor behavioural change here.
>> But I again see it as an improvement, as the result should be the same, we just
>> return earlier. There might be some more cases in linux-tdep.c, but I did not check
>> all of them.
>>
>> I think it's worth pointing out in the commit message.
>>
> 
> Amended this paragraph in the commit message:
> 
> The patch converts some of the existing Linux, AMD64, and SPARC code that
> reads files from /proc to use file_reader_t. As a side effect,
> amd64_linux_lam_untag_mask and linux_process_address_in_memtag_page
> may now return earlier in case the file is empty.
> 
>>> diff --git a/gdb/sparc64-tdep.c b/gdb/sparc64-tdep.c index
>>> 93db3417a2a..955631b87df 100644
>>> --- a/gdb/sparc64-tdep.c
>>> +++ b/gdb/sparc64-tdep.c
>>> @@ -301,18 +301,16 @@ adi_tag_fd ()
>>>  static bool
>>>  adi_is_addr_mapped (CORE_ADDR vaddr, size_t cnt)  {
>>> -  char filename[MAX_PROC_NAME_SIZE];
>>>    size_t i = 0;
>>>
>>>    pid_t pid = inferior_ptid.pid ();
>>> -  snprintf (filename, sizeof filename, "/proc/%ld/adi/maps", (long) pid);
>>> -  gdb::unique_xmalloc_ptr<char> data
>>> -    = target_fileio_read_stralloc (NULL, filename);
>>> -  if (data)
>>> +  file_reader_t<char> adi_maps_freader
>>> +    (string_printf ("/proc/%d/adi/maps", pid));  if
>>> + (adi_maps_freader.error ())
>>
>> Didn't you mean 
>>   if (!adi_maps_freader.error ())
>>
>> ?
>>
> 
> Yes, you're right.
> 
> However, I am wondering why the diagnostic message is not an error instead of warning.
> Any idea ?
> 
> I would also like to change the program flow to something like:
> 
>   file_reader_t<char> adi_maps_freader
>     (string_printf ("/proc/%d/adi/maps", pid));
>   if (adi_maps_freader)
>     {
>       // This is skipped when the file is empty
>       ...
>     }
>   else if (adi_maps_freader.error ())
>     error (_("unable to open /proc file '%s'"),
> 	     adi_maps_freader.c_filepath ());
>   return false;
> 
>>> diff --git a/gdb/target.h b/gdb/target.h index 819279c08fc..09830dfe3a9
>>> 100644
>>> --- a/gdb/target.h
>>> +++ b/gdb/target.h
>>> @@ -2341,6 +2341,85 @@ extern LONGEST target_fileio_read_alloc (struct
>>> inferior *inf,  extern gdb::unique_xmalloc_ptr<char> target_fileio_read_stralloc
>>>      (struct inferior *inf, const char *filename, LONGEST *len = nullptr);
>>>
>>> +/* Helper class for reading the content of a file on the target.  */
>>> +template <typename T> class file_reader_t {
>>> +  /* The filepath of the file being read.  */
>>> +  std::string m_filepath;
>>> +  /* Smart pointer to the data.  */
>>> +  gdb::unique_xmalloc_ptr<T> m_data;
>>> +  /* Number of bytes read.  */
>>> +  LONGEST m_size;
>>> +
>>> +public:
>>> +  file_reader_t (const std::string &filepath)
>>> +    : m_filepath (filepath)
>>> +    , m_size (0)
>>> +  {
>>> +    if constexpr (std::is_same_v<T, char>)
>>> +      m_data = target_fileio_read_stralloc (nullptr, m_filepath.c_str (),
>>> +					    &m_size);
>>> +    else
>>> +      {
>>> +	gdb_byte *buf = nullptr;
>>> +	m_size = target_fileio_read_alloc (nullptr, m_filepath.c_str (), &buf);
>>> +	m_data = gdb::unique_xmalloc_ptr<T> (reinterpret_cast<T *>(buf));
>>> +      }
>>> +  }
>>> +
>>> +  file_reader_t (file_reader_t &&other)
>>> +    : m_filepath (std::move (other.m_filepath))
>>> +    , m_data (std::move (other.m_data))
>>> +    , m_size (other.m_size)
>>> +  {}
>>
>> I hope I get the C++ rules right here:
>> AFAIK, since we have the move constructor, we implicitly delete the copy
>> constructor and the copy assignment operator.
>>
>> Wouldn't it be clearer if we'd write this explicitly using DISABLE_COPY_AND_ASSIGN ?
>>
> 
> I agree. Added.
> 
>> I also wondered if the default move constructor is doing the same thing, so we could write sth. like:
>> file_reader_t (file_reader_t &&other) = default;
>>
>> instead.
>>
> 
> Fixed as follows:
> 
>   file_reader_t (file_reader_t &&) = default;
>   file_reader_t &operator= (file_reader_t &&) = default;
> 
>   DISABLE_COPY_AND_ASSIGN (file_reader_t);
> 
>>> +  /* Return true if the file was read successfully but contained no
>>> + data.  */  bool empty () const noexcept  { return m_data != nullptr &&
>>> + m_size == 0; }
>>> +
>>> +  /* Return true if the file could not be read.  */  bool error ()
>>> + const noexcept  { return m_data == nullptr || m_size < 0; }
>>> +  /* Return true if the file was read successfully and is non-empty.
>>> + */  explicit operator bool () const noexcept  { return !(error () ||
>>> + empty ()); }
>>
>> To me it would feel more natural if we return true also in case the file is empty.
>> Then we could also omit the error function.
>>
>> But I don't have a very strong opinion about this.
>>
>>
>> Christina
> 
> The cases where we use .error (), the alternative should check whether the file is not empty. From
> this perspective, including !.empty () in the boolean operator makes sense.
> 
> See for instance linux_vsyscall_range_raw or adi_is_addr_mapped, the loop with strtok_r() will be
> skipped if the content is empty.
> 
> Matthieu

Hi Christina,

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

Regards,
Matthieu