[RFC] TI msp430 architecture support

[RFC] TI msp430 architecture support

[Kevin Buettner]

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.

Index: Makefile.in
===================================================================
RCS file: /cvs/src/src/gdb/Makefile.in,v
retrieving revision 1.1255
diff -u -p -r1.1255 Makefile.in
--- Makefile.in	15 May 2013 12:26:14 -0000	1.1255
+++ Makefile.in	17 May 2013 02:50:54 -0000
@@ -572,6 +572,7 @@ ALL_TARGET_OBS = \
 	mipsnbsd-tdep.o mips-tdep.o \
 	mn10300-linux-tdep.o mn10300-tdep.o \
 	moxie-tdep.o \
+	msp430-tdep.o \
 	mt-tdep.o \
 	nios2-tdep.o nios2-linux-tdep.o \
 	nto-tdep.o \
@@ -1520,6 +1521,7 @@ ALLDEPFILES = \
 	mips-tdep.c \
 	mipsnbsd-nat.c mipsnbsd-tdep.c \
 	mips64obsd-nat.c mips64obsd-tdep.c \
+	msp430-tdep.c \
 	nios2-tdep.c nios2-linux-tdep.c \
 	nbsd-nat.c nbsd-tdep.c obsd-tdep.c \
 	solib-osf.c \
Index: configure.tgt
===================================================================
RCS file: /cvs/src/src/gdb/configure.tgt,v
retrieving revision 1.276
diff -u -p -r1.276 configure.tgt
--- configure.tgt	7 May 2013 01:09:28 -0000	1.276
+++ configure.tgt	17 May 2013 02:50:54 -0000
@@ -391,6 +391,11 @@ mn10300-*-*)
 	gdb_sim=../sim/mn10300/libsim.a
 	;;
 
+msp430*-*-elf)
+	gdb_target_obs="msp430-tdep.o"
+	gdb_sim=../sim/msp430/libsim.a
+	;;
+
 mt-*-*)
 	# Target: Morpho Technologies ms1 processor
 	gdb_target_obs="mt-tdep.o"
