[PATCH v2 1/3] Use a newtype for remote file descriptor

[Tom Tromey <tromey@adacore.com>]

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.

New in v2:
- solib-rocm.c compiles now
- Renamed to target_fd (including scoped_target_fd)

Approved-By: Simon Marchi <simon.marchi@efficios.com>
---
 gdb/gdb_bfd.c      |  8 +++---
 gdb/remote.c       | 12 ++++-----
 gdb/solib-rocm.c   | 32 +++++++++++------------
 gdb/sparc64-tdep.c | 18 ++++++-------
 gdb/target.c       | 76 ++++++++++++++++++++++++++++--------------------------
 gdb/target.h       | 26 ++++++++++++-------
 6 files changed, 92 insertions(+), 80 deletions(-)

diff --git a/gdb/gdb_bfd.c b/gdb/gdb_bfd.c
index 1933166b5fb..70494745c82 100644
--- a/gdb/gdb_bfd.c
+++ b/gdb/gdb_bfd.c
@@ -365,7 +365,7 @@ gdb_bfd_open_from_target_memory (CORE_ADDR addr, ULONGEST size,
 
 struct target_fileio_stream : public gdb_bfd_iovec_base
 {
-  target_fileio_stream (bfd *nbfd, int fd)
+  target_fileio_stream (bfd *nbfd, target_fd fd)
     : m_bfd (nbfd),
       m_fd (fd)
   {
@@ -384,7 +384,7 @@ struct target_fileio_stream : public gdb_bfd_iovec_base
   bfd *m_bfd;
 
   /* The file descriptor.  */
-  int m_fd;
+  target_fd m_fd;
 };
 
 /* Wrapper for target_fileio_open suitable for use as a helper
@@ -394,7 +394,7 @@ static target_fileio_stream *
 gdb_bfd_iovec_fileio_open (struct bfd *abfd, inferior *inf, bool warn_if_slow)
 {
   const char *filename = bfd_get_filename (abfd);
-  int fd;
+  target_fd fd;
   fileio_error target_errno;
 
   gdb_assert (is_target_filename (filename));
@@ -403,7 +403,7 @@ gdb_bfd_iovec_fileio_open (struct bfd *abfd, inferior *inf, bool warn_if_slow)
 			   filename + strlen (TARGET_SYSROOT_PREFIX),
 			   FILEIO_O_RDONLY, 0, warn_if_slow,
 			   &target_errno);
-  if (fd == -1)
+  if (fd == target_fd::INVALID)
     {
       errno = fileio_error_to_host (target_errno);
       bfd_set_error (bfd_error_system_call);
diff --git a/gdb/remote.c b/gdb/remote.c
index 22f584f0c57..a5fb16381b1 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -13869,15 +13869,15 @@ remote_hostio_error (fileio_error errnum)
 
 /* A RAII wrapper around a remote file descriptor.  */
 
-class scoped_remote_fd
+class scoped_target_fd
 {
 public:
-  scoped_remote_fd (remote_target *remote, int fd)
+  scoped_target_fd (remote_target *remote, int fd)
     : m_remote (remote), m_fd (fd)
   {
   }
 
-  ~scoped_remote_fd ()
+  ~scoped_target_fd ()
   {
     if (m_fd != -1)
       {
@@ -13907,7 +13907,7 @@ class scoped_remote_fd
       }
   }
 
-  DISABLE_COPY_AND_ASSIGN (scoped_remote_fd);
+  DISABLE_COPY_AND_ASSIGN (scoped_target_fd);
 
   /* Release ownership of the file descriptor, and return it.  */
   ATTRIBUTE_UNUSED_RESULT int release () noexcept
@@ -13956,7 +13956,7 @@ remote_target::remote_file_put (const char *local_file, const char *remote_file,
   if (file == NULL)
     perror_with_name (local_file);
 
-  scoped_remote_fd fd
+  scoped_target_fd fd
     (this, remote_hostio_open (NULL,
 			       remote_file, (FILEIO_O_WRONLY | FILEIO_O_CREAT
 					     | FILEIO_O_TRUNC),
@@ -14044,7 +14044,7 @@ remote_target::remote_file_get (const char *remote_file, const char *local_file,
   int bytes, io_size;
   ULONGEST offset;
 
-  scoped_remote_fd fd
+  scoped_target_fd fd
     (this, remote_hostio_open (NULL,
 			       remote_file, FILEIO_O_RDONLY, 0, 0,
 			       &remote_errno));
diff --git a/gdb/solib-rocm.c b/gdb/solib-rocm.c
index 29169417682..d9ae4294c98 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 target_fd::INVALID is returned, and TARGET_ERRNO is set.  */
+  target_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 (target_fd fd, fileio_error *target_errno);
 
 private:
   struct refcnt_fd
   {
-    refcnt_fd (int fd, int refcnt) : fd (fd), refcnt (refcnt) {}
+    refcnt_fd (target_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;
+    target_fd fd = target_fd::INVALID;
     int refcnt = 0;
   };
 
@@ -72,7 +72,7 @@ struct rocm_solib_fd_cache
   gdb::unordered_string_map<refcnt_fd> m_cache;
 };
 
-int
+target_fd
 rocm_solib_fd_cache::open (const std::string &filename,
 			   fileio_error *target_errno)
 {
@@ -80,10 +80,10 @@ rocm_solib_fd_cache::open (const std::string &filename,
   if (it == m_cache.end ())
     {
       /* The file is not yet opened on the target.  */
-      int fd
+      target_fd fd
 	= target_fileio_open (m_inferior, filename.c_str (), FILEIO_O_RDONLY,
-			      false, 0, target_errno);
-      if (fd != -1)
+			      0, false, target_errno);
+      if (fd != target_fd::INVALID)
 	m_cache.emplace (std::piecewise_construct,
 			 std::forward_as_tuple (filename),
 			 std::forward_as_tuple (fd, 1));
@@ -94,13 +94,13 @@ 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 != target_fd::INVALID);
       return it->second.fd;
     }
 }
 
 int
-rocm_solib_fd_cache::close (int fd, fileio_error *target_errno)
+rocm_solib_fd_cache::close (target_fd fd, fileio_error *target_errno)
 {
   using cache_val = gdb::unordered_string_map<refcnt_fd>::value_type;
   auto it
@@ -349,7 +349,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 (inferior *inf, int fd, ULONGEST offset,
+  rocm_code_object_stream_file (inferior *inf, target_fd fd, ULONGEST offset,
 				ULONGEST size);
 
   file_ptr read (bfd *abfd, void *buf, file_ptr size,
@@ -365,7 +365,7 @@ struct rocm_code_object_stream_file final : rocm_code_object_stream
   inferior *m_inf;
 
   /* The target file descriptor for this stream.  */
-  int m_fd;
+  target_fd m_fd;
 
   /* The offset of the ELF file image in the target file.  */
   ULONGEST m_offset;
@@ -376,7 +376,7 @@ struct rocm_code_object_stream_file final : rocm_code_object_stream
 };
 
 rocm_code_object_stream_file::rocm_code_object_stream_file
-  (inferior *inf, int fd, ULONGEST offset, ULONGEST size)
+  (inferior *inf, target_fd fd, ULONGEST offset, ULONGEST size)
   : m_inf (inf), m_fd (fd), m_offset (offset), m_size (size)
 {
 }
@@ -588,9 +588,9 @@ 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);
+	  target_fd fd = info->fd_cache.open (decoded_path, &target_errno);
 
-	  if (fd == -1)
+	  if (fd == target_fd::INVALID)
 	    {
 	      errno = fileio_error_to_host (target_errno);
 	      bfd_set_error (bfd_error_system_call);
diff --git a/gdb/sparc64-tdep.c b/gdb/sparc64-tdep.c
index 15600a640fd..e4d06eeef61 100644
--- a/gdb/sparc64-tdep.c
+++ b/gdb/sparc64-tdep.c
@@ -101,7 +101,7 @@ struct adi_stat_t
   int max_version;
 
   /* ADI version tag file.  */
-  int tag_fd = 0;
+  target_fd tag_fd = target_fd::INVALID;
 
   /* ADI availability check has been done.  */
   bool checked_avail = false;
@@ -176,7 +176,7 @@ sparc64_forget_process (pid_t pid)
     {
       if ((*it).pid == pid)
 	{
-	  if ((*it).stat.tag_fd > 0)
+	  if ((*it).stat.tag_fd != target_fd::INVALID)
 	    target_fileio_close ((*it).stat.tag_fd, &target_errno);
 	  adi_proc_list.erase_after (pit);
 	  break;
@@ -275,13 +275,13 @@ adi_convert_byte_count (CORE_ADDR naddr, int nbytes, CORE_ADDR locl)
    K * adi_blksz, encoded as 1 version tag per byte.  The allowed
    version tag values are between 0 and adi_stat.max_version.  */
 
-static int
-adi_tag_fd (void)
+static target_fd
+adi_tag_fd ()
 {
   pid_t pid = inferior_ptid.pid ();
   sparc64_adi_info *proc = get_adi_info_proc (pid);
 
-  if (proc->stat.tag_fd != 0)
+  if (proc->stat.tag_fd != target_fd::INVALID)
     return proc->stat.tag_fd;
 
   char cl_name[MAX_PROC_NAME_SIZE];
@@ -338,8 +338,8 @@ adi_is_addr_mapped (CORE_ADDR vaddr, size_t cnt)
 static int
 adi_read_versions (CORE_ADDR vaddr, size_t size, gdb_byte *tags)
 {
-  int fd = adi_tag_fd ();
-  if (fd == -1)
+  target_fd fd = adi_tag_fd ();
+  if (fd == target_fd::INVALID)
     return -1;
 
   if (!adi_is_addr_mapped (vaddr, size))
@@ -359,8 +359,8 @@ adi_read_versions (CORE_ADDR vaddr, size_t size, gdb_byte *tags)
 static int
 adi_write_versions (CORE_ADDR vaddr, size_t size, unsigned char *tags)
 {
-  int fd = adi_tag_fd ();
-  if (fd == -1)
+  target_fd fd = adi_tag_fd ();
+  if (fd == target_fd::INVALID)
     return -1;
 
   if (!adi_is_addr_mapped (vaddr, size))
diff --git a/gdb/target.c b/gdb/target.c
index 4990e20d8d6..5264b0fb04b 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -3125,8 +3125,8 @@ fileio_handles_invalidate_target (target_ops *targ)
 
 /* Acquire a target fileio file descriptor.  */
 
-static int
-acquire_fileio_fd (target_ops *target, int target_fd)
+static target_fd
+acquire_fileio_fd (target_ops *target, int underlying_fd)
 {
   /* Search for closed handles to reuse.  */
   for (; lowest_closed_fd < fileio_fhandles.size (); lowest_closed_fd++)
@@ -3139,33 +3139,33 @@ acquire_fileio_fd (target_ops *target, int target_fd)
 
   /* Push a new handle if no closed handles were found.  */
   if (lowest_closed_fd == fileio_fhandles.size ())
-    fileio_fhandles.push_back (fileio_fh_t {target, target_fd});
+    fileio_fhandles.push_back (fileio_fh_t {target, underlying_fd});
   else
-    fileio_fhandles[lowest_closed_fd] = {target, target_fd};
+    fileio_fhandles[lowest_closed_fd] = {target, underlying_fd};
 
   /* Should no longer be marked closed.  */
   gdb_assert (!fileio_fhandles[lowest_closed_fd].is_closed ());
 
   /* Return its index, and start the next lookup at
      the next index.  */
-  return lowest_closed_fd++;
+  return target_fd (lowest_closed_fd++);
 }
 
 /* Release a target fileio file descriptor.  */
 
 static void
-release_fileio_fd (int fd, fileio_fh_t *fh)
+release_fileio_fd (target_fd fd, fileio_fh_t *fh)
 {
   fh->target_fd = -1;
-  lowest_closed_fd = std::min (lowest_closed_fd, fd);
+  lowest_closed_fd = std::min (lowest_closed_fd, int (fd));
 }
 
 /* Return a pointer to the fileio_fhandle_t corresponding to FD.  */
 
 static fileio_fh_t *
-fileio_fd_to_fh (int fd)
+fileio_fd_to_fh (target_fd fd)
 {
-  return &fileio_fhandles[fd];
+  return &fileio_fhandles[int (fd)];
 }
 
 
@@ -3239,7 +3239,7 @@ target_ops::fileio_readlink (struct inferior *inf, const char *filename,
 
 /* See target.h.  */
 
-int
+target_fd
 target_fileio_open (struct inferior *inf, const char *filename,
 		    int flags, int mode, bool warn_if_slow, fileio_error *target_errno)
 {
@@ -3251,25 +3251,28 @@ target_fileio_open (struct inferior *inf, const char *filename,
       if (fd == -1 && *target_errno == FILEIO_ENOSYS)
 	continue;
 
+      target_fd result;
       if (fd < 0)
-	fd = -1;
+	result = target_fd::INVALID;
       else
-	fd = acquire_fileio_fd (t, fd);
-
-      target_debug_printf_nofunc ("target_fileio_open (%d,%s,0x%x,0%o,%d) = %d (%d)",
-			   inf == NULL ? 0 : inf->num, filename, flags, mode,
-			   warn_if_slow, fd, fd != -1 ? 0 : *target_errno);
-      return fd;
+	result = acquire_fileio_fd (t, fd);
+
+      target_debug_printf_nofunc
+	("target_fileio_open (%d,%s,0x%x,0%o,%d) = %d (%d)",
+	 inf == NULL ? 0 : inf->num, filename,
+	 flags, mode, warn_if_slow, int (result),
+	 result != target_fd::INVALID ? 0 : *target_errno);
+      return result;
     }
 
   *target_errno = FILEIO_ENOSYS;
-  return -1;
+  return target_fd::INVALID;
 }
 
 /* See target.h.  */
 
 int
-target_fileio_pwrite (int fd, const gdb_byte *write_buf, int len,
+target_fileio_pwrite (target_fd fd, const gdb_byte *write_buf, int len,
 		      ULONGEST offset, fileio_error *target_errno)
 {
   fileio_fh_t *fh = fileio_fd_to_fh (fd);
@@ -3283,16 +3286,16 @@ target_fileio_pwrite (int fd, const gdb_byte *write_buf, int len,
     ret = fh->target->fileio_pwrite (fh->target_fd, write_buf,
 				     len, offset, target_errno);
 
-  target_debug_printf_nofunc ("target_fileio_pwrite (%d,...,%d,%s) = %d (%d)", fd,
-		       len, pulongest (offset), ret,
-		       ret != -1 ? 0 : *target_errno);
+  target_debug_printf_nofunc ("target_fileio_pwrite (%d,...,%d,%s) = %d (%d)",
+			      int (fd), len, pulongest (offset), ret,
+			      ret != -1 ? 0 : *target_errno);
   return ret;
 }
 
 /* See target.h.  */
 
 int
-target_fileio_pread (int fd, gdb_byte *read_buf, int len,
+target_fileio_pread (target_fd fd, gdb_byte *read_buf, int len,
 		     ULONGEST offset, fileio_error *target_errno)
 {
   fileio_fh_t *fh = fileio_fd_to_fh (fd);
@@ -3306,15 +3309,16 @@ target_fileio_pread (int fd, gdb_byte *read_buf, int len,
     ret = fh->target->fileio_pread (fh->target_fd, read_buf,
 				    len, offset, target_errno);
 
-  target_debug_printf_nofunc ("target_fileio_pread (%d,...,%d,%s) = %d (%d)", fd, len,
-		       pulongest (offset), ret, ret != -1 ? 0 : *target_errno);
+  target_debug_printf_nofunc ("target_fileio_pread (%d,...,%d,%s) = %d (%d)",
+			      int (fd), len, pulongest (offset), ret,
+			      ret != -1 ? 0 : *target_errno);
   return ret;
 }
 
 /* See target.h.  */
 
 int
-target_fileio_fstat (int fd, struct stat *sb, fileio_error *target_errno)
+target_fileio_fstat (target_fd fd, struct stat *sb, fileio_error *target_errno)
 {
   fileio_fh_t *fh = fileio_fd_to_fh (fd);
   int ret = -1;
@@ -3326,8 +3330,8 @@ target_fileio_fstat (int fd, struct stat *sb, fileio_error *target_errno)
   else
     ret = fh->target->fileio_fstat (fh->target_fd, sb, target_errno);
 
-  target_debug_printf_nofunc ("target_fileio_fstat (%d) = %d (%d)", fd, ret,
-		       ret != -1 ? 0 : *target_errno);
+  target_debug_printf_nofunc ("target_fileio_fstat (%d) = %d (%d)",
+			      int (fd), ret, ret != -1 ? 0 : *target_errno);
   return ret;
 }
 
@@ -3357,7 +3361,7 @@ target_fileio_lstat (struct inferior *inf, const char *filename,
 /* See target.h.  */
 
 int
-target_fileio_close (int fd, fileio_error *target_errno)
+target_fileio_close (target_fd fd, fileio_error *target_errno)
 {
   fileio_fh_t *fh = fileio_fd_to_fh (fd);
   int ret = -1;
@@ -3374,8 +3378,8 @@ target_fileio_close (int fd, fileio_error *target_errno)
       release_fileio_fd (fd, fh);
     }
 
-  target_debug_printf_nofunc ("target_fileio_close (%d) = %d (%d)", fd, ret,
-		       ret != -1 ? 0 : *target_errno);
+  target_debug_printf_nofunc ("target_fileio_close (%d) = %d (%d)",
+			      int (fd), ret, ret != -1 ? 0 : *target_errno);
   return ret;
 }
 
@@ -3432,14 +3436,14 @@ target_fileio_readlink (struct inferior *inf, const char *filename,
 class scoped_target_fd
 {
 public:
-  explicit scoped_target_fd (int fd) noexcept
+  explicit scoped_target_fd (target_fd fd) noexcept
     : m_fd (fd)
   {
   }
 
   ~scoped_target_fd ()
   {
-    if (m_fd >= 0)
+    if (m_fd != target_fd::INVALID)
       {
 	fileio_error target_errno;
 
@@ -3449,13 +3453,13 @@ class scoped_target_fd
 
   DISABLE_COPY_AND_ASSIGN (scoped_target_fd);
 
-  int get () const noexcept
+  target_fd get () const noexcept
   {
     return m_fd;
   }
 
 private:
-  int m_fd;
+  target_fd m_fd;
 };
 
 /* Read target file FILENAME, in the filesystem as seen by INF.  If
@@ -3477,7 +3481,7 @@ target_fileio_read_alloc_1 (struct inferior *inf, const char *filename,
 
   scoped_target_fd fd (target_fileio_open (inf, filename, FILEIO_O_RDONLY,
 					   0700, false, &target_errno));
-  if (fd.get () == -1)
+  if (fd.get () == target_fd::INVALID)
     return -1;
 
   /* Start by reading up to 4K at a time.  The target will throttle
diff --git a/gdb/target.h b/gdb/target.h
index 4db9c3c7d98..f76ac09ee82 100644
--- a/gdb/target.h
+++ b/gdb/target.h
@@ -2250,33 +2250,41 @@ extern int target_search_memory (CORE_ADDR start_addr,
 
 extern bool target_filesystem_is_local ();
 
+/* A remote file descriptor is just an integer, but we use a separate
+   type to avoid confusion with local file descriptors.  */
+enum class target_fd : int
+{
+  INVALID = -1,
+};
+
 /* Open FILENAME on the target, in the filesystem as seen by INF,
    using FLAGS and MODE.  If INF is NULL, use the filesystem seen by
    the debugger (GDB or, for remote targets, the remote stub).  Return
    a target file descriptor, or -1 if an error occurs (and set
    *TARGET_ERRNO).  If WARN_IF_SLOW is true, print a warning message
    if the file is being accessed over a link that may be slow.  */
-extern int target_fileio_open (struct inferior *inf,
-			       const char *filename, int flags,
-			       int mode, bool warn_if_slow,
-			       fileio_error *target_errno);
+extern target_fd target_fileio_open (struct inferior *inf,
+				     const char *filename, int flags,
+				     int mode, bool warn_if_slow,
+				     fileio_error *target_errno);
 
 /* Write up to LEN bytes from WRITE_BUF to FD on the target.
    Return the number of bytes written, or -1 if an error occurs
    (and set *TARGET_ERRNO).  */
-extern int target_fileio_pwrite (int fd, const gdb_byte *write_buf, int len,
-				 ULONGEST offset, fileio_error *target_errno);
+extern int target_fileio_pwrite (target_fd fd, const gdb_byte *write_buf,
+				 int len, ULONGEST offset,
+				 fileio_error *target_errno);
 
 /* Read up to LEN bytes FD on the target into READ_BUF.
    Return the number of bytes read, or -1 if an error occurs
    (and set *TARGET_ERRNO).  */
-extern int target_fileio_pread (int fd, gdb_byte *read_buf, int len,
+extern int target_fileio_pread (target_fd fd, gdb_byte *read_buf, int len,
 				ULONGEST offset, fileio_error *target_errno);
 
 /* Get information about the file opened as FD on the target
    and put it in SB.  Return 0 on success, or -1 if an error
    occurs (and set *TARGET_ERRNO).  */
-extern int target_fileio_fstat (int fd, struct stat *sb,
+extern int target_fileio_fstat (target_fd fd, struct stat *sb,
 				fileio_error *target_errno);
 
 /* Get information about the file at FILENAME on the target and put it in
@@ -2289,7 +2297,7 @@ extern int target_fileio_lstat (struct inferior *inf, const char *filename,
 
 /* Close FD on the target.  Return 0, or -1 if an error occurs
    (and set *TARGET_ERRNO).  */
-extern int target_fileio_close (int fd, fileio_error *target_errno);
+extern int target_fileio_close (target_fd fd, fileio_error *target_errno);
 
 /* Unlink FILENAME on the target, in the filesystem as seen by INF.
    If INF is NULL, use the filesystem seen by the debugger (GDB or,

-- 
2.53.0