Re: [RFC] Replace regcache readonly flag with detached flag

[Alan Hayward <Alan.Hayward@arm.com>]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 19443 bytes --]

Ping.
Anyone with any strong opinions on this?

Adding the change will allow code to create editable backups of the regcache.
For example, record-full can use a regcache copy instead of creating it’s own
buffer of (resize*numberofregs)

Thanks,
Alan.

> On 5 Jul 2017, at 15:54, Alan Hayward <Alan.Hayward@arm.com> wrote:
> 
> I've been looking at the readonly flag in the regcache, and have become
> convinced it's overloaded.
> 
> Currently readonly protects against two different things:
> 1 - Stop reads/writes from going through to the target.
> 2 - Stop writes to the regcache.
> 
> These are two conceptually different things, yet are both represented
> by the one bool flag.
> 
> After looking through the code, I've come to the conclusion that condition 1
> is very important, whereas condition 2 isn't so much.
> 
> A section of code will backup the current regache using regcache_dup (or the
> copy constructor or new regcache/regcache_xmalloc then a regcache_save).
> When it has finished with the copy, it'll restore the copy to the regcache.
> Whilst the copy exists, we absolutely don't want it to be able to read or
> write to the target. However, in certain circumstances, it might make sense
> to update the copy will new register values.
> 
> Therefore I'd like to propose removing m_readonly_p and replacing it with:
> 
>  /* Is this a detached cache?  A detached cache is not attached to a target.
>     It is used for saving the target's register state (e.g, across an inferior
>     function call or just before forcing a function return). A detached cache
>     can be written to and read from, however the values will not be passed
>     through to a target.
>     Using the copy constructor or regcache_dup on a regcache will always
>     create a detached regcache.  */
>  bool m_detached_p;
> 
> In most cases this is a 1:1 substitution of m_readonly_p for m_detached_p,
> except it for the write functions, where we now allow writing to the
> regcache buffers.
> 
> I've attached a patch below to show this in action.
> 
> If people are still against removing the readonly flag, then there is the
> option of re-introducing readonly as an additional flag which can optionally
> be set when calling the copy constructor or regcache_dup.
> This would have the advantage of extra security of protecting against any
> accidental writes to detached caches we really don't want to change.
> A regcache would then have both a m_detached_p and m_readonly_p.
> 
> In a previous email ("Re: [PATCH] Replace regbuf with regcache in record-full.c"),
> Yao made the suggestion of splitting the regcache into a detached regcache
> and an attached regcache that subclasses the detached regcache. The problem
> with this approach is it adds a whole lot of complexity, we still probably need
> to keep the bool flags for safety checks, and it would be very easy for the old
> "non-class" regcache_ functions (eg regcache_raw_write) to accidentally cast to
> the wrong class.
> 
> For the sake of verbosity, the current regcache read/writes work as follows:
> 
> raw_read	- If !readonly, update from target to regcache. Read from regcache.
> raw_write	- Assert !readonly. Write to regcache. Write to target.
> raw_collect	- Read from regcache.
> raw_supply	- Assert !readonly. Write to regcache.
> cooked_read	- If raw register, raw_read. Elif readonly read from regcache.
> 		  Else create pseudo from multiple raw_reads.
> cooked_write	- Assert !readonly. If raw register, raw_write.
> 		  Else split pseudo using multiple raw_writes.
> 
> After this suggested change:
> 
> raw_read	- If !detached, update from target to regcache. Read from regcache.
> raw_write	- Write to regcache. If !detached, Write to target.
> raw_collect	- Read from regcache.
> raw_supply	- Write to regcache.
> cooked_read	- If raw register, raw_read. Elif detached read from regcache.
> 		  Else create pseudo from multiple raw_reads.
> cooked_write	- If raw register, raw_write.
> 		  Else split pseudo using multiple raw_writes.
> 
> After this suggested change with additional readonly change:
> 
> raw_read	- If !detached, update from target to regcache. Read from regcache.
> raw_write	- Assert !readonly. Write to regcache. If !detached, Write to target.
> raw_collect	- Read from regcache.
> raw_supply	- Assert !readonly. Write to regcache.
> cooked_read	- If raw register, raw_read. Elif detached read from regcache.
> 		  Else create pseudo from multiple raw_reads.
> cooked_write	- Assert !readonly. If raw register, raw_write.
> 		  Else split pseudo using multiple raw_writes.
> 
> What do people think of this?
> 
> 
> Thanks,
> Alan.
> 
> 
> Tested on a --enable-targets=all build with board files unix and
> native-gdbserver.
> 
> 2017-07-05  Alan Hayward  <alan.hayward@arm.com>
> 
> 	* regcache.c (init_regcache_descr): Use detached instead of
> 	readonly.
> 	(regcache::regcache): Likewise.
> 	(regcache::save): Likewise.
> 	(regcache::restore): Likewise.
> 	(regcache_cpy): Likewise.
> 	(regcache::cpy_no_passthrough): Likewise.
> 	(regcache_dup): Likewise.
> 	(regcache::get_register_status): Likewise.
> 	(regcache::invalidate): Likewise.
> 	(regcache::raw_update): Likewise.
> 	(regcache::cooked_read): Likewise.
> 	(regcache::cooked_read_value): Likewise.
> 	(regcache::raw_write): Likewise, and move check.
> 	(regcache::raw_supply): Remove readonly check.
> 	(regcache::raw_supply_integer): Likewise.
> 	(regcache::raw_supply_zeroed): Likewise.
> 	* regcache.h (readonly_t): Replace with detached_t
> 	(m_readonly_p): Replace with m_detached_p 
> 	* infcmd.c (get_return_value): Use detached
> 
> 
> diff --git a/gdb/infcmd.c b/gdb/infcmd.c
> index defa7b0c48cd3a1a90786ba3d883a986247a3b5d..6a7406986dc4548192b8d854e8a75a561490e0c7 100644
> --- a/gdb/infcmd.c
> +++ b/gdb/infcmd.c
> @@ -1602,7 +1602,7 @@ advance_command (char *arg, int from_tty)
> struct value *
> get_return_value (struct value *function, struct type *value_type)
> {
> -  regcache stop_regs (regcache::readonly, *get_current_regcache ());
> +  regcache stop_regs (regcache::detached, *get_current_regcache ());
>   struct gdbarch *gdbarch = stop_regs.arch ();
>   struct value *value;
> 
> diff --git a/gdb/regcache.h b/gdb/regcache.h
> index b416d5e8b82cf9b942c23d50c259639ee8927628..8e7746d6421741a86f06c3589aaff0c3ba1f066d 100644
> --- a/gdb/regcache.h
> +++ b/gdb/regcache.h
> @@ -83,7 +83,7 @@ extern LONGEST regcache_raw_get_signed (struct regcache *regcache,
> 
> /* Set a raw register's value in the regcache's buffer.  Unlike
>    regcache_raw_write, this is not write-through.  The intention is
> -   allowing to change the buffer contents of a read-only regcache
> +   allowing to change the buffer contents of a detached regcache
>    allocated with regcache_xmalloc.  */
> 
> extern void regcache_raw_set_cached_value
> @@ -249,11 +249,11 @@ public:
>     : regcache (gdbarch, aspace_, true)
>   {}
> 
> -  struct readonly_t {};
> -  static constexpr readonly_t readonly {};
> +  struct detached_t {};
> +  static constexpr detached_t detached {};
> 
> -  /* Create a readonly regcache from a non-readonly regcache.  */
> -  regcache (readonly_t, const regcache &src);
> +  /* Create a detached regcache from a regcache attached to a target.  */
> +  regcache (detached_t, const regcache &src);
> 
>   regcache (const regcache &) = delete;
>   void operator= (const regcache &) = delete;
> @@ -360,7 +360,7 @@ public:
> 
>   static void regcache_thread_ptid_changed (ptid_t old_ptid, ptid_t new_ptid);
> protected:
> -  regcache (gdbarch *gdbarch, address_space *aspace_, bool readonly_p_);
> +  regcache (gdbarch *gdbarch, address_space *aspace_, bool detached_p_);
> 
>   static std::forward_list<regcache *> current_regcache;
> 
> @@ -387,20 +387,21 @@ private:
>      makes sense, like PC or SP).  */
>   struct address_space *m_aspace;
> 
> -  /* The register buffers.  A read-only register cache can hold the
> -     full [0 .. gdbarch_num_regs + gdbarch_num_pseudo_regs) while a read/write
> -     register cache can only hold [0 .. gdbarch_num_regs).  */
> +  /* The register buffers.  A detached register cache can hold the full
> +     [0 .. gdbarch_num_regs + gdbarch_num_pseudo_regs] while a non detached
> +     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
> -     the target's register state (e.g, across an inferior function
> -     call or just before forcing a function return).  A read-only
> -     cache can only be updated via the methods regcache_dup() and
> -     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
> +  /* Is this a detached cache?  A detached cache is not attached to a target.
> +     It is used for saving the target's register state (e.g, across an inferior
> +     function call or just before forcing a function return). A detached cache
> +     can be written to and read from, however the values will not be passed
> +     through to a target.
> +     Using the copy constructor or regcache_dup on a regcache will always
> +     create a detached regcache.  */
> +  bool m_detached_p;
> +  /* If this is non detached cache, which thread's registers is
>      it connected to?  */
>   ptid_t m_ptid;
> 
> @@ -415,13 +416,15 @@ private:
>   regcache_cpy (struct regcache *dst, struct regcache *src);
> };
> 
> -/* Copy/duplicate the contents of a register cache.  By default, the
> -   operation is pass-through.  Writes to DST and reads from SRC will
> -   go through to the target.  See also regcache_cpy_no_passthrough.
> -
> -   regcache_cpy can not have overlapping SRC and DST buffers.  */
> +/* Duplicate a register cache, including the contents.  The new regcache will
> +   always be created as a detached regcache.  */
> 
> extern struct regcache *regcache_dup (struct regcache *regcache);
> +
> +/* Copy the contents of a register cache.  If the DEST is not detached then the
> +   contents will be passed through to the target.  If the SRC is not detached
> +   then the contents will be updated from the target.  */
> +
> extern void regcache_cpy (struct regcache *dest, struct regcache *src);
> 
> extern void registers_changed (void);
> diff --git a/gdb/regcache.c b/gdb/regcache.c
> index 7eeb7376b2862c7f6b297d4fdefc2f769881eeed..ad26dd2b75e9d1f3f7ee06ea97f85d12a598e616 100644
> --- a/gdb/regcache.c
> +++ b/gdb/regcache.c
> @@ -138,7 +138,7 @@ init_regcache_descr (struct gdbarch *gdbarch)
> 	offset += descr->sizeof_register[i];
> 	gdb_assert (MAX_REGISTER_SIZE >= descr->sizeof_register[i]);
>       }
> -    /* Set the real size of the readonly register cache buffer.  */
> +    /* Set the real size of the detached register cache buffer.  */
>     descr->sizeof_cooked_registers = offset;
>   }
> 
> @@ -189,13 +189,13 @@ regcache_register_size (const struct regcache *regcache, int n)
> }
> 
> regcache::regcache (gdbarch *gdbarch, address_space *aspace_,
> -		    bool readonly_p_)
> -  : m_aspace (aspace_), m_readonly_p (readonly_p_)
> +		    bool detached_p_)
> +  : m_aspace (aspace_), m_detached_p (detached_p_)
> {
>   gdb_assert (gdbarch != NULL);
>   m_descr = regcache_descr (gdbarch);
> 
> -  if (m_readonly_p)
> +  if (m_detached_p)
>     {
>       m_registers = XCNEWVEC (gdb_byte, m_descr->sizeof_cooked_registers);
>       m_register_status = XCNEWVEC (signed char,
> @@ -218,10 +218,10 @@ do_cooked_read (void *src, int regnum, gdb_byte *buf)
>   return regcache_cooked_read (regcache, regnum, buf);
> }
> 
> -regcache::regcache (readonly_t, const regcache &src)
> +regcache::regcache (detached_t, const regcache &src)
>   : regcache (src.arch (), src.aspace (), true)
> {
> -  gdb_assert (!src.m_readonly_p);
> +  gdb_assert (!src.m_detached_p);
>   save (do_cooked_read, (void *) &src);
> }
> 
> @@ -330,10 +330,10 @@ regcache::save (regcache_cooked_read_ftype *cooked_read,
>   struct gdbarch *gdbarch = m_descr->gdbarch;
>   int regnum;
> 
> -  /* The DST should be `read-only', if it wasn't then the save would
> +  /* The DST should be detached, if it wasn't then the save would
>      end up trying to write the register values back out to the
>      target.  */
> -  gdb_assert (m_readonly_p);
> +  gdb_assert (m_detached_p);
>   /* Clear the dest.  */
>   memset (m_registers, 0, m_descr->sizeof_cooked_registers);
>   memset (m_register_status, 0, m_descr->sizeof_cooked_register_status);
> @@ -364,10 +364,10 @@ regcache::restore (struct regcache *src)
>   struct gdbarch *gdbarch = m_descr->gdbarch;
>   int regnum;
> 
> -  /* The dst had better not be read-only.  If it is, the `restore'
> +  /* The dst had better not be detached.  If it is, the `restore'
>      doesn't make much sense.  */
> -  gdb_assert (!m_readonly_p);
> -  gdb_assert (src->m_readonly_p);
> +  gdb_assert (!m_detached_p);
> +  gdb_assert (src->m_detached_p);
>   /* Copy over any registers, being careful to only restore those that
>      were both saved and need to be restored.  The full [0 .. gdbarch_num_regs
>      + gdbarch_num_pseudo_regs) range is checked since some architectures need
> @@ -388,11 +388,11 @@ regcache_cpy (struct regcache *dst, struct regcache *src)
>   gdb_assert (src != NULL && dst != NULL);
>   gdb_assert (src->m_descr->gdbarch == dst->m_descr->gdbarch);
>   gdb_assert (src != dst);
> -  gdb_assert (src->m_readonly_p || dst->m_readonly_p);
> +  gdb_assert (src->m_detached_p || dst->m_detached_p);
> 
> -  if (!src->m_readonly_p)
> +  if (!src->m_detached_p)
>     regcache_save (dst, do_cooked_read, src);
> -  else if (!dst->m_readonly_p)
> +  else if (!dst->m_detached_p)
>     dst->restore (src);
>   else
>     dst->cpy_no_passthrough (src);
> @@ -412,7 +412,7 @@ regcache::cpy_no_passthrough (struct regcache *src)
>      move of data into a thread's regcache.  Doing this would be silly
>      - it would mean that regcache->register_status would be
>      completely invalid.  */
> -  gdb_assert (m_readonly_p && src->m_readonly_p);
> +  gdb_assert (m_detached_p && src->m_detached_p);
> 
>   memcpy (m_registers, src->m_registers,
> 	  m_descr->sizeof_cooked_registers);
> @@ -423,7 +423,7 @@ regcache::cpy_no_passthrough (struct regcache *src)
> struct regcache *
> regcache_dup (struct regcache *src)
> {
> -  return new regcache (regcache::readonly, *src);
> +  return new regcache (regcache::detached, *src);
> }
> 
> enum register_status
> @@ -437,7 +437,7 @@ enum register_status
> regcache::get_register_status (int regnum) const
> {
>   gdb_assert (regnum >= 0);
> -  if (m_readonly_p)
> +  if (m_detached_p)
>     gdb_assert (regnum < m_descr->nr_cooked_registers);
>   else
>     gdb_assert (regnum < m_descr->nr_raw_registers);
> @@ -456,7 +456,7 @@ void
> regcache::invalidate (int regnum)
> {
>   gdb_assert (regnum >= 0);
> -  gdb_assert (!m_readonly_p);
> +  gdb_assert (!m_detached_p);
>   gdb_assert (regnum < m_descr->nr_raw_registers);
>   m_register_status[regnum] = REG_UNKNOWN;
> }
> @@ -625,7 +625,7 @@ regcache::raw_update (int regnum)
>      only there is still only one target side register cache.  Sigh!
>      On the bright side, at least there is a regcache object.  */
> 
> -  if (!m_readonly_p && get_register_status (regnum) == REG_UNKNOWN)
> +  if (!m_detached_p && get_register_status (regnum) == REG_UNKNOWN)
>     {
>       target_fetch_registers (this, regnum);
> 
> @@ -746,10 +746,10 @@ regcache::cooked_read (int regnum, gdb_byte *buf)
>   gdb_assert (regnum < m_descr->nr_cooked_registers);
>   if (regnum < m_descr->nr_raw_registers)
>     return raw_read (regnum, buf);
> -  else if (m_readonly_p
> +  else if (m_detached_p
> 	   && m_register_status[regnum] != REG_UNKNOWN)
>     {
> -      /* Read-only register cache, perhaps the cooked value was
> +      /* Detached register cache, perhaps the cooked value was
> 	 cached?  */
>       if (m_register_status[regnum] == REG_VALID)
> 	memcpy (buf, register_buffer (regnum),
> @@ -799,7 +799,7 @@ regcache::cooked_read_value (int regnum)
>   gdb_assert (regnum < m_descr->nr_cooked_registers);
> 
>   if (regnum < m_descr->nr_raw_registers
> -      || (m_readonly_p && m_register_status[regnum] != REG_UNKNOWN)
> +      || (m_detached_p && m_register_status[regnum] != REG_UNKNOWN)
>       || !gdbarch_pseudo_register_read_value_p (m_descr->gdbarch))
>     {
>       struct value *result;
> @@ -918,7 +918,6 @@ regcache::raw_write (int regnum, const gdb_byte *buf)
> 
>   gdb_assert (buf != NULL);
>   gdb_assert (regnum >= 0 && regnum < m_descr->nr_raw_registers);
> -  gdb_assert (!m_readonly_p);
> 
>   /* On the sparc, writing %g0 is a no-op, so we don't even want to
>      change the registers array if something writes to this register.  */
> @@ -932,18 +931,23 @@ regcache::raw_write (int regnum, const gdb_byte *buf)
> 		  m_descr->sizeof_register[regnum]) == 0))
>     return;
> 
> -  target_prepare_to_store (this);
> +  if (!m_detached_p)
> +    target_prepare_to_store (this);
> +
>   raw_set_cached_value (regnum, buf);
> 
> -  /* Register a cleanup function for invalidating the register after it is
> -     written, in case of a failure.  */
> -  old_chain = make_cleanup_regcache_invalidate (this, regnum);
> +  if (!m_detached_p)
> +    {
> +      /* Register a cleanup function for invalidating the register after it is
> +	 written, in case of a failure.  */
> +      old_chain = make_cleanup_regcache_invalidate (this, regnum);
> 
> -  target_store_registers (this, regnum);
> +      target_store_registers (this, regnum);
> 
> -  /* The target did not throw an error so we can discard invalidating the
> -     register and restore the cleanup chain to what it was.  */
> -  discard_cleanups (old_chain);
> +      /* The target did not throw an error so we can discard invalidating the
> +	 register and restore the cleanup chain to what it was.  */
> +      discard_cleanups (old_chain);
> +    }
> }
> 
> void
> @@ -1096,7 +1100,6 @@ regcache::raw_supply (int regnum, const void *buf)
>   size_t size;
> 
>   gdb_assert (regnum >= 0 && regnum < m_descr->nr_raw_registers);
> -  gdb_assert (!m_readonly_p);
> 
>   regbuf = register_buffer (regnum);
>   size = m_descr->sizeof_register[regnum];
> @@ -1131,7 +1134,6 @@ regcache::raw_supply_integer (int regnum, const gdb_byte *addr, int addr_len,
>   size_t regsize;
> 
>   gdb_assert (regnum >= 0 && regnum < m_descr->nr_raw_registers);
> -  gdb_assert (!m_readonly_p);
> 
>   regbuf = register_buffer (regnum);
>   regsize = m_descr->sizeof_register[regnum];
> @@ -1152,7 +1154,6 @@ regcache::raw_supply_zeroed (int regnum)
>   size_t size;
> 
>   gdb_assert (regnum >= 0 && regnum < m_descr->nr_raw_registers);
> -  gdb_assert (!m_readonly_p);
> 
>   regbuf = register_buffer (regnum);
>   size = m_descr->sizeof_register[regnum];
> 

\x16º&Öéj×!zÊÞ¶êç׍7׉b²Ö«r\x18\x1dn–­r\x17¬