add file desc to gdbserver client_state

Message ID 827a8c5f-e797-58a3-62bf-335e7a44cd9a@redhat.com
State New, archived
Headers

Commit Message

Stan Cox Dec. 24, 2019, 4:42 a.m. UTC
  This patch adds gdb_fildes_t support to gdbserver for later use by the
gdbserver multi-client patch.  That patch enables multiple clients
with each client associated with its corresponding file desc.   This
patch adds the corresponding file desc to the client_state and adds a
file desc parameter to the gdbserver I/O ops.  It also adds a minimal
multi_client_states which currently just manages a single client_state
but later will be expanded to be a collection of multiple
client_states where each client_state is the state for an individual
client.  In contrast to the previous patch, this patch
  1) uses client_state.remote_desc in remote-utils.c instead of the 
static version
  2) no longer uses the function get_client_state but instead defines 
get_client_state
     using multi_client_states
  The gdbserver multi-client patch is mentioned in:
  https://sourceware.org/ml/gdb-patches/2019-08/msg00577.html


> Will remote_desc be going away?  And hopefully get_remote_desc is also
> just a stepping stone?

remote_desc is now gone as mentioned in 1 above


> Can get_client_state be made static now?
> If not, why not?

get_client_state is now redefined as mentioned in 2) above

Thanks Tom!
  

Comments

Tom Tromey Jan. 24, 2020, 6:12 p.m. UTC | #1
>>>>> "Stan" == Stan Cox <scox@redhat.com> writes:

Stan>     Add struct multi_client_states
    
Stan>     * remote-utils.c (remote_desc):  Move to struct client_state
Stan>       (gdb_connected, handle_accept_event, remote_open, putpkt_binary_1)
Stan>       (putpkt_notif, input_interrupt, block_unblock_async_io)
Stan>       (nto_conctrl, getpkt): Change remote_desc reference.
Stan>     * server.c (captured_main): Move remote_desc initialization to
Stan>       remote_prepare.

Thanks for the patch.

Stan> diff --git a/gdb/gdbserver/remote-utils.c b/gdb/gdbserver/remote-utils.c
Stan> index d7da4b7aed..09095d9bbc 100644
Stan> --- a/gdb/gdbserver/remote-utils.c
Stan> +++ b/gdb/gdbserver/remote-utils.c
Stan> @@ -25,6 +25,7 @@
Stan>  #include "tdesc.h"
Stan>  #include "debug.h"
Stan>  #include "dll.h"
Stan> +#include "server.h"

I think this is already included near the top.
Normally, .c files in gdbserver include this first.

Stan> +
Stan>  int
Stan>  gdb_connected (void)

Spurious newline addition.

Stan> +  struct multi_client_states &client_states = get_client_states();

Needs a space before the "(".

Stan> +  get_client_states().set_current_client (cs);

Ditto.

