Re: [RFC/TileGX 2/2] fix gdbserver bug for 32bit elf

[Pedro Alves <palves at redhat dot com>]

On 02/05/2013 10:42 AM, Jiong Wang wrote:
> for tilegx, all general registers are 64bit, while the pseudo 'pc' is 32bit under tilegx32.

You mention the 'pc' as a pseudo register.  At the
hardware/kernel level, on tilegx32, is 'pc' 32-bit or 64-bit?
I thought Tile-GX was always 64-bit, and tilegx32 was just an ABI.
Is the kernel already trimming the PC?

I'm a bit ambivalent in these cases.

On the one hand, I tend to think it'd be better to send the
whole 64 bits to GDB, and have GDB trim to 32-bits.  There's always
the possibility the bug the user is debugging is caused by the PC
register ending up loaded with >32-bit (say, bad asm, or a bad function
pointer?), and a design that trims at such a low level leaves no
chance of gdb diagnosing it.  If you look at the x32 support, you'll
find we still send a 64-bit RIP down to GDB.

OTOH, I don't care that much about Tile-GX to argue, so I won't
and this is fine with me.  ;-)

> gdb client treat 'pc' correctly in
> 
> static struct type *
> tilegx_register_type (struct gdbarch *gdbarch, int regnum)
> {
>   if (regnum == TILEGX_PC_REGNUM)
>     return builtin_type (gdbarch)->builtin_func_ptr;
> 
> 
>   while gdbserver always treat it as 64bit, there
> is mismatch.
> 
> gdb/ChangeLog:


>         * gdbserver/Makefile.in: Add rule for reg-tilegx32.c
>         * gdbserver/configure.srv: Add rule for reg-tilegx32.o

gdbserver has it's own ChangeLog.  Move these entries there, and
drop the "gdbserver/" prefix.

Missing period at end of sentences.

>         * gdbserver/linux-tile-low.c: Add tile_arch_setup and
>         init_registers_tilegx32, tile_arch_setup will register
>         different register info according to elf class. also,

Double space after '.'.  "also" should be capitalized, but, just
remove it altogether and start with "Use".

>         use "uint_reg_t" to represent register size.
>         * regformats/reg-tilegx.dat: Change abi name to "tilegx.

Missing ".

>         * regformats/reg-tilegx32.dat: New.

Most of these entries are missing the "(context)" bits.
Please see the numerous entries in gdbserver/ChangeLog touching
similar code.

Other than that, it looks good.

Thanks,
-- 
Pedro Alves