[RFA 5/8] New port: TI C6x: gdb port
Yao Qi
yao@codesourcery.com
Tue Aug 9 02:52:00 GMT 2011
On 08/08/2011 09:03 PM, Pedro Alves wrote:
> On Saturday 06 August 2011 03:26:20, Yao Qi wrote:
>> On 08/04/2011 08:33 PM, Pedro Alves wrote:
>>>> +tic6x_arg_type_alignment (struct type *type)
>>>> +{
>>>> + int len = TYPE_LENGTH (type);
>>>> + enum type_code typecode = TYPE_CODE (type);
>>>
>>> I think a check_typedef is in order.
>>>
>>>
>>
>> This line is changed to:
>>
>> enum type_code typecode = TYPE_CODE (check_typedef (type));
>
> You should call check_typedef earlier, before TYPE_LENGTH, not just
> before TYPE_CODE.
>
OK, call check_typedef before both TYPE_LENGTH and TYPE_CODE.
>> +
>> +/* Macros */
>> +
>> +#define TIC6X_OPCODE_SIZE 4
>> +#define TIC6X_FETCH_PACKET_SIZE 32
>> +
>> +static int arg_regs[] = { 4, 20, 6, 22, 8, 24, 10, 26, 12, 28 };
>
> const? Can you add a comment meantioning what this is?
Added, and moved it out of "macro" section.
> It's not a "macro", so I'd say the "Macros" and "Structures" comments are
> bound for bit rot.
>
>> +/* Structures */
>> +struct register_info
>> +{
>> + int size;
>> + char *name;
>> +};
>
> Same.
>
Remove "struct register_info" since size field is not used anymore.
>> +/* tic6x_register_info_table[i] is the number of bytes of storage in
>> + GDB's register array occupied by register i. */
>> +static struct register_info tic6x_register_info_table[] =
>> +{
>> + /* 0 */ {4, "A0"},
>> + /* 1 */ {4, "A1"},
>> + /* 2 */ {4, "A2"},
>> + /* 3 */ {4, "A3"},
>> + /* 4 */ {4, "A4"},
>> + /* 5 */ {4, "A5"},
>> + /* 6 */ {4, "A6"},
>> + /* 7 */ {4, "A7"},
>> + /* 8 */ {4, "A8"},
>> + /* 9 */ {4, "A9"},
>> + /* 10 */ {4, "A10"},
>> + /* 11 */ {4, "A11"},
>> + /* 12 */ {4, "A12"},
>> + /* 13 */ {4, "A13"},
>> + /* 14 */ {4, "A14"},
>> + /* 15 */ {4, "A15"},
>> + /* 16 */ {4, "B0"},
>> + /* 17 */ {4, "B1"},
>> + /* 18 */ {4, "B2"},
>> + /* 19 */ {4, "B3"},
>> + /* 20 */ {4, "B4"},
>> + /* 21 */ {4, "B5"},
>> + /* 22 */ {4, "B6"},
>> + /* 23 */ {4, "B7"},
>> + /* 24 */ {4, "B8"},
>> + /* 25 */ {4, "B9"},
>> + /* 26 */ {4, "B10"},
>> + /* 27 */ {4, "B11"},
>> + /* 28 */ {4, "B12"},
>> + /* 29 */ {4, "B13"},
>> + /* 30 */ {4, "B14"},
>> + /* 31 */ {4, "B15"},
>> + /* 32 */ {4, "CSR"},
>> + /* 33 */ {4, "PC"},
>> +};
>
> (I do wonder why do you need the register size field, if you're
> always going to put '4' there anyway.)
>
Yeah, agree. Change tic6x_register_info_table to tic6x_register_names,
an array of char*.
>> +
>> +/* This is the implementation of gdbarch method register_name. */
>> +
>> +static const char *
>> +tic6x_register_name (struct gdbarch *gdbarch, int regno)
>> +{
>> + if (regno < 0)
>> + return NULL;
>> +
>> + if (tdesc_has_registers (gdbarch_target_desc (gdbarch)))
>> + return tdesc_register_name (gdbarch, regno);
>> + else if (regno >= ARRAY_SIZE (tic6x_register_info_table))
>
> Spurious double space.
>
Fixed.
>> + return "";
>> + else
>> + return tic6x_register_info_table[regno].name;
>> +}
>> +
>> +/* This is the implementation of gdbarch method register_type. */
>> +
>> +static struct type *
>> +tic6x_register_type (struct gdbarch *gdbarch, int regno)
>> +{
>> +
>> + if (regno == TIC6X_PC_REGNUM)
>
> Spurious double space.
>
Fixed.
>> + return builtin_type (gdbarch)->builtin_func_ptr;
>> + else
>> + return builtin_type (gdbarch)->builtin_uint32;
>> +}
>
>> +/* Our frame ID for a stub frame is the current SP and LR. */
>
> Is this comment right?
>
Hmm, the comment is out of date. Get rid of it.
>> +
>> +static void
>> +tic6x_stub_this_id (struct frame_info *this_frame, void **this_cache,
>> + struct frame_id *this_id)
>> +{
>> + struct tic6x_unwind_cache *cache;
>> +
>> + if (*this_cache == NULL)
>> + *this_cache = tic6x_make_stub_cache (this_frame);
>> + cache = *this_cache;
>> +
>> + *this_id = frame_id_build (cache->cfa, get_frame_pc (this_frame));
>> +}
>> +
>
>> +/* Get the register number by decoding raw bits REG, SIDE, and CORSSPATH in
>> + instruction. */
>
> Typo CORSSPATH.
>
Fixed.
>> +/* This is the implementation of gdbarch method frame_align. */
>> +
>> +static CORE_ADDR
>> +tic6x_frame_align (struct gdbarch *gdbarch, CORE_ADDR addr)
>> +{
>> + return align_down (addr, 8);
>> +}
>> +
>> +/* This is the implementation of gdbarch method register_to_value. */
>
> Spurious double space.
>
Fixed.
>> +
>> +static int
>> +tic6x_register_to_value (struct frame_info *frame, int regnum,
>> + struct type *type, gdb_byte * to,
>> + int *optimizedp, int *unavailablep)
>> +{
>> + get_frame_register (frame, regnum, (char *) to);
>> + *optimizedp = *unavailablep = 0;
>> + return 1;
>> +}
>> +
>> +/* This is the implementation of gdbarch method value_to_register. */
>> +
>> +static void
>> +tic6x_value_to_register (struct frame_info *frame, int regnum,
>> + struct type *type, const gdb_byte *from)
>> +{
>> + put_frame_register (frame, regnum, (const char *) from);
>> +}
>
> Please remove the casts as well.
>
Removed.
>> + /* Hook in ABI-specific overrides, if they have been registered. */
>> + gdbarch_init_osabi (info, gdbarch);
>> +
>> + if (tdesc_data)
>> + {
>> + tdesc_use_registers (gdbarch, tdesc, tdesc_data);
>> + }
>
> Unnecessary braces.
>
>
Removed.
>> +
>> + return gdbarch;
>> +}
>> +
>> +void
>> +_initialize_tic6x_tdep (void)
>> +{
>> + register_gdbarch_init (bfd_arch_tic6x, tic6x_gdbarch_init);
>> +}
>
> No target descriptions initialized for tic6x-*-* ?
>
Yes, we have target descriptions for tic6x-uclinux, but don't have for
tic6x-elf so far. So the target description is initialized in
tic6x-linux-tdep.c:_initialize_tic6x_linux_tdep.
I think the target description initialization should be moved to
tic6x-tdep.c:_initialize_tic6x_tdep, so tic6x-uclinux and tic6x-elf
share the target descriptions. In my new patch, target description
initialization is moved to tic6x-tdep.c:_initialize_tic6x_tdep, and
suffix "-linux" in feature names and files is removed (target
description is revised accordingly).
--
Yao (é½å°§)
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0006-gdb-tic6x-port.patch
Type: text/x-patch
Size: 52845 bytes
Desc: not available
URL: <http://sourceware.org/pipermail/gdb-patches/attachments/20110809/f97e4f0a/attachment.bin>
More information about the Gdb-patches
mailing list