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

[Alan Hayward <Alan dot Hayward at arm dot com>]

> 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 (齐尧)