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

[Simon Marchi <simon.marchi@polymtl.ca>]

Hi Alan,

I went through this even though my knowledge of regcaches is very 
limited, at least I learned from it :)

On 2017-07-05 16:54, Alan Hayward wrote:
> I've been looking at the readonly flag in the regcache, and have become
> convinced it's overloaded.
> 
> Currently readonly protects against two different things:
> 1 - Stop reads/writes from going through to the target.
> 2 - Stop writes to the regcache.
> 
> These are two conceptually different things, yet are both represented
> by the one bool flag.

Since for the current use cases those two concepts go hand in hand, I 
think that's fine to have a single boolean.  If we want to support more 
use cases (like the writable detached regcache you are proposing), then 
it makes sense to split them somehow.

> After looking through the code, I've come to the conclusion that 
> condition 1
> is very important, whereas condition 2 isn't so much.
> 
> A section of code will backup the current regache using regcache_dup 
> (or the
> copy constructor or new regcache/regcache_xmalloc then a 
> regcache_save).
> When it has finished with the copy, it'll restore the copy to the 
> regcache.
> Whilst the copy exists, we absolutely don't want it to be able to read 
> or
> write to the target. However, in certain circumstances, it might make 
> sense
> to update the copy will new register values.
> 
> 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'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.

I like the idea of some write protection as a safety net.

> 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
> to keep the bool flags for safety checks, and it would be very easy for 
> the old
> "non-class" regcache_ functions (eg regcache_raw_write) to accidentally 
> cast to
> the wrong class.

I like Yao's suggestion, at least in the long run.  Is a regcache 
instance either only detached or only attached during its lifetime?  If 
so it would make sense to split them.  Could we have more regcache types 
to represent all the use cases?  Something like

- interface writeable_regcache
- interface readable_regcache
- class readonly_detached_regcache implements readable_regcache
- class writeable_detached_regcache implements readable_regcache, 
writeable_regcache
- class attached_regcache extends writeable_detached_regcache

Sure, the current functions don't fit well with a class pattern, but 
they would evolve towards that.

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

That makes sense to me in general.  I am just not sure about the 
cooked_read case.  By reading regcache::cooked_read, it seems like if 
the regcache is readonly and the cooked value has not been computed yet, 
it will be computed.  That would make sense, since it does not actually 
change the value of the registers (it just creates a new value based on 
the existing values of raw registers).  The way you describe it, it 
sounds like if it's readonly and the value of the cooked register has 
not been cached yet, you'll get some unknown value.  I think

   cooked_rea   - If raw register, raw_read. Elif readonly read from 
regcache.
                  Else create pseudo from multiple raw_reads.

should read

   cooked_read   - If raw register, raw_read. Elif readonly and value 
cached read from regcache.
   	          Else create pseudo from multiple raw_reads.

> What do people think of this?
> 
> 
> Thanks,
> Alan.

Thanks,

Simon