Re: [PATCH 1/2] Use a newtype for remote file descriptor

[Simon Marchi <simark@simark.ca>]

On 2/25/26 4:25 PM, Tom Tromey wrote:
> I was digging through the remote fd code and got a little confused at
> some point about what kind of fd was used where.
> 
> It seemed better to me to avoid any confusion by making the remote
> file descriptor a newtype, i.e., incompatible with 'int'.
> 
> I limited the change to the "public" API.  That is, I let the file
> descriptor as used by the actual target implementation methods remain
> "int".
> 
> This found one bug, namely that sparc64-tdep.c assumed that 0 was an
> invalid value for a target fd.
> 
> The solib-rocm.c changes are best-effort, as I can't compile this
> file.

solib-rocm.c just needs one more change to build:

diff --git i/gdb/solib-rocm.c w/gdb/solib-rocm.c
index 651a958f1620..6ebe0c2b8cd2 100644
--- i/gdb/solib-rocm.c
+++ w/gdb/solib-rocm.c
@@ -94,7 +94,7 @@ rocm_solib_fd_cache::open (const std::string &filename,
       /* The file is already opened.  Increment the refcnt and return the
 	 already opened FD.  */
       it->second.refcnt++;
-      gdb_assert (it->second.fd != -1);
+      gdb_assert (it->second.fd != remote_fd::INVALID);
       return it->second.fd;
     }
 }

Otherwise, LGTM.  Just a naming nit below.

Approved-By: Simon Marchi <simon.marchi@efficios.com>

Btw, if you want to build the ROCm stuff as part of your all-targets
build, it's not that difficult, as long as your distro has some ROCm
packages.  Looks there are some for Fedora (which I believe you use):

https://src.fedoraproject.org/rpms/rocdbgapi

You should only need the dbgapi package.  You configure with
--with-amd-dbgapi, which will make configure fail if it can't find the
dbgapi library.  configure will hopefully find the dbgapi library in its
default installed location, otherwise you have to point PKG_CONFIG_PATH
to it.  That's pretty much it.

> diff --git a/gdb/solib-rocm.c b/gdb/solib-rocm.c
> index 29169417682..6e05913696a 100644
> --- a/gdb/solib-rocm.c
> +++ b/gdb/solib-rocm.c
> @@ -45,26 +45,26 @@ struct rocm_solib_fd_cache
>       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 remote_fd::INVALID is returned, and TARGET_ERRNO is set.  */
> +  remote_fd open (const std::string &filename, fileio_error *target_errno);
>  
>    /* 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);
> +  int close (remote_fd fd, fileio_error *target_errno);
>  
>  private:
>    struct refcnt_fd
>    {
> -    refcnt_fd (int fd, int refcnt) : fd (fd), refcnt (refcnt) {}
> +    refcnt_fd (remote_fd fd, int refcnt) : fd (fd), refcnt (refcnt) {}
>  
>      DISABLE_COPY_AND_ASSIGN (refcnt_fd);
>  
>      refcnt_fd (refcnt_fd &&) = default;
>      refcnt_fd &operator=(refcnt_fd &&other) = default;
>  
> -    int fd = -1;
> +    remote_fd fd = remote_fd::INVALID;

Calling it remote_fd in the context of ROCm is weird, since it's not
remote.  Perhaps call it target_fd?  It would make sense to me, since
it's the target file I/O interface.

Simon