[PATCH v2 00/16]

[PATCH v2 00/16]

[Tom Tromey]

Here's an updated version of my series to move remote.c global state
into struct remote_state.

Original note here:

    http://sourceware.org/ml/gdb-patches/2013-06/msg00605.html

I believe this version addresses all the comments I received.

Tom

[PATCH 05/16] push general_thread and continue_thread into struct remote_state

[Tom Tromey]

This moves the globals general_thread and continue_thread into
remote_state.

	* remote.c (struct remote_state) <general_thread, continue_thread>:
	New fields.
	(general_thread, continue_thread): Remove.
	(record_currthread, set_thread, set_general_process)
	(remote_open_1, extended_remote_attach_1, remote_wait_as)
	(extended_remote_mourn_1): Update.
---
 gdb/remote.c | 37 +++++++++++++++++--------------------
 1 file changed, 17 insertions(+), 20 deletions(-)

diff --git a/gdb/remote.c b/gdb/remote.c
index 9dfc73b..6d9478f 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -183,8 +183,6 @@ static ptid_t remote_current_thread (ptid_t oldptid);
 
 static void remote_find_new_threads (void);
 
-static void record_currthread (ptid_t currthread);
-
 static int fromhex (int a);
 
 static int putpkt_binary (char *buf, int cnt);
@@ -373,6 +371,11 @@ struct remote_state
      remote_open knows that we don't have a file open when the program
      starts.  */
   struct serial *remote_desc;
+
+  /* These are the threads which we last sent to the remote system.  The
+     TID member will be -1 for all or -2 for not sent yet.  */
+  ptid_t general_thread;
+  ptid_t continue_thread;
 };
 
 /* Private data that we'll store in (struct thread_info)->private.  */
@@ -1439,12 +1442,6 @@ static ptid_t magic_null_ptid;
 static ptid_t not_sent_ptid;
 static ptid_t any_thread_ptid;
 
-/* These are the threads which we last sent to the remote system.  The
-   TID member will be -1 for all or -2 for not sent yet.  */
-
-static ptid_t general_thread;
-static ptid_t continue_thread;
-
 /* This is the traceframe which we last selected on the remote system.
    It will be -1 if no traceframe is selected.  */
 static int remote_traceframe_number = -1;
@@ -1649,9 +1646,9 @@ demand_private_info (ptid_t ptid)
    3) Successful execution of set thread */
 
 static void
-record_currthread (ptid_t currthread)
+record_currthread (struct remote_state *rs, ptid_t currthread)
 {
-  general_thread = currthread;
+  rs->general_thread = currthread;
 }
 
 static char *last_pass_packet;
@@ -1775,7 +1772,7 @@ static void
 set_thread (struct ptid ptid, int gen)
 {
   struct remote_state *rs = get_remote_state ();
-  ptid_t state = gen ? general_thread : continue_thread;
+  ptid_t state = gen ? rs->general_thread : rs->continue_thread;
   char *buf = rs->buf;
   char *endbuf = rs->buf + get_remote_packet_size ();
 
@@ -1795,9 +1792,9 @@ set_thread (struct ptid ptid, int gen)
   putpkt (rs->buf);
   getpkt (&rs->buf, &rs->buf_size, 0);
   if (gen)
-    general_thread = ptid;
+    rs->general_thread = ptid;
   else
-    continue_thread = ptid;
+    rs->continue_thread = ptid;
 }
 
 static void
@@ -1832,7 +1829,7 @@ set_general_process (void)
 
   /* We only need to change the remote current thread if it's pointing
      at some other process.  */
-  if (ptid_get_pid (general_thread) != ptid_get_pid (inferior_ptid))
+  if (ptid_get_pid (rs->general_thread) != ptid_get_pid (inferior_ptid))
     set_general_thread (inferior_ptid);
 }
 
@@ -4347,8 +4344,8 @@ remote_open_1 (char *name, int from_tty,
   rs->waiting_for_stop_reply = 0;
   rs->ctrlc_pending_p = 0;
 
-  general_thread = not_sent_ptid;
-  continue_thread = not_sent_ptid;
+  rs->general_thread = not_sent_ptid;
+  rs->continue_thread = not_sent_ptid;
   remote_traceframe_number = -1;
 
   /* Probe for ability to use "ThreadInfo" query, as required.  */
@@ -4566,7 +4563,7 @@ extended_remote_attach_1 (struct target_ops *target, char *args, int from_tty)
 	inferior_ptid = pid_to_ptid (pid);
 
       /* Invalidate our notion of the remote current thread.  */
-      record_currthread (minus_one_ptid);
+      record_currthread (rs, minus_one_ptid);
     }
   else
     {
@@ -6065,13 +6062,13 @@ remote_wait_as (ptid_t ptid, struct target_waitstatus *status, int options)
 	   && status->kind != TARGET_WAITKIND_SIGNALLED)
     {
       if (!ptid_equal (event_ptid, null_ptid))
-	record_currthread (event_ptid);
+	record_currthread (rs, event_ptid);
       else
 	event_ptid = inferior_ptid;
     }
   else
     /* A process exit.  Invalidate our notion of current thread.  */
-    record_currthread (minus_one_ptid);
+    record_currthread (rs, minus_one_ptid);
 
   return event_ptid;
 }
@@ -7922,7 +7919,7 @@ extended_remote_mourn_1 (struct target_ops *target)
 
      To keep things simple, we always invalidate our notion of the
      current thread.  */
-  record_currthread (minus_one_ptid);
+  record_currthread (rs, minus_one_ptid);
 
   /* Unlike "target remote", we do not want to unpush the target; then
      the next time the user says "run", we won't be connected.  */
-- 
1.8.1.4

[PATCH 02/16] make remote_protocol_features "const"

[Tom Tromey]

This is a trivial patch to make remote_protocol_features "const".

	* remote.c (remote_protocol_features): Now const.
---
 gdb/remote.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gdb/remote.c b/gdb/remote.c
index e40dc4e..430ff2b 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -3968,7 +3968,7 @@ remote_augmented_libraries_svr4_read_feature
   rs->augmented_libraries_svr4_read = (support == PACKET_ENABLE);
 }
 
-static struct protocol_feature remote_protocol_features[] = {
+static const struct protocol_feature remote_protocol_features[] = {
   { "PacketSize", PACKET_DISABLE, remote_packet_size, -1 },
   { "qXfer:auxv:read", PACKET_DISABLE, remote_supported_packet,
     PACKET_qXfer_auxv },
-- 
1.8.1.4

[PATCH 14/16] move async_client_callback and async_client_context into remote_state

[Tom Tromey]

This moves async_client_callback and async_client_context into
remote_state.

	* remote.c (struct remote_state) <async_client_callback,
	async_client_context>:	New fields.
	(async_client_callback, async_client_context): Remove.
	(remote_async_serial_handler, remote_async): Update.
---
 gdb/remote.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/gdb/remote.c b/gdb/remote.c
index 381d596..8641094 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -406,6 +406,10 @@ struct remote_state
      and set to false when the target fails to recognize it).  */
   int use_threadinfo_query;
   int use_threadextra_query;
+
+  void (*async_client_callback) (enum inferior_event_type event_type,
+				 void *context);
+  void *async_client_context;
 };
 
 /* Private data that we'll store in (struct thread_info)->private.  */
@@ -11638,17 +11642,16 @@ remote_is_async_p (void)
    will be able to delay notifying the client of an event until the
    point where an entire packet has been received.  */
 
-static void (*async_client_callback) (enum inferior_event_type event_type,
-				      void *context);
-static void *async_client_context;
 static serial_event_ftype remote_async_serial_handler;
 
 static void
 remote_async_serial_handler (struct serial *scb, void *context)
 {
+  struct remote_state *rs = context;
+
   /* Don't propogate error information up to the client.  Instead let
      the client find out about the error by querying the target.  */
-  async_client_callback (INF_REG_EVENT, async_client_context);
+  rs->async_client_callback (INF_REG_EVENT, rs->async_client_context);
 }
 
 static void
