Patchwork add file desc to gdbserver client_state

login
register
mail settings
Submitter Stan Cox
Date Nov. 26, 2019, 6:01 p.m.
Message ID <8448b37f-ebdf-d2d9-829a-87f7f6ea102c@redhat.com>
Download mbox | patch
Permalink /patch/36239/
State New
Headers show

Comments

Stan Cox - Nov. 26, 2019, 6:01 p.m.
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.  The 
gdbserver multi-client patch is mentioned in: 
https://sourceware.org/ml/gdb-patches/2019-08/msg00577.html

gdbserver/ChangeLog:

2019-11-25  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 client state for this remote file desc.
	* remote-utils.c: (get_remote_desc):  New to access remote file 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.

---
  gdb/gdbserver/ChangeLog      | 11 ++++++++
  gdb/gdbserver/remote-utils.c | 55 +++++++++++++++++++++---------------
  gdb/gdbserver/remote-utils.h |  2 ++
  gdb/gdbserver/server.c       | 21 ++++++++++++++
  gdb/gdbserver/server.h       | 11 +++++++-
  5 files changed, 77 insertions(+), 23 deletions(-)
Tom Tromey - Dec. 13, 2019, 11:13 p.m.
>>>>> "Stan" == Stan Cox <scox@redhat.com> writes:

Stan> This patch adds gdb_fildes_t support to gdbserver for later use by the
Stan> gdbserver multi-client patch.  That patch enables multiple clients
Stan> with each client associated with its corresponding file desc.   This
Stan> patch adds the corresponding file desc to the client_state and adds a
Stan> file desc parameter to the gdbserver I/O ops.  It also adds a minimal
Stan> multi_client_states which currently just manages a single client_state
Stan> but later will be expanded to be a collection of multiple
Stan> client_states where each client_state is the state for an individual
Stan> client.  The gdbserver multi-client patch is mentioned in:
Stan> https://sourceware.org/ml/gdb-patches/2019-08/msg00577.html

Thanks.

This looks like a reasonable first step.  Hopefully a few things will
change in coming patches?  I'll note some below.  Also, I think in
general it needs a few more comments and some formatting cleanup.


