Re: [PATCH 4/11] gdb/solib-rocm: add cached_fd to manage cached fd lifetime

[Lancelot SIX <lancelot.six@amd.com>]

On Tue, Dec 09, 2025 at 02:32:08PM -0500, Simon Marchi wrote:
> Make the management of the cached fds automatic.
> 
> Change rocm_solib_fd_cache::open to return a cached_fd, an RAII type
> that calls rocm_solib_fd_cache::close on destruction.  cached_fd holds
> the actual fd and a reference to the cache it comes from.
> 
> Change rocm_code_object_stream_file to hold a cached_fd and remove the
> explicit destructor.
> 
> This is not essential, I just thought it would be nice.
> 
> Change-Id: I2529ac1caebbe6b7e6e60d228a403064f6f38d06
> ---
>  gdb/solib-rocm.c | 91 ++++++++++++++++++++++++++++++------------------
>  1 file changed, 58 insertions(+), 33 deletions(-)
> 
> diff --git a/gdb/solib-rocm.c b/gdb/solib-rocm.c
> index bde34bafd8b1..5560a6035184 100644
> --- a/gdb/solib-rocm.c
> +++ b/gdb/solib-rocm.c
> @@ -39,22 +39,58 @@ struct rocm_solib_fd_cache
>    explicit rocm_solib_fd_cache (inferior *inf) : m_inferior (inf) {}
>    DISABLE_COPY_AND_ASSIGN (rocm_solib_fd_cache);
>  
> +  /* The open method returns an object of this type.
> +
> +     On destruction, automatically call close to indicate the caller no longer
> +     uses the fd.  */
> +  struct cached_fd
> +  {
> +    cached_fd (int fd, rocm_solib_fd_cache &cache)
> +      : fd (fd),
> +	cache (cache)
> +    {
> +    }
> +
> +    cached_fd (cached_fd &&other)
> +      : fd (other.fd),
> +	cache (other.cache)
> +    {
> +      other.fd = -1;
> +    }
> +
> +    ~cached_fd ()
> +    {
> +      if (fd == -1)
> +	return;
> +
> +      fileio_error target_errno;
> +      if (cache.close (fd, &target_errno) != 0)
> +	warning (_("Failed to close solib: %s"),
> +		 strerror (fileio_error_to_host (target_errno)));
> +    }
> +
> +    DISABLE_COPY_AND_ASSIGN (cached_fd);
> +
> +    int fd;

Given some changes since this patch was sent, this is now a target_fd.

> +    rocm_solib_fd_cache &cache;

the cache field is never accessed outside of the class, there is no need
to have it public.

I expect quite a few conflicts when rebasing this patch, nevertheless
the direction so far works for me.

Best,
Lancelot.

> +  };
> +
>    /* Return a read-only file descriptor to FILENAME and increment the
>       associated reference count.
>  
>       Open the file FILENAME if it is not already opened, reuse the existing file
>       descriptor otherwise.
>  
> -     On error -1 is returned, and TARGET_ERRNO is set.  */
> -  int open (const std::string &filename, fileio_error *target_errno);
> +     On error, return a cached_fd with .fd == -1 and set *TARGET_ERRNO.  */
> +  cached_fd open (const std::string &filename, fileio_error *target_errno);
>  
> +private:
>    /* Decrement the reference count to FD and close FD if the reference count
>       reaches 0.
>  
>       On success, return 0.  On error, return -1 and set TARGET_ERRNO.  */
>    int close (int fd, fileio_error *target_errno);
>  
> -private:
>    struct refcnt_fd
>    {
>      refcnt_fd (int fd, int refcnt) : fd (fd), refcnt (refcnt) {}
> @@ -72,7 +108,7 @@ struct rocm_solib_fd_cache
>    gdb::unordered_map<std::string, refcnt_fd> m_cache;
>  };
>  
> -int
> +rocm_solib_fd_cache::cached_fd
>  rocm_solib_fd_cache::open (const std::string &filename,
>  			   fileio_error *target_errno)
>  {
> @@ -83,11 +119,14 @@ rocm_solib_fd_cache::open (const std::string &filename,
>        int fd
>  	= target_fileio_open (m_inferior, filename.c_str (), FILEIO_O_RDONLY,
>  			      false, 0, target_errno);
> -      if (fd != -1)
> -	m_cache.emplace (std::piecewise_construct,
> -			 std::forward_as_tuple (filename),
> -			 std::forward_as_tuple (fd, 1));
> -      return fd;
> +      if (fd == -1)
> +	return cached_fd { -1, *this };
> +
> +      m_cache.emplace (std::piecewise_construct,
> +		       std::forward_as_tuple (filename),
> +		       std::forward_as_tuple (fd, 1));
> +
> +      return cached_fd { fd, *this };
>      }
>    else
>      {
> @@ -95,7 +134,7 @@ rocm_solib_fd_cache::open (const std::string &filename,
>  	 already opened FD.  */
>        it->second.refcnt++;
>        gdb_assert (it->second.fd != -1);
> -      return it->second.fd;
> +      return cached_fd { it->second.fd, *this };
>      }
>  }
>  
> @@ -360,7 +399,7 @@ struct rocm_code_object_stream_file final : rocm_code_object_stream
>  {
>    DISABLE_COPY_AND_ASSIGN (rocm_code_object_stream_file);
>  
> -  rocm_code_object_stream_file (rocm_solib_fd_cache &fd_cache, int fd,
> +  rocm_code_object_stream_file (rocm_solib_fd_cache::cached_fd fd,
>  				ULONGEST offset, ULONGEST size);
>  
>    file_ptr read (bfd *abfd, void *buf, file_ptr size,
> @@ -368,14 +407,9 @@ struct rocm_code_object_stream_file final : rocm_code_object_stream
>  
>    LONGEST size () override;
>  
> -  ~rocm_code_object_stream_file () override;
> -
>  protected:
> -  /* The fd cache owning this code object stream.  */
> -  rocm_solib_fd_cache &m_fd_cache;
> -
>    /* The target file descriptor for this stream.  */
> -  int m_fd;
> +  rocm_solib_fd_cache::cached_fd m_fd;
>  
>    /* The offset of the ELF file image in the target file.  */
>    ULONGEST m_offset;
> @@ -386,9 +420,8 @@ struct rocm_code_object_stream_file final : rocm_code_object_stream
>  };
>  
>  rocm_code_object_stream_file::rocm_code_object_stream_file
> -  (rocm_solib_fd_cache &fd_cache, int fd, ULONGEST offset,
> -   ULONGEST size)
> -  : m_fd_cache (fd_cache), m_fd (fd), m_offset (offset), m_size (size)
> +  (rocm_solib_fd_cache::cached_fd fd, ULONGEST offset, ULONGEST size)
> +  : m_fd (std::move (fd)), m_offset (offset), m_size (size)
>  {
>  }
>  
> @@ -403,7 +436,7 @@ rocm_code_object_stream_file::read (bfd *, void *buf, file_ptr size,
>        QUIT;
>  
>        file_ptr bytes_read
> -	= target_fileio_pread (m_fd, static_cast<gdb_byte *> (buf) + nbytes,
> +	= target_fileio_pread (m_fd.fd, static_cast<gdb_byte *> (buf) + nbytes,
>  			       size, m_offset + offset + nbytes,
>  			       &target_errno);
>  
> @@ -431,7 +464,7 @@ rocm_code_object_stream_file::size ()
>      {
>        fileio_error target_errno;
>        struct stat stat;
> -      if (target_fileio_fstat (m_fd, &stat, &target_errno) < 0)
> +      if (target_fileio_fstat (m_fd.fd, &stat, &target_errno) < 0)
>  	{
>  	  errno = fileio_error_to_host (target_errno);
>  	  bfd_set_error (bfd_error_system_call);
> @@ -451,14 +484,6 @@ rocm_code_object_stream_file::size ()
>    return m_size;
>  }
>  
> -rocm_code_object_stream_file::~rocm_code_object_stream_file ()
> -{
> -  fileio_error target_errno;
> -  if (m_fd_cache.close (m_fd, &target_errno) != 0)
> -    warning (_("Failed to close solib: %s"),
> -	     strerror (fileio_error_to_host (target_errno)));
> -}
> -
>  /* Interface to a code object which lives in the inferior's memory.  */
>  
>  struct rocm_code_object_stream_memory final : public rocm_code_object_stream
> @@ -598,16 +623,16 @@ rocm_bfd_iovec_open (bfd *abfd, inferior *inferior)
>  	{
>  	  auto info = get_solib_info (inferior);
>  	  fileio_error target_errno;
> -	  int fd = info->fd_cache.open (decoded_path, &target_errno);
> +	  auto fd = info->fd_cache.open (decoded_path, &target_errno);
>  
> -	  if (fd == -1)
> +	  if (fd.fd == -1)
>  	    {
>  	      errno = fileio_error_to_host (target_errno);
>  	      bfd_set_error (bfd_error_system_call);
>  	      return nullptr;
>  	    }
>  
> -	  return new rocm_code_object_stream_file (info->fd_cache, fd, offset,
> +	  return new rocm_code_object_stream_file (std::move (fd), offset,
>  						   size);
>  	}
>  
> -- 
> 2.52.0