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: Alan Hayward <Alan dot Hayward at arm dot com>
- To: Yao Qi <qiyaoltc at gmail dot com>
- Cc: "gdb-patches at sourceware dot org" <gdb-patches at sourceware dot org>, nd <nd at arm dot com>
- Date: Fri, 14 Jul 2017 09:21:11 +0000
- Subject: Re: [RFC] Replace regcache readonly flag with detached flag
- Authentication-results: sourceware.org; auth=none
- Authentication-results: gmail.com; dkim=none (message not signed) header.d=none;gmail.com; dmarc=none action=none header.from=arm.com;
- Nodisclaimer: True
- References: <B209EACB-8FC0-4702-9C4A-2BD54D393925@arm.com> <8637a0r9mq.fsf@gmail.com>
- Spamdiagnosticmetadata: NSPM
- Spamdiagnosticoutput: 1:99
> On 13 Jul 2017, at 10:03, Yao Qi <qiyaoltc@gmail.com> wrote:
>
> Alan Hayward <Alan.Hayward@arm.com> writes:
>
>> 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 am not sure this replacement is reasonable. The regcache can be
> detached from target, and read-only or read-write. The regcache can be
> attached to target, and read-write. I can't think of a case that
> regcache is attached to target and read-only.
Agreed that you wouldn’t need an attached read-only regcache.
>
>>
>> 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.
>>
>
> Yes, regcache has these two orthogonal attributes. However, adding a
> new m_readonly_p makes regcache even more complicated.
>
>> 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
>
> What is the complexity? I thought my suggestion simplified regcache.
> regcache now has ~29 public methods, and there are two groups of apis
> which are not related to the other (read/write vs supply/collect). If
> we split them, each class has ~15 public methods, it improves the
> readability, IMO.
>
> What is more, the class regcache_detached can be propagated and "simplify"
> other part of GDB, like use it in target_ops.to_{fetch,store}_regsters,
> so that it enforces all target layer implementation only use
> supply/collect methods. IMO, using an object having ~15 public methods
> is simpler than using an object having ~29 public methods. To be clear,
> this is one benefit of splitting regcache, but you don't have to do this.
>
>> to keep the bool flags for safety checks, and it would be very easy
>> for the old
>
> We don't need that bool flag m_detached_p in my suggestion.
>
>> "non-class" regcache_ functions (eg regcache_raw_write) to accidentally cast to
>> the wrong class.
>>
>
> 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.
This would matter if regcache overrides any of the methods in regcache_1/detacted_regcache.
(Which I think is ok in your code.)
(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.
>
>> 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.
>> 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.
>>
>
> --
> Yao (齐尧)