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: Mon, 17 Jul 2017 10:36:16 +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> <298BA45B-4570-4A16-9C21-95F5A068F93C@arm.com> <86o9snoxtc.fsf@gmail.com>
- Spamdiagnosticmetadata: NSPM
- Spamdiagnosticoutput: 1:99
> On 14 Jul 2017, at 16:14, Yao Qi <qiyaoltc@gmail.com> wrote:
>
> Alan Hayward <Alan.Hayward@arm.com> writes:
>
>>> 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.
>>
>
> It doesn't break anything.
>
>> This would matter if regcache overrides any of the methods in
>> regcache_1/detacted_regcache.
>> (Which I think is ok in your code.)
>
> Yes, in my code, regcache doesn't override any methods from
> detached_regcache. We can even mark methods in detached_regcache "final”.
I like the use of final.
>
>>
>> (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.
>>
>>
>
> regcache_cpy is too complicated, and it doesn't have to be that
> complicated. The current use of regcache_cpy is that src is read-only
> and dst is not read-only. We can simplify regcache_cpy
> https://sourceware.org/ml/gdb-patches/2017-06/msg00715.html (I forgot to
> commit it, but good if you can review it). With my patch applied,
> regcache_cpy becomes dst->restore (src), and restore is a regcache
> method, void regcache::restore (struct regcache *src). It has nothing
> to do with detached_regcache.
>
Replied to patch.
>>>
>>>> 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.
>
> I am not sure I understand you here. Are you saying that dealing with
> cooked registers during save and store? In my code, save and restore
> are done against detached regcache. See the doxygen link for
> regcache_1, "save" is a public method in regcache_1, it is
>
> void regcache_1::save (regcache_cooked_read_ftype *cooked_read, void *src);
>
> and restore is a private method in regcache,
>
> void regcache::restore (struct regcache_1 *src);
>
> Note that in the doxygen html, src's type is regcache rather than
> regcache_1, but it can be changed easily.
>
> Overall, the meaning of save/restore is that, we save the contents (from
> frame, for example) to a detached regcache, and restore attached
> regcache from a detached one. We use detached regcache because we don't
> want to its contents go to target, and we still keep the freedom to mark
> the detached regcache read-write or read-only. In the existing uses of
> regcache_save, we need a read-only detached regcache, but we need a
> read-write detached regcache to replace record_full_core_regbuf.
>
In your regcache_1 constructor, you only NEW the cooked registers if the
regcache is readonly.
http://people.linaro.org/~yao.qi/gdb/doxy/regcache-split/gdb-xref/classregcache__1.html#acef3ef3bc85269cf04728901b4f28ee8
In my version I only NEW the cooked registers in a detached register cache.
As I understand it, the cooked registers exist because on some architectures
extra state needs saving in the cooked registers (code comment: "some architectures
need to save/restore `cooked registers that live in memory.”).
Therefore the cooked register state needs to be a property of detached and not of
readonly.
A different issue is that we treat save/restore differently.
In your code one of the recaches has to be both read-only (checking via gdb_assert) and detached.
In my code the check is that the regcache is detached or not. Read-only is not relevant.
> --
> Yao (齐尧)