diff mbox

[1/7] Regcache: Subclass ptid functionality to target_regcache

Message ID 6AAAA989-5B31-4E54-963C-57F2B7452BD7@arm.com
State New, archived
Headers

Commit Message

Alan Hayward Aug. 17, 2017, 8:47 a.m. UTC 
  This patch creates the target_regcache, subclassed from regcache.

All ptid related functions are moved to target_regcache.

A regcache retains the ptid () method, which always returns -1.
This ensures users can always test if a regcache is attached to a
target, without needing to know if it is a target_regcache.

The get_current_regcache and get_thread_* functions now all return
a target_regcache. It is perfectly safe for the existing code to
treat the result as a regcache.

A target_regcache cannot be read only, because it does not make sense
that a regcache connected to a target would be read only.


Tested on a --enable-targets=all build with board files unix,
native-gdbserver and unittest.exp.


2017-08-16  Alan Hayward  <alan.hayward@arm.com>

	* gdbarch-selftests.c: Use target_regcache.
	* regcache.c (target_regcache::target_regcache): New constructor.
	(get_thread_arch_aspace_regcache): Use target_regcache.
	(get_thread_regcache_for_ptid): Likewise.
	(target_regcache::regcache_thread_ptid_changed): Likewise.
	(registers_changed_ptid): Likewise.
	(current_regcache_test): Likewise.
	(_initialize_regcache): Likewise.
	* regcache.h: New class target_regcache.

Comments

Yao Qi Aug. 23, 2017, 10:33 a.m. UTC | #1 
Alan Hayward <Alan.Hayward@arm.com> writes:

> All ptid related functions are moved to target_regcache.
>

What is the rationale of this change?  The regcache is per-thread, even
it is disconnected from target.

> A regcache retains the ptid () method, which always returns -1.

ptid_t (-1) is minus_one_ptid, which has a special meaning.

> This ensures users can always test if a regcache is attached to a
> target, without needing to know if it is a target_regcache.

When do we need such test ("if a regcache is attached to a target")?
Alan Hayward Aug. 23, 2017, 1:24 p.m. UTC | #2 
> On 23 Aug 2017, at 11:33, Yao Qi <qiyaoltc@gmail.com> wrote:

> 

> Alan Hayward <Alan.Hayward@arm.com> writes:

> 

>> All ptid related functions are moved to target_regcache.

>> 

> 

> What is the rationale of this change?  The regcache is per-thread, even

> it is disconnected from target.


In the existing code, when calling regcache_dup / copy constructor,
ptid of the new recache is always set to -1.

In the save / restore functions the ptid is not updated.

The regcache.c functions which read/write the ptid only do that using the
target connected regcaches.

Calling regcache_get_ptid on a readonly regcache will result in an assert
firing.


Therefore, in existing code, a readonly recache will always have a ptid
of -1. In the new code this property now becomes part of a detached
regcache.

However.....

> 

>> A regcache retains the ptid () method, which always returns -1.

> 

> ptid_t (-1) is minus_one_ptid, which has a special meaning.


Agreed. The existing code already treats -1 tpid to mean different things.

I’ve been thinking about this again.
regcache_get_ptid asserts if ptid is -1. Therefore ptid() should also
assert on a detached recache ?
With this change, a detached recache would never have a ptid.
I think that simplifies the code too.

> 

>> This ensures users can always test if a regcache is attached to a

>> target, without needing to know if it is a target_regcache.

> 

