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

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

> -----Original Message-----
> From: Matthieu Longo <matthieu.longo@arm.com>
> Sent: Freitag, 14. August 2026 17:26
> To: Joos, Christina <christina.joos@intel.com>; 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>
> Subject: Re: [PATCH v1 3/6] gdb: introduce helper class file_reader_t
> 
> 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;

Just a more general comment on this:
This rather sounds like something which can be a separate commit, since the reason
for this change is not the introduction of the helper class IIUC.
I know this seems like a super tiny nit, but it would make the review easier in my opinion.
Then you can point out in the commit message why you think a warning is better.

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

As I said, I don't have a strong opinion on this. 😊

For the x86 related parts this patch lgtm, so for those parts:

Reviewed-by: Christina Joos <christina.joos@intel.com>

Christina
________________________________________
Intel Deutschland GmbH 

Registered Address: Dornacher Strasse 1, 85622 Feldkirchen, Germany 

Tel: +49 (89) 99143-0 

www.intel.de 

Managing Directors: Candice Moore, Jeffrey Schneiderman, Ramachandran Sitaraman

Chairperson of the Supervisory Board: Sonja Pierer

Registered Seat: Munich Commercial Register B: Amtsgericht Munich HRB 186928

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.