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

[Simon Marchi]

On 2019-02-08 11:46, John Baldwin wrote:
> 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?)

Good point.  I don't recall exactly, but the idea was probably to have 
the assert in a single place on the code path, to avoid asserting the 
same thing multiple times in a row, if possible.  I don't think that the 
frame version adds a similar assert, I'll look into adding it there.

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

I agree.  I'll look into doing this as a follow-up pass (it would just 
add noise to this patch).

Thanks,

Simon