Index: msp430-tdep.c
===================================================================
RCS file: msp430-tdep.c
diff -N msp430-tdep.c
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ msp430-tdep.c	17 May 2013 02:50:54 -0000
@@ -0,0 +1,1043 @@
+/* Target-dependent code for the Texas Instruments MSP430 for GDB, the
+   GNU debugger.
+
+   Copyright (C) 2012, 2013 Free Software Foundation, Inc.
+
+   Contributed by Red Hat, Inc.
+
+   This file is part of GDB.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#include "defs.h"
+#include "arch-utils.h"
+#include "prologue-value.h"
+#include "target.h"
+#include "regcache.h"
+#include "dis-asm.h"
+#include "gdbtypes.h"
+#include "frame.h"
+#include "frame-unwind.h"
+#include "frame-base.h"
+#include "value.h"
+#include "gdbcore.h"
+#include "dwarf2-frame.h"
+#include "reggroups.h"
+
+#include "elf/msp430.h"
+#include "opcode/msp430-decode.h"
+#include "elf-bfd.h"
+
+/* Register Numbers.  */
+
+enum
+{
+  MSP430_PC_RAW_REGNUM,
+  MSP430_SP_RAW_REGNUM,
+  MSP430_SR_RAW_REGNUM,
+  MSP430_CG_RAW_REGNUM,
+  MSP430_R4_RAW_REGNUM,
+  MSP430_R5_RAW_REGNUM,
+  MSP430_R6_RAW_REGNUM,
+  MSP430_R7_RAW_REGNUM,
+  MSP430_R8_RAW_REGNUM,
+  MSP430_R9_RAW_REGNUM,
+  MSP430_R10_RAW_REGNUM,
+  MSP430_R11_RAW_REGNUM,
+  MSP430_R12_RAW_REGNUM,
+  MSP430_R13_RAW_REGNUM,
+  MSP430_R14_RAW_REGNUM,
+  MSP430_R15_RAW_REGNUM,
+
+  MSP430_NUM_REGS,
+
+  MSP430_PC_REGNUM = MSP430_NUM_REGS,
+  MSP430_SP_REGNUM,
+  MSP430_SR_REGNUM,
+  MSP430_CG_REGNUM,
+  MSP430_R4_REGNUM,
+  MSP430_R5_REGNUM,
+  MSP430_R6_REGNUM,
+  MSP430_R7_REGNUM,
+  MSP430_R8_REGNUM,
+  MSP430_R9_REGNUM,
+  MSP430_R10_REGNUM,
+  MSP430_R11_REGNUM,
+  MSP430_R12_REGNUM,
+  MSP430_R13_REGNUM,
+  MSP430_R14_REGNUM,
+  MSP430_R15_REGNUM,
+
+  MSP430_NUM_TOTAL_REGS,
+  MSP430_NUM_PSEUDO_REGS = MSP430_NUM_TOTAL_REGS - MSP430_NUM_REGS
+};
+
+enum
+{
+  MSP_ISA_MSP430,
+  MSP_ISA_MSP430X
+};
+
+enum
+{
+  MSP_SMALL_CODE_MODEL,
+  MSP_LARGE_CODE_MODEL
+};
+
+/* Architecture specific data.  */
+
+struct gdbarch_tdep
+{
+  /* The ELF header flags specify the multilib used.  */
+  int elf_flags;
+  int isa;
+  int code_model;
+};
+
+/* This structure holds the results of a prologue analysis.  */
+
+struct msp430_prologue
+{
+  /* The offset from the frame base to the stack pointer --- always
+     zero or negative.
+
+     Calling this a "size" is a bit misleading, but given that the
+     stack grows downwards, using offsets for everything keeps one
+     from going completely sign-crazy: you never change anything's
+     sign for an ADD instruction; always change the second operand's
+     sign for a SUB instruction; and everything takes care of
+     itself.  */
+  int frame_size;
+
+  /* Non-zero if this function has initialized the frame pointer from
+     the stack pointer, zero otherwise.  */
+  int has_frame_ptr;
+
+  /* If has_frame_ptr is non-zero, this is the offset from the frame
+     base to where the frame pointer points.  This is always zero or
+     negative.  */
+  int frame_ptr_offset;
+
+  /* The address of the first instruction at which the frame has been
+     set up and the arguments are where the debug info says they are
+     --- as best as we can tell.  */
+  CORE_ADDR prologue_end;
+
+  /* reg_offset[R] is the offset from the CFA at which register R is
+     saved, or 1 if register R has not been saved.  (Real values are
+     always zero or negative.)  */
+  int reg_offset[MSP430_NUM_TOTAL_REGS];
+};
+
+/* Implement the "register_type" gdbarch method.  */
+
+static struct type *
+msp430_register_type (struct gdbarch *gdbarch, int reg_nr)
+{
+  if (reg_nr < MSP430_NUM_REGS)
+    return builtin_type (gdbarch)->builtin_uint32;
+  else if (reg_nr == MSP430_PC_REGNUM)
+    return builtin_type (gdbarch)->builtin_func_ptr;
+  else
+    return builtin_type (gdbarch)->builtin_uint16;
+}
+
+static struct type *
+msp430x_register_type (struct gdbarch *gdbarch, int reg_nr)
+{
+  if (reg_nr < MSP430_NUM_REGS)
+    return builtin_type (gdbarch)->builtin_uint32;
+  else if (reg_nr == MSP430_PC_REGNUM)
+    return builtin_type (gdbarch)->builtin_func_ptr;
+  else
+    return builtin_type (gdbarch)->builtin_uint32;
+}
+
+/* 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];
+}
+
+/* Implement the "register_reggroup_p" gdbarch method.  */
+
+static int
+msp430_register_reggroup_p (struct gdbarch *gdbarch, int regnum,
+			    struct reggroup *group)
+{
+  if (group == all_reggroup)
+    return 1;
+
+  /* All other registers are saved and restored.  */
+  if (group == save_reggroup || group == restore_reggroup)
+    return (MSP430_NUM_REGS <= regnum && regnum < MSP430_NUM_TOTAL_REGS);
+
+  return group == general_reggroup;
+}
+
+/* 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;
+}
+
+/* Implement the "pseudo_register_write" gdbarch method.  */
+
+static void
+msp430_pseudo_register_write (struct gdbarch *gdbarch,
+                              struct regcache *regcache,
+                              int regnum, const gdb_byte *buffer)
+{
+  enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
+  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;
+
+      val = extract_unsigned_integer (buffer, regsize, byte_order);
+      regcache_raw_write_unsigned (regcache, raw_regnum, val);
+
+    }
+  else
+    gdb_assert_not_reached ("invalid pseudo register number");
+}
+
+/* Implement the `register_sim_regno' gdbarch method.  */
+
+static int
+msp430_register_sim_regno (struct gdbarch *gdbarch, int regnum)
+{
+  gdb_assert (regnum < MSP430_NUM_REGS);
+
+  /* So long as regnum is in [0, RL78_NUM_REGS), it's valid.  We
+     just want to override the default here which disallows register
+     numbers which have no names.  */
+  return regnum;
+}
+
+/* Implement the "breakpoint_from_pc" gdbarch method.  */
+
+static const gdb_byte *
+msp430_breakpoint_from_pc (struct gdbarch *gdbarch, CORE_ADDR *pcptr,
+                         int *lenptr)
+{
+  static gdb_byte breakpoint[] = { 0x43, 0x43 };
+
+  *lenptr = sizeof breakpoint;
+  return breakpoint;
+}
+
+/* Define a "handle" struct for fetching the next opcode.  */
+
+struct msp430_get_opcode_byte_handle
+{
+  CORE_ADDR pc;
+};
+
+/* Fetch a byte on behalf of the opcode decoder.  HANDLE contains
+   the memory address of the next byte to fetch.  If successful,
+   the address in the handle is updated and the byte fetched is
+   returned as the value of the function.  If not successful, -1
+   is returned.  */
+
+static int
+msp430_get_opcode_byte (void *handle)
+{
+  struct msp430_get_opcode_byte_handle *opcdata = handle;
+  int status;
+  gdb_byte byte;
+
+  status = target_read_memory (opcdata->pc, &byte, 1);
+  if (status == 0)
+    {
+      opcdata->pc += 1;
+      return byte;
+    }
+  else
+    return -1;
+}
+
+/* Function for finding saved registers in a 'struct pv_area'; this
+   function is passed to pv_area_scan.
+
+   If VALUE is a saved register, ADDR says it was saved at a constant
+   offset from the frame base, and SIZE indicates that the whole
+   register was saved, record its offset.  */
+
+static void
+check_for_saved (void *result_untyped, pv_t addr, CORE_ADDR size,
+                 pv_t value)
+{
+  struct msp430_prologue *result = (struct msp430_prologue *) result_untyped;
+
+  if (value.kind == pvk_register
+      && value.k == 0
+      && pv_is_register (addr, MSP430_SP_REGNUM)
+      && size == register_size (target_gdbarch (), value.reg))
+    result->reg_offset[value.reg] = addr.k;
+}
+
+/* Analyze a prologue starting at START_PC, going no further than
+   LIMIT_PC.  Fill in RESULT as appropriate.  */
+
+static void
+msp430_analyze_prologue (struct gdbarch *gdbarch, CORE_ADDR start_pc,
+		         CORE_ADDR limit_pc, struct msp430_prologue *result)
+{
+  CORE_ADDR pc, next_pc;
+  int rn;
+  pv_t reg[MSP430_NUM_TOTAL_REGS];
+  struct pv_area *stack;
+  struct cleanup *back_to;
+  CORE_ADDR after_last_frame_setup_insn = start_pc;
+  int code_model = gdbarch_tdep (gdbarch)->code_model;
+  int sz;
+
+  memset (result, 0, sizeof (*result));
+
+  for (rn = 0; rn < MSP430_NUM_TOTAL_REGS; rn++)
+    {
+      reg[rn] = pv_register (rn, 0);
+      result->reg_offset[rn] = 1;
+    }
+
+  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);
+      next_pc = pc + bytes_read;
+
+      if (opc.id == MSO_push
+               && opc.op[0].type == MSP430_Operand_Register)
+	{
+	  int rsrc = opc.op[0].reg;
+
+	  reg[MSP430_SP_REGNUM] = pv_add_constant (reg[MSP430_SP_REGNUM], -2);
+	  pv_area_store (stack, reg[MSP430_SP_REGNUM], 2, reg[rsrc]);
+	  after_last_frame_setup_insn = next_pc;
+	}
+      else if (opc.id == MSO_push			/* PUSHM  */
+               && opc.op[0].type == MSP430_Operand_None
+               && opc.op[1].type == MSP430_Operand_Register)
+	{
+	  int rsrc = opc.op[1].reg;
+	  int count = opc.repeats + 1;
+	  int size = opc.size == 16 ? 2 : 4;
+
+	  while (count > 0)
+	    {
+	      reg[MSP430_SP_REGNUM] = pv_add_constant (reg[MSP430_SP_REGNUM], -size);
+	      pv_area_store (stack, reg[MSP430_SP_REGNUM], size, reg[rsrc]);
+	      rsrc--;
+	      count--;
+	    }
+	  after_last_frame_setup_insn = next_pc;
+	}
+      else if (opc.id == MSO_sub
+               && opc.op[0].type == MSP430_Operand_Register
+	       && opc.op[0].reg == MSR_SP
+	       && opc.op[1].type == MSP430_Operand_Immediate)
+	{
+	  int addend = opc.op[1].addend;
+
+	  reg[MSP430_SP_REGNUM] = pv_add_constant (reg[MSP430_SP_REGNUM],
+	                                           -addend);
+	  after_last_frame_setup_insn = next_pc;
+	}
+      else if (opc.id == MSO_mov
+               && opc.op[0].type == MSP430_Operand_Immediate
+	       && 12 <= opc.op[0].reg && opc.op[0].reg <= 15)
+	{
+	  after_last_frame_setup_insn = next_pc;
+	}
+      else
+	{
+	  /* Terminate the prologue scan.  */
+	  break;
+	}
+
+      pc = next_pc;
+    }
+
+  /* Is the frame size (offset, really) a known constant?  */
+  if (pv_is_register (reg[MSP430_SP_REGNUM], MSP430_SP_REGNUM))
+    result->frame_size = reg[MSP430_SP_REGNUM].k;
+
+  /* Record where all the registers were saved.  */
+  pv_area_scan (stack, check_for_saved, (void *) result);
+
+  result->prologue_end = after_last_frame_setup_insn;
+
+  do_cleanups (back_to);
+}
+
+/* Implement the "skip_prologue" gdbarch method.  */
+
+static CORE_ADDR
+msp430_skip_prologue (struct gdbarch *gdbarch, CORE_ADDR pc)
+{
+  const char *name;
+  CORE_ADDR func_addr, func_end;
+  struct msp430_prologue p;
+
+  /* Try to find the extent of the function that contains PC.  */
+  if (!find_pc_partial_function (pc, &name, &func_addr, &func_end))
+    return pc;
+
+  msp430_analyze_prologue (gdbarch, pc, func_end, &p);
+  return p.prologue_end;
+}
+
+/* Implement the "unwind_pc" gdbarch method.  */
+
+static CORE_ADDR
+msp430_unwind_pc (struct gdbarch *arch, struct frame_info *next_frame)
+{
+  return frame_unwind_register_unsigned (next_frame, MSP430_PC_REGNUM);
+}
+
+/* Implement the "unwind_sp" gdbarch method.  */
+
+static CORE_ADDR
+msp430_unwind_sp (struct gdbarch *arch, struct frame_info *next_frame)
+{
+  return frame_unwind_register_unsigned (next_frame, MSP430_SP_REGNUM);
+}
+
+/* Given a frame described by THIS_FRAME, decode the prologue of its
+   associated function if there is not cache entry as specified by
+   THIS_PROLOGUE_CACHE.  Save the decoded prologue in the cache and
+   return that struct as the value of this function.  */
+
+static struct msp430_prologue *
+msp430_analyze_frame_prologue (struct frame_info *this_frame,
+			   void **this_prologue_cache)
+{
+  if (!*this_prologue_cache)
+    {
+      CORE_ADDR func_start, stop_addr;
+
+      *this_prologue_cache = FRAME_OBSTACK_ZALLOC (struct msp430_prologue);
+
+      func_start = get_frame_func (this_frame);
+      stop_addr = get_frame_pc (this_frame);
+
+      /* If we couldn't find any function containing the PC, then
+         just initialize the prologue cache, but don't do anything.  */
+      if (!func_start)
+	stop_addr = func_start;
+
+      msp430_analyze_prologue (get_frame_arch (this_frame), func_start,
+                               stop_addr, *this_prologue_cache);
+    }
+
+  return *this_prologue_cache;
+}
+
+/* Given a frame and a prologue cache, return this frame's base.  */
+
+static CORE_ADDR
+msp430_frame_base (struct frame_info *this_frame, void **this_prologue_cache)
+{
+  struct msp430_prologue *p
+    = msp430_analyze_frame_prologue (this_frame, this_prologue_cache);
+  CORE_ADDR sp = get_frame_register_unsigned (this_frame, MSP430_SP_REGNUM);
+
+  return sp - p->frame_size;
+}
+
+/* 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));
+}
+
+/* Implement the "frame_prev_register" method for unwinding frames.  */
+
+static struct value *
+msp430_prev_register (struct frame_info *this_frame,
+                    void **this_prologue_cache, int regnum)
+{
+  struct msp430_prologue *p
+    = msp430_analyze_frame_prologue (this_frame, this_prologue_cache);
+  CORE_ADDR frame_base = msp430_frame_base (this_frame, this_prologue_cache);
+
+  if (regnum == MSP430_SP_REGNUM)
+    return frame_unwind_got_constant (this_frame, regnum, frame_base);
+
+  /* If prologue analysis says we saved this register somewhere,
+     return a description of the stack slot holding it.  */
+  else if (p->reg_offset[regnum] != 1)
+    {
+      struct value *rv =
+        frame_unwind_got_memory (this_frame, regnum,
+				 frame_base + p->reg_offset[regnum]);
+
+      if (regnum == MSP430_PC_REGNUM)
+	{
+	  ULONGEST pc = value_as_long (rv);
+
+	  return frame_unwind_got_constant (this_frame, regnum, pc);
+	}
+      return rv;
+    }
+
+  /* Otherwise, presume we haven't changed the value of this
+     register, and get it from the next frame.  */
+  else
+    return frame_unwind_got_register (this_frame, regnum, regnum);
+}
+
+static const struct frame_unwind msp430_unwind =
+{
+  NORMAL_FRAME,
+  default_frame_unwind_stop_reason,
+  msp430_this_id,
+  msp430_prev_register,
+  NULL,
+  default_frame_sniffer
+};
+
+/* Implement the "dwarf2_reg_to_regnum" gdbarch method.  */
+
+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);
+}
+
+/* Implement the "return_value" gdbarch method.  */
+
+static enum return_value_convention
+msp430_return_value (struct gdbarch *gdbarch,
+		     struct value *function,
+		     struct type *valtype,
+		     struct regcache *regcache,
+		     gdb_byte *readbuf, const gdb_byte *writebuf)
+{
+  enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
+  LONGEST valtype_len = TYPE_LENGTH (valtype);
+  int code_model = gdbarch_tdep (gdbarch)->code_model;
+
+  if (TYPE_LENGTH (valtype) > 8
+      || TYPE_CODE (valtype) == TYPE_CODE_STRUCT
+      || TYPE_CODE (valtype) == TYPE_CODE_UNION)
+    return RETURN_VALUE_STRUCT_CONVENTION;
+
+  if (readbuf)
+    {
+      ULONGEST u;
+      int argreg = MSP430_R12_REGNUM;
+      int offset = 0;
+
+      while (valtype_len > 0)
+	{
+	  int size = 2;
+
+	  if (code_model == MSP_LARGE_CODE_MODEL
+	      && TYPE_CODE (valtype) == TYPE_CODE_PTR)
+	    {
+	      size = 4;
+	    }
+
+	  regcache_cooked_read_unsigned (regcache, argreg, &u);
+	  store_unsigned_integer (readbuf + offset, size, byte_order, u);
+	  valtype_len -= size;
+	  offset += size;
+	  argreg++;
+	}
+    }
+
+  if (writebuf)
+    {
+      ULONGEST u;
+      int argreg = MSP430_R12_REGNUM;
+      int offset = 0;
+
+      while (valtype_len > 0)
+	{
+	  int size = 2;
+
+	  if (code_model == MSP_LARGE_CODE_MODEL
+	      && TYPE_CODE (valtype) == TYPE_CODE_PTR)
+	    {
+	      size = 4;
+	    }
+
+	  u = extract_unsigned_integer (writebuf + offset, size, byte_order);
+	  regcache_cooked_write_unsigned (regcache, argreg, u);
+	  valtype_len -= size;
+	  offset += size;
+	  argreg++;
+	}
+    }
+
+  return RETURN_VALUE_REGISTER_CONVENTION;
+}
+
+
+/* Implement the "frame_align" gdbarch method.  */
+
+static CORE_ADDR
+msp430_frame_align (struct gdbarch *gdbarch, CORE_ADDR sp)
+{
+  return align_down (sp, 2);
+}
+
+
+/* Implement the "dummy_id" gdbarch method.  */
+
+static struct frame_id
+msp430_dummy_id (struct gdbarch *gdbarch, struct frame_info *this_frame)
+{
+  return
+    frame_id_build (get_frame_register_unsigned
+		        (this_frame, MSP430_SP_REGNUM),
+		    get_frame_pc (this_frame));
+}
+
+
+/* Implement the "push_dummy_call" gdbarch method.  */
+
+static CORE_ADDR
+msp430_push_dummy_call (struct gdbarch *gdbarch, struct value *function,
+		        struct regcache *regcache, CORE_ADDR bp_addr,
+		        int nargs, struct value **args, CORE_ADDR sp,
+		        int struct_return, CORE_ADDR struct_addr)
+{
+  enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
+  int write_pass;
+  int sp_off = 0;
+  CORE_ADDR cfa;
+  int code_model = gdbarch_tdep (gdbarch)->code_model;
+
+  struct type *func_type = value_type (function);
+
+  /* Dereference function pointer types.  */
+  while (TYPE_CODE (func_type) == TYPE_CODE_PTR)
+    func_type = TYPE_TARGET_TYPE (func_type);
+
+  /* The end result had better be a function or a method.  */
+  gdb_assert (TYPE_CODE (func_type) == TYPE_CODE_FUNC
+	      || TYPE_CODE (func_type) == TYPE_CODE_METHOD);
+
+  /* We make two passes; the first does the stack allocation,
+     the second actually stores the arguments.  */
+  for (write_pass = 0; write_pass <= 1; write_pass++)
+    {
+      int i;
+      int arg_reg = MSP430_R12_REGNUM;
+      int args_on_stack = 0;
+
+      if (write_pass)
+	sp = align_down (sp - sp_off, 4);
+      sp_off = 0;
+
+      if (struct_return)
+	{
+	  if (write_pass)
+	    regcache_cooked_write_unsigned (regcache, arg_reg,
+					    struct_addr);
+	  arg_reg++;
+	}
+
+      /* Push the arguments.  */
+      for (i = 0; i < nargs; i++)
+	{
+	  struct value *arg = args[i];
+	  const gdb_byte *arg_bits = value_contents_all (arg);
+	  struct type *arg_type = check_typedef (value_type (arg));
+	  ULONGEST arg_size = TYPE_LENGTH (arg_type);
+	  int offset;
+	  int current_arg_on_stack;
+
+	  current_arg_on_stack = 0;
+
+	  if (TYPE_CODE (arg_type) == TYPE_CODE_STRUCT
+	       || TYPE_CODE (arg_type) == TYPE_CODE_UNION)
+	    {
+	      /* 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;
+	      arg_size = (code_model == MSP_LARGE_CODE_MODEL) ? 4 : 2;
+	    }
+	  else
+	    {
+	      /* Scalars bigger than 8 bytes such as complex doubles are passed
+	         on the stack.  */
+	      if (arg_size > 8)
+		current_arg_on_stack = 1;
+	    }
+
+
+	  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))
+		{
+		  int size = 2;
+
+		  if (code_model == MSP_LARGE_CODE_MODEL
+		      && TYPE_CODE (arg_type) == TYPE_CODE_PTR)
+		    {
+		      /* Pointer arguments using large memory model are passed
+		         using entire register.  */
+		      if (offset != 0)
+			continue;
+		      size = 4;
+		    }
+
+		  if (write_pass)
+		    {
+		      regcache_cooked_write_unsigned (regcache, arg_reg,
+						      extract_unsigned_integer
+						      (arg_bits + offset, size,
+						       byte_order));
+		    }
+		  arg_reg++;
+		}
+	      else
+		{
+		  if (write_pass)
+		    {
+		      write_memory (sp + sp_off, arg_bits + offset, 2);
+		    }
+		  sp_off += 2;
+		  args_on_stack = 1;
+		  current_arg_on_stack = 1;
+		}
+	    }
+	}
+    }
+
+  /* Keep track of the stack address prior to pushing the return address.
+     This is the value that we'll return.  */
+  cfa = sp;
+
+  /* Push the return address.  */
+  {
+    int sz = (gdbarch_tdep (gdbarch)->code_model == MSP_SMALL_CODE_MODEL) 
+             ? 2 : 4;
+    sp = sp - sz;
+    write_memory_unsigned_integer (sp, sz, byte_order, bp_addr);
+  }
+
+  /* Update the stack pointer.  */
+  regcache_cooked_write_unsigned (regcache, MSP430_SP_REGNUM, sp);
+
+  return cfa;
+}
+
+static const char msp430_epilog_name_prefix[] = "__mspabi_func_epilog_";
+
+static int
+msp430_in_return_stub (struct gdbarch *gdbarch, CORE_ADDR pc, const char *name)
+{
+  return (name != NULL
+       && strncmp (msp430_epilog_name_prefix, name,
+                   strlen (msp430_epilog_name_prefix)) == 0);
+}
+
+static CORE_ADDR
+msp430_skip_trampoline_code (struct frame_info *frame, CORE_ADDR pc)
+{
+  struct bound_minimal_symbol bms;
+  const char *stub_name;
+  struct gdbarch *gdbarch = get_frame_arch (frame);
+
+  bms = lookup_minimal_symbol_by_pc (pc);
+  if (!bms.minsym)
+    return pc;
+
+  stub_name = SYMBOL_LINKAGE_NAME (bms.minsym);
+
+  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
+        (sp + 2 * (stub_name[strlen (msp430_epilog_name_prefix)] - '0'),
+	 2,
+	 gdbarch_byte_order (gdbarch));
+    }
+
+  return pc;
+}
+
+/* Allocate and initialize a gdbarch object.  */
+
+static struct gdbarch *
+msp430_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
+{
+  struct gdbarch *gdbarch;
+  struct gdbarch_tdep *tdep;
+  int elf_flags, isa, code_model;
+
+  /* Extract the elf_flags if available.  */
+  if (info.abfd != NULL
+      && bfd_get_flavour (info.abfd) == bfd_target_elf_flavour)
+    elf_flags = elf_elfheader (info.abfd)->e_flags;
+  else
+    elf_flags = 0;
+
+  if (info.abfd != NULL)
+    switch (bfd_elf_get_obj_attr_int (info.abfd, OBJ_ATTR_PROC,
+				      OFBA_MSPABI_Tag_ISA))
+      {
+      case 1:
+	isa = MSP_ISA_MSP430;
+	code_model = MSP_SMALL_CODE_MODEL;
+	break;
+      case 2:
+	isa = MSP_ISA_MSP430X;
+	switch (bfd_elf_get_obj_attr_int (info.abfd, OBJ_ATTR_PROC,
+						 OFBA_MSPABI_Tag_Code_Model))
+	  {
+	  case 1:
+	    code_model = MSP_SMALL_CODE_MODEL;
+	    break;
+	  case 2:
+	    code_model = MSP_LARGE_CODE_MODEL;
+	    break;
+	  default:
+	    internal_error (__FILE__, __LINE__, _("Unknown msp430x code memory model"));
+	    break;
+	  }
+	break;
+      case 0:
+	/* This can happen when loading a previously dumped data structure.
+	   Use the ISA and code model from the current architecture, provided
+	   it's compatible.  */
+	{
+	  struct gdbarch *ca = get_current_arch ();
+	  if (ca && gdbarch_bfd_arch_info (ca)->arch == bfd_arch_msp430)
+	    {
+	      struct gdbarch_tdep *ca_tdep = gdbarch_tdep (ca);
+	      elf_flags = ca_tdep->elf_flags;
+	      isa = ca_tdep->isa;
+	      code_model = ca_tdep->code_model;
+	      break;
+	    }
+	  /* Otherwise, fall through...  */
+	}
+      default:
+	internal_error (__FILE__, __LINE__, _("Unknown msp430 isa"));
+	break;
+      }
+    else
+      {
+	isa = MSP_ISA_MSP430;
+	code_model = MSP_SMALL_CODE_MODEL;
+      }
+
+
+  /* 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
+          || candidate_tdep->isa != isa
+	  || candidate_tdep->code_model != code_model)
+	continue;
+
+      return arches->gdbarch;
+    }
+
+  /* None found, create a new architecture from the information
+     provided.  */
+  tdep = (struct gdbarch_tdep *) xmalloc (sizeof (struct gdbarch_tdep));
+  gdbarch = gdbarch_alloc (&info, tdep);
+  tdep->elf_flags = elf_flags;
+  tdep->isa = isa;
+  tdep->code_model = code_model;
+
+  /* Registers.  */
+  set_gdbarch_num_regs (gdbarch, MSP430_NUM_REGS);
+  set_gdbarch_num_pseudo_regs (gdbarch, MSP430_NUM_PSEUDO_REGS);
+  set_gdbarch_register_name (gdbarch, msp430_register_name);
+  if (isa == MSP_ISA_MSP430)
+    set_gdbarch_register_type (gdbarch, msp430_register_type);
+  else
+    set_gdbarch_register_type (gdbarch, msp430x_register_type);
+  set_gdbarch_pc_regnum (gdbarch, MSP430_PC_REGNUM);
+  set_gdbarch_sp_regnum (gdbarch, MSP430_SP_REGNUM);
+  set_gdbarch_register_reggroup_p (gdbarch, msp430_register_reggroup_p);
+  set_gdbarch_pseudo_register_read (gdbarch, msp430_pseudo_register_read);
+  set_gdbarch_pseudo_register_write (gdbarch, msp430_pseudo_register_write);
+  set_gdbarch_dwarf2_reg_to_regnum (gdbarch, msp430_dwarf2_reg_to_regnum);
+  set_gdbarch_register_sim_regno (gdbarch, msp430_register_sim_regno);
+
+  /* Data types.  */
+  set_gdbarch_char_signed (gdbarch, 0);
+  set_gdbarch_short_bit (gdbarch, 16);
+  set_gdbarch_int_bit (gdbarch, 16);
+  set_gdbarch_long_bit (gdbarch, 32);
+  set_gdbarch_long_long_bit (gdbarch, 64);
+  if (code_model == MSP_SMALL_CODE_MODEL)
+    {
+      set_gdbarch_ptr_bit (gdbarch, 16);
+      set_gdbarch_addr_bit (gdbarch, 16);
+    }
+  else /* MSP_LARGE_CODE_MODEL */
+    {
+      set_gdbarch_ptr_bit (gdbarch, 32);
+      set_gdbarch_addr_bit (gdbarch, 32);
+    }
+  set_gdbarch_dwarf2_addr_size (gdbarch, 4);
+  set_gdbarch_float_bit (gdbarch, 32);
+  set_gdbarch_float_format (gdbarch, floatformats_ieee_single);
+  set_gdbarch_double_bit (gdbarch, 64);
+  set_gdbarch_long_double_bit (gdbarch, 64);
+  set_gdbarch_double_format (gdbarch, floatformats_ieee_double);
+  set_gdbarch_long_double_format (gdbarch, floatformats_ieee_double);
+
+  /* Breakpoints.  */
+  set_gdbarch_breakpoint_from_pc (gdbarch, msp430_breakpoint_from_pc);
+  set_gdbarch_decr_pc_after_break (gdbarch, 1);
+
+  /* Disassembly.  */
+  set_gdbarch_print_insn (gdbarch, print_insn_msp430);
+
+  /* Frames, prologues, etc.  */
+  set_gdbarch_inner_than (gdbarch, core_addr_lessthan);
+  set_gdbarch_skip_prologue (gdbarch, msp430_skip_prologue);
+  set_gdbarch_unwind_pc (gdbarch, msp430_unwind_pc);
+  set_gdbarch_unwind_sp (gdbarch, msp430_unwind_sp);
+  set_gdbarch_frame_align (gdbarch, msp430_frame_align);
+  dwarf2_append_unwinders (gdbarch);
+  frame_unwind_append_unwinder (gdbarch, &msp430_unwind);
+
+  /* Dummy frames, return values.  */
+  set_gdbarch_dummy_id (gdbarch, msp430_dummy_id);
+  set_gdbarch_push_dummy_call (gdbarch, msp430_push_dummy_call);
+  set_gdbarch_return_value (gdbarch, msp430_return_value);
+
+  /* Trampolines.  */
+  set_gdbarch_in_solib_return_trampoline (gdbarch, msp430_in_return_stub);
+  set_gdbarch_skip_trampoline_code (gdbarch, msp430_skip_trampoline_code);
+
+  /* Virtual tables.  */
+  set_gdbarch_vbit_in_delta (gdbarch, 0);
+
+  return gdbarch;
+}
+
+/* -Wmissing-prototypes */
+extern initialize_file_ftype _initialize_msp430_tdep;
+
+/* Register the initialization routine.  */
+
+void
+_initialize_msp430_tdep (void)
+{
+  register_gdbarch_init (bfd_arch_msp430, msp430_gdbarch_init);
+}

