This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


On Saturday 06 August 2011 03:26:20, Yao Qi wrote:
> On 08/04/2011 08:33 PM, Pedro Alves wrote:
> 
> >> +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.

I won't make you go do that change, but using the prologue-value.h
machinery does not mean you don't parse the opcodes.  It means you
parse the opcodes about the same, but re-use a nice mechanism of tracking
register moves and saves, instead of cooking your own per-arch, which
in turn should be more robust in face of sequences you don't handle
now, easier to debug, and easier to extend.  I was actually thinking
you might have tried and found it lacking in some way.

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

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

Sorry about that.  Completely missed it.

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

Thanks.

> diff --git a/gdb/tic6x-tdep.c b/gdb/tic6x-tdep.c
> new file mode 100644
> index 0000000..7362d69
> --- /dev/null
> +++ b/gdb/tic6x-tdep.c
> @@ -0,0 +1,1414 @@

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

> +
> +struct tic6x_unwind_cache
> +{
> +  /* The frame's base, optionally used by the high-level debug info.  */
> +  CORE_ADDR base;
> +
> +  /* The previous frame's inner most stack address.  Used as this
> +     frame ID's stack_addr.  */
> +  CORE_ADDR cfa;
> +
> +  /* The address of the first instruction in this function */
> +  CORE_ADDR pc;
> +
> +  /* Which register holds the return address for the frame.  */
> +  int return_regnum;
> +
> +  /* The offset of register saved on stack.  If register is not saved, the
> +     corresponding element is -1.  */
> +  CORE_ADDR reg_saved[TIC6X_NUM_CORE_REGS];
> +};
> +
> +
> +/* 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.)

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

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

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

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

> +
> +static int
> +tic6x_register_number (int reg, int side, int crosspath)
> +{
> +  int r = (reg & 15) | ((crosspath ^ side) << 4);
> +  if ((reg & 16) != 0) /* A16 - A31, B16 - B31 */
> +    r += 37;
> +  return r;
> +}

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

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

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


> +
> +  return gdbarch;
> +}
> +
> +void
> +_initialize_tic6x_tdep (void)
> +{
> +  register_gdbarch_init (bfd_arch_tic6x, tic6x_gdbarch_init);
> +}

No target descriptions initialized for tic6x-*-* ?

Otherwise looks okay to me, thanks.

-- 
Pedro Alves


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]