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

[Yao Qi <qiyaoltc at gmail dot com>]

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.

-- 
Yao (齐尧)