From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 5681 invoked by alias); 8 Aug 2011 13:04:04 -0000 Received: (qmail 5671 invoked by uid 22791); 8 Aug 2011 13:04:03 -0000 X-SWARE-Spam-Status: No, hits=-2.3 required=5.0 tests=AWL,BAYES_00,RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mail.codesourcery.com (HELO mail.codesourcery.com) (38.113.113.100) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Mon, 08 Aug 2011 13:03:40 +0000 Received: (qmail 3926 invoked from network); 8 Aug 2011 13:03:39 -0000 Received: from unknown (HELO scottsdale.localnet) (pedro@127.0.0.2) by mail.codesourcery.com with ESMTPA; 8 Aug 2011 13:03:39 -0000 From: Pedro Alves To: gdb-patches@sourceware.org Subject: Re: [RFA 5/8] New port: TI C6x: gdb port Date: Mon, 08 Aug 2011 13:04:00 -0000 User-Agent: KMail/1.13.6 (Linux/2.6.38-8-generic; KDE/4.6.2; x86_64; ; ) Cc: Yao Qi References: <4E2638A6.1070406@codesourcery.com> <201108041333.42945.pedro@codesourcery.com> <4E3CA64C.1070901@codesourcery.com> In-Reply-To: <4E3CA64C.1070901@codesourcery.com> MIME-Version: 1.0 Content-Type: Text/Plain; charset="iso-8859-15" Content-Transfer-Encoding: 7bit Message-Id: <201108081403.37241.pedro@codesourcery.com> X-IsSubscribed: yes Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2011-08/txt/msg00140.txt.bz2 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