On 10 Mar 2015 16:56, James Bowman wrote:
> gdb/:
> 2015-03-10  James Bowman  <james.bowman@ftdichip.com>
> 
> 	* Mainfile.in: Add FT32 entries.
> 	* configure.tgt: Add FT32 entry.
> 	* ft32-tdep.c,h: New file, FT32 target-dependent code.

the file names should be spelled out

someone else will have to approve the gdb/ parts

> --- /dev/null
> +++ b/gdb/ft32-tdep.c
>
> +struct ft32_frame_cache
> +{
> +  /* Base address.  */
> +  CORE_ADDR base;
> +  CORE_ADDR pc;
> +  LONGEST framesize;
> +  CORE_ADDR saved_regs[FT32_NUM_REGS];
> +  CORE_ADDR saved_sp;
> +  bfd_boolean established;  /* Has the new frame been LINKed */

comment needs to be tweaked -- punctuation and two spaces at the end

i'd personally prefer it to be above the member rather than inline, but that's 
me ...

> +static const char * ft32_register_names[] =

one more const:
static const char * const ft32_register_names[] =

> +static CORE_ADDR
> +ft32_analyze_prologue (CORE_ADDR start_addr, CORE_ADDR end_addr,
> +                       struct ft32_frame_cache *cache,
> +                       struct gdbarch *gdbarch)
> +{
> ...
> +  /* It is a LINK */

style needs tweaking

> +static CORE_ADDR
> +ft32_skip_prologue (struct gdbarch *gdbarch, CORE_ADDR pc)
> +{
> ...
> +  /* No function symbol -- just return the PC.  */
> +  return (CORE_ADDR) pc;

why the cast ?  pc is already of type CORE_ADDR ...

> +static struct value *
> +ft32_frame_prev_register (struct frame_info *this_frame,
> +                          void **this_prologue_cache, int regnum)
> +{
> ...
> +  if (regnum < FT32_NUM_REGS && cache->saved_regs[regnum] != REG_UNAVAIL)
> +      return frame_unwind_got_memory (this_frame, regnum,
> +                                      0x800000 | cache->saved_regs[regnum]);

documenting that constant would be nice

> --- /dev/null
> +++ b/sim/ft32/Makefile.in
>
> +SIM_OBJS = \
> +  $(SIM_NEW_COMMON_OBJS) \

indent the entries in this var with tabs

> +SIM_RUN_OBJS = nrun.o

i flipped the default in the latest tree so you can delete this now

> --- /dev/null
> +++ b/sim/ft32/interp.c
>
> +static uint32_t safe_addr (uint32_t dw, uint32_t ea)
> +{
> +  ea &= 0x1ffff;
> +  switch (dw)
> +  {

the braces on the switch should be indented by two spaces -- the same as the 
case labels.  should fix in the whole file.

  switch (foo)
    {
    case 0:
      ...
      break;
    ...
    }

> +static uint32_t cpu_mem_read (SIM_DESC sd, uint32_t dw, uint32_t ea)
> +{
> ...
> +      switch (ea)
> +      {
> +      case 0x10000:
> +      case 0x10020:
> +        return getchar ();

err what's this for ?  you trying to simulate a UART or something ?

> +      case 0x1fff4:
> +        return (uint32_t)(cpu->state.cycles / 100);

no need for the cast

> +      default:
> +        sim_io_eprintf (sd, "Illegal IO read address %08x, pc %#x\n", ea, cpu->state.pc);
> +        exit (0);

the sim should never exit directly.  you should use sim_engine_halt instead.

also, we generally want code to wrap at 80 cols.  i'm not super strict on this 
(like in the code later on with the packed bit shifts / case statements), but 
you should wrap in simple cases like this printf.  you should scan the whole 
file for other long lines.

> +  r = cpu->state.ram[ea];
> +  if (1 <= dw)
> +    {
> +      r += (cpu->state.ram[ea + 1] << 8);
> +      if (2 <= dw)
> +        {
> +          r += cpu->state.ram[ea + 2] << 16;
> +          r += cpu->state.ram[ea + 3] << 24;
> +        }
> +    }

you shouldn't have a state.ram member ... the sim core takes care of that for 
you.  when you called "memory region" in sim_open, that initialized a chunk of 
ram for you.  you can access that by using the sim_core_write_buffer and 
sim_core_read_buffer helpers.

> +static void ft32_push (SIM_DESC sd, uint32_t v)
> +{
> +  sim_cpu *cpu = STATE_CPU (sd, 0);
> +  cpu->state.regs[31] -= 4;

rather than hardcode constants, you probably want to use symbols like 
FT32_SP_REGNUM.  or maybe use a union in your state:
	union {
		uint32_t raw[32];
		struct {
			uint32_t r0;
			uint32_t r1;
			...
			uint32_t fp;
			uint32_t sp;
			uint32_t pc;
		};
	} regs;

i'm just guessing at the reg names ... i have no idea what's going on :)

> +  cpu->state.regs[31] &= 0xffff;

the CPU itself will automatically wrap the SP to 16bits ?  that's kind of weird.

> +static int nunsigned (int siz, int bits)
> +{
> +  int mask = (1L << siz) - 1;
> +  return bits & mask;
> +}

for all these little utility functions, it'd be nice to have a one line sentence 
above them explaining what they're for.

> +static
> +void step_once (SIM_DESC sd)
> +{
> +  sim_cpu *cpu = STATE_CPU (sd, 0);
> +  address_word cia = CIA_GET (cpu);
> +  const char *ram_char = (const char *)cpu->state.ram;
> +
> +#define ILLEGAL_INSTRUCTION() \
> +  sim_engine_halt (sd, cpu, NULL, insnpc, sim_signalled, SIM_SIGILL)
> +
> +    {
> +      uint32_t inst;

what is with this indented block ?  doesn't seem like you need this at all.  
delete the braces and unindent everything by one level.

> +      /* Handle "call 8" (which is FT32's "break" equivalent) here */

use GNU style comments

> +      if (inst == 0x00340002)
> +        {
> +          goto escape;
> +        }

drop the braces for single statements

> +      dw   =              (inst >> FT32_FLD_DW_BIT) & ((1U << FT32_FLD_DW_SIZ) - 1);
> +      cb   =              (inst >> FT32_FLD_CB_BIT) & ((1U << FT32_FLD_CB_SIZ) - 1);
> +      r_d  =              (inst >> FT32_FLD_R_D_BIT) & ((1U << FT32_FLD_R_D_SIZ) - 1);
> +      cr   =              (inst >> FT32_FLD_CR_BIT) & ((1U << FT32_FLD_CR_SIZ) - 1);
> +      cv   =              (inst >> FT32_FLD_CV_BIT) & ((1U << FT32_FLD_CV_SIZ) - 1);
> +      bt   =              (inst >> FT32_FLD_BT_BIT) & ((1U << FT32_FLD_BT_SIZ) - 1);
> +      r_1 =               (inst >> FT32_FLD_R_1_BIT) & ((1U << FT32_FLD_R_1_SIZ) - 1);

looks like you were trying to align, but the = here is slightly off ;)

> +      cpu->state.pc += 4;
> +      switch (upper)
> +      {
> +        case FT32_PAT_TOC:

when you hit 8 spaces of indentation, you're supposed to replace with a tab

> +            if (upper == FT32_PAT_ALUOP)
> +              {
> +                cpu->state.regs[r_d] = result;
> +              }

drop the braces

> +              else

incorrect indentation for this else -- delete two spaces

this seems to come up a couple of times ... you should search & fix them all

> +int
> +sim_write (SIM_DESC sd,
> +           SIM_ADDR addr,
> +           const unsigned char *buffer,
> +           int size)
> +{
> +  sim_cpu *cpu = STATE_CPU (sd, 0);
> +
> +  if (addr < 0x800000)
> +    {
> +      sim_core_write_buffer (sd, cpu, write_map, buffer, addr, size);
> +    }
> +    else
> +    {
> +      int i;
> +
> +      addr -= 0x800000;
> +      for (i = 0; i < size; i++)
> +        cpu_mem_write (sd, 0, addr + i, buffer[i]);
> +    }
> +  return size;
> +}
> +
> +int
> +sim_read (SIM_DESC sd,
> +          SIM_ADDR addr,
> +          unsigned char *buffer,
> +          int size)
> +{
> +  sim_cpu *cpu = STATE_CPU (sd, 0);
> +
> +  if (addr < 0x800000)
> +    {
> +      sim_core_read_buffer (sd, cpu, read_map, buffer, addr, size);
> +    }
> +    else
> +    {
> +      int i;
> +
> +      addr -= 0x800000;
> +      for (i = 0; i < size; i++)
> +        buffer[i] = cpu_mem_read (sd, 0, addr + i);
> +    }
> +
> +  return size;
> +}

what's going on here with the magic 0x800000 address ?

> +static uint32_t*
> +ft32_lookup_register (SIM_DESC sd, int nr)

spae before the *

> +  switch (nr)
> +  {
> +  case 0:
> +    return &cpu->state.regs[29];
> +    break;
> +  case 1:
> +    return &cpu->state.regs[31];
> +    break;
> +  case 31:
> +    return &cpu->state.regs[30];
> +    break;
> +  case 32:
> +    return &cpu->state.pc;
> +    break;
> +  default:
> +    return &cpu->state.regs[nr - 2];

you probably want to check the value of nr to make sure it's in range and 
abort() when it isn't

> +sim_store_register (SIM_DESC sd,
> +sim_fetch_register (SIM_DESC sd,

you should change these to ft32_reg_{store,fetch}, and at the end of sim_open, 
do something like:
  /* CPU specific initialization.  */
  for (i = 0; i < MAX_NR_PROCESSORS; ++i)
    {
      SIM_CPU *cpu = STATE_CPU (sd, i);

      CPU_REG_FETCH (cpu) = ft32_reg_fetch;
      CPU_REG_STORE (cpu) = ft32_reg_store;
    }

then add sim-reg.o to your Makefile.in

you probably should also implement ft32_pc_{fetch,store} and assign them:
  CPU_PC_FETCH (cpu) = ft32_pc_fetch;
  CPU_PC_STORE (cpu) = ft32_pc_store;

> +SIM_DESC
> +sim_open (SIM_OPEN_KIND kind,
> +          host_callback *cb,
> +          struct bfd *abfd,
> +          char **argv)
> +{
> +  char c;
> +  SIM_DESC sd = sim_state_alloc (kind, cb);
> +
> +  /* The cpu data is kept in a separately allocated chunk of memory.  */
> +  if (sim_cpu_alloc_all (sd, 1, /*cgen_cpu_max_extra_bytes ()*/0) != SIM_RC_OK)
> +    {
> +      free_state (sd);
> +      return 0;
> +    }
> +
> +  if (sim_pre_argv_init (sd, argv[0]) != SIM_RC_OK)
> +    {
> +      free_state (sd);
> +      return 0;
> +    }

after this, you should do:
  /* getopt will print the error message so we just have to exit if this fails.
     FIXME: Hmmm...  in the case of gdb we need getopt to call
     print_filtered.  */
  if (sim_parse_args (sd, argv) != SIM_RC_OK)
    { 
      free_state (sd);
      return 0;
    }

> +  sd->base.prog_argv = argv + 1;

why do you need to do this ?
-mike