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

[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 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, 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 287b701..94aa5a8 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -496,8 +496,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

Re: [PATCH 13/16] move sizeof_pkt into remote_trace_find

[Yao Qi]

On 06/22/2013 01:25 AM, Tom Tromey wrote:
> 	* remote.c (sizeof_pkt): Remove.
> 	(remote_trace_find): Use rs->buf, not sizeof_pkt.
                              ^^^^^^^^^^^^
"Use rs->buf_size" is the right text to describe the change.

-- 
Yao (齐尧)

Re: [PATCH 13/16] move sizeof_pkt into remote_trace_find

[Tom Tromey]

>>>>> "Yao" == Yao Qi <yao@codesourcery.com> writes:

>> * remote.c (sizeof_pkt): Remove.
>> (remote_trace_find): Use rs->buf, not sizeof_pkt.

Yao> "Use rs->buf_size" is the right text to describe the change.

Thanks.  I've fixed this locally.

Tom

Re: [PATCH 13/16] move sizeof_pkt into remote_trace_find

[Pedro Alves]

On 06/21/2013 06:25 PM, Tom Tromey wrote:
> 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.

Yeah.  A leftover from making the tracepoint bits go through
the target vector.  A few of these globals, along with all
the tracepoint RSP handling, used to live in tracepoint.c,
got moved to remote.c, and then some of these redundant globals
were never eliminated.

> 
> 	* remote.c (sizeof_pkt): Remove.
> 	(remote_trace_find): Use rs->buf, 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 287b701..94aa5a8 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -496,8 +496,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."));
>  
> 


-- 
Pedro Alves

[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 | 34 +++++++++++++++++-----------------
 1 file changed, 17 insertions(+), 17 deletions(-)

diff --git a/gdb/remote.c b/gdb/remote.c
index c77cdaf..287b701 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -398,6 +398,17 @@ 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.  */
@@ -1439,17 +1450,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;
@@ -2799,7 +2799,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);
@@ -2847,7 +2847,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;
 }
@@ -2892,7 +2892,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 ();
@@ -2913,7 +2913,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));
@@ -4357,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 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 39ad2ef..0210b9f 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -413,6 +413,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.  */
@@ -795,17 +802,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 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 054fd39..7aa20f3 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.  */
@@ -11797,9 +11811,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

[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 | 38 ++++++++++++++++++--------------------
 1 file changed, 18 insertions(+), 20 deletions(-)

diff --git a/gdb/remote.c b/gdb/remote.c
index 8d94eba..832c055 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,12 @@ 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 +1443,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 +1647,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 +1773,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 +1793,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 +1830,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);
 }
 
@@ -4344,8 +4342,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.  */
@@ -4563,7 +4561,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
     {
@@ -6062,13 +6060,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;
 }
@@ -7919,7 +7917,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 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 1c5b27d..c77cdaf 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -394,6 +394,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.  */
@@ -8703,10 +8707,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;
 
@@ -8715,19 +8715,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
@@ -8766,9 +8766,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/21/2013 06:25 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.

It's a cache, so we can always discard and refetch the data
it if switching targets discarded it.  But this is definitely
per-target/connection data, so putting in remote_state is a better choice.

Thanks,
-- 
Pedro Alves

[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 832c055..ba8cbfa 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -377,6 +377,10 @@ struct remote_state
 
   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.  */
@@ -422,6 +426,7 @@ new_remote_state (void)
 
   result->buf_size = 400;
   result->buf = xmalloc (result->buf_size);
+  result->remote_traceframe_number = -1;
 
   return result;
 }
@@ -1443,10 +1448,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).  */
 
@@ -4344,7 +4345,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;
@@ -6294,12 +6295,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);
@@ -11006,7 +11008,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 | 38 +++++++++++++++++++++-----------------
 1 file changed, 21 insertions(+), 17 deletions(-)

diff --git a/gdb/remote.c b/gdb/remote.c
index 0210b9f..e69ed29 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -267,6 +267,13 @@ struct vCont_action_support
 
 static int use_range_stepping = 1;
 
