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

[Pedro Alves <palves@redhat.com>]

Hi Stan,

On 01/31/2018 03:41 AM, Stan Cox wrote:
>> consolidating the global variables in gdbserver into one of two
>> structures: client_state and server_state.  Client_state are those items
>> that have, or potentially could have, per client values, whereas
>> server_state are those items that have a per process value, i.e. will
>> have the same value for all clients.  

I've tried to take another fresh look at this today.
Sorry for the delay...

I'm afraid I am still confused about the client vs server states.
Why do we need server_state, if the value is the same for
all clients?

The structures in the code itself should have leading describing
comments, explaining their relations.

+/* Description of the remote protocol state for the currently
+   connected target.  This is per-target state, and independent of the
+   selected architecture. */
+
+struct server_state
+{

What is a "connected target" in this context?

I did not see description for client_state in the code.

I think the parts about multiple client states and the fd mapping
should be split to a separate patch.  The first patch would
only move the globals to structures, and start with a single
global instance of such structures.  Uses of map/set and other
data structure changes would be another patch.  Code to support
multiple connections would likely be another patch, etc.

 +enum packet_types { other_packet, vContc, vConts, vContt, vRun, vAttach, Hg, g_or_m, vStopped };
 +typedef enum packet_types packet_types;
 +
 +enum exit_types { no_exit, have_exit, sent_exit };
 +typedef enum exit_types exit_types;

These look like things that are not used yet and thus
should be split to a separate patch.

This patch, although large,
>> primarily moves global data into one of the two structures
> This is the same patch but with one major difference.  No use is made of access macros; instead the actual accesses are used.  For example, instead of a macro
>  #define current_thread    (get_client_state()->ss->current_thread_)
> through which all current_thread accesses are made
> instead:
>    client_state *cs = get_client_state();
>    ...
>    cs->ss->current_thread

Thanks.  I like the over-avoidance of macros.

I'm not super thrilled about the long "cs->ss->" chains though.

There is _always_ a client_state, right?  I.e., get_client_state()
never returns NULL?  If so, sounds like get_client_state()
could return a reference instead of a pointer?

> (Another possibility to make the patch smaller would be an additional
>  struct thread_info *current_thread = cs->ss->current_thread;

Yeah.  Since that would just be decomposing expressions, we could
make use of "auto &&" too, like:

   auto &&current_thread = client_state ()->ss->current_thread;
   auto &&all_threads = client_state ()->ss->all_threads;

Another option would be to add some convenient wrapper functions
for the most common cases.  Like

thread_info *current_thread ()
{
  return client_state ()->ss->current_thread;
}

>  ... )
> Distribution of global variables with more than 10 accesses:
> program_name 11
> last_ptid 11
> program_args 14
> remote_debug 22
> non_stop 23
> general_thread 23
> last_status 47
> own_buf 106
> current_thread 248



> 
> The patch below is present in the branch:
>  ssh://sourceware.org/git/archer.git::scox/gdbserver-multi-client
> (it is large, sorry about that, but much of it is mechanical)
> 
> diff --git a/gdb/gdbserver/event-loop.c b/gdb/gdbserver/event-loop.c
> index eec0bcf3af6..382d9b3efa7 100644
> --- a/gdb/gdbserver/event-loop.c
> +++ b/gdb/gdbserver/event-loop.c
> @@ -385,6 +385,9 @@ delete_file_handler (gdb_fildes_t fd)
>      ;
>        prev_ptr->next_file = file_ptr->next_file;
>      }
> +
> +  struct multi_client_states *client_states = get_client_states();
> +  client_states->delete_client_state (fd);
>    free (file_ptr);

Formatting nit: there should be a space before "(" in 
"get_client_states()" (and all function calls).

Note the patch has clear signs of global sed'ing. :-)
E.g., obviously this is incorrect:

 @@ -628,7 +628,7 @@ Could not convert the expanded inferior cwd to wide-char."));
     process with the process list.  */
  static int
  win32_create_inferior (const char *program,
 -                      const std::vector<char *> &program_args)
 +                      const std::vector<char *> &cs->program_args)
  {
 
Here too:

 --- a/gdb/gnulib/import/error.c
 +++ b/gdb/gnulib/import/error.c
 @@ -65,7 +65,7 @@ unsigned int error_message_count;
  #ifdef _LIBC
  /* In the GNU C library, there is a predefined variable for this.  */
  
 -# define program_name program_invocation_name
 +# define cs->program_name program_invocation_name
  # include <errno.h>
  # include <limits.h>
  # include <libio/libioP.h>
 @@ -115,7 +115,7 @@ int strerror_r ();
  #  endif
  # endif
  
 -#define program_name getprogname ()
 +#define cs->program_name getprogname ()

The patch should not be changing anything under gnulib/ at all.

There may be more cases.



> +  client_state () {};

Spurious ;.  

What is initializing all the fields in the structure?
Can we use in-class initialization?

> +  client_state (gdb_fildes_t, client_state*);
> +};

