Re: [RFC][PATCH v2] Consolidate gdbserver global variables

[Pedro Alves <palves at redhat dot com>]

Hi Stan,

On 05/03/2018 04:06 PM, Stan Cox wrote:

> Removing server_state removed all of this and simplified things considerably.

Great.

This makes it easier to see where we're going.

Some comments more.  I still have some design reservations.
It seems to me that simply making some of the globals be
per client along won't work correctly.  Particularly, the
cases where the globals are used in the backends.

More details below.

> Add client_state struct.
> 
> Collect per client specific global data items into struct client_state,
> which is similar in purpose to remote.c::remote_state.
> 
>     * server.h (struct client_state): New.
>     * server.c (cont_thread, server_waiting, multi_process)
>     (report_fork_events, report_vfork_events, report_exec_events)
>     (report_thread_events, swbreak_feature, hwbreak_feature)
>     (vCont_supported, disable_randomization, pass_signals)
>     (program_signals, program_signals_p, own_buf):
>     Moved to client_state.
>     * remote-utils.c (remote-debug, noack_mode)
>     (transport_is_reliable): Moved to client_state.
>     * tracepoint.c (current_traceframe): Moved to client_state.
> 
>     Update all callers.
>     * (server.c, remote-utils.c, tracepoint.c, fork-child.c,
>     gdbthread.h, hostio.c, linux-arm-low.c, linux-low.c, notif.c,
>     remote-utils.h, spu-low.c, target.c, win32-low.c):  Use    
>     client_state.

Remove ()s around files list after "Update all callers".

There's spurious whitespace before and after "Use" in the
last entry.

