[RFC] Replace regcache readonly flag with detached flag
Commit Message
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
Comments
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];
>
Hi Alan,
I went through this even though my knowledge of regcaches is very
limited, at least I learned from it :)
On 2017-07-05 16: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.
Since for the current use cases those two concepts go hand in hand, I
think that's fine to have a single boolean. If we want to support more
use cases (like the writable detached regcache you are proposing), then
it makes sense to split them somehow.
> 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.
I like the idea of some write protection as a safety net.
> 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.
I like Yao's suggestion, at least in the long run. Is a regcache
instance either only detached or only attached during its lifetime? If
so it would make sense to split them. Could we have more regcache types
to represent all the use cases? Something like
- interface writeable_regcache
- interface readable_regcache
- class readonly_detached_regcache implements readable_regcache
- class writeable_detached_regcache implements readable_regcache,
writeable_regcache
- class attached_regcache extends writeable_detached_regcache
Sure, the current functions don't fit well with a class pattern, but
they would evolve towards that.
> 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.
That makes sense to me in general. I am just not sure about the
cooked_read case. By reading regcache::cooked_read, it seems like if
the regcache is readonly and the cooked value has not been computed yet,
it will be computed. That would make sense, since it does not actually
change the value of the registers (it just creates a new value based on
the existing values of raw registers). The way you describe it, it
sounds like if it's readonly and the value of the cooked register has
not been cached yet, you'll get some unknown value. I think
cooked_rea - If raw register, raw_read. Elif readonly read from
regcache.
Else create pseudo from multiple raw_reads.
should read
cooked_read - If raw register, raw_read. Elif readonly and value
cached read from regcache.
Else create pseudo from multiple raw_reads.
> What do people think of this?
>
>
> Thanks,
> Alan.
Thanks,
Simon
Alan Hayward <Alan.Hayward@arm.com> writes:
> 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 am not sure this replacement is reasonable. The regcache can be
detached from target, and read-only or read-write. The regcache can be
attached to target, and read-write. I can't think of a case that
regcache is attached to target and read-only.
>
> 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.
>
Yes, regcache has these two orthogonal attributes. However, adding a
new m_readonly_p makes regcache even more complicated.
> 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
What is the complexity? I thought my suggestion simplified regcache.
regcache now has ~29 public methods, and there are two groups of apis
which are not related to the other (read/write vs supply/collect). If
we split them, each class has ~15 public methods, it improves the
readability, IMO.
What is more, the class regcache_detached can be propagated and "simplify"
other part of GDB, like use it in target_ops.to_{fetch,store}_regsters,
so that it enforces all target layer implementation only use
supply/collect methods. IMO, using an object having ~15 public methods
is simpler than using an object having ~29 public methods. To be clear,
this is one benefit of splitting regcache, but you don't have to do this.
> to keep the bool flags for safety checks, and it would be very easy
> for the old
We don't need that bool flag m_detached_p in my suggestion.
> "non-class" regcache_ functions (eg regcache_raw_write) to accidentally cast to
> the wrong class.
>
Compiler has the conversion check,
xxx.c:123:12: error: invalid conversion from ‘regcache_1*’ to ‘regcache*’ [-fpermissive]
unless static_cast is used, but that is wrong.
> 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.
>
If regcache is detached, the class doesn't have
{raw,cooked}_{read,write}_ methods at all. It only has collect and
supply methods.
http://people.linaro.org/~yao.qi/gdb/doxy/regcache-split/gdb-xref/classregcache__1.html
the "regcache" is the attached one, inherited from the detached
regcache, with new {raw,cooked}_{read,write}_ methods added.
http://people.linaro.org/~yao.qi/gdb/doxy/regcache-split/gdb-xref/classregcache.html
> 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.
>
> On 12 Jul 2017, at 22:52, Simon Marchi <simon.marchi@polymtl.ca> wrote:
>
> Hi Alan,
>
> I went through this even though my knowledge of regcaches is very limited, at least I learned from it :)
Thanks for taking a look!
>
> On 2017-07-05 16: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.
>
> Since for the current use cases those two concepts go hand in hand, I think that's fine to have a single boolean. If we want to support more use cases (like the writable detached regcache you are proposing), then it makes sense to split them somehow.
I probably should have mentioned my use case for this.
Record-full currently mallocs a buffer of size (MAX_REGISTER_SIZE * num_regs)
and then copies all the regcache values into it.
Later it on it copies individual register values back and too between the buffer
and the regcache.
Using a writable detached regcache instead of the buffer would both simplify
the record-full code and it’d no longer need MAX_REGISTER_SIZE (which I’m
trying to remove).
>
>> 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.
>
> I like the idea of some write protection as a safety net.
Ok. And it’d be fairly easy to make the detached regcache readonly by default too.
>
>> 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.
>
> I like Yao's suggestion, at least in the long run. Is a regcache instance either only detached or only attached during its lifetime? If so it would make sense to split them. Could we have more regcache types to represent all the use cases? Something like
>
> - interface writeable_regcache
> - interface readable_regcache
> - class readonly_detached_regcache implements readable_regcache
> - class writeable_detached_regcache implements readable_regcache, writeable_regcache
> - class attached_regcache extends writeable_detached_regcache
>
> Sure, the current functions don't fit well with a class pattern, but they would evolve towards that.
A regcache would remain either attached or detached for its lifetime.
Yes, I agree splitting the classes would be more sensible in the long run.
However, to make this safe it’d require the removing of all the older non-class functions -
which is quite a large clean up I’d rather avoid making if possible. A quick grep shows me
3340 occurrences to move. Even if scripted that’s a huge diff to commit.
>
>> 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.
>
> That makes sense to me in general. I am just not sure about the cooked_read case. By reading regcache::cooked_read, it seems like if the regcache is readonly and the cooked value has not been computed yet, it will be computed. That would make sense, since it does not actually change the value of the registers (it just creates a new value based on the existing values of raw registers). The way you describe it, it sounds like if it's readonly and the value of the cooked register has not been cached yet, you'll get some unknown value. I think
>
> cooked_rea - If raw register, raw_read. Elif readonly read from regcache.
> Else create pseudo from multiple raw_reads.
>
> should read
>
> cooked_read - If raw register, raw_read. Elif readonly and value cached read from regcache.
> Else create pseudo from multiple raw_reads.
>
The way you describe cooked_read it is how I meant it to be: create
a new value based on the existing values of raw registers.
>> What do people think of this?
>> Thanks,
>> Alan.
>
> Thanks,
>
> Simon
> On 13 Jul 2017, at 10:03, Yao Qi <qiyaoltc@gmail.com> wrote:
>
> Alan Hayward <Alan.Hayward@arm.com> writes:
>
>> 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 am not sure this replacement is reasonable. The regcache can be
> detached from target, and read-only or read-write. The regcache can be
> attached to target, and read-write. I can't think of a case that
> regcache is attached to target and read-only.
Agreed that you wouldn’t need an attached read-only regcache.
>
>>
>> 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.
>>
>
> Yes, regcache has these two orthogonal attributes. However, adding a
> new m_readonly_p makes regcache even more complicated.
>
>> 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
>
> What is the complexity? I thought my suggestion simplified regcache.
> regcache now has ~29 public methods, and there are two groups of apis
> which are not related to the other (read/write vs supply/collect). If
> we split them, each class has ~15 public methods, it improves the
> readability, IMO.
>
> What is more, the class regcache_detached can be propagated and "simplify"
> other part of GDB, like use it in target_ops.to_{fetch,store}_regsters,
> so that it enforces all target layer implementation only use
> supply/collect methods. IMO, using an object having ~15 public methods
> is simpler than using an object having ~29 public methods. To be clear,
> this is one benefit of splitting regcache, but you don't have to do this.
>
>> to keep the bool flags for safety checks, and it would be very easy
>> for the old
>
> We don't need that bool flag m_detached_p in my suggestion.
>
>> "non-class" regcache_ functions (eg regcache_raw_write) to accidentally cast to
>> the wrong class.
>>
>
> Compiler has the conversion check,
>
> xxx.c:123:12: error: invalid conversion from ‘regcache_1*’ to ‘regcache*’ [-fpermissive]
>
> unless static_cast is used, but that is wrong.
What about the other way? Accidentally casting regcache to regcache_1/detacted_regcache.
This would matter if regcache overrides any of the methods in regcache_1/detacted_regcache.
(Which I think is ok in your code.)
(This comment is only valid if the cooked register comment in the next block holds)
I think regcache_cpy might be broken?
The internal check needs to move from m_readonly_p to a detached check, as there needs to
Be different behaviour for:
cpy(regcache, regcache_1) - do a save
cpy(regcache_1, regcache_1) - do a restore
cpy(regcache, regcache) - don’t allow
cpy(regcache_1, regcache_1) - simple memcpy
Which I why I suggested you’d still need a m_detached_p to ensure incorrect casting doesn’t
break the above.
>
>> 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.
>>
>
> If regcache is detached, the class doesn't have
> {raw,cooked}_{read,write}_ methods at all. It only has collect and
> supply methods.
>
> http://people.linaro.org/~yao.qi/gdb/doxy/regcache-split/gdb-xref/classregcache__1.html
>
> the "regcache" is the attached one, inherited from the detached
> regcache, with new {raw,cooked}_{read,write}_ methods added.
>
> http://people.linaro.org/~yao.qi/gdb/doxy/regcache-split/gdb-xref/classregcache.html
>
A difference between mine and your code is the cooked registers
In your code the cooked registers are a product of readonly.
In my code the cooked registers are a product of detached.
The regcache code does become simpler if the cooked registers are a product of readonly.
But, I think they need to be a product of detached.
The code says "some architectures need to save/restore `cooked' registers that live in memory.”
To me, that says it’s required for a regcache that isn’t connected to a target.
>> 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.
>>
>
> --
> Yao (齐尧)
Alan Hayward <Alan.Hayward@arm.com> writes:
>> Compiler has the conversion check,
>>
>> xxx.c:123:12: error: invalid conversion from ‘regcache_1*’ to
>> ‘regcache*’ [-fpermissive]
>>
>> unless static_cast is used, but that is wrong.
>
> What about the other way? Accidentally casting regcache to
> regcache_1/detacted_regcache.
>
It doesn't break anything.
> This would matter if regcache overrides any of the methods in
> regcache_1/detacted_regcache.
> (Which I think is ok in your code.)
Yes, in my code, regcache doesn't override any methods from
detached_regcache. We can even mark methods in detached_regcache "final".
>
> (This comment is only valid if the cooked register comment in the next
> block holds)
> I think regcache_cpy might be broken?
> The internal check needs to move from m_readonly_p to a detached
> check, as there needs to
> Be different behaviour for:
> cpy(regcache, regcache_1) - do a save
> cpy(regcache_1, regcache_1) - do a restore
> cpy(regcache, regcache) - don’t allow
> cpy(regcache_1, regcache_1) - simple memcpy
> Which I why I suggested you’d still need a m_detached_p to ensure
> incorrect casting doesn’t
> break the above.
>
>
regcache_cpy is too complicated, and it doesn't have to be that
complicated. The current use of regcache_cpy is that src is read-only
and dst is not read-only. We can simplify regcache_cpy
https://sourceware.org/ml/gdb-patches/2017-06/msg00715.html (I forgot to
commit it, but good if you can review it). With my patch applied,
regcache_cpy becomes dst->restore (src), and restore is a regcache
method, void regcache::restore (struct regcache *src). It has nothing
to do with detached_regcache.
>>
>>> 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.
>>>
>>
>> If regcache is detached, the class doesn't have
>> {raw,cooked}_{read,write}_ methods at all. It only has collect and
>> supply methods.
>>
>> http://people.linaro.org/~yao.qi/gdb/doxy/regcache-split/gdb-xref/classregcache__1.html
>>
>> the "regcache" is the attached one, inherited from the detached
>> regcache, with new {raw,cooked}_{read,write}_ methods added.
>>
>> http://people.linaro.org/~yao.qi/gdb/doxy/regcache-split/gdb-xref/classregcache.html
>>
>
> A difference between mine and your code is the cooked registers
>
> In your code the cooked registers are a product of readonly.
> In my code the cooked registers are a product of detached.
>
> The regcache code does become simpler if the cooked registers are a
> product of readonly.
>
> But, I think they need to be a product of detached.
> The code says "some architectures need to save/restore `cooked'
> registers that live in memory.”
> To me, that says it’s required for a regcache that isn’t connected to a target.
I am not sure I understand you here. Are you saying that dealing with
cooked registers during save and store? In my code, save and restore
are done against detached regcache. See the doxygen link for
regcache_1, "save" is a public method in regcache_1, it is
void regcache_1::save (regcache_cooked_read_ftype *cooked_read, void *src);
and restore is a private method in regcache,
void regcache::restore (struct regcache_1 *src);
Note that in the doxygen html, src's type is regcache rather than
regcache_1, but it can be changed easily.
Overall, the meaning of save/restore is that, we save the contents (from
frame, for example) to a detached regcache, and restore attached
regcache from a detached one. We use detached regcache because we don't
want to its contents go to target, and we still keep the freedom to mark
the detached regcache read-write or read-only. In the existing uses of
regcache_save, we need a read-only detached regcache, but we need a
read-write detached regcache to replace record_full_core_regbuf.
> On 14 Jul 2017, at 16:14, Yao Qi <qiyaoltc@gmail.com> wrote:
>
> Alan Hayward <Alan.Hayward@arm.com> writes:
>
>>> Compiler has the conversion check,
>>>
>>> xxx.c:123:12: error: invalid conversion from ‘regcache_1*’ to
>>> ‘regcache*’ [-fpermissive]
>>>
>>> unless static_cast is used, but that is wrong.
>>
>> What about the other way? Accidentally casting regcache to
>> regcache_1/detacted_regcache.
>>
>
> It doesn't break anything.
>
>> This would matter if regcache overrides any of the methods in
>> regcache_1/detacted_regcache.
>> (Which I think is ok in your code.)
>
> Yes, in my code, regcache doesn't override any methods from
> detached_regcache. We can even mark methods in detached_regcache "final”.
I like the use of final.
>
>>
>> (This comment is only valid if the cooked register comment in the next
>> block holds)
>> I think regcache_cpy might be broken?
>> The internal check needs to move from m_readonly_p to a detached
>> check, as there needs to
>> Be different behaviour for:
>> cpy(regcache, regcache_1) - do a save
>> cpy(regcache_1, regcache_1) - do a restore
>> cpy(regcache, regcache) - don’t allow
>> cpy(regcache_1, regcache_1) - simple memcpy
>> Which I why I suggested you’d still need a m_detached_p to ensure
>> incorrect casting doesn’t
>> break the above.
>>
>>
>
> regcache_cpy is too complicated, and it doesn't have to be that
> complicated. The current use of regcache_cpy is that src is read-only
> and dst is not read-only. We can simplify regcache_cpy
> https://sourceware.org/ml/gdb-patches/2017-06/msg00715.html (I forgot to
> commit it, but good if you can review it). With my patch applied,
> regcache_cpy becomes dst->restore (src), and restore is a regcache
> method, void regcache::restore (struct regcache *src). It has nothing
> to do with detached_regcache.
>
Replied to patch.
>>>
>>>> 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.
>>>>
>>>
>>> If regcache is detached, the class doesn't have
>>> {raw,cooked}_{read,write}_ methods at all. It only has collect and
>>> supply methods.
>>>
>>> http://people.linaro.org/~yao.qi/gdb/doxy/regcache-split/gdb-xref/classregcache__1.html
>>>
>>> the "regcache" is the attached one, inherited from the detached
>>> regcache, with new {raw,cooked}_{read,write}_ methods added.
>>>
>>> http://people.linaro.org/~yao.qi/gdb/doxy/regcache-split/gdb-xref/classregcache.html
>>>
>>
>> A difference between mine and your code is the cooked registers
>>
>> In your code the cooked registers are a product of readonly.
>> In my code the cooked registers are a product of detached.
>>
>> The regcache code does become simpler if the cooked registers are a
>> product of readonly.
>>
>> But, I think they need to be a product of detached.
>> The code says "some architectures need to save/restore `cooked'
>> registers that live in memory.”
>> To me, that says it’s required for a regcache that isn’t connected to a target.
>
> I am not sure I understand you here. Are you saying that dealing with
> cooked registers during save and store? In my code, save and restore
> are done against detached regcache. See the doxygen link for
> regcache_1, "save" is a public method in regcache_1, it is
>
> void regcache_1::save (regcache_cooked_read_ftype *cooked_read, void *src);
>
> and restore is a private method in regcache,
>
> void regcache::restore (struct regcache_1 *src);
>
> Note that in the doxygen html, src's type is regcache rather than
> regcache_1, but it can be changed easily.
>
> Overall, the meaning of save/restore is that, we save the contents (from
> frame, for example) to a detached regcache, and restore attached
> regcache from a detached one. We use detached regcache because we don't
> want to its contents go to target, and we still keep the freedom to mark
> the detached regcache read-write or read-only. In the existing uses of
> regcache_save, we need a read-only detached regcache, but we need a
> read-write detached regcache to replace record_full_core_regbuf.
>
In your regcache_1 constructor, you only NEW the cooked registers if the
regcache is readonly.
http://people.linaro.org/~yao.qi/gdb/doxy/regcache-split/gdb-xref/classregcache__1.html#acef3ef3bc85269cf04728901b4f28ee8
In my version I only NEW the cooked registers in a detached register cache.
As I understand it, the cooked registers exist because on some architectures
extra state needs saving in the cooked registers (code comment: "some architectures
need to save/restore `cooked registers that live in memory.”).
Therefore the cooked register state needs to be a property of detached and not of
readonly.
A different issue is that we treat save/restore differently.
In your code one of the recaches has to be both read-only (checking via gdb_assert) and detached.
In my code the check is that the regcache is detached or not. Read-only is not relevant.
> --
> Yao (齐尧)
Alan Hayward <Alan.Hayward@arm.com> writes:
Hi Alan,
> In your regcache_1 constructor, you only NEW the cooked registers if the
> regcache is readonly.
> http://people.linaro.org/~yao.qi/gdb/doxy/regcache-split/gdb-xref/classregcache__1.html#acef3ef3bc85269cf04728901b4f28ee8
> In my version I only NEW the cooked registers in a detached register cache.
We can change regcache_1 constructor if it is wrong. I am not saying my
patches are correct, let's commit them. When I reviewed your patch, I
thought it is better to split regcache, then, I spent one day writing
the code to make sure the idea of splitting regcache makes sense. I
post the doxygen link rather than patches, because my code is still
poor, but I think the doxygen is sufficient to show the design.
>
> As I understand it, the cooked registers exist because on some architectures
> extra state needs saving in the cooked registers (code comment: "some
> architectures
> need to save/restore `cooked registers that live in memory.”).
>
> Therefore the cooked register state needs to be a property of detached
> and not of
> readonly.
>
m_registers and m_register_status are fields of detached regcache, we
can definitely save cooked register state in detached regcache.
>
> A different issue is that we treat save/restore differently.
> In your code one of the recaches has to be both read-only (checking
> via gdb_assert) and detached.
> In my code the check is that the regcache is detached or
> not. Read-only is not relevant.
It is read-only in my code, but it doesn't have to be. I don't see any
show-stoppers in the design of splitting regcache. The attributes
"detached" and "read-only" are orthogonal in design. Do you have some
comments on the overall design rather than the code details? I'll
rewrite my patches, and post them. It is unfortunate that it is hard to
review the overall design without the code.
> On 18 Jul 2017, at 10:47, Yao Qi <qiyaoltc@gmail.com> wrote:
>
> Alan Hayward <Alan.Hayward@arm.com> writes:
>
> Hi Alan,
>
>> In your regcache_1 constructor, you only NEW the cooked registers if the
>> regcache is readonly.
>> http://people.linaro.org/~yao.qi/gdb/doxy/regcache-split/gdb-xref/classregcache__1.html#acef3ef3bc85269cf04728901b4f28ee8
>> In my version I only NEW the cooked registers in a detached register cache.
>
> We can change regcache_1 constructor if it is wrong. I am not saying my
> patches are correct, let's commit them. When I reviewed your patch, I
> thought it is better to split regcache, then, I spent one day writing
> the code to make sure the idea of splitting regcache makes sense. I
> post the doxygen link rather than patches, because my code is still
> poor, but I think the doxygen is sufficient to show the design.
Understood.
>
>>
>> As I understand it, the cooked registers exist because on some architectures
>> extra state needs saving in the cooked registers (code comment: "some
>> architectures
>> need to save/restore `cooked registers that live in memory.”).
>>
>> Therefore the cooked register state needs to be a property of detached
>> and not of
>> readonly.
>>
>
> m_registers and m_register_status are fields of detached regcache, we
> can definitely save cooked register state in detached regcache.
>
>>
>> A different issue is that we treat save/restore differently.
>> In your code one of the recaches has to be both read-only (checking
>> via gdb_assert) and detached.
>> In my code the check is that the regcache is detached or
>> not. Read-only is not relevant.
>
> It is read-only in my code, but it doesn't have to be. I don't see any
> show-stoppers in the design of splitting regcache. The attributes
> "detached" and "read-only" are orthogonal in design. Do you have some
> comments on the overall design rather than the code details? I'll
> rewrite my patches, and post them. It is unfortunate that it is hard to
> review the overall design without the code.
I think we’ve covered my main points:
* Use an detached bool instead of splitting the class. (Happy to forget this now)
* Making sure incorrect casting doesn’t happen. (Happy that this is ok)
* cooked state is a property of detached, not readonly.
I’m happy with the rest of the design.
When you post the diff, I’ll apply my record-full patch on top and make
sure it still works.
Alan.
On Thu, 13 Jul 2017, Yao Qi wrote:
> > 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 am not sure this replacement is reasonable. The regcache can be
> detached from target, and read-only or read-write. The regcache can be
> attached to target, and read-write. I can't think of a case that
> regcache is attached to target and read-only.
As a matter of interest: what if the target is `core'?
Maciej
"Maciej W. Rozycki" <macro@imgtec.com> writes:
>> I am not sure this replacement is reasonable. The regcache can be
>> detached from target, and read-only or read-write. The regcache can be
>> attached to target, and read-write. I can't think of a case that
>> regcache is attached to target and read-only.
>
> As a matter of interest: what if the target is `core'?
Hmm, it is read-only and attached to the target :-)
@@ -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;
@@ -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);
@@ -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];