> On 14 Jul 2017, at 16:14, Yao Qi wrote: > > Alan Hayward 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 (齐尧) &j!z޶׍8b֫rnr