Re: [PATCH v2 1/3] Extract register_reader and register_readwriter interfaces from regcache

[John Baldwin <jhb@FreeBSD.org>]

On 10/23/18 6:43 PM, Simon Marchi wrote:
> From: Simon Marchi <simon.marchi@ericsson.com>
> 
> In the following patch, we make gdbarch pseudo-registers hooks read and
> write required registers from a frame instead of from the current
> regcache.  To avoid having to change the implementation of all
> architectures to use a different interface, we can re-use the regcache
> interface.
> 
> This patch extracts two interfaces, register_reader and
> register_readwriter, and make respectively readable_regcache and
> regcache inherit from them.  register_reader is "something from which
> you can read register values from".  It can of course be a regcache, but
> it will also be (in the following patch) something that unwinds
> registers for a particular frame.  As you would expect,
> register_readwriter is "something you can read and write registers
> from/to".
> 
> Some notes about the implementation.  This results in diamond
> inheritance: regcache inherits from both readable_regcache and
> register_readwriter, which both inherit from register_reader.  It is
> therefore necessary to use virtual inheritance (use "public virtual"),
> otherwises we end up with errors like:
> 
>   /home/emaisin/src/binutils-gdb/gdb/regcache.c:210:20: error: request
>   for member ‘cooked_read’ is ambiguous
> 
> Also, the raw_read method in readable_regcache hides the raw_read
> template method in register_reader.  So it's necessary to use "using
> register_reader::raw_read" so that clients of readable_regcache are able
> to call register_reader's raw_read.  Same thing for some cooked_read,
> raw_write and cooked_write.
> 
> All corresponding gdbarch hooks are updated to use register_reader or
> register_readwriter instead of readable_regcache and regcache, but
> otherwise they are not changed.
> 
> With this patch, no behavior change is expected.
> 
> @@ -539,20 +541,19 @@ regcache_raw_read_signed (struct regcache *regcache, int regnum, LONGEST *val)
>  
>  template<typename T, typename>
>  enum register_status
> -readable_regcache::raw_read (int regnum, T *val)
> +register_reader::raw_read (int regnum, T *val)
>  {
> -  gdb_byte *buf;
> -  enum register_status status;
> +  gdbarch *arch = this->arch ();
> +  int reg_size = register_size (arch, regnum);
> +  gdb_byte buf[reg_size];
> +
> +  register_status status = raw_read (regnum, buf);
>  
> -  assert_regnum (regnum);
> -  buf = (gdb_byte *) alloca (m_descr->sizeof_register[regnum]);
> -  status = raw_read (regnum, buf);

This "loses" the assert_regnum().  Is that replaced for regcache's by the
gdb_assert() you added in readable_regcache::raw_read()?  Did you consider
moving that assertion to here instead so it would also be enforced in the
frame version of register_reader?  (Or does the frame version also add its
own assertion in the following patches?)

Similar question about register_readwriter::raw_write,
register_reader::cooked_read, and register_readerwriter::cooked_write.

The only other general comment is that while I appreciate that this doesn't
require changing the tdep files very much (which is a good thing), it does
have the odd result that the variable names in the gdbarch methods are still
named 'regcache' even though they aren't regcache's anymore.  It's perhaps
not worth it, but it might be nice to do a followup pass eventually to
rename 'register_reader *regcache' to 'reader' and something similar for
the writer case?  It seems tedious though, and I don't think it should be a 
requirement/blocker, just a suggestion for the future perhaps.

-- 
John Baldwin