From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 7756 invoked by alias); 17 May 2013 06:29:22 -0000 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 Received: (qmail 7710 invoked by uid 89); 17 May 2013 06:29:22 -0000 X-Spam-SWARE-Status: No, score=-2.0 required=5.0 tests=AWL,BAYES_00,RCVD_IN_HOSTKARMA_NO,TW_EG autolearn=ham version=3.3.1 Received: from rock.gnat.com (HELO rock.gnat.com) (205.232.38.15) by sourceware.org (qpsmtpd/0.84/v0.84-167-ge50287c) with ESMTP; Fri, 17 May 2013 06:29:20 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id 1342C2EABC; Fri, 17 May 2013 02:29:19 -0400 (EDT) Received: from rock.gnat.com ([127.0.0.1]) by localhost (rock.gnat.com [127.0.0.1]) (amavisd-new, port 10024) with LMTP id FKo9gk5jhq2E; Fri, 17 May 2013 02:29:19 -0400 (EDT) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id 711A42EAAE; Fri, 17 May 2013 02:29:18 -0400 (EDT) Received: by joel.gnat.com (Postfix, from userid 1000) id 0B855C25F1; Fri, 17 May 2013 10:29:12 +0400 (RET) Date: Fri, 17 May 2013 06:29:00 -0000 From: Joel Brobecker To: Kevin Buettner Cc: gdb-patches@sourceware.org Subject: Re: [RFC] TI msp430 architecture support Message-ID: <20130517062911.GE4017@adacore.com> References: <20130516200050.17ab3ae7@mesquite.lan> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20130516200050.17ab3ae7@mesquite.lan> User-Agent: Mutt/1.5.21 (2010-09-15) X-SW-Source: 2013-05/txt/msg00665.txt.bz2 Hi Kevin, > This patch adds support for the TI msp430 architecture to GDB. > > Comments? > > gdb/ChangeLog: > > * Makefile.in (ALL_TARGET_OBS): Add msp430-tdep.o. > (ALLDEPFILES): Add msp430-tdep.c. > * configure.tgt (msp430*-*-elf): New target. > * msp430-tdep.c: New file. Overall, this looks pretty good to me. My comments below, which are almost exclusively related to coding style. Feel free to fix and commit... > +enum > +{ > + MSP_ISA_MSP430, > + MSP_ISA_MSP430X > +}; > + > +enum > +{ > + MSP_SMALL_CODE_MODEL, > + MSP_LARGE_CODE_MODEL > +}; Could you add a short description for each of these enum types? > +/* Architecture specific data. */ > + > +struct gdbarch_tdep > +{ > + /* The ELF header flags specify the multilib used. */ > + int elf_flags; > + int isa; > + int code_model; > +}; Also, would you mind adding a small comment for the last two fields of your struct. I suspect, from the name, that they will hold values from your enums above... > +static struct type * > +msp430x_register_type (struct gdbarch *gdbarch, int reg_nr) Can you add the usual trivial description of this function? I know this will repeat the description of msp430_register_type, but JIC the latter disappears, we'll still have this function properly documented... > +/* Implement the "register_name" gdbarch method. */ > + > +static const char * > +msp430_register_name (struct gdbarch *gdbarch, int regnr) > +{ > + static const char *const reg_names[] = > + { > + /* Raw registers. */ > + "", > + "", > + "", > + "", > + "", > + "", > + "", > + "", > + "", > + "", > + "", > + "", > + "", > + "", > + "", > + "", > + /* Pseudo registers. */ > + "pc", > + "sp", > + "sr", > + "cg", > + "r4", > + "r5", > + "r6", > + "r7", > + "r8", > + "r9", > + "r10", > + "r11", > + "r12", > + "r13", > + "r14", > + "r15" > + }; > + > + return reg_names[regnr]; > +} This one surprised me a little. My understanding is that you expect this function to never be called with raw register numbers? If that's the case, how about adding an assert? and only create an array with the pseudo registers? That's a infinitely small detail, but considering the size of the strings, I personally would have joined the register names into 1 or 2 lines. But that's a matter of personal preference, so do feel free to keep it the way it is. > +/* Implement the "pseudo_register_read" gdbarch method. */ > + > +static enum register_status > +msp430_pseudo_register_read (struct gdbarch *gdbarch, > + struct regcache *regcache, > + int regnum, gdb_byte *buffer) > +{ > + enum register_status status = REG_UNKNOWN; > + > + if (MSP430_NUM_REGS <= regnum && regnum < MSP430_NUM_TOTAL_REGS) > + { > + ULONGEST val; > + enum bfd_endian byte_order = gdbarch_byte_order (gdbarch); > + int regsize = register_size (gdbarch, regnum); > + int raw_regnum = regnum - MSP430_NUM_REGS; > + > + status = regcache_raw_read_unsigned (regcache, raw_regnum, &val); > + if (status == REG_VALID) > + store_unsigned_integer (buffer, regsize, byte_order, val); > + > + } > + else > + gdb_assert_not_reached ("invalid pseudo register number"); > + > + return status; > +} Interesting that you chose to use the "else gdb_assert_not_reached" idiom, instead of the usual gdb_assert at the start of the function. I like the fact that you do get a slightly more meaningful message that way. How about adding the register number as well? There are other locations where this can be applied as well, if you like the suggestion. > + stack = make_pv_area (MSP430_SP_REGNUM, gdbarch_addr_bit (gdbarch)); > + back_to = make_cleanup_free_pv_area (stack); > + > + /* The call instruction has saved the return address on the stack. */ > + sz = code_model == MSP_LARGE_CODE_MODEL ? 4 : 2; > + reg[MSP430_SP_REGNUM] = pv_add_constant (reg[MSP430_SP_REGNUM], -sz); > + pv_area_store (stack, reg[MSP430_SP_REGNUM], sz, reg[MSP430_PC_REGNUM]); > + > + pc = start_pc; > + while (pc < limit_pc) > + { > + int bytes_read; > + struct msp430_get_opcode_byte_handle opcode_handle; > + MSP430_Opcode_Decoded opc; > + > + opcode_handle.pc = pc; > + bytes_read = msp430_decode_opcode (pc, &opc, msp430_get_opcode_byte, > + &opcode_handle); The formatting of the second line seems off... > + if (opc.id == MSO_push > + && opc.op[0].type == MSP430_Operand_Register) Same here... > + while (count > 0) > + { > + reg[MSP430_SP_REGNUM] = pv_add_constant (reg[MSP430_SP_REGNUM], -size); Can you split this line in two? (too long) > + else if (opc.id == MSO_mov > + && opc.op[0].type == MSP430_Operand_Immediate > + && 12 <= opc.op[0].reg && opc.op[0].reg <= 15) Same here... > + { > + after_last_frame_setup_insn = next_pc; > + } > + else > + { > + /* Terminate the prologue scan. */ > + break; > + } The GDB coding style asks us to drop the curly braces if the condition block only has one statement. Your last block is the exception to the rule, as we treat the comment as a statement in this case. So: after_last_frame_setup_insn = next_pc; else { /* Terminate the prologue scan. */ break; } If you think it looks odd, just add a comment, Eg: { /* A silly comment so that I can keep the curly braces. */ after_last_frame_setup_insn = next_pc; } else { /* Terminate the prologue scan. */ break; } :) > + /* Record where all the registers were saved. */ > + pv_area_scan (stack, check_for_saved, (void *) result); Do you really need the cast, here? > +static struct msp430_prologue * > +msp430_analyze_frame_prologue (struct frame_info *this_frame, > + void **this_prologue_cache) The indentation of the last line needs to be adjusted. > +/* Implement the "frame_this_id" method for unwinding frames. */ > + > +static void > +msp430_this_id (struct frame_info *this_frame, > + void **this_prologue_cache, struct frame_id *this_id) > +{ > + *this_id = frame_id_build (msp430_frame_base (this_frame, > + this_prologue_cache), > + get_frame_func (this_frame)); > +} The indentation of the second line for the function parameters is off, as well as "this_prologue_cache" in the call to msp430_frame_base... > +static struct value * > +msp430_prev_register (struct frame_info *this_frame, > + void **this_prologue_cache, int regnum) Indentation... > +static int > +msp430_dwarf2_reg_to_regnum (struct gdbarch *gdbarch, int reg) > +{ > + if (reg < MSP430_NUM_REGS) > + return reg + MSP430_NUM_REGS; > + else > + internal_error (__FILE__, __LINE__, > + _("Undefined dwarf2 register mapping of reg %d"), > + reg); I wouldn't raise an internal_error in this case, because the invalid register number could come from outside sources. I'd follow what we do for x86_64 and return -1 instead, after having emitted a warning. See for instance amd64-tdep.c:amd64_dwarf_reg_to_regnum. Our error handing isn't great regardless, and probably we should start thinking about what the function is expected to do in case of invalid register number. But that's for another patch... > + /* Aggregates of any size are passed by reference. */ > + gdb_byte struct_addr[4]; > + store_unsigned_integer (struct_addr, 4, byte_order, value_address(arg)); > + arg_bits = struct_addr; Missing empty line after local variable declaration, and the second line is a little too long... > + for (offset = 0; offset < arg_size; offset += 2) > + { > + /* The condition below prevents 8 byte scalars from being split > + between registers and memory (stack). It also prevents other > + splits once the stack has been written to. */ > + if (!current_arg_on_stack > + &&(arg_reg > + + ((arg_size == 8 || args_on_stack) > + ? ((arg_size - offset) / 2 - 1) > + : 0) <= MSP430_R15_REGNUM)) There are a few trailing spaces at the end of the some of the lines above. Might as well get rid of them... Missing space between "&&" and "(arg_reg". (pretty complex condition expression...) > + if (write_pass) > + { > + write_memory (sp + sp_off, arg_bits + offset, 2); > + } Extraneous curly braces. > + /* Push the return address. */ > + { > + int sz = (gdbarch_tdep (gdbarch)->code_model == MSP_SMALL_CODE_MODEL) Trailing space at the end. > +static const char msp430_epilog_name_prefix[] = "__mspabi_func_epilog_"; Can you add a short description of this constant? > +static int > +msp430_in_return_stub (struct gdbarch *gdbarch, CORE_ADDR pc, const char *name) This function also needs a short description; can you also split this line in 2? > +{ > + return (name != NULL > + && strncmp (msp430_epilog_name_prefix, name, Formatting of the second line... > +static CORE_ADDR > +msp430_skip_trampoline_code (struct frame_info *frame, CORE_ADDR pc) Needs a short description... > + if (gdbarch_tdep (gdbarch)->code_model == MSP_SMALL_CODE_MODEL > + && msp430_in_return_stub (gdbarch, pc, stub_name)) > + { > + CORE_ADDR sp = get_frame_register_unsigned (frame, MSP430_SP_REGNUM); > + return read_memory_integer Can you add an empty line after the local variable declaration? > + { > + struct gdbarch *ca = get_current_arch (); > + if (ca && gdbarch_bfd_arch_info (ca)->arch == bfd_arch_msp430) Missing empty line after local var decl. > + { > + struct gdbarch_tdep *ca_tdep = gdbarch_tdep (ca); > + elf_flags = ca_tdep->elf_flags; Likewise. > + } > + default: > + internal_error (__FILE__, __LINE__, _("Unknown msp430 isa")); > + break; Same as with the other internal error. I'd use an error, here, because the problem may occur due to an improperly built executable, or one that GDB does not recognize yet.. > + /* Try to find the architecture in the list of already defined > + architectures. */ > + for (arches = gdbarch_list_lookup_by_info (arches, &info); > + arches != NULL; > + arches = gdbarch_list_lookup_by_info (arches->next, &info)) > + { > + struct gdbarch_tdep *candidate_tdep = gdbarch_tdep (arches->gdbarch); > + if (candidate_tdep->elf_flags != elf_flags Missing empty line after local var decl. > + || candidate_tdep->isa != isa > + || candidate_tdep->code_model != code_model) -- Joel