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


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


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