+#define OPAQUETHREADBYTES 8
+
+/* a 64 bit opaque identifier */
+typedef unsigned char threadref[OPAQUETHREADBYTES];
+
+#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.  */
@@ -420,6 +427,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.  */
@@ -1879,11 +1890,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.  */
@@ -2499,7 +2505,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.  */
@@ -2515,10 +2520,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.  */
@@ -2561,18 +2566,15 @@ remote_get_threadlist (int startflag, threadref *nextthread, int result_limit,
 
 /* 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)
@@ -2583,8 +2585,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;
@@ -2593,10 +2596,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

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

[Pedro Alves]

On 06/21/2013 06:25 PM, Tom Tromey wrote:
> 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.

Yeah.

>  static int use_range_stepping = 1;
>  
> +#define OPAQUETHREADBYTES 8
> +
> +/* a 64 bit opaque identifier */
> +typedef unsigned char threadref[OPAQUETHREADBYTES];
> +
> +#define MAXTHREADLISTRESULTS 32
> +

...

>      {
>        /* FIXME: This is a good reason to drop the packet.  */
>        /* Possably, there is a duplicate response.  */
> @@ -2561,18 +2566,15 @@ remote_get_threadlist (int startflag, threadref *nextthread, int result_limit,
>  
>  /* About this many threadisds fit in a packet.  */

This comment should be moved as well along with the macro.

>  
> -#define MAXTHREADLISTRESULTS 32
> -

-- 
Pedro Alves

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

[Tom Tromey]

>> /* About this many threadisds fit in a packet.  */

Pedro> This comment should be moved as well along with the macro.

Thanks, I made this change locally.

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

[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 19767bc..1c5b27d 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -392,6 +392,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.  */
@@ -4912,8 +4914,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)
@@ -4931,7 +4931,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.  */
@@ -6033,7 +6033,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 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 ba8cbfa..c60eb13 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -381,6 +381,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.  */
@@ -1653,8 +1655,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.  */
 
@@ -1665,6 +1665,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++)
@@ -1690,17 +1691,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);
@@ -4281,8 +4281,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 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 | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/gdb/remote.c b/gdb/remote.c
index c60eb13..9d57c27 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -383,6 +383,13 @@ 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.  */
@@ -1707,13 +1714,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.  */
 
@@ -1724,6 +1724,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++)
@@ -1749,17 +1750,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);
@@ -4286,8 +4286,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

Re: [PATCH 08/16] push last_program_signals_packet into struct remote_state

[Pedro Alves]

On 06/21/2013 06:24 PM, Tom Tromey wrote:
> +  /* 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;

I noticed a couple of the patches add an empty line between comment
and variable, while the rest of the structure doesn't do that.

-- 
Pedro Alves

Re: [PATCH 08/16] push last_program_signals_packet into struct remote_state

[Tom Tromey]

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

>> +  /* 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;

Pedro> I noticed a couple of the patches add an empty line between comment
Pedro> and variable, while the rest of the structure doesn't do that.

Thanks.  I fixed all the instances of this.

Tom

[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 7aa20f3..8d94eba 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.  */
@@ -4267,7 +4271,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? ")))
@@ -4290,29 +4294,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)
     {
@@ -4395,7 +4399,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;
@@ -7145,8 +7149,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;
@@ -7173,7 +7178,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."));
@@ -8902,7 +8909,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]);
@@ -8945,7 +8952,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);
@@ -9079,7 +9086,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.  */
@@ -9165,7 +9172,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)
@@ -9705,7 +9712,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;
@@ -10106,8 +10113,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");
@@ -10194,8 +10202,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);
@@ -10245,8 +10254,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);
@@ -11602,23 +11612,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
@@ -11648,14 +11662,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
@@ -11705,7 +11721,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 ();
 }
 
@@ -11774,14 +11792,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

Re: [PATCH 04/16] push remote_desc into struct remote_state

[Pedro Alves]

This is OK.

A comment on something that caught my eye:

On 06/21/2013 06:24 PM, Tom Tromey wrote:
> @@ -10194,8 +10202,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"));

This looks conceptually fishy.

A rs == NULL check would be less surprising.  After all, if
we were multi-target already, and not using the remote target,
get_remote_state would most naturally return NULL.

But if not using the remote target, we probably shouldn't
be getting here anyway.

remote_file_get could nowadays be using the target_fileio_XXX methods
instead of remote_hostio_XXX, and therefore the command could be
generalized to work with all targets.

Something to keep in mind, and file under fix-it-later...  Not sure
we have tests for these commands that try them when not connected to
a remote target.

-- 
Pedro Alves

Re: [PATCH 04/16] push remote_desc into struct remote_state

[Tom Tromey]

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

>> @@ -10194,8 +10202,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"));

Pedro> This looks conceptually fishy.

Pedro> A rs == NULL check would be less surprising.  After all, if
Pedro> we were multi-target already, and not using the remote target,
Pedro> get_remote_state would most naturally return NULL.

Pedro> But if not using the remote target, we probably shouldn't
Pedro> be getting here anyway.

Pedro> remote_file_get could nowadays be using the target_fileio_XXX methods
Pedro> instead of remote_hostio_XXX, and therefore the command could be
Pedro> generalized to work with all targets.

