Re: [PATCH, FT32] gdb and sim support

[Mike Frysinger <vapier at gentoo dot org>]

On 03 Feb 2015 04:06, James Bowman wrote:
> FT32 is a new high performance 32-bit RISC core developed by FTDI for embedded applications.

ah, we def don't have enough CPUs in the market as is ;)

> Support for FT32 has already been added to binutils. This patch adds gdb and sim support.

where's the testsuite man ? :)  it should be trivial to start one with .s
files  -- just look at sim/testsuite/sim/.  otherwise there's no way to keep
regressions from slipping in.

> 2014-02-03  James Bowman  <james.bowman@ftdichip.com>
> 
>         * gdb/Makefile.in, gdb/configure.tgt: FT32 target added
>         * sim/configure.tgt: FT32 target added
>         * sim/configure: Regenerated
>         * sim/ft32/configure: Regenerated
>         * gdb/ft32-tdep.c,h: Support FT32
>         * sim/ft32/*: FT32 simulator

notes:
 - the date reflects commit time, not original creation
 - ChangeLog entries are split up across dirs
 - line items are indented with tabs
 - the notes should be full sentences; i.e. you should have a period at the end

on to the code ... a few more notes:
 - i wrote this feedback over a few "sessions", so there might be incomplete 
thoughts as i paused to do other things
 - feel free to ask questions about new things or things that aren't clear
 - as you make the adjustments below, make sure to periodically save & build & 
run & test ... the sim can be really easy to run into the weeds and trying to 
figure out what exactly you got wrong hard to trackback

> --- a/gdb/Makefile.in
> +++ b/gdb/Makefile.in
>
> -	glibc-tdep.c \
> +	ft32-tdep.c \
> +        glibc-tdep.c \

looks like you broke the indentation for glibc-tdep.c

> --- /dev/null
> +++ b/gdb/ft32-tdep.c
>
>    Copyright (C) 2009-2014 Free Software Foundation, Inc.

it is 2015 now.  should fix in all your files.

> +#include <string.h>

do you really need to include this ?  other tdeps don't seem to.

> +// #include "record-full.h"

dead code -> delete ?

looks like you've got a couple of dead lines in your patch.  please go through 
the whole thing and clean this up.

> +/* Use an invalid address value as 'not available' marker.  */
> +enum { REG_UNAVAIL = (CORE_ADDR) -1 };

space after the -

> +  int established;  // Has the new frame been LINKed

use /* comments */ everywhere.  seems to come up a few times -- please fix 
globally.

we've got bfd_boolean for bool fields too rather than open code it with an int

> +static const unsigned char *
> +ft32_breakpoint_from_pc (struct gdbarch *gdbarch,
> +			  CORE_ADDR *pcptr, int *lenptr)

indentation is slightly broken

> +{
> +  static gdb_byte breakpoint[] = { 0x02, 0x00, 0x34, 0x00 };

const ?

> +char *ft32_register_names[] =

static const char * const ft32_register_names[] ?

> +static struct type *
> +ft32_register_type (struct gdbarch *gdbarch, int reg_nr)
> +{
> +  if (reg_nr == FT32_PC_REGNUM)
> +    return  builtin_type (gdbarch)->builtin_func_ptr;

only one space after the return

> #define IS_PUSH(inst)   (((inst) & 0xfff00000) == 0x84000000)

i think usually we put insn constants/etc... into the corresponding 
include/opcode/ header rather than pasting them inline.  this makes sharing 
between opcodes, gdb, sim, etc... much easier.

> +static CORE_ADDR
> +ft32_analyze_prologue (CORE_ADDR start_addr, CORE_ADDR end_addr,
> +			struct ft32_frame_cache *cache,
> +			struct gdbarch *gdbarch)

indentation is slightly off

>   if (start_addr >= end_addr)
>     {
>       return end_addr;
>     }

pretty sure you can (and should) omit braces in cases like this

> +      if (IS_PUSH(inst))

space needed between func and args:
	if (IS_PUSH (inst))

a quick scan shows this comes up multiple times -- please fix globally

>   for (regnum = FT32_R0_REGNUM; regnum < FT32_PC_REGNUM; regnum++)
>     {
>       if (cache->saved_regs[regnum] != REG_UNAVAIL)
>         cache->saved_regs[regnum] = cache->framesize - cache->saved_regs[regnum];

should use a tab here since you hit 8 spaces.  this comes up a couple of times 
-- please fix globally.

> +    }
> +  // printf("(Framesize=%lld,cache->saved_regs[FT32_PC_REGNUM]=%ld)\n", cache->framesize, cache->saved_regs[FT32_PC_REGNUM]);
> +  return next_addr;
> +
> +}

there should be a blank line above the return, not below it ;)