> 
> --- a/gdb/gdbserver/fork-child.c
> +++ b/gdb/gdbserver/fork-child.c
> @@ -44,6 +44,7 @@ restore_old_foreground_pgrp (void)
>  void
>  prefork_hook (const char *args)
>  {
> +  struct client_state & cs = get_client_state ();

Just like with * for pointers, no space after &.

While at it, no need for spelling out "struct" in C++, so,
please write:

     client_state &cs = get_client_state ();

throughout.

>    if (debug_threads)
>      {
>        debug_printf ("args: %s\n", args);
> diff --git a/gdb/gdbserver/gdbthread.h b/gdb/gdbserver/gdbthread.h
> index 0edf870c56..9a8c72e780 100644
> --- a/gdb/gdbserver/gdbthread.h
> +++ b/gdb/gdbserver/gdbthread.h
> @@ -224,4 +224,7 @@ lwpid_of (const thread_info *thread)
>    return thread->id.lwp ();
>  }
> 
> +/* Create a cleanup to restore current_thread.  */
> +struct cleanup *make_cleanup_restore_current_thread (void);

Please mention this in the ChangeLog.

> +
>  #endif /* GDB_THREAD_H */
> diff --git a/gdb/gdbserver/hostio.c b/gdb/gdbserver/hostio.c
> index d2b5a71bad..4e388a85dc 100644
> --- a/gdb/gdbserver/hostio.c
> +++ b/gdb/gdbserver/hostio.c
> @@ -29,8 +29,6 @@
>  #include <sys/stat.h>
>  #include "fileio.h"
> 
> -extern int remote_debug;
> -

This wasn't mentioned in the ChangeLog.

>  struct fd_list
>  {
>    int fd;
> diff --git a/gdb/gdbserver/linux-arm-low.c b/gdb/gdbserver/linux-arm-low.c
> index 6c2dcead03..3004018e5b 100644
> --- a/gdb/gdbserver/linux-arm-low.c
> +++ b/gdb/gdbserver/linux-arm-low.c
> @@ -160,6 +160,7 @@ static struct arm_get_next_pcs_ops get_next_pcs_ops = {
>    arm_linux_get_next_pcs_fixup,
>  };
> 
> +
>  static int
>  arm_cannot_store_register (int regno)
>  {

Spurious change.  Please remove it.

> diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
> index e64949504d..47dea21e5a 100644
> --- a/gdb/gdbserver/linux-low.c
> +++ b/gdb/gdbserver/linux-low.c
> @@ -475,6 +475,7 @@ linux_arch_setup_thread (struct thread_info *thread)
>  static int
>  handle_extended_wait (struct lwp_info **orig_event_lwp, int wstat)
>  {
> +  struct client_state & cs = get_client_state ();
>    struct lwp_info *event_lwp = *orig_event_lwp;
>    int event = linux_ptrace_get_extended_event (wstat);
>    struct thread_info *event_thr = get_lwp_thread (event_lwp);
> @@ -655,7 +656,7 @@ handle_extended_wait (struct lwp_info **orig_event_lwp, int wstat)
>        new_lwp->status_pending_p = 1;
>        new_lwp->status_pending = status;
>      }
> -      else if (report_thread_events)
> +      else if (cs.report_thread_events)
>      {
>        new_lwp->waitstatus.kind = TARGET_WAITKIND_THREAD_CREATED;
>        new_lwp->status_pending_p = 1;
> @@ -683,7 +684,7 @@ handle_extended_wait (struct lwp_info **orig_event_lwp, int wstat)
>        /* Report the event.  */
>        return 0;
>      }
> -  else if (event == PTRACE_EVENT_EXEC && report_exec_events)
> +  else if (event == PTRACE_EVENT_EXEC && cs.report_exec_events)
>      {
>        struct process_info *proc;
>        std::vector<int> syscalls_to_catch;

As mentioned, I'm skeptical of all client_state references
in the backends.

How will this work with multiple clients?   What if one client
wants exec events, and the other one doesn't?  This seems to
suggest that the backends need to enable exec events if _any_
client wants them (or unconditionally), and then filter out exec
events at a higher level, before reporting the event to
each client?

> @@ -998,13 +999,14 @@ static int
>  linux_create_inferior (const char *program,
>                 const std::vector<char *> &program_args)
>  {
> +  struct client_state & cs = get_client_state ();
>    struct lwp_info *new_lwp;
>    int pid;
>    ptid_t ptid;
> 
>    {
>      maybe_disable_address_space_randomization restore_personality
> -      (disable_randomization);
> +      (cs.disable_randomization);
>      std::string str_program_args = stringify_argv (program_args);
> 

This one might be OK-ish with the assumption that it's always
going to be the current client that is requesting the
inferior creation.

>      pid = fork_inferior (program,
> @@ -1429,6 +1431,7 @@ linux_kill (int pid)
>  static int
>  get_detach_signal (struct thread_info *thread)
>  {
> +  struct client_state & cs = get_client_state ();
>    enum gdb_signal signo = GDB_SIGNAL_0;
>    int status;
>    struct lwp_info *lp = get_thread_lwp (thread);
> @@ -1469,7 +1472,7 @@ get_detach_signal (struct thread_info *thread)
> 
>    signo = gdb_signal_from_host (WSTOPSIG (status));
> 
> -  if (program_signals_p && !program_signals[signo])
> +  if (cs.program_signals_p && !cs.program_signals[signo])

Similarly, what is the plan for multiple clients for this property,
when the clients don't agree, on both support for the packet,
and, what if they don't agree on pass/nopass state?

>      {
>        if (debug_threads)
>      debug_printf ("GPS: lwp %s had signal %s, but it is in nopass state\n",
> @@ -1477,7 +1480,7 @@ get_detach_signal (struct thread_info *thread)
>                gdb_signal_to_string (signo));
>        return 0;
>      }
> -  else if (!program_signals_p
> +  else if (!cs.program_signals_p
>         /* If we have no way to know which signals GDB does not
>            want to have passed to the program, assume
>            SIGTRAP/SIGINT, which is GDB's default.  */
> @@ -2328,18 +2331,19 @@ check_stopped_by_watchpoint (struct lwp_info *child)
>  static int
>  linux_low_ptrace_options (int attached)
>  {
> +  struct client_state & cs = get_client_state ();
>    int options = 0;
> 
>    if (!attached)
>      options |= PTRACE_O_EXITKILL;
> 
> -  if (report_fork_events)
> +  if (cs.report_fork_events)
>      options |= PTRACE_O_TRACEFORK;
> 
> -  if (report_vfork_events)
> +  if (cs.report_vfork_events)
>      options |= (PTRACE_O_TRACEVFORK | PTRACE_O_TRACEVFORKDONE);
> 
> -  if (report_exec_events)
> +  if (cs.report_exec_events)
>      options |= PTRACE_O_TRACEEXEC;
> 
>    options |= PTRACE_O_TRACESYSGOOD;

Ditto.

> @@ -2354,6 +2358,7 @@ linux_low_ptrace_options (int attached)
>  static struct lwp_info *
>  linux_low_filter_event (int lwpid, int wstat)
>  {
> +  struct client_state & cs = get_client_state ();
>    struct lwp_info *child;
>    struct thread_info *thread;
>    int have_stop_pc = 0;
> @@ -2425,7 +2430,7 @@ linux_low_filter_event (int lwpid, int wstat)
>        /* If there is at least one more LWP, then the exit signal was
>       not the end of the debugged application and should be
>       ignored, unless GDB wants to hear about thread exits.  */
> -      if (report_thread_events
> +      if (cs.report_thread_events
>        || last_thread_of_process_p (pid_of (thread)))
>      {
>        /* Since events are serialized to GDB core, and we can't

Ditto.

> @@ -3049,12 +3054,13 @@ static ptid_t
>  filter_exit_event (struct lwp_info *event_child,
>             struct target_waitstatus *ourstatus)
>  {
> +  struct client_state & cs = get_client_state ();
>    struct thread_info *thread = get_lwp_thread (event_child);
>    ptid_t ptid = ptid_of (thread);
> 
>    if (!last_thread_of_process_p (pid_of (thread)))
>      {
> -      if (report_thread_events)
> +      if (cs.report_thread_events)
>      ourstatus->kind = TARGET_WAITKIND_THREAD_EXITED;
>        else
>      ourstatus->kind = TARGET_WAITKIND_IGNORE;
> @@ -3106,6 +3112,7 @@ static ptid_t
>  linux_wait_1 (ptid_t ptid,
>            struct target_waitstatus *ourstatus, int target_options)
>  {
> +  struct client_state & cs = get_client_state ();
>    int w;
>    struct lwp_info *event_child;
>    int options;
> @@ -3475,7 +3482,7 @@ linux_wait_1 (ptid_t ptid,
>             || WSTOPSIG (w) == __SIGRTMIN + 1))
>        ||
>  #endif
> -      (pass_signals[gdb_signal_from_host (WSTOPSIG (w))]
> +      (cs.pass_signals[gdb_signal_from_host (WSTOPSIG (w))]
>         && !(WSTOPSIG (w) == SIGSTOP
>          && current_thread->last_resume_kind == resume_stop)
>         && !linux_wstatus_maybe_breakpoint (w))))
> @@ -3782,7 +3789,7 @@ linux_wait_1 (ptid_t ptid,
>       it was a software breakpoint, and the client doesn't know we can
>       adjust the breakpoint ourselves.  */
>    if (event_child->stop_reason == TARGET_STOPPED_BY_SW_BREAKPOINT
> -      && !swbreak_feature)
> +      && !cs.swbreak_feature)
>      {
>        int decr_pc = the_low_target.decr_pc_after_break;
> 
> @@ -5036,6 +5043,7 @@ linux_resume_one_thread (thread_info *thread, bool leave_all_stopped)
>  static void
>  linux_resume (struct thread_resume *resume_info, size_t n)
>  {
> +  struct client_state & cs = get_client_state ();
>    struct thread_info *need_step_over = NULL;
> 

This one looks unnecessary/unused.  'cs' is not used.

>    if (debug_threads)
> diff --git a/gdb/gdbserver/notif.c b/gdb/gdbserver/notif.c
> index 5ff7079123..2cd411f9bb 100644
> --- a/gdb/gdbserver/notif.c
> +++ b/gdb/gdbserver/notif.c
> @@ -79,6 +79,7 @@ notif_write_event (struct notif_server *notif, char *own_buf)
>  int
>  handle_notif_ack (char *own_buf, int packet_len)
>  {
> +  struct client_state & cs = get_client_state ();
>    size_t i;
>    struct notif_server *np;
> 
> @@ -103,7 +104,7 @@ handle_notif_ack (char *own_buf, int packet_len)
>        struct notif_event *head
>      = QUEUE_deque (notif_event_p, np->queue);
> 
> -      if (remote_debug)
> +      if (cs.remote_debug)
>      debug_printf ("%s: acking %d\n", np->ack_name,
>                QUEUE_length (notif_event_p, np->queue));
> 
> @@ -121,9 +122,10 @@ void
>  notif_event_enque (struct notif_server *notif,
>             struct notif_event *event)
>  {
> +  struct client_state & cs = get_client_state ();
>    QUEUE_enque (notif_event_p, notif->queue, event);
> 
> -  if (remote_debug)
> +  if (cs.remote_debug)
>      debug_printf ("pending events: %s %d\n", notif->notif_name,
>            QUEUE_length (notif_event_p, notif->queue));
> 
> diff --git a/gdb/gdbserver/remote-utils.c b/gdb/gdbserver/remote-utils.c
> index 3b5a459ae4..ea02631444 100644
> --- a/gdb/gdbserver/remote-utils.c
> +++ b/gdb/gdbserver/remote-utils.c
> @@ -26,6 +26,7 @@
>  #include "dll.h"
>  #include "rsp-low.h"
>  #include "gdbthread.h"
> +#include "server.h"

This should be unnecessary - "server.h" is already included above.
I could tell because "server.h" must always be the
first include in all .c files.

>  #include <ctype.h>
>  #if HAVE_SYS_IOCTL_H
>  #include <sys/ioctl.h>
> @@ -103,7 +104,6 @@ struct sym_cache
>    struct sym_cache *next;
>  };
> 
> -int remote_debug = 0;
>  struct ui_file *gdb_stdlog;
> 
>  static int remote_is_stdio = 0;
> @@ -115,11 +115,6 @@ static gdb_fildes_t listen_desc = INVALID_DESCRIPTOR;
>  extern int using_threads;
>  extern int debug_threads;
> 
> -/* If true, then GDB has requested noack mode.  */
> -int noack_mode = 0;
> -/* If true, then we tell GDB to use noack mode by default.  */
> -int transport_is_reliable = 0;
> -
>  #ifdef USE_WIN32API
>  # define read(fd, buf, len) recv (fd, (char *) buf, len, 0)
>  # define write(fd, buf, len) send (fd, (char *) buf, len, 0)


> diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c
> index 5027df5e10..b4b72b55f8 100644
> --- a/gdb/gdbserver/server.c
> +++ b/gdb/gdbserver/server.c
> @@ -83,36 +83,17 @@ ptid_t cont_thread;
>  /* The thread set with an `Hg' packet.  */
>  ptid_t general_thread;
> 
> -int server_waiting;
> -
>  static int extended_protocol;
>  static int response_needed;
>  static int exit_requested;
> 
>  /* --once: Exit after the first connection has closed.  */
> -int run_once;
> -
> -int multi_process;
> -int report_fork_events;
> -int report_vfork_events;
> -int report_exec_events;
> -int report_thread_events;
> +int run_once = 0;

This "= 0" is a spurious change.  Remove it.

> 
>  /* Whether to report TARGET_WAITKING_NO_RESUMED events.  */
>  static int report_no_resumed;
> 
>  int non_stop;
> -int swbreak_feature;
> -int hwbreak_feature;
> -
> -/* True if the "vContSupported" feature is active.  In that case, GDB
> -   wants us to report whether single step is supported in the reply to
> -   "vCont?" packet.  */
> -static int vCont_supported;
> -
> -/* Whether we should attempt to disable the operating system's address
> -   space randomization feature before starting an inferior.  */
> -int disable_randomization = 1;
> 
>  static struct {
>    /* Set the PROGRAM_PATH.  Here we adjust the path of the provided

> 
>  /* A sub-class of 'struct notif_event' for stop, holding information
> @@ -193,6 +169,18 @@ static struct btrace_config current_btrace_conf;
> 
>  DEFINE_QUEUE_P (notif_event_p);
> 
> +/* The client remote protocol state. */
> +
> +static struct client_state *client_state;

Give the global different name to avoid conflict with
the type name.  Maybe "g_client_state" or "the_client_state".

> +
> +struct client_state &
> +get_client_state (void)

We use (void) in many places because of C heritage, but it's
no longer needed in C++, so:

  (void) -> ()

> +{
> +  struct client_state & cs = *client_state;
> +  return cs;

Simply write:

  return *g_client_state;

Thus:

 client_state &
 get_client_state ()
 {
   return *g_client_state;
 }

> @@ -1835,10 +1830,11 @@ handle_qxfer_btrace (const char *annex,
>               gdb_byte *readbuf, const gdb_byte *writebuf,
>               ULONGEST offset, LONGEST len)
>  {
> +  struct client_state & cs = get_client_state ();
>    static struct buffer cache;
>    struct thread_info *thread;
>    enum btrace_read_type type;
> -  int result;
> +  int result = 0;

This "= 0" looks like a spurious change.

> 
>    if (writebuf != NULL)
>      return -2;
> @@ -1846,20 +1842,20 @@ handle_qxfer_btrace (const char *annex,
>    if (ptid_equal (general_thread, null_ptid)
>        || ptid_equal (general_thread, minus_one_ptid))
>      {
> -      strcpy (own_buf, "E.Must select a single thread.");
> +      strcpy (cs.own_buf, "E.Must select a single thread.");
>        return -3;
>      }
> 
>    thread = find_thread_ptid (general_thread);
>    if (thread == NULL)
>      {
> -      strcpy (own_buf, "E.No such thread.");
> +      strcpy (cs.own_buf, "E.No such thread.");
>        return -3;
>      }
> 
>    if (thread->btrace == NULL)
>      {
> -      strcpy (own_buf, "E.Btrace not enabled.");
> +      strcpy (cs.own_buf, "E.Btrace not enabled.");
>        return -3;
>      }
> 
> @@ -1871,7 +1867,7 @@ handle_qxfer_btrace (const char *annex,
>      type = BTRACE_READ_DELTA;
>    else
>      {
> -      strcpy (own_buf, "E.Bad annex.");
> +      strcpy (cs.own_buf, "E.Bad annex.");
>        return -3;
>      }
> 
> @@ -1883,11 +1879,11 @@ handle_qxfer_btrace (const char *annex,
>      {
>        result = target_read_btrace (thread->btrace, &cache, type);
>        if (result != 0)
> -        memcpy (own_buf, cache.buffer, cache.used_size);
> +        memcpy (cs.own_buf, cache.buffer, cache.used_size);
>      }
>        CATCH (exception, RETURN_MASK_ERROR)
>      {
> -      sprintf (own_buf, "E.%s", exception.message);
> +      sprintf (cs.own_buf, "E.%s", exception.message);
>        result = -1;
>      }
>        END_CATCH
> @@ -1916,9 +1912,10 @@ handle_qxfer_btrace_conf (const char *annex,
>                gdb_byte *readbuf, const gdb_byte *writebuf,
>                ULONGEST offset, LONGEST len)
>  {
> +  struct client_state & cs = get_client_state ();
>    static struct buffer cache;
>    struct thread_info *thread;
> -  int result;
> +  int result = 0;

Ditto.

> 
>    if (writebuf != NULL)
>      return -2;

> @@ -2729,13 +2727,14 @@ static int
>  handle_pending_status (const struct thread_resume *resumption,
>                 struct thread_info *thread)
>  {
> +  struct client_state & cs = get_client_state ();
>    if (thread->status_pending_p)
>      {
>        thread->status_pending_p = 0;
> 
>        last_status = thread->last_status;
>        last_ptid = thread->id;

What's the plan for last_status/last_ptid?
Shouldn't those be per-client too?

> -      prepare_resume_reply (own_buf, last_ptid, &last_status);
> +      prepare_resume_reply (cs.own_buf, last_ptid, &last_status);
>        return 1;
>      }
>    return 0;
> @@ -2855,6 +2854,7 @@ err:
>  static void
>  resume (struct thread_resume *actions, size_t num_actions)
>  {
> +  struct client_state & cs = get_client_state ();
>    if (!non_stop)
>      {
>        /* Check if among the threads that GDB wants actioned, there's


> @@ -3577,6 +3581,9 @@ captured_main (int argc, char *argv[])
>  #endif
> 
>    current_directory = getcwd (NULL, 0);
> +  client_state = new (struct client_state);

Write:

  g_client_state = new client_state ();

Or better, at this point (in this patch) there's really no
need to allocate this object on the heap, so let's remove the
'new' line, and make the g_client_state global not-a-pointer:

 client_state g_client_state;

We can move it to the heap in the patch that actually 
allocates more than one such objects.

> diff --git a/gdb/gdbserver/server.h b/gdb/gdbserver/server.h
> index 5970431d8e..7518309988 100644
> --- a/gdb/gdbserver/server.h
> +++ b/gdb/gdbserver/server.h
> @@ -60,8 +60,6 @@ int vsnprintf(char *str, size_t size, const char *format, va_list ap);
>  #include "gdb_signals.h"
>  #include "target.h"
>  #include "mem-break.h"
> -#include "gdbthread.h"
> -#include "inferiors.h"
>  #include "environ.h"
> 
>  /* Target-specific functions */
> @@ -72,40 +70,14 @@ void initialize_low ();
> 
>  extern ptid_t cont_thread;
>  extern ptid_t general_thread;

Should these two be per client?  I'd think so off hand,
since they correspond to the Hc/Hg threads?

> @@ -162,4 +134,62 @@ extern target_waitstatus last_status;
>  extern ptid_t last_ptid;
>  extern unsigned long signal_pid;
> 
> +
> +/* Description of the client remote protocol state for the currently
> +   connected target.  */

What is a "target" in this context?  Did you mean "connected client"
instead of "connected target"?

> +
> +struct client_state
> +{
> +  /* From server.c */

I'm thinking that this "From foo.c" comments won't make much sense
after this is merged.  (Ideally we'd add descriptions for what
the fields are instead, but we don't have one today, so this can
go without.)

> +  int server_waiting = 0;
> +  int multi_process = 0;
> +  int report_fork_events = 0;
> +  int report_vfork_events = 0;
> +  int report_exec_events = 0;
> +  int report_thread_events = 0;
> +
> +  /* True if the "swbreak+" feature is active.  In that case, GDB wants
> +     us to report whether a trap is explained by a software breakpoint
> +     and for the server to handle PC adjustment if necessary on this
> +     target.  Only enabled if the target supports it.  */
> +  int swbreak_feature = 0;
> +  /* True if the "hwbreak+" feature is active.  In that case, GDB wants
> +     us to report whether a trap is explained by a hardware breakpoint.
> +     Only enabled if the target supports it.  */
> +  int hwbreak_feature = 0;
> +
> +  /* True if the "vContSupported" feature is active.  In that case, GDB
> +     wants us to report whether single step is supported in the reply to
> +     "vCont?" packet.  */
> +  int vCont_supported = 0;
> +
> +  /* Whether we should attempt to disable the operating system's address
> +     space randomization feature before starting an inferior.  */
> +  int disable_randomization = 0;
> +  int pass_signals[GDB_SIGNAL_LAST];

Add line break between the two above, they're unrelated.

> +  int program_signals[GDB_SIGNAL_LAST];
> +  int program_signals_p = 0;
> +  char *own_buf;

Add line break between the two above, they're unrelated.

> +
> +  /* from remote-utils.c */
> +  int remote_debug = 0;
> +  /* If true, then GDB has requested noack mode.  */
> +  int noack_mode = 0;
> +  /* If true, then we tell GDB to use noack mode by default.  */
> +  int transport_is_reliable = 0;
> +  /* from tracepoint.c */
> +  /* The traceframe to be used as the source of data to send back to
> +     GDB.  A value of -1 means to get data from the live program.  */
> +
> +  int current_traceframe = -1;
> +
> +  client_state ():
> +    own_buf ((char *) xmalloc (PBUFSIZ + 1)) {}

Contructors should go above the data fields.  Please make
this the first entry in the struct.  Also, formatting, please
write:

  client_state ()
    : own_buf ((char *) xmalloc (PBUFSIZ + 1)) 
  {}

> +};
> +
> +struct client_state& get_client_state (void);

Write:

 extern client_state &get_client_state ();

> diff --git a/gdb/gdbserver/spu-low.c b/gdb/gdbserver/spu-low.c
> index 5b880d2a42..d909966199 100644
> --- a/gdb/gdbserver/spu-low.c
> +++ b/gdb/gdbserver/spu-low.c
> @@ -418,6 +418,7 @@ spu_resume (struct thread_resume *resume_info, size_t n)
>  static ptid_t
>  spu_wait (ptid_t ptid, struct target_waitstatus *ourstatus, int options)
>  {
> +  struct client_state & cs = get_client_state ();
>    int pid = ptid_get_pid (ptid);
>    int w;
>    int ret;
> @@ -439,7 +440,7 @@ spu_wait (ptid_t ptid, struct target_waitstatus *ourstatus, int options)
> 
>    /* On the first wait, continue running the inferior until we are
>       blocked inside an spu_run system call.  */
> -  if (!server_waiting)
> +  if (!cs.server_waiting)

This is another case of what does this mean for
multiple clients?  One will be waiting, but another
one won't.  How can the backend know what to do?

I suspect that it'd be better to make the server_waiting
global be per-process instead (and likely renamed), though
I haven't studied in detail what are all the cases it is
used for.

> --- a/gdb/gdbserver/target.c
> +++ b/gdb/gdbserver/target.c
> @@ -52,7 +52,6 @@ prepare_to_access_memory (void)
>    /* Save the general thread value, since prepare_to_access_memory could change
>       it.  */
>    prev_general_thread = general_thread;
> -
>    if (the_target->prepare_to_access_memory != NULL)

Spurious change.

Thanks,
Pedro Alves