Stan> +/* Container of client remote protocol states for all the currently
Stan> +   connected clients.  */
Stan> +
Stan> +#define get_client_state() get_client_states().get_current_client()
Stan> +
Stan> +class multi_client_states
Stan> +{
Stan> +private:
Stan> +  client_state *current_cs;
Stan> +
Stan> +public:
Stan> +  /* Return the current client we are focused on. */
Stan> +  client_state &get_current_client () { return *current_cs; }
Stan> +
Stan> +  /* Set the current client we wish to focus on. */
Stan> +  void set_current_client (client_state &cs) { current_cs = &cs; }

I think this class is a bit strange.
It stores a pointer but the set_ method takes a reference.

If this class is going to own multiple clients in the future, then I
think it makes sense to design the API that way from the start.

thanks,
Tom
  

Patch

commit bac0aa21ec (HEAD -> master)
Author: Stan Cox <scox@redhat.com>
Date:   Fri Dec 20 15:54:22 2019 -0500

    Add struct multi_client_states
    
    * remote-utils.c (remote_desc):  Move to struct client_state
      (gdb_connected, handle_accept_event, remote_open, putpkt_binary_1)
      (putpkt_notif, input_interrupt, block_unblock_async_io)
      (nto_conctrl, getpkt): Change remote_desc reference.
    * server.c (captured_main): Move remote_desc initialization to
      remote_prepare.
---
 gdb/gdbserver/ChangeLog      | 12 ++++++++++++
 gdb/gdbserver/remote-utils.c | 81 ++++++++++++++++++++++++++++++++++++++++++++++++---------------------------------
 gdb/gdbserver/remote-utils.h |  2 ++
 gdb/gdbserver/server.c       | 15 ++++++++++++++-
 gdb/gdbserver/server.h       | 25 ++++++++++++++++++++++++-
 5 files changed, 100 insertions(+), 35 deletions(-)

diff --git a/gdb/gdbserver/ChangeLog b/gdb/gdbserver/ChangeLog
index 02414385b7..bf43b232f5 100644
--- a/gdb/gdbserver/ChangeLog
+++ b/gdb/gdbserver/ChangeLog
@@ -1,3 +1,15 @@ 
+2019-12-23  Stan Cox  <scox@redhat.com>
+
+	* server.h (struct multi_client_states):  New.
+	* server.c (g_client_states)
+	(get_client_states, multi_client_sates::set_client_state):  New.
+	(captured_main):  Set the initial multi_client_states
+	* remote-utils.c (gdb_connected, remote_prepare, remote_close)
+	(putpkt_binary_1, getpkt)  Use client_state.remote_desc.
+	(handle_accept_event):  Set the client state for this remote file desc.
+	(write_prim, read_prim, readchar): Add gdb_fildes_t parameter.
+	Change all callers.
+
 2019-11-22  Tom Tromey  <tromey@adacore.com>
 
 	* gdb.ada/tasks.exp: Add -ada-task-info regression test.
diff --git a/gdb/gdbserver/remote-utils.c b/gdb/gdbserver/remote-utils.c
index d7da4b7aed..09095d9bbc 100644
--- a/gdb/gdbserver/remote-utils.c
+++ b/gdb/gdbserver/remote-utils.c
@@ -25,6 +25,7 @@ 
 #include "tdesc.h"
 #include "debug.h"
 #include "dll.h"
+#include "server.h"
 #include "gdbsupport/rsp-low.h"
 #include "gdbsupport/netstuff.h"
 #include "gdbsupport/filestuff.h"
@@ -93,7 +94,7 @@  enum {
    Either NOT_SCHEDULED or the callback id.  */
 static int readchar_callback = NOT_SCHEDULED;
 
-static int readchar (void);
+static int readchar (gdb_fildes_t);
 static void reset_readchar (void);
 static void reschedule (void);
 
@@ -107,7 +108,6 @@  struct sym_cache
 
 static int remote_is_stdio = 0;
 
-static gdb_fildes_t remote_desc = INVALID_DESCRIPTOR;
 static gdb_fildes_t listen_desc = INVALID_DESCRIPTOR;
 
 #ifdef USE_WIN32API
@@ -115,10 +115,12 @@  static gdb_fildes_t listen_desc = INVALID_DESCRIPTOR;
 # define write(fd, buf, len) send (fd, (char *) buf, len, 0)
 #endif
 
+
 int
 gdb_connected (void)
 {
-  return remote_desc != INVALID_DESCRIPTOR;
+  client_state &cs = get_client_state ();
+  return cs.remote_desc != INVALID_DESCRIPTOR;
 }
 
 /* Return true if the remote connection is over stdio.  */
@@ -148,6 +150,7 @@  handle_accept_event (int err, gdb_client_data client_data)
 {
   struct sockaddr_storage sockaddr;
   socklen_t len = sizeof (sockaddr);
+  gdb_fildes_t remote_desc;
 
   if (debug_threads)
     debug_printf ("handling possible accept event\n");
@@ -156,6 +159,9 @@  handle_accept_event (int err, gdb_client_data client_data)
   if (remote_desc == -1)
     perror_with_name ("Accept failed");
 
+  struct multi_client_states &client_states = get_client_states();
+  client_states.set_client_state (remote_desc);
+
   /* Enable TCP keep alive process. */
   socklen_t tmp = 1;
   setsockopt (remote_desc, SOL_SOCKET, SO_KEEPALIVE,
@@ -268,6 +274,8 @@  remote_prepare (const char *name)
     }
 #endif
 
+  cs.remote_desc = INVALID_DESCRIPTOR;
+
   int r = getaddrinfo (parsed.host_str.c_str (), parsed.port_str.c_str (),
 		       &hint, &ainfo);
 
@@ -324,6 +332,7 @@  void
 remote_open (const char *name)
 {
   const char *port_str;
+  gdb_fildes_t remote_desc;
 
   port_str = strchr (name, ':');
 #ifdef USE_WIN32API
@@ -416,17 +425,19 @@  remote_open (const char *name)
 void
 remote_close (void)
 {
-  delete_file_handler (remote_desc);
+  client_state &cs = get_client_state ();
+
+  delete_file_handler (cs.remote_desc);
 
   disable_async_io ();
 
 #ifdef USE_WIN32API
-  closesocket (remote_desc);
+  closesocket (cs.remote_desc);
 #else
   if (! remote_connection_is_stdio ())
-    close (remote_desc);
+    close (cs.remote_desc);
 #endif
-  remote_desc = INVALID_DESCRIPTOR;
+  cs.remote_desc = INVALID_DESCRIPTOR;
 
   reset_readchar ();
 }
@@ -607,12 +618,12 @@  read_ptid (const char *buf, const char **obuf)
    This may return less than COUNT.  */
 
 static int
-write_prim (const void *buf, int count)
+write_prim (gdb_fildes_t fd, const void *buf, int count)
 {
   if (remote_connection_is_stdio ())
     return write (fileno (stdout), buf, count);
   else
-    return write (remote_desc, buf, count);
+    return write (fd, buf, count);
 }
 
 /* Read COUNT bytes from the client and store in BUF.
@@ -620,12 +631,12 @@  write_prim (const void *buf, int count)
    This may return less than COUNT.  */
 
 static int
-read_prim (void *buf, int count)
+read_prim (gdb_fildes_t fd, void *buf, int count)
 {
   if (remote_connection_is_stdio ())
     return read (fileno (stdin), buf, count);
   else
-    return read (remote_desc, buf, count);
+    return read (fd, buf, count);
 }
 
 /* Send a packet to the remote machine, with error checking.
@@ -666,7 +677,7 @@  putpkt_binary_1 (char *buf, int cnt, int is_notif)
 
   do
     {
-      if (write_prim (buf2, p - buf2) != p - buf2)
+      if (write_prim (cs.remote_desc, buf2, p - buf2) != p - buf2)
 	{
 	  perror ("putpkt(write)");
 	  free (buf2);
@@ -679,9 +690,9 @@  putpkt_binary_1 (char *buf, int cnt, int is_notif)
 	  if (remote_debug)
 	    {
 	      if (is_notif)
-		debug_printf ("putpkt (\"%s\"); [notif]\n", buf2);
+	  	debug_printf ("putpkt (%d, \"%s\"); [notif]\n", cs.remote_desc, buf2);
 	      else
-		debug_printf ("putpkt (\"%s\"); [noack mode]\n", buf2);
+		debug_printf ("putpkt (%d, \"%s\"); [noack mode]\n", cs.remote_desc, buf2);
 	      debug_flush ();
 	    }
 	  break;
@@ -689,11 +700,11 @@  putpkt_binary_1 (char *buf, int cnt, int is_notif)
 
       if (remote_debug)
 	{
-	  debug_printf ("putpkt (\"%s\"); [looking for ack]\n", buf2);
+	  debug_printf ("putpkt (%d, \"%s\"); [looking for ack]\n", cs.remote_desc, buf2);
 	  debug_flush ();
 	}
 
-      cc = readchar ();
+      cc = readchar (cs.remote_desc);
 
       if (cc < 0)
 	{
@@ -748,6 +759,7 @@  putpkt_notif (char *buf)
 static void
 input_interrupt (int unused)
 {
+  client_state &cs = get_client_state ();
   fd_set readset;
   struct timeval immediate = { 0, 0 };
 
@@ -755,13 +767,13 @@  input_interrupt (int unused)
      be a problem under NetBSD 1.4 and 1.5.  */
 
   FD_ZERO (&readset);
-  FD_SET (remote_desc, &readset);
-  if (select (remote_desc + 1, &readset, 0, 0, &immediate) > 0)
+  FD_SET (cs.remote_desc, &readset);
+  if (select (cs.remote_desc + 1, &readset, 0, 0, &immediate) > 0)
     {
       int cc;
       char c = 0;
 
-      cc = read_prim (&c, 1);
+      cc = read_prim (cs.remote_desc, &c, 1);
 
       if (cc == 0)
 	{
@@ -786,10 +798,11 @@  input_interrupt (int unused)
 void
 check_remote_input_interrupt_request (void)
 {
+  client_state &cs = get_client_state ();
   /* This function may be called before establishing communications,
      therefore we need to validate the remote descriptor.  */
 
-  if (remote_desc == INVALID_DESCRIPTOR)
+  if (cs.remote_desc == INVALID_DESCRIPTOR)
     return;
 
   input_interrupt (0);
@@ -815,6 +828,7 @@  block_unblock_async_io (int block)
 static void
 nto_comctrl (int enable)
 {
+  client_state &cs = get_client_state ();
   struct sigevent event;
 
   if (enable)
@@ -824,11 +838,11 @@  nto_comctrl (int enable)
       event.sigev_code = 0;
       event.sigev_value.sival_ptr = NULL;
       event.sigev_priority = -1;
-      ionotify (remote_desc, _NOTIFY_ACTION_POLLARM, _NOTIFY_COND_INPUT,
+      ionotify (cs.remote_desc, _NOTIFY_ACTION_POLLARM, _NOTIFY_COND_INPUT,
 		&event);
     }
   else
-    ionotify (remote_desc, _NOTIFY_ACTION_POLL, _NOTIFY_COND_INPUT, NULL);
+    ionotify (cs.remote_desc, _NOTIFY_ACTION_POLL, _NOTIFY_COND_INPUT, NULL);
 }
 #endif /* __QNX__ */
 
@@ -891,13 +905,13 @@  static unsigned char *readchar_bufp;
 /* Returns next char from remote GDB.  -1 if error.  */
 
 static int
-readchar (void)
+readchar (gdb_fildes_t fd)
 {
   int ch;
 
   if (readchar_bufcnt == 0)
     {
-      readchar_bufcnt = read_prim (readchar_buf, sizeof (readchar_buf));
+      readchar_bufcnt = read_prim (fd, readchar_buf, sizeof (readchar_buf));
 
       if (readchar_bufcnt <= 0)
 	{
@@ -972,6 +986,7 @@  getpkt (char *buf)
   char *bp;
   unsigned char csum, c1, c2;
   int c;
+  gdb_fildes_t fd = cs.remote_desc;
 
   while (1)
     {
@@ -979,7 +994,7 @@  getpkt (char *buf)
 
       while (1)
 	{
-	  c = readchar ();
+	  c = readchar (fd);
 
 	  /* The '\003' may appear before or after each packet, so
 	     check for an input interrupt.  */
@@ -1004,7 +1019,7 @@  getpkt (char *buf)
       bp = buf;
       while (1)
 	{
-	  c = readchar ();
+	  c = readchar (fd);
 	  if (c < 0)
 	    return -1;
 	  if (c == '#')
@@ -1014,8 +1029,8 @@  getpkt (char *buf)
 	}
       *bp = 0;
 
-      c1 = fromhex (readchar ());
-      c2 = fromhex (readchar ());
+      c1 = fromhex (readchar (fd));
+      c2 = fromhex (readchar (fd));
 
       if (csum == (c1 << 4) + c2)
 	break;
@@ -1032,7 +1047,7 @@  getpkt (char *buf)
 
       fprintf (stderr, "Bad checksum, sentsum=0x%x, csum=0x%x, buf=%s\n",
 	       (c1 << 4) + c2, csum, buf);
-      if (write_prim ("-", 1) != 1)
+      if (write_prim (fd, "-", 1) != 1)
 	return -1;
     }
 
@@ -1040,11 +1055,11 @@  getpkt (char *buf)
     {
       if (remote_debug)
 	{
-	  debug_printf ("getpkt (\"%s\");  [sending ack] \n", buf);
+	  debug_printf ("getpkt (%d, \"%s\");  [sending ack] \n", fd, buf);
 	  debug_flush ();
 	}
 
-      if (write_prim ("+", 1) != 1)
+      if (write_prim (fd, "+", 1) != 1)
 	return -1;
 
       if (remote_debug)
@@ -1057,7 +1072,7 @@  getpkt (char *buf)
     {
       if (remote_debug)
 	{
-	  debug_printf ("getpkt (\"%s\");  [no ack sent] \n", buf);
+	  debug_printf ("getpkt (%d, \"%s\");  [no ack sent] \n", fd, buf);
 	  debug_flush ();
 	}
     }
@@ -1074,7 +1089,7 @@  getpkt (char *buf)
   while (readchar_bufcnt > 0 && *readchar_bufp == '\003')
     {
       /* Consume the interrupt character in the buffer.  */
-      readchar ();
+      readchar (fd);
       (*the_target->request_interrupt) ();
     }
 
diff --git a/gdb/gdbserver/remote-utils.h b/gdb/gdbserver/remote-utils.h
index 4ca5d9435e..606592852c 100644
--- a/gdb/gdbserver/remote-utils.h
+++ b/gdb/gdbserver/remote-utils.h
@@ -19,6 +19,8 @@ 
 #ifndef GDBSERVER_REMOTE_UTILS_H
 #define GDBSERVER_REMOTE_UTILS_H
 
+int get_remote_desc (void);
+
 int gdb_connected (void);
 
 #define STDIO_CONNECTION_NAME "stdio"
diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c
index c5f7176cff..40b8ea1b80 100644
--- a/gdb/gdbserver/server.c
+++ b/gdb/gdbserver/server.c
@@ -153,11 +153,21 @@  static struct btrace_config current_btrace_conf;
 /* The client remote protocol state. */
 
 static client_state g_client_state;
+static struct multi_client_states g_client_states;
+
+multi_client_states &
+get_client_states ()
+{
+  return g_client_states;
+}
 
 client_state &
-get_client_state ()
+multi_client_states::set_client_state (gdb_fildes_t fd)
 {
   client_state &cs = g_client_state;
+  
+  cs.remote_desc = fd;
+  get_client_states().set_current_client (cs);
   return cs;
 }
 
@@ -3556,6 +3566,9 @@  captured_main (int argc, char *argv[])
 #endif
 
   current_directory = getcwd (NULL, 0);
+
+  multi_client_states &client_states = get_client_states ();
+  client_states.set_client_state (-1);
   client_state &cs = get_client_state ();
 
   if (current_directory == NULL)
diff --git a/gdb/gdbserver/server.h b/gdb/gdbserver/server.h
index e01c4f146e..027adbfe63 100644
--- a/gdb/gdbserver/server.h
+++ b/gdb/gdbserver/server.h
@@ -142,6 +142,8 @@  struct client_state
     own_buf ((char *) xmalloc (PBUFSIZ + 1)) 
   {}
 
+  gdb_fildes_t remote_desc;
+
   /* The thread set with an `Hc' packet.  `Hc' is deprecated in favor of
      `vCont'.  Note the multi-process extensions made `vCont' a
      requirement, so `Hc pPID.TID' is pretty much undefined.  So
@@ -200,7 +202,28 @@  struct client_state
 
 };
 
-client_state &get_client_state ();
+/* Container of client remote protocol states for all the currently
+   connected clients.  */
+
+#define get_client_state() get_client_states().get_current_client()
+
+class multi_client_states
+{
+private:
+  client_state *current_cs;
+
+public:
+  /* Return the current client we are focused on. */
+  client_state &get_current_client () { return *current_cs; }
+
+  /* Set the current client we wish to focus on. */
+  void set_current_client (client_state &cs) { current_cs = &cs; }
+
+  /* Set, or create if necessary, the current client we wish to focus on */
+  client_state &set_client_state (gdb_fildes_t);
+};
+
+struct multi_client_states &get_client_states ();
 
 #include "gdbthread.h"
 #include "inferiors.h"