This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [RFC] Replace regcache readonly flag with detached flag
- From: Simon Marchi <simon dot marchi at polymtl dot ca>
- To: Alan Hayward <Alan dot Hayward at arm dot com>
- Cc: gdb-patches at sourceware dot org, nd <nd at arm dot com>
- Date: Wed, 12 Jul 2017 23:52:34 +0200
- Subject: Re: [RFC] Replace regcache readonly flag with detached flag
- Authentication-results: sourceware.org; auth=none
- References: <B209EACB-8FC0-4702-9C4A-2BD54D393925@arm.com>
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