Re: [RFC] TI msp430 architecture support

[Joel Brobecker]

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

Re: [RFC] TI msp430 architecture support

[Pedro Alves]

Could you also add a NEWS entry under "New targets"?  Thanks!

I think an entry for the simulator would be good too.

-- 
Pedro Alves

Re: [RFC] TI msp430 architecture support

[Mike Frysinger]

[-- Attachment #1: Type: Text/Plain, Size: 429 bytes --]

On Thursday 16 May 2013 23:00:50 Kevin Buettner wrote:
> RCS file: /cvs/src/src/gdb/configure.tgt,v
> retrieving revision 1.276
> diff -u -p -r1.276 configure.tgt
> --- configure.tgt	7 May 2013 01:09:28 -0000	1.276
> +++ configure.tgt	17 May 2013 02:50:54 -0000
> 
> +msp430*-*-elf)
> +	gdb_target_obs="msp430-tdep.o"
> +	gdb_sim=../sim/msp430/libsim.a
> +	;;

there is no sim/msp430/ dir, so this won't work
-mike

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [RFC] TI msp430 architecture support

[Mike Frysinger]

[-- Attachment #1: Type: Text/Plain, Size: 562 bytes --]

On Friday 17 May 2013 13:43:54 Mike Frysinger wrote:
> On Thursday 16 May 2013 23:00:50 Kevin Buettner wrote:
> > RCS file: /cvs/src/src/gdb/configure.tgt,v
> > retrieving revision 1.276
> > diff -u -p -r1.276 configure.tgt
> > --- configure.tgt	7 May 2013 01:09:28 -0000	1.276
> > +++ configure.tgt	17 May 2013 02:50:54 -0000
> > 
> > +msp430*-*-elf)
> > +	gdb_target_obs="msp430-tdep.o"
> > +	gdb_sim=../sim/msp430/libsim.a
> > +	;;
> 
> there is no sim/msp430/ dir, so this won't work

