Re: [RFA 5/8] New port: TI C6x: gdb port

[Pedro Alves <pedro at codesourcery dot com>]

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.

> +/* Support unwiding frame in signal trampoline.  We don't check sigreturn,
> +   since it is not used in kernel.  */

Typo unwiding.


> +/* Return the offset of register REGNUM in struct sigconext.  Return 0 if no
> +   such register in sigcontext.  */

Typo sigconext.

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

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

> +  /* 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?


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

> +/* 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};
> +#define INST_S_BIT(INST) ((INST >> 1) & 1)
> +#define INST_X_BIT(INST) ((INST >> 12) & 1)
> +
> +/* Structures */
> +struct register_info
> +{
> +  int size;
> +  char *name;
> +};
> +

...

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

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

> +static void
> +tic6x_setup_default(struct tic6x_unwind_cache *cache)
> +{

Missing space before parens.  Several instances of this as well.

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

> +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;
> +}



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



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



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


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



> +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 };



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



> +/* tic6x_software_single_step() is called just before we want to resume the

Drop the ()'s.  Or better just start with "Called just ..."

> +   inferior, if we want to single-step it but there is no hardware or kernel
> +   single-step support.  We find the target of the coming instruction
> +   and breakpoint it.  */
> +
> +int
> +tic6x_software_single_step (struct frame_info *frame)



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


> +
> +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);


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


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

> +/* Get the alignment requirement of TYPE.  */
> +int

static?

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


> +      else
> +       gdb_assert (0); /* Something wrong.  */

Either gdb_assert_not_reached, or internal error.  Here and elsewhere
gdb_assert(0) appears.

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



> +void
> +_initialize_tic6x_tdep (void)
> +{
> +  int i, offset = 0;

Looks like a left over pasto.

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

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

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

-- 
Pedro Alves