@@ -11665,9 +11668,9 @@ remote_async (void (*callback) (enum inferior_event_type event_type,
 
   if (callback != NULL)
     {
-      serial_async (rs->remote_desc, remote_async_serial_handler, NULL);
-      async_client_callback = callback;
-      async_client_context = context;
+      serial_async (rs->remote_desc, remote_async_serial_handler, rs);
+      rs->async_client_callback = callback;
+      rs->async_client_context = context;
     }
   else
     serial_async (rs->remote_desc, NULL, NULL);
-- 
1.8.1.4

Re: [PATCH 14/16] move async_client_callback and async_client_context into remote_state

[Pedro Alves]

On 06/28/2013 06:40 PM, Tom Tromey wrote:
> 
> 	* remote.c (struct remote_state) <async_client_callback,
> 	async_client_context>:	New fields.

                              ^^
Spurious double space.

-- 
Pedro Alves

Re: [PATCH 14/16] move async_client_callback and async_client_context into remote_state

[Tom Tromey]

>> * remote.c (struct remote_state) <async_client_callback,
>> async_client_context> :	New fields.

Pedro>                               ^^
Pedro> Spurious double space.

Fixed, thanks.

Tom

[PATCH 07/16] push last_pass_packet into struct remote_state

[Tom Tromey]

This moves the global last_pass_packet into remote_state.

	* remote.c (struct remote_state) <last_pass_packet>:
	New field.
	(last_pass_packet): Remove.
	(remote_pass_signals, remote_open_1): Update.
---
 gdb/remote.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/gdb/remote.c b/gdb/remote.c
index 3ef99df..136c3d9 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -380,6 +380,8 @@ struct remote_state
   /* This is the traceframe which we last selected on the remote system.
      It will be -1 if no traceframe is selected.  */
   int remote_traceframe_number;
+
+  char *last_pass_packet;
 };
 
 /* Private data that we'll store in (struct thread_info)->private.  */
@@ -1652,8 +1654,6 @@ record_currthread (struct remote_state *rs, ptid_t currthread)
   rs->general_thread = currthread;
 }
 
-static char *last_pass_packet;
-
 /* If 'QPassSignals' is supported, tell the remote stub what signals
    it can simply pass through to the inferior without reporting.  */
 
@@ -1664,6 +1664,7 @@ remote_pass_signals (int numsigs, unsigned char *pass_signals)
     {
       char *pass_packet, *p;
       int count = 0, i;
+      struct remote_state *rs = get_remote_state ();
 
       gdb_assert (numsigs < 256);
       for (i = 0; i < numsigs; i++)
@@ -1689,17 +1690,16 @@ remote_pass_signals (int numsigs, unsigned char *pass_signals)
 	    }
 	}
       *p = 0;
-      if (!last_pass_packet || strcmp (last_pass_packet, pass_packet))
+      if (!rs->last_pass_packet || strcmp (rs->last_pass_packet, pass_packet))
 	{
-	  struct remote_state *rs = get_remote_state ();
 	  char *buf = rs->buf;
 
 	  putpkt (pass_packet);
 	  getpkt (&rs->buf, &rs->buf_size, 0);
 	  packet_ok (buf, &remote_protocol_packets[PACKET_QPassSignals]);
-	  if (last_pass_packet)
-	    xfree (last_pass_packet);
-	  last_pass_packet = pass_packet;
+	  if (rs->last_pass_packet)
+	    xfree (rs->last_pass_packet);
+	  rs->last_pass_packet = pass_packet;
 	}
       else
 	xfree (pass_packet);
@@ -4283,8 +4283,8 @@ remote_open_1 (char *name, int from_tty,
   target_preopen (from_tty);
 
   /* Make sure we send the passed signals list the next time we resume.  */
-  xfree (last_pass_packet);
-  last_pass_packet = NULL;
+  xfree (rs->last_pass_packet);
+  rs->last_pass_packet = NULL;
 
   /* Make sure we send the program signals list the next time we
      resume.  */
-- 
1.8.1.4

[PATCH 03/16] Add new_remote_state

[Tom Tromey]

Add new_remote_state and change remote_state to be a pointer.  This is
a preparatory patch for a later series.  It could perhaps be omitted,
but new_remote_state also does some initialization that was previously
done for the globals.

	* remote.c (remote_state): Now a pointer.
	(get_remote_state_raw): Update.
	(new_remote_state): New function.
	(_initialize_remote): Use new_remote_state.
---
 gdb/remote.c | 22 +++++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)

diff --git a/gdb/remote.c b/gdb/remote.c
index 430ff2b..46ab2f8 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -395,12 +395,26 @@ remote_multi_process_p (struct remote_state *rs)
    have access to the current target when we need it, so for now it is
    static.  This will be fine for as long as only one target is in use
    at a time.  */
-static struct remote_state remote_state;
+static struct remote_state *remote_state;
 
 static struct remote_state *
 get_remote_state_raw (void)
 {
-  return &remote_state;
+  return remote_state;
+}
+
+/* Allocate a new struct remote_state with xmalloc, initialize it, and
+   return it.  */
+
+static struct remote_state *
+new_remote_state (void)
+{
+  struct remote_state *result = XCNEW (struct remote_state);
+
+  result->buf_size = 400;
+  result->buf = xmalloc (result->buf_size);
+
+  return result;
 }
 
 /* Description of the remote protocol for a given architecture.  */
@@ -11800,9 +11814,7 @@ _initialize_remote (void)
      of these, not one per target.  Only one target is active at a
      time.  The default buffer size is unimportant; it will be expanded
      whenever a larger buffer is needed.  */
-  rs = get_remote_state_raw ();
-  rs->buf_size = 400;
-  rs->buf = xmalloc (rs->buf_size);
+  remote_state = new_remote_state ();
 
   init_remote_ops ();
   add_target (&remote_ops);
-- 
1.8.1.4

Re: [PATCH 03/16] Add new_remote_state

[Pedro Alves]

On 06/28/2013 06:40 PM, Tom Tromey wrote:
> +/* Allocate a new struct remote_state with xmalloc, initialize it, and
> +   return it.  */
> +
> +static struct remote_state *
> +new_remote_state (void)
> +{
> +  struct remote_state *result = XCNEW (struct remote_state);
> +
> +  result->buf_size = 400;
> +  result->buf = xmalloc (result->buf_size);

I was re-reading this, and wondering "hmm, wait, where does that 400
come from?"

...

>  /* Description of the remote protocol for a given architecture.  */
> @@ -11800,9 +11814,7 @@ _initialize_remote (void)
>       of these, not one per target.  Only one target is active at a
>       time.  The default buffer size is unimportant; it will be expanded
>       whenever a larger buffer is needed.  */

... then I notice this "The default buffer ..." comment.  I think it'd be
good move it too.

Sorry for missing this one the first time around.

> -  rs = get_remote_state_raw ();
> -  rs->buf_size = 400;
> -  rs->buf = xmalloc (rs->buf_size);
> +  remote_state = new_remote_state ();
-- 
Pedro Alves

Re: [PATCH 03/16] Add new_remote_state

[Tom Tromey]

Pedro> ... then I notice this "The default buffer ..." comment.  I think it'd be
Pedro> good move it too.

Pedro> Sorry for missing this one the first time around.

It's no problem.  I've moved it.

Tom

[PATCH 08/16] push last_program_signals_packet into struct remote_state

[Tom Tromey]

This moves the global last_program_signals_packet into remote_state.

	* remote.c (struct remote_state) <last_program_signals_packet>:
	New field.
	(last_program_signals_packet): Remove.
	(remote_program_signals, remote_open_1): Update.
---
 gdb/remote.c | 27 +++++++++++++--------------
 1 file changed, 13 insertions(+), 14 deletions(-)

diff --git a/gdb/remote.c b/gdb/remote.c
index 136c3d9..a2d7e13 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -382,6 +382,12 @@ struct remote_state
   int remote_traceframe_number;
 
   char *last_pass_packet;
+
+  /* The last QProgramSignals packet sent to the target.  We bypass
+     sending a new program signals list down to the target if the new
+     packet is exactly the same as the last we sent.  IOW, we only let
+     the target know about program signals list changes.  */
+  char *last_program_signals_packet;
 };
 
 /* Private data that we'll store in (struct thread_info)->private.  */
@@ -1706,13 +1712,6 @@ remote_pass_signals (int numsigs, unsigned char *pass_signals)
     }
 }
 
-/* The last QProgramSignals packet sent to the target.  We bypass
-   sending a new program signals list down to the target if the new
-   packet is exactly the same as the last we sent.  IOW, we only let
-   the target know about program signals list changes.  */
-
-static char *last_program_signals_packet;
-
 /* If 'QProgramSignals' is supported, tell the remote stub what
    signals it should pass through to the inferior when detaching.  */
 
@@ -1723,6 +1722,7 @@ remote_program_signals (int numsigs, unsigned char *signals)
     {
       char *packet, *p;
       int count = 0, i;
+      struct remote_state *rs = get_remote_state ();
 
       gdb_assert (numsigs < 256);
       for (i = 0; i < numsigs; i++)
@@ -1748,17 +1748,16 @@ remote_program_signals (int numsigs, unsigned char *signals)
 	    }
 	}
       *p = 0;