sorry, ignore me.  i thought my inbox was synced.
-mike

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [RFC] TI msp430 architecture support

[Tom Tromey]

>>>>> "Mike" == Mike Frysinger <vapier@gentoo.org> writes:

>> +msp430*-*-elf)
>> +	gdb_target_obs="msp430-tdep.o"
>> +	gdb_sim=../sim/msp430/libsim.a
>> +	;;

Mike> there is no sim/msp430/ dir, so this won't work

There was an earlier patch to add it:

http://sourceware.org/ml/gdb-patches/2013-05/msg00658.html

Tom

Re: [RFC] TI msp430 architecture support

[Kevin Buettner]

On Fri, 17 May 2013 10:29:11 +0400
Joel Brobecker <brobecker@adacore.com> wrote:

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

Thanks for the review, Joel.

I fixed the formatting issues that you noticed and then, for
good measure, I ran the file through gdb_indent.sh.  I should've
run gdb_indent.sh over msp430-tdep.c prior to submitting it.

I've also added the comments that you requested.

I'll address some of your other comments below...

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

It's important that either NULL or "" is returned for either
unimplemented register numbers or numbers that we don't want
to show up in "info registers" and other register output.  
mi_cmd_data_list_register_names() requires this behavior, for
example.

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

I agree with this. The function looks like this now:

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];
}

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

