Re: [PATCH] add file desc to gdbserver client_state

Stan Cox
2019-12-24T04:42:54Z

[-- Attachment #1: Type: text/plain, Size: 1205 bytes --]

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!

[-- Attachment #2: gdbs-fd.patch --]
[-- Type: text/x-patch, Size: 13185 bytes --]

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"