[11/10] remote_target::m_remote_state, pointer -> object (Re: [PATCH 10/10] remote: one struct remote_state per struct remote_target)

Message ID 4d9a613f-b420-aa72-8561-549b383285fa@redhat.com
State New
Headers show
Series
  • remote: More multi-target preparation
Related show

Commit Message

Pedro Alves May 24, 2018, 3:53 p.m.
On 05/22/2018 06:30 PM, Pedro Alves wrote:
> On 05/22/2018 04:37 AM, Simon Marchi wrote:


>> Is there a reason not to make the remote_state object a simple field

>> of remote_target, does it have to be a pointer?  You would have to

>> shuffle things around a little bit more, but it seems to work fine.

> 

> Yeah, no reason other than struct remote_state not being complete yet 

> when the field is defined in struct remote_target.  I was thinking the

> moving would be done as follow up, to avoid even more churn mixed in with

> changes, very much like patch #4 started with a pointer and then patch #5

> moved to objects.

Like this?

(Note this is generated with "git format-patch --patience".
The default diff algorithm produces a much less readable patch.)

From 3ab58d94cb7b3b7039f3e02b5ed72571e546f80d Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>

Date: Thu, 24 May 2018 16:33:23 +0100
Subject: [PATCH] remote_target::m_remote_state, pointer -> object

The only reason remote_target::m_remote_state is a pointer is that
struct remote_state is incomplete when struct remote_target is
defined.

This commit thus moves struct remote_state (and its dependencies)
higher up and makes remote_target::m_remote_state an object instead of
a pointer.

gdb/ChangeLog:
2018-05-24  Pedro Alves  <palves@redhat.com>

	* remote.c (struct vCont_action_support, MAXTHREADLISTRESULTS)
	(struct readahead_cache, struct packet_reg, struct
	remote_arch_state, class remote_state): Move higher up in the
	file.
	(remote_target::m_remote_state): Now an object instead of a pointer.
	(remote_target::get_remote_state): Adjust.
---
 gdb/remote.c | 546 +++++++++++++++++++++++++++++------------------------------
 1 file changed, 272 insertions(+), 274 deletions(-)

-- 
2.14.3

Comments

Simon Marchi May 25, 2018, 3:30 a.m. | #1
On 2018-05-24 11:53, Pedro Alves wrote:
> On 05/22/2018 06:30 PM, Pedro Alves wrote:

>> On 05/22/2018 04:37 AM, Simon Marchi wrote:

> 

>>> Is there a reason not to make the remote_state object a simple field

>>> of remote_target, does it have to be a pointer?  You would have to

>>> shuffle things around a little bit more, but it seems to work fine.

>> 

>> Yeah, no reason other than struct remote_state not being complete yet

>> when the field is defined in struct remote_target.  I was thinking the

>> moving would be done as follow up, to avoid even more churn mixed in 

>> with

>> changes, very much like patch #4 started with a pointer and then patch 

>> #5

>> moved to objects.

> Like this?


Yep, looks good!

Patch

diff --git a/gdb/remote.c b/gdb/remote.c
index 3e4566d985..38ba7b581c 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -123,7 +123,273 @@  enum packet_result
 };
 
 struct threads_listing_context;
