[v2,2/2] consolidate gdbserver global data

  This patch consolidates global data into two structures.  Data that is 
related to the client is defined in the client_state structure, 
analogous to the remote_state structure in gdb/remote.c.   Data that is 
related to the inferior running under gdbserver is defined in the 
server_status structure.  This delineation is to allow, as a future 
expansion, the ability to have multiple clients, for example gdb and 
strace, connected to the same inferior.  To maintain source 
compatibility, macros are defined so that global variable references do 
not have to be changed to be structure member references.  New access 
functions are:  new_client_state, which sets up a single client state 
and get_client_state which returns it  They are analogous to 
new_remote_state and get_remote_state in gdb/remote.c.   Going forward 
this infrastructure can be expanded so that multiple clients can be 
supported.

gdb/gdbserver/Changelog

     * server.h (struct server_state, struct client_state):  New.
     Move global data items here and remove the extern declarations.
     Add defines for struct members for source compatibility.

     * server.c (cont_thread, server_waiting, extended_protocol)
     (response_needed, exit_requested, run_once, multi_process)
     (report_fork_events, report_vfork_events, report_exec_events)
     (non_stop, swbreak_feature, hwbreak_feature, vCont_supported)
     (disable_randomization, program_argv, wrapper_argv, pass_signals)
     (program_signals, signal_pid, disable_packet_vCont)
     (disable_packet_Tthread, disable_packet_qC)
     (disable_packet_qfThreadInfo, last_status, last_ptid, own_buf)
     (mem_buf):  Moved to struct client_status, struct server_status
     (new_client_state): Set the client_state struct.
     (get_client_state): Return the client_state.
     (captured_main):  Call new_client_state.

     * inferiors.h (all_processes, all_threads, current_thread)  Move
     to client_status


  typedef SOCKET gdb_fildes_t;
@@ -133,4 +97,143 @@ extern void discard_queued_stop_replies (ptid_t ptid);
     as large as the largest register set supported by gdbserver.  */
  #define PBUFSIZ 16384

