This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


> On 12 Jul 2017, at 22:52, Simon Marchi <simon.marchi@polymtl.ca> wrote:
> 
> Hi Alan,
> 
> I went through this even though my knowledge of regcaches is very limited, at least I learned from it :)

Thanks for taking a look!

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

I probably should have mentioned my use case for this.

Record-full currently mallocs a buffer of size (MAX_REGISTER_SIZE * num_regs)
and then copies all the regcache values into it.
Later it on it copies individual register values back and too between the buffer
and the regcache.
Using a writable detached regcache instead of the buffer would both simplify
the record-full code and it’d no longer need MAX_REGISTER_SIZE (which I’m
trying to remove).


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

Ok. And it’d be fairly easy to make the detached regcache readonly by default too.

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

A regcache would remain either attached or detached for its lifetime.

Yes, I agree splitting the classes would be more sensible in the long run.
However, to make this safe it’d require the removing of all the older non-class functions -
which is quite a large clean up I’d rather avoid making if possible. A quick grep shows me
3340 occurrences to move. Even if scripted that’s a huge diff to commit.


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

The way you describe cooked_read it is how I meant it to be: create
a new value based on the existing values of raw registers.

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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]