[PATCH, FT32] gdb and sim support
Mike Frysinger
vapier@gentoo.org
Thu Feb 19 07:06:00 GMT 2015
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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://sourceware.org/pipermail/gdb-patches/attachments/20150219/d09f9cee/attachment.sig>
More information about the Gdb-patches
mailing list