Pedro> Something to keep in mind, and file under fix-it-later...  Not sure
Pedro> we have tests for these commands that try them when not connected to
Pedro> a remote target.

Thanks Pedro.

You saw the future quite accurately here.  I did see crashes in this
code after the multi-target conversion, precisely because I changed
get_remote_state to return NULL when not connected; and this path is
exercised by the test suite.

So, a later patch will introduce the == NULL check as well.

In this series, though, get_remote_state cannot return NULL.  This may
seem odd, but it is consistent with the state of the code before the
series -- and I wanted to try to make this series obviously not
behavior-changing.

Tom

[PATCH] Make file transfer commands work with all (native) targets. (was: Re: [PATCH 04/16] push remote_desc into struct remote_state)

[Pedro Alves]

On 06/27/2013 09:21 PM, Tom Tromey wrote:
>>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

> Thanks Pedro.
> 
> You saw the future quite accurately here.  I did see crashes in this
> code after the multi-target conversion, precisely because I changed
> get_remote_state to return NULL when not connected; and this path is
> exercised by the test suite.
> 
> So, a later patch will introduce the == NULL check as well.
> 
> In this series, though, get_remote_state cannot return NULL.  This may
> seem odd, but it is consistent with the state of the code before the
> series -- and I wanted to try to make this series obviously not
> behavior-changing.

Ack.

> Pedro> remote_file_get could nowadays be using the target_fileio_XXX methods
> Pedro> instead of remote_hostio_XXX, and therefore the command could be
> Pedro> generalized to work with all targets.

Actually, doing this is quite easy, so I went ahead, in the name
of local/remote feature parity.  We can connect to a local gdbserver
and do file transfer in the local system; there seems to be no
reason we can't do it with native debugging too.

WDYT?

If this looks good, then a followup could move these routines
out of remote.c, and file-transfer.exp out of gdb.server/.

I considered aliasing "target put/get/delete" to "remote put/get/delete",
but didn't do it,as I didn't want to do too much in case you guys didn't
like this.  The MI commands, and their description in the manual are
already target agnostic.

-----
From 85c29ce88ec7f0d3533a6f900fe36571135bd452 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Fri, 28 Jun 2013 16:00:16 +0100
Subject: [PATCH] Make file transfer commands work with all (native) targets.

This makes the file transfer commands use the generic target_fileio_
hooks, therefore making them work the native targets too.

Tested on x86_64 Fedora 17.

gdb/
2013-06-28  Pedro Alves  <palves@redhat.com>

	* NEWS: Mention that file transfer commands now work with native
	targets.
	* remote.c (remote_fileio_errno_to_host): Rename to ...
	(fileio_errno_to_host_errno): ... this.  Add comment.
	(remote_hostio_error): Rename to ...
	(fileio_error): ... this.  Avoid saying "remote" in errors.
	(remote_hostio_close_cleanup): Rename to ...
	(target_fileio_close_cleanup): ... this.  Use the target_fileio
	interfaces rather than calling the remote methods directly.
	(remote_bfd_iovec_open, remote_bfd_iovec_pread): Adjust.
	(remote_file_put, remote_file_get, remote_file_delete): Use the
	target_fileio interfaces rather than calling the remote methods
	directly.  Rename locals and parameters.  Don't check if connected
	to a remote target.  Always try sending 4k bytes per transfer,
	instead of consulting the packet size.

gdb/doc/
2013-06-28  Pedro Alves  <palves@redhat.com>

	* gdb.texinfo (File Transfer): Mention that the file transfer
	commands also work with native targets.

gdb/testsuite/
2013-06-28  Pedro Alves  <palves@redhat.com>

	Make it work with all targets.
	* gdb.server/file-transfer.exp: Don't load gdbserver-support.exp.
	Don't check whether to skip gdbserver tests.  Don't force
	disconnect, and don't spawn gdbserver explicitly.  Run to main if
	testing with a remote stub.
---
 gdb/NEWS                                   |  8 +++
 gdb/doc/gdb.texinfo                        |  4 ++
 gdb/remote.c                               | 98 ++++++++++++++----------------
 gdb/testsuite/gdb.server/file-transfer.exp | 20 +++---
 4 files changed, 67 insertions(+), 63 deletions(-)

diff --git a/gdb/NEWS b/gdb/NEWS
index e469f1e..2e035aa 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -70,6 +70,10 @@ show range-stepping
 * The exception-related catchpoints, like "catch throw", now accept a
   regular expression which can be used to filter exceptions by type.
 
