[PATCH 0/6] Make remote-fileio per-target

[PATCH 0/6] Make remote-fileio per-target

[Tom Tromey]

I noticed that the remote-fileio code uses a global.  It seemed to me
that this should be per-target instead.

After writing this I noticed that gdbserver does not seem to implement
any of these packets.  So, I don't think this can really be tested.

---
Tom Tromey (6):
      Make remote_fio_func_map 'const'
      Use vector in remote-fileio.c
      Use methods for remote fileio
      Remove sentinel from remote_fio_func_map
      Move remote_fileio_data to header file
      Store remote fileio state in remote_state

 gdb/remote-fileio.c | 248 ++++++++++++++++++++++++----------------------------
 gdb/remote-fileio.h |  42 +++++++--
 gdb/remote.c        |   8 +-
 3 files changed, 153 insertions(+), 145 deletions(-)
---
base-commit: 276e7f5c8835cd300ee75d00556ab8839a30b9ef
change-id: 20231231-remote-fileio-24fbff0ba416

Best regards,
-- 
Tom Tromey <tom@tromey.com>

[PATCH 1/6] Make remote_fio_func_map 'const'

[Tom Tromey]

This changes remote_fio_func_map to be const, letting it move into the
readonly data section.
---
 gdb/remote-fileio.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gdb/remote-fileio.c b/gdb/remote-fileio.c
index 10bc86c1ba5..367edb54f74 100644
--- a/gdb/remote-fileio.c
+++ b/gdb/remote-fileio.c
@@ -1101,7 +1101,7 @@ remote_fileio_func_system (remote_target *remote, char *buf)
     remote_fileio_return_success (remote, WEXITSTATUS (ret));
 }
 