> +	  plg_end = ft32_analyze_prologue (func_addr,
> +					    func_end, &cache, gdbarch);

indentation is off

> +	  /* Don't use line number debug info for assembly source
> +	     files.  */

this doesn't need to be wrapped

> +	  /* No useable line symbol.  Use result of prologue parsing
> +	     method.  */

neither does this

> +static void
> +ft32_frame_this_id (struct frame_info *this_frame,
> +		    void **this_prologue_cache, struct frame_id *this_id)
> +{
> +  struct ft32_frame_cache *cache = ft32_frame_cache (this_frame,
> +						   this_prologue_cache);

indentation is off

> +static struct value *
> +ft32_frame_prev_register (struct frame_info *this_frame,
> +			  void **this_prologue_cache, int regnum)
> +{
> +  struct ft32_frame_cache *cache = ft32_frame_cache (this_frame,
> +						   this_prologue_cache);

same here

> +  if (regnum < FT32_NUM_REGS && cache->saved_regs[regnum] != REG_UNAVAIL)
> +    {
> +      return frame_unwind_got_memory (this_frame, regnum,
> +                                      0x800000 | cache->saved_regs[regnum]);
> +    }

drop the braces

> +static CORE_ADDR
> +ft32_frame_base_address (struct frame_info *this_frame, void **this_cache)
> +{
> +  struct ft32_frame_cache *cache = ft32_frame_cache (this_frame,
> +						       this_cache);

indentation is broken

> +#if 0

drop all the #if 0 code.  this comes up a few times -- please fix globally.

> +static int
> +ft32_process_record (struct gdbarch *gdbarch, struct regcache *regcache,
> +		      CORE_ADDR addr)
> +{
> +  return -1;
> +#if 0
> ... 400 more lines ...
> +#endif
> +}

err, what now ?

> --- /dev/null
> +++ b/gdb/ft32-tdep.h
>
> +/* Register numbers of various important registers.  */
> +
> +enum ft32_regnum
> +{
> +  FT32_FP_REGNUM,		/* Address of executing stack frame.  */
> +  FT32_SP_REGNUM,		/* Address of top of stack.  */
> +  FT32_R0_REGNUM,
> +  FT32_R1_REGNUM,
> +  FT32_PC_REGNUM = 32		/* Program counter.  */
> +};
> +
> +/* Number of machine registers.  */
> +#define FT32_NUM_REGS 33          /* 32 real registers + PC */

this probably should be in include/gdb/sim-ft32.h instead so you can share it

also, define FT32_NUM_REGS in terms of FT32_PC_REGNUM ?

> +#endif /* ft32-tdep.h */

the style is to list the macro (FT32_TDEP_H), not the file

> --- /dev/null
> +++ b/sim/ft32/Makefile.in
>
> +dtbdir = @datadir@/gdb-`sed q ${srcdir}/../../gdb/version.in`/dtb

unused -> delete

> +SIM_OBJS = interp.o sim-load.o sim-io.o sim-config.o sim-utils.o	\
> +sim-options.o sim-module.o sim-core.o sim-endian.o sim-trace.o	\
> +sim-engine.o sim-fpu.o sim-bits.o sim-profile.o sim-events.o \
> +sim-memopt.o

please one-line & sort this list:
SIM_OBJS = \
	bar.o \
	foo.o

you also want to use $(SIM_NEW_COMMON_OBJS) at the start and $(SIM_EXTRA_OBJS) 
at the end

missing:
SIM_RUN_OBJS = nrun.o

> +SIM_EXTRA_LIBS = -lm -lz -ldl

you don't seem to use any of these libs -> delete

> +SIM_EXTRA_CLEAN = ft32-clean
> +# SIM_EXTRA_INSTALL = install-dtb
> +# SIM_CFLAGS = -DDTB="\"$(dtbdir)/ft32-gdb.dtb\""

unused -> delete

> +all: interp.o
> +
> +interp.o: interp.c
> +
> +ft32-clean:

don't need any of this -> delete

> --- /dev/null
> +++ b/sim/ft32/configure.ac
>
> +AC_CHECK_TOOL(DTC, dtc)

unused -> delete

> +SIM_AC_OPTION_INLINE()

drop the parens

since you're a new port, you should start with SIM_AC_OPTION_WARNINGS enabled.  
obviously that also means cleaning up all the warnings generated in the ft32/ 
subdir once you do :).

> +AC_CHECK_HEADERS(unistd.h)

unused -> delete

> --- /dev/null
> +++ b/sim/ft32/interp.c
>
> +   Contributed by FTDI (support@ftdichip.com)

use <support@ftdichip.com> instead of (support@ftdichip.com)

> +This file is part of GDB, the GNU debugger.

this isn't part of gdb ;).  please fix the indentation in this comment block.  
these comments apply to all your sim files it looks like.

> +#include <sys/times.h>
> +#include <sys/param.h>

you've got a lot of weird system header includes in your files.  please check 
each one to see if they're actually used.

> +#include <netinet/in.h>	/* for byte ordering macros */

i really hope you're not using macros (like hton/ntoh) from this header :).  
that's not how the sim handles endianness.  a quick glance shows you're not, so 
just delete the header and be done.

> +typedef int word;
> +typedef unsigned int uword;

at least uword is not used, and word looks hardly used.  i guess you should punt 
both and tweak the little code relying on them.

> +extern const ft32_opc_info_t ft32_opc_info[128];
> +
> +host_callback *       callback;
> +
> +FILE *tracefile;
> +
> +static char *myname;
> +static SIM_OPEN_KIND sim_kind;
> +static int issue_messages = 0;
> +
> +struct {
> +  uint32_t regs[32];
> +  uint32_t pc;
> +  uint8_t pm[262144];
> +  uint8_t ram[65536];
> +  uint64_t num_i;
> +  uint64_t cycles;
> +  uint64_t next_tick_cycle;
> +  enum sim_stop reason;
> +  int pm_unlock;
> +  uint32_t pm_addr;
> +  int exception;
> +} cpu;

there should be no global state.  read-only constants are OK, but that's not 
what these are.

> +unsigned long
> +ft32_extract_unsigned_integer (addr, len)

should be static

> +     unsigned char * addr;
> +     int len;

we want these old style prototypes to die ... please convert globally

> +{
> +  unsigned long retval;
> +  unsigned char * p;
> +  unsigned char * startaddr = (unsigned char *)addr;
> +  unsigned char * endaddr = startaddr + len;

there should be no space after the * as part of the decl:
	unsigned char *p;

but there should be a space after the cast:
	... = (unsigned char *) addr;

> +  if (len > (int) sizeof (unsigned long))
> +    printf ("That operation is not available on integers of more than %ld bytes.",
> +	    sizeof (unsigned long));

general rule: a sim should never write directly to stdout/stderr (i.e. use 
printf or fprintf).  look at common/sim-io.h for the funcs you should instead.

> +void
> +ft32_store_unsigned_integer (addr, len, val)

same feedback for this func as above

> +void
> +sim_size (int s)
> +{
> +}

once you convert to nrun.c (see earlier comments), you can drop this

> +static void
> +set_initial_gprs ()

funcs that take no args must use (void) in C and not ().

> +{
> +  int i;
> +  long space;
> +}

pointless func ?  delete it.

> +static void
> +interrupt ()
> +{
> +  // cpu.asregs.exception = SIGINT;
> +}

pointless func ?  delete it.

> +uint32_t cpu_pm_read(SIM_DESC sd, int dw, uint32_t ea)

static, and space before the (

> +{
> +  sim_cpu *scpu = STATE_CPU (sd, 0); /* FIXME */
> +  address_word cia = CIA_GET (scpu);
> +  uint32_t r;
> +
> +  if ((ea & ~0x3ffff) != 0)
> +    {
> +      fprintf(stderr, "Illegal PM address %08x, pc %#x\n", ea, cpu.pc);
> +      exit(0);
> +    }

i'm not sure what this mask is trying to accomplish ... are you attemting to 
enforce some kind of address space ?  when you initialized the sim, you should 
have allocated the memory then, and let the sim itself take care of valid 
addresses.

> +void
> +sim_resume (sd, step, siggnal)

you should use sim-resume.o instead

then you'll need a sim_engine_run like so:
void
sim_engine_run (SIM_DESC sd,
        int next_cpu_nr, /* ignore  */
        int nr_cpus, /* ignore  */
        int siggnal) /* ignore  */
{ 
  SIM_CPU *cpu;

  SIM_ASSERT (STATE_MAGIC (sd) == SIM_MAGIC_NUMBER);

  cpu = STATE_CPU (sd, 0);

  while (1)
    { 
      step_once (cpu);
      if (sim_events_tick (sd))
	sim_events_process (sd);
    }
}

then declare a step_once func and put all the cpu-specific logc in there

> +int
> +sim_write (sd, addr, buffer, size)
>
> +int
> +sim_read (sd, addr, buffer, size)

i think once you switch to nrun.c, you won't need these anymore

> +int
> +sim_store_register (sd, rn, memory, length)
>
> +int
> +sim_fetch_register (sd, rn, memory, length)

you should create a helper func that does the rn<->pointer lookup so that you 
can use it here.  e.g. something like:
static unsigned_word
ft32_lookup_register (SIM_DESC sd, int nr)
{
  SIM_CPU *cpu = STATE_CPU (sd, 0);

  switch (nr)
    {
    case 0: return cpu->registers[0];
    ...
    default: return NULL;
    }
}

then the store/fetch funcs become much simpler

ideally you'd switch to sim-reg.o in your Makefile's SIM_OBJS ... that'll 
provide these entry points.  that would require also enabling sim-model.o 
& SIM_AC_OPTION_DEFAULT_MODEL support, but i don't think that'd be too hard.
if you look at bfin/machs.c and start at "sim_machs", i think you should be 
able to track it down easily enough.

> +int
> +sim_trace (sd)

once you switch to nrun.c, you can delete this

> +void
> +sim_stop_reason (sd, reason, sigrc)

add sim-reason.o to your SIM_OBJS to get this for free

> +int
> +sim_stop (sd)

add sim-stop.o to your SIM_OBJS to get this for free

> +void
> +sim_info (sd, verbose)

once you switch to nrun.c/SIM_NEW_COMMON_OBJS, you get this for free

> +SIM_DESC
> +sim_open (kind, cb, abfd, argv)

before this func, you should add:
/* Cover function of sim_state_free to free the cpu buffers as well.  */

static void
free_state (SIM_DESC sd)
{
  if (STATE_MODULES (sd) != NULL)
    sim_module_uninstall (sd);
  sim_cpu_free_all (sd);
  sim_state_free (sd);
}

> +{
> +  SIM_DESC sd = sim_state_alloc (kind, cb);
> +  SIM_ASSERT (STATE_MAGIC (sd) == SIM_MAGIC_NUMBER);

i don't think you need this assert

after this alloc, you should do:
  /* 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;
    }

you'll have to add sim-cpu.o to your SIM_OMJS

> +  if (sim_pre_argv_init (sd, argv[0]) != SIM_RC_OK)
> +    return 0;

you should call free_state in the error path here

after this section, you should call:
  if (sim_pre_argv_init (sd, argv[0]) != SIM_RC_OK)
    { 
      free_state (sd);
      return 0;
    }

  /* 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;
    }

> +  sim_do_command(sd," memory region 0x00000000,0x4000000") ;
> +  sim_do_command(sd," memory region 0xE0000000,0x10000") ;

instead, do:
  /* Allocate external memory if none specified by user.
     Use address 4 here in case the user wanted address 0 unmapped.  */
  if (sim_core_read_buffer (sd, NULL, read_map, &c, 4, 1) == 0)
    {
      sim_do_command(sd, "memory region 0x00000000,0x4000000");
      sim_do_command(sd, "memory region 0xE0000000,0x10000");
    }

notice the style fixes too

then you'll want to do:
  /* Check for/establish the a reference program image.  */
  if (sim_analyze_program (sd,
                           (STATE_PROG_ARGV (sd) != NULL
                            ? *STATE_PROG_ARGV (sd)
                            : NULL), abfd) != SIM_RC_OK)
    {
      free_state (sd);
      return 0;
    }

> +  myname = argv[0];

won't be needed after you drop sim_load (see below)

> +  callback = cb;

you shouldn't need this -- the call to sim_state_alloc above already assigned 
the callback

> +  if (kind == SIM_OPEN_STANDALONE)
> +    issue_messages = 1;

this variable looks pointless in this whole codebase -> delete

> +  set_initial_gprs ();	/* Reset the GPR registers.  */

you should move this to just before the return, and walk all cpus:
  /* CPU specific initialization.  */
  for (i = 0; i < MAX_NR_PROCESSORS; ++i)
    {
      SIM_CPU *cpu = STATE_CPU (sd, i);
      set_initial_gprs (sd, cpu);
    }

> +  if (sim_config (sd) != SIM_RC_OK)
> +    {
> +      sim_module_uninstall (sd);

call your new free_state() helper instead

> +  if (sim_post_argv_init (sd) != SIM_RC_OK)
> +    {
> +      /* Uninstall the modules to avoid memory leaks,
> +	 file descriptor leaks, etc.  */
> +      sim_module_uninstall (sd);

delete the comment & call your new free_state() helper instead

> +void
> +sim_close (sd, quitting)
> +     SIM_DESC sd;
> +     int quitting;
> +{
> +  /* nothing to do */
> +}

you should do:
  sim_module_uninstall (sd);

> +static void
> +load_dtb (SIM_DESC sd, const char *filename)

unused -> delete

> +SIM_RC
> +sim_load (sd, prog, abfd, from_tty)

add sim-hload.o to your SIM_OBJS and you get this for free

> +SIM_RC
> +sim_create_inferior (sd, prog_bfd, argv, env)
> +{
> +  sim_cpu *scpu = STATE_CPU (sd, 0); /* FIXME */

  SIM_CPU *cpu = STATE_CPU (sd, 0);

> +  /* Set the initial register set.  */
> +  l = issue_messages;
> +  issue_messages = 0;
> +  set_initial_gprs ();
> +  issue_messages = l;

you shouldn't need to do this.  you already handled this in sim_open.

> +  // printf("start address %#lx\n", bfd_get_start_address (prog_bfd));
> +  cpu.pc = bfd_get_start_address (prog_bfd);
> +  cpu.regs[31] = 0x00000;
> +  cpu.num_i = 0;
> +  cpu.cycles = 0;
> +  cpu.next_tick_cycle = 100000;

you should instead do:
  /* Set the PC.  */
  if (abfd != NULL)
    addr = bfd_get_start_address (abfd);
  else
    addr = 0;
  sim_pc_set (cpu, addr);

  if (STATE_OPEN_KIND (sd) == SIM_OPEN_DEBUG)
    { 
      freeargv (STATE_PROG_ARGV (sd));
      STATE_PROG_ARGV (sd) = dupargv (argv);
    }

> +void
> +sim_kill (sd)

you shouldn't need this -> delete

looks like i should clean up the tree too

> +void
> +sim_do_command (sd, cmd)

once you switch to nrun.c/SIM_NEW_COMMON_OBJS, you get this for free

> +void
> +sim_set_callbacks (ptr)

once you switch to nrun.c, you can delete this

> --- /dev/null
> +++ b/sim/ft32/sim-main.h
>
> +#define SIM_HAVE_BIENDIAN

once you switch to nrun.c, you can delete this

> +  /* The following are internal simulator state variables: */
> +#define CIA_GET(CPU) ((CPU)->registers[PCIDX] + 0)
> +#define CIA_SET(CPU,CIA) ((CPU)->registers[PCIDX] = (CIA))

you should define these to CPU_PC_SET/CPU_PC_GET instead.  we should probably 
make that the default too so ports don't have to do it.

> +/* To keep this default simulator simple, and fast, we use a direct
> +   vector of registers. The internal simulator engine then uses
> +   manifests to access the correct slot. */

gnu style says to put two spaces after periods

that said, i have a hard time believing the speed thing here when your current 
store/fetch register functions are doing byte level memory accesses

> +  unsigned_word registers[19];

per above, you should probably create dedicated fields for each register.  this 
would also make it easier to debug assuming you have named registers like $pc 
and $fp and $sp.

> --- /dev/null
> +++ b/sim/ft32/sysdep.h

looks like you copied & pasted this from another port.  fairly certain you don't 
need or want any of it.  just delete the file entirely.
-mike

Attachment: signature.asc
Description: Digital signature