-struct remote_state;
+
+/* Stub vCont actions support.
+
+   Each field is a boolean flag indicating whether the stub reports
+   support for the corresponding action.  */
+
+struct vCont_action_support
+{
+  /* vCont;t */
+  bool t = false;
+
+  /* vCont;r */
+  bool r = false;
+
+  /* vCont;s */
+  bool s = false;
+
+  /* vCont;S */
+  bool S = false;
+};
+
+/* About this many threadisds fit in a packet.  */
+
+#define MAXTHREADLISTRESULTS 32
+
+/* Data for the vFile:pread readahead cache.  */
+
+struct readahead_cache
+{
+  /* Invalidate the readahead cache.  */
+  void invalidate ();
+
+  /* Invalidate the readahead cache if it is holding data for FD.  */
+  void invalidate_fd (int fd);
+
+  /* Serve pread from the readahead cache.  Returns number of bytes
+     read, or 0 if the request can't be served from the cache.  */
+  int pread (int fd, gdb_byte *read_buf, size_t len, ULONGEST offset);
+
+  /* The file descriptor for the file that is being cached.  -1 if the
+     cache is invalid.  */
+  int fd = -1;
+
+  /* The offset into the file that the cache buffer corresponds
+     to.  */
+  ULONGEST offset = 0;
+
+  /* The buffer holding the cache contents.  */
+  gdb_byte *buf = nullptr;
+  /* The buffer's size.  We try to read as much as fits into a packet
+     at a time.  */
+  size_t bufsize = 0;
+
+  /* Cache hit and miss counters.  */
+  ULONGEST hit_count = 0;
+  ULONGEST miss_count = 0;
+};
+
+/* Description of the remote protocol for a given architecture.  */
+
+struct packet_reg
+{
+  long offset; /* Offset into G packet.  */
+  long regnum; /* GDB's internal register number.  */
+  LONGEST pnum; /* Remote protocol register number.  */
+  int in_g_packet; /* Always part of G packet.  */
+  /* long size in bytes;  == register_size (target_gdbarch (), regnum);
+     at present.  */
+  /* char *name; == gdbarch_register_name (target_gdbarch (), regnum);
+     at present.  */
+};
+
+struct remote_arch_state
+{
+  explicit remote_arch_state (struct gdbarch *gdbarch);
+
+  /* Description of the remote protocol registers.  */
+  long sizeof_g_packet;
+
+  /* Description of the remote protocol registers indexed by REGNUM
+     (making an array gdbarch_num_regs in size).  */
+  std::unique_ptr<packet_reg[]> regs;
+
+  /* This is the size (in chars) of the first response to the ``g''
+     packet.  It is used as a heuristic when determining the maximum
+     size of memory-read and memory-write packets.  A target will
+     typically only reserve a buffer large enough to hold the ``g''
+     packet.  The size does not include packet overhead (headers and
+     trailers).  */
+  long actual_register_packet_size;
+
+  /* This is the maximum size (in chars) of a non read/write packet.
+     It is also used as a cap on the size of read/write packets.  */
+  long remote_packet_size;
+};
+
+/* Description of the remote protocol state for the currently
+   connected target.  This is per-target state, and independent of the
+   selected architecture.  */
+
+class remote_state
+{
+public:
+
+  remote_state ();
+  ~remote_state ();
+
+  /* Get the remote arch state for GDBARCH.  */
+  struct remote_arch_state *get_remote_arch_state (struct gdbarch *gdbarch);
+
+public: /* data */
+
+  /* A buffer to use for incoming packets, and its current size.  The
+     buffer is grown dynamically for larger incoming packets.
+     Outgoing packets may also be constructed in this buffer.
+     BUF_SIZE is always at least REMOTE_PACKET_SIZE;
+     REMOTE_PACKET_SIZE should be used to limit the length of outgoing
+     packets.  */
+  char *buf;
+  long buf_size;
+
+  /* True if we're going through initial connection setup (finding out
+     about the remote side's threads, relocating symbols, etc.).  */
+  bool starting_up = false;
+
+  /* If we negotiated packet size explicitly (and thus can bypass
+     heuristics for the largest packet size that will not overflow
+     a buffer in the stub), this will be set to that packet size.
+     Otherwise zero, meaning to use the guessed size.  */
+  long explicit_packet_size = 0;
+
+  /* remote_wait is normally called when the target is running and
+     waits for a stop reply packet.  But sometimes we need to call it
+     when the target is already stopped.  We can send a "?" packet
+     and have remote_wait read the response.  Or, if we already have
+     the response, we can stash it in BUF and tell remote_wait to
+     skip calling getpkt.  This flag is set when BUF contains a
+     stop reply packet and the target is not waiting.  */
+  int cached_wait_status = 0;
+
+  /* True, if in no ack mode.  That is, neither GDB nor the stub will
+     expect acks from each other.  The connection is assumed to be
+     reliable.  */
+  bool noack_mode = false;
+
+  /* True if we're connected in extended remote mode.  */
+  bool extended = false;
+
+  /* True if we resumed the target and we're waiting for the target to
+     stop.  In the mean time, we can't start another command/query.
+     The remote server wouldn't be ready to process it, so we'd
+     timeout waiting for a reply that would never come and eventually
+     we'd close the connection.  This can happen in asynchronous mode
+     because we allow GDB commands while the target is running.  */
+  bool waiting_for_stop_reply = false;
+
+  /* The status of the stub support for the various vCont actions.  */
+  vCont_action_support supports_vCont;
+
+  /* True if the user has pressed Ctrl-C, but the target hasn't
+     responded to that.  */
+  bool ctrlc_pending_p = false;
+
+  /* True if we saw a Ctrl-C while reading or writing from/to the
+     remote descriptor.  At that point it is not safe to send a remote
+     interrupt packet, so we instead remember we saw the Ctrl-C and
+     process it once we're done with sending/receiving the current
+     packet, which should be shortly.  If however that takes too long,
+     and the user presses Ctrl-C again, we offer to disconnect.  */
+  bool got_ctrlc_during_io = false;
+
+  /* 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 = nullptr;
+
+  /* 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 = null_ptid;
+  ptid_t continue_thread = null_ptid;
+
+  /* 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 = -1;
+
+  char *last_pass_packet = nullptr;
+
+  /* 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 = nullptr;
+
+  gdb_signal last_sent_signal = GDB_SIGNAL_0;
+
+  bool last_sent_step = false;
+
+  /* The execution direction of the last resume we got.  */
+  exec_direction_kind last_resume_exec_dir = EXEC_FORWARD;
+
+  char *finished_object = nullptr;
+  char *finished_annex = nullptr;
+  ULONGEST finished_offset = 0;
+
+  /* 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).  */
+  bool use_threadinfo_query = false;
+  bool use_threadextra_query = false;
+
+  threadref echo_nextthread {};
+  threadref nextthread {};
+  threadref resultthreadlist[MAXTHREADLISTRESULTS] {};
+
+  /* The state of remote notification.  */
+  struct remote_notif_state *notif_state = nullptr;
+
+  /* The branch trace configuration.  */
+  struct btrace_config btrace_config {};
+
+  /* The argument to the last "vFile:setfs:" packet we sent, used
+     to avoid sending repeated unnecessary "vFile:setfs:" packets.
+     Initialized to -1 to indicate that no "vFile:setfs:" packet
+     has yet been sent.  */
+  int fs_pid = -1;
+
+  /* A readahead cache for vFile:pread.  Often, reading a binary
+     involves a sequence of small reads.  E.g., when parsing an ELF
+     file.  A readahead cache helps mostly the case of remote
+     debugging on a connection with higher latency, due to the
+     request/reply nature of the RSP.  We only cache data for a single
+     file descriptor at a time.  */
+  struct readahead_cache readahead_cache;
+
+  /* The list of already fetched and acknowledged stop events.  This
+     queue is used for notification Stop, and other notifications
+     don't need queue for their events, because the notification
+     events of Stop can't be consumed immediately, so that events
+     should be queued first, and be consumed by remote_wait_{ns,as}
+     one per time.  Other notifications can consume their events
+     immediately, so queue is not needed for them.  */
+  QUEUE (stop_reply_p) *stop_reply_queue;
+
+  /* Asynchronous signal handle registered as event loop source for
+     when we have pending events ready to be passed to the core.  */
+  struct async_event_handler *remote_async_inferior_event_token = nullptr;
+
+  /* FIXME: cagney/1999-09-23: Even though getpkt was called with
+     ``forever'' still use the normal timeout mechanism.  This is
+     currently used by the ASYNC code to guarentee that target reads
+     during the initial connect always time-out.  Once getpkt has been
+     modified to return a timeout indication and, in turn
+     remote_wait()/wait_for_inferior() have gained a timeout parameter
+     this can go away.  */
+  int wait_forever_enabled_p = 1;
+
+private:
+  /* Mapping of remote protocol data for each gdbarch.  Usually there
+     is only one entry here, though we may see more with stubs that
+     support multi-process.  */
+  std::unordered_map<struct gdbarch *, remote_arch_state>
+    m_arch_states;
+};
 
 static const target_info remote_target_info = {
   "remote",
@@ -671,7 +937,9 @@  public: /* Remote specific methods.  */
 
 private: /* data fields */
 
-  std::unique_ptr<struct remote_state> m_remote_state;
+  /* The remote state.  Don't reference this directly.  Use the
+     get_remote_state method instead.  */
+  remote_state m_remote_state;
 };
 
 static const target_info extended_remote_target_info = {
@@ -775,282 +1043,15 @@  static struct cmd_list_element *remote_cmdlist;
 static struct cmd_list_element *remote_set_cmdlist;
 static struct cmd_list_element *remote_show_cmdlist;
 
-/* Stub vCont actions support.
-
-   Each field is a boolean flag indicating whether the stub reports
-   support for the corresponding action.  */
-
-struct vCont_action_support
-{
-  /* vCont;t */
-  bool t = false;
-
-  /* vCont;r */
-  bool r = false;
-
-  /* vCont;s */
-  bool s = false;
-
-  /* vCont;S */
-  bool S = false;
-};
-
 /* Controls whether GDB is willing to use range stepping.  */
 
 static int use_range_stepping = 1;
 
-/* About this many threadisds fit in a packet.  */
-
-#define MAXTHREADLISTRESULTS 32
-
 /* The max number of chars in debug output.  The rest of chars are
    omitted.  */
 
 #define REMOTE_DEBUG_MAX_CHAR 512
 
-/* Data for the vFile:pread readahead cache.  */
-
-struct readahead_cache
-{
-  /* Invalidate the readahead cache.  */
-  void invalidate ();
-
-  /* Invalidate the readahead cache if it is holding data for FD.  */
-  void invalidate_fd (int fd);
-
-  /* Serve pread from the readahead cache.  Returns number of bytes
-     read, or 0 if the request can't be served from the cache.  */
-  int pread (int fd, gdb_byte *read_buf, size_t len, ULONGEST offset);
-
-  /* The file descriptor for the file that is being cached.  -1 if the
-     cache is invalid.  */
-  int fd = -1;
-
-  /* The offset into the file that the cache buffer corresponds
-     to.  */
-  ULONGEST offset = 0;
-
-  /* The buffer holding the cache contents.  */
-  gdb_byte *buf = nullptr;
-  /* The buffer's size.  We try to read as much as fits into a packet
-     at a time.  */
-  size_t bufsize = 0;
-
-  /* Cache hit and miss counters.  */
-  ULONGEST hit_count = 0;
-  ULONGEST miss_count = 0;
-};
-
-/* Description of the remote protocol for a given architecture.  */
-
-struct packet_reg
-{
-  long offset; /* Offset into G packet.  */
-  long regnum; /* GDB's internal register number.  */
-  LONGEST pnum; /* Remote protocol register number.  */
-  int in_g_packet; /* Always part of G packet.  */
-  /* long size in bytes;  == register_size (target_gdbarch (), regnum);
-     at present.  */
-  /* char *name; == gdbarch_register_name (target_gdbarch (), regnum);
-     at present.  */
-};
-
-struct remote_arch_state
-{
-  explicit remote_arch_state (struct gdbarch *gdbarch);
-
-  /* Description of the remote protocol registers.  */
-  long sizeof_g_packet;
-
-  /* Description of the remote protocol registers indexed by REGNUM
-     (making an array gdbarch_num_regs in size).  */
-  std::unique_ptr<packet_reg[]> regs;
-
-  /* This is the size (in chars) of the first response to the ``g''
-     packet.  It is used as a heuristic when determining the maximum
-     size of memory-read and memory-write packets.  A target will
-     typically only reserve a buffer large enough to hold the ``g''
-     packet.  The size does not include packet overhead (headers and
-     trailers).  */
-  long actual_register_packet_size;
-
-  /* This is the maximum size (in chars) of a non read/write packet.
-     It is also used as a cap on the size of read/write packets.  */
-  long remote_packet_size;
-};
-
-/* Description of the remote protocol state for the currently
-   connected target.  This is per-target state, and independent of the
-   selected architecture.  */
-
-class remote_state
-{
-public:
-
-  remote_state ();
-  ~remote_state ();
-
-  /* Get the remote arch state for GDBARCH.  */
-  struct remote_arch_state *get_remote_arch_state (struct gdbarch *gdbarch);
-
-public: /* data */
-
-  /* A buffer to use for incoming packets, and its current size.  The
-     buffer is grown dynamically for larger incoming packets.
-     Outgoing packets may also be constructed in this buffer.
-     BUF_SIZE is always at least REMOTE_PACKET_SIZE;
-     REMOTE_PACKET_SIZE should be used to limit the length of outgoing
-     packets.  */
-  char *buf;
-  long buf_size;
-
-  /* True if we're going through initial connection setup (finding out
-     about the remote side's threads, relocating symbols, etc.).  */
-  bool starting_up = false;
-
-  /* If we negotiated packet size explicitly (and thus can bypass
-     heuristics for the largest packet size that will not overflow
-     a buffer in the stub), this will be set to that packet size.
-     Otherwise zero, meaning to use the guessed size.  */
-  long explicit_packet_size = 0;
-
-  /* remote_wait is normally called when the target is running and
-     waits for a stop reply packet.  But sometimes we need to call it
-     when the target is already stopped.  We can send a "?" packet
-     and have remote_wait read the response.  Or, if we already have
-     the response, we can stash it in BUF and tell remote_wait to
-     skip calling getpkt.  This flag is set when BUF contains a
-     stop reply packet and the target is not waiting.  */
-  int cached_wait_status = 0;
-
-  /* True, if in no ack mode.  That is, neither GDB nor the stub will
-     expect acks from each other.  The connection is assumed to be
-     reliable.  */
-  bool noack_mode = false;
-
-  /* True if we're connected in extended remote mode.  */
-  bool extended = false;
-
-  /* True if we resumed the target and we're waiting for the target to
-     stop.  In the mean time, we can't start another command/query.
-     The remote server wouldn't be ready to process it, so we'd
-     timeout waiting for a reply that would never come and eventually
-     we'd close the connection.  This can happen in asynchronous mode
-     because we allow GDB commands while the target is running.  */
-  bool waiting_for_stop_reply = false;
-
-  /* The status of the stub support for the various vCont actions.  */
-  vCont_action_support supports_vCont;
-
-  /* True if the user has pressed Ctrl-C, but the target hasn't
-     responded to that.  */
-  bool ctrlc_pending_p = false;
-
-  /* True if we saw a Ctrl-C while reading or writing from/to the
-     remote descriptor.  At that point it is not safe to send a remote
-     interrupt packet, so we instead remember we saw the Ctrl-C and
-     process it once we're done with sending/receiving the current
-     packet, which should be shortly.  If however that takes too long,
-     and the user presses Ctrl-C again, we offer to disconnect.  */
-  bool got_ctrlc_during_io = false;
-
-  /* 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 = nullptr;
-
-  /* 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 = null_ptid;
-  ptid_t continue_thread = null_ptid;
-
-  /* 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 = -1;
-
-  char *last_pass_packet = nullptr;
-
-  /* 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 = nullptr;
-
-  gdb_signal last_sent_signal = GDB_SIGNAL_0;
-
-  bool last_sent_step = false;
-
-  /* The execution direction of the last resume we got.  */
-  exec_direction_kind last_resume_exec_dir = EXEC_FORWARD;
-
-  char *finished_object = nullptr;
-  char *finished_annex = nullptr;
-  ULONGEST finished_offset = 0;
-
-  /* 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).  */
-  bool use_threadinfo_query = false;
-  bool use_threadextra_query = false;
-
-  threadref echo_nextthread {};
-  threadref nextthread {};
-  threadref resultthreadlist[MAXTHREADLISTRESULTS] {};
-
-  /* The state of remote notification.  */
-  struct remote_notif_state *notif_state = nullptr;
-
-  /* The branch trace configuration.  */
-  struct btrace_config btrace_config {};
-
-  /* The argument to the last "vFile:setfs:" packet we sent, used
-     to avoid sending repeated unnecessary "vFile:setfs:" packets.
-     Initialized to -1 to indicate that no "vFile:setfs:" packet
-     has yet been sent.  */
-  int fs_pid = -1;
-
-  /* A readahead cache for vFile:pread.  Often, reading a binary
-     involves a sequence of small reads.  E.g., when parsing an ELF
-     file.  A readahead cache helps mostly the case of remote
-     debugging on a connection with higher latency, due to the
-     request/reply nature of the RSP.  We only cache data for a single
-     file descriptor at a time.  */
-  struct readahead_cache readahead_cache;
-
-  /* The list of already fetched and acknowledged stop events.  This
-     queue is used for notification Stop, and other notifications
-     don't need queue for their events, because the notification
-     events of Stop can't be consumed immediately, so that events
-     should be queued first, and be consumed by remote_wait_{ns,as}
-     one per time.  Other notifications can consume their events
-     immediately, so queue is not needed for them.  */
-  QUEUE (stop_reply_p) *stop_reply_queue;
-
-  /* Asynchronous signal handle registered as event loop source for
-     when we have pending events ready to be passed to the core.  */
-  struct async_event_handler *remote_async_inferior_event_token = nullptr;
-
-  /* FIXME: cagney/1999-09-23: Even though getpkt was called with
-     ``forever'' still use the normal timeout mechanism.  This is
-     currently used by the ASYNC code to guarentee that target reads
-     during the initial connect always time-out.  Once getpkt has been
-     modified to return a timeout indication and, in turn
-     remote_wait()/wait_for_inferior() have gained a timeout parameter
-     this can go away.  */
-  int wait_forever_enabled_p = 1;
-
-private:
-  /* Mapping of remote protocol data for each gdbarch.  Usually there
-     is only one entry here, though we may see more with stubs that
-     support multi-process.  */
-  std::unordered_map<struct gdbarch *, remote_arch_state>
-    m_arch_states;
-};
-
 /* Private data that we'll store in (struct thread_info)->priv.  */
 struct remote_thread_info : public private_thread_info
 {
@@ -1237,17 +1238,14 @@  remote_state::get_remote_arch_state (struct gdbarch *gdbarch)
 remote_state *
 remote_target::get_remote_state ()
 {
-  if (m_remote_state == nullptr)
-    m_remote_state.reset (new remote_state ());
-
   /* Make sure that the remote architecture state has been
      initialized, because doing so might reallocate rs->buf.  Any
      function which calls getpkt also needs to be mindful of changes
      to rs->buf, but this call limits the number of places which run
      into trouble.  */
-  m_remote_state->get_remote_arch_state (target_gdbarch ());
+  m_remote_state.get_remote_arch_state (target_gdbarch ());
 
-  return m_remote_state.get ();
+  return &m_remote_state;
 }
 
 /* Cleanup routine for the remote module's pspace data.  */