-      if (!last_program_signals_packet
-	  || strcmp (last_program_signals_packet, packet) != 0)
+      if (!rs->last_program_signals_packet
+	  || strcmp (rs->last_program_signals_packet, packet) != 0)
 	{
-	  struct remote_state *rs = get_remote_state ();
 	  char *buf = rs->buf;
 
 	  putpkt (packet);
 	  getpkt (&rs->buf, &rs->buf_size, 0);
 	  packet_ok (buf, &remote_protocol_packets[PACKET_QProgramSignals]);
-	  xfree (last_program_signals_packet);
-	  last_program_signals_packet = packet;
+	  xfree (rs->last_program_signals_packet);
+	  rs->last_program_signals_packet = packet;
 	}
       else
 	xfree (packet);
@@ -4288,8 +4287,8 @@ remote_open_1 (char *name, int from_tty,
 
   /* Make sure we send the program signals list the next time we
      resume.  */
-  xfree (last_program_signals_packet);
-  last_program_signals_packet = NULL;
+  xfree (rs->last_program_signals_packet);
+  rs->last_program_signals_packet = NULL;
 
   remote_fileio_reset ();
   reopen_exec_file ();
-- 
1.8.1.4

[PATCH 15/16] move remote_stopped_by_watchpoint_p and remote_watch_data_address into remote_state

[Tom Tromey]

This moves the globals remote_stopped_by_watchpoint_p and
remote_watch_data_address into remote_state.

	* remote.c (struct remote_state) <remote_stopped_by_watchpoint_p,
	remote_watch_data_address>: New fields.
	(remote_stopped_by_watchpoint_p, remote_watch_data_address): Remove.
	(process_stop_reply, remote_wait_as)
	(remote_check_watch_resources, remote_stopped_data_address): Update.
---
 gdb/remote.c | 33 +++++++++++++++++----------------
 1 file changed, 17 insertions(+), 16 deletions(-)

diff --git a/gdb/remote.c b/gdb/remote.c
index 8641094..56c6f6c 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -410,6 +410,13 @@ struct remote_state
   void (*async_client_callback) (enum inferior_event_type event_type,
 				 void *context);
   void *async_client_context;
+
+  /* This is set to the data address of the access causing the target
+     to stop for a watchpoint.  */
+  CORE_ADDR remote_watch_data_address;
+
+  /* This is non-zero if target stopped for a watchpoint.  */
+  int remote_stopped_by_watchpoint_p;
 };
 
 /* Private data that we'll store in (struct thread_info)->private.  */
@@ -792,17 +799,6 @@ packet_reg_from_pnum (struct remote_arch_state *rsa, LONGEST pnum)
   return NULL;
 }
 
-/* FIXME: graces/2002-08-08: These variables should eventually be
-   bound to an instance of the target object (as in gdbarch-tdep()),
-   when such a thing exists.  */
-
-/* This is set to the data address of the access causing the target
-   to stop for a watchpoint.  */
-static CORE_ADDR remote_watch_data_address;
-
-/* This is non-zero if target stopped for a watchpoint.  */
-static int remote_stopped_by_watchpoint_p;
-
 static struct target_ops remote_ops;
 
 static struct target_ops extended_remote_ops;
