[00/10,v2] Remove regcache::m_readonly_p

Message ID 1517999572-14987-1-git-send-email-yao.qi@linaro.org
Headers show
Series
  • Remove regcache::m_readonly_p
Related show

Message

Yao Qi Feb. 7, 2018, 10:32 a.m.
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.

*** BLURB HERE ***

Yao Qi (10):
  Class reg_buffer
  class readable_regcache and pass readable_regcache to gdbarch
    pseudo_register_read and pseudo_register_read_value
  Remove regcache_save and regcache_cpy
  Class readonly_detached_regcache
  Class detached_regcache
  Replace regcache::dump with class register_dump
  No longer create readonly regcache
  Remove regcache::m_readonly_p
  Move register_dump to regcache-dump.c
  Pass readable_regcache to gdbarch method read_pc

 gdb/Makefile.in      |   1 +
 gdb/aarch64-tdep.c   |   2 +-
 gdb/amd64-tdep.c     |   2 +-
 gdb/arm-tdep.c       |   6 +-
 gdb/avr-tdep.c       |   7 +-
 gdb/bfin-tdep.c      |   2 +-
 gdb/dummy-frame.c    |   6 +-
 gdb/frame.c          |  15 +-
 gdb/frame.h          |   3 +-
 gdb/frv-tdep.c       |   2 +-
 gdb/gdbarch.c        |   6 +-
 gdb/gdbarch.h        |  12 +-
 gdb/gdbarch.sh       |   6 +-
 gdb/h8300-tdep.c     |   4 +-
 gdb/hppa-tdep.c      |   8 +-
 gdb/i386-tdep.c      |   6 +-
 gdb/i386-tdep.h      |   2 +-
 gdb/ia64-tdep.c      |   8 +-
 gdb/infcmd.c         |   6 +-
 gdb/inferior.h       |   2 +-
 gdb/infrun.c         |   8 +-
 gdb/jit.c            |  10 +-
 gdb/linux-fork.c     |  20 ++-
 gdb/m32c-tdep.c      |  20 +--
 gdb/m68hc11-tdep.c   |   2 +-
 gdb/mep-tdep.c       |   6 +-
 gdb/mi/mi-main.c     |  12 +-
 gdb/mips-tdep.c      |   6 +-
 gdb/msp430-tdep.c    |   2 +-
 gdb/nds32-tdep.c     |   2 +-
 gdb/ppc-linux-tdep.c |  10 +-
 gdb/record-full.c    |  21 ++-
 gdb/regcache-dump.c  | 335 ++++++++++++++++++++++++++++++++++++++
 gdb/regcache.c       | 445 +++++++++++++--------------------------------------
 gdb/regcache.h       | 244 ++++++++++++++++------------
 gdb/rl78-tdep.c      |   2 +-
 gdb/rs6000-tdep.c    |  52 ++++--
 gdb/s390-tdep.c      |   2 +-
 gdb/sh-tdep.c        |   4 +-
 gdb/sh64-tdep.c      |   4 +-
 gdb/sparc-tdep.c     |   2 +-
 gdb/sparc64-tdep.c   |   2 +-
 gdb/spu-tdep.c       |  15 +-
 gdb/xtensa-tdep.c    |   4 +-
 44 files changed, 757 insertions(+), 579 deletions(-)
 create mode 100644 gdb/regcache-dump.c

-- 
1.9.1

Comments

Simon Marchi Feb. 17, 2018, 3:53 a.m. | #1
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
Yao Qi Feb. 21, 2018, 11:22 a.m. | #2
Simon Marchi <simon.marchi@polymtl.ca> writes:

> 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.


Hi Simon,
Thanks for the review.  I update these patches as you commented.  I
pushed them in.

-- 
Yao (齐尧)