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: [RFC] TI msp430 architecture support


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


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