+
+/* 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
+{
+  /* 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_;
+  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 */
+  struct inferior_list all_processes_;
+  struct inferior_list all_threads_;
+  struct thread_info *current_thread_;
+};
+
+typedef struct server_state server_state;
+
+struct client_state
+{
+  /* 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_;
+
+  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_;
+  /* 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_;
+
+  int disable_packet_vCont_;
+  int disable_packet_Tthread_;
+  int disable_packet_qC_;
+  int disable_packet_qfThreadInfo_;
+
+  char **program_argv_, **wrapper_argv_;
+
+  int pass_signals_[GDB_SIGNAL_LAST];
+  int program_signals_[GDB_SIGNAL_LAST];
+  int program_signals_p_;
+  char *own_buffer_;
+  struct server_state *ss;
+};
+
+typedef struct client_state client_state;
+
+struct client_state * get_client_state ();
+
+#define cont_thread (get_client_state()->ss->cont_thread_)
+#define general_thread (get_client_state()->ss->general_thread_)
+#define signal_pid (get_client_state()->ss->signal_pid_)
+#define last_status (get_client_state()->ss->last_status_)
+#define last_ptid (get_client_state()->ss->last_ptid_)
+#define mem_buf (get_client_state()->ss->mem_buf_)
+#define readchar_buf (get_client_state()->ss->readchar_buf_)
+#define readchar_bufcnt (get_client_state()->ss->readchar_bufcnt_)
+#define readchar_bufp (get_client_state()->ss->readchar_bufp_)
+#define all_processes (get_client_state()->ss->all_processes_)
+#define all_threads (get_client_state()->ss->all_threads_)
+#define current_thread (get_client_state()->ss->current_thread_)
+#define server_waiting (get_client_state()->server_waiting_)
+#define extended_protocol (get_client_state()->extended_protocol_)
+#define response_needed (get_client_state()->response_needed_)
+#define exit_requested (get_client_state()->exit_requested_)
+#define run_once (get_client_state()->run_once_)
+#define multi_process (get_client_state()->multi_process_)
+#define report_fork_events (get_client_state()->report_fork_events_)
+#define report_vfork_events (get_client_state()->report_vfork_events_)
+#define report_exec_events (get_client_state()->report_exec_events_)
+#define report_thread_events (get_client_state()->report_thread_events_)
+#define report_no_resumed (get_client_state()->report_no_resumed_)
+#define non_stop (get_client_state()->non_stop_)
+#define swbreak_feature (get_client_state()->swbreak_feature_)
+#define hwbreak_feature (get_client_state()->hwbreak_feature_)
+#define vCont_supported (get_client_state()->vCont_supported_)
+#define disable_randomization (get_client_state()->disable_randomization_)
+#define disable_packet_vCont (get_client_state()->disable_packet_vCont_)
+#define disable_packet_Tthread 
(get_client_state()->disable_packet_Tthread_)
+#define disable_packet_qC (get_client_state()->disable_packet_qC_)
+#define disable_packet_qfThreadInfo 
(get_client_state()->disable_packet_qfThreadInfo_)
+#define program_argv (get_client_state()->program_argv_)
+#define wrapper_argv (get_client_state()->wrapper_argv_)
+#define pass_signals (get_client_state()->pass_signals_)
+#define program_signals (get_client_state()->program_signals_)
+#define program_signals_p (get_client_state()->program_signals_p_)
+#define own_buffer (get_client_state()->own_buffer_)
+
  #endif /* SERVER_H */
  

Hi Stan,

On 12/17/2015 09:15 PM, Stan Cox wrote:
> This patch consolidates global data into two structures.  Data that is 
> related to the client is defined in the client_state structure, 
> analogous to the remote_state structure in gdb/remote.c.   Data that is 
> related to the inferior running under gdbserver is defined in the 
> server_status structure.  This delineation is to allow, as a future 
> expansion, the ability to have multiple clients, for example gdb and 
> strace, connected to the same inferior.  To maintain source 
> compatibility, macros are defined so that global variable references do 
> not have to be changed to be structure member references.  New access 
> functions are:  new_client_state, which sets up a single client state 
> and get_client_state which returns it  They are analogous to 
> new_remote_state and get_remote_state in gdb/remote.c.   Going forward 
> this infrastructure can be expanded so that multiple clients can be 
> supported.

Sorry, I tried looking at this several times, but I always end up
very much confused on the client/server split.  :-/

Most things in server_state seem specific to the currently
connected _GDB/client_ to me.  E.g, the thread set with Hg.
Likewise readchar_buf.  Etc.

Can you give an example on how this will work out in the end
when you have multiple instances of these objects?

- When will we have multiple server_state objects?

- When will we have multiple client_state objects?

- What are the relations between those objects?

We should also end up with a concise comment at the top
of these structs definitions explaining what they are for.
struct client_state has no intro comment, afaics.

Thanks,
Pedro Alves
  

The thinking was that it would be like, not a perfect analogy, database 
normalization.  So, logically, there would be one server structure per 
inferior that contains those items that are common to all remote clients 
accessing that inferior.  Each server would have one or more associated 
client structures, one per remote client.  So the server_state is, 
roughly, the gdbserver view of things and the client_state is the remote 
gdb view of things.

So at a high level
			/ [client 1, fd 4]
  [server for inferior 1]
  	     	        \ [client 2, fd 9]

This is "inverted" in the current implementation model.  So there is a 
list of client structures that are "indexed" by the file descriptor 
corresponding to the client.  So in this example there is one server 
state that has two client states.  The file_desc is the client "key" and 
finding a client_state involves searching the list of clients (via 
client_state.next) for the client_state that matches that file_desc.

  client_states.first
  -> /* client 1 --- fd 4 */ struct client_state
     file_desc = 4,
     attached_to_client = 1,
     packet_type = 0,
     last_packet_type = 4,
      /* enum packet_types { other_packet, vContc, vConts, vContt,
		    vStopped, vRun, vAttach }; */
     pending = 0,
      /* enum pending_types {none_pending, pending_waitee,
              pending_cont_waiter,pending_step_waiter,pending_stop}; */
     server_waiting_ = 0,
     extended_protocol_ = 1,
     response_needed_ = 0,
     exit_requested_ = 0,
     run_once_ = 0,
     multi_process_ = 1,
     report_fork_events_ = 1,
     report_vfork_events_ = 1,
     non_stop_ = 1,
     swbreak_feature_ = 1,
     hwbreak_feature_ = 1,
     disable_randomization_ = 1,
     program_argv_ = 0x683840,
     wrapper_argv_ = 0x0,
     packet_length_ = 17,
     pass_signals_ =     {0 <repeats 14 times>,
     program_signals_ =     {1,
     program_signals_p_ = 1,
     in_buffer_ = 0x6838a0 "z0,7ffff7df0f57,", <incomplete sequence \327>,
     own_buffer_ = 0x6848e0 "OK",
     client_breakpoints = 0x6836a0,
     ss = 0x681430,
     next = 0x688920

  client_states.first.next
  -> /* client 2 --- fd 9 */ struct client_state
     ...
     ss = 0x681430,
     next = 0

  client_states.first.ss
  -> /* server -- ss = 0x681430 */ struct server_state
     attach_count_ = 2, /* number of clients for this server_state */
     cont_thread_ = {...}
     general_thread_ = {...}
     signal_pid_ = 32251,
     last_status_ = {
       kind = TARGET_WAITKIND_NO_RESUMED,
       value = {
          integer = 5,
          sig = GDB_SIGNAL_TRAP,
          related_pid = {...}
       ...
     last_status_exited = 0,
     last_ptid_ = {...}
     mem_buf_ = 0x67cc70 "..."
     readchar_buf_ =     "$z0,7ffff7df0f57,1#050"...,
     readchar_bufcnt_ = 0,
     readchar_bufp_ = 0x6814cd "0,fff#03b;"...,
     all_processes_ = { head = 0x683a80, tail = 0x683a80  },
     all_threads_ = { head = 0x683cb0, tail = 0x683cb0  },
     current_thread_ = 0x683cb0

  client_states.current_cs
  -> /* there is always a "current" client_state
        this points to the "current" client_state */

Example "queries"
1) What is client 1's in_buffer?
    - Find client 1's fd in the client list
    - client_state.in_buffer
2) What is client 2's last_status?
    - Find Client 2's fd in the client list
    - client_state.ss.last_status
3) What is the current client_state?
    -client_states.current_cs

Thanks for looking Pedro.  I'm wide open to any ways to improve this to 
make it easier to understand and use.