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

[Alan Hayward <Alan.Hayward@arm.com>]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 6186 bytes --]


> On 14 Jul 2017, at 16:14, Yao Qi <qiyaoltc@gmail.com> wrote:
> 
> 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”.

I like the use of 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.
> 

Replied to patch.

>>> 
>>>> 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.
> 

In your regcache_1 constructor, you only NEW the cooked registers if the
regcache is readonly.
http://people.linaro.org/~yao.qi/gdb/doxy/regcache-split/gdb-xref/classregcache__1.html#acef3ef3bc85269cf04728901b4f28ee8
In my version I only NEW the cooked registers in a detached register cache.

As I understand it, the cooked registers exist because on some architectures
extra state needs saving in the cooked registers (code comment: "some architectures
need to save/restore `cooked registers that live in memory.”).

Therefore the cooked register state needs to be a property of detached and not of
readonly.


A different issue is that we treat save/restore differently.
In your code one of the recaches has to be both read-only (checking via gdb_assert) and detached.
In my code the check is that the regcache is detached or not. Read-only is not relevant.



> -- 
> Yao (齐尧)

\x16º&Öéj×!zÊÞ¶êç׍8ÛÉb²Ö«r\x18\x1dn–­r\x17¬