This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [RFC] Replace regcache readonly flag with detached flag
- From: Alan Hayward <Alan dot Hayward at arm dot com>
- To: "gdb-patches at sourceware dot org" <gdb-patches at sourceware dot org>
- Cc: nd <nd at arm dot com>
- Date: Wed, 12 Jul 2017 12:32:17 +0000
- Subject: Re: [RFC] Replace regcache readonly flag with detached flag
- Authentication-results: sourceware.org; auth=none
- Authentication-results: sourceware.org; dkim=none (message not signed) header.d=none;sourceware.org; dmarc=none action=none header.from=arm.com;
- Nodisclaimer: True
- References: <B209EACB-8FC0-4702-9C4A-2BD54D393925@arm.com>
- Spamdiagnosticmetadata: NSPM
- Spamdiagnosticoutput: 1:99
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];
>