Stan> +
Stan> +int
Stan> +get_remote_desc (void)
Stan> +{

In gdb new functions need a comment -- usually a description in the
header and then something like "see mumble.h" at the implementation.

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

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

Should be "&client_states", without a space.

Stan> +multi_client_states&

"multi_client_states &".

Stan> +get_client_states (void)

No more "(void)" now that we're using C++.  I think there's a few spots
for this.

Stan> +client_state&

"client_state &".

Stan> +multi_client_states::set_client_state (gdb_fildes_t fd)
Stan> +{
Stan> +  client_state &cs = get_client_state ();

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

Stan> +  int remote_desc = get_remote_desc();

"get_remote_desc ()".  But why does this use int while other spots use
gdb_fildes_t?

Stan> --- a/gdb/gdbserver/server.h
Stan> +++ b/gdb/gdbserver/server.h
Stan> @@ -142,6 +142,8 @@ struct client_state
Stan>      own_buf ((char *) xmalloc (PBUFSIZ + 1))
Stan>    {}

Stan> +  gdb_fildes_t file_desc;

Needs a comment.


Stan> -client_state &get_client_state ();
Stan> +struct multi_client_states
Stan> +{
Stan> +public:

Needs a comment.
Probably should say "class" and not "struct", particularly since it's
using "public:".

Stan> +  client_state & set_client_state (gdb_fildes_t);

"&set_client_state".  Needs a comment.

Stan> +client_state & get_client_state ();
Stan> +struct multi_client_states & get_client_states (void);

Extra spaces and lack of comments here too.

Tom

Patch

diff --git a/gdb/gdbserver/remote-utils.c b/gdb/gdbserver/remote-utils.c
index d7da4b7aed..5c5a119863 100644
--- a/gdb/gdbserver/remote-utils.c
+++ b/gdb/gdbserver/remote-utils.c
@@ -93,7 +93,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);

@@ -115,6 +115,13 @@  static gdb_fildes_t listen_desc = INVALID_DESCRIPTOR;
  # define write(fd, buf, len) send (fd, (char *) buf, len, 0)
  #endif

+
+int
+get_remote_desc (void)
+{
+  return remote_desc;
+}
+
  int
  gdb_connected (void)
  {
@@ -156,6 +163,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,
@@ -607,12 +617,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 +630,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 +676,7 @@  putpkt_binary_1 (char *buf, int cnt, int is_notif)

    do
      {
-      if (write_prim (buf2, p - buf2) != p - buf2)
+      if (write_prim (cs.file_desc, buf2, p - buf2) != p - buf2)
  	{
  	  perror ("putpkt(write)");
  	  free (buf2);
@@ -679,9 +689,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", (int)cs.file_desc, 
buf2);
  	      else
-		debug_printf ("putpkt (\"%s\"); [noack mode]\n", buf2);
+		debug_printf ("putpkt (%d, \"%s\"); [noack mode]\n", 
(int)cs.file_desc, buf2);
  	      debug_flush ();
  	    }
  	  break;
@@ -689,11 +699,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", 
(int)cs.file_desc, buf2);
  	  debug_flush ();
  	}

-      cc = readchar ();
+      cc = readchar (cs.file_desc);

        if (cc < 0)
  	{
@@ -761,7 +771,7 @@  input_interrupt (int unused)
        int cc;
        char c = 0;

-      cc = read_prim (&c, 1);
+      cc = read_prim (remote_desc, &c, 1);

        if (cc == 0)
  	{
@@ -891,13 +901,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 +982,7 @@  getpkt (char *buf)
    char *bp;
    unsigned char csum, c1, c2;
    int c;
+  gdb_fildes_t fd = cs.file_desc;

    while (1)
      {
@@ -979,7 +990,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 +1015,7 @@  getpkt (char *buf)
        bp = buf;
        while (1)
  	{
-	  c = readchar ();
+	  c = readchar (fd);
  	  if (c < 0)
  	    return -1;
  	  if (c == '#')
@@ -1014,8 +1025,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 +1043,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 +1051,11 @@  getpkt (char *buf)
      {
        if (remote_debug)
  	{
-	  debug_printf ("getpkt (\"%s\");  [sending ack] \n", buf);
+	  debug_printf ("getpkt (%d, \"%s\");  [sending ack] \n", (int)fd, buf);
  	  debug_flush ();
  	}

-      if (write_prim ("+", 1) != 1)
+      if (write_prim (fd, "+", 1) != 1)
  	return -1;

        if (remote_debug)
@@ -1057,7 +1068,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", (int)fd, buf);
  	  debug_flush ();
  	}
      }
@@ -1074,7 +1085,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..a686038af7 100644
--- a/gdb/gdbserver/server.c
+++ b/gdb/gdbserver/server.c
@@ -152,6 +152,23 @@  static struct btrace_config current_btrace_conf;

  /* The client remote protocol state. */

+static struct multi_client_states g_client_states;
+
+multi_client_states&
+get_client_states (void)
+{
+  return g_client_states;
+}
+
+client_state&
+multi_client_states::set_client_state (gdb_fildes_t fd)
+{
+  client_state &cs = get_client_state ();
+
+  cs.file_desc = fd;
+  return cs;
+}
+
  static client_state g_client_state;

  client_state &
@@ -3556,6 +3573,10 @@  captured_main (int argc, char *argv[])
  #endif

    current_directory = getcwd (NULL, 0);
+
+  int remote_desc = get_remote_desc();
+  multi_client_states &client_states = get_client_states ();
+  client_states.set_client_state (remote_desc);
    client_state &cs = get_client_state ();

    if (current_directory == NULL)
diff --git a/gdb/gdbserver/server.h b/gdb/gdbserver/server.h
index e01c4f146e..52fae32e3e 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 file_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,14 @@  struct client_state

  };

-client_state &get_client_state ();
+struct multi_client_states
+{
+public:
+  client_state & set_client_state (gdb_fildes_t);
+};
+
+client_state & get_client_state ();
+struct multi_client_states & get_client_states (void);

  #include "gdbthread.h"
  #include "inferiors.h"