+* The remote put/get/delete commands (file transfer) now work with
+  native targets too.  Previously, they only worked with remote
+  targets.
+
 * MI changes
 
   ** The -trace-save MI command can optionally save trace buffer in Common
@@ -84,6 +88,10 @@ show range-stepping
   ** The new command -trace-frame-collected dumps collected variables,
      computed expressions, tvars, memory and registers in a traceframe.
 
+  ** The file transfer commands (-target-file-put, -target-file-get,
+     -target-file-delete) now work with native targets too.  Previously,
+     they only worked with remote targets.
+
 * New system-wide configuration scripts
   A GDB installation now provides scripts suitable for use as system-wide
   configuration scripts for the following systems:
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index d75a3d1..f270c87 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -17979,6 +17979,10 @@ the only way to upload or download files.
 
 Not all remote targets support these commands.
 
+Although most useful with remote targets, note that these commands
+also work when native debugging.  In that case, the host and target
+systems are the same system.
+
 @table @code
 @kindex remote put
 @item remote put @var{hostfile} @var{targetfile}
diff --git a/gdb/remote.c b/gdb/remote.c
index a29fe23..672898d 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -9950,8 +9950,11 @@ remote_hostio_readlink (const char *filename, int *remote_errno)
   return ret;
 }
 
+/* Convert ERRNUM from "Hosted File I/O" error number to host
+   errno.  */
+
 static int
