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

[Pedro Alves <palves at redhat dot com>]

On 12/28/2012 04:41 AM, Kaushik Phatak wrote:
> 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.

Okay.

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

But "r10and11" is not defined as a 32-bit (pseudo) register anywhere
I could see.   Wouldn't writing

...
 16:r10
 16:r11
...

work?

Then collect/supply_register_by_name could use "r11" if it wanted,
and it work out the same?

But most importantly, I see in gdbserver:

+#define cr16_num_regs 16
+#define PC_REGNUM 11
+
+static int cr16_regmap[] = {
+ 0,	4,	8,	12,
+ 16,	20,	24,	28,
+ 32,	36,	40,	44,
+ 48,	52,	56,	60
+};

while in GDB you have:

+/* Number of registers available for Linux targets  */
+#define CR16_LINUX_NUM_REGS  21
+
+/* The breakpoint instruction used by uClinux target  */
+static const gdb_byte breakpoint_uclinux[] = { 0xC7, 0x00 };
+
+static const char *const reg_names[] =
+{
+  "r0",
+  "r1",
+  "r2",
+  "r3",
+  "r4",
+  "r5",
+  "r6",
+  "r7",
+  "r8",
+  "r9",
+  "r10",
+  "r11",
+  "r12",
+  "r13",
+  "ra",
+  "psr",
+  "pc",
+  "r0r1_orig",
+  "intbase",
+  "usp",
+  "cfg"
+};
+

So if GDB would happen to request register number 7
with the 'p' packet (GDBserver doesn't support that packet
today, but it serves to show the design problem), GDBserver
would return the wrong register.

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

Exactly, a manifestation of the above issue.

No, this needs to be fixed.  The .dat registers must agree with GDB's remote
register set.  Once that is fixed, you'll be able to put more registers
in the "expedite" set, which you do want -- you'll want to put there
the registers that gdb will need to fetch anyway at each single-step,
in order to reduce round-trip latency.  Usually, the PC, and stack pointer
registers and often the frame pointer too.

-- 
Pedro Alves