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: Yao Qi <qiyaoltc at gmail dot com>
- To: Alan Hayward <Alan dot Hayward at arm dot com>
- Cc: "gdb-patches\@sourceware.org" <gdb-patches at sourceware dot org>, nd <nd at arm dot com>
- Date: Thu, 13 Jul 2017 10:03:57 +0100
- Subject: Re: [RFC] Replace regcache readonly flag with detached flag
- Authentication-results: sourceware.org; auth=none
- References: <B209EACB-8FC0-4702-9C4A-2BD54D393925@arm.com>
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.
>
--
Yao (齐尧)