-remote_fileio_errno_to_host (int errnum)
+fileio_errno_to_host_errno (int errnum)
 {
   switch (errnum)
     {
@@ -10002,23 +10005,23 @@ remote_fileio_errno_to_host (int errnum)
 }
 
 static char *
-remote_hostio_error (int errnum)
+fileio_error (int target_errno)
 {
-  int host_error = remote_fileio_errno_to_host (errnum);
+  int host_error = fileio_errno_to_host_errno (target_errno);
 
   if (host_error == -1)
-    error (_("Unknown remote I/O error %d"), errnum);
+    error (_("Unknown I/O error %d"), target_errno);
   else
-    error (_("Remote I/O error: %s"), safe_strerror (host_error));
+    error (_("I/O error: %s"), safe_strerror (host_error));
 }
 
 static void
-remote_hostio_close_cleanup (void *opaque)
+target_fileio_close_cleanup (void *opaque)
 {
   int fd = *(int *) opaque;
-  int remote_errno;
+  int target_errno;
 
-  remote_hostio_close (fd, &remote_errno);
+  target_fileio_close (fd, &target_errno);
 }
 
 
@@ -10034,7 +10037,7 @@ remote_bfd_iovec_open (struct bfd *abfd, void *open_closure)
   fd = remote_hostio_open (filename + 7, FILEIO_O_RDONLY, 0, &remote_errno);
   if (fd == -1)
     {
-      errno = remote_fileio_errno_to_host (remote_errno);
+      errno = fileio_errno_to_host_errno (remote_errno);
       bfd_set_error (bfd_error_system_call);
       return NULL;
     }
@@ -10078,7 +10081,7 @@ remote_bfd_iovec_pread (struct bfd *abfd, void *stream, void *buf,
         break;
       if (bytes == -1)
 	{
-	  errno = remote_fileio_errno_to_host (remote_errno);
+	  errno = fileio_errno_to_host_errno (remote_errno);
 	  bfd_set_error (bfd_error_system_call);
 	  return -1;
 	}
@@ -10116,37 +10119,34 @@ remote_bfd_open (const char *remote_file, const char *target)
 }
 
 void
-remote_file_put (const char *local_file, const char *remote_file, int from_tty)
+remote_file_put (const char *local_file, const char *target_file, int from_tty)
 {
   struct cleanup *back_to, *close_cleanup;
-  int retcode, fd, remote_errno, bytes, io_size;
+  int retcode, fd, target_errno, bytes, io_size;
   FILE *file;
   gdb_byte *buffer;
   int bytes_in_buffer;
   int saw_eof;
   ULONGEST offset;
 
-  if (!remote_desc)
-    error (_("command can only be used with remote target"));
-
   file = gdb_fopen_cloexec (local_file, "rb");
   if (file == NULL)
     perror_with_name (local_file);
   back_to = make_cleanup_fclose (file);
 
-  fd = remote_hostio_open (remote_file, (FILEIO_O_WRONLY | FILEIO_O_CREAT
+  fd = target_fileio_open (target_file, (FILEIO_O_WRONLY | FILEIO_O_CREAT
 					 | FILEIO_O_TRUNC),
-			   0700, &remote_errno);
+			   0700, &target_errno);
   if (fd == -1)
-    remote_hostio_error (remote_errno);
+    fileio_error (target_errno);
 
-  /* Send up to this many bytes at once.  They won't all fit in the
-     remote packet limit, so we'll transfer slightly fewer.  */
-  io_size = get_remote_packet_size ();
+  /* Start by sending up to 16K at a time.  The target will throttle
+     this number down if necessary.  */
+  io_size = 16384;
   buffer = xmalloc (io_size);
   make_cleanup (xfree, buffer);
 
-  close_cleanup = make_cleanup (remote_hostio_close_cleanup, &fd);
+  close_cleanup = make_cleanup (target_fileio_close_cleanup, &fd);
 
   bytes_in_buffer = 0;
   saw_eof = 0;
@@ -10178,13 +10178,13 @@ remote_file_put (const char *local_file, const char *remote_file, int from_tty)
       bytes += bytes_in_buffer;
       bytes_in_buffer = 0;
 
-      retcode = remote_hostio_pwrite (fd, buffer, bytes,
-				      offset, &remote_errno);
+      retcode = target_fileio_pwrite (fd, buffer, bytes,
+				      offset, &target_errno);
 
       if (retcode < 0)
-	remote_hostio_error (remote_errno);
+	fileio_error (target_errno);
       else if (retcode == 0)
-	error (_("Remote write of %d bytes returned 0!"), bytes);
+	error (_("Write of %d bytes returned 0!"), bytes);
       else if (retcode < bytes)
 	{
 	  /* Short write.  Save the rest of the read data for the next
@@ -10197,8 +10197,8 @@ remote_file_put (const char *local_file, const char *remote_file, int from_tty)
     }
 
   discard_cleanups (close_cleanup);
-  if (remote_hostio_close (fd, &remote_errno))
-    remote_hostio_error (remote_errno);
+  if (target_fileio_close (fd, &target_errno))
+    fileio_error (target_errno);
 
   if (from_tty)
     printf_filtered (_("Successfully sent file \"%s\".\n"), local_file);
@@ -10206,43 +10206,40 @@ remote_file_put (const char *local_file, const char *remote_file, int from_tty)
 }
 
 void
-remote_file_get (const char *remote_file, const char *local_file, int from_tty)
+remote_file_get (const char *target_file, const char *local_file, int from_tty)
 {
   struct cleanup *back_to, *close_cleanup;
-  int fd, remote_errno, bytes, io_size;
+  int fd, target_errno, bytes, io_size;
   FILE *file;
   gdb_byte *buffer;
   ULONGEST offset;
 
-  if (!remote_desc)
-    error (_("command can only be used with remote target"));
-
-  fd = remote_hostio_open (remote_file, FILEIO_O_RDONLY, 0, &remote_errno);
+  fd = target_fileio_open (target_file, FILEIO_O_RDONLY, 0, &target_errno);
   if (fd == -1)
-    remote_hostio_error (remote_errno);
+    fileio_error (target_errno);
 
   file = gdb_fopen_cloexec (local_file, "wb");
   if (file == NULL)
     perror_with_name (local_file);
   back_to = make_cleanup_fclose (file);
 
-  /* Send up to this many bytes at once.  They won't all fit in the
-     remote packet limit, so we'll transfer slightly fewer.  */
-  io_size = get_remote_packet_size ();
+  /* Start by reading up to 16K at a time.  The target will throttle
+     this number down if necessary.  */
+  io_size = 16384;
   buffer = xmalloc (io_size);
   make_cleanup (xfree, buffer);
 
-  close_cleanup = make_cleanup (remote_hostio_close_cleanup, &fd);
+  close_cleanup = make_cleanup (target_fileio_close_cleanup, &fd);
 
   offset = 0;
   while (1)
     {
-      bytes = remote_hostio_pread (fd, buffer, io_size, offset, &remote_errno);
+      bytes = target_fileio_pread (fd, buffer, io_size, offset, &target_errno);
       if (bytes == 0)
 	/* Success, but no bytes, means end-of-file.  */
 	break;
       if (bytes == -1)
-	remote_hostio_error (remote_errno);
+	fileio_error (target_errno);
 
       offset += bytes;
 
@@ -10252,28 +10249,25 @@ remote_file_get (const char *remote_file, const char *local_file, int from_tty)
     }
 
   discard_cleanups (close_cleanup);
-  if (remote_hostio_close (fd, &remote_errno))
-    remote_hostio_error (remote_errno);
+  if (target_fileio_close (fd, &target_errno))
+    fileio_error (target_errno);
 
   if (from_tty)
-    printf_filtered (_("Successfully fetched file \"%s\".\n"), remote_file);
+    printf_filtered (_("Successfully fetched file \"%s\".\n"), target_file);
   do_cleanups (back_to);
 }
 
 void
-remote_file_delete (const char *remote_file, int from_tty)
+remote_file_delete (const char *target_file, int from_tty)
 {
-  int retcode, remote_errno;
-
-  if (!remote_desc)
-    error (_("command can only be used with remote target"));
+  int retcode, target_errno;
 
-  retcode = remote_hostio_unlink (remote_file, &remote_errno);
+  retcode = target_fileio_unlink (target_file, &target_errno);
   if (retcode == -1)
-    remote_hostio_error (remote_errno);
+    fileio_error (target_errno);
 
   if (from_tty)
-    printf_filtered (_("Successfully deleted file \"%s\".\n"), remote_file);
+    printf_filtered (_("Successfully deleted file \"%s\".\n"), target_file);
 }
 
 static void
diff --git a/gdb/testsuite/gdb.server/file-transfer.exp b/gdb/testsuite/gdb.server/file-transfer.exp
index aa56380..2e17216 100644
--- a/gdb/testsuite/gdb.server/file-transfer.exp
+++ b/gdb/testsuite/gdb.server/file-transfer.exp
@@ -16,23 +16,21 @@
 
 # Test gdbserver monitor commands.
 
-load_lib gdbserver-support.exp
-
 standard_testfile server.c
 
-if { [skip_gdbserver_tests] } {
-    return 0
-}
-
 if {[prepare_for_testing $testfile.exp $testfile $srcfile debug]} {
     return -1
 }
 
-# Make sure we're disconnected, in case we're testing with an
-# extended-remote board, therefore already connected.
-gdb_test "disconnect" ".*"
-
-gdbserver_run ""
+# If using a remote stub, then make sure we're connected, otherwise
+# fileio operations fall back to the native target.  IOW, we'd test
+# the wrong target.
+if [target_info exists use_gdb_stub] {
+    if { ![runto_main] } then {
+	fail "Can't run to main"
+	return -1
+    }
+}
 
 proc test_file_transfer { filename description } {
     gdb_test "remote put \"$filename\" down-server" \
-- 
1.7.11.7

Re: [PATCH] Make file transfer commands work with all (native) targets.

[Tom Tromey]

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

Pedro> Actually, doing this is quite easy, so I went ahead, in the name
Pedro> of local/remote feature parity.  We can connect to a local gdbserver
Pedro> and do file transfer in the local system; there seems to be no
Pedro> reason we can't do it with native debugging too.

Looks good to me.

Pedro> If this looks good, then a followup could move these routines
Pedro> out of remote.c, and file-transfer.exp out of gdb.server/.

Pedro> I considered aliasing "target put/get/delete" to "remote put/get/delete",
Pedro> but didn't do it,as I didn't want to do too much in case you guys didn't
Pedro> like this.  The MI commands, and their description in the manual are
Pedro> already target agnostic.

I could go either way on this.

Tom

Re: [PATCH] Make file transfer commands work with all (native) targets. (was: Re: [PATCH 04/16] push remote_desc into struct remote_state)

[Eli Zaretskii]

> Date: Fri, 28 Jun 2013 17:33:58 +0100
> From: Pedro Alves <palves@redhat.com>
> CC: gdb-patches@sourceware.org
> 
> > Pedro> remote_file_get could nowadays be using the target_fileio_XXX methods
> > Pedro> instead of remote_hostio_XXX, and therefore the command could be
> > Pedro> generalized to work with all targets.
> 
> Actually, doing this is quite easy, so I went ahead, in the name
> of local/remote feature parity.  We can connect to a local gdbserver
> and do file transfer in the local system; there seems to be no
> reason we can't do it with native debugging too.
> 
> WDYT?

What is the use case?

Without a good use case, having "remote get" serve like a poor man's
'cp' is confusing, IMO.

Re: [PATCH] Make file transfer commands work with all (native) targets.

[Pedro Alves]

On 06/28/2013 06:44 PM, Eli Zaretskii wrote:
>> Date: Fri, 28 Jun 2013 17:33:58 +0100
>> From: Pedro Alves <palves@redhat.com>
>> CC: gdb-patches@sourceware.org
>>
>>> Pedro> remote_file_get could nowadays be using the target_fileio_XXX methods
>>> Pedro> instead of remote_hostio_XXX, and therefore the command could be
>>> Pedro> generalized to work with all targets.
>>
>> Actually, doing this is quite easy, so I went ahead, in the name
>> of local/remote feature parity.  We can connect to a local gdbserver
>> and do file transfer in the local system; there seems to be no
>> reason we can't do it with native debugging too.
>>
>> WDYT?
> 
> What is the use case?

Yeah, I admit it fits more in the general "as fewer differences
we have between local/remote debugging, the better" theme than
driven by a particular use case.  A possible example would be something
like gdb scripts working the same whether connected to a remote
or local target (and unaware of whether the local target is implemented
by local gdbserver or the native built-in target).

> Without a good use case, having "remote get" serve like a poor man's
> 'cp' is confusing, IMO.

Would you be OK with, or prefer, adding "target get/put/delete", leaving
the "remote" variants in place?  The difference between the "target" and
"remote" variants would be that the former would work with any target,
while the later would still only work with the remote target.

-- 
Pedro Alves

Re: [PATCH] Make file transfer commands work with all (native) targets.

[Eli Zaretskii]

> Date: Fri, 28 Jun 2013 19:25:59 +0100
> From: Pedro Alves <palves@redhat.com>
> CC: tromey@redhat.com, gdb-patches@sourceware.org
> 
> Yeah, I admit it fits more in the general "as fewer differences
> we have between local/remote debugging, the better" theme than
> driven by a particular use case.  A possible example would be something
> like gdb scripts working the same whether connected to a remote
> or local target (and unaware of whether the local target is implemented
> by local gdbserver or the native built-in target).

But putting files on a remote target puts them on the board, no?
There's no analogous place in native debugging.

> > Without a good use case, having "remote get" serve like a poor man's
> > 'cp' is confusing, IMO.
> 
> Would you be OK with, or prefer, adding "target get/put/delete", leaving
> the "remote" variants in place?

I guess so, but then won't you lose backward compatibility?

Re: [PATCH] Make file transfer commands work with all (native) targets.

[Tom Tromey]

>> Yeah, I admit it fits more in the general "as fewer differences
>> we have between local/remote debugging, the better" theme than
>> driven by a particular use case.  A possible example would be something
>> like gdb scripts working the same whether connected to a remote
>> or local target (and unaware of whether the local target is implemented
>> by local gdbserver or the native built-in target).

Eli> But putting files on a remote target puts them on the board, no?
Eli> There's no analogous place in native debugging.

There's the local filesystem.
You can run gdbserver locally and still use these commands.

>> > Without a good use case, having "remote get" serve like a poor man's
>> > 'cp' is confusing, IMO.
>> 
>> Would you be OK with, or prefer, adding "target get/put/delete", leaving
>> the "remote" variants in place?

Eli> I guess so, but then won't you lose backward compatibility?

He means to leave the "remote" variants in place.

Tom

Re: [PATCH] Make file transfer commands work with all (native) targets.

[Pedro Alves]

On 07/01/2013 03:28 PM, Tom Tromey wrote:

> Eli> But putting files on a remote target puts them on the board, no?
> Eli> There's no analogous place in native debugging.
> 
> There's the local filesystem.
> You can run gdbserver locally and still use these commands.
> 
>>>> Without a good use case, having "remote get" serve like a poor man's
>>>> 'cp' is confusing, IMO.
>>>
>>> Would you be OK with, or prefer, adding "target get/put/delete", leaving
>>> the "remote" variants in place?
> 
> Eli> I guess so, but then won't you lose backward compatibility?
> 
> He means to leave the "remote" variants in place.

Yep.

Though I somehow missed realizing "target get/put/delete"
conflicts with the usual sense we use "target foo" commands
for...  The next obvious one, "file", is taken too...
"target-file" or "fileio" would be my next choices.  Either
changes tab completion behavior for "target" and "file", forcing
the user to type a space, so perhaps not 100% ideal.
Making "remote put/get/delete" was a lot simpler.  :-)

For MI, I'd still make -target-file-get work with native
targets without adding new commands.  These are not documented
as only working with remote targets.  If the frontend wants
to restrict them to remotes, it can do that itself before
calling into GDB.

-- 
Pedro Alves

Re: [PATCH] Make file transfer commands work with all (native) targets.

[Eli Zaretskii]

> From: Tom Tromey <tromey@redhat.com>
> Cc: Pedro Alves <palves@redhat.com>, gdb-patches@sourceware.org
> Date: Mon, 01 Jul 2013 08:28:24 -0600
> 
> Eli> But putting files on a remote target puts them on the board, no?
> Eli> There's no analogous place in native debugging.
> 
> There's the local filesystem.

Yes, I know; but "put" and "get" don't make much sense in that case.

> >> Would you be OK with, or prefer, adding "target get/put/delete", leaving
> >> the "remote" variants in place?
> 
> Eli> I guess so, but then won't you lose backward compatibility?
> 
> He means to leave the "remote" variants in place.

OK.

Re: [PATCH] Make file transfer commands work with all (native) targets.

[Pedro Alves]

On 07/01/2013 05:41 PM, Eli Zaretskii wrote:
>> From: Tom Tromey <tromey@redhat.com>
>> Cc: Pedro Alves <palves@redhat.com>, gdb-patches@sourceware.org
>> Date: Mon, 01 Jul 2013 08:28:24 -0600
>>
>> Eli> But putting files on a remote target puts them on the board, no?
>> Eli> There's no analogous place in native debugging.
>>
>> There's the local filesystem.
> 
> Yes, I know; but "put" and "get" don't make much sense in that case.

IMO, it makes as much sense as "ftp localhost; get; put".

-- 
Pedro Alves

[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 080d048..c2475f3 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -8523,36 +8523,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
@@ -8573,7 +8543,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

Re: [PATCH 01/16] use the libiberty crc code

[Pedro Alves]

On 06/21/2013 06:24 PM, Tom Tromey wrote:
> gdb has a copy of some CRC code that also appears in libiberty.
> This patch just removes the local copy.

No exactly the same, as libiberty's pre-computes the table, but,
yeah.

> 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.

That's fine.

(gdbserver also has it's own copy in server.c (and for that one
the return type does matter), but we don't use
libiberty-the-kitchen-sink there...  Oh well.)

Thanks,
-- 
Pedro Alves

Re: [PATCH 01/16] use the libiberty crc code

[Tom Tromey]

Pedro> (gdbserver also has it's own copy in server.c (and for that one
Pedro> the return type does matter), but we don't use
Pedro> libiberty-the-kitchen-sink there...  Oh well.)

If we used libiberty in gdbserver, we could replace this code.  We'd
just have to hoist the inferior-memory-reading bit outside of the crc
function.  This wouldn't be so bad; it might even be an improvement.  I
don't plan to mess with this though.

Tom

[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 9d57c27..19767bc 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -390,6 +390,8 @@ struct remote_state
      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.  */
@@ -436,6 +438,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;
 }
@@ -4909,8 +4912,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
@@ -4929,7 +4930,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.  */
@@ -6021,15 +6022,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

[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 c2475f3..054fd39 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -3965,7 +3965,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

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

[Pedro Alves]

On 06/21/2013 06:24 PM, Tom Tromey wrote:

> 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.

Thanks for doing this.

I read through the whole series and send off a few comments.  Where I
didn't reply, is because the patch looked fine as is.

Thanks,
-- 
Pedro Alves

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

[Yao Qi]

On 06/22/2013 01:24 AM, Tom Tromey wrote:
> * I think the client-stop notification code needs some update

Yes, the global 'notif_queue' should be moved for each remote state.

My pending patches on supported notifications and annexes probably 
should be updated for per remote target as well, because different 
remote targets may have different supported notifications and annexes.

   [PATCH v4 0/5] MI notification on trace started/stopped
   http://sourceware.org/ml/gdb-patches/2013-04/msg00019.html

I am not sure we are still interested in this patch series, as I get no 
response after a recent ping to Pedro on June.

> 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.
>

I agree.  The 'trace started/stopped' patch series were submitted some 
months ago, IWBN to review them now, and then I'd like to convert the 
whole async remote notification to a per remote target manner in the 
next step.  It will save a lot of efforts on rewriting patches, 
re-splitting and rebasing.  WDYT?

-- 
Yao (齐尧)

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

[Pedro Alves]

On 07/05/2013 04:06 AM, Yao Qi wrote:
> On 06/22/2013 01:24 AM, Tom Tromey wrote:
>> * I think the client-stop notification code needs some update
> 
> Yes, the global 'notif_queue' should be moved for each remote state.
> 
> My pending patches on supported notifications and annexes probably 
> should be updated for per remote target as well, because different 
> remote targets may have different supported notifications and annexes.
> 
>    [PATCH v4 0/5] MI notification on trace started/stopped
>    http://sourceware.org/ml/gdb-patches/2013-04/msg00019.html
> 
> I am not sure we are still interested in this patch series, as I get no 
> response after a recent ping to Pedro on June.

Yes, there's interest, but I haven't managed to get to it yet.  :-(

-- 
Pedro Alves

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

[Tom Tromey]

>>>>> "Tom" == Tom Tromey <tromey@redhat.com> writes:

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

Tom> Most of this cleanup amounts to moving various global variables
Tom> into struct remote_state.

I'm checking this in now.

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

I re-ran the regression tests in both modes.

Tom