-static struct {
+static const struct {
   const char *name;
   void (*func)(remote_target *remote, char *);
 } remote_fio_func_map[] = {

-- 
2.43.0

[PATCH 2/6] Use vector in remote-fileio.c

[Tom Tromey]

This changes remote_fio_data to be an object holding a vector.  This
simplifies the code somewhat.
---
 gdb/remote-fileio.c | 51 +++++++++++++++++----------------------------------
 1 file changed, 17 insertions(+), 34 deletions(-)

diff --git a/gdb/remote-fileio.c b/gdb/remote-fileio.c
index 367edb54f74..4891b0c5b92 100644
--- a/gdb/remote-fileio.c
+++ b/gdb/remote-fileio.c
@@ -37,9 +37,9 @@
 #endif
 #include <signal.h>
 
-static struct {
-  int *fd_map;
-  int fd_map_size;
+static struct
+{
+  std::vector<int> fd_map;
 } remote_fio_data;
 
 #define FIO_FD_INVALID		-1
@@ -51,16 +51,13 @@ static int remote_fio_system_call_allowed = 0;
 static int
 remote_fileio_init_fd_map (void)
 {
-  int i;
-
-  if (!remote_fio_data.fd_map)
+  if (remote_fio_data.fd_map.empty ())
     {
-      remote_fio_data.fd_map = XNEWVEC (int, 10);
-      remote_fio_data.fd_map_size = 10;
+      remote_fio_data.fd_map.resize (10);
       remote_fio_data.fd_map[0] = FIO_FD_CONSOLE_IN;
       remote_fio_data.fd_map[1] = FIO_FD_CONSOLE_OUT;
       remote_fio_data.fd_map[2] = FIO_FD_CONSOLE_OUT;
-      for (i = 3; i < 10; ++i)
+      for (int i = 3; i < 10; ++i)
 	remote_fio_data.fd_map[i] = FIO_FD_INVALID;
     }
   return 3;
@@ -69,25 +66,20 @@ remote_fileio_init_fd_map (void)
 static int
 remote_fileio_resize_fd_map (void)
 {
-  int i = remote_fio_data.fd_map_size;
-
-  if (!remote_fio_data.fd_map)
+  if (remote_fio_data.fd_map.empty ())
     return remote_fileio_init_fd_map ();
-  remote_fio_data.fd_map_size += 10;
-  remote_fio_data.fd_map =
-    (int *) xrealloc (remote_fio_data.fd_map,
-		      remote_fio_data.fd_map_size * sizeof (int));
-  for (; i < remote_fio_data.fd_map_size; i++)
+
+  int i = remote_fio_data.fd_map.size ();
+  remote_fio_data.fd_map.resize (i + 10);
+  for (; i < remote_fio_data.fd_map.size (); i++)
     remote_fio_data.fd_map[i] = FIO_FD_INVALID;
-  return remote_fio_data.fd_map_size - 10;
+  return remote_fio_data.fd_map.size () - 10;
 }
 
 static int
 remote_fileio_next_free_fd (void)
 {
-  int i;
-
-  for (i = 0; i < remote_fio_data.fd_map_size; ++i)
+  for (int i = 0; i < remote_fio_data.fd_map.size (); ++i)
     if (remote_fio_data.fd_map[i] == FIO_FD_INVALID)
       return i;
   return remote_fileio_resize_fd_map ();
@@ -106,7 +98,7 @@ static int
 remote_fileio_map_fd (int target_fd)
 {
   remote_fileio_init_fd_map ();
-  if (target_fd < 0 || target_fd >= remote_fio_data.fd_map_size)
+  if (target_fd < 0 || target_fd >= remote_fio_data.fd_map.size ())
     return FIO_FD_INVALID;
   return remote_fio_data.fd_map[target_fd];
 }
@@ -115,7 +107,7 @@ static void
 remote_fileio_close_target_fd (int target_fd)
 {
   remote_fileio_init_fd_map ();
-  if (target_fd >= 0 && target_fd < remote_fio_data.fd_map_size)
+  if (target_fd >= 0 && target_fd < remote_fio_data.fd_map.size ())
     remote_fio_data.fd_map[target_fd] = FIO_FD_INVALID;
 }
 
@@ -1147,21 +1139,12 @@ do_remote_fileio_request (remote_target *remote, char *buf)
 void
 remote_fileio_reset (void)
 {
-  int ix;
-
-  for (ix = 0; ix != remote_fio_data.fd_map_size; ix++)
+  for (int fd : remote_fio_data.fd_map)
     {
-      int fd = remote_fio_data.fd_map[ix];
-
       if (fd >= 0)
 	close (fd);
     }
-  if (remote_fio_data.fd_map)
-    {
-      xfree (remote_fio_data.fd_map);
-      remote_fio_data.fd_map = NULL;
-      remote_fio_data.fd_map_size = 0;
-    }
+  remote_fio_data.fd_map.clear ();
 }
 
 /* Handle a file I/O request.  BUF points to the packet containing the

-- 
2.43.0

Re: [PATCH 2/6] Use vector in remote-fileio.c

[Lancelot SIX]

Hi Tom,

I have a couple of comments below.

On Sun, Dec 31, 2023 at 01:25:39PM -0700, Tom Tromey wrote:
> This changes remote_fio_data to be an object holding a vector.  This
> simplifies the code somewhat.
> ---
>  gdb/remote-fileio.c | 51 +++++++++++++++++----------------------------------
>  1 file changed, 17 insertions(+), 34 deletions(-)
> 
> diff --git a/gdb/remote-fileio.c b/gdb/remote-fileio.c
> index 367edb54f74..4891b0c5b92 100644
> --- a/gdb/remote-fileio.c
> +++ b/gdb/remote-fileio.c
> @@ -37,9 +37,9 @@
>  #endif
>  #include <signal.h>
>  
> -static struct {
> -  int *fd_map;
> -  int fd_map_size;
> +static struct
> +{
> +  std::vector<int> fd_map;
>  } remote_fio_data;
>  
>  #define FIO_FD_INVALID		-1
> @@ -51,16 +51,13 @@ static int remote_fio_system_call_allowed = 0;
>  static int
>  remote_fileio_init_fd_map (void)
>  {
> -  int i;
> -
> -  if (!remote_fio_data.fd_map)
> +  if (remote_fio_data.fd_map.empty ())
>      {
> -      remote_fio_data.fd_map = XNEWVEC (int, 10);
> -      remote_fio_data.fd_map_size = 10;
> +      remote_fio_data.fd_map.resize (10);
>        remote_fio_data.fd_map[0] = FIO_FD_CONSOLE_IN;
>        remote_fio_data.fd_map[1] = FIO_FD_CONSOLE_OUT;
>        remote_fio_data.fd_map[2] = FIO_FD_CONSOLE_OUT;
> -      for (i = 3; i < 10; ++i)
> +      for (int i = 3; i < 10; ++i)
>  	remote_fio_data.fd_map[i] = FIO_FD_INVALID;

std::vector<T, Alloc>::resize has an overload which takes a value to use
for new elements of the vector.   This could be simplified as

    remote_fio_data.fd_map.resize (10, FIO_FD_INVALID);
    remote_fio_data.fd_map[0] = FIO_FD_CONSOLE_IN;
    remote_fio_data.fd_map[1] = FIO_FD_CONSOLE_OUT;
    remote_fio_data.fd_map[2] = FIO_FD_CONSOLE_OUT;

See below, but overall, I don’t really think we need to keep the "grow
by 10" strategy when using std::vector, this could simply be

    remote_fio_data.fd_map.emplace_back (FIO_FD_CONSOLE_IN)
    remote_fio_data.fd_map.emplace_back (FIO_FD_CONSOLE_OUT)
    remote_fio_data.fd_map.emplace_back (FIO_FD_CONSOLE_OUT)

>      }
>    return 3;
> @@ -69,25 +66,20 @@ remote_fileio_init_fd_map (void)
>  static int
>  remote_fileio_resize_fd_map (void)
>  {
> -  int i = remote_fio_data.fd_map_size;
> -
> -  if (!remote_fio_data.fd_map)
> +  if (remote_fio_data.fd_map.empty ())
>      return remote_fileio_init_fd_map ();
> -  remote_fio_data.fd_map_size += 10;
> -  remote_fio_data.fd_map =
> -    (int *) xrealloc (remote_fio_data.fd_map,
> -		      remote_fio_data.fd_map_size * sizeof (int));
> -  for (; i < remote_fio_data.fd_map_size; i++)
> +
> +  int i = remote_fio_data.fd_map.size ();
> +  remote_fio_data.fd_map.resize (i + 10);
> +  for (; i < remote_fio_data.fd_map.size (); i++)
>      remote_fio_data.fd_map[i] = FIO_FD_INVALID;

Similarly, if you keep the "grow by 10" strategy this could be

     remote_fio_data.fd_map.resize (i + 10, FIO_FD_INVALID);

> -  return remote_fio_data.fd_map_size - 10;
> +  return remote_fio_data.fd_map.size () - 10;
>  }
>  
>  static int
>  remote_fileio_next_free_fd (void)
>  {
> -  int i;
> -
> -  for (i = 0; i < remote_fio_data.fd_map_size; ++i)
> +  for (int i = 0; i < remote_fio_data.fd_map.size (); ++i)
>      if (remote_fio_data.fd_map[i] == FIO_FD_INVALID)
>        return i;

I think that’s mostly a personal preference thing, but std::find could
be used instead of an explicit loop

    auto it = std::find (remote_fio_data.fd_map.begin (),
                         remote_fio_data.fd_map.end (),
    		         FIO_FD_INVALID);
    if (it != remote_fio_data.fd_map.end ())
      return std::distance (remote_fio_data.fd_map.begin (), it);
    return remote_fileio_resize_fd_map ();

I think it could be possible to go one step further and remove
remote_fileio_resize_fd_map entirely.  The "grow by 10" strategy is not
necessary anymore if you use a std::vector.  A similar allocation
strategies comes for free.

    static int
    remote_fileio_next_free_fd ()
    {
        auto it = std::find (remote_fio_data.fd_map.begin (),
                             remote_fio_data.fd_map.end (),
        		         FIO_FD_INVALID);
        if (it != remote_fio_data.fd_map.end ())
          return std::ditance (remote_fio_data.fd_map.begin (), it);

        remote_fio_data.fd_map.emplace_back (FIO_FD_INVALID);
	return remote_fio_data.fd_map.size () - 1;
    }

One could also inline this inside remote_fileio_map_fd, and directly set
the appropriate FD value instead of using FIO_FD_INVALID temporarily.
But I would be happy keeping things this way for readability.

Best,
Lancelot.

>    return remote_fileio_resize_fd_map ();
> @@ -106,7 +98,7 @@ static int
>  remote_fileio_map_fd (int target_fd)
>  {
>    remote_fileio_init_fd_map ();
> -  if (target_fd < 0 || target_fd >= remote_fio_data.fd_map_size)
> +  if (target_fd < 0 || target_fd >= remote_fio_data.fd_map.size ())
>      return FIO_FD_INVALID;
>    return remote_fio_data.fd_map[target_fd];
>  }
> @@ -115,7 +107,7 @@ static void
>  remote_fileio_close_target_fd (int target_fd)
>  {
>    remote_fileio_init_fd_map ();
> -  if (target_fd >= 0 && target_fd < remote_fio_data.fd_map_size)
> +  if (target_fd >= 0 && target_fd < remote_fio_data.fd_map.size ())
>      remote_fio_data.fd_map[target_fd] = FIO_FD_INVALID;
>  }
>  
> @@ -1147,21 +1139,12 @@ do_remote_fileio_request (remote_target *remote, char *buf)
>  void
>  remote_fileio_reset (void)
>  {
> -  int ix;
> -
> -  for (ix = 0; ix != remote_fio_data.fd_map_size; ix++)
> +  for (int fd : remote_fio_data.fd_map)
>      {
> -      int fd = remote_fio_data.fd_map[ix];
> -
>        if (fd >= 0)
>  	close (fd);
>      }
> -  if (remote_fio_data.fd_map)
> -    {
> -      xfree (remote_fio_data.fd_map);
> -      remote_fio_data.fd_map = NULL;
> -      remote_fio_data.fd_map_size = 0;
> -    }
> +  remote_fio_data.fd_map.clear ();
>  }
>  
>  /* Handle a file I/O request.  BUF points to the packet containing the
> 
> -- 
> 2.43.0
> 

Re: [PATCH 2/6] Use vector in remote-fileio.c

[Simon Marchi]

On 2024-01-01 09:20, Lancelot SIX wrote:
> Hi Tom,
> 
> I have a couple of comments below.
> 
> On Sun, Dec 31, 2023 at 01:25:39PM -0700, Tom Tromey wrote:
>> This changes remote_fio_data to be an object holding a vector.  This
>> simplifies the code somewhat.
>> ---
>>  gdb/remote-fileio.c | 51 +++++++++++++++++----------------------------------
>>  1 file changed, 17 insertions(+), 34 deletions(-)
>>
>> diff --git a/gdb/remote-fileio.c b/gdb/remote-fileio.c
>> index 367edb54f74..4891b0c5b92 100644
>> --- a/gdb/remote-fileio.c
>> +++ b/gdb/remote-fileio.c
>> @@ -37,9 +37,9 @@
>>  #endif
>>  #include <signal.h>
>>  
>> -static struct {
>> -  int *fd_map;
>> -  int fd_map_size;
>> +static struct
>> +{
>> +  std::vector<int> fd_map;
>>  } remote_fio_data;
>>  
>>  #define FIO_FD_INVALID		-1
>> @@ -51,16 +51,13 @@ static int remote_fio_system_call_allowed = 0;
>>  static int
>>  remote_fileio_init_fd_map (void)
>>  {
>> -  int i;
>> -
>> -  if (!remote_fio_data.fd_map)
>> +  if (remote_fio_data.fd_map.empty ())
>>      {
>> -      remote_fio_data.fd_map = XNEWVEC (int, 10);
>> -      remote_fio_data.fd_map_size = 10;
>> +      remote_fio_data.fd_map.resize (10);
>>        remote_fio_data.fd_map[0] = FIO_FD_CONSOLE_IN;
>>        remote_fio_data.fd_map[1] = FIO_FD_CONSOLE_OUT;
>>        remote_fio_data.fd_map[2] = FIO_FD_CONSOLE_OUT;
>> -      for (i = 3; i < 10; ++i)
>> +      for (int i = 3; i < 10; ++i)
>>  	remote_fio_data.fd_map[i] = FIO_FD_INVALID;
> 
> std::vector<T, Alloc>::resize has an overload which takes a value to use
> for new elements of the vector.   This could be simplified as
> 
>     remote_fio_data.fd_map.resize (10, FIO_FD_INVALID);
>     remote_fio_data.fd_map[0] = FIO_FD_CONSOLE_IN;
>     remote_fio_data.fd_map[1] = FIO_FD_CONSOLE_OUT;
>     remote_fio_data.fd_map[2] = FIO_FD_CONSOLE_OUT;
> 
> See below, but overall, I don’t really think we need to keep the "grow
> by 10" strategy when using std::vector, this could simply be
> 
>     remote_fio_data.fd_map.emplace_back (FIO_FD_CONSOLE_IN)
>     remote_fio_data.fd_map.emplace_back (FIO_FD_CONSOLE_OUT)
>     remote_fio_data.fd_map.emplace_back (FIO_FD_CONSOLE_OUT)

I agree with Lancelot.  And from what I saw after quickly skimming the
code, remote_fileio_resize_fd_map and remote_fileio_next_free_fd could
be removed, since they callers can use simple vector operations.

Simon

[PATCH 3/6] Use methods for remote fileio

[Tom Tromey]

This changes various functions in remote-fileio.c to be methods on the
state object.
---
 gdb/remote-fileio.c | 248 +++++++++++++++++++++++++++++++---------------------
 1 file changed, 146 insertions(+), 102 deletions(-)

diff --git a/gdb/remote-fileio.c b/gdb/remote-fileio.c
index 4891b0c5b92..f6990989da6 100644
--- a/gdb/remote-fileio.c
+++ b/gdb/remote-fileio.c
@@ -37,9 +37,40 @@
 #endif
 #include <signal.h>
 
-static struct
+static struct remote_fileio_data
 {
-  std::vector<int> fd_map;
+public:
+
+  void request (remote_target *remote,
+		char *buf, int ctrlc_pending_p);
+
+  void reset ();
+
+private:
+
+  int fd_to_targetfd (int fd);
+  int map_fd (int target_fd);
+  void close_target_fd (int target_fd);
+
+  void func_open (remote_target *remote, char *buf);
+  void func_close (remote_target *remote, char *buf);
+  void func_read (remote_target *remote, char *buf);
+  void func_write (remote_target *remote, char *buf);
+  void func_lseek (remote_target *remote, char *buf);
+  void func_rename (remote_target *remote, char *buf);
+  void func_unlink (remote_target *remote, char *buf);
+  void func_stat (remote_target *remote, char *buf);
+  void func_fstat (remote_target *remote, char *buf);
+  void func_gettimeofday (remote_target *remote, char *buf);
+  void func_isatty (remote_target *remote, char *buf);
+  void func_system (remote_target *remote, char *buf);
+  void do_request (remote_target *remote, char *buf);
+
+  int init_fd_map ();
+  int resize_fd_map ();
+  int next_free_fd ();
+
+  std::vector<int> m_fd_map;
 } remote_fio_data;
 
 #define FIO_FD_INVALID		-1
@@ -48,67 +79,78 @@ static struct
 
 static int remote_fio_system_call_allowed = 0;
 
-static int
-remote_fileio_init_fd_map (void)
+int
+remote_fileio_data::init_fd_map ()
 {
-  if (remote_fio_data.fd_map.empty ())
+  if (m_fd_map.empty ())
     {
-      remote_fio_data.fd_map.resize (10);
-      remote_fio_data.fd_map[0] = FIO_FD_CONSOLE_IN;
-      remote_fio_data.fd_map[1] = FIO_FD_CONSOLE_OUT;
-      remote_fio_data.fd_map[2] = FIO_FD_CONSOLE_OUT;
+      m_fd_map.resize (10);
+      m_fd_map[0] = FIO_FD_CONSOLE_IN;
+      m_fd_map[1] = FIO_FD_CONSOLE_OUT;
+      m_fd_map[2] = FIO_FD_CONSOLE_OUT;
       for (int i = 3; i < 10; ++i)
-	remote_fio_data.fd_map[i] = FIO_FD_INVALID;
+	m_fd_map[i] = FIO_FD_INVALID;
     }
   return 3;
 }
 
-static int
-remote_fileio_resize_fd_map (void)
+int
+remote_fileio_data::resize_fd_map ()
 {
-  if (remote_fio_data.fd_map.empty ())
-    return remote_fileio_init_fd_map ();
-
-  int i = remote_fio_data.fd_map.size ();
-  remote_fio_data.fd_map.resize (i + 10);
-  for (; i < remote_fio_data.fd_map.size (); i++)
-    remote_fio_data.fd_map[i] = FIO_FD_INVALID;
-  return remote_fio_data.fd_map.size () - 10;
+  if (m_fd_map.empty ())
+    return init_fd_map ();
+
+  int i = m_fd_map.size ();
+  m_fd_map.resize (i + 10);
+  for (; i < m_fd_map.size (); i++)
+    m_fd_map[i] = FIO_FD_INVALID;
+  return m_fd_map.size () - 10;
 }
 
-static int
-remote_fileio_next_free_fd (void)
+int
+remote_fileio_data::next_free_fd ()
 {
-  for (int i = 0; i < remote_fio_data.fd_map.size (); ++i)
-    if (remote_fio_data.fd_map[i] == FIO_FD_INVALID)
+  for (int i = 0; i < m_fd_map.size (); ++i)
+    if (m_fd_map[i] == FIO_FD_INVALID)
       return i;
-  return remote_fileio_resize_fd_map ();
+  return resize_fd_map ();
 }
 
-static int
-remote_fileio_fd_to_targetfd (int fd)
+int
+remote_fileio_data::fd_to_targetfd (int fd)
 {
-  int target_fd = remote_fileio_next_free_fd ();
+  int target_fd = next_free_fd ();
 
-  remote_fio_data.fd_map[target_fd] = fd;
+  m_fd_map[target_fd] = fd;
   return target_fd;
 }
 
-static int
-remote_fileio_map_fd (int target_fd)
+int
+remote_fileio_data::map_fd (int target_fd)
 {
-  remote_fileio_init_fd_map ();
-  if (target_fd < 0 || target_fd >= remote_fio_data.fd_map.size ())
+  init_fd_map ();
+  if (target_fd < 0 || target_fd >= m_fd_map.size ())
     return FIO_FD_INVALID;
-  return remote_fio_data.fd_map[target_fd];
+  return m_fd_map[target_fd];
 }
 
-static void
-remote_fileio_close_target_fd (int target_fd)
+void
+remote_fileio_data::close_target_fd (int target_fd)
 {
-  remote_fileio_init_fd_map ();
-  if (target_fd >= 0 && target_fd < remote_fio_data.fd_map.size ())
-    remote_fio_data.fd_map[target_fd] = FIO_FD_INVALID;
+  init_fd_map ();
+  if (target_fd >= 0 && target_fd < m_fd_map.size ())
+    m_fd_map[target_fd] = FIO_FD_INVALID;
+}
+
+void
+remote_fileio_data::reset ()
+{
+  for (int fd : m_fd_map)
+    {
+      if (fd >= 0)
+	close (fd);
+    }
+  m_fd_map.clear ();
 }
 
 static int
@@ -359,8 +401,8 @@ remote_fileio_return_success (remote_target *remote, int retcode)
   remote_fileio_reply (remote, retcode, 0);
 }
 
-static void
-remote_fileio_func_open (remote_target *remote, char *buf)
+void
+remote_fileio_data::func_open (remote_target *remote, char *buf)
 {
   CORE_ADDR ptrval;
   int length;
@@ -424,12 +466,12 @@ remote_fileio_func_open (remote_target *remote, char *buf)
       return;
     }
 
-  fd = remote_fileio_fd_to_targetfd (fd);
+  fd = fd_to_targetfd (fd);
   remote_fileio_return_success (remote, fd);
 }
 
-static void
-remote_fileio_func_close (remote_target *remote, char *buf)
+void
+remote_fileio_data::func_close (remote_target *remote, char *buf)
 {
   long num;
   int fd;
@@ -440,7 +482,7 @@ remote_fileio_func_close (remote_target *remote, char *buf)
       remote_fileio_ioerror (remote);
       return;
     }
-  fd = remote_fileio_map_fd ((int) num);
+  fd = map_fd ((int) num);
   if (fd == FIO_FD_INVALID)
     {
       remote_fileio_badfd (remote);
@@ -449,12 +491,12 @@ remote_fileio_func_close (remote_target *remote, char *buf)
 
   if (fd != FIO_FD_CONSOLE_IN && fd != FIO_FD_CONSOLE_OUT && close (fd))
     remote_fileio_return_errno (remote, -1);
-  remote_fileio_close_target_fd ((int) num);
+  close_target_fd ((int) num);
   remote_fileio_return_success (remote, 0);
 }
 
-static void
-remote_fileio_func_read (remote_target *remote, char *buf)
+void
+remote_fileio_data::func_read (remote_target *remote, char *buf)
 {
   long target_fd, num;
   LONGEST lnum;
@@ -470,7 +512,7 @@ remote_fileio_func_read (remote_target *remote, char *buf)
       remote_fileio_ioerror (remote);
       return;
     }
-  fd = remote_fileio_map_fd ((int) target_fd);
+  fd = map_fd ((int) target_fd);
   if (fd == FIO_FD_INVALID)
     {
       remote_fileio_badfd (remote);
@@ -579,8 +621,8 @@ remote_fileio_func_read (remote_target *remote, char *buf)
   xfree (buffer);
 }
 
-static void
-remote_fileio_func_write (remote_target *remote, char *buf)
+void
+remote_fileio_data::func_write (remote_target *remote, char *buf)
 {
   long target_fd, num;
   LONGEST lnum;
@@ -595,7 +637,7 @@ remote_fileio_func_write (remote_target *remote, char *buf)
       remote_fileio_ioerror (remote);
       return;
     }
-  fd = remote_fileio_map_fd ((int) target_fd);
+  fd = map_fd ((int) target_fd);
   if (fd == FIO_FD_INVALID)
     {
       remote_fileio_badfd (remote);
@@ -654,8 +696,8 @@ remote_fileio_func_write (remote_target *remote, char *buf)
   xfree (buffer);
 }
 
-static void
-remote_fileio_func_lseek (remote_target *remote, char *buf)
+void
+remote_fileio_data::func_lseek (remote_target *remote, char *buf)
 {
   long num;
   LONGEST lnum;
@@ -668,7 +710,7 @@ remote_fileio_func_lseek (remote_target *remote, char *buf)
       remote_fileio_ioerror (remote);
       return;
     }
-  fd = remote_fileio_map_fd ((int) num);
+  fd = map_fd ((int) num);
   if (fd == FIO_FD_INVALID)
     {
       remote_fileio_badfd (remote);
@@ -707,8 +749,8 @@ remote_fileio_func_lseek (remote_target *remote, char *buf)
     remote_fileio_return_success (remote, ret);
 }
 
-static void
-remote_fileio_func_rename (remote_target *remote, char *buf)
+void
+remote_fileio_data::func_rename (remote_target *remote, char *buf)
 {
   CORE_ADDR old_ptr, new_ptr;
   int old_len, new_len;
@@ -800,8 +842,8 @@ remote_fileio_func_rename (remote_target *remote, char *buf)
     remote_fileio_return_success (remote, ret);
 }
 
-static void
-remote_fileio_func_unlink (remote_target *remote, char *buf)
+void
+remote_fileio_data::func_unlink (remote_target *remote, char *buf)
 {
   CORE_ADDR ptrval;
   int length;
@@ -839,8 +881,8 @@ remote_fileio_func_unlink (remote_target *remote, char *buf)
     remote_fileio_return_success (remote, ret);
 }
 
-static void
-remote_fileio_func_stat (remote_target *remote, char *buf)
+void
+remote_fileio_data::func_stat (remote_target *remote, char *buf)
 {
   CORE_ADDR statptr, nameptr;
   int ret, namelength;
@@ -900,8 +942,8 @@ remote_fileio_func_stat (remote_target *remote, char *buf)
   remote_fileio_return_success (remote, ret);
 }
 
-static void
-remote_fileio_func_fstat (remote_target *remote, char *buf)
+void
+remote_fileio_data::func_fstat (remote_target *remote, char *buf)
 {
   CORE_ADDR ptrval;
   int fd, ret;
@@ -917,7 +959,7 @@ remote_fileio_func_fstat (remote_target *remote, char *buf)
       remote_fileio_ioerror (remote);
       return;
     }
-  fd = remote_fileio_map_fd ((int) target_fd);
+  fd = map_fd ((int) target_fd);
   if (fd == FIO_FD_INVALID)
     {
       remote_fileio_badfd (remote);
@@ -977,8 +1019,8 @@ remote_fileio_func_fstat (remote_target *remote, char *buf)
   remote_fileio_return_success (remote, ret);
 }
 
-static void
-remote_fileio_func_gettimeofday (remote_target *remote, char *buf)
+void
+remote_fileio_data::func_gettimeofday (remote_target *remote, char *buf)
 {
   LONGEST lnum;
   CORE_ADDR ptrval;
@@ -1028,8 +1070,8 @@ remote_fileio_func_gettimeofday (remote_target *remote, char *buf)
   remote_fileio_return_success (remote, ret);
 }
 
-static void
-remote_fileio_func_isatty (remote_target *remote, char *buf)
+void
+remote_fileio_data::func_isatty (remote_target *remote, char *buf)
 {
   long target_fd;
   int fd;
@@ -1040,13 +1082,13 @@ remote_fileio_func_isatty (remote_target *remote, char *buf)
       remote_fileio_ioerror (remote);
       return;
     }
-  fd = remote_fileio_map_fd ((int) target_fd);
+  fd = map_fd ((int) target_fd);
   int ret = fd == FIO_FD_CONSOLE_IN || fd == FIO_FD_CONSOLE_OUT ? 1 : 0;
   remote_fileio_return_success (remote, ret);
 }
 
-static void
-remote_fileio_func_system (remote_target *remote, char *buf)
+void
+remote_fileio_data::func_system (remote_target *remote, char *buf)
 {
   CORE_ADDR ptrval;
   int ret, length;
@@ -1093,28 +1135,28 @@ remote_fileio_func_system (remote_target *remote, char *buf)
     remote_fileio_return_success (remote, WEXITSTATUS (ret));
 }
 
-static const struct {
-  const char *name;
-  void (*func)(remote_target *remote, char *);
-} remote_fio_func_map[] = {
-  { "open", remote_fileio_func_open },
-  { "close", remote_fileio_func_close },
-  { "read", remote_fileio_func_read },
-  { "write", remote_fileio_func_write },
-  { "lseek", remote_fileio_func_lseek },
-  { "rename", remote_fileio_func_rename },
-  { "unlink", remote_fileio_func_unlink },
-  { "stat", remote_fileio_func_stat },
-  { "fstat", remote_fileio_func_fstat },
-  { "gettimeofday", remote_fileio_func_gettimeofday },
-  { "isatty", remote_fileio_func_isatty },
-  { "system", remote_fileio_func_system },
-  { NULL, NULL }
-};
-
-static void
-do_remote_fileio_request (remote_target *remote, char *buf)
+void
+remote_fileio_data::do_request (remote_target *remote, char *buf)
 {
+  static const struct {
+    const char *name;
+    void (remote_fileio_data::*func)(remote_target *remote, char *);
+  } remote_fio_func_map[] = {
+    { "open", &remote_fileio_data::func_open },
+    { "close", &remote_fileio_data::func_close },
+    { "read", &remote_fileio_data::func_read },
+    { "write", &remote_fileio_data::func_write },
+    { "lseek", &remote_fileio_data::func_lseek },
+    { "rename", &remote_fileio_data::func_rename },
+    { "unlink", &remote_fileio_data::func_unlink },
+    { "stat", &remote_fileio_data::func_stat },
+    { "fstat", &remote_fileio_data::func_fstat },
+    { "gettimeofday", &remote_fileio_data::func_gettimeofday },
+    { "isatty", &remote_fileio_data::func_isatty },
+    { "system", &remote_fileio_data::func_system },
+    { nullptr, nullptr }
+  };
+
   char *c;
   int idx;
 
@@ -1131,20 +1173,15 @@ do_remote_fileio_request (remote_target *remote, char *buf)
   if (!remote_fio_func_map[idx].name)
     remote_fileio_reply (remote, -1, FILEIO_ENOSYS);
   else
-    remote_fio_func_map[idx].func (remote, c);
+    (this->*remote_fio_func_map[idx].func) (remote, c);
 }
 
 /* Close any open descriptors, and reinitialize the file mapping.  */
 
 void
-remote_fileio_reset (void)
+remote_fileio_reset ()
 {
-  for (int fd : remote_fio_data.fd_map)
-    {
-      if (fd >= 0)
-	close (fd);
-    }
-  remote_fio_data.fd_map.clear ();
+  remote_fio_data.reset ();
 }
 
 /* Handle a file I/O request.  BUF points to the packet containing the
@@ -1152,7 +1189,8 @@ remote_fileio_reset (void)
    acknowledged the Ctrl-C sent asynchronously earlier.  */
 
 void
-remote_fileio_request (remote_target *remote, char *buf, int ctrlc_pending_p)
+remote_fileio_data::request (remote_target *remote,
+			     char *buf, int ctrlc_pending_p)
 {
   /* Save the previous quit handler, so we can restore it.  No need
      for a cleanup since we catch all exceptions below.  Note that the
@@ -1172,7 +1210,7 @@ remote_fileio_request (remote_target *remote, char *buf, int ctrlc_pending_p)
     {
       try
 	{
-	  do_remote_fileio_request (remote, buf);
+	  do_request (remote, buf);
 	}
       catch (const gdb_exception_forced_quit &ex)
 	{
@@ -1190,6 +1228,12 @@ remote_fileio_request (remote_target *remote, char *buf, int ctrlc_pending_p)
 
   quit_handler = remote_fileio_o_quit_handler;
 }
+
+void
+remote_fileio_request (remote_target *remote, char *buf, int ctrlc_pending_p)
+{
+  remote_fio_data.request (remote, buf, ctrlc_pending_p);
+}
 \f
 
 /* Unpack an fio_uint_t.  */

-- 
2.43.0

[PATCH 4/6] Remove sentinel from remote_fio_func_map

[Tom Tromey]

With foreach, a static array like remote_fio_func_map does not need a
sentinel entry.  This patch removes it and slightly rearranges the
code to suit.
---
 gdb/remote-fileio.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/gdb/remote-fileio.c b/gdb/remote-fileio.c
index f6990989da6..e001c220522 100644
--- a/gdb/remote-fileio.c
+++ b/gdb/remote-fileio.c
@@ -1154,11 +1154,9 @@ remote_fileio_data::do_request (remote_target *remote, char *buf)
     { "gettimeofday", &remote_fileio_data::func_gettimeofday },
     { "isatty", &remote_fileio_data::func_isatty },
     { "system", &remote_fileio_data::func_system },
-    { nullptr, nullptr }
   };
 
   char *c;
-  int idx;
 
   quit_handler = remote_fileio_quit_handler;
 
@@ -1167,13 +1165,14 @@ remote_fileio_data::do_request (remote_target *remote, char *buf)
     *c++ = '\0';
   else
     c = strchr (buf, '\0');
-  for (idx = 0; remote_fio_func_map[idx].name; ++idx)
-    if (!strcmp (remote_fio_func_map[idx].name, buf))
-      break;
-  if (!remote_fio_func_map[idx].name)
-    remote_fileio_reply (remote, -1, FILEIO_ENOSYS);
-  else
-    (this->*remote_fio_func_map[idx].func) (remote, c);
+  for (const auto &entry : remote_fio_func_map)
+    if (!strcmp (entry.name, buf))
+      {
+	(this->*entry.func) (remote, c);
+	return;
+      }
+	
+  remote_fileio_reply (remote, -1, FILEIO_ENOSYS);
 }
 
 /* Close any open descriptors, and reinitialize the file mapping.  */

-- 
2.43.0

[PATCH 5/6] Move remote_fileio_data to header file

[Tom Tromey]

This moves remote_fileio_data to remote-fileio.h, in preparation for
removing the global.
---
 gdb/remote-fileio.c | 36 +-----------------------------------
 gdb/remote-fileio.h | 38 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 39 insertions(+), 35 deletions(-)

diff --git a/gdb/remote-fileio.c b/gdb/remote-fileio.c
index e001c220522..a954ab1bb55 100644
--- a/gdb/remote-fileio.c
+++ b/gdb/remote-fileio.c
@@ -37,41 +37,7 @@
 #endif
 #include <signal.h>
 
-static struct remote_fileio_data
-{
-public:
-
-  void request (remote_target *remote,
-		char *buf, int ctrlc_pending_p);
-
-  void reset ();
-
-private:
-
-  int fd_to_targetfd (int fd);
-  int map_fd (int target_fd);
-  void close_target_fd (int target_fd);
-
-  void func_open (remote_target *remote, char *buf);
-  void func_close (remote_target *remote, char *buf);
-  void func_read (remote_target *remote, char *buf);
-  void func_write (remote_target *remote, char *buf);
-  void func_lseek (remote_target *remote, char *buf);
-  void func_rename (remote_target *remote, char *buf);
-  void func_unlink (remote_target *remote, char *buf);
-  void func_stat (remote_target *remote, char *buf);
-  void func_fstat (remote_target *remote, char *buf);
-  void func_gettimeofday (remote_target *remote, char *buf);
-  void func_isatty (remote_target *remote, char *buf);
-  void func_system (remote_target *remote, char *buf);
-  void do_request (remote_target *remote, char *buf);
-
-  int init_fd_map ();
-  int resize_fd_map ();
-  int next_free_fd ();
-
-  std::vector<int> m_fd_map;
-} remote_fio_data;
+static remote_fileio_data remote_fio_data;
 
 #define FIO_FD_INVALID		-1
 #define FIO_FD_CONSOLE_IN	-2
diff --git a/gdb/remote-fileio.h b/gdb/remote-fileio.h
index 71d85c618a9..c632c9e40f8 100644
--- a/gdb/remote-fileio.h
+++ b/gdb/remote-fileio.h
@@ -27,6 +27,44 @@
 struct cmd_list_element;
 struct remote_target;
 
+/* This holds the state needed by the remote fileio code.  */
+
+struct remote_fileio_data
+{
+public:
+
+  void request (remote_target *remote,
+		char *buf, int ctrlc_pending_p);
+
+  void reset ();
+
+private:
+
+  int fd_to_targetfd (int fd);
+  int map_fd (int target_fd);
+  void close_target_fd (int target_fd);
+
+  void func_open (remote_target *remote, char *buf);
+  void func_close (remote_target *remote, char *buf);
+  void func_read (remote_target *remote, char *buf);
+  void func_write (remote_target *remote, char *buf);
+  void func_lseek (remote_target *remote, char *buf);
+  void func_rename (remote_target *remote, char *buf);
+  void func_unlink (remote_target *remote, char *buf);
+  void func_stat (remote_target *remote, char *buf);
+  void func_fstat (remote_target *remote, char *buf);
+  void func_gettimeofday (remote_target *remote, char *buf);
+  void func_isatty (remote_target *remote, char *buf);
+  void func_system (remote_target *remote, char *buf);
+  void do_request (remote_target *remote, char *buf);
+
+  int init_fd_map ();
+  int resize_fd_map ();
+  int next_free_fd ();
+
+  std::vector<int> m_fd_map;
+};
+
 /* Unified interface to remote fileio, called in remote.c from
    remote_wait () and remote_async_wait ().  */
 extern void remote_fileio_request (remote_target *remote,

-- 
2.43.0

[PATCH 6/6] Store remote fileio state in remote_state

[Tom Tromey]

This removes the global 'remote_fio_data', moving it to remote_state.
This in turn lets each remote target manage its own file descriptors.

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31189
---
 gdb/remote-fileio.c | 16 ----------------
 gdb/remote-fileio.h |  8 --------
 gdb/remote.c        |  8 +++++---
 3 files changed, 5 insertions(+), 27 deletions(-)

diff --git a/gdb/remote-fileio.c b/gdb/remote-fileio.c
index a954ab1bb55..14ca2ecca44 100644
--- a/gdb/remote-fileio.c
+++ b/gdb/remote-fileio.c
@@ -37,8 +37,6 @@
 #endif
 #include <signal.h>
 
-static remote_fileio_data remote_fio_data;
-
 #define FIO_FD_INVALID		-1
 #define FIO_FD_CONSOLE_IN	-2
 #define FIO_FD_CONSOLE_OUT	-3
@@ -1141,14 +1139,6 @@ remote_fileio_data::do_request (remote_target *remote, char *buf)
   remote_fileio_reply (remote, -1, FILEIO_ENOSYS);
 }
 
-/* Close any open descriptors, and reinitialize the file mapping.  */
-
-void
-remote_fileio_reset ()
-{
-  remote_fio_data.reset ();
-}
-
 /* Handle a file I/O request.  BUF points to the packet containing the
    request.  CTRLC_PENDING_P should be nonzero if the target has not
    acknowledged the Ctrl-C sent asynchronously earlier.  */
@@ -1193,12 +1183,6 @@ remote_fileio_data::request (remote_target *remote,
 
   quit_handler = remote_fileio_o_quit_handler;
 }
-
-void
-remote_fileio_request (remote_target *remote, char *buf, int ctrlc_pending_p)
-{
-  remote_fio_data.request (remote, buf, ctrlc_pending_p);
-}
 \f
 
 /* Unpack an fio_uint_t.  */
diff --git a/gdb/remote-fileio.h b/gdb/remote-fileio.h
index c632c9e40f8..295a4719436 100644
--- a/gdb/remote-fileio.h
+++ b/gdb/remote-fileio.h
@@ -65,14 +65,6 @@ struct remote_fileio_data
   std::vector<int> m_fd_map;
 };
 
-/* Unified interface to remote fileio, called in remote.c from
-   remote_wait () and remote_async_wait ().  */
-extern void remote_fileio_request (remote_target *remote,
-				   char *buf, int ctrlc_pending_p);
-
-/* Cleanup any remote fileio state.  */
-extern void remote_fileio_reset (void);
-
 /* Called from _initialize_remote ().  */
 extern void initialize_remote_fileio (
   struct cmd_list_element **remote_set_cmdlist,
diff --git a/gdb/remote.c b/gdb/remote.c
index dcc1a0d0639..7409b014ea4 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -603,6 +603,9 @@ class remote_state
      qSupported.  */
   gdb_thread_options supported_thread_options = 0;
 
+  /* Data needed for remote fileio.  */
+  remote_fileio_data remote_fio;
+
 private:
   /* Asynchronous signal handle registered as event loop source for
      when we have pending events ready to be passed to the core.  */
@@ -4428,7 +4431,7 @@ remote_target::extended_remote_restart ()
   xsnprintf (rs->buf.data (), get_remote_packet_size (), "R%x", 0);
   putpkt (rs->buf);
 
-  remote_fileio_reset ();
+  rs->remote_fio.reset ();
 }
 \f
 /* Clean up connection to a remote debugger.  */
@@ -6068,7 +6071,6 @@ remote_target::open_1 (const char *name, int from_tty, int extended_p)
   /* Here the possibly existing remote target gets unpushed.  */
   target_preopen (from_tty);
 
-  remote_fileio_reset ();
   reopen_exec_file ();
   reread_symbols (from_tty);
 
@@ -8602,7 +8604,7 @@ remote_target::wait_as (ptid_t ptid, target_waitstatus *status,
 	     for a stop reply.  See the comments in putpkt_binary.  Set
 	     waiting_for_stop_reply to 0 temporarily.  */
 	  rs->waiting_for_stop_reply = 0;
-	  remote_fileio_request (this, buf, rs->ctrlc_pending_p);
+	  rs->remote_fio.request (this, buf, rs->ctrlc_pending_p);
 	  rs->ctrlc_pending_p = 0;
 	  /* GDB handled the File-I/O request, and the target is running
 	     again.  Keep waiting for events.  */

-- 
2.43.0

Re: [PATCH 0/6] Make remote-fileio per-target

[Lancelot SIX]

Hi Tom,

I went through the series and sent comments on patch 2.  Overall this
all seem reasonable to me (I have not tested the code).

Best,
Lancelot.

On Sun, Dec 31, 2023 at 01:25:37PM -0700, Tom Tromey wrote:
> I noticed that the remote-fileio code uses a global.  It seemed to me
> that this should be per-target instead.
> 
> After writing this I noticed that gdbserver does not seem to implement
> any of these packets.  So, I don't think this can really be tested.
> 
> ---
> Tom Tromey (6):
>       Make remote_fio_func_map 'const'
>       Use vector in remote-fileio.c
>       Use methods for remote fileio
>       Remove sentinel from remote_fio_func_map
>       Move remote_fileio_data to header file
>       Store remote fileio state in remote_state
> 
>  gdb/remote-fileio.c | 248 ++++++++++++++++++++++++----------------------------
>  gdb/remote-fileio.h |  42 +++++++--
>  gdb/remote.c        |   8 +-
>  3 files changed, 153 insertions(+), 145 deletions(-)
> ---
> base-commit: 276e7f5c8835cd300ee75d00556ab8839a30b9ef
> change-id: 20231231-remote-fileio-24fbff0ba416
> 
> Best regards,
> -- 
> Tom Tromey <tom@tromey.com>
>