On 08/04/2011 08:33 PM, Pedro Alves wrote: > On Wednesday 20 July 2011 03:08:38, Yao Qi wrote: >> This patch adds the tdep stuff for tic6x in gdb. > > Thanks. > > On Wednesday 20 July 2011 03:08:38, Yao Qi wrote: >> +static const char breakpoint_bnop_be[] = {0x00, 0x00, 0xa1, 0x22}; >> +static const char breakpoint_bnop_le[] = {0x22, 0xa1, 0x00, 0x00}; > > Please use gdb_byte instead of char for byte arrays. > OK. >> +/* Support unwiding frame in signal trampoline. We don't check sigreturn, >> + since it is not used in kernel. */ > > Typo unwiding. > Fixed. > >> +/* Return the offset of register REGNUM in struct sigconext. Return 0 if no >> + such register in sigcontext. */ > > Typo sigconext. > Fixed. >> + CORE_ADDR base = sp + TIC6X_SP_RT_SIGFRAME >> + + 4 + 4 /* Pointer type *pinfo and *puc in struct rt_sigframe. */ >> + + TIC6X_SIGINFO_SIZE >> + + 4 + 4 /* uc_flags and *uc_link in struct ucontext. */ >> + + TIC6X_STACK_T_SIZE; > > Wrap the rhs on ()'s and realign. > OK. >> +static CORE_ADDR >> +tic6x_linux_syscall_next_pc (struct frame_info *frame) >> +{ >> + ULONGEST syscall_number = get_frame_register_unsigned (frame, B0_REGNUM); >> + CORE_ADDR pc = get_frame_pc (frame); >> + >> + if (syscall_number == 139 /* rt_sigreturn */) >> + { >> + if (get_frame_type (frame) == SIGTRAMP_FRAME) >> + return frame_unwind_caller_pc (frame); >> + } >> + >> + return pc + 4; >> +} > > Is the frame type check really necessary? > It is not really necessary. When syscall nuber is 139, the frame should be SIGTRAMP_FRAME. Remove this frame type check. >> + /* In tic6x linux kernel, breakpoint instructions varies on different archs. >> + When either macro __TMS320C6XPLUS__ or _TMS320C6400_PLUS is defined, > > "In the ... Linux kernel", capital L. Defined where? In the kernel's > sources? Sounds like a volatile implementation detail. Can we spell > out the arch names instead? > > Yes, it is a little confusing to describe the kernel implementation details here. The comment is changed to: "In tic6x Linux kernel, breakpoint instructions varies on different archs. On C64x+ and C67x+, breakpoint instruction is 0x56454314, which is an illegal opcode. On other arch, breakpoint instruction is 0x0000a122 (BNOP .S2 0,5)." >> + breakpoint instruction is 0x56454314, which is an illegal opcode. >> + Otherwise, breakpoint instruction is 0x0000a122 (BNOP .S2 0,5). */ > > On Wednesday 20 July 2011 03:08:38, Yao Qi wrote: >> + Copyright (C) 2010 > > Add 2011. > Added. > >> + CORE_ADDR reg_saved[TIC6X_NUM_REGS]; > > Missing descriptions. Several functions and variables miss them, I'm > not going to point them all out individually. > tic6x_analyze_prologue, tic6x_condition_true, and tic6x_register_number are documented with comments. There are still two struct frame_unwind variables and their functions undocumented. It looks they are self-described, so I don't comment them. >> +/* 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, "None"}, >> + /* 33 */ {4, "PC"}, >> + /* 34 */ {4, "IRP"}, >> + /* 35 */ {4, "IFR"}, >> + /* 36 */ {4, "NPR"}, >> + /* 37 */ {4, "A16"}, >> + /* 38 */ {4, "A17"}, >> + /* 39 */ {4, "A18"}, >> + /* 40 */ {4, "A19"}, >> + /* 41 */ {4, "A20"}, >> + /* 42 */ {4, "A21"}, >> + /* 43 */ {4, "A22"}, >> + /* 44 */ {4, "A23"}, >> + /* 45 */ {4, "A24"}, >> + /* 46 */ {4, "A25"}, >> + /* 47 */ {4, "A26"}, >> + /* 48 */ {4, "A27"}, >> + /* 49 */ {4, "A28"}, >> + /* 50 */ {4, "A29"}, >> + /* 51 */ {4, "A30"}, >> + /* 52 */ {4, "A31"}, >> + /* 53 */ {4, "B16"}, >> + /* 54 */ {4, "B17"}, >> + /* 55 */ {4, "B18"}, >> + /* 56 */ {4, "B19"}, >> + /* 57 */ {4, "B20"}, >> + /* 58 */ {4, "B21"}, >> + /* 59 */ {4, "B22"}, >> + /* 60 */ {4, "B23"}, >> + /* 61 */ {4, "B24"}, >> + /* 62 */ {4, "B25"}, >> + /* 63 */ {4, "B26"}, >> + /* 64 */ {4, "B27"}, >> + /* 65 */ {4, "B28"}, >> + /* 66 */ {4, "B29"}, >> + /* 67 */ {4, "B30"}, >> + /* 68 */ {4, "B31"}, >> + /* 69 */ {4, "AMR"}, >> + /* 70 */ {4, "CSR"}, >> + /* 71 */ {4, "ISR"}, >> + /* 72 */ {4, "ICR"}, >> + /* 73 */ {4, "IER"}, >> + /* 74 */ {4, "ISTP"}, >> + /* 75 */ {4, "IN"}, >> + /* 76 */ {4, "OUT"}, >> + /* 77 */ {4, "ACR"}, >> + /* 78 */ {4, "ADR"}, >> + /* 79 */ {4, "FADCR"}, >> + /* 80 */ {4, "FAUCR"}, >> + /* 81 */ {4, "FMCR"}, >> + /* 82 */ {4, "GFPGFR"}, >> + /* 83 */ {4, "DIER"}, >> + /* 84 */ {4, "REP"}, >> + /* 85 */ {4, "TSCL"}, >> + /* 86 */ {4, "TSCH"}, >> + /* 87 */ {4, "ARP"}, >> + /* 88 */ {4, "ILC"}, >> + /* 89 */ {4, "RILC"}, >> + /* 90 */ {4, "DNUM"}, >> + /* 91 */ {4, "SSR"}, >> + /* 92 */ {4, "GPLYA"}, >> + /* 93 */ {4, "GPLYB"}, >> + /* 94 */ {4, "TSR"}, >> + /* 95 */ {4, "ITSR"}, >> + /* 96 */ {4, "NTSR"}, >> + /* 97 */ {4, "EFR"}, >> + /* 98 */ {4, "IERR"}, >> + /* 99 */ {4, "DMSG"}, >> + /* 100 */ {4, "CMSG"}, >> + /* 101 */ {4, "DT_DMA_ADDR"}, >> + /* 102 */ {4, "DT_DMA_DATA"}, >> + /* 103 */ {4, "DT_DMA_CNTL"}, >> + /* 104 */ {4, "TCU_CNTL"}, >> + /* 105 */ {4, "RTDX_REC_CNTL"}, >> + /* 106 */ {4, "RTDX_XMT_CNTL"}, >> + /* 107 */ {4, "RTDX_CFG"}, >> + /* 108 */ {4, "RTDX_RDATA"}, >> + /* 109 */ {4, "RTDX_WDATA"}, >> + /* 110 */ {4, "RTDX_RADDR"}, >> + /* 111 */ {4, "RTDX_WADDR"}, >> + /* 112 */ {4, "MFREG0"}, >> + /* 113 */ {4, "DBG_STAT"}, >> + /* 114 */ {4, "BRK_EN"}, >> + /* 115 */ {4, "HWBP0_CNT"}, >> + /* 116 */ {4, "HWBP0"}, >> + /* 117 */ {4, "HWBP1"}, >> + /* 118 */ {4, "HWBP2"}, >> + /* 119 */ {4, "HWBP3"}, >> + /* 120 */ {4, "OVERLAY"}, >> + /* 121 */ {4, "PC_PROF"}, >> + /* 122 */ {4, "ATSR"}, >> + /* 123 */ {4, "TRR"}, >> + /* 124 */ {4, "TCRR"}, >> + /* 125 */ {4, "DESR"}, >> + /* 126 */ {4, "DETR"}, >> + /* 127 */ {4, ""} >> +}; >> + > > Seems like there are a lot of non-core registers here. Now that > the gdbserver patch has been updated to send in standard tic6x xml > register description features, all registers the target includes in the > description that are not part of the defined features should not need > to be hardcoded in gdb, since the xml description itself will describe > their sizes and names. See arm-tdep.c:arm_register_name for example. > More comments on this further down. > Yes, I kept these non-core registers here in order to handle the case when gdbserver or other stubs don't have such xml description file. I think it again, seems we don't have to worry that gdbserver or other stubs doesn't have such xml description file. In my new patch, only core-registers are kept here, and other registers are removed. >> +/* Return the GDB type object for the "standard" data type >> + of data in register 'regno'. */ >> +static struct type * >> +tic6x_register_type (struct gdbarch *gdbarch, int regno) > > Empty line between comment and function missing. Several instances of > this. > Fixed it here and elsewhere (tic6x_get_next_pc and tic6x_extract_return_value). >> +static void >> +tic6x_setup_default(struct tic6x_unwind_cache *cache) >> +{ > > Missing space before parens. Several instances of this as well. > Fixed. >> +CORE_ADDR >> +tic6x_analyze_prologue (struct gdbarch *gdbarch, const CORE_ADDR start_pc, >> + const CORE_ADDR current_pc, >> + struct tic6x_unwind_cache *cache, >> + struct frame_info *this_frame) >> +{ > > ... > >> +} > > OOC, did you investigate using the prologue-value.h machinery for this? > No. c6x prologue is quite simple, so I parse the binary opcode directly. >> +CORE_ADDR >> +tic6x_skip_prologue (struct gdbarch *gdbarch, CORE_ADDR start_pc) >> +{ >> + CORE_ADDR limit_pc; >> + CORE_ADDR func_addr; >> + >> + /* See if we can determine the end of the prologue via the symbol table. >> + If so, then return either PC, or the PC after the prologue, whichever is >> + greater. */ >> + if (find_pc_partial_function (start_pc, NULL, &func_addr, NULL)) >> + { >> + CORE_ADDR post_prologue_pc >> + = skip_prologue_using_sal (gdbarch, func_addr); >> + if (post_prologue_pc != 0) >> + return max (start_pc, post_prologue_pc); >> + } >> + > > Didn't you mean to call tic6x_analyze_prologue here too? > >> + /* Can't determine prologue from the symbol table, return. */ >> + return start_pc; >> +} > > > Yes, tic6x_analyze_prologue is needed here. The reason I didn't call tic6x_analyze_prologue here is that the test results has no difference. I thought we can determine prologue from symbol table quite well, so I didn't chain the prologue analyzer. It is no harm, at lease, to chain prologue analyzer here. So, I add a call to tic6x_analyze_prologue here in new patch. >> +tic6x_unwind_pc (struct gdbarch *gdbarch, struct frame_info *next_frame) >> +{ >> + char buf[8]; >> + >> + frame_unwind_register (next_frame, PC_REGNUM, buf); > > gdb_byte instead of char again. > > OK, fixed. > >> + cache = FRAME_OBSTACK_ZALLOC (struct tic6x_unwind_cache); >> + (*this_prologue_cache) = cache; >> + >> + /* Zero all fields. */ >> + cache->base = 0; >> + cache->cfa = 0; >> + cache->pc = 0; > > ZALLOC already does that for you. > > Right, fixed. > >> + /* If we've worked out where a register is stored then load it from there. >> + */ > > Odd placement for */. Write as: > > /* If we've worked out where a register is stored then load it from > there. */ > > Oh, yes. Fixed. >> +static const struct frame_unwind tic6x_frame_unwind = >> + { > > `{' should go on column 0. Here and elsewhere. > > >> +struct frame_unwind tic6x_stub_unwind = { >> + NORMAL_FRAME, > > `{' on next line, on column 0. Here and elsewhere. > Fixed. >> +static int >> +tic6x_condition_true (struct frame_info *frame, unsigned long inst) >> +{ >> + int register_number; >> + int register_value; >> + static int register_numbers[8] = {-1, 16, 17, 18, 1, 2, 0, -1}; > > const and missing spaces: > > static const int register_numbers[8] = { -1, 16, 17, 18, 1, 2, 0, -1 }; > > Fixed. > >> + int r = (reg & 15) | ((crosspath ^ side) << 4); >> + if ((reg & 16) != 0) /* A16 - A31, B16-B31 */ >> + r += 37; > > Mind distracting inconsistencies. Either: > > "A16 - A31, B16 - B31" > > or: > > "A16-A31, B16-B31" > > Fixed. > >> +/* tic6x_software_single_step() is called just before we want to resume the > > Drop the ()'s. Or better just start with "Called just ..." > () is removed. >> +/* We don't convert anything at the moment */ >> +static int > > Missing period and double-space, and an empty line. When will > we start converting things? Tonight? :-) > >> +tic6x_convert_register_p (struct gdbarch *gdbarch, int regnum, >> + struct type *type) >> +{ >> + return 0; >> +} >> + > > But more importantly, do you need to install this method at all? > generic_convert_register_p, the default hook, simply returns 0 too. > > Right, this function can be removed. >> + >> +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 + 0, (char *) to + 0); >> + *optimizedp = *unavailablep = 0; >> + return 1; >> +} >> + >> +static void >> +tic6x_value_to_register (struct frame_info *frame, int regnum, >> + struct type *type, const gdb_byte *from) >> +{ >> + put_frame_register (frame, regnum + 0, (const char *) from + 0); >> +} > > Unnecessary "+ 0"s and casts. Looks like a copy/past from some > implementation that reads a register in parts, like: > > get_frame_register (frame, regnum + 0, (char *) to + 0); > get_frame_register (frame, regnum + 1, (char *) to + 4); > > Remove "+ 0". >> +/* Given a return value in REGCACHE with a type VALTYPE, extract and copy its >> + value into VALBUF. */ >> +static void >> +tic6x_extract_return_value (struct type *valtype, struct regcache *regcache, >> + enum bfd_endian byte_order, gdb_byte *valbuf) >> +{ > > Empty line missing. Double space after period. Several other places > miss the double space. Please review all comments. > > Fixed. >> +/* Determine, for architecture GDBARCH, how a return value of TYPE >> + should be returned. If it is supposed to be returned in registers, >> + and READBUF is non-zero, read the appropriate value from REGCACHE, >> + and copy it into READBUF. If WRITEBUF is non-zero, write the value >> + from WRITEBUF into REGCACHE. */ >> + >> +static enum return_value_convention >> +tic6x_return_value (struct gdbarch *gdbarch, struct type *func_type, >> + struct type *type, struct regcache *regcache, >> + gdb_byte *readbuf, const gdb_byte *writebuf) > > We'd decided a while ago that we shouldn't be copy&pasting such > comments on all hook implementations, because it leads to comment bit > rot when the parent comment changes. Instead write something like > "This is the implementation of gdbarch method foo". Here and > elsewhere. > Fixed. >> +/* Get the alignment requirement of TYPE. */ >> +int > > static? > Oh, yes. Fixed. >> +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)); >> + else >> + gdb_assert (0); /* Something wrong. */ > > Either gdb_assert_not_reached, or internal error. Here and elsewhere > gdb_assert(0) appears. > Fixed. >> + char *msg = xstrprintf ("unexpected length %d of type", len); >> + gdb_assert_not_reached (msg); >> + return 0; > > Well, that `return 0' is not reached. :-) Just use internal_error > instead of leaking MSG. There are other instances of this. > Fixed. >> +void >> +_initialize_tic6x_tdep (void) >> +{ >> + int i, offset = 0; > > Looks like a left over pasto. > Looks like the patch you reviewed is sent on July 20th, while I sent an updated version on July 25th, in which these two local variables are removed. >> + >> + register_gdbarch_init (bfd_arch_tic6x, tic6x_gdbarch_init); >> +} > > > >> + >> +#undef FP_REGNUM >> +#define FP_REGNUM 15 /* Frame Pointer: A15 */ >> +#undef SP_REGNUM > > Hmm? The #undef's raise alarm bells. Please prefix the > constants instead.. TIC6X_FP_REGNUM or some such. > Add prefix "TIC6X_" to these macros. >> +#define SP_REGNUM 31 /* Stack Pointer: B15 */ >> +#define RA_REGNUM 19 /* Return address: B3 */ >> +#define DP_REGNUM 30 /* Data Page Pointer: B14 */ >> +#undef PC_REGNUM >> +#define PC_REGNUM 33 >> +#define Z_REGNUM 127 >> +#define TIC6X_NUM_REGS 128 > > Not sure this numbering is right. Now that gdbserver is > sending standard features in the target description, you should make > gdb validate the target description (usually in gdbarch_init callback, > see arm-tdep.c for example) and assign register numbers with > tdesc_numbered_register and friends. That `128' looked suspiciously > high, as if including registers not present in the standard > tic6x xml features. > This bits are in the patch I sent on July 20th, but they are changed in a new version sent on July 25th. In the latest version (sent on July 25th), it did what you suggested here (assign register numbers with tdesc_numbered_register). In my new patch, TIC6X_NUM_REGS is set to 69, which is equivalent to the max number of register that gdbserver can access via ptrace. TIC6X_NUM_REGS is used for set_gdbarch_num_regs. >> +#define A4_REGNUM 4 >> +#define A5_REGNUM 5 >> +#define B0_REGNUM 16 >> +#define B4_REGNUM 20 >> +#define B5_REGNUM 21 >> +#define A16_REGNUM 37 >> + >> +#define INST_SWE 0x10000000 >> + >> +static const char breakpoint_illegal_opcode_be[] = {0x56, 0x45, 0x43, 0x14}; >> +static const char breakpoint_illegal_opcode_le[] = {0x14, 0x43, 0x45, 0x56}; > > Might as well make these extern and defined only in tic6x-tdep.c. A tic6x_ > prefix would be good too. > These macros are changed to enum, and prefixed with "TIC6X_". Moved breakpoint definition to tic6x-tdep.c. -- Yao (齐尧)