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 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 > > * 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 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]; > &j!z޶׍7׉b֫rnr