Re: [PATCH v2 3/10] gdb/solib-rocm: add cached_target_fd to manage cached fd lifetime

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

On Mon, Jun 08, 2026 at 04:00:27PM -0400, Simon Marchi wrote:
> Make the management of the cached fds automatic.
> 
> Change rocm_solib_fd_cache::open to return a cached_target_fd, an RAII
> type that calls rocm_solib_fd_cache::close on destruction.
> cached_target_fd holds the actual fd and a reference to the cache it
> comes from.
> 
> Change rocm_code_object_stream_file to hold a cached_target_fd and
> remove the explicit destructor.
> 
> This is not essential, I just thought it would be nice.

Hi Simon,

This overall looks good.  Just have one minor nits below.  Other than that,

Approved-by: Lancelot Six <lancelot.six@amd.com> (amdgpu)

> 
> Change-Id: I2529ac1caebbe6b7e6e60d228a403064f6f38d06
> ---
>  gdb/solib-rocm.c | 90 +++++++++++++++++++++++++++++++-----------------
>  1 file changed, 59 insertions(+), 31 deletions(-)
> 
> diff --git a/gdb/solib-rocm.c b/gdb/solib-rocm.c
> index f96f0823200e..60e64c494f88 100644
> --- a/gdb/solib-rocm.c
> +++ b/gdb/solib-rocm.c
> @@ -40,22 +40,63 @@ 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_target_fd
> +  {
> +    cached_target_fd (target_fd fd, rocm_solib_fd_cache &cache)
> +      : m_fd (fd),
> +	m_cache (cache)
> +    {
> +    }
> +
> +    cached_target_fd (cached_target_fd &&other)
> +      : m_fd (other.m_fd),
> +	m_cache (other.m_cache)
> +    {
> +      other.m_fd = target_fd::INVALID;
> +    }
> +
> +    ~cached_target_fd ()
> +    {
> +      if (m_fd == target_fd::INVALID)
> +	return;
> +
> +      fileio_error target_errno;
> +      if (m_cache.close (m_fd, &target_errno) != 0)
> +	warning (_("Failed to close solib: %s"),
> +		 strerror (fileio_error_to_host (target_errno)));
> +    }
> +
> +    DISABLE_COPY_AND_ASSIGN (cached_target_fd);
> +
> +    target_fd fd () const { return m_fd; }
> +
> +private:

Indentation seems wrong here.

> +    target_fd m_fd;
> +    rocm_solib_fd_cache &m_cache;
> +  };
> +
>    /* 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 target_fd::INVALID is returned, and TARGET_ERRNO is set.  */
> -  target_fd open (const std::string &filename, fileio_error *target_errno);
> +     On error, return a cached_target_fd with target_fd::INVALID and set
> +     *TARGET_ERRNO.  */
> +  cached_target_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 (target_fd fd, fileio_error *target_errno);
>  
> -private:
>    struct refcnt_fd
>    {
>      refcnt_fd (target_fd fd, int refcnt) : fd (fd), refcnt (refcnt) {}
> @@ -73,7 +114,7 @@ struct rocm_solib_fd_cache
>    gdb::unordered_string_map<refcnt_fd> m_cache;
>  };
>  
> -target_fd
> +rocm_solib_fd_cache::cached_target_fd
>  rocm_solib_fd_cache::open (const std::string &filename,
>  			   fileio_error *target_errno)
>  {
> @@ -88,7 +129,7 @@ rocm_solib_fd_cache::open (const std::string &filename,
>  	m_cache.emplace (std::piecewise_construct,
>  			 std::forward_as_tuple (filename),
>  			 std::forward_as_tuple (fd, 1));
> -      return fd;
> +      return cached_target_fd { fd, *this };;

You have 2 ";" here.  One is too much.

>      }
>    else
>      {
> @@ -96,7 +137,7 @@ rocm_solib_fd_cache::open (const std::string &filename,
>  	 already opened FD.  */
>        it->second.refcnt++;
>        gdb_assert (it->second.fd != target_fd::INVALID);
> -      return it->second.fd;
> +      return cached_target_fd { it->second.fd, *this };
>      }
>  }
>  
> @@ -356,7 +397,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, target_fd fd,
> +  rocm_code_object_stream_file (rocm_solib_fd_cache::cached_target_fd fd,
>  				ULONGEST offset, ULONGEST size);
>  
>    file_ptr read (bfd *abfd, void *buf, file_ptr size,
> @@ -364,14 +405,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.  */
> -  target_fd m_fd;
> +  rocm_solib_fd_cache::cached_target_fd m_fd;
>  
>    /* The offset of the ELF file image in the target file.  */
>    ULONGEST m_offset;
> @@ -382,9 +418,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, target_fd fd, ULONGEST offset,
> -   ULONGEST size)
> -  : m_fd_cache (fd_cache), m_fd (fd), m_offset (offset), m_size (size)
> +  (rocm_solib_fd_cache::cached_target_fd fd, ULONGEST offset, ULONGEST size)
> +  : m_fd (std::move (fd)), m_offset (offset), m_size (size)
>  {
>  }
>  
> @@ -399,9 +434,9 @@ 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,
> -			       size, m_offset + offset + nbytes,
> -			       &target_errno);
> +	= target_fileio_pread (m_fd.fd (),
> +			       static_cast<gdb_byte *> (buf) + nbytes, size,
> +			       m_offset + offset + nbytes, &target_errno);
>  
>        if (bytes_read == 0)
>  	break;
> @@ -427,7 +462,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);
> @@ -447,14 +482,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
> @@ -610,16 +637,17 @@ rocm_bfd_iovec_open (bfd *abfd, inferior *inferior)
>  
>  	  auto info = get_solib_info (inferior);
>  	  fileio_error target_errno;
> -	  target_fd fd = info->fd_cache.open (decoded_path, &target_errno);
> +	  rocm_solib_fd_cache::cached_target_fd fd
> +	    = info->fd_cache.open (decoded_path, &target_errno);
>  
> -	  if (fd == target_fd::INVALID)
> +	  if (fd.fd () == target_fd::INVALID)
>  	    {
>  	      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.54.0