[RFA 5/8] New port: TI C6x: gdb port
Pedro Alves
pedro@codesourcery.com
Mon Aug 8 13:04:00 GMT 2011
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
More information about the Gdb-patches
mailing list