Re: [PATCH 00/10 v2] Remove regcache::m_readonly_p

[Simon Marchi <simon.marchi@polymtl.ca>]

On 2018-02-07 05:32, Yao Qi wrote:
> regcache is used in many places in gdb in different ways, so regcache
> becomes a flat and fat object.  That exposes some unnecessary APIs to
> different part, and some APIs are misused or abused:
> 
>  1) gdbarch methods pseudo_register_read, pseudo_register_read_value,
>  read_pc have a parameter 'regcache *', but these two gdbarch methods
>  only need raw_read* and cooked_read* methods.  So it is better to
>  pass a class which only has raw_read* and cooked_read* methods, and
>  other regcache methods are invisible to each gdbarch implementation.
> 
>  2) target_ops methods to_fetch_registers and to_store_registers have a
>  parameter 'regcache *', but these two target_ops methods only need
>  raw_supply and raw_collect methods, because raw registers are from 
> target
>  layer, pseudo registers are "composed" or "created" by gdbarch.
> 
>  3) jit.c uses regcache in an odd way, and record-full.c should use
>  a simple version regcache instead of an array (see patch 11)
> 
> Beside these api issues, one issue in regcache is that there is no
> type or class for readonly regcache.  We use a flag field m_readonly_p
> to indicate the regcache is readonly or not, so some regcache apis have
> assert that this regcache is or is not readonly.  The better way to do
> this is to create a new class for readonly regcache which doesn't have
> write methods at all.
> 
> This patch series fixes all of the problems above except 2) (I had a
> patch to fix 2 in my tree, but still need more time to polish it.) by
> designing a class hierarchy about regcache, like this,
> 
>                       reg_buffer
>                          ^
>                          |
>                    ------+-----
>                    ^
>                    |
>             readable_regcache
>                  ^
>                  |
>            ------+------
>            ^           ^
>            |           |
>     detached_regcache readonly_detached_regcache
>           ^
>           |
>       regcache
> 
> Class reg_buffer is a simple class, having register contents and status
> (in patch 1).  readable_regcache is an abstract class only having 
> raw_read*
> and cooked_read* methods (in patch 2).  detached_regcache is a class 
> which
> has read and write methods, but it detaches from target, IOW, the
> write doesn't go through.  Class readonly_detached_regcache is the 
> readonly
> regcache, created from regcache::save method.
> 
> This is the v2 of this patch series, v1 can be found
> https://sourceware.org/ml/gdb-patches/2017-12/msg00014.html Some
> changes compared with v1,
> 
>  - Some of the preparatory patches in v1 are already committed,
>  - Rename some classes,
>  - Pass readable_regcache to gdbarch read_pc.  We can pass 
> readable_regcache
>    to gdbarch breakpoint_kind_from_current_state as well, because this
>    gdbarch method doesn't need to write regcache.  The reason I don't
>    that is arm_breakpoint_kind_from_current_state uses a function
>    arm_get_next_pcs_ctor shared between GDB and GDBserver, and I don't
>    propagate the regcache changes to GDBserver at this moment,
> 
> This patch series is pushed to users/qiyao/regcache-split-4-2.  
> Regression
> tested on {x86_64,aarch64}-linux.

Hi Yao,

I did not spot anything fundamentally wrong in the series, though I'm 
not very proficient in this area.  I sent a few minor comments, but 
other than that I'm fine with the series.  It's hard to tell whether 
it's the right design, but I think it improves things by splitting the 
concerns in different classes rather than having one do-it-all class.

Thanks,

Simon