I copied the gdb_assert_not_reached from another port.  I can't really
claim credit for thinking of using it here.

I do like your idea of using a more meaningful message using the
register number, but this would require changing gdb_assert_not_reached()
so that it'll handle a printf type argument list.  Alternately, we
could constuct a suitable string to pass to gdb_assert_not_reached,
but this seems a bit excessive for handling a case that we don't
expect to ever happen.  (When things like this do happen, I place
a breakpoint in the assert code and then go up the necessary number
of frames to see the values of the variables which triggered it.)

> > +  /* Record where all the registers were saved.  */
> > +  pv_area_scan (stack, check_for_saved, (void *) result);
> 
> Do you really need the cast, here?

Nope.  I removed it.  (I'm pretty sure that I copied it from somewhere
else too.)

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

I agree.  The function in question now looks like this:

static int
msp430_dwarf2_reg_to_regnum (struct gdbarch *gdbarch, int reg)
{
  if (reg < MSP430_NUM_REGS)
    return reg + MSP430_NUM_REGS;
  else
    {
      warning (_("Unmapped DWARF Register #%d encountered."), reg);
      return -1;
    }
}

Thanks again for your review.

I've committed msp430-tdep.c and related changes to the configury.  I'll write
up a NEWS entry as requested by Pedro.