This needs a comment/description.  What is this ctor for?

> 
> 
> struct multi_client_states
> {
> public:
>   std::map<gdb_fildes_t,client_state*> cs;
>   client_state *current_cs;
> 
>   client_state* get_client_state (void) { return current_cs; }
>   void set_current_client (client_state* p_cs) { current_cs = p_cs; }
> 
>   client_state * set_client_state (gdb_fildes_t);
>   void free_client_state (client_state *cs);
>   void delete_client_state (gdb_fildes_t fd);
> };

Should this structure be part of this patch?
free_client_state does not look to be defined anywhere,
for instance.

>  }
> 
> diff --git a/gdb/gdbserver/fork-child.c b/gdb/gdbserver/fork-child.c
> index 831e7e13100..74fa5e424c7 100644
> --- a/gdb/gdbserver/fork-child.c
> +++ b/gdb/gdbserver/fork-child.c
> @@ -44,6 +44,8 @@ restore_old_foreground_pgrp (void)
>  void
>  prefork_hook (const char *args)
>  {
> +  client_state *cs = get_client_state();
> +
>    if (debug_threads)
>      {
>        debug_printf ("args: %s\n", args);
> @@ -57,7 +59,7 @@ prefork_hook (const char *args)
> 
>    /* Clear this so the backend doesn't get confused, thinking
>       CONT_THREAD died, and it needs to resume all threads.  */
> -  cont_thread = null_ptid;
> +  cs->ss->cont_thread = null_ptid;
>  }
> 
>  /* See nat/fork-inferior.h.  */
> @@ -97,6 +99,7 @@ void
>  post_fork_inferior (int pid, const char *program)
>  {
>  #ifdef SIGTTOU
> +  client_state *cs = get_client_state();
>    signal (SIGTTOU, SIG_IGN);
>    signal (SIGTTIN, SIG_IGN);
>    terminal_fd = fileno (stderr);
> @@ -106,10 +109,10 @@ post_fork_inferior (int pid, const char *program)
>  #endif
> 
>    startup_inferior (pid, START_INFERIOR_TRAPS_EXPECTED,
> -            &last_status, &last_ptid);
> -  current_thread->last_resume_kind = resume_stop;
> -  current_thread->last_status = last_status;
> -  signal_pid = pid;
> +            &cs->ss->last_status, &cs->ss->last_ptid);
> +  cs->ss->current_thread->last_resume_kind = resume_stop;
> +  cs->ss->current_thread->last_status = cs->ss->last_status;
> +  cs->ss->signal_pid = pid;
>    target_post_create_inferior ();
>    fprintf (stderr, "Process %s created; pid = %d\n", program, pid);
>    fflush (stderr);
> diff --git a/gdb/gdbserver/gdbthread.h b/gdb/gdbserver/gdbthread.h
> index 9a8c72e780e..c5bd1175f22 100644
> --- a/gdb/gdbserver/gdbthread.h
> +++ b/gdb/gdbserver/gdbthread.h
> @@ -73,8 +73,6 @@ struct thread_info
>    struct btrace_target_info *btrace;
>  };
> 
> -extern std::list<thread_info *> all_threads;
> -
>  void remove_thread (struct thread_info *thread);
>  struct thread_info *add_thread (ptid_t ptid, void *target_data);
> 
> @@ -95,9 +93,9 @@ template <typename Func>
>  static thread_info *
>  find_thread (Func func)
>  {
> -  std::list<thread_info *>::iterator next, cur = all_threads.begin ();
> +  std::list<thread_info *>::iterator next, cur = get_client_state()->ss->all_threads.begin ();

Please watch out for too-long lines.  The hard limit is 80 cols.

This would be case where doing instead:

 auto &&all_threads = get_client_state()->ss->all_threads;

would avoid the formatting issues, as well as ...

> 
> -  while (cur != all_threads.end ())
> +  while (cur != get_client_state()->ss->all_threads.end ())

... all this indirection for each iteration.

>      {
>        next = cur;
>        next++;
> @@ -141,9 +139,9 @@ template <typename Func>
>  static void
>  for_each_thread (Func func)
>  {
> -  std::list<thread_info *>::iterator next, cur = all_threads.begin ();
> +  std::list<thread_info *>::iterator next, cur = get_client_state()->ss->all_threads.begin ();
> 
> -  while (cur != all_threads.end ())
> +  while (cur != get_client_state()->ss->all_threads.end ())

Ditto.  Etc.

> +int
> +get_remote_desc (void)
> +{
> +  return remote_desc;
> +}

Is "remote_desc" a global?  If so, why do we need the wrapper function?

> @@ -1474,45 +1484,45 @@ look_up_one_symbol (const char *name, CORE_ADDR *addrp, int may_ask_gdb)
>       while it figures out the address of the symbol.  */
>    while (1)
>      {
> -      if (own_buf[0] == 'm')
> +      if (cs->own_buf[0] == 'm')
>      {
>        CORE_ADDR mem_addr;
> -      unsigned char *mem_buf;
> +      unsigned char *mem_buffer;

Renames that were necessary because of the macros
conflicts can/should be reverted.



> diff --git a/gdb/gdbserver/server.h b/gdb/gdbserver/server.h
> index 5970431d8ed..8e1459f74e2 100644
> --- a/gdb/gdbserver/server.h
> +++ b/gdb/gdbserver/server.h
> @@ -60,52 +60,19 @@ 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"
> +#include <list>
> +#include <map>
> 
>  /* Target-specific functions */
> 
>  void initialize_low ();
> 
> -/* Public variables in server.c */
> -
> -extern ptid_t cont_thread;
> -extern ptid_t general_thread;
> -
> -extern int server_waiting;
> -extern int pass_signals[];
> -extern int program_signals[];
> -extern int program_signals_p;
> -
>  extern int disable_packet_vCont;
>  extern int disable_packet_Tthread;
>  extern int disable_packet_qC;
>  extern int disable_packet_qfThreadInfo;
> 
> -extern char *own_buf;
> -
> -extern int run_once;
> -extern int multi_process;
> -extern int report_fork_events;
> -extern int report_vfork_events;
> -extern int report_exec_events;
> -extern int report_thread_events;
> -extern int non_stop;
> -
> -/* 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.  */
> -extern int swbreak_feature;
> -
> -/* 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.  */
> -extern int hwbreak_feature;
> -
> -extern int disable_randomization;
> -
>  #if USE_WIN32API
>  #include <winsock2.h>
>  typedef SOCKET gdb_fildes_t;
> @@ -158,8 +125,161 @@ extern void post_fork_inferior (int pid, const char *program);
>  /* Get the gdb_environ being used in the current session.  */
>  extern gdb_environ *get_environ ();
> 
> -extern target_waitstatus last_status;
> -extern ptid_t last_ptid;
> -extern unsigned long signal_pid;
> +// extern ptid_t cs->ss->last_ptid;
> +// extern unsigned long cs->ss->signal_pid;
> 
>  #endif /* SERVER_H */
> +
> +/* Description of the remote protocol state for the currently
> +   connected target.  This is per-target state, and independent of the
> +   selected architecture. */
> +
> +struct server_state
> +{
> +  int attach_count;

This looks like not used in this patch yet.

> +  /* From server.c */
> +  /* 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
> +     CONT_THREAD can be null_ptid for no `Hc' thread, minus_one_ptid for
> +     resuming all threads of the process (again, `Hc' isn't used for
> +     multi-process), or a specific thread ptid_t.  */
> +  ptid_t cont_thread;
> +  /* The thread set with an `Hg' packet.  */
> +  ptid_t general_thread;
> +  /* The PID of the originally created or attached inferior.  Used to
> +     send signals to the process when GDB sends us an asynchronous interrupt
> +     (user hitting Control-C in the client), and to wait for the child to exit
> +     when no longer debugging it.  */
> +
> +  unsigned long signal_pid;
> +  /* Last status reported to GDB.  */
> +  struct target_waitstatus last_status;
> +  /* Was last status an exit status? (sticky if yes) */
> +  int last_status_exited;

Ditto.

> +  ptid_t last_ptid;
> +  unsigned char *mem_buf;
> +
> +  /* from remote-utils.c */
> +  /* Internal buffer used by readchar.
> +     These are global to readchar because reschedule_remote needs to be
> +     able to tell whether the buffer is empty.  */
> +  unsigned char readchar_buf[BUFSIZ];
> +  int readchar_bufcnt;
> +  unsigned char *readchar_bufp;
> +  /* from inferiors.c */
> +  std::list<process_info *> all_processes;
> +  std::list<thread_info *> all_threads;
> +
> +  struct thread_info *current_thread;
> +};

But as I asked before, do we need this structure if
there is only ever one server?

> +
> +typedef struct server_state server_state;
> +
> +
> +enum packet_types { other_packet, vContc, vConts, vContt, vRun, vAttach, Hg, g_or_m, vStopped };
> +typedef enum packet_types packet_types;
> +
> +enum exit_types { no_exit, have_exit, sent_exit };
> +typedef enum exit_types exit_types;
> +
> +
> +struct client_state
> +{
> +  gdb_fildes_t file_desc;
> +  int attached_to_client;

More things not used in this patch.  There could be
more.

> +  int packet_type;
> +  int last_packet_type;
> +  int pending;
> +  int nonstop_pending;
> +  int catch_syscalls;
> +  ptid_t last_cont_ptid;
> +  ptid_t new_general_thread;
> +
> +  /* From server.c */
> +  int server_waiting;
> +
> +  int extended_protocol;
> +  int response_needed;
> +  int exit_requested;
> +
> +  /* --once: Exit after the first connection has closed.  */
> +  int run_once;

Is this one really per client?

> +
> +  int multi_process;
> +  int report_fork_events;
> +  int report_vfork_events;
> +  int report_exec_events;
> +  int report_thread_events;
> +  /* Whether to report TARGET_WAITKING_NO_RESUMED events.  */
> +  int report_no_resumed;
> +  int non_stop;

So what will happen if one client is in non-stop mode, and another
is in all-stop mode?  There are checks for cs->non_stop in the target
backend (linux-low.c).  I suspect that will not work correctly.  In
such cases, I wonder why make the global be per-client?

> +  /* 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;
> +  /* 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;
> +
> +  /* 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;
> +
> +  /* Whether we should attempt to disable the operating system's address
> +     space randomization feature before starting an inferior.  */
> +  int disable_randomization;
> +
> +  char *program_name = NULL;
> +  std::vector<char *> program_args;
> +  std::string wrapper_argv;
> +
> +  int packet_length;
> +
> +  int pass_signals[GDB_SIGNAL_LAST];
> +  int program_signals[GDB_SIGNAL_LAST];
> +  int program_signals_p;
> +
> +  char *notify_buffer;
> +  /* Renamed from own_buf to avoid macro name conflict with a common local variable name */
> +  char *own_buf;

As mentioned, such renames can be reverted.

> +
> +  /* from remote-utils.c */
> +  int remote_debug;
> +  /* If true, then GDB has requested noack mode.  */
> +  int noack_mode;
> +  /* If true, then we tell GDB to use noack mode by default.  */
> +  int transport_is_reliable;
> +
> +  server_state *ss;
> +
> +  client_state () {};
> +  client_state (gdb_fildes_t, client_state*);
> +};
> +
> +typedef struct client_state client_state;
> +

> diff --git a/gdb/nat/linux-personality.h b/gdb/nat/linux-personality.h
> index 229cc3ef1c9..3f2e0724c49 100644
> --- a/gdb/nat/linux-personality.h
> +++ b/gdb/nat/linux-personality.h
> @@ -27,7 +27,7 @@ public:
>    /* Disable the inferior's address space randomization if
>       DISABLE_RANDOMIZATION is not zero and if we have
>       <sys/personality.h>. */
> -  maybe_disable_address_space_randomization (int disable_randomization);
> +  maybe_disable_address_space_randomization (int p_disable_randomization);

This looks like a spurious change.

On your next update, please make sure to wrap the text in the
proposed commit log to 74-columns, so that it's easier to read
in "git show", "git log", etc.

Please address the comments above, squash the patches together,
rebased on current master, and repost a new self-contained
version (including updated proposed git commit log) with
"PATCH v2" in the subject line.  That should make it easier
to iterate on, and see if someone else has further
comments, too.

Thanks for the patience,
Pedro Alves