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

Simon Marchi
2018-02-17T03:53:38Z

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