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

Simon Marchi
2026-02-26T16:40:12Z

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