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

[Yao Qi <qiyaoltc@gmail.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 (齐尧)