@@ -5846,6 +5842,8 @@ process_stop_reply (struct stop_reply *stop_reply,
   if (status->kind != TARGET_WAITKIND_EXITED
       && status->kind != TARGET_WAITKIND_SIGNALLED)
     {
+      struct remote_state *rs = get_remote_state ();
+
       /* Expedited registers.  */
       if (stop_reply->regcache)
 	{
@@ -5861,8 +5859,8 @@ process_stop_reply (struct stop_reply *stop_reply,
 	  VEC_free (cached_reg_t, stop_reply->regcache);
 	}
 
-      remote_stopped_by_watchpoint_p = stop_reply->stopped_by_watchpoint_p;
-      remote_watch_data_address = stop_reply->watch_data_address;
+      rs->remote_stopped_by_watchpoint_p = stop_reply->stopped_by_watchpoint_p;
+      rs->remote_watch_data_address = stop_reply->watch_data_address;
 
       remote_notice_new_inferior (ptid, 0);
       demand_private_info (ptid)->core = stop_reply->core;
@@ -5988,7 +5986,7 @@ remote_wait_as (ptid_t ptid, struct target_waitstatus *status, int options)
 
   buf = rs->buf;
 
-  remote_stopped_by_watchpoint_p = 0;
+  rs->remote_stopped_by_watchpoint_p = 0;
 
   /* We got something.  */
   rs->waiting_for_stop_reply = 0;
@@ -8429,17 +8427,20 @@ remote_check_watch_resources (int type, int cnt, int ot)
 static int
 remote_stopped_by_watchpoint (void)
 {
-  return remote_stopped_by_watchpoint_p;
+  struct remote_state *rs = get_remote_state ();
+
+  return rs->remote_stopped_by_watchpoint_p;
 }
 
 static int
 remote_stopped_data_address (struct target_ops *target, CORE_ADDR *addr_p)
 {
+  struct remote_state *rs = get_remote_state ();
   int rc = 0;
 
   if (remote_stopped_by_watchpoint ())
     {
-      *addr_p = remote_watch_data_address;
+      *addr_p = rs->remote_watch_data_address;
       rc = 1;
     }
 
-- 
1.8.1.4

[PATCH 11/16] move some statics from remote_read_qxfer into struct remote_state

[Tom Tromey]

This moves a few static variables out of remote_read_qxfer and into
remote_state.  It is unclear to me if this data can ever be required
to be kept around across a potential target switch, but it is
definitely safe to move it into the remote state object.

	* remote.c (struct remote_state) <finished_object,
	finished_annex, finished_offset>: New fields.
	(remote_read_qxfer): Use remote_state fields; remove static
	variables.
---
 gdb/remote.c | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/gdb/remote.c b/gdb/remote.c
index e87f7e8..6c71ac1 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -392,6 +392,10 @@ struct remote_state
   enum gdb_signal last_sent_signal;
 
   int last_sent_step;
+
+  char *finished_object;
+  char *finished_annex;
+  ULONGEST finished_offset;
 };
 
 /* Private data that we'll store in (struct thread_info)->private.  */
@@ -8704,10 +8708,6 @@ remote_read_qxfer (struct target_ops *ops, const char *object_name,
 		   gdb_byte *readbuf, ULONGEST offset, LONGEST len,
 		   struct packet_config *packet)
 {
-  static char *finished_object;
-  static char *finished_annex;
-  static ULONGEST finished_offset;
-
   struct remote_state *rs = get_remote_state ();
   LONGEST i, n, packet_len;
 
@@ -8716,19 +8716,19 @@ remote_read_qxfer (struct target_ops *ops, const char *object_name,
 
   /* Check whether we've cached an end-of-object packet that matches
      this request.  */
-  if (finished_object)
+  if (rs->finished_object)
     {
-      if (strcmp (object_name, finished_object) == 0
-	  && strcmp (annex ? annex : "", finished_annex) == 0
-	  && offset == finished_offset)
+      if (strcmp (object_name, rs->finished_object) == 0
+	  && strcmp (annex ? annex : "", rs->finished_annex) == 0
+	  && offset == rs->finished_offset)
 	return 0;
 
       /* Otherwise, we're now reading something different.  Discard
 	 the cache.  */
-      xfree (finished_object);
-      xfree (finished_annex);
-      finished_object = NULL;
-      finished_annex = NULL;
+      xfree (rs->finished_object);
+      xfree (rs->finished_annex);
+      rs->finished_object = NULL;
+      rs->finished_annex = NULL;
     }
 
   /* Request only enough to fit in a single packet.  The actual data
@@ -8767,9 +8767,9 @@ remote_read_qxfer (struct target_ops *ops, const char *object_name,
      object, record this fact to bypass a subsequent partial read.  */
   if (rs->buf[0] == 'l' && offset + i > 0)
     {
-      finished_object = xstrdup (object_name);
-      finished_annex = xstrdup (annex ? annex : "");
-      finished_offset = offset + i;
+      rs->finished_object = xstrdup (object_name);
+      rs->finished_annex = xstrdup (annex ? annex : "");
+      rs->finished_offset = offset + i;
     }
 
   return i;
-- 
1.8.1.4

Re: [PATCH 11/16] move some statics from remote_read_qxfer into struct remote_state

[Pedro Alves]

On 06/28/2013 06:40 PM, Tom Tromey wrote:
> This moves a few static variables out of remote_read_qxfer and into
> remote_state.  It is unclear to me if this data can ever be required
> to be kept around across a potential target switch, but it is
> definitely safe to move it into the remote state object.

Hmm, are we still unclear about it?  It seems to me that if we
dropped the data, we'd always be able to re-fetch it, though obviously
we'd lose on the optimization.  It definitely seems to me that
putting it in the remote state object is the correct choice.
Maybe you're seeing something I'm not though.

-- 
Pedro Alves

Re: [PATCH 11/16] move some statics from remote_read_qxfer into struct remote_state

[Tom Tromey]

>> This moves a few static variables out of remote_read_qxfer and into
>> remote_state.  It is unclear to me if this data can ever be required
>> to be kept around across a potential target switch, but it is
>> definitely safe to move it into the remote state object.

Pedro> Hmm, are we still unclear about it?  It seems to me that if we
Pedro> dropped the data, we'd always be able to re-fetch it, though obviously
Pedro> we'd lose on the optimization.  It definitely seems to me that
Pedro> putting it in the remote state object is the correct choice.
Pedro> Maybe you're seeing something I'm not though.

Nope, I just neglected to update the note.
I've dropped everything after the first sentence.

Tom

[PATCH 06/16] push remote_traceframe_number into struct remote_state

[Tom Tromey]

This moves the global remote_traceframe_number into remote_state.

	* remote.c (struct remote_state) <remote_traceframe_number>:
	New field.
	(remote_traceframe_number): Remove.
	(new_remote_state, remote_open_1, set_remote_traceframe)
	(remote_trace_find): Update.
---
 gdb/remote.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/gdb/remote.c b/gdb/remote.c
index 6d9478f..3ef99df 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -376,6 +376,10 @@ struct remote_state
      TID member will be -1 for all or -2 for not sent yet.  */
   ptid_t general_thread;
   ptid_t continue_thread;
+
+  /* This is the traceframe which we last selected on the remote system.
+     It will be -1 if no traceframe is selected.  */
+  int remote_traceframe_number;
 };
 
 /* Private data that we'll store in (struct thread_info)->private.  */
@@ -421,6 +425,7 @@ new_remote_state (void)
 
   result->buf_size = 400;
   result->buf = xmalloc (result->buf_size);
+  result->remote_traceframe_number = -1;
 
   return result;
 }
@@ -1442,10 +1447,6 @@ static ptid_t magic_null_ptid;
 static ptid_t not_sent_ptid;
 static ptid_t any_thread_ptid;
 
-/* This is the traceframe which we last selected on the remote system.
-   It will be -1 if no traceframe is selected.  */
-static int remote_traceframe_number = -1;
-
 /* Find out if the stub attached to PID (and hence GDB should offer to
    detach instead of killing it when bailing out).  */
 
@@ -4346,7 +4347,7 @@ remote_open_1 (char *name, int from_tty,
 
   rs->general_thread = not_sent_ptid;
   rs->continue_thread = not_sent_ptid;
-  remote_traceframe_number = -1;
+  rs->remote_traceframe_number = -1;
 
   /* Probe for ability to use "ThreadInfo" query, as required.  */
   use_threadinfo_query = 1;
@@ -6296,12 +6297,13 @@ static void
 set_remote_traceframe (void)
 {
   int newnum;
+  struct remote_state *rs = get_remote_state ();
 
-  if (remote_traceframe_number == get_traceframe_number ())
+  if (rs->remote_traceframe_number == get_traceframe_number ())
     return;
 
   /* Avoid recursion, remote_trace_find calls us again.  */
-  remote_traceframe_number = get_traceframe_number ();
+  rs->remote_traceframe_number = get_traceframe_number ();
 
   newnum = target_trace_find (tfind_number,
 			      get_traceframe_number (), 0, 0, NULL);
@@ -11008,7 +11010,7 @@ remote_trace_find (enum trace_find_type type, int num,
   if (tpp)
     *tpp = target_tracept;
 
-  remote_traceframe_number = target_frameno;
+  rs->remote_traceframe_number = target_frameno;
   return target_frameno;
 }
 
-- 
1.8.1.4

[PATCH 16/16] move some static thread state into remote_state

[Tom Tromey]

This moves a few static variables from thread-info functions into
remote_state.  Pedro said on irc that these functions implement the
ancient thread-discovery method and that he wouldn't be surprised if
they had rotted; nevertheless it seems safer to me to make them
explicitly per-remote.

This necessitated moving a couple of macros and a typedef earlier in
the file.

	* remote.c (struct remote_state) <echo_nextthread, nextthread,
	resultthreadlist>: New fields.
	(OPAQUETHREADBYTES, threadref, MAXTHREADLISTRESULTS): Move earlier.
	(remote_get_threadlist, remote_threadlist_iterator): Use
	new fields.  Remove static variables.
---
 gdb/remote.c | 42 +++++++++++++++++++++++-------------------
 1 file changed, 23 insertions(+), 19 deletions(-)

diff --git a/gdb/remote.c b/gdb/remote.c
index 56c6f6c..86360c7 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -267,6 +267,15 @@ struct vCont_action_support
 
 static int use_range_stepping = 1;
 
+#define OPAQUETHREADBYTES 8
+
+/* a 64 bit opaque identifier */
+typedef unsigned char threadref[OPAQUETHREADBYTES];
+
+/* About this many threadisds fit in a packet.  */
+
+#define MAXTHREADLISTRESULTS 32
+
 /* Description of the remote protocol state for the currently
    connected target.  This is per-target state, and independent of the
    selected architecture.  */
@@ -417,6 +426,10 @@ struct remote_state
 
   /* This is non-zero if target stopped for a watchpoint.  */
   int remote_stopped_by_watchpoint_p;
+
+  threadref echo_nextthread;
+  threadref nextthread;
+  threadref resultthreadlist[MAXTHREADLISTRESULTS];
 };
 
 /* Private data that we'll store in (struct thread_info)->private.  */
@@ -1876,11 +1889,6 @@ remote_thread_alive (struct target_ops *ops, ptid_t ptid)
    remote protocol in general.  There is a matching unit test module
    in libstub.  */
 
-#define OPAQUETHREADBYTES 8
-
-/* a 64 bit opaque identifier */
-typedef unsigned char threadref[OPAQUETHREADBYTES];
-
 /* WARNING: This threadref data structure comes from the remote O.S.,
    libstub protocol encoding, and remote.c.  It is not particularly
    changable.  */
@@ -2496,7 +2504,6 @@ remote_get_threadlist (int startflag, threadref *nextthread, int result_limit,
 		       int *done, int *result_count, threadref *threadlist)
 {
   struct remote_state *rs = get_remote_state ();
-  static threadref echo_nextthread;
   int result = 1;
 
   /* Trancate result limit to be smaller than the packet size.  */
@@ -2512,10 +2519,10 @@ remote_get_threadlist (int startflag, threadref *nextthread, int result_limit,
     return 0;
   else
     *result_count =
-      parse_threadlist_response (rs->buf + 2, result_limit, &echo_nextthread,
-                                 threadlist, done);
+      parse_threadlist_response (rs->buf + 2, result_limit,
+				 &rs->echo_nextthread, threadlist, done);
 
-  if (!threadmatch (&echo_nextthread, nextthread))
+  if (!threadmatch (&rs->echo_nextthread, nextthread))
     {
       /* FIXME: This is a good reason to drop the packet.  */
       /* Possably, there is a duplicate response.  */
@@ -2556,20 +2563,15 @@ remote_get_threadlist (int startflag, threadref *nextthread, int result_limit,
    quit_flag is required.  */
 
 
-/* About this many threadisds fit in a packet.  */
-
-#define MAXTHREADLISTRESULTS 32
-
 static int
 remote_threadlist_iterator (rmt_thread_action stepfunction, void *context,
 			    int looplimit)
 {
+  struct remote_state *rs = get_remote_state ();
   int done, i, result_count;
   int startflag = 1;
   int result = 1;
   int loopcount = 0;
-  static threadref nextthread;
-  static threadref resultthreadlist[MAXTHREADLISTRESULTS];
 
   done = 0;
   while (!done)
@@ -2580,8 +2582,9 @@ remote_threadlist_iterator (rmt_thread_action stepfunction, void *context,
 	  warning (_("Remote fetch threadlist -infinite loop-."));
 	  break;
 	}
-      if (!remote_get_threadlist (startflag, &nextthread, MAXTHREADLISTRESULTS,
-				  &done, &result_count, resultthreadlist))
+      if (!remote_get_threadlist (startflag, &rs->nextthread,
+				  MAXTHREADLISTRESULTS,
+				  &done, &result_count, rs->resultthreadlist))
 	{
 	  result = 0;
 	  break;
@@ -2590,10 +2593,11 @@ remote_threadlist_iterator (rmt_thread_action stepfunction, void *context,
       startflag = 0;
       /* Setup to resume next batch of thread references, set nextthread.  */
       if (result_count >= 1)
-	copy_threadref (&nextthread, &resultthreadlist[result_count - 1]);
+	copy_threadref (&rs->nextthread,
+			&rs->resultthreadlist[result_count - 1]);
       i = 0;
       while (result_count--)
-	if (!(result = (*stepfunction) (&resultthreadlist[i++], context)))
+	if (!(result = (*stepfunction) (&rs->resultthreadlist[i++], context)))
 	  break;
     }
   return result;
-- 
1.8.1.4

[PATCH 12/16] move use_threadinfo_query and use_threadextra_query into struct remote_state

[Tom Tromey]

This moves the use_threadextra_query and use_threadinfo_query globals
into remote_state.

	* remote.c (struct remote_state) <use_threadinfo_query,
	use_threadextra_query>: New fields.
	(remote_threads_info, remote_threads_extra_info)
	(remote_open_1): Update.
---
 gdb/remote.c | 33 ++++++++++++++++-----------------
 1 file changed, 16 insertions(+), 17 deletions(-)

diff --git a/gdb/remote.c b/gdb/remote.c
index 6c71ac1..d7650b1 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -396,6 +396,16 @@ struct remote_state
   char *finished_object;
   char *finished_annex;
   ULONGEST finished_offset;
+
+  /* Should we try the 'ThreadInfo' query packet?
+
+     This variable (NOT available to the user: auto-detect only!)
+     determines whether GDB will use the new, simpler "ThreadInfo"
+     query or the older, more complex syntax for thread queries.
+     This is an auto-detect variable (set to true at each connect,
+     and set to false when the target fails to recognize it).  */
+  int use_threadinfo_query;
+  int use_threadextra_query;
 };
 
 /* Private data that we'll store in (struct thread_info)->private.  */
@@ -1437,17 +1447,6 @@ show_remote_protocol_Z_packet_cmd (struct ui_file *file, int from_tty,
     }
 }
 
-/* Should we try the 'ThreadInfo' query packet?
-
-   This variable (NOT available to the user: auto-detect only!)
-   determines whether GDB will use the new, simpler "ThreadInfo"
-   query or the older, more complex syntax for thread queries.
-   This is an auto-detect variable (set to true at each connect,
-   and set to false when the target fails to recognize it).  */
-
-static int use_threadinfo_query;
-static int use_threadextra_query;
-
 /* Tokens for use by the asynchronous signal handlers for SIGINT.  */
 static struct async_signal_handler *sigint_remote_twice_token;
 static struct async_signal_handler *sigint_remote_token;
@@ -2797,7 +2796,7 @@ remote_threads_info (struct target_ops *ops)
     }
 #endif
 
-  if (use_threadinfo_query)
+  if (rs->use_threadinfo_query)
     {
       putpkt ("qfThreadInfo");
       getpkt (&rs->buf, &rs->buf_size, 0);
@@ -2845,7 +2844,7 @@ remote_threads_info (struct target_ops *ops)
     return;
 
   /* Else fall back to old method based on jmetzler protocol.  */
-  use_threadinfo_query = 0;
+  rs->use_threadinfo_query = 0;
   remote_find_new_threads ();
   return;
 }
@@ -2890,7 +2889,7 @@ remote_threads_extra_info (struct thread_info *tp)
 	return NULL;
     }
 
-  if (use_threadextra_query)
+  if (rs->use_threadextra_query)
     {
       char *b = rs->buf;
       char *endb = rs->buf + get_remote_packet_size ();
@@ -2911,7 +2910,7 @@ remote_threads_extra_info (struct thread_info *tp)
     }
 
   /* If the above query fails, fall back to the old method.  */
-  use_threadextra_query = 0;
+  rs->use_threadextra_query = 0;
   set = TAG_THREADID | TAG_EXISTS | TAG_THREADNAME
     | TAG_MOREDISPLAY | TAG_DISPLAY;
   int_to_threadref (&id, ptid_get_tid (tp->ptid));
@@ -4358,8 +4357,8 @@ remote_open_1 (char *name, int from_tty,
   rs->remote_traceframe_number = -1;
 
   /* Probe for ability to use "ThreadInfo" query, as required.  */
-  use_threadinfo_query = 1;
-  use_threadextra_query = 1;
+  rs->use_threadinfo_query = 1;
+  rs->use_threadextra_query = 1;
 
   if (target_async_permitted)
     {
-- 
1.8.1.4

[PATCH 01/16] use the libiberty crc code

[Tom Tromey]

gdb has a copy of some CRC code that also appears in libiberty.
This patch just removes the local copy.

You may notice that "crc32" returns unsigned long but "xcrc32" returns
unsigned int.  However, this does not matter, because crc32 actually
does all its operations in unsigned int type, and only the return
result is widened.  So, the difference does not matter.

	* remote.c (crc32_table, crc32): Remove.
	(remote_verify_memory): Use xcrc32.
---
 gdb/remote.c | 32 +-------------------------------
 1 file changed, 1 insertion(+), 31 deletions(-)

diff --git a/gdb/remote.c b/gdb/remote.c
index a29fe23..e40dc4e 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -8526,36 +8526,6 @@ remote_remove_hw_breakpoint (struct gdbarch *gdbarch,
 		  _("remote_remove_hw_breakpoint: reached end of function"));
 }
 
-/* Table used by the crc32 function to calcuate the checksum.  */
-
-static unsigned long crc32_table[256] =
-{0, 0};
-
-static unsigned long
-crc32 (const unsigned char *buf, int len, unsigned int crc)
-{
-  if (!crc32_table[1])
-    {
-      /* Initialize the CRC table and the decoding table.  */
-      int i, j;
-      unsigned int c;
-
-      for (i = 0; i < 256; i++)
-	{
-	  for (c = i << 24, j = 8; j > 0; --j)
-	    c = c & 0x80000000 ? (c << 1) ^ 0x04c11db7 : (c << 1);
-	  crc32_table[i] = c;
-	}
-    }
-
-  while (len--)
-    {
-      crc = (crc << 8) ^ crc32_table[((crc >> 24) ^ *buf) & 255];
-      buf++;
-    }
-  return crc;
-}
-
 /* Verify memory using the "qCRC:" request.  */
 
 static int
@@ -8576,7 +8546,7 @@ remote_verify_memory (struct target_ops *ops,
 
   /* Be clever; compute the host_crc before waiting for target
      reply.  */
-  host_crc = crc32 (data, size, 0xffffffff);
+  host_crc = xcrc32 (data, size, 0xffffffff);
 
   getpkt (&rs->buf, &rs->buf_size, 0);
   if (rs->buf[0] == 'E')
-- 
1.8.1.4

[PATCH 04/16] push remote_desc into struct remote_state

[Tom Tromey]

This moves the "remote_desc" global into remote_state.

	* remote.c (struct remote_state) <remote_desc>: New field.
	(remote_desc): Remove.
	(remote_threads_info, remote_threads_extra_info, remote_close)
	(send_interrupt_sequence, remote_start_remote, remote_open_1)
	(readchar, remote_xfer_partial, remote_rcmd, packet_command)
	(remote_hostio_send_command, remote_file_put, remote_file_get)
	(remote_file_delete, remote_can_async_p, remote_is_async_p)
	(remote_async, remote_new_objfile, set_range_stepping): Update.
---
 gdb/remote.c | 98 +++++++++++++++++++++++++++++++++++-------------------------
 1 file changed, 58 insertions(+), 40 deletions(-)

diff --git a/gdb/remote.c b/gdb/remote.c
index 46ab2f8..9dfc73b 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -368,6 +368,11 @@ struct remote_state
   /* Nonzero if the user has pressed Ctrl-C, but the target hasn't
      responded to that.  */
   int ctrlc_pending_p;
+
+  /* Descriptor for I/O to remote machine.  Initialize it to NULL so that
+     remote_open knows that we don't have a file open when the program
+     starts.  */
+  struct serial *remote_desc;
 };
 
 /* Private data that we'll store in (struct thread_info)->private.  */
@@ -844,11 +849,6 @@ show_remotebreak (struct ui_file *file, int from_tty,
 {
 }
 
-/* Descriptor for I/O to remote machine.  Initialize it to NULL so that
-   remote_open knows that we don't have a file open when the program
-   starts.  */
-static struct serial *remote_desc = NULL;
-
 /* This variable sets the number of bits in an address that are to be
    sent in a memory ("M" or "m") packet.  Normally, after stripping
    leading zeros, the entire address would be sent.  This variable
@@ -2738,7 +2738,7 @@ remote_threads_info (struct target_ops *ops)
   char *bufp;
   ptid_t new_thread;
 
-  if (remote_desc == 0)		/* paranoia */
+  if (rs->remote_desc == 0)		/* paranoia */
     error (_("Command can only be used when connected to the remote target."));
 
 #if defined(HAVE_LIBEXPAT)
@@ -2864,7 +2864,7 @@ remote_threads_extra_info (struct thread_info *tp)
   static char display_buf[100];	/* arbitrary...  */
   int n = 0;                    /* position in display_buf */
 
-  if (remote_desc == 0)		/* paranoia */
+  if (rs->remote_desc == 0)		/* paranoia */
     internal_error (__FILE__, __LINE__,
 		    _("remote_threads_extra_info"));
 
@@ -3041,15 +3041,17 @@ extended_remote_restart (void)
 static void
 remote_close (void)
 {
-  if (remote_desc == NULL)
+  struct remote_state *rs = get_remote_state ();
+
+  if (rs->remote_desc == NULL)
     return; /* already closed */
 
   /* Make sure we leave stdin registered in the event loop, and we
      don't leave the async SIGINT signal handler installed.  */
   remote_terminal_ours ();
 
-  serial_close (remote_desc);
-  remote_desc = NULL;
+  serial_close (rs->remote_desc);
+  rs->remote_desc = NULL;
 
   /* We don't have a connection to the remote stub anymore.  Get rid
      of all the inferiors and their threads we were controlling.
@@ -3250,13 +3252,15 @@ set_stop_requested_callback (struct thread_info *thread, void *data)
 static void
 send_interrupt_sequence (void)
 {
+  struct remote_state *rs = get_remote_state ();
+
   if (interrupt_sequence_mode == interrupt_sequence_control_c)
     remote_serial_write ("\x03", 1);
   else if (interrupt_sequence_mode == interrupt_sequence_break)
-    serial_send_break (remote_desc);
+    serial_send_break (rs->remote_desc);
   else if (interrupt_sequence_mode == interrupt_sequence_break_g)
     {
-      serial_send_break (remote_desc);
+      serial_send_break (rs->remote_desc);
       remote_serial_write ("g", 1);
     }
   else
@@ -3373,7 +3377,7 @@ remote_start_remote (int from_tty, struct target_ops *target, int extended_p)
     send_interrupt_sequence ();
 
   /* Ack any packet which the remote side has already sent.  */
-  serial_write (remote_desc, "+", 1);
+  serial_write (rs->remote_desc, "+", 1);
 
   /* Signal other parts that we're going through the initial setup,
      and so things may not be stable yet.  */
@@ -4270,7 +4274,7 @@ remote_open_1 (char *name, int from_tty,
   /* If we're connected to a running target, target_preopen will kill it.
      Ask this question first, before target_preopen has a chance to kill
      anything.  */
-  if (remote_desc != NULL && !have_inferiors ())
+  if (rs->remote_desc != NULL && !have_inferiors ())
     {
       if (from_tty
 	  && !query (_("Already connected to a remote target.  Disconnect? ")))
@@ -4293,29 +4297,29 @@ remote_open_1 (char *name, int from_tty,
   reopen_exec_file ();
   reread_symbols ();
 
-  remote_desc = remote_serial_open (name);
-  if (!remote_desc)
+  rs->remote_desc = remote_serial_open (name);
+  if (!rs->remote_desc)
     perror_with_name (name);
 
   if (baud_rate != -1)
     {
-      if (serial_setbaudrate (remote_desc, baud_rate))
+      if (serial_setbaudrate (rs->remote_desc, baud_rate))
 	{
 	  /* The requested speed could not be set.  Error out to
 	     top level after closing remote_desc.  Take care to
 	     set remote_desc to NULL to avoid closing remote_desc
 	     more than once.  */
-	  serial_close (remote_desc);
-	  remote_desc = NULL;
+	  serial_close (rs->remote_desc);
+	  rs->remote_desc = NULL;
 	  perror_with_name (name);
 	}
     }
 
-  serial_raw (remote_desc);
+  serial_raw (rs->remote_desc);
 
   /* If there is something sitting in the buffer we might take it as a
      response to a command, which would be bad.  */
-  serial_flush_input (remote_desc);
+  serial_flush_input (rs->remote_desc);
 
   if (from_tty)
     {
@@ -4398,7 +4402,7 @@ remote_open_1 (char *name, int from_tty,
       {
 	/* Pop the partially set up target - unless something else did
 	   already before throwing the exception.  */
-	if (remote_desc != NULL)
+	if (rs->remote_desc != NULL)
 	  remote_unpush_target ();
 	if (target_async_permitted)
 	  wait_forever_enabled_p = 1;
@@ -7148,8 +7152,9 @@ static int
 readchar (int timeout)
 {
   int ch;
+  struct remote_state *rs = get_remote_state ();
 
-  ch = serial_readchar (remote_desc, timeout);
+  ch = serial_readchar (rs->remote_desc, timeout);
 
   if (ch >= 0)
     return ch;
@@ -7176,7 +7181,9 @@ readchar (int timeout)
 static void
 remote_serial_write (const char *str, int len)
 {
-  if (serial_write (remote_desc, str, len))
+  struct remote_state *rs = get_remote_state ();
+
+  if (serial_write (rs->remote_desc, str, len))
     {
       unpush_and_perror (_("Remote communication error.  "
 			   "Target disconnected."));
@@ -8905,7 +8912,7 @@ remote_xfer_partial (struct target_ops *ops, enum target_object object,
 
     case TARGET_OBJECT_OSDATA:
       /* Should only get here if we're connected.  */
-      gdb_assert (remote_desc);
+      gdb_assert (rs->remote_desc);
       return remote_read_qxfer
        (ops, "osdata", annex, readbuf, offset, len,
         &remote_protocol_packets[PACKET_qXfer_osdata]);
@@ -8948,7 +8955,7 @@ remote_xfer_partial (struct target_ops *ops, enum target_object object,
   len = get_remote_packet_size ();
 
   /* Except for querying the minimum buffer size, target must be open.  */
-  if (!remote_desc)
+  if (!rs->remote_desc)
     error (_("remote query is only available after target open"));
 
   gdb_assert (annex != NULL);
@@ -9082,7 +9089,7 @@ remote_rcmd (char *command,
   struct remote_state *rs = get_remote_state ();
   char *p = rs->buf;
 
-  if (!remote_desc)
+  if (!rs->remote_desc)
     error (_("remote rcmd is only available after target open"));
 
   /* Send a NULL command across as an empty command.  */
@@ -9168,7 +9175,7 @@ packet_command (char *args, int from_tty)
 {
   struct remote_state *rs = get_remote_state ();
 
-  if (!remote_desc)
+  if (!rs->remote_desc)
     error (_("command can only be used with remote target"));
 
   if (!args)
@@ -9708,7 +9715,7 @@ remote_hostio_send_command (int command_bytes, int which_packet,
   int ret, bytes_read;
   char *attachment_tmp;
 
-  if (!remote_desc
+  if (!rs->remote_desc
       || remote_protocol_packets[which_packet].support == PACKET_DISABLE)
     {
       *remote_errno = FILEIO_ENOSYS;
@@ -10109,8 +10116,9 @@ remote_file_put (const char *local_file, const char *remote_file, int from_tty)
   int bytes_in_buffer;
   int saw_eof;
   ULONGEST offset;
+  struct remote_state *rs = get_remote_state ();
 
-  if (!remote_desc)
+  if (!rs->remote_desc)
     error (_("command can only be used with remote target"));
 
   file = gdb_fopen_cloexec (local_file, "rb");
@@ -10197,8 +10205,9 @@ remote_file_get (const char *remote_file, const char *local_file, int from_tty)
   FILE *file;
   gdb_byte *buffer;
   ULONGEST offset;
+  struct remote_state *rs = get_remote_state ();
 
-  if (!remote_desc)
+  if (!rs->remote_desc)
     error (_("command can only be used with remote target"));
 
   fd = remote_hostio_open (remote_file, FILEIO_O_RDONLY, 0, &remote_errno);
@@ -10248,8 +10257,9 @@ void
 remote_file_delete (const char *remote_file, int from_tty)
 {
   int retcode, remote_errno;
+  struct remote_state *rs = get_remote_state ();
 
-  if (!remote_desc)
+  if (!rs->remote_desc)
     error (_("command can only be used with remote target"));
 
   retcode = remote_hostio_unlink (remote_file, &remote_errno);
@@ -11605,23 +11615,27 @@ Specify the serial device it is connected to (e.g. /dev/ttya).";
 static int
 remote_can_async_p (void)
 {
+  struct remote_state *rs = get_remote_state ();
+
   if (!target_async_permitted)
     /* We only enable async when the user specifically asks for it.  */
     return 0;
 
   /* We're async whenever the serial device is.  */
-  return serial_can_async_p (remote_desc);
+  return serial_can_async_p (rs->remote_desc);
 }
 
 static int
 remote_is_async_p (void)
 {
+  struct remote_state *rs = get_remote_state ();
+
   if (!target_async_permitted)
     /* We only enable async when the user specifically asks for it.  */
     return 0;
 
   /* We're async whenever the serial device is.  */
-  return serial_is_async_p (remote_desc);
+  return serial_is_async_p (rs->remote_desc);
 }
 
 /* Pass the SERIAL event on and up to the client.  One day this code
@@ -11651,14 +11665,16 @@ static void
 remote_async (void (*callback) (enum inferior_event_type event_type,
 				void *context), void *context)
 {
+  struct remote_state *rs = get_remote_state ();
+
   if (callback != NULL)
     {
-      serial_async (remote_desc, remote_async_serial_handler, NULL);
+      serial_async (rs->remote_desc, remote_async_serial_handler, NULL);
       async_client_callback = callback;
       async_client_context = context;
     }
   else
-    serial_async (remote_desc, NULL, NULL);
+    serial_async (rs->remote_desc, NULL, NULL);
 }
 
 static void
@@ -11708,7 +11724,9 @@ show_remote_cmd (char *args, int from_tty)
 static void
 remote_new_objfile (struct objfile *objfile)
 {
-  if (remote_desc != 0)		/* Have a remote connection.  */
+  struct remote_state *rs = get_remote_state ();
+
+  if (rs->remote_desc != 0)		/* Have a remote connection.  */
     remote_check_symbols ();
 }
 
@@ -11777,14 +11795,14 @@ static void
 set_range_stepping (char *ignore_args, int from_tty,
 		    struct cmd_list_element *c)
 {
+  struct remote_state *rs = get_remote_state ();
+
   /* Whene enabling, check whether range stepping is actually
      supported by the target, and warn if not.  */
   if (use_range_stepping)
     {
-      if (remote_desc != NULL)
+      if (rs->remote_desc != NULL)
 	{
-	  struct remote_state *rs = get_remote_state ();
-
 	  if (remote_protocol_packets[PACKET_vCont].support == PACKET_SUPPORT_UNKNOWN)
 	    remote_vcont_probe (rs);
 
-- 
1.8.1.4

[PATCH 13/16] move sizeof_pkt into remote_trace_find

[Tom Tromey]

The global sizeof_pkt is only used in remote_trace_find, like so:

  reply = remote_get_noisy_reply (&(rs->buf), &sizeof_pkt);

I think in this situation it is more correct to use the recorded size
of the buffer.  Otherwise it seems that some skew could result.

	* remote.c (sizeof_pkt): Remove.
	(remote_trace_find): Use rs->buf_size, not sizeof_pkt.
---
 gdb/remote.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/gdb/remote.c b/gdb/remote.c
index d7650b1..381d596 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -493,8 +493,6 @@ struct remote_arch_state
   long remote_packet_size;
 };
 
-long sizeof_pkt = 2000;
-
 /* Utility: generate error from an incoming stub packet.  */
 static void
 trace_error (char *buf)
@@ -10974,7 +10972,7 @@ remote_trace_find (enum trace_find_type type, int num,
     }
 
   putpkt (rs->buf);
-  reply = remote_get_noisy_reply (&(rs->buf), &sizeof_pkt);
+  reply = remote_get_noisy_reply (&(rs->buf), &rs->buf_size);
   if (*reply == '\0')
     error (_("Target does not support this command."));
 
-- 
1.8.1.4

[PATCH 10/16] push last_sent_step into struct remote_state

[Tom Tromey]

This moves the global last_sent_step into remote_state.

	* remote.c (struct remote_state) <last_sent_step>:
	New field.
	(last_sent_step): Remove.
	(remote_resume, remote_wait_as): Update.
---
 gdb/remote.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/gdb/remote.c b/gdb/remote.c
index f095b5e..e87f7e8 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -390,6 +390,8 @@ struct remote_state
   char *last_program_signals_packet;
 
   enum gdb_signal last_sent_signal;
+
+  int last_sent_step;
 };
 
 /* Private data that we'll store in (struct thread_info)->private.  */
@@ -4913,8 +4915,6 @@ remote_vcont_resume (ptid_t ptid, int step, enum gdb_signal siggnal)
 
 /* Tell the remote machine to resume.  */
 
-static int last_sent_step;
-
 static void
 remote_resume (struct target_ops *ops,
 	       ptid_t ptid, int step, enum gdb_signal siggnal)
@@ -4932,7 +4932,7 @@ remote_resume (struct target_ops *ops,
     remote_notif_process (&notif_client_stop);
 
   rs->last_sent_signal = siggnal;
-  last_sent_step = step;
+  rs->last_sent_step = step;
 
   /* The vCont packet doesn't need to specify threads via Hc.  */
   /* No reverse support (yet) for vCont.  */
@@ -6034,7 +6034,7 @@ remote_wait_as (ptid_t ptid, struct target_waitstatus *status, int options)
 	  rs->last_sent_signal = GDB_SIGNAL_0;
 	  target_terminal_inferior ();
 
-	  strcpy ((char *) buf, last_sent_step ? "s" : "c");
+	  strcpy ((char *) buf, rs->last_sent_step ? "s" : "c");
 	  putpkt ((char *) buf);
 
 	  /* We just told the target to resume, so a stop reply is in
-- 
1.8.1.4

[PATCH 09/16] push last_sent_signal into struct remote_state

[Tom Tromey]

This moves the global last_sent_signal into remote_state.

	* remote.c (struct remote_state) <last_sent_signal>:
	New field.
	(last_sent_signal): Remove.
	(new_remote_state, remote_resume, remote_wait_as): Update.
---
 gdb/remote.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/gdb/remote.c b/gdb/remote.c
index a2d7e13..f095b5e 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -388,6 +388,8 @@ struct remote_state
      packet is exactly the same as the last we sent.  IOW, we only let
      the target know about program signals list changes.  */
   char *last_program_signals_packet;
+
+  enum gdb_signal last_sent_signal;
 };
 
 /* Private data that we'll store in (struct thread_info)->private.  */
@@ -434,6 +436,7 @@ new_remote_state (void)
   result->buf_size = 400;
   result->buf = xmalloc (result->buf_size);
   result->remote_traceframe_number = -1;
+  result->last_sent_signal = GDB_SIGNAL_0;
 
   return result;
 }
@@ -4910,8 +4913,6 @@ remote_vcont_resume (ptid_t ptid, int step, enum gdb_signal siggnal)
 
 /* Tell the remote machine to resume.  */
 
-static enum gdb_signal last_sent_signal = GDB_SIGNAL_0;
-
 static int last_sent_step;
 
 static void
@@ -4930,7 +4931,7 @@ remote_resume (struct target_ops *ops,
   if (!non_stop)
     remote_notif_process (&notif_client_stop);
 
-  last_sent_signal = siggnal;
+  rs->last_sent_signal = siggnal;
   last_sent_step = step;
 
   /* The vCont packet doesn't need to specify threads via Hc.  */
@@ -6022,15 +6023,15 @@ remote_wait_as (ptid_t ptid, struct target_waitstatus *status, int options)
 
       break;
     case '\0':
-      if (last_sent_signal != GDB_SIGNAL_0)
+      if (rs->last_sent_signal != GDB_SIGNAL_0)
 	{
 	  /* Zero length reply means that we tried 'S' or 'C' and the
 	     remote system doesn't support it.  */
 	  target_terminal_ours_for_output ();
 	  printf_filtered
 	    ("Can't send signals to this remote system.  %s not sent.\n",
-	     gdb_signal_to_name (last_sent_signal));
-	  last_sent_signal = GDB_SIGNAL_0;
+	     gdb_signal_to_name (rs->last_sent_signal));
+	  rs->last_sent_signal = GDB_SIGNAL_0;
 	  target_terminal_inferior ();
 
 	  strcpy ((char *) buf, last_sent_step ? "s" : "c");
-- 
1.8.1.4

Re: [PATCH v2 00/16]

[Pedro Alves]

On 06/28/2013 06:40 PM, Tom Tromey wrote:
> Here's an updated version of my series to move remote.c global state
> into struct remote_state.
> 
> Original note here:
> 
>     http://sourceware.org/ml/gdb-patches/2013-06/msg00605.html
> 
> I believe this version addresses all the comments I received.

Thanks.  I thrown sent a comment on a minor issue I missed the
first time around, and a couple nits, but other than that, this
looks good to me.

Thanks!

-- 
Pedro Alves

[PATCH 00/16] clean up remote.c state

[Tom Tromey]

I was toying with David Taylor's idea of multi-target support, and I
came up with this preliminary series to do some cleanups of remote.c.

Most of this cleanup amounts to moving various global variables into
struct remote_state.  I did each variable (or in some cases, a few
clearly related variables) as a separate patch, to make the series
maximally understandable.  Each patch can essentially be read (though
not applied) in isolation.

I found the globals using nm.  Then I went through them by hand to
find ones that clearly must be global.


This patch does not fix all the globals in remote.c.  It is just a
start.  For example:

* I suspect the remote_g_packet_data stuff must be per-remote
* remote_protocol_packets has some per-remote state
* I think the client-stop notification code needs some update
* Maybe signal handling needs some fixing up, but what else is new


After this series goes in, I propose that all future remote.c changes
be reviewed to ensure that remote state is in remote_state and not a
new global variable.


This series also does a couple of other minor cleanups to remote.c.
It removes the gdb-specific CRC code in favor of libiberty's; and it
fixes an oddity I noticed with sizeof_pkg.


I built and regression tested this series on x86-64 Fedora 18 using
the native-gdbserver target board.

Tom

[PATCH 14/16] move async_client_callback and async_client_context into remote_state

[Tom Tromey]

This moves async_client_callback and async_client_context into
remote_state.

	* remote.c (struct remote_state) <async_client_callback,
	async_client_context>:	New fields.
	(async_client_callback, async_client_context): Remove.
	(record_currthread, set_thread, set_general_process)
	(remote_async_serial_handler, remote_async): Update.
---
 gdb/remote.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/gdb/remote.c b/gdb/remote.c
index 94aa5a8..39ad2ef 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -409,6 +409,10 @@ struct remote_state
 
   int use_threadinfo_query;
   int use_threadextra_query;
+
+  void (*async_client_callback) (enum inferior_event_type event_type,
+				 void *context);
+  void *async_client_context;
 };
 
 /* Private data that we'll store in (struct thread_info)->private.  */
@@ -11638,17 +11642,16 @@ remote_is_async_p (void)
    will be able to delay notifying the client of an event until the
    point where an entire packet has been received.  */
 
-static void (*async_client_callback) (enum inferior_event_type event_type,
-				      void *context);
-static void *async_client_context;
 static serial_event_ftype remote_async_serial_handler;
 
 static void
 remote_async_serial_handler (struct serial *scb, void *context)
 {
+  struct remote_state *rs = context;
+
   /* Don't propogate error information up to the client.  Instead let
      the client find out about the error by querying the target.  */
-  async_client_callback (INF_REG_EVENT, async_client_context);
+  rs->async_client_callback (INF_REG_EVENT, rs->async_client_context);
 }
 
 static void
@@ -11665,9 +11668,9 @@ remote_async (void (*callback) (enum inferior_event_type event_type,
 
   if (callback != NULL)
     {
-      serial_async (rs->remote_desc, remote_async_serial_handler, NULL);
-      async_client_callback = callback;
-      async_client_context = context;
+      serial_async (rs->remote_desc, remote_async_serial_handler, rs);
+      rs->async_client_callback = callback;
+      rs->async_client_context = context;
     }
   else
     serial_async (rs->remote_desc, NULL, NULL);
-- 
1.8.1.4

Re: [PATCH 14/16] move async_client_callback and async_client_context into remote_state

[Pedro Alves]

On 06/21/2013 06:25 PM, Tom Tromey wrote:
> This moves async_client_callback and async_client_context into
> remote_state.
> 
> 	* remote.c (struct remote_state) <async_client_callback,
> 	async_client_context>:	New fields.
> 	(async_client_callback, async_client_context): Remove.
> 	(record_currthread, set_thread, set_general_process)

This line looks a pasto from another patch.

> 	(remote_async_serial_handler, remote_async): Update.
> ---
>  gdb/remote.c | 17 ++++++++++-------
>  1 file changed, 10 insertions(+), 7 deletions(-)
> 
> diff --git a/gdb/remote.c b/gdb/remote.c
> index 94aa5a8..39ad2ef 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -409,6 +409,10 @@ struct remote_state
>  
>    int use_threadinfo_query;
>    int use_threadextra_query;
> +
> +  void (*async_client_callback) (enum inferior_event_type event_type,
> +				 void *context);
> +  void *async_client_context;
>  };
>  
>  /* Private data that we'll store in (struct thread_info)->private.  */
> @@ -11638,17 +11642,16 @@ remote_is_async_p (void)
>     will be able to delay notifying the client of an event until the
>     point where an entire packet has been received.  */

This comment above is related to the callback.

>  
> -static void (*async_client_callback) (enum inferior_event_type event_type,
> -				      void *context);
> -static void *async_client_context;
>  static serial_event_ftype remote_async_serial_handler;
>  
>  static void
>  remote_async_serial_handler (struct serial *scb, void *context)
>  {
> +  struct remote_state *rs = context;

Does this actually work?

> +
>    /* Don't propogate error information up to the client.  Instead let
>       the client find out about the error by querying the target.  */
> -  async_client_callback (INF_REG_EVENT, async_client_context);
> +  rs->async_client_callback (INF_REG_EVENT, rs->async_client_context);
>  }
>  
>  static void
> @@ -11665,9 +11668,9 @@ remote_async (void (*callback) (enum inferior_event_type event_type,
>  
>    if (callback != NULL)
>      {
> -      serial_async (rs->remote_desc, remote_async_serial_handler, NULL);
> -      async_client_callback = callback;
> -      async_client_context = context;
> +      serial_async (rs->remote_desc, remote_async_serial_handler, rs);
> +      rs->async_client_callback = callback;
> +      rs->async_client_context = context;

Because I think CONTEXT is always NULL here.

And it'd seem a wrong to me to store the remote state in the
meant for context passed in by the target_async's caller?

-- 
Pedro Alves

Re: [PATCH 14/16] move async_client_callback and async_client_context into remote_state

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

>> static void
>> remote_async_serial_handler (struct serial *scb, void *context)
>> {
>> +  struct remote_state *rs = context;

Pedro> Does this actually work?

>> +      serial_async (rs->remote_desc, remote_async_serial_handler, rs);
>> +      rs->async_client_callback = callback;
>> +      rs->async_client_context = context;

Pedro> Because I think CONTEXT is always NULL here.

Pedro> And it'd seem a wrong to me to store the remote state in the
Pedro> meant for context passed in by the target_async's caller?

I think it should work.

There are two callbacks and two "context" values here.

In that second snippet, "callback" and "context" are the arguments to
remote_async.  We stash those in the struct remote_state.

Then we call serial_async with remote_async_serial_handler as a callback
and with the remote_state as the context.

When remote_async_serial_handler is called, we fetch the original
callback and the original context from the remote_state, and call those.

We need this second callback layer because we want to add in the
INF_REG_EVENT argument.

I hope that's clear and correct.

Tom

Re: [PATCH 14/16] move async_client_callback and async_client_context into remote_state

[Pedro Alves]

On 06/24/2013 07:43 PM, Tom Tromey wrote:
>>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
> 
>>> static void
>>> remote_async_serial_handler (struct serial *scb, void *context)
>>> {
>>> +  struct remote_state *rs = context;
> 
> Pedro> Does this actually work?
> 
>>> +      serial_async (rs->remote_desc, remote_async_serial_handler, rs);
>>> +      rs->async_client_callback = callback;
>>> +      rs->async_client_context = context;
> 
> Pedro> Because I think CONTEXT is always NULL here.
> 
> Pedro> And it'd seem a wrong to me to store the remote state in the
> Pedro> meant for context passed in by the target_async's caller?
> 
> I think it should work.
> 
> There are two callbacks and two "context" values here.
> 
> In that second snippet, "callback" and "context" are the arguments to
> remote_async.  We stash those in the struct remote_state.
> 
> Then we call serial_async with remote_async_serial_handler as a callback
> and with the remote_state as the context.
> 
> When remote_async_serial_handler is called, we fetch the original
> callback and the original context from the remote_state, and call those.
> 
> We need this second callback layer because we want to add in the
> INF_REG_EVENT argument.

Ah, I see now.  Sorry for missing it.

> I hope that's clear and correct.

Thanks, it is.

-- 
Pedro Alves