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 Thursday 16 May 2013 20:53:00 Kevin Buettner wrote: > This patch adds a simulator for the TI MSP430 and MSP430X. DJ Delorie > wrote most of this code. Other contributors include Nick Clifton and > me. I'll make sure that the ChangeLog entries show this when it > goes in. could you omit generated files from patches in the future ? 75% (like 200k here) is just generated stuff. seems like your e-mail client has mangled this too ... > --- /dev/null 1 Jan 1970 00:00:00 -0000 > +++ msp430/msp430-sim.c 17 May 2013 00:48:28 -0000 > @@ -0,0 +1,1353 @@ > +/* Simulator for TI MSP430 and MSP > + > + Copyright (C) 2005-2013 Free Software Foundation, Inc. > + Contributed by Analog Devices, Inc. fairly certain ADI did not contribute this :). i would just say "based on the Blackfin simulator". assuming you started by copying bfin-sim.c, hopefully you'll have a good base to start with ;) > +static asymbol **symbol_table = NULL; > +static long number_of_symbols = -1; you only use this state in sim_open. why not localize to that function ? alternatively, push this into sim_state. we don't want global state anywhere. > +static long > +lookup_symbol (struct bfd *abfd, const char *name) > +{ > + long i; > + > + if (symbol_table == NULL) > + { > + long storage_needed; > + > + storage_needed = bfd_get_symtab_upper_bound (abfd); > + if (storage_needed <= 0) > + return -1; > + > + symbol_table = xmalloc (storage_needed); > + > + number_of_symbols = bfd_canonicalize_symtab (abfd, symbol_table); > + } > + > + for (i=0; i<number_of_symbols; i++) style is incorrect. needs spaces around the = and <. > + if (strcmp (symbol_table[i]->name, name) == 0) > + { > + long val = symbol_table[i]->section->vma + symbol_table[i]->value; > + return val; style is incorrect. but easy to fix by just returning directly rather than assigning through "val". > + } > + return -1; > +} you only use number_of_symbols in this func, so it should not be global. localize it to this function. > +static int > +msp430_reg_fetch (SIM_CPU *cpu, int regno, unsigned char *buf, int len) > +{ > + if (0 <= regno && regno < 16) > + { > + if (len == 2) > + { > + int val = cpu->state.regs[regno]; > + buf[0] = val & 0xff; > + buf[1] = (val >> 8) & 0xff; > + return 0; > + } > + else if (len == 4) > + { there's a bunch of places where the indentation is incorrect. 8 spaces get collapsed into one tab. > +SIM_DESC > +sim_open(SIM_OPEN_KIND kind, > + struct host_callback_struct *callback, > + struct bfd *abfd, char **argv) > +{ > + SIM_DESC sd = sim_state_alloc (kind, callback); > + char c; > + struct bfd *prog_bfd; > + > + /* I have no idea what this does. */ > + if (sim_cpu_alloc_all (sd, 1, /*cgen_cpu_max_extra_bytes ()*/0) != > SIM_RC_OK) each sim state has some number of cpus and each one needs state. so this allocates that storage. > + /* I have no idea what this does. */ > + if (sim_pre_argv_init (sd, argv[0]) != SIM_RC_OK) common on man, the documentation isn't *that* bad ;) common/sim-module.c: /* Initialize common parts before argument processing. */ SIM_RC sim_pre_argv_init (SIM_DESC sd, const char *myname) > + /* Or this one. */ could you just punt these useless comments ? > + memset (&sd->cpu->state, 0, sizeof (sd->cpu->state)); this should loop over all the cpus and use the macros designed to poke sd (STATE_CPU). you copied a bunch of Blackfin code but seemed to have deleted that. > + sd->cpu->state.cio_breakpoint = lookup_symbol (STATE_PROG_BFD (sd), "C= > $$IO$$"); > + sd->cpu->state.cio_buffer = lookup_symbol (STATE_PROG_BFD (sd), "__CIO= > BUF__"); > + if (sd->cpu->state.cio_buffer == -1) > + sd->cpu->state.cio_buffer = lookup_symbol (STATE_PROG_BFD (sd), "_CI= > OBUF_"); you shouldn't dereference sd->cpu anywhere. do: SIM_CPU *cpu = CPU_STATE (sd, 0); > + printf("invalid operand %d type %d\n", n, op->type); this probably should be writing to stderr and needs a space before the ( seems to come up a few times > + abort (); could you avoid calling abort() ? generally things should use sim_engine_halt() for stopping simulation ... > +#define REPEATS for (rept = 0; rept < n_repeats; rept ++) is this really necessary ? > + if (TRACE_INSN_P (sd->cpu)) > + { > + disassemble_info info; > + unsigned char b[10]; > + > + msp430_trace_one (opcode_pc); > + > + sim_core_read_buffer (sd, sd->cpu, 0, b, opcode_pc, opsize); > + > + init_disassemble_info (&info, stderr, fprintf); > + info.private_data = sd; > + info.read_memory_func = msp430_dis_read; > + fprintf (stderr, "%#8x ", opcode_pc); > + for (i = 0; i < opsize; i += 2) > + fprintf(stderr, " %02x%02x", b[i+1], b[i]); > + for (; i < 6; i += 2) > + fprintf (stderr, " "); > + fprintf (stderr, " "); > + print_insn_msp430 (opcode_pc, &info); > + fprintf (stderr, "\n"); > + fflush (stdout); > + } it's unfortunate that much of the blackfin base was copied, but you didn't preserve the integration with sim/common/. you end up re-implementing a number of things in hacky ways that the common framework provides you. > +#define SD (sd) > +#define CPU (sd->cpu) this doesn't seem necessary at all > + case MSO_call: this one function is already fairly large. seems like the syscall processing is ripe for putting into its own function. > --- /dev/null 1 Jan 1970 00:00:00 -0000 > +++ msp430/msp430-sim.h 17 May 2013 00:48:28 -0000 > > +#include <stdbool.h> > +#include <stdint.h> this (simple) file doesn't seem to use either of these headers > +#endif > + delete trailing new lines please > +#undef MAX > +#undef MIN > +#undef CLAMP > +#undef ALIGN > +#define MAX(a, b) ((a) > (b) ? (a) : (b)) > +#define MIN(a, b) ((a) < (b) ? (a) : (b)) > +#define CLAMP(a, b, c) MIN (MAX (a, b), c) > +#define ALIGN(addr, size) (((addr) + ((size)-1)) & ~((size)-1)) i don't think you use these helpers. punt them. > +/* Default memory size. */ > +#define MSP430_DEFAULT_MEM_SIZE (128 * 1024 * 1024) mmmkay, but your sim_open doesn't use this define (it has a different number hardcoded). i'd delete this. > --- /dev/null 1 Jan 1970 00:00:00 -0000 > +++ msp430/trace.c 17 May 2013 00:48:28 -0000 yet more duplication that makes me very very sad -mike
Attachment:
signature.asc
Description: This is a digitally signed message part.
Index Nav: | [Date Index] [Subject Index] [Author Index] [Thread Index] | |
---|---|---|
Message Nav: | [Date Prev] [Date Next] | [Thread Prev] [Thread Next] |