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]

Re: [RFC] TI MSP430 simulator


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]