> When do we need such test ("if a regcache is attached to a target”)?


We don’t. I’m happy to drop this statement.
If we do need to add a test, there will probably be a better way of doing it.

> 

> -- 

> Yao (齐尧)
diff mbox

Patch

diff --git a/gdb/gdbarch-selftests.c b/gdb/gdbarch-selftests.c
index 096ba975f75edf6e2de81a767f2b645ff2405fd7..a6251d2a9f87ccff310f90eb5c27bc4199844f22 100644
--- a/gdb/gdbarch-selftests.c
+++ b/gdb/gdbarch-selftests.c
@@ -27,11 +27,11 @@  namespace selftests {

/* A read-write regcache which doesn't write the target.  */

-class regcache_test : public regcache
+class regcache_test : public target_regcache
{
public:
  explicit regcache_test (struct gdbarch *gdbarch)
-    : regcache (gdbarch, NULL, false)
+    : target_regcache (gdbarch, NULL)
  {
    set_ptid (inferior_ptid);

diff --git a/gdb/regcache.h b/gdb/regcache.h
index aa64a000acf06b90dc7f2f519b07f8ff7e3903b4..bc3f24cffe1413d521998ab3c8081833a2735754 100644
--- a/gdb/regcache.h
+++ b/gdb/regcache.h
@@ -24,14 +24,15 @@ 
#include <forward_list>

struct regcache;
+class target_regcache;
struct regset;
struct gdbarch;
struct address_space;

-extern struct regcache *get_current_regcache (void);
-extern struct regcache *get_thread_regcache (ptid_t ptid);
-extern struct regcache *get_thread_arch_regcache (ptid_t, struct gdbarch *);
-extern struct regcache *get_thread_arch_aspace_regcache (ptid_t,
+extern target_regcache *get_current_regcache (void);
+extern target_regcache *get_thread_regcache (ptid_t ptid);
+extern target_regcache *get_thread_arch_regcache (ptid_t, struct gdbarch *);
+extern target_regcache *get_thread_arch_aspace_regcache (ptid_t,
							 struct gdbarch *,
							 struct address_space *);

@@ -240,7 +241,8 @@  typedef struct cached_reg
  gdb_byte *data;
} cached_reg_t;

-/* The register cache for storing raw register values.  */
+
+/* A register cache.  This is not connected to a target and ptid is unset.  */

class regcache
{
@@ -344,41 +346,23 @@  public:

  void dump (ui_file *file, enum regcache_dump_what what_to_dump);

-  ptid_t ptid () const
-  {
-    return m_ptid;
-  }
-
-  void set_ptid (const ptid_t ptid)
+  virtual ptid_t ptid () const
  {
-    this->m_ptid = ptid;
+    /* Ptid of a non-target regcache is always -1.  */
+    return (ptid_t) -1;
  }

/* Dump the contents of a register from the register cache to the target
   debug.  */
  void debug_print_register (const char *func, int regno);

-  static void regcache_thread_ptid_changed (ptid_t old_ptid, ptid_t new_ptid);
protected:
  regcache (gdbarch *gdbarch, address_space *aspace_, bool readonly_p_);

-  static std::forward_list<regcache *> current_regcache;
-
-private:
  gdb_byte *register_buffer (int regnum) const;

  void restore (struct regcache *src);

-  enum register_status xfer_part (int regnum, int offset, int len, void *in,
-				  const void *out,
-				  decltype (regcache_raw_read) read,
-				  decltype (regcache_raw_write) write);
-
-  void transfer_regset (const struct regset *regset,
-			struct regcache *out_regcache,
-			int regnum, const void *in_buf,
-			void *out_buf, size_t size) const;
-
  struct regcache_descr *m_descr;

  /* The address space of this register cache (for registers where it
@@ -389,6 +373,7 @@  private:
     full [0 .. gdbarch_num_regs + gdbarch_num_pseudo_regs) while a read/write
     register cache can only hold [0 .. gdbarch_num_regs).  */
  gdb_byte *m_registers;
+
  /* Register cache status.  */
  signed char *m_register_status;
  /* Is this a read-only cache?  A read-only cache is used for saving
@@ -398,21 +383,63 @@  private:
     regcache_cpy().  The actual contents are determined by the
     reggroup_save and reggroup_restore methods.  */
  bool m_readonly_p;
-  /* If this is a read-write cache, which thread's registers is
-     it connected to?  */
-  ptid_t m_ptid;

-  friend struct regcache *
-  get_thread_arch_aspace_regcache (ptid_t ptid, struct gdbarch *gdbarch,
-				   struct address_space *aspace);
+private:

-  friend void
-  registers_changed_ptid (ptid_t ptid);
+  enum register_status xfer_part (int regnum, int offset, int len, void *in,
+				  const void *out,
+				  decltype (regcache_raw_read) read,
+				  decltype (regcache_raw_write) write);
+
+  void transfer_regset (const struct regset *regset,
+			struct regcache *out_regcache,
+			int regnum, const void *in_buf,
+			void *out_buf, size_t size) const;
+
+private:

  friend void
  regcache_cpy (struct regcache *dst, struct regcache *src);
};

+
+/* A register cache that can be attached to a target. ptid can be set.  */
+
+class target_regcache : public regcache
+{
+public:
+
+  ptid_t ptid () const
+  {
+    return m_ptid;
+  }
+
+  static void regcache_thread_ptid_changed (ptid_t old_ptid,
+					    ptid_t new_ptid);
+
+protected:
+
+  /* Constructor is only called via get_thread_arch_aspace_regcache.  */
+  target_regcache (gdbarch *gdbarch, address_space *aspace_);
+
+  void set_ptid (const ptid_t ptid)
+  {
+    this->m_ptid = ptid;
+  }
+
+  static std::forward_list<target_regcache *> current_regcache;
+
+private:
+
+  /* The thread the cache is connected to, or -1 if not attached.  */
+  ptid_t m_ptid;
+
+  friend struct target_regcache * get_thread_arch_aspace_regcache
+    (ptid_t ptid, struct gdbarch *gdbarch, struct address_space *aspace);
+
+  friend void registers_changed_ptid (ptid_t ptid);
+};
+
/* Duplicate the contents of a register cache to a read-only register
   cache.  The operation is pass-through.  */
extern struct regcache *regcache_dup (struct regcache *regcache);
diff --git a/gdb/regcache.c b/gdb/regcache.c
index e8f92d684dd80a197e055aa1b1bbe2caaab2392a..3d6ca86006fa59463069808f6e5b355028c4d3cc 100644
--- a/gdb/regcache.c
+++ b/gdb/regcache.c
@@ -207,7 +207,15 @@  regcache::regcache (gdbarch *gdbarch, address_space *aspace_,
      m_register_status = XCNEWVEC (signed char,
				    m_descr->sizeof_raw_register_status);
    }
+}
+
+target_regcache::target_regcache (gdbarch *gdbarch, address_space *aspace_)
+  : regcache (gdbarch, aspace_, false)
+{
  m_ptid = minus_one_ptid;
+
+  /* A target_regcache should never be readonly.  */
+  gdb_assert (!m_readonly_p);
}

static enum register_status
@@ -440,25 +448,25 @@  regcache::invalidate (int regnum)
   recording if the register values have been changed (eg. by the
   user).  Therefore all registers must be written back to the
   target when appropriate.  */
-std::forward_list<regcache *> regcache::current_regcache;
+std::forward_list<target_regcache *> target_regcache::current_regcache;

-struct regcache *
+target_regcache *
get_thread_arch_aspace_regcache (ptid_t ptid, struct gdbarch *gdbarch,
				 struct address_space *aspace)
{
-  for (const auto &regcache : regcache::current_regcache)
+  for (const auto &regcache : target_regcache::current_regcache)
    if (ptid_equal (regcache->ptid (), ptid) && regcache->arch () == gdbarch)
      return regcache;

-  regcache *new_regcache = new regcache (gdbarch, aspace, false);
+  target_regcache *new_regcache = new target_regcache (gdbarch, aspace);

-  regcache::current_regcache.push_front (new_regcache);
+  target_regcache::current_regcache.push_front (new_regcache);
  new_regcache->set_ptid (ptid);

  return new_regcache;
}

-struct regcache *
+target_regcache *
get_thread_arch_regcache (ptid_t ptid, struct gdbarch *gdbarch)
{
  struct address_space *aspace;
@@ -479,7 +487,7 @@  get_thread_arch_regcache (ptid_t ptid, struct gdbarch *gdbarch)
static ptid_t current_thread_ptid;
static struct gdbarch *current_thread_arch;

-struct regcache *
+target_regcache *
get_thread_regcache (ptid_t ptid)
{
  if (!current_thread_arch || !ptid_equal (current_thread_ptid, ptid))
@@ -491,7 +499,7 @@  get_thread_regcache (ptid_t ptid)
  return get_thread_arch_regcache (ptid, current_thread_arch);
}

-struct regcache *
+target_regcache *
get_current_regcache (void)
{
  return get_thread_regcache (inferior_ptid);
@@ -502,7 +510,7 @@  get_current_regcache (void)
struct regcache *
get_thread_regcache_for_ptid (ptid_t ptid)
{
-  return get_thread_regcache (ptid);
+  return (struct regcache*) get_thread_regcache (ptid);
}

/* Observer for the target_changed event.  */
@@ -516,9 +524,9 @@  regcache_observer_target_changed (struct target_ops *target)
/* Update global variables old ptids to hold NEW_PTID if they were
   holding OLD_PTID.  */
void
-regcache::regcache_thread_ptid_changed (ptid_t old_ptid, ptid_t new_ptid)
+target_regcache::regcache_thread_ptid_changed (ptid_t old_ptid, ptid_t new_ptid)
{
-  for (auto &regcache : regcache::current_regcache)
+  for (auto &regcache : target_regcache::current_regcache)
    {
      if (ptid_equal (regcache->ptid (), old_ptid))
	regcache->set_ptid (new_ptid);
@@ -539,15 +547,15 @@  regcache::regcache_thread_ptid_changed (ptid_t old_ptid, ptid_t new_ptid)
void
registers_changed_ptid (ptid_t ptid)
{
-  for (auto oit = regcache::current_regcache.before_begin (),
+  for (auto oit = target_regcache::current_regcache.before_begin (),
	 it = std::next (oit);
-       it != regcache::current_regcache.end ();
+       it != target_regcache::current_regcache.end ();
       )
    {
      if (ptid_match ((*it)->ptid (), ptid))
	{
	  delete *it;
-	  it = regcache::current_regcache.erase_after (oit);
+	  it = target_regcache::current_regcache.erase_after (oit);
	}
      else
	oit = it++;
@@ -1668,7 +1676,7 @@  maintenance_print_remote_registers (char *args, int from_tty)

namespace selftests {

-class regcache_access : public regcache
+class regcache_access : public target_regcache
{
public:

@@ -1677,8 +1685,8 @@  public:
  static size_t
  current_regcache_size ()
  {
-    return std::distance (regcache::current_regcache.begin (),
-			  regcache::current_regcache.end ());
+    return std::distance (target_regcache::current_regcache.begin (),
+			  target_regcache::current_regcache.end ());
  }
};

@@ -1692,7 +1700,7 @@  current_regcache_test (void)

  /* Get regcache from ptid1, a new regcache is added to
     current_regcache.  */
-  regcache *regcache = get_thread_arch_aspace_regcache (ptid1,
+  target_regcache *regcache = get_thread_arch_aspace_regcache (ptid1,
							target_gdbarch (),
							NULL);

@@ -1745,7 +1753,8 @@  _initialize_regcache (void)
    = gdbarch_data_register_post_init (init_regcache_descr);

  observer_attach_target_changed (regcache_observer_target_changed);
-  observer_attach_thread_ptid_changed (regcache::regcache_thread_ptid_changed);
+  observer_attach_thread_ptid_changed
+    (target_regcache::regcache_thread_ptid_changed);

  add_com ("flushregs", class_maintenance, reg_flush_command,
	   _("Force gdb to flush its register cache (maintainer command)"));