This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.
Index Nav: | [Date Index] [Subject Index] [Author Index] [Thread Index] | |
---|---|---|
Message Nav: | [Date Prev] [Date Next] | [Thread Prev] [Thread Next] |
Other format: | [Raw text] |
On 05 Dec 2016 23:11, Dimitar Dimitrov wrote: a very good start! > --- a/sim/configure.tgt > +++ b/sim/configure.tgt > @@ -76,6 +76,9 @@ case "${target}" in > msp430*-*-*) > SIM_ARCH(msp430) > ;; > + pru*-*-*) > + SIM_ARCH(pru) > + ;; > rl78-*-*) > SIM_ARCH(rl78) > ;; you'll need to run `autoconf` in this dir to regen configure. then include that in your commit. > --- /dev/null > +++ b/sim/pru/Makefile.in > @@ -0,0 +1,25 @@ > +# Makefile template for Configure for the MCore sim library. looks like you'll need to update some of the comments -- this isn't mcore :) > +# Written by Cygnus Solutions. Cygnus didn't write this :) > +SIM_OBJS = \ > + interp.o \ > + $(SIM_NEW_COMMON_OBJS) \ > + sim-resume.o interp.o should come after $(SIM_NEW_COMMON_OBJS) > --- /dev/null > +++ b/sim/pru/configure.ac > > +SIM_AC_OPTION_ENDIAN can PRU be little or big endian ? or is it always one or the other ? if it's always one or the other, then you should pass in LITTLE or BIG here. > --- /dev/null > +++ b/sim/pru/interp.c > > + This file is part of GDB, the GNU debugger. should say: This file is part of simulators. > +#include <signal.h> doesn't look like you use this, so drop the include > +#include <assert.h> don't use this header or assert(). use the existing SIM_ASSERT() helpers instead. those provide better error messages and integrate with the sim framework. > +#include <sys/param.h> pretty sure you don't use stuff from it, so drop the include > +#include "sim-utils.h" your sim-main.h should include sim-base.h which includes sim-utils.h, so no need to include it here > +#include "sim-main.h" you included this already like 5 lines up > +#include "sim-base.h" your sim-main.h should be including sim-base.h, so no need to include it here > +#include "sim-options.h" you don't use these, so drop the include. unless you do below ... > +#define max(a,b) ((a) > (b) ? (a) : (b)) you don't need this -- sim-basics.h already provides it which is included (eventually) by your sim-main.h > +/* DMEM zero address is perfectly valid. But if CRT leaves the first word > + alone, we can use it as a trap to catch NULL pointer access. */ > +static const int abort_on_dmem_zero_access = 0; seems like this should be a debug option so people can change it on the fly ? you could leverage the sim-options.h API to change the value based on command line flags. > +static uint32_t > +pru_extract_unsigned_integer (uint8_t *addr, int len) > +{ > ... > + if (len > (int) sizeof (unsigned long)) change the prototype so that len is a size_t instead of len, then you don't need this cast. size_t is a better type anyways. > + printf ("That operation is not available on integers of more than " sim's should never use printf. you can use sim_io_eprintf instead. then again, the only caller of this func already does a len check which means this scenario should never happen. use sim_io_error instead and that'll trigger an exit/abort. after all, if you let this code continue to run, you'll clobber random memory. > + "%ld bytes.", sizeof (unsigned long)); sizeof returns a size_t which means this should be %zu, not %ld > +static void > +pru_store_unsigned_integer (uint8_t *addr, int len, uint32_t val) same feedback on this func as the extract one above > +static inline uint32_t > +extract_regval (uint32_t val, uint32_t regsel) > +{ > ... > + default: assert (0); return 0; if you want this to abort, call sim_io_error instead with a good error message > +static inline void > +write_regval (uint32_t val, uint32_t *reg, uint32_t regsel) > +{ > ... > + default: assert (0); mask = sh = 0; same here > +static inline void > +pru_reg2dmem (SIM_CPU *cpu, uint32_t addr, unsigned int nbytes, > + int regn, int regb) > +{ > + if (abort_on_dmem_zero_access && addr < 4) > + { > + sim_core_signal (CPU_STATE (cpu), cpu, PC_byteaddr, write_map, > + nbytes, addr, write_transfer, > + SIM_SIGSEGV); > + } > + else if ((addr >= PC_ADDR_SPACE_MARKER) > + || (addr + nbytes > PC_ADDR_SPACE_MARKER)) > + { > + sim_core_signal (CPU_STATE (cpu), cpu, PC_byteaddr, write_map, > + nbytes, addr, write_transfer, > + SIM_SIGSEGV); > + } > + else if ((regn * 4 + regb + nbytes) > (32 * 4)) > + { > + /* Register and load size are not valid. */ > + sim_core_signal (CPU_STATE (cpu), cpu, PC_byteaddr, write_map, > + nbytes, addr, write_transfer, > + SIM_SIGILL); > + } do you really need to do all this ? seems like the existing sim_core_write_1 function already deals properly with writes to out-of-bind addresses. > +static inline void > +pru_dmem2reg (SIM_CPU *cpu, uint32_t addr, unsigned int nbytes, > + int regn, int regb) > +{ > + if (abort_on_dmem_zero_access && addr < 4) > + { > + sim_core_signal (CPU_STATE (cpu), cpu, PC_byteaddr, read_map, > + nbytes, addr, read_transfer, > + SIM_SIGSEGV); > + } > + else if ((addr >= PC_ADDR_SPACE_MARKER) > + || (addr + nbytes > PC_ADDR_SPACE_MARKER)) > + { > + /* This check is necessary because our IMEM "address space" > + is not really accessible, yet we have mapped it as a generic > + memory space. */ > + sim_core_signal (CPU_STATE (cpu), cpu, PC_byteaddr, read_map, > + nbytes, addr, read_transfer, > + SIM_SIGSEGV); > + } > + else if ((regn * 4 + regb + nbytes) > (32 * 4)) > + { > + /* Register and load size are not valid. */ > + sim_core_signal (CPU_STATE (cpu), cpu, PC_byteaddr, read_map, > + nbytes, addr, read_transfer, > + SIM_SIGILL); > + } same here > +static void > +set_initial_gprs (SIM_CPU *cpu) > +{ > + long space; unused var -> delete > +static inline unsigned int > +regsel_width (uint32_t regsel) > +{ > ... > + default: assert (0); return 0; sim_io_error instead please > +static void > +pru_sim_xin_mac (SIM_DESC sd, SIM_CPU *cpu, unsigned int rd_regn, > + unsigned int rdb, unsigned int length) > +{ > + if (rd_regn < 25 || (rd_regn * 4 + rdb + length) > (27 + 1) * 4) > + { > + fprintf (stderr, "XIN MAC: invalid transfer regn=%u.%u, length=%u\n", > + rd_regn, rdb, length); > + RAISE_SIGILL (); > + return; use sim_io_error instead i think comes up multiple times in this file > +static void > +pru_sim_syscall (SIM_DESC sd, SIM_CPU *cpu) seems like you should use sim_syscall instead of implementing your own ad-hoc syscall ABI > +void > +sim_engine_run (SIM_DESC sd, > + int next_cpu_nr, /* ignore */ > + int nr_cpus, /* ignore */ > + int siggnal) /* ignore */ > +{ sim_engine_run should be pretty small. all the business work of your sim should be in a new function instead. so in the end, you'd want something like: while (1) { step_once (cpu); // Name this whatever you want. if (sim_events_tick (sd)) sim_events_process (sd); } > + /* don't treat r30 and r31 as regular registers, they are I/O! */ GNU style says "Don't", and two spaces after "!" > +void > +sim_info (SIM_DESC sd, int verbose) don't define this func -- let the common one do the work for you > +SIM_DESC > +sim_open (SIM_OPEN_KIND kind, host_callback *cb, > + struct bfd *abfd, char * const *argv) > +{ > ... > + cpu = STATE_CPU (sd, 0); doesn't seem like you use this, so delete it > + sim_do_commandf (sd, "memory-region 0x%lx,0x%lx", > + (unsigned long)0, > + (unsigned long)DMEM_DEFAULT_SIZE); > + sim_do_commandf (sd, "memory-region 0x%lx,0x%lx", > + (unsigned long)0x20000000, > + (unsigned long)IMEM_DEFAULT_SIZE); pretty sure you can drop the (unsigned long) cast and change %lx to %x > +void > +sim_close (SIM_DESC sd, int quitting) don't define this func -- let the common one do the work for you > +SIM_RC > +sim_create_inferior (SIM_DESC sd, struct bfd *prog_bfd, > + char * const *argv, char * const *env) > +{ i think you need to deal with STATE_PROG_ARGV here. grep for 'Standalone mode' in other sims as examples. > --- /dev/null > +++ b/sim/pru/pru.h > +#ifndef PRU_H > +#define PRU_H > + > +/* Copyright 2014-2016 Free Software Foundation, Inc. > + Contributed by Dimitar Dimitrov <dimitar@dinux.eu> the ifdef should be after the file comment, not before it > --- /dev/null > +++ b/sim/pru/sim-main.h > +#ifndef PRU_SIM_MAIN > +#define PRU_SIM_MAIN > + > +/* Copyright 2014-2016 Free Software Foundation, Inc. the ifdef should be after the file comment, not before it -mike
Attachment:
signature.asc
Description: Digital signature
Index Nav: | [Date Index] [Subject Index] [Author Index] [Thread Index] | |
---|---|---|
Message Nav: | [Date Prev] [Date Next] | [Thread Prev] [Thread Next] |