Re: [1/2] C-SKY Port

[Simon Marchi <simark@simark.ca>]

On 2018-10-20 11:04 p.m., Hafiz Abid Qadeer wrote:
> On 20/10/18 19:55, Simon Marchi wrote:
>> On 2018-07-25 6:54 a.m., Hafiz Abid Qadeer wrote:
> 
>> Hi Hafiz,
>>
>> I noticed the "cooked_read" selftest fails with c-sky (ever since the c-sky support was added):
>>
>> (gdb) maintenance selftest cooked_read
>> Running selftest regcache::cooked_read_test.
>> ...
>> Self test failed: arch csky: self-test failed at /home/simark/src/binutils-gdb/gdb/regcache.c:1697
>> Self test failed: arch csky:ck510: self-test failed at /home/simark/src/binutils-gdb/gdb/regcache.c:1697
>> Self test failed: arch csky:ck610: self-test failed at /home/simark/src/binutils-gdb/gdb/regcache.c:1697
>> Self test failed: arch csky:ck801: self-test failed at /home/simark/src/binutils-gdb/gdb/regcache.c:1697
>> Self test failed: arch csky:ck802: self-test failed at /home/simark/src/binutils-gdb/gdb/regcache.c:1697
>> Self test failed: arch csky:ck803: self-test failed at /home/simark/src/binutils-gdb/gdb/regcache.c:1697
>> Self test failed: arch csky:ck807: self-test failed at /home/simark/src/binutils-gdb/gdb/regcache.c:1697
>> Self test failed: arch csky:ck810: self-test failed at /home/simark/src/binutils-gdb/gdb/regcache.c:1697
>> Self test failed: arch csky:any: self-test failed at /home/simark/src/binutils-gdb/gdb/regcache.c:1697
>> ...
>> Self test failed: self-test failed at /home/simark/src/binutils-gdb/gdb/selftest-arch.c:86
>> Ran 1 unit tests, 1 failed
>>
>> Could you take a look?
> Hi Simon,
> Thanks for letting me know. I am traveling at the moment. Will take a
> look at it when I am back in a few days.

Ok, thanks!

A quick investigation shows it's because some raw registers are not in the
save reggroup.  So csky should probably be added to that big if in the test:

	  if (bfd_arch == bfd_arch_frv || bfd_arch == bfd_arch_h8300
	      || bfd_arch == bfd_arch_m32c || bfd_arch == bfd_arch_sh
	      || bfd_arch == bfd_arch_alpha || bfd_arch == bfd_arch_v850
	      || bfd_arch == bfd_arch_msp430 || bfd_arch == bfd_arch_mep
	      || bfd_arch == bfd_arch_mips || bfd_arch == bfd_arch_v850_rh850
	      || bfd_arch == bfd_arch_tic6x || bfd_arch == bfd_arch_mn10300
	      || bfd_arch == bfd_arch_rl78 || bfd_arch == bfd_arch_score
	      || bfd_arch == bfd_arch_riscv)
	    {
	      /* Raw registers.  If raw registers are not in save_reggroup,
		 their status are unknown.  */
	      if (gdbarch_register_reggroup_p (gdbarch, regnum, save_reggroup))
		SELF_CHECK (status == REG_VALID);
	      else
		SELF_CHECK (status == REG_UNKNOWN);
	    }
	  else
	    SELF_CHECK (status == REG_VALID);

But I'm tempted to replace all of that with simply;

	  /* Raw registers.  If raw registers are not in save_reggroup,
	     their status are unknown.  */
	  if (gdbarch_register_reggroup_p (gdbarch, regnum, save_reggroup))
	    SELF_CHECK (status == REG_VALID);
	  else
	    SELF_CHECK (status == REG_UNKNOWN);

We won't have to maintain this big list, and I don't think we lose any testing
coverage/safety.  For architectures for which all raw registers are in the
save_reggroup (all arches not listed above), we will still always assert that
status == REG_VALID.

I'll let you take a look when you have time.

Simon