RE: [RFA 4/5] New port: CR16: gdbserver

[Kaushik Phatak <Kaushik dot Phatak at kpitcummins dot com>]

Hi Pedro,
Thanks for the detailed review.

I will take care of all the formatting and re-base this with GDB mainline when
I resubmit the patch.

To answer some of your points,
> +cr16_supply_ptrace_register (struct regcache *regcache,
> Is the shift visible in GDB or not?  Is this a ptrace quirk,
> or an architecture quirk?  If the latter, why isn't GDB itself, and
> the cr16_set_pc cr16_get_pc routines in gdbserver handling this?

This is an architecture quirk as the PC is 24 bits wide and only top 23 bits
can be read. The last bit is always zero, so user has to left shift to get
the correct value. The 'supply_ptrace_register' seemed like the correct place
to make these manipulations similar to s390 or the ppc ports.
Manipulations in the get_pc and set_pc calls did not seem to get the 
desired effect while reading PC from the target.

> +32:r10and11
> Eh.  What's the rationale for this? Peeking at the GDB patch, I saw no
> pseudo registers support.
This is the way the kernel port defines the PT_REGS structure. This allows for
faster access as register pair move reg_names (movd rp,rp) instruction is 
supported. However, we needed individual registers to be viewed under gdb.

> +expedite:psr
> Why only psr?  That's surprising.
In case I add any other registers, then the offset of that register is 
incorrect with respect to the "reg_names", and the size does not match 
which is returned by "cr16_register_type".
For example, if I add 
expedite:psr,ra
Then 'ra' is register number 8, and has size 32 bits as per reg-cr16.dat, 
however,for number 8, I have r8 in "reg_names" (cr16-tdep.c) which has 
size 16 bits.
When gdb requests for the expedite registers, it gets data and throws error,
"Badly formatted packet" -> I guess this is expected, as it returns 32 bits
for 'ra' as per reg-cr16.dat, but it expects only 16 bits for 'r8'.
I know this is bit of a hack, however to get the gdbserver running on the
kernel and keeping it in sync with the host, this seemed my best bet.

>> +  (*the_target->read_memory) (where, (unsigned char *) &insn,
>> +			      cr16_breakpoint_len);
> Most ports fail to do this, but the above may fail.  That should be
> checked here.
I have probably borrowed this from cris port. Do I need to perform additional
here? Please let me know if there is any other port I can refer to.

Please let me know if the above justifications are ok.

Thanks & Best Regards,
Kaushik