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 (齐尧)