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