Kevin

Re: [RFC] TI msp430 architecture support

[Tom Tromey]

>>>>> "Kevin" == Kevin Buettner <kevinb@redhat.com> writes:

Kevin> I fixed the formatting issues that you noticed and then, for
Kevin> good measure, I ran the file through gdb_indent.sh.  I should've
Kevin> run gdb_indent.sh over msp430-tdep.c prior to submitting it.

I didn't know anybody used gdb_indent.sh.  Was this a one-off thing or
have you run it regularly on other source files?

Tom

Re: [RFC] TI msp430 architecture support

[Kevin Buettner]

On Tue, 25 Jun 2013 08:25:20 -0600
Tom Tromey <tromey@redhat.com> wrote:

> >>>>> "Kevin" == Kevin Buettner <kevinb@redhat.com> writes:
> 
> Kevin> I fixed the formatting issues that you noticed and then, for
> Kevin> good measure, I ran the file through gdb_indent.sh.  I should've
> Kevin> run gdb_indent.sh over msp430-tdep.c prior to submitting it.
> 
> I didn't know anybody used gdb_indent.sh.  Was this a one-off thing or
> have you run it regularly on other source files?

There was a time when the output of GNU indent was considered the
standard for GDB indentation.  If there was any disagreement about
correct indentation, a contributor would be asked to run GNU indent
over the code.  The problem is, of course, that different options
produce different results which is how gdb_indent.sh came into being.

