[RFC] TI MSP430 simulator
Mike Frysinger
vapier@gentoo.org
Fri May 17 18:46:00 GMT 2013
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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
URL: <http://sourceware.org/pipermail/gdb-patches/attachments/20130517/5cddf3d1/attachment.sig>
More information about the Gdb-patches
mailing list