My recollection is that there were at least two occasions when
significant portions of the gdb source tree were reindented.  (This
may have pre-dated the creation of gdb_indent.sh, however.) I don't
think that mass reindentation should be done very often because it
causes a lot more work for folks who have developed significant
changes prior to the the reindenation occurring.  I think it can make
sense for newly contributed files, however.

Kevin

Re: [RFC] TI msp430 architecture support

[Stan Shebs]

On 6/25/13 8:51 AM, Kevin Buettner wrote:
> On Tue, 25 Jun 2013 08:25:20 -0600
> Tom Tromey <tromey@redhat.com> wrote:
> 
>>>>>>> "Kevin" == Kevin Buettner <kevinb@redhat.com> writes:
>>
>> Kevin> I fixed the formatting issues that you noticed and then, for
>> Kevin> good measure, I ran the file through gdb_indent.sh.  I should've
>> Kevin> run gdb_indent.sh over msp430-tdep.c prior to submitting it.
>>
>> I didn't know anybody used gdb_indent.sh.  Was this a one-off thing or
>> have you run it regularly on other source files?
> 
> There was a time when the output of GNU indent was considered the
> standard for GDB indentation.  If there was any disagreement about
> correct indentation, a contributor would be asked to run GNU indent
> over the code.  The problem is, of course, that different options
> produce different results which is how gdb_indent.sh came into being.
> 
> My recollection is that there were at least two occasions when
> significant portions of the gdb source tree were reindented.  (This
> may have pre-dated the creation of gdb_indent.sh, however.) I don't
> think that mass reindentation should be done very often because it
> causes a lot more work for folks who have developed significant
> changes prior to the the reindenation occurring.  I think it can make
> sense for newly contributed files, however.
> 
> Kevin
> 

In the past when I've run GNU indent on GDB, its results were
reasonable, but subtly different from Emacs region indents; so while
indent was a good tool to clean up messes, Emacs users were going to
find code formatting changing around them unexpectedly.

The script came later, I don't know if its options fix that problem.
(I never discovered a combination of options that exactly matched
Emacs behavior.)

Stan
stan@codesourcery.com